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

Groups > linux.kernel > #1512852 > unrolled thread

[PATCH 0/6] (g_)NCR5380: Improve IRQ probing and some fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] (g_)NCR5380: Improve IRQ probing and some fixes Ondrej Zary <linux@rainbow-software.org> - 2016-10-31 21:30 +0100
    [PATCH 1/6] NCR5380: Use probe_irq_*() for IRQ probing Ondrej Zary <linux@rainbow-software.org> - 2016-10-31 21:30 +0100
      Re: [PATCH 1/6] NCR5380: Use probe_irq_*() for IRQ probing Finn Thain <fthain@telegraphics.com.au> - 2016-11-02 08:50 +0100
        Re: [PATCH 1/6] NCR5380: Use probe_irq_*() for IRQ probing Ondrej Zary <linux@rainbow-software.org> - 2016-11-02 09:10 +0100
          Re: [PATCH 1/6] NCR5380: Use probe_irq_*() for IRQ probing Finn Thain <fthain@telegraphics.com.au> - 2016-11-03 03:20 +0100
    [PATCH 4/6] g_NCR5380: Add IRQ auto-configuration for HP C2502 Ondrej Zary <linux@rainbow-software.org> - 2016-10-31 21:30 +0100
      Re: [PATCH 4/6] g_NCR5380: Add IRQ auto-configuration for HP C2502 Finn Thain <fthain@telegraphics.com.au> - 2016-11-02 08:50 +0100
        Re: [PATCH 4/6] g_NCR5380: Add IRQ auto-configuration for HP C2502 Ondrej Zary <linux@rainbow-software.org> - 2016-11-02 09:30 +0100
          Re: [PATCH 4/6] g_NCR5380: Add IRQ auto-configuration for HP C2502 Finn Thain <fthain@telegraphics.com.au> - 2016-11-03 03:20 +0100
            Re: [PATCH 4/6] g_NCR5380: Add IRQ auto-configuration for HP C2502 Ondrej Zary <linux@rainbow-software.org> - 2016-11-03 09:10 +0100
              Re: [PATCH 4/6] g_NCR5380: Add IRQ auto-configuration for HP C2502 Finn Thain <fthain@telegraphics.com.au> - 2016-11-04 04:10 +0100
    [PATCH 3/6] g_NCR5380: Check for chip presence before calling NCR5380_init() Ondrej Zary <linux@rainbow-software.org> - 2016-10-31 21:30 +0100
      Re: [PATCH 3/6] g_NCR5380: Check for chip presence before calling  NCR5380_init() Finn Thain <fthain@telegraphics.com.au> - 2016-11-02 08:50 +0100
    [PATCH 5/6] g_NCR5380: Autoprobe IRQ by default Ondrej Zary <linux@rainbow-software.org> - 2016-10-31 21:30 +0100
      Re: [PATCH 5/6] g_NCR5380: Autoprobe IRQ by default Finn Thain <fthain@telegraphics.com.au> - 2016-11-02 08:50 +0100

#1512852 — [PATCH 0/6] (g_)NCR5380: Improve IRQ probing and some fixes

Hello,
this patch series improves IRQ probing and moves it from NCR5380 to
g_NCR5380, adds IRQ auto-configuration for HP C2502 and enables all
this by default. It also adds IRQ and base address checks to prevent
hangs when wrong values are specified by user. There's also a small
release region fix in error handling.

[toc] | [next] | [standalone]

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

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) and move the code from
NCR5380 to g_NCR5380.

Also add missing IRQ reset before and after the probe.

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

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index d849ffa..4f5ca79 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -97,9 +97,6 @@
  * and macros and include this file in your driver.
  *
  * These macros control options :
- * AUTOPROBE_IRQ - if defined, the NCR5380_probe_irq() function will be
- * defined.
- *
  * AUTOSENSE - if defined, REQUEST SENSE will be performed automatically
  * for commands that return with a CHECK CONDITION status.
  *
@@ -127,9 +124,7 @@
  * NCR5380_dma_residual   - residual byte count
  *
  * The generic driver is initialized by calling NCR5380_init(instance),
- * after setting the appropriate host specific fields and ID.  If the
- * driver wishes to autoprobe for an IRQ line, the NCR5380_probe_irq(instance,
- * possible) function may be used.
+ * after setting the appropriate host specific fields and ID.
  */
 
 #ifndef NCR5380_io_delay
