Re: spi: OF module autoloading is still broken
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/12d4.spi/spi_master/spi2/spi2.0/uevent DRIVER=cros-ec-spi OF_NAME=cros-ec OF_FULLNAME=/spi@12d4/cros-ec@0 OF_COMPATIBLE_0=google,cros-ec-spi OF_COMPATIBLE_N=1 MODALIAS=spi:cros-ec-spi MODALIAS=of:Ncros-ecTCgoogle,cros-ec-spi udevadm test /sys/devices/platform/12d4.spi/spi_master/spi2/spi2.0 ACTION=add DEVPATH=/devices/platform/12d4.spi/spi_master/spi2/spi2.0 DRIVER=cros-ec-spi MODALIAS=of:Ncros-ecTCgoogle,cros-ec-spi OF_COMPATIBLE_0=google,cros-ec-spi OF_COMPATIBLE_N=1 OF_FULLNAME=/spi@12d4/cros-ec@0 OF_NAME=cros-ec SUBSYSTEM=spi USEC_INITIALIZED=52732787 run: 'kmod load of:Ncros-ecTCgoogle,cros-ec-spi' 2) SPI modalias reported after OF modalias -- cat /sys/devices/platform/12d4.spi/spi_master/spi2/spi2.0/uevent DRIVER=cros-ec-spi OF_NAME=cros-ec OF_FULLNAME=/spi@12d4/cros-ec@0 OF_COMPATIBLE_0=google,cros-ec-spi OF_COMPATIBLE_N=1 MODALIAS=of:Ncros-ecTCgoogle,cros-ec-spi MODALIAS=spi:cros-ec-spi udevadm test /sys/devices/platform/12d4.spi/spi_master/spi2/spi2.0 ACTION=add DEVPATH=/devices/platform/12d4.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@12d4/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, -- Javier Martinez Canillas Open Source Group Samsung Research America -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: spi: OF module autoloading is still broken
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/12d4.spi/spi_master/spi2/spi2.0/uevent MODALIAS=spi:cros-ec-spi MODALIAS=of:Ncros-ecTCgoogle,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/12d4.spi/spi_master/spi2/spi2.0/uevent MODALIAS=of:Ncros-ecTCgoogle,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:, 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, -- Javier Martinez Canillas Open Source Group Samsung Research America -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: spi: OF module autoloading is still broken
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:NfooTCfoo,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, -- Javier Martinez Canillas Open Source Group Samsung Research America -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: spi: OF module autoloading is still broken
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, -- Javier Martinez Canillas Open Source Group Samsung Research America -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: spi: OF module autoloading is still broken
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:NfooTCfoo,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. signature.asc Description: PGP signature
Re: spi: OF module autoloading is still broken
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. signature.asc Description: PGP signature
Re: spi: OF module autoloading is still broken
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:NpmicTCacme,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, -- Javier Martinez Canillas Open Source Group Samsung Research America -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: spi: OF module autoloading is still broken
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:NfooTCfoo,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:NpmicTCacme,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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: spi: OF module autoloading is still broken
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:NfooTCfoo,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:NfooTCfoo,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:NpmicTCacme,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, -- Javier Martinez Canillas Open Source Group Samsung Research America -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: spi: OF module autoloading is still broken
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:NfooTCfoo,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:NfooTCfoo,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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: spi: OF module autoloading is still broken
Hello Brian, On 11/16/2015 04:24 PM, Brian Norris wrote: > Hi Javier, > > On Mon, Nov 16, 2015 at 02:19:27PM -0300, Javier Martinez Canillas wrote: >> On 11/13/2015 08:48 PM, Brian Norris wrote: >>> On Fri, Nov 13, 2015 at 11:14:10PM +, Mark Brown wrote: On Fri, Nov 13, 2015 at 02:51:13PM -0800, Brian Norris wrote: > >> Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt >> >> And doesn't have a list of compatible strings. It points to a file in >> the Linux kernel source tree (drivers/mtd/devices/m25p80.c) which IMHO >> is wrong since the bindings should be OS agnostic. So instead, a list >> of the valid compatible strings (with both manufacturer and model) >> should be documented there. > > Yep, that's a sore spot that I'm aware of. We had enough trouble sorting > out what "jedec,spi-nor" would be, and I never moved on to the point of > fixing up that comment. Will do that this week. > >> The fact that having compatible = "garbage,valid-model" or "valid-model" >> worked was just a fortunate event due how the SPI core currently works. > > I'd call that "unfortunate", and I agree with Mark. Implementation > matters more than documentation when talking about ABI. > > Right, by fortunate I meant "just working by luck" but it seems I had chosen the wrong words and I agree that the event is indeed unfortunate. >>> No "nor-jedec" -- that was an intermediate name that got replaced >>> mid-release-cycle due to some late DT review comments. >>> >> >> I think the comments in the m25p80 driver should be updated then since I >> had the same confusion when reading the spi_device_id table. > > Oops, thanks for pointing that out. That's old garbage that should be > cleaned up. Will patch that soon. > You are welcome. >>> But yes, I suppose adding "spi-nor" back to the spi_device_id table >>> fixes *one* of the immediate problems (i.e., 'compatible = >>> "jedec,spi-nor"'). That would cover Heiner's report. But it doesn't >>> solve: >>> >>> compatible = "vendor,shiny-new-device", "jedec,spi-nor" >>> >>> I believe that the latter is sometimes the Right Way (TM) to do things >>> for device tree, so you have a fallback if auto-probing "jedec,spi-nor" >>> ever doesn't suffice. >>> >>> (This came up in Heiner's original post: "In case of m25p80 this means >>> that "jedec,spi-nor" has to be the first "compatible" value. This >>> constraint might be too strict ..") >>> >> >> I don't believe Heiner's statement is correct or maybe I misunderstood how >> module alias is reported for OF based platforms. But IIRC what happens is >> that the of_device_get_modalias() concatenates all the compatible strings >> that are present in the OF node. > > Heiner was only talking about the existing SPI core code, which doesn't > use of_device_get_modalias(). > OK. >> So in this particular example the reported modalias would be: >> >> of:Nshiny-new-deviceTCvendor,shiny-new-deviceCjedec,spi-nor >> >> and since the modaliases that will be stored in the module would be: >> >> alias: of:N*T*Cjedec,spi-nor* >> >> the latter will match the former since all compatible strings are in the >> reported modalias and the of_device_id .name was not set so is a wilcard. >> >> If there is also a "vendor,shiny-new-device", then the aliases would be: >> >> alias: of:N*T*Cvendor,shiny-new-device* >> alias: of:N*T*Cjedec,spi-nor* >> >> so of:N*T*Cvendor,shiny-new-device* will also match >> of:Nshiny-new-deviceTCvendor,shiny-new-deviceCjedec,spi-nor >> >> That covers the two use cases for valid compatible strings AFAICT and DT >> using invalid compatible strings should not be tried to be supported IMHO. > > But it doesn't cover cases like this: > > compatible = "vendor,m25p80"; > > which today yield uevent/modalias: > > spi:m25p80 > > and will match with m25p80.c's spi_device_id table (and therefore will > autoload). Your patch will change this to: > > of:N*T*vendor,m25p80* > > and unless I go and add "vendor,m25p80" to m25p80's of_match_table as > well, then this will NOT autoload. But, see how this can't be extended > to wildcard matches? So I think your patch requires a bit more thought > and care, or else you will break a lot more than you think. > You are absolutely right, I have a script that should had found this case (DT in mainline that are relying on the SPI device ID table to match a model used in a compatible string) but it seems my script has some bugs since it didn't find the IDs for this driver. 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:NfooTCfoo,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 ---
Re: spi: OF module autoloading is still broken
Hi Javier, On Mon, Nov 16, 2015 at 02:19:27PM -0300, Javier Martinez Canillas wrote: > On 11/13/2015 08:48 PM, Brian Norris wrote: > > On Fri, Nov 13, 2015 at 11:14:10PM +, Mark Brown wrote: > >> On Fri, Nov 13, 2015 at 02:51:13PM -0800, Brian Norris wrote: > Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt > > And doesn't have a list of compatible strings. It points to a file in > the Linux kernel source tree (drivers/mtd/devices/m25p80.c) which IMHO > is wrong since the bindings should be OS agnostic. So instead, a list > of the valid compatible strings (with both manufacturer and model) > should be documented there. Yep, that's a sore spot that I'm aware of. We had enough trouble sorting out what "jedec,spi-nor" would be, and I never moved on to the point of fixing up that comment. Will do that this week. > The fact that having compatible = "garbage,valid-model" or "valid-model" > worked was just a fortunate event due how the SPI core currently works. I'd call that "unfortunate", and I agree with Mark. Implementation matters more than documentation when talking about ABI. > > No "nor-jedec" -- that was an intermediate name that got replaced > > mid-release-cycle due to some late DT review comments. > > > > I think the comments in the m25p80 driver should be updated then since I > had the same confusion when reading the spi_device_id table. Oops, thanks for pointing that out. That's old garbage that should be cleaned up. Will patch that soon. > > But yes, I suppose adding "spi-nor" back to the spi_device_id table > > fixes *one* of the immediate problems (i.e., 'compatible = > > "jedec,spi-nor"'). That would cover Heiner's report. But it doesn't > > solve: > > > > compatible = "vendor,shiny-new-device", "jedec,spi-nor" > > > > I believe that the latter is sometimes the Right Way (TM) to do things > > for device tree, so you have a fallback if auto-probing "jedec,spi-nor" > > ever doesn't suffice. > > > > (This came up in Heiner's original post: "In case of m25p80 this means > > that "jedec,spi-nor" has to be the first "compatible" value. This > > constraint might be too strict ..") > > > > I don't believe Heiner's statement is correct or maybe I misunderstood how > module alias is reported for OF based platforms. But IIRC what happens is > that the of_device_get_modalias() concatenates all the compatible strings > that are present in the OF node. Heiner was only talking about the existing SPI core code, which doesn't use of_device_get_modalias(). > So in this particular example the reported modalias would be: > > of:Nshiny-new-deviceTCvendor,shiny-new-deviceCjedec,spi-nor > > and since the modaliases that will be stored in the module would be: > > alias: of:N*T*Cjedec,spi-nor* > > the latter will match the former since all compatible strings are in the > reported modalias and the of_device_id .name was not set so is a wilcard. > > If there is also a "vendor,shiny-new-device", then the aliases would be: > > alias: of:N*T*Cvendor,shiny-new-device* > alias: of:N*T*Cjedec,spi-nor* > > so of:N*T*Cvendor,shiny-new-device* will also match > of:Nshiny-new-deviceTCvendor,shiny-new-deviceCjedec,spi-nor > > That covers the two use cases for valid compatible strings AFAICT and DT > using invalid compatible strings should not be tried to be supported IMHO. But it doesn't cover cases like this: compatible = "vendor,m25p80"; which today yield uevent/modalias: spi:m25p80 and will match with m25p80.c's spi_device_id table (and therefore will autoload). Your patch will change this to: of:N*T*vendor,m25p80* and unless I go and add "vendor,m25p80" to m25p80's of_match_table as well, then this will NOT autoload. But, see how this can't be extended to wildcard matches? So I think your patch requires a bit more thought and care, or else you will break a lot more than you think. > I will of course add a comment to my patch explaining what could break when > the SPI core is modified to report a proper OF modalias > but I don't think we > should try to maintain FDTs that were not doing the right thing with regard > to using wrong and undocumented compatible strings. I don't think you have problems only with bad FDTs. I think you have a problem with perfectly valid DTs. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: spi: OF module autoloading is still broken
Hello Mark, On 11/16/2015 02:51 PM, Mark Brown wrote: > On Mon, Nov 16, 2015 at 02:26:49PM -0300, Javier Martinez Canillas wrote: >> On 11/16/2015 10:53 AM, Mark Brown wrote: > >>> What I don't really understand here is why we've decided to push all >>> this stuff into the subsystems, it seems like if we're managing to do >>> the matching based on the compatible we really ought to be able to have >>> the core figure out the uevents for us too. I need to go have a look at >>> that... > >> There is already a set of generic OF uevents that are reported by the core >> but IIUC those are not used by udev. Please take a look to of_device_uevent() >> in drivers/of/device.c and dev_uevent() that calls it in drivers/base/core.c. > > I know what the current situation is, my point is that I don't > understand why we would choose to do that ("What I don't really > understand here..."). > Got it, sorry for misunderstanding you. I also don't know the answer FWIW. Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: spi: OF module autoloading is still broken
Hello Mark, On 11/16/2015 02:49 PM, Mark Brown wrote: > On Mon, Nov 16, 2015 at 02:19:27PM -0300, Javier Martinez Canillas wrote: >> On 11/13/2015 08:48 PM, Brian Norris wrote: > >>> (I believe I avoided this in the first place for mostly-aesthetic >>> reasons; technically this allows people to put garbage in their DT, like >>> "garbage,spi-nor". It's unclear whether "garbage" becomes part of the >>> mythical DT ABI [1].) > >> I don't believe your examples are part of the mythical DT ABI. What I >> understand is that an ABI is whatever is documented in the DT binding >> docs but the only document that mentions the m25p80 is: > >> Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt > > Not really, in practice an ABI is something that people notice breaking. > This means that if enough people ship an undocumented ABI (or it goes > into important enough products) it's just as good as something that's > documented, perhaps better than something that's documented and nobody > ever uses as an ABI. > I see, fair enough. Let's see what Brian say about the spi-nor case and I'll also post my RFC patch but as a proper patch and adding the comments you asked me later today. It would be unfortunate if the SPI drivers would have as a requirement to always have an SPI device ID table even for OF-only IPs but I don't think that is that bad either. Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: spi: OF module autoloading is still broken
On Mon, Nov 16, 2015 at 02:26:49PM -0300, Javier Martinez Canillas wrote: > On 11/16/2015 10:53 AM, Mark Brown wrote: > > What I don't really understand here is why we've decided to push all > > this stuff into the subsystems, it seems like if we're managing to do > > the matching based on the compatible we really ought to be able to have > > the core figure out the uevents for us too. I need to go have a look at > > that... > There is already a set of generic OF uevents that are reported by the core > but IIUC those are not used by udev. Please take a look to of_device_uevent() > in drivers/of/device.c and dev_uevent() that calls it in drivers/base/core.c. I know what the current situation is, my point is that I don't understand why we would choose to do that ("What I don't really understand here..."). signature.asc Description: PGP signature
Re: spi: OF module autoloading is still broken
On Mon, Nov 16, 2015 at 02:19:27PM -0300, Javier Martinez Canillas wrote: > On 11/13/2015 08:48 PM, Brian Norris wrote: > > (I believe I avoided this in the first place for mostly-aesthetic > > reasons; technically this allows people to put garbage in their DT, like > > "garbage,spi-nor". It's unclear whether "garbage" becomes part of the > > mythical DT ABI [1].) > I don't believe your examples are part of the mythical DT ABI. What I > understand is that an ABI is whatever is documented in the DT binding > docs but the only document that mentions the m25p80 is: > Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt Not really, in practice an ABI is something that people notice breaking. This means that if enough people ship an undocumented ABI (or it goes into important enough products) it's just as good as something that's documented, perhaps better than something that's documented and nobody ever uses as an ABI. signature.asc Description: PGP signature
Re: spi: OF module autoloading is still broken
Hello Mark, On 11/16/2015 10:53 AM, Mark Brown wrote: > On Fri, Nov 13, 2015 at 03:48:57PM -0800, Brian Norris wrote: > >> I suspect we'll have to fully support both spi_device_id tables (fully >> supported already; if nothing else, to keep wildcard matching) and >> of_match_tables (not fully supported for module loading), and in some >> cases, the two will have to stay partially in sync. > > What I don't really understand here is why we've decided to push all > this stuff into the subsystems, it seems like if we're managing to do > the matching based on the compatible we really ought to be able to have > the core figure out the uevents for us too. I need to go have a look at > that... > There is already a set of generic OF uevents that are reported by the core but IIUC those are not used by udev. Please take a look to of_device_uevent() in drivers/of/device.c and dev_uevent() that calls it in drivers/base/core.c. Now, if the different struct bus_type .uevent handlers could be factored out to have a common helper or be deleted completely and handled by the core instead, that is a very good question. To be honest I haven't looked at this possibility and I'm not that familiar with the device model. But in any case I believe that modifying spi_uevent() to behave as other subsystems and properly report an OF based modalias is a step in the right direction. We can later identify the common logic and move all the bus_type modalias reporting to the core as a follow up IMHO. But of course if up to you to decide :) Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: spi: OF module autoloading is still broken
Hello Brian and Mark, Sorry for the delay, I was quite busy at the end of last week and didn't have time to look at my email closely. On 11/13/2015 08:48 PM, Brian Norris wrote: > Hi, > > On Fri, Nov 13, 2015 at 11:14:10PM +, Mark Brown wrote: >> On Fri, Nov 13, 2015 at 02:51:13PM -0800, Brian Norris wrote: >> >>> General problem: >>> >> >>> The SPI core doesn't use the OF compatible property for generating >>> uevent/modalias, and therefore can't autoload modules based on the full >>> compatible property of a device. It *only* can use the 'modalias', which >>> is a castrated version of the compatible property -- it only includes >>> part of the 1st entry in 'compatible'. >> >>> This forces SPI device drivers to use spi_device_id tables even when >>> they might be better suited for of_match_tables. >> That's correct, the series mentioned by Brian was meant to fix all the SPI drivers in mainline and the RFC patch changed spi_uevent() to report an OF modalias if the SPI device was registered through OF. I said that I would post it once all the fixes for SPI drivers landed. The patches made it to 4.4-rc1 so I'll repost it (addressing Mark's comments) targeting 4.5. >> Well, I don't actually see this as that bad a thing - it's good practice >> to include the Linux ID tables even if you also support DT since not all >> the world is DT. > Agreed if both DT and board files are supported but if the driver is for an IP that is only present in DT-only platforms, then there is point for a SPI ID table IMHO. > I suppose so, but that's still not the whole story. > > (I believe I avoided this in the first place for mostly-aesthetic > reasons; technically this allows people to put garbage in their DT, like > "garbage,spi-nor". It's unclear whether "garbage" becomes part of the > mythical DT ABI [1].) > I don't believe your examples are part of the mythical DT ABI. What I understand is that an ABI is whatever is documented in the DT binding docs but the only document that mentions the m25p80 is: Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt And doesn't have a list of compatible strings. It points to a file in the Linux kernel source tree (drivers/mtd/devices/m25p80.c) which IMHO is wrong since the bindings should be OS agnostic. So instead, a list of the valid compatible strings (with both manufacturer and model) should be documented there. But even that document says: - compatible : May include a device-specific string consisting of the manufacturer and name of the chip. So clearly a DT that is using a compatible string that doesn't have a valid and documented manufacturer and model, is not following the ABI. The fact that having compatible = "garbage,valid-model" or "valid-model" worked was just a fortunate event due how the SPI core currently works. >>> Specifics for m25p80: >>> = >> >>> We support many flash devices and have traditionally been doing so by >>> simply adding more entries to the spi_device_id table. Recently, we have >>> tried to get away from adding new entries and aliases for every single >>> variation by instead supporting a common OF match: "jedec,spi-nor". So >>> we might expect to see nodes like this: >> >>> flash@xxx { >>> compatible = "vendor,shiny-new-device", "jedec,spi-nor"; >>> ... >>> }; >> >>> We may or may not add "shiny-new-device" to the spi_device_id array. But >>> "jedec,spi-nor" should be sufficient to load the driver and check if the >>> READ ID string matches any known flash. If "shiny-new-device" isn't in >>> the spi_device_id array, then we don't get module autoloading. >> >> OK, so you're trying to do dynamic enumeration? Then you don't want >> specific things in any of the ID tables since you'll match it yourself >> at runtime (which is obviously good). > > Well, we do have to support existing cases (e.g., existing device trees > without "jedec,spi-nor") so we have to keep some around. But otherwise, > mostly yes. > Agreed, both "jedec,spi-nor" and the compatible for the devices that don't support the JEDEC READ ID opcode should be in the OF ID table. >>> There's also the case of omitting "vendor,shiny-new-device" entirely, >>> which is probably a little more dangerous, but still legal (and also >>> won't autoload modules): >> >>> flash@xxx { >>> compatible = "jedec,spi-nor"; >>> ... >>> }; >> This case will also be fixed by my patch modifying spi_uevent (more on that later). >> My immediate thought is that I'd expect to see spi-nor and (based on a >> quick scan of the m25p80 driver) nor-jedec to appear in the spi_device_id Agreed. >> table since regardless of what happens with Javier's patch we want the >> autoprobing mechanism to work for board file based platforms too >> (there's a bunch of architectures that still use them). That'd also >> have the side effect of solving your immediate problem I think? > > No "nor-jedec" -- that was
Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)
On Fri, Nov 13, 2015 at 03:48:57PM -0800, Brian Norris wrote: > I suspect we'll have to fully support both spi_device_id tables (fully > supported already; if nothing else, to keep wildcard matching) and > of_match_tables (not fully supported for module loading), and in some > cases, the two will have to stay partially in sync. What I don't really understand here is why we've decided to push all this stuff into the subsystems, it seems like if we're managing to do the matching based on the compatible we really ought to be able to have the core figure out the uevents for us too. I need to go have a look at that... signature.asc Description: PGP signature
Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)
Hi, On Fri, Nov 13, 2015 at 11:14:10PM +, Mark Brown wrote: > On Fri, Nov 13, 2015 at 02:51:13PM -0800, Brian Norris wrote: > > > General problem: > > > > > The SPI core doesn't use the OF compatible property for generating > > uevent/modalias, and therefore can't autoload modules based on the full > > compatible property of a device. It *only* can use the 'modalias', which > > is a castrated version of the compatible property -- it only includes > > part of the 1st entry in 'compatible'. > > > This forces SPI device drivers to use spi_device_id tables even when > > they might be better suited for of_match_tables. > > Well, I don't actually see this as that bad a thing - it's good practice > to include the Linux ID tables even if you also support DT since not all > the world is DT. I suppose so, but that's still not the whole story. (I believe I avoided this in the first place for mostly-aesthetic reasons; technically this allows people to put garbage in their DT, like "garbage,spi-nor". It's unclear whether "garbage" becomes part of the mythical DT ABI [1].) > > Specifics for m25p80: > > = > > > We support many flash devices and have traditionally been doing so by > > simply adding more entries to the spi_device_id table. Recently, we have > > tried to get away from adding new entries and aliases for every single > > variation by instead supporting a common OF match: "jedec,spi-nor". So > > we might expect to see nodes like this: > > > flash@xxx { > > compatible = "vendor,shiny-new-device", "jedec,spi-nor"; > > ... > > }; > > > We may or may not add "shiny-new-device" to the spi_device_id array. But > > "jedec,spi-nor" should be sufficient to load the driver and check if the > > READ ID string matches any known flash. If "shiny-new-device" isn't in > > the spi_device_id array, then we don't get module autoloading. > > OK, so you're trying to do dynamic enumeration? Then you don't want > specific things in any of the ID tables since you'll match it yourself > at runtime (which is obviously good). Well, we do have to support existing cases (e.g., existing device trees without "jedec,spi-nor") so we have to keep some around. But otherwise, mostly yes. > > There's also the case of omitting "vendor,shiny-new-device" entirely, > > which is probably a little more dangerous, but still legal (and also > > won't autoload modules): > > > flash@xxx { > > compatible = "jedec,spi-nor"; > > ... > > }; > > My immediate thought is that I'd expect to see spi-nor and (based on a > quick scan of the m25p80 driver) nor-jedec to appear in the spi_device_id > table since regardless of what happens with Javier's patch we want the > autoprobing mechanism to work for board file based platforms too > (there's a bunch of architectures that still use them). That'd also > have the side effect of solving your immediate problem I think? No "nor-jedec" -- that was an intermediate name that got replaced mid-release-cycle due to some late DT review comments. But yes, I suppose adding "spi-nor" back to the spi_device_id table fixes *one* of the immediate problems (i.e., 'compatible = "jedec,spi-nor"'). That would cover Heiner's report. But it doesn't solve: compatible = "vendor,shiny-new-device", "jedec,spi-nor" I believe that the latter is sometimes the Right Way (TM) to do things for device tree, so you have a fallback if auto-probing "jedec,spi-nor" ever doesn't suffice. (This came up in Heiner's original post: "In case of m25p80 this means that "jedec,spi-nor" has to be the first "compatible" value. This constraint might be too strict ..") > [Snip example with three different prefixes for m25p80 in compatible > strings] > > > All three are supported by SPI's current modalias code, and so are part > > of the ABI. Thus, m25p80.c will always contain both a spi_device_id > > table and an of_match_table. But I think Javier's patch would break > > these three cases. > > Right, IIRC I think that sort of thing was what I was looking for in > documentation for his patch. Now you mention it I'm not sure we can do > wildcarding with DT which is a bit unfortunate for cases like this. Yeah, I expect wildcards are a no-go. > Hrm. Not sure and it's getting late on a Friday night... :) I suspect we'll have to fully support both spi_device_id tables (fully supported already; if nothing else, to keep wildcard matching) and of_match_tables (not fully supported for module loading), and in some cases, the two will have to stay partially in sync. Brian [1] "Device Tree as a stable ABI: a fairy tale?" http://free-electrons.com/pub/conferences/2015/elc/petazzoni-dt-as-stable-abi-fairy-tale/petazzoni-dt-as-stable-abi-fairy-tale.pdf -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-inf
Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)
On Fri, Nov 13, 2015 at 02:51:13PM -0800, Brian Norris wrote: > On Fri, Nov 13, 2015 at 10:12:28PM +, Mark Brown wrote: > > On Fri, Nov 13, 2015 at 11:40:31AM -0800, Brian Norris wrote: > > > (Changing subject line, because apparently some people ignore mail if it > > > doesn't have 'SPI' in the subject line) > > Well, if you mean me I'm getting CCed on such a large number of large > > threads about MTD patches that only have relevance to SPI in that > > they're for a driver that uses SPI that I pretty delete a very large > > proportion of mail that looks like it's about MTD patch unread I'm > > afraid. It's almost all completely irrelevant and uninteresting to me. > I understand, but I'm not sure how to fix that. In some cases, it's > somewhat unavoidable, since there are series that need (or at least, > think they need) upgrades to SPI infrastructure in order to support new > things in MTD. But that's rare, and most of the time, people are just > CC'ing anything and anyone that looks relevant. Those I'm less worried about. It's the serieses that have no SPI content at all that get a bit much. > Any suggestions are welcome. I'll try to discourage it when I notice. > I'm not sure documentation helps, unless we can find something people > actually read. And tooling doesn't exactly help, since > scripts/get_maintainer.pl already doesn't suggest you or linux-spi@ for > any of the drivers/mtd/spi-nor/ or drivers/mtd/devices/m25p80.c. I get the impression a lot of it is "I once copied some vaugely related patch set to these people, I'll add them to this one too" and that it's mostly just about education. I supposed I should write some boiler plate to send to people, I've not > General problem: > > The SPI core doesn't use the OF compatible property for generating > uevent/modalias, and therefore can't autoload modules based on the full > compatible property of a device. It *only* can use the 'modalias', which > is a castrated version of the compatible property -- it only includes > part of the 1st entry in 'compatible'. > This forces SPI device drivers to use spi_device_id tables even when > they might be better suited for of_match_tables. Well, I don't actually see this as that bad a thing - it's good practice to include the Linux ID tables even if you also support DT since not all the world is DT. > Specifics for m25p80: > = > We support many flash devices and have traditionally been doing so by > simply adding more entries to the spi_device_id table. Recently, we have > tried to get away from adding new entries and aliases for every single > variation by instead supporting a common OF match: "jedec,spi-nor". So > we might expect to see nodes like this: > flash@xxx { > compatible = "vendor,shiny-new-device", "jedec,spi-nor"; > ... > }; > We may or may not add "shiny-new-device" to the spi_device_id array. But > "jedec,spi-nor" should be sufficient to load the driver and check if the > READ ID string matches any known flash. If "shiny-new-device" isn't in > the spi_device_id array, then we don't get module autoloading. OK, so you're trying to do dynamic enumeration? Then you don't want specific things in any of the ID tables since you'll match it yourself at runtime (which is obviously good). > There's also the case of omitting "vendor,shiny-new-device" entirely, > which is probably a little more dangerous, but still legal (and also > won't autoload modules): > flash@xxx { > compatible = "jedec,spi-nor"; > ... > }; My immediate thought is that I'd expect to see spi-nor and (based on a quick scan of the m25p80 driver) nor-jedec to appear in the spi_device_id table since regardless of what happens with Javier's patch we want the autoprobing mechanism to work for board file based platforms too (there's a bunch of architectures that still use them). That'd also have the side effect of solving your immediate problem I think? [Snip example with three different prefixes for m25p80 in compatible strings] > All three are supported by SPI's current modalias code, and so are part > of the ABI. Thus, m25p80.c will always contain both a spi_device_id > table and an of_match_table. But I think Javier's patch would break > these three cases. Right, IIRC I think that sort of thing was what I was looking for in documentation for his patch. Now you mention it I'm not sure we can do wildcarding with DT which is a bit unfortunate for cases like this. Hrm. Not sure and it's getting late on a Friday night... signature.asc Description: PGP signature
Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)
Hi Mark, On Fri, Nov 13, 2015 at 10:12:28PM +, Mark Brown wrote: > On Fri, Nov 13, 2015 at 11:40:31AM -0800, Brian Norris wrote: > > > (Changing subject line, because apparently some people ignore mail if it > > doesn't have 'SPI' in the subject line) > > Well, if you mean me I'm getting CCed on such a large number of large > threads about MTD patches that only have relevance to SPI in that > they're for a driver that uses SPI that I pretty delete a very large > proportion of mail that looks like it's about MTD patch unread I'm > afraid. It's almost all completely irrelevant and uninteresting to me. I understand, but I'm not sure how to fix that. In some cases, it's somewhat unavoidable, since there are series that need (or at least, think they need) upgrades to SPI infrastructure in order to support new things in MTD. But that's rare, and most of the time, people are just CC'ing anything and anyone that looks relevant. Any suggestions are welcome. I'll try to discourage it when I notice. I'm not sure documentation helps, unless we can find something people actually read. And tooling doesn't exactly help, since scripts/get_maintainer.pl already doesn't suggest you or linux-spi@ for any of the drivers/mtd/spi-nor/ or drivers/mtd/devices/m25p80.c. I feel bad for anyone on devicet...@vger.kernel.org for similar reasons, BTW. But I guess that's a product of their own decisions. See #2 in Documentation/devicetree/bindings/submitting-patches.txt. > > > Is this [1] getting fixed in SPI any time soon? Looks like there was > > > some progress [2], but AFAICT it's not completed. > > Please include human readable descriptions of things like commits IDs > and issues being discussed in e-mail in your mails, this makes them much > easier for humans to read especially when they have no internet access. > I do frequently catch up on my mail on flights or while otherwise > travelling so this is even more pressing for me than just being about > making things a bit easier to read. Sorry, I suppose I could have summarized a bit. But I didn't want to copy-and-paste the whole thing, and Javier's work pretty clearly explains the problem. > > > I'd just like to know what the way forward here should be for m25p80. > > > Really, "jedec,spi-nor" never autoloaded modules very reliably because > > > of the SPI core constaints. So I'm not sure I'd consider this a > > > regression, and I might be OK waiting around if it'll be fixed in a > > > reasonable time frame. > > Someone will need to tell me what the actual problem is for m25p80 > before I can understand what the way forward might be. From a brief > scan through of the thread it looks like if Javier's series solves the > problem it needs a bit more analysis and/or a clearer presentation and > probably a resubmit. General problem: The SPI core doesn't use the OF compatible property for generating uevent/modalias, and therefore can't autoload modules based on the full compatible property of a device. It *only* can use the 'modalias', which is a castrated version of the compatible property -- it only includes part of the 1st entry in 'compatible'. This forces SPI device drivers to use spi_device_id tables even when they might be better suited for of_match_tables. Specifics for m25p80: = We support many flash devices and have traditionally been doing so by simply adding more entries to the spi_device_id table. Recently, we have tried to get away from adding new entries and aliases for every single variation by instead supporting a common OF match: "jedec,spi-nor". So we might expect to see nodes like this: flash@xxx { compatible = "vendor,shiny-new-device", "jedec,spi-nor"; ... }; We may or may not add "shiny-new-device" to the spi_device_id array. But "jedec,spi-nor" should be sufficient to load the driver and check if the READ ID string matches any known flash. If "shiny-new-device" isn't in the spi_device_id array, then we don't get module autoloading. There's also the case of omitting "vendor,shiny-new-device" entirely, which is probably a little more dangerous, but still legal (and also won't autoload modules): flash@xxx { compatible = "jedec,spi-nor"; ... }; Addendum: = (This isn't the core problem I'm worried about, but I believe it serves as commentary on Javier's patch:) Cases like this are possible and should be considered: flash@xxx { compatible = "m25p80"; ... }; flash@xxx { compatible = "st,m25p80"; ... }; flash@xxx { compatible = "something-nonsensical,m25p80"; ... }; All three are supported by SPI's current modalias code, and so are part of the ABI. Thus, m25p80.c will always contain both a spi_device_id table and an of_match_table. But I think Javier's pa
Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)
On Fri, Nov 13, 2015 at 11:40:31AM -0800, Brian Norris wrote: > (Changing subject line, because apparently some people ignore mail if it > doesn't have 'SPI' in the subject line) Well, if you mean me I'm getting CCed on such a large number of large threads about MTD patches that only have relevance to SPI in that they're for a driver that uses SPI that I pretty delete a very large proportion of mail that looks like it's about MTD patch unread I'm afraid. It's almost all completely irrelevant and uninteresting to me. > > Is this [1] getting fixed in SPI any time soon? Looks like there was > > some progress [2], but AFAICT it's not completed. Please include human readable descriptions of things like commits IDs and issues being discussed in e-mail in your mails, this makes them much easier for humans to read especially when they have no internet access. I do frequently catch up on my mail on flights or while otherwise travelling so this is even more pressing for me than just being about making things a bit easier to read. > > I'd just like to know what the way forward here should be for m25p80. > > Really, "jedec,spi-nor" never autoloaded modules very reliably because > > of the SPI core constaints. So I'm not sure I'd consider this a > > regression, and I might be OK waiting around if it'll be fixed in a > > reasonable time frame. Someone will need to tell me what the actual problem is for m25p80 before I can understand what the way forward might be. From a brief scan through of the thread it looks like if Javier's series solves the problem it needs a bit more analysis and/or a clearer presentation and probably a resubmit. signature.asc Description: PGP signature