Re: [PATCH v4 34/78] atari_NCR5380: Use arbitration timeout

[Geert Uytterhoeven]

Hi Finn,

On Sun, Jan 3, 2016 at 6:05 AM, Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx> wrote:
> Allow target selection to fail with a timeout instead of waiting in
> infinite loops. This gets rid of the unused NCR_TIMEOUT macro, it is more
> defensive and has proved helpful in debugging.
>
> Signed-off-by: Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Hannes Reinecke <hare@xxxxxxxx>
> Tested-by: Ondrej Zary <linux@xxxxxxxxxxxxxxxxxxxx>
> Tested-by: Michael Schmitz <schmitzmic@xxxxxxxxx>

This patch (commit 55500d9b08295e3b6016b53879dea1cb7787f1b0) causes a hang
on ARAnyM with atari_defconfig after:

    scsi host0: Atari native SCSI, io_port 0x0, n_io_port 0, base 0x0,
irq 15, can_queue 8, cmd_per_lun 1, sg_tablesize 0, this_id 7, flags {
}, options { REAL_DMA SUPPORT_TAGS }
    blk_queue_max_segments: set to minimum 1

> --- linux.orig/drivers/scsi/atari_NCR5380.c     2016-01-03 16:03:43.000000000 +1100
> +++ linux/drivers/scsi/atari_NCR5380.c  2016-01-03 16:03:44.000000000 +1100
> @@ -1436,42 +1437,28 @@ static int NCR5380_select(struct Scsi_Ho
>         NCR5380_write(OUTPUT_DATA_REG, hostdata->id_mask);
>         NCR5380_write(MODE_REG, MR_ARBITRATE);
>
> -       local_irq_restore(flags);
> +       /* The chip now waits for BUS FREE phase. Then after the 800 ns
> +        * Bus Free Delay, arbitration will begin.
> +        */
>
> -       /* Wait for arbitration logic to complete */
> -#if defined(NCR_TIMEOUT)
> -       {
> -               unsigned long timeout = jiffies + 2*NCR_TIMEOUT;
> -
> -               while (!(NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_PROGRESS) &&
> -                      time_before(jiffies, timeout) && !hostdata->connected)
> -                       ;
> -               if (time_after_eq(jiffies, timeout)) {
> -                       printk("scsi : arbitration timeout at %d\n", __LINE__);
> +       local_irq_restore(flags);
> +       timeout = jiffies + HZ;
> +       while (1) {
> +               if (time_is_before_jiffies(timeout)) {
>                         NCR5380_write(MODE_REG, MR_BASE);
> -                       NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
> +                       shost_printk(KERN_ERR, instance,
> +                                    "select: arbitration timeout\n");
>                         return -1;
>                 }
> +               if (!(NCR5380_read(MODE_REG) & MR_ARBITRATE)) {

This newly added check always triggers, causing an infinite loop calling
NCR5380_select().
Perhaps this is an ARAnyM quirk?
If not, does it trigger (on some hardware) with drivers/scsi/NCR5380.c, too?

> +                       /* Reselection interrupt */
> +                       return -1;
> +               }
> +               if (NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_PROGRESS)
> +                       break;
>         }
> -#else /* NCR_TIMEOUT */
> -       while (!(NCR5380_read(INITIATOR_COMMAND_REG) & ICR_ARBITRATION_PROGRESS) &&
> -              !hostdata->connected)
> -               ;
> -#endif
> -
> -       dprintk(NDEBUG_ARBITRATION, "scsi%d: arbitration complete\n", HOSTNO);
> -
> -       if (hostdata->connected) {
> -               NCR5380_write(MODE_REG, MR_BASE);
> -               return -1;
> -       }
> -       /*
> -        * The arbitration delay is 2.2us, but this is a minimum and there is
> -        * no maximum so we can safely sleep for ceil(2.2) usecs to accommodate
> -        * the integral nature of udelay().
> -        *
> -        */
>
> +       /* The SCSI-2 arbitration delay is 2.4 us */
>         udelay(3);
>
>         /* Check for lost arbitration */

On current mainline, this (whitespace-damaged) patch fixed the issue for me:

--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -1253,10 +1253,6 @@ static struct scsi_cmnd *NCR5380_select(struct
Scsi_Host *instance,
                        INITIATOR_COMMAND_REG, ICR_ARBITRATION_PROGRESS,
                                               ICR_ARBITRATION_PROGRESS, HZ);
        spin_lock_irq(&hostdata->lock);
-       if (!(NCR5380_read(MODE_REG) & MR_ARBITRATE)) {
-               /* Reselection interrupt */
-               goto out;
-       }
        if (err < 0) {
                NCR5380_write(MODE_REG, MR_BASE);
                shost_printk(KERN_ERR, instance,
@@ -1297,10 +1293,6 @@ static struct scsi_cmnd *NCR5380_select(struct
Scsi_Host *instance,

        spin_lock_irq(&hostdata->lock);

-       /* NCR5380_reselect() clears MODE_REG after a reselection interrupt */
-       if (!(NCR5380_read(MODE_REG) & MR_ARBITRATE))
-               goto out;
-
        if (!hostdata->selecting) {
                NCR5380_write(MODE_REG, MR_BASE);
                NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds