| Message ID | 564A35EB.5080008@osg.samsung.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Brian Norris |
| Headers | show |
Hi, On Mon, Nov 16, 2015 at 05:00:43PM -0300, Javier Martinez Canillas wrote: > On 11/16/2015 04:24 PM, Brian Norris wrote: > I also didn't think about wilcards... I wonder why there are trailing > wildcards for a compatible string. After all a compatible string should > define a particular IP and there could be a foo,bar and foo,barbaz that > have different drivers and what prevents today the driver with alias > of:N*T*Cfoo,bar* to be loaded due a MODALIAS=of:NfooT<NULL>Cfoo,barbar ? > > So I think we need this regardless of my patch: > > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c > index 5b96206e9aab..cd0002f4a199 100644 > --- a/scripts/mod/file2alias.c > +++ b/scripts/mod/file2alias.c > @@ -704,7 +704,6 @@ static int do_of_entry (const char *filename, void *symval, char *alias) > if (isspace (*tmp)) > *tmp = '_'; > > - add_wildcard(alias); > return 1; > } > ADD_TO_DEVTABLE("of", of_device_id, do_of_entry); (I'm also not an expert in this stuff, but...) That looks reasonable. You might refer to commit ac551828993e ("modpost: i2c aliases need no trailing wildcard") for inspiration. You might also modify the "always" in: /* Always end in a wildcard, for future extension */ static inline void add_wildcard(char *str) { ... } And of course, you probably should CC those who are responsible for the core device tree probing and device/driver interactions for something like this. > Now that I think about it, there is another issue and is that today spi:foo > defines a namespace while changing to of: will make the namespace flat so > a platform driver that has the same vendor and model will have the same > modalias. > > IOW, for board files will be platform:bar and i2c:bar while for OF will be > of:NfooT<NULL>Cfoo,bar in both cases. I wonder if we should reuse the type > for that and store the subsystem prefix there. What do you think? I'm not sure I understand all the issues here to be able to comment properly. But I bet someone else might. (For me, it might help if you had a more concrete example to speak of.) > Thanks a lot for pointing out all these issues. It is indeed harder than > I thought. No problem! > > I don't think you have problems only with bad FDTs. I think you have a > > problem with perfectly valid DTs. > > > > Agreed, I wonder if spi_uevent() shouldn't be changed to report both the > OF and old SPI modaliases to avoid breaking a lot of drivers but at the > same time allowing DT-only drivers to not need an SPI ID table. That's the solution I imagined, though I haven't tested it yet. I don't see much precedent for reporting multiple modaliases, so there could be some kind of ABI issues around that too. Regards, Brian
Hello Brian, On 11/16/2015 05:47 PM, Brian Norris wrote: > Hi, > > On Mon, Nov 16, 2015 at 05:00:43PM -0300, Javier Martinez Canillas wrote: >> On 11/16/2015 04:24 PM, Brian Norris wrote: > >> I also didn't think about wilcards... I wonder why there are trailing >> wildcards for a compatible string. After all a compatible string should >> define a particular IP and there could be a foo,bar and foo,barbaz that >> have different drivers and what prevents today the driver with alias >> of:N*T*Cfoo,bar* to be loaded due a MODALIAS=of:NfooT<NULL>Cfoo,barbar ? >> >> So I think we need this regardless of my patch: >> >> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c >> index 5b96206e9aab..cd0002f4a199 100644 >> --- a/scripts/mod/file2alias.c >> +++ b/scripts/mod/file2alias.c >> @@ -704,7 +704,6 @@ static int do_of_entry (const char *filename, void *symval, char *alias) >> if (isspace (*tmp)) >> *tmp = '_'; >> >> - add_wildcard(alias); >> return 1; >> } >> ADD_TO_DEVTABLE("of", of_device_id, do_of_entry); > > (I'm also not an expert in this stuff, but...) That looks reasonable. > You might refer to commit ac551828993e ("modpost: i2c aliases need no Yes, that's exactly the commit I looked to come up with the above diff. > trailing wildcard") for inspiration. You might also modify the "always" > in: > > /* Always end in a wildcard, for future extension */ > static inline void add_wildcard(char *str) > { > ... > } Indeed. > > And of course, you probably should CC those who are responsible for the > core device tree probing and device/driver interactions for something > like this. > Sure. >> Now that I think about it, there is another issue and is that today spi:foo >> defines a namespace while changing to of: will make the namespace flat so >> a platform driver that has the same vendor and model will have the same >> modalias. >> >> IOW, for board files will be platform:bar and i2c:bar while for OF will be >> of:NfooT<NULL>Cfoo,bar in both cases. I wonder if we should reuse the type >> for that and store the subsystem prefix there. What do you think? > > I'm not sure I understand all the issues here to be able to comment > properly. But I bet someone else might. > > (For me, it might help if you had a more concrete example to speak of.) > From a quick look I couldn't find a real example (that doesn't mean there isn't one) but I'll make one just to explain the problem. Let's suppose you have 3 different IP's blocks (i.e: pmics) from the same vendor. The IP's are quite similar but only differ in that use a different bus to communicate with the SoC. So you could have a core driver and transport drivers for SPI and I2C. So currently you could use the not too creative compatible strings compatible string "acme,my-pmic" in all the drivers and is not a problem because the SPI and I2C subsystem will always report the MODALIAS uevent as: MODALIAS=i2c:my-pmic and MODALIAS=spi:my-pmic so as far as there is a "my-pmic" entry in the SPI and I2C id tables, module autoload will work and the match will also work since that happens per bus_type. But if SPI and I2C are migrated to OF modalias reporting, then both I2C and SPI will report (assuming the device node is called pmic in both cases): MODALIAS=of:NpmicT<NULL>Cacme,my-pmic That's what I meant when said that the modalias namespace is flat in the case of OF but is separated in the case of board files and the current implementation. What currently the drivers are doing is to name the model my-pmic-{i2c,spi,etc} but I think that the subsystem information should be explicitly present in the OF modalias information as it is for legacy device registration. >> Thanks a lot for pointing out all these issues. It is indeed harder than >> I thought. > > No problem! > >>> I don't think you have problems only with bad FDTs. I think you have a >>> problem with perfectly valid DTs. >>> >> >> Agreed, I wonder if spi_uevent() shouldn't be changed to report both the >> OF and old SPI modaliases to avoid breaking a lot of drivers but at the >> same time allowing DT-only drivers to not need an SPI ID table. > > That's the solution I imagined, though I haven't tested it yet. I don't > see much precedent for reporting multiple modaliases, so there could be > some kind of ABI issues around that too. > Ok, I'm glad that we agree. This definitely needs to be discussed with more people. I'll re-spin my patch and post a v2 reporting multiple modaliases tomorrow after testing. > Regards, > Brian > Best regards,
Hi Javier, On Mon, Nov 16, 2015 at 06:32:22PM -0300, Javier Martinez Canillas wrote: > On 11/16/2015 05:47 PM, Brian Norris wrote: > > On Mon, Nov 16, 2015 at 05:00:43PM -0300, Javier Martinez Canillas wrote: > >> Now that I think about it, there is another issue and is that today spi:foo > >> defines a namespace while changing to of: will make the namespace flat so > >> a platform driver that has the same vendor and model will have the same > >> modalias. > >> > >> IOW, for board files will be platform:bar and i2c:bar while for OF will be > >> of:NfooT<NULL>Cfoo,bar in both cases. I wonder if we should reuse the type > >> for that and store the subsystem prefix there. What do you think? > > > > I'm not sure I understand all the issues here to be able to comment > > properly. But I bet someone else might. > > > > (For me, it might help if you had a more concrete example to speak of.) > > > > From a quick look I couldn't find a real example (that doesn't mean there > isn't one) but I'll make one just to explain the problem. > > Let's suppose you have 3 different IP's blocks (i.e: pmics) from the same > vendor. The IP's are quite similar but only differ in that use a different > bus to communicate with the SoC. Ah, I thought that's what you might have meant, but then I reread enough times that I confused myself. I think my first understanding was correct :) > So you could have a core driver and transport drivers for SPI and I2C. > > So currently you could use the not too creative compatible strings compatible > string "acme,my-pmic" in all the drivers and is not a problem because the SPI > and I2C subsystem will always report the MODALIAS uevent as: > > MODALIAS=i2c:my-pmic and MODALIAS=spi:my-pmic > > so as far as there is a "my-pmic" entry in the SPI and I2C id tables, module > autoload will work and the match will also work since that happens per bus_type. > > But if SPI and I2C are migrated to OF modalias reporting, then both I2C and SPI > will report (assuming the device node is called pmic in both cases): > > MODALIAS=of:NpmicT<NULL>Cacme,my-pmic > > That's what I meant when said that the modalias namespace is flat in the case of > OF but is separated in the case of board files and the current implementation. Thanks for the additional explanation. > What currently the drivers are doing is to name the model my-pmic-{i2c,spi,etc} > but I think that the subsystem information should be explicitly present in the > OF modalias information as it is for legacy device registration. Lest someone else wonder whether this is theoretical or not, I'll save them the work in pointing at an example: "st,st33zp24". See: Documentation/devicetree/bindings/security/tpm/st33zp24-*.txt and the code is in drivers/char/tpm/st33zp24/, sharing the same core library, suggesting that the devices really are the same except simply the bus. In my limited opinion, then, it seems like a good idea to still try to separate the bus namespaces when reporting module-loading information. Brian
Hello Brian, On 11/16/2015 06:51 PM, Brian Norris wrote: > On Mon, Nov 16, 2015 at 06:32:22PM -0300, Javier Martinez Canillas wrote: [snip] >> >> Let's suppose you have 3 different IP's blocks (i.e: pmics) from the same >> vendor. The IP's are quite similar but only differ in that use a different >> bus to communicate with the SoC. > > Ah, I thought that's what you might have meant, but then I reread enough > times that I confused myself. I think my first understanding was correct > :) > :) >> So you could have a core driver and transport drivers for SPI and I2C. >> >> So currently you could use the not too creative compatible strings compatible >> string "acme,my-pmic" in all the drivers and is not a problem because the SPI >> and I2C subsystem will always report the MODALIAS uevent as: >> >> MODALIAS=i2c:my-pmic and MODALIAS=spi:my-pmic >> >> so as far as there is a "my-pmic" entry in the SPI and I2C id tables, module >> autoload will work and the match will also work since that happens per bus_type. >> >> But if SPI and I2C are migrated to OF modalias reporting, then both I2C and SPI >> will report (assuming the device node is called pmic in both cases): >> >> MODALIAS=of:NpmicT<NULL>Cacme,my-pmic >> >> That's what I meant when said that the modalias namespace is flat in the case of >> OF but is separated in the case of board files and the current implementation. > > Thanks for the additional explanation. > You are welcome. >> What currently the drivers are doing is to name the model my-pmic-{i2c,spi,etc} >> but I think that the subsystem information should be explicitly present in the >> OF modalias information as it is for legacy device registration. > > Lest someone else wonder whether this is theoretical or not, I'll save > them the work in pointing at an example: "st,st33zp24". See: > > Documentation/devicetree/bindings/security/tpm/st33zp24-*.txt > > and the code is in drivers/char/tpm/st33zp24/, sharing the same core > library, suggesting that the devices really are the same except simply > the bus. > Thanks for pointing out that example although for that specific case, the drivers' compatible are "st,st33zp24-i2c" and "st,st33zp24-spi" to avoid the issue explained before. Another example is Documentation/devicetree/bindings/mfd/cros-ec.txt and code in drivers/mfd/cros_ec* where the EC is the same and all the logic is in the core but only the transport bus changes for each driver. Compatible strings again are using IP + bus: "google,cros-ec-i2c" "google,cros-ec-spi" I still didn't find an example where the same compatible string is used for different drivers (i.e: "st,st33zp24" or "google,cros-ec") but the fact that is possible for legacy and not for OF is worrisome. > In my limited opinion, then, it seems like a good idea to still try to > separate the bus namespaces when reporting module-loading information. > Yes, I'll add it to my TODO list since this is orthogonal to the SPI discussion, for example this could also be a problem for platform drivers (i.e: MODALIAS=platform:bar vs of:N*T*Cfoo,bar). > Brian > Best regards,
On Tue, Nov 17, 2015 at 10:14:27AM -0300, Javier Martinez Canillas wrote: > On 11/16/2015 06:51 PM, Brian Norris wrote: > > Lest someone else wonder whether this is theoretical or not, I'll save > > them the work in pointing at an example: "st,st33zp24". See: > > Documentation/devicetree/bindings/security/tpm/st33zp24-*.txt > > and the code is in drivers/char/tpm/st33zp24/, sharing the same core > > library, suggesting that the devices really are the same except simply > > the bus. > Thanks for pointing out that example although for that specific case, > the drivers' compatible are "st,st33zp24-i2c" and "st,st33zp24-spi" to > avoid the issue explained before. Eew, that's gross. > I still didn't find an example where the same compatible string is > used for different drivers (i.e: "st,st33zp24" or "google,cros-ec") > but the fact that is possible for legacy and not for OF is worrisome. There's a bunch of audio CODEC and PMIC drivers, arizona is the first example that springs to mind but it's very common to have mixed signal devices devices which can run in both I2C and SPI modes.
On Mon, Nov 16, 2015 at 05:00:43PM -0300, Javier Martinez Canillas wrote: > Now that I think about it, there is another issue and is that today spi:foo > defines a namespace while changing to of: will make the namespace flat so > a platform driver that has the same vendor and model will have the same > modalias. > IOW, for board files will be platform:bar and i2c:bar while for OF will be > of:NfooT<NULL>Cfoo,bar in both cases. I wonder if we should reuse the type > for that and store the subsystem prefix there. What do you think? I'm not sure that's a big issue - if we end up loading an extra module with a second bus glue it'll waste a little memory but it's not going to be a huge amount. Obviously it'd be nice to fix but it doesn't seem super important compared to getting the modules loaded.
Hello Mark, On 11/17/2015 10:19 AM, Mark Brown wrote: > On Tue, Nov 17, 2015 at 10:14:27AM -0300, Javier Martinez Canillas wrote: >> On 11/16/2015 06:51 PM, Brian Norris wrote: > >>> Lest someone else wonder whether this is theoretical or not, I'll save >>> them the work in pointing at an example: "st,st33zp24". See: > >>> Documentation/devicetree/bindings/security/tpm/st33zp24-*.txt > >>> and the code is in drivers/char/tpm/st33zp24/, sharing the same core >>> library, suggesting that the devices really are the same except simply >>> the bus. > >> Thanks for pointing out that example although for that specific case, >> the drivers' compatible are "st,st33zp24-i2c" and "st,st33zp24-spi" to >> avoid the issue explained before. > > Eew, that's gross. > Well, I'm not the author of the driver but I've seen many drivers doing the same so I believe the reason is to avoid the issue explained before. >> I still didn't find an example where the same compatible string is >> used for different drivers (i.e: "st,st33zp24" or "google,cros-ec") >> but the fact that is possible for legacy and not for OF is worrisome. > > There's a bunch of audio CODEC and PMIC drivers, arizona is the first > example that springs to mind but it's very common to have mixed signal > devices devices which can run in both I2C and SPI modes. > Thanks a lot for the examples, I just looked at the arizona MFD drivers and indeed the same OF device ID table is used for both the SPI and I2C drivers. Best regards,
Hello Mark, On 11/17/2015 10:34 AM, Mark Brown wrote: > On Mon, Nov 16, 2015 at 05:00:43PM -0300, Javier Martinez Canillas wrote: > >> Now that I think about it, there is another issue and is that today spi:foo >> defines a namespace while changing to of: will make the namespace flat so >> a platform driver that has the same vendor and model will have the same >> modalias. > >> IOW, for board files will be platform:bar and i2c:bar while for OF will be >> of:NfooT<NULL>Cfoo,bar in both cases. I wonder if we should reuse the type >> for that and store the subsystem prefix there. What do you think? > > I'm not sure that's a big issue - if we end up loading an extra module > with a second bus glue it'll waste a little memory but it's not going to > be a huge amount. Obviously it'd be nice to fix but it doesn't seem > super important compared to getting the modules loaded. > Agreed, it has indeed lower priority. That's why I added it to my TODO list so it can be fixed later as a follow up. Best regards,
Hello Brian and Mark, On 11/16/2015 06:32 PM, Javier Martinez Canillas wrote: > On 11/16/2015 05:47 PM, Brian Norris wrote: >> On Mon, Nov 16, 2015 at 05:00:43PM -0300, Javier Martinez Canillas wrote: >>> On 11/16/2015 04:24 PM, Brian Norris wrote: [snip] >>>> I don't think you have problems only with bad FDTs. I think you have a >>>> problem with perfectly valid DTs. >>>> >>> >>> Agreed, I wonder if spi_uevent() shouldn't be changed to report both the >>> OF and old SPI modaliases to avoid breaking a lot of drivers but at the >>> same time allowing DT-only drivers to not need an SPI ID table. >> >> That's the solution I imagined, though I haven't tested it yet. I don't >> see much precedent for reporting multiple modaliases, so there could be >> some kind of ABI issues around that too. >> > > Ok, I'm glad that we agree. This definitely needs to be discussed with more > people. I'll re-spin my patch and post a v2 reporting multiple modaliases > tomorrow after testing. > So I had some time today to test this approach but unfortunately it seems this workaround will not be possible because AFAICT kmod only takes into account the last MODALIAS reported as a uevent. I still have to check the kmod source code but that's my conclusion from trying to report both aliases. When looking at the uevent sysfs entry for the device, I see that both aliases are reported but if only a OF alias is built into the module, then kmod does not auto load the module unless the OF MODALIAS is the last one reported, i.e: # grep MODALIAS /sys/devices/platform/12d40000.spi/spi_master/spi2/spi2.0/uevent MODALIAS=spi:cros-ec-spi MODALIAS=of:Ncros-ecT<NULL>Cgoogle,cros-ec-spi # modinfo cros_ec_spi | grep alias alias: of:N*T*Cgoogle,cros-ec-spi* If I invert the order on which the uevent are reported, then the module is not autoloaded, i.e: # grep MODALIAS /sys/devices/platform/12d40000.spi/spi_master/spi2/spi2.0/uevent MODALIAS=of:Ncros-ecT<NULL>Cgoogle,cros-ec-spi MODALIAS=spi:cros-ec-spi # modinfo cros_ec_spi | grep alias alias: of:N*T*Cgoogle,cros-ec-spi* In this case only is loaded if the module has a spi:<alias>, i.e: # modinfo cros_ec_spi | grep alias alias: of:N*T*Cgoogle,cros-ec-spi* alias: spi:cros-ec-spi IOW, even when is possible to report more than one MODALIAS, user-space seems to only use the last one reported so using both don't work. Of course kmod can be changed to check for more than one MODALIAS but since the kernel should not break old user-space, the fact that a single MODALIAS was reported seems to be an ABI now due how is used. Best regards,
Hello, On 11/18/2015 05:07 PM, Javier Martinez Canillas wrote: > Hello Brian and Mark, > > On 11/16/2015 06:32 PM, Javier Martinez Canillas wrote: >> On 11/16/2015 05:47 PM, Brian Norris wrote: >>> On Mon, Nov 16, 2015 at 05:00:43PM -0300, Javier Martinez Canillas wrote: >>>> On 11/16/2015 04:24 PM, Brian Norris wrote: > > [snip] > >>>>> I don't think you have problems only with bad FDTs. I think you have a >>>>> problem with perfectly valid DTs. >>>>> >>>> >>>> Agreed, I wonder if spi_uevent() shouldn't be changed to report both the >>>> OF and old SPI modaliases to avoid breaking a lot of drivers but at the >>>> same time allowing DT-only drivers to not need an SPI ID table. >>> >>> That's the solution I imagined, though I haven't tested it yet. I don't >>> see much precedent for reporting multiple modaliases, so there could be >>> some kind of ABI issues around that too. >>> >> >> Ok, I'm glad that we agree. This definitely needs to be discussed with more >> people. I'll re-spin my patch and post a v2 reporting multiple modaliases >> tomorrow after testing. >> > > So I had some time today to test this approach but unfortunately it seems > this workaround will not be possible because AFAICT kmod only takes into > account the last MODALIAS reported as a uevent. I still have to check the > kmod source code but that's my conclusion from trying to report both aliases. > I dig a little more on this and is udev and not kmod that can't cope with more than one MODALIAS, kmod just use the alias that udev tells it to use. 1) OF modalias reported after SPI modalias ------------------------------------------ cat /sys/devices/platform/12d40000.spi/spi_master/spi2/spi2.0/uevent DRIVER=cros-ec-spi OF_NAME=cros-ec OF_FULLNAME=/spi@12d40000/cros-ec@0 OF_COMPATIBLE_0=google,cros-ec-spi OF_COMPATIBLE_N=1 MODALIAS=spi:cros-ec-spi MODALIAS=of:Ncros-ecT<NULL>Cgoogle,cros-ec-spi udevadm test /sys/devices/platform/12d40000.spi/spi_master/spi2/spi2.0 ACTION=add DEVPATH=/devices/platform/12d40000.spi/spi_master/spi2/spi2.0 DRIVER=cros-ec-spi MODALIAS=of:Ncros-ecT<NULL>Cgoogle,cros-ec-spi OF_COMPATIBLE_0=google,cros-ec-spi OF_COMPATIBLE_N=1 OF_FULLNAME=/spi@12d40000/cros-ec@0 OF_NAME=cros-ec SUBSYSTEM=spi USEC_INITIALIZED=52732787 run: 'kmod load of:Ncros-ecT<NULL>Cgoogle,cros-ec-spi' 2) SPI modalias reported after OF modalias ------------------------------------------ cat /sys/devices/platform/12d40000.spi/spi_master/spi2/spi2.0/uevent DRIVER=cros-ec-spi OF_NAME=cros-ec OF_FULLNAME=/spi@12d40000/cros-ec@0 OF_COMPATIBLE_0=google,cros-ec-spi OF_COMPATIBLE_N=1 MODALIAS=of:Ncros-ecT<NULL>Cgoogle,cros-ec-spi MODALIAS=spi:cros-ec-spi udevadm test /sys/devices/platform/12d40000.spi/spi_master/spi2/spi2.0 ACTION=add DEVPATH=/devices/platform/12d40000.spi/spi_master/spi2/spi2.0 DRIVER=cros-ec-spi MODALIAS=spi:cros-ec-spi OF_COMPATIBLE_0=google,cros-ec-spi OF_COMPATIBLE_N=1 OF_FULLNAME=/spi@12d40000/cros-ec@0 OF_NAME=cros-ec SUBSYSTEM=spi USEC_INITIALIZED=288316488 run: 'kmod load spi:cros-ec-spi' So as you can see: 'kmod load $modalias' is only called for the last modalias. > > IOW, even when is possible to report more than one MODALIAS, user-space seems > to only use the last one reported so using both don't work. > > Of course kmod can be changed to check for more than one MODALIAS but since > the kernel should not break old user-space, the fact that a single MODALIAS > was reported seems to be an ABI now due how is used. > So I think our options are: a) Not change spi_uevent() to report an OF modalias and make a requirement to have a spi_device_id table even for OF-only to have autoload working. Regardless of the option, SPI ID tables should be present in drivers so these are supported in non-OF platforms as Mark said. b) Make sure that all OF drivers have a complete OF table with all entries in the SPI ID table before spi_uevent() is modified to report OF modaliases. My preference would be b) so the same table (OF or SPI ID) can be used for alias filling that match reported modalias and driver to device matching and will also be aligned with what other subsystems do. I'll check my script to see why the drivers/mtd/devices/m25p80.c was not found, likely because the entries in the spi_device_id table weren't in the DT binding doc before. Best regards,
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c index 5b96206e9aab..cd0002f4a199 100644 --- a/scripts/mod/file2alias.c +++ b/scripts/mod/file2alias.c @@ -704,7 +704,6 @@ static int do_of_entry (const char *filename, void *symval, char *alias) if (isspace (*tmp)) *tmp = '_'; - add_wildcard(alias); return 1; } ADD_TO_DEVTABLE("of", of_device_id, do_of_entry);