thunderbolt: Use common error handling code in update_property_block()

[PATCH] thunderbolt: Use common error handling code in update_property_block()

Posted by Markus Elfring 1 year, 4 months ago

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 25 Sep 2024 09:39:16 +0200

Add a jump target so that a bit of exception handling can be better reused
at the end of this function implementation.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/thunderbolt/xdomain.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
index 11a50c86a1e4..8e3cf95ca99c 100644
--- a/drivers/thunderbolt/xdomain.c
+++ b/drivers/thunderbolt/xdomain.c
@@ -670,23 +670,19 @@ static void update_property_block(struct tb_xdomain *xd)
 		ret = tb_property_format_dir(dir, NULL, 0);
 		if (ret < 0) {
 			dev_warn(&xd->dev, "local property block creation failed\n");
-			tb_property_free_dir(dir);
-			goto out_unlock;
+			goto out_free_dir;
 		}

 		block_len = ret;
 		block = kcalloc(block_len, sizeof(*block), GFP_KERNEL);
-		if (!block) {
-			tb_property_free_dir(dir);
-			goto out_unlock;
-		}
+		if (!block)
+			goto out_free_dir;

 		ret = tb_property_format_dir(dir, block, block_len);
 		if (ret) {
 			dev_warn(&xd->dev, "property block generation failed\n");
-			tb_property_free_dir(dir);
 			kfree(block);
-			goto out_unlock;
+			goto out_free_dir;
 		}

 		tb_property_free_dir(dir);
@@ -701,6 +697,11 @@ static void update_property_block(struct tb_xdomain *xd)
 out_unlock:
 	mutex_unlock(&xd->lock);
 	mutex_unlock(&xdomain_lock);
+	return;
+
+out_free_dir:
+	tb_property_free_dir(dir);
+	goto out_unlock;
 }

 static void start_handshake(struct tb_xdomain *xd)
--
2.46.1

Re: [PATCH] thunderbolt: Use common error handling code in update_property_block()

Posted by kernel test robot 1 year, 4 months ago

Hi Markus,

kernel test robot noticed the following build errors:

[auto build test ERROR on westeri-thunderbolt/next]
[also build test ERROR on linus/master v6.11 next-20240925]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Markus-Elfring/thunderbolt-Use-common-error-handling-code-in-update_property_block/20240925-161308
base:   https://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git next
patch link:    https://lore.kernel.org/r/26b7f215-4f83-413c-9dab-737d790053c0%40web.de
patch subject: [PATCH] thunderbolt: Use common error handling code in update_property_block()
config: x86_64-randconfig-001-20240926 (https://download.01.org/0day-ci/archive/20240926/202409260928.qBlg2dBU-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240926/202409260928.qBlg2dBU-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409260928.qBlg2dBU-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/thunderbolt/xdomain.c:703:23: error: use of undeclared identifier 'dir'
     703 |         tb_property_free_dir(dir);
         |                              ^
   1 error generated.


vim +/dir +703 drivers/thunderbolt/xdomain.c

   645	
   646	static void update_property_block(struct tb_xdomain *xd)
   647	{
   648		mutex_lock(&xdomain_lock);
   649		mutex_lock(&xd->lock);
   650		/*
   651		 * If the local property block is not up-to-date, rebuild it now
   652		 * based on the global property template.
   653		 */
   654		if (!xd->local_property_block ||
   655		    xd->local_property_block_gen < xdomain_property_block_gen) {
   656			struct tb_property_dir *dir;
   657			int ret, block_len;
   658			u32 *block;
   659	
   660			dir = tb_property_copy_dir(xdomain_property_dir);
   661			if (!dir) {
   662				dev_warn(&xd->dev, "failed to copy properties\n");
   663				goto out_unlock;
   664			}
   665	
   666			/* Fill in non-static properties now */
   667			tb_property_add_text(dir, "deviceid", utsname()->nodename);
   668			tb_property_add_immediate(dir, "maxhopid", xd->local_max_hopid);
   669	
   670			ret = tb_property_format_dir(dir, NULL, 0);
   671			if (ret < 0) {
   672				dev_warn(&xd->dev, "local property block creation failed\n");
   673				goto out_free_dir;
   674			}
   675	
   676			block_len = ret;
   677			block = kcalloc(block_len, sizeof(*block), GFP_KERNEL);
   678			if (!block)
   679				goto out_free_dir;
   680	
   681			ret = tb_property_format_dir(dir, block, block_len);
   682			if (ret) {
   683				dev_warn(&xd->dev, "property block generation failed\n");
   684				kfree(block);
   685				goto out_free_dir;
   686			}
   687	
   688			tb_property_free_dir(dir);
   689			/* Release the previous block */
   690			kfree(xd->local_property_block);
   691			/* Assign new one */
   692			xd->local_property_block = block;
   693			xd->local_property_block_len = block_len;
   694			xd->local_property_block_gen = xdomain_property_block_gen;
   695		}
   696	
   697	out_unlock:
   698		mutex_unlock(&xd->lock);
   699		mutex_unlock(&xdomain_lock);
   700		return;
   701	
   702	out_free_dir:
 > 703		tb_property_free_dir(dir);
   704		goto out_unlock;
   705	}
   706	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] thunderbolt: Use common error handling code in update_property_block()

Posted by kernel test robot 1 year, 4 months ago

Hi Markus,

kernel test robot noticed the following build errors:

[auto build test ERROR on westeri-thunderbolt/next]
[also build test ERROR on linus/master v6.11 next-20240925]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Markus-Elfring/thunderbolt-Use-common-error-handling-code-in-update_property_block/20240925-161308
base:   https://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git next
patch link:    https://lore.kernel.org/r/26b7f215-4f83-413c-9dab-737d790053c0%40web.de
patch subject: [PATCH] thunderbolt: Use common error handling code in update_property_block()
config: arc-randconfig-001-20240926 (https://download.01.org/0day-ci/archive/20240926/202409260728.uNVNmTvy-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240926/202409260728.uNVNmTvy-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409260728.uNVNmTvy-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/thunderbolt/xdomain.c: In function 'update_property_block':
>> drivers/thunderbolt/xdomain.c:703:30: error: 'dir' undeclared (first use in this function); did you mean 'idr'?
     703 |         tb_property_free_dir(dir);
         |                              ^~~
         |                              idr
   drivers/thunderbolt/xdomain.c:703:30: note: each undeclared identifier is reported only once for each function it appears in


vim +703 drivers/thunderbolt/xdomain.c

   645	
   646	static void update_property_block(struct tb_xdomain *xd)
   647	{
   648		mutex_lock(&xdomain_lock);
   649		mutex_lock(&xd->lock);
   650		/*
   651		 * If the local property block is not up-to-date, rebuild it now
   652		 * based on the global property template.
   653		 */
   654		if (!xd->local_property_block ||
   655		    xd->local_property_block_gen < xdomain_property_block_gen) {
   656			struct tb_property_dir *dir;
   657			int ret, block_len;
   658			u32 *block;
   659	
   660			dir = tb_property_copy_dir(xdomain_property_dir);
   661			if (!dir) {
   662				dev_warn(&xd->dev, "failed to copy properties\n");
   663				goto out_unlock;
   664			}
   665	
   666			/* Fill in non-static properties now */
   667			tb_property_add_text(dir, "deviceid", utsname()->nodename);
   668			tb_property_add_immediate(dir, "maxhopid", xd->local_max_hopid);
   669	
   670			ret = tb_property_format_dir(dir, NULL, 0);
   671			if (ret < 0) {
   672				dev_warn(&xd->dev, "local property block creation failed\n");
   673				goto out_free_dir;
   674			}
   675	
   676			block_len = ret;
   677			block = kcalloc(block_len, sizeof(*block), GFP_KERNEL);
   678			if (!block)
   679				goto out_free_dir;
   680	
   681			ret = tb_property_format_dir(dir, block, block_len);
   682			if (ret) {
   683				dev_warn(&xd->dev, "property block generation failed\n");
   684				kfree(block);
   685				goto out_free_dir;
   686			}
   687	
   688			tb_property_free_dir(dir);
   689			/* Release the previous block */
   690			kfree(xd->local_property_block);
   691			/* Assign new one */
   692			xd->local_property_block = block;
   693			xd->local_property_block_len = block_len;
   694			xd->local_property_block_gen = xdomain_property_block_gen;
   695		}
   696	
   697	out_unlock:
   698		mutex_unlock(&xd->lock);
   699		mutex_unlock(&xdomain_lock);
   700		return;
   701	
   702	out_free_dir:
 > 703		tb_property_free_dir(dir);
   704		goto out_unlock;
   705	}
   706	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] thunderbolt: Use common error handling code in update_property_block()

