Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1512225 > unrolled thread

[PATCH 1/3] NCR5380: Use probe_irq_*() for IRQ probing

Started byOndrej Zary <linux@rainbow-software.org>
First post2016-10-30 23:50 +0100
Last post2016-10-31 10:50 +0100
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/3] NCR5380: Use probe_irq_*() for IRQ probing Ondrej Zary <linux@rainbow-software.org> - 2016-10-30 23:50 +0100
    Re: [PATCH 1/3] NCR5380: Use probe_irq_*() for IRQ probing Finn Thain <fthain@telegraphics.com.au> - 2016-10-31 04:10 +0100
      Re: [PATCH 1/3] NCR5380: Use probe_irq_*() for IRQ probing Ondrej Zary <linux@rainbow-software.org> - 2016-10-31 09:20 +0100
        Re: [PATCH 1/3] NCR5380: Use probe_irq_*() for IRQ probing Finn Thain <fthain@telegraphics.com.au> - 2016-10-31 10:50 +0100

#1512225 — [PATCH 1/3] NCR5380: Use probe_irq_*() for IRQ probing

FromOndrej Zary <linux@rainbow-software.org>
Date2016-10-30 23:50 +0100
Subject[PATCH 1/3] NCR5380: Use probe_irq_*() for IRQ probing
Message-ID<sy75o-2jv-5@gated-at.bofh.it>
Use standard probe_irq_on() and probe_irq_off() functions instead of own
implementation.
This prevents warning messages like this in the kernel log:
genirq: Flags mismatch irq 1. 00000000 (NCR-probe) vs. 00000080 (i8042)

Move the IRQ trigger code to a separate function so it can be used for
other purposes (testing if the IRQ works).

Also add missing IRQ reset after probe.

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/scsi/NCR5380.c   |   72 +++++++++++++++++-----------------------------
 drivers/scsi/NCR5380.h   |    3 +-
 drivers/scsi/g_NCR5380.c |    2 +-
 3 files changed, 29 insertions(+), 48 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index d849ffa..01c0027 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -351,50 +351,13 @@ static void NCR5380_print_phase(struct Scsi_Host *instance)
 }
 #endif
 
-
-static int probe_irq;
-
-/**
- * probe_intr	-	helper for IRQ autoprobe
- * @irq: interrupt number
- * @dev_id: unused
- * @regs: unused
- *
- * Set a flag to indicate the IRQ in question was received. This is
- * used by the IRQ probe code.
- */
-
-static irqreturn_t probe_intr(int irq, void *dev_id)
-{
-	probe_irq = irq;
-	return IRQ_HANDLED;
-}
-
-/**
- * NCR5380_probe_irq	-	find the IRQ of an NCR5380
- * @instance: NCR5380 controller
- * @possible: bitmask of ISA IRQ lines
- *
- * Autoprobe for the IRQ line used by the NCR5380 by triggering an IRQ
- * and then looking to see what interrupt actually turned up.
- */
-
-static int __maybe_unused NCR5380_probe_irq(struct Scsi_Host *instance,
-						int possible)
+static void NCR5380_trigger_irq(struct Scsi_Host *instance)
 {
 	struct NCR5380_hostdata *hostdata = shost_priv(instance);
-	unsigned long timeout;
-	int trying_irqs, i, mask;
-
-	for (trying_irqs = 0, i = 1, mask = 2; i < 16; ++i, mask <<= 1)
-		if ((mask & possible) && (request_irq(i, &probe_intr, 0, "NCR-probe", NULL) == 0))
-			trying_irqs |= mask;
-
-	timeout = jiffies + msecs_to_jiffies(250);
-	probe_irq = NO_IRQ;
+	unsigned long timeout = jiffies + msecs_to_jiffies(2500);
 
 	/*
-	 * A interrupt is triggered whenever BSY = false, SEL = true
+	 * An interrupt is triggered whenever BSY = false, SEL = true
 	 * and a bit set in the SELECT_ENABLE_REG is asserted on the
 	 * SCSI bus.
 	 *
@@ -402,23 +365,40 @@ static int __maybe_unused NCR5380_probe_irq(struct Scsi_Host *instance,
 	 * (I/O, C/D, and MSG) match those in the TCR, so we must reset that
 	 * to zero.
 	 */
-
 	NCR5380_write(TARGET_COMMAND_REG, 0);
 	NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
 	NCR5380_write(OUTPUT_DATA_REG, hostdata->id_mask);
 	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE | ICR_ASSERT_DATA | ICR_ASSERT_SEL);
 
