Re: [PATCH 2/6] PCI: dwc: Add outbound ATU range check callback

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Aug 20, 2025 at 07:18:39PM +0800, Randolph Lin wrote:
> Introduce a callback for outbound ATU range checking to support
> range validations specific to cases that deviate from the generic
> check.
> 
> Signed-off-by: Randolph Lin <randolph@andestech.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 18 +++++++++++++-----
>  drivers/pci/controller/dwc/pcie-designware.h |  3 +++
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 89aad5a08928..f410aefaeb5e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -535,12 +535,20 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
>  	u32 retries, val;
>  	u64 limit_addr;
>  
> -	limit_addr = parent_bus_addr + atu->size - 1;
> +	if (pci->ops && pci->ops->outbound_atu_check) {
> +		val = pci->ops->outbound_atu_check(pci, atu, &limit_addr);

The return is not a "val" and not a "u32".  It should be named "ret"
or similar.  Should be "int" since the callback and
dw_pcie_prog_outbound_atu() return "int".  But see below for possible
signature change.

Also not 100% convinced this is needed, see other patch where this is
implemented.

> +		if (val)
> +			return val;
> +	} else {
> +		limit_addr = parent_bus_addr + atu->size - 1;
>  
> -	if ((limit_addr & ~pci->region_limit) != (parent_bus_addr & ~pci->region_limit) ||
> -	    !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
> -	    !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size) {
> -		return -EINVAL;
> +		if ((limit_addr & ~pci->region_limit) !=
> +		    (parent_bus_addr & ~pci->region_limit) ||
> +		    !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
> +		    !IS_ALIGNED(atu->pci_addr, pci->region_align) ||
> +		    !atu->size) {
> +			return -EINVAL;
> +		}
>  	}
>  
>  	dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_LOWER_BASE,
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 00f52d472dcd..40dd2c83b1c7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -469,6 +469,9 @@ struct dw_pcie_ep {
>  
>  struct dw_pcie_ops {
>  	u64	(*cpu_addr_fixup)(struct dw_pcie *pcie, u64 cpu_addr);
> +	u32	(*outbound_atu_check)(struct dw_pcie *pcie,
> +				      const struct dw_pcie_ob_atu_cfg *atu,
> +				      u64 *limit_addr);

I have kind of an allergic reaction to things named "check" because
the name doesn't suggest anything about what the function does or what
it will return.  For bool functions, I prefer a name that's a
predicate that can be either true or false, e.g., "valid".

This isn't a bool, but possibly *could* be, e.g.,
"outbound_atu_addr_valid()".  Then the caller would be something like:

  if (!pci->ops->outbound_atu_addr_valid(...))
    return -EINVAL;