| Message ID | 20250217140135.896574-6-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | IIO: Accelerometers: Sparse friendly claim of direct mode | expand |
On 17/02/2025 16:01, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > These new functions allow sparse to find failures to release > direct mode reducing chances of bugs over the claim_direct_mode() > functions that are deprecated. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Matti Vaittinen <mazziesaccount@gmail.com> > --- > drivers/iio/accel/kionix-kx022a.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c > index 727e007c5fc1..07dcf5f0599f 100644 > --- a/drivers/iio/accel/kionix-kx022a.c > +++ b/drivers/iio/accel/kionix-kx022a.c > @@ -577,13 +577,12 @@ static int kx022a_write_raw(struct iio_dev *idev, > * issues if users trust the watermark to be reached within known > * time-limit). > */ > - ret = iio_device_claim_direct_mode(idev); > - if (ret) > - return ret; > + if (!iio_device_claim_direct(idev)) > + return -EBUSY; Not really in the scope of this review - but in my opinion the logic of this check is terribly counter intuitive. I mean, > + if (iio_device_claim_direct(idev)) > + return -EBUSY; would feel much more familiar. I actually had to look up the implementation of the iio_device_claim_direct() to see this was not a bug. Other than that this looks very good to me. Yours, -- Matti
On 2/18/25 1:39 AM, Matti Vaittinen wrote: > On 17/02/2025 16:01, Jonathan Cameron wrote: >> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> >> These new functions allow sparse to find failures to release >> direct mode reducing chances of bugs over the claim_direct_mode() >> functions that are deprecated. >> >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> Cc: Matti Vaittinen <mazziesaccount@gmail.com> >> --- >> drivers/iio/accel/kionix-kx022a.c | 14 ++++++-------- >> 1 file changed, 6 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c >> index 727e007c5fc1..07dcf5f0599f 100644 >> --- a/drivers/iio/accel/kionix-kx022a.c >> +++ b/drivers/iio/accel/kionix-kx022a.c >> @@ -577,13 +577,12 @@ static int kx022a_write_raw(struct iio_dev *idev, >> * issues if users trust the watermark to be reached within known >> * time-limit). >> */ >> - ret = iio_device_claim_direct_mode(idev); >> - if (ret) >> - return ret; >> + if (!iio_device_claim_direct(idev)) >> + return -EBUSY; > > Not really in the scope of this review - but in my opinion the logic of this check is terribly counter intuitive. I mean, > >> + if (iio_device_claim_direct(idev)) >> + return -EBUSY; I'm curious how you read this then. I read this as: "If claiming direct mode succeeded, then return an error!" Returning an error on success seem very counterintuitive to me. And the way Jonathan implemented it seems the logical way to do it. "If claiming direct mode did not succeed, then return an error." > > would feel much more familiar. I actually had to look up the implementation of the iio_device_claim_direct() to see this was not a bug. > > Other than that this looks very good to me. > > Yours, > -- Matti
On 18/02/2025 17:42, David Lechner wrote: > On 2/18/25 1:39 AM, Matti Vaittinen wrote: >> On 17/02/2025 16:01, Jonathan Cameron wrote: >>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>> >>> These new functions allow sparse to find failures to release >>> direct mode reducing chances of bugs over the claim_direct_mode() >>> functions that are deprecated. >>> >>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>> Cc: Matti Vaittinen <mazziesaccount@gmail.com> >>> --- >>> drivers/iio/accel/kionix-kx022a.c | 14 ++++++-------- >>> 1 file changed, 6 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c >>> index 727e007c5fc1..07dcf5f0599f 100644 >>> --- a/drivers/iio/accel/kionix-kx022a.c >>> +++ b/drivers/iio/accel/kionix-kx022a.c >>> @@ -577,13 +577,12 @@ static int kx022a_write_raw(struct iio_dev *idev, >>> * issues if users trust the watermark to be reached within known >>> * time-limit). >>> */ >>> - ret = iio_device_claim_direct_mode(idev); >>> - if (ret) >>> - return ret; >>> + if (!iio_device_claim_direct(idev)) >>> + return -EBUSY; >> >> Not really in the scope of this review - but in my opinion the logic of this check is terribly counter intuitive. I mean, >> >>> + if (iio_device_claim_direct(idev)) >>> + return -EBUSY; > > I'm curious how you read this then. I read this as: > > "If claiming direct mode succeeded, then return an error!" I am used to seeing a pattern where function returning zero indicates a success. I have no statistics but I believe this is true for a vast majority of functions in the kernel. I believe this was the case with the old 'iio_device_claim_direct_mode(idev)' too. I am not saying this is 'absolutely' bad. I can only tell that _I_ really had to go and look up the implementation of the iio_device_claim_direct() in order to review this change to ensure the return value check was not inverted. Yours, -- Matti
On Wed, 2025-02-19 at 07:36 +0200, Matti Vaittinen wrote: > On 18/02/2025 17:42, David Lechner wrote: > > On 2/18/25 1:39 AM, Matti Vaittinen wrote: > > > On 17/02/2025 16:01, Jonathan Cameron wrote: > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > > > These new functions allow sparse to find failures to release > > > > direct mode reducing chances of bugs over the claim_direct_mode() > > > > functions that are deprecated. > > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Cc: Matti Vaittinen <mazziesaccount@gmail.com> > > > > --- > > > > drivers/iio/accel/kionix-kx022a.c | 14 ++++++-------- > > > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/iio/accel/kionix-kx022a.c > > > > b/drivers/iio/accel/kionix-kx022a.c > > > > index 727e007c5fc1..07dcf5f0599f 100644 > > > > --- a/drivers/iio/accel/kionix-kx022a.c > > > > +++ b/drivers/iio/accel/kionix-kx022a.c > > > > @@ -577,13 +577,12 @@ static int kx022a_write_raw(struct iio_dev *idev, > > > > * issues if users trust the watermark to be reached within known > > > > * time-limit). > > > > */ > > > > - ret = iio_device_claim_direct_mode(idev); > > > > - if (ret) > > > > - return ret; > > > > + if (!iio_device_claim_direct(idev)) > > > > + return -EBUSY; > > > > > > Not really in the scope of this review - but in my opinion the logic of > > > this check is terribly counter intuitive. I mean, > > > > > > > + if (iio_device_claim_direct(idev)) > > > > + return -EBUSY; > > > > I'm curious how you read this then. I read this as: > > > > "If claiming direct mode succeeded, then return an error!" > > I am used to seeing a pattern where function returning zero indicates a > success. I have no statistics but I believe this is true for a vast > majority of functions in the kernel. I believe this was the case with > the old 'iio_device_claim_direct_mode(idev)' too. > Fair enough... Note though this is returning a boolean where true makes total sense for the "good" case. I do agree it's not super clear just by reading the code that the API is supposed to return a boolean. - Nuno Sá
On 19/02/2025 12:51, Nuno Sá wrote: > On Wed, 2025-02-19 at 07:36 +0200, Matti Vaittinen wrote: >> On 18/02/2025 17:42, David Lechner wrote: >>> On 2/18/25 1:39 AM, Matti Vaittinen wrote: >>>> On 17/02/2025 16:01, Jonathan Cameron wrote: >>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>>>> >>>>> These new functions allow sparse to find failures to release >>>>> direct mode reducing chances of bugs over the claim_direct_mode() >>>>> functions that are deprecated. >>>>> >>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>>>> Cc: Matti Vaittinen <mazziesaccount@gmail.com> >>>>> --- >>>>> drivers/iio/accel/kionix-kx022a.c | 14 ++++++-------- >>>>> 1 file changed, 6 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/drivers/iio/accel/kionix-kx022a.c >>>>> b/drivers/iio/accel/kionix-kx022a.c >>>>> index 727e007c5fc1..07dcf5f0599f 100644 >>>>> --- a/drivers/iio/accel/kionix-kx022a.c >>>>> +++ b/drivers/iio/accel/kionix-kx022a.c >>>>> @@ -577,13 +577,12 @@ static int kx022a_write_raw(struct iio_dev *idev, >>>>> * issues if users trust the watermark to be reached within known >>>>> * time-limit). >>>>> */ >>>>> - ret = iio_device_claim_direct_mode(idev); >>>>> - if (ret) >>>>> - return ret; >>>>> + if (!iio_device_claim_direct(idev)) >>>>> + return -EBUSY; >>>> >>>> Not really in the scope of this review - but in my opinion the logic of >>>> this check is terribly counter intuitive. I mean, >>>> >>>>> + if (iio_device_claim_direct(idev)) >>>>> + return -EBUSY; >>> >>> I'm curious how you read this then. I read this as: >>> >>> "If claiming direct mode succeeded, then return an error!" >> >> I am used to seeing a pattern where function returning zero indicates a >> success. I have no statistics but I believe this is true for a vast >> majority of functions in the kernel. I believe this was the case with >> the old 'iio_device_claim_direct_mode(idev)' too. >> > > Fair enough... Note though this is returning a boolean where true makes total > sense for the "good" case. I do agree it's not super clear just by reading the > code that the API is supposed to return a boolean. Exactly. Just seeing the call in code was not obvious to me. It required finding the prototype to understand what happens. Anyways, I guess this discussion is out of the scope of this patch and if no one else sees this important enough to go and change the iio_device_claim_direct() - then I am fine with this patch. So, with a bit of teeth grinding: Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> Yours, -- Matti
On 2/19/25 6:21 AM, Matti Vaittinen wrote: > On 19/02/2025 12:51, Nuno Sá wrote: >> On Wed, 2025-02-19 at 07:36 +0200, Matti Vaittinen wrote: >>> On 18/02/2025 17:42, David Lechner wrote: >>>> On 2/18/25 1:39 AM, Matti Vaittinen wrote: >>>>> On 17/02/2025 16:01, Jonathan Cameron wrote: >>>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>>>>> >>>>>> These new functions allow sparse to find failures to release >>>>>> direct mode reducing chances of bugs over the claim_direct_mode() >>>>>> functions that are deprecated. >>>>>> >>>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>>>>> Cc: Matti Vaittinen <mazziesaccount@gmail.com> >>>>>> --- >>>>>> drivers/iio/accel/kionix-kx022a.c | 14 ++++++-------- >>>>>> 1 file changed, 6 insertions(+), 8 deletions(-) >>>>>> >>>>>> diff --git a/drivers/iio/accel/kionix-kx022a.c >>>>>> b/drivers/iio/accel/kionix-kx022a.c >>>>>> index 727e007c5fc1..07dcf5f0599f 100644 >>>>>> --- a/drivers/iio/accel/kionix-kx022a.c >>>>>> +++ b/drivers/iio/accel/kionix-kx022a.c >>>>>> @@ -577,13 +577,12 @@ static int kx022a_write_raw(struct iio_dev *idev, >>>>>> * issues if users trust the watermark to be reached within known >>>>>> * time-limit). >>>>>> */ >>>>>> - ret = iio_device_claim_direct_mode(idev); >>>>>> - if (ret) >>>>>> - return ret; >>>>>> + if (!iio_device_claim_direct(idev)) >>>>>> + return -EBUSY; >>>>> >>>>> Not really in the scope of this review - but in my opinion the logic of >>>>> this check is terribly counter intuitive. I mean, >>>>> >>>>>> + if (iio_device_claim_direct(idev)) >>>>>> + return -EBUSY; >>>> >>>> I'm curious how you read this then. I read this as: >>>> >>>> "If claiming direct mode succeeded, then return an error!" >>> >>> I am used to seeing a pattern where function returning zero indicates a >>> success. I have no statistics but I believe this is true for a vast >>> majority of functions in the kernel. I believe this was the case with >>> the old 'iio_device_claim_direct_mode(idev)' too. >>> >> >> Fair enough... Note though this is returning a boolean where true makes total >> sense for the "good" case. I do agree it's not super clear just by reading the >> code that the API is supposed to return a boolean. > > Exactly. Just seeing the call in code was not obvious to me. It required finding the prototype to understand what happens. > > Anyways, I guess this discussion is out of the scope of this patch and if no one else sees this important enough to go and change the iio_device_claim_direct() - then I am fine with this patch. So, with a bit of teeth grinding: > > Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> > > Yours, > -- Matti > > Would a name like iio_device_try_claim_direct_mode() make it more obvious that it returned a bool instead of int?
On Wed, 19 Feb 2025 14:21:51 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 19/02/2025 12:51, Nuno Sá wrote: > > On Wed, 2025-02-19 at 07:36 +0200, Matti Vaittinen wrote: > >> On 18/02/2025 17:42, David Lechner wrote: > >>> On 2/18/25 1:39 AM, Matti Vaittinen wrote: > >>>> On 17/02/2025 16:01, Jonathan Cameron wrote: > >>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >>>>> > >>>>> These new functions allow sparse to find failures to release > >>>>> direct mode reducing chances of bugs over the claim_direct_mode() > >>>>> functions that are deprecated. > >>>>> > >>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >>>>> Cc: Matti Vaittinen <mazziesaccount@gmail.com> > >>>>> --- > >>>>> drivers/iio/accel/kionix-kx022a.c | 14 ++++++-------- > >>>>> 1 file changed, 6 insertions(+), 8 deletions(-) > >>>>> > >>>>> diff --git a/drivers/iio/accel/kionix-kx022a.c > >>>>> b/drivers/iio/accel/kionix-kx022a.c > >>>>> index 727e007c5fc1..07dcf5f0599f 100644 > >>>>> --- a/drivers/iio/accel/kionix-kx022a.c > >>>>> +++ b/drivers/iio/accel/kionix-kx022a.c > >>>>> @@ -577,13 +577,12 @@ static int kx022a_write_raw(struct iio_dev *idev, > >>>>> * issues if users trust the watermark to be reached within known > >>>>> * time-limit). > >>>>> */ > >>>>> - ret = iio_device_claim_direct_mode(idev); > >>>>> - if (ret) > >>>>> - return ret; > >>>>> + if (!iio_device_claim_direct(idev)) > >>>>> + return -EBUSY; > >>>> > >>>> Not really in the scope of this review - but in my opinion the logic of > >>>> this check is terribly counter intuitive. I mean, > >>>> > >>>>> + if (iio_device_claim_direct(idev)) > >>>>> + return -EBUSY; > >>> > >>> I'm curious how you read this then. I read this as: > >>> > >>> "If claiming direct mode succeeded, then return an error!" > >> > >> I am used to seeing a pattern where function returning zero indicates a > >> success. I have no statistics but I believe this is true for a vast > >> majority of functions in the kernel. I believe this was the case with > >> the old 'iio_device_claim_direct_mode(idev)' too. > >> > > > > Fair enough... Note though this is returning a boolean where true makes total > > sense for the "good" case. I do agree it's not super clear just by reading the > > code that the API is supposed to return a boolean. > > Exactly. Just seeing the call in code was not obvious to me. It required > finding the prototype to understand what happens. > > Anyways, I guess this discussion is out of the scope of this patch and > if no one else sees this important enough to go and change the > iio_device_claim_direct() - then I am fine with this patch. So, with a > bit of teeth grinding: > > Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> This is copying what happens for the locks that can fail. I agree that it would have been nice to get the advantages of sparse with the old interface but from what I recall I got a lot more false positives so wanted it to look more lock like. Jonathan > > Yours, > -- Matti > >
On Wed, 19 Feb 2025 09:25:00 -0600 David Lechner <dlechner@baylibre.com> wrote: > On 2/19/25 6:21 AM, Matti Vaittinen wrote: > > On 19/02/2025 12:51, Nuno Sá wrote: > >> On Wed, 2025-02-19 at 07:36 +0200, Matti Vaittinen wrote: > >>> On 18/02/2025 17:42, David Lechner wrote: > >>>> On 2/18/25 1:39 AM, Matti Vaittinen wrote: > >>>>> On 17/02/2025 16:01, Jonathan Cameron wrote: > >>>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >>>>>> > >>>>>> These new functions allow sparse to find failures to release > >>>>>> direct mode reducing chances of bugs over the claim_direct_mode() > >>>>>> functions that are deprecated. > >>>>>> > >>>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >>>>>> Cc: Matti Vaittinen <mazziesaccount@gmail.com> > >>>>>> --- > >>>>>> drivers/iio/accel/kionix-kx022a.c | 14 ++++++-------- > >>>>>> 1 file changed, 6 insertions(+), 8 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/iio/accel/kionix-kx022a.c > >>>>>> b/drivers/iio/accel/kionix-kx022a.c > >>>>>> index 727e007c5fc1..07dcf5f0599f 100644 > >>>>>> --- a/drivers/iio/accel/kionix-kx022a.c > >>>>>> +++ b/drivers/iio/accel/kionix-kx022a.c > >>>>>> @@ -577,13 +577,12 @@ static int kx022a_write_raw(struct iio_dev *idev, > >>>>>> * issues if users trust the watermark to be reached within known > >>>>>> * time-limit). > >>>>>> */ > >>>>>> - ret = iio_device_claim_direct_mode(idev); > >>>>>> - if (ret) > >>>>>> - return ret; > >>>>>> + if (!iio_device_claim_direct(idev)) > >>>>>> + return -EBUSY; > >>>>> > >>>>> Not really in the scope of this review - but in my opinion the logic of > >>>>> this check is terribly counter intuitive. I mean, > >>>>> > >>>>>> + if (iio_device_claim_direct(idev)) > >>>>>> + return -EBUSY; > >>>> > >>>> I'm curious how you read this then. I read this as: > >>>> > >>>> "If claiming direct mode succeeded, then return an error!" > >>> > >>> I am used to seeing a pattern where function returning zero indicates a > >>> success. I have no statistics but I believe this is true for a vast > >>> majority of functions in the kernel. I believe this was the case with > >>> the old 'iio_device_claim_direct_mode(idev)' too. > >>> > >> > >> Fair enough... Note though this is returning a boolean where true makes total > >> sense for the "good" case. I do agree it's not super clear just by reading the > >> code that the API is supposed to return a boolean. > > > > Exactly. Just seeing the call in code was not obvious to me. It required finding the prototype to understand what happens. > > > > Anyways, I guess this discussion is out of the scope of this patch and if no one else sees this important enough to go and change the iio_device_claim_direct() - then I am fine with this patch. So, with a bit of teeth grinding: > > > > Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> > > > > Yours, > > -- Matti > > > > > > Would a name like iio_device_try_claim_direct_mode() make it more > obvious that it returned a bool instead of int? FWIW I'd consider this a reasonable change if people in general find it more intuitive. Conveys to those not familiar with the fun of IIO that failure is something we kind of expect to happen. Slightly messy to change the patches already applied to my tree but cleaner to do so now than later as I haven't pushed the branch out as togreg yet (it's just the testing branch for 0-day). Jonathan >
On 19/02/2025 17:25, David Lechner wrote: > On 2/19/25 6:21 AM, Matti Vaittinen wrote: >> On 19/02/2025 12:51, Nuno Sá wrote: >>> On Wed, 2025-02-19 at 07:36 +0200, Matti Vaittinen wrote: >>>> On 18/02/2025 17:42, David Lechner wrote: >>>>> On 2/18/25 1:39 AM, Matti Vaittinen wrote: >>>>>> On 17/02/2025 16:01, Jonathan Cameron wrote: >>>>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>>>>>> >>>>>>> These new functions allow sparse to find failures to release >>>>>>> direct mode reducing chances of bugs over the claim_direct_mode() >>>>>>> functions that are deprecated. >>>>>>> >>>>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>>>>>> Cc: Matti Vaittinen <mazziesaccount@gmail.com> >>>>>>> --- >>>>>>> drivers/iio/accel/kionix-kx022a.c | 14 ++++++-------- >>>>>>> 1 file changed, 6 insertions(+), 8 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/iio/accel/kionix-kx022a.c >>>>>>> b/drivers/iio/accel/kionix-kx022a.c >>>>>>> index 727e007c5fc1..07dcf5f0599f 100644 >>>>>>> --- a/drivers/iio/accel/kionix-kx022a.c >>>>>>> +++ b/drivers/iio/accel/kionix-kx022a.c >>>>>>> @@ -577,13 +577,12 @@ static int kx022a_write_raw(struct iio_dev *idev, >>>>>>> * issues if users trust the watermark to be reached within known >>>>>>> * time-limit). >>>>>>> */ >>>>>>> - ret = iio_device_claim_direct_mode(idev); >>>>>>> - if (ret) >>>>>>> - return ret; >>>>>>> + if (!iio_device_claim_direct(idev)) >>>>>>> + return -EBUSY; >>>>>> >>>>>> Not really in the scope of this review - but in my opinion the logic of >>>>>> this check is terribly counter intuitive. I mean, >>>>>> >>>>>>> + if (iio_device_claim_direct(idev)) >>>>>>> + return -EBUSY; >>>>> >>>>> I'm curious how you read this then. I read this as: >>>>> >>>>> "If claiming direct mode succeeded, then return an error!" >>>> >>>> I am used to seeing a pattern where function returning zero indicates a >>>> success. I have no statistics but I believe this is true for a vast >>>> majority of functions in the kernel. I believe this was the case with >>>> the old 'iio_device_claim_direct_mode(idev)' too. >>>> >>> >>> Fair enough... Note though this is returning a boolean where true makes total >>> sense for the "good" case. I do agree it's not super clear just by reading the >>> code that the API is supposed to return a boolean. >> >> Exactly. Just seeing the call in code was not obvious to me. It required finding the prototype to understand what happens. >> >> Anyways, I guess this discussion is out of the scope of this patch and if no one else sees this important enough to go and change the iio_device_claim_direct() - then I am fine with this patch. So, with a bit of teeth grinding: >> >> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> >> >> Yours, >> -- Matti > > Would a name like iio_device_try_claim_direct_mode() make it more > obvious that it returned a bool instead of int? In general? I don't know. For me ... I am afraid I wouldn't have guessed the type of the return value (or 0 == "failure to claim direct") even with such name. It's still fair to say that I do _really_ rarely use stuff like mutex_trylock(), so I can't say if different naming would help someone else who uses those variants more. What I would expect is -EBUSY when claiming fails, 0 if it succeeds :) If this won't work for what ever reasons, then I'll just live with this function using bool and returning true on success, and move on ;) Yours, -- Matti
On 19/02/2025 21:05, Jonathan Cameron wrote: > On Wed, 19 Feb 2025 09:25:00 -0600 > David Lechner <dlechner@baylibre.com> wrote: > >> On 2/19/25 6:21 AM, Matti Vaittinen wrote: >>> On 19/02/2025 12:51, Nuno Sá wrote: >>>> On Wed, 2025-02-19 at 07:36 +0200, Matti Vaittinen wrote: >>>>> On 18/02/2025 17:42, David Lechner wrote: >>>>>> On 2/18/25 1:39 AM, Matti Vaittinen wrote: >>>>>>> On 17/02/2025 16:01, Jonathan Cameron wrote: >>>>>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>>>>>>> >>>>>>>> These new functions allow sparse to find failures to release >>>>>>>> direct mode reducing chances of bugs over the claim_direct_mode() >>>>>>>> functions that are deprecated. >>>>>>>> >>>>>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>>>>>>> Cc: Matti Vaittinen <mazziesaccount@gmail.com> >>>>>>>> --- >>>>>>>> drivers/iio/accel/kionix-kx022a.c | 14 ++++++-------- >>>>>>>> 1 file changed, 6 insertions(+), 8 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/iio/accel/kionix-kx022a.c >>>>>>>> b/drivers/iio/accel/kionix-kx022a.c >>>>>>>> index 727e007c5fc1..07dcf5f0599f 100644 >>>>>>>> --- a/drivers/iio/accel/kionix-kx022a.c >>>>>>>> +++ b/drivers/iio/accel/kionix-kx022a.c >>>>>>>> @@ -577,13 +577,12 @@ static int kx022a_write_raw(struct iio_dev *idev, >>>>>>>> * issues if users trust the watermark to be reached within known >>>>>>>> * time-limit). >>>>>>>> */ >>>>>>>> - ret = iio_device_claim_direct_mode(idev); >>>>>>>> - if (ret) >>>>>>>> - return ret; >>>>>>>> + if (!iio_device_claim_direct(idev)) >>>>>>>> + return -EBUSY; >>>>>>> >>>>>>> Not really in the scope of this review - but in my opinion the logic of >>>>>>> this check is terribly counter intuitive. I mean, >>>>>>> >>>>>>>> + if (iio_device_claim_direct(idev)) >>>>>>>> + return -EBUSY; >>>>>> >>>>>> I'm curious how you read this then. I read this as: >>>>>> >>>>>> "If claiming direct mode succeeded, then return an error!" >>>>> >>>>> I am used to seeing a pattern where function returning zero indicates a >>>>> success. I have no statistics but I believe this is true for a vast >>>>> majority of functions in the kernel. I believe this was the case with >>>>> the old 'iio_device_claim_direct_mode(idev)' too. >>>>> >>>> >>>> Fair enough... Note though this is returning a boolean where true makes total >>>> sense for the "good" case. I do agree it's not super clear just by reading the >>>> code that the API is supposed to return a boolean. >>> >>> Exactly. Just seeing the call in code was not obvious to me. It required finding the prototype to understand what happens. >>> >>> Anyways, I guess this discussion is out of the scope of this patch and if no one else sees this important enough to go and change the iio_device_claim_direct() - then I am fine with this patch. So, with a bit of teeth grinding: >>> >>> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> >>> >>> Yours, >>> -- Matti >>> >>> >> >> Would a name like iio_device_try_claim_direct_mode() make it more >> obvious that it returned a bool instead of int? > > FWIW I'd consider this a reasonable change if people in general > find it more intuitive. Conveys to those not familiar with the > fun of IIO that failure is something we kind of expect to happen. As I replied to David's mail - for me renaming is not likely to make a big difference - but maybe it would help someone who is more used to the mutex_trylock() and alike. I'd still like to see someone else thinking that renaming would help before asking for anyone to go through that hassle. Yours, -- Matti
diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c index 727e007c5fc1..07dcf5f0599f 100644 --- a/drivers/iio/accel/kionix-kx022a.c +++ b/drivers/iio/accel/kionix-kx022a.c @@ -577,13 +577,12 @@ static int kx022a_write_raw(struct iio_dev *idev, * issues if users trust the watermark to be reached within known * time-limit). */ - ret = iio_device_claim_direct_mode(idev); - if (ret) - return ret; + if (!iio_device_claim_direct(idev)) + return -EBUSY; ret = __kx022a_write_raw(idev, chan, val, val2, mask); - iio_device_release_direct_mode(idev); + iio_device_release_direct(idev); return ret; } @@ -624,15 +623,14 @@ static int kx022a_read_raw(struct iio_dev *idev, switch (mask) { case IIO_CHAN_INFO_RAW: - ret = iio_device_claim_direct_mode(idev); - if (ret) - return ret; + if (!iio_device_claim_direct(idev)) + return -EBUSY; mutex_lock(&data->mutex); ret = kx022a_get_axis(data, chan, val); mutex_unlock(&data->mutex); - iio_device_release_direct_mode(idev); + iio_device_release_direct(idev); return ret;