> -----Original Message-----
> From: linux-i2c-owner@xxxxxxxxxxxxxxx <linux-i2c-owner@xxxxxxxxxxxxxxx> On
> Behalf Of Marek Vasut
> Sent: Saturday, June 13, 2020 8:38 PM
> To: linux-i2c@xxxxxxxxxxxxxxx
> Cc: Marek Vasut <marex@xxxxxxx>; Michal Simek <michals@xxxxxxxxxx>;
> Shubhrajyoti Datta <shubhraj@xxxxxxxxxx>; Wolfram Sang <wsa@xxxxxxxxxx>
> Subject: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler
>
> The interrupt handler is missing locking when reading out registers and is
> racing with other threads which might access the driver. Drop it altogether, so
> that the threaded interrupt is always executed, as that one is already serialized
> by the driver mutex. This also allows dropping
> local_irq_save()/local_irq_restore() in xiic_start_recv().
>
> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> Cc: Michal Simek <michal.simek@xxxxxxxxxx>
> Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxxxxx>
> Cc: Wolfram Sang <wsa@xxxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-xiic.c | 25 +------------------------
> 1 file changed, 1 insertion(+), 24 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index
> 0777e577720db..6db71c0fb6583 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -543,7 +543,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c) {
> u8 rx_watermark;
> struct i2c_msg *msg = i2c->rx_msg = i2c->tx_msg;
> - unsigned long flags;
>
> /* Clear and enable Rx full interrupt. */
> xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK |
> XIIC_INTR_TX_ERROR_MASK); @@ -559,7 +558,6 @@ static void
> xiic_start_recv(struct xiic_i2c *i2c)
> rx_watermark = IIC_RX_FIFO_DEPTH;
> xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rx_watermark - 1);
>
> - local_irq_save(flags);
It is added as part of (i2c: xiic: Make the start and the byte count write atomic - ae7304c3ea28a3ba47a7a8312c76c654ef24967e) commit
to make the below 2 register writes atomic so that the controller doesn't produce a wrong transaction. (If there is a delay between the 2 register
writes, controller is messing up with the transaction). It is not intended for locking between this function and isr. So, this can't be removed.
> if (!(msg->flags & I2C_M_NOSTART))
> /* write the address */
> xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, @@ -569,7 +567,6
> @@ static void xiic_start_recv(struct xiic_i2c *i2c)
>
> xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
> msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK :
> 0));
> - local_irq_restore(flags);
>
> if (i2c->nmsgs == 1)
> /* very last, enable bus not busy as well */ @@ -609,26 +606,6
> @@ static void xiic_start_send(struct xiic_i2c *i2c)
> XIIC_INTR_BNB_MASK);
> }
>
> -static irqreturn_t xiic_isr(int irq, void *dev_id) -{
> - struct xiic_i2c *i2c = dev_id;
> - u32 pend, isr, ier;
> - irqreturn_t ret = IRQ_NONE;
> - /* Do not processes a devices interrupts if the device has no
> - * interrupts pending
> - */
> -
> - dev_dbg(i2c->adap.dev.parent, "%s entry\n", __func__);
> -
> - isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET);
> - ier = xiic_getreg32(i2c, XIIC_IIER_OFFSET);
> - pend = isr & ier;
> - if (pend)
> - ret = IRQ_WAKE_THREAD;
> -
> - return ret;
> -}
> -
> static void __xiic_start_xfer(struct xiic_i2c *i2c) {
> int first = 1;
> @@ -807,7 +784,7 @@ static int xiic_i2c_probe(struct platform_device *pdev)
> pm_runtime_use_autosuspend(i2c->dev);
> pm_runtime_set_active(i2c->dev);
> pm_runtime_enable(i2c->dev);
> - ret = devm_request_threaded_irq(&pdev->dev, irq, xiic_isr,
> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> xiic_process, IRQF_ONESHOT,
> pdev->name, i2c);
>
Removal of isr is fine.
Raviteja N
