linux-kernel - Re: [PATCH 1/1] Add 10EC5280 to bmi160_i2c ACPI IDs to allow binding on some devices

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: <20240205100208.00007a50@Huawei.com>
Date: Mon, 5 Feb 2024 10:02:08 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Jesus Miguel Gonzalez Herrero <jesusmgh@...il.com>
CC: Jonathan Cameron <jic23@...nel.org>, <lars@...afoo.de>,
	<linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Andy Shevchenko
	<andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH 1/1] Add 10EC5280 to bmi160_i2c ACPI IDs to allow
 binding on some devices

On Sun, 4 Feb 2024 18:53:42 +0100
Jesus Miguel Gonzalez Herrero <jesusmgh@...il.com> wrote:

> Hello Mr. Cameron
> 
> First of all thank you for reviewing the patch.
> 
> And I most definitely agree with you and Mr. Shevchenko: this absolutely
> is a firmware bug that manufacturers should fix. For this reason some
> people started talks with affected manufacturers to change it. In my
> case it was with GPD, together with some others, including some which
> historically had a more direct line with them. This was finally dismissed
> as WONTFIX, since their main focus is Windows and their driver supports
> the ID, so the end result of those conversations is a lack of a fixed
> firmware, and a surplus of frustration.
> 
> As far as I know people have been in talks with Aya too, and I do not
> know the status of conversations with Lenovo or other manufacturers. I
> do not know of any conversation with Realtek, besides what was mentioned
> in those emails you linked to from 2021.
> 
> I will amend the patch to include a big disclaimer and the reason as
> a comment in the code, and send it again in reply to this message. I
> don't think I'd go as far as tainting the kernel, but I'm not opposed,
> happy anyway if the IMU finally becomes usable, and VERY far from any
> expertise whatsoever concerning kernel development!
> 
> Here is the relevant extract from the DSDT of my GPD Win Max 2 (AMD
> 6800U model) with the latest firmware 1.05 installed.

Great - This is exactly the info that should support such a patch like this.

Include the blob below and a statement that GPD have refused to fix due
to the windows driver in the patch description and I'm fine with taking this.

