Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Cc:     <helgaas@kernel.org>, <Austin.Bolen@dell.com>,
        <Shyam.Iyer@dell.com>, <bhelgaas@google.com>,
        <rafael.j.wysocki@intel.com>, <keith.busch@intel.com>,
        <poza@codeaurora.org>, <mika.westerberg@linux.intel.com>,
        <fred@fredlawl.com>, <linux-kernel@vger.kernel.org>,
        <linux-pci@vger.kernel.org>
From:   <Alex_Gagniuc@Dellteam.com>
To:     <lukas@wunner.de>, <mr.nuke.me@gmail.com>
Subject: Re: [PATCH v2] PCI: pciehp: Report degraded links via link bandwidth
 notification
Thread-Topic: [PATCH v2] PCI: pciehp: Report degraded links via link bandwidth
 notification
Thread-Index: AQHUjlm7N2lQKXAHOEmL/Rp7Y7d0Qw==
Date:   Wed, 27 Feb 2019 20:21:58 +0000
Message-ID: <a941304f8cc244adb265aa7c27027938@ausx13mps317.AMER.DELL.COM>
References: <20181129230454.GF178809@google.com>
 <20181207182021.16344-1-mr.nuke.me@gmail.com>
 <20190225022857.awuspuxb4bedirjp@wunner.de>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2/24/19 8:29 PM, Lukas Wunner wrote:=0A=
> On Fri, Dec 07, 2018 at 12:20:00PM -0600, Alexandru Gagniuc wrote:=0A=
> =0A=
> =0A=
>> Q: Why is this unconditionally compiled in?=0A=
>> A: The symmetrical check in pci probe() is also always compiled in.=0A=
> =0A=
> Hm, it looks like the convention is to provide a separate Kconfig entry=
=0A=
> for each port service.=0A=
=0A=
Does the convention still make sense in light of the symmetry reason?=0A=
=0A=
>> Q: Why call module_init() instead of adding a call in pcie_init_services=
() ?=0A=
>> A: A call in pcie_init_services() also requires a prototype in portdrv.h=
, a=0A=
>> non-static implementation in bw_notification.c. Using module_init() is=
=0A=
>> functionally equivalent, and takes less code.=0A=
> =0A=
> Commit c29de84149ab ("PCI: portdrv: Initialize service drivers directly")=
=0A=
> moved away from module_init() on purpose, apparently to fix a race=0A=
> condition.=0A=
=0A=
*GROWL*=0A=
=0A=
=0A=
> What if the link is retrained at the same speed/width?  Intuitively=0A=
> I'd compare the speed in the Link Status Register to what is cached=0A=
> in the cur_bus_speed member of struct pci_bus and print a message=0A=
> only if the speed has changed.  (Don't we need to cache the width as=0A=
> well?)=0A=
=0A=
There are two mechanisms to bring a degraded link back to full BW.=0A=
   1. Secondary bus reset, which results in the device being tore down =0A=
along with our cached speed value.=0A=
   2. Set the PCI_EXP_LNKCTL_LD (Link Disable) bit and clear it. We do =0A=
that as part of the pciehp teardown path. We'd lose our cached value =0A=
just like in the first case.=0A=
=0A=
>> +static irqreturn_t pcie_bw_notification_irq(int irq, void *context)=0A=
>> [...]=0A=
> =0A=
> You need to hold pci_bus_sem [...]=0A=
> This may sleep, so request the IRQ with request_threaded_irq() [...]=0A=
=0A=
Good catch! Thanks!=0A=
All other issues you pointed out should be resolved in next version.=0A=
=0A=
Alex=0A=