[PATCH v2 0/3] PCI: Fix and improve default MPS configuration

[PATCH v2 0/3] PCI: Fix and improve default MPS configuration

[Niklas Cassel]

This series reworks Hans's series "PCI: Configure Root Port MPS during
host probing" (v9, link below).  Besides configuring Root Ports, which
that series was about, it fixes other problems in how mainline programs
the Maximum Payload Size (MPS), and lists the problems it doesn't fix.

It is based on pci/next, which has the first patch of the RFC (link
below), "PCI: Update saved Max Payload Size in pcie_set_mps()".

The changes compared to v9, including the commit messages and this
cover letter, were developed with the help of an LLM, based on its
review of v9 and of the sashiko-bot reports, and reviewed by me.

Background
==========

A PCIe device must not send a TLP with a payload larger than the MPS it
is programmed with, and a receiver treats a TLP larger than its own MPS
as Malformed, so all devices on a path need compatible MPS settings.

Unless a "pci=pcie_bus_*" option is given, the kernel uses the default
strategy, PCIE_BUS_DEFAULT: pci_configure_mps() programs each device, as
it is enumerated, to the MPS of its upstream bridge.  If the device's MPS
Supported (MPSS) is smaller and the upstream bridge is a Root Port, the
Root Port is lowered instead (9f0e89359775).  The other strategies either
configure whole hierarchies in pcie_bus_configure_settings()
(pcie_bus_safe, pcie_bus_perf, pcie_bus_peer2peer) or don't touch MPS at
all (pcie_bus_tune_off).

Problems found in mainline
==========================

Enumeration:

1. Root Ports keep whatever MPS they came up with.  Fixed by patch 2.

   Root Ports have no upstream bridge, so pci_configure_mps() never
   configures them, and every hierarchy inherits the MPS that firmware or
   the hardware reset left in its Root Port.  On platforms whose firmware
   doesn't configure MPS, which is typical for DT-based systems with
   native host controller drivers, every hierarchy runs at the 128-byte
   reset value even if the Root Port and all devices below it support
   more, at a cost in throughput.  Host controller drivers work around
   this: the Meson driver programs its Root Port to 256 bytes itself,
   regardless of what the devices below it support, which patch 3
   removes.

2. Lowering the MPS only works directly below a Root Port.  Fixed by
   patch 1.

   - Below a Switch, the Switch ports keep the larger MPS, and a device
     that supports less is left mismatched with a "use
     pci=pcie_bus_safe" warning.  This happens at boot whenever firmware
     or a host controller driver programmed a larger MPS than a device
     below a Switch supports.  It has been a known limitation since the
     default strategy was introduced (27d868b5e6cf).

   - For a multi-function device directly below a Root Port, function 0
     is programmed to the Root Port's MPS first.  If function 1 supports
     less, the Root Port is lowered, but function 0 is left at the larger
     value and can send TLPs larger than the Root Port now accepts.  Only
     an informational message about the Root Port is printed.  This was
     introduced by 9f0e89359775.

Hotplug and rescan:

3. A device hot-added directly below a Root Port.  Fixed by patches 1
   and 2.

   If the device supports less than the Root Port's MPS, the Root Port
   is lowered, with the multi-function problem from 2.  If it supports
   more, it is only matched to the Root Port's current MPS: a Root Port
   is never raised again, e.g. after a device that supported less has
   been replaced, and without firmware MPS setup it stays at 128 bytes.
   Devices found by a rescan below an empty Root Port slot behave the
   same.

4. A Switch hot-added directly below a Root Port.  Fixed by patches 1
   and 2.

   The Upstream Port can lower the Root Port, but the Downstream Ports
   and the devices below them are only matched to their upstream bridge,
   and any of them that supports less is left mismatched with a warning.
   With patch 1, nothing below the Root Port is in use yet, so the whole
   new hierarchy is lowered.  If the new Switch has hotplug-capable
   ports, patch 2 then limits the hierarchy to 128 bytes, so devices
   hot-added below it later can match (5).

5. A device hot-added below a Switch.  Fixed by patches 1 and 2, unless
   endpoints below the Root Port are already in use (7).

   Only a Root Port is ever lowered, so a device that supports less than
   the MPS of the Switch ports, e.g. less than what firmware programmed,
   is left mismatched with a warning.  Patch 2 limits hierarchies with
   hotplug-capable Switch ports to 128 bytes, like pcie_bus_safe does, so
   devices hot-added below such ports always match.  Below a Switch
   without hotplug support, e.g. when a device is found by a rescan,
   patch 1 lowers the whole hierarchy as long as no endpoint below the
   Root Port is in use, since the Switch ports themselves don't do DMA.

6. A Switch hot-added below another Switch.  Fixed by patches 1 and 2,
   unless endpoints below the Root Port are already in use (7).

   Same as 5, for every port and device of the new Switch.  With patch
   2, a hierarchy whose Switch has hotplug-capable ports, e.g. a chain of
   Thunderbolt devices, already runs at 128 bytes, so the new Switch can
   match.  Below a Switch without hotplug support, patch 1 lowers the
   hierarchy as in 5.

7. A device added next to endpoints already in use below the same Root
   Port, e.g. a new function found by a rescan while function 0 is
   bound, or a device below one Downstream Port of a Switch while an
   endpoint below another one is bound.  Only partly addressed.

   Directly below a Root Port, mainline lowers the Root Port below the
   active function, which is left at the larger MPS.  With patch 1, the
   hierarchy isn't changed, and the new device is left mismatched with a
   warning instead, as below a Switch today.

Other:

8. With pci=pcie_bus_safe or pcie_bus_perf, devices found by a rescan
   aren't configured at all: pci_configure_mps() leaves them to
   pcie_bus_configure_settings(), which the rescan helpers don't call.
   This doesn't affect the default strategy, where pci_configure_mps()
   configures each device as it's enumerated.  Calling
   pcie_bus_configure_settings() from the rescan helpers wouldn't fix
   this safely, since those strategies reprogram every device of the
   hierarchy, including ones in use.  Not addressed.

Approach
========

Two constraints drive the design:

- The MPS of a device that may have a driver bound can't be changed
  safely: the device may have DMA in flight, its driver may have cached
  the value, and the core's read-modify-write of Device Control isn't
  serialized against the driver's own updates of that register.
  Changes to a hierarchy are therefore limited to hierarchies in which
  no device below the Root Port, other than a Switch port, has been
  added or made available for driver binding yet, i.e. during the
  initial scan, or when devices are hot-added or rescanned into a
  hierarchy without endpoints in use, such as an empty slot directly
  below a Root Port.  As today, the Root Port itself may still be
  changed, and so may Switch ports, since they don't do DMA and the port
  driver doesn't change Device Control after enumeration.

- A device hot-added below a Switch later can't lower the MPS of a
  hierarchy with endpoints in use, so it only works if it supports the
  MPS the hierarchy already runs at (5).  Hierarchies with
  hotplug-capable Switch ports are therefore limited to 128 bytes, which
  every device supports.

A better solution would be to quiesce the drivers of all devices below
the Root Port, e.g. the way a reset does with ->reset_prepare() and
->reset_done(), change the MPS of the whole hierarchy, and then resume
them.  That would allow changing the MPS of a system that is in use,
including when a device that supports less shows up next to endpoints
in use (7), and hierarchies with hotplug-capable Switch ports wouldn't
need to be limited to 128 bytes.  It requires every affected driver to
support being quiesced and to cope with a changed MPS, e.g. drivers
that cache it, which is beyond this series.

Only PCIE_BUS_DEFAULT changes; the other strategies are unchanged.

Why the Root Port MPS is set from pcie_bus_configure_settings()
===============================================================

v9 raised each Root Port to its maximum MPS when pci_configure_mps() ran
for the Root Port itself, and relied on lowering it again as devices
below it were enumerated.  At that point, nothing below the Root Port
has been enumerated yet, so the kernel can't know the smallest MPSS
below it, whether the hierarchy has hotplug-capable Switch ports, or
whether the slot is empty.  Raising the Root Port there:

- can't take hotplug-capable Switch ports into account, since they
  haven't been found yet: hierarchies that have to be limited to 128
  bytes (5) would be raised first and lowered again as those ports are
  found, and bridges that are only marked as hotplug-capable later, e.g.
  by acpiphp, would be missed;

- raises empty slots, so a Switch hot-added into one later inherits the
  raised MPS;

- happens only once, since a Root Port isn't enumerated again on
  hot-add, so the Root Port can't be raised again after a device that
  supported less has been replaced (3).

pcie_bus_configure_settings() runs after the hierarchy below a Root Port
has been scanned and before its drivers are bound: from pci_host_probe()
and the ACPI and arch-specific root bus scans, and from pciehp, acpiphp
and shpchp after a hot-add.  It is where the other strategies already
configure MPS for a whole hierarchy, and pcie_find_smpss() already
computes the smallest MPSS of a hierarchy, returning 128 bytes for
hierarchies with a hotplug bridge other than the Root Port, which is
exactly the limit patch 2 applies to them.  An empty slot isn't
configured until something is hot-added or rescanned there, and every
hot-add directly below a Root Port re-evaluates its hierarchy.  The
rescan helpers don't call pcie_bus_configure_settings(), so patch 2 does
the same from them.

Configuring the MPS in pci_bus_add_device() instead would also run after
the scan, but once per device rather than once per hierarchy, and it
would move all of the default MPS configuration into the driver binding
path.

Behavior changes
================

- With firmware that programs MPS, e.g. on x86, a hierarchy may now run
  at a larger MPS than firmware chose, and different Root Port
  hierarchies may end up with different values.  This could matter for
  peer-to-peer DMA between devices below different Root Ports, unless
  the Root Complex splits such TLPs.  pci=pcie_bus_peer2peer remains
  available for that case.

- Hierarchies with hotplug-capable Switch ports are limited to 128
  bytes, even where firmware programmed more.  This costs throughput,
  but makes every device hot-added below them work.

- A single device with a small MPSS found during enumeration lowers the
  MPS of every device below its Root Port.

- A device that shows up later below a Switch without hotplug support,
  e.g. found by a rescan, next to endpoints already in use, may find its
  hierarchy raised by patch 2, and is left mismatched with a warning if
  it supports less (7).

- Read completion coalescing is now also disabled on Intel 5000/5100 in
  the default mode (quirk_intel_mc_errata()), since the default mode can
  now raise MPS above what firmware programmed.

- Scan paths that call neither pcie_bus_configure_settings() nor the
  rescan helpers, mostly legacy ones or ones without Root Ports, are left
  as they are today.

---
Changes in v2:
- Drop "PCI: Update saved Max Payload Size in pcie_set_mps()", which has
  been applied to pci/next, and rebase on pci/next.
- Patch 1: Don't count Switch ports as devices in use, since they don't
  do DMA and the port driver doesn't change Device Control after
  enumeration, so that a device that shows up below a Switch without
  endpoints in use can still lower the hierarchy.
- Patch 2 (was 3): Limit hierarchies with hotplug-capable Switch ports to
  128 bytes, so that devices hot-added below them always match, instead
  of leaving them at their current MPS.  The hierarchy's MPS is now set
  rather than only raised.
- Patch 2: Stop at the first device whose MPS can't be set and restore the
  previous MPS of the hierarchy, instead of carrying on and leaving it
  with mismatched settings (reported by sashiko-bot:
  https://lore.kernel.org/r/20260930150617.CE88A1F000FF@smtp.kernel.org/).
- Patch 3 (was 4): Update the comment in meson_size_to_mrrs() to refer to
  MRRS (reported by sashiko-bot:
  https://lore.kernel.org/r/20260930145752.C0D4D1F00893@smtp.kernel.org/).
- No longer an RFC: raising the MPS above what firmware programmed is
  acceptable, hierarchies with hotplug-capable Switch ports are limited to
  128 bytes, and a device that can't match a hierarchy in use stays bound
  with a warning, as today.

RFC: https://lore.kernel.org/linux-pci/20260930145017.1356088-6-cassel@kernel.org/
v9: https://lore.kernel.org/linux-pci/20260916153907.60344-1-18255117159@163.com/

Hans Zhang (2):
  PCI: Match the hierarchy's MPS to a device's MPSS as necessary
  PCI: meson: Remove redundant MPS configuration

Niklas Cassel (1):
  PCI: Configure Root Port MPS after scanning its hierarchy

 drivers/pci/controller/dwc/pci-meson.c |  27 +---
 drivers/pci/probe.c                    | 194 ++++++++++++++++++++++++-
 drivers/pci/quirks.c                   |   3 +-
 3 files changed, 195 insertions(+), 29 deletions(-)


base-commit: a907c0643fb44bb260f3133b9c73bd34fd125e4c
-- 
2.56.0

[PATCH v2 0/3] PCI: Fix and improve default MPS configuration

[Niklas Cassel]

This series reworks Hans's series "PCI: Configure Root Port MPS during
host probing" (v9, link below).  Besides configuring Root Ports, which
that series was about, it fixes other problems in how mainline programs
the Maximum Payload Size (MPS), and lists the problems it doesn't fix.

It is based on pci/next, which has the first patch of the RFC (link
below), "PCI: Update saved Max Payload Size in pcie_set_mps()".

The changes compared to v9, including the commit messages and this
cover letter, were developed with the help of an LLM, based on its
review of v9 and of the sashiko-bot reports, and reviewed by me.

Background
==========

A PCIe device must not send a TLP with a payload larger than the MPS it
is programmed with, and a receiver treats a TLP larger than its own MPS
as Malformed, so all devices on a path need compatible MPS settings.

Unless a "pci=pcie_bus_*" option is given, the kernel uses the default
strategy, PCIE_BUS_DEFAULT: pci_configure_mps() programs each device, as
it is enumerated, to the MPS of its upstream bridge.  If the device's MPS
Supported (MPSS) is smaller and the upstream bridge is a Root Port, the
Root Port is lowered instead (9f0e89359775).  The other strategies either
configure whole hierarchies in pcie_bus_configure_settings()
(pcie_bus_safe, pcie_bus_perf, pcie_bus_peer2peer) or don't touch MPS at
all (pcie_bus_tune_off).

Problems found in mainline
==========================

Enumeration:

1. Root Ports keep whatever MPS they came up with.  Fixed by patch 2.

   Root Ports have no upstream bridge, so pci_configure_mps() never
   configures them, and every hierarchy inherits the MPS that firmware or
   the hardware reset left in its Root Port.  On platforms whose firmware
   doesn't configure MPS, which is typical for DT-based systems with
   native host controller drivers, every hierarchy runs at the 128-byte
   reset value even if the Root Port and all devices below it support
   more, at a cost in throughput.  Host controller drivers work around
   this: the Meson driver programs its Root Port to 256 bytes itself,
   regardless of what the devices below it support, which patch 3
   removes.

2. Lowering the MPS only works directly below a Root Port.  Fixed by
   patch 1.

   - Below a Switch, the Switch ports keep the larger MPS, and a device
     that supports less is left mismatched with a "use
     pci=pcie_bus_safe" warning.  This happens at boot whenever firmware
     or a host controller driver programmed a larger MPS than a device
     below a Switch supports.  It has been a known limitation since the
     default strategy was introduced (27d868b5e6cf).

   - For a multi-function device directly below a Root Port, function 0
     is programmed to the Root Port's MPS first.  If function 1 supports
     less, the Root Port is lowered, but function 0 is left at the larger
     value and can send TLPs larger than the Root Port now accepts.  Only
     an informational message about the Root Port is printed.  This was
     introduced by 9f0e89359775.

Hotplug and rescan:

3. A device hot-added directly below a Root Port.  Fixed by patches 1
   and 2.

   If the device supports less than the Root Port's MPS, the Root Port
   is lowered, with the multi-function problem from 2.  If it supports
   more, it is only matched to the Root Port's current MPS: a Root Port
   is never raised again, e.g. after a device that supported less has
   been replaced, and without firmware MPS setup it stays at 128 bytes.
   Devices found by a rescan below an empty Root Port slot behave the
   same.

4. A Switch hot-added directly below a Root Port.  Fixed by patches 1
   and 2.

   The Upstream Port can lower the Root Port, but the Downstream Ports
   and the devices below them are only matched to their upstream bridge,
   and any of them that supports less is left mismatched with a warning.
   With patch 1, nothing below the Root Port is in use yet, so the whole
   new hierarchy is lowered.  If the new Switch has hotplug-capable
   ports, patch 2 then limits the hierarchy to 128 bytes, so devices
   hot-added below it later can match (5).

5. A device hot-added below a Switch.  Fixed by patches 1 and 2, unless
   endpoints below the Root Port are already in use (7).

   Only a Root Port is ever lowered, so a device that supports less than
   the MPS of the Switch ports, e.g. less than what firmware programmed,
   is left mismatched with a warning.  Patch 2 limits hierarchies with
   hotplug-capable Switch ports to 128 bytes, like pcie_bus_safe does, so
   devices hot-added below such ports always match.  Below a Switch
   without hotplug support, e.g. when a device is found by a rescan,
   patch 1 lowers the whole hierarchy as long as no endpoint below the
   Root Port is in use, since the Switch ports themselves don't do DMA.

6. A Switch hot-added below another Switch.  Fixed by patches 1 and 2,
   unless endpoints below the Root Port are already in use (7).

   Same as 5, for every port and device of the new Switch.  With patch
   2, a hierarchy whose Switch has hotplug-capable ports, e.g. a chain of
   Thunderbolt devices, already runs at 128 bytes, so the new Switch can
   match.  Below a Switch without hotplug support, patch 1 lowers the
   hierarchy as in 5.

7. A device added next to endpoints already in use below the same Root
   Port, e.g. a new function found by a rescan while function 0 is
   bound, or a device below one Downstream Port of a Switch while an
   endpoint below another one is bound.  Only partly addressed.

   Directly below a Root Port, mainline lowers the Root Port below the
   active function, which is left at the larger MPS.  With patch 1, the
   hierarchy isn't changed, and the new device is left mismatched with a
   warning instead, as below a Switch today.

Other:

8. With pci=pcie_bus_safe or pcie_bus_perf, devices found by a rescan
   aren't configured at all: pci_configure_mps() leaves them to
   pcie_bus_configure_settings(), which the rescan helpers don't call.
   This doesn't affect the default strategy, where pci_configure_mps()
   configures each device as it's enumerated.  Calling
   pcie_bus_configure_settings() from the rescan helpers wouldn't fix
   this safely, since those strategies reprogram every device of the
   hierarchy, including ones in use.  Not addressed.

Approach
========

Two constraints drive the design:

- The MPS of a device that may have a driver bound can't be changed
  safely: the device may have DMA in flight, its driver may have cached
  the value, and the core's read-modify-write of Device Control isn't
  serialized against the driver's own updates of that register.
  Changes to a hierarchy are therefore limited to hierarchies in which
  no device below the Root Port, other than a Switch port, has been
  added or made available for driver binding yet, i.e. during the
  initial scan, or when devices are hot-added or rescanned into a
  hierarchy without endpoints in use, such as an empty slot directly
  below a Root Port.  As today, the Root Port itself may still be
  changed, and so may Switch ports, since they don't do DMA and the port
  driver doesn't change Device Control after enumeration.

- A device hot-added below a Switch later can't lower the MPS of a
  hierarchy with endpoints in use, so it only works if it supports the
  MPS the hierarchy already runs at (5).  Hierarchies with
  hotplug-capable Switch ports are therefore limited to 128 bytes, which
  every device supports.

A better solution would be to quiesce the drivers of all devices below
the Root Port, e.g. the way a reset does with ->reset_prepare() and
->reset_done(), change the MPS of the whole hierarchy, and then resume
them.  That would allow changing the MPS of a system that is in use,
including when a device that supports less shows up next to endpoints
in use (7), and hierarchies with hotplug-capable Switch ports wouldn't
need to be limited to 128 bytes.  It requires every affected driver to
support being quiesced and to cope with a changed MPS, e.g. drivers
that cache it, which is beyond this series.

Only PCIE_BUS_DEFAULT changes; the other strategies are unchanged.

Why the Root Port MPS is set from pcie_bus_configure_settings()
===============================================================

v9 raised each Root Port to its maximum MPS when pci_configure_mps() ran
for the Root Port itself, and relied on lowering it again as devices
below it were enumerated.  At that point, nothing below the Root Port
has been enumerated yet, so the kernel can't know the smallest MPSS
below it, whether the hierarchy has hotplug-capable Switch ports, or
whether the slot is empty.  Raising the Root Port there:

- can't take hotplug-capable Switch ports into account, since they
  haven't been found yet: hierarchies that have to be limited to 128
  bytes (5) would be raised first and lowered again as those ports are
  found, and bridges that are only marked as hotplug-capable later, e.g.
  by acpiphp, would be missed;

- raises empty slots, so a Switch hot-added into one later inherits the
  raised MPS;

- happens only once, since a Root Port isn't enumerated again on
  hot-add, so the Root Port can't be raised again after a device that
  supported less has been replaced (3).

pcie_bus_configure_settings() runs after the hierarchy below a Root Port
has been scanned and before its drivers are bound: from pci_host_probe()
and the ACPI and arch-specific root bus scans, and from pciehp, acpiphp
and shpchp after a hot-add.  It is where the other strategies already
configure MPS for a whole hierarchy, and pcie_find_smpss() already
computes the smallest MPSS of a hierarchy, returning 128 bytes for
hierarchies with a hotplug bridge other than the Root Port, which is
exactly the limit patch 2 applies to them.  An empty slot isn't
configured until something is hot-added or rescanned there, and every
hot-add directly below a Root Port re-evaluates its hierarchy.  The
rescan helpers don't call pcie_bus_configure_settings(), so patch 2 does
the same from them.

Configuring the MPS in pci_bus_add_device() instead would also run after
the scan, but once per device rather than once per hierarchy, and it
would move all of the default MPS configuration into the driver binding
path.

Behavior changes
================

- With firmware that programs MPS, e.g. on x86, a hierarchy may now run
  at a larger MPS than firmware chose, and different Root Port
  hierarchies may end up with different values.  This could matter for
  peer-to-peer DMA between devices below different Root Ports, unless
  the Root Complex splits such TLPs.  pci=pcie_bus_peer2peer remains
  available for that case.

- Hierarchies with hotplug-capable Switch ports are limited to 128
  bytes, even where firmware programmed more.  This costs throughput,
  but makes every device hot-added below them work.

- A single device with a small MPSS found during enumeration lowers the
  MPS of every device below its Root Port.

- A device that shows up later below a Switch without hotplug support,
  e.g. found by a rescan, next to endpoints already in use, may find its
  hierarchy raised by patch 2, and is left mismatched with a warning if
  it supports less (7).

- Read completion coalescing is now also disabled on Intel 5000/5100 in
  the default mode (quirk_intel_mc_errata()), since the default mode can
  now raise MPS above what firmware programmed.

- Scan paths that call neither pcie_bus_configure_settings() nor the
  rescan helpers, mostly legacy ones or ones without Root Ports, are left
  as they are today.

---
Changes in v2:
- Drop "PCI: Update saved Max Payload Size in pcie_set_mps()", which has
  been applied to pci/next, and rebase on pci/next.
- Patch 1: Don't count Switch ports as devices in use, since they don't
  do DMA and the port driver doesn't change Device Control after
  enumeration, so that a device that shows up below a Switch without
  endpoints in use can still lower the hierarchy.
- Patch 2 (was 3): Limit hierarchies with hotplug-capable Switch ports to
  128 bytes, so that devices hot-added below them always match, instead
  of leaving them at their current MPS.  The hierarchy's MPS is now set
  rather than only raised.
- Patch 2: Stop at the first device whose MPS can't be set and restore the
  previous MPS of the hierarchy, instead of carrying on and leaving it
  with mismatched settings (reported by sashiko-bot:
  https://lore.kernel.org/r/20260930150617.CE88A1F000FF@smtp.kernel.org/).
- Patch 3 (was 4): Update the comment in meson_size_to_mrrs() to refer to
  MRRS (reported by sashiko-bot:
  https://lore.kernel.org/r/20260930145752.C0D4D1F00893@smtp.kernel.org/).
- No longer an RFC: raising the MPS above what firmware programmed is
  acceptable, hierarchies with hotplug-capable Switch ports are limited to
  128 bytes, and a device that can't match a hierarchy in use stays bound
  with a warning, as today.

RFC: https://lore.kernel.org/linux-pci/20260930145017.1356088-6-cassel@kernel.org/
v9: https://lore.kernel.org/linux-pci/20260916153907.60344-1-18255117159@163.com/

Hans Zhang (2):
  PCI: Match the hierarchy's MPS to a device's MPSS as necessary
  PCI: meson: Remove redundant MPS configuration

Niklas Cassel (1):
  PCI: Configure Root Port MPS after scanning its hierarchy

 drivers/pci/controller/dwc/pci-meson.c |  27 +---
 drivers/pci/probe.c                    | 194 ++++++++++++++++++++++++-
 drivers/pci/quirks.c                   |   3 +-
 3 files changed, 195 insertions(+), 29 deletions(-)


base-commit: a907c0643fb44bb260f3133b9c73bd34fd125e4c
-- 
2.56.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 0/3] PCI: Fix and improve default MPS configuration

[Niklas Cassel]

This series reworks Hans's series "PCI: Configure Root Port MPS during
host probing" (v9, link below).  Besides configuring Root Ports, which
that series was about, it fixes other problems in how mainline programs
the Maximum Payload Size (MPS), and lists the problems it doesn't fix.

It is based on pci/next, which has the first patch of the RFC (link
below), "PCI: Update saved Max Payload Size in pcie_set_mps()".

The changes compared to v9, including the commit messages and this
cover letter, were developed with the help of an LLM, based on its
review of v9 and of the sashiko-bot reports, and reviewed by me.

Background
==========

A PCIe device must not send a TLP with a payload larger than the MPS it
is programmed with, and a receiver treats a TLP larger than its own MPS
as Malformed, so all devices on a path need compatible MPS settings.

Unless a "pci=pcie_bus_*" option is given, the kernel uses the default
strategy, PCIE_BUS_DEFAULT: pci_configure_mps() programs each device, as
it is enumerated, to the MPS of its upstream bridge.  If the device's MPS
Supported (MPSS) is smaller and the upstream bridge is a Root Port, the
Root Port is lowered instead (9f0e89359775).  The other strategies either
configure whole hierarchies in pcie_bus_configure_settings()
(pcie_bus_safe, pcie_bus_perf, pcie_bus_peer2peer) or don't touch MPS at
all (pcie_bus_tune_off).

Problems found in mainline
==========================

Enumeration:

1. Root Ports keep whatever MPS they came up with.  Fixed by patch 2.

   Root Ports have no upstream bridge, so pci_configure_mps() never
   configures them, and every hierarchy inherits the MPS that firmware or
   the hardware reset left in its Root Port.  On platforms whose firmware
   doesn't configure MPS, which is typical for DT-based systems with
   native host controller drivers, every hierarchy runs at the 128-byte
   reset value even if the Root Port and all devices below it support
   more, at a cost in throughput.  Host controller drivers work around
   this: the Meson driver programs its Root Port to 256 bytes itself,
   regardless of what the devices below it support, which patch 3
   removes.

2. Lowering the MPS only works directly below a Root Port.  Fixed by
   patch 1.

   - Below a Switch, the Switch ports keep the larger MPS, and a device
     that supports less is left mismatched with a "use
     pci=pcie_bus_safe" warning.  This happens at boot whenever firmware
     or a host controller driver programmed a larger MPS than a device
     below a Switch supports.  It has been a known limitation since the
     default strategy was introduced (27d868b5e6cf).

   - For a multi-function device directly below a Root Port, function 0
     is programmed to the Root Port's MPS first.  If function 1 supports
     less, the Root Port is lowered, but function 0 is left at the larger
     value and can send TLPs larger than the Root Port now accepts.  Only
     an informational message about the Root Port is printed.  This was
     introduced by 9f0e89359775.

Hotplug and rescan:

3. A device hot-added directly below a Root Port.  Fixed by patches 1
   and 2.

   If the device supports less than the Root Port's MPS, the Root Port
   is lowered, with the multi-function problem from 2.  If it supports
   more, it is only matched to the Root Port's current MPS: a Root Port
   is never raised again, e.g. after a device that supported less has
   been replaced, and without firmware MPS setup it stays at 128 bytes.
   Devices found by a rescan below an empty Root Port slot behave the
   same.

4. A Switch hot-added directly below a Root Port.  Fixed by patches 1
   and 2.

   The Upstream Port can lower the Root Port, but the Downstream Ports
   and the devices below them are only matched to their upstream bridge,
   and any of them that supports less is left mismatched with a warning.
   With patch 1, nothing below the Root Port is in use yet, so the whole
   new hierarchy is lowered.  If the new Switch has hotplug-capable
   ports, patch 2 then limits the hierarchy to 128 bytes, so devices
   hot-added below it later can match (5).

5. A device hot-added below a Switch.  Fixed by patches 1 and 2, unless
   endpoints below the Root Port are already in use (7).

   Only a Root Port is ever lowered, so a device that supports less than
   the MPS of the Switch ports, e.g. less than what firmware programmed,
   is left mismatched with a warning.  Patch 2 limits hierarchies with
   hotplug-capable Switch ports to 128 bytes, like pcie_bus_safe does, so
   devices hot-added below such ports always match.  Below a Switch
   without hotplug support, e.g. when a device is found by a rescan,
   patch 1 lowers the whole hierarchy as long as no endpoint below the
   Root Port is in use, since the Switch ports themselves don't do DMA.

6. A Switch hot-added below another Switch.  Fixed by patches 1 and 2,
   unless endpoints below the Root Port are already in use (7).

   Same as 5, for every port and device of the new Switch.  With patch
   2, a hierarchy whose Switch has hotplug-capable ports, e.g. a chain of
   Thunderbolt devices, already runs at 128 bytes, so the new Switch can
   match.  Below a Switch without hotplug support, patch 1 lowers the
   hierarchy as in 5.

7. A device added next to endpoints already in use below the same Root
   Port, e.g. a new function found by a rescan while function 0 is
   bound, or a device below one Downstream Port of a Switch while an
   endpoint below another one is bound.  Only partly addressed.

   Directly below a Root Port, mainline lowers the Root Port below the
   active function, which is left at the larger MPS.  With patch 1, the
   hierarchy isn't changed, and the new device is left mismatched with a
   warning instead, as below a Switch today.

Other:

8. With pci=pcie_bus_safe or pcie_bus_perf, devices found by a rescan
   aren't configured at all: pci_configure_mps() leaves them to
   pcie_bus_configure_settings(), which the rescan helpers don't call.
   This doesn't affect the default strategy, where pci_configure_mps()
   configures each device as it's enumerated.  Calling
   pcie_bus_configure_settings() from the rescan helpers wouldn't fix
   this safely, since those strategies reprogram every device of the
   hierarchy, including ones in use.  Not addressed.

Approach
========

Two constraints drive the design:

- The MPS of a device that may have a driver bound can't be changed
  safely: the device may have DMA in flight, its driver may have cached
  the value, and the core's read-modify-write of Device Control isn't
  serialized against the driver's own updates of that register.
  Changes to a hierarchy are therefore limited to hierarchies in which
  no device below the Root Port, other than a Switch port, has been
  added or made available for driver binding yet, i.e. during the
  initial scan, or when devices are hot-added or rescanned into a
  hierarchy without endpoints in use, such as an empty slot directly
  below a Root Port.  As today, the Root Port itself may still be
  changed, and so may Switch ports, since they don't do DMA and the port
  driver doesn't change Device Control after enumeration.

- A device hot-added below a Switch later can't lower the MPS of a
  hierarchy with endpoints in use, so it only works if it supports the
  MPS the hierarchy already runs at (5).  Hierarchies with
  hotplug-capable Switch ports are therefore limited to 128 bytes, which
  every device supports.

A better solution would be to quiesce the drivers of all devices below
the Root Port, e.g. the way a reset does with ->reset_prepare() and
->reset_done(), change the MPS of the whole hierarchy, and then resume
them.  That would allow changing the MPS of a system that is in use,
including when a device that supports less shows up next to endpoints
in use (7), and hierarchies with hotplug-capable Switch ports wouldn't
need to be limited to 128 bytes.  It requires every affected driver to
support being quiesced and to cope with a changed MPS, e.g. drivers
that cache it, which is beyond this series.

Only PCIE_BUS_DEFAULT changes; the other strategies are unchanged.

Why the Root Port MPS is set from pcie_bus_configure_settings()
===============================================================

v9 raised each Root Port to its maximum MPS when pci_configure_mps() ran
for the Root Port itself, and relied on lowering it again as devices
below it were enumerated.  At that point, nothing below the Root Port
has been enumerated yet, so the kernel can't know the smallest MPSS
below it, whether the hierarchy has hotplug-capable Switch ports, or
whether the slot is empty.  Raising the Root Port there:

- can't take hotplug-capable Switch ports into account, since they
  haven't been found yet: hierarchies that have to be limited to 128
  bytes (5) would be raised first and lowered again as those ports are
  found, and bridges that are only marked as hotplug-capable later, e.g.
  by acpiphp, would be missed;

- raises empty slots, so a Switch hot-added into one later inherits the
  raised MPS;

- happens only once, since a Root Port isn't enumerated again on
  hot-add, so the Root Port can't be raised again after a device that
  supported less has been replaced (3).

pcie_bus_configure_settings() runs after the hierarchy below a Root Port
has been scanned and before its drivers are bound: from pci_host_probe()
and the ACPI and arch-specific root bus scans, and from pciehp, acpiphp
and shpchp after a hot-add.  It is where the other strategies already
configure MPS for a whole hierarchy, and pcie_find_smpss() already
computes the smallest MPSS of a hierarchy, returning 128 bytes for
hierarchies with a hotplug bridge other than the Root Port, which is
exactly the limit patch 2 applies to them.  An empty slot isn't
configured until something is hot-added or rescanned there, and every
hot-add directly below a Root Port re-evaluates its hierarchy.  The
rescan helpers don't call pcie_bus_configure_settings(), so patch 2 does
the same from them.

Configuring the MPS in pci_bus_add_device() instead would also run after
the scan, but once per device rather than once per hierarchy, and it
would move all of the default MPS configuration into the driver binding
path.

Behavior changes
================

- With firmware that programs MPS, e.g. on x86, a hierarchy may now run
  at a larger MPS than firmware chose, and different Root Port
  hierarchies may end up with different values.  This could matter for
  peer-to-peer DMA between devices below different Root Ports, unless
  the Root Complex splits such TLPs.  pci=pcie_bus_peer2peer remains
  available for that case.

- Hierarchies with hotplug-capable Switch ports are limited to 128
  bytes, even where firmware programmed more.  This costs throughput,
  but makes every device hot-added below them work.

- A single device with a small MPSS found during enumeration lowers the
  MPS of every device below its Root Port.

- A device that shows up later below a Switch without hotplug support,
  e.g. found by a rescan, next to endpoints already in use, may find its
  hierarchy raised by patch 2, and is left mismatched with a warning if
  it supports less (7).

- Read completion coalescing is now also disabled on Intel 5000/5100 in
  the default mode (quirk_intel_mc_errata()), since the default mode can
  now raise MPS above what firmware programmed.

- Scan paths that call neither pcie_bus_configure_settings() nor the
  rescan helpers, mostly legacy ones or ones without Root Ports, are left
  as they are today.

---
Changes in v2:
- Drop "PCI: Update saved Max Payload Size in pcie_set_mps()", which has
  been applied to pci/next, and rebase on pci/next.
- Patch 1: Don't count Switch ports as devices in use, since they don't
  do DMA and the port driver doesn't change Device Control after
  enumeration, so that a device that shows up below a Switch without
  endpoints in use can still lower the hierarchy.
- Patch 2 (was 3): Limit hierarchies with hotplug-capable Switch ports to
  128 bytes, so that devices hot-added below them always match, instead
  of leaving them at their current MPS.  The hierarchy's MPS is now set
  rather than only raised.
- Patch 2: Stop at the first device whose MPS can't be set and restore the
  previous MPS of the hierarchy, instead of carrying on and leaving it
  with mismatched settings (reported by sashiko-bot:
  https://lore.kernel.org/r/20260930150617.CE88A1F000FF@smtp.kernel.org/).
- Patch 3 (was 4): Update the comment in meson_size_to_mrrs() to refer to
  MRRS (reported by sashiko-bot:
  https://lore.kernel.org/r/20260930145752.C0D4D1F00893@smtp.kernel.org/).
- No longer an RFC: raising the MPS above what firmware programmed is
  acceptable, hierarchies with hotplug-capable Switch ports are limited to
  128 bytes, and a device that can't match a hierarchy in use stays bound
  with a warning, as today.

RFC: https://lore.kernel.org/linux-pci/20260930145017.1356088-6-cassel@kernel.org/
v9: https://lore.kernel.org/linux-pci/20260916153907.60344-1-18255117159@163.com/

Hans Zhang (2):
  PCI: Match the hierarchy's MPS to a device's MPSS as necessary
  PCI: meson: Remove redundant MPS configuration

Niklas Cassel (1):
  PCI: Configure Root Port MPS after scanning its hierarchy

 drivers/pci/controller/dwc/pci-meson.c |  27 +---
 drivers/pci/probe.c                    | 194 ++++++++++++++++++++++++-
 drivers/pci/quirks.c                   |   3 +-
 3 files changed, 195 insertions(+), 29 deletions(-)


base-commit: a907c0643fb44bb260f3133b9c73bd34fd125e4c
-- 
2.56.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v2 1/3] PCI: Match the hierarchy's MPS to a device's MPSS as necessary

[Niklas Cassel]

From: Hans Zhang <18255117159@163.com>

pci_configure_mps() enumerates top-down and programs each device's Maximum
Payload Size (MPS) to match its upstream bridge.  When a device's MPS
Supported (MPSS) is too small to match, commit 9f0e89359775 ("PCI: Match
Root Port's MPS to endpoint's MPSS as necessary") reduces the upstream
bridge instead, but only when that bridge is a Root Port.

That covers an endpoint directly below a Root Port and nothing else.  With
a Switch in between, the Switch ports have already inherited the Root
Port's larger MPS, the reduction is skipped because the upstream bridge is
a Switch Downstream Port, and pcie_set_mps() then fails with -EINVAL for
the endpoint.  The endpoint is left below a port programmed for a larger
MPS, so any larger TLP it receives is treated as Malformed.

Multi-function devices hit the same hole from the other direction:
reducing the Root Port for a function with a small MPSS leaves the sibling
functions already programmed to the larger value.

Walk the hierarchy from the Root Port down and reduce every device that is
above the new value.  Reducing only the ports between the device and the
Root Port is not sufficient, because a Switch does not split TLPs: an
already programmed sibling left at the larger MPS could emit a TLP too
large for its egress port.  As a result, a single device with a small MPSS
now lowers the MPS of every device below its Root Port.

Only do this while no device below the Root Port, other than a Switch
port, has been added or made available for driver binding, i.e., during
the initial scan, or when devices are hot-added or rescanned into a
hierarchy without endpoints in use, such as an empty slot directly below
the Root Port or a Switch with nothing below it.  Such devices may have
drivers bound and DMA in flight, so their MPS can't be changed safely.
This is the same constraint that makes PCIE_BUS_SAFE limit fabrics with
hotplug bridges below a Root Port to 128 bytes in pcie_find_smpss().
Like the Root Port itself, Switch ports can still be changed: they don't
do DMA, and the port driver doesn't change Device Control after
enumeration.  A device added next to endpoints that are already in use
is left at its current MPS and pci_configure_mps() warns and suggests
"pci=pcie_bus_safe", which is what already happens below a Switch today.

This only affects PCIE_BUS_DEFAULT.  PCIE_BUS_SAFE, PCIE_BUS_PERFORMANCE
and PCIE_BUS_PEER2PEER program MPS in pcie_bus_configure_settings() and
PCIE_BUS_TUNE_OFF doesn't touch it, so all of them return before this
point.

Fixes: 9f0e89359775 ("PCI: Match Root Port's MPS to endpoint's MPSS as necessary")
Signed-off-by: Hans Zhang <18255117159@163.com>
Co-developed-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/probe.c | 78 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 74 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 9a178e0e5f7f..ccc3e9202c2c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2207,9 +2207,60 @@ int pci_setup_device(struct pci_dev *dev)
 	return 0;
 }
 
+static int pcie_reduce_mps(struct pci_dev *dev, void *data)
+{
+	int mps = *(int *)data;
+	int ret;
+
+	/* MPS is of type 'RsvdP' for VFs */
+	if (!pci_is_pcie(dev) || dev->is_virtfn)
+		return 0;
+
+	if (pcie_get_mps(dev) > mps) {
+		ret = pcie_set_mps(dev, mps);
+		if (ret)
+			pci_warn(dev, "can't set Max Payload Size to %d; if necessary, use \"pci=pcie_bus_safe\" and report a bug\n",
+				 mps);
+	}
+
+	return 0;
+}
+
+static int pci_dev_check_in_use(struct pci_dev *dev, void *data)
+{
+	bool *in_use = data;
+
+	/*
+	 * Switch ports don't do DMA, and the port driver doesn't change Device
+	 * Control after enumeration, so their MPS can be changed as long as no
+	 * device below them is in use.
+	 */
+	if (pci_is_pcie(dev) &&
+	    (pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM ||
+	     pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
+		return 0;
+
+	*in_use = pci_dev_is_added(dev) || !pci_dev_binding_disallowed(dev);
+	return *in_use;
+}
+
+/*
+ * Return true if any device on or below @bus, other than a Switch port, has
+ * been added or made available for driver binding, i.e., may have a driver
+ * bound and DMA in flight.
+ */
+static bool pci_bus_in_use(struct pci_bus *bus)
+{
+	bool in_use = false;
+
+	pci_walk_bus(bus, pci_dev_check_in_use, &in_use);
+	return in_use;
+}
+
 static void pci_configure_mps(struct pci_dev *dev)
 {
 	struct pci_dev *bridge = pci_upstream_bridge(dev);
+	struct pci_dev *rp;
 	int mps, mpss, p_mps, rc;
 
 	if (!pci_is_pcie(dev))
@@ -2259,10 +2310,29 @@ static void pci_configure_mps(struct pci_dev *dev)
 		return;
 
 	mpss = 128 << dev->pcie_mpss;
-	if (mpss < p_mps && pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) {
-		pcie_set_mps(bridge, mpss);
-		pci_info(dev, "Upstream bridge's Max Payload Size set to %d (was %d, max %d)\n",
-			 mpss, p_mps, 128 << bridge->pcie_mpss);
+	rp = pcie_find_root_port(bridge);
+	if (mpss < p_mps && rp && !pci_bus_in_use(rp->subordinate)) {
+		/*
+		 * dev cannot be programmed to the MPS already in use above
+		 * it, so reduce the hierarchy to what dev supports.  A Switch
+		 * does not split TLPs, so reducing only the upstream bridge
+		 * is not enough: every port up to the Root Port has to come
+		 * down as well, and so do the devices already programmed
+		 * below that Root Port, which would otherwise be left sending
+		 * TLPs too large for their egress port.
+		 *
+		 * Only do this while no device below the Root Port, other than
+		 * a Switch port, has been added or made available for driver
+		 * binding, e.g., during the initial scan or when hot-adding
+		 * into an empty slot directly below the Root Port or below a
+		 * Switch with no endpoint in use.  Such devices may have
+		 * drivers bound and DMA in flight, so their MPS can't be
+		 * changed safely (see pcie_find_smpss()).
+		 */
+		pcie_reduce_mps(rp, &mpss);
+		pci_walk_bus(rp->subordinate, pcie_reduce_mps, &mpss);
+		pci_info(dev, "Max Payload Size of %s hierarchy set to %d (was %d)\n",
+			 pci_name(rp), mpss, p_mps);
 		p_mps = pcie_get_mps(bridge);
 	}
 
-- 
2.56.0

[PATCH v2 1/3] PCI: Match the hierarchy's MPS to a device's MPSS as necessary

[Niklas Cassel]

From: Hans Zhang <18255117159@163.com>

pci_configure_mps() enumerates top-down and programs each device's Maximum
Payload Size (MPS) to match its upstream bridge.  When a device's MPS
Supported (MPSS) is too small to match, commit 9f0e89359775 ("PCI: Match
Root Port's MPS to endpoint's MPSS as necessary") reduces the upstream
bridge instead, but only when that bridge is a Root Port.

That covers an endpoint directly below a Root Port and nothing else.  With
a Switch in between, the Switch ports have already inherited the Root
Port's larger MPS, the reduction is skipped because the upstream bridge is
a Switch Downstream Port, and pcie_set_mps() then fails with -EINVAL for
the endpoint.  The endpoint is left below a port programmed for a larger
MPS, so any larger TLP it receives is treated as Malformed.

Multi-function devices hit the same hole from the other direction:
reducing the Root Port for a function with a small MPSS leaves the sibling
functions already programmed to the larger value.

Walk the hierarchy from the Root Port down and reduce every device that is
above the new value.  Reducing only the ports between the device and the
Root Port is not sufficient, because a Switch does not split TLPs: an
already programmed sibling left at the larger MPS could emit a TLP too
large for its egress port.  As a result, a single device with a small MPSS
now lowers the MPS of every device below its Root Port.

Only do this while no device below the Root Port, other than a Switch
port, has been added or made available for driver binding, i.e., during
the initial scan, or when devices are hot-added or rescanned into a
hierarchy without endpoints in use, such as an empty slot directly below
the Root Port or a Switch with nothing below it.  Such devices may have
drivers bound and DMA in flight, so their MPS can't be changed safely.
This is the same constraint that makes PCIE_BUS_SAFE limit fabrics with
hotplug bridges below a Root Port to 128 bytes in pcie_find_smpss().
Like the Root Port itself, Switch ports can still be changed: they don't
do DMA, and the port driver doesn't change Device Control after
enumeration.  A device added next to endpoints that are already in use
is left at its current MPS and pci_configure_mps() warns and suggests
"pci=pcie_bus_safe", which is what already happens below a Switch today.

This only affects PCIE_BUS_DEFAULT.  PCIE_BUS_SAFE, PCIE_BUS_PERFORMANCE
and PCIE_BUS_PEER2PEER program MPS in pcie_bus_configure_settings() and
PCIE_BUS_TUNE_OFF doesn't touch it, so all of them return before this
point.

Fixes: 9f0e89359775 ("PCI: Match Root Port's MPS to endpoint's MPSS as necessary")
Signed-off-by: Hans Zhang <18255117159@163.com>
Co-developed-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/probe.c | 78 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 74 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 9a178e0e5f7f..ccc3e9202c2c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2207,9 +2207,60 @@ int pci_setup_device(struct pci_dev *dev)
 	return 0;
 }
 
+static int pcie_reduce_mps(struct pci_dev *dev, void *data)
+{
+	int mps = *(int *)data;
+	int ret;
+
+	/* MPS is of type 'RsvdP' for VFs */
+	if (!pci_is_pcie(dev) || dev->is_virtfn)
+		return 0;
+
+	if (pcie_get_mps(dev) > mps) {
+		ret = pcie_set_mps(dev, mps);
+		if (ret)
+			pci_warn(dev, "can't set Max Payload Size to %d; if necessary, use \"pci=pcie_bus_safe\" and report a bug\n",
+				 mps);
+	}
+
+	return 0;
+}
+
+static int pci_dev_check_in_use(struct pci_dev *dev, void *data)
+{
+	bool *in_use = data;
+
+	/*
+	 * Switch ports don't do DMA, and the port driver doesn't change Device
+	 * Control after enumeration, so their MPS can be changed as long as no
+	 * device below them is in use.
+	 */
+	if (pci_is_pcie(dev) &&
+	    (pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM ||
+	     pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
+		return 0;
+
+	*in_use = pci_dev_is_added(dev) || !pci_dev_binding_disallowed(dev);
+	return *in_use;
+}
+
+/*
+ * Return true if any device on or below @bus, other than a Switch port, has
+ * been added or made available for driver binding, i.e., may have a driver
+ * bound and DMA in flight.
+ */
+static bool pci_bus_in_use(struct pci_bus *bus)
+{
+	bool in_use = false;
+
+	pci_walk_bus(bus, pci_dev_check_in_use, &in_use);
+	return in_use;
+}
+
 static void pci_configure_mps(struct pci_dev *dev)
 {
 	struct pci_dev *bridge = pci_upstream_bridge(dev);
+	struct pci_dev *rp;
 	int mps, mpss, p_mps, rc;
 
 	if (!pci_is_pcie(dev))
@@ -2259,10 +2310,29 @@ static void pci_configure_mps(struct pci_dev *dev)
 		return;
 
 	mpss = 128 << dev->pcie_mpss;
-	if (mpss < p_mps && pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) {
-		pcie_set_mps(bridge, mpss);
-		pci_info(dev, "Upstream bridge's Max Payload Size set to %d (was %d, max %d)\n",
-			 mpss, p_mps, 128 << bridge->pcie_mpss);
+	rp = pcie_find_root_port(bridge);
+	if (mpss < p_mps && rp && !pci_bus_in_use(rp->subordinate)) {
+		/*
+		 * dev cannot be programmed to the MPS already in use above
+		 * it, so reduce the hierarchy to what dev supports.  A Switch
+		 * does not split TLPs, so reducing only the upstream bridge
+		 * is not enough: every port up to the Root Port has to come
+		 * down as well, and so do the devices already programmed
+		 * below that Root Port, which would otherwise be left sending
+		 * TLPs too large for their egress port.
+		 *
+		 * Only do this while no device below the Root Port, other than
+		 * a Switch port, has been added or made available for driver
+		 * binding, e.g., during the initial scan or when hot-adding
+		 * into an empty slot directly below the Root Port or below a
+		 * Switch with no endpoint in use.  Such devices may have
+		 * drivers bound and DMA in flight, so their MPS can't be
+		 * changed safely (see pcie_find_smpss()).
+		 */
+		pcie_reduce_mps(rp, &mpss);
+		pci_walk_bus(rp->subordinate, pcie_reduce_mps, &mpss);
+		pci_info(dev, "Max Payload Size of %s hierarchy set to %d (was %d)\n",
+			 pci_name(rp), mpss, p_mps);
 		p_mps = pcie_get_mps(bridge);
 	}
 
-- 
2.56.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 1/3] PCI: Match the hierarchy's MPS to a device's MPSS as necessary

[Niklas Cassel]

From: Hans Zhang <18255117159@163.com>

pci_configure_mps() enumerates top-down and programs each device's Maximum
Payload Size (MPS) to match its upstream bridge.  When a device's MPS
Supported (MPSS) is too small to match, commit 9f0e89359775 ("PCI: Match
Root Port's MPS to endpoint's MPSS as necessary") reduces the upstream
bridge instead, but only when that bridge is a Root Port.

That covers an endpoint directly below a Root Port and nothing else.  With
a Switch in between, the Switch ports have already inherited the Root
Port's larger MPS, the reduction is skipped because the upstream bridge is
a Switch Downstream Port, and pcie_set_mps() then fails with -EINVAL for
the endpoint.  The endpoint is left below a port programmed for a larger
MPS, so any larger TLP it receives is treated as Malformed.

Multi-function devices hit the same hole from the other direction:
reducing the Root Port for a function with a small MPSS leaves the sibling
functions already programmed to the larger value.

Walk the hierarchy from the Root Port down and reduce every device that is
above the new value.  Reducing only the ports between the device and the
Root Port is not sufficient, because a Switch does not split TLPs: an
already programmed sibling left at the larger MPS could emit a TLP too
large for its egress port.  As a result, a single device with a small MPSS
now lowers the MPS of every device below its Root Port.

Only do this while no device below the Root Port, other than a Switch
port, has been added or made available for driver binding, i.e., during
the initial scan, or when devices are hot-added or rescanned into a
hierarchy without endpoints in use, such as an empty slot directly below
the Root Port or a Switch with nothing below it.  Such devices may have
drivers bound and DMA in flight, so their MPS can't be changed safely.
This is the same constraint that makes PCIE_BUS_SAFE limit fabrics with
hotplug bridges below a Root Port to 128 bytes in pcie_find_smpss().
Like the Root Port itself, Switch ports can still be changed: they don't
do DMA, and the port driver doesn't change Device Control after
enumeration.  A device added next to endpoints that are already in use
is left at its current MPS and pci_configure_mps() warns and suggests
"pci=pcie_bus_safe", which is what already happens below a Switch today.

This only affects PCIE_BUS_DEFAULT.  PCIE_BUS_SAFE, PCIE_BUS_PERFORMANCE
and PCIE_BUS_PEER2PEER program MPS in pcie_bus_configure_settings() and
PCIE_BUS_TUNE_OFF doesn't touch it, so all of them return before this
point.

Fixes: 9f0e89359775 ("PCI: Match Root Port's MPS to endpoint's MPSS as necessary")
Signed-off-by: Hans Zhang <18255117159@163.com>
Co-developed-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/probe.c | 78 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 74 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 9a178e0e5f7f..ccc3e9202c2c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2207,9 +2207,60 @@ int pci_setup_device(struct pci_dev *dev)
 	return 0;
 }
 
+static int pcie_reduce_mps(struct pci_dev *dev, void *data)
+{
+	int mps = *(int *)data;
+	int ret;
+
+	/* MPS is of type 'RsvdP' for VFs */
+	if (!pci_is_pcie(dev) || dev->is_virtfn)
+		return 0;
+
+	if (pcie_get_mps(dev) > mps) {
+		ret = pcie_set_mps(dev, mps);
+		if (ret)
+			pci_warn(dev, "can't set Max Payload Size to %d; if necessary, use \"pci=pcie_bus_safe\" and report a bug\n",
+				 mps);
+	}
+
+	return 0;
+}
+
+static int pci_dev_check_in_use(struct pci_dev *dev, void *data)
+{
+	bool *in_use = data;
+
+	/*
+	 * Switch ports don't do DMA, and the port driver doesn't change Device
+	 * Control after enumeration, so their MPS can be changed as long as no
+	 * device below them is in use.
+	 */
+	if (pci_is_pcie(dev) &&
+	    (pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM ||
+	     pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM))
+		return 0;
+
+	*in_use = pci_dev_is_added(dev) || !pci_dev_binding_disallowed(dev);
+	return *in_use;
+}
+
+/*
+ * Return true if any device on or below @bus, other than a Switch port, has
+ * been added or made available for driver binding, i.e., may have a driver
+ * bound and DMA in flight.
+ */
+static bool pci_bus_in_use(struct pci_bus *bus)
+{
+	bool in_use = false;
+
+	pci_walk_bus(bus, pci_dev_check_in_use, &in_use);
+	return in_use;
+}
+
 static void pci_configure_mps(struct pci_dev *dev)
 {
 	struct pci_dev *bridge = pci_upstream_bridge(dev);
+	struct pci_dev *rp;
 	int mps, mpss, p_mps, rc;
 
 	if (!pci_is_pcie(dev))
@@ -2259,10 +2310,29 @@ static void pci_configure_mps(struct pci_dev *dev)
 		return;
 
 	mpss = 128 << dev->pcie_mpss;
-	if (mpss < p_mps && pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) {
-		pcie_set_mps(bridge, mpss);
-		pci_info(dev, "Upstream bridge's Max Payload Size set to %d (was %d, max %d)\n",
-			 mpss, p_mps, 128 << bridge->pcie_mpss);
+	rp = pcie_find_root_port(bridge);
+	if (mpss < p_mps && rp && !pci_bus_in_use(rp->subordinate)) {
+		/*
+		 * dev cannot be programmed to the MPS already in use above
+		 * it, so reduce the hierarchy to what dev supports.  A Switch
+		 * does not split TLPs, so reducing only the upstream bridge
+		 * is not enough: every port up to the Root Port has to come
+		 * down as well, and so do the devices already programmed
+		 * below that Root Port, which would otherwise be left sending
+		 * TLPs too large for their egress port.
+		 *
+		 * Only do this while no device below the Root Port, other than
+		 * a Switch port, has been added or made available for driver
+		 * binding, e.g., during the initial scan or when hot-adding
+		 * into an empty slot directly below the Root Port or below a
+		 * Switch with no endpoint in use.  Such devices may have
+		 * drivers bound and DMA in flight, so their MPS can't be
+		 * changed safely (see pcie_find_smpss()).
+		 */
+		pcie_reduce_mps(rp, &mpss);
+		pci_walk_bus(rp->subordinate, pcie_reduce_mps, &mpss);
+		pci_info(dev, "Max Payload Size of %s hierarchy set to %d (was %d)\n",
+			 pci_name(rp), mpss, p_mps);
 		p_mps = pcie_get_mps(bridge);
 	}
 
-- 
2.56.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2 1/3] PCI: Match the hierarchy's MPS to a device's MPSS as necessary

[sashiko-bot]

> From: Hans Zhang <18255117159@163.com>
> 
> pci_configure_mps() enumerates top-down and programs each device's Maximum
> Payload Size (MPS) to match its upstream bridge.  When a device's MPS
> Supported (MPSS) is too small to match, commit 9f0e89359775 ("PCI: Match
> Root Port's MPS to endpoint's MPSS as necessary") reduces the upstream
> bridge instead, but only when that bridge is a Root Port.
> 
> That covers an endpoint directly below a Root Port and nothing else.  With
> a Switch in between, the Switch ports have already inherited the Root
> Port's larger MPS, the reduction is skipped because the upstream bridge is
> a Switch Downstream Port, and pcie_set_mps() then fails with -EINVAL for
> the endpoint.  The endpoint is left below a port programmed for a larger
> MPS, so any larger TLP it receives is treated as Malformed.
> 
> [ ... ]
> 
> Fixes: 9f0e89359775 ("PCI: Match Root Port's MPS to endpoint's MPSS as necessary")
> Signed-off-by: Hans Zhang <18255117159@163.com>
> Co-developed-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020122854.1500883-5-cassel@kernel.org?part=1

[PATCH v2 2/3] PCI: Configure Root Port MPS after scanning its hierarchy

[Niklas Cassel]

With the default MPS strategy (PCIE_BUS_DEFAULT), pci_configure_mps()
only matches each device's Maximum Payload Size (MPS) to its upstream
bridge.  Root Ports have no upstream bridge, so their MPS stays at
whatever firmware or the hardware default (128 bytes) left there, and the
hierarchy below them inherits that value even if the Root Port and all
devices below it support more.

Once the hierarchy below a Root Port has been scanned, and before drivers
are bound, set the Root Port and every device below it to the largest MPS
they all support, even if that is more than firmware programmed.  Do this
from pcie_bus_configure_settings(), which the host bridge, ACPI and
hotplug paths call after scanning, and from pci_rescan_bus() and
pci_rescan_bus_bridge_resize(), which don't.

If the hierarchy contains a hotplug bridge other than the Root Port,
typically a Switch Downstream Port, limit it to 128 bytes instead, like
pcie_find_smpss() does for PCIE_BUS_SAFE: a device hot-added below such a
port later can't lower the MPS of a hierarchy in use, but every device
supports 128 bytes.  This costs throughput where firmware programmed a
larger MPS for such a hierarchy, in exchange for hot-added devices always
working.

An empty slot directly below a Root Port is left alone; it is evaluated
when a device is hot-added or rescanned there, which also allows raising
the Root Port again after a device that supported less has been replaced.
Hierarchies in which a device other than a Switch port may already have
a driver bound are never changed.  If the MPS of a device can't be set,
restore the previous MPS of the whole hierarchy rather than leaving it
with mismatched settings.

Since PCIE_BUS_DEFAULT can now raise MPS above what firmware programmed,
apply the Intel 5000/5100 read completion coalescing quirk to it as well.

The other strategies are unchanged: PCIE_BUS_TUNE_OFF doesn't touch MPS,
and PCIE_BUS_SAFE, PCIE_BUS_PERFORMANCE and PCIE_BUS_PEER2PEER already
configure the hierarchy in pcie_bus_configure_settings().

Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
Co-developed-by: Hans Zhang <18255117159@163.com>
Signed-off-by: Hans Zhang <18255117159@163.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/probe.c  | 116 +++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/quirks.c |   3 +-
 2 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ccc3e9202c2c..ce3cd235a6a6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3102,6 +3102,115 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data)
 	return 0;
 }
 
+struct pcie_mps_update {
+	int mps;
+	bool restore;
+	int err;
+};
+
+static int pcie_update_mps(struct pci_dev *dev, void *data)
+{
+	struct pcie_mps_update *update = data;
+	int ret;
+
+	/* MPS is of type 'RsvdP' for VFs */
+	if (!pci_is_pcie(dev) || dev->is_virtfn ||
+	    pcie_get_mps(dev) == update->mps)
+		return 0;
+
+	ret = pcie_set_mps(dev, update->mps);
+	if (!ret)
+		return 0;
+
+	pci_err(dev, "can't set Max Payload Size to %d\n", update->mps);
+	update->err = ret;
+
+	/* When restoring, keep going to restore as many devices as possible */
+	return update->restore ? 0 : ret;
+}
+
+/*
+ * Set the MPS of @rp and of every device below it to @mps, top-down.  Unless
+ * @restore is set, stop at the first device that can't be set and return its
+ * error, so the caller can restore the previous MPS.
+ */
+static int pcie_set_hierarchy_mps(struct pci_dev *rp, int mps, bool restore)
+{
+	struct pcie_mps_update update = { .mps = mps, .restore = restore };
+
+	if (!pcie_update_mps(rp, &update))
+		pci_walk_bus(rp->subordinate, pcie_update_mps, &update);
+
+	return update.err;
+}
+
+/*
+ * With PCIE_BUS_DEFAULT, pci_configure_mps() only matches each device to its
+ * upstream bridge, so a hierarchy inherits whatever MPS firmware, or the
+ * hardware default of 128 bytes, left in its Root Port.  Once the hierarchy
+ * below a Root Port has been enumerated, set it to the largest MPS that all
+ * of its devices support.
+ *
+ * A device hot-added below a hotplug bridge other than the Root Port can't
+ * lower the MPS once the hierarchy is in use, so limit such hierarchies to
+ * 128 bytes, which every device supports, like pcie_find_smpss() does for
+ * PCIE_BUS_SAFE.  A hotplug slot directly below the Root Port doesn't need
+ * this: it is evaluated when a device is hot-added there, and an empty slot
+ * is left alone until then.  Hierarchies are also left alone once any of
+ * their devices, other than Switch ports, may have a driver bound.
+ */
+static void pcie_bus_configure_default_mps(struct pci_bus *bus)
+{
+	struct pci_dev *rp = bus->self;
+	u8 smpss = rp->pcie_mpss;
+	int mps, old_mps;
+
+	if (pci_pcie_type(rp) != PCI_EXP_TYPE_ROOT_PORT ||
+	    list_empty(&bus->devices) || pci_bus_in_use(bus))
+		return;
+
+	pci_walk_bus(bus, pcie_find_smpss, &smpss);
+	mps = 128 << smpss;
+	old_mps = pcie_get_mps(rp);
+
+	/* Don't leave the hierarchy with mismatched MPS settings on failure */
+	if (pcie_set_hierarchy_mps(rp, mps, false)) {
+		pci_warn(rp, "can't set Max Payload Size of hierarchy to %d, restoring %d\n",
+			 mps, old_mps);
+		pcie_set_hierarchy_mps(rp, old_mps, true);
+		return;
+	}
+
+	if (mps != old_mps)
+		pci_info(rp, "Max Payload Size of hierarchy set to %d (was %d)\n",
+			 mps, old_mps);
+}
+
+/*
+ * A rescan doesn't go through pcie_bus_configure_settings(), so configure
+ * the Root Port hierarchies it may have populated here: every hierarchy
+ * below a rescanned root bus, or the one @bus belongs to.
+ */
+static void pcie_rescan_configure_default_mps(struct pci_bus *bus)
+{
+	struct pci_bus *child;
+	struct pci_dev *rp;
+
+	if (pcie_bus_config != PCIE_BUS_DEFAULT)
+		return;
+
+	if (pci_is_root_bus(bus)) {
+		list_for_each_entry(child, &bus->children, node)
+			if (child->self && pci_is_pcie(child->self))
+				pcie_bus_configure_default_mps(child);
+		return;
+	}
+
+	rp = pcie_find_root_port(bus->self);
+	if (rp && rp->subordinate)
+		pcie_bus_configure_default_mps(rp->subordinate);
+}
+
 /*
  * pcie_bus_configure_settings() requires that pci_walk_bus work in a top-down,
  * parents then children fashion.  If this changes, then this code will not
@@ -3117,6 +3226,11 @@ void pcie_bus_configure_settings(struct pci_bus *bus)
 	if (!pci_is_pcie(bus->self))
 		return;
 
+	if (pcie_bus_config == PCIE_BUS_DEFAULT) {
+		pcie_bus_configure_default_mps(bus);
+		return;
+	}
+
 	/*
 	 * FIXME - Peer to peer DMA is possible, though the endpoint would need
 	 * to be aware of the MPS of the destination.  To work around this,
@@ -3555,6 +3669,7 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
 	max = pci_scan_child_bus(bus);
 
 	pci_assign_unassigned_bridge_resources(bridge);
+	pcie_rescan_configure_default_mps(bus);
 
 	pci_bus_add_devices(bus);
 
@@ -3576,6 +3691,7 @@ unsigned int pci_rescan_bus(struct pci_bus *bus)
 
 	max = pci_scan_child_bus(bus);
 	pci_assign_unassigned_bus_resources(bus);
+	pcie_rescan_configure_default_mps(bus);
 	pci_bus_add_devices(bus);
 
 	return max;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f1e6566542a4..31cbe7e91bb1 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3477,8 +3477,7 @@ static void quirk_intel_mc_errata(struct pci_dev *dev)
 	int err;
 	u16 rcc;
 
-	if (pcie_bus_config == PCIE_BUS_TUNE_OFF ||
-	    pcie_bus_config == PCIE_BUS_DEFAULT)
+	if (pcie_bus_config == PCIE_BUS_TUNE_OFF)
 		return;
 
 	/*
-- 
2.56.0

[PATCH v2 2/3] PCI: Configure Root Port MPS after scanning its hierarchy

[Niklas Cassel]

With the default MPS strategy (PCIE_BUS_DEFAULT), pci_configure_mps()
only matches each device's Maximum Payload Size (MPS) to its upstream
bridge.  Root Ports have no upstream bridge, so their MPS stays at
whatever firmware or the hardware default (128 bytes) left there, and the
hierarchy below them inherits that value even if the Root Port and all
devices below it support more.

Once the hierarchy below a Root Port has been scanned, and before drivers
are bound, set the Root Port and every device below it to the largest MPS
they all support, even if that is more than firmware programmed.  Do this
from pcie_bus_configure_settings(), which the host bridge, ACPI and
hotplug paths call after scanning, and from pci_rescan_bus() and
pci_rescan_bus_bridge_resize(), which don't.

If the hierarchy contains a hotplug bridge other than the Root Port,
typically a Switch Downstream Port, limit it to 128 bytes instead, like
pcie_find_smpss() does for PCIE_BUS_SAFE: a device hot-added below such a
port later can't lower the MPS of a hierarchy in use, but every device
supports 128 bytes.  This costs throughput where firmware programmed a
larger MPS for such a hierarchy, in exchange for hot-added devices always
working.

An empty slot directly below a Root Port is left alone; it is evaluated
when a device is hot-added or rescanned there, which also allows raising
the Root Port again after a device that supported less has been replaced.
Hierarchies in which a device other than a Switch port may already have
a driver bound are never changed.  If the MPS of a device can't be set,
restore the previous MPS of the whole hierarchy rather than leaving it
with mismatched settings.

Since PCIE_BUS_DEFAULT can now raise MPS above what firmware programmed,
apply the Intel 5000/5100 read completion coalescing quirk to it as well.

The other strategies are unchanged: PCIE_BUS_TUNE_OFF doesn't touch MPS,
and PCIE_BUS_SAFE, PCIE_BUS_PERFORMANCE and PCIE_BUS_PEER2PEER already
configure the hierarchy in pcie_bus_configure_settings().

Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
Co-developed-by: Hans Zhang <18255117159@163.com>
Signed-off-by: Hans Zhang <18255117159@163.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/probe.c  | 116 +++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/quirks.c |   3 +-
 2 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ccc3e9202c2c..ce3cd235a6a6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3102,6 +3102,115 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data)
 	return 0;
 }
 
+struct pcie_mps_update {
+	int mps;
+	bool restore;
+	int err;
+};
+
+static int pcie_update_mps(struct pci_dev *dev, void *data)
+{
+	struct pcie_mps_update *update = data;
+	int ret;
+
+	/* MPS is of type 'RsvdP' for VFs */
+	if (!pci_is_pcie(dev) || dev->is_virtfn ||
+	    pcie_get_mps(dev) == update->mps)
+		return 0;
+
+	ret = pcie_set_mps(dev, update->mps);
+	if (!ret)
+		return 0;
+
+	pci_err(dev, "can't set Max Payload Size to %d\n", update->mps);
+	update->err = ret;
+
+	/* When restoring, keep going to restore as many devices as possible */
+	return update->restore ? 0 : ret;
+}
+
+/*
+ * Set the MPS of @rp and of every device below it to @mps, top-down.  Unless
+ * @restore is set, stop at the first device that can't be set and return its
+ * error, so the caller can restore the previous MPS.
+ */
+static int pcie_set_hierarchy_mps(struct pci_dev *rp, int mps, bool restore)
+{
+	struct pcie_mps_update update = { .mps = mps, .restore = restore };
+
+	if (!pcie_update_mps(rp, &update))
+		pci_walk_bus(rp->subordinate, pcie_update_mps, &update);
+
+	return update.err;
+}
+
+/*
+ * With PCIE_BUS_DEFAULT, pci_configure_mps() only matches each device to its
+ * upstream bridge, so a hierarchy inherits whatever MPS firmware, or the
+ * hardware default of 128 bytes, left in its Root Port.  Once the hierarchy
+ * below a Root Port has been enumerated, set it to the largest MPS that all
+ * of its devices support.
+ *
+ * A device hot-added below a hotplug bridge other than the Root Port can't
+ * lower the MPS once the hierarchy is in use, so limit such hierarchies to
+ * 128 bytes, which every device supports, like pcie_find_smpss() does for
+ * PCIE_BUS_SAFE.  A hotplug slot directly below the Root Port doesn't need
+ * this: it is evaluated when a device is hot-added there, and an empty slot
+ * is left alone until then.  Hierarchies are also left alone once any of
+ * their devices, other than Switch ports, may have a driver bound.
+ */
+static void pcie_bus_configure_default_mps(struct pci_bus *bus)
+{
+	struct pci_dev *rp = bus->self;
+	u8 smpss = rp->pcie_mpss;
+	int mps, old_mps;
+
+	if (pci_pcie_type(rp) != PCI_EXP_TYPE_ROOT_PORT ||
+	    list_empty(&bus->devices) || pci_bus_in_use(bus))
+		return;
+
+	pci_walk_bus(bus, pcie_find_smpss, &smpss);
+	mps = 128 << smpss;
+	old_mps = pcie_get_mps(rp);
+
+	/* Don't leave the hierarchy with mismatched MPS settings on failure */
+	if (pcie_set_hierarchy_mps(rp, mps, false)) {
+		pci_warn(rp, "can't set Max Payload Size of hierarchy to %d, restoring %d\n",
+			 mps, old_mps);
+		pcie_set_hierarchy_mps(rp, old_mps, true);
+		return;
+	}
+
+	if (mps != old_mps)
+		pci_info(rp, "Max Payload Size of hierarchy set to %d (was %d)\n",
+			 mps, old_mps);
+}
+
+/*
+ * A rescan doesn't go through pcie_bus_configure_settings(), so configure
+ * the Root Port hierarchies it may have populated here: every hierarchy
+ * below a rescanned root bus, or the one @bus belongs to.
+ */
+static void pcie_rescan_configure_default_mps(struct pci_bus *bus)
+{
+	struct pci_bus *child;
+	struct pci_dev *rp;
+
+	if (pcie_bus_config != PCIE_BUS_DEFAULT)
+		return;
+
+	if (pci_is_root_bus(bus)) {
+		list_for_each_entry(child, &bus->children, node)
+			if (child->self && pci_is_pcie(child->self))
+				pcie_bus_configure_default_mps(child);
+		return;
+	}
+
+	rp = pcie_find_root_port(bus->self);
+	if (rp && rp->subordinate)
+		pcie_bus_configure_default_mps(rp->subordinate);
+}
+
 /*
  * pcie_bus_configure_settings() requires that pci_walk_bus work in a top-down,
  * parents then children fashion.  If this changes, then this code will not
@@ -3117,6 +3226,11 @@ void pcie_bus_configure_settings(struct pci_bus *bus)
 	if (!pci_is_pcie(bus->self))
 		return;
 
+	if (pcie_bus_config == PCIE_BUS_DEFAULT) {
+		pcie_bus_configure_default_mps(bus);
+		return;
+	}
+
 	/*
 	 * FIXME - Peer to peer DMA is possible, though the endpoint would need
 	 * to be aware of the MPS of the destination.  To work around this,
@@ -3555,6 +3669,7 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
 	max = pci_scan_child_bus(bus);
 
 	pci_assign_unassigned_bridge_resources(bridge);
+	pcie_rescan_configure_default_mps(bus);
 
 	pci_bus_add_devices(bus);
 
@@ -3576,6 +3691,7 @@ unsigned int pci_rescan_bus(struct pci_bus *bus)
 
 	max = pci_scan_child_bus(bus);
 	pci_assign_unassigned_bus_resources(bus);
+	pcie_rescan_configure_default_mps(bus);
 	pci_bus_add_devices(bus);
 
 	return max;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f1e6566542a4..31cbe7e91bb1 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3477,8 +3477,7 @@ static void quirk_intel_mc_errata(struct pci_dev *dev)
 	int err;
 	u16 rcc;
 
-	if (pcie_bus_config == PCIE_BUS_TUNE_OFF ||
-	    pcie_bus_config == PCIE_BUS_DEFAULT)
+	if (pcie_bus_config == PCIE_BUS_TUNE_OFF)
 		return;
 
 	/*
-- 
2.56.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 2/3] PCI: Configure Root Port MPS after scanning its hierarchy

[Niklas Cassel]

With the default MPS strategy (PCIE_BUS_DEFAULT), pci_configure_mps()
only matches each device's Maximum Payload Size (MPS) to its upstream
bridge.  Root Ports have no upstream bridge, so their MPS stays at
whatever firmware or the hardware default (128 bytes) left there, and the
hierarchy below them inherits that value even if the Root Port and all
devices below it support more.

Once the hierarchy below a Root Port has been scanned, and before drivers
are bound, set the Root Port and every device below it to the largest MPS
they all support, even if that is more than firmware programmed.  Do this
from pcie_bus_configure_settings(), which the host bridge, ACPI and
hotplug paths call after scanning, and from pci_rescan_bus() and
pci_rescan_bus_bridge_resize(), which don't.

If the hierarchy contains a hotplug bridge other than the Root Port,
typically a Switch Downstream Port, limit it to 128 bytes instead, like
pcie_find_smpss() does for PCIE_BUS_SAFE: a device hot-added below such a
port later can't lower the MPS of a hierarchy in use, but every device
supports 128 bytes.  This costs throughput where firmware programmed a
larger MPS for such a hierarchy, in exchange for hot-added devices always
working.

An empty slot directly below a Root Port is left alone; it is evaluated
when a device is hot-added or rescanned there, which also allows raising
the Root Port again after a device that supported less has been replaced.
Hierarchies in which a device other than a Switch port may already have
a driver bound are never changed.  If the MPS of a device can't be set,
restore the previous MPS of the whole hierarchy rather than leaving it
with mismatched settings.

Since PCIE_BUS_DEFAULT can now raise MPS above what firmware programmed,
apply the Intel 5000/5100 read completion coalescing quirk to it as well.

The other strategies are unchanged: PCIE_BUS_TUNE_OFF doesn't touch MPS,
and PCIE_BUS_SAFE, PCIE_BUS_PERFORMANCE and PCIE_BUS_PEER2PEER already
configure the hierarchy in pcie_bus_configure_settings().

Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
Co-developed-by: Hans Zhang <18255117159@163.com>
Signed-off-by: Hans Zhang <18255117159@163.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/probe.c  | 116 +++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/quirks.c |   3 +-
 2 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ccc3e9202c2c..ce3cd235a6a6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3102,6 +3102,115 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data)
 	return 0;
 }
 
+struct pcie_mps_update {
+	int mps;
+	bool restore;
+	int err;
+};
+
+static int pcie_update_mps(struct pci_dev *dev, void *data)
+{
+	struct pcie_mps_update *update = data;
+	int ret;
+
+	/* MPS is of type 'RsvdP' for VFs */
+	if (!pci_is_pcie(dev) || dev->is_virtfn ||
+	    pcie_get_mps(dev) == update->mps)
+		return 0;
+
+	ret = pcie_set_mps(dev, update->mps);
+	if (!ret)
+		return 0;
+
+	pci_err(dev, "can't set Max Payload Size to %d\n", update->mps);
+	update->err = ret;
+
+	/* When restoring, keep going to restore as many devices as possible */
+	return update->restore ? 0 : ret;
+}
+
+/*
+ * Set the MPS of @rp and of every device below it to @mps, top-down.  Unless
+ * @restore is set, stop at the first device that can't be set and return its
+ * error, so the caller can restore the previous MPS.
+ */
+static int pcie_set_hierarchy_mps(struct pci_dev *rp, int mps, bool restore)
+{
+	struct pcie_mps_update update = { .mps = mps, .restore = restore };
+
+	if (!pcie_update_mps(rp, &update))
+		pci_walk_bus(rp->subordinate, pcie_update_mps, &update);
+
+	return update.err;
+}
+
+/*
+ * With PCIE_BUS_DEFAULT, pci_configure_mps() only matches each device to its
+ * upstream bridge, so a hierarchy inherits whatever MPS firmware, or the
+ * hardware default of 128 bytes, left in its Root Port.  Once the hierarchy
+ * below a Root Port has been enumerated, set it to the largest MPS that all
+ * of its devices support.
+ *
+ * A device hot-added below a hotplug bridge other than the Root Port can't
+ * lower the MPS once the hierarchy is in use, so limit such hierarchies to
+ * 128 bytes, which every device supports, like pcie_find_smpss() does for
+ * PCIE_BUS_SAFE.  A hotplug slot directly below the Root Port doesn't need
+ * this: it is evaluated when a device is hot-added there, and an empty slot
+ * is left alone until then.  Hierarchies are also left alone once any of
+ * their devices, other than Switch ports, may have a driver bound.
+ */
+static void pcie_bus_configure_default_mps(struct pci_bus *bus)
+{
+	struct pci_dev *rp = bus->self;
+	u8 smpss = rp->pcie_mpss;
+	int mps, old_mps;
+
+	if (pci_pcie_type(rp) != PCI_EXP_TYPE_ROOT_PORT ||
+	    list_empty(&bus->devices) || pci_bus_in_use(bus))
+		return;
+
+	pci_walk_bus(bus, pcie_find_smpss, &smpss);
+	mps = 128 << smpss;
+	old_mps = pcie_get_mps(rp);
+
+	/* Don't leave the hierarchy with mismatched MPS settings on failure */
+	if (pcie_set_hierarchy_mps(rp, mps, false)) {
+		pci_warn(rp, "can't set Max Payload Size of hierarchy to %d, restoring %d\n",
+			 mps, old_mps);
+		pcie_set_hierarchy_mps(rp, old_mps, true);
+		return;
+	}
+
+	if (mps != old_mps)
+		pci_info(rp, "Max Payload Size of hierarchy set to %d (was %d)\n",
+			 mps, old_mps);
+}
+
+/*
+ * A rescan doesn't go through pcie_bus_configure_settings(), so configure
+ * the Root Port hierarchies it may have populated here: every hierarchy
+ * below a rescanned root bus, or the one @bus belongs to.
+ */
+static void pcie_rescan_configure_default_mps(struct pci_bus *bus)
+{
+	struct pci_bus *child;
+	struct pci_dev *rp;
+
+	if (pcie_bus_config != PCIE_BUS_DEFAULT)
+		return;
+
+	if (pci_is_root_bus(bus)) {
+		list_for_each_entry(child, &bus->children, node)
+			if (child->self && pci_is_pcie(child->self))
+				pcie_bus_configure_default_mps(child);
+		return;
+	}
+
+	rp = pcie_find_root_port(bus->self);
+	if (rp && rp->subordinate)
+		pcie_bus_configure_default_mps(rp->subordinate);
+}
+
 /*
  * pcie_bus_configure_settings() requires that pci_walk_bus work in a top-down,
  * parents then children fashion.  If this changes, then this code will not
@@ -3117,6 +3226,11 @@ void pcie_bus_configure_settings(struct pci_bus *bus)
 	if (!pci_is_pcie(bus->self))
 		return;
 
+	if (pcie_bus_config == PCIE_BUS_DEFAULT) {
+		pcie_bus_configure_default_mps(bus);
+		return;
+	}
+
 	/*
 	 * FIXME - Peer to peer DMA is possible, though the endpoint would need
 	 * to be aware of the MPS of the destination.  To work around this,
@@ -3555,6 +3669,7 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge)
 	max = pci_scan_child_bus(bus);
 
 	pci_assign_unassigned_bridge_resources(bridge);
+	pcie_rescan_configure_default_mps(bus);
 
 	pci_bus_add_devices(bus);
 
@@ -3576,6 +3691,7 @@ unsigned int pci_rescan_bus(struct pci_bus *bus)
 
 	max = pci_scan_child_bus(bus);
 	pci_assign_unassigned_bus_resources(bus);
+	pcie_rescan_configure_default_mps(bus);
 	pci_bus_add_devices(bus);
 
 	return max;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f1e6566542a4..31cbe7e91bb1 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3477,8 +3477,7 @@ static void quirk_intel_mc_errata(struct pci_dev *dev)
 	int err;
 	u16 rcc;
 
-	if (pcie_bus_config == PCIE_BUS_TUNE_OFF ||
-	    pcie_bus_config == PCIE_BUS_DEFAULT)
+	if (pcie_bus_config == PCIE_BUS_TUNE_OFF)
 		return;
 
 	/*
-- 
2.56.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2 2/3] PCI: Configure Root Port MPS after scanning its hierarchy

[sashiko-bot]

> With the default MPS strategy (PCIE_BUS_DEFAULT), pci_configure_mps()
> only matches each device's Maximum Payload Size (MPS) to its upstream
> bridge.  Root Ports have no upstream bridge, so their MPS stays at
> whatever firmware or the hardware default (128 bytes) left there, and the
> hierarchy below them inherits that value even if the Root Port and all
> devices below it support more.
> 
> Once the hierarchy below a Root Port has been scanned, and before drivers
> are bound, set the Root Port and every device below it to the largest MPS
> they all support, even if that is more than firmware programmed.  Do this
> from pcie_bus_configure_settings(), which the host bridge, ACPI and
> hotplug paths call after scanning, and from pci_rescan_bus() and
> pci_rescan_bus_bridge_resize(), which don't.
> 
> If the hierarchy contains a hotplug bridge other than the Root Port,
> [ ... ]
> 
> Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
> Co-developed-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020122854.1500883-5-cassel@kernel.org?part=2

[PATCH v2 3/3] PCI: meson: Remove redundant MPS configuration

[Niklas Cassel]

From: Hans Zhang <18255117159@163.com>

The Meson PCIe controller driver manually configures maximum payload
size (MPS) through meson_set_max_payload, duplicating functionality now
centralized in the PCI core.  Deprecating redundant code simplifies the
driver and aligns it with the consolidated MPS management strategy,
improving long-term maintainability.

With meson_set_max_payload() gone, meson_size_to_payload() is only used
for MRRS.  Rename it to meson_size_to_mrrs(), update its comment and
warning message, and drop the now-unused PCIE_CAP_MAX_PAYLOAD_SIZE and
MAX_PAYLOAD_SIZE macros.

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Hans Zhang <18255117159@163.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pci-meson.c | 27 ++++----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
index 8559d132dcde..48e88e425474 100644
--- a/drivers/pci/controller/dwc/pci-meson.c
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -21,7 +21,6 @@
 
 #define to_meson_pcie(x) dev_get_drvdata((x)->dev)
 
-#define PCIE_CAP_MAX_PAYLOAD_SIZE(x)	((x) << 5)
 #define PCIE_CAP_MAX_READ_REQ_SIZE(x)	((x) << 12)
 
 /* PCIe specific config registers */
@@ -37,7 +36,6 @@
 #define PM_CURRENT_STATE(x)		(((x) >> 7) & 0x1)
 
 #define PORT_CLK_RATE			100000000UL
-#define MAX_PAYLOAD_SIZE		256
 #define MAX_READ_REQ_SIZE		256
 #define PCIE_RESET_DELAY		500
 #define PCIE_SHARED_RESET		1
@@ -256,45 +254,29 @@ static void meson_pcie_ltssm_enable(struct meson_pcie *mp)
 	meson_cfg_writel(mp, val, PCIE_CFG0);
 }
 
-static int meson_size_to_payload(struct meson_pcie *mp, int size)
+static int meson_size_to_mrrs(struct meson_pcie *mp, int size)
 {
 	struct device *dev = mp->pci.dev;
 
 	/*
-	 * dwc supports 2^(val+7) payload size, which val is 0~5 default to 1.
+	 * dwc supports 2^(val+7) MRRS, which val is 0~5 default to 1.
 	 * So if input size is not 2^order alignment or less than 2^7 or bigger
 	 * than 2^12, just set to default size 2^(1+7).
 	 */
 	if (!is_power_of_2(size) || size < 128 || size > 4096) {
-		dev_warn(dev, "payload size %d, set to default 256\n", size);
+		dev_warn(dev, "MRRS %d, set to default 256\n", size);
 		return 1;
 	}
 
 	return fls(size) - 8;
 }
 
-static void meson_set_max_payload(struct meson_pcie *mp, int size)
-{
-	struct dw_pcie *pci = &mp->pci;
-	u32 val;
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	int max_payload_size = meson_size_to_payload(mp, size);
-
-	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
-	val &= ~PCI_EXP_DEVCTL_PAYLOAD;
-	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
-
-	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
-	val |= PCIE_CAP_MAX_PAYLOAD_SIZE(max_payload_size);
-	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
-}
-
 static void meson_set_max_rd_req_size(struct meson_pcie *mp, int size)
 {
 	struct dw_pcie *pci = &mp->pci;
 	u32 val;
 	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	int max_rd_req_size = meson_size_to_payload(mp, size);
+	int max_rd_req_size = meson_size_to_mrrs(mp, size);
 
 	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
 	val &= ~PCI_EXP_DEVCTL_READRQ;
@@ -363,7 +345,6 @@ static int meson_pcie_host_init(struct dw_pcie_rp *pp)
 
 	pp->bridge->ops = &meson_pci_ops;
 
-	meson_set_max_payload(mp, MAX_PAYLOAD_SIZE);
 	meson_set_max_rd_req_size(mp, MAX_READ_REQ_SIZE);
 
 	return 0;
-- 
2.56.0

[PATCH v2 3/3] PCI: meson: Remove redundant MPS configuration

[Niklas Cassel]

From: Hans Zhang <18255117159@163.com>

The Meson PCIe controller driver manually configures maximum payload
size (MPS) through meson_set_max_payload, duplicating functionality now
centralized in the PCI core.  Deprecating redundant code simplifies the
driver and aligns it with the consolidated MPS management strategy,
improving long-term maintainability.

With meson_set_max_payload() gone, meson_size_to_payload() is only used
for MRRS.  Rename it to meson_size_to_mrrs(), update its comment and
warning message, and drop the now-unused PCIE_CAP_MAX_PAYLOAD_SIZE and
MAX_PAYLOAD_SIZE macros.

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Hans Zhang <18255117159@163.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pci-meson.c | 27 ++++----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
index 8559d132dcde..48e88e425474 100644
--- a/drivers/pci/controller/dwc/pci-meson.c
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -21,7 +21,6 @@
 
 #define to_meson_pcie(x) dev_get_drvdata((x)->dev)
 
-#define PCIE_CAP_MAX_PAYLOAD_SIZE(x)	((x) << 5)
 #define PCIE_CAP_MAX_READ_REQ_SIZE(x)	((x) << 12)
 
 /* PCIe specific config registers */
@@ -37,7 +36,6 @@
 #define PM_CURRENT_STATE(x)		(((x) >> 7) & 0x1)
 
 #define PORT_CLK_RATE			100000000UL
-#define MAX_PAYLOAD_SIZE		256
 #define MAX_READ_REQ_SIZE		256
 #define PCIE_RESET_DELAY		500
 #define PCIE_SHARED_RESET		1
@@ -256,45 +254,29 @@ static void meson_pcie_ltssm_enable(struct meson_pcie *mp)
 	meson_cfg_writel(mp, val, PCIE_CFG0);
 }
 
-static int meson_size_to_payload(struct meson_pcie *mp, int size)
+static int meson_size_to_mrrs(struct meson_pcie *mp, int size)
 {
 	struct device *dev = mp->pci.dev;
 
 	/*
-	 * dwc supports 2^(val+7) payload size, which val is 0~5 default to 1.
+	 * dwc supports 2^(val+7) MRRS, which val is 0~5 default to 1.
 	 * So if input size is not 2^order alignment or less than 2^7 or bigger
 	 * than 2^12, just set to default size 2^(1+7).
 	 */
 	if (!is_power_of_2(size) || size < 128 || size > 4096) {
-		dev_warn(dev, "payload size %d, set to default 256\n", size);
+		dev_warn(dev, "MRRS %d, set to default 256\n", size);
 		return 1;
 	}
 
 	return fls(size) - 8;
 }
 
-static void meson_set_max_payload(struct meson_pcie *mp, int size)
-{
-	struct dw_pcie *pci = &mp->pci;
-	u32 val;
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	int max_payload_size = meson_size_to_payload(mp, size);
-
-	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
-	val &= ~PCI_EXP_DEVCTL_PAYLOAD;
-	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
-
-	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
-	val |= PCIE_CAP_MAX_PAYLOAD_SIZE(max_payload_size);
-	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
-}
-
 static void meson_set_max_rd_req_size(struct meson_pcie *mp, int size)
 {
 	struct dw_pcie *pci = &mp->pci;
 	u32 val;
 	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	int max_rd_req_size = meson_size_to_payload(mp, size);
+	int max_rd_req_size = meson_size_to_mrrs(mp, size);
 
 	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
 	val &= ~PCI_EXP_DEVCTL_READRQ;
@@ -363,7 +345,6 @@ static int meson_pcie_host_init(struct dw_pcie_rp *pp)
 
 	pp->bridge->ops = &meson_pci_ops;
 
-	meson_set_max_payload(mp, MAX_PAYLOAD_SIZE);
 	meson_set_max_rd_req_size(mp, MAX_READ_REQ_SIZE);
 
 	return 0;
-- 
2.56.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 3/3] PCI: meson: Remove redundant MPS configuration

[Niklas Cassel]

From: Hans Zhang <18255117159@163.com>

The Meson PCIe controller driver manually configures maximum payload
size (MPS) through meson_set_max_payload, duplicating functionality now
centralized in the PCI core.  Deprecating redundant code simplifies the
driver and aligns it with the consolidated MPS management strategy,
improving long-term maintainability.

With meson_set_max_payload() gone, meson_size_to_payload() is only used
for MRRS.  Rename it to meson_size_to_mrrs(), update its comment and
warning message, and drop the now-unused PCIE_CAP_MAX_PAYLOAD_SIZE and
MAX_PAYLOAD_SIZE macros.

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Hans Zhang <18255117159@163.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pci-meson.c | 27 ++++----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
index 8559d132dcde..48e88e425474 100644
--- a/drivers/pci/controller/dwc/pci-meson.c
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -21,7 +21,6 @@
 
 #define to_meson_pcie(x) dev_get_drvdata((x)->dev)
 
-#define PCIE_CAP_MAX_PAYLOAD_SIZE(x)	((x) << 5)
 #define PCIE_CAP_MAX_READ_REQ_SIZE(x)	((x) << 12)
 
 /* PCIe specific config registers */
@@ -37,7 +36,6 @@
 #define PM_CURRENT_STATE(x)		(((x) >> 7) & 0x1)
 
 #define PORT_CLK_RATE			100000000UL
-#define MAX_PAYLOAD_SIZE		256
 #define MAX_READ_REQ_SIZE		256
 #define PCIE_RESET_DELAY		500
 #define PCIE_SHARED_RESET		1
@@ -256,45 +254,29 @@ static void meson_pcie_ltssm_enable(struct meson_pcie *mp)
 	meson_cfg_writel(mp, val, PCIE_CFG0);
 }
 
-static int meson_size_to_payload(struct meson_pcie *mp, int size)
+static int meson_size_to_mrrs(struct meson_pcie *mp, int size)
 {
 	struct device *dev = mp->pci.dev;
 
 	/*
-	 * dwc supports 2^(val+7) payload size, which val is 0~5 default to 1.
+	 * dwc supports 2^(val+7) MRRS, which val is 0~5 default to 1.
 	 * So if input size is not 2^order alignment or less than 2^7 or bigger
 	 * than 2^12, just set to default size 2^(1+7).
 	 */
 	if (!is_power_of_2(size) || size < 128 || size > 4096) {
-		dev_warn(dev, "payload size %d, set to default 256\n", size);
+		dev_warn(dev, "MRRS %d, set to default 256\n", size);
 		return 1;
 	}
 
 	return fls(size) - 8;
 }
 
-static void meson_set_max_payload(struct meson_pcie *mp, int size)
-{
-	struct dw_pcie *pci = &mp->pci;
-	u32 val;
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	int max_payload_size = meson_size_to_payload(mp, size);
-
-	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
-	val &= ~PCI_EXP_DEVCTL_PAYLOAD;
-	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
-
-	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
-	val |= PCIE_CAP_MAX_PAYLOAD_SIZE(max_payload_size);
-	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
-}
-
 static void meson_set_max_rd_req_size(struct meson_pcie *mp, int size)
 {
 	struct dw_pcie *pci = &mp->pci;
 	u32 val;
 	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	int max_rd_req_size = meson_size_to_payload(mp, size);
+	int max_rd_req_size = meson_size_to_mrrs(mp, size);
 
 	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
 	val &= ~PCI_EXP_DEVCTL_READRQ;
@@ -363,7 +345,6 @@ static int meson_pcie_host_init(struct dw_pcie_rp *pp)
 
 	pp->bridge->ops = &meson_pci_ops;
 
-	meson_set_max_payload(mp, MAX_PAYLOAD_SIZE);
 	meson_set_max_rd_req_size(mp, MAX_READ_REQ_SIZE);
 
 	return 0;
-- 
2.56.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2 3/3] PCI: meson: Remove redundant MPS configuration

[sashiko-bot]

> From: Hans Zhang <18255117159@163.com>
> 
> The Meson PCIe controller driver manually configures maximum payload
> size (MPS) through meson_set_max_payload, duplicating functionality now
> centralized in the PCI core.  Deprecating redundant code simplifies the
> driver and aligns it with the consolidated MPS management strategy,
> improving long-term maintainability.
> 
> With meson_set_max_payload() gone, meson_size_to_payload() is only used
> for MRRS.  Rename it to meson_size_to_mrrs(), update its comment and
> warning message, and drop the now-unused PCIE_CAP_MAX_PAYLOAD_SIZE and
> MAX_PAYLOAD_SIZE macros.
> 
> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020122854.1500883-5-cassel@kernel.org?part=3