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

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

[Ondrej Zary]

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.

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

[Ondrej Zary]

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

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

[Ondrej Zary]

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@rainbow-software.org>
---
 drivers/scsi/g_NCR5380.c |   44 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 09c660b..0d1f6ad 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -115,6 +115,32 @@ static int NCR5380_probe_irq(struct Scsi_Host *instance)
 	return irq;
 }
 
+static bool irq_working;
+
+static irqreturn_t test_irq(int irq, void *dev_id)
+{
+	irq_working = true;
+	return IRQ_HANDLED;
+}
+
+/* test if the IRQ is working */
+static int NCR5380_test_irq(struct Scsi_Host *instance, int irq)
+{
+	struct NCR5380_hostdata *hostdata = shost_priv(instance);
+
+	irq_working = false;
+	if (request_irq(irq, test_irq, 0, "NCR5380-irqtest", NULL))
+		return -EBUSY;
+	NCR5380_trigger_irq(instance);
+	NCR5380_read(RESET_PARITY_INTERRUPT_REG);
+	free_irq(irq, NULL);
+
+	if (!irq_working)
+		return -EIO;
+
+	return 0;
+}
+
 /*
  * Configure I/O address of 53C400A or DTC436 by writing magic numbers
  * to ports 0x779 and 0x379.
@@ -323,9 +349,21 @@ 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)) {
-			printk(KERN_WARNING "scsi%d : IRQ%d not free, interrupts disabled\n", instance->host_no, instance->irq);
+		ret = NCR5380_test_irq(instance, instance->irq);
+		if (ret) {
+			printk(KERN_WARNING "scsi%d : IRQ%d not %s, interrupts disabled\n",
+			       instance->host_no, instance->irq,
+			       (ret == -EBUSY) ? "free" : "working");
+			instance->irq = NO_IRQ;
+		}
+	}
+
+	if (instance->irq != NO_IRQ) {
+		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;
 		}
 	}
-- 
Ondrej Zary

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

[Ondrej Zary]

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

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

[Ondrej Zary]

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

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

[Ondrej Zary]

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

[PATCH 6/6] g_NCR5380: Fix release region in error handling

[Ondrej Zary]

When a SW-configurable card is specified but not found, the driver
releases wrong region, causing the following message in kernel log:
Trying to free nonexistent resource <0000000000000000-000000000000000f>

Fix it by assigning base earlier.

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

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 6a08d3e..d33e71f 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -258,12 +258,12 @@ static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
 		if (ports[i]) {
 			/* At this point we have our region reserved */
 			magic_configure(i, 0, magic); /* no IRQ yet */
-			outb(0xc0, ports[i] + 9);
-			if (inb(ports[i] + 9) != 0x80) {
+			base = ports[i];
+			outb(0xc0, base + 9);
+			if (inb(base + 9) != 0x80) {
 				ret = -ENODEV;
 				goto out_release;
 			}
-			base = ports[i];
 			port_idx = i;
 		} else
 			return -EINVAL;
-- 
Ondrej Zary

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

[Finn Thain]

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)
> 

-- 

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

[Finn Thain]

On Mon, 31 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.
> 

Once again, how does it cause a hang?

If the user specifies an IRQ, we should trust them. If they don't specify 
an IRQ then probe (as in patch 5/6).

> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
>  drivers/scsi/g_NCR5380.c |   44 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 09c660b..0d1f6ad 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -115,6 +115,32 @@ static int NCR5380_probe_irq(struct Scsi_Host *instance)
>  	return irq;
>  }
>  
> +static bool irq_working;
> +
> +static irqreturn_t test_irq(int irq, void *dev_id)
> +{
> +	irq_working = true;
> +	return IRQ_HANDLED;
> +}
> +
> +/* test if the IRQ is working */
> +static int NCR5380_test_irq(struct Scsi_Host *instance, int irq)
> +{
> +	struct NCR5380_hostdata *hostdata = shost_priv(instance);
> +
> +	irq_working = false;
> +	if (request_irq(irq, test_irq, 0, "NCR5380-irqtest", NULL))
> +		return -EBUSY;
> +	NCR5380_trigger_irq(instance);
> +	NCR5380_read(RESET_PARITY_INTERRUPT_REG);
> +	free_irq(irq, NULL);
> +
> +	if (!irq_working)
> +		return -EIO;
> +
> +	return 0;
> +}
> +

