From: Markus Elfring <[email protected]>
Date: Mon, 12 Mar 2018 22:15:59 +0100
Adjust jump targets so that a bit of exception handling can be better
reused at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/hwmon/sch5627.c | 60 ++++++++++++++++++++++++-------------------------
1 file changed, 29 insertions(+), 31 deletions(-)
diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
index 91544f2312e6..e7a2a974ab74 100644
--- a/drivers/hwmon/sch5627.c
+++ b/drivers/hwmon/sch5627.c
@@ -480,72 +480,64 @@ static int sch5627_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, data);
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_HWMON_ID);
- if (val < 0) {
- err = val;
- goto error;
- }
+ if (val < 0)
+ goto set_error_code;
+
if (val != SCH5627_HWMON_ID) {
pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "hwmon",
val, SCH5627_HWMON_ID);
- err = -ENODEV;
- goto error;
+ goto e_nodev;
}
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_COMPANY_ID);
- if (val < 0) {
- err = val;
- goto error;
- }
+ if (val < 0)
+ goto set_error_code;
+
if (val != SCH5627_COMPANY_ID) {
pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "company",
val, SCH5627_COMPANY_ID);
- err = -ENODEV;
- goto error;
+ goto e_nodev;
}
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_PRIMARY_ID);
- if (val < 0) {
- err = val;
- goto error;
- }
+ if (val < 0)
+ goto set_error_code;
+
if (val != SCH5627_PRIMARY_ID) {
pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "primary",
val, SCH5627_PRIMARY_ID);
- err = -ENODEV;
- goto error;
+ goto e_nodev;
}
build_code = sch56xx_read_virtual_reg(data->addr,
SCH5627_REG_BUILD_CODE);
if (build_code < 0) {
err = build_code;
- goto error;
+ goto remove_device;
}
build_id = sch56xx_read_virtual_reg16(data->addr,
SCH5627_REG_BUILD_ID);
if (build_id < 0) {
err = build_id;
- goto error;
+ goto remove_device;
}
hwmon_rev = sch56xx_read_virtual_reg(data->addr,
SCH5627_REG_HWMON_REV);
if (hwmon_rev < 0) {
err = hwmon_rev;
- goto error;
+ goto remove_device;
}
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_CTRL);
- if (val < 0) {
- err = val;
- goto error;
- }
+ if (val < 0)
+ goto set_error_code;
+
data->control = val;
if (!(data->control & 0x01)) {
pr_err("hardware monitoring not enabled\n");
- err = -ENODEV;
- goto error;
+ goto e_nodev;
}
/* Trigger a Vbat voltage measurement, so that we get a valid reading
the first time we read Vbat */
@@ -559,7 +551,7 @@ static int sch5627_probe(struct platform_device *pdev)
*/
err = sch5627_read_limits(data);
if (err)
- goto error;
+ goto remove_device;
pr_info("found %s chip at %#hx\n", DEVNAME, data->addr);
pr_info("firmware build: code 0x%02X, id 0x%04X, hwmon: rev 0x%02X\n",
@@ -568,13 +560,13 @@ static int sch5627_probe(struct platform_device *pdev)
/* Register sysfs interface files */
err = sysfs_create_group(&pdev->dev.kobj, &sch5627_group);
if (err)
- goto error;
+ goto remove_device;
data->hwmon_dev = hwmon_device_register(&pdev->dev);
if (IS_ERR(data->hwmon_dev)) {
err = PTR_ERR(data->hwmon_dev);
data->hwmon_dev = NULL;
- goto error;
+ goto remove_device;
}
/* Note failing to register the watchdog is not a fatal error */
@@ -584,7 +576,13 @@ static int sch5627_probe(struct platform_device *pdev)
return 0;
-error:
+set_error_code:
+ err = val;
+ goto remove_device;
+
+e_nodev:
+ err = -ENODEV;
+remove_device:
sch5627_remove(pdev);
return err;
}
--
2.16.2
Hi,
On 12-03-18 22:23, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Mon, 12 Mar 2018 22:15:59 +0100
>
> Adjust jump targets so that a bit of exception handling can be better
> reused at the end of this function.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <[email protected]>
So now we have a goto set_error_code with the code at the set_error_code
doing something and then doing another goto. No, just no. goto-s going to
a label calling another goto is completely unreadable.
I really do not see any reason for the proposed changes, they may remove
a small amount of code duplication, but at a hugh cost wrt readability.
TL;DR: NACK
Regards,
Hans
> ---
> drivers/hwmon/sch5627.c | 60 ++++++++++++++++++++++++-------------------------
> 1 file changed, 29 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
> index 91544f2312e6..e7a2a974ab74 100644
> --- a/drivers/hwmon/sch5627.c
> +++ b/drivers/hwmon/sch5627.c
> @@ -480,72 +480,64 @@ static int sch5627_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, data);
>
> val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_HWMON_ID);
> - if (val < 0) {
> - err = val;
> - goto error;
> - }
> + if (val < 0)
> + goto set_error_code;
> +
> if (val != SCH5627_HWMON_ID) {
> pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "hwmon",
> val, SCH5627_HWMON_ID);
> - err = -ENODEV;
> - goto error;
> + goto e_nodev;
> }
>
> val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_COMPANY_ID);
> - if (val < 0) {
> - err = val;
> - goto error;
> - }
> + if (val < 0)
> + goto set_error_code;
> +
> if (val != SCH5627_COMPANY_ID) {
> pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "company",
> val, SCH5627_COMPANY_ID);
> - err = -ENODEV;
> - goto error;
> + goto e_nodev;
> }
>
> val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_PRIMARY_ID);
> - if (val < 0) {
> - err = val;
> - goto error;
> - }
> + if (val < 0)
> + goto set_error_code;
> +
> if (val != SCH5627_PRIMARY_ID) {
> pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "primary",
> val, SCH5627_PRIMARY_ID);
> - err = -ENODEV;
> - goto error;
> + goto e_nodev;
> }
>
> build_code = sch56xx_read_virtual_reg(data->addr,
> SCH5627_REG_BUILD_CODE);
> if (build_code < 0) {
> err = build_code;
> - goto error;
> + goto remove_device;
> }
>
> build_id = sch56xx_read_virtual_reg16(data->addr,
> SCH5627_REG_BUILD_ID);
> if (build_id < 0) {
> err = build_id;
> - goto error;
> + goto remove_device;
> }
>
> hwmon_rev = sch56xx_read_virtual_reg(data->addr,
> SCH5627_REG_HWMON_REV);
> if (hwmon_rev < 0) {
> err = hwmon_rev;
> - goto error;
> + goto remove_device;
> }
>
> val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_CTRL);
> - if (val < 0) {
> - err = val;
> - goto error;
> - }
> + if (val < 0)
> + goto set_error_code;
> +
> data->control = val;
> if (!(data->control & 0x01)) {
> pr_err("hardware monitoring not enabled\n");
> - err = -ENODEV;
> - goto error;
> + goto e_nodev;
> }
> /* Trigger a Vbat voltage measurement, so that we get a valid reading
> the first time we read Vbat */
> @@ -559,7 +551,7 @@ static int sch5627_probe(struct platform_device *pdev)
> */
> err = sch5627_read_limits(data);
> if (err)
> - goto error;
> + goto remove_device;
>
> pr_info("found %s chip at %#hx\n", DEVNAME, data->addr);
> pr_info("firmware build: code 0x%02X, id 0x%04X, hwmon: rev 0x%02X\n",
> @@ -568,13 +560,13 @@ static int sch5627_probe(struct platform_device *pdev)
> /* Register sysfs interface files */
> err = sysfs_create_group(&pdev->dev.kobj, &sch5627_group);
> if (err)
> - goto error;
> + goto remove_device;
>
> data->hwmon_dev = hwmon_device_register(&pdev->dev);
> if (IS_ERR(data->hwmon_dev)) {
> err = PTR_ERR(data->hwmon_dev);
> data->hwmon_dev = NULL;
> - goto error;
> + goto remove_device;
> }
>
> /* Note failing to register the watchdog is not a fatal error */
> @@ -584,7 +576,13 @@ static int sch5627_probe(struct platform_device *pdev)
>
> return 0;
>
> -error:
> +set_error_code:
> + err = val;
> + goto remove_device;
> +
> +e_nodev:
> + err = -ENODEV;
> +remove_device:
> sch5627_remove(pdev);
> return err;
> }
>
>> Adjust jump targets so that a bit of exception handling can be better
>> reused at the end of this function.
…
> goto-s going to a label calling another goto is completely unreadable.
I got an other software development view.
> I really do not see any reason for the proposed changes,
I suggest to look once more.
> they may remove a small amount of code duplication,
This was my software design goal in this case.
> but at a hugh cost wrt readability.
I proposed a different trade-off here.
Regards,
Markus
Hi,
On 13-03-18 09:25, SF Markus Elfring wrote:
>>> Adjust jump targets so that a bit of exception handling can be better
>>> reused at the end of this function.
> …
>> goto-s going to a label calling another goto is completely unreadable.
>
> I got an other software development view.
>
>
>> I really do not see any reason for the proposed changes,
>
> I suggest to look once more.
>
>
>> they may remove a small amount of code duplication,
>
> This was my software design goal in this case.
The diffstat of your patch is:
1 file changed, 29 insertions(+), 31 deletions(-)
So you are asking people to review 60 changed lines to save 2,
that alone should be the point where you stop yourself from
*even* sending this patch. Review time is not an endless free
resource and this patch is wasting it for very little gain.
I see in the kernel git log that you've e.g. also send a lot
of patches removing pr_err / dev_err calls from memory
allocation failures. Those are good patches to have, they are
easy to review and significantly shrink the compiled kernel
size because of all the text strings you are removing.
This patch however will likely result in a binary which is
hardly smaller at all (if at all, the compiler does common
code elimination itself) while it is a complex patch to
review so comes with a large review cost.
Next time before you send a patch please carefully think if the
saving is worth the combination of reviewers time + the risk of
regressions (and keep in mind that both the reviewers time and
the risk of regressions cost increase for more complex changes).
>> but at a hugh cost wrt readability.
>
> I proposed a different trade-off here.
<sigh> not this again. Cleanup patches are appreciated, but there
always is a cost to making changes to perfectly working good code.
You've had this same discussion with Jonathan Cameron, the IIO
subsys maintainer at the end of 2017, it would be nice if you
were to actually listen to what people are saying about this.
As for this specific discussion, there are certain "design-patterns"
in the kernel, goto style error handling is one of them, the pattern
there ALWAYS is:
error_cleanup4:
cleanup4();
/* fall through to next cleanup */
error_cleanup3:
cleanup3();
/* fall through to next cleanup */
error_cleanup2:
cleanup2();
/* fall through to next cleanup */
error_cleanup1:
cleanup1();
/* fall through to next cleanup */
return error;
Notice the fall-thoughs those are ALWAYS there, never, ever is
there a goto after a cleanup label. The idea is that resources
are allocated in the order of resource1 (cleaned by cleanup1())
then resource2, etc. and the goto + fallthrough will cleanup
all resources which have been allocated at the time of the goto
in *reverse* order of allocation. The whole point of this design-
pattern to be able to a) do partial cleanup if we fail halfway
through; b) do it in reverse order. Your patches black goto magic
completely messes this up and clearly falls under the CS101
rule: never use goto.
Your proposed changes break how every experienced kernel developer
is expecting the code to be / work, causing your proposed changes
to have a HUGE cost wrt maintainability and readability, which
means the trade-off you're suggesting is anything but worth-while.
TL;DR: still NACK.
Regards,
Hans
> 1 file changed, 29 insertions(+), 31 deletions(-)
>
> So you are asking people to review 60 changed lines to save 2,
A bit of object code reduction might become useful also in this case.
> that alone should be the point where you stop yourself from
> *even* sending this patch.
I proposed just another collateral evolution.
> Next time before you send a patch please carefully think if the
> saving is worth the combination of reviewers time + the risk of
> regressions (and keep in mind that both the reviewers time and
> the risk of regressions cost increase for more complex changes).
Source code transformations were integrated in other software areas
according to such a change pattern.
> As for this specific discussion, there are certain "design-patterns"
> in the kernel, goto style error handling is one of them, the pattern
> there ALWAYS is:
…
> Notice the fall-thoughs those are ALWAYS there, never, ever is
> there a goto after a cleanup label.
It seems that I present an unusual update suggestion as a software
design variant.
> Your patches black goto magic completely messes this up
You can view the proposal in such a way.
> and clearly falls under the CS101 rule: never use goto.
There might a target conflict with information from the section
“7) Centralized exiting of functions” in the document “coding-style.rst”.
Regards,
Markus
On Tue, Mar 13, 2018 at 09:19:22AM +0100, Hans de Goede wrote:
> Hi,
>
> On 12-03-18 22:23, SF Markus Elfring wrote:
> >From: Markus Elfring <[email protected]>
> >Date: Mon, 12 Mar 2018 22:15:59 +0100
> >
> >Adjust jump targets so that a bit of exception handling can be better
> >reused at the end of this function.
> >
> >This issue was detected by using the Coccinelle software.
> >
> >Signed-off-by: Markus Elfring <[email protected]>
>
> So now we have a goto set_error_code with the code at the set_error_code
> doing something and then doing another goto. No, just no. goto-s going to
> a label calling another goto is completely unreadable.
>
> I really do not see any reason for the proposed changes, they may remove
> a small amount of code duplication, but at a hugh cost wrt readability.
>
> TL;DR: NACK
>
Fully agree. NACK confirmed.
Guenter
> Regards,
>
> Hans
>
>
>
> >---
> > drivers/hwmon/sch5627.c | 60 ++++++++++++++++++++++++-------------------------
> > 1 file changed, 29 insertions(+), 31 deletions(-)
> >
> >diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
> >index 91544f2312e6..e7a2a974ab74 100644
> >--- a/drivers/hwmon/sch5627.c
> >+++ b/drivers/hwmon/sch5627.c
> >@@ -480,72 +480,64 @@ static int sch5627_probe(struct platform_device *pdev)
> > platform_set_drvdata(pdev, data);
> > val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_HWMON_ID);
> >- if (val < 0) {
> >- err = val;
> >- goto error;
> >- }
> >+ if (val < 0)
> >+ goto set_error_code;
> >+
> > if (val != SCH5627_HWMON_ID) {
> > pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "hwmon",
> > val, SCH5627_HWMON_ID);
> >- err = -ENODEV;
> >- goto error;
> >+ goto e_nodev;
> > }
> > val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_COMPANY_ID);
> >- if (val < 0) {
> >- err = val;
> >- goto error;
> >- }
> >+ if (val < 0)
> >+ goto set_error_code;
> >+
> > if (val != SCH5627_COMPANY_ID) {
> > pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "company",
> > val, SCH5627_COMPANY_ID);
> >- err = -ENODEV;
> >- goto error;
> >+ goto e_nodev;
> > }
> > val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_PRIMARY_ID);
> >- if (val < 0) {
> >- err = val;
> >- goto error;
> >- }
> >+ if (val < 0)
> >+ goto set_error_code;
> >+
> > if (val != SCH5627_PRIMARY_ID) {
> > pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "primary",
> > val, SCH5627_PRIMARY_ID);
> >- err = -ENODEV;
> >- goto error;
> >+ goto e_nodev;
> > }
> > build_code = sch56xx_read_virtual_reg(data->addr,
> > SCH5627_REG_BUILD_CODE);
> > if (build_code < 0) {
> > err = build_code;
> >- goto error;
> >+ goto remove_device;
> > }
> > build_id = sch56xx_read_virtual_reg16(data->addr,
> > SCH5627_REG_BUILD_ID);
> > if (build_id < 0) {
> > err = build_id;
> >- goto error;
> >+ goto remove_device;
> > }
> > hwmon_rev = sch56xx_read_virtual_reg(data->addr,
> > SCH5627_REG_HWMON_REV);
> > if (hwmon_rev < 0) {
> > err = hwmon_rev;
> >- goto error;
> >+ goto remove_device;
> > }
> > val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_CTRL);
> >- if (val < 0) {
> >- err = val;
> >- goto error;
> >- }
> >+ if (val < 0)
> >+ goto set_error_code;
> >+
> > data->control = val;
> > if (!(data->control & 0x01)) {
> > pr_err("hardware monitoring not enabled\n");
> >- err = -ENODEV;
> >- goto error;
> >+ goto e_nodev;
> > }
> > /* Trigger a Vbat voltage measurement, so that we get a valid reading
> > the first time we read Vbat */
> >@@ -559,7 +551,7 @@ static int sch5627_probe(struct platform_device *pdev)
> > */
> > err = sch5627_read_limits(data);
> > if (err)
> >- goto error;
> >+ goto remove_device;
> > pr_info("found %s chip at %#hx\n", DEVNAME, data->addr);
> > pr_info("firmware build: code 0x%02X, id 0x%04X, hwmon: rev 0x%02X\n",
> >@@ -568,13 +560,13 @@ static int sch5627_probe(struct platform_device *pdev)
> > /* Register sysfs interface files */
> > err = sysfs_create_group(&pdev->dev.kobj, &sch5627_group);
> > if (err)
> >- goto error;
> >+ goto remove_device;
> > data->hwmon_dev = hwmon_device_register(&pdev->dev);
> > if (IS_ERR(data->hwmon_dev)) {
> > err = PTR_ERR(data->hwmon_dev);
> > data->hwmon_dev = NULL;
> >- goto error;
> >+ goto remove_device;
> > }
> > /* Note failing to register the watchdog is not a fatal error */
> >@@ -584,7 +576,13 @@ static int sch5627_probe(struct platform_device *pdev)
> > return 0;
> >-error:
> >+set_error_code:
> >+ err = val;
> >+ goto remove_device;
> >+
> >+e_nodev:
> >+ err = -ENODEV;
> >+remove_device:
> > sch5627_remove(pdev);
> > return err;
> > }
> >