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

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

[Guenter Roeck]

With CONFIG_ACPI=n and -Werror, 0-day reports:

drivers/iio/chemical/bme680_i2c.c:46:36: error:
	'bme680_acpi_match' defined but not used

Reported-by: kernel test robot <lkp@intel.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
Given the other patch, question of course is if this ACPI ID
is real. A Google search suggests that this might not be the case.
Should we remove it as well ? STK8312 has the same problem.

Jonathan, I think this needs your input before I send more patches.

 drivers/iio/chemical/bme680_i2c.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/chemical/bme680_i2c.c b/drivers/iio/chemical/bme680_i2c.c
index 29c0dfa4702b..b5e75f145c19 100644
--- a/drivers/iio/chemical/bme680_i2c.c
+++ b/drivers/iio/chemical/bme680_i2c.c
@@ -42,11 +42,13 @@ static const struct i2c_device_id bme680_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, bme680_i2c_id);
 
+#ifdef CONFIG_ACPI
 static const struct acpi_device_id bme680_acpi_match[] = {
 	{"BME0680", 0},
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, bme680_acpi_match);
+#endif
 
 static const struct of_device_id bme680_of_i2c_match[] = {
 	{ .compatible = "bosch,bme680", },
-- 
2.25.1

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

[Andy Shevchenko]

On Tue, May 4, 2021 at 8:40 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> With CONFIG_ACPI=n and -Werror, 0-day reports:
>
> drivers/iio/chemical/bme680_i2c.c:46:36: error:
>         'bme680_acpi_match' defined but not used

> Given the other patch, question of course is if this ACPI ID
> is real. A Google search suggests that this might not be the case.
> Should we remove it as well ? STK8312 has the same problem.

For this one definitely removal. Looking into the code it suggests a
cargo cult taken that time by a few contributors to invent fake ACPI
IDs while submitting new drivers.
Feel free to add my tag or if you wish me I'll add it explicitly.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, May 4, 2021 at 8:44 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, May 4, 2021 at 8:40 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > With CONFIG_ACPI=n and -Werror, 0-day reports:
> >
> > drivers/iio/chemical/bme680_i2c.c:46:36: error:
> >         'bme680_acpi_match' defined but not used
>
> > Given the other patch, question of course is if this ACPI ID
> > is real. A Google search suggests that this might not be the case.
> > Should we remove it as well ? STK8312 has the same problem.
>
> For this one definitely removal. Looking into the code it suggests a
> cargo cult taken that time by a few contributors to invent fake ACPI
> IDs while submitting new drivers.
> Feel free to add my tag or if you wish me I'll add it explicitly.

Had just looked at STK, the same guy as for AOS2315. So, also to remove.

-- 
With Best Regards,
Andy Shevchenko

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

[Guenter Roeck]

On 5/4/21 10:44 AM, Andy Shevchenko wrote:
> On Tue, May 4, 2021 at 8:40 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> With CONFIG_ACPI=n and -Werror, 0-day reports:
>>
>> drivers/iio/chemical/bme680_i2c.c:46:36: error:
>>         'bme680_acpi_match' defined but not used
> 
>> Given the other patch, question of course is if this ACPI ID
>> is real. A Google search suggests that this might not be the case.
>> Should we remove it as well ? STK8312 has the same problem.
> 
> For this one definitely removal. Looking into the code it suggests a
> cargo cult taken that time by a few contributors to invent fake ACPI
> IDs while submitting new drivers.
> Feel free to add my tag or if you wish me I'll add it explicitly.
> 

I'll resend and let you add the tag, and send a similar patch
for STK8312. I'll wait until tomorrow, though - I sent a number of
patches today already, and I want to avoid yet another "account
suspended" notice from gmail.

Thanks,
Guenter

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

[Jonathan Cameron]

On Tue, 4 May 2021 11:00:52 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 5/4/21 10:44 AM, Andy Shevchenko wrote:
> > On Tue, May 4, 2021 at 8:40 PM Guenter Roeck <linux@roeck-us.net> wrote:  
> >>
> >> With CONFIG_ACPI=n and -Werror, 0-day reports:
> >>
> >> drivers/iio/chemical/bme680_i2c.c:46:36: error:
> >>         'bme680_acpi_match' defined but not used  
> >   
> >> Given the other patch, question of course is if this ACPI ID
> >> is real. A Google search suggests that this might not be the case.
> >> Should we remove it as well ? STK8312 has the same problem.  
> > 
> > For this one definitely removal. Looking into the code it suggests a
> > cargo cult taken that time by a few contributors to invent fake ACPI
> > IDs while submitting new drivers.
> > Feel free to add my tag or if you wish me I'll add it explicitly.
> >   
> 
> I'll resend and let you add the tag, and send a similar patch
> for STK8312. I'll wait until tomorrow, though - I sent a number of
> patches today already, and I want to avoid yet another "account
> suspended" notice from gmail.

If you find some valid ACPI entries that are hitting this problem,
I'd prefer we just got rid of the ACPI_PTR() usecases rather than
added IFDEF magic.

The space wasted by having these is trivial and I'd rather not
introduce ifdef around any of these tables.

Dropping the ones we are fairly sure are spurious is even better!

Thanks,

Jonathan

> 
> Thanks,
> Guenter

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

[Jonathan Cameron]

On Wed, 5 May 2021 09:32:35 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Tue, 4 May 2021 11:00:52 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > On 5/4/21 10:44 AM, Andy Shevchenko wrote:  
> > > On Tue, May 4, 2021 at 8:40 PM Guenter Roeck <linux@roeck-us.net> wrote:    
> > >>
> > >> With CONFIG_ACPI=n and -Werror, 0-day reports:
> > >>
> > >> drivers/iio/chemical/bme680_i2c.c:46:36: error:
> > >>         'bme680_acpi_match' defined but not used    
> > >     
> > >> Given the other patch, question of course is if this ACPI ID
> > >> is real. A Google search suggests that this might not be the case.
> > >> Should we remove it as well ? STK8312 has the same problem.    
> > > 
> > > For this one definitely removal. Looking into the code it suggests a
> > > cargo cult taken that time by a few contributors to invent fake ACPI
> > > IDs while submitting new drivers.
> > > Feel free to add my tag or if you wish me I'll add it explicitly.
> > >     
> > 
> > I'll resend and let you add the tag, and send a similar patch
> > for STK8312. I'll wait until tomorrow, though - I sent a number of
> > patches today already, and I want to avoid yet another "account
> > suspended" notice from gmail.  
> 
> If you find some valid ACPI entries that are hitting this problem,
> I'd prefer we just got rid of the ACPI_PTR() usecases rather than
> added IFDEF magic.
> 
> The space wasted by having these is trivial and I'd rather not
> introduce ifdef around any of these tables.
> 
> 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. 

Thanks,

Jonathan

> 
> Thanks,
> 
> Jonathan
> 
> > 
> > Thanks,
> > Guenter  
> 

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

[Guenter Roeck]

On Wed, May 05, 2021 at 09:34:38AM +0100, Jonathan Cameron wrote:
> On Wed, 5 May 2021 09:32:35 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Tue, 4 May 2021 11:00:52 -0700
> > Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> > > On 5/4/21 10:44 AM, Andy Shevchenko wrote:  
> > > > On Tue, May 4, 2021 at 8:40 PM Guenter Roeck <linux@roeck-us.net> wrote:    
> > > >>
> > > >> With CONFIG_ACPI=n and -Werror, 0-day reports:
> > > >>
> > > >> drivers/iio/chemical/bme680_i2c.c:46:36: error:
> > > >>         'bme680_acpi_match' defined but not used    
> > > >     
> > > >> Given the other patch, question of course is if this ACPI ID
> > > >> is real. A Google search suggests that this might not be the case.
> > > >> Should we remove it as well ? STK8312 has the same problem.    
> > > > 
> > > > For this one definitely removal. Looking into the code it suggests a
> > > > cargo cult taken that time by a few contributors to invent fake ACPI
> > > > IDs while submitting new drivers.
> > > > Feel free to add my tag or if you wish me I'll add it explicitly.
> > > >     
> > > 
> > > I'll resend and let you add the tag, and send a similar patch
> > > for STK8312. I'll wait until tomorrow, though - I sent a number of
> > > patches today already, and I want to avoid yet another "account
> > > suspended" notice from gmail.  
> > 
> > If you find some valid ACPI entries that are hitting this problem,
> > I'd prefer we just got rid of the ACPI_PTR() usecases rather than
> > added IFDEF magic.
> > 
> > The space wasted by having these is trivial and I'd rather not
> > introduce ifdef around any of these tables.
> > 
> > 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. 
> 

The ones reported by 0-day are AOS2315, BME0680, and STK8312.
You just accepted a patch for -next which claims that all users of
STK8312 would be ACPI, yet Andy says that STK8312 isn't a valid ACPI ID.

Not really sure from the above if I should send patches to remove
acpi support from those drivers or if you want to handle that yourself.

Other drivers such as FXOS8700 should also generate a warning with
CONFIG_ACPI=n but for some reason don't. I have not tried to track down
the reason.

Anyway, for an "outsider" it seems all but impossible to determine if
an ACPI ID is real or made up. How does one know ?

Note that I didn't actually get any of your e-mails (nor, I notice, several
other e-mails I was copied on). I picked this one up from the linux-kernel
mailing list. My apologies if I don't reply to any of your e-mails;
that would be the reason.

