[PATCH] cs5520: add missing IRQ setup for the second port

[PATCH] cs5520: add missing IRQ setup for the second port

[Bartlomiej Zolnierkiewicz]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] cs5520: add missing IRQ setup for the second port

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
This is obviously correct regression fix.  The only problem is that
it cannot be applied under the new rigid policy before somebody with
the hardware verifies it.  This will only result in a needless delay
in this case (IMHO a common sense works better than rigid policies).

 drivers/ide/cs5520.c |    1 +
 1 file changed, 1 insertion(+)

Index: b/drivers/ide/cs5520.c
===================================================================
--- a/drivers/ide/cs5520.c
+++ b/drivers/ide/cs5520.c
@@ -135,6 +135,7 @@ static int __devinit cs5520_init_one(str
 
 	ide_pci_setup_ports(dev, d, &hw[0], &hws[0]);
 	hw[0].irq = 14;
+	hw[1].irq = 15;
 
 	return ide_host_add(d, hws, 2, NULL);
 }

Re: [PATCH] cs5520: add missing IRQ setup for the second port

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Mon, 22 Jun 2009 13:48:02 +0200

> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH] cs5520: add missing IRQ setup for the second port
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
> This is obviously correct regression fix.  The only problem is that
> it cannot be applied under the new rigid policy before somebody with
> the hardware verifies it.  This will only result in a needless delay
> in this case (IMHO a common sense works better than rigid policies).

Such hard rules don't apply to regression fixes.  Patch
applied, thanks.

But I certainly would have required some positive testing for the
commit which introduced this problem!

I've added some verbosity to the commit message, so that people can
track where the problem was introduced, and exactly how this problem
arose, like so:

cs5520: add missing IRQ setup for the second port

This fixes a regression introduced by commit
86ccf37c6acd74cf7e4b7751ee045de19943c5a0 ("ide: remove pciirq argument
from ide_pci_setup_ports()")

ide_pci_setup_ports() would loop over the available ports, one
by one, recording IRQ numbers increasingly from the one passed
in as "pciirq".  The conversion only assigned the initial port's
IRQ, 14, but left the second one not setup.

[ Make commit message more verbose -DaveM ]

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] cs5520: add missing IRQ setup for the second port

[Frans Pop]

David Miller wrote:
> ide_pci_setup_ports() would loop over the available ports, one
> by one, recording IRQ numbers increasingly from the one passed
> in as "pciirq".

I looked at the commit and at the code and called functions several times 
(just out of curiosity), but have been completely unable to find where 
that increase of pciirq actually happened.
AFAICT ide_pci_setup_ports() just passed it on to ide_hw_configure() and 
all that did was 'hw->irq = irq', so AFAICT in the old case both ports 
would get the *same* IRQ, and not consecutive ones.

Am I missing something here?

Re: [PATCH] cs5520: add missing IRQ setup for the second port

[Bartlomiej Zolnierkiewicz]

On Tuesday 23 June 2009 01:31:36 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Mon, 22 Jun 2009 13:48:02 +0200
> 
> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Subject: [PATCH] cs5520: add missing IRQ setup for the second port
> > 
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > ---
> > This is obviously correct regression fix.  The only problem is that
> > it cannot be applied under the new rigid policy before somebody with
> > the hardware verifies it.  This will only result in a needless delay
> > in this case (IMHO a common sense works better than rigid policies).
> 
> Such hard rules don't apply to regression fixes.  Patch
> applied, thanks.
> 
> But I certainly would have required some positive testing for the
> commit which introduced this problem!

You can from now on.

> I've added some verbosity to the commit message, so that people can
> track where the problem was introduced, and exactly how this problem
> arose, like so:
> 
> cs5520: add missing IRQ setup for the second port

