spi: OF module autoloading is still broken

Commit Message

Javier Martinez Canillas Nov. 16, 2015, 8 p.m. UTC

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 +0000, 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-deviceT<NULL>Cvendor,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-deviceT<NULL>Cvendor,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:NfooT<NULL>Cfoo,barbar ?

So I think we need this regardless of my patch:


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?

Thanks a lot for pointing out all these issues. It is indeed harder than
I thought.

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

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

Best regards,

Comments

Brian Norris Nov. 16, 2015, 8:47 p.m. UTC | #1

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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Javier Martinez Canillas Nov. 16, 2015, 9:32 p.m. UTC | #2

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,

Brian Norris Nov. 16, 2015, 9:51 p.m. UTC | #3

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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Javier Martinez Canillas Nov. 17, 2015, 1:14 p.m. UTC | #4

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,

Mark Brown Nov. 17, 2015, 1:19 p.m. UTC | #5

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.

Mark Brown Nov. 17, 2015, 1:34 p.m. UTC | #6

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.

Javier Martinez Canillas Nov. 17, 2015, 1:36 p.m. UTC | #7

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 Nov. 17, 2015, 1:38 p.m. UTC | #8

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,

Javier Martinez Canillas Nov. 18, 2015, 8:07 p.m. UTC | #9

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,

Javier Martinez Canillas Nov. 19, 2015, 12:47 p.m. UTC | #10

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,

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