[PATCH] memory/tegra: Add missing of_node_get() in tegra_emc_find_node_by_ram_code

[PATCH] memory/tegra: Add missing of_node_get() in tegra_emc_find_node_by_ram_code

[Liang He]

of_find_node_by_name() will decrease the refcount of its first arg and
we need to add a of_node_put() to keep refcount balance.

Besides, during the 'for' loop execution, the refcount of 'np' will be
automatically increased and decreased. There is no need to call
of_node_put() again.

Signed-off-by: Liang He <windhl@126.com>
---
 drivers/memory/tegra/tegra20-emc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
index 25ba3c5e4ad6..e43a5f8e188e 100644
--- a/drivers/memory/tegra/tegra20-emc.c
+++ b/drivers/memory/tegra/tegra20-emc.c
@@ -477,6 +477,7 @@ tegra_emc_find_node_by_ram_code(struct tegra_emc *emc)
 
 	ram_code = tegra_read_ram_code();
 
+	of_get_node(dev->of_node);
 	for (np = of_find_node_by_name(dev->of_node, "emc-tables"); np;
 	     np = of_find_node_by_name(np, "emc-tables")) {
 		err = of_property_read_u32(np, "nvidia,ram-code", &value);
@@ -484,6 +485,7 @@ tegra_emc_find_node_by_ram_code(struct tegra_emc *emc)
 			struct device_node *lpddr2_np;
 			bool cfg_mismatches = false;
 
+			of_node_get(np);
 			lpddr2_np = of_find_node_by_name(np, "lpddr2");
 			if (lpddr2_np) {
 				const struct lpddr2_info *info;
@@ -521,7 +523,6 @@ tegra_emc_find_node_by_ram_code(struct tegra_emc *emc)
 			}
 
 			if (cfg_mismatches) {
-				of_node_put(np);
 				continue;
 			}
 		}
-- 
2.25.1

Re: [PATCH] memory/tegra: Add missing of_node_get() in tegra_emc_find_node_by_ram_code

[Krzysztof Kozlowski]

On 22/06/2022 06:28, Liang He wrote:
> of_find_node_by_name() will decrease the refcount of its first arg and
> we need to add a of_node_put() to keep refcount balance.

We do NOT need to add an of_node_put?

Anyway why do you add of_node_get which is not explained here?

> 
> Besides, during the 'for' loop execution, the refcount of 'np' will be
> automatically increased and decreased. There is no need to call
> of_node_put() again.
> 
> Signed-off-by: Liang He <windhl@126.com>
> ---
>  drivers/memory/tegra/tegra20-emc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
> index 25ba3c5e4ad6..e43a5f8e188e 100644
> --- a/drivers/memory/tegra/tegra20-emc.c
> +++ b/drivers/memory/tegra/tegra20-emc.c
> @@ -477,6 +477,7 @@ tegra_emc_find_node_by_ram_code(struct tegra_emc *emc)
>  
>  	ram_code = tegra_read_ram_code();
>  
> +	of_get_node(dev->of_node);
>  	for (np = of_find_node_by_name(dev->of_node, "emc-tables"); np;
>  	     np = of_find_node_by_name(np, "emc-tables")) {
>  		err = of_property_read_u32(np, "nvidia,ram-code", &value);
> @@ -484,6 +485,7 @@ tegra_emc_find_node_by_ram_code(struct tegra_emc *emc)
>  			struct device_node *lpddr2_np;
>  			bool cfg_mismatches = false;
>  
> +			of_node_get(np);
>  			lpddr2_np = of_find_node_by_name(np, "lpddr2");
>  			if (lpddr2_np) {
>  				const struct lpddr2_info *info;
> @@ -521,7 +523,6 @@ tegra_emc_find_node_by_ram_code(struct tegra_emc *emc)
>  			}
>  
>  			if (cfg_mismatches) {
> -				of_node_put(np);
>  				continue;
>  			}
>  		}


Best regards,
Krzysztof

Re:Re: [PATCH] memory/tegra: Add missing of_node_get() in tegra_emc_find_node_by_ram_code

[Liang He]

At 2022-06-22 14:50:41, "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> wrote:
>On 22/06/2022 06:28, Liang He wrote:
>> of_find_node_by_name() will decrease the refcount of its first arg and
>> we need to add a of_node_put() to keep refcount balance.
>
>We do NOT need to add an of_node_put?
>

Sorry, this is a typo, I want to say we need to add a 'of_node_get()'

>Anyway why do you add of_node_get which is not explained here?
>

This is a common mistake that missing the of_node_get(X) before we call
of_find_node_by_name(X,..) as the of_find_xxx will decrease the refcount of X.

A similar error is:

https://lore.kernel.org/all/20200720152806.443262648@linuxfoundation.org/

If I am wrong, please correct me, thanks.
>> 
>> Besides, during the 'for' loop execution, the refcount of 'np' will be
>> automatically increased and decreased. There is no need to call
>> of_node_put() again.
>> 
>> Signed-off-by: Liang He <windhl@126.com>
>> ---
>>  drivers/memory/tegra/tegra20-emc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
>> index 25ba3c5e4ad6..e43a5f8e188e 100644
>> --- a/drivers/memory/tegra/tegra20-emc.c
>> +++ b/drivers/memory/tegra/tegra20-emc.c
>> @@ -477,6 +477,7 @@ tegra_emc_find_node_by_ram_code(struct tegra_emc *emc)
>>  
>>  	ram_code = tegra_read_ram_code();
>>  
>> +	of_get_node(dev->of_node);
>>  	for (np = of_find_node_by_name(dev->of_node, "emc-tables"); np;
>>  	     np = of_find_node_by_name(np, "emc-tables")) {
>>  		err = of_property_read_u32(np, "nvidia,ram-code", &value);
>> @@ -484,6 +485,7 @@ tegra_emc_find_node_by_ram_code(struct tegra_emc *emc)
>>  			struct device_node *lpddr2_np;
>>  			bool cfg_mismatches = false;
>>  
>> +			of_node_get(np);
>>  			lpddr2_np = of_find_node_by_name(np, "lpddr2");
>>  			if (lpddr2_np) {
>>  				const struct lpddr2_info *info;
>> @@ -521,7 +523,6 @@ tegra_emc_find_node_by_ram_code(struct tegra_emc *emc)
>>  			}
>>  
>>  			if (cfg_mismatches) {
>> -				of_node_put(np);
>>  				continue;
>>  			}
>>  		}
>
>
>Best regards,
>Krzysztof

Re: [PATCH] memory/tegra: Add missing of_node_get() in tegra_emc_find_node_by_ram_code

[kernel test robot]

