Hi Wolfram, Thanks for your patch. On 2020-07-26 18:16:06 +0200, Wolfram Sang wrote: > Due to the lockless design of the driver, it is theoretically possible > to access a NULL pointer, if a slave interrupt was running while we were > unregistering the slave. To make this rock solid, disable the interrupt > for a short time while we are clearing the interrupt_enable register. > This patch is purely based on code inspection. The OOPS is super-hard to > trigger because clearing SAR (the address) makes interrupts even more > unlikely to happen as well. While here, reinit SCR to SDBS because this > bit should always be set according to documentation. There is no effect, > though, because the interface is disabled. > > Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx> > --- > > Some people on CC here which encountered the same issue with the > bcm-iproc driver. Does something like this work for you, too? > > drivers/i2c/busses/i2c-rcar.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c > index 8dd35522d95a..0f73f0681a6e 100644 > --- a/drivers/i2c/busses/i2c-rcar.c > +++ b/drivers/i2c/busses/i2c-rcar.c > @@ -871,12 +871,14 @@ static int rcar_unreg_slave(struct i2c_client *slave) > > WARN_ON(!priv->slave); > > - /* disable irqs and ensure none is running before clearing ptr */ > + /* ensure no irq is running before clearing ptr */ > + disable_irq(priv->irq); > rcar_i2c_write(priv, ICSIER, 0); > - rcar_i2c_write(priv, ICSCR, 0); > + rcar_i2c_write(priv, ICSSR, 0); > + enable_irq(priv->irq); > + rcar_i2c_write(priv, ICSCR, SDBS); > rcar_i2c_write(priv, ICSAR, 0); /* Gen2: must be 0 if not using slave */ > > - synchronize_irq(priv->irq); > priv->slave = NULL; > > pm_runtime_put(rcar_i2c_priv_to_dev(priv)); > -- > 2.20.1 > -- Regards, Niklas Söderlund
