| Message ID | 20240702045535.2000393-1-dirk.behme@de.bosch.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [RFC] i2c: rcar: Clear interrupt registers in probe() | expand |
Hi Dirk, On Tue, Jul 2, 2024 at 6:56 AM Dirk Behme <dirk.behme@de.bosch.com> wrote: > We are getting crash reports where irqhandler crashes because it > uses priv->msg being NULL. This can happen if an interrupt is issued > before rcar_i2c_master_xfer() has initialized priv->msg. > > The rcar_i2c_probe() function assumes that the I2C hardware is > uninitialized and connects the interrupt handler via devm_request_irq(). > From this point in time irqhandler can be called. Normally, this is > just prevented by the I2C hardware being in reset state. > > However, there might be cases where rcar_i2c_probe() is called, but > the I2C hardware is *not* in reset state. E.g. if just the Linux > operating system was re-started by a supervisor. But the hardware didn't > get a reset. For cases like this make sure that the I2C hardware > interrupts are cleared properly before devm_request_irq() is called. > > This is inspired by rcar_i2c_init(). > > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> Thanks for your patch! > --- a/drivers/i2c/busses/i2c-rcar.c > +++ b/drivers/i2c/busses/i2c-rcar.c > @@ -1183,6 +1183,12 @@ static int rcar_i2c_probe(struct platform_device *pdev) > ret = platform_get_irq(pdev, 0); > if (ret < 0) > goto out_pm_put; > + > + /* reset master mode */ > + rcar_i2c_write(priv, ICMIER, 0); > + rcar_i2c_write(priv, ICMCR, MDBS); > + rcar_i2c_write(priv, ICMSR, 0); LGTM, but I think you want to do the same for slave mode: rcar_i2c_write(priv, ICSIER, 0); rcar_i2c_write(priv, ICSCR, SDBS); rcar_i2c_write(priv, ICSSR, 0); > + > priv->irq = ret; > ret = devm_request_irq(dev, priv->irq, irqhandler, irqflags, dev_name(dev), priv); > if (ret < 0) { Gr{oetje,eeting}s, Geert
Hi Dirk, thank you for another valuable bug report! On Tue, Jul 02, 2024 at 06:55:35AM +0200, Dirk Behme wrote: > We are getting crash reports where irqhandler crashes because it > uses priv->msg being NULL. This can happen if an interrupt is issued > before rcar_i2c_master_xfer() has initialized priv->msg. > > The rcar_i2c_probe() function assumes that the I2C hardware is > uninitialized and connects the interrupt handler via devm_request_irq(). > From this point in time irqhandler can be called. Normally, this is > just prevented by the I2C hardware being in reset state. > > However, there might be cases where rcar_i2c_probe() is called, but > the I2C hardware is *not* in reset state. E.g. if just the Linux > operating system was re-started by a supervisor. But the hardware didn't > get a reset. For cases like this make sure that the I2C hardware > interrupts are cleared properly before devm_request_irq() is called. > > This is inspired by rcar_i2c_init(). > > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> > --- > drivers/i2c/busses/i2c-rcar.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > Notes: This is untested. Does this make sense and is acceptable? Yes, it makes sense to me. HW should be put to a sane state first in probe. IIRC this slipped through during some refactorization. Maybe it is also the reason for this code in the irq handler? 547 /* FIXME: sometimes, unknown interrupt happened. Do nothing */ 548 if (!(msr & MDE)) 549 return; Although this code existed even before the refactorization. Also, I will add a WARN there, otherwise we never know if this issue still exists. Anyhow, I think Geert's comment also makes sense. But instead of just adding the instructions, I'd prefer to reorganize the code. I hope it is okay for you if I send a "counterpatch". I can't really tell what outcome I actually prefer before I try out myself. Happy hacking, Wolfram
> I hope it is okay for you if I send a "counterpatch".
Sent this now. Please try it if you have a chance.
I think your patch here might not work because you write to registers
when the module clock could be off in most cases. My patch should be
good there. As I said, proper testing scheduled for tomorrow.
Hi Wolfram, On Wed, Jul 3, 2024 at 9:19 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > I think your patch here might not work because you write to registers > when the module clock could be off in most cases. My patch should be > good there. As I said, proper testing scheduled for tomorrow. Good point, I had checked that, but totally missed the conditional pm_runtime_put() :-( Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index 828aa2ea0fe4..5f46955167c4 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -1183,6 +1183,12 @@ static int rcar_i2c_probe(struct platform_device *pdev) ret = platform_get_irq(pdev, 0); if (ret < 0) goto out_pm_put; + + /* reset master mode */ + rcar_i2c_write(priv, ICMIER, 0); + rcar_i2c_write(priv, ICMCR, MDBS); + rcar_i2c_write(priv, ICMSR, 0); + priv->irq = ret; ret = devm_request_irq(dev, priv->irq, irqhandler, irqflags, dev_name(dev), priv); if (ret < 0) {
We are getting crash reports where irqhandler crashes because it uses priv->msg being NULL. This can happen if an interrupt is issued before rcar_i2c_master_xfer() has initialized priv->msg. The rcar_i2c_probe() function assumes that the I2C hardware is uninitialized and connects the interrupt handler via devm_request_irq(). From this point in time irqhandler can be called. Normally, this is just prevented by the I2C hardware being in reset state. However, there might be cases where rcar_i2c_probe() is called, but the I2C hardware is *not* in reset state. E.g. if just the Linux operating system was re-started by a supervisor. But the hardware didn't get a reset. For cases like this make sure that the I2C hardware interrupts are cleared properly before devm_request_irq() is called. This is inspired by rcar_i2c_init(). Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> --- drivers/i2c/busses/i2c-rcar.c | 6 ++++++ 1 file changed, 6 insertions(+) Notes: This is untested. Does this make sense and is acceptable? This might be testable by shutting down Linux to e.g. suspend to RAM. And then, instead of resume, re-start (cold start) the kernel. So that probe() is called and uses a non-reset I2C peripheral.