@@ -351,76 +346,6 @@ 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)
-{
-	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;
-
-	/*
-	 * A interrupt is triggered whenever BSY = false, SEL = true
-	 * and a bit set in the SELECT_ENABLE_REG is asserted on the
-	 * SCSI bus.
-	 *
-	 * Note that the bus is only driven when the phase control signals
-	 * (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))
-		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);
-
-	return probe_irq;
-}
-
 /**
  * NCR58380_info - report driver and host information
  * @instance: relevant scsi host instance
diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
index 3c6ce54..4724558 100644
--- a/drivers/scsi/NCR5380.h
+++ b/drivers/scsi/NCR5380.h
@@ -290,7 +290,6 @@ 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 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..09c660b 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -67,6 +67,54 @@
 MODULE_ALIAS("g_NCR5380_mmio");
 MODULE_LICENSE("GPL");
 
+static void NCR5380_trigger_irq(struct Scsi_Host *instance)
+{
+	struct NCR5380_hostdata *hostdata = shost_priv(instance);
+
+	/*
+	 * An interrupt is triggered whenever BSY = false, SEL = true
+	 * and a bit set in the SELECT_ENABLE_REG is asserted on the
+	 * SCSI bus.
+	 *
+	 * Note that the bus is only driven when the phase control signals
+	 * (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);
+
+	usleep_range(1000, 20000);
+
+	NCR5380_write(SELECT_ENABLE_REG, 0);
+	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
+}
+
+/**
+ * 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 irq_mask, irq;
+
+	NCR5380_read(RESET_PARITY_INTERRUPT_REG);
+	irq_mask = probe_irq_on();
+	NCR5380_trigger_irq(instance);
+	irq = probe_irq_off(irq_mask);
+	NCR5380_read(RESET_PARITY_INTERRUPT_REG);
+	if (irq < 0)
+		irq = NO_IRQ;
+
+	return irq;
+}
+
 /*
  * Configure I/O address of 53C400A or DTC436 by writing magic numbers
  * to ports 0x779 and 0x379.
@@ -265,7 +313,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] | [prev] | [next] | [standalone]

#1513711 — Re: [PATCH 1/6] NCR5380: Use probe_irq_*() for IRQ probing

On Mon, 31 Oct 2016, Ondrej Zary wrote:

> 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) and move the code from NCR5380 
> to g_NCR5380.
> 
> Also add missing IRQ reset before and after the probe.
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
>  drivers/scsi/NCR5380.c   |   77 +---------------------------------------------
>  drivers/scsi/NCR5380.h   |    1 -
>  drivers/scsi/g_NCR5380.c |   50 +++++++++++++++++++++++++++++-
>  3 files changed, 50 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> index d849ffa..4f5ca79 100644
> --- a/drivers/scsi/NCR5380.c
> +++ b/drivers/scsi/NCR5380.c
> @@ -97,9 +97,6 @@
>   * and macros and include this file in your driver.
>   *
>   * These macros control options :
> - * AUTOPROBE_IRQ - if defined, the NCR5380_probe_irq() function will be
> - * defined.
> - *
>   * AUTOSENSE - if defined, REQUEST SENSE will be performed automatically
>   * for commands that return with a CHECK CONDITION status.
>   *
> @@ -127,9 +124,7 @@
>   * NCR5380_dma_residual   - residual byte count
>   *
>   * The generic driver is initialized by calling NCR5380_init(instance),
> - * after setting the appropriate host specific fields and ID.  If the
> - * driver wishes to autoprobe for an IRQ line, the NCR5380_probe_irq(instance,
> - * possible) function may be used.
> + * after setting the appropriate host specific fields and ID.
>   */
>  
>  #ifndef NCR5380_io_delay
> @@ -351,76 +346,6 @@ 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)
> -{
> -	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;
> -
> -	/*
> -	 * A interrupt is triggered whenever BSY = false, SEL = true
> -	 * and a bit set in the SELECT_ENABLE_REG is asserted on the
> -	 * SCSI bus.
> -	 *
> -	 * Note that the bus is only driven when the phase control signals
> -	 * (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))
> -		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);
> -
> -	return probe_irq;
> -}
> -
>  /**
>   * NCR58380_info - report driver and host information
>   * @instance: relevant scsi host instance
> diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
> index 3c6ce54..4724558 100644
> --- a/drivers/scsi/NCR5380.h
> +++ b/drivers/scsi/NCR5380.h
> @@ -290,7 +290,6 @@ 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 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..09c660b 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -67,6 +67,54 @@
>  MODULE_ALIAS("g_NCR5380_mmio");
>  MODULE_LICENSE("GPL");
>  
> +static void NCR5380_trigger_irq(struct Scsi_Host *instance)

That should be 'g_NCR5380_trigger_irq' (if it needs a prefix).

> +{
> +	struct NCR5380_hostdata *hostdata = shost_priv(instance);
> +
> +	/*
> +	 * An interrupt is triggered whenever BSY = false, SEL = true
> +	 * and a bit set in the SELECT_ENABLE_REG is asserted on the
> +	 * SCSI bus.
> +	 *
> +	 * Note that the bus is only driven when the phase control signals
> +	 * (I/O, C/D, and MSG) match those in the TCR, so we must reset that
> +	 * to zero.
> +	 */
> +	NCR5380_write(TARGET_COMMAND_REG, 0);

That assumes bus free phase, which we hope is the case but 
NCR5380_maybe_reset_bus() doesn't guarantee it. This is more robust:

	 * Note that the bus is only driven when the phase control signals
	 * (I/O, C/D, and MSG) match those in the TCR.
	 */
	NCR5380_write(TARGET_COMMAND_REG,
		PHASE_SR_TO_TCR(NCR5380_read(STATUS_REG) & PHASE_MASK));


> +	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);
> +
> +	usleep_range(1000, 20000);