IMO the extra complexity is not justified by the possibility of machines 
with misconfigured BIOS or incorrect module parameters.

I don't want another irq probing mechanism. We just got rid of one.

But I won't mind if the SCSI maintainers who know ISA better than I do 
would like to review this.

>  /*
>   * Configure I/O address of 53C400A or DTC436 by writing magic numbers
>   * to ports 0x779 and 0x379.
> @@ -323,9 +349,21 @@ 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)) {
> -			printk(KERN_WARNING "scsi%d : IRQ%d not free, interrupts disabled\n", instance->host_no, instance->irq);
> +		ret = NCR5380_test_irq(instance, instance->irq);
> +		if (ret) {
> +			printk(KERN_WARNING "scsi%d : IRQ%d not %s, interrupts disabled\n",
> +			       instance->host_no, instance->irq,
> +			       (ret == -EBUSY) ? "free" : "working");
> +			instance->irq = NO_IRQ;
> +		}
> +	}
> +
> +	if (instance->irq != NO_IRQ) {
> +		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;
>  		}
>  	}
> 

-- 

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

[Finn Thain]

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;
> 

-- 

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

[Finn Thain]

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)
> 

-- 

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

[Finn Thain]

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)

-- 

Re: [PATCH 6/6] g_NCR5380: Fix release region in error handling

[Finn Thain]

On Mon, 31 Oct 2016, Ondrej Zary wrote:

> When a SW-configurable card is specified but not found, the driver
> releases wrong region, causing the following message in kernel log:
> Trying to free nonexistent resource <0000000000000000-000000000000000f>
> 
> Fix it by assigning base earlier.
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>

This fix is not related to the others in this series. It could go at the 
beginning where it is easy to cherry-pick.

Fixes: a8cfbcaec0c1 ("scsi: g_NCR5380: Stop using scsi_module.c")
Acked-by: Finn Thain <fthain@telegraphics.com.au>

> ---
>  drivers/scsi/g_NCR5380.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 6a08d3e..d33e71f 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -258,12 +258,12 @@ static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
>  		if (ports[i]) {
>  			/* At this point we have our region reserved */
>  			magic_configure(i, 0, magic); /* no IRQ yet */
> -			outb(0xc0, ports[i] + 9);
> -			if (inb(ports[i] + 9) != 0x80) {
> +			base = ports[i];
> +			outb(0xc0, base + 9);
> +			if (inb(base + 9) != 0x80) {
>  				ret = -ENODEV;
>  				goto out_release;
>  			}
> -			base = ports[i];
>  			port_idx = i;
>  		} else
>  			return -EINVAL;
> 

-- 

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

[Ondrej Zary]

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

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

[Ondrej Zary]

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

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

[Ondrej Zary]

On Wednesday 02 November 2016 08:45:26 Finn Thain wrote:
> On Mon, 31 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.
>
> Once again, how does it cause a hang?

Kernel scans the bus, finds a HDD, then attempts to read MBR. modprobe process 
is stuck but the system is still running. Then the transfer probably times 
out and everything locks up hard, even fbcon cursor stops blinking. I guess 
that kernel is trying to abort or reset.
BTW. rescan-scsi-bus also causes hang, anytime, even without IRQ.

