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
