[RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Milton Miller]

After the following patch to mark the isa region busy and applying a few
patches[1], I was able to kexec boot into an all-yes-config kernel from 
linux-next, with the following KCONFIG_ALLCONFIG file:

# CONFIG_KALLSYMS_EXTRA_PASS is not set
# CONFIG_CRASH_DUMP is not set
CONFIG_PPC_EARLY_DEBUG_RTAS_CONSOLE=y
# CONFIG_NSC_FIR is not set
# CONFIG_SMC_IRCC_FIR is not set
# CONFIG_ALI_FIR is not set
# CONFIG_TCG_NSC is not set
# CONFIG_TCG_ATMEL is not set
# CONFIG_SENSORS_F71805F is not set
# CONFIG_SENSORS_F71882FG is not set
# CONFIG_SENSORS_F75375S is not set
# CONFIG_SENSORS_IT87 is not set
# CONFIG_SENSORS_PC87360 is not set
# CONFIG_SENSORS_PC87427 is not set
# CONFIG_SENSORS_DME1737 is not set
# CONFIG_SENSORS_SMSC47M1 is not set
# CONFIG_SENSORS_SMSC47B397 is not set
# CONFIG_SENSORS_VT1211 is not set
# CONFIG_SENSORS_W83627HF is not set
# CONFIG_SENSORS_W83627EHF is not set

While the first two might not be required, and the third is just
selecting the right platform, it would be nice to get the drivers
to play as well as the rest of the kernel.

The drivers all are for super-io chips, either irda/FIR drivers or hwmon,
and poke at isa ports without checking request_region.

It would be great if these 17 super-io drivers would check for resource
availability before poking at io ports.

[not that I expect this to merge as it, but maybe insert-resource or something]



[1] 3 section layout patches and the MTD DISKONCHIP patch,
see linuxppc-dev for full list


diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c
index 2c6ded2..db54620 100644
--- a/arch/powerpc/platforms/pseries/pci.c
+++ b/arch/powerpc/platforms/pseries/pci.c
@@ -58,8 +58,10 @@ DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, pcibios_name_device);
 
 static void __init pSeries_request_regions(void)
 {
-	if (!isa_io_base)
+	if (!isa_io_base) {
+		request_region(0x0,0x400,"isa");
 		return;
+		}
 
 	request_region(0x20,0x20,"pic1");
 	request_region(0xa0,0x20,"pic2");

mtd: remove __PPC__ hardcoded address from nand/diskonchip and devices/docprobe

[Milton Miller]

Such a hardcoded address can cause a checkstop or machine check if
the driver is in the kernel but the address is not acknowledged.

Both drivers allow an address to be specified as either a module
parameter or config option.   Any future powerpc board should either
use one of these methods or find the address in the device tree.

Signed-off-by: Milton Miller <miltonm@bga.com>
---
There is no powerpc defconfig that sets either CONFIG_DOC200{0,1*} nor
CONFIG_MTD_NAND_DISKONCHIP in next-20080710.

diff --git a/drivers/mtd/devices/docprobe.c b/drivers/mtd/devices/docprobe.c
index 6e5d811..6e62922 100644
--- a/drivers/mtd/devices/docprobe.c
+++ b/drivers/mtd/devices/docprobe.c
@@ -76,8 +76,6 @@ static unsigned long __initdata doc_locations[] = {
 	0xe0000, 0xe2000, 0xe4000, 0xe6000,
 	0xe8000, 0xea000, 0xec000, 0xee000,
 #endif /*  CONFIG_MTD_DOCPROBE_HIGH */
-#elif defined(__PPC__)
-	0xe4000000,
 #else
 #warning Unknown architecture for DiskOnChip. No default probe locations defined
 #endif
diff --git a/drivers/mtd/nand/diskonchip.c b/drivers/mtd/nand/diskonchip.c
index cd4393c..765d4f0 100644
--- a/drivers/mtd/nand/diskonchip.c
+++ b/drivers/mtd/nand/diskonchip.c
@@ -52,8 +52,6 @@ static unsigned long __initdata doc_locations[] = {
 	0xe0000, 0xe2000, 0xe4000, 0xe6000,
 	0xe8000, 0xea000, 0xec000, 0xee000,
 #endif /*  CONFIG_MTD_DOCPROBE_HIGH */
-#elif defined(__PPC__)
-	0xe4000000,
 #else
 #warning Unknown architecture for DiskOnChip. No default probe locations defined
 #endif

Re: [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Hans de Goede]

Milton Miller wrote:
> After the following patch to mark the isa region busy and applying a few
> patches[1], I was able to kexec boot into an all-yes-config kernel from 
> linux-next, with the following KCONFIG_ALLCONFIG file:
> 
> # CONFIG_KALLSYMS_EXTRA_PASS is not set
> # CONFIG_CRASH_DUMP is not set
> CONFIG_PPC_EARLY_DEBUG_RTAS_CONSOLE=y
> # CONFIG_NSC_FIR is not set
> # CONFIG_SMC_IRCC_FIR is not set
> # CONFIG_ALI_FIR is not set
> # CONFIG_TCG_NSC is not set
> # CONFIG_TCG_ATMEL is not set
> # CONFIG_SENSORS_F71805F is not set
> # CONFIG_SENSORS_F71882FG is not set
> # CONFIG_SENSORS_F75375S is not set
> # CONFIG_SENSORS_IT87 is not set
> # CONFIG_SENSORS_PC87360 is not set
> # CONFIG_SENSORS_PC87427 is not set
> # CONFIG_SENSORS_DME1737 is not set
> # CONFIG_SENSORS_SMSC47M1 is not set
> # CONFIG_SENSORS_SMSC47B397 is not set
> # CONFIG_SENSORS_VT1211 is not set
> # CONFIG_SENSORS_W83627HF is not set
> # CONFIG_SENSORS_W83627EHF is not set
> 
> While the first two might not be required, and the third is just
> selecting the right platform, it would be nice to get the drivers
> to play as well as the rest of the kernel.
> 
> The drivers all are for super-io chips, either irda/FIR drivers or hwmon,
> and poke at isa ports without checking request_region.
> 

Erm,

The superio sensor drivers only poke the superio chip registers without request 
region during the probe phase, iow they try to detect the chip, using a widely 
document and standardized (part of isa pnp AFAIK) procedure on standardized ports.

Let me try to explain a bit about superio chips, they have 2 superio control 
registers (an index and data register) with which things like a manufacturer 
and device id can be read, besides these id registers they also have a set of 
registers with config for different logical devices. Once the id is matched, 
the driver knows which logical device config to read, reads a (different) isa 
base address + range from the logical device config, and then does a 
request_region on the region actually used by the logical device.

The superio control registers are thus a sort of pci configuration space if you 
want, doing a request_region on these is not such a good idea, as multiple 
drivers (for different logical devices within the superio device) may use 
these, so trying to gain exclusive access will lead to troubles.

I hope with this info about the problem space, that you maybe have a suggestion 
on howto fix this?

Regards,

Hans
> 

Re: [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Milton Miller]

On Jul 10, 2008, at 4:33 PM, Hans de Goede wrote:

> Milton Miller wrote:
>> After the following patch to mark the isa region busy and applying a 
>> few
>> patches[1], I was able to kexec boot into an all-yes-config kernel 
>> from linux-next, with the following KCONFIG_ALLCONFIG file:
...
>> While the first two might not be required, and the third is just
>> selecting the right platform, it would be nice to get the drivers
>> to play as well as the rest of the kernel.
>> The drivers all are for super-io chips, either irda/FIR drivers or 
>> hwmon,
>> and poke at isa ports without checking request_region.
>
> Erm,
>
> The superio sensor drivers only poke the superio chip registers 
> without request region during the probe phase, iow they try to detect 
> the chip, using a widely document and standardized (part of isa pnp 
> AFAIK) procedure on standardized ports.
>
> Let me try to explain a bit about superio chips, they have 2 superio 
> control registers (an index and data register) with which things like 
> a manufacturer and device id can be read, besides these id registers 
> they also have a set of registers with config for different logical 
> devices. Once the id is matched, the driver knows which logical device 
> config to read, reads a (different) isa base address + range from the 
> logical device config, and then does a request_region on the region 
> actually used by the logical device.
>
> The superio control registers are thus a sort of pci configuration 
> space if you want, doing a request_region on these is not such a good 
> idea, as multiple drivers (for different logical devices within the 
> superio device) may use these, so trying to gain exclusive access will 
> lead to troubles.
>
> I hope with this info about the problem space, that you maybe have a 
> suggestion on howto fix this?
>

My point is that they are probing without even knowing that a such a IO 
range exists.

These are the only drivers left in the tree that still do this.  (At 
least that are not
blocked on a powerpc allyesconfig for 64-bit, !CONFIG_VIRT_TO_BUS).

One could make a superio driver, and create sub-devices for the IR, 
I2C, floppy, parallel, etc
nodes.

I would even accept a check_region (horrors!), it would impove the 
situation.

But most other drivers do this by request_region, probe, then release 
the region afterwards.

Besides, one could argue the superio region should be requested while 
probing, to prevent other cpus from poking at the super io chip at the 
same time.   Think of it as missing locking.

milton

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Jean Delvare]

Hi Hans, hi Milton,

On Thu, 10 Jul 2008 16:51:33 -0500, Milton Miller wrote:
> 
> On Jul 10, 2008, at 4:33 PM, Hans de Goede wrote:
> 
> > Milton Miller wrote:
> >> After the following patch to mark the isa region busy and applying a 
> >> few
> >> patches[1], I was able to kexec boot into an all-yes-config kernel 
> >> from linux-next, with the following KCONFIG_ALLCONFIG file:
> ...
> >> While the first two might not be required, and the third is just
> >> selecting the right platform, it would be nice to get the drivers
> >> to play as well as the rest of the kernel.
> >> The drivers all are for super-io chips, either irda/FIR drivers or 
> >> hwmon,
> >> and poke at isa ports without checking request_region.
> >
> > Erm,
> >
> > The superio sensor drivers only poke the superio chip registers 
> > without request region during the probe phase, iow they try to detect 
> > the chip, using a widely document and standardized (part of isa pnp 
> > AFAIK) procedure on standardized ports.
> >
> > Let me try to explain a bit about superio chips, they have 2 superio 
> > control registers (an index and data register) with which things like 
> > a manufacturer and device id can be read, besides these id registers 
> > they also have a set of registers with config for different logical 
> > devices. Once the id is matched, the driver knows which logical device 
> > config to read, reads a (different) isa base address + range from the 
> > logical device config, and then does a request_region on the region 
> > actually used by the logical device.
> >
> > The superio control registers are thus a sort of pci configuration 
> > space if you want, doing a request_region on these is not such a good 
> > idea, as multiple drivers (for different logical devices within the 
> > superio device) may use these, so trying to gain exclusive access will 
> > lead to troubles.
> >
> > I hope with this info about the problem space, that you maybe have a 
> > suggestion on howto fix this?
> 
> My point is that they are probing without even knowing that a such a IO 
> range exists.
> 
> These are the only drivers left in the tree that still do this.  (At 
> least that are not
> blocked on a powerpc allyesconfig for 64-bit, !CONFIG_VIRT_TO_BUS).

I guess we could make all these drivers depend on X86, I suspect that
these chips are only used in PCs?

> One could make a superio driver, and create sub-devices for the IR, 
> I2C, floppy, parallel, etc
> nodes.

There have been proposals to do this, and this would indeed be a very
good idea, but unfortunately nobody took the time to implement this
properly, push it upstream and volunteer to maintain it. The problem is
that you don't need just a "driver", but a new subsystem, that needs to
be designed and maintained.

> I would even accept a check_region (horrors!), it would impove the 
> situation.
> 
> But most other drivers do this by request_region, probe, then release 
> the region afterwards.

I agree that the hwmon drivers should do this, as long as no superio
subsystem exist. This should solve the problem at hand and might even
help spot bad drivers which use the configuration space for longer than
initialization time.

> Besides, one could argue the superio region should be requested while 
> probing, to prevent other cpus from poking at the super io chip at the 
> same time.   Think of it as missing locking.

Agreed.

Milton, care to submit a patch fixing the affected hwmon drivers?

Thanks,
-- 
Jean Delvare

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans, hi Milton,
> 

<snip>

>> One could make a superio driver, and create sub-devices for the IR, 
>> I2C, floppy, parallel, etc
>> nodes.
> 
> There have been proposals to do this, and this would indeed be a very
> good idea, but unfortunately nobody took the time to implement this
> properly, push it upstream and volunteer to maintain it. The problem is
> that you don't need just a "driver", but a new subsystem, that needs to
> be designed and maintained.
> 

Well, I believe there have been some lightweight superio locking coordinator 
patches been floating around on the lm_sensors list, and I have reviewed them 
and then a new version was done with my issues fixed.

I kinda liked the proposed solution there, it was quite simple, moved all the 
generic superio stuff into generic superio code, and added locking for super io 
access from multiple drivers, what ever happened to those patches?

If were to start using those, we could actually do a request region and then 
never release it, as things should be.

Regards,

Hans

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Jean Delvare]

On Fri, 11 Jul 2008 09:27:26 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > Hi Hans, hi Milton,
> > 
> 
> <snip>
> 
> >> One could make a superio driver, and create sub-devices for the IR, 
> >> I2C, floppy, parallel, etc
> >> nodes.
> > 
> > There have been proposals to do this, and this would indeed be a very
> > good idea, but unfortunately nobody took the time to implement this
> > properly, push it upstream and volunteer to maintain it. The problem is
> > that you don't need just a "driver", but a new subsystem, that needs to
> > be designed and maintained.
> 
> Well, I believe there have been some lightweight superio locking coordinator 
> patches been floating around on the lm_sensors list, and I have reviewed them 
> and then a new version was done with my issues fixed.
> 
> I kinda liked the proposed solution there, it was quite simple, moved all the 
> generic superio stuff into generic superio code, and added locking for super io 
> access from multiple drivers, what ever happened to those patches?

As far as I know, nothing, and this is the problem. Somebody needs to
step up and call him/herself the maintainer of the new code, and push
it upstream and convert all the drivers (hwmon, watchdog, parallel
port...) to make use of it. And I am not the one to do this, I am busy
enough as is with i2c and hwmon.

> If were to start using those, we could actually do a request region and then 
> never release it, as things should be.

Yes, if we have a superio access coordinator, it can request the region
and not release it. But as long as we don't have that, I agree with
Milton that the individual drivers should temporarily request the
Super-I/O region before accessing it.

-- 
Jean Delvare

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Hans de Goede]

Jean Delvare wrote:
> On Fri, 11 Jul 2008 09:27:26 +0200, Hans de Goede wrote:
>> Jean Delvare wrote:
>>> Hi Hans, hi Milton,
>>>
>> <snip>
>>
>>>> One could make a superio driver, and create sub-devices for the IR, 
>>>> I2C, floppy, parallel, etc
>>>> nodes.
>>> There have been proposals to do this, and this would indeed be a very
>>> good idea, but unfortunately nobody took the time to implement this
>>> properly, push it upstream and volunteer to maintain it. The problem is
>>> that you don't need just a "driver", but a new subsystem, that needs to
>>> be designed and maintained.
>> Well, I believe there have been some lightweight superio locking coordinator 
>> patches been floating around on the lm_sensors list, and I have reviewed them 
>> and then a new version was done with my issues fixed.
>>
>> I kinda liked the proposed solution there, it was quite simple, moved all the 
>> generic superio stuff into generic superio code, and added locking for super io 
>> access from multiple drivers, what ever happened to those patches?
> 
> As far as I know, nothing, and this is the problem. Somebody needs to
> step up and call him/herself the maintainer of the new code, and push
> it upstream and convert all the drivers (hwmon, watchdog, parallel
> port...) to make use of it. And I am not the one to do this, I am busy
> enough as is with i2c and hwmon.
>

Ok, I've mailed Jim Cromie, the author of the superio access patches 
from end 2007 and told him we're still interested in this and asked him 
if he is willing to drive this forward.

Regards,

Hans

Re: [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[David Hubbard]

Hi Hans, Milton, Jean,

On Sun, Jul 13, 2008 at 12:31 AM, Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
> Jean Delvare wrote:
>> On Fri, 11 Jul 2008 09:27:26 +0200, Hans de Goede wrote:
>>> Jean Delvare wrote:
>>>> Hi Hans, hi Milton,
>>>>
>>> <snip>
>>>
>>>>> One could make a superio driver, and create sub-devices for the IR,
>>>>> I2C, floppy, parallel, etc
>>>>> nodes.
>>>> There have been proposals to do this, and this would indeed be a very
>>>> good idea, but unfortunately nobody took the time to implement this
>>>> properly, push it upstream and volunteer to maintain it. The problem is
>>>> that you don't need just a "driver", but a new subsystem, that needs to
>>>> be designed and maintained.
>>> Well, I believe there have been some lightweight superio locking coordinator
>>> patches been floating around on the lm_sensors list, and I have reviewed them
>>> and then a new version was done with my issues fixed.
>>>
>>> I kinda liked the proposed solution there, it was quite simple, moved all the
>>> generic superio stuff into generic superio code, and added locking for super io
>>> access from multiple drivers, what ever happened to those patches?
>>
>> As far as I know, nothing, and this is the problem. Somebody needs to
>> step up and call him/herself the maintainer of the new code, and push
>> it upstream and convert all the drivers (hwmon, watchdog, parallel
>> port...) to make use of it. And I am not the one to do this, I am busy
>> enough as is with i2c and hwmon.
>>
>
> Ok, I've mailed Jim Cromie, the author of the superio access patches
> from end 2007 and told him we're still interested in this and asked him
> if he is willing to drive this forward.

I propose writing a subsystem driver. (Is that properly called "The
SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
put together some code and submit it for review, and maintain it.

Some hwmon chips have odd / unique probe sequences. IMHO this is where
the design needs to be inspected. One of those is the w83627ehf, which
Jean and Hans are familiar enough with to check my work.

Thoughts?
David

Re: [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Hans de Goede]

David Hubbard wrote:
> Hi Hans, Milton, Jean,
> 
> On Sun, Jul 13, 2008 at 12:31 AM, Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
>> Jean Delvare wrote:
>>> On Fri, 11 Jul 2008 09:27:26 +0200, Hans de Goede wrote:
>>>> Jean Delvare wrote:
>>>>> Hi Hans, hi Milton,
>>>>>
>>>> <snip>
>>>>
>>>>>> One could make a superio driver, and create sub-devices for the IR,
>>>>>> I2C, floppy, parallel, etc
>>>>>> nodes.
>>>>> There have been proposals to do this, and this would indeed be a very
>>>>> good idea, but unfortunately nobody took the time to implement this
>>>>> properly, push it upstream and volunteer to maintain it. The problem is
>>>>> that you don't need just a "driver", but a new subsystem, that needs to
>>>>> be designed and maintained.
>>>> Well, I believe there have been some lightweight superio locking coordinator
>>>> patches been floating around on the lm_sensors list, and I have reviewed them
>>>> and then a new version was done with my issues fixed.
>>>>
>>>> I kinda liked the proposed solution there, it was quite simple, moved all the
>>>> generic superio stuff into generic superio code, and added locking for super io
>>>> access from multiple drivers, what ever happened to those patches?
>>> As far as I know, nothing, and this is the problem. Somebody needs to
>>> step up and call him/herself the maintainer of the new code, and push
>>> it upstream and convert all the drivers (hwmon, watchdog, parallel
>>> port...) to make use of it. And I am not the one to do this, I am busy
>>> enough as is with i2c and hwmon.
>>>
>> Ok, I've mailed Jim Cromie, the author of the superio access patches
>> from end 2007 and told him we're still interested in this and asked him
>> if he is willing to drive this forward.
> 
> I propose writing a subsystem driver. (Is that properly called "The
> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
> put together some code and submit it for review, and maintain it.
> 
> Some hwmon chips have odd / unique probe sequences. IMHO this is where
> the design needs to be inspected. One of those is the w83627ehf, which
> Jean and Hans are familiar enough with to check my work.
> 
> Thoughts?

I'm afraid that making this a "bus" will be a bit overkill. Jim's patches are 
quite simple and seem todo the job.

Also keep in mind that most users will be platform devices which just want to 
use the superio registers to find out the baseaddress of their logical device, 
a whole bus seems overkill for this, would the hwmon driver then need to be a 
superio_driver (as well as an platform_driver) or can the bus be queried / used
without having to be a bustype-driver?

Regards,

Hans

Re: [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[David Hubbard]

Hi Hans,

>> I propose writing a subsystem driver. (Is that properly called "The
>> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
>> put together some code and submit it for review, and maintain it.
>>
>> Some hwmon chips have odd / unique probe sequences. IMHO this is where
>> the design needs to be inspected. One of those is the w83627ehf, which
>> Jean and Hans are familiar enough with to check my work.
>>
>> Thoughts?
>
> I'm afraid that making this a "bus" will be a bit overkill. Jim's patches
> are quite simple and seem todo the job.
>
> Also keep in mind that most users will be platform devices which just want
> to use the superio registers to find out the baseaddress of their logical
> device, a whole bus seems overkill for this, would the hwmon driver then
> need to be a superio_driver (as well as an platform_driver) or can the bus
> be queried / used
> without having to be a bustype-driver?

I think that's a valid point. I am willing to keep it small, but I
would prefer to follow the pattern set in other subsystems. It may be
my lack of experience in designing a subsystem showing here! What are
some alternative ways to implement it?

In other words, Jim's patches are a good start, but maybe I am
misunderstanding them. I see it as the superio-locks module, a driver
that other drivers would depend on / auto-load. Is that something
other subsystems also do?

Regards,
David

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Jean Delvare]

On Sun, 13 Jul 2008 15:26:56 -0600, David Hubbard wrote:
> Hi Hans,
> 
> >> I propose writing a subsystem driver. (Is that properly called "The
> >> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
> >> put together some code and submit it for review, and maintain it.
> >>
> >> Some hwmon chips have odd / unique probe sequences. IMHO this is where
> >> the design needs to be inspected. One of those is the w83627ehf, which
> >> Jean and Hans are familiar enough with to check my work.
> >>
> >> Thoughts?
> >
> > I'm afraid that making this a "bus" will be a bit overkill. Jim's patches
> > are quite simple and seem todo the job.
> >
> > Also keep in mind that most users will be platform devices which just want
> > to use the superio registers to find out the baseaddress of their logical
> > device, a whole bus seems overkill for this, would the hwmon driver then
> > need to be a superio_driver (as well as an platform_driver) or can the bus
> > be queried / used
> > without having to be a bustype-driver?
> 
> I think that's a valid point. I am willing to keep it small, but I
> would prefer to follow the pattern set in other subsystems. It may be
> my lack of experience in designing a subsystem showing here! What are
> some alternative ways to implement it?
> 
> In other words, Jim's patches are a good start, but maybe I am
> misunderstanding them. I see it as the superio-locks module, a driver
> that other drivers would depend on / auto-load. Is that something
> other subsystems also do?

Well, there are two approaches to the problem. The first approach
(which I think Jim took in his patches? I don't really remember) is to
simply solve the problem of concurrent I/O access to the Super-I/O
configuration ports (typically 0x2e/0x2f or 0x4e/0x4f). That would be a
simple driver requesting the ports in question and exporting an API for
other drivers to access them in a safe way.

The second approach is to make it a whole subsystem, as David is
suggesting. The Super-I/O driver would then not only request the I/O
ports, it would also identify which Super-I/O is there, and it would
create devices (in the Linux device driver model sense of the term) for
every logical device we are interested in (amongst which the hwmon
ones.) The hwmon drivers would have to be converted from platform
drivers to superio drivers.

Each approach has its advantages. The first one is rather simple and
also very generic in nature. It could be reused for other purposes. The
second one offers automatic loading of hwmon drivers, which would be
nice to have.

There's probably a middle way which would keep the simplicity of the
first approach while still allowing for driver auto-loading, without
changing the bus type of all drivers. It would probably take some
research though.

Me, I don't really care which path we choose, given that I am not the
one who will take care of it. All I have to say is that this has been
discussed several times over the past 2 years and nothing came out of
it (as far as the mainline kernel is concerned, at least) so whatever
is done now, what really matters is that it makes it into the kernel
quickly. We have some drivers missing features because of this (for
example real-time reading of VID pin values.)

This should also not prevent us from fixing the drivers now so that
they temporarily request the Super-I/O ports they are using, as Milton
suggested. Not only we don't know when we will have something better to
offer, but it might also help us find conflicts between drivers, so
that we know which drivers should make use of the future superio
driver. This could also reveal conflicts with PNP BIOS reservations,
etc. Milton, will you write a patch?

-- 
Jean Delvare

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Milton Miller]

On Jul 14, 2008, at 2:59 AM, Jean Delvare wrote:
> On Sun, 13 Jul 2008 15:26:56 -0600, David Hubbard wrote:
>> Hi Hans,
>>
>>>> I propose writing a subsystem driver. (Is that properly called "The
>>>> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
>>>> put together some code and submit it for review, and maintain it.
>>>>
>>>> Some hwmon chips have odd / unique probe sequences. IMHO this is 
>>>> where
>>>> the design needs to be inspected. One of those is the w83627ehf, 
>>>> which
>>>> Jean and Hans are familiar enough with to check my work.
>>>>
>>>> Thoughts?
>>>
>>> I'm afraid that making this a "bus" will be a bit overkill. Jim's 
>>> patches
>>> are quite simple and seem todo the job.
>>>
>>> Also keep in mind that most users will be platform devices which 
>>> just want
>>> to use the superio registers to find out the baseaddress of their 
>>> logical
>>> device, a whole bus seems overkill for this, would the hwmon driver 
>>> then
>>> need to be a superio_driver (as well as an platform_driver) or can 
>>> the bus
>>> be queried / used
>>> without having to be a bustype-driver?
>>
>> I think that's a valid point. I am willing to keep it small, but I
>> would prefer to follow the pattern set in other subsystems. It may be
>> my lack of experience in designing a subsystem showing here! What are
>> some alternative ways to implement it?
>>
>> In other words, Jim's patches are a good start, but maybe I am
>> misunderstanding them. I see it as the superio-locks module, a driver
>> that other drivers would depend on / auto-load. Is that something
>> other subsystems also do?
>
> Well, there are two approaches to the problem. The first approach
> (which I think Jim took in his patches? I don't really remember) is to
> simply solve the problem of concurrent I/O access to the Super-I/O
> configuration ports (typically 0x2e/0x2f or 0x4e/0x4f). That would be a
> simple driver requesting the ports in question and exporting an API for
> other drivers to access them in a safe way.
>
> The second approach is to make it a whole subsystem, as David is
> suggesting. The Super-I/O driver would then not only request the I/O
> ports, it would also identify which Super-I/O is there, and it would
> create devices (in the Linux device driver model sense of the term) for
> every logical device we are interested in (amongst which the hwmon
> ones.) The hwmon drivers would have to be converted from platform
> drivers to superio drivers.
>
> Each approach has its advantages. The first one is rather simple and
> also very generic in nature. It could be reused for other purposes. The
> second one offers automatic loading of hwmon drivers, which would be
> nice to have.
>
> There's probably a middle way which would keep the simplicity of the
> first approach while still allowing for driver auto-loading, without
> changing the bus type of all drivers. It would probably take some
> research though.

I haven't done the research, but it might be keep superio as
a platform driver, and keep the clients as platform drivers.  Only
have the superio driver probe and discover the subcomponent
addresses and then create the platform devices as children
instead of having each driver create its own platform device.
(This all assumes they are all platform devices in sysfs, I have
not looked).

This is all because in the platform bus the bus driver does not
discover the addresses but relies on drivers or platform setup code.

> Me, I don't really care which path we choose, given that I am not the
> one who will take care of it. All I have to say is that this has been
> discussed several times over the past 2 years and nothing came out of
> it (as far as the mainline kernel is concerned, at least) so whatever
> is done now, what really matters is that it makes it into the kernel
> quickly. We have some drivers missing features because of this (for
> example real-time reading of VID pin values.)
>
> This should also not prevent us from fixing the drivers now so that
> they temporarily request the Super-I/O ports they are using, as Milton
> suggested. Not only we don't know when we will have something better to
> offer, but it might also help us find conflicts between drivers, so
> that we know which drivers should make use of the future superio
> driver. This could also reveal conflicts with PNP BIOS reservations,
> etc. Milton, will you write a patch?

Well, that is the second time you asked me, so I guess I should respond.

While I it is possible for me to write this patch, my schedule and
priority list predict it would not be before the merge window closes.  
In
fact, I'm not sure when it would come out.   It might be argued it could
go in early -rc, but I would fear somebody's chip will not be detected
with the additional check.  For example, the port may reserved as mother
board resources or something.  Also, I have no hardware to test any
proposed patch, so it would be compile check only.

milton

Re: [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Hans de Goede]

Milton Miller wrote:
> On Jul 14, 2008, at 2:59 AM, Jean Delvare wrote:
>> On Sun, 13 Jul 2008 15:26:56 -0600, David Hubbard wrote:
>>> Hi Hans,
>>>
>>>>> I propose writing a subsystem driver. (Is that properly called "The
>>>>> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
>>>>> put together some code and submit it for review, and maintain it.
>>>>>
>>>>> Some hwmon chips have odd / unique probe sequences. IMHO this is 
>>>>> where
>>>>> the design needs to be inspected. One of those is the w83627ehf, 
>>>>> which
>>>>> Jean and Hans are familiar enough with to check my work.
>>>>>
>>>>> Thoughts?
>>>> I'm afraid that making this a "bus" will be a bit overkill. Jim's 
>>>> patches
>>>> are quite simple and seem todo the job.
>>>>
>>>> Also keep in mind that most users will be platform devices which 
>>>> just want
>>>> to use the superio registers to find out the baseaddress of their 
>>>> logical
>>>> device, a whole bus seems overkill for this, would the hwmon driver 
>>>> then
>>>> need to be a superio_driver (as well as an platform_driver) or can 
>>>> the bus
>>>> be queried / used
>>>> without having to be a bustype-driver?
>>> I think that's a valid point. I am willing to keep it small, but I
>>> would prefer to follow the pattern set in other subsystems. It may be
>>> my lack of experience in designing a subsystem showing here! What are
>>> some alternative ways to implement it?
>>>
>>> In other words, Jim's patches are a good start, but maybe I am
>>> misunderstanding them. I see it as the superio-locks module, a driver
>>> that other drivers would depend on / auto-load. Is that something
>>> other subsystems also do?
>> Well, there are two approaches to the problem. The first approach
>> (which I think Jim took in his patches? I don't really remember) is to
>> simply solve the problem of concurrent I/O access to the Super-I/O
>> configuration ports (typically 0x2e/0x2f or 0x4e/0x4f). That would be a
>> simple driver requesting the ports in question and exporting an API for
>> other drivers to access them in a safe way.
>>
>> The second approach is to make it a whole subsystem, as David is
>> suggesting. The Super-I/O driver would then not only request the I/O
>> ports, it would also identify which Super-I/O is there, and it would
>> create devices (in the Linux device driver model sense of the term) for
>> every logical device we are interested in (amongst which the hwmon
>> ones.) The hwmon drivers would have to be converted from platform
>> drivers to superio drivers.
>>
>> Each approach has its advantages. The first one is rather simple and
>> also very generic in nature. It could be reused for other purposes. The
>> second one offers automatic loading of hwmon drivers, which would be
>> nice to have.
>>
>> There's probably a middle way which would keep the simplicity of the
>> first approach while still allowing for driver auto-loading, without
>> changing the bus type of all drivers. It would probably take some
>> research though.
> 
> I haven't done the research, but it might be keep superio as
> a platform driver, and keep the clients as platform drivers.  Only
> have the superio driver probe and discover the subcomponent
> addresses and then create the platform devices as children
> instead of having each driver create its own platform device.
> (This all assumes they are all platform devices in sysfs, I have
> not looked).
> 
> This is all because in the platform bus the bus driver does not
> discover the addresses but relies on drivers or platform setup code.
> 

This sounds like a good plan, rather then add a new bus type add a superio 
platform driver which does superio probing and registering of platform devices 
for discovered logical devices.

This superio platform driver then needs to also export some functions of those 
few logical devices which need access to the superio registers for more then 
just finding out their own base address.

I guess that it then would be best to load this superio driver by default on 
most systems.

How does this all mix and match with isapnp, it feels to me we're doing 
somewhat the same as isapnp here.

Regards,

Hans

Re: [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[David Hubbard]

Hi Hans,

On Mon, Jul 14, 2008 at 11:30 AM, Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
> Milton Miller wrote:
>> I haven't done the research, but it might be keep superio as
>> a platform driver, and keep the clients as platform drivers.  Only
>> have the superio driver probe and discover the subcomponent
>> addresses and then create the platform devices as children
>> instead of having each driver create its own platform device.
>> (This all assumes they are all platform devices in sysfs, I have
>> not looked).
>>
>> This is all because in the platform bus the bus driver does not
>> discover the addresses but relies on drivers or platform setup code.
>>
>
> This sounds like a good plan, rather then add a new bus type add a superio
> platform driver which does superio probing and registering of platform devices
> for discovered logical devices.
>
> This superio platform driver then needs to also export some functions of those
> few logical devices which need access to the superio registers for more then
> just finding out their own base address.
>
> I guess that it then would be best to load this superio driver by default on
> most systems.
>
> How does this all mix and match with isapnp, it feels to me we're doing
> somewhat the same as isapnp here.

Is there any way to use lspci and start at the LPC bridge, then find
the SuperIO chip's IO address? What about ACPI tables? Perhaps probing
logic could look for an LPC bridge before probing certain IO addresses
even if the addresses are not in the LPC bridge config.

A superio platform driver is a good way to go -- it fits with the way
the platform bus does things. Also, Jim's patches are almost there
already.

Thanks,
David

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Jean Delvare]

On Mon, 14 Jul 2008 11:55:01 -0600, David Hubbard wrote:
> Hi Hans,
> 
> On Mon, Jul 14, 2008 at 11:30 AM, Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
> > Milton Miller wrote:
> >> I haven't done the research, but it might be keep superio as
> >> a platform driver, and keep the clients as platform drivers.  Only
> >> have the superio driver probe and discover the subcomponent
> >> addresses and then create the platform devices as children
> >> instead of having each driver create its own platform device.
> >> (This all assumes they are all platform devices in sysfs, I have
> >> not looked).
> >>
> >> This is all because in the platform bus the bus driver does not
> >> discover the addresses but relies on drivers or platform setup code.
> >
> > This sounds like a good plan, rather then add a new bus type add a superio
> > platform driver which does superio probing and registering of platform devices
> > for discovered logical devices.
> >
> > This superio platform driver then needs to also export some functions of those
> > few logical devices which need access to the superio registers for more then
> > just finding out their own base address.
> >
> > I guess that it then would be best to load this superio driver by default on
> > most systems.
> >
> > How does this all mix and match with isapnp, it feels to me we're doing
> > somewhat the same as isapnp here.
> 
> Is there any way to use lspci and start at the LPC bridge, then find
> the SuperIO chip's IO address? What about ACPI tables? Perhaps probing
> logic could look for an LPC bridge before probing certain IO addresses
> even if the addresses are not in the LPC bridge config.

I always assumed that there was no way to know in advance if a
Super-I/O (LPC) chip was present or not, let alone the exact model of
the chip. The I/O addresses are decoded by the Super-I/O chip itself,
and in general it has no relation to PCI. And I've never seen ports
0x2e/0x2f nor 0x4e/0x4f listed in /proc/ioports.

But of course if there is a way to know, we should use it. Avoiding
random access to I/O ports, even if they are relatively standard in
this case, is always good.

> A superio platform driver is a good way to go -- it fits with the way
> the platform bus does things. Also, Jim's patches are almost there
> already.

Good.

-- 
Jean Delvare

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[David Hubbard]

Hi Jean,

On Tue, Jul 15, 2008 at 2:36 AM, Jean Delvare <khali@linux-fr.org> wrote:
>> Is there any way to use lspci and start at the LPC bridge, then find
>> the SuperIO chip's IO address? What about ACPI tables? Perhaps probing
>> logic could look for an LPC bridge before probing certain IO addresses
>> even if the addresses are not in the LPC bridge config.
>
> I always assumed that there was no way to know in advance if a
> Super-I/O (LPC) chip was present or not, let alone the exact model of
> the chip. The I/O addresses are decoded by the Super-I/O chip itself,
> and in general it has no relation to PCI. And I've never seen ports
> 0x2e/0x2f nor 0x4e/0x4f listed in /proc/ioports.
>
> But of course if there is a way to know, we should use it. Avoiding
> random access to I/O ports, even if they are relatively standard in
> this case, is always good.

I looked at my lspci output and did a little researching, and I think
the only thing we can deduce is that there is an LPC bridge, so
looking for a SuperIO is a good idea. If there is no LPC bridge
listed, I can't say whether probing the ports is a good idea or not.

David

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Jean Delvare]

Hi David,

On Tue, 15 Jul 2008 09:31:29 -0600, David Hubbard wrote:
> On Tue, Jul 15, 2008 at 2:36 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > I always assumed that there was no way to know in advance if a
> > Super-I/O (LPC) chip was present or not, let alone the exact model of
> > the chip. The I/O addresses are decoded by the Super-I/O chip itself,
> > and in general it has no relation to PCI. And I've never seen ports
> > 0x2e/0x2f nor 0x4e/0x4f listed in /proc/ioports.
> >
> > But of course if there is a way to know, we should use it. Avoiding
> > random access to I/O ports, even if they are relatively standard in
> > this case, is always good.
> 
> I looked at my lspci output and did a little researching, and I think
> the only thing we can deduce is that there is an LPC bridge, so
> looking for a SuperIO is a good idea. If there is no LPC bridge
> listed, I can't say whether probing the ports is a good idea or not.

Machines I have here have an "ISA bridge" PCI device. Some of them have
"LPC" in their name, but not all, and anyway the kernel only knows the
device ID, not its user-friendly name:

VIA:
00:11.0 ISA bridge: VIA Technologies, Inc. VT8237 ISA bridge [KT600/K8T800/K8T890 South]

Intel:
00:01.0 ISA bridge: Intel Corporation 82371AB/EB/MB PIIX4 ISA (rev 01)
00:1f.0 ISA bridge: Intel Corporation 82801CAM ISA Bridge (LPC) (rev 01)
00:1f.0 ISA bridge: Intel Corporation 82801EB/ER (ICH5/ICH5R) LPC Interface Bridge (rev 02)

nVidia:
00:01.0 ISA bridge: nVidia Corporation nForce2 ISA Bridge (rev a4)

One of these machines has a LPC bridge but no (detected) Super-I/O chip.

The VIA and nVidia machines do not have "LPC" in their bridge name, but
they do have a Super-I/O chip, while the first Intel machine doesn't.

As a conclusion, there is no clear relationship between the presence of
an ISA or LPC bridge and the presence of a Super-I/O chip. All we can
say is that apparently all PC systems have an ISA bridge. So indeed we
can probably make the probes conditional upon the presence of an "ISA
bridge" PCI device. That's very easy to test. In practice it might be
equivalent to making the driver x86-only.

-- 
Jean Delvare

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Rene Herman]

On 16-07-08 09:46, Jean Delvare wrote:

> As a conclusion, there is no clear relationship between the presence
> of an ISA or LPC bridge and the presence of a Super-I/O chip. All we
> can say is that apparently all PC systems have an ISA bridge. So
> indeed we can probably make the probes conditional upon the presence
> of an "ISA bridge" PCI device. That's very easy to test. In practice
> it might be equivalent to making the driver x86-only.

And foregoing non (pre-) PCI. Admittedly, that might not be much of an 
issue and I don't know if "Super-I/O" is maybe even defined such as to 
make it a NON issue but I do for example remember my old 386 chipset 
providing for extended serial rates which I thought was way cool back 
then. If that's Super-I/O (or something that should/could be covered by 
the infrastructure at least) then PCI wouldn't be helpful.

Rene.

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Jean Delvare]

Hi Milton,

On Mon, 14 Jul 2008 12:09:03 -0500, Milton Miller wrote:
> On Jul 14, 2008, at 2:59 AM, Jean Delvare wrote:
> > Well, there are two approaches to the problem. The first approach
> > (which I think Jim took in his patches? I don't really remember) is to
> > simply solve the problem of concurrent I/O access to the Super-I/O
> > configuration ports (typically 0x2e/0x2f or 0x4e/0x4f). That would be a
> > simple driver requesting the ports in question and exporting an API for
> > other drivers to access them in a safe way.
> >
> > The second approach is to make it a whole subsystem, as David is
> > suggesting. The Super-I/O driver would then not only request the I/O
> > ports, it would also identify which Super-I/O is there, and it would
> > create devices (in the Linux device driver model sense of the term) for
> > every logical device we are interested in (amongst which the hwmon
> > ones.) The hwmon drivers would have to be converted from platform
> > drivers to superio drivers.
> >
> > Each approach has its advantages. The first one is rather simple and
> > also very generic in nature. It could be reused for other purposes. The
> > second one offers automatic loading of hwmon drivers, which would be
> > nice to have.
> >
> > There's probably a middle way which would keep the simplicity of the
> > first approach while still allowing for driver auto-loading, without
> > changing the bus type of all drivers. It would probably take some
> > research though.
> 
> I haven't done the research, but it might be keep superio as
> a platform driver, and keep the clients as platform drivers.  Only
> have the superio driver probe and discover the subcomponent
> addresses and then create the platform devices as children
> instead of having each driver create its own platform device.
> (This all assumes they are all platform devices in sysfs, I have
> not looked).
> 
> This is all because in the platform bus the bus driver does not
> discover the addresses but relies on drivers or platform setup code.

That's more or less what I had in mind, yes.

> > (...) Milton, will you write a patch?
> 
> Well, that is the second time you asked me, so I guess I should respond.
> 
> While I it is possible for me to write this patch, my schedule and
> priority list predict it would not be before the merge window closes.  
> In fact, I'm not sure when it would come out.  It might be argued it
> could go in early -rc, but I would fear somebody's chip will not be detected
> with the additional check.  For example, the port may reserved as mother
> board resources or something.

I really don't see this as something for kernel 2.6.27, it's too late
already. It doesn't fix any actual problem anyway (none that can be
fixed by not loading drivers you don't need, at least.) That would be
for 2.6.28, so we have plenty of time to test the changes and ensure
they do not break anything. As you are the one who reported the issue
as something that was bothering you personally, I expected you to also
spend some time trying to address it.

> (...) Also, I have no hardware to test any
> proposed patch, so it would be compile check only.

If you write the patches and post them to the lm-sensors list, I am
certain that you will find several volunteers for testing. Me, I can
offer testing of the it87, f71805f and w83627ehf drivers.

Thanks,
-- 
Jean Delvare

[PATCH/RESEND] pci: dynids.use_driver_data considered harmful

[Milton Miller]

I typo'd on the cc to linux-kernel the first time, and wanted to take make a
second pass at the long narritive of my debug session promoting this patch.
I think I got the new linux-pci list, but google doesn't seem to find any
archives of the new list or this patch.

Andrew,
	Thanks for picking this up and the expanded cc list.  I took the
patch from mmotm and only changed the changlog.  Please update your copy.

Jesse,
	I don't care if you commit this version with the full narritive
or just the part through the footnote.

I can supply the debug patch if someone is intrested, but it just did
the classification in __pci_regsiter_driver.

Thanks,
milton

=== cut here ===

The driver flag dynids.use_driver_data is almost consistently not set, and
causes more problems than it solves.  The only use of this flag is to
prevent the system administrator from telling a pci driver additional data
when adding a dynamic match table entry via sysfs.

We do not as a policy protect the kernel from bad acts by root.

If we are worried about drivers doing the wrong thing due the match data, we
should be setting a taint flag, not attempting to cut off the admin.

[1] Searching the next-20080709 tree shows the bit is set by exactly
3 pci drivers.  However, the use of per-match-entry driver data is
much more prevalent: A boot of allyesconfig on a PowerPC64 pSeries
with a debug patch shows 27 drivers apparently use the field for a
pointer, 14 use it for setting flags, and 98 use it as a table
index.  (Pointers are defined as values above>PAGE_OFFSET, aka in
the 64 bit kernel linear mapping.  Flags are defined as the maximum
value exceeds the number of entries in the match table.  Any other
nonzero value is classified as an index).

This behavior actually caused me several hours of debug recently.

I was working working with a cxgb3 card in an environment based on mainline,
at the time on the 2.6.24 kernel.  I was told a version of the driver was
working on a distro kernel with this card and the necessary support should
be upstream.  I configured the driver into the kernel, inserted the the card
into the system, and booted, but the card was not recognized by the driver.
Assuming the card had some unique device id, I tried adding a the vendor and
device via sysfs, and the kernel crashed.  I then looked closer at the
driver, and discovered the vendor and device were listed, but the match
table also expected the subsystem device id (owned by the subsystem vendor,
not the silicon vendor) was being matched to the value 1.  I also noted the
driver used the id match table driver data field.  I peeked at the pci sysfs
code to find out which parameter would be driver data, rebooted, and tried
again.   The kernel crashed again.

Ok supporting this card isn't going to be trivial.   I then acquired a card
for my own setup instead of trying to debug using remote access to a user's
machine across the country.  After a few days to locate a card in town,
specificly one that had been tested with the distro kernel driver to rule
out faulty hardware, I replicated the failure.  Digging further into the
crash, I quickly determined the crash was a branch to what appeared to be
ascii text from somewhere in the device probe routine.  I looked at what had
been put in 2.6.25 (this was after the merge window, but still early -rc
time frame), and saw the driver id table match was relaxed, and some other
changes.  However, they didn't appear to be related, and, due to net wide
changes, would require some time to backport.

Seeing the corruption was ascii, I figured the problem was some data
dependent overwrite or buffer overflow, probably trashing the stack.
Perhaps the problem was reading the vpd, parsing it, or some endian issue
reading a size somewhere.  The vpd parsing code uses some macros to grab
data out of a structure, sounds suspicious, something probably doesn't line
up.

I looked at the assembly code around the return address and could tell the
call was through a function pointer, but, like many probe functions full of
calls to use-once functions, it was not clear which pointer was being
dereferenced, only that it was some large offset into some driver structure.

This was on PowerPC64, so the function pointer is a descriptor pointer.  The
pointer itself looks valid, but instead of pointing to a descriptor with a
code address and a constant pool, it points to random kernel data.  Of
course, when debugging, one sees some pointer that looks valid, but points
to ascii garbage instead of the code and toc.  What function should be
there?  Was a pointer used uninitialized?

I looked closer at the failure point, unraveling the code flow and few
non-pointer function calles and determined it was past the vpd parsing, and
definitely into the port initialization code, but not clear which function
was breaking.

I tried sprinkling printk and even while(1) into the driver (debugging with
tools to inspect memory without the cpu) and the called port enable
functions, but couldn't track it down, it seemed like it worked as expected
and I couldn't find any memory corruption, yet the loop didn't reach its
exit point.

Eventually I found both the device pointer and the port pointer, and
unraveled the offset of the port pointer nested in several large structures
of driver data was pointing to the second port.  Huh? I told it this was a
card that only had one port.  Is the loop control on the stack getting
stomped upon?  No, the structure says it has two ports.  I double checked
the match table index, yes that was correct.  I double checked the parse of
new_id, yes, I am using the right parameter.

I eventually determined the following sequence of events had occurred:

The pci driver sysfs code took my id, but since dynids.use_driver_data was
not set, filled the driver data field with 0.  It then re-scanned the
devices and found the new match, and called the driver probe routine.  The
driver took the driver data and performed a range check.  It then indexed
into a table, and (since it was given 0 when I specified 2), it incorrectly
determined the card should have two ports instead of one.  It then copied
the vpd into an array, and indexed into the array based on a template of
what it thought the vpd would contain (as opposed to parsing the tags in the
vpd with their length fields).  The code then searched in the vpd for
descriptions of the phy on each possible port.  It found the phy on physical
port 0, indexed into another table and filled out its pointers.  It then
searched for the non-existent card port 1, looping to find the next port
with a non-zero value for the type of phy.  Because the card actually only
had one port, it went off the end of the vpd data and found some random
number in memory.  It then used that random value as the type to index into
its second array, again ran well beyond the end of the array, and filled out
the function pointers to be used for the second port.  It happened to land
on kernel data containting a pointer, so the function descriptor looked to
be a valid pointer.  It proceeded on, in the same function after inlining,
to iterate over the ports on the card to initialize each port.  It calls the
function for the first port, all goes well.  It advances a pointer to point
to the next port, calls the first function, dereferences the function
descriptor to get the code address, and dies with a branch to nowhere.

Only after careful analysis of the driver data structure did I find it was
looking for more ports.  After all, I had told the driver to use slot two,
and that said to expect the card to only have one port described in the vpd.

While there is could certainly be more defensive coding in the driver (eg:
parse the vpd instead of relying on knowing the tag layout, only search
until the end of the field in the vpd data, range check the port type before
indexing into an array), the whole session should have been avoided once I
looked at the driver and carefully set the driver data to entry 2, to match
the other users of the silicon.

Signed-off-by: Milton Miller <miltonm@bga.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: James Bottomley <James.Bottomley@SteelEye.com>
Cc: Brian King <brking@us.ibm.com>
Cc: Jean Delvare <khali@linux-fr.org>
Cc: Tejun Heo <htejun@gmail.com>
Cc: Michael Ellerman <michael@ellerman.id.au>
Cc: Jeff Garzik <jeff@garzik.org>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/i2c/busses/i2c-amd756.c |    1 -
 drivers/i2c/busses/i2c-viapro.c |    1 -
 drivers/pci/pci-driver.c        |    3 +--
 drivers/scsi/ipr.c              |    1 -
 include/linux/pci.h             |    1 -
 5 files changed, 1 insertion(+), 6 deletions(-)

diff -puN drivers/i2c/busses/i2c-amd756.c~pci-dynidsuse_driver_data-considered-harmful drivers/i2c/busses/i2c-amd756.c
--- a/drivers/i2c/busses/i2c-amd756.c~pci-dynidsuse_driver_data-considered-harmful
+++ a/drivers/i2c/busses/i2c-amd756.c
@@ -412,7 +412,6 @@ static struct pci_driver amd756_driver =
 	.id_table	= amd756_ids,
 	.probe		= amd756_probe,
 	.remove		= __devexit_p(amd756_remove),
-	.dynids.use_driver_data = 1,
 };
 
 static int __init amd756_init(void)
diff -puN drivers/i2c/busses/i2c-viapro.c~pci-dynidsuse_driver_data-considered-harmful drivers/i2c/busses/i2c-viapro.c
--- a/drivers/i2c/busses/i2c-viapro.c~pci-dynidsuse_driver_data-considered-harmful
+++ a/drivers/i2c/busses/i2c-viapro.c
@@ -468,7 +468,6 @@ static struct pci_driver vt596_driver = 
 	.name		= "vt596_smbus",
 	.id_table	= vt596_ids,
 	.probe		= vt596_probe,
-	.dynids.use_driver_data = 1,
 };
 
 static int __init i2c_vt596_init(void)
diff -puN drivers/pci/pci-driver.c~pci-dynidsuse_driver_data-considered-harmful drivers/pci/pci-driver.c
--- a/drivers/pci/pci-driver.c~pci-dynidsuse_driver_data-considered-harmful
+++ a/drivers/pci/pci-driver.c
@@ -65,8 +65,7 @@ store_new_id(struct device_driver *drive
 	dynid->id.subdevice = subdevice;
 	dynid->id.class = class;
 	dynid->id.class_mask = class_mask;
-	dynid->id.driver_data = pdrv->dynids.use_driver_data ?
-		driver_data : 0UL;
+	dynid->id.driver_data = driver_data;
 
 	spin_lock(&pdrv->dynids.lock);
 	list_add_tail(&dynid->node, &pdrv->dynids.list);
diff -puN drivers/scsi/ipr.c~pci-dynidsuse_driver_data-considered-harmful drivers/scsi/ipr.c
--- a/drivers/scsi/ipr.c~pci-dynidsuse_driver_data-considered-harmful
+++ a/drivers/scsi/ipr.c
@@ -7854,7 +7854,6 @@ static struct pci_driver ipr_driver = {
 	.remove = ipr_remove,
 	.shutdown = ipr_shutdown,
 	.err_handler = &ipr_err_handler,
-	.dynids.use_driver_data = 1
 };
 
 /**
diff -puN include/linux/pci.h~pci-dynidsuse_driver_data-considered-harmful include/linux/pci.h
--- a/include/linux/pci.h~pci-dynidsuse_driver_data-considered-harmful
+++ a/include/linux/pci.h
@@ -339,7 +339,6 @@ struct pci_bus_region {
 struct pci_dynids {
 	spinlock_t lock;            /* protects list, index */
 	struct list_head list;      /* for IDs added at runtime */
-	unsigned int use_driver_data:1; /* pci_device_id->driver_data is used */
 };
 
 /* ---------------------------------------------------------------- */
_

Re: [PATCH/RESEND] pci: dynids.use_driver_data considered harmful

[Greg KH]

On Sat, Jul 12, 2008 at 03:02:22PM -0500, Milton Miller wrote:
> I typo'd on the cc to linux-kernel the first time, and wanted to take make a
> second pass at the long narritive of my debug session promoting this patch.
> I think I got the new linux-pci list, but google doesn't seem to find any
> archives of the new list or this patch.
> 
> Andrew,
> 	Thanks for picking this up and the expanded cc list.  I took the
> patch from mmotm and only changed the changlog.  Please update your copy.

Andrew, please drop this patch, it is not correct, the code should stay
as-is.  See my previous reply for why this is so.

> Jesse,
> 	I don't care if you commit this version with the full narritive
> or just the part through the footnote.

Jesse, please don't commit this either.

Milton, if you wish to ignore my response, well, that's up to you :(

thanks,

greg k-h

Re: [PATCH/RESEND] pci: dynids.use_driver_data considered harmful

[Jean Delvare]

Hi Greg, Milton,

On Sat, 12 Jul 2008 13:17:03 -0700, Greg KH wrote:
> On Sat, Jul 12, 2008 at 03:02:22PM -0500, Milton Miller wrote:
> > I typo'd on the cc to linux-kernel the first time, and wanted to take make a
> > second pass at the long narritive of my debug session promoting this patch.
> > I think I got the new linux-pci list, but google doesn't seem to find any
> > archives of the new list or this patch.
> > 
> > Andrew,
> > 	Thanks for picking this up and the expanded cc list.  I took the
> > patch from mmotm and only changed the changlog.  Please update your copy.
> 
> Andrew, please drop this patch, it is not correct, the code should stay
> as-is.  See my previous reply for why this is so.

Care to copy it here? Apparently it all happened in a private
discussion.

> > Jesse,
> > 	I don't care if you commit this version with the full narritive
> > or just the part through the footnote.
> 
> Jesse, please don't commit this either.
> 
> Milton, if you wish to ignore my response, well, that's up to you :(

-- 
Jean Delvare

Re: [PATCH/RESEND] pci: dynids.use_driver_data considered harmful

[Milton Miller]

On Jul 12, 2008, at 3:17 PM, Greg KH wrote:

> On Sat, Jul 12, 2008 at 03:02:22PM -0500, Milton Miller wrote:
>
> Milton, if you wish to ignore my response, well, that's up to you :(
>

I did not find your response until after I had done the resend,
which was triggererd by the typo to linux-kernel.

I have now replied to your NAK and hope you reconsider.

milton

Re: [PATCH/RESEND] pci: dynids.use_driver_data considered harmful

[Milton Miller]

On Jul 12, 2008, at 4:17 PM, Milton Miller wrote:

>
> On Jul 12, 2008, at 3:17 PM, Greg KH wrote:
>
>> On Sat, Jul 12, 2008 at 03:02:22PM -0500, Milton Miller wrote:
>>
>> Milton, if you wish to ignore my response, well, that's up to you :(
>>
>
> I did not find your response until after I had done the resend,
> which was triggererd by the typo to linux-kernel.
>
> I have now replied to your NAK and hope you reconsider.
>
> milton
>


Final-Recipient: rfc822;james.bottomley@steeleye.com
Action: failed
Status: 5.1.1

Aparently it should be James Bottomley 
<James.Bottomley@HansenPartnership.com>,
looking at signed-off-by logs.   Andrew, it came from your cc list.

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Milton Miller]

For those on lkml, I mistyped the address in the first mail.  I just
did a second pass edit on the long debug session prompting this patch
fixed the cc to linux-kernel.  So all those not on linux-pci should
be seeing that shortly, or one can look in mmotm.

On Jul 11, 2008, at 11:11 PM, Greg KH wrote:
> On Thu, Jul 10, 2008 at 04:29:37PM -0500, Milton Miller wrote:
>> The driver flag dynids.use_driver_data is almost consistently not 
>> set, and
>> causes more problems than it solves.  The only use of this flag is to
>> prevent the system administrator from telling a pci driver additional 
>> data
>> when adding a dynamic match table entry via sysfs.
>>
>> We do not as a policy protect the kernel from bad acts by root.
>
> But we can try.
>
> As you found out, this flag should have been set in that specific
> driver, right?  Why not just fix the driver instead of declaring that
> the flag is not useful at all, despite it actually being used properly
> by a number of drivers already?

Its not one driver, its more like 100 to 150.

Why is giving the driver the data it needs to support a card bad?
The debug session shows that not giving it the data it needs
is worse.

Worse, the kernel thinks it superior to the human and actively ignores
input with out even telling the user it did so.

The only argument I can come up with is if the data is a pointer, then
its likely that that root will not be able to find the correct value or
put it in a script that doesn't get updated on the next kernel install
and causes a crash.  Root isn't likely to add driver data unless it
discovers that it is needed, so the existing default of passing 0
should remain.

If you want to argue that being able to tell the driver that it should
treat this card like card X is wrong, you should be arguing that the
whole notion of adding match table entries to the driver is wrong.
When it comes down to it, its the same thing.  Yet, we support adding
ids today.  The feature was added, and is used successfully with many
drivers.   I'm trying to extend the population of drivers where it works
to include those that support more than one similar card but need to
know which one they are dealing with.

You left out:
>> Searching the next-20080709 tree shows the bit is set by exactly 3 pci
>> drivers.  However, the use of per-match-entry driver data is much more
>> prevalent: A boot of allyesconfig on a powerpc64 pseries with a debug 
>> patch
>> shows 27 drivers apparently use the field for a pointer, 14 use it for
>> setting flags, and 98 use it as a table index.  (Pointers are defined 
>> as
>> >PAGE_OFFSET, aka in the 64 bit kernel linear mapping.  Flags are 
>> defined as
>> the maximum value exceeds the number of entries in the match table.  
>> Any
>> other nonzero value is classified as an index).

I did the work to establish the scope of the problem.

So 3 drivers set the flag.  98 + 14 should set it immediately.  The 27 
that
use the value as a pointer probably can be re-written to use it as an 
index.  And
the hundreds of other drivers (I don't have the tree here to count) 
apparently
don't care, as they most likely never look at that field.  (I am 
ingoring
conditional copiles that are not enabled by all-yes-config).

> So no, this patch is not good, it should stay as-is.

Obviously I disagree.

Since we added passing the id table entry to get the driver data from 
the
table entry, we have close to 150 drivers that use it, instead of old 
methods
such has hard coded lists of chips in case statements.  Besides making 
it easier
to maintain the driver, it actually improves the chances that the 
driver can be
used on a new revision or subsystem id of the card.

But we have not been reviewing the setting of the flag with the use in 
the driver.
A usage pattern of storing an index or flags has developed, and I claim 
that the
flag now hurts more than it helps.

milton

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Milton Miller]

On Jul 12, 2008, at 4:08 PM, Milton Miller wrote:

> You left out:
>>> Searching the next-20080709 tree shows the bit is set by exactly 3 
>>> pci
>>> drivers.  However, the use of per-match-entry driver data is much 
>>> more
>>> prevalent: A boot of allyesconfig on a powerpc64 pseries with a 
>>> debug patch
>>> shows 27 drivers apparently use the field for a pointer, 14 use it 
>>> for
>>> setting flags, and 98 use it as a table index.  (Pointers are 
>>> defined as
>>> >PAGE_OFFSET, aka in the 64 bit kernel linear mapping.  Flags are 
>>> defined as
>>> the maximum value exceeds the number of entries in the match table.  
>>> Any
>>> other nonzero value is classified as an index).
>
> I did the work to establish the scope of the problem.
>

For the record or independent investigation, here is the patch and grep 
of
output from the kernel log.   Hopefully you can read them after this 
mailer
mangles them.

milton

[   30.412634] PCI Driver CyberPro(in cyber2000fb) uses data 3/3 times 
for indexes
[   30.418967] PCI Driver aty128fb(in aty128fb) uses data 37/47 times 
for indexes
[   30.419787] PCI Driver radeonfb(in radeonfb) uses data 98/98 times 
for flags
[   30.421104] PCI Driver sisfb(in sisfb) uses data 11/12 times for 
indexes
[   30.422332] PCI Driver savagefb(in savagefb) uses data 23/23 times 
for flags
[   30.423088] PCI Driver neofb(in neofb) uses data 9/9 times for flags
[   30.424431] PCI Driver imsttfb(in imsttfb) uses data 1/2 times for 
indexes
[   30.427089] PCI Driver s3fb(in s3fb) uses data 12/12 times for flags
[   30.428495] PCI Driver sstfb(in sstfb) uses data 1/2 times for 
indexes
[   30.429439] PCI Driver cirrusfb(in cirrusfb) uses data 11/11 times 
for indexes
[   30.431251] PCI Driver gxt4500(in gxt4500) uses data 1/2 times for 
indexes
[   33.082152] PCI Driver epca(in epca) uses data 3/4 times for indexes
[   37.857569] PCI Driver serial(in 8250_pci) uses data 177/180 times 
for indexes
[   37.868296] PCI Driver jsm(in jsm) uses data 4/5 times for indexes
[   37.901045] PCI Driver parport_pc(in parport_pc) uses data 52/53 
times for indexes
[   37.906019] PCI Driver parport_serial(in parport_serial) uses data 
30/31 times for indexes
[   38.253736] PCI Driver DAC960(in DAC960) uses data 7/7 times for 
pointers
[   38.983951] PCI Driver e1000e(in e1000e) uses data 29/38 times for 
indexes
[   39.026014] PCI Driver Sundance Technology IPG Triple-Speed 
Ethernet(in ipg) uses data 5/6 times for indexes
[   39.033218] PCI Driver cxgb(in cxgb) uses data 5/7 times for indexes
[   39.039465] PCI Driver cxgb3(in cxgb3) uses data 9/10 times for 
indexes
[   39.092305] PCI Driver 3c59x(in 3c59x) uses data 37/38 times for 
indexes
[   39.098029] PCI Driver typhoon(in typhoon) uses data 12/13 times for 
indexes
[   39.103976] PCI Driver ne2k-pci(in ne2k_pci) uses data 10/11 times 
for indexes
[   39.122763] PCI Driver e100(in e100) uses data 36/41 times for 
indexes
[   39.129041] PCI Driver tlan(in tlan) uses data 12/13 times for 
indexes
[   39.141155] PCI Driver epic100(in epic100) uses data 2/3 times for 
indexes
[   39.146232] PCI Driver sis190(in sis190) uses data 1/2 times for 
indexes
[   39.151792] PCI Driver sis900(in sis900) uses data 1/2 times for 
indexes
[   39.161699] PCI Driver yellowfin(in yellowfin) uses data 1/2 times 
for indexes
[   39.173144] PCI Driver natsemi(in natsemi) uses data 1/2 times for 
indexes
[   39.184923] PCI Driver fealnx(in fealnx) uses data 2/3 times for 
indexes
[   39.196097] PCI Driver bnx2(in bnx2) uses data 8/9 times for indexes
[   39.201880] PCI Driver bnx2x(in bnx2x) uses data 2/3 times for 
indexes
[   39.347752] PCI Driver sundance(in sundance) uses data 6/7 times for 
indexes
[   39.372095] PCI Driver forcedeth(in forcedeth) uses data 39/39 times 
for flags
[   39.426772] PCI Driver 8139too(in 8139too) uses data 1/23 times for 
indexes
[   39.455602] PCI Driver r8169(in r8169) uses data 3/10 times for 
indexes
[   39.508688] PCI Driver tmspci(in tmspci) uses data 3/4 times for 
indexes
[   39.520548] PCI Driver fst(in farsync) uses data 7/7 times for 
indexes
[   39.530531] PCI Driver pc300(in pc300) uses data 6/6 times for flags
[   39.559427] PCI Driver com20020(in com20020_pci) uses data 16/24 
times for indexes
[   39.832122] PCI Driver iwl3945(in iwl3945) uses data 6/6 times for 
pointers
[   39.839834] PCI Driver iwl4965(in iwl4965) uses data 5/5 times for 
pointers
[   39.845806] PCI Driver rt2400pci(in rt2400pci) uses data 1/1 times 
for pointers
[   39.851029] PCI Driver rt2500pci(in rt2500pci) uses data 1/1 times 
for pointers
[   39.856159] PCI Driver rt61pci(in rt61pci) uses data 3/3 times for 
pointers
[   39.877628] PCI Driver ath5k_pci(in ath5k) uses data 17/19 times for 
indexes
[   39.942037] PCI Driver dmfe(in dmfe) uses data 4/4 times for flags
[   39.949311] PCI Driver winbond-840(in winbond_840) uses data 2/3 
times for indexes
[   39.955770] PCI Driver de2104x(in de2104x) uses data 1/2 times for 
indexes
[   39.961608] PCI Driver tulip(in tulip) uses data 35/35 times for 
indexes
[   39.967483] PCI Driver de4x5(in de4x5) uses data 3/4 times for 
indexes
[   39.974029] PCI Driver uli526x(in uli526x) uses data 2/2 times for 
flags
[   40.101590] PCI Driver via-ircc(in via_ircc) uses data 4/5 times for 
indexes
[   40.150602] PCI Driver sfc(in sfc) uses data 2/2 times for pointers
[   40.273109] PCI Driver saa7134(in saa7134) uses data 169/173 times 
for flags
[   40.385014] PCI Driver Multimedia eXtension Board(in saa7146) uses 
data 1/1 times for pointers
[   40.392699] PCI Driver hexium HV-PCI6 Orion(in saa7146) uses data 
3/3 times for pointers
[   40.399454] PCI Driver hexium gemini(in saa7146) uses data 2/2 times 
for pointers
[   40.405585] PCI Driver dpc7146 demonstration board(in saa7146) uses 
data 1/1 times for pointers
[   40.694876] PCI Driver budget dvb(in saa7146) uses data 9/9 times 
for pointers
[   40.701401] PCI Driver budget_av(in saa7146) uses data 25/25 times 
for pointers
[   40.707745] PCI Driver budget_ci dvb(in saa7146) uses data 7/7 times 
for pointers
[   40.715095] PCI Driver budget_patch dvb(in saa7146) uses data 1/1 
times for pointers
[   40.722309] PCI Driver dvb(in saa7146) uses data 11/11 times for 
pointers
[   40.757291] PCI Driver bt878(in bt878) uses data 12/12 times for 
pointers
[   40.922318] PCI Driver fore_200e(in fore_200e) uses data 1/1 times 
for pointers
[   40.928212] PCI Driver eni(in eni) uses data 1/2 times for indexes
[   41.017464] PCI Driver AEC62xx_IDE(in <NULL>) uses data 4/5 times 
for indexes
[   41.023069] PCI Driver ALI15x3_IDE(in <NULL>) uses data 1/2 times 
for indexes
[   41.028564] PCI Driver AMD_IDE(in <NULL>) uses data 22/23 times for 
indexes
[   41.038241] PCI Driver CMD64x_IDE(in <NULL>) uses data 3/4 times for 
indexes
[   41.043407] PCI Driver Cyrix_IDE(in <NULL>) uses data 1/2 times for 
indexes
[   41.057769] PCI Driver HPT366_IDE(in <NULL>) uses data 5/6 times for 
indexes
[   41.084702] PCI Driver Promise_Old_IDE(in <NULL>) uses data 4/5 
times for indexes
[   41.086387] PCI Driver Promise_IDE(in <NULL>) uses data 6/7 times 
for indexes
[   41.086833] PCI Driver PIIX_IDE(in <NULL>) uses data 24/25 times for 
indexes
[   41.087269] PCI Driver Serverworks_IDE(in <NULL>) uses data 4/5 
times for indexes
[   41.087687] PCI Driver SiI_IDE(in <NULL>) uses data 2/3 times for 
indexes
[   41.090972] PCI Driver VIA_IDE(in <NULL>) uses data 2/5 times for 
indexes
[   41.091405] PCI Driver PCI_IDE(in <NULL>) uses data 14/15 times for 
indexes
[   41.296455] PCI Driver aacraid(in aacraid) uses data 63/64 times for 
indexes
[   41.314088] PCI Driver aic94xx(in aic94xx) uses data 9/9 times for 
indexes
[   41.333209] PCI Driver qla1280(in qla1280) uses data 5/6 times for 
indexes
[   41.370864] PCI Driver dmx3191d(in dmx3191d) uses data 1/1 times for 
flags
[   41.473648] PCI Driver ipr sets use_driver_data
[   41.474349] PCI Driver ipr(in ipr) uses data 11/23 times for indexes
[   50.137032] PCI Driver hptiop(in hptiop) uses data 13/13 times for 
pointers
[   50.137434] PCI Driver stex(in stex) uses data 7/11 times for indexes
[   50.137794] PCI Driver mvsas(in mvsas) uses data 4/5 times for 
indexes
[   50.821457] PCI Driver ahci(in ahci) uses data 24/115 times for 
indexes
[   50.821921] PCI Driver sata_svw(in sata_svw) uses data 3/7 times for 
indexes
[   50.822295] PCI Driver ata_piix(in ata_piix) uses data 46/47 times 
for indexes
[   50.822788] PCI Driver sata_promise(in sata_promise) uses data 13/17 
times for indexes
[   50.823838] PCI Driver sata_sil(in sata_sil) uses data 4/7 times for 
indexes
[   50.824261] PCI Driver sata_sil24(in sata_sil24) uses data 4/6 times 
for indexes
[   50.824675] PCI Driver sata_via(in sata_via) uses data 1/7 times for 
indexes
[   50.826219] PCI Driver sata_nv(in sata_nv) uses data 11/14 times for 
indexes
[   50.826611] PCI Driver sata_uli(in sata_uli) uses data 2/3 times for 
indexes
[   50.826979] PCI Driver sata_mv(in sata_mv) uses data 14/16 times for 
indexes
[   50.828821] PCI Driver pata_amd(in pata_amd) uses data 19/20 times 
for indexes
[   50.829188] PCI Driver pata_artop(in pata_artop) uses data 4/5 times 
for indexes
[   50.830278] PCI Driver pata_cmd64x(in pata_cmd64x) uses data 3/4 
times for flags
[   50.836264] PCI Driver pata_opti(in pata_opti) uses data 1/2 times 
for indexes
[   50.838717] PCI Driver pata_pdc2027x(in pata_pdc2027x) uses data 5/7 
times for indexes
[   50.839105] PCI Driver pata_pdc202xx_old(in pata_pdc202xx_old) uses 
data 4/5 times for indexes
[   50.840546] PCI Driver pata_serverworks(in pata_serverworks) uses 
data 4/5 times for indexes
[   50.843885] PCI Driver ata_generic(in ata_generic) uses data 1/13 
times for indexes
[   50.137032] PCI Driver hptiop(in hptiop) uses data 13/13 times for 
pointers
[   50.137434] PCI Driver stex(in stex) uses data 7/11 times for indexes
[   50.137794] PCI Driver mvsas(in mvsas) uses data 4/5 times for 
indexes
[   50.821457] PCI Driver ahci(in ahci) uses data 24/115 times for 
indexes
[   50.821921] PCI Driver sata_svw(in sata_svw) uses data 3/7 times for 
indexes
[   50.822295] PCI Driver ata_piix(in ata_piix) uses data 46/47 times 
for indexes
[   50.822788] PCI Driver sata_promise(in sata_promise) uses data 13/17 
times for indexes
[   50.823838] PCI Driver sata_sil(in sata_sil) uses data 4/7 times for 
indexes
[   50.824261] PCI Driver sata_sil24(in sata_sil24) uses data 4/6 times 
for indexes
[   50.824675] PCI Driver sata_via(in sata_via) uses data 1/7 times for 
indexes
[   50.826219] PCI Driver sata_nv(in sata_nv) uses data 11/14 times for 
indexes
[   50.826611] PCI Driver sata_uli(in sata_uli) uses data 2/3 times for 
indexes
[   50.826979] PCI Driver sata_mv(in sata_mv) uses data 14/16 times for 
indexes
[   50.828821] PCI Driver pata_amd(in pata_amd) uses data 19/20 times 
for indexes
[   50.829188] PCI Driver pata_artop(in pata_artop) uses data 4/5 times 
for indexes
[   50.830278] PCI Driver pata_cmd64x(in pata_cmd64x) uses data 3/4 
times for flags
[   50.836264] PCI Driver pata_opti(in pata_opti) uses data 1/2 times 
for indexes
[   50.838717] PCI Driver pata_pdc2027x(in pata_pdc2027x) uses data 5/7 
times for indexes
[   50.839105] PCI Driver pata_pdc202xx_old(in pata_pdc202xx_old) uses 
data 4/5 times for indexes
[   50.840546] PCI Driver pata_serverworks(in pata_serverworks) uses 
data 4/5 times for indexes
[   50.843885] PCI Driver ata_generic(in ata_generic) uses data 1/13 
times for indexes
[   52.594478] PCI Driver MTD PCI(in pci) uses data 2/2 times for 
pointers
[   52.784303] PCI Driver yenta_cardbus(in yenta_socket) uses data 
47/49 times for pointers
[   54.779825] PCI Driver ehci_hcd(in ehci_hcd) uses data 1/1 times for 
pointers
[   55.043495] PCI Driver ohci_hcd(in ohci_hcd) uses data 1/1 times for 
pointers
[   55.616852] PCI Driver uhci_hcd(in uhci_hcd) uses data 1/1 times for 
pointers
[   57.372871] PCI Driver i2c_amd756 sets use_driver_data
[   57.372874] PCI Driver amd756_smbus(in i2c_amd756) uses data 4/5 
times for indexes
[   57.375964] PCI Driver i2c_viapro sets use_driver_data
[   57.375968] PCI Driver vt596_smbus(in i2c_viapro) uses data 12/12 
times for flags
[   58.609660] PCI Driver c4(in c4) uses data 2/2 times for flags
[   58.618070] PCI Driver divas(in divas) uses data 11/11 times for 
flags
[   58.660828] PCI Driver hfc4s8s_l1(in hfc4s8s_l1) uses data 4/4 times 
for pointers
[   58.667738] PCI Driver fcpci(in hisax_fcpcipnp) uses data 2/2 times 
for pointers
[   58.725420] PCI Driver sdhci-pci(in sdhci_pci) uses data 7/8 times 
for pointers
[   58.802705] PCI Driver ib_mthca(in ib_mthca) uses data 8/10 times 
for indexes
[   58.908537] PCI Driver trident(in trident) uses data 4/5 times for 
indexes
[   59.167098] PCI Driver ALS300(in snd_als300) uses data 1/2 times for 
indexes
[   59.193937] PCI Driver Bt87x(in snd_bt87x) uses data 10/10 times for 
indexes
[   59.229590] PCI Driver ES1968 (ESS Maestro)(in snd_es1968) uses data 
2/3 times for indexes
[   59.242452] PCI Driver Intel ICH(in snd_intel8x0) uses data 16/23 
times for indexes
[   59.249023] PCI Driver Intel ICH Modem(in snd_intel8x0m) uses data 
5/15 times for indexes
[   59.275975] PCI Driver VIA 82xx Audio(in snd_via82xx) uses data 2/2 
times for indexes
[   59.282351] PCI Driver VIA 82xx Modem(in snd_via82xx_modem) uses 
data 1/1 times for indexes
[   59.293970] PCI Driver au8810(in snd_au8810) uses data 1/1 times for 
indexes
[   59.399694] PCI Driver EMU10K1_Audigy(in snd_emu10k1) uses data 2/3 
times for indexes
[   59.411858] PCI Driver HDA Intel(in snd_hda_intel) uses data 42/51 
times for indexes
[   59.448649] PCI Driver CMI8788(in snd_oxygen) uses data 1/10 times 
for indexes
[   59.454958] PCI Driver AV200(in snd_virtuoso) uses data 2/3 times 
for indexes
[   59.460819] PCI Driver Digigram pcxhr(in snd_pcxhr) uses data 5/6 
times for indexes
[   59.507144] PCI Driver Digigram VX222(in snd_vx222) uses data 1/2 
times for indexes


diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index a13f534..0aeac3a 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -696,6 +696,32 @@ int __pci_register_driver(struct pci_driver *drv, 
struct module *owner,
	spin_lock_init(&drv->dynids.lock);
	INIT_LIST_HEAD(&drv->dynids.list);
+
+	{
+		const struct pci_device_id *ids = drv->id_table;
+		unsigned long max_dat = 0;
+		int count = 0, addr = 0, uses = 0;
+
+		while (ids->vendor || ids->subvendor || ids->class_mask) {
+			count++;
+			if (ids->driver_data)
+				uses++;
+
+			if (ids->driver_data >= PAGE_OFFSET)
+				addr++;
+			else
+				max_dat = max(max_dat, ids->driver_data);
+			ids++;
+		}
+		if (drv->dynids.use_driver_data)
+			pr_info("PCI Driver %s sets use_driver_data\n", mod_name);
+		if (uses)
+			pr_info("PCI Driver %s(in %s) uses data %d/%d times for %s\n",
+				drv->name, mod_name, uses, count,
+				addr ? "pointers" :
+				(max_dat > count) ? "flags" : "indexes");
+	}
+
	/* register with core */
	error = driver_register(&drv->driver);
	if (error)

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Milton Miller]

Greg,

Please respond to this email and explain why the patch

pci: dynids.use_driver_data considered harmful

http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188

should not be applied.   I am not arguing the correctness of
the removed code, rather its utility and benefit to the linux
community.

As far as I can tell, the code only succeeds in limiting the
usefulness of the pci dynamic id addition mechanism.  We chose
not to limit which drivers can have a table entry added, now
let us not limit telling the driver which previous entry is
most similar to our new entry.

If a driver doesn't set this bit, and only 3 out of 419 do,
then the facility can only be used if the driver can function
correctly with the data zero.  In some drivers (radeonfb) a
nonzero flag is always set, in some a list of boards or
chipsets is listed in an arbitrary order (e1000e), and in yet
others the field is used as a pointer without checking for NULL
(DAC960, iwl3945).



You sent your request for others to withdraw the patch
from consideration when I resent the patch without seeing your
comments that were less than 12 hours old, but have been silent
for the last 60 hours since I responded to them over the next
several hours.   If I do not hear from you with technical
arguments for keeping the code, I will resend the patch for
consideration.

milton

On Jul 12, 2008, at 5:48 PM, Milton Miller wrote:
> On Jul 12, 2008, at 4:08 PM, Milton Miller wrote:
>> You left out:
>>>> Searching the next-20080709 tree shows the bit is set by exactly 3 
>>>> pci
>>>> drivers.  However, the use of per-match-entry driver data is much 
>>>> more
>>>> prevalent: A boot of allyesconfig on a powerpc64 pseries with a 
>>>> debug patch
>>>> shows 27 drivers apparently use the field for a pointer, 14 use it 
>>>> for
>>>> setting flags, and 98 use it as a table index.  (Pointers are 
>>>> defined as
>>>> >PAGE_OFFSET, aka in the 64 bit kernel linear mapping.  Flags are 
>>>> defined as
>>>> the maximum value exceeds the number of entries in the match table. 
>>>>  Any
>>>> other nonzero value is classified as an index).
>>
>> I did the work to establish the scope of the problem.
>>
>
> For the record or independent investigation, here is the patch and 
> grep of
> output from the kernel log.   Hopefully you can read them after this 
> mailer
> mangles them.
>
> milton
>
> [   30.412634] PCI Driver CyberPro(in cyber2000fb) uses data 3/3 times 
> for indexes
> [   30.418967] PCI Driver aty128fb(in aty128fb) uses data 37/47 times 
> for indexes
> [   30.419787] PCI Driver radeonfb(in radeonfb) uses data 98/98 times 
> for flags
> [   30.421104] PCI Driver sisfb(in sisfb) uses data 11/12 times for 
> indexes
> [   30.422332] PCI Driver savagefb(in savagefb) uses data 23/23 times 
> for flags
> [   30.423088] PCI Driver neofb(in neofb) uses data 9/9 times for flags
> [   30.424431] PCI Driver imsttfb(in imsttfb) uses data 1/2 times for 
> indexes
> [   30.427089] PCI Driver s3fb(in s3fb) uses data 12/12 times for flags
> [   30.428495] PCI Driver sstfb(in sstfb) uses data 1/2 times for 
> indexes
> [   30.429439] PCI Driver cirrusfb(in cirrusfb) uses data 11/11 times 
> for indexes
> [   30.431251] PCI Driver gxt4500(in gxt4500) uses data 1/2 times for 
> indexes
> [   33.082152] PCI Driver epca(in epca) uses data 3/4 times for indexes
> [   37.857569] PCI Driver serial(in 8250_pci) uses data 177/180 times 
> for indexes
> [   37.868296] PCI Driver jsm(in jsm) uses data 4/5 times for indexes
> [   37.901045] PCI Driver parport_pc(in parport_pc) uses data 52/53 
> times for indexes
> [   37.906019] PCI Driver parport_serial(in parport_serial) uses data 
> 30/31 times for indexes
> [   38.253736] PCI Driver DAC960(in DAC960) uses data 7/7 times for 
> pointers
> [   38.983951] PCI Driver e1000e(in e1000e) uses data 29/38 times for 
> indexes
> [   39.026014] PCI Driver Sundance Technology IPG Triple-Speed 
> Ethernet(in ipg) uses data 5/6 times for indexes
> [   39.033218] PCI Driver cxgb(in cxgb) uses data 5/7 times for indexes
> [   39.039465] PCI Driver cxgb3(in cxgb3) uses data 9/10 times for 
> indexes
> [   39.092305] PCI Driver 3c59x(in 3c59x) uses data 37/38 times for 
> indexes
> [   39.098029] PCI Driver typhoon(in typhoon) uses data 12/13 times 
> for indexes
> [   39.103976] PCI Driver ne2k-pci(in ne2k_pci) uses data 10/11 times 
> for indexes
> [   39.122763] PCI Driver e100(in e100) uses data 36/41 times for 
> indexes
> [   39.129041] PCI Driver tlan(in tlan) uses data 12/13 times for 
> indexes
> [   39.141155] PCI Driver epic100(in epic100) uses data 2/3 times for 
> indexes
> [   39.146232] PCI Driver sis190(in sis190) uses data 1/2 times for 
> indexes
> [   39.151792] PCI Driver sis900(in sis900) uses data 1/2 times for 
> indexes
> [   39.161699] PCI Driver yellowfin(in yellowfin) uses data 1/2 times 
> for indexes
> [   39.173144] PCI Driver natsemi(in natsemi) uses data 1/2 times for 
> indexes
> [   39.184923] PCI Driver fealnx(in fealnx) uses data 2/3 times for 
> indexes
> [   39.196097] PCI Driver bnx2(in bnx2) uses data 8/9 times for indexes
> [   39.201880] PCI Driver bnx2x(in bnx2x) uses data 2/3 times for 
> indexes
> [   39.347752] PCI Driver sundance(in sundance) uses data 6/7 times 
> for indexes
> [   39.372095] PCI Driver forcedeth(in forcedeth) uses data 39/39 
> times for flags
> [   39.426772] PCI Driver 8139too(in 8139too) uses data 1/23 times for 
> indexes
> [   39.455602] PCI Driver r8169(in r8169) uses data 3/10 times for 
> indexes
> [   39.508688] PCI Driver tmspci(in tmspci) uses data 3/4 times for 
> indexes
> [   39.520548] PCI Driver fst(in farsync) uses data 7/7 times for 
> indexes
> [   39.530531] PCI Driver pc300(in pc300) uses data 6/6 times for flags
> [   39.559427] PCI Driver com20020(in com20020_pci) uses data 16/24 
> times for indexes
> [   39.832122] PCI Driver iwl3945(in iwl3945) uses data 6/6 times for 
> pointers
> [   39.839834] PCI Driver iwl4965(in iwl4965) uses data 5/5 times for 
> pointers
> [   39.845806] PCI Driver rt2400pci(in rt2400pci) uses data 1/1 times 
> for pointers
> [   39.851029] PCI Driver rt2500pci(in rt2500pci) uses data 1/1 times 
> for pointers
> [   39.856159] PCI Driver rt61pci(in rt61pci) uses data 3/3 times for 
> pointers
> [   39.877628] PCI Driver ath5k_pci(in ath5k) uses data 17/19 times 
> for indexes
> [   39.942037] PCI Driver dmfe(in dmfe) uses data 4/4 times for flags
> [   39.949311] PCI Driver winbond-840(in winbond_840) uses data 2/3 
> times for indexes
> [   39.955770] PCI Driver de2104x(in de2104x) uses data 1/2 times for 
> indexes
> [   39.961608] PCI Driver tulip(in tulip) uses data 35/35 times for 
> indexes
> [   39.967483] PCI Driver de4x5(in de4x5) uses data 3/4 times for 
> indexes
> [   39.974029] PCI Driver uli526x(in uli526x) uses data 2/2 times for 
> flags
> [   40.101590] PCI Driver via-ircc(in via_ircc) uses data 4/5 times 
> for indexes
> [   40.150602] PCI Driver sfc(in sfc) uses data 2/2 times for pointers
> [   40.273109] PCI Driver saa7134(in saa7134) uses data 169/173 times 
> for flags
> [   40.385014] PCI Driver Multimedia eXtension Board(in saa7146) uses 
> data 1/1 times for pointers
> [   40.392699] PCI Driver hexium HV-PCI6 Orion(in saa7146) uses data 
> 3/3 times for pointers
> [   40.399454] PCI Driver hexium gemini(in saa7146) uses data 2/2 
> times for pointers
> [   40.405585] PCI Driver dpc7146 demonstration board(in saa7146) uses 
> data 1/1 times for pointers
> [   40.694876] PCI Driver budget dvb(in saa7146) uses data 9/9 times 
> for pointers
> [   40.701401] PCI Driver budget_av(in saa7146) uses data 25/25 times 
> for pointers
> [   40.707745] PCI Driver budget_ci dvb(in saa7146) uses data 7/7 
> times for pointers
> [   40.715095] PCI Driver budget_patch dvb(in saa7146) uses data 1/1 
> times for pointers
> [   40.722309] PCI Driver dvb(in saa7146) uses data 11/11 times for 
> pointers
> [   40.757291] PCI Driver bt878(in bt878) uses data 12/12 times for 
> pointers
> [   40.922318] PCI Driver fore_200e(in fore_200e) uses data 1/1 times 
> for pointers
> [   40.928212] PCI Driver eni(in eni) uses data 1/2 times for indexes
> [   41.017464] PCI Driver AEC62xx_IDE(in <NULL>) uses data 4/5 times 
> for indexes
> [   41.023069] PCI Driver ALI15x3_IDE(in <NULL>) uses data 1/2 times 
> for indexes
> [   41.028564] PCI Driver AMD_IDE(in <NULL>) uses data 22/23 times for 
> indexes
> [   41.038241] PCI Driver CMD64x_IDE(in <NULL>) uses data 3/4 times 
> for indexes
> [   41.043407] PCI Driver Cyrix_IDE(in <NULL>) uses data 1/2 times for 
> indexes
> [   41.057769] PCI Driver HPT366_IDE(in <NULL>) uses data 5/6 times 
> for indexes
> [   41.084702] PCI Driver Promise_Old_IDE(in <NULL>) uses data 4/5 
> times for indexes
> [   41.086387] PCI Driver Promise_IDE(in <NULL>) uses data 6/7 times 
> for indexes
> [   41.086833] PCI Driver PIIX_IDE(in <NULL>) uses data 24/25 times 
> for indexes
> [   41.087269] PCI Driver Serverworks_IDE(in <NULL>) uses data 4/5 
> times for indexes
> [   41.087687] PCI Driver SiI_IDE(in <NULL>) uses data 2/3 times for 
> indexes
> [   41.090972] PCI Driver VIA_IDE(in <NULL>) uses data 2/5 times for 
> indexes
> [   41.091405] PCI Driver PCI_IDE(in <NULL>) uses data 14/15 times for 
> indexes
> [   41.296455] PCI Driver aacraid(in aacraid) uses data 63/64 times 
> for indexes
> [   41.314088] PCI Driver aic94xx(in aic94xx) uses data 9/9 times for 
> indexes
> [   41.333209] PCI Driver qla1280(in qla1280) uses data 5/6 times for 
> indexes
> [   41.370864] PCI Driver dmx3191d(in dmx3191d) uses data 1/1 times 
> for flags
> [   41.473648] PCI Driver ipr sets use_driver_data
> [   41.474349] PCI Driver ipr(in ipr) uses data 11/23 times for indexes
> [   50.137032] PCI Driver hptiop(in hptiop) uses data 13/13 times for 
> pointers
> [   50.137434] PCI Driver stex(in stex) uses data 7/11 times for 
> indexes
> [   50.137794] PCI Driver mvsas(in mvsas) uses data 4/5 times for 
> indexes
> [   50.821457] PCI Driver ahci(in ahci) uses data 24/115 times for 
> indexes
> [   50.821921] PCI Driver sata_svw(in sata_svw) uses data 3/7 times 
> for indexes
> [   50.822295] PCI Driver ata_piix(in ata_piix) uses data 46/47 times 
> for indexes
> [   50.822788] PCI Driver sata_promise(in sata_promise) uses data 
> 13/17 times for indexes
> [   50.823838] PCI Driver sata_sil(in sata_sil) uses data 4/7 times 
> for indexes
> [   50.824261] PCI Driver sata_sil24(in sata_sil24) uses data 4/6 
> times for indexes
> [   50.824675] PCI Driver sata_via(in sata_via) uses data 1/7 times 
> for indexes
> [   50.826219] PCI Driver sata_nv(in sata_nv) uses data 11/14 times 
> for indexes
> [   50.826611] PCI Driver sata_uli(in sata_uli) uses data 2/3 times 
> for indexes
> [   50.826979] PCI Driver sata_mv(in sata_mv) uses data 14/16 times 
> for indexes
> [   50.828821] PCI Driver pata_amd(in pata_amd) uses data 19/20 times 
> for indexes
> [   50.829188] PCI Driver pata_artop(in pata_artop) uses data 4/5 
> times for indexes
> [   50.830278] PCI Driver pata_cmd64x(in pata_cmd64x) uses data 3/4 
> times for flags
> [   50.836264] PCI Driver pata_opti(in pata_opti) uses data 1/2 times 
> for indexes
> [   50.838717] PCI Driver pata_pdc2027x(in pata_pdc2027x) uses data 
> 5/7 times for indexes
> [   50.839105] PCI Driver pata_pdc202xx_old(in pata_pdc202xx_old) uses 
> data 4/5 times for indexes
> [   50.840546] PCI Driver pata_serverworks(in pata_serverworks) uses 
> data 4/5 times for indexes
> [   50.843885] PCI Driver ata_generic(in ata_generic) uses data 1/13 
> times for indexes
> [   50.137032] PCI Driver hptiop(in hptiop) uses data 13/13 times for 
> pointers
> [   50.137434] PCI Driver stex(in stex) uses data 7/11 times for 
> indexes
> [   50.137794] PCI Driver mvsas(in mvsas) uses data 4/5 times for 
> indexes
> [   50.821457] PCI Driver ahci(in ahci) uses data 24/115 times for 
> indexes
> [   50.821921] PCI Driver sata_svw(in sata_svw) uses data 3/7 times 
> for indexes
> [   50.822295] PCI Driver ata_piix(in ata_piix) uses data 46/47 times 
> for indexes
> [   50.822788] PCI Driver sata_promise(in sata_promise) uses data 
> 13/17 times for indexes
> [   50.823838] PCI Driver sata_sil(in sata_sil) uses data 4/7 times 
> for indexes
> [   50.824261] PCI Driver sata_sil24(in sata_sil24) uses data 4/6 
> times for indexes
> [   50.824675] PCI Driver sata_via(in sata_via) uses data 1/7 times 
> for indexes
> [   50.826219] PCI Driver sata_nv(in sata_nv) uses data 11/14 times 
> for indexes
> [   50.826611] PCI Driver sata_uli(in sata_uli) uses data 2/3 times 
> for indexes
> [   50.826979] PCI Driver sata_mv(in sata_mv) uses data 14/16 times 
> for indexes
> [   50.828821] PCI Driver pata_amd(in pata_amd) uses data 19/20 times 
> for indexes
> [   50.829188] PCI Driver pata_artop(in pata_artop) uses data 4/5 
> times for indexes
> [   50.830278] PCI Driver pata_cmd64x(in pata_cmd64x) uses data 3/4 
> times for flags
> [   50.836264] PCI Driver pata_opti(in pata_opti) uses data 1/2 times 
> for indexes
> [   50.838717] PCI Driver pata_pdc2027x(in pata_pdc2027x) uses data 
> 5/7 times for indexes
> [   50.839105] PCI Driver pata_pdc202xx_old(in pata_pdc202xx_old) uses 
> data 4/5 times for indexes
> [   50.840546] PCI Driver pata_serverworks(in pata_serverworks) uses 
> data 4/5 times for indexes
> [   50.843885] PCI Driver ata_generic(in ata_generic) uses data 1/13 
> times for indexes
> [   52.594478] PCI Driver MTD PCI(in pci) uses data 2/2 times for 
> pointers
> [   52.784303] PCI Driver yenta_cardbus(in yenta_socket) uses data 
> 47/49 times for pointers
> [   54.779825] PCI Driver ehci_hcd(in ehci_hcd) uses data 1/1 times 
> for pointers
> [   55.043495] PCI Driver ohci_hcd(in ohci_hcd) uses data 1/1 times 
> for pointers
> [   55.616852] PCI Driver uhci_hcd(in uhci_hcd) uses data 1/1 times 
> for pointers
> [   57.372871] PCI Driver i2c_amd756 sets use_driver_data
> [   57.372874] PCI Driver amd756_smbus(in i2c_amd756) uses data 4/5 
> times for indexes
> [   57.375964] PCI Driver i2c_viapro sets use_driver_data
> [   57.375968] PCI Driver vt596_smbus(in i2c_viapro) uses data 12/12 
> times for flags
> [   58.609660] PCI Driver c4(in c4) uses data 2/2 times for flags
> [   58.618070] PCI Driver divas(in divas) uses data 11/11 times for 
> flags
> [   58.660828] PCI Driver hfc4s8s_l1(in hfc4s8s_l1) uses data 4/4 
> times for pointers
> [   58.667738] PCI Driver fcpci(in hisax_fcpcipnp) uses data 2/2 times 
> for pointers
> [   58.725420] PCI Driver sdhci-pci(in sdhci_pci) uses data 7/8 times 
> for pointers
> [   58.802705] PCI Driver ib_mthca(in ib_mthca) uses data 8/10 times 
> for indexes
> [   58.908537] PCI Driver trident(in trident) uses data 4/5 times for 
> indexes
> [   59.167098] PCI Driver ALS300(in snd_als300) uses data 1/2 times 
> for indexes
> [   59.193937] PCI Driver Bt87x(in snd_bt87x) uses data 10/10 times 
> for indexes
> [   59.229590] PCI Driver ES1968 (ESS Maestro)(in snd_es1968) uses 
> data 2/3 times for indexes
> [   59.242452] PCI Driver Intel ICH(in snd_intel8x0) uses data 16/23 
> times for indexes
> [   59.249023] PCI Driver Intel ICH Modem(in snd_intel8x0m) uses data 
> 5/15 times for indexes
> [   59.275975] PCI Driver VIA 82xx Audio(in snd_via82xx) uses data 2/2 
> times for indexes
> [   59.282351] PCI Driver VIA 82xx Modem(in snd_via82xx_modem) uses 
> data 1/1 times for indexes
> [   59.293970] PCI Driver au8810(in snd_au8810) uses data 1/1 times 
> for indexes
> [   59.399694] PCI Driver EMU10K1_Audigy(in snd_emu10k1) uses data 2/3 
> times for indexes
> [   59.411858] PCI Driver HDA Intel(in snd_hda_intel) uses data 42/51 
> times for indexes
> [   59.448649] PCI Driver CMI8788(in snd_oxygen) uses data 1/10 times 
> for indexes
> [   59.454958] PCI Driver AV200(in snd_virtuoso) uses data 2/3 times 
> for indexes
> [   59.460819] PCI Driver Digigram pcxhr(in snd_pcxhr) uses data 5/6 
> times for indexes
> [   59.507144] PCI Driver Digigram VX222(in snd_vx222) uses data 1/2 
> times for indexes
>
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index a13f534..0aeac3a 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -696,6 +696,32 @@ int __pci_register_driver(struct pci_driver *drv, 
> struct module *owner,
> 	spin_lock_init(&drv->dynids.lock);
> 	INIT_LIST_HEAD(&drv->dynids.list);
> +
> +	{
> +		const struct pci_device_id *ids = drv->id_table;
> +		unsigned long max_dat = 0;
> +		int count = 0, addr = 0, uses = 0;
> +
> +		while (ids->vendor || ids->subvendor || ids->class_mask) {
> +			count++;
> +			if (ids->driver_data)
> +				uses++;
> +
> +			if (ids->driver_data >= PAGE_OFFSET)
> +				addr++;
> +			else
> +				max_dat = max(max_dat, ids->driver_data);
> +			ids++;
> +		}
> +		if (drv->dynids.use_driver_data)
> +			pr_info("PCI Driver %s sets use_driver_data\n", mod_name);
> +		if (uses)
> +			pr_info("PCI Driver %s(in %s) uses data %d/%d times for %s\n",
> +				drv->name, mod_name, uses, count,
> +				addr ? "pointers" :
> +				(max_dat > count) ? "flags" : "indexes");
> +	}
> +
> 	/* register with core */
> 	error = driver_register(&drv->driver);
> 	if (error)
>

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Greg KH]

On Wed, Jul 16, 2008 at 05:18:18AM -0500, Milton Miller wrote:
>
> Greg,
>
> Please respond to this email and explain why the patch
>
> pci: dynids.use_driver_data considered harmful
>
> http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
>
> should not be applied.   I am not arguing the correctness of
> the removed code, rather its utility and benefit to the linux
> community.
>
> As far as I can tell, the code only succeeds in limiting the
> usefulness of the pci dynamic id addition mechanism.  We chose
> not to limit which drivers can have a table entry added, now
> let us not limit telling the driver which previous entry is
> most similar to our new entry.
>
> If a driver doesn't set this bit, and only 3 out of 419 do,
> then the facility can only be used if the driver can function
> correctly with the data zero.  In some drivers (radeonfb) a
> nonzero flag is always set, in some a list of boards or
> chipsets is listed in an arbitrary order (e1000e), and in yet
> others the field is used as a pointer without checking for NULL
> (DAC960, iwl3945).
>
>
>
> You sent your request for others to withdraw the patch
> from consideration when I resent the patch without seeing your
> comments that were less than 12 hours old, but have been silent
> for the last 60 hours since I responded to them over the next
> several hours.   If I do not hear from you with technical
> arguments for keeping the code, I will resend the patch for
> consideration.

Sorry, I'm on the road right now and will not get back until Friday.
Then I have the big merges with Linus to get through.  I'll try to get
to this by Monday, but my original point still stands, this was
implemented for a reason, saying that not enough drivers use it properly
does not make the need for it to go away.  It is required for them, so
perhaps the other 419 drivers also need to have the flag set.  That's
pretty trivial to do, right?

thanks,

greg k-h

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Milton Miller]

On Jul 17, 2008, at 2:07 AM, Greg KH wrote:

> On Wed, Jul 16, 2008 at 05:18:18AM -0500, Milton Miller wrote:
>>
>> Greg,
>>
>> Please respond to this email and explain why the patch
>>
>> pci: dynids.use_driver_data considered harmful
>>
>> http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
>>
>> should not be applied.   I am not arguing the correctness of
>> the removed code, rather its utility and benefit to the linux
>> community.
>>
>> As far as I can tell, the code only succeeds in limiting the
>> usefulness of the pci dynamic id addition mechanism.
...
>
> Sorry, I'm on the road right now and will not get back until Friday.
> Then I have the big merges with Linus to get through.  I'll try to get
> to this by Monday, but my original point still stands, this was
> implemented for a reason, saying that not enough drivers use it 
> properly
> does not make the need for it to go away.  It is required for them, so
> perhaps the other 419 drivers also need to have the flag set.  That's
> pretty trivial to do, right?
>

Fine, I'll give you a little time.  But I want to hear specifics how it
helps drivers.   I have shown how it hurts many drivers.   My arguement
is that once we set the flag on the drivers, we will end up with it set
on all drivers or the remaining drivers will not care.   There were 413+
drivers in linux-next that were compiled by allyesconfig, about 150 used
driver data.

If the purpose is to enforce range checking, then I'll start working on
patches for those 100 drivers to do that.   But I don't see why a binary
flag helps any driver.   The flag is not "I will accept a additional id
table", its "I will accept the additional driver data that I need to
operate" on my dynamically added id table.

milton

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Jean Delvare]

