| Message ID | 0f26a13b-7bbf-525b-a864-8aa8e13c3aef@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 25 Oct 2017 22:16:31 +0200 SF Markus Elfring <elfring@users.sourceforge.net> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Wed, 25 Oct 2017 21:36:03 +0200 > > * Add a jump target so that a specific error message is stored only once > at the end of this function implementation. > > * Replace two calls of the function "dev_err" by goto statements. > > * Adjust condition checks. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> Again readability is hurt by the more complex code flow. As such this is never going to be an acceptable change in the kernel. Sorry Markus, but all patches have to pass a test on whether any advantages to outweight complexity This is definitely not true here. Again, a general kernel development rule is to float only one patch of a given type until you have had feedback on it. These backwards goto cases are sufficiently different from your earlier patch that got reviews that, at most, you should have sent one to the list and then given time for it to be properly reviewed (up to a week on IIO list typically). That would have saved your time and mine. As I said in one of the other patches, I always reply to all patches I am rejecting so that anyone coming across them later on their own from an archive or similar can immediately see the reasons why they are a bad idea without having to know the mailing list context. Jonathan > --- > drivers/iio/accel/stk8ba50.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/accel/stk8ba50.c b/drivers/iio/accel/stk8ba50.c > index 576b6b140f08..afe90567ad37 100644 > --- a/drivers/iio/accel/stk8ba50.c > +++ b/drivers/iio/accel/stk8ba50.c > @@ -426,16 +426,13 @@ static int stk8ba50_probe(struct i2c_client *client, > /* Set up interrupts */ > ret = i2c_smbus_write_byte_data(client, > STK8BA50_REG_INTEN2, STK8BA50_DREADY_INT_MASK); > - if (ret < 0) { > - dev_err(&client->dev, "failed to set up interrupts\n"); > - goto err_power_off; > - } > + if (ret) > + goto report_failure; > + > ret = i2c_smbus_write_byte_data(client, > STK8BA50_REG_INTMAP2, STK8BA50_DREADY_INT_MAP); > - if (ret < 0) { > - dev_err(&client->dev, "failed to set up interrupts\n"); > - goto err_power_off; > - } > + if (ret) > + goto report_failure; > > if (client->irq > 0) { > ret = devm_request_threaded_irq(&client->dev, client->irq, > @@ -495,6 +492,10 @@ static int stk8ba50_probe(struct i2c_client *client, > err_power_off: > stk8ba50_set_power(data, STK8BA50_MODE_SUSPEND); > return ret; > + > +report_failure: > + dev_err(&client->dev, "failed to set up interrupts\n"); > + goto err_power_off; Simple code flow is replaced by more complex flow for little gain. Not a good idea. > } > > static int stk8ba50_remove(struct i2c_client *client) -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Again, a general kernel development rule is to float only one > patch of a given type until you have had feedback on it. Some update suggestions according also to this general source code transformation pattern were already integrated in other software areas. > These backwards goto cases are sufficiently different from your earlier > patch that got reviews that, They are required if you would ever like to support software refactorings in directions which I propose occasionally. > at most, you should have sent one to the list and then given time > for it to be properly reviewed (up to a week on IIO list typically). Did I present my ideas a bit too quick in this case? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Again, a general kernel development rule is to float only one > patch of a given type until you have had feedback on it. A few weeks later … > As I said in one of the other patches, I always reply to all > patches I am rejecting so that anyone coming across them later > on their own from an archive or similar can immediately see the > reasons why they are a bad idea without having to know the mailing > list context. Can you get into the mood to clarify any remaining change possibilities a bit more? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 10 Dec 2017 13:33:01 +0100 SF Markus Elfring <elfring@users.sourceforge.net> wrote: > > Again, a general kernel development rule is to float only one > > patch of a given type until you have had feedback on it. > > A few weeks later … > > > > As I said in one of the other patches, I always reply to all > > patches I am rejecting so that anyone coming across them later > > on their own from an archive or similar can immediately see the > > reasons why they are a bad idea without having to know the mailing > > list context. > > Can you get into the mood to clarify any remaining change possibilities > a bit more? Hi Markus, I've accepted the ones that I think made an improvement outweighing the inherent small costs of making any change. So in short, any changes around common error handling need to improve the maintainability of the code. So if we are unifying error paths they need to be simple and obviously the same. We also need to avoid code constructs that are unusual in error handling such as backwards gotos. Note however that most of the changes made so far are only minor improvements. I am not saying I don't appreciate them, but rather than that they are of of low importance. Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Hi Markus, I've accepted the ones that I think made an improvement > outweighing the inherent small costs of making any change. Does such a kind of feedback mean that you reconsidered any places where you expressed a rejection initially? > We also need to avoid code constructs that are unusual in error handling > such as backwards gotos. Why would you like to exclude this approach if anything useful could be achieved in the shown software design direction? > Note however that most of the changes made so far are only minor improvements. I agree that corresponding effects are small just because the discussed source code adjustments affected specific function implementations. > I am not saying I don't appreciate them, Thanks. > but rather than that they are of of low importance. A lot of details are competing also for our software development attention. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 10 Dec 2017 17:43:34 +0100 SF Markus Elfring <elfring@users.sourceforge.net> wrote: > > Hi Markus, I've accepted the ones that I think made an improvement > > outweighing the inherent small costs of making any change. > > Does such a kind of feedback mean that you reconsidered any places > where you expressed a rejection initially? No. Once I have expressed strong reservations about a patch it would require some change in the facts to make me reevaluate. > > > > We also need to avoid code constructs that are unusual in error handling > > such as backwards gotos. > > Why would you like to exclude this approach if anything useful could be achieved > in the shown software design direction? Yes - exclude this. It trades of ease of review against briefness of code. Ease of review and hence verification of correctness is more important in these cases. > > > > Note however that most of the changes made so far are only minor improvements. > > I agree that corresponding effects are small just because the discussed > source code adjustments affected specific function implementations. > > > > I am not saying I don't appreciate them, > > Thanks. > > > > but rather than that they are of of low importance. > > A lot of details are competing also for our software development attention. > Exactly. Jonathan > Regards, > Markus -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iio/accel/stk8ba50.c b/drivers/iio/accel/stk8ba50.c index 576b6b140f08..afe90567ad37 100644 --- a/drivers/iio/accel/stk8ba50.c +++ b/drivers/iio/accel/stk8ba50.c @@ -426,16 +426,13 @@ static int stk8ba50_probe(struct i2c_client *client, /* Set up interrupts */ ret = i2c_smbus_write_byte_data(client, STK8BA50_REG_INTEN2, STK8BA50_DREADY_INT_MASK); - if (ret < 0) { - dev_err(&client->dev, "failed to set up interrupts\n"); - goto err_power_off; - } + if (ret) + goto report_failure; + ret = i2c_smbus_write_byte_data(client, STK8BA50_REG_INTMAP2, STK8BA50_DREADY_INT_MAP); - if (ret < 0) { - dev_err(&client->dev, "failed to set up interrupts\n"); - goto err_power_off; - } + if (ret) + goto report_failure; if (client->irq > 0) { ret = devm_request_threaded_irq(&client->dev, client->irq, @@ -495,6 +492,10 @@ static int stk8ba50_probe(struct i2c_client *client, err_power_off: stk8ba50_set_power(data, STK8BA50_MODE_SUSPEND); return ret; + +report_failure: + dev_err(&client->dev, "failed to set up interrupts\n"); + goto err_power_off; } static int stk8ba50_remove(struct i2c_client *client)