Guenter

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

[Andy Shevchenko]

On Wed, May 5, 2021 at 11:36 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
> On Wed, 5 May 2021 09:32:35 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > On Tue, 4 May 2021 11:00:52 -0700
> > Guenter Roeck <linux@roeck-us.net> 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


-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 5/5/21 3:22 PM, Andy Shevchenko wrote:
> On Wed, May 5, 2021 at 11:36 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
>> On Wed, 5 May 2021 09:32:35 +0100
>> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>>> On Tue, 4 May 2021 11:00:52 -0700
>>> Guenter Roeck <linux@roeck-us.net> 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...

Regards,

Hans

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

[Andy Shevchenko]

On Wed, May 5, 2021 at 4:39 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 5/5/21 3:22 PM, Andy Shevchenko wrote:
> > On Wed, May 5, 2021 at 11:36 AM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:
> >> On Wed, 5 May 2021 09:32:35 +0100
> >> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >>> On Tue, 4 May 2021 11:00:52 -0700
> >>> Guenter Roeck <linux@roeck-us.net> 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.

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.

Some big companies sometimes produce fake IDs themselves (and
sometimes for the components they are not even producing) without
following the process (as described on uefi.org) which is a pity part
of the story.

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 5/5/21 3:53 PM, Andy Shevchenko wrote:
> On Wed, May 5, 2021 at 4:39 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 5/5/21 3:22 PM, Andy Shevchenko wrote:
>>> On Wed, May 5, 2021 at 11:36 AM Jonathan Cameron
>>> <Jonathan.Cameron@huawei.com> wrote:
>>>> On Wed, 5 May 2021 09:32:35 +0100
>>>> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>>>>> On Tue, 4 May 2021 11:00:52 -0700
>>>>> Guenter Roeck <linux@roeck-us.net> 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. 

