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

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