Re: [PATCH] libata queue updated

[Tejun]

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/