[PATCH RFC] ata: Intel IDE-R support

[PATCH RFC] ata: Intel IDE-R support

[Alan Cox]

Intel IDE-R devices are part of the Intel AMT management setup. They don't
have any special configuration registers or settings so the ata_generic
driver will support them fully.

Rather than add a huge table of IDs for each chipset and keep sending in
new ones this patch autodetects them.

(And yes Jeff I'll resurrect the delay patches in a couple of weeks)

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/ata/ata_generic.c |   49 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 48 insertions(+), 1 deletions(-)


diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
index cc5f772..476f194 100644
--- a/drivers/ata/ata_generic.c
+++ b/drivers/ata/ata_generic.c
@@ -35,6 +35,7 @@
 enum {
 	ATA_GEN_CLASS_MATCH		= (1 << 0),
 	ATA_GEN_FORCE_DMA		= (1 << 1),
+	ATA_GEN_INTEL_IDER		= (1 << 2),
 };
 
 /**
@@ -109,6 +110,44 @@ static struct ata_port_operations generic_port_ops = {
 static int all_generic_ide;		/* Set to claim all devices */
 
 /**
+ *	is_intel_ider		-	identify intel IDE-R devices
+ *	@dev: PCI device
+ *
+ *	Distinguish Intel IDE-R controller devices from other Intel IDE
+ *	devices. IDE-R devices have no timing registers and are in
+ *	most respects virtual. They should be driven by the ata_generic
+ *	driver.
+ */
+
+static int is_intel_ider(struct pci_dev *dev)
+{
+	/* For Intel IDE the value at 0xF8 is only zero on IDE-R
+	   interfaces */
+	u32 r;
+	u16 t;
+
+	pci_read_config_dword(dev, 0xF8, &r);
+	/* Not IDE-R: punt so that ata_(old)piix gets it */
+	if (r != 0)
+		return 0;
+	/* 0xF8 is also be zero on some early Intel IDE devices
+	   but they will have a sane timing register */
+	pci_read_config_word(dev, 0x40, &t);
+	if (t != 0)
+		return 0;
+	/* Finally check if the timing register is writable so that
+	   we eliminate any early devices hot-docked in a docking
+	   station */
+	pci_write_config_word(dev, 0x40, 1);
+	pci_read_config_word(dev, 0x40, &t);
+	if (t) {
+		pci_write_config_word(dev, 0x40, 0);
+		return 0;
+	}
+	return 1;
+}
+
+/**
  *	ata_generic_init		-	attach generic IDE
  *	@dev: PCI device found
  *	@id: match entry
@@ -134,6 +173,10 @@ static int ata_generic_init_one(struct pci_dev *dev, const struct pci_device_id
 	if ((id->driver_data & ATA_GEN_CLASS_MATCH) && all_generic_ide == 0)
 		return -ENODEV;
 
+	if (id->driver_data && ATA_GEN_INTEL_IDER)
+		if (!is_intel_ider(dev))
+			return -ENODEV;
+
 	/* Devices that need care */
 	if (dev->vendor == PCI_VENDOR_ID_UMC &&
 	    dev->device == PCI_DEVICE_ID_UMC_UM8886A &&
@@ -186,7 +229,11 @@ static struct pci_device_id ata_generic[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_2),  },
 	{ PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_3),  },
 	{ PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_5),  },
-#endif	
+#endif
+	/* Intel, IDE class device */
+	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
+	  PCI_CLASS_STORAGE_IDE << 8, 0xFFFFFF00UL, 
+	  .driver_data = ATA_GEN_INTEL_IDER },
 	/* Must come last. If you add entries adjust this table appropriately */
 	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_IDE << 8, 0xFFFFFF00UL),
 	  .driver_data = ATA_GEN_CLASS_MATCH },

Re: [PATCH RFC] ata: Intel IDE-R support

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

> Intel IDE-R devices are part of the Intel AMT management setup. They don't
> have any special configuration registers or settings so the ata_generic
> driver will support them fully.

> Rather than add a huge table of IDs for each chipset and keep sending in
> new ones this patch autodetects them.

