| Message ID | 200906221348.02632.bzolnier@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | David Miller |
| Headers | show |
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> -- 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
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? -- 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
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> -- 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
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).. -- 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
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. -- 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
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 -- 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
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).. -- 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
> > 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 -- 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
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... -- 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
> 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 -- 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
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! -- 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
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); }