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

From: Hans de Goede <hdegoede@redhat.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: "Ike Panhc" <ike.pan@canonical.com>,
	"Barnabás Pőcze" <pobrn@protonmail.com>,
	"Platform Driver" <platform-driver-x86@vger.kernel.org>,
	"Mark Gross" <mgross@linux.intel.com>
Subject: Re: [PATCH v2 09/24] platform/x86: ideapad-laptop: always propagate error codes from device attributes' show() callback
Date: Mon, 25 Jan 2021 12:40:01 +0100	[thread overview]
Message-ID: <cbee040a-53c8-b58f-3231-58d774bbda0a@redhat.com> (raw)
In-Reply-To: <CAHp75VdmwxZeqY3qdO6AuK3QTF=p+Wn9qByMsLEzaV4VV78QHQ@mail.gmail.com>

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