> If the user specifies an IRQ, we should trust them. If they don't specify
> an IRQ then probe (as in patch 5/6).
>
> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> > ---
> >  drivers/scsi/g_NCR5380.c |   44
> > +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 41
> > insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> > index 09c660b..0d1f6ad 100644
> > --- a/drivers/scsi/g_NCR5380.c
> > +++ b/drivers/scsi/g_NCR5380.c
> > @@ -115,6 +115,32 @@ static int NCR5380_probe_irq(struct Scsi_Host
> > *instance) return irq;
> >  }
> >
> > +static bool irq_working;
> > +
> > +static irqreturn_t test_irq(int irq, void *dev_id)
> > +{
> > +	irq_working = true;
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +/* test if the IRQ is working */
> > +static int NCR5380_test_irq(struct Scsi_Host *instance, int irq)
> > +{
> > +	struct NCR5380_hostdata *hostdata = shost_priv(instance);
> > +
> > +	irq_working = false;
> > +	if (request_irq(irq, test_irq, 0, "NCR5380-irqtest", NULL))
> > +		return -EBUSY;
> > +	NCR5380_trigger_irq(instance);
> > +	NCR5380_read(RESET_PARITY_INTERRUPT_REG);
> > +	free_irq(irq, NULL);
> > +
> > +	if (!irq_working)
> > +		return -EIO;
> > +
> > +	return 0;
> > +}
> > +
>
> IMO the extra complexity is not justified by the possibility of machines
> with misconfigured BIOS or incorrect module parameters.
>
> I don't want another irq probing mechanism. We just got rid of one.
>
> But I won't mind if the SCSI maintainers who know ISA better than I do
> would like to review this.
>
> >  /*
> >   * Configure I/O address of 53C400A or DTC436 by writing magic numbers
> >   * to ports 0x779 and 0x379.
> > @@ -323,9 +349,21 @@ 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)) {
> > -			printk(KERN_WARNING "scsi%d : IRQ%d not free, interrupts disabled\n",
> > instance->host_no, instance->irq); +		ret = NCR5380_test_irq(instance,
> > instance->irq);
> > +		if (ret) {
> > +			printk(KERN_WARNING "scsi%d : IRQ%d not %s, interrupts disabled\n",
> > +			       instance->host_no, instance->irq,
> > +			       (ret == -EBUSY) ? "free" : "working");
> > +			instance->irq = NO_IRQ;
> > +		}
> > +	}
> > +
> > +	if (instance->irq != NO_IRQ) {
> > +		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;
> >  		}
> >  	}


-- 
Ondrej Zary

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

[Finn Thain]

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.

-- 

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

[Finn Thain]

On Wed, 2 Nov 2016, Ondrej Zary wrote:

> On Wednesday 02 November 2016 08:45:26 Finn Thain wrote:
> > On Mon, 31 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.
> >
> > Once again, how does it cause a hang?
> 
> Kernel scans the bus, finds a HDD, then attempts to read MBR. modprobe 
> process is stuck but the system is still running. Then the transfer 
> probably times out and everything locks up hard, even fbcon cursor stops 
> blinking. I guess that kernel is trying to abort or reset.

I don't think this issue relates to the patch, because the chip irq is not 
needed for exception handling.

A backtrace from the soft lockup detector should help explain this.

> BTW. rescan-scsi-bus also causes hang, anytime, even without IRQ.

I would try "scsi_logging_level -s -a 7" to find out what is going on 
during the bus scan (for modprobe or rescan-scsi-bus).

The polling loops in generic_NCR5380_pread/pwrite can cause a lockup 
because they lack timeouts. Better to call NCR5380_poll_politely, as in 
macscsi_pread/pwrite.

-- 

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

[Finn Thain]

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.

-- 

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

[Ondrej Zary]

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

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

[Finn Thain]

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).

-- 

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

[Finn Thain]

Hi Ondrej,

On Wed, 2 Nov 2016, I wrote:

> 
> 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);

Oops, this doesn't even compile! Sorry about that.

What I was trying to achieve was,

-static int ncr_irq;
+static int ncr_irq = IRQ_AUTO;

I will update my patch queue, compile test it, and ask you to review.

--