[PATCH] ide: make ide_pci_check_iomem() actually work

[PATCH] ide: make ide_pci_check_iomem() actually work

[Sergei Shtylyov]

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.

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);

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

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday 07 April 2008, Sergei Shtylyov 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.

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

[ IIRC for managed PCI devices these checks are done by generic code so
  once IDE got converted to use it we will get them as an added bonus... ]

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);

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

[Sergei Shtylyov]

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

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

[Sergei Shtylyov]

Hi, I just 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.

     Looks like we actually have this problem with scc_pata -- it calls 
ide_setup_pci_device() which should lead to calling ide_hwif_configure(). But 
this is broken since this call chain expects a normal PCI IDE controller with 
BAR0..BAR3 either non-existant or being primary/secondary port bases in I/O space.

>> Thanks,
>> Bart

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

WBR, Sergei

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

[Bartlomiej Zolnierkiewicz]

[ added Akira & Kou to cc: ]

On Tuesday 08 April 2008, Sergei Shtylyov wrote:
> Hi, I just 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.
> 
>      Looks like we actually have this problem with scc_pata -- it calls 
> ide_setup_pci_device() which should lead to calling ide_hwif_configure(). But 
> this is broken since this call chain expects a normal PCI IDE controller with 
> BAR0..BAR3 either non-existant or being primary/secondary port bases in I/O space.

Yep, scc_pata needs fixing before your patch can be applied.

Looks like it just needs to do all the needed setup itself in init_setup_scc()
instead of calling ide_setup_pci_device() [ similarly to how cs5520 host driver
handles this in cs5520.c::cs5520_init_one() ].

PS it would be also nice to call pci_enable_device() before setup_mmio_scc()
   (which accesses PCI BARs, calls pci_set_master() etc.) while we are at it
   (driver seems to work fine without it ATM but it may break in future).

Thanks,
Bart

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

[Bartlomiej Zolnierkiewicz]

On Wednesday 09 April 2008, Bartlomiej Zolnierkiewicz wrote:
> 
> [ added Akira & Kou to cc: ]
> 
> On Tuesday 08 April 2008, Sergei Shtylyov wrote:
> > Hi, I just 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.
> > 
> >      Looks like we actually have this problem with scc_pata -- it calls 
> > ide_setup_pci_device() which should lead to calling ide_hwif_configure(). But 
> > this is broken since this call chain expects a normal PCI IDE controller with 
> > BAR0..BAR3 either non-existant or being primary/secondary port bases in I/O space.
> 
> Yep, scc_pata needs fixing before your patch can be applied.

[...]

Sergei, I applied your patch just after scc_pata's one.

Thanks,
Bart