| Message ID | 1440054451-1223-4-git-send-email-javier@osg.samsung.com |
|---|---|
| State | Accepted |
| Commit | d1d97b76c4af41c8e836e73742c91cbf97d7483c |
| Headers | show |
On Thu, Aug 20, 2015 at 09:07:16AM +0200, Javier Martinez Canillas wrote: > The SPI core always reports the MODALIAS uevent as "spi:<modalias>" > regardless of the mechanism that was used to register the device > (i.e: OF or board code) and the table that is used later to match > the driver with the device (i.e: SPI id table or OF match table). > > So drivers needs to export the SPI id table and this be built into > the module or udev won't have the necessary information to autoload > the needed driver module when the device is added. > > But this means that OF-only drivers needs to have both OF and SPI id > tables that have to be kept in sync and also the dev node compatible > manufacturer prefix is stripped when reporting the MODALIAS. Which can > lead to issues if two vendors use the same SPI device name for example. > > To avoid the above, the SPI core behavior may be changed in the future > to not require an SPI device table for OF-only drivers and report the > OF module alias. So, it's better to also export the OF table even when > is unused now to prevent breaking module loading when the core changes. > > Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> I appreciate the context for the whole problem here, but this commit subject mostly doesn't apply to this driver, and it deson't seem to match the problem this is solving. As I see it, the patch description for this patch should be more like: "The OF ID table is used for auto-probing the device, but it is not exported to the module device table. That means this driver won't autoload when built as a module. Export the OF ID table to fix that. (insert some reference to the larger issue here)" Do you want to rewrite the message, shall I just prepend my modifications, or am I off-base? > --- > > drivers/mtd/devices/mtd_dataflash.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c > index 0099aba72a8b..df6f61137376 100644 > --- a/drivers/mtd/devices/mtd_dataflash.c > +++ b/drivers/mtd/devices/mtd_dataflash.c > @@ -102,6 +102,7 @@ static const struct of_device_id dataflash_dt_ids[] = { > { .compatible = "atmel,dataflash", }, > { /* sentinel */ } > }; > +MODULE_DEVICE_TABLE(of, dataflash_dt_ids); > #endif > > /* ......................................................................... */ For the patch: Reviewed-by: Brian Norris <computersforpeace@gmail.com>
Hello Brian, On 08/20/2015 11:54 PM, Brian Norris wrote: > On Thu, Aug 20, 2015 at 09:07:16AM +0200, Javier Martinez Canillas wrote: >> The SPI core always reports the MODALIAS uevent as "spi:<modalias>" >> regardless of the mechanism that was used to register the device >> (i.e: OF or board code) and the table that is used later to match >> the driver with the device (i.e: SPI id table or OF match table). >> >> So drivers needs to export the SPI id table and this be built into >> the module or udev won't have the necessary information to autoload >> the needed driver module when the device is added. >> >> But this means that OF-only drivers needs to have both OF and SPI id >> tables that have to be kept in sync and also the dev node compatible >> manufacturer prefix is stripped when reporting the MODALIAS. Which can >> lead to issues if two vendors use the same SPI device name for example. >> >> To avoid the above, the SPI core behavior may be changed in the future >> to not require an SPI device table for OF-only drivers and report the >> OF module alias. So, it's better to also export the OF table even when >> is unused now to prevent breaking module loading when the core changes. >> >> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> > > I appreciate the context for the whole problem here, but this commit > subject mostly doesn't apply to this driver, and it deson't seem to > match the problem this is solving. As I see it, the patch description > for this patch should be more like: > > "The OF ID table is used for auto-probing the device, but it is not > exported to the module device table. That means this driver won't > autoload when built as a module. > > Export the OF ID table to fix that. > > (insert some reference to the larger issue here)" > > Do you want to rewrite the message, shall I just prepend my > modifications, or am I off-base? > Well, I believe my commit message is more accurate than yours :-) I mean, right now the OF modalias information is not really used neither by the SPI core since always MODALIAS=spi:foo is reported nor by user space since the MODALIAS uevent will never match the OF exported modalias. So this patch really is a no-op right now and is not fixing anything. As long as the driver has a MODULE_DEVICE_TABLE(spi,...), things will work without a MODULE_DEVICE_TABLE(of,...). However, this patch is needed to avoid breaking module autoloading in the future once RFC patch 18/18 is applied and the SPI core is changed to report a MODALIAS=of:N*T*Cfoo,bar for SPI devices registered by OF. Having said that, I will of course re-spin the patch and write a commit message that you agree on. So I'll be happy to trim it down if is providing too much content or is not correct. I just don't think your suggestion better matches reality than mine. Best regards,
On Fri, Aug 21, 2015 at 12:13:34AM +0200, Javier Martinez Canillas wrote: > So this patch really is a no-op right now and is not fixing anything. > As long as the driver has a MODULE_DEVICE_TABLE(spi,...), things will This driver does not have a MODULE_DEVICE_TABLE() at all, nor does it use spi_device_id. So, it is currently broken for the module use case. > work without a MODULE_DEVICE_TABLE(of,...). Brian
Hello Brian, On 08/21/2015 12:34 AM, Brian Norris wrote: > On Fri, Aug 21, 2015 at 12:13:34AM +0200, Javier Martinez Canillas wrote: >> So this patch really is a no-op right now and is not fixing anything. >> As long as the driver has a MODULE_DEVICE_TABLE(spi,...), things will > > This driver does not have a MODULE_DEVICE_TABLE() at all, nor does it > use spi_device_id. So, it is currently broken for the module use case. > That is correct but also that does not mean that this patch will fix module autoloading right now. It will though once the SPI core is changed to report a OF type module alias. So what about something along this lines? mtd: dataflash: Export OF module alias information The SPI core currently reports the MODALIAS uevent as "spi:<modalias>" even for SPI devices that were registered by OF. That means the OF module alias exported by MODULE_OF_TABLE(of,...) is currently not used and user-space has no way to autoload this module. But is still a good practice to add the OF module alias information into the kernel module even when currently is unused so once the SPI core is changed to report a correct OF modalias uevent, module autoloading will be working for this driver. >> work without a MODULE_DEVICE_TABLE(of,...). > > Brian > Best regards,
Hi Javier, On Fri, Aug 21, 2015 at 12:57:54AM +0200, Javier Martinez Canillas wrote: > On 08/21/2015 12:34 AM, Brian Norris wrote: > > On Fri, Aug 21, 2015 at 12:13:34AM +0200, Javier Martinez Canillas wrote: > >> So this patch really is a no-op right now and is not fixing anything. > >> As long as the driver has a MODULE_DEVICE_TABLE(spi,...), things will > > > > This driver does not have a MODULE_DEVICE_TABLE() at all, nor does it > > use spi_device_id. So, it is currently broken for the module use case. > > > > That is correct but also that does not mean that this patch will fix > module autoloading right now. It will though once the SPI core is > changed to report a OF type module alias. Ah, thanks for the patience. I missed the point that it will still be broken. > So what about something along this lines? > > mtd: dataflash: Export OF module alias information > > The SPI core currently reports the MODALIAS uevent as "spi:<modalias>" > even for SPI devices that were registered by OF. > > That means the OF module alias exported by MODULE_OF_TABLE(of,...) is > currently not used and user-space has no way to autoload this module. > > But is still a good practice to add the OF module alias information nit: s/is/it is/ > into the kernel module even when currently is unused so once the SPI ditto > core is changed to report a correct OF modalias uevent, module > autoloading will be working for this driver. Otherwise, looks good. I'll either patch in this commit message, or await v2. Your call. Regards, Brian
Hello Brian, On 08/22/2015 12:47 AM, Brian Norris wrote: > Hi Javier, > > On Fri, Aug 21, 2015 at 12:57:54AM +0200, Javier Martinez Canillas wrote: >> On 08/21/2015 12:34 AM, Brian Norris wrote: >>> On Fri, Aug 21, 2015 at 12:13:34AM +0200, Javier Martinez Canillas wrote: >>>> So this patch really is a no-op right now and is not fixing anything. >>>> As long as the driver has a MODULE_DEVICE_TABLE(spi,...), things will >>> >>> This driver does not have a MODULE_DEVICE_TABLE() at all, nor does it >>> use spi_device_id. So, it is currently broken for the module use case. >>> >> >> That is correct but also that does not mean that this patch will fix >> module autoloading right now. It will though once the SPI core is >> changed to report a OF type module alias. > > Ah, thanks for the patience. I missed the point that it will still be > broken. > No worries, I'm glad that we are on the same page now. >> So what about something along this lines? >> >> mtd: dataflash: Export OF module alias information >> >> The SPI core currently reports the MODALIAS uevent as "spi:<modalias>" >> even for SPI devices that were registered by OF. >> >> That means the OF module alias exported by MODULE_OF_TABLE(of,...) is >> currently not used and user-space has no way to autoload this module. >> >> But is still a good practice to add the OF module alias information > > nit: s/is/it is/ > >> into the kernel module even when currently is unused so once the SPI > > ditto > >> core is changed to report a correct OF modalias uevent, module >> autoloading will be working for this driver. > > Otherwise, looks good. I'll either patch in this commit message, or > await v2. Your call. > If you don't mind fixing it up yourself when applying, then that would be really helpful. Otherwise I can post a v2 on Monday. > Regards, > Brian > Best regards,
On Sat, Aug 22, 2015 at 02:26:14AM +0200, Javier Martinez Canillas wrote: > On 08/22/2015 12:47 AM, Brian Norris wrote: > > Otherwise, looks good. I'll either patch in this commit message, or > > await v2. Your call. > > If you don't mind fixing it up yourself when applying, then that > would be really helpful. Otherwise I can post a v2 on Monday. Queued to l2-mtd.git/next (for 4.4), with the edited commit message.
Hello Brian, On 08/22/2015 03:05 AM, Brian Norris wrote: > On Sat, Aug 22, 2015 at 02:26:14AM +0200, Javier Martinez Canillas wrote: >> On 08/22/2015 12:47 AM, Brian Norris wrote: >>> Otherwise, looks good. I'll either patch in this commit message, or >>> await v2. Your call. >> >> If you don't mind fixing it up yourself when applying, then that >> would be really helpful. Otherwise I can post a v2 on Monday. > > Queued to l2-mtd.git/next (for 4.4), with the edited commit message. > Great, thanks a lot for your help! Best regards,
diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c index 0099aba72a8b..df6f61137376 100644 --- a/drivers/mtd/devices/mtd_dataflash.c +++ b/drivers/mtd/devices/mtd_dataflash.c @@ -102,6 +102,7 @@ static const struct of_device_id dataflash_dt_ids[] = { { .compatible = "atmel,dataflash", }, { /* sentinel */ } }; +MODULE_DEVICE_TABLE(of, dataflash_dt_ids); #endif /* ......................................................................... */
The SPI core always reports the MODALIAS uevent as "spi:<modalias>" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table). So drivers needs to export the SPI id table and this be built into the module or udev won't have the necessary information to autoload the needed driver module when the device is added. But this means that OF-only drivers needs to have both OF and SPI id tables that have to be kept in sync and also the dev node compatible manufacturer prefix is stripped when reporting the MODALIAS. Which can lead to issues if two vendors use the same SPI device name for example. To avoid the above, the SPI core behavior may be changed in the future to not require an SPI device table for OF-only drivers and report the OF module alias. So, it's better to also export the OF table even when is unused now to prevent breaking module loading when the core changes. Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> --- drivers/mtd/devices/mtd_dataflash.c | 1 + 1 file changed, 1 insertion(+)