[patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Ben Dooks]

[-- Attachment #1: simtec/simtec-libata-deal-with-unmapped-irqs.patch --]
[-- Type: text/plain, Size: 1292 bytes --]

The Simtec CATS board is an example of an ALI M5229 that
is not correclty supported by libata as the pdev->irq field
has no valid interrupt set. This is due to the legacy IRQ
check in ata_pci_sff_activate_host() only checking to see 
if the device class is legacy, not that the IRQ has been
setup.

Change ata_pci_sff_activate_host() to try the legacy IRQ
numbers (which works correctly) when the dev->irq has not
been set. The current check for !legacy_mode && dev->irq
does not deal with devices that are not in legacy mode
but do not have mapped interrupts.

Signed-off-by: Ben Dooks <ben-linux@fluff.org>

Index: linux-2.6.26-quilt2/drivers/ata/libata-sff.c
===================================================================
--- linux-2.6.26-quilt2.orig/drivers/ata/libata-sff.c	2008-07-23 15:11:36.000000000 +0100
+++ linux-2.6.26-quilt2/drivers/ata/libata-sff.c	2008-07-23 15:11:48.000000000 +0100
@@ -2702,7 +2702,7 @@ int ata_pci_sff_activate_host(struct ata
 
 		ata_port_desc(host->ports[0], "irq %d", pdev->irq);
 		ata_port_desc(host->ports[1], "irq %d", pdev->irq);
-	} else if (legacy_mode) {
+	} else if (legacy_mode || !pdev->irq) {
 		if (!ata_port_is_dummy(host->ports[0])) {
 			rc = devm_request_irq(dev, ATA_PRIMARY_IRQ(pdev),
 					      irq_handler, IRQF_SHARED,

-- 

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Alan Cox]

> is not correclty supported by libata as the pdev->irq field
> has no valid interrupt set. This is due to the legacy IRQ
> check in ata_pci_sff_activate_host() only checking to see 
> if the device class is legacy, not that the IRQ has been
> setup.

It does that check as that is the correct check to do. If the device is
in native mode with no assigned IRQ we must poll, we cannot assume that
legacy interrupts are in use.

> Change ata_pci_sff_activate_host() to try the legacy IRQ
> numbers (which works correctly) when the dev->irq has not
> been set. The current check for !legacy_mode && dev->irq
> does not deal with devices that are not in legacy mode
> but do not have mapped interrupts.

If they have no mapped interrupt and are not in legacy mode then they
have no interrupt we can use and we should poll. Your assumption that if
no IRQ is assigned we should use the legacy interrupts is incorrect for
PC class systems.

Can you not correct this with a PCI quirk handler for your board instead ?

Alan

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Ben Dooks]

On Wed, Jul 23, 2008 at 04:32:16PM +0100, Alan Cox wrote:
> > is not correclty supported by libata as the pdev->irq field
> > has no valid interrupt set. This is due to the legacy IRQ
> > check in ata_pci_sff_activate_host() only checking to see 
> > if the device class is legacy, not that the IRQ has been
> > setup.
> 
> It does that check as that is the correct check to do. If the device is
> in native mode with no assigned IRQ we must poll, we cannot assume that
> legacy interrupts are in use.
> 
> > Change ata_pci_sff_activate_host() to try the legacy IRQ
> > numbers (which works correctly) when the dev->irq has not
> > been set. The current check for !legacy_mode && dev->irq
> > does not deal with devices that are not in legacy mode
> > but do not have mapped interrupts.
> 
> If they have no mapped interrupt and are not in legacy mode then they
> have no interrupt we can use and we should poll. Your assumption that if
> no IRQ is assigned we should use the legacy interrupts is incorrect for
> PC class systems.
> 
> Can you not correct this with a PCI quirk handler for your board instead ?

Ok, however the drivers/ide/pci/alim15x3.c still works fine
on this board, so there's something that something in the libata
driver path not working properly. Possibly drivers/ata/pata_ali.c
that is missing something

I will check this over tommorow and come up with a new solution.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Alan Cox]

> Ok, however the drivers/ide/pci/alim15x3.c still works fine
> on this board, so there's something that something in the libata
> driver path not working properly. Possibly drivers/ata/pata_ali.c
> that is missing something

The old driver makes some rather iffy assumptions when dev->irq == 0 in
part because the old IDE code predates PCI quirks.

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Ben Dooks]

