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

[Hans de Goede]

Hi,

On 5/5/21 4:18 PM, Andy Shevchenko wrote:
> 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

Ok I cannot find any reference to those in the DSDT-s which I have,
nor in systemd's hwdb.

Regards,

Hans