| Message ID | 20221004134909.1692021-16-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Make 'mlock' really private | expand |
On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > The pattern used in this device does not quite fit in the > iio_device_claim_direct_mode() typical usage. In this case, we want to > know if we are in buffered mode or not to know if the device is powered > (buffer mode) or not. And depending on that max30102_get_temp() will > power on the device if needed. Hence, in order to keep the same > functionality, we try to: > > 1. Claim Buffered mode; > 2: If 1) succeeds call max30102_get_temp() without powering on the > device; > 3: Release Buffered mode; > 4: If 1) fails, Claim Direct mode; > 5: If 4) succeeds call max30102_get_temp() with powering on the device; > 6: Release Direct mode; > 7: If 4) fails, goto to 1) and try again. > > This dance between buffered and direct mode is not particularly pretty > (as well as the loop introduced by the goto statement) but it does allow > us to get rid of the mlock usage while keeping the same behavior. ... > + if (iio_device_claim_buffer_mode(indio_dev)) { Why is ret not used here? > + /* > + * This one is a *bit* hacky. If we cannot claim buffer > + * mode, then try direct mode so that we make sure > + * things cannot concurrently change. And we just keep > + * trying until we get one of the modes... > + */ > + if (iio_device_claim_direct_mode(indio_dev)) ...and here? > + goto any_mode_retry; > + } else { > + } I.o.w. what error code will be visible to the caller and why.
On Tue, 2022-10-04 at 17:15 +0300, Andy Shevchenko wrote: > On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > The pattern used in this device does not quite fit in the > > iio_device_claim_direct_mode() typical usage. In this case, we want > > to > > know if we are in buffered mode or not to know if the device is > > powered > > (buffer mode) or not. And depending on that max30102_get_temp() > > will > > power on the device if needed. Hence, in order to keep the same > > functionality, we try to: > > > > 1. Claim Buffered mode; > > 2: If 1) succeeds call max30102_get_temp() without powering on the > > device; > > 3: Release Buffered mode; > > 4: If 1) fails, Claim Direct mode; > > 5: If 4) succeeds call max30102_get_temp() with powering on the > > device; > > 6: Release Direct mode; > > 7: If 4) fails, goto to 1) and try again. > > > > This dance between buffered and direct mode is not particularly > > pretty > > (as well as the loop introduced by the goto statement) but it does > > allow > > us to get rid of the mlock usage while keeping the same behavior. > > ... > > > + if (iio_device_claim_buffer_mode(indio_dev)) { > > Why is ret not used here? > > > + /* > > + * This one is a *bit* hacky. If we cannot > > claim buffer > > + * mode, then try direct mode so that we > > make sure > > + * things cannot concurrently change. And > > we just keep > > + * trying until we get one of the modes... > > + */ > > + if > > (iio_device_claim_direct_mode(indio_dev)) > > ...and here? > > > + goto any_mode_retry; > > > + } else { > > > + } > > I.o.w. what error code will be visible to the caller and why. > Note that we do not really care about the error code returned by both iio_device_claim_buffer_mode() and iio_device_claim_direct_mode(). We just loop until we get one of the modes (thus ret = 0) so that we can safely call one of the max30102_get_temp() variants. And that will be the visible error code (if any). That said, you can look at the first version of the series about this (and the previous patch) and why this is being done like this (note that I'm also not 100% convinced about this ping pong between getting one of the IIO modes but it's also not that bad and if there's no major complains, I'm fine with it). - Nuno Sá
On Wed, 05 Oct 2022 10:17:00 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Tue, 2022-10-04 at 17:15 +0300, Andy Shevchenko wrote: > > On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > > > The pattern used in this device does not quite fit in the > > > iio_device_claim_direct_mode() typical usage. In this case, we want > > > to > > > know if we are in buffered mode or not to know if the device is > > > powered > > > (buffer mode) or not. And depending on that max30102_get_temp() > > > will > > > power on the device if needed. Hence, in order to keep the same > > > functionality, we try to: > > > > > > 1. Claim Buffered mode; > > > 2: If 1) succeeds call max30102_get_temp() without powering on the > > > device; > > > 3: Release Buffered mode; > > > 4: If 1) fails, Claim Direct mode; > > > 5: If 4) succeeds call max30102_get_temp() with powering on the > > > device; > > > 6: Release Direct mode; > > > 7: If 4) fails, goto to 1) and try again. > > > > > > This dance between buffered and direct mode is not particularly > > > pretty > > > (as well as the loop introduced by the goto statement) but it does > > > allow > > > us to get rid of the mlock usage while keeping the same behavior. > > > > ... > > > > > + if (iio_device_claim_buffer_mode(indio_dev)) { > > > > Why is ret not used here? > > > > > + /* > > > + * This one is a *bit* hacky. If we cannot > > > claim buffer > > > + * mode, then try direct mode so that we > > > make sure > > > + * things cannot concurrently change. And > > > we just keep > > > + * trying until we get one of the modes... > > > + */ > > > + if > > > (iio_device_claim_direct_mode(indio_dev)) > > > > ...and here? > > > > > + goto any_mode_retry; > > > > > + } else { > > > > > + } > > > > I.o.w. what error code will be visible to the caller and why. > > > > Note that we do not really care about the error code returned by both > iio_device_claim_buffer_mode() and iio_device_claim_direct_mode(). We > just loop until we get one of the modes (thus ret = 0) so that we can > safely call one of the max30102_get_temp() variants. And that will be > the visible error code (if any). That said, you can look at the first > version of the series about this (and the previous patch) and why this > is being done like this (note that I'm also not 100% convinced about > this ping pong between getting one of the IIO modes but it's also not > that bad and if there's no major complains, I'm fine with it). This is a vanishingly rare corner case and not in a remotely high performance path so I'm not keen on introducing a more complex API just to handle this corner. If we added a means to get the lock independent of mode we'd have an interface that is far to likely to get missused. What you have here does the job and looks nasty enough to put people off copying it unless they really need it! Jonathan > > - Nuno Sá
On Sun, 9 Oct 2022 12:44:40 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Wed, 05 Oct 2022 10:17:00 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Tue, 2022-10-04 at 17:15 +0300, Andy Shevchenko wrote: > > > On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > > > > > The pattern used in this device does not quite fit in the > > > > iio_device_claim_direct_mode() typical usage. In this case, we want > > > > to > > > > know if we are in buffered mode or not to know if the device is > > > > powered > > > > (buffer mode) or not. And depending on that max30102_get_temp() > > > > will > > > > power on the device if needed. Hence, in order to keep the same > > > > functionality, we try to: > > > > > > > > 1. Claim Buffered mode; > > > > 2: If 1) succeeds call max30102_get_temp() without powering on the > > > > device; > > > > 3: Release Buffered mode; > > > > 4: If 1) fails, Claim Direct mode; > > > > 5: If 4) succeeds call max30102_get_temp() with powering on the > > > > device; > > > > 6: Release Direct mode; > > > > 7: If 4) fails, goto to 1) and try again. > > > > > > > > This dance between buffered and direct mode is not particularly > > > > pretty > > > > (as well as the loop introduced by the goto statement) but it does > > > > allow > > > > us to get rid of the mlock usage while keeping the same behavior. > > > > > > ... > > > > > > > + if (iio_device_claim_buffer_mode(indio_dev)) { > > > > > > Why is ret not used here? > > > > > > > + /* > > > > + * This one is a *bit* hacky. If we cannot > > > > claim buffer > > > > + * mode, then try direct mode so that we > > > > make sure > > > > + * things cannot concurrently change. And > > > > we just keep > > > > + * trying until we get one of the modes... > > > > + */ > > > > + if > > > > (iio_device_claim_direct_mode(indio_dev)) > > > > > > ...and here? > > > > > > > + goto any_mode_retry; > > > > > > > + } else { > > > > > > > + } > > > > > > I.o.w. what error code will be visible to the caller and why. > > > > > > > Note that we do not really care about the error code returned by both > > iio_device_claim_buffer_mode() and iio_device_claim_direct_mode(). We > > just loop until we get one of the modes (thus ret = 0) so that we can > > safely call one of the max30102_get_temp() variants. And that will be > > the visible error code (if any). That said, you can look at the first > > version of the series about this (and the previous patch) and why this > > is being done like this (note that I'm also not 100% convinced about > > this ping pong between getting one of the IIO modes but it's also not > > that bad and if there's no major complains, I'm fine with it). > > This is a vanishingly rare corner case and not in a remotely high performance > path so I'm not keen on introducing a more complex API just to handle > this corner. If we added a means to get the lock independent of mode > we'd have an interface that is far to likely to get missused. > What you have here does the job and looks nasty enough to put people off > copying it unless they really need it! > > Jonathan > I should probably have said lgtm for how you have it here. Jonathan
On Sun, 2022-10-09 at 12:44 +0100, Jonathan Cameron wrote: > On Wed, 05 Oct 2022 10:17:00 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Tue, 2022-10-04 at 17:15 +0300, Andy Shevchenko wrote: > > > On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> > > > wrote: > > > > > > > > The pattern used in this device does not quite fit in the > > > > iio_device_claim_direct_mode() typical usage. In this case, we > > > > want > > > > to > > > > know if we are in buffered mode or not to know if the device is > > > > powered > > > > (buffer mode) or not. And depending on that max30102_get_temp() > > > > will > > > > power on the device if needed. Hence, in order to keep the same > > > > functionality, we try to: > > > > > > > > 1. Claim Buffered mode; > > > > 2: If 1) succeeds call max30102_get_temp() without powering on > > > > the > > > > device; > > > > 3: Release Buffered mode; > > > > 4: If 1) fails, Claim Direct mode; > > > > 5: If 4) succeeds call max30102_get_temp() with powering on the > > > > device; > > > > 6: Release Direct mode; > > > > 7: If 4) fails, goto to 1) and try again. > > > > > > > > This dance between buffered and direct mode is not particularly > > > > pretty > > > > (as well as the loop introduced by the goto statement) but it > > > > does > > > > allow > > > > us to get rid of the mlock usage while keeping the same > > > > behavior. > > > > > > ... > > > > > > > + if (iio_device_claim_buffer_mode(indio_dev)) { > > > > > > Why is ret not used here? > > > > > > > + /* > > > > + * This one is a *bit* hacky. If we > > > > cannot > > > > claim buffer > > > > + * mode, then try direct mode so that > > > > we > > > > make sure > > > > + * things cannot concurrently change. > > > > And > > > > we just keep > > > > + * trying until we get one of the > > > > modes... > > > > + */ > > > > + if > > > > (iio_device_claim_direct_mode(indio_dev)) > > > > > > ...and here? > > > > > > > + goto any_mode_retry; > > > > > > > + } else { > > > > > > > + } > > > > > > I.o.w. what error code will be visible to the caller and why. > > > > > > > Note that we do not really care about the error code returned by > > both > > iio_device_claim_buffer_mode() and iio_device_claim_direct_mode(). > > We > > just loop until we get one of the modes (thus ret = 0) so that we > > can > > safely call one of the max30102_get_temp() variants. And that will > > be > > the visible error code (if any). That said, you can look at the > > first > > version of the series about this (and the previous patch) and why > > this > > is being done like this (note that I'm also not 100% convinced > > about > > this ping pong between getting one of the IIO modes but it's also > > not > > that bad and if there's no major complains, I'm fine with it). > > This is a vanishingly rare corner case and not in a remotely high > performance > path so I'm not keen on introducing a more complex API just to handle > this corner. If we added a means to get the lock independent of mode > we'd have an interface that is far to likely to get missused. Totally agree. It crossed my mind to have an helper for getting the lock but that would defeat the purpose of this patchset! Well, I'm also fine with the code as it stands so I'll leave it for v3 and if no one complains I guess we're good to go... - Nuno Sá
diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c index abbcef563807..b8b5ccf582fb 100644 --- a/drivers/iio/health/max30102.c +++ b/drivers/iio/health/max30102.c @@ -477,12 +477,23 @@ static int max30102_read_raw(struct iio_dev *indio_dev, * Temperature reading can only be acquired when not in * shutdown; leave shutdown briefly when buffer not running */ - mutex_lock(&indio_dev->mlock); - if (!iio_buffer_enabled(indio_dev)) +any_mode_retry: + if (iio_device_claim_buffer_mode(indio_dev)) { + /* + * This one is a *bit* hacky. If we cannot claim buffer + * mode, then try direct mode so that we make sure + * things cannot concurrently change. And we just keep + * trying until we get one of the modes... + */ + if (iio_device_claim_direct_mode(indio_dev)) + goto any_mode_retry; + ret = max30102_get_temp(data, val, true); - else + iio_device_release_direct_mode(indio_dev); + } else { ret = max30102_get_temp(data, val, false); - mutex_unlock(&indio_dev->mlock); + iio_device_release_buffer_mode(indio_dev); + } if (ret) return ret;
The pattern used in this device does not quite fit in the iio_device_claim_direct_mode() typical usage. In this case, we want to know if we are in buffered mode or not to know if the device is powered (buffer mode) or not. And depending on that max30102_get_temp() will power on the device if needed. Hence, in order to keep the same functionality, we try to: 1. Claim Buffered mode; 2: If 1) succeeds call max30102_get_temp() without powering on the device; 3: Release Buffered mode; 4: If 1) fails, Claim Direct mode; 5: If 4) succeeds call max30102_get_temp() with powering on the device; 6: Release Direct mode; 7: If 4) fails, goto to 1) and try again. This dance between buffered and direct mode is not particularly pretty (as well as the loop introduced by the goto statement) but it does allow us to get rid of the mlock usage while keeping the same behavior. Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- drivers/iio/health/max30102.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)