| Message ID | 20241018145732.2181309-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1,1/1] iio: gyro: bmg160: Drop most likely fake ACPI IDs | expand |
On Fri, 18 Oct 2024 17:57:32 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > The commits in question do not proove that ACPI IDs exist. > Quite likely it was a cargo cult addition while doing that > for DT-based enumeration. Drop most likely fake ACPI IDs. > > The to be removed IDs has been checked against the following resources: > 1) DuckDuckGo > 2) Google > 3) MS catalog: https://www.catalog.update.microsoft.com/Search.aspx > This gives no useful results in regard to DSDT, moreover, the official > vendor IDs in the registry for Bosh are BSG and BOSC. > I'm nervous about Bosch drivers in the wild given recent report from Justin https://lore.kernel.org/linux-iio/87jzeboi3g.fsf@justinweiss.com/ Justin, I couldn't find the driver you were referring to for the bmi160, is it online somewhere? Also if you have other bosch drivers could you check for these + bmc150 IDs Andy is proposing dropping in: https://lore.kernel.org/linux-iio/20241018145805.2181682-1-andriy.shevchenko@linux.intel.com/ > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/iio/gyro/bmg160_i2c.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/iio/gyro/bmg160_i2c.c b/drivers/iio/gyro/bmg160_i2c.c > index 672d0b720f61..a81814df5205 100644 > --- a/drivers/iio/gyro/bmg160_i2c.c > +++ b/drivers/iio/gyro/bmg160_i2c.c > @@ -39,8 +39,6 @@ static void bmg160_i2c_remove(struct i2c_client *client) > > static const struct acpi_device_id bmg160_acpi_match[] = { > {"BMG0160", 0}, > - {"BMI055B", 0}, > - {"BMI088B", 0}, > {}, > }; >
Jonathan Cameron <jic23@kernel.org> writes: > On Fri, 18 Oct 2024 17:57:32 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> The commits in question do not proove that ACPI IDs exist. >> Quite likely it was a cargo cult addition while doing that >> for DT-based enumeration. Drop most likely fake ACPI IDs. >> >> The to be removed IDs has been checked against the following resources: >> 1) DuckDuckGo >> 2) Google >> 3) MS catalog: https://www.catalog.update.microsoft.com/Search.aspx >> This gives no useful results in regard to DSDT, moreover, the official >> vendor IDs in the registry for Bosh are BSG and BOSC. >> > > I'm nervous about Bosch drivers in the wild given recent report > from Justin > https://lore.kernel.org/linux-iio/87jzeboi3g.fsf@justinweiss.com/ > > Justin, I couldn't find the driver you were referring to for the bmi160, > is it online somewhere? Yes, the BMI160/260 Windows driver is on this page: https://ayaneo.com/support/download It's at the very bottom, under "AYANEO Universal Gyro Driver." GPD also has a copy inside their driver pack under the "Drivers & BIOS" tab here: https://www.gpd.hk/gpdwinminifirmwaredriver, but the download is often at capacity unless you're signed into Google. > Also if you have other bosch drivers could you check for these + bmc150 > IDs Andy is proposing dropping in: > > https://lore.kernel.org/linux-iio/20241018145805.2181682-1-andriy.shevchenko@linux.intel.com/ Unfortunately, the 160/260 driver is the only one I have. I wanted gyro support in Linux for a handheld PC I bought, and it was the IMU that happened to be inside. Justin > >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> --- >> drivers/iio/gyro/bmg160_i2c.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/iio/gyro/bmg160_i2c.c b/drivers/iio/gyro/bmg160_i2c.c >> index 672d0b720f61..a81814df5205 100644 >> --- a/drivers/iio/gyro/bmg160_i2c.c >> +++ b/drivers/iio/gyro/bmg160_i2c.c >> @@ -39,8 +39,6 @@ static void bmg160_i2c_remove(struct i2c_client *client) >> >> static const struct acpi_device_id bmg160_acpi_match[] = { >> {"BMG0160", 0}, >> - {"BMI055B", 0}, >> - {"BMI088B", 0}, >> {}, >> }; >>
Hi, On 18-Oct-24 4:57 PM, Andy Shevchenko wrote: > The commits in question do not proove that ACPI IDs exist. > Quite likely it was a cargo cult addition while doing that > for DT-based enumeration. Drop most likely fake ACPI IDs. > > The to be removed IDs has been checked against the following resources: > 1) DuckDuckGo > 2) Google > 3) MS catalog: https://www.catalog.update.microsoft.com/Search.aspx > This gives no useful results in regard to DSDT, moreover, the official > vendor IDs in the registry for Bosh are BSG and BOSC. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/iio/gyro/bmg160_i2c.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/iio/gyro/bmg160_i2c.c b/drivers/iio/gyro/bmg160_i2c.c > index 672d0b720f61..a81814df5205 100644 > --- a/drivers/iio/gyro/bmg160_i2c.c > +++ b/drivers/iio/gyro/bmg160_i2c.c > @@ -39,8 +39,6 @@ static void bmg160_i2c_remove(struct i2c_client *client) > > static const struct acpi_device_id bmg160_acpi_match[] = { > {"BMG0160", 0}, > - {"BMI055B", 0}, > - {"BMI088B", 0}, > {}, > }; > Doing a grep on my acpidump collection shows that the BMI prefix is used for some Bosch IMU-s. It seems that some of the Bosch ACPI HID prefixes are like this: Bosch Measurement Accel -> BMAxxxx Bosch Measurement Gyro -> BMGxxxx Bosch Measurement IMU -> BMIxxxx In itself these 3 non official vendor prefixis seem to be quite wildly used and such are not an indication that a HID is not in use. But using BMI which is for IMU-s in a non IMU driver is suspect. Note that has Jonathan has pointed out there is a valid BMI0160 HID used by the drivers/iio/imu/bmi160/ code. I also notice a "BMI055A" HID in the bmc150-accel-[i2c|spi].c drivers, which seems equally wrong. It seems that if anything there should be a "BMI055" (no suffix) HID for drivers/iio/imu/bno055/bno055_i2c.c, but we should only add that if we actually encounter it in the wild. TL;DR: I agree with the removal of the "BMI055B" and "BMI088B" ACPI HIDs from this driver, because if these are valid for anything they are valid for the bno055 + some unknown 088 IMU drivers and not for a gyro driver. Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans
On Sat, 19 Oct 2024 12:33:55 +0200 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 18-Oct-24 4:57 PM, Andy Shevchenko wrote: > > The commits in question do not proove that ACPI IDs exist. > > Quite likely it was a cargo cult addition while doing that > > for DT-based enumeration. Drop most likely fake ACPI IDs. > > > > The to be removed IDs has been checked against the following resources: > > 1) DuckDuckGo > > 2) Google > > 3) MS catalog: https://www.catalog.update.microsoft.com/Search.aspx > > This gives no useful results in regard to DSDT, moreover, the official > > vendor IDs in the registry for Bosh are BSG and BOSC. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > drivers/iio/gyro/bmg160_i2c.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/iio/gyro/bmg160_i2c.c b/drivers/iio/gyro/bmg160_i2c.c > > index 672d0b720f61..a81814df5205 100644 > > --- a/drivers/iio/gyro/bmg160_i2c.c > > +++ b/drivers/iio/gyro/bmg160_i2c.c > > @@ -39,8 +39,6 @@ static void bmg160_i2c_remove(struct i2c_client *client) > > > > static const struct acpi_device_id bmg160_acpi_match[] = { > > {"BMG0160", 0}, > > - {"BMI055B", 0}, > > - {"BMI088B", 0}, > > {}, > > }; > > > > Doing a grep on my acpidump collection shows that the BMI prefix is used > for some Bosch IMU-s. It seems that some of the Bosch ACPI HID > prefixes are like this: > > Bosch Measurement Accel -> BMAxxxx > Bosch Measurement Gyro -> BMGxxxx > Bosch Measurement IMU -> BMIxxxx > > In itself these 3 non official vendor prefixis seem to be quite > wildly used and such are not an indication that a HID is not in use. > > But using BMI which is for IMU-s in a non IMU driver is suspect. > Note that has Jonathan has pointed out there is a valid BMI0160 > HID used by the drivers/iio/imu/bmi160/ code. > > I also notice a "BMI055A" HID in the bmc150-accel-[i2c|spi].c drivers, > which seems equally wrong. > > It seems that if anything there should be a "BMI055" (no suffix) HID > for drivers/iio/imu/bno055/bno055_i2c.c, but we should only add that > if we actually encounter it in the wild. > > TL;DR: > > I agree with the removal of the "BMI055B" and "BMI088B" ACPI HIDs > from this driver, because if these are valid for anything they > are valid for the bno055 + some unknown 088 IMU drivers and not > for a gyro driver. Not so simple. The BMI055 is a multi sensor package. So two MEMS devices shoved in the same chip package. Bus is shared, but separate chip selects and entirely unrelated register sets. We handle these in DT via _gyro / _accel postfixes on the compatible. These used to be common in IMUs though it seems fully integrated ones have replaced them. https://lore.kernel.org/all/1409655597-28781-1-git-send-email-irina.tirdea@intel.com/ The A/B thing makes me particularly suspicious as that's a novel invention for something not in the wild. Still, if we have no evidence this one is actually used (and it's an ancient device so I doubt we'll get new users!) I'm not against removing the IDs and see if anyone shouts. On that basis I'll queue this up. Jonathan > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > Regards, > > Hans > >
On Fri, Oct 18, 2024 at 01:27:57PM -0700, Justin Weiss wrote: > Jonathan Cameron <jic23@kernel.org> writes: > > > On Fri, 18 Oct 2024 17:57:32 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > >> The commits in question do not proove that ACPI IDs exist. > >> Quite likely it was a cargo cult addition while doing that > >> for DT-based enumeration. Drop most likely fake ACPI IDs. > >> > >> The to be removed IDs has been checked against the following resources: > >> 1) DuckDuckGo > >> 2) Google > >> 3) MS catalog: https://www.catalog.update.microsoft.com/Search.aspx > >> This gives no useful results in regard to DSDT, moreover, the official > >> vendor IDs in the registry for Bosh are BSG and BOSC. > >> > > > > I'm nervous about Bosch drivers in the wild given recent report > > from Justin > > https://lore.kernel.org/linux-iio/87jzeboi3g.fsf@justinweiss.com/ I know about Realtek case, but for BMI0/BMI it's interesting that MS catalog shows hits on BMI160 (note no leading zero!), but not on BMI0160. > > Justin, I couldn't find the driver you were referring to for the bmi160, > > is it online somewhere? > > Yes, the BMI160/260 Windows driver is on this page: > https://ayaneo.com/support/download > > It's at the very bottom, under "AYANEO Universal Gyro Driver." GPD also > has a copy inside their driver pack under the "Drivers & BIOS" tab here: > https://www.gpd.hk/gpdwinminifirmwaredriver, but the download is often >> at capacity unless you're signed into Google. I was not Cc'ed on ACPI ID parts, please make sure that the patch that adds it has a pointer to the actual device which uses it _and_ DSDT excerpt of the respective Device object. Without that info provided, NAK from me. > > Also if you have other bosch drivers could you check for these + bmc150 > > IDs Andy is proposing dropping in: > > > > https://lore.kernel.org/linux-iio/20241018145805.2181682-1-andriy.shevchenko@linux.intel.com/ > > Unfortunately, the 160/260 driver is the only one I have. I wanted gyro > support in Linux for a handheld PC I bought, and it was the IMU that > happened to be inside.
On Mon, 21 Oct 2024 09:47:16 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, Oct 18, 2024 at 01:27:57PM -0700, Justin Weiss wrote: > > Jonathan Cameron <jic23@kernel.org> writes: > > > > > On Fri, 18 Oct 2024 17:57:32 +0300 > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > >> The commits in question do not proove that ACPI IDs exist. > > >> Quite likely it was a cargo cult addition while doing that > > >> for DT-based enumeration. Drop most likely fake ACPI IDs. > > >> > > >> The to be removed IDs has been checked against the following resources: > > >> 1) DuckDuckGo > > >> 2) Google > > >> 3) MS catalog: https://www.catalog.update.microsoft.com/Search.aspx > > >> This gives no useful results in regard to DSDT, moreover, the official > > >> vendor IDs in the registry for Bosh are BSG and BOSC. > > >> > > > > > > I'm nervous about Bosch drivers in the wild given recent report > > > from Justin > > > https://lore.kernel.org/linux-iio/87jzeboi3g.fsf@justinweiss.com/ > > I know about Realtek case, but for BMI0/BMI it's interesting that MS > catalog shows hits on BMI160 (note no leading zero!), but not on BMI0160. > > > > Justin, I couldn't find the driver you were referring to for the bmi160, > > > is it online somewhere? > > > > Yes, the BMI160/260 Windows driver is on this page: > > https://ayaneo.com/support/download > > > > It's at the very bottom, under "AYANEO Universal Gyro Driver." GPD also > > has a copy inside their driver pack under the "Drivers & BIOS" tab here: > > https://www.gpd.hk/gpdwinminifirmwaredriver, but the download is often > >> at capacity unless you're signed into Google. > > I was not Cc'ed on ACPI ID parts, please make sure that the patch > that adds it has a pointer to the actual device which uses it _and_ > DSDT excerpt of the respective Device object. Without that info provided, > NAK from me. Device we have in the patch comments. Justin if you can supply the DSDT excerpts in reply to that patch that would be great. I'll try and remember to check you are on the CC next time Andy. Jonathan > > > > Also if you have other bosch drivers could you check for these + bmc150 > > > IDs Andy is proposing dropping in: > > > > > > https://lore.kernel.org/linux-iio/20241018145805.2181682-1-andriy.shevchenko@linux.intel.com/ > > > > Unfortunately, the 160/260 driver is the only one I have. I wanted gyro > > support in Linux for a handheld PC I bought, and it was the IMU that > > happened to be inside. >
diff --git a/drivers/iio/gyro/bmg160_i2c.c b/drivers/iio/gyro/bmg160_i2c.c index 672d0b720f61..a81814df5205 100644 --- a/drivers/iio/gyro/bmg160_i2c.c +++ b/drivers/iio/gyro/bmg160_i2c.c @@ -39,8 +39,6 @@ static void bmg160_i2c_remove(struct i2c_client *client) static const struct acpi_device_id bmg160_acpi_match[] = { {"BMG0160", 0}, - {"BMI055B", 0}, - {"BMI088B", 0}, {}, };
The commits in question do not proove that ACPI IDs exist. Quite likely it was a cargo cult addition while doing that for DT-based enumeration. Drop most likely fake ACPI IDs. The to be removed IDs has been checked against the following resources: 1) DuckDuckGo 2) Google 3) MS catalog: https://www.catalog.update.microsoft.com/Search.aspx This gives no useful results in regard to DSDT, moreover, the official vendor IDs in the registry for Bosh are BSG and BOSC. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/iio/gyro/bmg160_i2c.c | 2 -- 1 file changed, 2 deletions(-)