On Mon, 31 Oct 2016, Ondrej Zary wrote:
> On Monday 31 October 2016, Finn Thain wrote:
> > On Sun, 30 Oct 2016, Ondrej Zary wrote:
> > > Trigger an IRQ first with a test IRQ handler to find out if it really
> > > works. Disable the IRQ if not.
> > >
> > > This prevents hang when incorrect IRQ was specified by user.
> > >
> > > Signed-off-by: Ondrej Zary <li...@rainbow-software.org>
> > > ---
> > > drivers/scsi/g_NCR5380.c | 32 ++++++++++++++++++++++++++++++--
> > > 1 file changed, 30 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> > > index 3790ed5..261e168 100644
> > > --- a/drivers/scsi/g_NCR5380.c
> > > +++ b/drivers/scsi/g_NCR5380.c
> > > @@ -67,6 +67,14 @@
> > > MODULE_ALIAS("g_NCR5380_mmio");
> > > MODULE_LICENSE("GPL");
> > >
> > > +static bool irq_working;
> > > +
> > > +static irqreturn_t test_irq(int irq, void *dev_id)
> > > +{
> > > + irq_working = true;
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > /*
> > > * Configure I/O address of 53C400A or DTC436 by writing magic numbers
> > > * to ports 0x779 and 0x379.
> > > @@ -275,10 +283,30 @@ static int generic_NCR5380_init_one(struct
> > > scsi_host_template *tpnt, /* set IRQ for HP C2502 */
> > > if (board == BOARD_HP_C2502)
> > > magic_configure(port_idx, instance->irq, magic);
> > > - if (request_irq(instance->irq, generic_NCR5380_intr,
> > > - 0, "NCR5380", instance)) {
> > > + /* test if the IRQ is working */
> > > + irq_working = false;
> > > + if (request_irq(instance->irq, test_irq,
> > > + 0, "NCR5380-irqtest", NULL)) {
> > > printk(KERN_WARNING "scsi%d : IRQ%d not free,
> > > interrupts disabled\n",
> > > instance->host_no, instance->irq); instance->irq = NO_IRQ;
> > > + } else {
> > > + NCR5380_trigger_irq(instance);
> > > + NCR5380_read(RESET_PARITY_INTERRUPT_REG);
> > > + free_irq(instance->irq, NULL);
> > > + if (irq_working) {
> > > + if (request_irq(instance->irq,
> > > + generic_NCR5380_intr, 0,
> > > + "NCR5380", instance)) {
> > > + printk(KERN_WARNING "scsi%d : IRQ%d not
> > > free, interrupts
> > > disabled\n", +
> > > instance->host_no,
> > > + instance->irq);
> > > + instance->irq = NO_IRQ;
> > > + }
> > > + } else {
> > > + printk(KERN_WARNING "scsi%d : IRQ%d not
> > > working, interrupts
> > > disabled\n", + instance->host_no,
> > > instance->irq);
> > > + instance->irq = NO_IRQ;
> > > + }
> > > }
> > > }
> >
> > If the user omits to specify an irq, you can just default to IRQ_AUTO.
> > This might result in NO_IRQ, which gives the same result as this
> > patch.
>
> Looks like a good idea.
>
> > And when the user does specify an IRQ, we should trust them. So this
> > compexity doesn't add any value AFAICT. Thanks but no thanks.
>
> This fixes a real problem: specifying wrong IRQ hangs the machine
> completely.
>
> It's really easy - if the IRQ is free but configured in BIOS as PCI IRQ
> (not ISA). Everything seems fine except the IRQ will never trigger.
>
>
How does that cause a hang? I'd expect scsi command timeouts, but there
are any number of module parameters that the user can stuff up which will
lead to command timeouts. I expect the same could be said of BIOS
settings.
--
