| Message ID | 1479811311-3080-2-git-send-email-bgolaszewski@baylibre.com |
|---|---|
| State | New |
| Headers | show |
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
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 > -- Regards, Sudeep
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. -- Regards, Sudeep
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(+) -- 2.9.3