-	while (probe_irq == NO_IRQ && time_before(jiffies, timeout))
+	while (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_IRQ) &&
+	       time_before(jiffies, timeout))
 		schedule_timeout_uninterruptible(1);
 
 	NCR5380_write(SELECT_ENABLE_REG, 0);
 	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
+}
 
-	for (i = 1, mask = 2; i < 16; ++i, mask <<= 1)
-		if (trying_irqs & mask)
-			free_irq(i, NULL);
+/**
+ * NCR5380_probe_irq	-	find the IRQ of an NCR5380
+ * @instance: NCR5380 controller
+ *
+ * Autoprobe for the IRQ line used by the NCR5380 by triggering an IRQ
+ * and then looking to see what interrupt actually turned up.
+ */
+
+static int NCR5380_probe_irq(struct Scsi_Host *instance)
+{
+	struct NCR5380_hostdata *hostdata = shost_priv(instance);
+	int irqs, irq;
+
+	irqs = probe_irq_on();
+	NCR5380_trigger_irq(instance);
+	irq = probe_irq_off(irqs);
+	NCR5380_read(RESET_PARITY_INTERRUPT_REG);
+	if (irq < 0)
+		irq = 0;
 
-	return probe_irq;
+	return irq;
 }
 
 /**
diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
index 3c6ce54..b2fca52 100644
--- a/drivers/scsi/NCR5380.h
+++ b/drivers/scsi/NCR5380.h
@@ -290,7 +290,8 @@ static inline struct scsi_cmnd *NCR5380_to_scmd(struct NCR5380_cmd *ncmd_ptr)
 #define NCR5380_dprint_phase(flg, arg) do {} while (0)
 #endif
 
-static int NCR5380_probe_irq(struct Scsi_Host *instance, int possible);
+static void NCR5380_trigger_irq(struct Scsi_Host *instance);
+static int NCR5380_probe_irq(struct Scsi_Host *instance);
 static int NCR5380_init(struct Scsi_Host *instance, int flags);
 static int NCR5380_maybe_reset_bus(struct Scsi_Host *);
 static void NCR5380_exit(struct Scsi_Host *instance);
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 7299ad9..3790ed5 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -265,7 +265,7 @@ static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
 	if (irq != IRQ_AUTO)
 		instance->irq = irq;
 	else
-		instance->irq = NCR5380_probe_irq(instance, 0xffff);
+		instance->irq = NCR5380_probe_irq(instance);
 
 	/* Compatibility with documented NCR5380 kernel parameters */
 	if (instance->irq == 255)
-- 
Ondrej Zary

[toc] | [next] | [standalone]

#1512299

FromFinn Thain <fthain@telegraphics.com.au>
Date2016-10-31 04:10 +0100
Message-ID<syb8Z-52O-13@gated-at.bofh.it>
In reply to#1512225
On Sun, 30 Oct 2016, Ondrej Zary wrote:

> Use standard probe_irq_on() and probe_irq_off() functions instead of own
> implementation.

Thanks for doing this.

