On Tue, Jun 21, 2022 at 10:33:15PM -0700, Jakub Kicinski wrote:
> On Tue, 21 Jun 2022 10:32:09 +0800 Jiawen Wu wrote:
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -5942,3 +5942,18 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b1, aspm_l1_acceptable_latency
> > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, aspm_l1_acceptable_latency);
> > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, aspm_l1_acceptable_latency);
> > #endif
> > +
> > +static void quirk_wangxun_set_read_req_size(struct pci_dev *pdev)
> > +{
> > + u16 ctl;
> > +
> > + pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &ctl);
> > +
> > + if (((ctl & PCI_EXP_DEVCTL_READRQ) != PCI_EXP_DEVCTL_READRQ_128B) &&
> > + ((ctl & PCI_EXP_DEVCTL_READRQ) != PCI_EXP_DEVCTL_READRQ_256B))
> > + pcie_capability_clear_and_set_word(pdev, PCI_EXP_DEVCTL,
> > + PCI_EXP_DEVCTL_READRQ,
> > + PCI_EXP_DEVCTL_READRQ_256B);
> > +}
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WANGXUN, PCI_ANY_ID,
> > + quirk_wangxun_set_read_req_size);
>
> Hi Bjorn! Other than the fact that you should obviously have been CCed
> on the patch [1] - what are the general rules on the quirks? Should
> this be sent separately to your PCI tree?
This is a little bit ugly because the PCI core assumes that it
controls PCI_EXP_DEVCTL_READRQ (Max_Read_Request_Size / MRRS) and uses
it as part of the hierarchy-wide strategy for managing
Max_Payload_Size / MPS.
This is all in pcie_bus_configure_settings() and is called after
enumerating all devices, so I think it happens *after* all the quirks
have been run. So whatever this quirk does might be overwritten by
pcie_bus_configure_settings().
I assume wangxun needs to set MRRS to 128 or 256 bytes. The power-up
default is supposed to be 512 bytes, and pcie_bus_configure_settings()
may choose something else. There are some drivers that call
pcie_set_readrq() from their probe functions, and that's probably what
you should do, too.
I do see that quirk_brcm_5719_limit_mrrs() does this as a quirk after
0b471506712d ("tg3: Recode PCI MRRS adjustment as a PCI quirk"), but I
don't think that is reliable. Apparently it *used* to be done during
probe, and I don't know why it was changed to be a quirk.
> [1]
> https://lore.kernel.org/all/20220621023209.599386-1-jiawenwu@xxxxxxxxxxxxxx/
