linux-kernel - Re: [PATCH] PCI: pciehp: Report degraded links via link bandwidth notification

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 29 Nov 2018 19:00:58 +0000
From:   <Alex_Gagniuc@...lteam.com>
To:     <mika.westerberg@...ux.intel.com>, <mr.nuke.me@...il.com>
Cc:     <bhelgaas@...gle.com>, <Austin.Bolen@...l.com>,
        <keith.busch@...el.com>, <Shyam.Iyer@...l.com>, <lukas@...ner.de>,
        <okaya@...eaurora.org>, <rafael.j.wysocki@...el.com>,
        <poza@...eaurora.org>, <linux-pci@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] PCI: pciehp: Report degraded links via link bandwidth
 notification

On 11/29/2018 10:06 AM, Mika Westerberg wrote:
>> @@ -515,7 +515,8 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
>>   	struct controller *ctrl = (struct controller *)dev_id;
>>   	struct pci_dev *pdev = ctrl_dev(ctrl);
>>   	struct device *parent = pdev->dev.parent;
>> -	u16 status, events;
>> +	struct pci_dev *endpoint;
>> +	u16 status, events, link_status;
> 
> Looks better if you write them in opposite order (reverse xmas-tree):
> 
> 	u16 status, events, link_status;
> 	struct pci_dev *endpoint;
> 

I don't decorate in November :p

>> +	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &link_status);
>> +
> 
> Unnecessary empty line.

However Bjorn wants it, though I don't like the crowded look with this 
line removed.

>> +	if (link_status & PCI_EXP_LNKSTA_LBMS) {
>> +		if (pdev->subordinate && pdev->subordinate->self)
>> +			endpoint = pdev->subordinate->self;
> 
> Hmm, I thought pdev->subordinate->self == pdev, no?

That makes no sense, but I think you're right. I'm trying to get to the 
other end of the PCIe link. Is there a simple way to do that? (other 
than convoluted logic that all leads to the same mistake)

>>   static void pcie_enable_notification(struct controller *ctrl)
>>   {
>>   	u16 cmd, mask;
>> @@ -713,6 +743,9 @@ static void pcie_enable_notification(struct controller *ctrl)
>>   	pcie_write_cmd_nowait(ctrl, cmd, mask);
>>   	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
>>   		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, cmd);
>> +
>> +	if (pcie_link_bandwidth_notification_supported(ctrl))
>> +		pcie_enable_link_bandwidth_notification(ctrl);
> 
> Do we ever need to disable it?

I can't think of a case where that would be needed.

Alex