On Tue, Oct 06, 2020 at 01:24:28PM -0500, Sanjay R Mehta wrote:
> if DL_ACTIVE bit is set it means that there is no need to check
> PCI_EXP_LNKSTA_LT bit, as DL_ACTIVE would have set only if the link
> is already trained. Hence adding a check which takes care of this
> scenario.
Sorry for being dense but I don't understand this at all:
The PCI_EXP_DPC_CAP_DL_ACTIVE bit which you check here indicates
that the port is capable of sending an ERR_COR interrupt whenever
the link transitions from inactive to active.
What is the connection to the PCI_EXP_LNKSTA_LT bit (which indicates
that the link is still being trained)?
Also, the negation of a bitwise AND is always either 0 or 1
(!(lnk_status & PCI_EXP_DPC_CAP_DL_ACTIVE)), so bit 0 is set or not set.
However PCI_EXP_LNKSTA_LT is bit 11. A bitwise AND of bit 11 and 0 is
always 0, so the expression can never be 1.
Am I missing something?
Thanks,
Lukas
> Signed-off-by: Sanjay R Mehta <[email protected]>
> ---
> drivers/pci/hotplug/pciehp_hpc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 53433b3..81d1348 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -309,7 +309,8 @@ int pciehp_check_link_status(struct controller *ctrl)
>
> pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
> ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
> - if ((lnk_status & PCI_EXP_LNKSTA_LT) ||
> + if (((lnk_status & PCI_EXP_LNKSTA_LT) &
> + !(lnk_status & PCI_EXP_DPC_CAP_DL_ACTIVE)) ||
> !(lnk_status & PCI_EXP_LNKSTA_NLW)) {
> ctrl_err(ctrl, "link training error: status %#06x\n",
> lnk_status);
> --
> 2.7.4
On 10/7/2020 1:08 AM, Lukas Wunner wrote:
> [CAUTION: External Email]
>
> On Tue, Oct 06, 2020 at 01:24:28PM -0500, Sanjay R Mehta wrote:
>> if DL_ACTIVE bit is set it means that there is no need to check
>> PCI_EXP_LNKSTA_LT bit, as DL_ACTIVE would have set only if the link
>> is already trained. Hence adding a check which takes care of this
>> scenario.
>
> Sorry for being dense but I don't understand this at all:
>
> The PCI_EXP_DPC_CAP_DL_ACTIVE bit which you check here indicates
> that the port is capable of sending an ERR_COR interrupt whenever
> the link transitions from inactive to active.
>
> What is the connection to the PCI_EXP_LNKSTA_LT bit (which indicates
> that the link is still being trained)?
>
> Also, the negation of a bitwise AND is always either 0 or 1
> (!(lnk_status & PCI_EXP_DPC_CAP_DL_ACTIVE)), so bit 0 is set or not set.
> However PCI_EXP_LNKSTA_LT is bit 11. A bitwise AND of bit 11 and 0 is
> always 0, so the expression can never be 1.
>
> Am I missing something?
>
Please accept my sincere apologies for sending the wrong patch.
I am supposed to use PCI_EXP_LNKSTA_DLLLA bit in my patch but have used PCI_EXP_DPC_CAP_DL_ACTIVE.
The correct code should be as below,
- if ((lnk_status & PCI_EXP_LNKSTA_LT) ||
+ if (((lnk_status & PCI_EXP_LNKSTA_LT) &
+ !(lnk_status & PCI_EXP_LNKSTA_DLLLA )) ||
Is it right? please share your feedback, if I am wrong. Will send out V2 patch, once you confirm on this.
> Thanks,
>
> Lukas
>
>> Signed-off-by: Sanjay R Mehta <[email protected]>
>> ---
>> drivers/pci/hotplug/pciehp_hpc.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>> index 53433b3..81d1348 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -309,7 +309,8 @@ int pciehp_check_link_status(struct controller *ctrl)
>>
>> pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
>> ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
>> - if ((lnk_status & PCI_EXP_LNKSTA_LT) ||
>> + if (((lnk_status & PCI_EXP_LNKSTA_LT) &
>> + !(lnk_status & PCI_EXP_DPC_CAP_DL_ACTIVE)) ||
>> !(lnk_status & PCI_EXP_LNKSTA_NLW)) {
>> ctrl_err(ctrl, "link training error: status %#06x\n",
>> lnk_status);
>> --
>> 2.7.4
On Thu, Oct 08, 2020 at 12:43:17PM +0530, Sanjay R Mehta wrote:
> On 10/7/2020 1:08 AM, Lukas Wunner wrote:
> > On Tue, Oct 06, 2020 at 01:24:28PM -0500, Sanjay R Mehta wrote:
> >> if DL_ACTIVE bit is set it means that there is no need to check
> >> PCI_EXP_LNKSTA_LT bit, as DL_ACTIVE would have set only if the link
> >> is already trained. Hence adding a check which takes care of this
> >> scenario.
> >
> > Sorry for being dense but I don't understand this at all:
> >
> > The PCI_EXP_DPC_CAP_DL_ACTIVE bit which you check here indicates
> > that the port is capable of sending an ERR_COR interrupt whenever
> > the link transitions from inactive to active.
> >
> > What is the connection to the PCI_EXP_LNKSTA_LT bit (which indicates
> > that the link is still being trained)?
> >
> > Also, the negation of a bitwise AND is always either 0 or 1
> > (!(lnk_status & PCI_EXP_DPC_CAP_DL_ACTIVE)), so bit 0 is set or not set.
> > However PCI_EXP_LNKSTA_LT is bit 11. A bitwise AND of bit 11 and 0 is
> > always 0, so the expression can never be 1.
> >
> > Am I missing something?
> >
> Please accept my sincere apologies for sending the wrong patch.
>
> I am supposed to use PCI_EXP_LNKSTA_DLLLA bit in my patch but have used PCI_EXP_DPC_CAP_DL_ACTIVE.
>
> The correct code should be as below,
>
> - if ((lnk_status & PCI_EXP_LNKSTA_LT) ||
> + if (((lnk_status & PCI_EXP_LNKSTA_LT) &
> + !(lnk_status & PCI_EXP_LNKSTA_DLLLA )) ||
>
> Is it right? please share your feedback, if I am wrong. Will send out V2 patch, once you confirm on this.
At least you are ignoring LKP valid warning...
--
With Best Regards,
Andy Shevchenko
On Thu, Oct 08, 2020 at 12:43:17PM +0530, Sanjay R Mehta wrote:
> On 10/7/2020 1:08 AM, Lukas Wunner wrote:
> > On Tue, Oct 06, 2020 at 01:24:28PM -0500, Sanjay R Mehta wrote:
> I am supposed to use PCI_EXP_LNKSTA_DLLLA bit in my patch but have
> used PCI_EXP_DPC_CAP_DL_ACTIVE.
>
> The correct code should be as below,
>
> - if ((lnk_status & PCI_EXP_LNKSTA_LT) ||
> + if (((lnk_status & PCI_EXP_LNKSTA_LT) &
> + !(lnk_status & PCI_EXP_LNKSTA_DLLLA )) ||
So you want to ignore a set Link Training bit if the DLLLA bit is also
set (i.e. the link is up). Why do you need this? Is there broken AMD
hardware which fails to clear the Link Training bit when the LTSSM
exits the Configuration/Recovery state?
Again, please note that you need && instead of &.
Thanks,
Lukas
> >> Signed-off-by: Sanjay R Mehta <[email protected]>
> >> ---
> >> drivers/pci/hotplug/pciehp_hpc.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> >> index 53433b3..81d1348 100644
> >> --- a/drivers/pci/hotplug/pciehp_hpc.c
> >> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> >> @@ -309,7 +309,8 @@ int pciehp_check_link_status(struct controller *ctrl)
> >>
> >> pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
> >> ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
> >> - if ((lnk_status & PCI_EXP_LNKSTA_LT) ||
> >> + if (((lnk_status & PCI_EXP_LNKSTA_LT) &
> >> + !(lnk_status & PCI_EXP_DPC_CAP_DL_ACTIVE)) ||
> >> !(lnk_status & PCI_EXP_LNKSTA_NLW)) {
> >> ctrl_err(ctrl, "link training error: status %#06x\n",
> >> lnk_status);
> >> --
> >> 2.7.4