On 8/7/2020 8:55 PM, Dhananjay Phadke wrote: > On 8/7/2020, Florian Fainelli wrote: >>> When i2c client unregisters, synchronize irq before setting >>> iproc_i2c->slave to NULL. >>> >>> (1) disable_irq() >>> (2) Mask event enable bits in control reg >>> (3) Erase slave address (avoid further writes to rx fifo) >>> (4) Flush tx and rx FIFOs >>> (5) Clear pending event (interrupt) bits in status reg >>> (6) enable_irq() >>> (7) Set client pointer to NULL >>> >> >>> @@ -1091,6 +1091,17 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave) >>> tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT); >>> iproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, tmp); >>> >>> + /* flush TX/RX FIFOs */ >>> + tmp = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT)); >>> + iproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, tmp); >>> + >>> + /* clear all pending slave interrupts */ >>> + iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, ISR_MASK_SLAVE); >>> + >>> + enable_irq(iproc_i2c->irq); >>> + >>> + iproc_i2c->slave = NULL; >> >> There is nothing that checks on iproc_i2c->slave being valid within the >> interrupt handler, we assume that the pointer is valid which is fin, >> however non functional it may be, it may feel more natural to move the >> assignment before the enable_irq()? > > As far as the teardown sequence ensures no more interrupts arrive after > enable_irq() and they are enabled only after setting pointer during > client register(); checking for NULL in ISR isn't necessary. Agreed. > > If The teardown sequence doesn't guarantee quiescing of interrupts, > setting NULL before or after enable_irq() is equally vulnerable. The teardown sequence is sort of a critical section if we may say, so ensuring that everything happens within it and that enable_irq() is the last operation would seem more natural to me at least. Thanks -- Florian
