diff mbox series

[v2,09/24] platform/x86: ideapad-laptop: always propagate error codes from device attributes' show() callback

Message ID 20210113182016.166049-10-pobrn@protonmail.com (mailing list archive)
State Changes Requested, archived
Headers show
Series platform/x86: ideapad-laptop: cleanup, keyboard backlight and "always on USB charging" control support, reenable touchpad control | expand

Commit Message

Barnabás Pőcze Jan. 13, 2021, 6:21 p.m. UTC 
Consumers can differentiate an error from a successful read much more
easily if the read() call fails with the appropriate errno instead of
returning a magic string like "-1".

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Comments

Andy Shevchenko Jan. 16, 2021, 7:49 p.m. UTC | #1 
On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Consumers can differentiate an error from a successful read much more
> easily if the read() call fails with the appropriate errno instead of
> returning a magic string like "-1".

Is user space ready for this (for the record, it seems an ABI breakage)?
Ike Panhc Jan. 25, 2021, 7:36 a.m. UTC | #2 
On 1/17/21 3:49 AM, Andy Shevchenko wrote:
> On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>
>> Consumers can differentiate an error from a successful read much more
>> easily if the read() call fails with the appropriate errno instead of
>> returning a magic string like "-1".
> 
> Is user space ready for this (for the record, it seems an ABI breakage)?
> 

read() and getting errno looks sysfs/driver broken to me. I think
if button/method is not available, it's better to be something like
sysfs_emit(buf, "%d\n", -ENODEV)

--
Ike Panhc
Andy Shevchenko Jan. 25, 2021, 10:57 a.m. UTC | #3 
On Mon, Jan 25, 2021 at 9:37 AM Ike Panhc <ike.pan@canonical.com> wrote:
>
> On 1/17/21 3:49 AM, Andy Shevchenko wrote:
> > On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >>
> >> Consumers can differentiate an error from a successful read much more
> >> easily if the read() call fails with the appropriate errno instead of
> >> returning a magic string like "-1".
> >
> > Is user space ready for this (for the record, it seems an ABI breakage)?
> >
>
> read() and getting errno looks sysfs/driver broken to me. I think
> if button/method is not available, it's better to be something like
> sysfs_emit(buf, "%d\n", -ENODEV)

Either way it will be an ABI breakage.
Hans de Goede Jan. 25, 2021, 11:26 a.m. UTC | #4 
Hi,

On 1/25/21 11:57 AM, Andy Shevchenko wrote:
> On Mon, Jan 25, 2021 at 9:37 AM Ike Panhc <ike.pan@canonical.com> wrote:
>>
>> On 1/17/21 3:49 AM, Andy Shevchenko wrote:
>>> On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>>>
>>>> Consumers can differentiate an error from a successful read much more
>>>> easily if the read() call fails with the appropriate errno instead of
>>>> returning a magic string like "-1".
>>>
>>> Is user space ready for this (for the record, it seems an ABI breakage)?
>>>
>>
>> read() and getting errno looks sysfs/driver broken to me. I think
>> if button/method is not available, it's better to be something like
>> sysfs_emit(buf, "%d\n", -ENODEV)
> 
> Either way it will be an ABI breakage.

True any change here will be an ABI breakage, but I really do not expect
anything to be dependent on this weird behavior of returning errors by
writing some magic value to the buffer, rather then just error-ing out
of the read() call.

The kernel-convention here clearly is to make the read() syscall fail with
-ESOMETHING on errors. So I see this as making the driver conform to the
expected sysfs API behavior. Since this change is nicely split out into a
separate patch, we can always revert it if it turns out there actually
is something depending on this. But again I see that as highly
unlikely.

Regards,