Hi Greg,

On Thu, 17 Jul 2008 00:07:59 -0700, Greg KH wrote:
> On Wed, Jul 16, 2008 at 05:18:18AM -0500, Milton Miller wrote:
> >
> > Greg,
> >
> > Please respond to this email and explain why the patch
> >
> > pci: dynids.use_driver_data considered harmful
> >
> > http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
> >
> > should not be applied.   I am not arguing the correctness of
> > the removed code, rather its utility and benefit to the linux
> > community.
>
> (...)  I'll try to get
> to this by Monday, but my original point still stands, this was
> implemented for a reason,

Not a good enough argument, sorry. There have been many cases in the
past where code has been withdrawn after some times because we
realized that we got it wrong in the first place.

So, please explain what the current code is good for. Honestly, my
initial reaction to Milton's proposal was "what an idiot, this flag is
there for an obvious safety reason and we don't want to remove it" but
after reading both his arguments and the code, I found that I have
nothing to backup my claim. If you do, please let us know your
technical reasons.

>                           saying that not enough drivers use it properly
> does not make the need for it to go away.  It is required for them, so
> perhaps the other 419 drivers also need to have the flag set.  That's
> pretty trivial to do, right?

If you are suggesting to blindly set the flag to all PCI drivers (or
even just all the ones which make use of the driver_data field -
doesn't make a difference), this simply shows how useless this flag is.
If you don't, then one would have to check the code of all drivers and
add validation code for the driver_data value; but then this no longer
falls into the "trivial" category.

Thanks,
-- 
Jean Delvare

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Greg KH]

