Re: [PATCH v4 2/3] drm/xe/i2c: Fix the interrupt handling

[Heikki Krogerus]

Hi Raag,

> > > > > -	/* Forward interrupt to I2C adapter */
> > > > > -	generic_handle_irq_safe(xe->i2c->adapter_irq);
> > > > > +	xe_i2c_handle_smbus_alert(xe->i2c);
> > > > 
> > > > [1] Can we move the below re-assert code to wq now? Or do you suspect any
> > > > side-effects?
> > > 
> > > I think that you know this better than I do. But at this point
> > > interrupt is cleared, so why should we wait for the wq?
> > 
> > When does AMC clear the alert signal? Is it when you query from the wq?
> > If the answer is yes, there's a possibility we might end up with an
> > interrupt storm here.
> > 
> > > To play it safe, can we change this as a followup if necessary?
> > 
> > Sure, I'll leave it to you.
> 
> I'll move it to the wq. There are no side-effects.

Can you check does this work:

diff --git a/drivers/gpu/drm/xe/xe_amc.c b/drivers/gpu/drm/xe/xe_amc.c
index b44d5765f2ed5..1a517b79d0bb3 100644
--- a/drivers/gpu/drm/xe/xe_amc.c
+++ b/drivers/gpu/drm/xe/xe_amc.c
@@ -11,9 +11,12 @@
 #include <linux/string.h>
 #include <linux/workqueue.h>
 
+#include "regs/xe_i2c_regs.h"
+
 #include "xe_amc.h"
 #include "xe_device.h"
 #include "xe_i2c.h"
+#include "xe_mmio.h"
 
 /**
  * DOC: Add-In Management Controller (AMC)
@@ -103,7 +106,7 @@ static void xe_amc_work(struct work_struct *work)
        ret = i2c_master_send(client, (u8 *)request, sizeof(*request));
        if (ret < 0) {
                dev_err(&client->dev, "failed to send request (%d)\n", ret);
-               return;
+               goto out_reassert_interrupt;
        }
 
        /* AMC needs 20ms to generate the response. */
@@ -112,22 +115,22 @@ static void xe_amc_work(struct work_struct *work)
        ret = i2c_master_recv(client, (u8 *)&response, sizeof(response));
        if (ret < 0) {
                dev_err(&client->dev, "failed to read response (%d)\n", ret);
-               return;
+               goto out_reassert_interrupt;
        }
 
        if (!response.header.len) {
                dev_err(&client->dev, "empty response from AMC\n");
-               return;
+               goto out_reassert_interrupt;
        }
 
        if (memcmp(&response.message, &request->message, sizeof(struct amc_message))) {
                dev_err(&client->dev, "response does not match the request\n");
-               return;
+               goto out_reassert_interrupt;
        }
 
        if (response.error) {
                dev_err(&client->dev, "AMC error 0x%02x\n", response.error);
-               return;
+               goto out_reassert_interrupt;
        }
 
        dev_dbg(&client->dev, "%s: Alert reason: %d\n", __func__, response.value);
@@ -143,12 +146,19 @@ static void xe_amc_work(struct work_struct *work)
        default:
                break;
        }
+
+out_reassert_interrupt:
+       xe_mmio_rmw32(amc->i2c->mmio, I2C_CONFIG_CMD, PCI_COMMAND_INTX_DISABLE, 0);
 }
 
 void xe_amc_handle_alert(struct xe_i2c *i2c)
 {
+       xe_mmio_rmw32(i2c->mmio, I2C_CONFIG_CMD, 0, PCI_COMMAND_INTX_DISABLE);
+
        if (i2c->client[XE_I2C_CLIENT_AMC])
                queue_work(system_long_wq, &i2c->amc->work);
+       else
+               xe_mmio_rmw32(i2c->mmio, I2C_CONFIG_CMD, PCI_COMMAND_INTX_DISABLE, 0);
 }
 
 int xe_amc_init(struct xe_i2c *i2c)
diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
index 8a978526e2b66..3859d4a9f75e4 100644
--- a/drivers/gpu/drm/xe/xe_i2c.c
+++ b/drivers/gpu/drm/xe/xe_i2c.c
@@ -87,6 +87,8 @@ static void xe_i2c_client_work(struct work_struct *work)
        };
 
        i2c->client[XE_I2C_CLIENT_AMC] = i2c_new_client_device(i2c->adapter, &info);
+
+       xe_i2c_irq_postinstall(i2c_adapter_to_xe_device(i2c->adapter));
 }
 
 static int xe_i2c_notifier(struct notifier_block *nb, unsigned long action, void *data)
@@ -183,17 +185,10 @@ static bool xe_i2c_irq_present(struct xe_device *xe)
  */
 void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl)
 {
-       struct xe_mmio *mmio = xe_root_tile_mmio(xe);
-
        if (!(master_ctl & I2C_IRQ) || !xe_i2c_irq_present(xe))
                return;
 
        xe_i2c_handle_smbus_alert(xe->i2c);
-
-       /* Deassert after I2C adapter clears the interrupt */
-       xe_mmio_rmw32(mmio, I2C_CONFIG_CMD, 0, PCI_COMMAND_INTX_DISABLE);
-       /* Reassert to allow subsequent interrupt generation */
-       xe_mmio_rmw32(mmio, I2C_CONFIG_CMD, PCI_COMMAND_INTX_DISABLE, 0);
 }
 
 void xe_i2c_irq_reset(struct xe_device *xe)
@@ -392,6 +387,5 @@ int xe_i2c_probe(struct xe_device *xe)
                return ret;
        }
 
-       xe_i2c_irq_postinstall(xe);
        return devm_add_action_or_reset(drm_dev, xe_i2c_remove, i2c);
 }

Thanks,

-- 
heikki