> This prevents warning messages like this in the kernel log:
> genirq: Flags mismatch irq 1. 00000000 (NCR-probe) vs. 00000080 (i8042)
> 
> Move the IRQ trigger code to a separate function so it can be used for
> other purposes (testing if the IRQ works).
> 
> Also add missing IRQ reset after probe.
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
>  drivers/scsi/NCR5380.c   |   72 +++++++++++++++++-----------------------------
>  drivers/scsi/NCR5380.h   |    3 +-
>  drivers/scsi/g_NCR5380.c |    2 +-
>  3 files changed, 29 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> index d849ffa..01c0027 100644
> --- a/drivers/scsi/NCR5380.c
> +++ b/drivers/scsi/NCR5380.c
> @@ -351,50 +351,13 @@ static void NCR5380_print_phase(struct Scsi_Host *instance)
>  }
>  #endif
>  
> -
> -static int probe_irq;
> -
> -/**
> - * probe_intr	-	helper for IRQ autoprobe
> - * @irq: interrupt number
> - * @dev_id: unused
> - * @regs: unused
> - *
> - * Set a flag to indicate the IRQ in question was received. This is
> - * used by the IRQ probe code.
> - */
> -
> -static irqreturn_t probe_intr(int irq, void *dev_id)
> -{
> -	probe_irq = irq;
> -	return IRQ_HANDLED;
> -}
> -
> -/**
> - * NCR5380_probe_irq	-	find the IRQ of an NCR5380
> - * @instance: NCR5380 controller
> - * @possible: bitmask of ISA IRQ lines
> - *
> - * Autoprobe for the IRQ line used by the NCR5380 by triggering an IRQ
> - * and then looking to see what interrupt actually turned up.
> - */
> -
> -static int __maybe_unused NCR5380_probe_irq(struct Scsi_Host *instance,
> -						int possible)
> +static void NCR5380_trigger_irq(struct Scsi_Host *instance)
>  {
>  	struct NCR5380_hostdata *hostdata = shost_priv(instance);
> -	unsigned long timeout;
> -	int trying_irqs, i, mask;
> -
> -	for (trying_irqs = 0, i = 1, mask = 2; i < 16; ++i, mask <<= 1)
> -		if ((mask & possible) && (request_irq(i, &probe_intr, 0, "NCR-probe", NULL) == 0))
> -			trying_irqs |= mask;
> -
> -	timeout = jiffies + msecs_to_jiffies(250);
> -	probe_irq = NO_IRQ;
> +	unsigned long timeout = jiffies + msecs_to_jiffies(2500);
>  
>  	/*
> -	 * A interrupt is triggered whenever BSY = false, SEL = true
> +	 * An interrupt is triggered whenever BSY = false, SEL = true
>  	 * and a bit set in the SELECT_ENABLE_REG is asserted on the
>  	 * SCSI bus.
>  	 *
> @@ -402,23 +365,40 @@ static int __maybe_unused NCR5380_probe_irq(struct Scsi_Host *instance,
>  	 * (I/O, C/D, and MSG) match those in the TCR, so we must reset that
>  	 * to zero.
>  	 */
> -
>  	NCR5380_write(TARGET_COMMAND_REG, 0);
>  	NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
>  	NCR5380_write(OUTPUT_DATA_REG, hostdata->id_mask);
>  	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE | ICR_ASSERT_DATA | ICR_ASSERT_SEL);
>  
> -	while (probe_irq == NO_IRQ && time_before(jiffies, timeout))
> +	while (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_IRQ) &&
> +	       time_before(jiffies, timeout))
>  		schedule_timeout_uninterruptible(1);

You don't need a 2.5 second timeout here. To raise the interrupt the chip 
should not need more than a bus settle delay (400ns). Calling msleep(1) 
should be plenty.

Better still, please drop the loop, the delay and the BASR_IRQ test here, 
and if need be, put the delay in the caller. This routine discards the 
result of that sequence anyway.

