Re: [PATCH v2 2/3] phy: renesas: rcar-gen2: Return -EINVAL for out-of-range channel reg

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

Pre-existing issues:
- [High] Pointer to device node 'np' is stored in 'channel->of_node' without holding a reference, leading to a dangling pointer.
- [High] Race condition in probe: PHY provider is registered before driver data is set.
- [High] The return value of clk_prepare_enable() is ignored, leading to a potential system lockup if the clock fails to enable.
- [Medium] Holding a spinlock with interrupts disabled for 340 microseconds violates kernel latency constraints.
--

--- Patch [2]: [PATCH v2 2/3] phy: renesas: rcar-gen2: Return -EINVAL for out-of-range channel reg ---
commit b403c6500188baed7349ae963beab9f0eda61eb2
Author: Felix Gu <[email protected]>

    phy: renesas: rcar-gen2: Return -EINVAL for out-of-range channel reg
    
    When of_property_read_u32() succeeds but channel_num exceeds
    data->num_channels, rcar_gen2_phy_probe() returns error which is 0,
    so probe reports success even though no PHY provider is registered.
    
    Return -EINVAL in that case.
    
    Fixes: 1233f59f745b ("phy: Renesas R-Car Gen2 PHY driver")
    Signed-off-by: Felix Gu <[email protected]>

> diff --git a/drivers/phy/renesas/phy-rcar-gen2.c b/drivers/phy/renesas/phy-rcar-gen2.c
> index 5a272e25e051c..b18727ed41a10 100644
> --- a/drivers/phy/renesas/phy-rcar-gen2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen2.c
> @@ -390,7 +390,7 @@ static int rcar_gen2_phy_probe(struct platform_device *pdev)
>  		error = of_property_read_u32(np, "reg", &channel_num);
>  		if (error || channel_num >= data->num_channels) {
>  			dev_err(dev, "Invalid \"reg\" property\n");
> -			return error;
> +			return error ?: -EINVAL;
>  		}
>  		channel->select_mask = select_mask[channel_num];
>  

While reviewing this patch, I noticed a few pre-existing issues in this file
that might be worth addressing:

In rcar_gen2_phy_probe():

> 	for_each_child_of_node_scoped(dev->of_node, np) {
> 		struct rcar_gen2_channel *channel = drv->channels + i;
> 		u32 channel_num;
> 		int error, n;
> 
> 		channel->of_node = np;

[Severity: High]
This isn't a bug introduced by this patch, but the device node pointer 'np' is
assigned to the persistent channel structure without taking a reference.
Since 'for_each_child_of_node_scoped' automatically drops the reference on loop
exit, does this leave a dangling pointer in 'channel->of_node'? Later,
rcar_gen2_phy_xlate() compares 'np == drv->channels[i].of_node', which might
lead to an ABA problem if the node is freed and reallocated. Should we call
of_node_get() here?

Later in rcar_gen2_phy_probe():

> 	provider = devm_of_phy_provider_register(dev, rcar_gen2_phy_xlate);
> 	if (IS_ERR(provider)) {
> 		dev_err(dev, "Failed to register PHY provider\n");
> 		return PTR_ERR(provider);
> 	}
> 
> 	dev_set_drvdata(dev, drv);

[Severity: High]
This is also a pre-existing issue, but is there a race condition here during
probe? We register the PHY provider and expose it to consumers before we
initialize the driver data with dev_set_drvdata(). If a consumer driver tries
to acquire the PHY right after devm_of_phy_provider_register(),
rcar_gen2_phy_xlate() could execute, call dev_get_drvdata(), and receive NULL.

In rcar_gen2_phy_init():

> 	clk_prepare_enable(drv->clk);
> 
> 	spin_lock_irqsave(&drv->lock, flags);
> 	ugctrl2 = readl(drv->base + USBHS_UGCTRL2);

[Severity: High]
This is a pre-existing issue, but does this code need to check the return value
of clk_prepare_enable()? If the clock fails to enable, we proceed to
immediately access the hardware registers which could lock up the system or
cause a synchronous external abort.

In rz_g1c_phy_power_on():

> 	spin_lock_irqsave(&drv->lock, flags);
> 
> 	/* Power on USBHS PHY */
> 	value = readl(base + USBHS_UGCTRL);
> 	value &= ~USBHS_UGCTRL_PLLRESET;
> 	writel(value, base + USBHS_UGCTRL);
> 
> 	/* As per the data sheet wait 340 micro sec for power stable */
> 	udelay(340);

[Severity: Medium]
This is a pre-existing issue, but is it safe to hold a spinlock with
interrupts disabled for 340 microseconds here? This seems like it would
severely impact system latency, potentially causing missed interrupts.

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

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