Re: [PATCH] libata queue updated

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux