Message-ID: <20250614140330.31183fd5@jic23-huawei>
Date: Sat, 14 Jun 2025 14:03:30 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Marek Vasut <marek.vasut@...lbox.org>
Cc: Andy Shevchenko <andriy.shevchenko@...el.com>, Marek Vasut
 <marek.vasut+bmc150@...lbox.org>, Hans de Goede <hansg@...nel.org>,
 linux-iio@...r.kernel.org, Nuno Sá <nuno.sa@...log.com>, Andy
 Shevchenko <andy@...nel.org>, David Lechner <dlechner@...libre.com>, Julien
 Stephan <jstephan@...libre.com>, Peter Zijlstra <peterz@...radead.org>,
 Salvatore Bonaccorso <carnil@...ian.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: accel: bmc150: Do not configure IRQ registers if
 no IRQ connected

On Fri, 13 Jun 2025 19:02:28 +0200
Marek Vasut <marek.vasut@...lbox.org> wrote:

> On 6/13/25 5:09 PM, Andy Shevchenko wrote:
> > Strange I don't see Hans in the Cc list, so added.
> > Thanks for the report and patch, my comments below.
> > 
> > On Fri, Jun 13, 2025 at 02:45:22PM +0200, Marek Vasut wrote:  
> >> The BMC150 on Onemix 2S does not have IRQ line described in ACPI tables,
> >> which leads to bmc150_accel_core_probe() being called with irq=0, which
> >> leads to bmc150_accel_interrupts_setup() never being called, which leads
> >> to struct bmc150_accel_data *data ->interrupts[i].info being left unset
> >> to NULL. Later, userspace can indirectly trigger bmc150_accel_set_interrupt()
> >> which depends on struct bmc150_accel_data *data ->interrupts[i].info being
> >> non-NULL, and which triggers NULL pointer dereference. This is triggered
> >> e.g. from iio-sensor-proxy.
> >>
> >> Fix this by skipping the IRQ register configuration in case there is no
> >> IRQ connected in hardware, in a manner similar to what the driver did in
> >> the very first commit which added the driver.
> >>
> >> ACPI table dump:  
> >   
> >>          Device (BMA2)
> >>          {
> >>              Name (_ADR, Zero)  // _ADR: Address
> >>              Name (_HID, "BOSC0200")  // _HID: Hardware ID
> >>              Name (_CID, "BOSC0200")  // _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 (0x0019, ControllerInitiated, 0x00061A80,
> >>                          AddressingMode7Bit, "\\_SB.PCI0.I2C0",
> >>                          0x00, ResourceConsumer, , Exclusive,
> >>                          )
> >>                  })
> >>                  Return (RBUF) /* \_SB_.PCI0.I2C0.BMA2._CRS.RBUF */
> >>              }  
> > 
> > These lines...
> >   
> >>              Method (ROTM, 0, NotSerialized)
> >>              {
> >>                  Name (SBUF, Package (0x03)
> >>                  {
> >>                      "0 1 0",
> >>                      "1 0 0 ",
> >>                      "0 0 1"
> >>                  })
> >>                  Return (SBUF) /* \_SB_.PCI0.I2C0.BMA2.ROTM.SBUF */
> >>              }
> >>
> >>              Method (_STA, 0, NotSerialized)  // _STA: Status
> >>              {
> >>                  Return (0x0F)
> >>              }  
> > 
> > ...are irrelevant.
> >   
> >>          }
> >> "
> >>
> >> Splat, collected from debian unstable, probably not very useful:  
> > 
> > Oh my gosh, please leave only ~3-5 *important* lines out of this, or move it
> > completely to the comment block (after '---' cutter line).
> > 
> > This is requirement written in Submitting Patches.
> > 
> > ...
> > 
> > As for the solution, are you sure the line is not wired at all?  
> 
> No . It is some cheap mini-laptop , I have no schematics or any other 
> info really .
> 
> Note that I am not really familiar with x86 and ACPI, so there is that.
> 
> > IIRC Hans had a broken tales where it was simply forgotten, meaning
> > the Android / Windows driver simply hardcoded needed info.
> > 
> > If it's the case, it should be solved differently around PDx86 special quirk
> > driver for the cases like this.  
> There are likely two issues.
> 
> First, this driver needs to handle i2c_client->irq == 0 correctly if it 
> should work without IRQ line, which the driver seems to indicate that it 
> does. The current crashing the kernel is not the correct way of handling 
> that. That's this patch, in some form.

Hi Marek,

For this, the driver shouldn't be giving the impression it can do either
events or fifo based capture if it doesn't have an irq.
(the fifo case could be made to work but that's a complex job).
Right now the driver only correctly doesn't initialize a trigger
for data ready use when the interrupt isn't there - it does all the
rest unconditionally :(

Usual way to do that is to have two iio_info structures and chan spec
arrays and pick between them.  That should remove any chance of getting
to the code that messes with the interrupts.  It seems to be using
the watermark setting to mess with the fifo interrupt so hiding that
as well in the info structure should block that one and anything that
indicates there might be a fifo.

What you have here stops it crashing and is sensible defence in depth
but cutting off the pointless paths at the top is also needed
to avoid giving userspace the wrong impression.

Jonathan


> 
> Second, if this laptop has some IRQ line for this chip hidden somewhere, 
> then it might need a quirk of sorts, sure. Is there some way to find 
> out, without taking the thing apart and poking around with a scope ?

Powered by blists - more mailing lists