Received-SPF: pass (google.com: domain of linux-kernel+bounces-161-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231214063717.992-1-mattc@purestorage.com> <20231214202856.GA1101963@bhelgaas>
 <CAPYCUs41ZfLUCLAYqfjyzXoDELkt1+6nMz6U68FAOx9TXoCbYQ@mail.gmail.com>
In-Reply-To: <CAPYCUs41ZfLUCLAYqfjyzXoDELkt1+6nMz6U68FAOx9TXoCbYQ@mail.gmail.com>
Reply-To: bjorn@helgaas.com
From: Bjorn Helgaas <bjorn.helgaas@gmail.com>
Date: Thu, 14 Dec 2023 15:47:04 -0600
Message-ID: <CABhMZUX9UVkyq5nx6qFuKJRjzHm_suRXyN=rWgqsYQ4cgV0f_A@mail.gmail.com>
Subject: Re: [PATCH v3] PCI/portdrv: Allow AER service only for Root Ports & RCECs
To: Matthew Carlis <mattc@purestorage.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>, bhelgaas@google.com, linux-kernel@vger.kernel.org, 
	linux-pci@vger.kernel.org, mika.westerberg@linux.intel.com, 
	sathyanarayanan.kuppuswamy@linux.intel.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Dec 14, 2023 at 3:22=E2=80=AFPM Matthew Carlis <mattc@purestorage.c=
om> wrote:
>
> Hi Bjorn.
>
> Thank you for the quick response, it looks correct that this is first in =
v6.2.  My thinking is that the kernel should use DPC on a switch port if it=
 would use it on a root port when dpc-native is not set.  I would be happy =
to post a formal patch for this.  Maybe using host->native_aer is the corre=
ct way to ensure that the kernel in this system will be using AER, maybe no=
t on this device but it will on some device. Then, we can proceed to use DP=
C on the device.
>
> I will submit something in the next few days here.

I think your message got rejected from the mailing lists because they
only accept plain-text email:
http://vger.kernel.org/majordomo-info.html

More importantly, I forgot that there is a native_dpc flag, too, so
it's not clear that testing native_aer is the right thing here.  If
native_aer *is* the right thing, it would certainly need a comment
because it would look like a typo.  I haven't investigated enough to
know what the right answer is.

Bjorn

> On Thu, Dec 14, 2023 at 12:29=E2=80=AFPM Bjorn Helgaas <helgaas@kernel.or=
g> wrote:
>>
>> On Wed, Dec 13, 2023 at 11:37:17PM -0700, Matthew W Carlis wrote:
>> > Hello Any Interested
>> >
>> > Recently found that this patch had the affect of requiring us to set
>> > pcie_ports_dpc_native in order to use the kernel DPC driver with PCIe =
switch
>> > downstream ports. The kernel check for the DPC capability in portdrv.c=
 has;
>> > if pci_aer_available() and (dpc-native or using AER port service drive=
r on
>> > the device). I wonder if we couldn't do away with the requirement of t=
he
>> > AER service being used on the port if pci_aer_available() & host->nati=
ve_aer
>> > don't lie. I'm still trying to decide exactly what the condition ought=
 to
>> > look like, but it might draw from the AER service check above it. For =
example:
>> >
>> >         if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
>> > -           pci_aer_available() &&
>> > -           (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AE=
R)))
>> > +           dev->aer_cap && pci_aer_available() &&
>> > +           (pcie_ports_dpc_native || host->native_aer))
>> >                 services |=3D PCIE_PORT_SERVICE_DPC;
>>
>> This sounds like it might be a regression report for d8d2b65a940b
>> ("PCI/portdrv: Allow AER service only for Root Ports & RCECs"), which
>> appeared in v6.2.  Is that true?
>>
>> If d8d2b65a940b requires you to use the "pcie_ports=3Ddpc-native" kernel
>> parameter when you didn't need it before, that sounds like a
>> regression.
>>
>> Looking at the code, that "services & PCIE_PORT_SERVICE_AER"
>> definitely looks like a problem.  We added that with
>> https://git.kernel.org/linus/4e5fad429bd1 ("PCI/DPC: Do not enable DPC
>> if AER control is not allowed by the BIOS"), but I think your
>> suggestion of checking host->native_aer is better.
>>
>> Do you want to post a formal patch for it?
>>
>> Bjorn