> 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.

Regards,

Hans

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@redhat.com> wrote:
> On 5/5/21 3:53 PM, Andy Shevchenko wrote:
> > On Wed, May 5, 2021 at 4:39 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 5/5/21 3:22 PM, Andy Shevchenko wrote:
> >>> On Wed, May 5, 2021 at 11:36 AM Jonathan Cameron
> >>> <Jonathan.Cameron@huawei.com> wrote:
> >>>> On Wed, 5 May 2021 09:32:35 +0100
> >>>> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >>>>> On Tue, 4 May 2021 11:00:52 -0700
> >>>>> Guenter Roeck <linux@roeck-us.net> 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

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@redhat.com> wrote:
>> On 5/5/21 3:53 PM, Andy Shevchenko wrote:
>>> On Wed, May 5, 2021 at 4:39 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> On 5/5/21 3:22 PM, Andy Shevchenko wrote:
>>>>> On Wed, May 5, 2021 at 11:36 AM Jonathan Cameron
>>>>> <Jonathan.Cameron@huawei.com> wrote:
>>>>>> On Wed, 5 May 2021 09:32:35 +0100
>>>>>> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>>>>>>> On Tue, 4 May 2021 11:00:52 -0700
>>>>>>> Guenter Roeck <linux@roeck-us.net> 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

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

[Andy Shevchenko]

On Wed, May 5, 2021 at 6:19 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 5/5/21 4:18 PM, Andy Shevchenko wrote:
> > On Wed, May 5, 2021 at 5:11 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> > 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.

Thanks for checking.

By the way. Do you have DSDTs in some kind of Git repository?
Shouldn't we have (means to create if not exists) one publicly
available? It would be nice to gather them under the hood for the
purpose of looking into. I have some (small) set of those as well
which might be useful.


-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 5/5/21 6:26 PM, Andy Shevchenko wrote:
> On Wed, May 5, 2021 at 6:19 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 5/5/21 4:18 PM, Andy Shevchenko wrote:
>>> On Wed, May 5, 2021 at 5:11 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> ...
> 
>>> 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.
> 
> Thanks for checking.
> 
> By the way. Do you have DSDTs in some kind of Git repository?
> Shouldn't we have (means to create if not exists) one publicly
> available? It would be nice to gather them under the hood for the
> purpose of looking into. I have some (small) set of those as well
> which might be useful.

Interesting idea, the issue is that arguably these are copyrightable
and we don't have any permission to distribute them. Things like
attaching them to bugs, having them emailed to me, etc. can be
considered fair use of sorts I guess (IANAL), but putting them into
a git repo would really be a bit of a grey area...

Regards,

Hans

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

Hi!
On Wed 2021-05-05 16:22:07, Andy Shevchenko wrote:
> On Wed, May 5, 2021 at 11:36 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> > On Wed, 5 May 2021 09:32:35 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > > On Tue, 4 May 2021 11:00:52 -0700
> > > Guenter Roeck <linux@roeck-us.net> 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@

Thanks for Cc. I prefer @ucw.cz address for the LED work.

> > > 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.

As far as I can tell, this means asking "is this real ID or did you
just invent it" at patch submission. Okay...

Best regards,
								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Andy Shevchenko]

On Fri, May 7, 2021 at 2:53 PM Pavel Machek <pavel@denx.de> wrote:
> On Wed 2021-05-05 16:22:07, Andy Shevchenko wrote:
> > On Wed, May 5, 2021 at 11:36 AM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:
> > > On Wed, 5 May 2021 09:32:35 +0100
> > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > > > On Tue, 4 May 2021 11:00:52 -0700
> > > > Guenter Roeck <linux@roeck-us.net> 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@
>
> Thanks for Cc. I prefer @ucw.cz address for the LED work.

Noted!

> > > > 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.
>
> As far as I can tell, this means asking "is this real ID or did you
> just invent it" at patch submission. Okay...

I would put it in a way that "Please, provide an ASL excerpt / ACPI
tables dump" or something alike. Because it may also show some
additional information that would make sense to consider when adding
an ID to the driver.


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, May 5, 2021 at 11:34 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
> On Tue, 4 May 2021 11:00:52 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> > On 5/4/21 10:44 AM, Andy Shevchenko wrote:
> > > On Tue, May 4, 2021 at 8:40 PM Guenter Roeck <linux@roeck-us.net> wrote:

> > I'll resend and let you add the tag, and send a similar patch
> > for STK8312. I'll wait until tomorrow, though - I sent a number of
> > patches today already, and I want to avoid yet another "account
> > suspended" notice from gmail.
>
> If you find some valid ACPI entries that are hitting this problem,
> I'd prefer we just got rid of the ACPI_PTR() usecases rather than
> added IFDEF magic.

Agree,

> The space wasted by having these is trivial and I'd rather not
> introduce ifdef around any of these tables.

> Dropping the ones we are fairly sure are spurious is even better!

For the record, I have checked all three Guenter pointed out and to me
all of them sounds like fake (two from the same author). So, I can
deduce that if we have same author for a few looking very suspicious
ACPI IDs, they are quite likely fake and must be removed sooner than
later.

-- 
With Best Regards,
Andy Shevchenko