Re: [PATCH] jailhouse: Hold reference returned from of_find_xxx API

Re: [PATCH] jailhouse: Hold reference returned from of_find_xxx API

[Juergen Gross via Virtualization]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 457 bytes --]

On 15.09.22 04:23, Liang He wrote:
> In jailhouse_paravirt(), we should hold the reference returned from
> of_find_compatible_node() which has increased the refcount and then
> call of_node_put() with it when done.
> 
> Fixes: 63338a38db95 ("jailhouse: Provide detection for non-x86 systems")
> Signed-off-by: Liang He <windhl@126.com>
> Signed-off-by: Kelin Wang <wangkelin2023@163.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] jailhouse: Hold reference returned from of_find_xxx API

[Srivatsa S. Bhat]

[ Adding author and reviewers of commit 63338a38db95 ]

On 9/14/22 7:23 PM, Liang He wrote:
> In jailhouse_paravirt(), we should hold the reference returned from
> of_find_compatible_node() which has increased the refcount and then
> call of_node_put() with it when done.
> 
> Fixes: 63338a38db95 ("jailhouse: Provide detection for non-x86 systems")
> Signed-off-by: Liang He <windhl@126.com>
> Signed-off-by: Kelin Wang <wangkelin2023@163.com>
> ---
>  include/linux/hypervisor.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/hypervisor.h b/include/linux/hypervisor.h
> index 9efbc54e35e5..7fe1e8c6211c 100644
> --- a/include/linux/hypervisor.h
> +++ b/include/linux/hypervisor.h
> @@ -27,7 +27,11 @@ static inline void hypervisor_pin_vcpu(int cpu)
>  
>  static inline bool jailhouse_paravirt(void)
>  {
> -	return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> +	struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> +
> +	of_node_put(np);
> +
> +	return np;
>  }
>  

Thank you for the fix, but returning a pointer from a function with a
bool return type looks odd. Can we also fix that up please?


Regards,
Srivatsa
VMware Photon OS
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] jailhouse: Hold reference returned from of_find_xxx API

[Srivatsa S. Bhat]

[ Adding author and reviewers of commit 63338a38db95 ]

On 9/14/22 7:23 PM, Liang He wrote:
> In jailhouse_paravirt(), we should hold the reference returned from
> of_find_compatible_node() which has increased the refcount and then
> call of_node_put() with it when done.
> 
> Fixes: 63338a38db95 ("jailhouse: Provide detection for non-x86 systems")
> Signed-off-by: Liang He <windhl@126.com>
> Signed-off-by: Kelin Wang <wangkelin2023@163.com>
> ---
>  include/linux/hypervisor.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/hypervisor.h b/include/linux/hypervisor.h
> index 9efbc54e35e5..7fe1e8c6211c 100644
> --- a/include/linux/hypervisor.h
> +++ b/include/linux/hypervisor.h
> @@ -27,7 +27,11 @@ static inline void hypervisor_pin_vcpu(int cpu)
>  
>  static inline bool jailhouse_paravirt(void)
>  {
> -	return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> +	struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> +
> +	of_node_put(np);
> +
> +	return np;
>  }
>  

Thank you for the fix, but returning a pointer from a function with a
bool return type looks odd. Can we also fix that up please?


Regards,
Srivatsa
VMware Photon OS

Re:Re: [PATCH] jailhouse: Hold reference returned from of_find_xxx API

[Liang He]

At 2022-09-16 07:29:06, "Srivatsa S. Bhat" <srivatsa@csail.mit.edu> wrote:
>
>[ Adding author and reviewers of commit 63338a38db95 ]
>
>On 9/14/22 7:23 PM, Liang He wrote:
>> In jailhouse_paravirt(), we should hold the reference returned from
>> of_find_compatible_node() which has increased the refcount and then
>> call of_node_put() with it when done.
>> 
>> Fixes: 63338a38db95 ("jailhouse: Provide detection for non-x86 systems")
>> Signed-off-by: Liang He <windhl@126.com>
>> Signed-off-by: Kelin Wang <wangkelin2023@163.com>
>> ---
>>  include/linux/hypervisor.h | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/include/linux/hypervisor.h b/include/linux/hypervisor.h
>> index 9efbc54e35e5..7fe1e8c6211c 100644
>> --- a/include/linux/hypervisor.h
>> +++ b/include/linux/hypervisor.h
>> @@ -27,7 +27,11 @@ static inline void hypervisor_pin_vcpu(int cpu)
>>  
>>  static inline bool jailhouse_paravirt(void)
>>  {
>> -	return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
>> +	struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
>> +
>> +	of_node_put(np);
>> +
>> +	return np;
>>  }
>>  
>
>Thank you for the fix, but returning a pointer from a function with a
>bool return type looks odd. Can we also fix that up please?
>

