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

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.

Quoted 4 lines · 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:

Quoted 6 lines

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?

Quoted 5 lines · Hans de Goede

Quoted 4 lines · 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:

...

Quoted 23 lines

Quoted 6 lines

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.

Quoted 8 lines

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.

Hi Hans, hi Milton,

Quoted 2 lines

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.

Quoted 2 lines · Jean Delvare

Hi Hans, hi Milton,

<snip>

Quoted 9 lines

Quoted 3 lines

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.

Quoted 13 lines · Hans de Goede

Quoted 2 lines · Jean Delvare

Hi Hans, hi Milton,

<snip>

Quoted 8 lines

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 22 lines · Jean Delvare

Quoted 13 lines · Hans de Goede

Quoted 2 lines · Jean Delvare

Hi Hans, hi Milton,

<snip>

Quoted 8 lines

Quoted 3 lines

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.

Quoted 7 lines

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?

Quoted 6 lines

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.

Hi Hans, Milton, Jean,

Quoted 28 lines · Hans de Goede

Quoted 18 lines · Jean Delvare

Quoted 17 lines · Hans de Goede

Quoted 2 lines · Jean Delvare

Hi Hans, hi Milton,

<snip>

Quoted 8 lines

Quoted 3 lines

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.

Quoted 9 lines

Quoted 3 lines

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.

Quoted 3 lines

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?

Quoted 9 lines

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?

Hi Hans,

Quoted 13 lines

Quoted 9 lines

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.

Quoted 6 lines

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?

Quoted 27 lines · David Hubbard

Hi Hans,

Quoted 13 lines

Quoted 9 lines

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.

Quoted 6 lines

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?

Quoted 4 lines

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?

Quoted 50 lines · Jean Delvare

Quoted 25 lines · David Hubbard

Hi Hans,

Quoted 12 lines

Quoted 9 lines

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.

Quoted 6 lines

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?

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 11 lines · Milton Miller

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.

Hi Hans,

Quoted 23 lines · Hans de Goede

Quoted 10 lines · Milton Miller

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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 6 lines · Jean Delvare

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.

Quoted 3 lines

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.

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.

Quoted 20 lines · Jean Delvare

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.

Quoted 4 lines

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.

Quoted 1 line

(...) 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.

kcalloc is supposed to be called with the count as its first argument and the element size as the second.

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.

Quoted 5 lines · 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.

Quoted 3 lines

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.

Quoted 3 lines

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 :(

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 :(

Quoted 4 lines · Greg KH

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

Quoted 5 lines · Milton Miller

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.

Quoted 1 line

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?

Quoted 9 lines

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

Quoted 1 line

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).

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

You left out:

Quoted 5 lines

Quoted 5 lines

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

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 6 lines · Milton Miller

You left out:

Quoted 5 lines

Quoted 5 lines

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

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

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

drivers/pci/pci-driver.c +26 -0

index a13f534..0aeac3a 100644
@@ -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)

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.

Quoted 15 lines · 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.

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?

Quoted 9 lines · 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

Quoted 3 lines

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,

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?

Hi Greg,

Quoted 14 lines · Greg KH

Quoted 12 lines · 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.

Quoted 3 lines

(...) 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.

Quoted 4 lines

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.

Quoted 17 lines · Jean Delvare

Hi Greg,

Quoted 14 lines · Greg KH

Quoted 12 lines · 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.

Quoted 7 lines

Quoted 3 lines

(...) 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 :)

Quoted 6 lines

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.

Quoted 7 lines

Quoted 6 lines

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?

Quoted 6 lines

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?

Quoted 17 lines · Jean Delvare

Hi Greg,

Quoted 14 lines · Greg KH

Quoted 12 lines · 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.

Quoted 7 lines

Quoted 3 lines

(...) 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 :)

Quoted 6 lines

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.

Quoted 11 lines

Quoted 4 lines

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?

Quoted 2 lines

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.

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.

Quoted 9 lines · Jean Delvare

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.

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 21 lines · Jesse Barnes

Quoted 9 lines · Jean Delvare

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.

Quoted 7 lines

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.

Quoted 2 lines

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.

Hi all,

Quoted 22 lines · Greg KH

Quoted 21 lines · Jesse Barnes

Quoted 9 lines · Jean Delvare

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.

Quoted 7 lines

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.

Quoted 6 lines

Quoted 2 lines

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 <[email protected]>
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 <[email protected]>

Cc Jesse Barnes <[email protected]>

Cc Milton Miller <[email protected]>

Cc Greg KH <[email protected]>

Hi all,
From: Jean Delvare <[email protected]>
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 <[email protected]>

Cc Jesse Barnes <[email protected]>

Cc Milton Miller <[email protected]>

Cc Greg KH <[email protected]>

---
 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

Documentation/PCI/pci.txt +1 -0

 	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

drivers/i2c/busses/i2c-amd756.c +0 -5

 	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

drivers/i2c/busses/i2c-viapro.c +0 -5

 	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
{

drivers/pci/pci-driver.c +16 -2

 	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.

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 <[email protected]>

Cc Jesse Barnes <[email protected]>

Cc Milton Miller <[email protected]>

Cc Greg KH <[email protected]>

Quoted 12 lines · Greg KH

Quoted 11 lines · Jesse Barnes

Quoted 9 lines · Jean Delvare

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.

Quoted 1 line

Quoted 1 line

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

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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.

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

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.

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.