Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230705200617.GA72825@bhelgaas> <9d1095ab-23e5-3df3-58d6-b2974f87ee72@amd.com>
 <CAAd53p7L27dkzwb_Q9vhENhBye-JTcx2AuCG_YXAgb0F6MG-9w@mail.gmail.com> <60b2f5fb-8294-d104-16d8-0acfc70426c1@amd.com>
In-Reply-To: <60b2f5fb-8294-d104-16d8-0acfc70426c1@amd.com>
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
Date:   Mon, 17 Jul 2023 11:34:10 +0800
Message-ID: <CAAd53p42jiTCOsRZwEY0jtBejMDs1FbTOBNEknijnVNk3ENxuA@mail.gmail.com>
Subject: Re: [Intel-wired-lan] [PATCH] PCI/ASPM: Enable ASPM on external PCIe devices
To:     Mario Limonciello <mario.limonciello@amd.com>
Cc:     Bjorn Helgaas <helgaas@kernel.org>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        linux-pci@vger.kernel.org,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
        linux-kernel@vger.kernel.org, Vidya Sagar <vidyas@nvidia.com>,
        Michael Bottini <michael.a.bottini@linux.intel.com>,
        intel-wired-lan@osuosl.org, bhelgaas@google.com,
        Mika Westerberg <mika.westerberg@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Jul 15, 2023 at 12:37=E2=80=AFAM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 7/14/23 03:17, Kai-Heng Feng wrote:
> > On Thu, Jul 6, 2023 at 12:07=E2=80=AFPM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> On 7/5/23 15:06, Bjorn Helgaas wrote:
> >>> On Wed, Jun 28, 2023 at 01:09:49PM +0800, Kai-Heng Feng wrote:
> >>>> On Wed, Jun 28, 2023 at 4:54=E2=80=AFAM Bjorn Helgaas <helgaas@kerne=
l.org> wrote:
> >>>>> On Tue, Jun 27, 2023 at 04:35:25PM +0800, Kai-Heng Feng wrote:
> >>>>>> On Fri, Jun 23, 2023 at 7:06=E2=80=AFAM Bjorn Helgaas <helgaas@ker=
nel.org> wrote:
> >>>>>>> On Tue, Jun 20, 2023 at 01:36:59PM -0500, Limonciello, Mario wrot=
e:
> >>>
> >>>>> It's perfectly fine for the IP to support PCI features that are not
> >>>>> and can not be enabled in a system design.  But I expect that
> >>>>> strapping or firmware would disable those features so they are not
> >>>>> advertised in config space.
> >>>>>
> >>>>> If BIOS leaves features disabled because they cannot work, but at t=
he
> >>>>> same time leaves them advertised in config space, I'd say that's a
> >>>>> BIOS defect.  In that case, we should have a DMI quirk or something=
 to
> >>>>> work around the defect.
> >>>>
> >>>> That means most if not all BIOS are defected.
> >>>> BIOS vendors and ODM never bothered (and probably will not) to chang=
e
> >>>> the capabilities advertised by config space because "it already work=
s
> >>>> under Windows".
> >>>
> >>> This is what seems strange to me.  Are you saying that Windows never
> >>> enables these power-saving features?  Or that Windows includes quirks
> >>> for all these broken BIOSes?  Neither idea seems very convincing.
> >>>
> >>
> >> I see your point.  I was looking through Microsoft documentation for
> >> hints and came across this:
> >>
> >> https://learn.microsoft.com/en-us/windows-hardware/customize/power-set=
tings/pci-express-settings-link-state-power-management
> >>
> >> They have a policy knob to globally set L0 or L1 for PCIe links.
> >>
> >> They don't explicitly say it, but surely it's based on what the device=
s
> >> advertise in the capabilities registers.
> >
> > So essentially it can be achieved via boot time kernel parameter
> > and/or sysfs knob.
> >
> > The main point is OS should stick to the BIOS default, which is the
> > only ASPM setting tested before putting hardware to the market.
>
> Unfortunately; I don't think you can jump to this conclusion.
>
> A big difference in the Windows world to Linux world is that OEMs ship
> with a factory Windows image that may set policies like this.  OEM
> "platform" drivers can set registry keys too.

Thanks. This is new to me.

>
> I think the next ASPM issue that comes up what we (collectively) need to
> do is compare ASPM policy and PCI registers for:
> 1) A "clean" Windows install from Microsoft media before all the OEM
> drivers are installed.
> 2) A Windows install that the drivers have been installed.
> 3) A up to date mainline Linux kernel.
>
> Actually as this thread started for determining policy for external PCIe
> devices, maybe that would be good to check with those.

Did that before submitting the patch.
From very limited devices I tested, ASPM is enabled for external
connected PCIe device via TBT ports.

I wonder if there's any particular modification should be improved for
this patch?

Kai-Heng

>
> >
> > Kai-Heng
> >
> >>
> >>>>>> So the logic is to ignore the capability and trust the default set
> >>>>>> by BIOS.
> >>>>>
> >>>>> I think limiting ASPM support to whatever BIOS configured at boot-t=
ime
> >>>>> is problematic.  I don't think we can assume that all platforms hav=
e
> >>>>> firmware that configures ASPM as aggressively as possible, and
> >>>>> obviously firmware won't configure hot-added devices at all (in
> >>>>> general; I know ACPI _HPX can do some of that).
> >>>>
> >>>> Totally agree. I was not suggesting to limiting the setting at all.
> >>>> A boot-time parameter to flip ASPM setting is very useful. If none h=
as
> >>>> been set, default to BIOS setting.
> >>>
> >>> A boot-time parameter for debugging and workarounds is fine.  IMO,
> >>> needing a boot-time parameter in the course of normal operation is
> >>> not OK.
> >>>
> >>> Bjorn
> >>
>