Posted by Mika Westerberg 1 year, 4 months ago

On Wed, Sep 25, 2024 at 10:10:38AM +0200, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 25 Sep 2024 09:39:16 +0200
> 
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function implementation.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/thunderbolt/xdomain.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
> index 11a50c86a1e4..8e3cf95ca99c 100644
> --- a/drivers/thunderbolt/xdomain.c
> +++ b/drivers/thunderbolt/xdomain.c
> @@ -670,23 +670,19 @@ static void update_property_block(struct tb_xdomain *xd)
>  		ret = tb_property_format_dir(dir, NULL, 0);
>  		if (ret < 0) {
>  			dev_warn(&xd->dev, "local property block creation failed\n");
> -			tb_property_free_dir(dir);
> -			goto out_unlock;
> +			goto out_free_dir;
>  		}
> 
>  		block_len = ret;
>  		block = kcalloc(block_len, sizeof(*block), GFP_KERNEL);
> -		if (!block) {
> -			tb_property_free_dir(dir);
> -			goto out_unlock;
> -		}
> +		if (!block)
> +			goto out_free_dir;
> 
>  		ret = tb_property_format_dir(dir, block, block_len);
>  		if (ret) {
>  			dev_warn(&xd->dev, "property block generation failed\n");
> -			tb_property_free_dir(dir);
>  			kfree(block);
> -			goto out_unlock;
> +			goto out_free_dir;
>  		}
> 
>  		tb_property_free_dir(dir);
> @@ -701,6 +697,11 @@ static void update_property_block(struct tb_xdomain *xd)
>  out_unlock:
>  	mutex_unlock(&xd->lock);
>  	mutex_unlock(&xdomain_lock);
> +	return;
> +
> +out_free_dir:
> +	tb_property_free_dir(dir);
> +	goto out_unlock;

No way, this kind of spaghetti is really hard to follow.

Re: [PATCH] thunderbolt: Use common error handling code in update_property_block()

Posted by Markus Elfring 1 year, 4 months ago

>> Add a jump target so that a bit of exception handling can be better reused
>> at the end of this function implementation.
…
>> +++ b/drivers/thunderbolt/xdomain.c
>> @@ -670,23 +670,19 @@ static void update_property_block(struct tb_xdomain *xd)
>>  		ret = tb_property_format_dir(dir, NULL, 0);
>>  		if (ret < 0) {
>>  			dev_warn(&xd->dev, "local property block creation failed\n");
>> -			tb_property_free_dir(dir);
>> -			goto out_unlock;
>> +			goto out_free_dir;
>>  		}
>>
>>  		block_len = ret;
>>  		block = kcalloc(block_len, sizeof(*block), GFP_KERNEL);
>> -		if (!block) {
>> -			tb_property_free_dir(dir);
>> -			goto out_unlock;
>> -		}
>> +		if (!block)
>> +			goto out_free_dir;
>>
>>  		ret = tb_property_format_dir(dir, block, block_len);
>>  		if (ret) {
>>  			dev_warn(&xd->dev, "property block generation failed\n");
>> -			tb_property_free_dir(dir);
>>  			kfree(block);
>> -			goto out_unlock;
>> +			goto out_free_dir;
>>  		}
>>
>>  		tb_property_free_dir(dir);
>> @@ -701,6 +697,11 @@ static void update_property_block(struct tb_xdomain *xd)
>>  out_unlock:
>>  	mutex_unlock(&xd->lock);
>>  	mutex_unlock(&xdomain_lock);
>> +	return;
>> +
>> +out_free_dir:
>> +	tb_property_free_dir(dir);
>> +	goto out_unlock;
>
> No way, this kind of spaghetti is really hard to follow.