> (And yes Jeff I'll resurrect the delay patches in a couple of weeks)

    Doesn't have to be in the changelog...

> Signed-off-by: Alan Cox <alan@linux.intel.com>

[...]

> diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
> index cc5f772..476f194 100644
> --- a/drivers/ata/ata_generic.c
> +++ b/drivers/ata/ata_generic.c
[...]
> @@ -109,6 +110,44 @@ static struct ata_port_operations generic_port_ops = {
>  static int all_generic_ide;		/* Set to claim all devices */
>  
>  /**
> + *	is_intel_ider		-	identify intel IDE-R devices
> + *	@dev: PCI device
> + *
> + *	Distinguish Intel IDE-R controller devices from other Intel IDE
> + *	devices. IDE-R devices have no timing registers and are in
> + *	most respects virtual. They should be driven by the ata_generic
> + *	driver.
> + */
> +
> +static int is_intel_ider(struct pci_dev *dev)
> +{
> +	/* For Intel IDE the value at 0xF8 is only zero on IDE-R
> +	   interfaces */

    Preferred style for multi-line comments is:

/*
  * bla
  * bla
  */

> +	u32 r;
> +	u16 t;
> +
> +	pci_read_config_dword(dev, 0xF8, &r);
> +	/* Not IDE-R: punt so that ata_(old)piix gets it */
> +	if (r != 0)
> +		return 0;
> +	/* 0xF8 is also be zero on some early Intel IDE devices

    Grammar.

> +	   but they will have a sane timing register */

    The same remark about the comment style...

> +	pci_read_config_word(dev, 0x40, &t);
> +	if (t != 0)
> +		return 0;
> +	/* Finally check if the timing register is writable so that
> +	   we eliminate any early devices hot-docked in a docking
> +	   station */

    ... and here too.

> @@ -134,6 +173,10 @@ static int ata_generic_init_one(struct pci_dev *dev, const struct pci_device_id
>  	if ((id->driver_data & ATA_GEN_CLASS_MATCH) && all_generic_ide == 0)
>  		return -ENODEV;
>  
> +	if (id->driver_data && ATA_GEN_INTEL_IDER)

    I think you mean & here, rather than &&...

MBR, Sergei

Re: [PATCH RFC] ata: Intel IDE-R support

[Alan Cox]

> > +static int is_intel_ider(struct pci_dev *dev)
> > +{
> > +	/* For Intel IDE the value at 0xF8 is only zero on IDE-R
> > +	   interfaces */
> 
>     Preferred style for multi-line comments is:

This driver uses the style I have throughout.

> > @@ -134,6 +173,10 @@ static int ata_generic_init_one(struct pci_dev
> > *dev, const struct pci_device_id if ((id->driver_data &
> > ATA_GEN_CLASS_MATCH) && all_generic_ide == 0) return -ENODEV;
> >  
> > +	if (id->driver_data && ATA_GEN_INTEL_IDER)
> 
>     I think you mean & here, rather than &&...

Oh yes most definitely, that would have unfortunate consequences.

Thanks
Alan

Re: [PATCH RFC] ata: Intel IDE-R support

[Tejun Heo]

On 08/10/2010 05:56 PM, Alan Cox wrote:
> Intel IDE-R devices are part of the Intel AMT management setup. They don't
> have any special configuration registers or settings so the ata_generic
> driver will support them fully.
> 
> Rather than add a huge table of IDs for each chipset and keep sending in
> new ones this patch autodetects them.
> 
> (And yes Jeff I'll resurrect the delay patches in a couple of weeks)
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>
...
> +static int is_intel_ider(struct pci_dev *dev)
> +{
> +	/* For Intel IDE the value at 0xF8 is only zero on IDE-R
> +	   interfaces */
> +	u32 r;
> +	u16 t;
> +
> +	pci_read_config_dword(dev, 0xF8, &r);
> +	/* Not IDE-R: punt so that ata_(old)piix gets it */
> +	if (r != 0)
> +		return 0;
> +	/* 0xF8 is also be zero on some early Intel IDE devices
> +	   but they will have a sane timing register */
> +	pci_read_config_word(dev, 0x40, &t);
> +	if (t != 0)
> +		return 0;
> +	/* Finally check if the timing register is writable so that
> +	   we eliminate any early devices hot-docked in a docking
> +	   station */
> +	pci_write_config_word(dev, 0x40, 1);
> +	pci_read_config_word(dev, 0x40, &t);
> +	if (t) {
> +		pci_write_config_word(dev, 0x40, 0);
> +		return 0;
> +	}
> +	return 1;
> +}

This looks scary to me.  Is this something documented somewhere?  It's
not like we can avoid adding PCI device IDs completely anyway, so I
would suggest just doing it good old fashioned way.

Thanks.

-- 
tejun

Re: [PATCH RFC] ata: Intel IDE-R support

[Alan Cox]

> This looks scary to me.  Is this something documented somewhere?  It's
> not like we can avoid adding PCI device IDs completely anyway, so I
> would suggest just doing it good old fashioned way.

The trouble with adding all the ids is it will keep needing updates every
chipset (and a lot of existing idents).

Is it documented ? The instructions it was written to came from the
people who do the chips.

Alan

Re: [PATCH RFC] ata: Intel IDE-R support

[Tejun Heo]

Hello,

On 08/17/2010 06:42 PM, Alan Cox wrote:
>> This looks scary to me.  Is this something documented somewhere?  It's
>> not like we can avoid adding PCI device IDs completely anyway, so I
>> would suggest just doing it good old fashioned way.
> 
> The trouble with adding all the ids is it will keep needing updates every
> chipset (and a lot of existing idents).

But we do that for some controllers anyway so it's not something new.
ata_piix already has list of all the devices it supports, so maybe
it's safe to grab all intel IDE devices from ata_generic?  ata_piix
always has higher priority than ata_generic anyway and if a device
isn't grabbed by ata_piix, we don't lose anything by grabbing it from
ata_generic.

> Is it documented ? The instructions it was written to came from the
> people who do the chips.

Yeap, that's much better, but I still think it would be better to
avoid such detection magics if possible.

Thanks.

-- 
tejun

Re: [PATCH RFC] ata: Intel IDE-R support

[Alan Cox]

> ata_piix already has list of all the devices it supports, so maybe
> it's safe to grab all intel IDE devices from ata_generic?  ata_piix

I can't guarantee that and I'm not sure I can find anyone at Intel with
that deep a knowledge of the earliest chip errata.

> always has higher priority than ata_generic anyway and if a device
> isn't grabbed by ata_piix, we don't lose anything by grabbing it from
> ata_generic.

Priority only works if you know what is there to use, and the piix one is
for example broken if you change the bindings with sysfs. You don't for
example want to forget to compile in one driver and get generic when that
is unsafe.

The extreme example would be if you have ata_generic binding to all
devices and you forgot to compile in RZ1000 support. On the relevant
board that just ate your file system.

So the grab it all approach was dismissed.

> > Is it documented ? The instructions it was written to came from the
> > people who do the chips.
> 
> Yeap, that's much better, but I still think it would be better to
> avoid such detection magics if possible.

Well yes but the logic is simple.

0xF8 is non zero on all later Intel ATA PCI chipsets
0x40 is writable on all earlier Intel ATA PCI chipsets, and zero + non
writable on the IDE-R devices.

The other thing I did consider was submitting an Intel IDE-R driver that
contained the check and matched IDE CLASS & vendor == INTEL. That has the
advantage of not getting autoloaded unnecessarily so easily, but means we
have another driver.

Alan

Re: [PATCH RFC] ata: Intel IDE-R support

[Tejun Heo]

Hello, Alan.

On 08/17/2010 07:01 PM, Alan Cox wrote:
>> ata_piix already has list of all the devices it supports, so maybe
>> it's safe to grab all intel IDE devices from ata_generic?  ata_piix
> 
> I can't guarantee that and I'm not sure I can find anyone at Intel with
> that deep a knowledge of the earliest chip errata.

But ata_generic grabbing a controller doesn't mean it's gonna be
messed up in anyway.  That's what windows does for unknown IDE
controllers after all.  The problem we want to avoid here is using
ata_generic for a controller which already has a proper driver.

>> always has higher priority than ata_generic anyway and if a device
>> isn't grabbed by ata_piix, we don't lose anything by grabbing it from
>> ata_generic.
> 
> Priority only works if you know what is there to use, and the piix one is
> for example broken if you change the bindings with sysfs. You don't for
> example want to forget to compile in one driver and get generic when that
> is unsafe.

I don't really think it would be dangerous to grab intel IDE
controllers with ata_generic.  Again, that's what windows would do.
And for sysfs unbind case, if the user specifically unbinds the
controller from ata_piix, what is broken?

> The extreme example would be if you have ata_generic binding to all
> devices and you forgot to compile in RZ1000 support. On the relevant
> board that just ate your file system.
> 
> So the grab it all approach was dismissed.

For RZ1000, sure, but we're talking about only intel IDEs here.

>>> Is it documented ? The instructions it was written to came from the
>>> people who do the chips.
>>
>> Yeap, that's much better, but I still think it would be better to
>> avoid such detection magics if possible.
> 
> Well yes but the logic is simple.
> 
> 0xF8 is non zero on all later Intel ATA PCI chipsets
> 0x40 is writable on all earlier Intel ATA PCI chipsets, and zero + non
> writable on the IDE-R devices.

Maybe it's okay now but who's gonna remember what's going on there
after five years and nobody guarantees the above would continue to
hold in the future.

> The other thing I did consider was submitting an Intel IDE-R driver that
> contained the check and matched IDE CLASS & vendor == INTEL. That has the
> advantage of not getting autoloaded unnecessarily so easily, but means we
> have another driver.

Yeah, I think it would be better to do it in ata_generic one way or
the other.

Thanks.

-- 
tejun

Re: [PATCH RFC] ata: Intel IDE-R support

[Alan Cox]

> I don't really think it would be dangerous to grab intel IDE
> controllers with ata_generic.  Again, that's what windows would do.
> And for sysfs unbind case, if the user specifically unbinds the
> controller from ata_piix, what is broken?

If the user changes the PCI id list what happens ?

> 
> > The extreme example would be if you have ata_generic binding to all
> > devices and you forgot to compile in RZ1000 support. On the relevant
> > board that just ate your file system.
> > 
> > So the grab it all approach was dismissed.
> 
> For RZ1000, sure, but we're talking about only intel IDEs here.

I don't know. I do know what the Intel folks have told me about detecting
them.

> > 0xF8 is non zero on all later Intel ATA PCI chipsets
> > 0x40 is writable on all earlier Intel ATA PCI chipsets, and zero + non
> > writable on the IDE-R devices.
> 
> Maybe it's okay now but who's gonna remember what's going on there
> after five years and nobody guarantees the above would continue to

Add a comment ?

> hold in the future.

It did occur to me to check this would be true in the future. Either way
- an id table would go out of date more often.

Alan

Re: [PATCH RFC] ata: Intel IDE-R support

[Tejun Heo]

Hello,

On 08/17/2010 08:23 PM, Alan Cox wrote:
>> I don't really think it would be dangerous to grab intel IDE
>> controllers with ata_generic.  Again, that's what windows would do.
>> And for sysfs unbind case, if the user specifically unbinds the
>> controller from ata_piix, what is broken?
> 
> If the user changes the PCI id list what happens ?

Then that's upto the user, right?  That's what the (root) user wants.
You can add any random device ID to any random driver and if you do it
wrong something will break.  That's not different from any other
driver.

>> Maybe it's okay now but who's gonna remember what's going on there
>> after five years and nobody guarantees the above would continue to
> 
> Add a comment ?

Yeah, definitely.  Fat amount of them.

>> hold in the future.
> 
> It did occur to me to check this would be true in the future. Either way
> - an id table would go out of date more often.

Sure, the thing is that your patch doesn't mean we don't have to keep
ata_piix device table up-to-date.  We would still need to be
maintaining that table whether ata_generic can detect IDE-R by itself
or not.  The device ID table in ata_piix is given, and ata_generic
picking up the rest of intel IDE's wouldn't miss anything.  So, unless
IDE-R devices need some special treatment, I don't really see how the
detection code would be useful.

Thanks.

-- 
tejun

Re: [PATCH RFC] ata: Intel IDE-R support

[Alan Cox]

> Sure, the thing is that your patch doesn't mean we don't have to keep
> ata_piix device table up-to-date.  We would still need to be
> maintaining that table whether ata_generic can detect IDE-R by itself

I'm not sure I follow

> or not.  The device ID table in ata_piix is given, and ata_generic
> picking up the rest of intel IDE's wouldn't miss anything.  So, unless
> IDE-R devices need some special treatment, I don't really see how the
> detection code would be useful.

The trend is towards AHCI so the problem goes away for the other bits.

IDE-R is not ata_piix drivable, and lots of the ICH stuff really wants
driving via ata_piix, so having the generic driver grab all intel stuff
isn't as far as I can see going to be safe given a system may load the
ata_generic module but not PIIX then meet a piix by hotplug.

Alan

Re: [PATCH RFC] ata: Intel IDE-R support

[Tejun Heo]

Hello,

On 08/18/2010 12:03 PM, Alan Cox wrote:
>> Sure, the thing is that your patch doesn't mean we don't have to keep
>> ata_piix device table up-to-date.  We would still need to be
>> maintaining that table whether ata_generic can detect IDE-R by itself
> 
> I'm not sure I follow

I was trying to say that IDE-R basically being the complement of Intel
IDE's not drive by ata_piix, we don't need to maintain a separate PCI
device ID table for IDE-R's.

>> or not.  The device ID table in ata_piix is given, and ata_generic
>> picking up the rest of intel IDE's wouldn't miss anything.  So, unless
>> IDE-R devices need some special treatment, I don't really see how the
>> detection code would be useful.
> 
> The trend is towards AHCI so the problem goes away for the other bits.

Yeah, it has helped a lot but ata_piix's are here to stay for the
foreseeable future and we'll be maintaining its device ID table.

> IDE-R is not ata_piix drivable, and lots of the ICH stuff really wants
> driving via ata_piix, so having the generic driver grab all intel stuff
> isn't as far as I can see going to be safe given a system may load the
> ata_generic module but not PIIX then meet a piix by hotplug.

That's the reason why we have module priorities.  The link priority
becomes module priority and modprobe will deterministically prefer
ata_piix over ata_generic if a controller is supported by both.

Thanks.

-- 
tejun

Re: [PATCH RFC] ata: Intel IDE-R support

[Alan Cox]

> That's the reason why we have module priorities.  The link priority
> becomes module priority and modprobe will deterministically prefer
> ata_piix over ata_generic if a controller is supported by both.

Sure - but if ata_generic is already compiled in or already loaded ?

Re: [PATCH RFC] ata: Intel IDE-R support

[Tejun Heo]

Hello,

On 08/18/2010 05:15 PM, Alan Cox wrote:
>> That's the reason why we have module priorities.  The link priority
>> becomes module priority and modprobe will deterministically prefer
>> ata_piix over ata_generic if a controller is supported by both.
> 
> Sure - but if ata_generic is already compiled in or already loaded ?

Well, what if pata_acpi is already compiled in or already loaded?  Or
what if ata_piix is already compiled in or already loaded but ahci is
not for an ich6 controller?  We have priorities in the drivers, if the
user/admin wants to use the lower priority ones that much, he/she
shall.  This isn't an issue.  There's no way to hotplug intel IDEs
anyway.

Thanks.

-- 
tejun

Re: [PATCH RFC] ata: Intel IDE-R support

[Alan Cox]

> > Sure - but if ata_generic is already compiled in or already loaded ?
> 
> Well, what if pata_acpi is already compiled in or already loaded?  Or

Yes I am aware that one also has problems but unlike IDER I don't see a
cure.

> what if ata_piix is already compiled in or already loaded but ahci is
> not for an ich6 controller?  We have priorities in the drivers, if the
> user/admin wants to use the lower priority ones that much, he/she
> shall.  This isn't an issue.  There's no way to hotplug intel IDEs
> anyway.

The PIIX4 in the IBM docking stations is hot plug - and its why I also
have to check 0x40 and write it if zero. So the case exists.

Alan

Re: [PATCH RFC] ata: Intel IDE-R support

[Tejun Heo]

(cc'ing Kay, hi!)
Hello,

On 08/19/2010 12:09 PM, Alan Cox wrote:
>> what if ata_piix is already compiled in or already loaded but ahci is
>> not for an ich6 controller?  We have priorities in the drivers, if the
>> user/admin wants to use the lower priority ones that much, he/she
>> shall.  This isn't an issue.  There's no way to hotplug intel IDEs
>> anyway.
> 
> The PIIX4 in the IBM docking stations is hot plug - and its why I also
> have to check 0x40 and write it if zero. So the case exists.

I see.  Most of the problems are solved by driver priority but yeah
that's an actual corner case.  Driver priority doesn't work unless all
the matching drivers are loaded by the time driver matching starts, so
the same problem could happen with pata_acpi depending on probe order
during normal boot, so pata_acpi should be blacklisted by default.

Kay, there are a few ATA drivers with overlapping hardware support.
When all the drivers are built-in the kernel or none of them is loaded
as modules, priority among them as defined by link order is followed;
however, if one of the drivers is already loaded while other
overlapping ones are not, there's nothing which guarantees driver
probing would happen after modprobe for the device ID is complete,
right?  So, the already loaded driver may be attached to the
controller even when there is a higher priority driver.  Am I missing
something?

Thanks.

-- 
tejun

Re: [PATCH RFC] ata: Intel IDE-R support

[Kay Sievers]

On Thu, 2010-08-19 at 13:22 +0200, Tejun Heo wrote:
> Kay, there are a few ATA drivers with overlapping hardware support.
> When all the drivers are built-in the kernel or none of them is loaded
> as modules, priority among them as defined by link order is followed;
> however, if one of the drivers is already loaded while other
> overlapping ones are not, there's nothing which guarantees driver
> probing would happen after modprobe for the device ID is complete,

Not sure it if I get it right. You mean the first driver binds to the
device already, right? But you want the later one to be used?

Any driver that binds to a device will prevent any other later loaded
driver from binding to, or handling the device. The only way to have the
second driver seeing the device is unbinding the first driver in sysfs,
and binding the second one in sysfs.

Kay

Re: [PATCH RFC] ata: Intel IDE-R support

[Tejun Heo]

Hello,

On 08/19/2010 01:35 PM, Kay Sievers wrote:
> On Thu, 2010-08-19 at 13:22 +0200, Tejun Heo wrote:
>> Kay, there are a few ATA drivers with overlapping hardware support.
>> When all the drivers are built-in the kernel or none of them is loaded
>> as modules, priority among them as defined by link order is followed;
>> however, if one of the drivers is already loaded while other
>> overlapping ones are not, there's nothing which guarantees driver
>> probing would happen after modprobe for the device ID is complete,
> 
> Not sure it if I get it right. You mean the first driver binds to the
> device already, right? But you want the later one to be used?
> 
> Any driver that binds to a device will prevent any other later loaded
> driver from binding to, or handling the device. The only way to have the
> second driver seeing the device is unbinding the first driver in sysfs,
> and binding the second one in sysfs.

It's a bit messy to explain.  So, if all the drivers are already
inside the kernel device - driver matching would follow the priority.
If none of the drivers is in the kernel, udev will call modprobe to
with device ID and modprobe will follow the link order and load the
matching drivers in link order.  What I was curious about is what
happens if some of them are already in the kernel but not all of them.
udev would still be invoked the same and modprobe too but device -
driver matching can and is probably likely to happen before modprobe
finishes loading all the drivers, right?  Or is there a mechanism to
hold off device - driver matching before the udev finishes handling
the event for the controller?

Thanks.

-- 
tejun

Re: [PATCH RFC] ata: Intel IDE-R support

[Kay Sievers]

On Thu, Aug 19, 2010 at 13:42, Tejun Heo <tj@kernel.org> wrote:
> What I was curious about is what
> happens if some of them are already in the kernel but not all of them.

> udev would still be invoked the same and modprobe too but device -
> driver matching can and is probably likely to happen before modprobe
> finishes loading all the drivers, right?  Or is there a mechanism to
> hold off device - driver matching before the udev finishes handling
> the event for the controller?

The compiled-in drivers have matched and bound and blocked-out any
other driver of the device long before udev is even started load the
other drivers. The matching compiled-in drivers will always win over
the modules, regardless of their priority.

Is that what you mean? :)

Kay

Re: [PATCH RFC] ata: Intel IDE-R support

[Tejun Heo]

Hello,

On 08/19/2010 02:24 PM, Kay Sievers wrote:
> The compiled-in drivers have matched and bound and blocked-out any
> other driver of the device long before udev is even started load the
> other drivers. The matching compiled-in drivers will always win over
> the modules, regardless of their priority.
> 
> Is that what you mean? :)

It's still a bit fuzzy to me.  So, the exact scenario I'm thinking
about is something like the following.

1. System discovers an unsupported IDE class controller.

2. udev kicks in and loads pata_acpi for it which can serve as a
   fallback driver for all IDE class controllers.

3. System discovers a controller which is supported by ata_piix.
   ata_piix is the correct driver for it w/ higher priority than
   pata_acpi.

I'm curious about the followings,

a. udev invokes modprobe on device added events, no?  Does it matter
   whether there's a matching driver in the kernel (built-in or an
   already loaded module) to udev behavior?  You seem to be implying
   that if there's a matching driver, udev wouldn't invoke modprobe at
   all.

b. modprobe, when invoked, will load all the matching drivers in link
   order.  If udev invokes modprobe regardless of existing drivers, is
   there a mechanism to hold off device - driver matching before
   modprobe is finished?  Probably not, right?

The whole priority thing seems kind of broken.  It's good enough for
simple cases but just breaks down as soon as things get closer to
corner cases.  Maybe it's best to leave it alone as mostly working
workaround for simple problems.  :-(

Thanks.

-- 
tejun

Re: [PATCH RFC] ata: Intel IDE-R support

[Kay Sievers]

On Thu, Aug 19, 2010 at 14:33, Tejun Heo <tj@kernel.org> wrote:
> I'm curious about the followings,
>
> a. udev invokes modprobe on device added events, no?

Yeah, in this case it from off of the pci devices. Most of the events
for pci devices run in parallel.

>  Does it matter
>   whether there's a matching driver in the kernel (built-in or an
>   already loaded module) to udev behavior?

If the device we are currently handling has already a driver bound,
udev will not call modprobe.
It's:
  DRIVER!="?*", ENV{MODALIAS}=="?*", RUN+="/sbin/modprobe -bv $env{MODALIAS}"
which means:
  !DRIVER && MODALIAS -> modprobe

> You seem to be implying
>   that if there's a matching driver, udev wouldn't invoke modprobe at
>   all.

It doesn't call modprobe for this device in that case, yes. But some
other similar device may still cause the driver to be loaded and every
driver load will scan all devices and try to bind them.

> b. modprobe, when invoked, will load all the matching drivers in link
>   order.  If udev invokes modprobe regardless of existing drivers,

Yeah, udev only cares about the device state, and if the device has a
driver bound, not if there are other drivers (bound or unbound to
devices) in the kernel already.

>   is
>   there a mechanism to hold off device - driver matching before
>   modprobe is finished?  Probably not, right?

What does 'modprobe finished' mean? The modprobe called for this
specific device? The first driver loaded and initialized will win. If
multiple drivers are loaded with the same modprobe call, the later
ones will never see the device.

Kay

Re: [PATCH RFC] ata: Intel IDE-R support

[Tejun Heo]

Hello,

On 08/19/2010 02:52 PM, Kay Sievers wrote:
> If the device we are currently handling has already a driver bound,
> udev will not call modprobe.
> It's:
>   DRIVER!="?*", ENV{MODALIAS}=="?*", RUN+="/sbin/modprobe -bv $env{MODALIAS}"
> which means:
>   !DRIVER && MODALIAS -> modprobe

Okay, thanks for the explanation.

>>   is
>>   there a mechanism to hold off device - driver matching before
>>   modprobe is finished?  Probably not, right?
> 
> What does 'modprobe finished' mean? The modprobe called for this
> specific device? The first driver loaded and initialized will win. If
> multiple drivers are loaded with the same modprobe call, the later
> ones will never see the device.

Yeap, you're right.  Loading all drivers is for cases where
controllers supported by different drivers share the same PCI ID (some
of hpt's need this).  So, the priority thing is mostly broken except
for pretty simple cases. :-( Oh well, it has worked well enough till
now.  Let's leave it alone for now.

Thanks.

-- 
tejun

Re: [PATCH RFC] ata: Intel IDE-R support

[Kay Sievers]

On Thu, Aug 19, 2010 at 14:54, Tejun Heo <tj@kernel.org> wrote:
> Loading all drivers is for cases where
> controllers supported by different drivers share the same PCI ID (some
> of hpt's need this).  So, the priority thing is mostly broken except
> for pretty simple cases. :-( Oh well, it has worked well enough till
> now.  Let's leave it alone for now.

Well, I wouldn't really call it broken. It's very helpful to have
these priorities, and it solved a bunch of real problems because in
most cases it produces predictable results, unlike it was before.

That compiled-in and and modules don't really mix here regarding your
use case does not really matter in the general picture, it's still a
predictable behavior, and we really need the priorities.

If we need finer-grained policy here, we need to move the driver
binding to userspace. The driver core and udev can do that already.
But nobody of us is so crazy to enable it, and handle all the fallout.
But it's possible in theory ... :)

Kay

Re: [PATCH RFC] ata: Intel IDE-R support

[Tejun Heo]

Hello,

On 08/19/2010 03:08 PM, Kay Sievers wrote:
> Well, I wouldn't really call it broken. It's very helpful to have
> these priorities, and it solved a bunch of real problems because in
> most cases it produces predictable results, unlike it was before.
> 
> That compiled-in and and modules don't really mix here regarding your
> use case does not really matter in the general picture, it's still a
> predictable behavior, and we really need the priorities.

Yeah, in most usual cases, it has been fine.  I was just hoping that
it would work better so that fallback drivers can be handled with it.
For now, it seems there is no safe way to have a generic fallback
driver (be it pata_acpi or ata_generic).

> If we need finer-grained policy here, we need to move the driver
> binding to userspace. The driver core and udev can do that already.
> But nobody of us is so crazy to enable it, and handle all the fallout.
> But it's possible in theory ... :)

Right, userland already has enough mechanism and information to handle
the driver binding problem correctly, but given how little problem it
has been causing till now, I think that would be an overkill, for now
at least.

Thanks.

-- 
tejun

Re: [PATCH RFC] ata: Intel IDE-R support

[Tejun Heo]

Hello, again.

On 08/19/2010 01:22 PM, Tejun Heo wrote:
> On 08/19/2010 12:09 PM, Alan Cox wrote:
>>> what if ata_piix is already compiled in or already loaded but ahci is
>>> not for an ich6 controller?  We have priorities in the drivers, if the
>>> user/admin wants to use the lower priority ones that much, he/she
>>> shall.  This isn't an issue.  There's no way to hotplug intel IDEs
>>> anyway.
>>
>> The PIIX4 in the IBM docking stations is hot plug - and its why I also
>> have to check 0x40 and write it if zero. So the case exists.
> 
> I see.  Most of the problems are solved by driver priority but yeah
> that's an actual corner case.  Driver priority doesn't work unless all
> the matching drivers are loaded by the time driver matching starts, so
> the same problem could happen with pata_acpi depending on probe order
> during normal boot, so pata_acpi should be blacklisted by default.

It seems priority thing can't handle this at all.  So, to the original
patch,

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH RFC] ata: Intel IDE-R support

[Jeff Garzik]

On 08/19/2010 08:56 AM, Tejun Heo wrote:
> Hello, again.
>
> On 08/19/2010 01:22 PM, Tejun Heo wrote:
>> On 08/19/2010 12:09 PM, Alan Cox wrote:
>>>> what if ata_piix is already compiled in or already loaded but ahci is
>>>> not for an ich6 controller?  We have priorities in the drivers, if the
>>>> user/admin wants to use the lower priority ones that much, he/she
>>>> shall.  This isn't an issue.  There's no way to hotplug intel IDEs
>>>> anyway.
>>>
>>> The PIIX4 in the IBM docking stations is hot plug - and its why I also
>>> have to check 0x40 and write it if zero. So the case exists.
>>
>> I see.  Most of the problems are solved by driver priority but yeah
>> that's an actual corner case.  Driver priority doesn't work unless all
>> the matching drivers are loaded by the time driver matching starts, so
>> the same problem could happen with pata_acpi depending on probe order
>> during normal boot, so pata_acpi should be blacklisted by default.
>
> It seems priority thing can't handle this at all.  So, to the original
> patch,
>
> Acked-by: Tejun Heo<tj@kernel.org>

Cool, though still waiting on resend to fix problems mentioned earlier 
in the thread, notably s/&&/&/

Or if Alan wants to sign-off on my doing that correction myself, since a 
resend hasn't appeared?

Re: [PATCH RFC] ata: Intel IDE-R support

[Tim Small]

Tejun Heo wrote:
> There's no way to hotplug intel IDEs
> anyway.
>   

Won't kvm (or similar virtualisation system) + PCI passthrough allow you
to hot-plug arbitrary PCI devices?  Bit of a corner-case I know, but I
can see people occasionally doing this...

Tim.