Re: [PATCH] iio: bme680_i2c: Make bme680_acpi_match depend on CONFIG_ACPI

[Andy Shevchenko]

On Wed, May 5, 2021 at 5:11 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> On 5/5/21 3:53 PM, Andy Shevchenko wrote:
> > On Wed, May 5, 2021 at 4:39 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >> On 5/5/21 3:22 PM, Andy Shevchenko wrote:
> >>> On Wed, May 5, 2021 at 11:36 AM Jonathan Cameron
> >>> <Jonathan.Cameron@xxxxxxxxxx> wrote:
> >>>> On Wed, 5 May 2021 09:32:35 +0100
> >>>> Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
> >>>>> On Tue, 4 May 2021 11:00:52 -0700
> >>>>> Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> >>>
> >>> +Cc: Paul (I hope you are related to coreboot somehow and can
> >>> communicate this further), Pavel and Jacek (LED subsystem suffered
> >>> with this as well), Hans, Rafael and linux-acpi@
> >>>
> >>>>> Dropping the ones we are fairly sure are spurious is even better!
> >>>>
> >>>> If I get bored I'll just do a scrub of all the instances of this that
> >>>> you haven't already cleaned up.  It's worth noting that we do
> >>>> know some highly suspicious looking entries are out there in the wild.
> >>>
> >>> I have counted ~60 users of acpi_device_id in IIO. Brief looking at
> >>> the IDs themselves rings an alarm about half of them.
> >>>
> >>> So, here we may have a chicken and egg problem, i.e. somebody has been
> >>> using (or used) fake IDs from Linux kernel in the real products. What
> >>> I can consider as a course of action is the following:
> >>> 1. Clean up (by removing as quickly as possible) the IDs that have no
> >>> proof to be real from the Linux kernel sources (perhaps marked as
> >>> stable material)
> >>> 2. Notify ASWG / UEFI forum about all IDs that abuse ACPI
> >>> specification and are in Linux kernel, so at least we can keep some
> >>> kind of "reserved/do not use" list on the official level (Rafael?)
> >>> 3. Do not accept any IDs without an evidence provided that they are
> >>> being in use in the real products (this should be done on Linux
> >>> maintainer level in all subsystems that accept drivers
> >>
> >> So my 2 cents on this are that we need to be very careful with
> >> removing "bogus" ACPI-ids.
> >>
> >> A couple of examples from a quick check under drivers/iio/accel:
> >>
> >> drivers/iio/accel/bmc150-accel-i2c.c:
> >>
> >> static const struct i2c_device_id bmc150_accel_id[] = {
> >>         {"bmc150_accel",        bmc150},
> >>         {"bmi055_accel",        bmi055},
> >>         {"bma255",              bma255},
> >>         {"bma250e",             bma250e},
> >>         {"bma222",              bma222},
> >>         {"bma222e",             bma222e},
> >>         {"bma280",              bma280},
> >>         {}
> >> };
> >>
> >> static const struct acpi_device_id bmc150_accel_acpi_match[] = {
> >>         {"BSBA0150",    bmc150},
> >>         {"BMC150A",     bmc150},
> >>         {"BMI055A",     bmi055},
> >>         {"BMA0255",     bma255},
> >>         {"BMA250E",     bma250e},
> >>         {"BMA222",      bma222},
> >>         {"BMA222E",     bma222e},
> >>         {"BMA0280",     bma280},
> >>         {"BOSC0200"},
> >>         { },
> >> };
> >>
> >> With the exception of the  "BSBA0150" and "BOSC0200"
> >> ids, these look like they were invented. But at least the
> >> "BMA250E" one is actually being used! The other BMA###?
> >> ones are probably fake, but given that the "BMA250E"
> >> one is actually real ...
> >>
> >> drivers/iio/accel/bmc150-accel-spi.c
> >>
> >> This uses the same set of ACPI ids as bmc150-accel-i2c.c
> >> minus the "BOSC0200" one. I'm not aware if these
> >> being used in spi mode on any x86 devices, but again
> >> I'm not 100% sure ...
> >>
> >> drivers/iio/accel/da280.c
> >>
> >> static const struct acpi_device_id da280_acpi_match[] = {
> >>         {"MIRAACC", da280},
> >>         {},
> >> };
> >> MODULE_DEVICE_TABLE(acpi, da280_acpi_match);
> >>
> >> This looks like a fake-id, but it was actually added
> >> in a separate commit adding ACPI support because the
> >> chip is used with this id on a Linx 820 Windows tablet.
> >>
> >> So figuring out of any ids are real or not is really tricky
> >> and removing them if they are real will lead to regressions.
> >>
> >> So summarizing IMHO we need to be careful and not just
> >> start removing a whole bunch of these...
> >
> > That's all true. However, I have a few hints on how to distinguish
> > them (fake ones):
> > 1. The ID has been added from day 1 with I2C or SPI ID table with just
> > capitalized name
> > 2. If there are a few drivers by the same author and at least one of
> > the contributions has confirmed fake ID
> > 3. The ID is single in the list and mimics the part number (capitalized form)
> > 4. Google/DuckDuckGo/etc searches give no meaningful results
> >
> > Either combination of the above can be a good hint to at least be
> > sceptical that it's being used
> May I suggest for accelerometers to also grep for the id in
> 60-sensors.hwdb from systemd ?  E.g. the BMA250E id can be found
> there.

Right, it's a very good suggestion! It will definitely tell us what
may not be removed even if we don't see any evidence otherwise.

> > So, Hans, as you already noticed, drivers with a long list of IDs or
> > when ID added separately can be considered less fakish, but we really
> > want evidence of the hardware that has it.
>
> If you want to move ahead with pruning some of these please Cc me
> on the patches, then I'll check them against my collection of
> Bay and Cherry Trail DSDTs, which are devices where these sensors
> are often found.

Currently the scope is of
AOS2315
BME0680
STK8312


-- 
With Best Regards,
Andy Shevchenko