>  
>  	NCR5380_write(SELECT_ENABLE_REG, 0);
>  	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
> +}
>  
> -	for (i = 1, mask = 2; i < 16; ++i, mask <<= 1)
> -		if (trying_irqs & mask)
> -			free_irq(i, NULL);
> +/**
> + * NCR5380_probe_irq	-	find the IRQ of an NCR5380
> + * @instance: NCR5380 controller
> + *
> + * Autoprobe for the IRQ line used by the NCR5380 by triggering an IRQ
> + * and then looking to see what interrupt actually turned up.
> + */
> +
> +static int NCR5380_probe_irq(struct Scsi_Host *instance)
> +{
> +	struct NCR5380_hostdata *hostdata = shost_priv(instance);
> +	int irqs, irq;
> +

Please rename irqs to irq_mask.

Other drivers like tty/cyclades.c begin with,

	/* forget possible initially masked and pending IRQ */
	irq = probe_irq_off(probe_irq_on());

Perhaps we need that here too?

> +	irqs = probe_irq_on();

You should clear any pending interrupt on the chip before you try to 
trigger another one.

> +	NCR5380_trigger_irq(instance);

Please do the msleep(1) here.

> +	irq = probe_irq_off(irqs);
> +	NCR5380_read(RESET_PARITY_INTERRUPT_REG);
> +	if (irq < 0)
> +		irq = 0;

Please use NO_IRQ instead of 0.

>  
> -	return probe_irq;
> +	return irq;
>  }
>  
>  /**
> diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
> index 3c6ce54..b2fca52 100644
> --- a/drivers/scsi/NCR5380.h
> +++ b/drivers/scsi/NCR5380.h
> @@ -290,7 +290,8 @@ static inline struct scsi_cmnd *NCR5380_to_scmd(struct NCR5380_cmd *ncmd_ptr)
>  #define NCR5380_dprint_phase(flg, arg) do {} while (0)
>  #endif
>  
> -static int NCR5380_probe_irq(struct Scsi_Host *instance, int possible);
> +static void NCR5380_trigger_irq(struct Scsi_Host *instance);
> +static int NCR5380_probe_irq(struct Scsi_Host *instance);
>  static int NCR5380_init(struct Scsi_Host *instance, int flags);
>  static int NCR5380_maybe_reset_bus(struct Scsi_Host *);
>  static void NCR5380_exit(struct Scsi_Host *instance);

Please move NCR5380_probe_irq and NCR5380_trigger_irq implementations and 
prototypes out of the core driver and into g_NCR5380.c. The reason is, ISA 
cards are probably the only 5380 cards ever likely to need manual IRQ 
configuration. It would better to have g_NCR5380 support more ISA cards 
than to add more ISA drivers, therefore this code isn't likely to be 
needed in the core driver.

Thanks.

-- 

> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 7299ad9..3790ed5 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -265,7 +265,7 @@ static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
>  	if (irq != IRQ_AUTO)
>  		instance->irq = irq;
>  	else
> -		instance->irq = NCR5380_probe_irq(instance, 0xffff);
> +		instance->irq = NCR5380_probe_irq(instance);
>  
>  	/* Compatibility with documented NCR5380 kernel parameters */
>  	if (instance->irq == 255)
>

[toc] | [prev] | [next] | [standalone]

#1512387