Thanks for your review, how about following patch:

-	return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
+	struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
+
+	of_node_put(np);
+
+	return (np==NULL);

>
>Regards,
>Srivatsa
>VMware Photon OS

Re: Re: [PATCH] jailhouse: Hold reference returned from of_find_xxx API

[Andy Shevchenko]

On Fri, Sep 16, 2022 at 5:02 AM Liang He <windhl@126.com> wrote:
> At 2022-09-16 07:29:06, "Srivatsa S. Bhat" <srivatsa@csail.mit.edu> wrote:
> >On 9/14/22 7:23 PM, Liang He wrote:

..

> >>  static inline bool jailhouse_paravirt(void)
> >>  {
> >> -    return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> +    struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> +
> >> +    of_node_put(np);
> >> +
> >> +    return np;
> >>  }
> >
> >Thank you for the fix, but returning a pointer from a function with a
> >bool return type looks odd. Can we also fix that up please?
> >
>
> Thanks for your review, how about following patch:
>
> -       return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> +       struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> +
> +       of_node_put(np);
> +
> +       return (np==NULL);

This will be opposite to the above. Perhaps you wanted

  return  !!np;

Also possible (but why?)

  return np ? true : false;

-- 
With Best Regards,
Andy Shevchenko

Re: Re: [PATCH] jailhouse: Hold reference returned from of_find_xxx API

[Andy Shevchenko]

On Fri, Sep 16, 2022 at 5:02 AM Liang He <windhl@126.com> wrote:
> At 2022-09-16 07:29:06, "Srivatsa S. Bhat" <srivatsa@csail.mit.edu> wrote:
> >On 9/14/22 7:23 PM, Liang He wrote:

..

> >>  static inline bool jailhouse_paravirt(void)
> >>  {
> >> -    return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> +    struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> +
> >> +    of_node_put(np);
> >> +
> >> +    return np;
> >>  }
> >
> >Thank you for the fix, but returning a pointer from a function with a
> >bool return type looks odd. Can we also fix that up please?
> >
>
> Thanks for your review, how about following patch:
>
> -       return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> +       struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> +
> +       of_node_put(np);
> +
> +       return (np==NULL);

This will be opposite to the above. Perhaps you wanted

  return  !!np;

Also possible (but why?)

  return np ? true : false;

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re:Re: Re: [PATCH] jailhouse: Hold reference returned from of_find_xxx API

[Liang He]

At 2022-09-16 13:38:39, "Andy Shevchenko" <andy.shevchenko@gmail.com> wrote:
>On Fri, Sep 16, 2022 at 5:02 AM Liang He <windhl@126.com> wrote:
>> At 2022-09-16 07:29:06, "Srivatsa S. Bhat" <srivatsa@csail.mit.edu> wrote:
>> >On 9/14/22 7:23 PM, Liang He wrote:
>
>..
>
>> >>  static inline bool jailhouse_paravirt(void)
>> >>  {
>> >> -    return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
>> >> +    struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
>> >> +
>> >> +    of_node_put(np);
>> >> +
>> >> +    return np;
>> >>  }
>> >
>> >Thank you for the fix, but returning a pointer from a function with a
>> >bool return type looks odd. Can we also fix that up please?
>> >
>>
>> Thanks for your review, how about following patch:
>>
>> -       return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
>> +       struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
>> +
>> +       of_node_put(np);
>> +
>> +       return (np==NULL);
>

>This will be opposite to the above. Perhaps you wanted

Sorry, I wanted to use 'np!=NULL'

>  return  !!np;
>
>Also possible (but why?)
>
>  return np ? true : false;

>

So, can I chose 'return np?true: false;' as the final patch?


Thanks,


Liang




>-- >With Best Regards,
>Andy Shevchenko

Re: Re: Re: [PATCH] jailhouse: Hold reference returned from of_find_xxx API

[Andy Shevchenko]

On Fri, Sep 16, 2022 at 10:09 AM Liang He <windhl@126.com> wrote:
> At 2022-09-16 13:38:39, "Andy Shevchenko" <andy.shevchenko@gmail.com> wrote:
> >On Fri, Sep 16, 2022 at 5:02 AM Liang He <windhl@126.com> wrote:
> >> At 2022-09-16 07:29:06, "Srivatsa S. Bhat" <srivatsa@csail.mit.edu> wrote:
> >> >On 9/14/22 7:23 PM, Liang He wrote:

