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

[Rick Wertenbroek]

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

static const struct pci_epc_ops rockchip_pcie_epc_ops = {
       .write_header = rockchip_pcie_ep_write_header,
       .set_bar = rockchip_pcie_ep_set_bar,
       .clear_bar = rockchip_pcie_ep_clear_bar,
       .map_addr = rockchip_pcie_ep_map_addr,
       .unmap_addr = rockchip_pcie_ep_unmap_addr,
       .set_msi = rockchip_pcie_ep_set_msi,
       .get_msi = rockchip_pcie_ep_get_msi,
       .raise_irq = rockchip_pcie_ep_raise_irq,
       .start = rockchip_pcie_ep_start,
       .get_features = rockchip_pcie_ep_get_features,
};

>
> > +
> > +     if (mmc > 0x5) {
> > +             dev_err(&epc->dev, "Number of MSI IRQs cannot be more than 32\n");
>
> Long line. Please split it after the comma.
>
> > +             return -EINVAL;
> > +     }
> > +
> >       flags = rockchip_pcie_read(rockchip,
> >                                  ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
> >                                  ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
>
> 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.

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

Thank you for your comments.

Regards
Rick