| Message ID | 20110312223212.27020.9839.stgit@otae.warmcat.com |
|---|---|
| State | New |
| Headers | show |
Hi, On Saturday, March 12, 2011, Andy Green wrote: > This structure allows tagging arbitrary platform_data that can't be attached > to a device until after it is probed, with the device path name that it is > to be attached to. > > Signed-off-by: Andy Green <andy.green@linaro.org> > --- > > include/linux/platform_device.h | 17 +++++++++++++++++ > 1 files changed, 17 insertions(+), 0 deletions(-) > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > index 2e700ec..d8c0ba9 100644 > --- a/include/linux/platform_device.h > +++ b/include/linux/platform_device.h > @@ -193,4 +193,21 @@ static inline char *early_platform_driver_setup_func(void) \ > } > #endif /* MODULE */ > > +/** > + * platform_async_platform_data - maps a known bus + device name on to > + * platform_data to be attached to that device > + * when it is eventually instantiated. For use > + * with onboard devices on buses that probe > + * asynchronously. Device path fields must > + * be separated with '/'. > + * @device_path: bus / device path, eg, "usb1/1-1/1-1.1" > + * @platform_data: platform_data to attach to device matching the > + * device_path > + */ > + > +struct platform_async_platform_data { > + const char *device_path; > + void *platform_data; > +}; > + > #endif /* _PLATFORM_DEVICE_H_ */ Using device paths for this purpose seems to be very fragile to me. Isn't there any better solution? Rafael
On 03/12/2011 11:29 PM, Somebody in the thread at some point said: Hi - > On Saturday, March 12, 2011, Andy Green wrote: >> This structure allows tagging arbitrary platform_data that can't be attached >> to a device until after it is probed, with the device path name that it is >> to be attached to. >> +struct platform_async_platform_data { >> + const char *device_path; >> + void *platform_data; >> +}; >> + >> #endif /* _PLATFORM_DEVICE_H_ */ > > Using device paths for this purpose seems to be very fragile to me. Isn't > there any better solution? Given that this targets board definition files which commonly do the platform_add_device for the USB bus controller synchronously, and the bus-connected devices it is aimed at are soldered on to the board connected to specific bus controllers, the bus paths are completely deterministic. -Andy
On Sat, Mar 12, 2011 at 11:39:15PM +0000, Andy Green wrote: > On 03/12/2011 11:29 PM, Somebody in the thread at some point said: > > Hi - > > >On Saturday, March 12, 2011, Andy Green wrote: > >>This structure allows tagging arbitrary platform_data that can't be attached > >>to a device until after it is probed, with the device path name that it is > >>to be attached to. > > >>+struct platform_async_platform_data { > >>+ const char *device_path; > >>+ void *platform_data; > >>+}; > >>+ > >> #endif /* _PLATFORM_DEVICE_H_ */ > > > >Using device paths for this purpose seems to be very fragile to me. Isn't > >there any better solution? > > Given that this targets board definition files which commonly do the > platform_add_device for the USB bus controller synchronously, and > the bus-connected devices it is aimed at are soldered on to the > board connected to specific bus controllers, the bus paths are > completely deterministic. No they are not. The physical layout is deterministic, but the bus number, and device number, is not. You are using the bus number here in this path, so that is not going to work, sorry. thanks, greg k-h
On 03/13/2011 01:03 AM, Somebody in the thread at some point said: >>> Using device paths for this purpose seems to be very fragile to me. Isn't >>> there any better solution? >> >> Given that this targets board definition files which commonly do the >> platform_add_device for the USB bus controller synchronously, and >> the bus-connected devices it is aimed at are soldered on to the >> board connected to specific bus controllers, the bus paths are >> completely deterministic. > > No they are not. > > The physical layout is deterministic, but the bus number, and device > number, is not. You are using the bus number here in this path, so that > is not going to work, sorry. Okay. This is not a PC we are talking about. If the platform / board definition file is registering the USB hosts synchronously at boot time, the driver is composed into the monolithic kernel, there are no PCI busses or whatever on the SoC, the bus indexing is totally deterministic. This is extremely common in the platform / SoC case and is the case the patchset is targeted at. Even further, the only time you'd use it is to reach a USB asset that is wired up the same board permanently as well. Anyway this seems moot by now. -Andy
On Sunday, March 13, 2011, Andy Green wrote: > On 03/13/2011 01:03 AM, Somebody in the thread at some point said: > > >>> Using device paths for this purpose seems to be very fragile to me. Isn't > >>> there any better solution? > >> > >> Given that this targets board definition files which commonly do the > >> platform_add_device for the USB bus controller synchronously, and > >> the bus-connected devices it is aimed at are soldered on to the > >> board connected to specific bus controllers, the bus paths are > >> completely deterministic. > > > > No they are not. > > > > The physical layout is deterministic, but the bus number, and device > > number, is not. You are using the bus number here in this path, so that > > is not going to work, sorry. > > Okay. This is not a PC we are talking about. > > If the platform / board definition file is registering the USB hosts > synchronously at boot time, the driver is composed into the monolithic > kernel, there are no PCI busses or whatever on the SoC, the bus indexing > is totally deterministic. This is extremely common in the platform / > SoC case and is the case the patchset is targeted at. Even further, the > only time you'd use it is to reach a USB asset that is wired up the same > board permanently as well. > > Anyway this seems moot by now. However, if you add a new infrastructure like this, it should be at least usable on systems that you description doesn't apply to. Thanks, Rafael
On 03/13/2011 12:51 PM, Somebody in the thread at some point said: >> Okay. This is not a PC we are talking about. >> >> If the platform / board definition file is registering the USB hosts >> synchronously at boot time, the driver is composed into the monolithic >> kernel, there are no PCI busses or whatever on the SoC, the bus indexing >> is totally deterministic. This is extremely common in the platform / >> SoC case and is the case the patchset is targeted at. Even further, the >> only time you'd use it is to reach a USB asset that is wired up the same >> board permanently as well. >> >> Anyway this seems moot by now. > > However, if you add a new infrastructure like this, it should be at least > usable on systems that you description doesn't apply to. Sounds reasonable, except the platform data is coming from a board-specific board definition file at boot-time to talk about assets that are on fixed interfaces of a specific board. It's not really applicable to wider generic bus use, just like platform_data usually isn't and has to be targeted at "device at XYZ on I2C bus n" with knowledge of what driver is bound to that device. Despite that "impedence mismatch", it covers the SoC onboard USB asset case just fine as it is. If you mean though, that the patch series implements new configuration options in usbnet that are anyway interesting to expose for the general case, I can see the point but don't know enough about udev / usb subsystem internals to suggest a way to expose the options nicely. It's also commented the "right" thing to do is to have the driver set the device up to wrong defaults like inappropriate interface name / random MAC and have userland clean up the mess, rather than have the board file ask the driver to set things appropriately. -Andy
On Sun, Mar 13, 2011 at 11:22:40AM +0000, Andy Green wrote: > On 03/13/2011 01:03 AM, Somebody in the thread at some point said: > > >>>Using device paths for this purpose seems to be very fragile to me. Isn't > >>>there any better solution? > >> > >>Given that this targets board definition files which commonly do the > >>platform_add_device for the USB bus controller synchronously, and > >>the bus-connected devices it is aimed at are soldered on to the > >>board connected to specific bus controllers, the bus paths are > >>completely deterministic. > > > >No they are not. > > > >The physical layout is deterministic, but the bus number, and device > >number, is not. You are using the bus number here in this path, so that > >is not going to work, sorry. > > Okay. This is not a PC we are talking about. I know that, and again, it doesn't matter. You CAN NOT GUARANTEE the USB device ordering of bus numbers or device numbers. It's that simple. > If the platform / board definition file is registering the USB hosts > synchronously at boot time, the driver is composed into the > monolithic kernel, there are no PCI busses or whatever on the SoC, > the bus indexing is totally deterministic. Not true, it could change for a number of reasons, not the least being your kernel version changed. So again NEVER rely on this, bad things could happen in the field when you least expect it. thanks, greg k-h
On Sunday, March 13, 2011, Andy Green wrote: > On 03/13/2011 12:51 PM, Somebody in the thread at some point said: > > >> Okay. This is not a PC we are talking about. > >> > >> If the platform / board definition file is registering the USB hosts > >> synchronously at boot time, the driver is composed into the monolithic > >> kernel, there are no PCI busses or whatever on the SoC, the bus indexing > >> is totally deterministic. This is extremely common in the platform / > >> SoC case and is the case the patchset is targeted at. Even further, the > >> only time you'd use it is to reach a USB asset that is wired up the same > >> board permanently as well. > >> > >> Anyway this seems moot by now. > > > > However, if you add a new infrastructure like this, it should be at least > > usable on systems that you description doesn't apply to. > > Sounds reasonable, except the platform data is coming from a > board-specific board definition file at boot-time to talk about assets > that are on fixed interfaces of a specific board. I'm not sure how this contradicts what I said above. > It's not really applicable to wider generic bus use, just like platform_data > usually isn't and has to be targeted at "device at XYZ on I2C bus n" with > knowledge of what driver is bound to that device. Despite that > "impedence mismatch", it covers the SoC onboard USB asset case just fine > as it is. So, you want to have a mechanism telling the driver "if the device happens to have this particular path, use that platform data", right? And it works because the initialization code kind of knows what path the device is going to be at, so it can predict that and provide the mathing data. Unfortunately, this relies on how device paths are constructed at the moment, so if this approach is adopted in general, it will prevent us from changing that way in the future (or at least it will make that very difficult). Perhaps you could use some other kind of device identification here? Rafael
On 03/13/2011 04:58 PM, Somebody in the thread at some point said: > So, you want to have a mechanism telling the driver "if the device > happens to have this particular path, use that platform data", right? Yeah. > And it works because the initialization code kind of knows what path the > device is going to be at, so it can predict that and provide the mathing data. > > Unfortunately, this relies on how device paths are constructed at the moment, > so if this approach is adopted in general, it will prevent us from changing > that way in the future (or at least it will make that very difficult). > > Perhaps you could use some other kind of device identification here? I'm sorry what prevents you changing paths in the future? Nothing does, if you change the bus tag from like usb1 to UsB_1 you just fix up the strings in the board definition files at the same time, they are sitting there in the same tree and grep platform_async_platform_data arch/* -R will show them all up in one hit. It's no different if you changed the name of a driver, you'd patch the board definition files with devices that need to bind to that driver name to uplevel them to the new name. The board definition file for these SoC cases usually has access to a pointer to the host controller directly since it instantiates them, if this was the only stumbling point and you thought it helped something the matching system can look for that particular pointer being a parent of the candidate probed device. -Andy
On 03/13/2011 04:14 PM, Somebody in the thread at some point said: Hi - > You CAN NOT GUARANTEE the USB device ordering of bus numbers or device > numbers. It's that simple. > >> If the platform / board definition file is registering the USB hosts >> synchronously at boot time, the driver is composed into the >> monolithic kernel, there are no PCI busses or whatever on the SoC, >> the bus indexing is totally deterministic. > > Not true, it could change for a number of reasons, not the least being > your kernel version changed. > > So again NEVER rely on this, bad things could happen in the field when > you least expect it. Okay, I can't see how and you did not explain how, but let's agree with what you are saying. Unlike the Shiny Device Tree path where the binding device path string is in the bootloader, with this patch series the binding device path is in the board definition file, ie, part of the same kernel. If an upgrade breaks it, the guy can look in /sys on his new broken kernel, find the new path and uplevel it to that and he's consistent again. If he goes back to an older kernel, it still works consistently (unlike if he updated his bootloader that now only knows the new way). However as I said to Rafael if he thought bus name part of this path was too shaky, and you also think it is, it can be changed to use a pointer to the host controller since that's also coming from platform or board definition file directly in this kind of SoC implementation and is available. -Andy
On Sunday, March 13, 2011, Andy Green wrote: > On 03/13/2011 04:58 PM, Somebody in the thread at some point said: > > > So, you want to have a mechanism telling the driver "if the device > > happens to have this particular path, use that platform data", right? > > Yeah. > > > And it works because the initialization code kind of knows what path the > > device is going to be at, so it can predict that and provide the mathing data. > > > > Unfortunately, this relies on how device paths are constructed at the moment, > > so if this approach is adopted in general, it will prevent us from changing > > that way in the future (or at least it will make that very difficult). > > > > Perhaps you could use some other kind of device identification here? > > I'm sorry what prevents you changing paths in the future? > > Nothing does, if you change the bus tag from like usb1 to UsB_1 you just > fix up the strings in the board definition files at the same time, they > are sitting there in the same tree and > > grep platform_async_platform_data arch/* -R > > will show them all up in one hit. It's no different if you changed the > name of a driver, you'd patch the board definition files with devices > that need to bind to that driver name to uplevel them to the new name. Well, I'm in the process of doing something similar with sysdevs and, trust me, it is not fun. :-/ > The board definition file for these SoC cases usually has access to a > pointer to the host controller directly since it instantiates them, if > this was the only stumbling point and you thought it helped something > the matching system can look for that particular pointer being a parent > of the candidate probed device. I think that would have been better than matching based on device paths. Whether or not it's the only stumbling point depends on what other people think. Thanks, Rafael
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h index 2e700ec..d8c0ba9 100644 --- a/include/linux/platform_device.h +++ b/include/linux/platform_device.h @@ -193,4 +193,21 @@ static inline char *early_platform_driver_setup_func(void) \ } #endif /* MODULE */ +/** + * platform_async_platform_data - maps a known bus + device name on to + * platform_data to be attached to that device + * when it is eventually instantiated. For use + * with onboard devices on buses that probe + * asynchronously. Device path fields must + * be separated with '/'. + * @device_path: bus / device path, eg, "usb1/1-1/1-1.1" + * @platform_data: platform_data to attach to device matching the + * device_path + */ + +struct platform_async_platform_data { + const char *device_path; + void *platform_data; +}; + #endif /* _PLATFORM_DEVICE_H_ */
This structure allows tagging arbitrary platform_data that can't be attached to a device until after it is probed, with the device path name that it is to be attached to. Signed-off-by: Andy Green <andy.green@linaro.org> --- include/linux/platform_device.h | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-)