* [PATCH] PCI: pciehp: Add check for DL_ACTIVE bit in pciehp_check_link_status()
@ 2020-10-06 18:24 Sanjay R Mehta
2020-10-06 19:38 ` Lukas Wunner
2020-10-07 9:30 ` [kbuild] " Dan Carpenter
0 siblings, 2 replies; 6+ messages in thread
From: Sanjay R Mehta @ 2020-10-06 18:24 UTC (permalink / raw)
To: bhelgaas, lukas, andriy.shevchenko, stuart.w.hayes, mr.nuke.me
Cc: linux-kernel, Sanjay R Mehta
From: Sanjay R Mehta <sanju.mehta@amd.com>
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.
Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: pciehp: Add check for DL_ACTIVE bit in pciehp_check_link_status()
2020-10-06 18:24 [PATCH] PCI: pciehp: Add check for DL_ACTIVE bit in pciehp_check_link_status() Sanjay R Mehta
@ 2020-10-06 19:38 ` Lukas Wunner
2020-10-08 7:13 ` Sanjay R Mehta
2020-10-07 9:30 ` [kbuild] " Dan Carpenter
1 sibling, 1 reply; 6+ messages in thread
From: Lukas Wunner @ 2020-10-06 19:38 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: bhelgaas, andriy.shevchenko, stuart.w.hayes, mr.nuke.me, linux-kernel
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 <sanju.mehta@amd.com>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [kbuild] Re: [PATCH] PCI: pciehp: Add check for DL_ACTIVE bit in pciehp_check_link_status()
2020-10-06 18:24 [PATCH] PCI: pciehp: Add check for DL_ACTIVE bit in pciehp_check_link_status() Sanjay R Mehta
2020-10-06 19:38 ` Lukas Wunner
@ 2020-10-07 9:30 ` Dan Carpenter
1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-10-07 9:30 UTC (permalink / raw)
To: kbuild, Sanjay R Mehta, bhelgaas, lukas, andriy.shevchenko,
stuart.w.hayes, mr.nuke.me
Cc: lkp, kbuild-all, linux-kernel, Sanjay R Mehta
[-- Attachment #1: Type: text/plain, Size: 3759 bytes --]
Hi Sanjay,
url: https://github.com/0day-ci/linux/commits/Sanjay-R-Mehta/PCI-pciehp-Add-check-for-DL_ACTIVE-bit-in-pciehp_check_link_status/20201007-022638
base: 549738f15da0e5a00275977623be199fbbf7df50
config: x86_64-randconfig-m001-20201005 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/pci/hotplug/pciehp_hpc.c:313 pciehp_check_link_status() warn: maybe use && instead of &
vim +313 drivers/pci/hotplug/pciehp_hpc.c
82a9e79ef132cbf Kenji Kaneshige 2009-09-15 290 int pciehp_check_link_status(struct controller *ctrl)
^1da177e4c3f415 Linus Torvalds 2005-04-16 291 {
cd84d34074f5659 Bjorn Helgaas 2013-05-09 292 struct pci_dev *pdev = ctrl_dev(ctrl);
1a84b99ccbb954c Bjorn Helgaas 2013-12-14 293 bool found;
^1da177e4c3f415 Linus Torvalds 2005-04-16 294 u16 lnk_status;
^1da177e4c3f415 Linus Torvalds 2005-04-16 295
f0157160b359b1d Keith Busch 2018-09-20 296 if (!pcie_wait_for_link(pdev, true))
f0157160b359b1d Keith Busch 2018-09-20 297 return -1;
f18e9625e02bb3e Kenji Kaneshige 2008-10-22 298
f496648b99f8f7f Alexandru Gagniuc 2019-10-25 299 if (ctrl->inband_presence_disabled)
f496648b99f8f7f Alexandru Gagniuc 2019-10-25 300 pcie_wait_for_presence(pdev);
f496648b99f8f7f Alexandru Gagniuc 2019-10-25 301
2f5d8e4ff947ad6 Yinghai Lu 2012-01-27 302 found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
2f5d8e4ff947ad6 Yinghai Lu 2012-01-27 303 PCI_DEVFN(0, 0));
0027cb3e1947d0f Kenji Kaneshige 2011-11-10 304
6c35a1ac3da63a7 Lukas Wunner 2018-07-19 305 /* ignore link or presence changes up to this point */
6c35a1ac3da63a7 Lukas Wunner 2018-07-19 306 if (found)
6c35a1ac3da63a7 Lukas Wunner 2018-07-19 307 atomic_and(~(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC),
6c35a1ac3da63a7 Lukas Wunner 2018-07-19 308 &ctrl->pending_events);
6c35a1ac3da63a7 Lukas Wunner 2018-07-19 309
1a84b99ccbb954c Bjorn Helgaas 2013-12-14 310 pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
7f2feec140f1f1e Taku Izumi 2008-09-05 311 ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
818dc520d6f8472 Sanjay R Mehta 2020-10-06 312 if (((lnk_status & PCI_EXP_LNKSTA_LT) &
^
This won't ever be true. PCI_EXP_LNKSTA_LT is 0x0800. It needs to be
&&.
818dc520d6f8472 Sanjay R Mehta 2020-10-06 @313 !(lnk_status & PCI_EXP_DPC_CAP_DL_ACTIVE)) ||
322162a71bd9fc4 Kenji Kaneshige 2008-12-19 314 !(lnk_status & PCI_EXP_LNKSTA_NLW)) {
3784e0c6b02d4fa Bjorn Helgaas 2015-06-15 315 ctrl_err(ctrl, "link training error: status %#06x\n",
3784e0c6b02d4fa Bjorn Helgaas 2015-06-15 316 lnk_status);
1a84b99ccbb954c Bjorn Helgaas 2013-12-14 317 return -1;
^1da177e4c3f415 Linus Torvalds 2005-04-16 318 }
^1da177e4c3f415 Linus Torvalds 2005-04-16 319
fdbd3ce9efb3a04 Yinghai Lu 2011-11-07 320 pcie_update_link_speed(ctrl->pcie->port->subordinate, lnk_status);
fdbd3ce9efb3a04 Yinghai Lu 2011-11-07 321
1a84b99ccbb954c Bjorn Helgaas 2013-12-14 322 if (!found)
1a84b99ccbb954c Bjorn Helgaas 2013-12-14 323 return -1;
2f5d8e4ff947ad6 Yinghai Lu 2012-01-27 324
1a84b99ccbb954c Bjorn Helgaas 2013-12-14 325 return 0;
^1da177e4c3f415 Linus Torvalds 2005-04-16 326 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37370 bytes --]
[-- Attachment #3: Type: text/plain, Size: 149 bytes --]
_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: pciehp: Add check for DL_ACTIVE bit in pciehp_check_link_status()
2020-10-06 19:38 ` Lukas Wunner
@ 2020-10-08 7:13 ` Sanjay R Mehta
2020-10-08 11:29 ` Andy Shevchenko
2020-10-09 10:34 ` Lukas Wunner
0 siblings, 2 replies; 6+ messages in thread
From: Sanjay R Mehta @ 2020-10-08 7:13 UTC (permalink / raw)
To: Lukas Wunner, Sanjay R Mehta
Cc: bhelgaas, andriy.shevchenko, stuart.w.hayes, mr.nuke.me, linux-kernel
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 <sanju.mehta@amd.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: pciehp: Add check for DL_ACTIVE bit in pciehp_check_link_status()
2020-10-08 7:13 ` Sanjay R Mehta
@ 2020-10-08 11:29 ` Andy Shevchenko
2020-10-09 10:34 ` Lukas Wunner
1 sibling, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2020-10-08 11:29 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: Lukas Wunner, Sanjay R Mehta, bhelgaas, stuart.w.hayes,
mr.nuke.me, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: pciehp: Add check for DL_ACTIVE bit in pciehp_check_link_status()
2020-10-08 7:13 ` Sanjay R Mehta
2020-10-08 11:29 ` Andy Shevchenko
@ 2020-10-09 10:34 ` Lukas Wunner
1 sibling, 0 replies; 6+ messages in thread
From: Lukas Wunner @ 2020-10-09 10:34 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: Sanjay R Mehta, bhelgaas, andriy.shevchenko, stuart.w.hayes,
mr.nuke.me, linux-kernel
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 <sanju.mehta@amd.com>
> >> ---
> >> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-09 10:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-06 18:24 [PATCH] PCI: pciehp: Add check for DL_ACTIVE bit in pciehp_check_link_status() Sanjay R Mehta
2020-10-06 19:38 ` Lukas Wunner
2020-10-08 7:13 ` Sanjay R Mehta
2020-10-08 11:29 ` Andy Shevchenko
2020-10-09 10:34 ` Lukas Wunner
2020-10-07 9:30 ` [kbuild] " Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).