Re: [PATCH 3/6] PCI: restore memory decoding after reallocation

Bjorn Helgaas
2024-07-09T16:16:48Z

On Tue, Jul 09, 2024 at 09:36:00AM -0400, Stewart Hildebrand wrote:
> Currently, the PCI subsystem unconditionally clears the memory decoding
> bit of devices with resource alignment specified. Unfortunately, some
> drivers assume memory decoding was enabled by firmware. Restore the
> memory decoding bit after the resource has been reallocated.

Which drivers have you tripped over? Those drivers apparently don't
call pci_enable_device() and assume the firmware has left memory
decoding enabled. I don't think there's any guarantee that firmware
must do that, so the drivers are probably broken on some platforms,
and we could improve things overall by adding the pci_enable_device()
to them.

> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> Relevant prior discussion at [1]
>
> [1] https://lore.kernel.org/linux-pci/20160906165652.GE1214@localhost/
> ---
> drivers/pci/pci.c | 1 +
> drivers/pci/setup-bus.c | 25 +++++++++++++++++++++++++
> include/linux/pci.h | 2 ++
> 3 files changed, 28 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f017e7a8f028..7953e75b9129 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6633,6 +6633,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>
> pci_read_config_word(dev, PCI_COMMAND, &command);
> if (command & PCI_COMMAND_MEMORY) {
> + dev->dev_flags |= PCI_DEV_FLAGS_MEMORY_ENABLE;
> command &= ~PCI_COMMAND_MEMORY;
> pci_write_config_word(dev, PCI_COMMAND, command);
> }

It would be nice if this could be contained to
pci_reassigndev_resource_alignment() so the clear and restore could be
in the same function. But I suppose the concern is that re-enabling
decoding too early could be an issue in a hierarchy where bridge
windows are also reassigned?

> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index ab7510ce6917..6847b251e19a 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -2131,6 +2131,29 @@ pci_root_bus_distribute_available_resources(struct pci_bus *bus,
> }
> }
>
> +static void restore_memory_decoding(struct pci_bus *bus)
> +{
> + struct pci_dev *dev;
> +
> + list_for_each_entry(dev, &bus->devices, bus_list) {
> + struct pci_bus *b;
> +
> + if (dev->dev_flags & PCI_DEV_FLAGS_MEMORY_ENABLE) {
> + u16 command;
> +
> + pci_read_config_word(dev, PCI_COMMAND, &command);
> + command |= PCI_COMMAND_MEMORY;
> + pci_write_config_word(dev, PCI_COMMAND, command);
> + }
> +
> + b = dev->subordinate;
> + if (!b)
> + continue;
> +
> + restore_memory_decoding(b);
> + }
> +}
> +
> /*
> * First try will not touch PCI bridge res.
> * Second and later try will clear small leaf bridge res.
> @@ -2229,6 +2252,8 @@ void pci_assign_unassigned_root_bus_resources(struct pci_bus *bus)
> goto again;
>
> dump:
> + restore_memory_decoding(bus);
> +
> /* Dump the resource on buses */
> pci_bus_dump_resources(bus);
> }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index e83ac93a4dcb..cb5df4c6a999 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -245,6 +245,8 @@ enum pci_dev_flags {
> PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
> /* Device does honor MSI masking despite saying otherwise */
> PCI_DEV_FLAGS_HAS_MSI_MASKING = (__force pci_dev_flags_t) (1 << 12),
> + /* Firmware enabled memory decoding, to be restored if BAR is updated */
> + PCI_DEV_FLAGS_MEMORY_ENABLE = (__force pci_dev_flags_t) (1 << 13),
> };
>
> enum pci_irq_reroute_variant {
> --
> 2.45.2
>