FromOndrej Zary <linux@rainbow-software.org>
Date2016-10-31 09:20 +0100
Message-ID<syfYZ-8gd-1@gated-at.bofh.it>
In reply to#1512299
On Monday 31 October 2016, Finn Thain wrote:
> On Sun, 30 Oct 2016, Ondrej Zary wrote:
> > Use standard probe_irq_on() and probe_irq_off() functions instead of own
> > implementation.
>
> Thanks for doing this.
>
> > This prevents warning messages like this in the kernel log:
> > genirq: Flags mismatch irq 1. 00000000 (NCR-probe) vs. 00000080 (i8042)
> >
> > Move the IRQ trigger code to a separate function so it can be used for
> > other purposes (testing if the IRQ works).
> >
> > Also add missing IRQ reset after probe.
> >
> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> > ---
> >  drivers/scsi/NCR5380.c   |   72
> > +++++++++++++++++----------------------------- drivers/scsi/NCR5380.h   |
> >    3 +-
> >  drivers/scsi/g_NCR5380.c |    2 +-
> >  3 files changed, 29 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> > index d849ffa..01c0027 100644
> > --- a/drivers/scsi/NCR5380.c
> > +++ b/drivers/scsi/NCR5380.c
> > @@ -351,50 +351,13 @@ static void NCR5380_print_phase(struct Scsi_Host
> > *instance) }
> >  #endif
> >
> > -
> > -static int probe_irq;
> > -
> > -/**
> > - * probe_intr	-	helper for IRQ autoprobe
> > - * @irq: interrupt number
> > - * @dev_id: unused
> > - * @regs: unused
> > - *
> > - * Set a flag to indicate the IRQ in question was received. This is
> > - * used by the IRQ probe code.
> > - */
> > -
> > -static irqreturn_t probe_intr(int irq, void *dev_id)
> > -{
> > -	probe_irq = irq;
> > -	return IRQ_HANDLED;
> > -}
> > -
> > -/**
> > - * NCR5380_probe_irq	-	find the IRQ of an NCR5380
> > - * @instance: NCR5380 controller
> > - * @possible: bitmask of ISA IRQ lines
> > - *
> > - * Autoprobe for the IRQ line used by the NCR5380 by triggering an IRQ
> > - * and then looking to see what interrupt actually turned up.
> > - */
> > -
> > -static int __maybe_unused NCR5380_probe_irq(struct Scsi_Host *instance,
> > -						int possible)
> > +static void NCR5380_trigger_irq(struct Scsi_Host *instance)
> >  {
> >  	struct NCR5380_hostdata *hostdata = shost_priv(instance);
> > -	unsigned long timeout;
> > -	int trying_irqs, i, mask;
> > -
> > -	for (trying_irqs = 0, i = 1, mask = 2; i < 16; ++i, mask <<= 1)
> > -		if ((mask & possible) && (request_irq(i, &probe_intr, 0, "NCR-probe",
> > NULL) == 0)) -			trying_irqs |= mask;
> > -
> > -	timeout = jiffies + msecs_to_jiffies(250);
> > -	probe_irq = NO_IRQ;
> > +	unsigned long timeout = jiffies + msecs_to_jiffies(2500);
> >
> >  	/*
> > -	 * A interrupt is triggered whenever BSY = false, SEL = true
> > +	 * An interrupt is triggered whenever BSY = false, SEL = true
> >  	 * and a bit set in the SELECT_ENABLE_REG is asserted on the
> >  	 * SCSI bus.
> >  	 *
> > @@ -402,23 +365,40 @@ static int __maybe_unused NCR5380_probe_irq(struct
> > Scsi_Host *instance, * (I/O, C/D, and MSG) match those in the TCR, so we
> > must reset that * to zero.
> >  	 */
> > -
> >  	NCR5380_write(TARGET_COMMAND_REG, 0);
> >  	NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
> >  	NCR5380_write(OUTPUT_DATA_REG, hostdata->id_mask);
> >  	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE | ICR_ASSERT_DATA |
> > ICR_ASSERT_SEL);
> >
> > -	while (probe_irq == NO_IRQ && time_before(jiffies, timeout))
> > +	while (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_IRQ) &&
> > +	       time_before(jiffies, timeout))
> >  		schedule_timeout_uninterruptible(1);
>
> You don't need a 2.5 second timeout here. To raise the interrupt the chip
> should not need more than a bus settle delay (400ns). Calling msleep(1)
> should be plenty.

Yes, 2.5s is a mistake (leftover from testing), sorry.

> Better still, please drop the loop, the delay and the BASR_IRQ test here,
> and if need be, put the delay in the caller. This routine discards the
> result of that sequence anyway.

Will if work if the following lines are executed before the delay?