On Wed, Aug 06, 2008 at 09:31:18AM +0200, Jean Delvare wrote:
> Hi Greg,
> 
> On Thu, 17 Jul 2008 00:07:59 -0700, Greg KH wrote:
> > On Wed, Jul 16, 2008 at 05:18:18AM -0500, Milton Miller wrote:
> > >
> > > Greg,
> > >
> > > Please respond to this email and explain why the patch
> > >
> > > pci: dynids.use_driver_data considered harmful
> > >
> > > http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
> > >
> > > should not be applied.   I am not arguing the correctness of
> > > the removed code, rather its utility and benefit to the linux
> > > community.
> >
> > (...)  I'll try to get
> > to this by Monday, but my original point still stands, this was
> > implemented for a reason,
> 
> Not a good enough argument, sorry. There have been many cases in the
> past where code has been withdrawn after some times because we
> realized that we got it wrong in the first place.

Fair enough :)

> So, please explain what the current code is good for. Honestly, my
> initial reaction to Milton's proposal was "what an idiot, this flag is
> there for an obvious safety reason and we don't want to remove it" but
> after reading both his arguments and the code, I found that I have
> nothing to backup my claim. If you do, please let us know your
> technical reasons.

The technical reason was that this flag was needed to let some drivers
work properly with the new_id file, right?

