Re: [PATCH v2 4/6] PCI: endpoint: cleanup set_msi() callback

[Damien Le Moal <dlemoal@kernel.org>]

On 5/13/25 16:30, Niklas Cassel wrote:
> The kdoc for pci_epc_set_msi() says:
> "Invoke to set the required number of MSI interrupts."
> the kdoc for the callback pci_epc_ops->set_msi() says:
> "ops to set the requested number of MSI interrupts in the MSI capability
> register"
> 
> pci_epc_ops->set_msi() does however expect the parameter 'interrupts' to
> be in the encoding as defined by the MMC Multiple Message Capable field.
> 
> Nowhere in the kdoc does it say that the number of interrupts should be
> in MMC encoding.
> 
> Thus, it is very confusing that the wrapper function (pci_epc_set_msi())
> and the callback function (pci_epc_ops->set_msi()) both take a parameter
> named interrupts, but they both expect completely different encodings.
> 
> Cleanup the API so that the wrapper function and the callback function
> will have the same semantics.

Same comment as patch 3. Mention the semantic the patch implements.

> 
> Cc: <stable+noautosel@kernel.org> # this is simply a cleanup
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

A few nits below, but other than that, looks good to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> ---
>  drivers/pci/controller/cadence/pcie-cadence-ep.c | 4 +++-
>  drivers/pci/controller/dwc/pcie-designware-ep.c  | 3 ++-
>  drivers/pci/controller/pcie-rcar-ep.c            | 3 ++-
>  drivers/pci/controller/pcie-rockchip-ep.c        | 5 +++--
>  drivers/pci/endpoint/pci-epc-core.c              | 5 +----
>  5 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> index 78b4d009cd04..f307256826e6 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -220,10 +220,12 @@ static void cdns_pcie_ep_unmap_addr(struct pci_epc *epc, u8 fn, u8 vfn,
>  	clear_bit(r, &ep->ob_region_map);
>  }
>  
> -static int cdns_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, u8 mmc)
> +static int cdns_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn,
> +				u8 interrupts)

To be extra clear, I would rename this num_interrupts or nr_interrupts. No
confusion possible with such name.

> diff --git a/drivers/pci/controller/pcie-rcar-ep.c b/drivers/pci/controller/pcie-rcar-ep.c
> index 9da39a4617b6..b25ad23bedb7 100644
> --- a/drivers/pci/controller/pcie-rcar-ep.c
> +++ b/drivers/pci/controller/pcie-rcar-ep.c
> @@ -261,10 +261,11 @@ static int rcar_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn,
>  {
>  	struct rcar_pcie_endpoint *ep = epc_get_drvdata(epc);
>  	struct rcar_pcie *pcie = &ep->pcie;
> +	u8 mmc = order_base_2(interrupts);

Same rename suggested here and for the other drivers.


-- 
Damien Le Moal
Western Digital Research