Re: [PATCH 1/2] PCI: Fix the wrong reading of register fields

[Jiwei]

On 1/10/25 22:06, Ilpo Järvinen wrote:
> On Fri, 10 Jan 2025, Jiwei Sun wrote:
>
> > From: Jiwei Sun <sunjw10@xxxxxxxxxx>
> >
> > Since commit de9a6c8d5dbf ("PCI/bwctrl: Add pcie_set_target_speed() to set
> > PCIe Link Speed"), there are two potential issues in the function
> > pcie_failed_link_retrain().
> >
> > (1) The macro PCIE_LNKCTL2_TLS2SPEED() and PCIE_LNKCAP_SLS2SPEED() just
> > uses the link speed field of the registers. However, there are many other
> > different function fields in the Link Control 2 Register or the Link
> > Capabilities Register. If the register value is directly used by the two
> > macros, it may cause getting an error link speed value (PCI_SPEED_UNKNOWN).
> >
> > (2) In the pcie_failed_link_retrain(), the local variable lnkctl2 is not
> > changed after reading from PCI_EXP_LNKCTL2. It might cause that the
> > removing 2.5GT/s downstream link speed restriction codes are not executed.
>
> Thanks for finding these issues and coming up with a patch.
>
> These cases seem two different problems to me so I'd put them into
> different patches, which would also make it easeir focus on
> describing the issue in case 2 which is currently a bit vague (when
> looking how de9a6c8d5dbf managed to break it by removing the lnkctl2
> change prior to writing into the reg).

Thanks for your suggestion. I will divide them into different patches.

> > In order to avoid the above-mentioned potential issues, only keep link
> > speed field of the two registers before using by pcie_set_target_speed()
> > and reread the Link Control 2 Register before using.
> >
> > Fixes: de9a6c8d5dbf ("PCI/bwctrl: Add pcie_set_target_speed() to set PCIe Link Speed")
> > Signed-off-by: Jiwei Sun <sunjw10@xxxxxxxxxx>
> > ---
> >   drivers/pci/quirks.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 76f4df75b08a..605628c810a5 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -118,11 +118,13 @@ int pcie_failed_link_retrain(struct pci_dev *dev)
> >   		ret = pcie_set_target_speed(dev, PCIE_SPEED_2_5GT, false);
> >   		if (ret) {
> >   			pci_info(dev, "retraining failed\n");
> > +			oldlnkctl2 &= PCI_EXP_LNKCTL2_TLS;
> >   			pcie_set_target_speed(dev, PCIE_LNKCTL2_TLS2SPEED(oldlnkctl2),
> >   					      true);
>
> I'd prefer these get fixed inside the macros so that the callers don't
> need to take handle what seem something that is always needed.


It is a good idea,  I will do that in the following patch.


Thanks,
Regards,
Jiwei

> >   			return ret;
> >   		}
> >   
> > +		pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &lnkctl2);
> >   		pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
> >   	}
> >   
> > @@ -133,6 +135,7 @@ int pcie_failed_link_retrain(struct pci_dev *dev)
> >   
> >   		pci_info(dev, "removing 2.5GT/s downstream link speed restriction\n");
> >   		pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> > +		lnkcap &= PCI_EXP_LNKCAP_SLS;
> >   		ret = pcie_set_target_speed(dev, PCIE_LNKCAP_SLS2SPEED(lnkcap), false);