On Wed, Aug 02, 2023 at 10:08:53AM +0200, Hans Verkuil wrote: > On 02/08/2023 09:49, Dan Carpenter wrote: > > On Wed, Aug 02, 2023 at 09:27:17AM +0200, Hans Verkuil wrote: > >> Hmm, this is old code and when digging into this I noticed inconsistencies > >> in how it is used and how it is documented. > >> > >> It's used by the v4l2_subdev interrupt_service_routine callback. The header > >> include/media/v4l2-subdev.h claims that it is called from IRQ context, but this > >> isn't true in most cases. It's used primarily to pass on interrupts meant for > >> i2c video receivers/transmitters that arrive at the bridge chip to the actual > >> i2c device. And that happens in work handlers that allow the use of locking. > >> > >> I found one exception where it is also used for an IR (infrared) receiver and > >> there it is actually called from IRQ context. It doesn't use i2c or sleeps/locking > >> in that case, so it is safe. > >> > >> But what confuses me is that you state 'This looks at functions which are passed to > >> request_irq()', but adv7511_isr isn't passed to request_irq. So why is this warning > >> raised? > >> > > > > Actually you're right. I was thrown off a bit by the name. > > > > Was the exception cx23885_irq()? Because that's what Smatch is > > concerned about. That calls "v4l2_subdev_call(dev->sd_ir, core, > > interrupt_service_routine," and Smatch thinks interrupt_service_routine > > can point to here. > > Indeed, that's the exception. I wasn't even aware of this myself until I started > grepping the code. > > I wonder if there is better kernel support for things like this, i.e. the irq > from one device is hooked up to another device. Typical for PCI devices where > the i2c devices on the PCI card route their interrupt lines to the PCI bridge > device. > > An alternative is to change the sd_ir handling so that it doesn't use > the interrupt_service_routine callback and then change the documentation for > the interrupt_service_routine callback to allow sleeping. It's rather ugly > the way these two modes are mixed today. I wouldn't worry too much about it... This check isn't anywhere near being published yet. Ideally, I could silence this in Smatch. This is a flaw in Smatch where it treats all function pointers of a given type as the same. So every time ->interrupt_service_routine() is called it thinks they can be treated as equivalent. The main issue so far as this check is concerned is that it thinks (struct spi_message)->complete functions are equivalent when they're not. So I probably will add a table of function pointers like that where it's hard coded that they aren't called from interrupt context. I'd like to fix this in a better way, but in this case it's really hard. regards, dan carpenter
