[PATCH] iio: imu: inv_mpu6050: Remove duplicate code between labels

[PATCH] iio: imu: inv_mpu6050: Remove duplicate code between labels

[Gyeyoung Baek]

'flush_fifo' label performs same task as 'endsession' label
immediately after calling 'env_reset_fifo' function.
so i remove that duplication.

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index 45c37525c2f1..40107b4457d4 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -192,15 +192,12 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 		iio_push_to_buffers_with_timestamp(indio_dev, st->data, timestamp);
 	}
 
-end_session:
-	mutex_unlock(&st->lock);
-	iio_trigger_notify_done(indio_dev->trig);
-
-	return IRQ_HANDLED;
 
 flush_fifo:
 	/* Flush HW and SW FIFOs. */
 	inv_reset_fifo(indio_dev);
+
+end_session:
 	mutex_unlock(&st->lock);
 	iio_trigger_notify_done(indio_dev->trig);
 
-- 
2.34.1

Re: [PATCH] iio: imu: inv_mpu6050: Remove duplicate code between labels

[Markus Elfring]

> 'flush_fifo' label performs same task as 'endsession' label

                                            end_session?

The number of actions differ between involved jump targets.


> immediately after calling 'env_reset_fifo' function.
> so i remove that duplication.

* You would like to specify a corresponding goto chain at the moment,
  don't you?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.11-rc5#n526

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


Regards,
Markus

Re: [PATCH] iio: imu: inv_mpu6050: Remove duplicate code between labels

[gyeyoung]

Hello, I apologize for the insufficient explanation.

---
Before the change:

"end_session:
    mutex_unlock(&st->lock);
    iio_trigger_notify_done(indio_dev->trig);
    return IRQ_HANDLED;

flush_fifo:
    /* Flush HW and SW FIFOs. */
    inv_reset_fifo(indio_dev);
    mutex_unlock(&st->lock);
    iio_trigger_notify_done(indio_dev->trig);
    return IRQ_HANDLED;
"
---
After the change:

"flush_fifo:
/* Flush HW and SW FIFOs. */
inv_reset_fifo(indio_dev);

end_session:
mutex_unlock(&st->lock);
iio_trigger_notify_done(indio_dev->trig);

return IRQ_HANDLED;"
---

Here, 'flush_fifo' and 'end_session' are not the same. However, the
work of 'flush_fifo' is a superset of 'end_session'.


On Sun, Sep 1, 2024 at 9:08 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > 'flush_fifo' label performs same task as 'endsession' label
>
>                                             end_session?
>
> The number of actions differ between involved jump targets.
>
>
> > immediately after calling 'env_reset_fifo' function.
> > so i remove that duplication.
>
> * You would like to specify a corresponding goto chain at the moment,
>   don't you?
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.11-rc5#n526
>
> * How do you think about to increase the application of scope-based resource management?

firstly I understood that you might be referring to RAII. but I think
this issue is not related to RAII.
thanks for response.

>
>
> Regards,
> Markus

Re: iio: imu: inv_mpu6050: Remove duplicate code between labels

[Markus Elfring]

> Hello, I apologize for the insufficient explanation.

How will the commit message be improved further?

Regards,
Markus

Re: iio: imu: inv_mpu6050: Remove duplicate code between labels

[gyeyoung]

On Mon, Sep 2, 2024 at 3:00 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >>> Hello, I apologize for the insufficient explanation.
> >>
> >> How will the commit message be improved further?
> …
> > Since the code is short,
>
> This implementation detail can be nice.
>
>
> > I think it's fine for now.
>
> Please reconsider such a view once more.
> Are imperative wordings also more desirable for a better change description?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc6#n45
>
> Regards,
> Markus

Hello, as a newbie, thanks for providing several guidelines, But I
just suggested a minor thing as, and since the maintainer made the
decision and approved, I did not feel the need to go any further.

Re: iio: imu: inv_mpu6050: Remove duplicate code between labels