On Wed, Jul 23, 2008 at 08:13:49PM +0100, Alan Cox wrote:
> > Ok, however the drivers/ide/pci/alim15x3.c still works fine
> > on this board, so there's something that something in the libata
> > driver path not working properly. Possibly drivers/ata/pata_ali.c
> > that is missing something
> 
> The old driver makes some rather iffy assumptions when dev->irq == 0 in
> part because the old IDE code predates PCI quirks.

>From reading the ALI M1543 datasheets that the interrupt pin in either
mode (compatibility or native) is routed directly into the inbuilt ISA-PIC,
and thus the IRQ field in the PCI child device is not available. This
means we need to be able to override the IRQ numbers for these devices.

I had a look, and there doesn't seem to be any way of specifying an
IRQ for a host port when registering with libata-sff.c, which is a
problem since we need to be able to either pass-in or have a callback
to allow our "quirk" to specify a new IRQ for the device.

Does anyone know if the ALi M5229 actually exists outside of one of
the M1543 bridges?

Note, this hardly seems to be a board-quirk, it is more of a device
quirk...

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Alan Cox]

> I had a look, and there doesn't seem to be any way of specifying an
> IRQ for a host port when registering with libata-sff.c, which is a
> problem since we need to be able to either pass-in or have a callback
> to allow our "quirk" to specify a new IRQ for the device.

Currently there is not. However if the device as a whole is wired to IRQ
14 you want to quirk pdev->irq.

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Ben Dooks]

On Thu, Jul 24, 2008 at 12:13:28PM +0100, Alan Cox wrote:
> > I had a look, and there doesn't seem to be any way of specifying an
> > IRQ for a host port when registering with libata-sff.c, which is a
> > problem since we need to be able to either pass-in or have a callback
> > to allow our "quirk" to specify a new IRQ for the device.
> 
> Currently there is not. However if the device as a whole is wired to IRQ
> 14 you want to quirk pdev->irq.

That would be great if both channels are mapped to 14, however the
mapping set by default is for each channel to have an unique IRQ.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Alan Cox]

On Thu, 24 Jul 2008 12:50:42 +0100
Ben Dooks <ben-linux@fluff.org> wrote:

> On Thu, Jul 24, 2008 at 12:13:28PM +0100, Alan Cox wrote:
> > > I had a look, and there doesn't seem to be any way of specifying an
> > > IRQ for a host port when registering with libata-sff.c, which is a
> > > problem since we need to be able to either pass-in or have a callback
> > > to allow our "quirk" to specify a new IRQ for the device.
> > 
> > Currently there is not. However if the device as a whole is wired to IRQ
> > 14 you want to quirk pdev->irq.
> 
> That would be great if both channels are mapped to 14, however the
> mapping set by default is for each channel to have an unique IRQ.

Ok so you've got a board reporting native mode using the legacy IRQ
numbering (14/15 - or platform equivalents) ? In which case may I suggest
you rewrite the header to indicate it is in legacy mode as per the BIOS
guide ?

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Ben Dooks]

On Thu, Jul 24, 2008 at 12:59:22PM +0100, Alan Cox wrote:
> On Thu, 24 Jul 2008 12:50:42 +0100
> Ben Dooks <ben-linux@fluff.org> wrote:
> 
> > On Thu, Jul 24, 2008 at 12:13:28PM +0100, Alan Cox wrote:
> > > > I had a look, and there doesn't seem to be any way of specifying an
> > > > IRQ for a host port when registering with libata-sff.c, which is a
> > > > problem since we need to be able to either pass-in or have a callback
> > > > to allow our "quirk" to specify a new IRQ for the device.
> > > 
> > > Currently there is not. However if the device as a whole is wired to IRQ
> > > 14 you want to quirk pdev->irq.
> > 
> > That would be great if both channels are mapped to 14, however the
> > mapping set by default is for each channel to have an unique IRQ.
> 
> Ok so you've got a board reporting native mode using the legacy IRQ
> numbering (14/15 - or platform equivalents) ? In which case may I suggest
> you rewrite the header to indicate it is in legacy mode as per the BIOS
> guide ?