Again, msleep(1) would be more appropriate here.

> +
> +	NCR5380_write(SELECT_ENABLE_REG, 0);
> +	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
> +}
> +
> +/**
> + * NCR5380_probe_irq	-	find the IRQ of an NCR5380

This should be 'g_NCR5380_probe_irq' (if it needs a prefix).

> + * @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 irq_mask, irq;
> +
> +	NCR5380_read(RESET_PARITY_INTERRUPT_REG);
> +	irq_mask = probe_irq_on();
> +	NCR5380_trigger_irq(instance);
> +	irq = probe_irq_off(irq_mask);
> +	NCR5380_read(RESET_PARITY_INTERRUPT_REG);
> +	if (irq < 0)
> +		irq = NO_IRQ;
> +
> +	return irq;
> +}
> +
>  /*
>   * Configure I/O address of 53C400A or DTC436 by writing magic numbers
>   * to ports 0x779 and 0x379.
> @@ -265,7 +313,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]

#1513723 — Re: [PATCH 1/6] NCR5380: Use probe_irq_*() for IRQ probing

On Wednesday 02 November 2016, Finn Thain wrote:
> On Mon, 31 Oct 2016, Ondrej Zary wrote:
> > 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) and move the code from NCR5380
> > to g_NCR5380.
> >
> > Also add missing IRQ reset before and after the probe.
> >
> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> > ---
> >  drivers/scsi/NCR5380.c   |   77
> > +--------------------------------------------- drivers/scsi/NCR5380.h   |
> >    1 -
> >  drivers/scsi/g_NCR5380.c |   50 +++++++++++++++++++++++++++++-
> >  3 files changed, 50 insertions(+), 78 deletions(-)
> >
> > diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> > index d849ffa..4f5ca79 100644
> > --- a/drivers/scsi/NCR5380.c
> > +++ b/drivers/scsi/NCR5380.c
> > @@ -97,9 +97,6 @@
> >   * and macros and include this file in your driver.
> >   *
> >   * These macros control options :
> > - * AUTOPROBE_IRQ - if defined, the NCR5380_probe_irq() function will be
> > - * defined.
> > - *
> >   * AUTOSENSE - if defined, REQUEST SENSE will be performed automatically
> >   * for commands that return with a CHECK CONDITION status.
> >   *
> > @@ -127,9 +124,7 @@
> >   * NCR5380_dma_residual   - residual byte count
> >   *
> >   * The generic driver is initialized by calling NCR5380_init(instance),
> > - * after setting the appropriate host specific fields and ID.  If the
> > - * driver wishes to autoprobe for an IRQ line, the
> > NCR5380_probe_irq(instance, - * possible) function may be used.
> > + * after setting the appropriate host specific fields and ID.
> >   */
> >
> >  #ifndef NCR5380_io_delay
> > @@ -351,76 +346,6 @@ 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)
> > -{
> > -	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;
> > -
> > -	/*
> > -	 * A interrupt is triggered whenever BSY = false, SEL = true
> > -	 * and a bit set in the SELECT_ENABLE_REG is asserted on the
> > -	 * SCSI bus.
> > -	 *
> > -	 * Note that the bus is only driven when the phase control signals
> > -	 * (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))
> > -		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);
> > -
> > -	return probe_irq;
> > -}
> > -
> >  /**
> >   * NCR58380_info - report driver and host information
> >   * @instance: relevant scsi host instance
> > diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
> > index 3c6ce54..4724558 100644
> > --- a/drivers/scsi/NCR5380.h
> > +++ b/drivers/scsi/NCR5380.h
> > @@ -290,7 +290,6 @@ 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 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..09c660b 100644
> > --- a/drivers/scsi/g_NCR5380.c
> > +++ b/drivers/scsi/g_NCR5380.c
> > @@ -67,6 +67,54 @@
> >  MODULE_ALIAS("g_NCR5380_mmio");
> >  MODULE_LICENSE("GPL");
> >
> > +static void NCR5380_trigger_irq(struct Scsi_Host *instance)
>
> That should be 'g_NCR5380_trigger_irq' (if it needs a prefix).
>
> > +{
> > +	struct NCR5380_hostdata *hostdata = shost_priv(instance);
> > +
> > +	/*
> > +	 * An interrupt is triggered whenever BSY = false, SEL = true
> > +	 * and a bit set in the SELECT_ENABLE_REG is asserted on the
> > +	 * SCSI bus.
> > +	 *
> > +	 * Note that the bus is only driven when the phase control signals
> > +	 * (I/O, C/D, and MSG) match those in the TCR, so we must reset that
> > +	 * to zero.
> > +	 */
> > +	NCR5380_write(TARGET_COMMAND_REG, 0);
>
> That assumes bus free phase, which we hope is the case but
> NCR5380_maybe_reset_bus() doesn't guarantee it. This is more robust:
>
> 	 * Note that the bus is only driven when the phase control signals
> 	 * (I/O, C/D, and MSG) match those in the TCR.
> 	 */
> 	NCR5380_write(TARGET_COMMAND_REG,
> 		PHASE_SR_TO_TCR(NCR5380_read(STATUS_REG) & PHASE_MASK));
>
> > +	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); +
> > +	usleep_range(1000, 20000);
>
> Again, msleep(1) would be more appropriate here.

WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
#164: FILE: drivers/scsi/g_NCR5380.c:88:
+       msleep(1);

> > +
> > +	NCR5380_write(SELECT_ENABLE_REG, 0);
> > +	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
> > +}
> > +
> > +/**
> > + * NCR5380_probe_irq	-	find the IRQ of an NCR5380
>
> This should be 'g_NCR5380_probe_irq' (if it needs a prefix).
>
> > + * @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 irq_mask, irq;
> > +
> > +	NCR5380_read(RESET_PARITY_INTERRUPT_REG);
> > +	irq_mask = probe_irq_on();
> > +	NCR5380_trigger_irq(instance);
> > +	irq = probe_irq_off(irq_mask);
> > +	NCR5380_read(RESET_PARITY_INTERRUPT_REG);
> > +	if (irq < 0)
> > +		irq = NO_IRQ;
> > +
> > +	return irq;
> > +}
> > +
> >  /*
> >   * Configure I/O address of 53C400A or DTC436 by writing magic numbers
> >   * to ports 0x779 and 0x379.
> > @@ -265,7 +313,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] | [prev] | [next] | [standalone]

#1514251 — Re: [PATCH 1/6] NCR5380: Use probe_irq_*() for IRQ probing

On Wed, 2 Nov 2016, Ondrej Zary wrote:

> On Wednesday 02 November 2016, Finn Thain wrote:
> > On Mon, 31 Oct 2016, Ondrej Zary wrote:
> >
> > > +	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);
> > > + 
> > > +	usleep_range(1000, 20000);
> >
> > Again, msleep(1) would be more appropriate here.
> 
> WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> #164: FILE: drivers/scsi/g_NCR5380.c:88:
> +       msleep(1);
> 

That link is almost 10 years old. I wonder if it is still accurate.

Anyway, I take it that you chose usleep_range() so as to make the 
checkpatch warning go away. Why not use checkpatch.conf for that, or just 
ignore it? The upper bound for the delay is unimportant and so the warning 
isn't applicable.

Since the lower bound is some unknown number of microseconds, I think 
msleep(1) nicely expresses the two constraints. Whereas, 
usleep_range(1000, 20000) misrepresents them.

We really don't need three significant figures. If you want precision, 
surely you would have to measure the time it takes for the IRQ to fire, 
and derive a worst case (lower bound) from the measurements.

-- 

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

#1512860 — [PATCH 4/6] g_NCR5380: Add IRQ auto-configuration for HP C2502

Find free and working IRQ automatically on HP C2502 cards.
Also allow IRQ 9 to work (aliases to IRQ 2 on the card).

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/scsi/g_NCR5380.c |   29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index e713dba..27fc499 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -156,6 +156,8 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
 	outb(magic[4], 0x379);
 
 	/* allowed IRQs for HP C2502 */
+	if (irq == 9)
+		irq = 2;
 	if (irq != 2 && irq != 3 && irq != 4 && irq != 5 && irq != 7)
 		irq = 0;
 	if (idx >= 0 && idx <= 7)
@@ -163,6 +165,21 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
 	outb(cfg, 0x379);
 }
 