If the flag goes away, they break from what I can tell.

> >                           saying that not enough drivers use it properly
> > does not make the need for it to go away.  It is required for them, so
> > perhaps the other 419 drivers also need to have the flag set.  That's
> > pretty trivial to do, right?
> 
> If you are suggesting to blindly set the flag to all PCI drivers (or
> even just all the ones which make use of the driver_data field -
> doesn't make a difference), this simply shows how useless this flag is.
> If you don't, then one would have to check the code of all drivers and
> add validation code for the driver_data value; but then this no longer
> falls into the "trivial" category.

It's pretty "trivial" to look to see if the field is set in the pci_id
structure, that should be all that is needed, right?

thanks,

greg k-h

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Milton Miller]

On Aug 14, 2008, at 5:12 PM, Greg KH wrote:

> On Wed, Aug 06, 2008 at 09:31:18AM +0200, Jean Delvare wrote:
>> Hi Greg,
>>
>> On Thu, 17 Jul 2008 00:07:59 -0700, Greg KH wrote:
>>> On Wed, Jul 16, 2008 at 05:18:18AM -0500, Milton Miller wrote:
>>>>
>>>> Greg,
>>>>
>>>> Please respond to this email and explain why the patch
>>>>
>>>> pci: dynids.use_driver_data considered harmful
>>>>
>>>> http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
>>>>
>>>> should not be applied.   I am not arguing the correctness of
>>>> the removed code, rather its utility and benefit to the linux
>>>> community.
>>>
>>> (...)  I'll try to get
>>> to this by Monday, but my original point still stands, this was
>>> implemented for a reason,
>>
>> Not a good enough argument, sorry. There have been many cases in the
>> past where code has been withdrawn after some times because we
>> realized that we got it wrong in the first place.
>
> Fair enough :)
>
>> So, please explain what the current code is good for. Honestly, my
>> initial reaction to Milton's proposal was "what an idiot, this flag is
>> there for an obvious safety reason and we don't want to remove it" but
>> after reading both his arguments and the code, I found that I have
>> nothing to backup my claim. If you do, please let us know your
>> technical reasons.
>
> The technical reason was that this flag was needed to let some drivers
> work properly with the new_id file, right?
>
> If the flag goes away, they break from what I can tell.