Under which circumstances would you follow advice more from the section
“7) Centralized exiting of functions” (according to a well-known information source)?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.11#n526

How do you think about to increase the application of scope-based resource management?

Regards,
Markus

Re: [PATCH] thunderbolt: Use common error handling code in update_property_block()

Posted by Mika Westerberg 1 year, 4 months ago

On Wed, Sep 25, 2024 at 11:20:45AM +0200, Markus Elfring wrote:
> >>  out_unlock:
> >>  	mutex_unlock(&xd->lock);
> >>  	mutex_unlock(&xdomain_lock);
> >> +	return;
> >> +
> >> +out_free_dir:
> >> +	tb_property_free_dir(dir);
> >> +	goto out_unlock;
> >
> > No way, this kind of spaghetti is really hard to follow.
> 
> Under which circumstances would you follow advice more from the section
> “7) Centralized exiting of functions” (according to a well-known information source)?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.11#n526
> 
> How do you think about to increase the application of scope-based resource management?

It is fine to use goto as it is described in the document you linked but
this what you are doing is certainly not fine, at least in the code I'm
maintaining:

out_unlock:
 	mutex_unlock(&xd->lock);
  	mutex_unlock(&xdomain_lock);
	return;

out_free_dir:
	tb_property_free_dir(dir);
	goto out_unlock;

This "goto out_unlock" adds another goto to upwards which makes it
really hard to follow because the flow is not anymore just downwards.

Re: thunderbolt: Use common error handling code in update_property_block()

Posted by Markus Elfring 1 year, 4 months ago

> It is fine to use goto as it is described in the document you linked but
> this what you are doing is certainly not fine, at least in the code I'm
> maintaining:
>
> out_unlock:
>  	mutex_unlock(&xd->lock);
>   	mutex_unlock(&xdomain_lock);
> 	return;
>
> out_free_dir:
> 	tb_property_free_dir(dir);
> 	goto out_unlock;
>
> This "goto out_unlock" adds another goto to upwards which makes it
> really hard to follow because the flow is not anymore just downwards.

Would you like to benefit any more from the application of
scope-based resource management?

Regards,
Markus

Re: thunderbolt: Use common error handling code in update_property_block()

Posted by Greg KH 1 year, 4 months ago

On Wed, Sep 25, 2024 at 11:40:09AM +0200, Markus Elfring wrote:
> > It is fine to use goto as it is described in the document you linked but
> > this what you are doing is certainly not fine, at least in the code I'm
> > maintaining:
> >
> > out_unlock:
> >  	mutex_unlock(&xd->lock);
> >   	mutex_unlock(&xdomain_lock);
> > 	return;
> >
> > out_free_dir:
> > 	tb_property_free_dir(dir);
> > 	goto out_unlock;
> >
> > This "goto out_unlock" adds another goto to upwards which makes it
> > really hard to follow because the flow is not anymore just downwards.
> 
> Would you like to benefit any more from the application of
> scope-based resource management?

Hi,

This is the semi-friendly patch-bot of Greg Kroah-Hartman.

Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list.  I strongly suggest that you not do this anymore.  Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.

Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all.  The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback.  Please feel free to also ignore emails
from them.

thanks,

greg k-h's patch email bot