Hi Liang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tegra/for-next]
[also build test ERROR on tegra-drm/drm/tegra/for-next linus/master v5.19-rc3 next-20220621]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Liang-He/memory-tegra-Add-missing-of_node_get-in-tegra_emc_find_node_by_ram_code/20220622-123052
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
config: hexagon-randconfig-r041-20220622 (https://download.01.org/0day-ci/archive/20220622/202206221602.odN70SHs-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 8b8d126598ce7bd5243da7f94f69fa1104288bee)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/6f5dd7a2fb5c7d45a25cdf4409c5aa03c4df0a96
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Liang-He/memory-tegra-Add-missing-of_node_get-in-tegra_emc_find_node_by_ram_code/20220622-123052
        git checkout 6f5dd7a2fb5c7d45a25cdf4409c5aa03c4df0a96
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/memory/tegra/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/memory/tegra/tegra20-emc.c:480:2: error: call to undeclared function 'of_get_node'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
           of_get_node(dev->of_node);
           ^
   1 error generated.


vim +/of_get_node +480 drivers/memory/tegra/tegra20-emc.c

   456	
   457	static struct device_node *
   458	tegra_emc_find_node_by_ram_code(struct tegra_emc *emc)
   459	{
   460		struct device *dev = emc->dev;
   461		struct device_node *np;
   462		u32 value, ram_code;
   463		int err;
   464	
   465		if (emc->mrr_error) {
   466			dev_warn(dev, "memory timings skipped due to MRR error\n");
   467			return NULL;
   468		}
   469	
   470		if (of_get_child_count(dev->of_node) == 0) {
   471			dev_info_once(dev, "device-tree doesn't have memory timings\n");
   472			return NULL;
   473		}
   474	
   475		if (!of_property_read_bool(dev->of_node, "nvidia,use-ram-code"))
   476			return of_node_get(dev->of_node);
   477	
   478		ram_code = tegra_read_ram_code();
   479	
 > 480		of_get_node(dev->of_node);
   481		for (np = of_find_node_by_name(dev->of_node, "emc-tables"); np;
   482		     np = of_find_node_by_name(np, "emc-tables")) {
   483			err = of_property_read_u32(np, "nvidia,ram-code", &value);
   484			if (err || value != ram_code) {
   485				struct device_node *lpddr2_np;
   486				bool cfg_mismatches = false;
   487	
   488				of_node_get(np);
   489				lpddr2_np = of_find_node_by_name(np, "lpddr2");
   490				if (lpddr2_np) {
   491					const struct lpddr2_info *info;
   492	
   493					info = of_lpddr2_get_info(lpddr2_np, dev);
   494					if (info) {
   495						if (info->manufacturer_id >= 0 &&
   496						    info->manufacturer_id != emc->manufacturer_id)
   497							cfg_mismatches = true;
   498	
   499						if (info->revision_id1 >= 0 &&
   500						    info->revision_id1 != emc->revision_id1)
   501							cfg_mismatches = true;
   502	
   503						if (info->revision_id2 >= 0 &&
   504						    info->revision_id2 != emc->revision_id2)
   505							cfg_mismatches = true;
   506	
   507						if (info->density != emc->basic_conf4.density)
   508							cfg_mismatches = true;
   509	
   510						if (info->io_width != emc->basic_conf4.io_width)
   511							cfg_mismatches = true;
   512	
   513						if (info->arch_type != emc->basic_conf4.arch_type)
   514							cfg_mismatches = true;
   515					} else {
   516						dev_err(dev, "failed to parse %pOF\n", lpddr2_np);
   517						cfg_mismatches = true;
   518					}
   519	
   520					of_node_put(lpddr2_np);
   521				} else {
   522					cfg_mismatches = true;
   523				}
   524	
   525				if (cfg_mismatches) {
   526					continue;
   527				}
   528			}
   529	
   530			return np;
   531		}
   532	
   533		dev_err(dev, "no memory timings for RAM code %u found in device tree\n",
   534			ram_code);
   535	
   536		return NULL;
   537	}
   538	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH] memory/tegra: Add missing of_node_get() in tegra_emc_find_node_by_ram_code

[Krzysztof Kozlowski]

On 22/06/2022 06:28, Liang He wrote:
> of_find_node_by_name() will decrease the refcount of its first arg and
> we need to add a of_node_put() to keep refcount balance.
> 
> Besides, during the 'for' loop execution, the refcount of 'np' will be
> automatically increased and decreased. There is no need to call
> of_node_put() again.
> 
> Signed-off-by: Liang He <windhl@126.com>
> ---
>  drivers/memory/tegra/tegra20-emc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

The patch was not even compiled... I consider it harmful approach. NAK.
See: https://lwn.net/Articles/854645/

Best regards,
Krzysztof

Re:Re: [PATCH] memory/tegra: Add missing of_node_get() in tegra_emc_find_node_by_ram_code

[Liang He]

At 2022-06-22 16:41:28, "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> wrote:
>On 22/06/2022 06:28, Liang He wrote:
>> of_find_node_by_name() will decrease the refcount of its first arg and
>> we need to add a of_node_put() to keep refcount balance.
>> 
>> Besides, during the 'for' loop execution, the refcount of 'np' will be
>> automatically increased and decreased. There is no need to call
>> of_node_put() again.
>> 
>> Signed-off-by: Liang He <windhl@126.com>
>> ---
>>  drivers/memory/tegra/tegra20-emc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>
>The patch was not even compiled... I consider it harmful approach. NAK.
>See: https://lwn.net/Articles/854645/
>
>Best regards,
>Krzysztof

Sorry, Krzysztof.

As the patch is so simple two lines , I thought I can handle it without compiling it.

Thanks very much for your notice about the article and I hope I have not done anyother harmful things.

And I will take care of my patch code in future no matter how simple it is.

Re: [PATCH] memory/tegra: Add missing of_node_get() in tegra_emc_find_node_by_ram_code

[Krzysztof Kozlowski]