That is not a detailed technical argument, its an  assumption.

>
>>>                           saying that not enough drivers use it 
>>> properly
>>> does not make the need for it to go away.  It is required for them, 
>>> so
>>> perhaps the other 419 drivers also need to have the flag set.  That's
>>> pretty trivial to do, right?
>>
>> If you are suggesting to blindly set the flag to all PCI drivers (or
>> even just all the ones which make use of the driver_data field -
>> doesn't make a difference), this simply shows how useless this flag 
>> is.
>> If you don't, then one would have to check the code of all drivers and
>> add validation code for the driver_data value; but then this no longer
>> falls into the "trivial" category.
>
> It's pretty "trivial" to look to see if the field is set in the pci_id
> structure, that should be all that is needed, right?


So please identify at least one such driver where the only correct 
answer is 0.  I even made it easy for you, i identified which drivers 
set dynamic_id and how.  I identified specific drivers where its the 
wrong answer.

So:  If you are arguing the code is still needed, please identify  at 
least one case that it is correct.  (Oh, I don't buy that if 0 is safe 
but 1 is equally safe, that the existing code is correct).

milton

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Jean Delvare]

Hi Greg,

On Thu, 14 Aug 2008 15:12:14 -0700, Greg KH wrote:
> On Wed, Aug 06, 2008 at 09:31:18AM +0200, Jean Delvare wrote:
> > Hi Greg,
> > 
> > On Thu, 17 Jul 2008 00:07:59 -0700, Greg KH wrote:
> > > On Wed, Jul 16, 2008 at 05:18:18AM -0500, Milton Miller wrote:
> > > >
> > > > Greg,
> > > >
> > > > Please respond to this email and explain why the patch
> > > >
> > > > pci: dynids.use_driver_data considered harmful
> > > >
> > > > http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
> > > >
> > > > should not be applied.   I am not arguing the correctness of
> > > > the removed code, rather its utility and benefit to the linux
> > > > community.
> > >
> > > (...)  I'll try to get
> > > to this by Monday, but my original point still stands, this was
> > > implemented for a reason,
> > 
> > Not a good enough argument, sorry. There have been many cases in the
> > past where code has been withdrawn after some times because we
> > realized that we got it wrong in the first place.
> 
> Fair enough :)
> 
> > So, please explain what the current code is good for. Honestly, my
> > initial reaction to Milton's proposal was "what an idiot, this flag is
> > there for an obvious safety reason and we don't want to remove it" but
> > after reading both his arguments and the code, I found that I have
> > nothing to backup my claim. If you do, please let us know your
> > technical reasons.
> 
> The technical reason was that this flag was needed to let some drivers
> work properly with the new_id file, right?

