Messages in this thread Patch in this message | | | Date | Mon, 16 Oct 2023 10:42:27 +0200 | From | Alvin Šipraga <> | Subject | Re: [PATCH] drm/bridge: adv7511: fix crash on irq during probe |
| |
Hi Laurent,
Thanks for the quick review!
On Mon, Oct 16, 2023 at 11:14:44AM +0300, Laurent Pinchart wrote: > Hello Alvin, > > On Sat, Oct 14, 2023 at 08:46:12PM +0200, Alvin Šipraga wrote: > > From: Mads Bligaard Nielsen <bli@bang-olufsen.dk> > > > > Moved IRQ registration down to end of adv7511_probe(). > > > > If an IRQ already is pending during adv7511_probe > > (before adv7511_cec_init) then cec_received_msg_ts > > could crash using uninitialized data: > > > > Unable to handle kernel read from unreadable memory at virtual address 00000000000003d5 > > Internal error: Oops: 96000004 [#1] PREEMPT_RT SMP > > Call trace: > > cec_received_msg_ts+0x48/0x990 [cec] > > adv7511_cec_irq_process+0x1cc/0x308 [adv7511] > > adv7511_irq_process+0xd8/0x120 [adv7511] > > adv7511_irq_handler+0x1c/0x30 [adv7511] > > irq_thread_fn+0x30/0xa0 > > irq_thread+0x14c/0x238 > > kthread+0x190/0x1a8 > > > > Fixes: 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support") > > Isn't the issue older than that ?
I don't think so. The stacktrace shows the crash is in CEC handling code, which was added in the blamed commit. A static analysis of adv7511_irq_process() suggests that the only other place where data could be uninitialized is if the hpd_work is scheduled:
if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder) schedule_work(&adv7511->hpd_work);
... but this has a check on bridge.encoder, which seems to have been introduced in a similar fix here:
| commit a1d0503d26ea2ef04f3f013d379e8f4d29c27127 | Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> | Date: Thu May 14 00:31:07 2015 +0300 | | drm: adv7511: Fix crash in IRQ handler when no encoder is associated | | The ADV7511 is probed before its slave encoder init function associates | it with an encoder. This creates a time window during which hot plug | detection interrupts can occur with an encoder, resulting in a crash in | the IRQ handler. | | Fix this by ignoring hot plug detection IRQs when no encoder is | associated yet. | | Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> | Acked-by: Lars-Peter Clausen <lars@metafoo.de> | | diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c | index b728523e194f..2aaa3c88999e 100644 | --- a/drivers/gpu/drm/i2c/adv7511.c | +++ b/drivers/gpu/drm/i2c/adv7511.c | @@ -438,7 +438,7 @@ static int adv7511_irq_process(struct adv7511 *adv7511) | regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0); | regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1); | | - if (irq0 & ADV7511_INT0_HDP) | + if (irq0 & ADV7511_INT0_HDP && adv7511->encoder) | drm_helper_hpd_irq_event(adv7511->encoder->dev); | | if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
So assuming that is the case, I am not sure which Fixes: tag I ought to otherwise use. What do you think?
> > > Signed-off-by: Mads Bligaard Nielsen <bli@bang-olufsen.dk> > > Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk> > > With the Fixes: tag updated, > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Kind regards, Alvin
| |