| Message ID | 1479811311-3080-2-git-send-email-bgolaszewski@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22/11/16 10:41, Bartosz Golaszewski wrote: > Add a function allowing to retrieve the compatible string of the root > node of the device tree. > Rob has queued [1] and it's in -next today. You can reuse that if you are planning to target this for v4.11 or just use open coding in your driver for v4.10 and target this move for v4.11 to avoid cross tree dependencies as I already mentioned in your previous thread.
2016-11-22 11:53 GMT+01:00 Sudeep Holla <sudeep.holla@arm.com>: > > > On 22/11/16 10:41, Bartosz Golaszewski wrote: >> >> Add a function allowing to retrieve the compatible string of the root >> node of the device tree. >> > > Rob has queued [1] and it's in -next today. You can reuse that if you > are planning to target this for v4.11 or just use open coding in your > driver for v4.10 and target this move for v4.11 to avoid cross tree > dependencies as I already mentioned in your previous thread. > > -- > Regards, > Sudeep > > [1] http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1274549.html Rob's patch checks the model first - I'm not sure this is the behavior we want here as Sekhar suggested we print the machine compatible. Thanks, Bartosz Golaszewski
2016-11-22 11:57 GMT+01:00 Bartosz Golaszewski <bgolaszewski@baylibre.com>: > 2016-11-22 11:53 GMT+01:00 Sudeep Holla <sudeep.holla@arm.com>: >> >> >> On 22/11/16 10:41, Bartosz Golaszewski wrote: >>> >>> Add a function allowing to retrieve the compatible string of the root >>> node of the device tree. >>> >> >> Rob has queued [1] and it's in -next today. You can reuse that if you >> are planning to target this for v4.11 or just use open coding in your >> driver for v4.10 and target this move for v4.11 to avoid cross tree >> dependencies as I already mentioned in your previous thread. >> >> -- >> Regards, >> Sudeep >> >> [1] http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1274549.html > > Rob's patch checks the model first - I'm not sure this is the behavior > we want here as Sekhar suggested we print the machine compatible. > I meant your patch of course. Thanks, Bartosz
On 22/11/16 10:57, Bartosz Golaszewski wrote: > 2016-11-22 11:53 GMT+01:00 Sudeep Holla <sudeep.holla@arm.com>: >> >> >> On 22/11/16 10:41, Bartosz Golaszewski wrote: >>> >>> Add a function allowing to retrieve the compatible string of the root >>> node of the device tree. >>> >> >> Rob has queued [1] and it's in -next today. You can reuse that if you >> are planning to target this for v4.11 or just use open coding in your >> driver for v4.10 and target this move for v4.11 to avoid cross tree >> dependencies as I already mentioned in your previous thread. > > Rob's patch checks the model first - I'm not sure this is the behavior > we want here as Sekhar suggested we print the machine compatible. IIUC, you are replacing of_flat_dt_get_machine_name and of_machine_get_model_name does exactly same. So I don't see any point in adding this new function and it's just used for logging purpose. Also Sekhar just gave example by using just compatible adding that function in the driver itself. As Arnd suggested me[1], you should for v4.10 fix it in the driver itself to avoid the cross tree dependencies at this point similar to [2] -- Regards, Sudeep [1] https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg111428.html [2] http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1274502.html
On Tuesday 22 November 2016 04:36 PM, Sudeep Holla wrote: > > > On 22/11/16 10:57, Bartosz Golaszewski wrote: >> 2016-11-22 11:53 GMT+01:00 Sudeep Holla <sudeep.holla@arm.com>: >>> >>> >>> On 22/11/16 10:41, Bartosz Golaszewski wrote: >>>> >>>> Add a function allowing to retrieve the compatible string of the root >>>> node of the device tree. >>>> >>> >>> Rob has queued [1] and it's in -next today. You can reuse that if you >>> are planning to target this for v4.11 or just use open coding in your >>> driver for v4.10 and target this move for v4.11 to avoid cross tree >>> dependencies as I already mentioned in your previous thread. >> >> Rob's patch checks the model first - I'm not sure this is the behavior >> we want here as Sekhar suggested we print the machine compatible. > > IIUC, you are replacing of_flat_dt_get_machine_name and > of_machine_get_model_name does exactly same. So I don't see any point in > adding this new function and it's just used for logging purpose. > Also Sekhar just gave example by using just compatible adding that > function in the driver itself. > > As Arnd suggested me[1], you should for v4.10 fix it in the driver > itself to avoid the cross tree dependencies at this point similar to [2] +1. Bartosz, can you please fix it in the driver for v4.10. If there is an API available, we can move to it for v4.11 Thanks, Sekhar
Hi Sudeep, On Tuesday 22 November 2016 04:23 PM, Sudeep Holla wrote: > > > On 22/11/16 10:41, Bartosz Golaszewski wrote: >> Add a function allowing to retrieve the compatible string of the root >> node of the device tree. >> > > Rob has queued [1] and it's in -next today. You can reuse that if you > are planning to target this for v4.11 or just use open coding in your > driver for v4.10 and target this move for v4.11 to avoid cross tree > dependencies as I already mentioned in your previous thread. I dont have your original patch in my mailbox, but I wonder if returning a pointer to property string for a node whose reference has already been released is safe to do? Probably not an issue for the root node, but still feels counter-intuitive. This is the code for reference: +int of_machine_get_model_name(const char **model) +{ + int error; + + if (!of_node_get(of_root)) + return -EINVAL; + + error = of_property_read_string(of_root, "model", model); + if (error) + error = of_property_read_string_index(of_root, "compatible", + 0, model); + of_node_put(of_root); + + return error; +} +EXPORT_SYMBOL(of_machine_get_model_name); Thanks, Sekhar
Hi Sekhar, On 22/11/16 15:06, Sekhar Nori wrote: > Hi Sudeep, > > On Tuesday 22 November 2016 04:23 PM, Sudeep Holla wrote: >> >> >> On 22/11/16 10:41, Bartosz Golaszewski wrote: >>> Add a function allowing to retrieve the compatible string of the root >>> node of the device tree. >>> >> >> Rob has queued [1] and it's in -next today. You can reuse that if you >> are planning to target this for v4.11 or just use open coding in your >> driver for v4.10 and target this move for v4.11 to avoid cross tree >> dependencies as I already mentioned in your previous thread. > > I dont have your original patch in my mailbox, but I wonder if > returning a pointer to property string for a node whose reference has > already been released is safe to do? Probably not an issue for the root > node, but still feels counter-intuitive. > I am not sure if I understand the issue here. Are you referring a case where of_root is freed ? Also I have seen drivers today just using this pointer directly, but it's better to copy the string(I just saw this done in one case) > This is the code for reference: > > +int of_machine_get_model_name(const char **model) > +{ > + int error; > + > + if (!of_node_get(of_root)) > + return -EINVAL; > + > + error = of_property_read_string(of_root, "model", model); > + if (error) > + error = of_property_read_string_index(of_root, "compatible", > + 0, model); > + of_node_put(of_root); > + > + return error; > +} > +EXPORT_SYMBOL(of_machine_get_model_name); > > Thanks, > Sekhar >
On Tuesday 22 November 2016 09:16 PM, Sudeep Holla wrote: > Hi Sekhar, > > On 22/11/16 15:06, Sekhar Nori wrote: >> Hi Sudeep, >> >> On Tuesday 22 November 2016 04:23 PM, Sudeep Holla wrote: >>> >>> >>> On 22/11/16 10:41, Bartosz Golaszewski wrote: >>>> Add a function allowing to retrieve the compatible string of the root >>>> node of the device tree. >>>> >>> >>> Rob has queued [1] and it's in -next today. You can reuse that if you >>> are planning to target this for v4.11 or just use open coding in your >>> driver for v4.10 and target this move for v4.11 to avoid cross tree >>> dependencies as I already mentioned in your previous thread. >> >> I dont have your original patch in my mailbox, but I wonder if >> returning a pointer to property string for a node whose reference has >> already been released is safe to do? Probably not an issue for the root >> node, but still feels counter-intuitive. >> > > I am not sure if I understand the issue here. Are you referring a case > where of_root is freed ? Yes, right, thats what I was hinting at. Since you are giving up the reference to the device node before the function returns, the user can be left with a dangling reference. > Also I have seen drivers today just using this pointer directly, but > it's better to copy the string(I just saw this done in one case) Hmm, the reference is given up before the API returns, so I doubt copying it later is any additional benefit. I suspect this is a theoretical issue though since root device node is probably never freed. Thanks, Sekhar
On 23/11/16 07:49, Sekhar Nori wrote: > On Tuesday 22 November 2016 09:16 PM, Sudeep Holla wrote: >> Hi Sekhar, >> >> On 22/11/16 15:06, Sekhar Nori wrote: >>> Hi Sudeep, >>> >>> On Tuesday 22 November 2016 04:23 PM, Sudeep Holla wrote: >>>> >>>> >>>> On 22/11/16 10:41, Bartosz Golaszewski wrote: >>>>> Add a function allowing to retrieve the compatible string of the root >>>>> node of the device tree. >>>>> >>>> >>>> Rob has queued [1] and it's in -next today. You can reuse that if you >>>> are planning to target this for v4.11 or just use open coding in your >>>> driver for v4.10 and target this move for v4.11 to avoid cross tree >>>> dependencies as I already mentioned in your previous thread. >>> >>> I dont have your original patch in my mailbox, but I wonder if >>> returning a pointer to property string for a node whose reference has >>> already been released is safe to do? Probably not an issue for the root >>> node, but still feels counter-intuitive. >>> >> >> I am not sure if I understand the issue here. Are you referring a case >> where of_root is freed ? > > Yes, right, thats what I was hinting at. Since you are giving up the > reference to the device node before the function returns, the user can > be left with a dangling reference. > Yes I agree. >> Also I have seen drivers today just using this pointer directly, but >> it's better to copy the string(I just saw this done in one case) > > Hmm, the reference is given up before the API returns, so I doubt > copying it later is any additional benefit. > True. > I suspect this is a theoretical issue though since root device node is > probably never freed. > Indeed, not sure if it's worth adding additional code to release the nod at all call sites.
On Wednesday 23 November 2016 03:35 PM, Sudeep Holla wrote: > > > On 23/11/16 07:49, Sekhar Nori wrote: >> On Tuesday 22 November 2016 09:16 PM, Sudeep Holla wrote: >>> Hi Sekhar, >>> >>> On 22/11/16 15:06, Sekhar Nori wrote: >>>> Hi Sudeep, >>>> >>>> On Tuesday 22 November 2016 04:23 PM, Sudeep Holla wrote: >>>>> >>>>> >>>>> On 22/11/16 10:41, Bartosz Golaszewski wrote: >>>>>> Add a function allowing to retrieve the compatible string of the root >>>>>> node of the device tree. >>>>>> >>>>> >>>>> Rob has queued [1] and it's in -next today. You can reuse that if you >>>>> are planning to target this for v4.11 or just use open coding in your >>>>> driver for v4.10 and target this move for v4.11 to avoid cross tree >>>>> dependencies as I already mentioned in your previous thread. >>>> >>>> I dont have your original patch in my mailbox, but I wonder if >>>> returning a pointer to property string for a node whose reference has >>>> already been released is safe to do? Probably not an issue for the root >>>> node, but still feels counter-intuitive. >>>> >>> >>> I am not sure if I understand the issue here. Are you referring a case >>> where of_root is freed ? >> >> Yes, right, thats what I was hinting at. Since you are giving up the >> reference to the device node before the function returns, the user can >> be left with a dangling reference. >> > > Yes I agree. So, the if(!of_node_get()) is just an expensive NULL pointer check. I think it is better to be explicit about it by not using of_node_get/put() at all. How about: +int of_machine_get_model_name(const char **model) +{ + int error; + + if (!of_root) + return -EINVAL; + + error = of_property_read_string(of_root, "model", model); + if (error) + error = of_property_read_string_index(of_root, "compatible", + 0, model); + return error; +} +EXPORT_SYMBOL(of_machine_get_model_name); I know the patch is already in -next so I guess it depends on how strongly Rob feels about this. Thanks, Sekhar
On 23/11/16 11:47, Sekhar Nori wrote: > On Wednesday 23 November 2016 03:35 PM, Sudeep Holla wrote: >> >> >> On 23/11/16 07:49, Sekhar Nori wrote: >>> On Tuesday 22 November 2016 09:16 PM, Sudeep Holla wrote: >>>> Hi Sekhar, >>>> >>>> On 22/11/16 15:06, Sekhar Nori wrote: >>>>> Hi Sudeep, >>>>> >>>>> On Tuesday 22 November 2016 04:23 PM, Sudeep Holla wrote: >>>>>> >>>>>> >>>>>> On 22/11/16 10:41, Bartosz Golaszewski wrote: >>>>>>> Add a function allowing to retrieve the compatible string of the root >>>>>>> node of the device tree. >>>>>>> >>>>>> >>>>>> Rob has queued [1] and it's in -next today. You can reuse that if you >>>>>> are planning to target this for v4.11 or just use open coding in your >>>>>> driver for v4.10 and target this move for v4.11 to avoid cross tree >>>>>> dependencies as I already mentioned in your previous thread. >>>>> >>>>> I dont have your original patch in my mailbox, but I wonder if >>>>> returning a pointer to property string for a node whose reference has >>>>> already been released is safe to do? Probably not an issue for the root >>>>> node, but still feels counter-intuitive. >>>>> >>>> >>>> I am not sure if I understand the issue here. Are you referring a case >>>> where of_root is freed ? >>> >>> Yes, right, thats what I was hinting at. Since you are giving up the >>> reference to the device node before the function returns, the user can >>> be left with a dangling reference. >>> >> >> Yes I agree. > > So, the if(!of_node_get()) is just an expensive NULL pointer check. I think > it is better to be explicit about it by not using of_node_get/put() at all. > How about: > Are we planning to use this in any time sensitive paths? Anyways I am fine removing them. > +int of_machine_get_model_name(const char **model) > +{ > + int error; > + > + if (!of_root) > + return -EINVAL; > + > + error = of_property_read_string(of_root, "model", model); > + if (error) > + error = of_property_read_string_index(of_root, "compatible", > + 0, model); > + return error; > +} > +EXPORT_SYMBOL(of_machine_get_model_name); > > I know the patch is already in -next so I guess it depends on how strongly > Rob feels about this. Frank expressed his concerns and it may be reverted.
On Wednesday 23 November 2016 05:37 PM, Sudeep Holla wrote: >> So, the if(!of_node_get()) is just an expensive NULL pointer check. I >> think >> it is better to be explicit about it by not using of_node_get/put() at >> all. >> How about: >> > > Are we planning to use this in any time sensitive paths? Anyways I am > fine removing them. Not worried about the time taken as much as it serving as a bad example and getting carried over to other places where the impact might actually be real. > >> +int of_machine_get_model_name(const char **model) >> +{ >> + int error; >> + >> + if (!of_root) >> + return -EINVAL; >> + >> + error = of_property_read_string(of_root, "model", model); >> + if (error) >> + error = of_property_read_string_index(of_root, >> "compatible", >> + 0, model); >> + return error; >> +} >> +EXPORT_SYMBOL(of_machine_get_model_name); >> >> I know the patch is already in -next so I guess it depends on how >> strongly >> Rob feels about this. > > Frank expressed his concerns and it may be reverted. Didn't notice that. I will check that thread. Thanks! Regards, Sekhar
On 23/11/16 12:13, Sekhar Nori wrote: > On Wednesday 23 November 2016 05:37 PM, Sudeep Holla wrote: >>> So, the if(!of_node_get()) is just an expensive NULL pointer check. I >>> think >>> it is better to be explicit about it by not using of_node_get/put() at >>> all. >>> How about: >>> >> >> Are we planning to use this in any time sensitive paths? Anyways I am >> fine removing them. > > Not worried about the time taken as much as it serving as a bad example > and getting carried over to other places where the impact might actually > be real. > Ah OK, sure.
diff --git a/drivers/of/base.c b/drivers/of/base.c index a0bccb5..bbfe5e9 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -546,6 +546,28 @@ int of_machine_is_compatible(const char *compat) EXPORT_SYMBOL(of_machine_is_compatible); /** + * of_machine_get_compatible - Get the compatible property of the root node + * + * Returns a NULL-terminated string containing the compatible if it could + * be found, NULL otherwise. + */ +const char *of_machine_get_compatible(void) +{ + struct device_node *root; + const char *compatible; + int ret = -1; + + root = of_find_node_by_path("/"); + if (root) { + ret = of_property_read_string(root, "compatible", &compatible); + of_node_put(root); + } + + return ret ? NULL : compatible; +} +EXPORT_SYMBOL(of_machine_get_compatible); + +/** * __of_device_is_available - check if a device is available for use * * @device: Node to check for availability, with locks already held diff --git a/include/linux/of.h b/include/linux/of.h index 299aeb1..664b734 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -367,6 +367,7 @@ extern int of_alias_get_id(struct device_node *np, const char *stem); extern int of_alias_get_highest_id(const char *stem); extern int of_machine_is_compatible(const char *compat); +extern const char *of_machine_get_compatible(void); extern int of_add_property(struct device_node *np, struct property *prop); extern int of_remove_property(struct device_node *np, struct property *prop); @@ -788,6 +789,11 @@ static inline int of_machine_is_compatible(const char *compat) return 0; } +static inline const char *of_machine_get_compatible(void) +{ + return NULL; +} + static inline bool of_console_check(const struct device_node *dn, const char *name, int index) { return false;
Add a function allowing to retrieve the compatible string of the root node of the device tree. Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> --- drivers/of/base.c | 22 ++++++++++++++++++++++ include/linux/of.h | 6 ++++++ 2 files changed, 28 insertions(+)