Re: [PATCH v2 9/9] PCI: rockchip: Add parameter check for RK3399 PCIe endpoint core set_msi()

[Rick Wertenbroek]

On Tue, Feb 21, 2023 at 11:55 AM Damien Le Moal
<damien.lemoal@xxxxxxxxxxxxxxxxxx> wrote:
>
> On 2/21/23 19:47, Rick Wertenbroek wrote:
> > On Wed, Feb 15, 2023 at 2:39 AM Damien Le Moal
> > <damien.lemoal@xxxxxxxxxxxxxxxxxx> wrote:
> >>
> >> On 2/14/23 23:08, Rick Wertenbroek wrote:
> >>> The RK3399 PCIe endpoint core supports only a single PCIe physcial
> >>> function (function number 0), therefore return -EINVAL if set_msi() is
> >>> called with a function number greater than 0.
> >>> The PCIe standard only allows the multi message capability (MMC) value
> >>> to be up to 0x5 (32 messages), therefore return -EINVAL if set_msi() is
> >>> called with a MMC value of over 0x5.
> >>>
> >>> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@xxxxxxxxx>
> >>> ---
> >>>  drivers/pci/controller/pcie-rockchip-ep.c | 10 ++++++++++
> >>>  1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> >>> index b7865a94e..80634b690 100644
> >>> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> >>> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> >>> @@ -294,6 +294,16 @@ static int rockchip_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn,
> >>>       struct rockchip_pcie *rockchip = &ep->rockchip;
> >>>       u32 flags;
> >>>
> >>> +     if (fn) {
> >>> +             dev_err(&epc->dev, "This endpoint controller only supports a single physical function\n");
> >>> +             return -EINVAL;
> >>> +     }
> >>
> >> Checking this here is late... Given that at most only one physical
> >> function is supported, the check should be in rockchip_pcie_parse_ep_dt().
> >> Something like:
> >>
> >>         err = of_property_read_u8(dev->of_node, "max-functions",
> >>                                   &ep->epc->max_functions);
> >>
> >>         if (err < 0 || ep->epc->max_functions > 1)
> >>
> >>                 ep->epc->max_functions = 1;
> >>
> >
> > Yes, this could be moved to the probe, thanks.
> >
> >> And all the macros with the (fn) argument could also be simplified
> >> (argument fn removed) since fn will always be 0.
> >
> > These functions cannot be simplified because they have to follow the signature
> > given by "pci_epc_ops" (include/linux/pci-epc.h). And this signature has the
> > function number as a parameter. If we change the function signature we won't
> > be able to assign these functions to the pc_epc_ops structure
>
> I was not suggesting to change the functions signature. I was suggesting
> dropping the fn argument for the *macros*, e.g.
>
> ROCKCHIP_PCIE_EP_FUNC_BASE(fn) -> ROCKCHIP_PCIE_EP_FUNC_BASE
>
> since fn is always 0.
>
> That said, I am not entirely sure if the limit really is 1 function at most. The
> TRM seems to be suggesting that up to 4 functions can be supported...
>
> [...]
>
> >> Another nice cleanup: define ROCKCHIP_PCIE_EP_MSI_CTRL_REG to include the
> >> ROCKCHIP_PCIE_EP_FUNC_BASE(fn) addition so that we do not have to do it
> >> here all the time.
> >
> > Yes, this could be an improvement but this is the way it is written
> > everywhere in this
> > driver, I chose to keep it so as to remain coherent with the rest of the driver.
> > Cleaning this is not so important since this code will not be
> > rewritten / changed so
> > often. But I agree that it might be nicer. But, on the other side if
> > at some point
> > support for virtual functions would be added, the offsets would need
> > to be computed
> > based on the virtual function number and the code would be written
> > like it is now,
> > so I suggest keeping this the way it is for now.
>
> Yes, sure, this can be cleaned later.
>
> A more pressing problem is the lack of support for MSIX despite the fact that
> the controller supports that *and* advertize it as a capability. That is what
> was causing my problem with the Linux nvme driver and my prototype nvme epf
> function driver: the host driver was seeing MSIX support (1 vector supported by
> default), and so was allocating one MSIX for the device probe. But on the EP
> end, it is MSI or INTX only... Working on adding that to solve this issue.
>

I have seen this too, the controller advertises the capability. However, the TRM
(section 17.5.9) says that MSI-X is not supported (MSI / INTx only as you said).
So the solution should be to modify the probe function of the endpoint
controller
so that the MSI-X capability would not be advertised, this should fix
your problem.

I wonder if one could still implement MSI-X because from the endpoint we would
just need to implement it as a message (TLP) over PCIe (because the space for
the vectors is allocated and written, so that part should be ok). I am
not an expert
on MSI-X, but the reason the endpoint cannot send them could be because MSI-X
requires some fields in the PCIe header descriptor to be filled with values that
cannot be set through the "desc0-3" registers of the RK3399 PCIe endpoint core.

Anyways, the endpoint should not advertise the MSI-X capabilities when it cannot
send such interrupts. Once this is fixed you should be able to have your NVMe
function running.

Regards.
Rick


> --
> Damien Le Moal
> Western Digital Research
>