> This fixes a regression introduced by commit
> 86ccf37c6acd74cf7e4b7751ee045de19943c5a0 ("ide: remove pciirq argument
> from ide_pci_setup_ports()")
> 
> ide_pci_setup_ports() would loop over the available ports, one
> by one, recording IRQ numbers increasingly from the one passed
> in as "pciirq".  The conversion only assigned the initial port's
> IRQ, 14, but left the second one not setup.
> 
> [ Make commit message more verbose -DaveM ]

I didn't remember which commit was it so I didn't include it in the patch
description.  This is certainly not the commit above (if you're making such,
changes please make sure that you actually understand the code -- you have
it easy now as it is orders of magnitude simpler than it was few years ago).

Looking a bit more in depth it could be that it was never a regression
and I'm no longer sure that the change is correct (sorry for that, I was
blinded by code in pata_cs5520.c).

> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] cs5520: add missing IRQ setup for the second port

[Bartlomiej Zolnierkiewicz]

On Tuesday 23 June 2009 02:30:11 Bartlomiej Zolnierkiewicz wrote:
> On Tuesday 23 June 2009 01:31:36 David Miller wrote:
> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Date: Mon, 22 Jun 2009 13:48:02 +0200
> > 
> > > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > > Subject: [PATCH] cs5520: add missing IRQ setup for the second port
> > > 
> > > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > > ---
> > > This is obviously correct regression fix.  The only problem is that
> > > it cannot be applied under the new rigid policy before somebody with
> > > the hardware verifies it.  This will only result in a needless delay
> > > in this case (IMHO a common sense works better than rigid policies).
> > 
> > Such hard rules don't apply to regression fixes.  Patch
> > applied, thanks.
> > 
> > But I certainly would have required some positive testing for the
> > commit which introduced this problem!
> 
> You can from now on.
> 
> > I've added some verbosity to the commit message, so that people can
> > track where the problem was introduced, and exactly how this problem
> > arose, like so:
> > 
> > cs5520: add missing IRQ setup for the second port
> 
> > This fixes a regression introduced by commit
> > 86ccf37c6acd74cf7e4b7751ee045de19943c5a0 ("ide: remove pciirq argument
> > from ide_pci_setup_ports()")
> > 
> > ide_pci_setup_ports() would loop over the available ports, one
> > by one, recording IRQ numbers increasingly from the one passed
> > in as "pciirq".  The conversion only assigned the initial port's
> > IRQ, 14, but left the second one not setup.
> > 
> > [ Make commit message more verbose -DaveM ]
> 
> I didn't remember which commit was it so I didn't include it in the patch
> description.  This is certainly not the commit above (if you're making such,
> changes please make sure that you actually understand the code -- you have
> it easy now as it is orders of magnitude simpler than it was few years ago).
> 
> Looking a bit more in depth it could be that it was never a regression
> and I'm no longer sure that the change is correct (sorry for that, I was
> blinded by code in pata_cs5520.c).
> 
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Signed-off-by: David S. Miller <davem@davemloft.net>

Please give people some time (few days) to review and comment patches before
applying them.  I don't trust my own patches too much so I'm always looking
for some peer review (usually all host driver patches go through Sergei, in
this case Frans would have probably shot the patch down)..

Re: [PATCH] cs5520: add missing IRQ setup for the second port

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Tue, 23 Jun 2009 03:05:55 +0200

> Please give people some time (few days) to review and comment patches before
> applying them.  I don't trust my own patches too much so I'm always looking
> for some peer review (usually all host driver patches go through Sergei, in
> this case Frans would have probably shot the patch down)..

Just because it's in my tree doesn't mean it's on it's way to
Linus yet.

Every patch will sit for at least a day or two until I really
push things to him.  Allowing for such errors to be spotted.

I can easily revert this and that's exactly what I will do.

Re: [PATCH] cs5520: add missing IRQ setup for the second port

[Alan Cox]

On Tue, 23 Jun 2009 01:58:11 +0200
Frans Pop <elendil@planet.nl> wrote:

