| Message ID | 1382540779-6334-1-git-send-email-treding@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2013-10-23 at 17:06 +0200, Thierry Reding wrote: > + /* check if binding is experimental */ > + if (dev != device || drv != driver) { > + pr_warn("of: device %s (%s) uses an experimental binding\n", > + np->name, np->full_name); > + In the discussions earlier I think we decided that this should set a taint flag too. If you've built a kernel with CONFIG_OF_EXPERIMENTAL (which I think we were calling CONFIG_UNSTABLE_DT) then you have no expectation that it will boot tomorrow, although it might work with your DTB today.
On 10/23/2013 04:06 PM, Thierry Reding wrote: > Past and recent discussions have shown that there's some concensus that > device tree bindings should be considered an ABI and therefore need to > remain backwards-compatible once code to implement them is included in > the final release of a Linux kernel. > > At the same time there is a desire to keep some manoeuvre while we're > trying to figure things out. The fact is that many of us lack the > experience to design good bindings from the start. At the same time > there is a shortage of people that can review bindings and help design > better ones. > > Progress and the addition of new features (and restoration of features > that used to work before the advent of DT for that matter) are blocked > to a large degree because of that. > > This patch attempts to restore some degree of freedom by introducing an > easy way to mark device tree bindings as experimental as well as a way > for users to disable the use of experimental bindings if they choose > functionality at the cost of potential device tree incompatibilities. > > Bindings are marked experimental by prefixing the compatible value with > an exclamation mark (!). In order to make it clear that experimental > bindings are undesirable in the long run, a warning will be output when > an experimental binding is encountered. > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig > +static int of_compat_match(const char *device, const char *driver, > + const struct device_node *np) > +{ > + const char *dev = device; > + const char *drv = driver; > + > + if (device[0] == '!') > + device++; > + > + if (driver[0] == '!') > + driver++; > + > + if (of_compat_cmp(device, driver, strlen(driver)) != 0) > + return 0; Do we really want to polute the drivers and DT files with a ! in the compatible values? I thought we'd considered that, but chosen having the drivers that use unstable bindings depend on a Kconfig option as an alternative, not an additional step? The one issue with doing this is that if a binding is thought to be unstable, but becomes stable later without any changes, we'll have to do busy-work to remove the ! in all the DT files, thus artificially introducing an incompatibility. Perhaps that's fine though?
On Wed, Oct 23, 2013 at 05:05:32PM +0100, David Woodhouse wrote: > On Wed, 2013-10-23 at 17:06 +0200, Thierry Reding wrote: > > + /* check if binding is experimental */ > > + if (dev != device || drv != driver) { > > + pr_warn("of: device %s (%s) uses an experimental binding\n", > > + np->name, np->full_name); > > + > > In the discussions earlier I think we decided that this should set a > taint flag too. If you've built a kernel with CONFIG_OF_EXPERIMENTAL > (which I think we were calling CONFIG_UNSTABLE_DT) then you have no > expectation that it will boot tomorrow, although it might work with your > DTB today. > So to avoid tainting the kernel and clogging the kernel log I'll have to remove all the "!" from the dt sources, or not use any "!" in the dt bindings in the first place. Given that, not sure if anyone will really use this mechanism. And CONFIG_OF_EXPERIMENTAL/CONFIG_UNSTABLE_DT might have the same ultimate fate as CONFIG_EXPERIMENTAL. Guenter
On Wed, 2013-10-23 at 09:55 -0700, Guenter Roeck wrote: > > So to avoid tainting the kernel and clogging the kernel log I'll have > to remove all the "!" from the dt sources, or not use any "!" in the > dt bindings in the first place. Given that, not sure if anyone will > really use this mechanism. And CONFIG_OF_EXPERIMENTAL/CONFIG_UNSTABLE_DT > might have the same ultimate fate as CONFIG_EXPERIMENTAL. It's not as if it's hard for you to patch out the check if you really want to do that. This is about making it clear to people what they should expect if they use staging code/bindings.
> Do we really want to polute the drivers and DT files with a ! in the > compatible values? I thought we'd considered that, but chosen having the > drivers that use unstable bindings depend on a Kconfig option as an > alternative, not an additional step? I'd even go further and use "unstable-" as the prefix instead of "!" which is way more explicit. > The one issue with doing this is that if a binding is thought to be > unstable, but becomes stable later without any changes, we'll have to do > busy-work to remove the ! in all the DT files, thus artificially > introducing an incompatibility. Perhaps that's fine though? I'd say yes. Going from unstable to stable is quite a step for a binding and that should be visible and worth a patch IMO. Also, when looking at a DTS file or some driver code, it will avoid confusion/misinterpretation if one can see immediately the status of a binding.
On Wed, Oct 23, 2013 at 05:05:32PM +0100, David Woodhouse wrote: > On Wed, 2013-10-23 at 17:06 +0200, Thierry Reding wrote: > > + /* check if binding is experimental */ > > + if (dev != device || drv != driver) { > > + pr_warn("of: device %s (%s) uses an experimental binding\n", > > + np->name, np->full_name); > > + > > In the discussions earlier I think we decided that this should set a > taint flag too. A taint flag seems somewhat drastic. It's not like using an experimental binding should have an influence on the stability of the running kernel. I always thought that taint flags were supposed to flag conditions where code of unknown origin or code known to be broken was being executed because they may destabilize the running kernel. The worst that should happen if you run an experimental binding is that some part of the system will just not come up. > If you've built a kernel with CONFIG_OF_EXPERIMENTAL (which I think we > were calling CONFIG_UNSTABLE_DT) then you have no expectation that it > will boot tomorrow, although it might work with your DTB today. Yes, something like that. Although I would hope that we can actually come up with reasonably simple and stable bindings for essential things required to boot. That way you could at least output some kind of warning to the user that something went wrong. If the system just won't boot at all then a taint flag won't help very much either. Thierry
On Wed, Oct 23, 2013 at 06:05:16PM +0100, David Woodhouse wrote: > On Wed, 2013-10-23 at 09:55 -0700, Guenter Roeck wrote: > > > > So to avoid tainting the kernel and clogging the kernel log I'll have > > to remove all the "!" from the dt sources, or not use any "!" in the > > dt bindings in the first place. Given that, not sure if anyone will > > really use this mechanism. And CONFIG_OF_EXPERIMENTAL/CONFIG_UNSTABLE_DT > > might have the same ultimate fate as CONFIG_EXPERIMENTAL. > > It's not as if it's hard for you to patch out the check if you really > want to do that. This is about making it clear to people what they > should expect if they use staging code/bindings. I agree. The kernel is open source, so people are free to patch it in any way they want. The best we can do is to come up with a good solution in an upstream kernel. But there's no way we can control things beyond that, and we don't have to. Thierry
On Wed, Oct 23, 2013 at 06:20:02PM +0100, Wolfram Sang wrote: > > > Do we really want to polute the drivers and DT files with a ! in the > > compatible values? I thought we'd considered that, but chosen having the > > drivers that use unstable bindings depend on a Kconfig option as an > > alternative, not an additional step? > > I'd even go further and use "unstable-" as the prefix instead of "!" > which is way more explicit. I guess unstable- is as good as anything. I personally think that "!" is disturbing enough to the eye to make it abundantly clear that something is fishy. > > The one issue with doing this is that if a binding is thought to be > > unstable, but becomes stable later without any changes, we'll have to do > > busy-work to remove the ! in all the DT files, thus artificially > > introducing an incompatibility. Perhaps that's fine though? > > I'd say yes. Going from unstable to stable is quite a step for a binding > and that should be visible and worth a patch IMO. Also, when looking at > a DTS file or some driver code, it will avoid > confusion/misinterpretation if one can see immediately the status of a > binding. Yes, I fully agree. It might look like churn, but I think this could actually be a part of the formal process to stabilize a binding. It would be final step of that process, actually. Thierry
On Wed, Oct 23, 2013 at 08:59:10PM +0200, Thierry Reding wrote: > > I'd say yes. Going from unstable to stable is quite a step for a binding > > and that should be visible and worth a patch IMO. Also, when looking at > > a DTS file or some driver code, it will avoid > > confusion/misinterpretation if one can see immediately the status of a > > binding. > > Yes, I fully agree. It might look like churn, but I think this could > actually be a part of the formal process to stabilize a binding. It > would be final step of that process, actually. I actually think this makes things worse. Ostensibly the purpose of stable DT is to allow the DT and kernel to be separate, so you should minimize the churn in the DTs, and they should trend to stable. Having a flag day where someone goes and churns the DT to remove a !, and then changes the kernel so all old DTs with a ! won't work at all makes this whole thing seem kinda contrary to the basic motivating premis. Also, what happens during development? If you incompatibly change the binding you should change the name, so maybe <version>!marvell,foo is the way to go. v1 of the binding is 1!marvell,foo - version 2 is 2!marvell,foo, etc. When stablized the last bang is kept and the non-bang version is added. The boot warning is supressed once stable no matter the compatible string used in the dt... Jason
> > I'd even go further and use "unstable-" as the prefix instead of "!" > > which is way more explicit. > > I guess unstable- is as good as anything. I personally think that "!" is > disturbing enough to the eye to make it abundantly clear that something > is fishy. "!" marks the binding as "special" whatever that is. A busy person might decide to not look that up as long as it works right now. "unstable-" (or maybe "unstable!-" ;)) is explicit so people know what they get.
On Wed, Oct 23, 2013 at 01:34:50PM -0600, Jason Gunthorpe wrote: > On Wed, Oct 23, 2013 at 08:59:10PM +0200, Thierry Reding wrote: > > > > I'd say yes. Going from unstable to stable is quite a step for a binding > > > and that should be visible and worth a patch IMO. Also, when looking at > > > a DTS file or some driver code, it will avoid > > > confusion/misinterpretation if one can see immediately the status of a > > > binding. > > > > Yes, I fully agree. It might look like churn, but I think this could > > actually be a part of the formal process to stabilize a binding. It > > would be final step of that process, actually. > > I actually think this makes things worse. > > Ostensibly the purpose of stable DT is to allow the DT and kernel to > be separate, so you should minimize the churn in the DTs, and they > should trend to stable. Well, I do think that stable DT has benefits. And quite frankly I think the majority of bindings will eventually converge to some stable state anyway, if only because active development stops. In an ideal world that would be when a product ships. So this proposal isn't so much about making a decision for stable or experimental DT, but rather about giving users a choice. If they are willing to live with the additional "burden" of updating the DTB every once in a while, then they can enable the option and get additional functionality. If they don't want any part of that, they can just leave the option disabled and only get the parts that are stable. > Having a flag day where someone goes and churns the DT to remove a !, > and then changes the kernel so all old DTs with a ! won't work at all > makes this whole thing seem kinda contrary to the basic motivating > premis. No. Matching doesn't include the ! marker. So if you remove it from DTS files and/or drivers, the only thing that goes away is the warning at runtime. Feature-wise there should be no difference. > Also, what happens during development? If you incompatibly change the > binding you should change the name, so maybe <version>!marvell,foo is > the way to go. > > v1 of the binding is 1!marvell,foo - version 2 is 2!marvell,foo, etc. > > When stablized the last bang is kept and the non-bang version is > added. The boot warning is supressed once stable no matter the > compatible string used in the dt... Why would we want to go through all that trouble if we define up front that the binding is experimental in the first place? Encoding a version number in it somehow entails that earlier versions are still supported in some way. But the whole point of experimental bindings is so that we don't have to worry about backwards compatibility. Thierry
On Wed, Oct 23, 2013 at 08:40:00PM +0100, Wolfram Sang wrote: > > > > I'd even go further and use "unstable-" as the prefix instead of "!" > > > which is way more explicit. > > > > I guess unstable- is as good as anything. I personally think that "!" is > > disturbing enough to the eye to make it abundantly clear that something > > is fishy. > > "!" marks the binding as "special" whatever that is. A busy person might > decide to not look that up as long as it works right now. "unstable-" > (or maybe "unstable!-" ;)) is explicit so people know what they get. We'll need to document this somewhere to make people aware of it. And if nobody bothers to read that documentation then they're not entitled to complain. That said, in my experience people are just as likely to ignore anything with an "unstable" in it until it breaks. Also I don't think "unstable" is the right term. "unstable" implies that it somehow influences the system stability. But that's not the case. "experimental" is much more accurate in that developers are experimenting with the representation. Thierry
On Wed, Oct 23, 2013 at 09:58:50PM +0200, Thierry Reding wrote: > > Having a flag day where someone goes and churns the DT to remove a !, > > and then changes the kernel so all old DTs with a ! won't work at all > > makes this whole thing seem kinda contrary to the basic motivating > > premis. > > No. Matching doesn't include the ! marker. So if you remove it from DTS > files and/or drivers, the only thing that goes away is the warning at > runtime. Feature-wise there should be no difference. Sounds good > > Also, what happens during development? If you incompatibly change the > > binding you should change the name, so maybe <version>!marvell,foo is > > the way to go. > > > > v1 of the binding is 1!marvell,foo - version 2 is 2!marvell,foo, etc. > > > > When stablized the last bang is kept and the non-bang version is > > added. The boot warning is supressed once stable no matter the > > compatible string used in the dt... > > Why would we want to go through all that trouble if we define up front > that the binding is experimental in the first place? Encoding a version > number in it somehow entails that earlier versions are still supported > in some way. No, certainly not. It just says the binding has changed and every DT stanza that uses the old version needs to be reviewed and updated. > But the whole point of experimental bindings is so that we don't have to > worry about backwards compatibility. The purpose is to not silently throw users/testers under the bus. A driver that doesn't bind is much better than a driver that blows up in some crazy, hard to determine way because the DT binding has been silently incompatibly changed. The rule for experimental bindings should be that incompatible changes to the binding must bump the version number at the same time, clearly signalling to everyone using that binding that they need to take some action. Jason
On Wed, Oct 23, 2013 at 12:58 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Oct 23, 2013 at 01:34:50PM -0600, Jason Gunthorpe wrote: >> On Wed, Oct 23, 2013 at 08:59:10PM +0200, Thierry Reding wrote: >> >> > > I'd say yes. Going from unstable to stable is quite a step for a binding >> > > and that should be visible and worth a patch IMO. Also, when looking at >> > > a DTS file or some driver code, it will avoid >> > > confusion/misinterpretation if one can see immediately the status of a >> > > binding. >> > >> > Yes, I fully agree. It might look like churn, but I think this could >> > actually be a part of the formal process to stabilize a binding. It >> > would be final step of that process, actually. >> >> I actually think this makes things worse. >> >> Ostensibly the purpose of stable DT is to allow the DT and kernel to >> be separate, so you should minimize the churn in the DTs, and they >> should trend to stable. > > Well, I do think that stable DT has benefits. And quite frankly I think > the majority of bindings will eventually converge to some stable state > anyway, if only because active development stops. In an ideal world that > would be when a product ships. > > So this proposal isn't so much about making a decision for stable or > experimental DT, but rather about giving users a choice. If they are > willing to live with the additional "burden" of updating the DTB every > once in a while, then they can enable the option and get additional > functionality. If they don't want any part of that, they can just leave > the option disabled and only get the parts that are stable. > >> Having a flag day where someone goes and churns the DT to remove a !, >> and then changes the kernel so all old DTs with a ! won't work at all >> makes this whole thing seem kinda contrary to the basic motivating >> premis. > > No. Matching doesn't include the ! marker. So if you remove it from DTS > files and/or drivers, the only thing that goes away is the warning at > runtime. Feature-wise there should be no difference. > >> Also, what happens during development? If you incompatibly change the >> binding you should change the name, so maybe <version>!marvell,foo is >> the way to go. >> >> v1 of the binding is 1!marvell,foo - version 2 is 2!marvell,foo, etc. >> [I'm a random bystander -- I don't really know anything about DT.] This sounds awfully like the -moz, -webkit, etc. CSS property selectors. They are AFAICT nearly universally considered to have been a mistake. --Andy
On Wed, Oct 23, 2013 at 03:08:49PM -0600, Jason Gunthorpe wrote: > On Wed, Oct 23, 2013 at 09:58:50PM +0200, Thierry Reding wrote: [...] > > > Also, what happens during development? If you incompatibly change the > > > binding you should change the name, so maybe <version>!marvell,foo is > > > the way to go. > > > > > > v1 of the binding is 1!marvell,foo - version 2 is 2!marvell,foo, etc. > > > > > > When stablized the last bang is kept and the non-bang version is > > > added. The boot warning is supressed once stable no matter the > > > compatible string used in the dt... > > > > Why would we want to go through all that trouble if we define up front > > that the binding is experimental in the first place? Encoding a version > > number in it somehow entails that earlier versions are still supported > > in some way. > > No, certainly not. It just says the binding has changed and every DT > stanza that uses the old version needs to be reviewed and updated. > > > But the whole point of experimental bindings is so that we don't have to > > worry about backwards compatibility. > > The purpose is to not silently throw users/testers under the bus. A > driver that doesn't bind is much better than a driver that blows up in > some crazy, hard to determine way because the DT binding has been > silently incompatibly changed. > > The rule for experimental bindings should be that incompatible changes > to the binding must bump the version number at the same time, clearly > signalling to everyone using that binding that they need to take some > action. I disagree. I think that we should apply the same rule to DT bindings (at least experimental ones) that we apply to code within the Linux kernel. If you change an experimental binding in an incompatible way then it should be your responsibility to update all users of it so that they don't break. In reality I would hope that this isn't much of a problem really. Given the nature of a compatible value there will typically only be a single driver implementing it. Any users of it (DTS files) should be pretty easy to fix up at the same time. That's the same way that platform data used to work, except it had the advantage of the compiler actually pointing out incompatible changes. There has been some discussion about adding validation functionality to DTC, which should make it easy to find DTS files that require updating as well. The above would also indicate that because of their nature, experimental bindings might be better kept within the Linux kernel tree. That would make it easy to keep a good overview of what needs to be updated when a binding changes. It would also make the promotion to stable much more explicit by physically moving the binding document to a different repository. Obviously that comes with its own set of problem that we'll need to find a way to extend the board DTS files that will presumably be kept in the external stable binding repository with experimental content only available in the Linux kernel tree. Thierry
On Wed, 23 Oct 2013 18:20:02 +0100, Wolfram Sang <wsa@the-dreams.de> wrote: > > > Do we really want to polute the drivers and DT files with a ! in the > > compatible values? I thought we'd considered that, but chosen having the > > drivers that use unstable bindings depend on a Kconfig option as an > > alternative, not an additional step? > > I'd even go further and use "unstable-" as the prefix instead of "!" > which is way more explicit. > > > > The one issue with doing this is that if a binding is thought to be > > unstable, but becomes stable later without any changes, we'll have to do > > busy-work to remove the ! in all the DT files, thus artificially > > introducing an incompatibility. Perhaps that's fine though? > > I'd say yes. Going from unstable to stable is quite a step for a binding > and that should be visible and worth a patch IMO. Also, when looking at > a DTS file or some driver code, it will avoid > confusion/misinterpretation if one can see immediately the status of a > binding. No, it shouldn't. Going from unstable to stable is not a large step, rather it is coming to the point of looking around and realizing that the binding is working quite well. I don't think the solution is to put this into the kernel to be checked at runtime. The better solution is to put it into DTC and make it complain (either warn or error; depending on build config?) about usage of compatible strings that are marked in the binding documentation as unstable. g.
On Thu, Oct 24, 2013 at 09:34:59AM +0100, Grant Likely wrote: > On Wed, 23 Oct 2013 18:20:02 +0100, Wolfram Sang <wsa@the-dreams.de> wrote: > > > > > Do we really want to polute the drivers and DT files with a ! in the > > > compatible values? I thought we'd considered that, but chosen having the > > > drivers that use unstable bindings depend on a Kconfig option as an > > > alternative, not an additional step? > > > > I'd even go further and use "unstable-" as the prefix instead of "!" > > which is way more explicit. > > > > > > > The one issue with doing this is that if a binding is thought to be > > > unstable, but becomes stable later without any changes, we'll have to do > > > busy-work to remove the ! in all the DT files, thus artificially > > > introducing an incompatibility. Perhaps that's fine though? > > > > I'd say yes. Going from unstable to stable is quite a step for a binding > > and that should be visible and worth a patch IMO. Also, when looking at > > a DTS file or some driver code, it will avoid > > confusion/misinterpretation if one can see immediately the status of a > > binding. > > No, it shouldn't. Going from unstable to stable is not a large step, > rather it is coming to the point of looking around and realizing that > the binding is working quite well. Yes, the difference between the unstable binding before it is declared stable and the stable one shouldn't be big. In fact it should be no different at all. However the decision is still a conscious one. And it is a big step, because when you declare it stable you assert that it will never change in an incompatible way. > I don't think the solution is to put this into the kernel to be checked > at runtime. The better solution is to put it into DTC and make it > complain (either warn or error; depending on build config?) about usage > of compatible strings that are marked in the binding documentation as > unstable. Perhaps. Doing it in the kernel seemed easier. Furthermore not every user might generate their own DTB and whoever generates the DTB may not make the same choice as every user might have made. Granted, that might be a little far fetched. I personally don't mind where exactly it is checked for, as long as we can settle on something. What I'm primarily concerned about is that the current situation hinders progress and early adoption, which I consider both essential for upstream Linux development. Thierry
On Thu, Oct 24, 2013 at 10:04:19AM +0200, Thierry Reding wrote: > > The rule for experimental bindings should be that incompatible changes > > to the binding must bump the version number at the same time, clearly > > signalling to everyone using that binding that they need to take some > > action. > > I disagree. I think that we should apply the same rule to DT bindings > (at least experimental ones) that we apply to code within the Linux > kernel. If you change an experimental binding in an incompatible way > then it should be your responsibility to update all users of it so that > they don't break. Absolutely, but these things are going to get out of the kernel tree and people are going to be using them in broad contexts (eg flashing them into firmware) - especially if we imagine the stablization window is multiple kernel releases. Clearly designating which revision is supported lets people know what is going on, very explicitly. Also, it lets people that might have the need to support multiple versions in their out-of-tree DT by having repeated nodes. I just think it is good practice to get people into the habit that the compatible string indicates a single exact schema, and if you change the schema you have to change the compatible string. Always. Jason
On Wed, Oct 23, 2013 at 11:06 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > Past and recent discussions have shown that there's some concensus that > device tree bindings should be considered an ABI and therefore need to > remain backwards-compatible once code to implement them is included in > the final release of a Linux kernel. Doing it this way clutters up the DTS source files. Schemas are a way to do this without clutter. There should be a master schema that would validate all accepted device tree bindings. So experimental bindings naturally drop right of of this - they'll generate errors when validated against the master schema. When the binding is finally sorted out it gets added to the master schema and the validation errors go away. > > At the same time there is a desire to keep some manoeuvre while we're > trying to figure things out. The fact is that many of us lack the > experience to design good bindings from the start. At the same time > there is a shortage of people that can review bindings and help design > better ones. > > Progress and the addition of new features (and restoration of features > that used to work before the advent of DT for that matter) are blocked > to a large degree because of that. > > This patch attempts to restore some degree of freedom by introducing an > easy way to mark device tree bindings as experimental as well as a way > for users to disable the use of experimental bindings if they choose > functionality at the cost of potential device tree incompatibilities. > > Bindings are marked experimental by prefixing the compatible value with > an exclamation mark (!). In order to make it clear that experimental > bindings are undesirable in the long run, a warning will be output when > an experimental binding is encountered. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/of/Kconfig | 7 +++++++ > drivers/of/base.c | 32 +++++++++++++++++++++++++++++++- > 2 files changed, 38 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig > index 78cc760..dc482f8 100644 > --- a/drivers/of/Kconfig > +++ b/drivers/of/Kconfig > @@ -24,6 +24,13 @@ config OF_SELFTEST > > If unsure, say N here, but this option is safe to enable. > > +config OF_EXPERIMENTAL > + bool "Support experimental device tree bindings" > + help > + This option allows experimental device tree bindings to be used. > + Note that experimental bindings are subject to change, possibly > + requiring the DTB to be updated. > + > config OF_FLATTREE > bool > select DTC > diff --git a/drivers/of/base.c b/drivers/of/base.c > index a96f850..b0b8371 100644 > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -342,6 +342,36 @@ struct device_node *of_get_cpu_node(int cpu, unsigned int *thread) > } > EXPORT_SYMBOL(of_get_cpu_node); > > +static int of_compat_match(const char *device, const char *driver, > + const struct device_node *np) > +{ > + const char *dev = device; > + const char *drv = driver; > + > + if (device[0] == '!') > + device++; > + > + if (driver[0] == '!') > + driver++; > + > + if (of_compat_cmp(device, driver, strlen(driver)) != 0) > + return 0; > + > + /* check if binding is experimental */ > + if (dev != device || drv != driver) { > + pr_warn("of: device %s (%s) uses an experimental binding\n", > + np->name, np->full_name); > + > + /* don't match if we don't want experimental bindings */ > + if (!IS_ENABLED(CONFIG_OF_EXPERIMENTAL)) { > + pr_err("of: refusing to use binding \"%s\"\n", device); > + return 0; > + } > + } > + > + return 1; > +} > + > /** Checks if the given "compat" string matches one of the strings in > * the device's "compatible" property > */ > @@ -355,7 +385,7 @@ static int __of_device_is_compatible(const struct device_node *device, > if (cp == NULL) > return 0; > while (cplen > 0) { > - if (of_compat_cmp(cp, compat, strlen(compat)) == 0) > + if (of_compat_match(cp, compat, device)) > return 1; > l = strlen(cp) + 1; > cp += l; > -- > 1.8.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Oct 24, 2013 at 3:34 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Wed, 23 Oct 2013 18:20:02 +0100, Wolfram Sang <wsa@the-dreams.de> wrote: >> >> I'd say yes. Going from unstable to stable is quite a step for a binding >> and that should be visible and worth a patch IMO. A patch to remove the ! in front of !marvell,dma-controller is still going to be as visible as a patch to remove "unstable-!-version2-!-!-!!-omg!marvel,dma-controller" isn't it? My worry is some bindings will never go stable, and the kernel will keep printing warnings. At this point, marking it unstable is like marking a driver as depends on EXPERIMENTAL in Kconfig - everyone enables EXPERIMENTAL so there was no reason for it to exist. So, a few warnings pop out on a console with a ! in front, people ignore those and carry on. Who are you really trying to affect by implementing an unstable binding warning? >> a DTS file or some driver code, it will avoid >> confusion/misinterpretation if one can see immediately the status of a >> binding. > > No, it shouldn't. Going from unstable to stable is not a large step, rather it is coming to the point of looking around and realizing that the binding is working quite well. Who decides that? > I don't think the solution is to put this into the kernel to be checked > at runtime. I think the warning needs to be selectable at runtime; distributions should mandate it turned on (therefore they can tell if weird crap happens in their bug trackers same as the staging taint flag) but normal people can turn it off. In the meantime, everything keeps working as it was; except that new device trees will no longer work on older systems since they will not be ignoring the bang and devices will disappear. It also implies that every current binding has been implicitly made stable, and again the worry that new bindings will stay unstable forever (and forever knock out that warning until it gets someone patching away the bang upstream whether it is really 'stable' or not.) > The better solution is to put it into DTC and make it > complain (either warn or error; depending on build config?) about usage > of compatible strings that are marked in the binding documentation as > unstable. I would agree with this, except less people care about the kernel build output than console output. People using precompiled distros won't even notice.. Again, DTC would have to strip the bang before handing it to the kernel, or every current (as of today) binding is stable. But it is less intrusive. The state of the binding - being experimental or not - should probably just be denoted by the binding being in an "experimental" directory in the source, or marked with a X-DT-Binding-Status: TOTALLY_WHACKED_OUT tag at the top of the binding itself. If you build a kernel and stuff acts weird, check the binding for the device that acts weird. If it's marked as such, go harangue the developers...
On 10/23/2013 07:51 PM, Thierry Reding wrote: > On Wed, Oct 23, 2013 at 05:05:32PM +0100, David Woodhouse wrote: >> On Wed, 2013-10-23 at 17:06 +0200, Thierry Reding wrote: >>> + /* check if binding is experimental */ >>> + if (dev != device || drv != driver) { >>> + pr_warn("of: device %s (%s) uses an experimental binding\n", >>> + np->name, np->full_name); >>> + >> >> In the discussions earlier I think we decided that this should set a >> taint flag too. > > A taint flag seems somewhat drastic. It's not like using an experimental > binding should have an influence on the stability of the running kernel. > I always thought that taint flags were supposed to flag conditions where > code of unknown origin or code known to be broken was being executed > because they may destabilize the running kernel. > > The worst that should happen if you run an experimental binding is that > some part of the system will just not come up. IIRC, the purpose of the taint flag was to make it clear that the kernel or DT was not expected to function in the future, so don't be surpised if you upgrade it, and it stops working, without you taking explicit action, such as revising your DT to match the new kernel or vice-versa.
On 10/24/2013 09:34 AM, Grant Likely wrote: > On Wed, 23 Oct 2013 18:20:02 +0100, Wolfram Sang <wsa@the-dreams.de> wrote: >> >>> Do we really want to polute the drivers and DT files with a ! in the >>> compatible values? I thought we'd considered that, but chosen having the >>> drivers that use unstable bindings depend on a Kconfig option as an >>> alternative, not an additional step? >> >> I'd even go further and use "unstable-" as the prefix instead of "!" >> which is way more explicit. >> >> >>> The one issue with doing this is that if a binding is thought to be >>> unstable, but becomes stable later without any changes, we'll have to do >>> busy-work to remove the ! in all the DT files, thus artificially >>> introducing an incompatibility. Perhaps that's fine though? >> >> I'd say yes. Going from unstable to stable is quite a step for a binding >> and that should be visible and worth a patch IMO. Also, when looking at >> a DTS file or some driver code, it will avoid >> confusion/misinterpretation if one can see immediately the status of a >> binding. > > No, it shouldn't. Going from unstable to stable is not a large step, rather it is coming to the point of looking around and realizing that the binding is working quite well. > > I don't think the solution is to put this into the kernel to be checked > at runtime. The better solution is to put it into DTC and make it > complain (either warn or error; depending on build config?) about usage > of compatible strings that are marked in the binding documentation as > unstable. I don't think that's what we talked about on Wednesday though. At a quick glance, this didn't make it into the meeting notes though[1], but is in the presentation we created for the kernel summit readout. Is sharing a link to that before it's presented OK? [1] http://etherpad.osuosl.org/arm-ksummit-2013-day-2
On Thu, Oct 24, 2013 at 11:26:19PM +0100, Stephen Warren wrote: > On 10/23/2013 07:51 PM, Thierry Reding wrote: > > On Wed, Oct 23, 2013 at 05:05:32PM +0100, David Woodhouse wrote: > >> On Wed, 2013-10-23 at 17:06 +0200, Thierry Reding wrote: > >>> + /* check if binding is experimental */ > >>> + if (dev != device || drv != driver) { > >>> + pr_warn("of: device %s (%s) uses an experimental binding\n", > >>> + np->name, np->full_name); > >>> + > >> > >> In the discussions earlier I think we decided that this should set a > >> taint flag too. > > > > A taint flag seems somewhat drastic. It's not like using an experimental > > binding should have an influence on the stability of the running kernel. > > I always thought that taint flags were supposed to flag conditions where > > code of unknown origin or code known to be broken was being executed > > because they may destabilize the running kernel. > > > > The worst that should happen if you run an experimental binding is that > > some part of the system will just not come up. > > IIRC, the purpose of the taint flag was to make it clear that the kernel > or DT was not expected to function in the future, so don't be surpised > if you upgrade it, and it stops working, without you taking explicit > action, such as revising your DT to match the new kernel or vice-versa. I understand that, but I was arguing that it doesn't match existing uses of taint flags. The various flags that are currently defined all seem to be set whenever some event occurs that could cause instability of the currently running system, such as loading a proprietary or out-of-tree module, forcing a module to be loaded, overriding firmware parameters... All those seem to have the goal of appearing in crash logs, so that whoever looks at the bug report can point users somewhere else since the problem is likely to be caused by their own (bad) decision. Or ask users to reproduce crashes or bugs without doing whatever they did to cause the taint flag(s) to be set. Experimental bindings shouldn't cause any crashes in the first place, or not cause memory corruption or similar for that matter. If we don't want to support an experimental binding, then all we should do is not support any functionality that relies on them. That doesn't mean that runtime stability is in any way affected. Thierry
On 10/25/2013 09:22 AM, Thierry Reding wrote: > On Thu, Oct 24, 2013 at 11:26:19PM +0100, Stephen Warren wrote: >> On 10/23/2013 07:51 PM, Thierry Reding wrote: >>> On Wed, Oct 23, 2013 at 05:05:32PM +0100, David Woodhouse >>> wrote: >>>> On Wed, 2013-10-23 at 17:06 +0200, Thierry Reding wrote: >>>>> + /* check if binding is experimental */ + if >>>>> (dev != device || drv != driver) { + >>>>> pr_warn("of: device %s (%s) uses an experimental >>>>> binding\n", + np->name, >>>>> np->full_name); + >>>> >>>> In the discussions earlier I think we decided that this >>>> should set a taint flag too. >>> >>> A taint flag seems somewhat drastic. It's not like using an >>> experimental binding should have an influence on the stability >>> of the running kernel. I always thought that taint flags were >>> supposed to flag conditions where code of unknown origin or >>> code known to be broken was being executed because they may >>> destabilize the running kernel. >>> >>> The worst that should happen if you run an experimental binding >>> is that some part of the system will just not come up. >> >> IIRC, the purpose of the taint flag was to make it clear that the >> kernel or DT was not expected to function in the future, so don't >> be surpised if you upgrade it, and it stops working, without you >> taking explicit action, such as revising your DT to match the new >> kernel or vice-versa. > > I understand that, but I was arguing that it doesn't match existing > uses of taint flags. The various flags that are currently defined > all seem to be set whenever some event occurs that could cause > instability of the currently running system, such as loading a > proprietary or out-of-tree module, forcing a module to be loaded, > overriding firmware parameters... Executing a driver that supports an unstable binding does produce instability in the system; the kernel configuration might not continue to work if rebooted with an updated DT. Admittedly, this is a slightly different concept than other taint flags, but seems like a logical extension.
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig index 78cc760..dc482f8 100644 --- a/drivers/of/Kconfig +++ b/drivers/of/Kconfig @@ -24,6 +24,13 @@ config OF_SELFTEST If unsure, say N here, but this option is safe to enable. +config OF_EXPERIMENTAL + bool "Support experimental device tree bindings" + help + This option allows experimental device tree bindings to be used. + Note that experimental bindings are subject to change, possibly + requiring the DTB to be updated. + config OF_FLATTREE bool select DTC diff --git a/drivers/of/base.c b/drivers/of/base.c index a96f850..b0b8371 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -342,6 +342,36 @@ struct device_node *of_get_cpu_node(int cpu, unsigned int *thread) } EXPORT_SYMBOL(of_get_cpu_node); +static int of_compat_match(const char *device, const char *driver, + const struct device_node *np) +{ + const char *dev = device; + const char *drv = driver; + + if (device[0] == '!') + device++; + + if (driver[0] == '!') + driver++; + + if (of_compat_cmp(device, driver, strlen(driver)) != 0) + return 0; + + /* check if binding is experimental */ + if (dev != device || drv != driver) { + pr_warn("of: device %s (%s) uses an experimental binding\n", + np->name, np->full_name); + + /* don't match if we don't want experimental bindings */ + if (!IS_ENABLED(CONFIG_OF_EXPERIMENTAL)) { + pr_err("of: refusing to use binding \"%s\"\n", device); + return 0; + } + } + + return 1; +} + /** Checks if the given "compat" string matches one of the strings in * the device's "compatible" property */ @@ -355,7 +385,7 @@ static int __of_device_is_compatible(const struct device_node *device, if (cp == NULL) return 0; while (cplen > 0) { - if (of_compat_cmp(cp, compat, strlen(compat)) == 0) + if (of_compat_match(cp, compat, device)) return 1; l = strlen(cp) + 1; cp += l;
Past and recent discussions have shown that there's some concensus that device tree bindings should be considered an ABI and therefore need to remain backwards-compatible once code to implement them is included in the final release of a Linux kernel. At the same time there is a desire to keep some manoeuvre while we're trying to figure things out. The fact is that many of us lack the experience to design good bindings from the start. At the same time there is a shortage of people that can review bindings and help design better ones. Progress and the addition of new features (and restoration of features that used to work before the advent of DT for that matter) are blocked to a large degree because of that. This patch attempts to restore some degree of freedom by introducing an easy way to mark device tree bindings as experimental as well as a way for users to disable the use of experimental bindings if they choose functionality at the cost of potential device tree incompatibilities. Bindings are marked experimental by prefixing the compatible value with an exclamation mark (!). In order to make it clear that experimental bindings are undesirable in the long run, a warning will be output when an experimental binding is encountered. Signed-off-by: Thierry Reding <treding@nvidia.com> --- drivers/of/Kconfig | 7 +++++++ drivers/of/base.c | 32 +++++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 1 deletion(-)