[gyeyoung]

On Mon, Sep 2, 2024 at 3:16 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > Hello, I apologize for the insufficient explanation.
>
> How will the commit message be improved further?
>
> Regards,
> Markus

Since the code is short, I think it's fine for now. thanks.

-Gyeyoung

Re: iio: imu: inv_mpu6050: Remove duplicate code between labels

[Markus Elfring]

>>> Hello, I apologize for the insufficient explanation.
>>
>> How will the commit message be improved further?
…
> Since the code is short,

This implementation detail can be nice.


> I think it's fine for now.

Please reconsider such a view once more.
Are imperative wordings also more desirable for a better change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc6#n45

Regards,
Markus

Re: iio: imu: inv_mpu6050: Remove duplicate code between labels

[Jean-Baptiste Maneyrol]

Hello,

beware this patch is buggy. It will break the IRQ handler function of inv_mpu6050 driver.

The normal code path is going through end_session label without goto, and expect the function return before executing inv_reset_fifo. Without it, the reset FIFO function will be called for every interrupt and is breaking normal functioning of the driver.

Best regards,
JB

________________________________________
From: Markus Elfring <Markus.Elfring@web.de>
Sent: Monday, September 2, 2024 08:00
To: Gyeyoung Baek <gye976@gmail.com>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>
Cc: LKML <linux-kernel@vger.kernel.org>
Subject: Re: iio: imu: inv_mpu6050: Remove duplicate code between labels
 
This Message Is From an Untrusted Sender
You have not previously corresponded with this sender.
 
>>> Hello, I apologize for the insufficient explanation.
>>
>> How will the commit message be improved further?
…
> Since the code is short,

This implementation detail can be nice.


> I think it's fine for now.

Please reconsider such a view once more.
Are imperative wordings also more desirable for a better change description?
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc6*n45__;Iw!!FtrhtPsWDhZ6tw!Hb9yipjKJXmB-DO9gWKADZfQZHI84WEFUc6Ns1iGhpAfvAAyjrnLQRJZLU2Ha0nI8Fs-HBqHFlFbq0Kl-O1CJwYLe776xbRywQ$[git[.]kernel[.]org]

Regards,
Markus

Re: [PATCH] iio: imu: inv_mpu6050: Remove duplicate code between labels

[Jean-Baptiste Maneyrol]

Hello,

it would be true if there was a return statement before the 2 labels, and that the 2 lables were only error handling case.

But this is not the case, since end_session label is the end of the normal end of the function, not error handling. This is very old code and not very clear, I'm sorry about that.

But the modifications are breaking the code and interrupt handling will not work correctly anymore, since inv_reset_fifo() will be called now every time the function is called.

Best regards,
JB

________________________________________
From: gyeyoung <gye976@gmail.com>
Sent: Sunday, September 1, 2024 15:40
To: Markus Elfring <Markus.Elfring@web.de>
Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] iio: imu: inv_mpu6050: Remove duplicate code between labels
 
This Message Is From an Untrusted Sender
You have not previously corresponded with this sender.
 
Hello, I apologize for the insufficient explanation.

---
Before the change:

"end_session:
    mutex_unlock(&st->lock);
    iio_trigger_notify_done(indio_dev->trig);
    return IRQ_HANDLED;

flush_fifo:
    /* Flush HW and SW FIFOs. */
    inv_reset_fifo(indio_dev);
    mutex_unlock(&st->lock);
    iio_trigger_notify_done(indio_dev->trig);
    return IRQ_HANDLED;
"
---
After the change:

"flush_fifo:
/* Flush HW and SW FIFOs. */
inv_reset_fifo(indio_dev);

end_session:
mutex_unlock(&st->lock);
iio_trigger_notify_done(indio_dev->trig);

return IRQ_HANDLED;"
---

Here, 'flush_fifo' and 'end_session' are not the same. However, the
work of 'flush_fifo' is a superset of 'end_session'.


On Sun, Sep 1, 2024 at 9:08 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > 'flush_fifo' label performs same task as 'endsession' label
>
>                                             end_session?
>
> The number of actions differ between involved jump targets.
>
>
> > immediately after calling 'env_reset_fifo' function.
> > so i remove that duplication.
>
> * You would like to specify a corresponding goto chain at the moment,
>   don't you?
>   https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.11-rc5*n526__;Iw!!FtrhtPsWDhZ6tw!EFMuPFwY7oiEXqGmGjNGpAH4EZl_mGVb10vu2XoO2X4fNR50y46xuljrl3hOjULZkOE_d6FlNqFz3XCZY9d-1V0$[git[.]kernel[.]org]
>
> * How do you think about to increase the application of scope-based resource management?

firstly I understood that you might be referring to RAII. but I think
this issue is not related to RAII.
thanks for response.

>
>
> Regards,
> Markus

Re: iio: imu: inv_mpu6050: Remove duplicate code between labels

[Jonathan Cameron]

On Mon, 2 Sep 2024 09:54:14 +0000
Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote:

> Hello,
> 
> beware this patch is buggy. It will break the IRQ handler function of inv_mpu6050 driver.
> 
> The normal code path is going through end_session label without goto, and expect the function return before executing inv_reset_fifo. Without it, the reset FIFO function will be called for every interrupt and is breaking normal functioning of the driver.

Doh.  Indeed.  I missed that entirely by focusing on the error paths, not the good one.


> 
> Best regards,
> JB
> 
> ________________________________________
> From: Markus Elfring <Markus.Elfring@web.de>
> Sent: Monday, September 2, 2024 08:00
> To: Gyeyoung Baek <gye976@gmail.com>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>
> Cc: LKML <linux-kernel@vger.kernel.org>
> Subject: Re: iio: imu: inv_mpu6050: Remove duplicate code between labels
>  
> This Message Is From an Untrusted Sender
> You have not previously corresponded with this sender.
>  
> >>> Hello, I apologize for the insufficient explanation.  
> >>
> >> How will the commit message be improved further?  
> …
> > Since the code is short,  
> 
> This implementation detail can be nice.
> 
> 
> > I think it's fine for now.  
> 
> Please reconsider such a view once more.
> Are imperative wordings also more desirable for a better change description?
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc6*n45__;Iw!!FtrhtPsWDhZ6tw!Hb9yipjKJXmB-DO9gWKADZfQZHI84WEFUc6Ns1iGhpAfvAAyjrnLQRJZLU2Ha0nI8Fs-HBqHFlFbq0Kl-O1CJwYLe776xbRywQ$[git[.]kernel[.]org]
> 
> Regards,
> Markus
> 
> 

Re: [PATCH] iio: imu: inv_mpu6050: Remove duplicate code between labels

[gyeyoung]

On Mon, Sep 2, 2024 at 7:04 PM Jean-Baptiste Maneyrol
<Jean-Baptiste.Maneyrol@tdk.com> wrote:
>
> Hello,
>
> it would be true if there was a return statement before the 2 labels, and that the 2 lables were only error handling case.
>
> But this is not the case, since end_session label is the end of the normal end of the function, not error handling. This is very old code and not very clear, I'm sorry about that.
>
> But the modifications are breaking the code and interrupt handling will not work correctly anymore, since inv_reset_fifo() will be called now every time the function is called.
>
> Best regards,
> JB

I apologize for the confusion, I just realized the logic is incorrect
as well. I didn't test it because I thought it trivial, I will send
patch after verification.

Re: iio: imu: inv_mpu6050: Remove duplicate code between labels

[Markus Elfring]

> …, But I just suggested a minor thing as,

This suggestion can trigger further patch review consequences.


> and since the maintainer made the decision and approved,

How did you get such an impression?


> I did not feel the need to go any further.

There are other software design options to take better into account
for further development considerations (according to the original control flow).

Regards,
Markus