[PATCH v2] ufs: host: ufschd-pltfrm: Hold reference returned by of_parse_phandle()

[PATCH v2] ufs: host: ufschd-pltfrm: Hold reference returned by of_parse_phandle()

[Liang He]

In ufshcd_populate_vreg(), we should hold the reference returned by
of_parse_phandle() and then use it to call of_node_put() for refcount
balance.

Fixes: aa4976130934 ("ufs: Add regulator enable support")
Signed-off-by: Liang He <windhl@126.com>
---
 changelog:

 v2: use a proper helper name advised by Bart Van Assche.
 v1: add a helper to fix the bug

 v1 link: https://lore.kernel.org/all/20220714055413.373449-1-windhl@126.com/


 drivers/ufs/host/ufshcd-pltfrm.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
index e7332cc65b1f..46ded7813c42 100644
--- a/drivers/ufs/host/ufshcd-pltfrm.c
+++ b/drivers/ufs/host/ufshcd-pltfrm.c
@@ -108,6 +108,21 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
 	return ret;
 }
 
+static bool phandle_exists(const struct device_node *np,
+						const char *phandle_name,
+						int index)
+{
+	struct device_node *parse_np = of_parse_phandle(np, phandle_name, index);
+	bool ret = false;
+
+	if (parse_np) {
+		ret = true;
+		of_node_put(parse_np);
+	}
+
+	return ret;
+}
+
 #define MAX_PROP_SIZE 32
 static int ufshcd_populate_vreg(struct device *dev, const char *name,
 		struct ufs_vreg **out_vreg)
@@ -122,7 +137,7 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name,
 	}
 
 	snprintf(prop_name, MAX_PROP_SIZE, "%s-supply", name);
-	if (!of_parse_phandle(np, prop_name, 0)) {
+	if (!phandle_exists(np, prop_name, 0)) {
 		dev_info(dev, "%s: Unable to find %s regulator, assuming enabled\n",
 				__func__, prop_name);
 		goto out;
-- 
2.25.1

Re: [PATCH v2] ufs: host: ufschd-pltfrm: Hold reference returned by of_parse_phandle()

[Bart Van Assche]

On 7/14/22 17:17, Liang He wrote:
> +static bool phandle_exists(const struct device_node *np,
> +						const char *phandle_name,
> +						int index)

Indentation of the arguments now looks really odd :-(

> +{
> +	struct device_node *parse_np = of_parse_phandle(np, phandle_name, index);
> +	bool ret = false;
> +
> +	if (parse_np) {
> +		ret = true;
> +		of_node_put(parse_np);
> +	}
> +
> +	return ret;
> +}

The 'ret' variable is not necessary. If "return ret" is changed into 
"return parse_np" then the variable 'ret' can be left out.

Thanks,

Bart.

Re:Re: [PATCH v2] ufs: host: ufschd-pltfrm: Hold reference returned by of_parse_phandle()

[Liang He]

At 2022-07-16 21:50:08, "Bart Van Assche" <bvanassche@acm.org> wrote:
>On 7/14/22 17:17, Liang He wrote:
>> +static bool phandle_exists(const struct device_node *np,
>> +						const char *phandle_name,
>> +						int index)
>
>Indentation of the arguments now looks really odd :-(

Yes, Bart, I also wonder this coding style, however I learned that 
from the definition of 'of_parse_phandle' in of.h.

Is it OK if I put all of them in one line？

Thanks,

>
>> +{
>> +	struct device_node *parse_np = of_parse_phandle(np, phandle_name, index);
>> +	bool ret = false;
>> +
>> +	if (parse_np) {
>> +		ret = true;
>> +		of_node_put(parse_np);
>> +	}
>> +
>> +	return ret;
>> +}
>
>The 'ret' variable is not necessary. If "return ret" is changed into 
>"return parse_np" then the variable 'ret' can be left out.
>

OK, I will use 'return parse_np' in new version when you confirm above coding style.

>Thanks,
>
>Bart.

Thanks, 

Liang

Re: [PATCH v2] ufs: host: ufschd-pltfrm: Hold reference returned by of_parse_phandle()

[Bart Van Assche]

On 7/16/22 20:03, Liang He wrote:
> At 2022-07-16 21:50:08, "Bart Van Assche" <bvanassche@acm.org> wrote:
>> On 7/14/22 17:17, Liang He wrote:
>>> +static bool phandle_exists(const struct device_node *np,
>>> +						const char *phandle_name,
>>> +						int index)
>>
>> Indentation of the arguments now looks really odd :-(
> 
> Yes, Bart, I also wonder this coding style, however I learned that
> from the definition of 'of_parse_phandle' in of.h.
> 
> Is it OK if I put all of them in one line？

No. From Documentation/process/coding-style.rst (please read that 
document in its entirety): "The preferred limit on the length of a 
single line is 80 columns. [...] A very commonly used style
is to align descendants to a function open parenthesis."

Consider to use the following formatting:

static bool phandle_exists(const struct device_node *np,
			   const char *phandle_name, int index)
{
	[ ... ]
}

>>> +{
>>> +	struct device_node *parse_np = of_parse_phandle(np, phandle_name, index);
>>> +	bool ret = false;
>>> +
>>> +	if (parse_np) {
>>> +		ret = true;
>>> +		of_node_put(parse_np);
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>
>> The 'ret' variable is not necessary. If "return ret" is changed into
>> "return parse_np" then the variable 'ret' can be left out.
>>
> 
> OK, I will use 'return parse_np' in new version when you confirm above coding style.

You may want to use "return parse_np != NULL" if you want to be sure 
that nobody else will complain about an implicit conversion of a pointer 
to a boolean type.

Thanks,

Bart.

Re:Re: [PATCH v2] ufs: host: ufschd-pltfrm: Hold reference returned by of_parse_phandle()

[Liang He]

At 2022-07-17 22:58:45, "Bart Van Assche" <bvanassche@acm.org> wrote:
>On 7/16/22 20:03, Liang He wrote:
>> At 2022-07-16 21:50:08, "Bart Van Assche" <bvanassche@acm.org> wrote:
>>> On 7/14/22 17:17, Liang He wrote:
>>>> +static bool phandle_exists(const struct device_node *np,
>>>> +						const char *phandle_name,
>>>> +						int index)
>>>
>>> Indentation of the arguments now looks really odd :-(
>> 
>> Yes, Bart, I also wonder this coding style, however I learned that
>> from the definition of 'of_parse_phandle' in of.h.
>> 
>> Is it OK if I put all of them in one line？
>
>No. From Documentation/process/coding-style.rst (please read that 
>document in its entirety): "The preferred limit on the length of a 
>single line is 80 columns. [...] A very commonly used style
>is to align descendants to a function open parenthesis."
>
>Consider to use the following formatting:
>
>static bool phandle_exists(const struct device_node *np,
>			   const char *phandle_name, int index)
>{
>	[ ... ]
>}
>

Thanks very much, Bart,

I will follow your advice and I will read carefully the Documentation/process/coding-style.rst

Liang

>>>> +{
>>>> +	struct device_node *parse_np = of_parse_phandle(np, phandle_name, index);
>>>> +	bool ret = false;
>>>> +
>>>> +	if (parse_np) {
>>>> +		ret = true;
>>>> +		of_node_put(parse_np);
>>>> +	}
>>>> +
>>>> +	return ret;
>>>> +}
>>>
>>> The 'ret' variable is not necessary. If "return ret" is changed into
>>> "return parse_np" then the variable 'ret' can be left out.
>>>
>> 
>> OK, I will use 'return parse_np' in new version when you confirm above coding style.
>
>You may want to use "return parse_np != NULL" if you want to be sure 
>that nobody else will complain about an implicit conversion of a pointer 
>to a boolean type.
>
OK, I think 'return parse_np != NULL' is better.

I will prepare new version patch soon.

>Thanks,
>
>Bart.

Thanks, 
Liang

Re:Re: [PATCH v2] ufs: host: ufschd-pltfrm: Hold reference returned by of_parse_phandle()

[Liang He]

At 2022-07-17 22:58:45, "Bart Van Assche" <bvanassche@acm.org> wrote:
>On 7/16/22 20:03, Liang He wrote:
>> At 2022-07-16 21:50:08, "Bart Van Assche" <bvanassche@acm.org> wrote:
>>> On 7/14/22 17:17, Liang He wrote:
>>>> +static bool phandle_exists(const struct device_node *np,
>>>> +						const char *phandle_name,
>>>> +						int index)
>>>
>>> Indentation of the arguments now looks really odd :-(
>> 
>> Yes, Bart, I also wonder this coding style, however I learned that
>> from the definition of 'of_parse_phandle' in of.h.
>> 
>> Is it OK if I put all of them in one line？
>
>No. From Documentation/process/coding-style.rst (please read that 
>document in its entirety): "The preferred limit on the length of a 
>single line is 80 columns. [...] A very commonly used style
>is to align descendants to a function open parenthesis."
>
>Consider to use the following formatting:
>
>static bool phandle_exists(const struct device_node *np,
>			   const char *phandle_name, int index)
>{
>	[ ... ]
>}
>

Hi, Bart, 

Can you help me as I have a trouble about the indentation.

When I align descendants to a function open parenthesis in VIM editor,
but when I generate the patch, I find the second line always missing one space in
patch format. So is there any problem if I send this patch?

I make sure that the alignment in VIM is OK.

Thanks, 
Liang


>>>> +{
>>>> +	struct device_node *parse_np = of_parse_phandle(np, phandle_name, index);
>>>> +	bool ret = false;
>>>> +
>>>> +	if (parse_np) {
>>>> +		ret = true;
>>>> +		of_node_put(parse_np);
>>>> +	}
>>>> +
>>>> +	return ret;
>>>> +}
>>>
>>> The 'ret' variable is not necessary. If "return ret" is changed into
>>> "return parse_np" then the variable 'ret' can be left out.
>>>
>> 
>> OK, I will use 'return parse_np' in new version when you confirm above coding style.
>
>You may want to use "return parse_np != NULL" if you want to be sure 
>that nobody else will complain about an implicit conversion of a pointer 
>to a boolean type.
>
>Thanks,
>
>Bart.

Re: [PATCH v2] ufs: host: ufschd-pltfrm: Hold reference returned by of_parse_phandle()

[Bart Van Assche]

On 7/18/22 19:46, Liang He wrote:
> Can you help me as I have a trouble about the indentation.
> 
> When I align descendants to a function open parenthesis in VIM editor,
> but when I generate the patch, I find the second line always missing one space in
> patch format. So is there any problem if I send this patch?
> 
> I make sure that the alignment in VIM is OK.

Hi Liang,

Please don't worry about this. When a code change is converted into a 
patch, a single character is inserted at the start of each line (plus, 
minus or space). That makes code that is indented with tabs look weird. 
This is normal.

Bart.

Re:Re: [PATCH v2] ufs: host: ufschd-pltfrm: Hold reference returned by of_parse_phandle()

[Liang He]

At 2022-07-20 02:12:36, "Bart Van Assche" <bvanassche@acm.org> wrote:
>On 7/18/22 19:46, Liang He wrote:
>> Can you help me as I have a trouble about the indentation.
>> 
>> When I align descendants to a function open parenthesis in VIM editor,
>> but when I generate the patch, I find the second line always missing one space in
>> patch format. So is there any problem if I send this patch?
>> 
>> I make sure that the alignment in VIM is OK.
>
>Hi Liang,
>
>Please don't worry about this. When a code change is converted into a 
>patch, a single character is inserted at the start of each line (plus, 
>minus or space). That makes code that is indented with tabs look weird. 
>This is normal.
>
>Bart.

Thanks very much for all your help and your effort to review my code.

Liang