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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>