>
> >  	NCR5380_write(SELECT_ENABLE_REG, 0);
> >  	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
> > +}
> >
> > -	for (i = 1, mask = 2; i < 16; ++i, mask <<= 1)
> > -		if (trying_irqs & mask)
> > -			free_irq(i, NULL);
> > +/**
> > + * NCR5380_probe_irq	-	find the IRQ of an NCR5380
> > + * @instance: NCR5380 controller
> > + *
> > + * Autoprobe for the IRQ line used by the NCR5380 by triggering an IRQ
> > + * and then looking to see what interrupt actually turned up.
> > + */
> > +
> > +static int NCR5380_probe_irq(struct Scsi_Host *instance)
> > +{
> > +	struct NCR5380_hostdata *hostdata = shost_priv(instance);
> > +	int irqs, irq;
> > +
>
> Please rename irqs to irq_mask.
>
> Other drivers like tty/cyclades.c begin with,
>
> 	/* forget possible initially masked and pending IRQ */
> 	irq = probe_irq_off(probe_irq_on());
>
> Perhaps we need that here too?

Only cyclades and 8250_port do that, other drivers don't.
Don't know what exact problem should that solve.

>
> > +	irqs = probe_irq_on();
>
> You should clear any pending interrupt on the chip before you try to
> trigger another one.
>
> > +	NCR5380_trigger_irq(instance);
>
> Please do the msleep(1) here.
>
> > +	irq = probe_irq_off(irqs);
> > +	NCR5380_read(RESET_PARITY_INTERRUPT_REG);
> > +	if (irq < 0)
> > +		irq = 0;
>
> Please use NO_IRQ instead of 0.
>
> > -	return probe_irq;
> > +	return irq;
> >  }
> >
> >  /**
> > diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
> > index 3c6ce54..b2fca52 100644
> > --- a/drivers/scsi/NCR5380.h
> > +++ b/drivers/scsi/NCR5380.h
> > @@ -290,7 +290,8 @@ static inline struct scsi_cmnd
> > *NCR5380_to_scmd(struct NCR5380_cmd *ncmd_ptr) #define
> > NCR5380_dprint_phase(flg, arg) do {} while (0)
> >  #endif
> >
> > -static int NCR5380_probe_irq(struct Scsi_Host *instance, int possible);
> > +static void NCR5380_trigger_irq(struct Scsi_Host *instance);
> > +static int NCR5380_probe_irq(struct Scsi_Host *instance);
> >  static int NCR5380_init(struct Scsi_Host *instance, int flags);
> >  static int NCR5380_maybe_reset_bus(struct Scsi_Host *);
> >  static void NCR5380_exit(struct Scsi_Host *instance);
>
> Please move NCR5380_probe_irq and NCR5380_trigger_irq implementations and
> prototypes out of the core driver and into g_NCR5380.c. The reason is, ISA
> cards are probably the only 5380 cards ever likely to need manual IRQ
> configuration. It would better to have g_NCR5380 support more ISA cards
> than to add more ISA drivers, therefore this code isn't likely to be
> needed in the core driver.
>
> Thanks.



-- 
Ondrej Zary

[toc] | [prev] | [next] | [standalone]

#1512434

FromFinn Thain <fthain@telegraphics.com.au>
Date2016-10-31 10:50 +0100
Message-ID<syho6-xY-31@gated-at.bofh.it>
In reply to#1512387
On Mon, 31 Oct 2016, Ondrej Zary wrote:

> On Monday 31 October 2016, Finn Thain wrote:
> > On Sun, 30 Oct 2016, Ondrej Zary wrote:
> > > Use standard probe_irq_on() and probe_irq_off() functions instead of own
> > > implementation.
> >
> > Thanks for doing this.
> >
> > > This prevents warning messages like this in the kernel log:
> > > genirq: Flags mismatch irq 1. 00000000 (NCR-probe) vs. 00000080 (i8042)
> > >
> > > Move the IRQ trigger code to a separate function so it can be used for
> > > other purposes (testing if the IRQ works).
> > >
> > > Also add missing IRQ reset after probe.
> > >
> > > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> > > ---
> > >  drivers/scsi/NCR5380.c   |   72
> > > +++++++++++++++++----------------------------- drivers/scsi/NCR5380.h   |
> > >    3 +-
> > >  drivers/scsi/g_NCR5380.c |    2 +-
> > >  3 files changed, 29 insertions(+), 48 deletions(-)
> > >
> > > diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> > > index d849ffa..01c0027 100644
> > > --- a/drivers/scsi/NCR5380.c
> > > +++ b/drivers/scsi/NCR5380.c
> > > @@ -351,50 +351,13 @@ static void NCR5380_print_phase(struct Scsi_Host
> > > *instance) }
> > >  #endif
> > >
> > > -
> > > -static int probe_irq;
> > > -
> > > -/**
> > > - * probe_intr	-	helper for IRQ autoprobe
> > > - * @irq: interrupt number
> > > - * @dev_id: unused
> > > - * @regs: unused
> > > - *
> > > - * Set a flag to indicate the IRQ in question was received. This is
> > > - * used by the IRQ probe code.
> > > - */
> > > -
> > > -static irqreturn_t probe_intr(int irq, void *dev_id)
> > > -{
> > > -	probe_irq = irq;
> > > -	return IRQ_HANDLED;
> > > -}
> > > -
> > > -/**
> > > - * NCR5380_probe_irq	-	find the IRQ of an NCR5380
> > > - * @instance: NCR5380 controller
> > > - * @possible: bitmask of ISA IRQ lines
> > > - *
> > > - * Autoprobe for the IRQ line used by the NCR5380 by triggering an IRQ
> > > - * and then looking to see what interrupt actually turned up.
> > > - */
> > > -
> > > -static int __maybe_unused NCR5380_probe_irq(struct Scsi_Host *instance,
> > > -						int possible)
> > > +static void NCR5380_trigger_irq(struct Scsi_Host *instance)
> > >  {
> > >  	struct NCR5380_hostdata *hostdata = shost_priv(instance);
> > > -	unsigned long timeout;
> > > -	int trying_irqs, i, mask;
> > > -
> > > -	for (trying_irqs = 0, i = 1, mask = 2; i < 16; ++i, mask <<= 1)
> > > -		if ((mask & possible) && (request_irq(i, &probe_intr, 0, "NCR-probe",
> > > NULL) == 0)) -			trying_irqs |= mask;
> > > -
> > > -	timeout = jiffies + msecs_to_jiffies(250);
> > > -	probe_irq = NO_IRQ;
> > > +	unsigned long timeout = jiffies + msecs_to_jiffies(2500);
> > >
> > >  	/*
> > > -	 * A interrupt is triggered whenever BSY = false, SEL = true
> > > +	 * An interrupt is triggered whenever BSY = false, SEL = true
> > >  	 * and a bit set in the SELECT_ENABLE_REG is asserted on the
> > >  	 * SCSI bus.
> > >  	 *
> > > @@ -402,23 +365,40 @@ static int __maybe_unused NCR5380_probe_irq(struct
> > > Scsi_Host *instance, * (I/O, C/D, and MSG) match those in the TCR, so we
> > > must reset that * to zero.
> > >  	 */
> > > -
> > >  	NCR5380_write(TARGET_COMMAND_REG, 0);
> > >  	NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
> > >  	NCR5380_write(OUTPUT_DATA_REG, hostdata->id_mask);
> > >  	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE | ICR_ASSERT_DATA |
> > > ICR_ASSERT_SEL);
> > >
> > > -	while (probe_irq == NO_IRQ && time_before(jiffies, timeout))
> > > +	while (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_IRQ) &&
> > > +	       time_before(jiffies, timeout))
> > >  		schedule_timeout_uninterruptible(1);
> >
> > You don't need a 2.5 second timeout here. To raise the interrupt the chip
> > should not need more than a bus settle delay (400ns). Calling msleep(1)
> > should be plenty.
> 
> Yes, 2.5s is a mistake (leftover from testing), sorry.
> 
> > Better still, please drop the loop, the delay and the BASR_IRQ test here,
> > and if need be, put the delay in the caller. This routine discards the
> > result of that sequence anyway.
> 
> Will if work if the following lines are executed before the delay?