+/* find a free and working IRQ (for HP C2502) */
+static int NCR5380_find_irq(struct Scsi_Host *instance, u8 irqs[],
+			    int port_idx, u8 magic[])
+{
+	int i;
+
+	for (i = 0; irqs[i]; i++) {
+		magic_configure(port_idx, irqs[i], magic);
+		if (NCR5380_test_irq(instance, irqs[i]) == 0)
+			return irqs[i];
+	}
+	magic_configure(port_idx, 0, magic);
+	return NO_IRQ;
+}
+
 static unsigned int ncr_53c400a_ports[] = {
 	0x280, 0x290, 0x300, 0x310, 0x330, 0x340, 0x348, 0x350, 0
 };
@@ -175,6 +192,9 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
 static u8 hp_c2502_magic[] = {	/* HP C2502 */
 	0x0f, 0x22, 0xf0, 0x20, 0x80
 };
+static u8 hp_c2502_irqs[] = {
+	9, 5, 7, 3, 4, 0
+};
 
 static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
 			struct device *pdev, int base, int irq, int board)
@@ -345,8 +365,13 @@ 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);
+	else {
+		if (board == BOARD_HP_C2502)
+			instance->irq = NCR5380_find_irq(instance,
+						hp_c2502_irqs, port_idx, magic);
+		else
+			instance->irq = NCR5380_probe_irq(instance);
+	}
 
 	/* Compatibility with documented NCR5380 kernel parameters */
 	if (instance->irq == 255)
-- 
Ondrej Zary

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

#1513708 — Re: [PATCH 4/6] g_NCR5380: Add IRQ auto-configuration for HP C2502

On Mon, 31 Oct 2016, Ondrej Zary wrote:

> Find free and working IRQ automatically on HP C2502 cards.
> Also allow IRQ 9 to work (aliases to IRQ 2 on the card).
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
>  drivers/scsi/g_NCR5380.c |   29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index e713dba..27fc499 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -156,6 +156,8 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
>  	outb(magic[4], 0x379);
>  
>  	/* allowed IRQs for HP C2502 */
> +	if (irq == 9)
> +		irq = 2;
>  	if (irq != 2 && irq != 3 && irq != 4 && irq != 5 && irq != 7)
>  		irq = 0;
>  	if (idx >= 0 && idx <= 7)
> @@ -163,6 +165,21 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
>  	outb(cfg, 0x379);
>  }
>  
> +/* find a free and working IRQ (for HP C2502) */
> +static int NCR5380_find_irq(struct Scsi_Host *instance, u8 irqs[],
> +			    int port_idx, u8 magic[])
> +{
> +	int i;
> +
> +	for (i = 0; irqs[i]; i++) {
> +		magic_configure(port_idx, irqs[i], magic);
> +		if (NCR5380_test_irq(instance, irqs[i]) == 0)
> +			return irqs[i];

The NCR5380_test_irq routine in patch 2/6 doesn't work for shared irqs, so 
you may not get the IRQ you would expect. (The core driver does support 
shared irqs, BTW.)

Also, you've ignored the irq module parameters. From the user's point of 
view, surely the least surprising thing is to attempt to configure the 
card for whatever irq the user asked for.

If the specified irq isn't supported by the board, just log an error and 
fail. If you want to be user friendly, print a message to tell them what 
irqs the card supports.

If the user asks for IRQ_AUTO, just configure the board for a hard-coded 
default, say 9, and print a warning message to say so.

Either way, if request_irq fails just continue with NO_IRQ, as per usual.

To me that's the most flexible and least surprising behaviour. But again, 
if someone with more ISA knowledge wishes to weigh in, that's fine too.

> +	}
> +	magic_configure(port_idx, 0, magic);
> +	return NO_IRQ;
> +}
> +
>  static unsigned int ncr_53c400a_ports[] = {
>  	0x280, 0x290, 0x300, 0x310, 0x330, 0x340, 0x348, 0x350, 0
>  };
> @@ -175,6 +192,9 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
>  static u8 hp_c2502_magic[] = {	/* HP C2502 */
>  	0x0f, 0x22, 0xf0, 0x20, 0x80
>  };
> +static u8 hp_c2502_irqs[] = {
> +	9, 5, 7, 3, 4, 0
> +};
>  
>  static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
>  			struct device *pdev, int base, int irq, int board)
> @@ -345,8 +365,13 @@ 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);
> +	else {
> +		if (board == BOARD_HP_C2502)
> +			instance->irq = NCR5380_find_irq(instance,
> +						hp_c2502_irqs, port_idx, magic);
> +		else
> +			instance->irq = NCR5380_probe_irq(instance);
> +	}
>  
>  	/* Compatibility with documented NCR5380 kernel parameters */
>  	if (instance->irq == 255)
> 

