Re: [PATCH] i3c: master: adi: add OF module alias for autoloading

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

Pre-existing issues:
- [High] Missing `return` statement when peripheral version check fails in probe function.
- [High] When an active I3C transfer times out, the queue stalls because the next pending transfer is not started, and new transfers bypass the queue.
--

--- Patch [1]: [PATCH] i3c: master: adi: add OF module alias for autoloading ---
commit 5b2cc41f2cf6deba8abe337092f652d525b6756d
Author: Can Peng <[email protected]>

i3c: master: adi: add OF module alias for autoloading

The Analog Devices I3C master driver can be built as a module and uses
adi_i3c_master_of_match as its OF match table, but the table is not
exported for module alias generation. This commit adds the
MODULE_DEVICE_TABLE entry so modpost can generate OF module aliases.

> diff --git a/drivers/i3c/master/adi-i3c-master.c b/drivers/i3c/master/adi-i3c-master.c
> index e29aac2869577..fb4cfc9026ccf 100644
> --- a/drivers/i3c/master/adi-i3c-master.c
> +++ b/drivers/i3c/master/adi-i3c-master.c
> @@ -929,6 +929,7 @@ static const struct of_device_id adi_i3c_master_of_match[] = {
>  	{ .compatible = "adi,i3c-master-v1" },
>  	{}
>  };
> +MODULE_DEVICE_TABLE(of, adi_i3c_master_of_match);
>  
>  static int adi_i3c_master_probe(struct platform_device *pdev)
>  {

[ ... ]

> 	version = readl(master->regs + ADI_AXI_REG_VERSION);
> 	if (ADI_AXI_PCORE_VER_MAJOR(version) != 1)
> 		dev_err_probe(&pdev->dev, -ENODEV, "Unsupported peripheral version %u.%u.%u\n",
> 			      ADI_AXI_PCORE_VER_MAJOR(version),
> 			      ADI_AXI_PCORE_VER_MINOR(version),
> 			      ADI_AXI_PCORE_VER_PATCH(version));
> 
> 	writel(0x00, master->regs + REG_ENABLE);

[Severity: High]
This is a pre-existing issue, but does adi_i3c_master_probe() need a return
statement when the peripheral version check fails?

It looks like if an unsupported hardware version is detected, it logs an
error via dev_err_probe() but execution falls through and continues hardware
initialization.

[Severity: High]
This isn't a bug introduced by this patch, but can the queue stall if an
active I3C transfer times out?

Looking at adi_i3c_master_unqueue_xfer(), when a timeout cancels the active
transfer, it sets master->xferqueue.cur to NULL but doesn't check the list
to start the next pending transfer:

drivers/i3c/master/adi-i3c-master.c:adi_i3c_master_unqueue_xfer() {
...
	guard(spinlock_irqsave)(&master->xferqueue.lock);
	if (master->xferqueue.cur == xfer)
		master->xferqueue.cur = NULL;
	else
		list_del_init(&xfer->node);
...
}

Additionally, newly queued transfers in adi_i3c_master_queue_xfer() will
bypass any waiting transfers if the timeout left the current pointer as NULL:

drivers/i3c/master/adi-i3c-master.c:adi_i3c_master_queue_xfer() {
...
	guard(spinlock_irqsave)(&master->xferqueue.lock);
	if (master->xferqueue.cur) {
		list_add_tail(&xfer->node, &master->xferqueue.list);
	} else {
		master->xferqueue.cur = xfer;
		adi_i3c_master_start_xfer_locked(master);
	}
...
}

Does the driver need to call adi_i3c_master_start_xfer_locked() for the
next item in master->xferqueue.list when clearing the active transfer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c