I may see if I can dig out a Lenovo contact as they are big enough and
have been around long enough to know better... (big company mess however
I'm sure...)

Jonathan


> 
>      Scope (_SB.I2CC) {
>          Device (BMA2) {
>              Name (_ADR, Zero)  // _ADR: Address Name (_HID, "10EC5280")
>              // _HID: Hardware ID Name (_CID, "10EC5280")  // _CID:
>              Compatible ID Name (_DDN, "Accelerometer")  // _DDN: DOS
>              Device Name Name (_UID, One)  // _UID: Unique ID Method
>              (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings {
>                  Name (RBUF, ResourceTemplate () {
>                      I2cSerialBusV2 (0x0069, ControllerInitiated,
>                      0x00061A80,
>                          AddressingMode7Bit, "\\_SB.I2CC", 0x00,
>                          ResourceConsumer, , Exclusive, )
>                  }) Return (RBUF) /* \_SB_.I2CC.BMA2._CRS.RBUF */
>              }
> 
>              OperationRegion (CMS2, SystemIO, 0x72, 0x02) Field (CMS2,
>              ByteAcc, NoLock, Preserve) {
>                  IND2,   8, DAT2,   8
>              }
> 
>              IndexField (IND2, DAT2, ByteAcc, NoLock, Preserve) {
>                  Offset (0x74), BACS,   32
>              }
> 
>              Method (ROMS, 0, NotSerialized) {
>                  Name (RBUF, Package (0x03) {
>                      "0 -1 0", "-1 0 0", "0 0 1"
>                  }) Return (RBUF) /* \_SB_.I2CC.BMA2.ROMS.RBUF */
>              }
> 
>              Method (CALS, 1, NotSerialized) {
>                  Local0 = Arg0 If (((Local0 == Zero) || (Local0 ==
>                  Ones))) {
>                      Return (Local0)
>                  } Else {
>                      BACS = Local0
>                  }
>              }
> 
>              Method (_STA, 0, NotSerialized)  // _STA: Status {
>                  Return (0x0F)
>              }
>          }
>      }
> 
> Thank you for taking this into consideration!
> 
> Jesus Gonzalez
> 
> 
> 
> On 04/02/2024 15:00, Jonathan Cameron wrote:
> > On Fri,  2 Feb 2024 18:30:41 +0100
> > Jesus Gonzalez <jesusmgh@...il.com> wrote:
> >  
> >> "10EC5280" is used by several manufacturers like Lenovo, GPD, or AYA (and
> >> probably others) in their ACPI table as the ID for the bmi160 IMU. This
> >> means the bmi160_i2c driver won't bind to it, and the IMU is unavailable
> >> to the user. Manufacturers have been approached on several occasions to
> >> try getting a BIOS with a fixed ID, mostly without actual positive
> >> results, and since affected devices are already a few years old, this is
> >> not expected to change. This patch enables using the bmi160_i2c driver for
> >> the bmi160 IMU on these devices.  
> > Hi Jesus,
> >
> > https://lore.kernel.org/lkml/CAHp75Vct-AXnU7QQmdE7nyYZT-=n=p67COPLiiZTet7z7snL-g@mail.gmail.com/
> > Lays out what Andy (and for that matter I) consider necessary for such
> > a patch.
> >
> > In short, we want to see devices called out here - with a DSDT section.
> > + a clear comment in the code.
> >
> > The big problem here is this tramples on Realtech's ID space. It's not just
> > a made up code (incidentally the BMI0160 isn't valid either),
> > it's a valid code but for an entirely different (PCI) device.
> >
> > So we need as much info as possible in the patch description and the driver
> > itself to justify carrying this.   Tempting to add a firmware bug taint on
> > it as well but that might scare people :)
> >
> > Jonathan
> >
> >  
> >> Signed-off-by: Jesus Gonzalez <jesusmgh@...il.com>
> >> ---
> >> A device-specific transformation matrix can then be provided in a second
> >> step through udev hwdb.
> >>
> >> This has been discussed before in 2021, see here:
> >> https://lore.kernel.org/lkml/CACAwPwYQHRcrabw9=0tvenPzAcwwW1pTaR6a+AEWBF9Hqf_wXQ@mail.gmail.com/
> >>
> >> Lenovo, as an example of a big manufacturer, is also using this ID:
> >> https://www.reddit.com/r/linux/comments/r6f9de/comment/hr8bdfs/?context=3
> >>
> >> At least some discussions with GPD took place on the GPD server Discord,
> >> for which I can provide proof on demand via screenshot (if not accessible
> >> directly).
> >>
> >> I have read the patch submission instructions and followed them to the
> >> best of my knowledge. Still, this is my first kernel patch submission,
> >> so I'd be glad if you could please point out any mistakes. Thank you!
> >>
> >>
> >>   drivers/iio/imu/bmi160/bmi160_spi.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/iio/imu/bmi160/bmi160_spi.c b/drivers/iio/imu/bmi160/bmi160_spi.c
> >> index 8b573ea99af2..0874c37c6670 100644
> >> --- a/drivers/iio/imu/bmi160/bmi160_spi.c
> >> +++ b/drivers/iio/imu/bmi160/bmi160_spi.c
> >> @@ -41,6 +41,7 @@ MODULE_DEVICE_TABLE(spi, bmi160_spi_id);
> >>   
> >>   static const struct acpi_device_id bmi160_acpi_match[] = {
> >>   	{"BMI0160", 0},
> >> +	{"10EC5280", 0},
> >>   	{ },
> >>   };
> >>   MODULE_DEVICE_TABLE(acpi, bmi160_acpi_match);  
>