-- 

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

#1513733 — Re: [PATCH 4/6] g_NCR5380: Add IRQ auto-configuration for HP C2502

On Wednesday 02 November 2016, Finn Thain wrote:
> On Mon, 31 Oct 2016, Ondrej Zary wrote:
> > Find free and working IRQ automatically on HP C2502 cards.
> > Also allow IRQ 9 to work (aliases to IRQ 2 on the card).
> >
> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> > ---
> >  drivers/scsi/g_NCR5380.c |   29 +++++++++++++++++++++++++++--
> >  1 file changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> > index e713dba..27fc499 100644
> > --- a/drivers/scsi/g_NCR5380.c
> > +++ b/drivers/scsi/g_NCR5380.c
> > @@ -156,6 +156,8 @@ static void magic_configure(int idx, u8 irq, u8
> > magic[]) outb(magic[4], 0x379);
> >
> >  	/* allowed IRQs for HP C2502 */
> > +	if (irq == 9)
> > +		irq = 2;
> >  	if (irq != 2 && irq != 3 && irq != 4 && irq != 5 && irq != 7)
> >  		irq = 0;
> >  	if (idx >= 0 && idx <= 7)
> > @@ -163,6 +165,21 @@ static void magic_configure(int idx, u8 irq, u8
> > magic[]) outb(cfg, 0x379);
> >  }
> >
> > +/* find a free and working IRQ (for HP C2502) */
> > +static int NCR5380_find_irq(struct Scsi_Host *instance, u8 irqs[],
> > +			    int port_idx, u8 magic[])
> > +{
> > +	int i;
> > +
> > +	for (i = 0; irqs[i]; i++) {
> > +		magic_configure(port_idx, irqs[i], magic);
> > +		if (NCR5380_test_irq(instance, irqs[i]) == 0)
> > +			return irqs[i];
>
> The NCR5380_test_irq routine in patch 2/6 doesn't work for shared irqs, so
> you may not get the IRQ you would expect. (The core driver does support
> shared irqs, BTW.)

ISA bus does not support IRQ sharing.

> Also, you've ignored the irq module parameters. From the user's point of
> view, surely the least surprising thing is to attempt to configure the
> card for whatever irq the user asked for.

I haven't. NCR5380_find_irq is only called when irq is set to IRQ_AUTO.

> If the specified irq isn't supported by the board, just log an error and
> fail. If you want to be user friendly, print a message to tell them what
> irqs the card supports.

If the IRQ is not supported (or does not work), user gets a warning and the 
driver continues with IRQ disabled.

> If the user asks for IRQ_AUTO, just configure the board for a hard-coded
> default, say 9, and print a warning message to say so.

The card is almost Plug&Play. The base address is already configured 
automatically by the driver so doing the same for IRQ makes sense.

> Either way, if request_irq fails just continue with NO_IRQ, as per usual.
>
> To me that's the most flexible and least surprising behaviour. But again,
> if someone with more ISA knowledge wishes to weigh in, that's fine too.
>
> > +	}
> > +	magic_configure(port_idx, 0, magic);
> > +	return NO_IRQ;
> > +}
> > +
> >  static unsigned int ncr_53c400a_ports[] = {
> >  	0x280, 0x290, 0x300, 0x310, 0x330, 0x340, 0x348, 0x350, 0
> >  };
> > @@ -175,6 +192,9 @@ static void magic_configure(int idx, u8 irq, u8
> > magic[]) static u8 hp_c2502_magic[] = {	/* HP C2502 */
> >  	0x0f, 0x22, 0xf0, 0x20, 0x80
> >  };
> > +static u8 hp_c2502_irqs[] = {
> > +	9, 5, 7, 3, 4, 0
> > +};
> >
> >  static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
> >  			struct device *pdev, int base, int irq, int board)
> > @@ -345,8 +365,13 @@ 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);
> > +	else {
> > +		if (board == BOARD_HP_C2502)
> > +			instance->irq = NCR5380_find_irq(instance,
> > +						hp_c2502_irqs, port_idx, magic);
> > +		else
> > +			instance->irq = NCR5380_probe_irq(instance);
> > +	}
> >
> >  	/* Compatibility with documented NCR5380 kernel parameters */
> >  	if (instance->irq == 255)



-- 
Ondrej Zary

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

#1514250 — Re: [PATCH 4/6] g_NCR5380: Add IRQ auto-configuration for HP C2502

On Wed, 2 Nov 2016, Ondrej Zary wrote:

