Re: [PATCH 1/3] of: base: add support to get machine compatible string

[Sudeep Holla]

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.

--
Regards,
Sudeep