| Message ID | 20240901091214.15199-1-gye976@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: imu: inv_mpu6050: Remove duplicate code between labels | expand |
> '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
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
> Hello, I apologize for the insufficient explanation.
How will the commit message be improved further?
Regards,
Markus
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.
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
>>> 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, 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
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
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 > >
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.
> …, 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
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);
'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(-)