Hmm, so in fact you don't have a clue? ;)

I wasn't involved in the addition of this flag, but my impression is
that it was added in the hope that it would prevent users from passing
additional data to drivers using the driver_data field but not prepared
(read: not robust enough) to handle possibly wrong data from user-space.
Probably, the idea was that the person adding the
dynids.use_driver_data flag would be responsible for verifying that the
driver had the required checks in place to guarantee that "nothing"
would go wrong even if the data provided by the user wasn't correct.

(This doesn't make much sense IMHO, as there is zero guarantee that the
developer actually did that - but I believe this is the idea behind the
dynids.use_driver_data flag.)

> If the flag goes away, they break from what I can tell.

If the flag goes away, nothing will break per se. In particular, the
drivers which were using the flag won't stop working - you don't prevent
things from happening when removing a safety, on the contrary, you
allow more things to happen. What will change is that all the drivers
which _do_ use the driver_data field but didn't set the
dynids.use_driver_data flag, will now possibly receive their driver_data
from the user.

Which you may say is unsafe, because these drivers may not expect that
(i.e. they probably don't have any check to protect themselves against
bogus driver_data values.) But OTOH most of these drivers can't really
take dynamic IDs at the moment (because the code prevents the user from
passing the required driver_data, silently forcing it to 0) so this
would be a big win in terms of usability. On top of that, considering
that (silently) defaulting to 0 for driver_data is safe, is just plain
wrong. 0 might be an invalid driver_data value for some drivers.

> > >                           saying that not enough drivers use it properly
> > > does not make the need for it to go away.  It is required for them, so
> > > perhaps the other 419 drivers also need to have the flag set.  That's
> > > pretty trivial to do, right?
> > 
> > If you are suggesting to blindly set the flag to all PCI drivers (or
> > even just all the ones which make use of the driver_data field -
> > doesn't make a difference), this simply shows how useless this flag is.
> > If you don't, then one would have to check the code of all drivers and
> > add validation code for the driver_data value; but then this no longer
> > falls into the "trivial" category.
> 
> It's pretty "trivial" to look to see if the field is set in the pci_id
> structure, that should be all that is needed, right?

Well... If you consider that all drivers using the driver_data field
should have the dynids.use_driver_data flag set unconditionally and
without looking at the code, then in fact you agree with Milton and
myself that this flag should be dropped. The whole point of the flag,
if my guess is correct, was to hint the developer that he/she should
double check his/her code before setting the flag. If you set it for
all these drivers, then all it does is preventing users from passing a
driver_data value to drivers which do _not_ use the driver_data... but
doing that has zero effect at the moment, so that wouldn't actually be
any different from dropping dynids.use_driver_data altogether.

So I have to reiterate my support to Milton's idea of dropping
the dynids.use_driver_data flag. It does more harm than good.

Thanks,
-- 
Jean Delvare

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Jesse Barnes]

> > It's pretty "trivial" to look to see if the field is set in the pci_id
> > structure, that should be all that is needed, right?
>
> Well... If you consider that all drivers using the driver_data field
> should have the dynids.use_driver_data flag set unconditionally and
> without looking at the code, then in fact you agree with Milton and
> myself that this flag should be dropped. The whole point of the flag,
> if my guess is correct, was to hint the developer that he/she should
> double check his/her code before setting the flag. If you set it for
> all these drivers, then all it does is preventing users from passing a
> driver_data value to drivers which do _not_ use the driver_data... but
> doing that has zero effect at the moment, so that wouldn't actually be
> any different from dropping dynids.use_driver_data altogether.
>
> So I have to reiterate my support to Milton's idea of dropping
> the dynids.use_driver_data flag. It does more harm than good.

Yeah it doesn't seem to be very heavily used at this point. :)

I see around 400 uses of id->driver_data right now, and only 2 of 
use_driver_data.  If we remove the use_driver_data field, drivers (except for 
the 2 using the field) will start to see whatever values userspace passes to 
them rather than 0, and at least in some of the few cases I looked at that 
could cause breakage due to out of bounds references.

So I think your point about dynamic IDs in general is a good one; this flag 
really does look like an "audit was done" bit, but doesn't go as far is it 
should.

The patch you posted to forbid dynamic binding unless use_driver_data is iset 
is probably the safest thing to do, given that drivers that *don't* set 
use_driver_data look like they might misbehave even with a driver_data value 
of 0...

What do you think, Greg?  Convinced yet? :)

Thanks,
Jesse

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Jean Delvare]

Hi Jesse,

On Fri, 15 Aug 2008 10:46:59 -0700, Jesse Barnes wrote:
> So I think your point about dynamic IDs in general is a good one; this flag 
> really does look like an "audit was done" bit, but doesn't go as far is it 
> should.
> 
> The patch you posted to forbid dynamic binding unless use_driver_data is iset 
> is probably the safest thing to do, given that drivers that *don't* set 
> use_driver_data look like they might misbehave even with a driver_data value 
> of 0...

In fact we can do even better than that. We could accept from
user-space only driver_data values which at least one device ID entry in
the driver already uses. That should be fairly easy to implement, and
would offer a level of safety an order of magnitude above what we have
at the moment... And it works both ways: if 0 is not a valid data for
some driver, that would force the user to provide a non-zero (and
valid) data value. And it guarantees that the user can't ask for
something the driver doesn't expect, so drivers don't even need extra
checks. And no need for a use_driver_data flag either.

The only drawback is that it prevents the user from passing a "new"
data value even if it would be valid. But honestly, I don't expect that
case to happen frequently... if ever at all. So I'd say the benefits
totally outweight the drawback.

If the interested people agree with the idea, I'll look into
implementing it.

-- 
Jean Delvare

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Jesse Barnes]

On Friday, August 15, 2008 11:55 am Jean Delvare wrote:
> In fact we can do even better than that. We could accept from
> user-space only driver_data values which at least one device ID entry in
> the driver already uses. That should be fairly easy to implement, and
> would offer a level of safety an order of magnitude above what we have
> at the moment... And it works both ways: if 0 is not a valid data for
> some driver, that would force the user to provide a non-zero (and
> valid) data value. And it guarantees that the user can't ask for
> something the driver doesn't expect, so drivers don't even need extra
> checks. And no need for a use_driver_data flag either.

Meaning a driver audit of the usage?  Yeah that would be great.

> The only drawback is that it prevents the user from passing a "new"
> data value even if it would be valid. But honestly, I don't expect that
> case to happen frequently... if ever at all. So I'd say the benefits
> totally outweight the drawback.
>
> If the interested people agree with the idea, I'll look into
> implementing it.

Well the audit would show if user supplied "new" values are needed; otherwise 
the approach sounds good to me.

Thanks
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Greg KH]

On Fri, Aug 15, 2008 at 12:15:01PM -0700, Jesse Barnes wrote:
> On Friday, August 15, 2008 11:55 am Jean Delvare wrote:
> > In fact we can do even better than that. We could accept from
> > user-space only driver_data values which at least one device ID entry in
> > the driver already uses. That should be fairly easy to implement, and
> > would offer a level of safety an order of magnitude above what we have
> > at the moment... And it works both ways: if 0 is not a valid data for
> > some driver, that would force the user to provide a non-zero (and
> > valid) data value. And it guarantees that the user can't ask for
> > something the driver doesn't expect, so drivers don't even need extra
> > checks. And no need for a use_driver_data flag either.
> 
> Meaning a driver audit of the usage?  Yeah that would be great.
> 
> > The only drawback is that it prevents the user from passing a "new"
> > data value even if it would be valid. But honestly, I don't expect that
> > case to happen frequently... if ever at all. So I'd say the benefits
> > totally outweight the drawback.
> >
> > If the interested people agree with the idea, I'll look into
> > implementing it.
> 
> Well the audit would show if user supplied "new" values are needed; otherwise 
> the approach sounds good to me.

That sounds reasonable, and should work properly.

No objection from me.

thanks,

greg k-h

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Jean Delvare]

Hi all,

On Fri, 15 Aug 2008 23:22:59 -0700, Greg KH wrote:
> On Fri, Aug 15, 2008 at 12:15:01PM -0700, Jesse Barnes wrote:
> > On Friday, August 15, 2008 11:55 am Jean Delvare wrote:
> > > In fact we can do even better than that. We could accept from
> > > user-space only driver_data values which at least one device ID entry in
> > > the driver already uses. That should be fairly easy to implement, and
> > > would offer a level of safety an order of magnitude above what we have
> > > at the moment... And it works both ways: if 0 is not a valid data for
> > > some driver, that would force the user to provide a non-zero (and
> > > valid) data value. And it guarantees that the user can't ask for
> > > something the driver doesn't expect, so drivers don't even need extra
> > > checks. And no need for a use_driver_data flag either.
> > 
> > Meaning a driver audit of the usage?  Yeah that would be great.
> > 
> > > The only drawback is that it prevents the user from passing a "new"
> > > data value even if it would be valid. But honestly, I don't expect that
> > > case to happen frequently... if ever at all. So I'd say the benefits
> > > totally outweight the drawback.
> > >
> > > If the interested people agree with the idea, I'll look into
> > > implementing it.
> > 
> > Well the audit would show if user supplied "new" values are needed; otherwise 
> > the approach sounds good to me.
> 
> That sounds reasonable, and should work properly.
> 
> No objection from me.

Ok, here's what it could look like:

* * * * *

From: Jean Delvare <khali@linux-fr.org>
Subject: PCI: Check dynids driver_data value for validity

Only accept dynids those driver_data value matches one of the driver's
pci_driver_id entry. This prevents the user from accidentally passing
values the drivers do not expect.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Milton Miller <miltonm@bga.com>
Cc: Greg KH <greg@kroah.com>
---
 Documentation/PCI/pci.txt       |    4 ++++
 drivers/i2c/busses/i2c-amd756.c |    4 ----
 drivers/i2c/busses/i2c-viapro.c |    4 ----
 drivers/pci/pci-driver.c        |   18 ++++++++++++++++--
 4 files changed, 20 insertions(+), 10 deletions(-)

--- linux-2.6.27-rc3.orig/Documentation/PCI/pci.txt	2008-08-17 18:24:33.000000000 +0200
+++ linux-2.6.27-rc3/Documentation/PCI/pci.txt	2008-08-17 18:24:38.000000000 +0200
@@ -163,6 +163,10 @@ need pass only as many optional fields a
 	o class and classmask fields default to 0
 	o driver_data defaults to 0UL.
 
+Note that driver_data must match the value used by any of the pci_device_id
+entries defined in the driver. This makes the driver_data field mandatory
+if all the pci_device_id entries have a non-zero driver_data value.
+
 Once added, the driver probe routine will be invoked for any unclaimed
 PCI devices listed in its (newly updated) pci_ids list.
 
--- linux-2.6.27-rc3.orig/drivers/i2c/busses/i2c-amd756.c	2008-08-17 17:15:57.000000000 +0200
+++ linux-2.6.27-rc3/drivers/i2c/busses/i2c-amd756.c	2008-08-17 19:42:14.000000000 +0200
@@ -332,10 +332,6 @@ static int __devinit amd756_probe(struct
 	int error;
 	u8 temp;
 	
-	/* driver_data might come from user-space, so check it */
-	if (id->driver_data >= ARRAY_SIZE(chipname))
-		return -EINVAL;
-
 	if (amd756_ioport) {
 		dev_err(&pdev->dev, "Only one device supported "
 		       "(you have a strange motherboard, btw)\n");
--- linux-2.6.27-rc3.orig/drivers/i2c/busses/i2c-viapro.c	2008-08-17 17:15:57.000000000 +0200
+++ linux-2.6.27-rc3/drivers/i2c/busses/i2c-viapro.c	2008-08-17 19:42:24.000000000 +0200
@@ -320,10 +320,6 @@ static int __devinit vt596_probe(struct 
 	unsigned char temp;
 	int error = -ENODEV;
 
-	/* driver_data might come from user-space, so check it */
-	if (id->driver_data & 1 || id->driver_data > 0xff)
-		return -EINVAL;
-
 	/* Determine the address of the SMBus areas */
 	if (force_addr) {
 		vt596_smba = force_addr & 0xfff0;
--- linux-2.6.27-rc3.orig/drivers/pci/pci-driver.c	2008-08-17 17:15:57.000000000 +0200
+++ linux-2.6.27-rc3/drivers/pci/pci-driver.c	2008-08-17 19:17:55.000000000 +0200
@@ -43,18 +43,32 @@ store_new_id(struct device_driver *drive
 {
 	struct pci_dynid *dynid;
 	struct pci_driver *pdrv = to_pci_driver(driver);
+	const struct pci_device_id *ids = pdrv->id_table;
 	__u32 vendor, device, subvendor=PCI_ANY_ID,
 		subdevice=PCI_ANY_ID, class=0, class_mask=0;
 	unsigned long driver_data=0;
 	int fields=0;
-	int retval = 0;
+	int retval;
 
-	fields = sscanf(buf, "%x %x %x %x %x %x %lux",
+	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
 			&vendor, &device, &subvendor, &subdevice,
 			&class, &class_mask, &driver_data);
 	if (fields < 2)
 		return -EINVAL;
 
+	/* Only accept driver_data values that match an existing id_table
+	   entry */
+	retval = -EINVAL;
+	while (ids->vendor || ids->subvendor || ids->class_mask) {
+		if (driver_data == ids->driver_data) {
+			retval = 0;
+			break;
+		}
+		ids++;
+	}
+	if (retval)	/* No match */
+		return retval;
+
 	dynid = kzalloc(sizeof(*dynid), GFP_KERNEL);
 	if (!dynid)
 		return -ENOMEM;


* * * * *

The patch above applies on top of Milton's patch removing
dynids.use_driver_data.

Note that I fixed a bug in the code: the driver_data value was scanned
with format "%lux" which isn't valid. It's either "%lu" or "%lx". It
went unnoticed so far because it's the last field. I've made it "%lx"
because that's what our documentation says it should be. The old code
was behaving like "%lu" instead, so that's an interface change. But I
doubt it matters much, given that only 3 drivers were using this field
so far.

As mentioned before, this safety check might be too tight in some
cases (for example if driver_data is a bit field and the new device
needs a flag combination that no supported device uses.) It wouldn't be
difficult to let drivers set a flag saying they will care about the
safety check themselves. I can even implement it now if anybody thinks
that my code is too restrictive.

Thanks,
-- 
Jean Delvare

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Greg KH]

On Sun, Aug 17, 2008 at 09:06:59PM +0200, Jean Delvare wrote:
> Hi all,
> 
> On Fri, 15 Aug 2008 23:22:59 -0700, Greg KH wrote:
> > On Fri, Aug 15, 2008 at 12:15:01PM -0700, Jesse Barnes wrote:
> > > On Friday, August 15, 2008 11:55 am Jean Delvare wrote:
> > > > In fact we can do even better than that. We could accept from
> > > > user-space only driver_data values which at least one device ID entry in
> > > > the driver already uses. That should be fairly easy to implement, and
> > > > would offer a level of safety an order of magnitude above what we have
> > > > at the moment... And it works both ways: if 0 is not a valid data for
> > > > some driver, that would force the user to provide a non-zero (and
> > > > valid) data value. And it guarantees that the user can't ask for
> > > > something the driver doesn't expect, so drivers don't even need extra
> > > > checks. And no need for a use_driver_data flag either.
> > > 
> > > Meaning a driver audit of the usage?  Yeah that would be great.
> > > 
> > > > The only drawback is that it prevents the user from passing a "new"
> > > > data value even if it would be valid. But honestly, I don't expect that
> > > > case to happen frequently... if ever at all. So I'd say the benefits
> > > > totally outweight the drawback.
> > > >
> > > > If the interested people agree with the idea, I'll look into
> > > > implementing it.
> > > 
> > > Well the audit would show if user supplied "new" values are needed; otherwise 
> > > the approach sounds good to me.
> > 
> > That sounds reasonable, and should work properly.
> > 
> > No objection from me.
> 
> Ok, here's what it could look like:
> 
> * * * * *
> 
> From: Jean Delvare <khali@linux-fr.org>
> Subject: PCI: Check dynids driver_data value for validity
> 
> Only accept dynids those driver_data value matches one of the driver's
> pci_driver_id entry. This prevents the user from accidentally passing
> values the drivers do not expect.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Milton Miller <miltonm@bga.com>
> Cc: Greg KH <greg@kroah.com>

Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

Looks good, thanks for sticking with it and creating this.

greg k-h

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Jesse Barnes]

On Sunday, August 17, 2008 12:06 pm Jean Delvare wrote:
> Hi all,
> From: Jean Delvare <khali@linux-fr.org>
> Subject: PCI: Check dynids driver_data value for validity
>
> Only accept dynids those driver_data value matches one of the driver's
> pci_driver_id entry. This prevents the user from accidentally passing
> values the drivers do not expect.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Milton Miller <miltonm@bga.com>
> Cc: Greg KH <greg@kroah.com>
> ---
>  Documentation/PCI/pci.txt       |    4 ++++
>  drivers/i2c/busses/i2c-amd756.c |    4 ----
>  drivers/i2c/busses/i2c-viapro.c |    4 ----
>  drivers/pci/pci-driver.c        |   18 ++++++++++++++++--
>  4 files changed, 20 insertions(+), 10 deletions(-)
>
> --- linux-2.6.27-rc3.orig/Documentation/PCI/pci.txt	2008-08-17
> 18:24:33.000000000 +0200 +++
> linux-2.6.27-rc3/Documentation/PCI/pci.txt	2008-08-17 18:24:38.000000000
> +0200 @@ -163,6 +163,10 @@ need pass only as many optional fields a
>  	o class and classmask fields default to 0
>  	o driver_data defaults to 0UL.
>
> +Note that driver_data must match the value used by any of the
> pci_device_id +entries defined in the driver. This makes the driver_data
> field mandatory +if all the pci_device_id entries have a non-zero
> driver_data value. +
>  Once added, the driver probe routine will be invoked for any unclaimed
>  PCI devices listed in its (newly updated) pci_ids list.
>
> --- linux-2.6.27-rc3.orig/drivers/i2c/busses/i2c-amd756.c	2008-08-17
> 17:15:57.000000000 +0200 +++
> linux-2.6.27-rc3/drivers/i2c/busses/i2c-amd756.c	2008-08-17
> 19:42:14.000000000 +0200 @@ -332,10 +332,6 @@ static int __devinit
> amd756_probe(struct
>  	int error;
>  	u8 temp;
>
> -	/* driver_data might come from user-space, so check it */
> -	if (id->driver_data >= ARRAY_SIZE(chipname))
> -		return -EINVAL;
> -
>  	if (amd756_ioport) {
>  		dev_err(&pdev->dev, "Only one device supported "
>  		       "(you have a strange motherboard, btw)\n");
> --- linux-2.6.27-rc3.orig/drivers/i2c/busses/i2c-viapro.c	2008-08-17
> 17:15:57.000000000 +0200 +++
> linux-2.6.27-rc3/drivers/i2c/busses/i2c-viapro.c	2008-08-17
> 19:42:24.000000000 +0200 @@ -320,10 +320,6 @@ static int __devinit
> vt596_probe(struct
>  	unsigned char temp;
>  	int error = -ENODEV;
>
> -	/* driver_data might come from user-space, so check it */
> -	if (id->driver_data & 1 || id->driver_data > 0xff)
> -		return -EINVAL;
> -
>  	/* Determine the address of the SMBus areas */
>  	if (force_addr) {
>  		vt596_smba = force_addr & 0xfff0;
> --- linux-2.6.27-rc3.orig/drivers/pci/pci-driver.c	2008-08-17
> 17:15:57.000000000 +0200 +++
> linux-2.6.27-rc3/drivers/pci/pci-driver.c	2008-08-17 19:17:55.000000000
> +0200 @@ -43,18 +43,32 @@ store_new_id(struct device_driver *drive
>  {
>  	struct pci_dynid *dynid;
>  	struct pci_driver *pdrv = to_pci_driver(driver);
> +	const struct pci_device_id *ids = pdrv->id_table;
>  	__u32 vendor, device, subvendor=PCI_ANY_ID,
>  		subdevice=PCI_ANY_ID, class=0, class_mask=0;
>  	unsigned long driver_data=0;
>  	int fields=0;
> -	int retval = 0;
> +	int retval;
>
> -	fields = sscanf(buf, "%x %x %x %x %x %x %lux",
> +	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
>  			&vendor, &device, &subvendor, &subdevice,
>  			&class, &class_mask, &driver_data);
>  	if (fields < 2)
>  		return -EINVAL;
>
> +	/* Only accept driver_data values that match an existing id_table
> +	   entry */
> +	retval = -EINVAL;
> +	while (ids->vendor || ids->subvendor || ids->class_mask) {
> +		if (driver_data == ids->driver_data) {
> +			retval = 0;
> +			break;
> +		}
> +		ids++;
> +	}
> +	if (retval)	/* No match */
> +		return retval;
> +
>  	dynid = kzalloc(sizeof(*dynid), GFP_KERNEL);
>  	if (!dynid)
>  		return -ENOMEM;
>
>
> * * * * *
>
> The patch above applies on top of Milton's patch removing
> dynids.use_driver_data.

Looks good; I think we'll want to put this into linux-next along with Milton's 
change.  I'll push them out after a quick smoke test.

Thanks,
Jesse

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Jesse Barnes]

On Sunday, August 17, 2008 12:06 pm Jean Delvare wrote:
> Subject: PCI: Check dynids driver_data value for validity
>
> Only accept dynids those driver_data value matches one of the driver's
> pci_driver_id entry. This prevents the user from accidentally passing
> values the drivers do not expect.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Milton Miller <miltonm@bga.com>
> Cc: Greg KH <greg@kroah.com>

Applied to linux-next, thanks Jean.

Jesse

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Milton Miller]

On Aug 17, 2008, at 2:06 PM, Jean Delvare wrote:
> On Fri, 15 Aug 2008 23:22:59 -0700, Greg KH wrote:
>> On Fri, Aug 15, 2008 at 12:15:01PM -0700, Jesse Barnes wrote:
>>> On Friday, August 15, 2008 11:55 am Jean Delvare wrote:
>>>> In fact we can do even better than that. We could accept from
>>>> user-space only driver_data values which at least one device ID 
>>>> entry in
>>>> the driver already uses. That should be fairly easy to implement, 
>>>> and
>>>> would offer a level of safety an order of magnitude above what we 
>>>> have
>>>> at the moment... And it works both ways: if 0 is not a valid data 
>>>> for
>>>> some driver, that would force the user to provide a non-zero (and
>>>> valid) data value. And it guarantees that the user can't ask for
>>>> something the driver doesn't expect, so drivers don't even need 
>>>> extra
>>>> checks. And no need for a use_driver_data flag either.
>>>
>>> Meaning a driver audit of the usage?  Yeah that would be great.

Thanks Jean for doing this.  Sometimes things move quickly after a long 
stall.  I thought about proposing a similar patch and therefore have to 
say Ack.

>>>> The only drawback is that it prevents the user from passing a "new"
>>>> data value even if it would be valid. But honestly, I don't expect 
>>>> that
>>>> case to happen frequently... if ever at all. So I'd say the benefits
>>>> totally outweight the drawback.

There are a few drivers that could benefit, mainly ones that I 
identified as using flags.  For example, the radeon driver uses 
different fields of the data to specify crt controller, video output 
device, etc.   I'm fine with deferring a flag for such drivers until 
someone audits a driver and wants the support.

>>>>
>>>> If the interested people agree with the idea, I'll look into
>>>> implementing it.
>>>
>>> Well the audit would show if user supplied "new" values are needed; 
>>> otherwise
>>> the approach sounds good to me.
>>
>> That sounds reasonable, and should work properly.
>>
>> No objection from me.

so, if anyone asks,

Concept-Acked-By: Milton Miller <miltonm@bga.com>

milton

Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Jean Delvare]

Hi Milton, hi Greg,

On Wed, 16 Jul 2008 05:18:18 -0500, Milton Miller wrote:
> Greg,
> 
> Please respond to this email and explain why the patch
> 
> pci: dynids.use_driver_data considered harmful
> 
> http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
> 
> should not be applied.   I am not arguing the correctness of
> the removed code, rather its utility and benefit to the linux
> community.

I _am_ arguing the correctness of the removed code. That code silently
defaults driver_data to 0 when the driver doesn't set
dynids.use_driver_data. This assumes that 0 is always a safe value,
which is not true for all drivers as Milton explains below.

> As far as I can tell, the code only succeeds in limiting the
> usefulness of the pci dynamic id addition mechanism.  We chose
> not to limit which drivers can have a table entry added, now
> let us not limit telling the driver which previous entry is
> most similar to our new entry.
> 
> If a driver doesn't set this bit, and only 3 out of 419 do,
> then the facility can only be used if the driver can function
> correctly with the data zero.  In some drivers (radeonfb) a
> nonzero flag is always set, in some a list of boards or
> chipsets is listed in an arbitrary order (e1000e), and in yet
> others the field is used as a pointer without checking for NULL
> (DAC960, iwl3945).

I have to agree with Milton. Allowing all drivers to use dynamic IDs
first, and then limiting the use of the driver_data field to drivers
setting a specific flag, doesn't make sense. For one thing, just
because the driver sets the flag, doesn't mean that it does the proper
checks on the passed value. For another, as written above, assuming
that 0 is a safe value (that doesn't need to be checked by the driver)
is incorrect.

The correct approach here if you really want to play it safe, would be
to not allow dynamic IDs by default. Drivers would set a flag when they
want to use them (instead of when they want to use driver_data as we
currently do.) Setting the flag would suggest that you have checked
that nothing can go wrong when a dynamic ID is added. As I said above,
it's impossible to actually enforce other than with careful code
review, but if you insist on trying, maybe we can have these drivers
set a validation callback function rather than a flag. In the absence of
validation callback function, dynamic IDs would be forbidden. This has
a cost in terms of driver size though.

That would be a pretty big change, as someone would have to go through
all the PCI drivers and update them. I certainly do not have the time
to do that, but maybe Milton does if we agree on that?

The alternative is to just apply Milton's patch and get away with all
checks. That's perfectly fine with me, as I can't see how it is worse
than what we currently have, but apparently not with Greg (although I
still don't know why.) If Milton's patch isn't going to be applied,
then I would like to suggest applying the following patch of mine as a
measure to prevent what happened to Milton and started this discussion:

* * * * *

Subject: PCI: Don't silently ignore dynids driver data

If driver data is provided when adding a dynamic ID to a PCI driver,
but the driver doesn't actually support that, fail, instead of
silently defaulting to a value of 0.

Likewise, if the driver expects driver_data and the user didn't
provide it, fail, instead of silently defaulting to a value of 0.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Milton Miller <miltonm@bga.com>
Cc: Greg KH <gregkh@suse.de>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/pci/pci-driver.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

--- linux-2.6.26-rc9.orig/drivers/pci/pci-driver.c	2008-06-23 08:38:32.000000000 +0200
+++ linux-2.6.26-rc9/drivers/pci/pci-driver.c	2008-07-13 09:59:39.000000000 +0200
@@ -54,6 +54,10 @@ store_new_id(struct device_driver *drive
 			&class, &class_mask, &driver_data);
 	if (fields < 2)
 		return -EINVAL;
+	/* Presence of driver_data must match the use_driver_data flag */
+	if ((fields >= 7 && !pdrv->dynids.use_driver_data)
+	 || (fields < 7 && pdrv->dynids.use_driver_data))
+		return -EINVAL;
 
 	dynid = kzalloc(sizeof(*dynid), GFP_KERNEL);
 	if (!dynid)
@@ -65,8 +69,7 @@ store_new_id(struct device_driver *drive
 	dynid->id.subdevice = subdevice;
 	dynid->id.class = class;
 	dynid->id.class_mask = class_mask;
-	dynid->id.driver_data = pdrv->dynids.use_driver_data ?
-		driver_data : 0UL;
+	dynid->id.driver_data = driver_data;
 
 	spin_lock(&pdrv->dynids.lock);
 	list_add_tail(&dynid->node, &pdrv->dynids.list);

* * * * *

> You sent your request for others to withdraw the patch
> from consideration when I resent the patch without seeing your
> comments that were less than 12 hours old, but have been silent
> for the last 60 hours since I responded to them over the next
> several hours.   If I do not hear from you with technical
> arguments for keeping the code, I will resend the patch for
> consideration.

Milton, please keep in mind that many people are on vacation or
meetings at this time of the year. Delayed replies aren't unusual, and
neither is plain lack of reply. When you come back from vacation and
have over 1300 mails to process, you _have_ to ignore some of the
discussions. Important things will resurface later anyway.

Thanks,
-- 
Jean Delvare