Re: [PATCH v4 7/7] PCI: Add comments about ROM BAR updating

[Gavin Shan <gwshan@xxxxxxxxxxxxxxxxxx>]

On Mon, Nov 28, 2016 at 10:16:48PM -0600, Bjorn Helgaas wrote:
>pci_update_resource() updates a hardware BAR so its address matches the
>kernel's struct resource UNLESS it's a disabled ROM BAR.  We only update
>those when we enable the ROM.
>
>It's not obvious from the code why ROM BARs should be handled specially.
>Apparently there are Matrox devices with defective ROM BARs that read as
>zero when disabled.  That means that if pci_enable_rom() reads the disabled
>BAR, sets PCI_ROM_ADDRESS_ENABLE (without re-inserting the address), and
>writes it back, it would enable the ROM at address zero.
>
>Add comments and references to explain why we can't make the code look more
>rational.
>
>The code changes are from 755528c860b0 ("Ignore disabled ROM resources at
>setup") and 8085ce084c0f ("[PATCH] Fix PCI ROM mapping").
>
>Link: https://lkml.org/lkml/2005/8/30/138
>Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>

Reviewed-by: Gavin Shan <gwshan@xxxxxxxxxxxxxxxxxx>

>---
> drivers/pci/rom.c       |    5 +++++
> drivers/pci/setup-res.c |    6 ++++++
> 2 files changed, 11 insertions(+)
>
>diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
>index 06663d3..b6edb18 100644
>--- a/drivers/pci/rom.c
>+++ b/drivers/pci/rom.c
>@@ -35,6 +35,11 @@ int pci_enable_rom(struct pci_dev *pdev)
> 	if (res->flags & IORESOURCE_ROM_SHADOW)
> 		return 0;
>
>+	/*
>+	 * Ideally pci_update_resource() would update the ROM BAR address,
>+	 * and we would only set the enable bit here.  But apparently some
>+	 * devices have buggy ROM BARs that read as zero when disabled.
>+	 */
> 	pcibios_resource_to_bus(pdev->bus, &region, res);
> 	pci_read_config_dword(pdev, pdev->rom_base_reg, &rom_addr);
> 	rom_addr &= ~PCI_ROM_ADDRESS_MASK;
>diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
>index 09bdff7..57d7041 100644
>--- a/drivers/pci/setup-res.c
>+++ b/drivers/pci/setup-res.c
>@@ -67,6 +67,12 @@ static void pci_std_update_resource(struct pci_dev *dev, int resno)
> 	if (resno < PCI_ROM_RESOURCE) {
> 		reg = PCI_BASE_ADDRESS_0 + 4 * resno;
> 	} else if (resno == PCI_ROM_RESOURCE) {
>+
>+		/*
>+		 * Apparently some Matrox devices have ROM BARs that read
>+		 * as zero when disabled, so don't update ROM BARs unless
>+		 * they're enabled.  See https://lkml.org/lkml/2005/8/30/138.
>+		 */
> 		if (!(res->flags & IORESOURCE_ROM_ENABLE))
> 			return;
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>the body of a message to majordomo@xxxxxxxxxxxxxxx
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html