The drivers/ide/pci/alim15x3.c driver currently has code to correctly
set the IRQ fields for anything that isn't SPARC. Does this mean we
must disable the libata driver for anything that isn't SPARC? What about
other boards where this device combination is present?

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Alan Cox]

> > Ok so you've got a board reporting native mode using the legacy IRQ
> > numbering (14/15 - or platform equivalents) ? In which case may I suggest
> > you rewrite the header to indicate it is in legacy mode as per the BIOS
> > guide ?
> 
> The drivers/ide/pci/alim15x3.c driver currently has code to correctly
> set the IRQ fields for anything that isn't SPARC. Does this mean we
> must disable the libata driver for anything that isn't SPARC? What about
> other boards where this device combination is present?

There should be no boards where this combination is present. IRQ 0 in
native mode means "polled". It would therefore be helpful if you would
start considering your board as a problem special case - one we need to
support yes - rather than trying to argue that we should break support
for standard configurations.

Alan

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Ben Dooks]

On Thu, Jul 24, 2008 at 02:52:15PM +0100, Alan Cox wrote:
> > > Ok so you've got a board reporting native mode using the legacy IRQ
> > > numbering (14/15 - or platform equivalents) ? In which case may I suggest
> > > you rewrite the header to indicate it is in legacy mode as per the BIOS
> > > guide ?
> > 
> > The drivers/ide/pci/alim15x3.c driver currently has code to correctly
> > set the IRQ fields for anything that isn't SPARC. Does this mean we
> > must disable the libata driver for anything that isn't SPARC? What about
> > other boards where this device combination is present?
> 
> There should be no boards where this combination is present. IRQ 0 in
> native mode means "polled". It would therefore be helpful if you would
> start considering your board as a problem special case - one we need to
> support yes - rather than trying to argue that we should break support
> for standard configurations.

So just because we fit a chip, we're suddenyly a special case? Moving
to libata has ignored the code in the old IDE driver which ensures that
the IRQ driver is used. I have no idea how many other systems have this
same problems, but the systems we've shipped have had this chip setup
for nearly 10 years now.

I admit the original fix is wrong, the change should be handled by some
form of callback or a method of passing the interrupt numbers in when
registering with the libata-sff.c driver.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Alan Cox]

> So just because we fit a chip, we're suddenyly a special case? Moving
> to libata has ignored the code in the old IDE driver which ensures that
> the IRQ driver is used. I have no idea how many other systems have this
> same problems, but the systems we've shipped have had this chip setup
> for nearly 10 years now.

Yes and it happened to work by luck. Now it doesn't, instead standards
compliant systems work better.

> I admit the original fix is wrong, the change should be handled by some
> form of callback or a method of passing the interrupt numbers in when
> registering with the libata-sff.c driver.

Why do you want to hack up the libata driver code ? You've said yourself
you have put the chip into native mode yet are using the legacy
interrupts. Thats misconfiguration and you can undo that by configuring
the chip right in the first place - as per the BIOS guide.

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Bartlomiej Zolnierkiewicz]

Hi,

On Thursday 24 July 2008, Ben Dooks wrote:
> On Thu, Jul 24, 2008 at 02:52:15PM +0100, Alan Cox wrote:
> > > > Ok so you've got a board reporting native mode using the legacy IRQ
> > > > numbering (14/15 - or platform equivalents) ? In which case may I suggest
> > > > you rewrite the header to indicate it is in legacy mode as per the BIOS
> > > > guide ?
> > > 
> > > The drivers/ide/pci/alim15x3.c driver currently has code to correctly
> > > set the IRQ fields for anything that isn't SPARC. Does this mean we
> > > must disable the libata driver for anything that isn't SPARC? What about
> > > other boards where this device combination is present?
> > 
> > There should be no boards where this combination is present. IRQ 0 in
> > native mode means "polled". It would therefore be helpful if you would
> > start considering your board as a problem special case - one we need to
> > support yes - rather than trying to argue that we should break support
> > for standard configurations.
> 
> So just because we fit a chip, we're suddenyly a special case? Moving
> to libata has ignored the code in the old IDE driver which ensures that
> the IRQ driver is used. I have no idea how many other systems have this
> same problems, but the systems we've shipped have had this chip setup
> for nearly 10 years now.

Please note that the recommended PATA support in kernel.org kernels
is IDE subsystem and libata PATA is still considered experimental.

> I admit the original fix is wrong, the change should be handled by some
> form of callback or a method of passing the interrupt numbers in when
> registering with the libata-sff.c driver.

Seems like the good solution would be to:

- add PCI HEADER quirk to claim legacy mode (as suggested by Alan)

- move ALi IRQ handling code to PCI layer and then hook it into
  pci_get_legacy_ide_irq()

Thanks,
Bart

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Alan Cox]

> Please note that the recommended PATA support in kernel.org kernels
> is IDE subsystem and libata PATA is still considered experimental.

This is false. Most distributions now use libata, and the old IDE code
has recently had an enormous amount of patching so is if anything far
more experimental than libata.

> - add PCI HEADER quirk to claim legacy mode (as suggested by Alan)
> 
> - move ALi IRQ handling code to PCI layer and then hook it into
>   pci_get_legacy_ide_irq()

The second isn't needed and will happen automatically if the chip is
properly configured (again see the BIOS writers guide)

Alan

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Bartlomiej Zolnierkiewicz]

On Thursday 24 July 2008, Alan Cox wrote:
> > Please note that the recommended PATA support in kernel.org kernels
> > is IDE subsystem and libata PATA is still considered experimental.
> 
> This is false. Most distributions now use libata, and the old IDE code
> has recently had an enormous amount of patching so is if anything far
> more experimental than libata.

*) I was talking primarily in ALi PATA context:

config PATA_ALI
        tristate "ALi PATA support (Experimental)"
        depends on PCI && EXPERIMENTAL
...

config BLK_DEV_ALI15X3
        tristate "ALI M15x3 chipset support"
        select IDE_TIMINGS
        select BLK_DEV_IDEDMA_PCI
...

*) Distributions are of course free to do make their own decisions.

*) By using this measurement of 'experimental', x86 would probably won
   as the most experimental arch and networking as the most experimental
   subsystem.  I'm proud that you put 'old' IDE code in the same class! :)

> > - add PCI HEADER quirk to claim legacy mode (as suggested by Alan)
> > 
> > - move ALi IRQ handling code to PCI layer and then hook it into
> >   pci_get_legacy_ide_irq()
> 
> The second isn't needed and will happen automatically if the chip is
> properly configured (again see the BIOS writers guide)

The second is of course optional for properly configured chips but it is
not much of work and better be safe than sorry (also which 'BIOS writers
guide' exactly are you referring to?).

Thanks,
Bart

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Alan Cox]

> config PATA_ALI
>         tristate "ALi PATA support (Experimental)"
>         depends on PCI && EXPERIMENTAL
> ...

I should update that now the ATAPI stuff is done.

> The second is of course optional for properly configured chips but it is
> not much of work and better be safe than sorry (also which 'BIOS writers

That might make sense for the specific chipset.

> guide' exactly are you referring to?).

The ALi one for the chipset. You might need to be nice to ULi to get a
copy however as I don't think its on the web site 8(

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Alan Cox]

> Does anyone know if the ALi M5229 actually exists outside of one of
> the M1543 bridges?

Yes - it exists in various forms in all sorts of ALi/ULi chipset products.

> Note, this hardly seems to be a board-quirk, it is more of a device
> quirk...

Your description so far is of "device not put in a valid configuration
according to the spec"  - that seems to me to be board problems.

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Ben Dooks]

On Thu, Jul 24, 2008 at 12:14:50PM +0100, Alan Cox wrote:
> > Does anyone know if the ALi M5229 actually exists outside of one of
> > the M1543 bridges?
> 
> Yes - it exists in various forms in all sorts of ALi/ULi chipset products.
> 
> > Note, this hardly seems to be a board-quirk, it is more of a device
> > quirk...
> 
> Your description so far is of "device not put in a valid configuration
> according to the spec"  - that seems to me to be board problems.

No, the configuration is correct, you cannot assign this device an
PCI IRQ when it is embedded in the M1543 bridge. The bridge itself
has other configuration registers that specify where the IDE IRQs
get routed.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Alan Cox]

On Thu, 24 Jul 2008 12:50:00 +0100
Ben Dooks <ben-linux@fluff.org> wrote:

> On Thu, Jul 24, 2008 at 12:14:50PM +0100, Alan Cox wrote:
> > > Does anyone know if the ALi M5229 actually exists outside of one of
> > > the M1543 bridges?
> > 
> > Yes - it exists in various forms in all sorts of ALi/ULi chipset products.
> > 
> > > Note, this hardly seems to be a board-quirk, it is more of a device
> > > quirk...
> > 
> > Your description so far is of "device not put in a valid configuration
> > according to the spec"  - that seems to me to be board problems.
> 
> No, the configuration is correct, you cannot assign this device an
> PCI IRQ when it is embedded in the M1543 bridge. The bridge itself
> has other configuration registers that specify where the IDE IRQs
> get routed.

You aren't listening: "according to spec" - there is a specification for
how legacy and native mode ATA controllers function and should be
configured. You are describing a situation which appears to be outside
that spec entirely.

Alan

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Ben Dooks]

On Thu, Jul 24, 2008 at 12:58:23PM +0100, Alan Cox wrote:
> On Thu, 24 Jul 2008 12:50:00 +0100
> Ben Dooks <ben-linux@fluff.org> wrote:
> 
> > On Thu, Jul 24, 2008 at 12:14:50PM +0100, Alan Cox wrote:
> > > > Does anyone know if the ALi M5229 actually exists outside of one of
> > > > the M1543 bridges?
> > > 
> > > Yes - it exists in various forms in all sorts of ALi/ULi chipset products.
> > > 
> > > > Note, this hardly seems to be a board-quirk, it is more of a device
> > > > quirk...
> > > 
> > > Your description so far is of "device not put in a valid configuration
> > > according to the spec"  - that seems to me to be board problems.
> > 
> > No, the configuration is correct, you cannot assign this device an
> > PCI IRQ when it is embedded in the M1543 bridge. The bridge itself
> > has other configuration registers that specify where the IDE IRQs
> > get routed.
> 
> You aren't listening: "according to spec" - there is a specification for
> how legacy and native mode ATA controllers function and should be
> configured. You are describing a situation which appears to be outside
> that spec entirely.

Ok, so this controller is not performing to the IRQ portion of the PCI
IDE specification, but is this any reason to force it back into legacy
mode?

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Alan Cox]

> Ok, so this controller is not performing to the IRQ portion of the PCI
> IDE specification, but is this any reason to force it back into legacy
> mode?

Well the obvious reason to do that is that is what the BIOS guide says
and what the standards expect. Given that legacy mode has no performance
impact at all as it just changes the IRQ interpretation and the port
mapping rules it seems silly not to do that

Alan

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Ben Dooks]

On Thu, Jul 24, 2008 at 03:33:38PM +0100, Alan Cox wrote:
> > Ok, so this controller is not performing to the IRQ portion of the PCI
> > IDE specification, but is this any reason to force it back into legacy
> > mode?
> 
> Well the obvious reason to do that is that is what the BIOS guide says
> and what the standards expect. Given that legacy mode has no performance
> impact at all as it just changes the IRQ interpretation and the port
> mapping rules it seems silly not to do that

I've changed drivers/ata/pata_ali.c to change the mode back to compatibility
mode, but libata-sff.c is still trying to use the pci configuration BARs
to map the control registers. If the copy of the "PCI IDE Controller
Specification, Revision 1.0" [1] is to be belived, the values in the PCI BARs
should be ignored.

[1] http://suif.stanford.edu/~csapuntz/specs/pciide.ps
> Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [patch 1/1] LIBATA: Allow devices without IRQ specified to fall back

[Alan Cox]

> I've changed drivers/ata/pata_ali.c to change the mode back to compatibility
> mode, but libata-sff.c is still trying to use the pci configuration BARs
> to map the control registers. If the copy of the "PCI IDE Controller
> Specification, Revision 1.0" [1] is to be belived, the values in the PCI BARs
> should be ignored.

The PCI code knows about this and fills in BAR0 to BAR3 correctly
according to the host platform (defaulting to the usual PC values). We
have to do that anyway to keep the PCI resource management straight.

If you switch it in pata_ali that'll break, use a PCI quirk as I said
before - switch it with a HEADER quirk for your platform.

Alan