| Message ID | 20181108165156.60073-4-heikki.krogerus@linux.intel.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | device property: Add fwnode_get_name() helper | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On Thu, Nov 8, 2018 at 10:52 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > This implements get_name fwnode op for DT. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/of/property.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index f46828e3b082..9bc8fe136fa3 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -823,6 +823,16 @@ static void of_fwnode_put(struct fwnode_handle *fwnode) > of_node_put(to_of_node(fwnode)); > } > > +static int of_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf) > +{ > + const char *name = kbasename(to_of_node(fwnode)->full_name); > + size_t len = strchrnul(name, '@') - name; > + > + snprintf(buf, len + 1, "%s", name); This can be simplified to: snprintf(..., "%pOFn", to_of_node(fwnode)) But that presents a problem with knowing the length. Not passing in the buf length is not good design because you can't tell if you overflow the buffer. Either you can pass in the length of buf or do the allocation here. In the latter case, then you can use kasnprintf. The downside to doing the allocation here is then get_name() has side effect of allocating memory that the caller needs to be aware of. However, I think the current API is better. It leaves low-level details up to the firmware implementation. But as long as .get_name() is not exposed to drivers I don't really care that much. Rob
On Thu, Nov 08, 2018 at 12:23:51PM -0600, Rob Herring wrote: > On Thu, Nov 8, 2018 at 10:52 AM Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > > This implements get_name fwnode op for DT. > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > --- > > drivers/of/property.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index f46828e3b082..9bc8fe136fa3 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -823,6 +823,16 @@ static void of_fwnode_put(struct fwnode_handle *fwnode) > > of_node_put(to_of_node(fwnode)); > > } > > > > +static int of_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf) > > +{ > > + const char *name = kbasename(to_of_node(fwnode)->full_name); > > + size_t len = strchrnul(name, '@') - name; > > + > > + snprintf(buf, len + 1, "%s", name); > > This can be simplified to: > > snprintf(..., "%pOFn", to_of_node(fwnode)) > > But that presents a problem with knowing the length. Not passing in > the buf length is not good design because you can't tell if you > overflow the buffer. Either you can pass in the length of buf or do > the allocation here. In the latter case, then you can use kasnprintf. > The downside to doing the allocation here is then get_name() has side > effect of allocating memory that the caller needs to be aware of. Agree on matter of potential overflow. I wouldn't limit users with 32 characters for node name if it's not by both ACPI and DT specifications. OTOH allocating and freeing memory in a loop each time when we would like to go through the child nodes sounds much worse scenario to me. Thus, giving a length of the buffer is a good enough compromise. > However, I think the current API is better. It leaves low-level > details up to the firmware implementation. But as long as .get_name() > is not exposed to drivers I don't really care that much. I don't think this concept is changed by Heikki's patch series. It provides a common abstract function on top of more low-level firmware implementation which I consider a good approach.
On Thu, Nov 8, 2018 at 1:25 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 08, 2018 at 12:23:51PM -0600, Rob Herring wrote: > > On Thu, Nov 8, 2018 at 10:52 AM Heikki Krogerus > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > This implements get_name fwnode op for DT. > > > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > --- > > > drivers/of/property.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > > index f46828e3b082..9bc8fe136fa3 100644 > > > --- a/drivers/of/property.c > > > +++ b/drivers/of/property.c > > > @@ -823,6 +823,16 @@ static void of_fwnode_put(struct fwnode_handle *fwnode) > > > of_node_put(to_of_node(fwnode)); > > > } > > > > > > +static int of_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf) > > > +{ > > > + const char *name = kbasename(to_of_node(fwnode)->full_name); > > > + size_t len = strchrnul(name, '@') - name; > > > + > > > + snprintf(buf, len + 1, "%s", name); > > > > This can be simplified to: > > > > snprintf(..., "%pOFn", to_of_node(fwnode)) > > > > But that presents a problem with knowing the length. Not passing in > > the buf length is not good design because you can't tell if you > > overflow the buffer. Either you can pass in the length of buf or do > > the allocation here. In the latter case, then you can use kasnprintf. > > The downside to doing the allocation here is then get_name() has side > > effect of allocating memory that the caller needs to be aware of. > > Agree on matter of potential overflow. > > I wouldn't limit users with 32 characters for node name if it's not by both > ACPI and DT specifications. While the DT spec says 31 characters, this has never been enforced. As you might guess, we have node names longer than 31 characters. There's been some discussion about what to do and the consensus seems to be change the spec. > OTOH allocating and freeing memory in a loop each > time when we would like to go through the child nodes sounds much worse > scenario to me. Yes, I wrote that before looking at how you were using it... Of course, if you want efficient, then you shouldn't use sprintf either and use of_node_name_eq() as I've suggested. > Thus, giving a length of the buffer is a good enough compromise. > > > However, I think the current API is better. It leaves low-level > > details up to the firmware implementation. But as long as .get_name() > > is not exposed to drivers I don't really care that much. > > I don't think this concept is changed by Heikki's patch series. It provides a > common abstract function on top of more low-level firmware implementation which > I consider a good approach. Generally, I would agree that's a worthwhile goal. However, in this case you aren't saving anything. We still have at least a DT version of the same thing (of_get_child_by_name). Maybe there's some dream that the fwnode API will become the only one for both drivers and non-drivers, but I really don't see that happening. As long as both DT and fwnode APIs are in use, it is best to keep the APIs aligned. There's another aspect that the node name comparison is case insensitive on powerpc (and until 4.20, was for everything but Sparc). I changed it in 4.20 and hopefully don't have to revert. Patch 4 does a case sensitive compare. That probably is fine (as lots of powerpc code already does case sensitive name compares), but no one really knows until we break users. Another issue is how are disabled nodes dealt with by different firmwares? It's a frequent bug that we don't honor the 'status' property (such as in the very code we're discussing). But then there are some cases were want to ignore it so we can't just go add that check in and we end up needing 2 flavors of everything. You're probably okay though. Most devices with child nodes are enabled/disabled only in the parent device node. Rob Rob
On Thu, Nov 08, 2018 at 03:18:21PM -0600, Rob Herring wrote: > On Thu, Nov 8, 2018 at 1:25 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Thu, Nov 08, 2018 at 12:23:51PM -0600, Rob Herring wrote: > > > On Thu, Nov 8, 2018 at 10:52 AM Heikki Krogerus > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > This implements get_name fwnode op for DT. > > > > > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > --- > > > > drivers/of/property.c | 11 +++++++++++ > > > > 1 file changed, 11 insertions(+) > > > > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > > > index f46828e3b082..9bc8fe136fa3 100644 > > > > --- a/drivers/of/property.c > > > > +++ b/drivers/of/property.c > > > > @@ -823,6 +823,16 @@ static void of_fwnode_put(struct fwnode_handle *fwnode) > > > > of_node_put(to_of_node(fwnode)); > > > > } > > > > > > > > +static int of_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf) > > > > +{ > > > > + const char *name = kbasename(to_of_node(fwnode)->full_name); > > > > + size_t len = strchrnul(name, '@') - name; > > > > + > > > > + snprintf(buf, len + 1, "%s", name); > > > > > > This can be simplified to: > > > > > > snprintf(..., "%pOFn", to_of_node(fwnode)) > > > > > > But that presents a problem with knowing the length. Not passing in > > > the buf length is not good design because you can't tell if you > > > overflow the buffer. Either you can pass in the length of buf or do > > > the allocation here. In the latter case, then you can use kasnprintf. > > > The downside to doing the allocation here is then get_name() has side > > > effect of allocating memory that the caller needs to be aware of. > > > > Agree on matter of potential overflow. > > > > I wouldn't limit users with 32 characters for node name if it's not by both > > ACPI and DT specifications. > > While the DT spec says 31 characters, this has never been enforced. As > you might guess, we have node names longer than 31 characters. There's > been some discussion about what to do and the consensus seems to be > change the spec. > > > OTOH allocating and freeing memory in a loop each > > time when we would like to go through the child nodes sounds much worse > > scenario to me. > > Yes, I wrote that before looking at how you were using it... Of > course, if you want efficient, then you shouldn't use sprintf either > and use of_node_name_eq() as I've suggested. Since the fwnode API is just a wrapper layer (at least IMO), I don't think there should be any assumptions that it provides the optimal solution for anything. The low-level APIs should be the ones providing the optimal solutions. > > Thus, giving a length of the buffer is a good enough compromise. OK. That's what we'll do then. > > > However, I think the current API is better. It leaves low-level > > > details up to the firmware implementation. But as long as .get_name() > > > is not exposed to drivers I don't really care that much. Side note: I would prefer that we had something like of_node_name_get() function that of_fwnode_get_name() could simply call. I don't know how you want that implemented, but I'm expecting you will implement something like that in any case once you start removing that ->name member. I figured that at that point we can update of_fwnode_get_name() as well. > > I don't think this concept is changed by Heikki's patch series. It provides a > > common abstract function on top of more low-level firmware implementation which > > I consider a good approach. > > Generally, I would agree that's a worthwhile goal. However, in this > case you aren't saving anything. We still have at least a DT version > of the same thing (of_get_child_by_name). Maybe there's some dream > that the fwnode API will become the only one for both drivers and > non-drivers, but I really don't see that happening. As long as both DT > and fwnode APIs are in use, it is best to keep the APIs aligned. I don't think that anybody was planning to have the fwnode API as the only available API for the drivers. It's just a helper for the drivers on top of the low-level APIs, so the low-level APIs really need to always stay available for the drivers. There will always be corner cases. > There's another aspect that the node name comparison is case > insensitive on powerpc (and until 4.20, was for everything but Sparc). > I changed it in 4.20 and hopefully don't have to revert. Patch 4 does > a case sensitive compare. That probably is fine (as lots of powerpc > code already does case sensitive name compares), but no one really > knows until we break users. I actually used stccasecmp() in the first version. I don't know how, or why, I've changed it to strcmp(). I'll fix that. > Another issue is how are disabled nodes dealt with by different > firmwares? It's a frequent bug that we don't honor the 'status' > property (such as in the very code we're discussing). But then there > are some cases were want to ignore it so we can't just go add that > check in and we end up needing 2 flavors of everything. You're > probably okay though. Most devices with child nodes are > enabled/disabled only in the parent device node. thanks,
On Fri, Nov 9, 2018 at 7:34 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Thu, Nov 08, 2018 at 03:18:21PM -0600, Rob Herring wrote: > > On Thu, Nov 8, 2018 at 1:25 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Thu, Nov 08, 2018 at 12:23:51PM -0600, Rob Herring wrote: > > > > On Thu, Nov 8, 2018 at 10:52 AM Heikki Krogerus > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > This implements get_name fwnode op for DT. > > > > > > > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > > --- > > > > > drivers/of/property.c | 11 +++++++++++ > > > > > 1 file changed, 11 insertions(+) > > > > > > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > > > > index f46828e3b082..9bc8fe136fa3 100644 > > > > > --- a/drivers/of/property.c > > > > > +++ b/drivers/of/property.c > > > > > @@ -823,6 +823,16 @@ static void of_fwnode_put(struct fwnode_handle *fwnode) > > > > > of_node_put(to_of_node(fwnode)); > > > > > } > > > > > > > > > > +static int of_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf) > > > > > +{ > > > > > + const char *name = kbasename(to_of_node(fwnode)->full_name); > > > > > + size_t len = strchrnul(name, '@') - name; > > > > > + > > > > > + snprintf(buf, len + 1, "%s", name); > > > > > > > > This can be simplified to: > > > > > > > > snprintf(..., "%pOFn", to_of_node(fwnode)) > > > > > > > > But that presents a problem with knowing the length. Not passing in > > > > the buf length is not good design because you can't tell if you > > > > overflow the buffer. Either you can pass in the length of buf or do > > > > the allocation here. In the latter case, then you can use kasnprintf. > > > > The downside to doing the allocation here is then get_name() has side > > > > effect of allocating memory that the caller needs to be aware of. > > > > > > Agree on matter of potential overflow. > > > > > > I wouldn't limit users with 32 characters for node name if it's not by both > > > ACPI and DT specifications. > > > > While the DT spec says 31 characters, this has never been enforced. As > > you might guess, we have node names longer than 31 characters. There's > > been some discussion about what to do and the consensus seems to be > > change the spec. > > > > > OTOH allocating and freeing memory in a loop each > > > time when we would like to go through the child nodes sounds much worse > > > scenario to me. > > > > Yes, I wrote that before looking at how you were using it... Of > > course, if you want efficient, then you shouldn't use sprintf either > > and use of_node_name_eq() as I've suggested. > > Since the fwnode API is just a wrapper layer (at least IMO), I don't > think there should be any assumptions that it provides the optimal > solution for anything. The low-level APIs should be the ones providing > the optimal solutions. > > > > Thus, giving a length of the buffer is a good enough compromise. > > OK. That's what we'll do then. > > > > > However, I think the current API is better. It leaves low-level > > > > details up to the firmware implementation. But as long as .get_name() > > > > is not exposed to drivers I don't really care that much. > > Side note: > > I would prefer that we had something like of_node_name_get() function > that of_fwnode_get_name() could simply call. I don't know how you want > that implemented, but I'm expecting you will implement something like > that in any case once you start removing that ->name member. I figured > that at that point we can update of_fwnode_get_name() as well. I don't plan to implement anything like that. Here's what I'm planning: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/log/?h=dt/testing Which is eliminating the need to access .name by: using %pOFn (mostly in 4.20), using of_node_name_eq() wrapper, or using full_name instead. The last case generally is cases where the name is not important such as request_irq() or irq_chip.name. > > > I don't think this concept is changed by Heikki's patch series. It provides a > > > common abstract function on top of more low-level firmware implementation which > > > I consider a good approach. > > > > Generally, I would agree that's a worthwhile goal. However, in this > > case you aren't saving anything. We still have at least a DT version > > of the same thing (of_get_child_by_name). Maybe there's some dream > > that the fwnode API will become the only one for both drivers and > > non-drivers, but I really don't see that happening. As long as both DT > > and fwnode APIs are in use, it is best to keep the APIs aligned. > > I don't think that anybody was planning to have the fwnode API as > the only available API for the drivers. It's just a helper for the > drivers on top of the low-level APIs, so the low-level APIs really > need to always stay available for the drivers. There will always be > corner cases. So either it needs to be the only interface to drivers or the fwnode and DT APIs need to have the same calls. We don't want drivers to use different style/design of API based on picking fwnode or DT API. If you have the same calls, there's no point to trying to copy the common parts into fwnode code. Then you have 3 implementations of the same thing instead of 2. > > There's another aspect that the node name comparison is case > > insensitive on powerpc (and until 4.20, was for everything but Sparc). > > I changed it in 4.20 and hopefully don't have to revert. Patch 4 does > > a case sensitive compare. That probably is fine (as lots of powerpc > > code already does case sensitive name compares), but no one really > > knows until we break users. > > I actually used stccasecmp() in the first version. I don't know how, > or why, I've changed it to strcmp(). I'll fix that. That's not what I'm suggesting. I'm saying that is a firmware level detail that should remain in firmware code. I'm actually hoping to move things over to be case sensitive in most cases. That may mean having some work-around like: if (IS_ENABLED(CONFIG_PPC_PMAC)) ... strcasecmp() else ... strcmp() Or whatever system needs this. I think it is old PowerMacs, but am not really sure. And yeah, that would be ugly, but at least we'd know what exactly needs it. Do you want this crap is fwnode code? Rob
Hi, On Fri, Nov 09, 2018 at 10:14:49AM -0600, Rob Herring wrote: > > I would prefer that we had something like of_node_name_get() function > > that of_fwnode_get_name() could simply call. I don't know how you want > > that implemented, but I'm expecting you will implement something like > > that in any case once you start removing that ->name member. I figured > > that at that point we can update of_fwnode_get_name() as well. > > I don't plan to implement anything like that. Here's what I'm planning: > https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/log/?h=dt/testing > > Which is eliminating the need to access .name by: using %pOFn (mostly > in 4.20), using of_node_name_eq() wrapper, or using full_name instead. > The last case generally is cases where the name is not important such > as request_irq() or irq_chip.name. OK. Thanks for the explanation. thanks,
diff --git a/drivers/of/property.c b/drivers/of/property.c index f46828e3b082..9bc8fe136fa3 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -823,6 +823,16 @@ static void of_fwnode_put(struct fwnode_handle *fwnode) of_node_put(to_of_node(fwnode)); } +static int of_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf) +{ + const char *name = kbasename(to_of_node(fwnode)->full_name); + size_t len = strchrnul(name, '@') - name; + + snprintf(buf, len + 1, "%s", name); + + return 0; +} + static bool of_fwnode_device_is_available(const struct fwnode_handle *fwnode) { return of_device_is_available(to_of_node(fwnode)); @@ -987,6 +997,7 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode, const struct fwnode_operations of_fwnode_ops = { .get = of_fwnode_get, .put = of_fwnode_put, + .get_name = of_fwnode_get_name, .device_is_available = of_fwnode_device_is_available, .device_get_match_data = of_fwnode_device_get_match_data, .property_present = of_fwnode_property_present,
This implements get_name fwnode op for DT. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/of/property.c | 11 +++++++++++ 1 file changed, 11 insertions(+)