Re: [PATCH v7 5/6] i2c: qcom-geni: Store of_device_id data in driver private struct

[sashiko-bot@kernel.org]

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

New issues:
- [High] Accessing a non-existent member `core_clk` in `struct geni_se` causes a build failure and alters fallback logic.
- [High] DMA channels are permanently leaked if `i2c_add_adapter()` fails during probe.

Pre-existing issues:
- [High] Multi-descriptor DMA buffer arrays are leaked on early transfer errors or partial allocation failures.
--

commit 1f9fcc815478494b91c04d32de4b9e7efa6fac42
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

i2c: qcom-geni: Store of_device_id data in driver private struct

This commit updates the driver to store the struct of_device_id data
directly in the i2c private structure. The goal is to improve code
maintainability and avoid repeatedly fetching and checking platform
data across different functions.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 8f8b74c4e88a4..92cccb1cce698 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c

[ ... ]

> @@ -1033,8 +1023,8 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
>  		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
>  
>  		/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
> -		if (!tx_depth && desc)
> -			tx_depth = desc->tx_fifo_depth;
> +		if (!tx_depth && gi2c->se.core_clk)
> +			tx_depth = gi2c->dev_data->tx_fifo_depth;

[Severity: High]
Does struct geni_se have a core_clk member? Looking at geni-se.h, it appears
this will cause a build failure since core_clk doesn't exist.

Additionally, if this is changed to gi2c->se.clk, will it alter the original
fallback logic? Previously, if a master-hub device lacked this clock, it
correctly fell back to using desc->tx_fifo_depth. With this change, if the
clock is missing, tx_depth might remain 0 and cause a probe failure.

[ ... ]

> @@ -1089,7 +1078,9 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  	if (IS_ERR(gi2c->se.base))
>  		return PTR_ERR(gi2c->se.base);
>  
> -	desc = device_get_match_data(&pdev->dev);
> +	gi2c->dev_data = device_get_match_data(&pdev->dev);
> +	if (!gi2c->dev_data)
> +		return -EINVAL;

[Severity: High]
During the refactoring in this patch series, did we lose the cleanup for
DMA channels in geni_i2c_probe()?

If i2c_add_adapter() fails after geni_i2c_init() has successfully requested
GPI DMA channels via setup_gpi_dma(), release_gpi_dma() is not called:

geni_i2c_probe() {
    ...
    ret = geni_i2c_init(gi2c);
    if (ret < 0) { ... }

    ret = i2c_add_adapter(&gi2c->adap);
    if (ret) {
        dev_err_probe(dev, ret, "Error adding i2c adapter\n");
        pm_runtime_disable(gi2c->se.dev);
        return ret;
    }
    ...
}

Are tx_c and rx_c permanently leaked here on failure?

[ ... ]

[Severity: High]
This is a pre-existing issue, but do multi-descriptor DMA buffer arrays
leak on early transfer errors or partial allocation failures?

In geni_i2c_gpi_xfer(), if the second allocation fails, it jumps to the
error label:

geni_i2c_gpi_xfer() {
    ...
    tx_multi_xfer->dma_buf = kcalloc(gi2c->num_msgs, ...);
    tx_multi_xfer->dma_addr = kcalloc(gi2c->num_msgs, ...);
    if (!tx_multi_xfer->dma_buf || !tx_multi_xfer->dma_addr) {
        ret = -ENOMEM;
        goto err;
    }
    ...
err:
    geni_i2c_gpi_multi_desc_unmap(gi2c, msgs, &i);
}

Inside geni_i2c_gpi_multi_desc_unmap(), the arrays are only freed if
the loop reaches the end:

geni_i2c_gpi_multi_desc_unmap() {
    ...
    if (tx_multi_xfer->unmap_msg_cnt == gi2c->num_msgs - 1) {
        kfree(tx_multi_xfer->dma_buf);
        kfree(tx_multi_xfer->dma_addr);
        break;
    }
    ...
}

During an early abort or partial allocation failure, the arrays are not
freed. Should they be cleaned up unconditionally?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com?part=5