Re: [PATCH v6 1/1] PCI: of: Remove max-link-speed generation validation

[Bjorn Helgaas]

[+cc Rob]

On Thu, Dec 18, 2025 at 09:20:36PM +0800, Hans Zhang wrote:
> The current implementation of of_pci_get_max_link_speed() validates
> max-link-speed property values to be in the range 1~4 (Gen1~Gen4).
> However, this creates maintenance overhead as each new PCIe generation
> requires updating this validation logic.
> 
> Since device tree binding validation already enforces the allowed
> values through the schema, and the callers of this function perform
> their own validation checks, this intermediate validation becomes
> redundant.
> 
> Furthermore, with upcoming SOCs using Synopsys/Cadence IP requiring
> Gen5/Gen6 support, removing this hardcoded check enables seamless
> support for future PCIe generations without requiring kernel updates
> for each new speed grade.

The upcoming SoC info seems like too much detail for the commit log.

> Remove the max-link-speed > 4 validation check while retaining the
> property existence and non-zero check. This simplifies maintenance
> and aligns with the existing validation architecture where DT binding
> and driver-level checks provide sufficient validation.

I don't think it's legit to rely on DT binding checks.  The code
should protect itself regardless of what offline binding checkers
find.

I'm not convinced that callers do their own validation.  For example,
dw_pcie_get_resources() assigns of_pci_get_max_link_speed() to
pci->max_link_speed with no validation at all.

That's in the DWC core, so it's probably OK to expect the actual
controller drivers to validate pci->max_link_speed when they use it,
but dw_pcie_config_presets() uses pcie_link_speed[pci->max_link_speed]
and I don't see an obvious check that prevents an out-of-bounds
reference there.

I think this patch is the right direction -- I don't think there's
anything in drivers/pci/of.c that *uses* the value of
"max-link-speed", so it would be nice if it didn't have to be updated
for every generation.

> Signed-off-by: Hans Zhang <18255117159@xxxxxxx>
> Acked-by: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> ---
> Changes for v6:
> - It'd be good to return the actual errno as of_property_read_u32() can return
>   -EINVAL, -ENODATA and -EOVERFLOW. (Mani)
> 
> Changes for v5:
> https://patchwork.kernel.org/project/linux-pci/patch/20251218125909.305300-1-18255117159@xxxxxxx/
> 
> - Delete the check for speed. (Mani)
> 
> Changes for v4:
> https://patchwork.kernel.org/project/linux-pci/patch/20251105134701.182795-1-18255117159@xxxxxxx/
> 
> - Add pcie_max_supported_link_speed.(Ilpo)
> 
> Changes for v3:
> https://patchwork.kernel.org/project/linux-pci/patch/20251101164132.14145-1-18255117159@xxxxxxx/
> 
> - Modify the commit message.
> - Add Reviewed-by tag.
> 
> Changes for v2:
> https://patchwork.kernel.org/project/linux-pci/cover/20250529021026.475861-1-18255117159@xxxxxxx/
> - The following files have been deleted:
>   Documentation/devicetree/bindings/pci/pci.txt
> 
>   Update to this file again:
>   dtschema/schemas/pci/pci-bus-common.yaml
> ---
>  drivers/pci/of.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 3579265f1198..b56fdbcb3d72 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -888,10 +888,11 @@ bool of_pci_supply_present(struct device_node *np)
>  int of_pci_get_max_link_speed(struct device_node *node)
>  {
>  	u32 max_link_speed;
> +	int ret;
>  
> -	if (of_property_read_u32(node, "max-link-speed", &max_link_speed) ||
> -	    max_link_speed == 0 || max_link_speed > 4)
> -		return -EINVAL;
> +	ret = of_property_read_u32(node, "max-link-speed", &max_link_speed);
> +	if (ret)
> +		return ret;
>  
>  	return max_link_speed;
>  }
> -- 
> 2.34.1
>