Re: [PATCH v6 3/3] PCI: rockchip: drive at 2.5 GT/s, error other speeds

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Omitting the optional `max-link-speed` device tree property causes a spurious "invalid max-link-speed" warning on every boot.
- [Low] The warning "5.0 GT/s may cause data loss or worse" in `rockchip_pcie_init_port()` is unreachable dead code.
--

commit af559876a98865f8e79209df2d4604f56fef03fd
Author: Geraldo Nascimento <geraldogabriel@gmail.com>

PCI: rockchip: drive at 2.5 GT/s, error other speeds

Configure the core to be driven at 2.5 GT/s Link Speed and ignore any
other speed with a warning. Also drop the 5.0 GT/s Link Speed defines
from Rockchip PCIe header.

Link: https://lore.kernel.org/all/ffd05070-9879-4468-94e3-b88968b4c21b@rock-chips.com/

> diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
> index 0f88da3788054..5a2876d7c8547 100644
> --- a/drivers/pci/controller/pcie-rockchip.c
> +++ b/drivers/pci/controller/pcie-rockchip.c
> @@ -66,8 +66,10 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
>  	}
>  
>  	rockchip->link_gen = of_pci_get_max_link_speed(node);
> -	if (rockchip->link_gen < 0 || rockchip->link_gen > 2)
> -		rockchip->link_gen = 2;
> +	if (rockchip->link_gen < 0 || rockchip->link_gen >= 2) {
> +		rockchip->link_gen = 1;
> +		dev_warn(dev, "invalid max-link-speed, limited to 2.5 GT/s\n");
> +	}

[Severity: Medium]
Will this cause a spurious warning on every boot if the optional
"max-link-speed" device tree property is omitted?

If the property is not present, of_pci_get_max_link_speed() returns
-EINVAL, which is less than 0. This condition catches that negative return
value and triggers the "invalid max-link-speed" warning, even though omitting
an optional property is a standard configuration.

[ ... ]

> @@ -147,12 +149,12 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>  		goto err_exit_phy;
>  	}
>  
> +	/* 5.0 GT/s may cause catastrophic failure for this core */
>  	if (rockchip->link_gen == 2)
> -		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_2,
> -				    PCIE_CLIENT_CONFIG);
> -	else
> -		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_1,
> -				    PCIE_CLIENT_CONFIG);
> +		dev_warn(dev, "5.0 GT/s may cause data loss or worse\n");

[Severity: Low]
Is this warning unreachable dead code?

In rockchip_pcie_parse_dt() executed earlier during probe,
rockchip->link_gen is explicitly capped to 1 if it is >= 2.
Therefore, when this check in rockchip_pcie_init_port() evaluates
whether rockchip->link_gen == 2, it appears it will always be false.

> +
> +	rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_1,
> +			    PCIE_CLIENT_CONFIG);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781207474.git.geraldogabriel@gmail.com?part=3