* Re: [PATCH] thunderbolt: Use common error handling code in update_property_block()
2024-09-25 8:10 [PATCH] thunderbolt: Use common error handling code in update_property_block() Markus Elfring
@ 2024-09-25 8:45 ` Mika Westerberg
2024-09-25 9:20 ` Markus Elfring
2024-09-25 23:58 ` [PATCH] " kernel test robot
2024-09-26 1:52 ` kernel test robot
2 siblings, 1 reply; 8+ messages in thread
From: Mika Westerberg @ 2024-09-25 8:45 UTC (permalink / raw)
To: Markus Elfring
Cc: linux-usb, Andreas Noever, Michael Jamet, Yehezkel Bernat, LKML,
kernel-janitors
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] thunderbolt: Use common error handling code in update_property_block()
2024-09-25 8:45 ` Mika Westerberg
@ 2024-09-25 9:20 ` Markus Elfring
2024-09-25 9:35 ` Mika Westerberg
0 siblings, 1 reply; 8+ messages in thread
From: Markus Elfring @ 2024-09-25 9:20 UTC (permalink / raw)
To: Mika Westerberg, linux-usb, Andreas Noever, Michael Jamet,
Yehezkel Bernat
Cc: LKML, kernel-janitors
>> 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] thunderbolt: Use common error handling code in update_property_block()
2024-09-25 9:20 ` Markus Elfring
@ 2024-09-25 9:35 ` Mika Westerberg
2024-09-25 9:40 ` Markus Elfring
0 siblings, 1 reply; 8+ messages in thread
From: Mika Westerberg @ 2024-09-25 9:35 UTC (permalink / raw)
To: Markus Elfring
Cc: linux-usb, Andreas Noever, Michael Jamet, Yehezkel Bernat, LKML,
kernel-janitors
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: thunderbolt: Use common error handling code in update_property_block()
2024-09-25 9:35 ` Mika Westerberg
@ 2024-09-25 9:40 ` Markus Elfring
2024-09-25 9:42 ` Greg KH
0 siblings, 1 reply; 8+ messages in thread
From: Markus Elfring @ 2024-09-25 9:40 UTC (permalink / raw)
To: Mika Westerberg, linux-usb, Andreas Noever, Michael Jamet,
Yehezkel Bernat
Cc: LKML, kernel-janitors
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: thunderbolt: Use common error handling code in update_property_block()
2024-09-25 9:40 ` Markus Elfring
@ 2024-09-25 9:42 ` Greg KH
0 siblings, 0 replies; 8+ messages in thread
From: Greg KH @ 2024-09-25 9:42 UTC (permalink / raw)
To: Markus Elfring
Cc: Mika Westerberg, linux-usb, Andreas Noever, Michael Jamet,
Yehezkel Bernat, LKML, kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] thunderbolt: Use common error handling code in update_property_block()
2024-09-25 8:10 [PATCH] thunderbolt: Use common error handling code in update_property_block() Markus Elfring
2024-09-25 8:45 ` Mika Westerberg
@ 2024-09-25 23:58 ` kernel test robot
2024-09-26 1:52 ` kernel test robot
2 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2024-09-25 23:58 UTC (permalink / raw)
To: Markus Elfring, linux-usb, Andreas Noever, Michael Jamet,
Mika Westerberg, Yehezkel Bernat
Cc: oe-kbuild-all, LKML, kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] thunderbolt: Use common error handling code in update_property_block()
2024-09-25 8:10 [PATCH] thunderbolt: Use common error handling code in update_property_block() Markus Elfring
2024-09-25 8:45 ` Mika Westerberg
2024-09-25 23:58 ` [PATCH] " kernel test robot
@ 2024-09-26 1:52 ` kernel test robot
2 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2024-09-26 1:52 UTC (permalink / raw)
To: Markus Elfring, linux-usb, Andreas Noever, Michael Jamet,
Mika Westerberg, Yehezkel Bernat
Cc: llvm, oe-kbuild-all, LKML, kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread