Re: [PATCH] ide: make ide_pci_check_iomem() actually work

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>This function didn't actually check if a given BAR is in I/O space because of
>>using the bogus PCI_BASE_ADDRESS_IO_MASK (which equals ~3) to test the resource
>>flags instead of IORESOURCE_IO -- fix this, make ide_hwif_configure() check the
>>results failing if necessary, and move the printk() call to the failure path.

> This change is OK in itself but I worry that ide_pci_check_iomem() may now
> return "false" errors (bogus PCI_BASE_ADDRESS_IO_MASK check resulted in MEM
> resources always surviving ide_pci_check_iomem() calls before the fix) for
> some host drivers (siimage, scc_pata...) resulting in failed initialization.

    The SiI chips do have normal I/O resources at BAR0..BAR3. As for scc_pata, 
the control should not even get there because BAR0..BAR3 are *not* IDE 
command/control block bases on this chip (BAR0/1 are control/DMA bases if you 
look into setup_mmio_scc()) but they are treated as such by the code 
immediately following ide_pci_check_iomem() calls in ide_hwif_configure(), 
i.e. we might have an error here. The same can be said about the PowerMAC 
driver which has all its MMIO registers at BAR0.

> How's about removing this dead/broken function instead for now?

    If we indeed have a MMIO problem here, it's not in this function but in 
its callers.

> Thanks,
> Bart

>>Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

>>---
>>The patch is against today's Linus' tree...

>> drivers/ide/setup-pci.c |   23 ++++++++++++-----------
>> 1 files changed, 12 insertions(+), 11 deletions(-)

>>Index: linux-2.6/drivers/ide/setup-pci.c
>>===================================================================
>>--- linux-2.6.orig/drivers/ide/setup-pci.c
>>+++ linux-2.6/drivers/ide/setup-pci.c
>>@@ -312,11 +312,12 @@ static int ide_pci_configure(struct pci_
>>  *	@d: IDE port info
>>  *	@bar: BAR number
>>  *
>>- *	Checks if a BAR is configured and points to MMIO space. If so
>>- *	print an error and return an error code. Otherwise return 0
>>+ *	Checks if a BAR is configured and points to MMIO space. If so,
>>+ *	return an error code. Otherwise return 0
>>  */
>> 
>>-static int ide_pci_check_iomem(struct pci_dev *dev, const struct ide_port_info *d, int bar)
>>+static int ide_pci_check_iomem(struct pci_dev *dev, const struct ide_port_info *d,
>>+			       int bar)
>> {
>> 	ulong flags = pci_resource_flags(dev, bar);
>> 	
>>@@ -324,14 +325,11 @@ static int ide_pci_check_iomem(struct pc
>> 	if (!flags || pci_resource_len(dev, bar) == 0)
>> 		return 0;
>> 
>>-	/* I/O space */		
>>-	if(flags & PCI_BASE_ADDRESS_IO_MASK)
>>+	/* I/O space */
>>+	if (flags & IORESOURCE_IO)
>> 		return 0;
>> 		
>> 	/* Bad */
>>-	printk(KERN_ERR "%s: IO baseregs (BIOS) are reported "
>>-			"as MEM, report to "
>>-			"<andre@linux-ide.org>.\n", d->name);
>> 	return -EINVAL;
>> }
>> 
>>@@ -360,9 +358,12 @@ static ide_hwif_t *ide_hwif_configure(st
>> 	struct hw_regs_s hw;
>> 
>> 	if ((d->host_flags & IDE_HFLAG_ISA_PORTS) == 0) {
>>-		/*  Possibly we should fail if these checks report true */
>>-		ide_pci_check_iomem(dev, d, 2*port);
>>-		ide_pci_check_iomem(dev, d, 2*port+1);
>>+		if (ide_pci_check_iomem(dev, d, 2 * port) ||
>>+		    ide_pci_check_iomem(dev, d, 2 * port + 1)) {
>>+			printk(KERN_ERR "%s: I/O baseregs (BIOS) are reported "
>>+					"as MEM for port %d!\n", d->name, port);
>>+			return NULL;
>>+		}
>>  
>> 		ctl  = pci_resource_start(dev, 2*port+1);
>> 		base = pci_resource_start(dev, 2*port);

    Above's the code that I was talking about...

WBR, Sergei