On 22/06/2022 10:49, Liang He wrote:
> 
> 
> At 2022-06-22 16:41:28, "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> wrote:
>> On 22/06/2022 06:28, Liang He wrote:
>>> of_find_node_by_name() will decrease the refcount of its first arg and
>>> we need to add a of_node_put() to keep refcount balance.
>>>
>>> Besides, during the 'for' loop execution, the refcount of 'np' will be
>>> automatically increased and decreased. There is no need to call
>>> of_node_put() again.
>>>
>>> Signed-off-by: Liang He <windhl@126.com>
>>> ---
>>>  drivers/memory/tegra/tegra20-emc.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>
>> The patch was not even compiled... I consider it harmful approach. NAK.
>> See: https://lwn.net/Articles/854645/
>>
>> Best regards,
>> Krzysztof
> 
> Sorry, Krzysztof.
> 
> As the patch is so simple two lines , I thought I can handle it without compiling it.
> 
> Thanks very much for your notice about the article and I hope I have not done anyother harmful things.
> 
> And I will take care of my patch code in future no matter how simple it is.


Never send patches which were not built. It seems you didn't build them
in the past either, even though setting up cross compile is trivial (few
commands). QEMU is also an easy option.

Even though your contribution might be valuable, lack of building is a
sign for me that approach is not serious, so I am not going to take
risky code.

Best regards,
Krzysztof

Re: [PATCH] memory/tegra: Add missing of_node_get() in tegra_emc_find_node_by_ram_code

[Krzysztof Kozlowski]

On 22/06/2022 11:08, Krzysztof Kozlowski wrote:
>>> The patch was not even compiled... I consider it harmful approach. NAK.
>>> See: https://lwn.net/Articles/854645/
>>>
>>> Best regards,
>>> Krzysztof
>>
>> Sorry, Krzysztof.
>>
>> As the patch is so simple two lines , I thought I can handle it without compiling it.
>>
>> Thanks very much for your notice about the article and I hope I have not done anyother harmful things.
>>
>> And I will take care of my patch code in future no matter how simple it is.
> 
> 
> Never send patches which were not built. It seems you didn't build them
> in the past either, even though setting up cross compile is trivial (few
> commands). QEMU is also an easy option.
> 
> Even though your contribution might be valuable, lack of building is a
> sign for me that approach is not serious, so I am not going to take
> risky code.

Unless of course someone's reviews your code. :)

Best regards,
Krzysztof

Re:Re: [PATCH] memory/tegra: Add missing of_node_get() in tegra_emc_find_node_by_ram_code

[Liang He]

At 2022-06-22 17:08:32, "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> wrote:
>On 22/06/2022 10:49, Liang He wrote:
>> 
>> 
>> At 2022-06-22 16:41:28, "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org> wrote:
>>> On 22/06/2022 06:28, Liang He wrote:
>>>> of_find_node_by_name() will decrease the refcount of its first arg and
>>>> we need to add a of_node_put() to keep refcount balance.
>>>>
>>>> Besides, during the 'for' loop execution, the refcount of 'np' will be
>>>> automatically increased and decreased. There is no need to call
>>>> of_node_put() again.
>>>>
>>>> Signed-off-by: Liang He <windhl@126.com>
>>>> ---
>>>>  drivers/memory/tegra/tegra20-emc.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>
>>> The patch was not even compiled... I consider it harmful approach. NAK.
>>> See: https://lwn.net/Articles/854645/
>>>
>>> Best regards,
>>> Krzysztof
>> 
>> Sorry, Krzysztof.
>> 
>> As the patch is so simple two lines , I thought I can handle it without compiling it.
>> 
>> Thanks very much for your notice about the article and I hope I have not done anyother harmful things.
>> 
>> And I will take care of my patch code in future no matter how simple it is.
>
>
>Never send patches which were not built. It seems you didn't build them
>in the past either, even though setting up cross compile is trivial (few
>commands). QEMU is also an easy option.
>
>Even though your contribution might be valuable, lack of building is a
>sign for me that approach is not serious, so I am not going to take
>risky code.
>
>Best regards,
>Krzysztof

Thanks, Krzysztof.

This a valuable lesson for me. 

Last few days, I just want to report the bugs as quickly as I can.

And I only focus the bugs I found but not the patch compiling problem.

Yes, I admit this approach (way) is not serious and this is my fault, sorry again and sorry to waste your time.

From now on, I will not send any new patch commit before I compile the patch code.

Sorry for my behavior.