Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading

Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading

[Brian Norris]

+ others

On Sat, Oct 03, 2015 at 11:54:16PM +0200, Heiner Kallweit wrote:
> Seems like commit 43163022927b6e7d202a7e6f939c3f392465494d
> (allow arbitrary OF matching for "jedec,spi-nor") broke autoloading
> of the m25p80 module.
> MODALIAS is "spi:spi-nor" and removing "spi-nor" as device alias
> prevents module autoloading.
> 
> Of course we could revert the removal of the "spi-nor" device alias.
> However it might be better to switch to DT-based matching for
> DT-configured devices.
> 
> Adding a call to of_device_uevent_modalias to spi_uevent in spi.c
> solved the issue for me, drawback however is that just the first
> "compatible" value is used as modalias. In case of m25p80 this means
> that "jedec,spi-nor" has to be the first "compatible" value.
> This constraint might be too strict ..
> 
> Having said that I'm not sure what could be a better way to fix
> the issue than just re-introducing the "spi-nor" device alias.

Is this [1] getting fixed in SPI any time soon? Looks like there was
some progress [2], but AFAICT it's not completed.

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.

FWIW, I can do testing if somebody's tackling this still. Just CC me.

Regards,
Brian

[1] https://lkml.org/lkml/2014/9/11/458
[2] https://lkml.org/lkml/2015/8/20/109

spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)

[Brian Norris]

(Changing subject line, because apparently some people ignore mail if it
doesn't have 'SPI' in the subject line)

On Thu, Nov 12, 2015 at 10:59:26AM -0800, Brian Norris wrote:
> + others
> 
> On Sat, Oct 03, 2015 at 11:54:16PM +0200, Heiner Kallweit wrote:
> > Seems like commit 43163022927b6e7d202a7e6f939c3f392465494d
> > (allow arbitrary OF matching for "jedec,spi-nor") broke autoloading
> > of the m25p80 module.
> > MODALIAS is "spi:spi-nor" and removing "spi-nor" as device alias
> > prevents module autoloading.
> > 
> > Of course we could revert the removal of the "spi-nor" device alias.
> > However it might be better to switch to DT-based matching for
> > DT-configured devices.
> > 
> > Adding a call to of_device_uevent_modalias to spi_uevent in spi.c
> > solved the issue for me, drawback however is that just the first
> > "compatible" value is used as modalias. In case of m25p80 this means
> > that "jedec,spi-nor" has to be the first "compatible" value.
> > This constraint might be too strict ..
> > 
> > Having said that I'm not sure what could be a better way to fix
> > the issue than just re-introducing the "spi-nor" device alias.
> 
> Is this [1] getting fixed in SPI any time soon? Looks like there was
> some progress [2], but AFAICT it's not completed.
> 
> 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.
> 
> FWIW, I can do testing if somebody's tackling this still. Just CC me.
> 
> Regards,
> Brian
> 
> [1] https://lkml.org/lkml/2014/9/11/458
> [2] https://lkml.org/lkml/2015/8/20/109

Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)

[Brian Norris]

Hi Mark,

On Fri, Nov 13, 2015 at 10:12:28PM +0000, 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 devicetree@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 patch would break
these three cases.

Brian

Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4648 bytes --]

On Fri, Nov 13, 2015 at 02:51:13PM -0800, Brian Norris wrote:
> On Fri, Nov 13, 2015 at 10:12:28PM +0000, 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...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)

[Brian Norris]

Hi,

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

Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: spi: OF module autoloading is still broken

[Javier Martinez Canillas]

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

Re: spi: OF module autoloading is still broken

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: spi: OF module autoloading is still broken

[Javier Martinez Canillas]

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

Re: spi: OF module autoloading is still broken

[Javier Martinez Canillas]

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 +0000, 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 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.

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

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.

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

As I explained above, I don't believe these cases are part of the DT ABI.

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

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.

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

I remember reading older threads on which the DT maintainers said that
they were against wilcards so I don't think that is an option indeed.

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

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: spi: OF module autoloading is still broken

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: spi: OF module autoloading is still broken

[Javier Martinez Canillas]

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

Re: spi: OF module autoloading is still broken

[Brian Norris]

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.


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

> 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

Re: spi: OF module autoloading is still broken

[Javier Martinez Canillas]

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:

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

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,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: spi: OF module autoloading is still broken

[Brian Norris]

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

Re: spi: OF module autoloading is still broken

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: spi: OF module autoloading is still broken

[Brian Norris]

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

Re: spi: OF module autoloading is still broken

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: spi: OF module autoloading is still broken

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: spi: OF module autoloading is still broken

[Javier Martinez Canillas]

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

Re: spi: OF module autoloading is still broken

[Javier Martinez Canillas]

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
Open Source Group
Samsung Research America

Re: spi: OF module autoloading is still broken

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: spi: OF module autoloading is still broken

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 808 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: spi: OF module autoloading is still broken

[Javier Martinez Canillas]

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
Open Source Group
Samsung Research America