I think so... but you're right, it is better if the delay remains here.

> 
> >
> > >  	NCR5380_write(SELECT_ENABLE_REG, 0);
> > >  	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
> > > +}
> > >
> > > -	for (i = 1, mask = 2; i < 16; ++i, mask <<= 1)
> > > -		if (trying_irqs & mask)
> > > -			free_irq(i, NULL);
> > > +/**
> > > + * NCR5380_probe_irq	-	find the IRQ of an NCR5380
> > > + * @instance: NCR5380 controller
> > > + *
> > > + * Autoprobe for the IRQ line used by the NCR5380 by triggering an IRQ
> > > + * and then looking to see what interrupt actually turned up.
> > > + */
> > > +
> > > +static int NCR5380_probe_irq(struct Scsi_Host *instance)
> > > +{
> > > +	struct NCR5380_hostdata *hostdata = shost_priv(instance);
> > > +	int irqs, irq;
> > > +
> >
> > Please rename irqs to irq_mask.
> >
> > Other drivers like tty/cyclades.c begin with,
> >
> > 	/* forget possible initially masked and pending IRQ */
> > 	irq = probe_irq_off(probe_irq_on());
> >
> > Perhaps we need that here too?
> 
> Only cyclades and 8250_port do that, other drivers don't.
> Don't know what exact problem should that solve.

OK. I'm no expert on ISA so I'll leave it up to you.

-- 

> 
> >
> > > +	irqs = probe_irq_on();
> >
> > You should clear any pending interrupt on the chip before you try to
> > trigger another one.
> >
> > > +	NCR5380_trigger_irq(instance);
> >
> > Please do the msleep(1) here.
> >
> > > +	irq = probe_irq_off(irqs);
> > > +	NCR5380_read(RESET_PARITY_INTERRUPT_REG);
> > > +	if (irq < 0)
> > > +		irq = 0;
> >
> > Please use NO_IRQ instead of 0.
> >
> > > -	return probe_irq;
> > > +	return irq;
> > >  }
> > >
> > >  /**
> > > diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
> > > index 3c6ce54..b2fca52 100644
> > > --- a/drivers/scsi/NCR5380.h
> > > +++ b/drivers/scsi/NCR5380.h
> > > @@ -290,7 +290,8 @@ static inline struct scsi_cmnd
> > > *NCR5380_to_scmd(struct NCR5380_cmd *ncmd_ptr) #define
> > > NCR5380_dprint_phase(flg, arg) do {} while (0)
> > >  #endif
> > >
> > > -static int NCR5380_probe_irq(struct Scsi_Host *instance, int possible);
> > > +static void NCR5380_trigger_irq(struct Scsi_Host *instance);
> > > +static int NCR5380_probe_irq(struct Scsi_Host *instance);
> > >  static int NCR5380_init(struct Scsi_Host *instance, int flags);
> > >  static int NCR5380_maybe_reset_bus(struct Scsi_Host *);
> > >  static void NCR5380_exit(struct Scsi_Host *instance);
> >
> > Please move NCR5380_probe_irq and NCR5380_trigger_irq implementations 
> > and prototypes out of the core driver and into g_NCR5380.c. The reason 
> > is, ISA cards are probably the only 5380 cards ever likely to need 
> > manual IRQ configuration. It would better to have g_NCR5380 support 
> > more ISA cards than to add more ISA drivers, therefore this code isn't 
> > likely to be needed in the core driver.
> >
> > Thanks.
> 
> 
> 
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web