Re: [PATCH 2/3] g_NCR5380: Test the IRQ before accepting it

[Ondrej Zary]

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 <linux@xxxxxxxxxxxxxxxxxxxx>
> > ---
> >  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.

-- 
Ondrej Zary