...

> >> >>  static inline bool jailhouse_paravirt(void)
> >> >>  {
> >> >> -    return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> >> +    struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> >> +
> >> >> +    of_node_put(np);
> >> >> +
> >> >> +    return np;
> >> >>  }
> >> >
> >> >Thank you for the fix, but returning a pointer from a function with a
> >> >bool return type looks odd. Can we also fix that up please?
> >>
> >> Thanks for your review, how about following patch:
> >>
> >> -       return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> +       struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> +
> >> +       of_node_put(np);
> >> +
> >> +       return (np==NULL);
>
> >This will be opposite to the above. Perhaps you wanted
>
> Sorry, I wanted to use 'np!=NULL'
>
> >  return  !!np;
> >
> >Also possible (but why?)
> >
> >  return np ? true : false;
>
> So, can I chose 'return np?true: false;' as the final patch?

Of course you can, it's up to the maintainer(s) what to accept.

-- 
With Best Regards,
Andy Shevchenko

Re: Re: Re: [PATCH] jailhouse: Hold reference returned from of_find_xxx API

[Andy Shevchenko]

On Fri, Sep 16, 2022 at 10:09 AM Liang He <windhl@126.com> wrote:
> At 2022-09-16 13:38:39, "Andy Shevchenko" <andy.shevchenko@gmail.com> wrote:
> >On Fri, Sep 16, 2022 at 5:02 AM Liang He <windhl@126.com> wrote:
> >> At 2022-09-16 07:29:06, "Srivatsa S. Bhat" <srivatsa@csail.mit.edu> wrote:
> >> >On 9/14/22 7:23 PM, Liang He wrote:

...

> >> >>  static inline bool jailhouse_paravirt(void)
> >> >>  {
> >> >> -    return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> >> +    struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> >> +
> >> >> +    of_node_put(np);
> >> >> +
> >> >> +    return np;
> >> >>  }
> >> >
> >> >Thank you for the fix, but returning a pointer from a function with a
> >> >bool return type looks odd. Can we also fix that up please?
> >>
> >> Thanks for your review, how about following patch:
> >>
> >> -       return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> +       struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> +
> >> +       of_node_put(np);
> >> +
> >> +       return (np==NULL);
>
> >This will be opposite to the above. Perhaps you wanted
>
> Sorry, I wanted to use 'np!=NULL'
>
> >  return  !!np;
> >
> >Also possible (but why?)
> >
> >  return np ? true : false;
>
> So, can I chose 'return np?true: false;' as the final patch?

Of course you can, it's up to the maintainer(s) what to accept.

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re:Re: Re: Re: [PATCH] jailhouse: Hold reference returned from of_find_xxx API

[Liang He]

At 2022-09-16 16:46:36, "Andy Shevchenko" <andy.shevchenko@gmail.com> wrote:
>On Fri, Sep 16, 2022 at 10:09 AM Liang He <windhl@126.com> wrote:
>> At 2022-09-16 13:38:39, "Andy Shevchenko" <andy.shevchenko@gmail.com> wrote:
>> >On Fri, Sep 16, 2022 at 5:02 AM Liang He <windhl@126.com> wrote:
>> >> At 2022-09-16 07:29:06, "Srivatsa S. Bhat" <srivatsa@csail.mit.edu> wrote:
>> >> >On 9/14/22 7:23 PM, Liang He wrote:
>
>...
>
>> >> >>  static inline bool jailhouse_paravirt(void)
>> >> >>  {
>> >> >> -    return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
>> >> >> +    struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
>> >> >> +
>> >> >> +    of_node_put(np);
>> >> >> +
>> >> >> +    return np;
>> >> >>  }
>> >> >
>> >> >Thank you for the fix, but returning a pointer from a function with a
>> >> >bool return type looks odd. Can we also fix that up please?
>> >>
>> >> Thanks for your review, how about following patch:
>> >>
>> >> -       return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
>> >> +       struct device_node *np = of_find_compatible_node(NULL, NULL, "jailhouse,cell");
>> >> +
>> >> +       of_node_put(np);
>> >> +
>> >> +       return (np==NULL);
>>
>> >This will be opposite to the above. Perhaps you wanted
>>
>> Sorry, I wanted to use 'np!=NULL'
>>
>> >  return  !!np;
>> >
>> >Also possible (but why?)
>> >
>> >  return np ? true : false;
>>
>> So, can I chose 'return np?true: false;' as the final patch?
>
>Of course you can, it's up to the maintainer(s) what to accept.
>
>-- 
>With Best Regards,
>Andy Shevchenko

Thanks, I will do it now.