linux-kernel - Re: [PATCH] HID: amd_sfh: Ignore uninitialized device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3ff059d8-2d89-6ea1-209b-3c8855561acb@amd.com>
Date:   Tue, 28 Jun 2022 10:14:26 -0500
From:   "Limonciello, Mario" <mario.limonciello@....com>
To:     Akihiko Odaki <akihiko.odaki@...il.com>
Cc:     Basavaraj Natikar <basavaraj.natikar@....com>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] HID: amd_sfh: Ignore uninitialized device

On 6/28/2022 10:11, Akihiko Odaki wrote:
> On 2022/06/28 23:42, Limonciello, Mario wrote:
>> On 6/26/2022 03:13, Akihiko Odaki wrote:
>>> Lenovo ThinkPad C13 Yoga has AMD Sensor Fusion Hub, but it is not used
>>> because Chrome OS EC Sensor Hub is used instead. The system therefore
>>> never loads the firmware for MP2 and MP2 does not work. It results in
>>> AMD_P2C_MSG3 register to have -1 as its value.
>>>
>>> Without this change, the driver interprets the value as it supports all
>>> sensor types and exposes them, which confuses a userspace program,
>>> iio-sensor-proxy, and makes it to use the non-functioning sensors
>>> instead of functioning sensors exposed via Chrome OS EC Sensor Hub.
>>>
>>> Check the version bits included in AMD_P2C_MSG3 register and ignore the
>>> device if all of the bits are set.
>>>
>>
>> Have you already confirmed this failure happens in 5.19-rc1 or later 
>> as well?  I would think that b5d7f43e97dabfa04a4be5ff027ce7da119332be 
>> should have fixed it.
> 
> Yes. I confirmed it with 78ca55889a549a9a194c6ec666836329b774ab6d.
> 

Thanks for confirming.

> b5d7f43e97dabfa04a4be5ff027ce7da119332be deals with the case where it 
> advertises v2 but it doesn't in my case.

In your case it actually goes down the v1 ops path then right?

Basavaraj - is discovery unique to v2?  Or does it also exist for v1?
If it also exists for v1 I think that's a cleaner solution.

> 
> Regards,
> Akihiko Odaki
> 
>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@...il.com>
>>> ---
>>>   drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c 
>>> b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
>>> index dadc491bbf6b..4137e5da77ad 100644
>>> --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
>>> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
>>> @@ -271,6 +271,8 @@ static void mp2_select_ops(struct amd_mp2_dev 
>>> *privdata)
>>>       case V2_STATUS:
>>>           privdata->mp2_ops = &amd_sfh_ops_v2;
>>>           break;
>>> +    case 15:
>>> +        break;
>>>       default:
>>>           privdata->mp2_ops = &amd_sfh_ops;
>>>           break;
>>> @@ -317,6 +319,8 @@ static int amd_mp2_pci_probe(struct pci_dev 
>>> *pdev, const struct pci_device_id *i
>>>           return -ENOMEM;
>>>       mp2_select_ops(privdata);
>>> +    if (!privdata->mp2_ops)
>>> +        return -ENODEV;
>>>       rc = amd_sfh_irq_init(privdata);
>>>       if (rc) {
>>
>