| Message ID | 20220626081339.7243-1-akihiko.odaki@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: amd_sfh: Ignore uninitialized device | expand |
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. > Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.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) {
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. b5d7f43e97dabfa04a4be5ff027ce7da119332be deals with the case where it advertises v2 but it doesn't in my case. Regards, Akihiko Odaki > >> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.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) { >
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@gmail.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) { >> >
On 2022/06/29 0:14, Limonciello, Mario wrote: > 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? Yes, but I doubt even that is correct in this case. I guess the v1 protocol would have a value 1 for acs in mp2_select_ops(), but it is 15 in this case. It would be nice if you confirm that hypothesis. Regards, Akihiko Odaki > > 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@gmail.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) { >>> >> >
[Public] > -----Original Message----- > From: Akihiko Odaki <akihiko.odaki@gmail.com> > Sent: Tuesday, June 28, 2022 10:21 > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>; Jiri Kosina > <jikos@kernel.org>; Benjamin Tissoires <benjamin.tissoires@redhat.com>; > linux-input@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] HID: amd_sfh: Ignore uninitialized device > > On 2022/06/29 0:14, Limonciello, Mario wrote: > > 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? > > Yes, but I doubt even that is correct in this case. I guess the v1 > protocol would have a value 1 for acs in mp2_select_ops(), but it is 15 > in this case. It would be nice if you confirm that hypothesis. I had some offline conversation about this, and it's probably best to include a way to block amd_sfh from loading on the DMI of Chromebooks. What you're finding in these registers is garbage without firmware loaded and isn't deterministic. Could you come up with a patch that blocks from the DMI data? I don't think it should be just your specific chromebook, but maybe one of the strings that indicates "any chromebook" and has a "Google Inc" or something in it. > > Regards, > Akihiko Odaki > > > > > 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@gmail.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) { > >>> > >> > >
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) {
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. Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com> --- drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 4 ++++ 1 file changed, 4 insertions(+)