[PATCH v3 05/77] ncr5380: Remove NCR5380_local_declare and NCR5380_setup macros

[PATCH v3 05/77] ncr5380: Remove NCR5380_local_declare and NCR5380_setup macros

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-eliminate-local_declare-macros
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151222/549109a4/attachment.ksh>

[PATCH v3 07/77] ncr5380: Split NCR5380_init() into two functions

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-bus-wedge
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151222/5b68b3ec/attachment.ksh>

[PATCH v3 19/77] ncr5380: Cleanup bogus {request, release}_region() calls

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-cleanup-request_region-release_region
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151222/25241aca/attachment.ksh>

[PATCH v3 20/77] ncr5380: Introduce unbound workqueue

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-new-workqueue
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151222/6b3b55b9/attachment.ksh>

[PATCH v3 24/77] ncr5380: Implement NCR5380_dma_xfer_len and remove LIMIT_TRANSFERSIZE macro

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-implement-NCR5380_dma_xfer_len
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151222/6fda5267/attachment.ksh>

[PATCH v3 45/77] ncr5380: Cleanup #include directives

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-move-core-driver-includes
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151222/30b5adb2/attachment.ksh>

[PATCH v3 51/77] ncr5380: Remove command list debug code

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-remove-cmd-list-debug-code
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151222/315cec0b/attachment.ksh>

[PATCH v3 57/77] ncr5380: Use standard list data structure

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-use-list_head
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151222/075e489a/attachment.ksh>

[PATCH v3 66/77] ncr5380: Fix soft lockups

[Finn Thain]

An embedded and charset-unspecified text was scrubbed...
Name: ncr5380-max_sectors
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151222/03d978a4/attachment.ksh>

[PATCH v3 19/77] ncr5380: Cleanup bogus {request,release}_region() calls

[Hannes Reinecke]