> David Miller wrote:
> > ide_pci_setup_ports() would loop over the available ports, one
> > by one, recording IRQ numbers increasingly from the one passed
> > in as "pciirq".
> 
> I looked at the commit and at the code and called functions several times 
> (just out of curiosity), but have been completely unable to find where 
> that increase of pciirq actually happened.
> AFAICT ide_pci_setup_ports() just passed it on to ide_hw_configure() and 
> all that did was 'hw->irq = irq', so AFAICT in the old case both ports 
> would get the *same* IRQ, and not consecutive ones.

The cs5520 isn't a standard IDE controller in the first place and doesn't
have two channels anyway. It's special as it uses non standard bar
registers on the bridge to access the virtual DMA region, PCI registers
on the bridge to set timings (much like say MPIIX) and standard I/O ports
for the ATA accesses.

Alan

Re: [PATCH] cs5520: add missing IRQ setup for the second port

[Bartlomiej Zolnierkiewicz]

On Tuesday 23 June 2009 10:42:06 Alan Cox wrote:
> On Tue, 23 Jun 2009 01:58:11 +0200
> Frans Pop <elendil@planet.nl> wrote:
> 
> > David Miller wrote:
> > > ide_pci_setup_ports() would loop over the available ports, one
> > > by one, recording IRQ numbers increasingly from the one passed
> > > in as "pciirq".
> > 
> > I looked at the commit and at the code and called functions several times 
> > (just out of curiosity), but have been completely unable to find where 
> > that increase of pciirq actually happened.
> > AFAICT ide_pci_setup_ports() just passed it on to ide_hw_configure() and 
> > all that did was 'hw->irq = irq', so AFAICT in the old case both ports 
> > would get the *same* IRQ, and not consecutive ones.
> 
> The cs5520 isn't a standard IDE controller in the first place and doesn't
> have two channels anyway. It's special as it uses non standard bar

Please note that both cs5520.c / pata_cs5520.c assume that the controller
has two ports (please verify whether they need some fixing, thanks)..

Re: [PATCH] cs5520: add missing IRQ setup for the second port

[Alan Cox]

> > The cs5520 isn't a standard IDE controller in the first place and doesn't
> > have two channels anyway. It's special as it uses non standard bar
> 
> Please note that both cs5520.c / pata_cs5520.c assume that the controller
> has two ports (please verify whether they need some fixing, thanks)..

It can it turns out have two if the relevant external logic is present.

Alan

Re: [PATCH] cs5520: add missing IRQ setup for the second port

[David Miller]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Tue, 23 Jun 2009 16:33:24 +0100

>> > The cs5520 isn't a standard IDE controller in the first place and doesn't
>> > have two channels anyway. It's special as it uses non standard bar
>> 
>> Please note that both cs5520.c / pata_cs5520.c assume that the controller
>> has two ports (please verify whether they need some fixing, thanks)..
> 
> It can it turns out have two if the relevant external logic is present.

Ok, so previously the code would set both port IRQs to 14.  But now
only the first port IRQ gets initialized to 14, any subsequent port
has it's IRQ left uninitiailized.

What should we do?  I guess "do nothing and wait until we see a bug
report from an actual user" is a valid response...

Re: [PATCH] cs5520: add missing IRQ setup for the second port

[Alan Cox]

> What should we do?  I guess "do nothing and wait until we see a bug
> report from an actual user" is a valid response...


I doubt you ever will.

The docs don't say so I'd assume its hard coded 14/15 as if it was ISA
(its not an IDE storage class device so doesn't have the PCI config bits
to switch between legacy and native modes).

Alan

Re: [PATCH] cs5520: add missing IRQ setup for the second port

[David Miller]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Wed, 24 Jun 2009 10:22:00 +0100

>> What should we do?  I guess "do nothing and wait until we see a bug
>> report from an actual user" is a valid response...
> 
> I doubt you ever will.
> 
> The docs don't say so I'd assume its hard coded 14/15 as if it was ISA
> (its not an IDE storage class device so doesn't have the PCI config bits
> to switch between legacy and native modes).

Ok, that makes sense.  I'll set the second port's IRQ to 15.

Thanks!