> > Also, you've ignored the irq module parameters. From the user's point 
> > of view, surely the least surprising thing is to attempt to configure 
> > the card for whatever irq the user asked for.
> 
> I haven't. NCR5380_find_irq is only called when irq is set to IRQ_AUTO.
> 

My mistake.

> > If the specified irq isn't supported by the board, just log an error 
> > and fail. If you want to be user friendly, print a message to tell 
> > them what irqs the card supports.
> 
> If the IRQ is not supported (or does not work), user gets a warning and 
> the driver continues with IRQ disabled.
> 
> > If the user asks for IRQ_AUTO, just configure the board for a 
> > hard-coded default, say 9, and print a warning message to say so.
> 
> The card is almost Plug&Play. The base address is already configured 
> automatically by the driver so doing the same for IRQ makes sense.

Why don't we see any other drivers doing this?

If the card was really plug and play, I expect we would just call 
pnp_irq(), as the other PNP drivers do.

> 
> > Either way, if request_irq fails just continue with NO_IRQ, as per 
> > usual.
> >
> > To me that's the most flexible and least surprising behaviour. But 
> > again, if someone with more ISA knowledge wishes to weigh in, that's 
> > fine too.

-- 

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

#1514361 — Re: [PATCH 4/6] g_NCR5380: Add IRQ auto-configuration for HP C2502

On Thursday 03 November 2016, Finn Thain wrote:
> On Wed, 2 Nov 2016, Ondrej Zary wrote:
> > > Also, you've ignored the irq module parameters. From the user's point
> > > of view, surely the least surprising thing is to attempt to configure
> > > the card for whatever irq the user asked for.
> >
> > I haven't. NCR5380_find_irq is only called when irq is set to IRQ_AUTO.
>
> My mistake.
>
> > > If the specified irq isn't supported by the board, just log an error
> > > and fail. If you want to be user friendly, print a message to tell
> > > them what irqs the card supports.
> >
> > If the IRQ is not supported (or does not work), user gets a warning and
> > the driver continues with IRQ disabled.
> >
> > > If the user asks for IRQ_AUTO, just configure the board for a
> > > hard-coded default, say 9, and print a warning message to say so.
> >
> > The card is almost Plug&Play. The base address is already configured
> > automatically by the driver so doing the same for IRQ makes sense.
>
> Why don't we see any other drivers doing this?

Many ISA sound card drivers do this - there's even a function for this:
static int snd_legacy_find_free_irq(int *irq_table)

Unfortunately, it's defined in ALSA headers and even protected with an #ifdef.

> If the card was really plug and play, I expect we would just call
> pnp_irq(), as the other PNP drivers do.

The card predates the PnP standard so we can't.

> > > Either way, if request_irq fails just continue with NO_IRQ, as per
> > > usual.
> > >
> > > To me that's the most flexible and least surprising behaviour. But
> > > again, if someone with more ISA knowledge wishes to weigh in, that's
> > > fine too.



-- 
Ondrej Zary

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

#1514983 — Re: [PATCH 4/6] g_NCR5380: Add IRQ auto-configuration for HP C2502

On Thu, 3 Nov 2016, Ondrej Zary wrote:

> On Thursday 03 November 2016, Finn Thain wrote:
> > On Wed, 2 Nov 2016, Ondrej Zary wrote:
> > > 
> > > The card is almost Plug&Play. The base address is already configured 
> > > automatically by the driver so doing the same for IRQ makes sense.
> >
> > Why don't we see any other drivers doing this?
> 
> Many ISA sound card drivers do this - there's even a function for this: 
> static int snd_legacy_find_free_irq(int *irq_table)

Well, if snd_legacy_find_free_irq() is good enough for the ALSA 
maintainers, it is good enough for me.

But I still don't like this patch. I would prefer to cut-and-paste 
snd_legacy_find_free_irq() as g_NCR5380_find_free_irq(), and 
snd_legacy_empty_irq_handler() as g_NCR5380_empty_irq_handler(), to allow 
for refactoring all that into a common header file later on.

When the user asks for board == BOARD_HP_C2502 and irq == IRQ_AUTO, just 
use the first result from g_NCR5380_find_free_irq() to program the card. 
If no irq is free, program the card for irq 0.

Then just proceed with the usual IRQ_AUTO probing, like you would for any 
other board. That way, there is no need for NCR5380_test_irq() at all, and 
you can drop patch 2/5 (at least until we figure out why a command timeout 
during modprobe hangs your system).

-- 

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

#1512864 — [PATCH 3/6] g_NCR5380: Check for chip presence before calling NCR5380_init()

Write and read back MODE_REG to check if the chip is really there
before doing more initialization.

This prevents hang when incorrect I/O address was specified by user (in
the most common case where no device is present there so all reads
result in 0xff).

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/scsi/g_NCR5380.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 0d1f6ad..e713dba 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -322,6 +322,13 @@ static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
 		}
 	}
 
+	/* check if the chip is really there */
+	NCR5380_write(MODE_REG, 0);
+	if (NCR5380_read(MODE_REG) != 0) {
+		ret = -ENODEV;
+		goto out_unregister;
+	}
+
 	ret = NCR5380_init(instance, flags | FLAG_LATE_DMA_SETUP);
 	if (ret)
 		goto out_unregister;
-- 
Ondrej Zary

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

#1513706 — Re: [PATCH 3/6] g_NCR5380: Check for chip presence before calling NCR5380_init()

On Mon, 31 Oct 2016, Ondrej Zary wrote:

> Write and read back MODE_REG to check if the chip is really there
> before doing more initialization.
> 
> This prevents hang when incorrect I/O address was specified by user (in
> the most common case where no device is present there so all reads
> result in 0xff).
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
>  drivers/scsi/g_NCR5380.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 0d1f6ad..e713dba 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -322,6 +322,13 @@ static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
>  		}
>  	}
>  
> +	/* check if the chip is really there */
> +	NCR5380_write(MODE_REG, 0);
> +	if (NCR5380_read(MODE_REG) != 0) {

Wouldn't it be more accurate to write,

	/* Check for some kind of device. A vacant slot reads 0xff. */

?

> +		ret = -ENODEV;
> +		goto out_unregister;
> +	}
> +
>  	ret = NCR5380_init(instance, flags | FLAG_LATE_DMA_SETUP);
>  	if (ret)
>  		goto out_unregister;
> 

-- 

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

#1512868 — [PATCH 5/6] g_NCR5380: Autoprobe IRQ by default

IRQ probing seems to work fine now. Default to autoprobe for IRQ instead
of disabling it.

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/scsi/g_NCR5380.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 27fc499..6a08d3e 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -52,9 +52,9 @@
 module_param(dtc_3181e, int, 0);
 module_param(hp_c2502, int, 0);
 
-static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+static int irq[] = { IRQ_AUTO, IRQ_AUTO, IRQ_AUTO, IRQ_AUTO, IRQ_AUTO, IRQ_AUTO, IRQ_AUTO, IRQ_AUTO };
 module_param_array(irq, int, NULL, 0);
-MODULE_PARM_DESC(irq, "IRQ number(s)");
+MODULE_PARM_DESC(irq, "IRQ number(s) (0=disable, 254=auto [default])");
 
 static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
 module_param_array(base, int, NULL, 0);
-- 
Ondrej Zary

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

#1513712 — Re: [PATCH 5/6] g_NCR5380: Autoprobe IRQ by default

On Mon, 31 Oct 2016, Ondrej Zary wrote:

> IRQ probing seems to work fine now. Default to autoprobe for IRQ instead
> of disabling it.
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
>  drivers/scsi/g_NCR5380.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 27fc499..6a08d3e 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -52,9 +52,9 @@
>  module_param(dtc_3181e, int, 0);
>  module_param(hp_c2502, int, 0);
>  
> -static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +static int irq[] = { IRQ_AUTO, IRQ_AUTO, IRQ_AUTO, IRQ_AUTO, IRQ_AUTO, IRQ_AUTO, IRQ_AUTO, IRQ_AUTO };
>  module_param_array(irq, int, NULL, 0);
> -MODULE_PARM_DESC(irq, "IRQ number(s)");
> +MODULE_PARM_DESC(irq, "IRQ number(s) (0=disable, 254=auto [default])");
>  
>  static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
>  module_param_array(base, int, NULL, 0);
> 

I think this patch is incomplete and you should add these changes:

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 7299ad9..0bf0322 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -44,7 +44,7 @@ static int ncr_53c400;
 static int ncr_53c400a;
 static int dtc_3181e;
 static int hp_c2502;
-module_param(ncr_irq, int, 0);
+module_param(ncr_irq, int, IRQ_AUTO);
 module_param(ncr_addr, int, 0);
 module_param(ncr_5380, int, 0);
 module_param(ncr_53c400, int, 0);
@@ -597,7 +597,7 @@ static int __init generic_NCR5380_init(void)
 	int ret = 0;
 
 	/* compatibility with old-style parameters */
-	if (irq[0] == 0 && base[0] == 0 && card[0] == -1) {
+	if (irq[0] == IRQ_AUTO && base[0] == 0 && card[0] == -1) {
 		irq[0] = ncr_irq;
 		base[0] = ncr_addr;
 		if (ncr_5380)

-- 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web