Re: [PATCH] libata queue updated

[Tejun <htejun@gmail.com>]

Ingo Oeser wrote:
> Hi,
> 
> On Monday 30 January 2006 09:44, Tejun Heo wrote:
> 
>>So, are you saying....
>>
>>struct ata_classes {
>>	unsigned int classes[2];
>>|;
>>
>>is safer than
>>
>>unsigned int *class;
>>
>>?
> 
>  
> No, but with a little bit of additional code it CAN be safer.
> 
> Or maybe, we can store the classification in a different way.
> 
> What about putting the information directly into "ap->device[INDEX].class" 
> in the sole caller (ata_drive_probe_reset) so far?
> 

Not altering ->class directly in lldd driver is one major point of this 
whole patchset such that higher level driving logic has a say on whether 
a device is online or not, not the low level driver.  Primarily this is 
useful for sharing low-level codes with hot plugging / EH but it's also 
possible to retry some of the operations during probing in limited cases.

> 
>>>So please let the core layer pass a bounded array here or provide
>>>a function from core layer to set that and check the index.
>>>
>>
>>Can you show me what you have in mind as code?
> 
>  
> /* Define this to 15, if you need to */
> #define ATA_MAX_CLASSES 2
> struct ata_set {
>         unsigned int class[ATA_MAX_CLASSES];
> };
> 
> void set_ata_class(struct ata_set *cls, unsigned int idx, unsigned int what)
> {
>         BUG_ON(idx >= ARRAY_SIZE(cls->class);
>         cls->class[idx] = what;
> }
> 
> set_ata_class(&myclass, 0, what);
> 
> You can enforce that even better by making "what" 
> a typedef like we do it with pte/pmd/pud/pgd in the VM.

First of all, I'm not a big fan of safety through typedef/structure kind 
of stuff.  For VM, I think it's justifiable, but this class thing 
doesn't involve any complex operation around it.  Drivers just do what 
they do and record the result into the @classes array.  I mean, how/why 
a driver would touch classes[1] when it can recognize only one device. 
It's dictated by the hardware spec and reflected in the driver code.  If 
a driver doesn't get this right, things wouldn't work at all.  @classes 
safety is a minor issue at that point.

> But I prefer not passing this class stuff around, which would even safe
> arguments and thus reduce code size.

No boudnary check is done for accessing ap->device[i] and this is really 
not a place to worry about code size, IMHO.

> Maybe we should even have a classify ata port operation instead?

In ATA, probe and reset are closely related.  There's only one way to 
get class code without resetting - EDD, and it doesn't always work well. 
  That's why the callback is named ->probe_reset.  ATA devices are 
designed to be classfied by resetting them.

-- 
tejun