Message-Id: <DAXMVOI4AXHY.18HUV9THTG0DJ@gmail.com>
Date: Fri, 27 Jun 2025 18:29:19 -0300
From: "Kurt Borja" <kuurtb@...il.com>
To: "Armin Wolf" <W_Armin@....de>, <xy-jackie@....com>,
 <alireza.bestboyy@...il.com>, <atescula@...il.com>
Cc: <mpearson-lenovo@...ebb.ca>, <hdegoede@...hat.com>,
 <ilpo.jarvinen@...ux.intel.com>, <platform-driver-x86@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] platform/x86: lenovo-hotkey: Handle missing hardware
 features gracefully

On Fri Jun 27, 2025 at 6:17 PM -03, Armin Wolf wrote:
> Am 27.06.25 um 22:38 schrieb Kurt Borja:
>
>> Hi Armin,
>>
>> On Fri Jun 27, 2025 at 4:54 PM -03, Armin Wolf wrote:
>>> Not all devices support audio mute and microphone mute LEDs, so the
>>> explicitly checks for hardware support while probing. However missing
>>> hardware features are treated as errors, causing the driver so fail
>>> probing on devices that do not support both LEDs.
>>>
>>> Fix this by simply ignoring hardware features that are not present.
>>> This way the driver will properly load on devices not supporting both
>>> LEDs and will stop throwing error messages on devices with no LEDS
>>> at all.
>> This patch makes me wonder what is the policy around issues like this.
>> In fact I've submitted and changes that do the exact opposite :p
>> Like commit: 4630b99d2e93 ("platform/x86: dell-pc: Propagate errors when
>> detecting feature support")
>>
>> IMO missing features should be treated as errors. i.e. The probe should
>> fail.
>
> IMHO the probe should only fail if some features are deemed essential, like
> required ACPI methods. Optional features like in this case LEDs should be
> handled by the driver in a graceful manner if possible.
>
>>
>> Quoting documentation [1]:
>>
>> 	If a match is found, the device’s driver field is set to the
>> 	driver and the driver’s probe callback is called. This gives the
>> 	driver a chance to verify that it really does support the
>> 	hardware, and that it’s in a working state.
>>
>> And again [2]:
>>
>> 	This callback holds the driver-specific logic to bind the driver
>> 	to a given device. That includes verifying that the device is
>> 	present, that it’s a version the driver can handle, that driver
>> 	data structures can be allocated and initialized, and that any
>> 	hardware can be initialized.
>>
>> Both of these makes me wonder if such a "fail" or error message should
>> be fixed in the first place. In this case the probe correctly checks for
>> device support and fails if it's not found, which is suggested to be the
>> correct behavior.
>
> The driver should only fail probing if it cannot handle some missing features.
> In this case however both features (audio mute LED and mic mute LED) are completely
> optional and the driver should not fail to load just because one of them is absent.

I agree, both are individually optional, but at least one should be
required.

>
> Just think about machines supporting only a single LED (audio or mic mute). Currently
> the driver would fail to load on such devices leaving the users with nothing.

That's very true.

But I do still think if both fail the probe should still fail. Maybe
there is a way to accomplish this?

I'm thinking of something like

if (lenovo_super_hotkey_wmi_led_init(MIC_MUTE, dev) ||
    lenovo_super_hotkey_wmi_led_init(AUDIO_MUTE, dev))
    return -ENODEV;

What do you think?

>
>>
>> BTW this also leaks `wpriv`, which would remain allocated for no reason.
>
> wpriv will be freed using devres, so no memory leak here. However i admit that there is
> some room for optimizations, however i leave this to the maintainer of the driver in
> question.

Leak was a bit of an overstatement :) But if both features are missing
it would be kinda leaked, in practice.

>
> Thanks,
> Armin Wolf
>
>>
>>
>> [1] https://docs.kernel.org/driver-api/driver-model/binding.html
>> [2] https://docs.kernel.org/driver-api/driver-model/driver.html
>>


-- 
 ~ Kurt


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists