Hi Dan,
On 31/07/2023 10:36, Dan Carpenter wrote:
> Hello Hans Verkuil,
>
> The patch 5a544cce2177: "[media] adv7511: add new video encoder" from
> Aug 23, 2013 (linux-next), leads to the following Smatch static
> checker warning:
>
> drivers/media/i2c/adv7511-v4l2.c:929 adv7511_isr()
> warn: sleeping in IRQ context
>
> drivers/media/i2c/adv7511-v4l2.c
> 912 static int adv7511_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
>
> This is a new check so I'm not positive I understand everything
> correctly yet. This looks at functions which are passed to
> request_irq() and assumes that they are not threaded. Also the "_isr"
> here stands for Interrupt Service Routine.
>
> 913 {
> 914 u8 irq_status;
> 915 u8 cec_irq;
> 916
> 917 /* disable interrupts to prevent a race condition */
> 918 adv7511_set_isr(sd, false);
> 919 irq_status = adv7511_rd(sd, 0x96);
> 920 cec_irq = adv7511_rd(sd, 0x97);
> 921 /* clear detected interrupts */
> 922 adv7511_wr(sd, 0x96, irq_status);
> 923 adv7511_wr(sd, 0x97, cec_irq);
> 924
> 925 v4l2_dbg(1, debug, sd, "%s: irq 0x%x, cec-irq 0x%x\n", __func__,
> 926 irq_status, cec_irq);
> 927
> 928 if (irq_status & (MASK_ADV7511_HPD_INT | MASK_ADV7511_MSEN_INT))
> --> 929 adv7511_check_monitor_present_status(sd);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> v4l2_ctrl_s_ctrl() takes a mutex so this is a sleeping function.
>
> 930 if (irq_status & MASK_ADV7511_EDID_RDY_INT)
> 931 adv7511_check_edid_status(sd);
> 932
> 933 #if IS_ENABLED(CONFIG_VIDEO_ADV7511_CEC)
> 934 if (cec_irq & 0x38)
> 935 adv_cec_tx_raw_status(sd, cec_irq);
> 936
> 937 if (cec_irq & 1) {
> 938 struct adv7511_state *state = get_adv7511_state(sd);
> 939 struct cec_msg msg;
> 940
> 941 msg.len = adv7511_cec_read(sd, 0x25) & 0x1f;
>
> regards,
> dan carpenter
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?
Regards,
Hans
