[PATCH] i2c: imx-lpi2c: avoid accessing target registers on master-only controllers

[PATCH] i2c: imx-lpi2c: avoid accessing target registers on master-only controllers

[carlos.song]

From: Carlos Song <carlos.song@nxp.com>

Not all LPI2C controller instances implement the Target block.
Since commit 90311787f483 ("i2c: imx-lpi2c: reset controller in
probe stage"), the driver unconditionally resets both the Master
and Target blocks during probe.

On controllers that do not support target mode, accessing the
Target registers triggers an asynchronous SError and prevents the
driver from probing successfully. For example on i.MX8QM:

  SError Interrupt on CPU2, code 0x00000000bf000002 -- SError
  Hardware name: Freescale i.MX8QM MEK (DT)
  pc : lpi2c_imx_probe+0x280/0x594
  lr : lpi2c_imx_probe+0x224/0x594
  Kernel panic - not syncing: Asynchronous SError Interrupt

The VERID register is implemented in the Master block and can be
safely accessed on all controller variants. Its FEATURE field
indicates whether target mode is supported.

Read VERID during probe and use it to determine whether the
Target block is present. Only access Target registers when target
mode is supported and reject target registration requests with
-EOPNOTSUPP otherwise.

Fixes: 90311787f483 ("i2c: imx-lpi2c: reset controller in probe stage")
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index e1a4338bc51e..1cfd7a4c8237 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -29,6 +29,7 @@
 
 #define DRIVER_NAME "imx-lpi2c"
 
+#define LPI2C_VERID	0x00	/* i2c version ID */
 #define LPI2C_PARAM	0x04	/* i2c RX/TX FIFO size */
 #define LPI2C_MCR	0x10	/* i2c contrl register */
 #define LPI2C_MSR	0x14	/* i2c status register */
@@ -136,6 +137,9 @@
 #define I2C_PM_LONG_TIMEOUT_MS	1000 /* Avoid dead lock caused by big clock prepare lock */
 #define I2C_DMA_THRESHOLD	8 /* bytes */
 
+/* Bit 0 indicates the presence of the target feature */
+#define VERID_FEATURE_TARGET_PRESENT	BIT(0)
+
 enum lpi2c_imx_mode {
 	STANDARD,	/* 100+Kbps */
 	FAST,		/* 400+Kbps */
@@ -194,6 +198,7 @@ struct lpi2c_imx_struct {
 	bool			can_use_dma;
 	struct lpi2c_imx_dma	*dma;
 	struct i2c_client	*target;
+	bool			target_supported;
 	int			irq;
 	const struct imx_lpi2c_hwdata *hwdata;
 };
@@ -1330,6 +1335,10 @@ static int lpi2c_imx_register_target(struct i2c_client *client)
 	struct lpi2c_imx_struct *lpi2c_imx = i2c_get_adapdata(client->adapter);
 	int ret;
 
+	/* Reject target-mode registration on controllers that don't support it. */
+	if (!lpi2c_imx->target_supported)
+		return -EOPNOTSUPP;
+
 	if (lpi2c_imx->target)
 		return -EBUSY;
 
@@ -1546,13 +1555,23 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 
 	/*
-	 * Reset all internal controller registers of both Master and Target
-	 * to avoid effects of previous status.
+	 * Reset all internal controller registers to avoid effects of any
+	 * state left over from a previous stage (e.g. the bootloader).
+	 *
+	 * The Master block (MCR) is present on every controller, so reset it
+	 * unconditionally. VERID shows whether the target feature is supported.
+	 * Do not touch the Target block (SCR) on a master-only controller to
+	 * avoid an asynchronous SError.
 	 */
 	writel(MCR_RST, lpi2c_imx->base + LPI2C_MCR);
-	writel(SCR_RST, lpi2c_imx->base + LPI2C_SCR);
 	writel(0, lpi2c_imx->base + LPI2C_MCR);
-	writel(0, lpi2c_imx->base + LPI2C_SCR);
+
+	lpi2c_imx->target_supported = !!(readl(lpi2c_imx->base + LPI2C_VERID) &
+					 VERID_FEATURE_TARGET_PRESENT);
+	if (lpi2c_imx->target_supported) {
+		writel(SCR_RST, lpi2c_imx->base + LPI2C_SCR);
+		writel(0, lpi2c_imx->base + LPI2C_SCR);
+	}
 
 	ret = devm_request_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx_isr, IRQF_NO_SUSPEND,
 			       pdev->name, lpi2c_imx);
-- 
2.43.0

Re: [PATCH] i2c: imx-lpi2c: avoid accessing target registers on master-only controllers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing rollback in `lpi2c_imx_register_target()` when `pm_runtime_resume_and_get()` fails, leaving a dangling pointer in `lpi2c_imx->target`.
--

--- Patch [1]: [PATCH] i2c: imx-lpi2c: avoid accessing target registers on master-only controllers ---
commit c2a5eb969b33b95968f240043a2bfa7ba6e85caf
Author: Carlos Song <carlos.song@nxp.com>

i2c: imx-lpi2c: avoid accessing target registers on master-only controllers

This commit reads the VERID register during probe to check for target mode
support. It prevents accessing target registers on master-only controllers
and rejects target registration requests if unsupported.

> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
[ ... ]
> @@ -1330,6 +1335,10 @@ static int lpi2c_imx_register_target(struct i2c_client *client)
>  	struct lpi2c_imx_struct *lpi2c_imx = i2c_get_adapdata(client->adapter);
>  	int ret;
>  
> +	/* Reject target-mode registration on controllers that don't support it. */
> +	if (!lpi2c_imx->target_supported)
> +		return -EOPNOTSUPP;
> +
>  	if (lpi2c_imx->target)
>  		return -EBUSY;
>  
>  	lpi2c_imx->target = client;
>  
>  	ret = pm_runtime_resume_and_get(lpi2c_imx->adapter.dev.parent);
>  	if (ret < 0) {
>  		dev_err(&lpi2c_imx->adapter.dev, "failed to resume i2c controller");
>  		return ret;
>  	}

[Severity: High]
This is a pre-existing issue, but does this error path leave a dangling
pointer in the controller data?

If pm_runtime_resume_and_get() fails in lpi2c_imx_register_target(), the
function returns without clearing lpi2c_imx->target. Because the registration
failed, the target driver probe will fail and the i2c_client memory can be
freed (e.g., if dynamically removed or unbound).

Later, if the system is suspended and resumed, lpi2c_resume_noirq() will see
that lpi2c_imx->target is non-NULL and unconditionally dereference it:

drivers/i2c/busses/i2c-imx-lpi2c.c:lpi2c_resume_noirq() {
    ...
    if (lpi2c_imx->target)
        lpi2c_imx_target_init(lpi2c_imx);
    ...
}

Should lpi2c_imx->target be set back to NULL in this error path to avoid a
potential use-after-free crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803032705.2250373-1-carlos.song@oss.nxp.com?part=1