| Message ID | 20221012151620.1725215-4-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Make 'mlock' really private | expand |
Hi Nuno, nuno.sa@analog.com wrote on Wed, 12 Oct 2022 17:16:19 +0200: > 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. What about adding a TODO comment saying something like: "this comes from static analysis and helped dropping mlock access, but someone with the device needs to figure out if we can simplify this dance"? Because the reason behind all this is that we don't want to risk breaking the driver, but perhaps a simpler approach would work, right? > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > --- > drivers/iio/health/max30102.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/health/max30102.c > b/drivers/iio/health/max30102.c index 437298a29f2d..66df4aaa31a7 > 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; > Thanks, Miquèl
On Wed, 2022-10-12 at 20:45 +0200, Miquel Raynal wrote: > Hi Nuno, > > nuno.sa@analog.com wrote on Wed, 12 Oct 2022 17:16:19 +0200: > > > 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. > > What about adding a TODO comment saying something like: "this comes > from static analysis and helped dropping mlock access, but someone > with > the device needs to figure out if we can simplify this dance"? > Because > the reason behind all this is that we don't want to risk breaking the > driver, but perhaps a simpler approach would work, right? > Hi Miquel, AFAIU, either the device is powered (when buffer mode enabled) and we can do the reading or it's not and we need to power it on/off "manually" while making sure we don't race against enable/disabling buffers. This "dance" is needed mainly to make sure that we grab 'mlock' one way or another... The other way would be to use some specific device lock together with a flag (as discussed) but as discussed with Jonathan we decided to go down this road... So, honestly, I don't really see the necessity of "marking" this code with a TODO but of course if someone comes in with something simpler, great :). Anyways, as I said, I'm not really keen in spinning a new version to add this comment so I will defer the decision to Jonathan :) Thanks for the help! - Nuno Sá
On Fri, 14 Oct 2022 09:25:59 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Wed, 2022-10-12 at 20:45 +0200, Miquel Raynal wrote: > > Hi Nuno, > > > > nuno.sa@analog.com wrote on Wed, 12 Oct 2022 17:16:19 +0200: > > > > > 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. > > > > What about adding a TODO comment saying something like: "this comes > > from static analysis and helped dropping mlock access, but someone > > with > > the device needs to figure out if we can simplify this dance"? > > Because > > the reason behind all this is that we don't want to risk breaking the > > driver, but perhaps a simpler approach would work, right? > > > > Hi Miquel, > > AFAIU, either the device is powered (when buffer mode enabled) and we > can do the reading or it's not and we need to power it on/off > "manually" while making sure we don't race against enable/disabling > buffers. This "dance" is needed mainly to make sure that we grab > 'mlock' one way or another... The other way would be to use some > specific device lock together with a flag (as discussed) but as > discussed with Jonathan we decided to go down this road... So, > honestly, I don't really see the necessity of "marking" this code with > a TODO but of course if someone comes in with something simpler, great > :). Agreed. I don't expect to see any improvement in this in the future so a TODO would just be noise and might encourage people to propose the 'get the lock on it's own function' that we are going through this dance to avoid adding. Jonathan > > Anyways, as I said, I'm not really keen in spinning a new version to > add this comment so I will defer the decision to Jonathan :) > > Thanks for the help! > - Nuno Sá >
diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c index 437298a29f2d..66df4aaa31a7 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(-)