Re: [PATCH net-next v7] net: txgbe: Add build support for txgbe

Re: [PATCH net-next v7] net: txgbe: Add build support for txgbe

[Jakub Kicinski]

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?

[1]
https://lore.kernel.org/all/20220621023209.599386-1-jiawenwu@trustnetic.com/

Re: [PATCH net-next v7] net: txgbe: Add build support for txgbe

[Bjorn Helgaas]

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@trustnetic.com/