Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)
On Fri, Nov 13, 2015 at 03:48:57PM -0800, Brian Norris wrote: > I suspect we'll have to fully support both spi_device_id tables (fully > supported already; if nothing else, to keep wildcard matching) and > of_match_tables (not fully supported for module loading), and in some > cases, the two will have to stay partially in sync. What I don't really understand here is why we've decided to push all this stuff into the subsystems, it seems like if we're managing to do the matching based on the compatible we really ought to be able to have the core figure out the uevents for us too. I need to go have a look at that... signature.asc Description: PGP signature
Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)
Hi,
On Fri, Nov 13, 2015 at 11:14:10PM +, Mark Brown wrote:
> On Fri, Nov 13, 2015 at 02:51:13PM -0800, Brian Norris wrote:
>
> > General problem:
> >
>
> > The SPI core doesn't use the OF compatible property for generating
> > uevent/modalias, and therefore can't autoload modules based on the full
> > compatible property of a device. It *only* can use the 'modalias', which
> > is a castrated version of the compatible property -- it only includes
> > part of the 1st entry in 'compatible'.
>
> > This forces SPI device drivers to use spi_device_id tables even when
> > they might be better suited for of_match_tables.
>
> Well, I don't actually see this as that bad a thing - it's good practice
> to include the Linux ID tables even if you also support DT since not all
> the world is DT.
I suppose so, but that's still not the whole story.
(I believe I avoided this in the first place for mostly-aesthetic
reasons; technically this allows people to put garbage in their DT, like
"garbage,spi-nor". It's unclear whether "garbage" becomes part of the
mythical DT ABI [1].)
> > Specifics for m25p80:
> > =
>
> > We support many flash devices and have traditionally been doing so by
> > simply adding more entries to the spi_device_id table. Recently, we have
> > tried to get away from adding new entries and aliases for every single
> > variation by instead supporting a common OF match: "jedec,spi-nor". So
> > we might expect to see nodes like this:
>
> > flash@xxx {
> > compatible = "vendor,shiny-new-device", "jedec,spi-nor";
> > ...
> > };
>
> > We may or may not add "shiny-new-device" to the spi_device_id array. But
> > "jedec,spi-nor" should be sufficient to load the driver and check if the
> > READ ID string matches any known flash. If "shiny-new-device" isn't in
> > the spi_device_id array, then we don't get module autoloading.
>
> OK, so you're trying to do dynamic enumeration? Then you don't want
> specific things in any of the ID tables since you'll match it yourself
> at runtime (which is obviously good).
Well, we do have to support existing cases (e.g., existing device trees
without "jedec,spi-nor") so we have to keep some around. But otherwise,
mostly yes.
> > There's also the case of omitting "vendor,shiny-new-device" entirely,
> > which is probably a little more dangerous, but still legal (and also
> > won't autoload modules):
>
> > flash@xxx {
> > compatible = "jedec,spi-nor";
> > ...
> > };
>
> My immediate thought is that I'd expect to see spi-nor and (based on a
> quick scan of the m25p80 driver) nor-jedec to appear in the spi_device_id
> table since regardless of what happens with Javier's patch we want the
> autoprobing mechanism to work for board file based platforms too
> (there's a bunch of architectures that still use them). That'd also
> have the side effect of solving your immediate problem I think?
No "nor-jedec" -- that was an intermediate name that got replaced
mid-release-cycle due to some late DT review comments.
But yes, I suppose adding "spi-nor" back to the spi_device_id table
fixes *one* of the immediate problems (i.e., 'compatible =
"jedec,spi-nor"'). That would cover Heiner's report. But it doesn't
solve:
compatible = "vendor,shiny-new-device", "jedec,spi-nor"
I believe that the latter is sometimes the Right Way (TM) to do things
for device tree, so you have a fallback if auto-probing "jedec,spi-nor"
ever doesn't suffice.
(This came up in Heiner's original post: "In case of m25p80 this means
that "jedec,spi-nor" has to be the first "compatible" value. This
constraint might be too strict ..")
> [Snip example with three different prefixes for m25p80 in compatible
> strings]
>
> > All three are supported by SPI's current modalias code, and so are part
> > of the ABI. Thus, m25p80.c will always contain both a spi_device_id
> > table and an of_match_table. But I think Javier's patch would break
> > these three cases.
>
> Right, IIRC I think that sort of thing was what I was looking for in
> documentation for his patch. Now you mention it I'm not sure we can do
> wildcarding with DT which is a bit unfortunate for cases like this.
Yeah, I expect wildcards are a no-go.
> Hrm. Not sure and it's getting late on a Friday night...
:)
I suspect we'll have to fully support both spi_device_id tables (fully
supported already; if nothing else, to keep wildcard matching) and
of_match_tables (not fully supported for module loading), and in some
cases, the two will have to stay partially in sync.
Brian
[1] "Device Tree as a stable ABI: a fairy tale?"
http://free-electrons.com/pub/conferences/2015/elc/petazzoni-dt-as-stable-abi-fairy-tale/petazzoni-dt-as-stable-abi-fairy-tale.pdf
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-inf
Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)
On Fri, Nov 13, 2015 at 02:51:13PM -0800, Brian Norris wrote:
> On Fri, Nov 13, 2015 at 10:12:28PM +, Mark Brown wrote:
> > On Fri, Nov 13, 2015 at 11:40:31AM -0800, Brian Norris wrote:
> > > (Changing subject line, because apparently some people ignore mail if it
> > > doesn't have 'SPI' in the subject line)
> > Well, if you mean me I'm getting CCed on such a large number of large
> > threads about MTD patches that only have relevance to SPI in that
> > they're for a driver that uses SPI that I pretty delete a very large
> > proportion of mail that looks like it's about MTD patch unread I'm
> > afraid. It's almost all completely irrelevant and uninteresting to me.
> I understand, but I'm not sure how to fix that. In some cases, it's
> somewhat unavoidable, since there are series that need (or at least,
> think they need) upgrades to SPI infrastructure in order to support new
> things in MTD. But that's rare, and most of the time, people are just
> CC'ing anything and anyone that looks relevant.
Those I'm less worried about. It's the serieses that have no SPI
content at all that get a bit much.
> Any suggestions are welcome. I'll try to discourage it when I notice.
> I'm not sure documentation helps, unless we can find something people
> actually read. And tooling doesn't exactly help, since
> scripts/get_maintainer.pl already doesn't suggest you or linux-spi@ for
> any of the drivers/mtd/spi-nor/ or drivers/mtd/devices/m25p80.c.
I get the impression a lot of it is "I once copied some vaugely related
patch set to these people, I'll add them to this one too" and that it's
mostly just about education. I supposed I should write some boiler
plate to send to people, I've not
> General problem:
>
> The SPI core doesn't use the OF compatible property for generating
> uevent/modalias, and therefore can't autoload modules based on the full
> compatible property of a device. It *only* can use the 'modalias', which
> is a castrated version of the compatible property -- it only includes
> part of the 1st entry in 'compatible'.
> This forces SPI device drivers to use spi_device_id tables even when
> they might be better suited for of_match_tables.
Well, I don't actually see this as that bad a thing - it's good practice
to include the Linux ID tables even if you also support DT since not all
the world is DT.
> Specifics for m25p80:
> =
> We support many flash devices and have traditionally been doing so by
> simply adding more entries to the spi_device_id table. Recently, we have
> tried to get away from adding new entries and aliases for every single
> variation by instead supporting a common OF match: "jedec,spi-nor". So
> we might expect to see nodes like this:
> flash@xxx {
> compatible = "vendor,shiny-new-device", "jedec,spi-nor";
> ...
> };
> We may or may not add "shiny-new-device" to the spi_device_id array. But
> "jedec,spi-nor" should be sufficient to load the driver and check if the
> READ ID string matches any known flash. If "shiny-new-device" isn't in
> the spi_device_id array, then we don't get module autoloading.
OK, so you're trying to do dynamic enumeration? Then you don't want
specific things in any of the ID tables since you'll match it yourself
at runtime (which is obviously good).
> There's also the case of omitting "vendor,shiny-new-device" entirely,
> which is probably a little more dangerous, but still legal (and also
> won't autoload modules):
> flash@xxx {
> compatible = "jedec,spi-nor";
> ...
> };
My immediate thought is that I'd expect to see spi-nor and (based on a
quick scan of the m25p80 driver) nor-jedec to appear in the spi_device_id
table since regardless of what happens with Javier's patch we want the
autoprobing mechanism to work for board file based platforms too
(there's a bunch of architectures that still use them). That'd also
have the side effect of solving your immediate problem I think?
[Snip example with three different prefixes for m25p80 in compatible
strings]
> All three are supported by SPI's current modalias code, and so are part
> of the ABI. Thus, m25p80.c will always contain both a spi_device_id
> table and an of_match_table. But I think Javier's patch would break
> these three cases.
Right, IIRC I think that sort of thing was what I was looking for in
documentation for his patch. Now you mention it I'm not sure we can do
wildcarding with DT which is a bit unfortunate for cases like this.
Hrm. Not sure and it's getting late on a Friday night...
signature.asc
Description: PGP signature
Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)
Hi Mark,
On Fri, Nov 13, 2015 at 10:12:28PM +, Mark Brown wrote:
> On Fri, Nov 13, 2015 at 11:40:31AM -0800, Brian Norris wrote:
>
> > (Changing subject line, because apparently some people ignore mail if it
> > doesn't have 'SPI' in the subject line)
>
> Well, if you mean me I'm getting CCed on such a large number of large
> threads about MTD patches that only have relevance to SPI in that
> they're for a driver that uses SPI that I pretty delete a very large
> proportion of mail that looks like it's about MTD patch unread I'm
> afraid. It's almost all completely irrelevant and uninteresting to me.
I understand, but I'm not sure how to fix that. In some cases, it's
somewhat unavoidable, since there are series that need (or at least,
think they need) upgrades to SPI infrastructure in order to support new
things in MTD. But that's rare, and most of the time, people are just
CC'ing anything and anyone that looks relevant.
Any suggestions are welcome. I'll try to discourage it when I notice.
I'm not sure documentation helps, unless we can find something people
actually read. And tooling doesn't exactly help, since
scripts/get_maintainer.pl already doesn't suggest you or linux-spi@ for
any of the drivers/mtd/spi-nor/ or drivers/mtd/devices/m25p80.c.
I feel bad for anyone on devicet...@vger.kernel.org for similar reasons,
BTW. But I guess that's a product of their own decisions. See #2 in
Documentation/devicetree/bindings/submitting-patches.txt.
> > > Is this [1] getting fixed in SPI any time soon? Looks like there was
> > > some progress [2], but AFAICT it's not completed.
>
> Please include human readable descriptions of things like commits IDs
> and issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.
Sorry, I suppose I could have summarized a bit. But I didn't want to
copy-and-paste the whole thing, and Javier's work pretty clearly
explains the problem.
> > > I'd just like to know what the way forward here should be for m25p80.
> > > Really, "jedec,spi-nor" never autoloaded modules very reliably because
> > > of the SPI core constaints. So I'm not sure I'd consider this a
> > > regression, and I might be OK waiting around if it'll be fixed in a
> > > reasonable time frame.
>
> Someone will need to tell me what the actual problem is for m25p80
> before I can understand what the way forward might be. From a brief
> scan through of the thread it looks like if Javier's series solves the
> problem it needs a bit more analysis and/or a clearer presentation and
> probably a resubmit.
General problem:
The SPI core doesn't use the OF compatible property for generating
uevent/modalias, and therefore can't autoload modules based on the full
compatible property of a device. It *only* can use the 'modalias', which
is a castrated version of the compatible property -- it only includes
part of the 1st entry in 'compatible'.
This forces SPI device drivers to use spi_device_id tables even when
they might be better suited for of_match_tables.
Specifics for m25p80:
=
We support many flash devices and have traditionally been doing so by
simply adding more entries to the spi_device_id table. Recently, we have
tried to get away from adding new entries and aliases for every single
variation by instead supporting a common OF match: "jedec,spi-nor". So
we might expect to see nodes like this:
flash@xxx {
compatible = "vendor,shiny-new-device", "jedec,spi-nor";
...
};
We may or may not add "shiny-new-device" to the spi_device_id array. But
"jedec,spi-nor" should be sufficient to load the driver and check if the
READ ID string matches any known flash. If "shiny-new-device" isn't in
the spi_device_id array, then we don't get module autoloading.
There's also the case of omitting "vendor,shiny-new-device" entirely,
which is probably a little more dangerous, but still legal (and also
won't autoload modules):
flash@xxx {
compatible = "jedec,spi-nor";
...
};
Addendum:
=
(This isn't the core problem I'm worried about, but I believe it serves
as commentary on Javier's patch:)
Cases like this are possible and should be considered:
flash@xxx {
compatible = "m25p80";
...
};
flash@xxx {
compatible = "st,m25p80";
...
};
flash@xxx {
compatible = "something-nonsensical,m25p80";
...
};
All three are supported by SPI's current modalias code, and so are part
of the ABI. Thus, m25p80.c will always contain both a spi_device_id
table and an of_match_table. But I think Javier's pa
Re: spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading)
On Fri, Nov 13, 2015 at 11:40:31AM -0800, Brian Norris wrote: > (Changing subject line, because apparently some people ignore mail if it > doesn't have 'SPI' in the subject line) Well, if you mean me I'm getting CCed on such a large number of large threads about MTD patches that only have relevance to SPI in that they're for a driver that uses SPI that I pretty delete a very large proportion of mail that looks like it's about MTD patch unread I'm afraid. It's almost all completely irrelevant and uninteresting to me. > > Is this [1] getting fixed in SPI any time soon? Looks like there was > > some progress [2], but AFAICT it's not completed. Please include human readable descriptions of things like commits IDs and issues being discussed in e-mail in your mails, this makes them much easier for humans to read especially when they have no internet access. I do frequently catch up on my mail on flights or while otherwise travelling so this is even more pressing for me than just being about making things a bit easier to read. > > I'd just like to know what the way forward here should be for m25p80. > > Really, "jedec,spi-nor" never autoloaded modules very reliably because > > of the SPI core constaints. So I'm not sure I'd consider this a > > regression, and I might be OK waiting around if it'll be fixed in a > > reasonable time frame. Someone will need to tell me what the actual problem is for m25p80 before I can understand what the way forward might be. From a brief scan through of the thread it looks like if Javier's series solves the problem it needs a bit more analysis and/or a clearer presentation and probably a resubmit. signature.asc Description: PGP signature