On 12/22/2015 02:17 AM, Finn Thain wrote:
> Commit 8b801ead3d7a ("[ARM] rpc: update Acorn SCSI drivers to modern ecard
> interfaces") neglected to remove a request_region() call in cumana_1.c.
>
> Commit eda32612f7b2 ("[PATCH] give all LLDD driver a ->release method") in
> history/history.git added some pointless release_region() calls in dtc.c,
> pas16.c and t128.c.
>
> Fix these issues.
>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
>

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: J. Hawn, J. Guild, F. Imend?rffer, HRB 16746 (AG N?rnberg)

[PATCH v3 20/77] ncr5380: Introduce unbound workqueue

[Hannes Reinecke]

On 12/22/2015 02:17 AM, Finn Thain wrote:
> Allocate a work queue that will permit busy waiting and sleeping. This
> means NCR5380_init() can potentially fail, so add this error path.
>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
>
> ---
>
> In subsequent patches, the work function adopts this work queue so it
> can sleep while polling, which allows the removal of some flawed and
> complicated code in NCR5380_select() in NCR5380.c.
>
> Changed since v1:
> - Dropped WQ_CPU_INTENSIVE flag because Documentation/workqueue.txt says it
>    "is meaningless for unbound wq".
>
> ---
>   drivers/scsi/NCR5380.c       |   15 +++++++++++----
>   drivers/scsi/NCR5380.h       |    1 +
>   drivers/scsi/arm/cumana_1.c  |    8 ++++++--
>   drivers/scsi/arm/oak.c       |    8 ++++++--
>   drivers/scsi/atari_NCR5380.c |    8 +++++++-
>   drivers/scsi/atari_scsi.c    |    5 ++++-
>   drivers/scsi/dmx3191d.c      |   17 +++++++++++------
>   drivers/scsi/dtc.c           |   11 +++++++++--
>   drivers/scsi/g_NCR5380.c     |   31 +++++++++++++++----------------
>   drivers/scsi/mac_scsi.c      |    5 ++++-
>   drivers/scsi/pas16.c         |   10 ++++++++--
>   drivers/scsi/sun3_scsi.c     |    5 ++++-
>   drivers/scsi/t128.c          |   13 ++++++++++---
>   13 files changed, 96 insertions(+), 41 deletions(-)
>
> Index: linux/drivers/scsi/NCR5380.c
> ===================================================================
> --- linux.orig/drivers/scsi/NCR5380.c	2015-12-22 12:15:52.000000000 +1100
> +++ linux/drivers/scsi/NCR5380.c	2015-12-22 12:15:56.000000000 +1100
> @@ -514,7 +514,7 @@ static int should_disconnect(unsigned ch
>   static void NCR5380_set_timer(struct NCR5380_hostdata *hostdata, unsigned long timeout)
>   {
>   	hostdata->time_expires = jiffies + timeout;
> -	schedule_delayed_work(&hostdata->coroutine, timeout);
> +	queue_delayed_work(hostdata->work_q, &hostdata->coroutine, timeout);
>   }
>
>
> @@ -791,7 +791,12 @@ static int NCR5380_init(struct Scsi_Host
>   	hostdata->disconnected_queue = NULL;
>   	
>   	INIT_DELAYED_WORK(&hostdata->coroutine, NCR5380_main);
> -	
> +	hostdata->work_q = alloc_workqueue("ncr5380_%d",
> +	                        WQ_UNBOUND | WQ_MEM_RECLAIM,
> +	                        1, instance->host_no);
> +	if (!hostdata->work_q)
> +		return -ENOMEM;
> +
>   	/* The CHECK code seems to break the 53C400. Will check it later maybe */
>   	if (flags & FLAG_NCR53C400)
>   		hostdata->flags = FLAG_HAS_LAST_BYTE_SENT | flags;

Wouldn't it be better to use a normal (ie bound) workqueue here?
SCSI-2 is pretty much single-threaded, so shifting things onto arbitrary 
CPUs don't sound very appealing.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: J. Hawn, J. Guild, F. Imend?rffer, HRB 16746 (AG N?rnberg)

[PATCH v3 24/77] ncr5380: Implement NCR5380_dma_xfer_len and remove LIMIT_TRANSFERSIZE macro

[Hannes Reinecke]

On 12/22/2015 02:18 AM, Finn Thain wrote:
> Follow the example of the atari_NCR5380.c core driver and adopt the
> NCR5380_dma_xfer_len() hook. Implement NCR5380_dma_xfer_len() for dtc.c
> and g_NCR5380.c to take care of the limitations of these cards. Keep the
> default for drivers using PSEUDO_DMA.
>
> Eliminate the unused macro LIMIT_TRANSFERSIZE.
>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
>
> ---
>   drivers/scsi/NCR5380.c       |   32 +++++---------------------------
>   drivers/scsi/arm/cumana_1.c  |    3 +++
>   drivers/scsi/arm/oak.c       |    2 ++
>   drivers/scsi/atari_NCR5380.c |    8 +++++---
>   drivers/scsi/dtc.c           |   14 ++++++++++++++
>   drivers/scsi/dtc.h           |    3 +++
>   drivers/scsi/g_NCR5380.c     |   15 +++++++++++++++
>   drivers/scsi/g_NCR5380.h     |    3 +++
>   drivers/scsi/mac_scsi.c      |    1 +
>   drivers/scsi/pas16.h         |    2 ++
>   drivers/scsi/t128.h          |    2 ++
>   11 files changed, 55 insertions(+), 30 deletions(-)
>
Hmm. I really, _really_, wish we could move away from the 'magic' 
definitions and use a proper function template.
But maybe next time.

So:

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: J. Hawn, J. Guild, F. Imend?rffer, HRB 16746 (AG N?rnberg)

[PATCH v3 45/77] ncr5380: Cleanup #include directives

[Hannes Reinecke]

On 12/22/2015 02:18 AM, Finn Thain wrote:
> Remove unused includes (stat.h, signal.h, proc_fs.h) and move includes
> needed by the core drivers into the common header (delay.h etc).
>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
>
> ---
>   drivers/scsi/NCR5380.c       |    2 --
>   drivers/scsi/NCR5380.h       |    4 ++++
>   drivers/scsi/arm/cumana_1.c  |    4 ----
>   drivers/scsi/arm/oak.c       |    2 --
>   drivers/scsi/atari_NCR5380.c |    5 -----
>   drivers/scsi/atari_scsi.c    |    1 -
>   drivers/scsi/dmx3191d.c      |    5 -----
>   drivers/scsi/dtc.c           |    4 +---
>   drivers/scsi/g_NCR5380.c     |    6 ++----
>   drivers/scsi/mac_scsi.c      |    1 -
>   drivers/scsi/pas16.c         |    4 ----
>   drivers/scsi/t128.c          |    3 ---
>   12 files changed, 7 insertions(+), 34 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: J. Hawn, J. Guild, F. Imend?rffer, HRB 16746 (AG N?rnberg)

[PATCH v3 51/77] ncr5380: Remove command list debug code

[Hannes Reinecke]

On 12/22/2015 02:18 AM, Finn Thain wrote:
> Some NCR5380 hosts offer a .show_info method to access the contents of
> the various command list data structures from a procfs file. When NDEBUG
> is set, the same information is sent to the console during EH.
>
> The two core drivers, atari_NCR5380.c and NCR5380.c differ here. Because
> it is just for debugging, the easiest way to fix the discrepancy is
> simply remove this code.
>
> The only remaining users of NCR5380_show_info() and NCR5380_write_info()
> are drivers that define PSEUDO_DMA. The others have no use for the
> .show_info method, so don't initialize it.
>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
>
> ---
>   drivers/scsi/NCR5380.c       |   70 ++------------------------------
>   drivers/scsi/arm/oak.c       |    2
>   drivers/scsi/atari_NCR5380.c |   94 +------------------------------------------
>   drivers/scsi/atari_scsi.c    |    2
>   drivers/scsi/g_NCR5380.c     |    1
>   drivers/scsi/sun3_scsi.c     |    2
>   6 files changed, 9 insertions(+), 162 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: J. Hawn, J. Guild, F. Imend?rffer, HRB 16746 (AG N?rnberg)

[PATCH v3 57/77] ncr5380: Use standard list data structure

[Hannes Reinecke]

On 12/22/2015 02:18 AM, Finn Thain wrote:
> The NCR5380 drivers have a home-spun linked list implementation for
> scsi_cmnd structs that uses cmd->host_scribble as a 'next' pointer. Adopt
> the standard list_head data structure and list operations instead. Remove
> the eh_abort_handler rather than convert it. Doing the conversion would
> only be churn because the existing EH handlers don't work and get replaced
> in a subsequent patch.
>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
>
> ---
>
> Changed since v2:
> - Fix NULL pointer dereference in NCR5380_reselect() when SUPPORT_TAGS is
>    enabled in the atari_NCR5380.c core driver.
>
Well, using ->host_scribble allows for an easy check on the midlayer if 
a command has been properly released by the LLDD. But that's just a 
side-note.

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: J. Hawn, J. Guild, F. Imend?rffer, HRB 16746 (AG N?rnberg)

[PATCH v3 66/77] ncr5380: Fix soft lockups

[Hannes Reinecke]

On 12/22/2015 02:18 AM, Finn Thain wrote:
> Because of the rudimentary design of the chip, it is necessary to poll the
> SCSI bus signals during PIO and this tends to hog the CPU. The driver will
> accept new commands while others execute, and this causes a soft lockup
> because the workqueue item will not terminate until the issue queue is
> emptied.
>
> When exercising dmx3191d using sequential IO from dd, the driver is sent
> 512 KiB WRITE commands and 128 KiB READs. For a PIO transfer, the rate is
> is only about 300 KiB/s, so these are long-running commands. And although
> PDMA may run at several MiB/s, interrupts are disabled for the duration
> of the transfer.
>
> Fix the unresponsiveness and soft lockup issues by calling cond_resched()
> after each command is completed and by limiting max_sectors for drivers
> that don't implement real DMA.
>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
>
> ---
>
> Changed since v2:
> - Moved max_sectors initialization to wrapper drivers. It isn't really
>    relevant to the core driver and compile-time configuration using macros
>    like REAL_DMA should be avoided.
>
> ---
>   drivers/scsi/NCR5380.c       |    6 ++++--
>   drivers/scsi/arm/cumana_1.c  |    1 +
>   drivers/scsi/arm/oak.c       |    1 +
>   drivers/scsi/atari_NCR5380.c |    6 ++++--
>   drivers/scsi/dmx3191d.c      |    1 +
>   drivers/scsi/dtc.c           |    1 +
>   drivers/scsi/g_NCR5380.c     |    1 +
>   drivers/scsi/mac_scsi.c      |    1 +
>   drivers/scsi/pas16.c         |    1 +
>   drivers/scsi/t128.c          |    1 +
>   10 files changed, 16 insertions(+), 4 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: J. Hawn, J. Guild, F. Imend?rffer, HRB 16746 (AG N?rnberg)

[PATCH v3 66/77] ncr5380: Fix soft lockups

[One Thousand Gnomes]

On Tue, 22 Dec 2015 12:18:44 +1100
Finn Thain <fthain@telegraphics.com.au> wrote:

> Because of the rudimentary design of the chip, it is necessary to poll the
> SCSI bus signals during PIO and this tends to hog the CPU. The driver will
> accept new commands while others execute, and this causes a soft lockup
> because the workqueue item will not terminate until the issue queue is
> emptied.
> 
> When exercising dmx3191d using sequential IO from dd, the driver is sent
> 512 KiB WRITE commands and 128 KiB READs. For a PIO transfer, the rate is
> is only about 300 KiB/s, so these are long-running commands. And although
> PDMA may run at several MiB/s, interrupts are disabled for the duration
> of the transfer.
> 
> Fix the unresponsiveness and soft lockup issues by calling cond_resched()
> after each command is completed and by limiting max_sectors for drivers
> that don't implement real DMA.

Is there a reason for not doing some limiting in the DMA case too. A 512K
write command even with DMA on a low end 68K box introduces a second of
latency before another I/O can be scheduled ?

Alan

[PATCH v3 20/77] ncr5380: Introduce unbound workqueue

[Finn Thain]

On Tue, 22 Dec 2015, Hannes Reinecke wrote:

> On 12/22/2015 02:17 AM, Finn Thain wrote:
> > Allocate a work queue that will permit busy waiting and sleeping. This
> > means NCR5380_init() can potentially fail, so add this error path.
> >
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> >
> > ---
> >
> > In subsequent patches, the work function adopts this work queue so it
> > can sleep while polling, which allows the removal of some flawed and
> > complicated code in NCR5380_select() in NCR5380.c.
> >
> > Changed since v1:
> > - Dropped WQ_CPU_INTENSIVE flag because Documentation/workqueue.txt says it
> >    "is meaningless for unbound wq".
> >
> > ---
> >   drivers/scsi/NCR5380.c       |   15 +++++++++++----
> >   drivers/scsi/NCR5380.h       |    1 +
> >   drivers/scsi/arm/cumana_1.c  |    8 ++++++--
> >   drivers/scsi/arm/oak.c       |    8 ++++++--
> >   drivers/scsi/atari_NCR5380.c |    8 +++++++-
> >   drivers/scsi/atari_scsi.c    |    5 ++++-
> >   drivers/scsi/dmx3191d.c      |   17 +++++++++++------
> >   drivers/scsi/dtc.c           |   11 +++++++++--
> >   drivers/scsi/g_NCR5380.c     |   31 +++++++++++++++----------------
> >   drivers/scsi/mac_scsi.c      |    5 ++++-
> >   drivers/scsi/pas16.c         |   10 ++++++++--
> >   drivers/scsi/sun3_scsi.c     |    5 ++++-
> >   drivers/scsi/t128.c          |   13 ++++++++++---
> >   13 files changed, 96 insertions(+), 41 deletions(-)
> >
> > Index: linux/drivers/scsi/NCR5380.c
> > ===================================================================
> > --- linux.orig/drivers/scsi/NCR5380.c	2015-12-22 12:15:52.000000000 +1100
> > +++ linux/drivers/scsi/NCR5380.c	2015-12-22 12:15:56.000000000 +1100
> > @@ -514,7 +514,7 @@ static int should_disconnect(unsigned ch
> >   static void NCR5380_set_timer(struct NCR5380_hostdata *hostdata, unsigned
> >   long timeout)
> >   {
> >   	hostdata->time_expires = jiffies + timeout;
> > -	schedule_delayed_work(&hostdata->coroutine, timeout);
> > +	queue_delayed_work(hostdata->work_q, &hostdata->coroutine, timeout);
> >   }
> >
> >
> > @@ -791,7 +791,12 @@ static int NCR5380_init(struct Scsi_Host
> >    hostdata->disconnected_queue = NULL;
> >    
> >    INIT_DELAYED_WORK(&hostdata->coroutine, NCR5380_main);
> > -	
> > +	hostdata->work_q = alloc_workqueue("ncr5380_%d",
> > +	                        WQ_UNBOUND | WQ_MEM_RECLAIM,
> > +	                        1, instance->host_no);
> > +	if (!hostdata->work_q)
> > +		return -ENOMEM;
> > +
> >    /* The CHECK code seems to break the 53C400. Will check it later maybe */
> >    if (flags & FLAG_NCR53C400)
> >     hostdata->flags = FLAG_HAS_LAST_BYTE_SENT | flags;
> 
> Wouldn't it be better to use a normal (ie bound) workqueue here?

The polling algorithm I've used requires that the workqueue item is free 
to busy-wait and sleep. Perhaps a kthread_worker would be better?

> SCSI-2 is pretty much single-threaded, so shifting things onto arbitrary 
> CPUs don't sound very appealing.

Most of these drivers only run on UP systems. For the x86 drivers, I 
suspect that the cache miss penalty would be insignificant compared to 
some of the other overheads. The 5380 chip requires that the CPU is 
involved in SCSI bus signalling and merely accessing a chip register 
takes over a microsecond.

-- 

> 
> Cheers,
> 
> Hannes
> 

[PATCH v3 66/77] ncr5380: Fix soft lockups

[Finn Thain]

On Tue, 22 Dec 2015, One Thousand Gnomes wrote:

> On Tue, 22 Dec 2015 12:18:44 +1100 Finn Thain 
> <fthain@telegraphics.com.au> wrote:
> 
> > Because of the rudimentary design of the chip, it is necessary to poll 
> > the SCSI bus signals during PIO and this tends to hog the CPU. The 
> > driver will accept new commands while others execute, and this causes 
> > a soft lockup because the workqueue item will not terminate until the 
> > issue queue is emptied.
> > 
> > When exercising dmx3191d using sequential IO from dd, the driver is 
> > sent 512 KiB WRITE commands and 128 KiB READs. For a PIO transfer, the 
> > rate is is only about 300 KiB/s, so these are long-running commands. 
> > And although PDMA may run at several MiB/s, interrupts are disabled 
> > for the duration of the transfer.
> > 
> > Fix the unresponsiveness and soft lockup issues by calling 
> > cond_resched() after each command is completed and by limiting 
> > max_sectors for drivers that don't implement real DMA.
> 
> Is there a reason for not doing some limiting in the DMA case too. A 
> 512K write command even with DMA on a low end 68K box introduces a 
> second of latency before another I/O can be scheduled ?

The DMA case is the atari_scsi case. I'd like to think that atari_scsi 
would have only the latency issues that might be expected from any SCSI-2 
host adapter driver.

Unlike PDMA, interrupts are not disabled for these DMA transfers. Note 
that this patch isn't really relevant to DMA, because the main loop 
iterates only when done == 0, that is, !hostdata->dmalen.

-- 

> 
> Alan

[PATCH v3 20/77] ncr5380: Introduce unbound workqueue

[Hannes Reinecke]

On 12/22/2015 01:44 PM, Finn Thain wrote:
>
> On Tue, 22 Dec 2015, Hannes Reinecke wrote:
>
>> On 12/22/2015 02:17 AM, Finn Thain wrote:
>>> Allocate a work queue that will permit busy waiting and sleeping. This
>>> means NCR5380_init() can potentially fail, so add this error path.
>>>
>>> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
>>>
>>> ---
>>>
>>> In subsequent patches, the work function adopts this work queue so it
>>> can sleep while polling, which allows the removal of some flawed and
>>> complicated code in NCR5380_select() in NCR5380.c.
>>>
>>> Changed since v1:
>>> - Dropped WQ_CPU_INTENSIVE flag because Documentation/workqueue.txt says it
>>>     "is meaningless for unbound wq".
>>>
>>> ---
>>>    drivers/scsi/NCR5380.c       |   15 +++++++++++----
>>>    drivers/scsi/NCR5380.h       |    1 +
>>>    drivers/scsi/arm/cumana_1.c  |    8 ++++++--
>>>    drivers/scsi/arm/oak.c       |    8 ++++++--
>>>    drivers/scsi/atari_NCR5380.c |    8 +++++++-
>>>    drivers/scsi/atari_scsi.c    |    5 ++++-
>>>    drivers/scsi/dmx3191d.c      |   17 +++++++++++------
>>>    drivers/scsi/dtc.c           |   11 +++++++++--
>>>    drivers/scsi/g_NCR5380.c     |   31 +++++++++++++++----------------
>>>    drivers/scsi/mac_scsi.c      |    5 ++++-
>>>    drivers/scsi/pas16.c         |   10 ++++++++--
>>>    drivers/scsi/sun3_scsi.c     |    5 ++++-
>>>    drivers/scsi/t128.c          |   13 ++++++++++---
>>>    13 files changed, 96 insertions(+), 41 deletions(-)
>>>
>>> Index: linux/drivers/scsi/NCR5380.c
>>> ===================================================================
>>> --- linux.orig/drivers/scsi/NCR5380.c	2015-12-22 12:15:52.000000000 +1100
>>> +++ linux/drivers/scsi/NCR5380.c	2015-12-22 12:15:56.000000000 +1100
>>> @@ -514,7 +514,7 @@ static int should_disconnect(unsigned ch
>>>    static void NCR5380_set_timer(struct NCR5380_hostdata *hostdata, unsigned
>>>    long timeout)
>>>    {
>>>    	hostdata->time_expires = jiffies + timeout;
>>> -	schedule_delayed_work(&hostdata->coroutine, timeout);
>>> +	queue_delayed_work(hostdata->work_q, &hostdata->coroutine, timeout);
>>>    }
>>>
>>>
>>> @@ -791,7 +791,12 @@ static int NCR5380_init(struct Scsi_Host
>>>     hostdata->disconnected_queue = NULL;
>>>
>>>     INIT_DELAYED_WORK(&hostdata->coroutine, NCR5380_main);
>>> -	
>>> +	hostdata->work_q = alloc_workqueue("ncr5380_%d",
>>> +	                        WQ_UNBOUND | WQ_MEM_RECLAIM,
>>> +	                        1, instance->host_no);
>>> +	if (!hostdata->work_q)
>>> +		return -ENOMEM;
>>> +
>>>     /* The CHECK code seems to break the 53C400. Will check it later maybe */
>>>     if (flags & FLAG_NCR53C400)
>>>      hostdata->flags = FLAG_HAS_LAST_BYTE_SENT | flags;
>>
>> Wouldn't it be better to use a normal (ie bound) workqueue here?
>
> The polling algorithm I've used requires that the workqueue item is free
> to busy-wait and sleep. Perhaps a kthread_worker would be better?
>
>> SCSI-2 is pretty much single-threaded, so shifting things onto arbitrary
>> CPUs don't sound very appealing.
>
> Most of these drivers only run on UP systems. For the x86 drivers, I
> suspect that the cache miss penalty would be insignificant compared to
> some of the other overheads. The 5380 chip requires that the CPU is
> involved in SCSI bus signalling and merely accessing a chip register
> takes over a microsecond.
>
I know.
But using a bound workqueue would mean you could use 
'create_workqueue()' instead of open-coding it :-)

But in the end it's up to you. If the thing works I'm not that concerned.

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: J. Hawn, J. Guild, F. Imend?rffer, HRB 16746 (AG N?rnberg)

[PATCH v3 66/77] ncr5380: Fix soft lockups

[Michael Schmitz]

I'd like to think that, too - probably true for the Atari TT SCSI case
(can do scatter-gather, can do more than one command per LUN). Worse
for the Falcon SCSI which is the only one I can test (no
scatter-gather, one command per LUN, interrupt shared with IDE and IDE
driver locked out while SCSI command handled).

But that only affects balancing of I/O between IDE and SCSI drivers.
Is that what you are worried about, Alan?

Happy to test whether limiting max_sectors makes a difference in the DMA case.

Cheers,

  Michael



On Wed, Dec 23, 2015 at 2:47 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
>
> On Tue, 22 Dec 2015, One Thousand Gnomes wrote:
>
>> On Tue, 22 Dec 2015 12:18:44 +1100 Finn Thain
>> <fthain@telegraphics.com.au> wrote:
>>
>> > Because of the rudimentary design of the chip, it is necessary to poll
>> > the SCSI bus signals during PIO and this tends to hog the CPU. The
>> > driver will accept new commands while others execute, and this causes
>> > a soft lockup because the workqueue item will not terminate until the
>> > issue queue is emptied.
>> >
>> > When exercising dmx3191d using sequential IO from dd, the driver is
>> > sent 512 KiB WRITE commands and 128 KiB READs. For a PIO transfer, the
>> > rate is is only about 300 KiB/s, so these are long-running commands.
>> > And although PDMA may run at several MiB/s, interrupts are disabled
>> > for the duration of the transfer.
>> >
>> > Fix the unresponsiveness and soft lockup issues by calling
>> > cond_resched() after each command is completed and by limiting
>> > max_sectors for drivers that don't implement real DMA.
>>
>> Is there a reason for not doing some limiting in the DMA case too. A
>> 512K write command even with DMA on a low end 68K box introduces a
>> second of latency before another I/O can be scheduled ?
>
> The DMA case is the atari_scsi case. I'd like to think that atari_scsi
> would have only the latency issues that might be expected from any SCSI-2
> host adapter driver.
>
> Unlike PDMA, interrupts are not disabled for these DMA transfers. Note
> that this patch isn't really relevant to DMA, because the main loop
> iterates only when done == 0, that is, !hostdata->dmalen.
>
> --
>
>>
>> Alan
>