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@xxxxxxxxxx> # this is simply a cleanup
> Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
A few nits below, but other than that, looks good to me.
Reviewed-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> ---
> 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
