[PATCH] [SCSI] gdth: Remove buggy ROM handling

[PATCH] [SCSI] gdth: Remove buggy ROM handling

[Bjorn Helgaas]

The ROM address handling in gdth_init_pci() is useless and possibly
dangerous.  This patch removes it.

"pci_resource_start(pdev, 8)" is not well-defined.  PCI resources 0-5 are
standard PCI BARs and 6 is the expansion ROM.  Resource 8 is either an
SR-IOV BAR (if CONFIG_PCI_IOV=y, resources 7-12 are SR-IOV BARs) or a
bridge window (resources 7-10).

The GDT device is neither an SR-IOV device nor a bridge, so in either case
resource 8 should be zero since struct pci_dev is allocated with kzalloc().

It is illegal for a driver to write an arbitrary address to the ROM BAR
because it has no way of knowing whether the ROM will conflict with another
device.

I think the only effect of the code being removed was to:

  1) Enable the ROM at 0xFEFF0000 (possibly causing a conflict with
     another device)
  2) Delay one millisecond
  3) Write zero to the ROM BAR, disabling it

I doubt the delay is needed, but I left it since it seems innocuous.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/scsi/gdth.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index 5d72274..3efe4ef 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -1107,14 +1107,8 @@ static int __devinit gdth_init_pci(struct pci_dev *pdev, gdth_pci_str *pcistr,
 	pci_read_config_word(pdev, PCI_COMMAND, &command);
         command |= 6;
 	pci_write_config_word(pdev, PCI_COMMAND, command);
-	if (pci_resource_start(pdev, 8) == 1UL)
-	    pci_resource_start(pdev, 8) = 0UL;
-        i = 0xFEFF0001UL;
-	pci_write_config_dword(pdev, PCI_ROM_ADDRESS, i);
-        gdth_delay(1);
-	pci_write_config_dword(pdev, PCI_ROM_ADDRESS,
-			       pci_resource_start(pdev, 8));
-        
+	gdth_delay(1);
+
         dp6m_ptr = ha->brd;
 
         /* Ensure that it is safe to access the non HW portions of DPMEM.

Re: [PATCH] [SCSI] gdth: Remove buggy ROM handling

[Bjorn Helgaas]

On Tue, Nov 6, 2012 at 3:04 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> The ROM address handling in gdth_init_pci() is useless and possibly
> dangerous.  This patch removes it.
>
> "pci_resource_start(pdev, 8)" is not well-defined.  PCI resources 0-5 are
> standard PCI BARs and 6 is the expansion ROM.  Resource 8 is either an
> SR-IOV BAR (if CONFIG_PCI_IOV=y, resources 7-12 are SR-IOV BARs) or a
> bridge window (resources 7-10).
>
> The GDT device is neither an SR-IOV device nor a bridge, so in either case
> resource 8 should be zero since struct pci_dev is allocated with kzalloc().
>
> It is illegal for a driver to write an arbitrary address to the ROM BAR
> because it has no way of knowing whether the ROM will conflict with another
> device.
>
> I think the only effect of the code being removed was to:
>
>   1) Enable the ROM at 0xFEFF0000 (possibly causing a conflict with
>      another device)
>   2) Delay one millisecond
>   3) Write zero to the ROM BAR, disabling it
>
> I doubt the delay is needed, but I left it since it seems innocuous.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/scsi/gdth.c |   10 ++--------
>  1 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index 5d72274..3efe4ef 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -1107,14 +1107,8 @@ static int __devinit gdth_init_pci(struct pci_dev *pdev, gdth_pci_str *pcistr,
>         pci_read_config_word(pdev, PCI_COMMAND, &command);
>          command |= 6;
>         pci_write_config_word(pdev, PCI_COMMAND, command);
> -       if (pci_resource_start(pdev, 8) == 1UL)
> -           pci_resource_start(pdev, 8) = 0UL;
> -        i = 0xFEFF0001UL;
> -       pci_write_config_dword(pdev, PCI_ROM_ADDRESS, i);
> -        gdth_delay(1);
> -       pci_write_config_dword(pdev, PCI_ROM_ADDRESS,
> -                              pci_resource_start(pdev, 8));
> -
> +       gdth_delay(1);
> +
>          dp6m_ptr = ha->brd;
>
>          /* Ensure that it is safe to access the non HW portions of DPMEM.
>

Ping?