Hans
Andy Shevchenko Jan. 25, 2021, 11:35 a.m. UTC | #5 
On Mon, Jan 25, 2021 at 1:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 1/25/21 11:57 AM, Andy Shevchenko wrote:
> > On Mon, Jan 25, 2021 at 9:37 AM Ike Panhc <ike.pan@canonical.com> wrote:
> >>
> >> On 1/17/21 3:49 AM, Andy Shevchenko wrote:
> >>> On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >>>>
> >>>> Consumers can differentiate an error from a successful read much more
> >>>> easily if the read() call fails with the appropriate errno instead of
> >>>> returning a magic string like "-1".
> >>>
> >>> Is user space ready for this (for the record, it seems an ABI breakage)?
> >>>
> >>
> >> read() and getting errno looks sysfs/driver broken to me. I think
> >> if button/method is not available, it's better to be something like
> >> sysfs_emit(buf, "%d\n", -ENODEV)
> >
> > Either way it will be an ABI breakage.
>
> True any change here will be an ABI breakage, but I really do not expect
> anything to be dependent on this weird behavior of returning errors by
> writing some magic value to the buffer, rather then just error-ing out
> of the read() call.
>
> The kernel-convention here clearly is to make the read() syscall fail with
> -ESOMETHING on errors. So I see this as making the driver conform to the
> expected sysfs API behavior. Since this change is nicely split out into a
> separate patch, we can always revert it if it turns out there actually
> is something depending on this. But again I see that as highly
> unlikely.

Me too. My point is that every stakeholder here understands that.
Perhaps elaborated in the commit message.
Hans de Goede Jan. 25, 2021, 11:40 a.m. UTC | #6 
Hi,

On 1/25/21 12:35 PM, Andy Shevchenko wrote:
> On Mon, Jan 25, 2021 at 1:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 1/25/21 11:57 AM, Andy Shevchenko wrote:
>>> On Mon, Jan 25, 2021 at 9:37 AM Ike Panhc <ike.pan@canonical.com> wrote:
>>>>
>>>> On 1/17/21 3:49 AM, Andy Shevchenko wrote:
>>>>> On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>>>>>
>>>>>> Consumers can differentiate an error from a successful read much more
>>>>>> easily if the read() call fails with the appropriate errno instead of
>>>>>> returning a magic string like "-1".
>>>>>
>>>>> Is user space ready for this (for the record, it seems an ABI breakage)?
>>>>>
>>>>
>>>> read() and getting errno looks sysfs/driver broken to me. I think
>>>> if button/method is not available, it's better to be something like
>>>> sysfs_emit(buf, "%d\n", -ENODEV)
>>>
>>> Either way it will be an ABI breakage.
>>
>> True any change here will be an ABI breakage, but I really do not expect
>> anything to be dependent on this weird behavior of returning errors by
>> writing some magic value to the buffer, rather then just error-ing out
>> of the read() call.
>>
>> The kernel-convention here clearly is to make the read() syscall fail with
>> -ESOMETHING on errors. So I see this as making the driver conform to the
>> expected sysfs API behavior. Since this change is nicely split out into a
>> separate patch, we can always revert it if it turns out there actually
>> is something depending on this. But again I see that as highly
>> unlikely.
> 
> Me too. My point is that every stakeholder here understands that.
> Perhaps elaborated in the commit message.

Ack, adding a note about this to the commit message would be good.

Barnabás, can you add a note about this to the commit message?

Also I think we are about ready for you to post a v3 of this
series (when you have time to do so).

Regards,

Hans
Barnabás Pőcze Jan. 25, 2021, 2:17 p.m. UTC | #7 
2021. január 25., hétfő 12:40 keltezéssel, Hans de Goede írta:

> Hi,
>
> On 1/25/21 12:35 PM, Andy Shevchenko wrote:
> > On Mon, Jan 25, 2021 at 1:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 1/25/21 11:57 AM, Andy Shevchenko wrote:
> >>> On Mon, Jan 25, 2021 at 9:37 AM Ike Panhc <ike.pan@canonical.com> wrote:
> >>>>
> >>>> On 1/17/21 3:49 AM, Andy Shevchenko wrote:
> >>>>> On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >>>>>>
> >>>>>> Consumers can differentiate an error from a successful read much more
> >>>>>> easily if the read() call fails with the appropriate errno instead of
> >>>>>> returning a magic string like "-1".
> >>>>>
> >>>>> Is user space ready for this (for the record, it seems an ABI breakage)?
> >>>>>
> >>>>
> >>>> read() and getting errno looks sysfs/driver broken to me. I think
> >>>> if button/method is not available, it's better to be something like
> >>>> sysfs_emit(buf, "%d\n", -ENODEV)
> >>>
> >>> Either way it will be an ABI breakage.
> >>
> >> True any change here will be an ABI breakage, but I really do not expect
> >> anything to be dependent on this weird behavior of returning errors by
> >> writing some magic value to the buffer, rather then just error-ing out
> >> of the read() call.
> >>
> >> The kernel-convention here clearly is to make the read() syscall fail with
> >> -ESOMETHING on errors. So I see this as making the driver conform to the
> >> expected sysfs API behavior. Since this change is nicely split out into a
> >> separate patch, we can always revert it if it turns out there actually
> >> is something depending on this. But again I see that as highly
> >> unlikely.
> >
> > Me too. My point is that every stakeholder here understands that.
> > Perhaps elaborated in the commit message.
>
> Ack, adding a note about this to the commit message would be good.
>
> Barnabás, can you add a note about this to the commit message?
>
> Also I think we are about ready for you to post a v3 of this
> series (when you have time to do so).
>

Yes, I can add a note about this. I've been relatively busy for some time,
but I'm planning to submit the new version shortly (by the end of this week).
diff mbox series

Patch

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 9bc6c7340876..677d4e10352e 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -353,9 +353,11 @@  static ssize_t show_ideapad_cam(struct device *dev,
 {
 	unsigned long result;
 	struct ideapad_private *priv = dev_get_drvdata(dev);
+	int err;
 
-	if (read_ec_data(priv->adev->handle, VPCCMD_R_CAMERA, &result))
-		return sysfs_emit(buf, "-1\n");
+	err = read_ec_data(priv->adev->handle, VPCCMD_R_CAMERA, &result);
+	if (err)
+		return err;
 	return sysfs_emit(buf, "%lu\n", result);
 }
 
@@ -384,9 +386,11 @@  static ssize_t show_ideapad_fan(struct device *dev,
 {
 	unsigned long result;
 	struct ideapad_private *priv = dev_get_drvdata(dev);
+	int err;
 
-	if (read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result))
-		return sysfs_emit(buf, "-1\n");
+	err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result);
+	if (err)
+		return err;
 	return sysfs_emit(buf, "%lu\n", result);
 }
 
@@ -417,9 +421,11 @@  static ssize_t touchpad_show(struct device *dev,
 {
 	struct ideapad_private *priv = dev_get_drvdata(dev);
 	unsigned long result;
+	int err;
 
-	if (read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result))
-		return sysfs_emit(buf, "-1\n");
+	err = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result);
+	if (err)
+		return err;
 	return sysfs_emit(buf, "%lu\n", result);
 }
 
@@ -450,9 +456,11 @@  static ssize_t conservation_mode_show(struct device *dev,
 {
 	struct ideapad_private *priv = dev_get_drvdata(dev);
 	unsigned long result;
+	int err;
 
-	if (method_gbmd(priv->adev->handle, &result))
-		return sysfs_emit(buf, "-1\n");
+	err = method_gbmd(priv->adev->handle, &result);
+	if (err)
+		return err;
 	return sysfs_emit(buf, "%u\n", test_bit(BM_CONSERVATION_BIT, &result));
 }
 
@@ -488,7 +496,7 @@  static ssize_t fn_lock_show(struct device *dev,
 	int fail = read_method_int(priv->adev->handle, "HALS", &hals);
 
 	if (fail)
-		return sysfs_emit(buf, "-1\n");
+		return fail;
 
 	result = hals;
 	return sysfs_emit(buf, "%u\n", test_bit(HA_FNLOCK_BIT, &result));