Re: [PATCH 13/24] scsi: Kill DRIVER_SENSE

[Finn Thain]

> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c 
> b/drivers/scsi/megaraid/megaraid_sas_base.c
> index c40fbea06cc5..649f9610ca72 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -1,3 +1,4 @@
> +
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   *  Linux MegaRAID driver for SAS based RAID controllers

Typo?

> index 59443e0184fd..d6ecb773c512 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -203,8 +203,8 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
>* If we have valid sense information, then some kind of recovery
>* must have taken place.  Make a note of this.
>*/
> - if (SCSI_SENSE_VALID(cmd))
> - cmd->result |= (DRIVER_SENSE << 24);
> + if (SCSI_SENSE_VALID(cmd) && status_byte(cmd->result) == SAM_STAT_GOOD)
> + set_status_byte(cmd, SAM_STAT_CHECK_CONDITION);

This means that a REQUEST SENSE command can never result in SAM_STAT_GOOD, 
right? Are there implications for higher layers?

-- 

Re: [PATCH 12/24] scsi: introduce scsi_build_sense()

[Finn Thain]

On Mon, 21 Oct 2019, Hannes Reinecke wrote:

> Introduce scsi_build_sense() as a wrapper around
> scsi_build_sense_buffer() to format the buffer and set
> the correct SCSI status.
> 
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/ata/libata-scsi.c |  7 ++--
>  drivers/s390/scsi/zfcp_scsi.c |  5 +--
>  drivers/scsi/3w-.c|  3 +-
>  drivers/scsi/libiscsi.c   |  5 +--
>  drivers/scsi/lpfc/lpfc_scsi.c | 30 -
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c  |  5 +--
>  drivers/scsi/mvumi.c  |  5 +--
>  drivers/scsi/myrb.c   | 61 
> ---
>  drivers/scsi/myrs.c   |  9 ++
>  drivers/scsi/ps3rom.c |  3 +-
>  drivers/scsi/qla2xxx/qla_isr.c| 15 ++---
>  drivers/scsi/scsi_debug.c | 11 +++
>  drivers/scsi/scsi_lib.c   | 18 +++
>  drivers/scsi/smartpqi/smartpqi_init.c |  3 +-
>  drivers/scsi/stex.c   |  5 +--
>  include/scsi/scsi_cmnd.h  |  3 ++
>  16 files changed, 60 insertions(+), 128 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index b197d2fbe3f8..0fd3cb8e4e49 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -342,9 +342,7 @@ void ata_scsi_set_sense(struct ata_device *dev, struct 
> scsi_cmnd *cmd,
>   if (!cmd)
>   return;
>  
> - cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
> -
> - scsi_build_sense_buffer(d_sense, cmd->sense_buffer, sk, asc, ascq);
> + scsi_build_sense(cmd, d_sense, sk, asc, ascq);
>  }
>  
>  void ata_scsi_set_sense_information(struct ata_device *dev,
> @@ -1092,8 +1090,7 @@ static void ata_gen_passthru_sense(struct 
> ata_queued_cmd *qc)
>* ATA PASS-THROUGH INFORMATION AVAILABLE
>* Always in descriptor format sense.
>*/
> - scsi_build_sense_buffer(1, cmd->sense_buffer,
> - RECOVERED_ERROR, 0, 0x1D);
> + scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
>   }
>  
>   if ((cmd->sense_buffer[0] & 0x7f) >= 0x72) {
> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
> index e9ded2befa0d..da52d7649f4d 100644
> --- a/drivers/s390/scsi/zfcp_scsi.c
> +++ b/drivers/s390/scsi/zfcp_scsi.c
> @@ -834,10 +834,7 @@ void zfcp_scsi_set_prot(struct zfcp_adapter *adapter)
>   */
>  void zfcp_scsi_dif_sense_error(struct scsi_cmnd *scmd, int ascq)
>  {
> - scsi_build_sense_buffer(1, scmd->sense_buffer,
> - ILLEGAL_REQUEST, 0x10, ascq);
> - set_driver_byte(scmd, DRIVER_SENSE);
> - scmd->result |= SAM_STAT_CHECK_CONDITION;
> + scsi_build_sense(scmd, 1, ILLEGAL_REQUEST, 0x10, ascq);
>   set_host_byte(scmd, DID_SOFT_ERROR);
>  }
>  
> diff --git a/drivers/scsi/3w-.c b/drivers/scsi/3w-.c
> index 79eca8f1fd05..381723634c13 100644
> --- a/drivers/scsi/3w-.c
> +++ b/drivers/scsi/3w-.c
> @@ -1981,8 +1981,7 @@ static int tw_scsi_queue_lck(struct scsi_cmnd *SCpnt, 
> void (*done)(struct scsi_c
>   printk(KERN_NOTICE "3w-: scsi%d: Unknown scsi 
> opcode: 0x%x\n", tw_dev->host->host_no, *command);
>   tw_dev->state[request_id] = TW_S_COMPLETED;
>   tw_state_request_finish(tw_dev, request_id);
> - SCpnt->result = (DRIVER_SENSE << 24) | 
> SAM_STAT_CHECK_CONDITION;
> - scsi_build_sense_buffer(1, SCpnt->sense_buffer, 
> ILLEGAL_REQUEST, 0x20, 0);
> + scsi_build_sense(SCpnt, 1, ILLEGAL_REQUEST, 0x20, 0);
>   done(SCpnt);
>   retval = 0;
>   }
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index ebd47c0cf9e9..9c85d7902faa 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -813,10 +813,7 @@ static void iscsi_scsi_cmd_rsp(struct iscsi_conn *conn, 
> struct iscsi_hdr *hdr,
>  
>   ascq = session->tt->check_protection(task, §or);
>   if (ascq) {
> - sc->result = DRIVER_SENSE << 24 |
> -  SAM_STAT_CHECK_CONDITION;
> - scsi_build_sense_buffer(1, sc->sense_buffer,
> - ILLEGAL_REQUEST, 0x10, ascq);
> + scsi_build_sense(sc, 1, ILLEGAL_REQUEST, 0x10, ascq);
>   scsi_set_sense_information(sc->sense_buffer,
>  SCSI_SENSE_BUFFERSIZE,
>  sector);
> diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
> index f06f63e58596..aa8431fe9c1f 100644
> --- a/drivers/scsi/lpfc/lpfc_scsi.c
> +++ b/drivers/scsi/lpfc/lpfc_scsi.c
> @@ -2843,10 +2843,7 @@ lpfc_calc_bg_err(struct lpfc_hba *phba, struc

Re: [PATCH RFC 00/24] scsi: Revamp result values

[Finn Thain]

On Mon, 21 Oct 2019, Douglas Gilbert wrote:

> 
> > As usual, comments and reviews are welcome.
> 
> It is hard to make an omelette without breaking some eggs.
> 

Coccinelle can minimize the breakage; particularly the 
straight-forward conversion of (FOO << 1) to SAM_STAT_BAR.

-- 

> Doug Gilbert
> 

Re: [PATCH 05/24] scsi: use standard SAM status codes

[Finn Thain]

On Mon, 21 Oct 2019, Hannes Reinecke wrote:

> Use standard SAM status codes and omit the explicit shift to convert
> to linus-specific ones.

"Linux-specific".

-- 

> 
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/ata/libata-scsi.c |  2 +-
>  drivers/infiniband/ulp/srp/ib_srp.c   |  2 +-
>  drivers/scsi/3w-9xxx.c|  5 +++--
>  drivers/scsi/3w-sas.c |  3 ++-
>  drivers/scsi/3w-.c|  4 ++--
>  drivers/scsi/arcmsr/arcmsr_hba.c  |  4 ++--
>  drivers/scsi/bfa/bfad_im.c|  2 +-
>  drivers/scsi/dc395x.c | 18 +-
>  drivers/scsi/dpt_i2o.c|  2 +-
>  drivers/scsi/gdth.c   | 12 ++--
>  drivers/scsi/megaraid.c   | 10 +-
>  drivers/scsi/megaraid/megaraid_mbox.c | 12 ++--
>  12 files changed, 35 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 76d0f9de767b..b197d2fbe3f8 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -856,7 +856,7 @@ static struct ata_queued_cmd *ata_scsi_qc_new(struct 
> ata_device *dev,
>   if (cmd->request->rq_flags & RQF_QUIET)
>   qc->flags |= ATA_QCFLAG_QUIET;
>   } else {
> - cmd->result = (DID_OK << 16) | (QUEUE_FULL << 1);
> + cmd->result = (DID_OK << 16) | SAM_STAT_TASK_SET_FULL;
>   cmd->scsi_done(cmd);
>   }
>  
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
> b/drivers/infiniband/ulp/srp/ib_srp.c
> index b5960351bec0..4570e3c79ea5 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -2404,7 +2404,7 @@ static int srp_queuecommand(struct Scsi_Host *shost, 
> struct scsi_cmnd *scmnd)
>* to reduce queue depth temporarily.
>*/
>   scmnd->result = len == -ENOMEM ?
> - DID_OK << 16 | QUEUE_FULL << 1 : DID_ERROR << 16;
> + DID_OK << 16 | SAM_STAT_TASK_SET_FULL : DID_ERROR << 16;
>   goto err_iu;
>   }
>  
> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
> index 3337b1e80412..ada77c136f8b 100644
> --- a/drivers/scsi/3w-9xxx.c
> +++ b/drivers/scsi/3w-9xxx.c
> @@ -1018,7 +1018,8 @@ static int twa_fill_sense(TW_Device_Extension *tw_dev, 
> int request_id, int copy_
>  
>   if (copy_sense) {
>   memcpy(tw_dev->srb[request_id]->sense_buffer, 
> full_command_packet->header.sense_data, TW_SENSE_DATA_LENGTH);
> - tw_dev->srb[request_id]->result = 
> (full_command_packet->command.newcommand.status << 1);
> + tw_dev->srb[request_id]->result =
> + full_command_packet->command.newcommand.status;
>   retval = TW_ISR_DONT_RESULT;
>   goto out;
>   }
> @@ -1342,7 +1343,7 @@ static irqreturn_t twa_interrupt(int irq, void 
> *dev_instance)
>   /* If error, command failed */
>   if (error == 1) {
>   /* Ask for a host reset */
> - cmd->result = (DID_OK << 16) | 
> (CHECK_CONDITION << 1);
> + cmd->result = (DID_OK << 16) | 
> SAM_STAT_CHECK_CONDITION;
>   }
>  
>   /* Report residual bytes for single sgl */
> diff --git a/drivers/scsi/3w-sas.c b/drivers/scsi/3w-sas.c
> index dda6fa857709..d11f62c60877 100644
> --- a/drivers/scsi/3w-sas.c
> +++ b/drivers/scsi/3w-sas.c
> @@ -891,7 +891,8 @@ static int twl_fill_sense(TW_Device_Extension *tw_dev, 
> int i, int request_id, in
>  
>   if (copy_sense) {
>   memcpy(tw_dev->srb[request_id]->sense_buffer, 
> header->sense_data, TW_SENSE_DATA_LENGTH);
> - tw_dev->srb[request_id]->result = 
> (full_command_packet->command.newcommand.status << 1);
> + tw_dev->srb[request_id]->result =
> + full_command_packet->command.newcommand.status;
>   goto out;
>   }
>  out:
> diff --git a/drivers/scsi/3w-.c b/drivers/scsi/3w-.c
> index 2b1e0d503020..79eca8f1fd05 100644
> --- a/drivers/scsi/3w-.c
> +++ b/drivers/scsi/3w-.c
> @@ -429,7 +429,7 @@ static int tw_decode_sense(TW_Device_Extension *tw_dev, 
> int request_id, int fill
>   /* Additional sense code qualifier */
>   
> tw_dev->srb[request_id]->sense_buffer[13] = tw_sense_table[i][3];
>  
> - tw_dev->srb[request_id]->result = 
> (DID_OK << 16) | (CHECK_CONDITION << 1);
> + tw_dev->srb[request_id]->result = 
> (DID_OK << 16) | SAM_STAT_CHECK_CONDITION;
>   return TW_ISR_DONT_RESULT; /* Special 
> case for isr to not over-write result */
>   

Re: [PATCH 03/24] wd33c93: use SCSI status

[Finn Thain]

On Mon, 21 Oct 2019, Hannes Reinecke wrote:

> Use standard SCSI status and drop usage of the linux-specific ones.
> 
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/wd33c93.c | 21 -
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/scsi/wd33c93.c b/drivers/scsi/wd33c93.c
> index f81046f0e68a..98e04a7b9d63 100644
> --- a/drivers/scsi/wd33c93.c
> +++ b/drivers/scsi/wd33c93.c
> @@ -1176,10 +1176,8 @@ wd33c93_intr(struct Scsi_Host *instance)
>   if (cmd->SCp.Status == ILLEGAL_STATUS_BYTE)
>   cmd->SCp.Status = lun;
>   if (cmd->cmnd[0] == REQUEST_SENSE
> - && cmd->SCp.Status != GOOD)
> - cmd->result =
> - (cmd->
> -  result & 0x00) | (DID_ERROR << 16);
> + && cmd->SCp.Status != SAM_STAT_GOOD)
> + set_host_byte(cmd, DID_ERROR);

This isn't obviously equivalent. Perhaps the set_host_byte() changes 
should be done in a separate patch to the SAM_STAT_FOO changes (?)

>   else
>   cmd->result =
>   cmd->SCp.Status | (cmd->SCp.Message << 8);
> @@ -1262,9 +1260,8 @@ wd33c93_intr(struct Scsi_Host *instance)
>   hostdata->connected = NULL;
>   hostdata->busy[cmd->device->id] &= ~(1 << (cmd->device->lun & 
> 0xff));
>   hostdata->state = S_UNCONNECTED;
> - if (cmd->cmnd[0] == REQUEST_SENSE && cmd->SCp.Status != GOOD)
> - cmd->result =
> - (cmd->result & 0x00) | (DID_ERROR << 16);
> + if (cmd->cmnd[0] == REQUEST_SENSE && cmd->SCp.Status != 
> SAM_STAT_GOOD)
> + set_host_byte(cmd, DID_ERROR);

Same.

>   else
>   cmd->result = cmd->SCp.Status | (cmd->SCp.Message << 8);
>   cmd->scsi_done(cmd);
> @@ -1294,12 +1291,10 @@ wd33c93_intr(struct Scsi_Host *instance)
>   hostdata->connected = NULL;
>   hostdata->busy[cmd->device->id] &= ~(1 << 
> (cmd->device->lun & 0xff));
>   hostdata->state = S_UNCONNECTED;
> - DB(DB_INTR, printk(":%d", cmd->SCp.Status))
> - if (cmd->cmnd[0] == REQUEST_SENSE
> - && cmd->SCp.Status != GOOD)
> - cmd->result =
> - (cmd->
> -  result & 0x00) | (DID_ERROR << 16);
> + DB(DB_INTR, printk(":%d", cmd->SCp.Status));
> + if (cmd->cmnd[0] == REQUEST_SENSE
> + && cmd->SCp.Status != SAM_STAT_GOOD)
> + set_host_byte(cmd->result, DID_ERROR);

Same.

-- 

>   else
>   cmd->result =
>   cmd->SCp.Status | (cmd->SCp.Message << 8);
> 

Re: [PATCH 10/24] scsi: introduce set_status_byte()

[Finn Thain]

On Mon, 21 Oct 2019, Hannes Reinecke wrote:

> To be in-line with the other set_XX_byte() functions.
> 
> Signed-off-by: Hannes Reinecke 
> ---
>  include/scsi/scsi_cmnd.h | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 91bd749a02f7..6932d91472d5 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -307,6 +307,11 @@ static inline struct scsi_data_buffer *scsi_prot(struct 
> scsi_cmnd *cmd)
>  #define scsi_for_each_prot_sg(cmd, sg, nseg, __i)\
>   for_each_sg(scsi_prot_sglist(cmd), sg, nseg, __i)
>  
> +static inline void set_status_byte(struct scsi_cmnd *cmd, char status)
> +{
> + cmd->result = (cmd->result & 0xff00) | status;

Is sign-extension desirable here? Do callers need it?

-- 

> +}
> +
>  static inline void set_msg_byte(struct scsi_cmnd *cmd, char status)
>  {
>   cmd->result = (cmd->result & 0x00ff) | (status << 8);
> 

[RFC] scsi: Avoid sign extension when setting command result bytes, was Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr

[Finn Thain]

On Fri, 18 Oct 2019, Martin K. Petersen wrote:

>
> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
> and most arcane code in SCSI)
>

A call to set_host_byte(cmd, x) or set_msg_byte(cmd, x) when x & 0x80 is 
set clobbers the most significant bytes in cmd->result.

Avoid this implicit sign extension when shifting bits by using an
unsigned formal parameter.

This is a theoretical bug, found by inspection, so I'm sending an untested 
RFC patch.

I didn't try to find possible callers that might pass a negative parameter
and neither did I check whether the clobber might be intentional...

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 76ed5e4acd38..ae814fa68bb8 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -306,17 +306,17 @@ static inline struct scsi_data_buffer *scsi_prot(struct 
scsi_cmnd *cmd)
 #define scsi_for_each_prot_sg(cmd, sg, nseg, __i)  \
for_each_sg(scsi_prot_sglist(cmd), sg, nseg, __i)
 
-static inline void set_msg_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_msg_byte(struct scsi_cmnd *cmd, unsigned char status)
 {
cmd->result = (cmd->result & 0x00ff) | (status << 8);
 }
 
-static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_host_byte(struct scsi_cmnd *cmd, unsigned char status)
 {
cmd->result = (cmd->result & 0xff00) | (status << 16);
 }
 
-static inline void set_driver_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_driver_byte(struct scsi_cmnd *cmd, unsigned char status)
 {
cmd->result = (cmd->result & 0x00ff) | (status << 24);
 }

Re: [PATCH v3] scsi: core: fix uninit-value access of variable sshdr

[Finn Thain]

On Sat, 12 Oct 2019, zhengbin wrote:

> kmsan report a warning in 5.1-rc4:
> 
> BUG: KMSAN: uninit-value in sr_get_events drivers/scsi/sr.c:207 [inline]
> BUG: KMSAN: uninit-value in sr_check_events+0x2cf/0x1090 drivers/scsi/sr.c:243
> CPU: 1 PID: 13858 Comm: syz-executor.0 Tainted: GB 5.1.0-rc4+ 
> #8
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> Ubuntu-1.8.2-1ubuntu1 04/01/2014
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x173/0x1d0 lib/dump_stack.c:113
>  kmsan_report+0x131/0x2a0 mm/kmsan/kmsan.c:619
>  __msan_warning+0x7a/0xf0 mm/kmsan/kmsan_instr.c:310
>  sr_get_events drivers/scsi/sr.c:207 [inline]
>  sr_check_events+0x2cf/0x1090 drivers/scsi/sr.c:243
> 
> The reason is as follows:
> sr_get_events
>   struct scsi_sense_hdr sshdr;  -->uninit
>   scsi_execute_req  -->If fail, will not set sshdr
>   scsi_sense_valid(&sshdr)  -->access sshdr
> 
> We can init sshdr in sr_get_events, but there have many callers of
> scsi_execute, scsi_execute_req, we have to troubleshoot all callers,
> the simpler way is init sshdr in __scsi_execute.
> 
> BTW: sr_do_ioctl should check whether sshdr is valid, fix this
> 
> Signed-off-by: zhengbin 
> ---
> v1->v2: modify the comments suggested by Bart
> v2->v3: fix bug in sr_do_ioctl
>  drivers/scsi/scsi_lib.c | 7 +++
>  drivers/scsi/sr_ioctl.c | 5 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 5447738..d5e29c5 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -255,6 +255,13 @@ int __scsi_execute(struct scsi_device *sdev, const 
> unsigned char *cmd,
>   struct scsi_request *rq;
>   int ret = DRIVER_ERROR << 24;
> 
> + /*
> +  * Zero-initialize sshdr for those callers that check the *sshdr
> +  * contents even if no sense data is available.
> +  */
> + if (sshdr)
> + memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
> +
>   req = blk_get_request(sdev->request_queue,
>   data_direction == DMA_TO_DEVICE ?
>   REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);

Does this have any effect? It appears __scsi_execute() calls 
scsi_normalize_sense(), which already does
memset(sshdr, 0, sizeof(struct scsi_sense_hdr)).

-- 

> diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
> index ffcf902..335cfdd 100644
> --- a/drivers/scsi/sr_ioctl.c
> +++ b/drivers/scsi/sr_ioctl.c
> @@ -206,6 +206,11 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
> 
>   /* Minimal error checking.  Ignore cases we know about, and report the 
> rest. */
>   if (driver_byte(result) != 0) {
> + if (!scsi_sense_valid(sshdr)) {
> + err = -EIO;
> + goto out;
> + }
> +
>   switch (sshdr->sense_key) {
>   case UNIT_ATTENTION:
>   SDev->changed = 1;
> --
> 2.7.4
> 
> 

Re: [PATCH V2] scsi: save/restore command resid for error handling

[Finn Thain]

On Sat, 28 Sep 2019, Damien Le Moal wrote:

> When a non-passthrough command is terminated with CHECK CONDITION,
> request sense is executed by hijacking the command descriptor. Since
> scsi_eh_prep_cmnd() and scsi_eh_restore_cmnd() do not save/restore the
> original command resid, the value returned on failure of the original
> command is lost and replaced with the value set by the execution of the
> request sense command. This value may in many instances be unaligned to
> the device sector size, causing sd_done() to print a warning message
> about the incorrect unaligned resid before the command is retried or
> aborted.
> 
> Fix this problem by saving the original command resid in struct
> scsi_eh_save using scsi_eh_prep_cmnd() and restoring it in
> scsi_eh_restore_cmnd(). In addition, to make sure that the request sense
> command is executed with a correctly initialized command structure, also
> reset resid to 0 in scsi_eh_prep_cmnd() after saving the original
> command resid value in struct scsi_eh_save.
> 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Damien Le Moal 
> ---
> 
> Changes from V1:
> * Dropped patch 2
> * Add resid reset in scsi_eh_prep_cmnd()
> 
>  drivers/scsi/scsi_error.c | 3 +++
>  include/scsi/scsi_eh.h| 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 1c470e31ae81..f53828bf7ad7 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -967,6 +967,7 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct 
> scsi_eh_save *ses,
>   ses->data_direction = scmd->sc_data_direction;
>   ses->sdb = scmd->sdb;
>   ses->result = scmd->result;
> + ses->resid = scsi_get_resid(scmd);
>   ses->underflow = scmd->underflow;
>   ses->prot_op = scmd->prot_op;
>   ses->eh_eflags = scmd->eh_eflags;
> @@ -977,6 +978,7 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct 
> scsi_eh_save *ses,
>   memset(scmd->cmnd, 0, BLK_MAX_CDB);
>   memset(&scmd->sdb, 0, sizeof(scmd->sdb));
>   scmd->result = 0;
> + scsi_set_resid(scmd, 0);
>  
>   if (sense_bytes) {
>   scmd->sdb.length = min_t(unsigned, SCSI_SENSE_BUFFERSIZE,
> @@ -1029,6 +1031,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, 
> struct scsi_eh_save *ses)
>   scmd->sc_data_direction = ses->data_direction;
>   scmd->sdb = ses->sdb;
>   scmd->result = ses->result;
> + scsi_set_resid(scmd, ses->resid);

When saving and restoring state, perhaps it makes more sense to bypass the 
higher level getter/setter API? Open-coded assignment statements are 
already prevalent here, rather than calls to e.g. scsi_set_prot_op(), 
set_msg_byte() etc. (There may be no code elsewhere that could tell the 
difference, but we can't use "private" members to prove it, unlike C++.)

>   scmd->underflow = ses->underflow;
>   scmd->prot_op = ses->prot_op;
>   scmd->eh_eflags = ses->eh_eflags;
> diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
> index 3810b340551c..9caa9b262a32 100644
> --- a/include/scsi/scsi_eh.h
> +++ b/include/scsi/scsi_eh.h
> @@ -32,6 +32,7 @@ extern int scsi_ioctl_reset(struct scsi_device *, int 
> __user *);
>  struct scsi_eh_save {
>   /* saved state */
>   int result;
> + unsigned int resid;

There seems to be an inconsistency here. A signed int would be consistent 
with the getter and setter helpers. Whereas, if you open-coded the 
assignments instead, your unsigned int would make sense because 
scsi_request.resid_len really is an unsigned int.

-- 

>   int eh_eflags;
>   enum dma_data_direction data_direction;
>   unsigned underflow;
> 

Re: [PATCH 0/2] Fix SCSI & USB Storage CHECK CONDITION handling

[Finn Thain]

[snipped selective measurements of word usage]

On Fri, 27 Sep 2019, Alan Stern wrote:

> 
> So I guess this was never defined precisely.
> 

The O.E.D. defines these terms:

residual
a. [...]
n. 1. a quantity remaining after other things have been subtracted 
  or allowed for. [...]
   2. [...]
   3. [...]

and

residue
n. a small amount of something that remains after the main part 
   has gone or been taken or used.


So, writing "residue" could be misleading as the connotation is that the 
remaining part is the lesser part.

I think that's probably why the term "residual" gets used in finance (and 
in SCSI transfers).

-- 

Re: [PATCH V5 10/16] s390: zfcp_fc: use sg helper to operate scatterlist

[Finn Thain]

On Tue, 25 Jun 2019, Ming Lei wrote:

> On Tue, Jun 25, 2019 at 12:01:24PM +1000, Finn Thain wrote:
> > > diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
> > > index dccdb41bed8c..c7129f5234f0 100644
> > > --- a/drivers/s390/scsi/zfcp_dbf.c
> > > +++ b/drivers/s390/scsi/zfcp_dbf.c
> > > @@ -552,7 +552,7 @@ static u16 zfcp_dbf_san_res_cap_len_if_gpn_ft(char 
> > > *tag,
> > >   if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
> > >   acc++;
> > >   else
> > > - acc = sg_virt(++resp_entry);
> > > + acc = sg_virt(resp_entry = sg_next(resp_entry));
> > 
> > Shouldn't that be,
> > 
> > acc = sg_virt(sg_next(resp_entry));
> 
> resp_entry needs to be updated.
> 

Right, sorry for the noise.

-- 

> > 
> > >  
> > >   last = acc->fp_flags & FC_NS_FID_LAST;
> > >   }
> > > diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
> > > index b018b61bd168..5048812ce660 100644
> > > --- a/drivers/s390/scsi/zfcp_fc.c
> > > +++ b/drivers/s390/scsi/zfcp_fc.c
> > > @@ -742,7 +742,7 @@ static int zfcp_fc_eval_gpn_ft(struct zfcp_fc_req 
> > > *fc_req,
> > >   if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
> > >   acc++;
> > >   else
> > > - acc = sg_virt(++sg);
> > > + acc = sg_virt(sg = sg_next(sg));
> > 
> > and
> > 
> > acc = sg_virt(sg_next(sg));
> > 
> > ?
> 
> Same with above.
> 
> Thanks,
> Ming
> 

Re: [PATCH V5 10/16] s390: zfcp_fc: use sg helper to operate scatterlist

[Finn Thain]

> diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c
> index dccdb41bed8c..c7129f5234f0 100644
> --- a/drivers/s390/scsi/zfcp_dbf.c
> +++ b/drivers/s390/scsi/zfcp_dbf.c
> @@ -552,7 +552,7 @@ static u16 zfcp_dbf_san_res_cap_len_if_gpn_ft(char *tag,
>   if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
>   acc++;
>   else
> - acc = sg_virt(++resp_entry);
> + acc = sg_virt(resp_entry = sg_next(resp_entry));

Shouldn't that be,

acc = sg_virt(sg_next(resp_entry));

>  
>   last = acc->fp_flags & FC_NS_FID_LAST;
>   }
> diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
> index b018b61bd168..5048812ce660 100644
> --- a/drivers/s390/scsi/zfcp_fc.c
> +++ b/drivers/s390/scsi/zfcp_fc.c
> @@ -742,7 +742,7 @@ static int zfcp_fc_eval_gpn_ft(struct zfcp_fc_req *fc_req,
>   if (x % (ZFCP_FC_GPN_FT_ENT_PAGE + 1))
>   acc++;
>   else
> - acc = sg_virt(++sg);
> + acc = sg_virt(sg = sg_next(sg));

and

acc = sg_virt(sg_next(sg));

?

-- 

>  
>   last = acc->fp_flags & FC_NS_FID_LAST;
>   d_id = ntoh24(acc->fp_fid);
> -- 
> 2.20.1
> 
> 
> Thanks,
> Ming
> 

Re: [PATCH V5 11/16] scsi: aha152x: use sg helper to operate scatterlist

[Finn Thain]

On Tue, 18 Jun 2019, Ming Lei wrote:

> From: Finn Thain 
> 
> Use the scatterlist iterators and remove direct indexing of the
> scatterlist array.
> 
> This way allows us to pre-allocate one small scatterlist, which can be
> chained with one runtime allocated scatterlist if the pre-allocated one
> isn't enough for the whole request.
> 
> Finn added the change to replace SCp.buffers_residual with sg_is_last()
> for fixing updating it, and the similar change has been applied on
> NCR5380.c
> 
> Cc: Finn Thain 
> Signed-off-by: Ming Lei 

Signed-off-by: Finn Thain 

> ---
>  drivers/scsi/aha152x.c | 46 +-
>  1 file changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
> index 97872838b983..f07f3fa9b58d 100644
> --- a/drivers/scsi/aha152x.c
> +++ b/drivers/scsi/aha152x.c
> @@ -948,7 +948,6 @@ static int aha152x_internal_queue(struct scsi_cmnd *SCpnt,
>  SCp.ptr  : buffer pointer
>  SCp.this_residual: buffer length
>  SCp.buffer   : next buffer
> -SCp.buffers_residual : left buffers in list
>  SCp.phase: current state of the command */
>  
>   if ((phase & resetting) || !scsi_sglist(SCpnt)) {
> @@ -956,13 +955,11 @@ static int aha152x_internal_queue(struct scsi_cmnd 
> *SCpnt,
>   SCpnt->SCp.this_residual = 0;
>   scsi_set_resid(SCpnt, 0);
>   SCpnt->SCp.buffer   = NULL;
> - SCpnt->SCp.buffers_residual = 0;
>   } else {
>   scsi_set_resid(SCpnt, scsi_bufflen(SCpnt));
>   SCpnt->SCp.buffer   = scsi_sglist(SCpnt);
>   SCpnt->SCp.ptr  = SG_ADDRESS(SCpnt->SCp.buffer);
>   SCpnt->SCp.this_residual= SCpnt->SCp.buffer->length;
> - SCpnt->SCp.buffers_residual = scsi_sg_count(SCpnt) - 1;
>   }
>  
>   DO_LOCK(flags);
> @@ -2030,10 +2027,9 @@ static void datai_run(struct Scsi_Host *shpnt)
>   }
>  
>   if (CURRENT_SC->SCp.this_residual == 0 &&
> - CURRENT_SC->SCp.buffers_residual > 0) {
> + !sg_is_last(CURRENT_SC->SCp.buffer)) {
>   /* advance to next buffer */
> - CURRENT_SC->SCp.buffers_residual--;
> - CURRENT_SC->SCp.buffer++;
> + CURRENT_SC->SCp.buffer = 
> sg_next(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.ptr   = 
> SG_ADDRESS(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.this_residual = 
> CURRENT_SC->SCp.buffer->length;
>   }
> @@ -2136,10 +2132,10 @@ static void datao_run(struct Scsi_Host *shpnt)
>   CMD_INC_RESID(CURRENT_SC, -2 * data_count);
>   }
>  
> - if(CURRENT_SC->SCp.this_residual==0 && 
> CURRENT_SC->SCp.buffers_residual>0) {
> + if (CURRENT_SC->SCp.this_residual == 0 &&
> + !sg_is_last(CURRENT_SC->SCp.buffer)) {
>   /* advance to next buffer */
> - CURRENT_SC->SCp.buffers_residual--;
> - CURRENT_SC->SCp.buffer++;
> + CURRENT_SC->SCp.buffer = 
> sg_next(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.ptr   = 
> SG_ADDRESS(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.this_residual = 
> CURRENT_SC->SCp.buffer->length;
>   }
> @@ -2158,22 +2154,26 @@ static void datao_run(struct Scsi_Host *shpnt)
>  static void datao_end(struct Scsi_Host *shpnt)
>  {
>   if(TESTLO(DMASTAT, DFIFOEMP)) {
> - int data_count = (DATA_LEN - scsi_get_resid(CURRENT_SC)) -
> - GETSTCNT();
> + u32 datao_cnt = GETSTCNT();
> + int datao_out = DATA_LEN - scsi_get_resid(CURRENT_SC);
> + int done;
> + struct scatterlist *sg = scsi_sglist(CURRENT_SC);
>  
> - CMD_INC_RESID(CURRENT_SC, data_count);
> + CMD_INC_RESID(CURRENT_SC, datao_out - datao_cnt);
>  
> - data_count -= CURRENT_SC->SCp.ptr -
> - SG_ADDRESS(CURRENT_SC->SCp.buffer);
> - while(data_count>0) {
> - CURRENT_SC->SCp.buffer--;
> - CURRENT_SC->

Re: [PATCH V4 11/16] scsi: aha152x: use sg helper to operate scatterlist

[Finn Thain]

On Mon, 17 Jun 2019, Finn Thain wrote:

> On Mon, 17 Jun 2019, Ming Lei wrote:
> 
> > Use the scatterlist iterators and remove direct indexing of the
> > scatterlist array.
> > 
> > This way allows us to pre-allocate one small scatterlist, which can be
> > chained with one runtime allocated scatterlist if the pre-allocated one
> > isn't enough for the whole request.
> > 
> > Finn added the change to replace SCp.buffers_residual with sg_is_last()
> > for fixing updating it, and the similar change has been applied on
> > NCR5380.c
> > 
> > Cc: Finn Thain 
> > Signed-off-by: Ming Lei 
> 
> Reviewed-by: Finn Thain 
> 

I have to retract that. I think this patch is still wrong.

GETSTCNT() appears to be the number of bytes sent since datao_init() and 
not the number of bytes sent since the start of the command. (Note the 
CLRSTCNT in datao_init() which appears to clear the transfer counter.) I 
don't see how the existing datao_end() could work otherwise. (Juergen?)

So here's another attempt. Ming, I'd be happy to take the blame/credit (in 
the form of a From tag etc.) in case you don't want to spend more time on 
this.

diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index 97872838b983..f07f3fa9b58d 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -948,7 +948,6 @@ static int aha152x_internal_queue(struct scsi_cmnd *SCpnt,
   SCp.ptr  : buffer pointer
   SCp.this_residual: buffer length
   SCp.buffer   : next buffer
-  SCp.buffers_residual : left buffers in list
   SCp.phase: current state of the command */
 
if ((phase & resetting) || !scsi_sglist(SCpnt)) {
@@ -956,13 +955,11 @@ static int aha152x_internal_queue(struct scsi_cmnd *SCpnt,
SCpnt->SCp.this_residual = 0;
scsi_set_resid(SCpnt, 0);
SCpnt->SCp.buffer   = NULL;
-   SCpnt->SCp.buffers_residual = 0;
} else {
scsi_set_resid(SCpnt, scsi_bufflen(SCpnt));
SCpnt->SCp.buffer   = scsi_sglist(SCpnt);
SCpnt->SCp.ptr  = SG_ADDRESS(SCpnt->SCp.buffer);
SCpnt->SCp.this_residual= SCpnt->SCp.buffer->length;
-   SCpnt->SCp.buffers_residual = scsi_sg_count(SCpnt) - 1;
}
 
DO_LOCK(flags);
@@ -2030,10 +2027,9 @@ static void datai_run(struct Scsi_Host *shpnt)
}
 
if (CURRENT_SC->SCp.this_residual == 0 &&
-   CURRENT_SC->SCp.buffers_residual > 0) {
+   !sg_is_last(CURRENT_SC->SCp.buffer)) {
/* advance to next buffer */
-   CURRENT_SC->SCp.buffers_residual--;
-   CURRENT_SC->SCp.buffer++;
+   CURRENT_SC->SCp.buffer = 
sg_next(CURRENT_SC->SCp.buffer);
CURRENT_SC->SCp.ptr   = 
SG_ADDRESS(CURRENT_SC->SCp.buffer);
CURRENT_SC->SCp.this_residual = 
CURRENT_SC->SCp.buffer->length;
}
@@ -2136,10 +2132,10 @@ static void datao_run(struct Scsi_Host *shpnt)
CMD_INC_RESID(CURRENT_SC, -2 * data_count);
}
 
-   if(CURRENT_SC->SCp.this_residual==0 && 
CURRENT_SC->SCp.buffers_residual>0) {
+   if (CURRENT_SC->SCp.this_residual == 0 &&
+   !sg_is_last(CURRENT_SC->SCp.buffer)) {
/* advance to next buffer */
-   CURRENT_SC->SCp.buffers_residual--;
-   CURRENT_SC->SCp.buffer++;
+   CURRENT_SC->SCp.buffer = 
sg_next(CURRENT_SC->SCp.buffer);
CURRENT_SC->SCp.ptr   = 
SG_ADDRESS(CURRENT_SC->SCp.buffer);
CURRENT_SC->SCp.this_residual = 
CURRENT_SC->SCp.buffer->length;
}
@@ -2158,22 +2154,26 @@ static void datao_run(struct Scsi_Host *shpnt)
 static void datao_end(struct Scsi_Host *shpnt)
 {
if(TESTLO(DMASTAT, DFIFOEMP)) {
-   int data_count = (DATA_LEN - scsi_get_resid(CURRENT_SC)) -
-   GETSTCNT();
+   u32 datao_cnt = GETSTCNT();
+   int datao_out = DATA_LEN - scsi_get_resid(CURRENT_SC);
+   int done;
+   struct scatterlist *sg = scsi_sglist(CURRENT_SC);
 
-   CMD_INC_RESID(CURRENT_SC, data_count);
+   CMD_INC_RESID(CURRENT_SC, datao_out - datao_cnt);
 
-   data_count -= CURRENT_SC->SCp.ptr -
-   

Re: [PATCH V4 11/16] scsi: aha152x: use sg helper to operate scatterlist

[Finn Thain]

On Mon, 17 Jun 2019, Ming Lei wrote:

> Use the scatterlist iterators and remove direct indexing of the
> scatterlist array.
> 
> This way allows us to pre-allocate one small scatterlist, which can be
> chained with one runtime allocated scatterlist if the pre-allocated one
> isn't enough for the whole request.
> 
> Finn added the change to replace SCp.buffers_residual with sg_is_last()
> for fixing updating it, and the similar change has been applied on
> NCR5380.c
> 
> Cc: Finn Thain 
> Signed-off-by: Ming Lei 

Reviewed-by: Finn Thain 

-- 

> ---
>  drivers/scsi/aha152x.c | 42 --
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
> index 97872838b983..6d0518f616cb 100644
> --- a/drivers/scsi/aha152x.c
> +++ b/drivers/scsi/aha152x.c
> @@ -948,7 +948,6 @@ static int aha152x_internal_queue(struct scsi_cmnd *SCpnt,
>  SCp.ptr  : buffer pointer
>  SCp.this_residual: buffer length
>  SCp.buffer   : next buffer
> -SCp.buffers_residual : left buffers in list
>  SCp.phase: current state of the command */
>  
>   if ((phase & resetting) || !scsi_sglist(SCpnt)) {
> @@ -956,13 +955,11 @@ static int aha152x_internal_queue(struct scsi_cmnd 
> *SCpnt,
>   SCpnt->SCp.this_residual = 0;
>   scsi_set_resid(SCpnt, 0);
>   SCpnt->SCp.buffer   = NULL;
> - SCpnt->SCp.buffers_residual = 0;
>   } else {
>   scsi_set_resid(SCpnt, scsi_bufflen(SCpnt));
>   SCpnt->SCp.buffer   = scsi_sglist(SCpnt);
>   SCpnt->SCp.ptr  = SG_ADDRESS(SCpnt->SCp.buffer);
>   SCpnt->SCp.this_residual= SCpnt->SCp.buffer->length;
> - SCpnt->SCp.buffers_residual = scsi_sg_count(SCpnt) - 1;
>   }
>  
>   DO_LOCK(flags);
> @@ -2030,10 +2027,9 @@ static void datai_run(struct Scsi_Host *shpnt)
>   }
>  
>   if (CURRENT_SC->SCp.this_residual == 0 &&
> - CURRENT_SC->SCp.buffers_residual > 0) {
> + !sg_is_last(CURRENT_SC->SCp.buffer)) {
>   /* advance to next buffer */
> - CURRENT_SC->SCp.buffers_residual--;
> - CURRENT_SC->SCp.buffer++;
> + CURRENT_SC->SCp.buffer = 
> sg_next(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.ptr   = 
> SG_ADDRESS(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.this_residual = 
> CURRENT_SC->SCp.buffer->length;
>   }
> @@ -2136,10 +2132,10 @@ static void datao_run(struct Scsi_Host *shpnt)
>   CMD_INC_RESID(CURRENT_SC, -2 * data_count);
>   }
>  
> - if(CURRENT_SC->SCp.this_residual==0 && 
> CURRENT_SC->SCp.buffers_residual>0) {
> + if(CURRENT_SC->SCp.this_residual==0 &&
> +!sg_is_last(CURRENT_SC->SCp.buffer)) {
>   /* advance to next buffer */
> - CURRENT_SC->SCp.buffers_residual--;
> - CURRENT_SC->SCp.buffer++;
> + CURRENT_SC->SCp.buffer = 
> sg_next(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.ptr   = 
> SG_ADDRESS(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.this_residual = 
> CURRENT_SC->SCp.buffer->length;
>   }
> @@ -2158,22 +2154,24 @@ static void datao_run(struct Scsi_Host *shpnt)
>  static void datao_end(struct Scsi_Host *shpnt)
>  {
>   if(TESTLO(DMASTAT, DFIFOEMP)) {
> - int data_count = (DATA_LEN - scsi_get_resid(CURRENT_SC)) -
> - GETSTCNT();
> + int done = GETSTCNT();
> + int data_count = (DATA_LEN - scsi_get_resid(CURRENT_SC)) - done;
> + struct scatterlist *sg = scsi_sglist(CURRENT_SC);
>  
>   CMD_INC_RESID(CURRENT_SC, data_count);
>  
> - data_count -= CURRENT_SC->SCp.ptr -
> - SG_ADDRESS(CURRENT_SC->SCp.buffer);
> - while(data_count>0) {
> - CURRENT_SC->SCp.buffer--;
> - CURRENT_SC->SCp.buffers_residual++;
> - data_count -= CURRENT_SC->SCp.buffer->length;
> + /*

Re: [PATCH V3 10/15] scsi: aha152x: use sg helper to operate scatterlist

[Finn Thain]

On Fri, 14 Jun 2019, Ming Lei wrote:

> 
> Follows the fixed version, could you review again?
> 
> From f03484d4bac083c39d70665cfbadb641093b63de Mon Sep 17 00:00:00 2001
> From: Ming Lei 
> Date: Wed, 12 Jun 2019 20:37:35 +0800
> Subject: [PATCH] scsi: aha152x: use sg helper to operate scatterlist
> 
> Use the scatterlist iterators and remove direct indexing of the
> scatterlist array.
> 
> This way allows us to pre-allocate one small scatterlist, which can be
> chained with one runtime allocated scatterlist if the pre-allocated one
> isn't enough for the whole request.
> 
> Signed-off-by: Ming Lei 
> ---
>  drivers/scsi/aha152x.c | 34 --
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
> index 97872838b983..7faecdefda56 100644
> --- a/drivers/scsi/aha152x.c
> +++ b/drivers/scsi/aha152x.c
> @@ -2033,7 +2033,7 @@ static void datai_run(struct Scsi_Host *shpnt)
>   CURRENT_SC->SCp.buffers_residual > 0) {
>   /* advance to next buffer */
>   CURRENT_SC->SCp.buffers_residual--;
> - CURRENT_SC->SCp.buffer++;
> + CURRENT_SC->SCp.buffer = 
> sg_next(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.ptr   = 
> SG_ADDRESS(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.this_residual = 
> CURRENT_SC->SCp.buffer->length;
>   }
> @@ -2139,7 +2139,7 @@ static void datao_run(struct Scsi_Host *shpnt)
>   if(CURRENT_SC->SCp.this_residual==0 && 
> CURRENT_SC->SCp.buffers_residual>0) {
>   /* advance to next buffer */
>   CURRENT_SC->SCp.buffers_residual--;
> - CURRENT_SC->SCp.buffer++;
> + CURRENT_SC->SCp.buffer = 
> sg_next(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.ptr   = 
> SG_ADDRESS(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.this_residual = 
> CURRENT_SC->SCp.buffer->length;
>   }
> @@ -2158,22 +2158,28 @@ static void datao_run(struct Scsi_Host *shpnt)
>  static void datao_end(struct Scsi_Host *shpnt)
>  {
>   if(TESTLO(DMASTAT, DFIFOEMP)) {
> - int data_count = (DATA_LEN - scsi_get_resid(CURRENT_SC)) -
> - GETSTCNT();
> + int done = GETSTCNT();
> + int data_count = (DATA_LEN - scsi_get_resid(CURRENT_SC)) - done;

I think that's better than my suggestion.

> + struct scatterlist *sg = scsi_sglist(CURRENT_SC);
> + int i;
>  
>   CMD_INC_RESID(CURRENT_SC, data_count);
>  
> - data_count -= CURRENT_SC->SCp.ptr -
> - SG_ADDRESS(CURRENT_SC->SCp.buffer);
> - while(data_count>0) {
> - CURRENT_SC->SCp.buffer--;
> - CURRENT_SC->SCp.buffers_residual++;
> - data_count -= CURRENT_SC->SCp.buffer->length;
> + /*
> +  * rewind where we have done, and we have to start from
> +  * the beginning
> +  */

How about, "Locate the first SG entry not yet sent".

We could use sg_nents_for_len() but it returns a count of sg entries not a 
scatterlist pointer so it's not very helpful here.

> + for (i = 0; done > 0 && !sg_is_last(sg); i++, sg = sg_next(sg)) 
> {
> + if (done < sg->length)
> + break;
> + done -= sg->length;
>   }
> - CURRENT_SC->SCp.ptr = SG_ADDRESS(CURRENT_SC->SCp.buffer) -
> - data_count;
> - CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length +
> - data_count;
> +
> + CURRENT_SC->SCp.buffers_residual = i;

Contradicting my previous email, that's still not right. I think it would 
have to be,

CURRENT_SC->SCp.buffers_residual = scsi_sg_count(CURRENT_SC) - 
i;

But we could remove all references to SCp.buffers_residual, like I did in 
patch 15/15 for NCR5380.c.

> + CURRENT_SC->SCp.buffer = sg;
> + CURRENT_SC->SCp.ptr = SG_ADDRESS(CURRENT_SC->SCp.buffer) + done;
> + CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length -
> + done;
>   }
>  
>   SETPORT(SXFRCTL0, CH1|CLRCH1|CLRSTCNT);
> 

What do you think of the revised patch below?

diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index 97872838b983..6d0518f616cb 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -948,7 +948,6 @@ static int aha152x_internal_queue(struct scsi_cmnd *SCpnt,
   SCp.ptr  : buffer pointer
   SCp.this_residual: buffer length
   SCp.buffer   : n

Re: [PATCH V3 07/15] usb: image: microtek: use sg helper to operate scatterlist

[Finn Thain]

On Fri, 14 Jun 2019, Ming Lei wrote:

> Use the scatterlist iterators and remove direct indexing of the
> scatterlist array.
> 
> This way allows us to pre-allocate one small scatterlist, which can be
> chained with one runtime allocated scatterlist if the pre-allocated one
> isn't enough for the whole request.
> 
> Cc: Oliver Neukum 
> Cc: Greg Kroah-Hartman 
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Ming Lei 
> ---
>  drivers/usb/image/microtek.c | 20 
>  drivers/usb/image/microtek.h |  2 +-
>  2 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/image/microtek.c b/drivers/usb/image/microtek.c
> index 607be1f4fe27..0a57c2cc8e5a 100644
> --- a/drivers/usb/image/microtek.c
> +++ b/drivers/usb/image/microtek.c
> @@ -488,7 +488,6 @@ static void mts_command_done( struct urb *transfer )
>  
>  static void mts_do_sg (struct urb* transfer)
>  {
> - struct scatterlist * sg;
>   int status = transfer->status;
>   MTS_INT_INIT();
>  
> @@ -500,13 +499,12 @@ static void mts_do_sg (struct urb* transfer)
>   mts_transfer_cleanup(transfer);
>  }
>  
> - sg = scsi_sglist(context->srb);
> - context->fragment++;
> + context->curr_sg = sg_next(context->curr_sg);
>   mts_int_submit_urb(transfer,
>  context->data_pipe,
> -sg_virt(&sg[context->fragment]),
> -sg[context->fragment].length,
> -context->fragment + 1 == scsi_sg_count(context->srb) 
> ?
> +sg_virt(context->curr_sg),
> +context->curr_sg->length,
> +sg_is_last(context->curr_sg) ?
>  mts_data_done : mts_do_sg);
>  }
>  
> @@ -526,22 +524,20 @@ static void
>  mts_build_transfer_context(struct scsi_cmnd *srb, struct mts_desc* desc)
>  {
>   int pipe;
> - struct scatterlist * sg;
> - 
> +
>   MTS_DEBUG_GOT_HERE();
>  
>   desc->context.instance = desc;
>   desc->context.srb = srb;
> - desc->context.fragment = 0;
>  
>   if (!scsi_bufflen(srb)) {
>   desc->context.data = NULL;
>   desc->context.data_length = 0;
>   return;
>   } else {
> - sg = scsi_sglist(srb);
> - desc->context.data = sg_virt(&sg[0]);
> - desc->context.data_length = sg[0].length;
> + desc->context.curr_sg = scsi_sglist(srb);
> + desc->context.data = sg_virt(desc->context.curr_sg);
> + desc->context.data_length = desc->context.curr_sg->length;
>   }
>  

Would it not be better to initialize desc->context.curr_sg in both 
branches of this conditional?

-- 

>  
> diff --git a/drivers/usb/image/microtek.h b/drivers/usb/image/microtek.h
> index 66685e59241a..7bd5f4639c4a 100644
> --- a/drivers/usb/image/microtek.h
> +++ b/drivers/usb/image/microtek.h
> @@ -21,7 +21,7 @@ struct mts_transfer_context
>   void *data;
>   unsigned data_length;
>   int data_pipe;
> - int fragment;
> + struct scatterlist *curr_sg;
>  
>   u8 *scsi_status; /* status returned from ep_response after command 
> completion */
>  };
> 

Re: [PATCH V3 10/15] scsi: aha152x: use sg helper to operate scatterlist

[Finn Thain]

Hi Ming,

On Fri, 14 Jun 2019, Ming Lei wrote:

> Use the scatterlist iterators and remove direct indexing of the
> scatterlist array.
> 
> This way allows us to pre-allocate one small scatterlist, which can be
> chained with one runtime allocated scatterlist if the pre-allocated one
> isn't enough for the whole request.
> 
> Signed-off-by: Ming Lei 
> ---
>  drivers/scsi/aha152x.c | 29 +++--
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
> index 97872838b983..bc9d12aa7880 100644
> --- a/drivers/scsi/aha152x.c
> +++ b/drivers/scsi/aha152x.c
> @@ -2033,7 +2033,7 @@ static void datai_run(struct Scsi_Host *shpnt)
>   CURRENT_SC->SCp.buffers_residual > 0) {
>   /* advance to next buffer */
>   CURRENT_SC->SCp.buffers_residual--;
> - CURRENT_SC->SCp.buffer++;
> + CURRENT_SC->SCp.buffer = 
> sg_next(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.ptr   = 
> SG_ADDRESS(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.this_residual = 
> CURRENT_SC->SCp.buffer->length;
>   }
> @@ -2139,7 +2139,7 @@ static void datao_run(struct Scsi_Host *shpnt)
>   if(CURRENT_SC->SCp.this_residual==0 && 
> CURRENT_SC->SCp.buffers_residual>0) {
>   /* advance to next buffer */
>   CURRENT_SC->SCp.buffers_residual--;
> - CURRENT_SC->SCp.buffer++;
> + CURRENT_SC->SCp.buffer = 
> sg_next(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.ptr   = 
> SG_ADDRESS(CURRENT_SC->SCp.buffer);
>   CURRENT_SC->SCp.this_residual = 
> CURRENT_SC->SCp.buffer->length;
>   }
> @@ -2160,20 +2160,29 @@ static void datao_end(struct Scsi_Host *shpnt)
>   if(TESTLO(DMASTAT, DFIFOEMP)) {
>   int data_count = (DATA_LEN - scsi_get_resid(CURRENT_SC)) -
>   GETSTCNT();

data_count appears to be the number of bytes remaining in the FIFO. (I 
have to infer that much from the surrounding code. I don't have 
documentation for this controller.)

Some comments would be nice.

> + struct scatterlist *sg = scsi_sglist(CURRENT_SC);
> + int left, i = 0;
>  
>   CMD_INC_RESID(CURRENT_SC, data_count);
>  

Apparently the aim is to increase the residual by the number of bytes that 
will never leave the FIFO. Above we can see that increase performed by 
scsi_set_resid() and now the same has to be done to the SCp struct.

>   data_count -= CURRENT_SC->SCp.ptr -
>   SG_ADDRESS(CURRENT_SC->SCp.buffer);

Here, data_count effectively has SCp.this_residual subtracted from it.

> - while(data_count>0) {
> - CURRENT_SC->SCp.buffer--;
> - CURRENT_SC->SCp.buffers_residual++;
> - data_count -= CURRENT_SC->SCp.buffer->length;
> - }

So far, so good.

> - CURRENT_SC->SCp.ptr = SG_ADDRESS(CURRENT_SC->SCp.buffer) -
> - data_count;
> - CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length +
> - data_count;

This is like saying ptr = buffer + residual, which is bogus. This must be 
an old bug, but we never hit it because the FIFO is always empty when we 
get a DISCONNECT message. Probably because every SG segment has a length 
that is a multiple of 128 bytes. (Juergen?)

> +
> + left = CURRENT_SC->transfersize - data_count;

Are you sure about that? Perhaps you meant to write,
left = scsi_bufflen(CURRENT_SC) - scsi_get_resid(CURRENT_SC);

Is there a better name for this variable? Maybe 'sent' or 'bytes_sent'?

> + for (i = 0; left > 0 && !sg_is_last(sg); i++, sg = sg_next(sg)) 
> {
> + if (left < sg->length)
> + break;
> + left -= sg->length;
> + }
> +
> + if (data_count > 0) {
> + CURRENT_SC->SCp.buffers_residual += i;

Shouldn't that be,
CURRENT_SC->SCp.buffers_residual = i;

> + CURRENT_SC->SCp.buffer = sg;
> +
> + CURRENT_SC->SCp.ptr = 
> SG_ADDRESS(CURRENT_SC->SCp.buffer) + left;
> + CURRENT_SC->SCp.this_residual = 
> CURRENT_SC->SCp.buffer->length -
> + left;
> + }
>   }
>  
>   SETPORT(SXFRCTL0, CH1|CLRCH1|CLRSTCNT);
> 

BTW, datao_run() seems to guarantee that the FIFO will never contain more 
than min(128, SCp.this_residual) so I suspect that this code can be 
simplified. That's just BTW. I wouldn't attempt to optimize this branch as 
it w

Re: [PATCH V3 0/3] scsi: three SG_CHAIN related fixes

[Finn Thain]

On Thu, 6 Jun 2019, Martin K. Petersen wrote:

> 
> Ming,
> 
> > Guenter reported scsi boot issue caused by commit c3288dd8c232 ("scsi:
> > core: avoid pre-allocating big SGL for data").
> 
> Applied to 5.3/scsi-queue, thank you!
> 

Thanks, that seems to fix the esp_scsi regression.

Am I right in thinking that commit c3288dd8c232 ("scsi: core: avoid 
pre-allocating big SGL for data") has the effect that any scsi host with 
sg_tablesize > 2 must now support chained sg lists?

In commit 4af14d113bcf ("[PATCH] scsi: remove the use_clustering flag"), I 
read that "setting the dma_boundary to PAGE_SIZE - 1 and the 
max_segment_size to PAGE_SIZE" is sufficient to inhibit clustering. Is 
that sufficient to inhibit chained sg lists for LLDs?

Does it follow that #define SCSI_INLINE_SG_CNT 2 is now the upper bound 
for sg list entries (clamping sg_tablesize) for those LLDs (regardless 
support for chained sg lists)?

Does commit c3288dd8c232 have similar implications for any LLD running on 
an architecture with CONFIG_ARCH_NO_SG_CHAIN=y?

I can't find answers to these questions in Documentation/block/biodoc.txt 
etc. Any clarification or insight you can offer would be appreciated.

Thanks.

-- 

Re: [PATCH V2 3/3] scsi: esp: make it working on SG_CHAIN

[Finn Thain]

On Wed, 5 Jun 2019, Ming Lei wrote:

> The driver expects that SCSI SGL is a simply array of SG, and itereate
> the SGL via integer index. This way is obviously wrong, because of
> SG_CHAIN.
> 
> Fixes it by using sgl helper.
> 
> V2:
>   - as suggested by Finn Thain, introduce .prv_sg for getting previous
>   scatterlist pointer
> 

Thanks. I'd forgotten about the consequences for SAVE/RESTORE POINTERS 
until I saw your patch.

I can't test the change to the IGNORE WIDE RESIDUE message handling but I 
did confirm that the patch series works in QEMU/m68k.

Reviewed-by: Finn Thain 

-- 

> Cc: Christoph Hellwig 
> Cc: Bart Van Assche 
> Cc: Ewan D. Milne 
> Cc: Hannes Reinecke 
> Cc: Finn Thain 
> Cc: Guenter Roeck 
> Reported-by: Guenter Roeck 
> Signed-off-by: Ming Lei 
> ---
>  drivers/scsi/esp_scsi.c | 20 +---
>  drivers/scsi/esp_scsi.h |  2 ++
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index 76e7ca864d6a..bb88995a12c7 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -371,6 +371,7 @@ static void esp_map_dma(struct esp *esp, struct scsi_cmnd 
> *cmd)
>   struct esp_cmd_priv *spriv = ESP_CMD_PRIV(cmd);
>   struct scatterlist *sg = scsi_sglist(cmd);
>   int total = 0, i;
> + struct scatterlist *s;
>  
>   if (cmd->sc_data_direction == DMA_NONE)
>   return;
> @@ -381,16 +382,18 @@ static void esp_map_dma(struct esp *esp, struct 
> scsi_cmnd *cmd)
>* a dma address, so perform an identity mapping.
>*/
>   spriv->num_sg = scsi_sg_count(cmd);
> - for (i = 0; i < spriv->num_sg; i++) {
> - sg[i].dma_address = (uintptr_t)sg_virt(&sg[i]);
> - total += sg_dma_len(&sg[i]);
> +
> + scsi_for_each_sg(cmd, s, spriv->num_sg, i) {
> + s->dma_address = (uintptr_t)sg_virt(s);
> + total += sg_dma_len(s);
>   }
>   } else {
>   spriv->num_sg = scsi_dma_map(cmd);
> - for (i = 0; i < spriv->num_sg; i++)
> - total += sg_dma_len(&sg[i]);
> + scsi_for_each_sg(cmd, s, spriv->num_sg, i)
> + total += sg_dma_len(s);
>   }
>   spriv->cur_residue = sg_dma_len(sg);
> + spriv->prv_sg = NULL;
>   spriv->cur_sg = sg;
>   spriv->tot_residue = total;
>  }
> @@ -444,7 +447,8 @@ static void esp_advance_dma(struct esp *esp, struct 
> esp_cmd_entry *ent,
>   p->tot_residue = 0;
>   }
>   if (!p->cur_residue && p->tot_residue) {
> - p->cur_sg++;
> + p->prv_sg = p->cur_sg;
> + p->cur_sg = sg_next(p->cur_sg);
>   p->cur_residue = sg_dma_len(p->cur_sg);
>   }
>  }
> @@ -465,6 +469,7 @@ static void esp_save_pointers(struct esp *esp, struct 
> esp_cmd_entry *ent)
>   return;
>   }
>   ent->saved_cur_residue = spriv->cur_residue;
> + ent->saved_prv_sg = spriv->prv_sg;
>   ent->saved_cur_sg = spriv->cur_sg;
>   ent->saved_tot_residue = spriv->tot_residue;
>  }
> @@ -479,6 +484,7 @@ static void esp_restore_pointers(struct esp *esp, struct 
> esp_cmd_entry *ent)
>   return;
>   }
>   spriv->cur_residue = ent->saved_cur_residue;
> + spriv->prv_sg = ent->saved_prv_sg;
>   spriv->cur_sg = ent->saved_cur_sg;
>   spriv->tot_residue = ent->saved_tot_residue;
>  }
> @@ -1647,7 +1653,7 @@ static int esp_msgin_process(struct esp *esp)
>   spriv = ESP_CMD_PRIV(ent->cmd);
>  
>   if (spriv->cur_residue == sg_dma_len(spriv->cur_sg)) {
> - spriv->cur_sg--;
> + spriv->cur_sg = spriv->prv_sg;
>   spriv->cur_residue = 1;
>   } else
>   spriv->cur_residue++;
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index aa87a6b72dcc..91b32f2a1a1b 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -251,6 +251,7 @@
>  struct esp_cmd_priv {
>   int num_sg;
>   int cur_residue;
> + struct scatterlist  *prv_sg;
>   struct scatterlist  *cur_sg;
>   int tot_residue;
>  };
> @@ -273,6 +274,7 @@ struct esp_cmd_entry {
>   struct scsi_cmnd*cmd;
>  
>   unsigned intsaved_cur_residue;
> + struct scatterlist  *saved_prv_sg;
>   struct scatterlist  *saved_cur_sg;
>   unsigned intsaved_tot_residue;
>  
> 

Re: [PATCH 2/2] scsi: esp: make it working on SG_CHAIN

[Finn Thain]

On Tue, 4 Jun 2019, Ming Lei wrote:

> The driver supporses that there isn't sg chain, and itereate the
> list one by one. This way is obviously wrong.
> 
> Fixes it by sgl helper.
> 
> Cc: Christoph Hellwig 
> Cc: Bart Van Assche 
> Cc: Ewan D. Milne 
> Cc: Hannes Reinecke 
> Cc: Guenter Roeck 
> Reported-by: Guenter Roeck 
> Signed-off-by: Ming Lei 
> ---
>  drivers/scsi/esp_scsi.c | 32 +---
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index 76e7ca864d6a..58b4e059dcfb 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -371,6 +371,7 @@ static void esp_map_dma(struct esp *esp, struct scsi_cmnd 
> *cmd)
>   struct esp_cmd_priv *spriv = ESP_CMD_PRIV(cmd);
>   struct scatterlist *sg = scsi_sglist(cmd);
>   int total = 0, i;
> + struct scatterlist *sgt;
>  
>   if (cmd->sc_data_direction == DMA_NONE)
>   return;
> @@ -381,14 +382,15 @@ static void esp_map_dma(struct esp *esp, struct 
> scsi_cmnd *cmd)
>* a dma address, so perform an identity mapping.
>*/
>   spriv->num_sg = scsi_sg_count(cmd);
> - for (i = 0; i < spriv->num_sg; i++) {
> - sg[i].dma_address = (uintptr_t)sg_virt(&sg[i]);
> - total += sg_dma_len(&sg[i]);
> +
> + scsi_for_each_sg(cmd, sgt, spriv->num_sg, i) {
> + sgt->dma_address = (uintptr_t)sg_virt(sgt);
> + total += sg_dma_len(sgt);
>   }
>   } else {
>   spriv->num_sg = scsi_dma_map(cmd);
> - for (i = 0; i < spriv->num_sg; i++)
> - total += sg_dma_len(&sg[i]);
> + scsi_for_each_sg(cmd, sgt, spriv->num_sg, i)
> + total += sg_dma_len(sgt);
>   }
>   spriv->cur_residue = sg_dma_len(sg);
>   spriv->cur_sg = sg;
> @@ -444,7 +446,7 @@ static void esp_advance_dma(struct esp *esp, struct 
> esp_cmd_entry *ent,
>   p->tot_residue = 0;
>   }
>   if (!p->cur_residue && p->tot_residue) {
> - p->cur_sg++;
> + p->cur_sg = sg_next(p->cur_sg);
>   p->cur_residue = sg_dma_len(p->cur_sg);
>   }
>  }
> @@ -1610,6 +1612,22 @@ static void esp_msgin_extended(struct esp *esp)
>   scsi_esp_cmd(esp, ESP_CMD_SATN);
>  }
>  
> +static struct scatterlist *esp_sg_prev(struct scsi_cmnd *cmd,
> + struct scatterlist *sg)
> +{
> + int i;
> + struct scatterlist *tmp;
> + struct scatterlist *prev = NULL;
> +
> + scsi_for_each_sg(cmd, tmp, scsi_sg_count(cmd), i) {
> + if (tmp == sg)
> + break;
> + prev = tmp;
> + }
> +
> + return prev;
> +}
> +

Did you consider recording the previous scatterlist pointer using an 
additional member in struct esp_cmd_priv, to be assigned in 
esp_advance_dma()? I think it would execute faster and require less code.

-- 

>  /* Analyze msgin bytes received from target so far.  Return non-zero
>   * if there are more bytes needed to complete the message.
>   */
> @@ -1647,7 +1665,7 @@ static int esp_msgin_process(struct esp *esp)
>   spriv = ESP_CMD_PRIV(ent->cmd);
>  
>   if (spriv->cur_residue == sg_dma_len(spriv->cur_sg)) {
> - spriv->cur_sg--;
> + spriv->cur_sg = esp_sg_prev(ent->cmd, spriv->cur_sg);
>   spriv->cur_residue = 1;
>   } else
>   spriv->cur_residue++;
> 

Re: [PATCH] scsi/NCR5380: Avoid compiler warning when -Wimplicit-fallthrough is enabled

[Finn Thain]

On Mon, 8 Apr 2019, Gustavo A. R. Silva wrote:

> Hi all,
> 
> Friendly ping:
> 
> Who can take this?
> 
> Thanks
> 

It has been queued up by Martin and James on git.kernel.org. Apparently it 
is to be pushed in the v5.2 merge window.

-- 

[PATCH] scsi/NCR5380: Remove set but unused variable

[Finn Thain]

Cc: Michael Schmitz 
Signed-off-by: Finn Thain 
---
 drivers/scsi/NCR5380.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 01c23d27f290..7fed9bb72784 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -272,9 +272,8 @@ mrs[] = {
 static void NCR5380_print(struct Scsi_Host *instance)
 {
struct NCR5380_hostdata *hostdata = shost_priv(instance);
-   unsigned char status, data, basr, mr, icr, i;
+   unsigned char status, basr, mr, icr, i;
 
-   data = NCR5380_read(CURRENT_SCSI_DATA_REG);
status = NCR5380_read(STATUS_REG);
mr = NCR5380_read(MODE_REG);
icr = NCR5380_read(INITIATOR_COMMAND_REG);
-- 
2.19.2

[PATCH] scsi/NCR5380: Avoid compiler warning when -Wimplicit-fallthrough is enabled

[Finn Thain]

Adjust comments accordingly.

Cc: Gustavo A. R. Silva 
Cc: Michael Schmitz 
Signed-off-by: Finn Thain 
---
 drivers/scsi/NCR5380.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 7fed9bb72784..fe0535affc14 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -1932,13 +1932,13 @@ static void NCR5380_information_transfer(struct 
Scsi_Host *instance)
if (!hostdata->connected)
return;
 
-   /* Fall through to reject message */
-
+   /* Reject message */
+   /* Fall through */
+   default:
/*
 * If we get something weird that we 
aren't expecting,
-* reject it.
+* log it.
 */
-   default:
if (tmp == EXTENDED_MESSAGE)
scmd_printk(KERN_INFO, cmd,
"rejecting unknown 
extended message code %02x, length %d\n",
-- 
2.19.2

Re: [PATCH v2] scsi: NCR5380: Mark expected switch fall-through

[Finn Thain]

On Thu, 28 Feb 2019, Gustavo A. R. Silva wrote:

> In preparation to enabling -Wimplicit-fallthrough, mark switch
> cases where we are expecting to fall through.
> 

This switch case is already marked. So I think the patch description 
should state that this patch is actually a workaround for a gcc deficiency 
which prevents it from locating the marker.

> This patch fixes the following warning:
> 
> In file included from drivers/scsi/dmx3191d.c:48:
> drivers/scsi/NCR5380.c: In function ?NCR5380_information_transfer?:
> drivers/scsi/NCR5380.c:1933:9: warning: this statement may fall through 
> [-Wimplicit-fallthrough=]
>   if (!hostdata->connected)
>  ^
> drivers/scsi/NCR5380.c:1937:5: note: here
>  default:
>  ^~~
> 
> Warning level 3 was used: -Wimplicit-fallthrough=3
> 
> Notice that, in this particular case, the code comment is modified
> in accordance with what GCC is expecting to find.
> 
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
> 
> Signed-off-by: Gustavo A. R. Silva 
> ---
> Changes in v2:
>  - Update commit log.
>  - Move code comment after the default label and
>retain reason for fall-through in comment as
>requested by Michael Schmitz.
> 
>  drivers/scsi/NCR5380.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> index 01c23d27f290..985d1c053578 100644
> --- a/drivers/scsi/NCR5380.c
> +++ b/drivers/scsi/NCR5380.c
> @@ -1933,13 +1933,12 @@ static void NCR5380_information_transfer(struct 
> Scsi_Host *instance)
>   if (!hostdata->connected)
>   return;
>  
> - /* Fall through to reject message */
> -
> + /* Fall through - to reject message */

This new hyphen is wrong and harms readability for humans.

I did confirm that gcc can be appeased by the use of a hyphen but not by 
correct grammar such as "Fall through to reject message" or "Fall through. 
Reject message."

> + default:
>   /*
> -  * If we get something weird that we 
> aren't expecting,
> -  * reject it.
> +  * If we get something weird that we
> +  * aren't expecting, reject it.

This reformatting isn't relevant to this patch. The comments can be 
improved however (see below).

>*/
> - default:

Moving the 'default' keyword closer to the 'fall through' comment makes 
sense to me -- I could understand if gcc had simple, unambiguous rules for 
annotations.

Do compilers and static analysers agree as to what a correctly annotated 
switch label should look like? If not, we would have to try to mangle code 
and comments in such a way that might satisfy all of the failings in all 
of the tools.

>   if (tmp == EXTENDED_MESSAGE)
>   scmd_printk(KERN_INFO, cmd,
>   "rejecting unknown 
> extended message code %02x, length %d\n",
> 

Here's an alternative patch, which has the virtue that a simple heuristic 
will work. This patch does not require that other static analysis tools 
will follow gcc's weird rules about hyphens. (I assume they don't but I 
didn't check.)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 7fed9bb72784..fe0535affc14 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -1932,13 +1932,13 @@ static void NCR5380_information_transfer(struct 
Scsi_Host *instance)
if (!hostdata->connected)
return;
 
-   /* Fall through to reject message */
-
+   /* Reject message */
+   /* Fall through */
+   default:
/*
 * If we get something weird that we 
aren't expecting,
-* reject it.
+* log it.
 */
-   default:
if (tmp == EXTENDED_MESSAGE)
scmd_printk(KERN_INFO, cmd,
"rejecting unknown 
extended message code %02x, length %d\n",

-- 

Re: [PATCH v9 00/22] Re-use nvram module

[Finn Thain]

On Tue, 22 Jan 2019, Greg Kroah-Hartman wrote:

> On Tue, Jan 15, 2019 at 03:18:56PM +1100, Finn Thain wrote:
> > The "generic" NVRAM module, drivers/char/generic_nvram.c, implements a
> > /dev/nvram misc device. This module is used only by 32-bit PowerPC
> > platforms.
> > 
> > The RTC "CMOS" NVRAM module, drivers/char/nvram.c, also implements a
> > /dev/nvram misc device. This module is now used only by x86 and m68k
> > thanks to commit 3ba9faedc180 ("char: nvram: disable on ARM").
> > 
> > The "generic" module cannot be used by x86 or m68k platforms because it
> > cannot co-exist with the "CMOS" module. One reason for that is the
> > CONFIG_GENERIC_NVRAM kludge in drivers/char/Makefile. Another reason is
> > that automatically loading the appropriate module would be impossible
> > because only one module can provide the char-major-10-144 alias.
> > 
> > A multi-platform kernel binary needs a single, generic module. With this
> > patch series, drivers/char/nvram.c becomes more generic and some of the
> > arch-specific code gets moved under arch/. The nvram module is then
> > usable by all m68k, powerpc and x86 platforms.
> > 
> > This allows for removal of drivers/char/generic_nvram.c as well as a
> > duplicate in arch/powerpc/kernel/nvram_64.c. By reducing the number of
> > /dev/nvram char misc device implementations, the number of bugs and
> > inconsistencies is also reduced.
> > 
> > This approach reduces inconsistencies between PPC32 and PPC64 and also
> > between PPC_PMAC and MAC. A uniform API has benefits for userspace.
> > 
> > For example, some error codes for some ioctl calls become consistent
> > across PowerPC platforms. The uniform API can potentially benefit any
> > bootloader that works across the various platforms having XPRAM
> > (e.g. Emile).
> > 
> > This patch series was tested on Atari, Mac, PowerMac (both 32-bit and
> > 64-bit) and ThinkPad hardware. AFAIK, it has not yet been tested on
> > pSeries or CHRP.
> > 
> > I think there are two possible merge strategies for this patch series.
> > The char misc maintainer could take the entire series. Alternatively,
> > the m68k maintainer could take patches 1 thru 16 (though some of these
> > have nothing to do with m68k) and after those patches reach mainline
> > the powerpc maintainer could take 17 thru 22.
> 
> I just took the whole series, thanks for doing this, looks good.
> 

Thanks, Greg.

I haven't seen any acks from powerpc maintainers yet...

-- 

> greg k-h
> 

[PATCH v9 01/22] scsi/atari_scsi: Don't select CONFIG_NVRAM

[Finn Thain]

On powerpc, setting CONFIG_NVRAM=n builds a kernel with no NVRAM support.
Setting CONFIG_NVRAM=m enables the /dev/nvram misc device module without
enabling NVRAM support in drivers. Setting CONFIG_NVRAM=y enables the
misc device (built-in) and also enables NVRAM support in drivers.

m68k shares the valkyriefb driver with powerpc, and since that driver uses
NVRAM, it is affected by CONFIG_ATARI_SCSI, because of the use of
"select NVRAM". We can avoid the "select" here, but drivers still have
to interpret the CONFIG_NVRAM symbol consistently regardless of platform.

In this patch and the subsequent fbdev driver patch, the convention is
adopted across all relevant platforms whereby NVRAM functionality gets
enabled in a given device driver when the nvram misc device is built-in
or when both drivers are modules.

Acked-by: Michael Schmitz 
Signed-off-by: Finn Thain 
---
This patch temporarily disables CONFIG_NVRAM on Atari, to prevent build
failures when bisecting the rest of this patch series. It gets enabled
again with the introduction of CONFIG_HAVE_ARCH_NVRAM_OPS, once the
nvram_* global functions have been moved to an ops struct.

Changed since v8:
 - Replaced defined(CONFIG_NVRAM) with IS_REACHABLE(CONFIG_NVRAM) as
suggested by James Bottomley.
 - Changed #ifdef to if as suggested by Christophe Leroy.
---
 drivers/char/Kconfig  | 5 +
 drivers/scsi/Kconfig  | 6 +++---
 drivers/scsi/atari_scsi.c | 2 +-
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 2e2ffe7010aa..a8cac68de177 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -244,7 +244,7 @@ source "drivers/char/hw_random/Kconfig"
 
 config NVRAM
tristate "/dev/nvram support"
-   depends on ATARI || X86 || GENERIC_NVRAM
+   depends on X86 || GENERIC_NVRAM
---help---
  If you say Y here and create a character special file /dev/nvram
  with major number 10 and minor number 144 using mknod ("man mknod"),
@@ -262,9 +262,6 @@ config NVRAM
  should NEVER idly tamper with it. See Ralf Brown's interrupt list
  for a guide to the use of CMOS bytes by your BIOS.
 
- On Atari machines, /dev/nvram is always configured and does not need
- to be selected.
-
  To compile this driver as a module, choose M here: the
  module will be called nvram.
 
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index f38882f6f37d..8f9d9e9fa695 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -1369,14 +1369,14 @@ config ATARI_SCSI
tristate "Atari native SCSI support"
depends on ATARI && SCSI
select SCSI_SPI_ATTRS
-   select NVRAM
---help---
  If you have an Atari with built-in NCR5380 SCSI controller (TT,
  Falcon, ...) say Y to get it supported. Of course also, if you have
  a compatible SCSI controller (e.g. for Medusa).
 
- To compile this driver as a module, choose M here: the
- module will be called atari_scsi.
+ To compile this driver as a module, choose M here: the module will
+ be called atari_scsi. If you also enable NVRAM support, the SCSI
+ host's ID is taken from the setting in TT RTC NVRAM.
 
  This driver supports both styles of NCR integration into the
  system: the TT style (separate DMA), and the Falcon style (via
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index a503dc50c4f8..78b43200c99e 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -757,7 +757,7 @@ static int __init atari_scsi_probe(struct platform_device 
*pdev)
 
if (setup_hostid >= 0) {
atari_scsi_template.this_id = setup_hostid & 7;
-   } else {
+   } else if (IS_REACHABLE(CONFIG_NVRAM)) {
/* Test if a host id is set in the NVRam */
if (ATARIHW_PRESENT(TT_CLK) && nvram_check_checksum()) {
unsigned char b = nvram_read_byte(16);
-- 
2.19.2

[PATCH v9 04/22] nvram: Replace nvram_* function exports with static functions

[Finn Thain]

Replace nvram_* functions with static functions in nvram.h. These will
become wrappers for struct nvram_ops method calls.

This patch effectively disables existing NVRAM functionality so as to
allow the rest of the series to be bisected without build failures.
That functionality is gradually re-implemented in subsequent patches.

Replace the sole validate-checksum-and-read-byte sequence with a call to
nvram_read() which will gain the same semantics in subsequent patches.

Remove unused exports.

Acked-by: Geert Uytterhoeven 
Signed-off-by: Finn Thain 
---
 arch/m68k/atari/nvram.c   | 39 +++
 drivers/char/nvram.c  | 27 +--
 drivers/scsi/atari_scsi.c |  8 +---
 include/linux/nvram.h | 32 +---
 4 files changed, 38 insertions(+), 68 deletions(-)

diff --git a/arch/m68k/atari/nvram.c b/arch/m68k/atari/nvram.c
index a8c457e40b0b..1d767847ffa6 100644
--- a/arch/m68k/atari/nvram.c
+++ b/arch/m68k/atari/nvram.c
@@ -34,38 +34,17 @@
  * periodic 11 min sync from kernel/time/ntp.c vs. this driver.)
  */
 
-unsigned char __nvram_read_byte(int i)
+static unsigned char __nvram_read_byte(int i)
 {
return CMOS_READ(NVRAM_FIRST_BYTE + i);
 }
 
-unsigned char nvram_read_byte(int i)
-{
-   unsigned long flags;
-   unsigned char c;
-
-   spin_lock_irqsave(&rtc_lock, flags);
-   c = __nvram_read_byte(i);
-   spin_unlock_irqrestore(&rtc_lock, flags);
-   return c;
-}
-EXPORT_SYMBOL(nvram_read_byte);
-
 /* This races nicely with trying to read with checksum checking */
-void __nvram_write_byte(unsigned char c, int i)
+static void __nvram_write_byte(unsigned char c, int i)
 {
CMOS_WRITE(c, NVRAM_FIRST_BYTE + i);
 }
 
-void nvram_write_byte(unsigned char c, int i)
-{
-   unsigned long flags;
-
-   spin_lock_irqsave(&rtc_lock, flags);
-   __nvram_write_byte(c, i);
-   spin_unlock_irqrestore(&rtc_lock, flags);
-}
-
 /* On Ataris, the checksum is over all bytes except the checksum bytes
  * themselves; these are at the very end.
  */
@@ -73,7 +52,7 @@ void nvram_write_byte(unsigned char c, int i)
 #define ATARI_CKS_RANGE_END47
 #define ATARI_CKS_LOC  48
 
-int __nvram_check_checksum(void)
+static int __nvram_check_checksum(void)
 {
int i;
unsigned char sum = 0;
@@ -84,18 +63,6 @@ int __nvram_check_checksum(void)
   (__nvram_read_byte(ATARI_CKS_LOC + 1) == (sum & 0xff));
 }
 
-int nvram_check_checksum(void)
-{
-   unsigned long flags;
-   int rv;
-
-   spin_lock_irqsave(&rtc_lock, flags);
-   rv = __nvram_check_checksum();
-   spin_unlock_irqrestore(&rtc_lock, flags);
-   return rv;
-}
-EXPORT_SYMBOL(nvram_check_checksum);
-
 static void __nvram_set_checksum(void)
 {
int i;
diff --git a/drivers/char/nvram.c b/drivers/char/nvram.c
index c660cff9faf4..c98775bfd896 100644
--- a/drivers/char/nvram.c
+++ b/drivers/char/nvram.c
@@ -74,13 +74,12 @@ static int nvram_open_mode; /* special open modes */
  * periodic 11 min sync from kernel/time/ntp.c vs. this driver.)
  */
 
-unsigned char __nvram_read_byte(int i)
+static unsigned char __nvram_read_byte(int i)
 {
return CMOS_READ(NVRAM_FIRST_BYTE + i);
 }
-EXPORT_SYMBOL(__nvram_read_byte);
 
-unsigned char nvram_read_byte(int i)
+static unsigned char pc_nvram_read_byte(int i)
 {
unsigned long flags;
unsigned char c;
@@ -90,16 +89,14 @@ unsigned char nvram_read_byte(int i)
spin_unlock_irqrestore(&rtc_lock, flags);
return c;
 }
-EXPORT_SYMBOL(nvram_read_byte);
 
 /* This races nicely with trying to read with checksum checking (nvram_read) */
-void __nvram_write_byte(unsigned char c, int i)
+static void __nvram_write_byte(unsigned char c, int i)
 {
CMOS_WRITE(c, NVRAM_FIRST_BYTE + i);
 }
-EXPORT_SYMBOL(__nvram_write_byte);
 
-void nvram_write_byte(unsigned char c, int i)
+static void pc_nvram_write_byte(unsigned char c, int i)
 {
unsigned long flags;
 
@@ -107,14 +104,13 @@ void nvram_write_byte(unsigned char c, int i)
__nvram_write_byte(c, i);
spin_unlock_irqrestore(&rtc_lock, flags);
 }
-EXPORT_SYMBOL(nvram_write_byte);
 
 /* On PCs, the checksum is built only over bytes 2..31 */
 #define PC_CKS_RANGE_START 2
 #define PC_CKS_RANGE_END   31
 #define PC_CKS_LOC 32
 
-int __nvram_check_checksum(void)
+static int __nvram_check_checksum(void)
 {
int i;
unsigned short sum = 0;
@@ -126,19 +122,6 @@ int __nvram_check_checksum(void)
__nvram_read_byte(PC_CKS_LOC+1);
return (sum & 0x) == expect;
 }
-EXPORT_SYMBOL(__nvram_check_checksum);
-
-int nvram_check_checksum(void)
-{
-   unsigned long flags;
-   int rv;
-
-   spin_lock_irqsave(&rtc_lock, flags);
-   rv = __nvram_check_checksum();
-   spin_unlock_irqrestore(&rtc_lock, flags);
-   return rv;
-}
-EXPORT_SYMBOL(nvram_check_che

[PATCH v9 00/22] Re-use nvram module

[Finn Thain]

The "generic" NVRAM module, drivers/char/generic_nvram.c, implements a
/dev/nvram misc device. This module is used only by 32-bit PowerPC
platforms.

The RTC "CMOS" NVRAM module, drivers/char/nvram.c, also implements a
/dev/nvram misc device. This module is now used only by x86 and m68k
thanks to commit 3ba9faedc180 ("char: nvram: disable on ARM").

The "generic" module cannot be used by x86 or m68k platforms because it
cannot co-exist with the "CMOS" module. One reason for that is the
CONFIG_GENERIC_NVRAM kludge in drivers/char/Makefile. Another reason is
that automatically loading the appropriate module would be impossible
because only one module can provide the char-major-10-144 alias.

A multi-platform kernel binary needs a single, generic module. With this
patch series, drivers/char/nvram.c becomes more generic and some of the
arch-specific code gets moved under arch/. The nvram module is then
usable by all m68k, powerpc and x86 platforms.

This allows for removal of drivers/char/generic_nvram.c as well as a
duplicate in arch/powerpc/kernel/nvram_64.c. By reducing the number of
/dev/nvram char misc device implementations, the number of bugs and
inconsistencies is also reduced.

This approach reduces inconsistencies between PPC32 and PPC64 and also
between PPC_PMAC and MAC. A uniform API has benefits for userspace.

For example, some error codes for some ioctl calls become consistent
across PowerPC platforms. The uniform API can potentially benefit any
bootloader that works across the various platforms having XPRAM
(e.g. Emile).

This patch series was tested on Atari, Mac, PowerMac (both 32-bit and
64-bit) and ThinkPad hardware. AFAIK, it has not yet been tested on
pSeries or CHRP.

I think there are two possible merge strategies for this patch series.
The char misc maintainer could take the entire series. Alternatively,
the m68k maintainer could take patches 1 thru 16 (though some of these
have nothing to do with m68k) and after those patches reach mainline
the powerpc maintainer could take 17 thru 22.

Changed since v8:
 - Replaced defined(CONFIG_NVRAM) with IS_REACHABLE(CONFIG_NVRAM) as
suggested by James Bottomley.
 - Changed #ifdef to if as suggested by Christophe Leroy.
 - Expanded the fbdev patch to include controlfb.c and platinumfb.c.
 - Added kernel-doc comment to describe struct nvram_ops.
 - Moved the HAVE_ARCH_NVRAM_OPS symbol to common code as suggested
by Christoph Hellwig.
 - Abandoned conversion of powerpc drivers to arch_nvram_ops, as discussed
with Arnd Bergmann.
 - Dropped patch 6 ("x86/thinkpad_acpi: Use arch_nvram_ops methods").
 - Dropped patch 17 ("powerpc: Implement arch_nvram_ops.get_size() ...").
 - Dropped patch 20 ("powerpc, fbdev: Use arch_nvram_ops methods ...").
 - Dropped patch 25 ("powerpc: Remove pmac_xpram_{read,write} functions").
 - Added portable static functions to nvram.h which wrap both arch_nvram_ops
and ppc_md method calls.
 - Re-ordered and revised patches to resolve conflicts with existing extern
definitions in nvram.h and elsewhere.
 - Rebased on v5.0-rc2.
 - Added patch 14 ("macintosh/via-cuda: Don't rely on Cuda to end a transfer").

Changed since v7:
 - Rebased.
 - Dropped patch 9/26, "char/nvram: Use generic fixed_size_llseek()"
because generic_file_llseek_size() was adopted in commit b808b1d632f6.
 - Reordered the m68k and powerpc patches to simplify the merge strategy.
 - Addressed some trivial checkpatch.pl complaints.
 - Improved some commit log entries.
 - Changed the CONFIG_NVRAM default to better approximate the present code.
In particular, the CONFIG_GENERIC_NVRAM default and use of "select NVRAM".
 - Added more tested-by tags.

For older change logs, please refer to,
https://lore.kernel.org/lkml/20151101104202.301856...@telegraphics.com.au/


Finn Thain (22):
  scsi/atari_scsi: Don't select CONFIG_NVRAM
  m68k/atari: Move Atari-specific code out of drivers/char/nvram.c
  char/nvram: Re-order functions to remove forward declarations and
#ifdefs
  nvram: Replace nvram_* function exports with static functions
  m68k/atari: Implement arch_nvram_ops struct
  powerpc: Replace nvram_* extern declarations with standard header
  char/nvram: Adopt arch_nvram_ops
  char/nvram: Allow the set_checksum and initialize ioctls to be omitted
  char/nvram: Implement NVRAM read/write methods
  m68k/atari: Implement arch_nvram_ops methods and enable
CONFIG_HAVE_ARCH_NVRAM_OPS
  m68k/mac: Adopt naming and calling conventions for PRAM routines
  m68k/mac: Use macros for RTC accesses not magic numbers
  m68k/mac: Fix PRAM accessors
  macintosh/via-cuda: Don't rely on Cuda to end a transfer
  m68k: Dispatch nvram_ops calls to Atari or Mac functions
  char/nvram: Add "devname:nvram" module alias
  powerpc: Define missing ppc_md.nvram_size for CHRP and PowerMac
  powerpc: Implement nvram ioctls
  powerpc, fbdev: Use NV

Re: [PATCH v8 01/25] scsi/atari_scsi: Don't select CONFIG_NVRAM

[Finn Thain]

On Mon, 31 Dec 2018, Finn Thain wrote:

> On Sun, 30 Dec 2018, James Bottomley wrote:
> 
> > 
> > That said, as has been pointed out, the current #ifdef has a failing 
> > corner case when both are modular (because the code should then be 
> > included).  The runtime macro that correctly expresses this is 
> > IS_REACHABLE(CONFIG_NVRAM).
> > 
> 
> No, in the case of CONFIG_NVRAM=m, the conditional code is deliberately 
> excluded. This is discussed in the patch description. The convention on 
> PPC32 is that device drivers drop support for NVRAM in this situation. 
> 
> I've adopted the PPC32 convention here because M68K drivers and PPC32 
> drivers have to co-exist (I'm thinking of valkyriefb, but there are other 
> examples).
> 

I agree with your comment in principle, that these drivers should be using 
IS_REACHABLE(CONFIG_NVRAM), not defined(CONFIG_NVRAM).

$ grep -lrw CONFIG_NVRAM drivers/
drivers/video/fbdev/matrox/matroxfb_base.c
drivers/video/fbdev/platinumfb.c
drivers/video/fbdev/controlfb.c
drivers/video/fbdev/valkyriefb.c
drivers/video/fbdev/imsttfb.c
drivers/scsi/atari_scsi.c
$ 

But I think this is not the fault of this patch; it's just a historical 
accident.

Having said that, I will rework this series to convert all of the above 
drivers to IS_REACHABLE(CONFIG_NVRAM). I think it would be an improvement.

Besides, it seems likely that some rework involving ppc_md will be needed 
too because as Arnd pointed out, it would be good to avoid two sets of 
nvram accessor methods.

-- 

Re: [PATCH v8 01/25] scsi/atari_scsi: Don't select CONFIG_NVRAM

[Finn Thain]

On Sun, 30 Dec 2018, James Bottomley wrote:

> 
> That said, as has been pointed out, the current #ifdef has a failing 
> corner case when both are modular (because the code should then be 
> included).  The runtime macro that correctly expresses this is 
> IS_REACHABLE(CONFIG_NVRAM).
> 

No, in the case of CONFIG_NVRAM=m, the conditional code is deliberately 
excluded. This is discussed in the patch description. The convention on 
PPC32 is that device drivers drop support for NVRAM in this situation. 

I've adopted the PPC32 convention here because M68K drivers and PPC32 
drivers have to co-exist (I'm thinking of valkyriefb, but there are other 
examples).

-- 

> James
> 
> 

Re: [PATCH v8 01/25] scsi/atari_scsi: Don't select CONFIG_NVRAM

[Finn Thain]

On Sat, 29 Dec 2018, Michael Schmitz wrote:

> 
> IS_BUILTIN(CONFIG_NVRAM) is probably what Christophe really meant to suggest.
> 
> Or (really going out on a limb here):
> 
> IS_BUILTIN(CONFIG_NVRAM) ||
> ( IS_MODULE(CONFIG_ATARI_SCSI) && IS_ENABLED(CONFIG_NVRAM) )
> 
> Not that I'd advocate that, for this series.
> 

Well, you are a maintainer for atari_scsi.c.

Are you saying that you want IS_BUILTIN(CONFIG_NVRAM) used here instead of 
ifdef?

OTOH, if you approve of the existing patch, please send your acked-by.

-- 

> Cheers,
> 
>   Michael
> 
> 
> 
> 

Re: [PATCH v8 01/25] scsi/atari_scsi: Don't select CONFIG_NVRAM

[Finn Thain]

On Fri, 28 Dec 2018, LEROY Christophe wrote:

> Finn Thain  a ?crit?:
> 
> > On powerpc, setting CONFIG_NVRAM=n builds a kernel with no NVRAM support.
> > Setting CONFIG_NVRAM=m enables the /dev/nvram misc device module without
> > enabling NVRAM support in drivers. Setting CONFIG_NVRAM=y enables the
> > misc device (built-in) and also enables NVRAM support in drivers.
> >
> > m68k shares the valkyriefb driver with powerpc, and since that driver uses
> > NVRAM, it is affected by CONFIG_ATARI_SCSI, because of the use of
> > "select NVRAM".
> >
> > Adopt the powerpc convention on m68k to avoid surprises.
> >
> > Signed-off-by: Finn Thain 
> > Tested-by: Christian T. Steigies 
> > ---
> > This patch temporarily disables CONFIG_NVRAM on Atari, to prevent build
> > failures when bisecting the rest of this patch series. It gets enabled
> > again with the introduction of CONFIG_HAVE_ARCH_NVRAM_OPS, once the
> > nvram_* global functions have been moved to an ops struct.
> > ---
> >  drivers/char/Kconfig  | 5 +
> >  drivers/scsi/Kconfig  | 6 +++---
> >  drivers/scsi/atari_scsi.c | 7 ---
> >  3 files changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> > index 9d03b2ff5df6..5b54595dfe30 100644
> > --- a/drivers/char/Kconfig
> > +++ b/drivers/char/Kconfig
> > @@ -236,7 +236,7 @@ source "drivers/char/hw_random/Kconfig"
> >
> >  config NVRAM
> > tristate "/dev/nvram support"
> > -   depends on ATARI || X86 || GENERIC_NVRAM
> > +   depends on X86 || GENERIC_NVRAM
> > ---help---
> >   If you say Y here and create a character special file /dev/nvram
> >   with major number 10 and minor number 144 using mknod ("man mknod"),
> > @@ -254,9 +254,6 @@ config NVRAM
> >   should NEVER idly tamper with it. See Ralf Brown's interrupt list
> >   for a guide to the use of CMOS bytes by your BIOS.
> >
> > - On Atari machines, /dev/nvram is always configured and does not need
> > - to be selected.
> > -
> >   To compile this driver as a module, choose M here: the
> >   module will be called nvram.
> >
> > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> > index 640cd1b31a18..924eb69e7fc4 100644
> > --- a/drivers/scsi/Kconfig
> > +++ b/drivers/scsi/Kconfig
> > @@ -1381,14 +1381,14 @@ config ATARI_SCSI
> > tristate "Atari native SCSI support"
> > depends on ATARI && SCSI
> > select SCSI_SPI_ATTRS
> > -   select NVRAM
> > ---help---
> >   If you have an Atari with built-in NCR5380 SCSI controller (TT,
> >   Falcon, ...) say Y to get it supported. Of course also, if you have
> >   a compatible SCSI controller (e.g. for Medusa).
> >
> > - To compile this driver as a module, choose M here: the
> > - module will be called atari_scsi.
> > + To compile this driver as a module, choose M here: the module will
> > + be called atari_scsi. If you also enable NVRAM support, the SCSI
> > + host's ID is taken from the setting in TT RTC NVRAM.
> >
> >   This driver supports both styles of NCR integration into the
> >   system: the TT style (separate DMA), and the Falcon style (via
> > diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
> > index 89f5154c40b6..99e5729d910d 100644
> > --- a/drivers/scsi/atari_scsi.c
> > +++ b/drivers/scsi/atari_scsi.c
> > @@ -755,9 +755,10 @@ static int __init atari_scsi_probe(struct  
> > platform_device *pdev)
> > if (ATARIHW_PRESENT(TT_SCSI) && setup_sg_tablesize >= 0)
> > atari_scsi_template.sg_tablesize = setup_sg_tablesize;
> >
> > -   if (setup_hostid >= 0) {
> > +   if (setup_hostid >= 0)
> > atari_scsi_template.this_id = setup_hostid & 7;
> > -   } else {
> > +#ifdef CONFIG_NVRAM
> > +   else
> 
> Such ifdefs should be avoided in C files.
> It would be better to use
> 
> } else if (IS_ENABLED(CONFIG_NVRAM)) {
> 

I don't like #ifdefs either. However, as the maintainer of this file, I am 
okay with this one.

The old #ifdef CONFIG_NVRAM conditional compilation convention that gets 
used here and under drivers/video/fbdev could probably be improved upon 
but I consider this to be out-of-scope for this series, which is 
complicated enough.

And as explained in the commit log, CONFIG_NVRAM=y and CONFIG_NVRAM=m are 
treaded differently by drivers. Therefore, IS_ENABLED would be incorrect.

-- 

> > /* Test if a host id is set in the NVRam */
> > if (ATARIHW_PRESENT(TT_CLK) && nvram_check_checksum()) {
> > unsigned char b = nvram_read_byte(16);
> > @@ -768,7 +769,7 @@ static int __init atari_scsi_probe(struct  
> > platform_device *pdev)
> > if (b & 0x80)
> > atari_scsi_template.this_id = b & 7;
> > }
> > -   }
> > +#endif
> >
> > /* If running on a Falcon and if there's TT-Ram (i.e., more than one
> >  * memory block, since there's always ST-Ram in a Falcon), then
> > --
> > 2.19.2
> 
> 
> 

[PATCH v8 01/25] scsi/atari_scsi: Don't select CONFIG_NVRAM

[Finn Thain]

On powerpc, setting CONFIG_NVRAM=n builds a kernel with no NVRAM support.
Setting CONFIG_NVRAM=m enables the /dev/nvram misc device module without
enabling NVRAM support in drivers. Setting CONFIG_NVRAM=y enables the
misc device (built-in) and also enables NVRAM support in drivers.

m68k shares the valkyriefb driver with powerpc, and since that driver uses
NVRAM, it is affected by CONFIG_ATARI_SCSI, because of the use of
"select NVRAM".

Adopt the powerpc convention on m68k to avoid surprises.

Signed-off-by: Finn Thain 
Tested-by: Christian T. Steigies 
---
This patch temporarily disables CONFIG_NVRAM on Atari, to prevent build
failures when bisecting the rest of this patch series. It gets enabled
again with the introduction of CONFIG_HAVE_ARCH_NVRAM_OPS, once the
nvram_* global functions have been moved to an ops struct.
---
 drivers/char/Kconfig  | 5 +
 drivers/scsi/Kconfig  | 6 +++---
 drivers/scsi/atari_scsi.c | 7 ---
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 9d03b2ff5df6..5b54595dfe30 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -236,7 +236,7 @@ source "drivers/char/hw_random/Kconfig"
 
 config NVRAM
tristate "/dev/nvram support"
-   depends on ATARI || X86 || GENERIC_NVRAM
+   depends on X86 || GENERIC_NVRAM
---help---
  If you say Y here and create a character special file /dev/nvram
  with major number 10 and minor number 144 using mknod ("man mknod"),
@@ -254,9 +254,6 @@ config NVRAM
  should NEVER idly tamper with it. See Ralf Brown's interrupt list
  for a guide to the use of CMOS bytes by your BIOS.
 
- On Atari machines, /dev/nvram is always configured and does not need
- to be selected.
-
  To compile this driver as a module, choose M here: the
  module will be called nvram.
 
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 640cd1b31a18..924eb69e7fc4 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -1381,14 +1381,14 @@ config ATARI_SCSI
tristate "Atari native SCSI support"
depends on ATARI && SCSI
select SCSI_SPI_ATTRS
-   select NVRAM
---help---
  If you have an Atari with built-in NCR5380 SCSI controller (TT,
  Falcon, ...) say Y to get it supported. Of course also, if you have
  a compatible SCSI controller (e.g. for Medusa).
 
- To compile this driver as a module, choose M here: the
- module will be called atari_scsi.
+ To compile this driver as a module, choose M here: the module will
+ be called atari_scsi. If you also enable NVRAM support, the SCSI
+ host's ID is taken from the setting in TT RTC NVRAM.
 
  This driver supports both styles of NCR integration into the
  system: the TT style (separate DMA), and the Falcon style (via
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 89f5154c40b6..99e5729d910d 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -755,9 +755,10 @@ static int __init atari_scsi_probe(struct platform_device 
*pdev)
if (ATARIHW_PRESENT(TT_SCSI) && setup_sg_tablesize >= 0)
atari_scsi_template.sg_tablesize = setup_sg_tablesize;
 
-   if (setup_hostid >= 0) {
+   if (setup_hostid >= 0)
atari_scsi_template.this_id = setup_hostid & 7;
-   } else {
+#ifdef CONFIG_NVRAM
+   else
/* Test if a host id is set in the NVRam */
if (ATARIHW_PRESENT(TT_CLK) && nvram_check_checksum()) {
unsigned char b = nvram_read_byte(16);
@@ -768,7 +769,7 @@ static int __init atari_scsi_probe(struct platform_device 
*pdev)
if (b & 0x80)
atari_scsi_template.this_id = b & 7;
}
-   }
+#endif
 
/* If running on a Falcon and if there's TT-Ram (i.e., more than one
 * memory block, since there's always ST-Ram in a Falcon), then
-- 
2.19.2

[PATCH v8 03/25] m68k/atari: Replace nvram_{read,write}_byte with arch_nvram_ops

[Finn Thain]

By implementing an arch_nvram_ops struct, any platform can re-use the
drivers/char/nvram.c module without needing any arch-specific code
in that module. Atari does so here.

Atari has one user of nvram_check_checksum() whereas the other "CMOS"
platforms don't use that function at all. Replace this
validate-checksum-and-read-byte sequence with the equivalent
rtc_nvram_ops.read() call and remove the now unused functions.

Signed-off-by: Finn Thain 
Tested-by: Christian T. Steigies 
Acked-by: Geert Uytterhoeven 
---
The advantage of the new ops struct over the old global nvram_* functions
is that the misc device module can be shared by different platforms
without requiring every platform to implement every nvram_* function.
E.g. only RTC "CMOS" NVRAMs have a checksum for the entire NVRAM
and only PowerPC platforms have a "sync" ioctl.
---
 arch/m68k/atari/nvram.c   | 89 ---
 drivers/scsi/atari_scsi.c |  8 ++--
 include/linux/nvram.h |  9 
 3 files changed, 70 insertions(+), 36 deletions(-)

diff --git a/arch/m68k/atari/nvram.c b/arch/m68k/atari/nvram.c
index 3e620ee955ba..bafc9dc32830 100644
--- a/arch/m68k/atari/nvram.c
+++ b/arch/m68k/atari/nvram.c
@@ -39,33 +39,12 @@ unsigned char __nvram_read_byte(int i)
return CMOS_READ(NVRAM_FIRST_BYTE + i);
 }
 
-unsigned char nvram_read_byte(int i)
-{
-   unsigned long flags;
-   unsigned char c;
-
-   spin_lock_irqsave(&rtc_lock, flags);
-   c = __nvram_read_byte(i);
-   spin_unlock_irqrestore(&rtc_lock, flags);
-   return c;
-}
-EXPORT_SYMBOL(nvram_read_byte);
-
 /* This races nicely with trying to read with checksum checking */
 void __nvram_write_byte(unsigned char c, int i)
 {
CMOS_WRITE(c, NVRAM_FIRST_BYTE + i);
 }
 
-void nvram_write_byte(unsigned char c, int i)
-{
-   unsigned long flags;
-
-   spin_lock_irqsave(&rtc_lock, flags);
-   __nvram_write_byte(c, i);
-   spin_unlock_irqrestore(&rtc_lock, flags);
-}
-
 /* On Ataris, the checksum is over all bytes except the checksum bytes
  * themselves; these are at the very end.
  */
@@ -84,18 +63,6 @@ int __nvram_check_checksum(void)
   (__nvram_read_byte(ATARI_CKS_LOC + 1) == (sum & 0xff));
 }
 
-int nvram_check_checksum(void)
-{
-   unsigned long flags;
-   int rv;
-
-   spin_lock_irqsave(&rtc_lock, flags);
-   rv = __nvram_check_checksum();
-   spin_unlock_irqrestore(&rtc_lock, flags);
-   return rv;
-}
-EXPORT_SYMBOL(nvram_check_checksum);
-
 static void __nvram_set_checksum(void)
 {
int i;
@@ -107,6 +74,62 @@ static void __nvram_set_checksum(void)
__nvram_write_byte(sum, ATARI_CKS_LOC + 1);
 }
 
+static ssize_t nvram_read(char *buf, size_t count, loff_t *ppos)
+{
+   char *p = buf;
+   loff_t i;
+
+   spin_lock_irq(&rtc_lock);
+
+   if (!__nvram_check_checksum()) {
+   spin_unlock_irq(&rtc_lock);
+   return -EIO;
+   }
+
+   for (i = *ppos; count > 0 && i < NVRAM_BYTES; --count, ++i, ++p)
+   *p = __nvram_read_byte(i);
+
+   spin_unlock_irq(&rtc_lock);
+
+   *ppos = i;
+   return p - buf;
+}
+
+static ssize_t nvram_write(char *buf, size_t count, loff_t *ppos)
+{
+   char *p = buf;
+   loff_t i;
+
+   spin_lock_irq(&rtc_lock);
+
+   if (!__nvram_check_checksum()) {
+   spin_unlock_irq(&rtc_lock);
+   return -EIO;
+   }
+
+   for (i = *ppos; count > 0 && i < NVRAM_BYTES; --count, ++i, ++p)
+   __nvram_write_byte(*p, i);
+
+   __nvram_set_checksum();
+
+   spin_unlock_irq(&rtc_lock);
+
+   *ppos = i;
+   return p - buf;
+}
+
+static ssize_t nvram_get_size(void)
+{
+   return NVRAM_BYTES;
+}
+
+const struct nvram_ops arch_nvram_ops = {
+   .read   = nvram_read,
+   .write  = nvram_write,
+   .get_size   = nvram_get_size,
+};
+EXPORT_SYMBOL(arch_nvram_ops);
+
 #ifdef CONFIG_PROC_FS
 static struct {
unsigned char val;
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 99e5729d910d..f55d13e400da 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -760,13 +760,15 @@ static int __init atari_scsi_probe(struct platform_device 
*pdev)
 #ifdef CONFIG_NVRAM
else
/* Test if a host id is set in the NVRam */
-   if (ATARIHW_PRESENT(TT_CLK) && nvram_check_checksum()) {
-   unsigned char b = nvram_read_byte(16);
+   if (ATARIHW_PRESENT(TT_CLK)) {
+   unsigned char b;
+   loff_t offset = 16;
+   ssize_t count = arch_nvram_ops.read(&b, 1, &offset);
 
/* Arbitration enabled? (for TOS)
 * If yes, use configu

Re: [PATCH 01/10] gdth: refactor ioc_general

[Finn Thain]

On Thu, 6 Dec 2018, Christoph Hellwig wrote:

> This function is a huge mess with duplicated error handling.  Split out
> a few useful helpers and use goto labels to untangle the error handling
> and no-data ioctl handling.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/scsi/gdth.c | 244 +++-
>  1 file changed, 126 insertions(+), 118 deletions(-)
> 
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index 16709735b546..45e67d4cb3af 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -4155,131 +4155,139 @@ static int ioc_resetdrv(void __user *arg, char 
> *cmnd)
>  return 0;
>  }
>  
> -static int ioc_general(void __user *arg, char *cmnd)
> +static void gdth_ioc_addr32(gdth_ha_str *ha, gdth_ioctl_general *gen,
> + u64 paddr)
>  {
> -gdth_ioctl_general gen;
> -char *buf = NULL;
> -u64 paddr; 
> -gdth_ha_str *ha;
> -int rval;
> + if (ha->cache_feat & SCATTER_GATHER) {
> + gen->command.u.cache.DestAddr = 0x;
> + gen->command.u.cache.sg_canz = 1;
> + gen->command.u.cache.sg_lst[0].sg_ptr = (u32)paddr;
> + gen->command.u.cache.sg_lst[0].sg_len = gen->data_len;
> + gen->command.u.cache.sg_lst[1].sg_len = 0;
> + } else {
> + gen->command.u.cache.DestAddr = paddr;
> + gen->command.u.cache.sg_canz = 0;
> + }
> +}
>  
> -if (copy_from_user(&gen, arg, sizeof(gdth_ioctl_general)))
> -return -EFAULT;
> -ha = gdth_find_ha(gen.ionode);
> -if (!ha)
> -return -EFAULT;
> +static void gdth_ioc_addr64(gdth_ha_str *ha, gdth_ioctl_general *gen,
> + u64 paddr)
> +{
> + if (ha->cache_feat & SCATTER_GATHER) {
> + gen->command.u.cache64.DestAddr = (u64)-1;
> + gen->command.u.cache64.sg_canz = 1;
> + gen->command.u.cache64.sg_lst[0].sg_ptr = paddr;
> + gen->command.u.cache64.sg_lst[0].sg_len = gen->data_len;
> + gen->command.u.cache64.sg_lst[1].sg_len = 0;
> + } else {
> + gen->command.u.cache64.DestAddr = paddr;
> + gen->command.u.cache64.sg_canz = 0;
> + }
> +}
>  
> -if (gen.data_len > INT_MAX)
> -return -EINVAL;
> -if (gen.sense_len > INT_MAX)
> -return -EINVAL;
> -if (gen.data_len + gen.sense_len > INT_MAX)
> -return -EINVAL;
> +static void gdth_ioc_cacheservice(gdth_ha_str *ha, gdth_ioctl_general *gen,
> + u64 paddr)
> +{
> + if (ha->cache_feat & GDT_64BIT) {
> + /* copy elements from 32-bit IOCTL structure */
> + gen->command.u.cache64.BlockCnt = gen->command.u.cache.BlockCnt;
> + gen->command.u.cache64.BlockNo = gen->command.u.cache.BlockNo;
> + gen->command.u.cache64.DeviceNo = gen->command.u.cache.DeviceNo;
>  
> -if (gen.data_len + gen.sense_len != 0) {
> -if (!(buf = gdth_ioctl_alloc(ha, gen.data_len + gen.sense_len,
> - FALSE, &paddr)))
> -return -EFAULT;
> -if (copy_from_user(buf, arg + sizeof(gdth_ioctl_general),  
> -   gen.data_len + gen.sense_len)) {
> -gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
> -return -EFAULT;
> -}
> + gdth_ioc_addr64(ha, gen, paddr);
> + } else {
> + gdth_ioc_addr32(ha, gen, paddr);
> + }
> +}
>  
> -if (gen.command.OpCode == GDT_IOCTL) {
> -gen.command.u.ioctl.p_param = paddr;
> -} else if (gen.command.Service == CACHESERVICE) {
> -if (ha->cache_feat & GDT_64BIT) {
> -/* copy elements from 32-bit IOCTL structure */
> -gen.command.u.cache64.BlockCnt = 
> gen.command.u.cache.BlockCnt;
> -gen.command.u.cache64.BlockNo = gen.command.u.cache.BlockNo;
> -gen.command.u.cache64.DeviceNo = 
> gen.command.u.cache.DeviceNo;
> -/* addresses */
> -if (ha->cache_feat & SCATTER_GATHER) {
> -gen.command.u.cache64.DestAddr = (u64)-1;
> -gen.command.u.cache64.sg_canz = 1;
> -gen.command.u.cache64.sg_lst[0].sg_ptr = paddr;
> -gen.command.u.cache64.sg_lst[0].sg_len = gen.data_len;
> -gen.command.u.cache64.sg_lst[1].sg_len = 0;
> -} else {
> -gen.command.u.cache64.DestAddr = paddr;
> -gen.command.u.cache64.sg_canz = 0;
> -}
> -} else {
> -if (ha->cache_feat & SCATTER_GATHER) {
> -gen.command.u.cache.DestAddr = 0x;
> -gen.command.u.cache.sg_canz = 1;
> -gen.command.u.cache.sg_lst[0].sg_ptr = (u32)paddr;
> -gen.command.u.cache.sg_lst[0].sg_len = gen.data_len;
> -gen.c

Re: DISABLE_CLUSTERING in scsi drivers

[Finn Thain]

On Mon, 3 Dec 2018, Hannes Reinecke wrote:

> As I said: I need to do PIO for the last two bytes of the data buffer. 
> For everything else DMA works nicely, it's just the last two bytes which 
> might be left over in the FIFO buffer under certain circumstances.

I read the driver a few times already, thanks.

> If you have an alternative suggestion without scsi_kmap_atomic_sg() I'd 
> be happy to convert it.

I'm not trying to avoid scsi_kmap_atomic_sg(). Nevermind.

-- 

> 
> Cheers,
> 
> Hannes
> 

Re: DISABLE_CLUSTERING in scsi drivers

[Finn Thain]

On Sun, 2 Dec 2018, Hannes Reinecke wrote:

> On 12/2/18 10:21 PM, Finn Thain wrote:
> > On Sun, 2 Dec 2018, Hannes Reinecke wrote:
> > 
> > > Well, that lone 'kmap' is due to a quirk/errata in the datasheet;
> > > essentially
> > > we have to PIO a lone byte out of the FIFO to clear it up.
> > > And this byte is technically still part of the SCSI data, so we need to
> > > stuff it onto the end of the actual data sg list. Which is what the kmap()
> > > thingie does.
> > > So it really shouldn't be affected by the clustering algorithm.
> > > 
> > 
> > Sorry, I don't follow.
> > 
> > If it's dead code, can it be removed
> > 
> Oh, it's not dead code. It's required as per datasheet.
> 
> > If it's not, does it require DISABLE_CLUSTERING?
> > 
> No, not really. It just affects the very last byte of the sglist,
> so I can't really see how it should be affected by clustering.
> 

AIUI, the scsi_kmap_atomic_sg() call which you added to esp_scsi.c assumes 
that the sg list elements are page sized and page aligned. 

DISABLE_CLUSTERING provides that guarantee, but am53c974.c doesn't use it. 

Is this not a bug? What am I missing?

BTW, Documentation/block/biodoc.txt suggests a bounce buffer as an 
alternative.

There are some situations when pages from high memory may need to be 
kmapped, even if bounce buffers are not necessary. For example a 
device may need to abort DMA operations and revert to PIO for the 
transfer, in which case a virtual mapping of the page is required. For 
SCSI it is also done in some scenarios where the low level driver 
cannot be trusted to handle a single sg entry correctly. The driver is 
expected to perform the kmaps as needed on such occasions as 
appropriate. A driver could also use the blk_queue_bounce() routine on 
its own to bounce highmem i/o to low memory for specific requests if 
so desired.

-- 

> Cheers,
> 
> Hannes
> 
> 

Re: DISABLE_CLUSTERING in scsi drivers

[Finn Thain]

On Sun, 2 Dec 2018, Hannes Reinecke wrote:

> Well, that lone 'kmap' is due to a quirk/errata in the datasheet; essentially
> we have to PIO a lone byte out of the FIFO to clear it up.
> And this byte is technically still part of the SCSI data, so we need to
> stuff it onto the end of the actual data sg list. Which is what the kmap()
> thingie does.
> So it really shouldn't be affected by the clustering algorithm.
> 

Sorry, I don't follow.

If it's dead code, can it be removed?

If it's not, does it require DISABLE_CLUSTERING?

-- 

> Cheers,
> 
> Hannes
> 

Re: DISABLE_CLUSTERING in scsi drivers

[Finn Thain]

On Mon, 26 Nov 2018, Christoph Hellwig wrote:

> On Thu, Nov 22, 2018 at 09:02:13AM +1100, Finn Thain wrote:
> > > you in the To list maintain or wrote SCSI drivers that set the
> > > DISABLE_CLUSTERING flag, which basically disable merges of any
> > > bio segments.  We already have the actual max_segment size limit
> > > to say which length a segment should have, independent of merged
> > > or originally created, so this limit generally should rarely if
> > > ever be required, and mostly is an old cut an paste error.
> > > 
> > 
> > Are you referring to
> > blk_queue_max_segment_size(q, dma_get_max_seg_size(dev));
> > in drivers/scsi/scsi_lib.c?
> > 
> > Is the segment size limitation of the DMA controller the only reason to 
> > want DISABLE_CLUSTERING?
> 
> DISABLE_CLUSTERING mixes up two not really related things:
> 
>  1) limit the size of each segment to a single page size
>  2) limit each segment to not actually span a page boundary.
> 
> Both could be valid limit for DMA engines, but also might be particularly
> relevant for pio, if you e.g. kmap each page of a scatterlist do do
> pio you'd want to see both limits.
> 

I looked through all the drivers based on esp_scsi.c and NCR5380.c which 
use DISABLE_CLUSTERING.

There is one driver that uses DMA and sometimes kmap too, which is 
am53c974.c. It defaults to ENABLE_CLUSTERING, which would seem to be a bug 
if kmap isn't compatible with that (Hannes?).

For g_NCR5380.c (ISA bus) and dmx3191d.c (PCI bus) these drivers need 
DISABLE_CLUSTERING because apparently they don't use kmap or DMA. This 
will need to be addressed if you remove DISABLE_CLUSTERING.

Then we have the ARM drivers, cumana_1.c and oak.c. I believe that ecard 
drivers like these are only used with CONFIG_ARCH_RPC, and I think that 
would imply !CONFIG_HIGHMEM (Russell?).

m68k that doesn't seem to have CONFIG_HIGHMEM either, so no need for kmap 
or DISABLE_CLUSTERING when doing PIO or PDMA. That includes mac_esp.c and 
mac_scsi.c. Michael has already answered for atari_scsi.c.

That leaves the Sun 3 drivers, sun3_scsi.c and sun3_scsi_vme.c, which may 
have DMA limitations I don't know about (Sam?).

-- 

Re: [PATCH AUTOSEL 4.19 09/36] scsi: NCR5380: Return false instead of NULL

[Finn Thain]

This patch is not relevant to any -stable branch. Please don't backport.

-- 

On Thu, 22 Nov 2018, Sasha Levin wrote:

> From: Finn Thain 
> 
> [ Upstream commit 96edebd6bb995f2acb7694bed6e01bf6f5a7b634 ]
> 
> I overlooked this statement when I recently converted the function result
> type from struct scsi_cmnd * to bool. No change to object code.
> 
> Signed-off-by: Finn Thain 
> Signed-off-by: Martin K. Petersen 
> Signed-off-by: Sasha Levin 
> ---
>  drivers/scsi/NCR5380.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> index 90ea0f5d9bdb..fbcbd6db371f 100644
> --- a/drivers/scsi/NCR5380.c
> +++ b/drivers/scsi/NCR5380.c
> @@ -1190,7 +1190,7 @@ static struct scsi_cmnd *NCR5380_select(struct 
> Scsi_Host *instance,
>  
>  out:
>   if (!hostdata->selecting)
> - return NULL;
> + return false;
>   hostdata->selecting = NULL;
>   return cmd;
>  }
> 

Re: DISABLE_CLUSTERING in scsi drivers

[Finn Thain]

On Wed, 21 Nov 2018, Christoph Hellwig wrote:

> Hi all,
> 
> you in the To list maintain or wrote SCSI drivers that set the
> DISABLE_CLUSTERING flag, which basically disable merges of any
> bio segments.  We already have the actual max_segment size limit
> to say which length a segment should have, independent of merged
> or originally created, so this limit generally should rarely if
> ever be required, and mostly is an old cut an paste error.
> 

Are you referring to
blk_queue_max_segment_size(q, dma_get_max_seg_size(dev));
in drivers/scsi/scsi_lib.c?

Is the segment size limitation of the DMA controller the only reason to 
want DISABLE_CLUSTERING?

> Can you go over your drivers and check if it could be removed?
> 

Adding Sun 3 maintainer to Cc.

Besides sun3_scsi.c and atari_scsi.c, the SCSI drivers I take an interest 
in don't actually use a DMA controller. If my question about DMA 
controllers gets a "yes" then I guess I can send a patch for those 
drivers.

-- 

Re: remove exofs and the T10 OSD code V2

[Finn Thain]

On Wed, 31 Oct 2018, Boaz Harrosh wrote:

> > [...] All changes made by other kernel developers than you are the 
> > result of tree-wide refactoring, compiler warning fixes, fixes for 
> > issues detected by static source code analyzers or spelling fixes. 
> > Hence my question: how big is the user base of the exofs and osd 
> > kernel drivers?
> > 
> 
> Not big at all. And none of them production setups. As I said mainly 
> used by academia.
> 

For its part, academia has provided tools to automate the mindless and 
error-prone refactoring which Bart points to. I'm mainly thinking of LLVM 
and Ocaml. (I'm not a computer scientist.)

But I fear that code transformation tools lie beyond the skill and ability 
of those of us who submit spelling fixes.

So the tools need to be made more usable in this setting. Corporate users 
of kernel code who happen to have their own IDE products (Microsoft, 
Google, etc.) have an opportunity here.

-- 

> Thanks
> Boaz
> 
> > Thanks,
> > Bart.
> 
> 

[PATCH] NCR5380: Return false instead of NULL

[Finn Thain]

I overlooked this statement when I recently converted the function result
type from struct scsi_cmnd * to bool. No change to object code.

Signed-off-by: Finn Thain 
---
 drivers/scsi/NCR5380.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 8429c855701f..01c23d27f290 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -1198,7 +1198,7 @@ static bool NCR5380_select(struct Scsi_Host *instance, 
struct scsi_cmnd *cmd)
 
 out:
if (!hostdata->selecting)
-   return NULL;
+   return false;
hostdata->selecting = NULL;
return ret;
 }
-- 
2.18.1

Re: [PATCH 2/4] gdth: reuse dma coherent allocation in gdth_show_info

[Finn Thain]

On Thu, 18 Oct 2018, Christoph Hellwig wrote:

> gdth_show_info currently allocs and frees a dma buffer four times,
> which isn't very efficient. Reuse a single allocation instead.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/scsi/gdth_proc.c | 18 +-
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/scsi/gdth_proc.c b/drivers/scsi/gdth_proc.c
> index 3a9751a80225..63d851398e38 100644
> --- a/drivers/scsi/gdth_proc.c
> +++ b/drivers/scsi/gdth_proc.c
> @@ -226,11 +226,13 @@ int gdth_show_info(struct seq_file *m, struct Scsi_Host 
> *host)
>  #endif
>  
>  if (ha->more_proc) {
> +size_t size = max(GDTH_SCRATCH, sizeof(gdth_hget_str));
> +
>  /* more information: 2. about physical devices */
>  seq_puts(m, "\nPhysical Devices:");
>  flag = FALSE;
>  
> -buf = gdth_ioctl_alloc(ha, GDTH_SCRATCH, FALSE, &paddr);
> +buf = gdth_ioctl_alloc(ha, size, FALSE, &paddr);
>  if (!buf) 
>  goto stop_output;
>  for (i = 0; i < ha->bus_cnt; ++i) {
> @@ -323,7 +325,6 @@ int gdth_show_info(struct seq_file *m, struct Scsi_Host 
> *host)
>  }
>  }
>  }
> -gdth_ioctl_free(ha, GDTH_SCRATCH, buf, paddr);
>  
>  if (!flag)
>  seq_puts(m, "\n --\n");
> @@ -332,7 +333,6 @@ int gdth_show_info(struct seq_file *m, struct Scsi_Host 
> *host)
>  seq_puts(m, "\nLogical Drives:");
>  flag = FALSE;
>  
> -buf = gdth_ioctl_alloc(ha, GDTH_SCRATCH, FALSE, &paddr);
>  if (!buf) 
>  goto stop_output;

I think this !buf test is redundant.

-- 

>  for (i = 0; i < MAX_LDRIVES; ++i) {
> @@ -408,7 +408,6 @@ int gdth_show_info(struct seq_file *m, struct Scsi_Host 
> *host)
>  seq_printf(m,
> " To Array Drv.:\t%s\n", hrec);
>  }   
> -gdth_ioctl_free(ha, GDTH_SCRATCH, buf, paddr);
>  
>  if (!flag)
>  seq_puts(m, "\n --\n");
> @@ -417,9 +416,6 @@ int gdth_show_info(struct seq_file *m, struct Scsi_Host 
> *host)
>  seq_puts(m, "\nArray Drives:");
>  flag = FALSE;
>  
> -buf = gdth_ioctl_alloc(ha, GDTH_SCRATCH, FALSE, &paddr);
> -if (!buf) 
> -goto stop_output;
>  for (i = 0; i < MAX_LDRIVES; ++i) {
>  if (!(ha->hdr[i].is_arraydrv && ha->hdr[i].is_master))
>  continue;
> @@ -468,8 +464,7 @@ int gdth_show_info(struct seq_file *m, struct Scsi_Host 
> *host)
> hrec);
>  }
>  }
> -gdth_ioctl_free(ha, GDTH_SCRATCH, buf, paddr);
> -
> +
>  if (!flag)
>  seq_puts(m, "\n --\n");
>  
> @@ -477,9 +472,6 @@ int gdth_show_info(struct seq_file *m, struct Scsi_Host 
> *host)
>  seq_puts(m, "\nHost Drives:");
>  flag = FALSE;
>  
> -buf = gdth_ioctl_alloc(ha, sizeof(gdth_hget_str), FALSE, &paddr);
> -if (!buf) 
> -goto stop_output;
>  for (i = 0; i < MAX_LDRIVES; ++i) {
>  if (!ha->hdr[i].is_logdrv || 
>  (ha->hdr[i].is_arraydrv && !ha->hdr[i].is_master))
> @@ -510,7 +502,7 @@ int gdth_show_info(struct seq_file *m, struct Scsi_Host 
> *host)
>  }
>  }
>  }
> -gdth_ioctl_free(ha, sizeof(gdth_hget_str), buf, paddr);
> +gdth_ioctl_free(ha, size, buf, paddr);
>  
>  for (i = 0; i < MAX_HDRIVES; ++i) {
>  if (!(ha->hdr[i].present))
> 

Re: [PATCH 1/4] gdth: refactor ioc_general

[Finn Thain]

On Thu, 18 Oct 2018, Christoph Hellwig wrote:

> +
> +static int ioc_general(void __user *arg, char *cmnd)
> +{
> + gdth_ioctl_general gen;
> + gdth_ha_str *ha;
> + char *buf = NULL;
> + u64 paddr;
> + int rval;
> +
> + if (copy_from_user(&gen, arg, sizeof(gdth_ioctl_general)))
> + return -EFAULT;
> + ha = gdth_find_ha(gen.ionode);
> + if (!ha)
> + return -EFAULT;
> +
> + if (gen.data_len > INT_MAX)
> + return -EINVAL;
> + if (gen.sense_len > INT_MAX)
> + return -EINVAL;
> + if (gen.data_len + gen.sense_len > INT_MAX)
> + return -EINVAL;
> +
> + if (gen.data_len + gen.sense_len == 0)
> + goto execute;
> +
> + buf = gdth_ioctl_alloc(ha, gen.data_len + gen.sense_len, FALSE, &paddr);
> + if (!buf)
> + return -EFAULT;
> +
> + rval = -EFAULT;
> + if (copy_from_user(buf, arg + sizeof(gdth_ioctl_general),
> +gen.data_len + gen.sense_len))
> + goto out_free_buf;
> +
> + switch (gen.command.OpCode) {
> + case GDT_IOCTL:
> + gen.command.u.ioctl.p_param = paddr;
> + break;
> + case CACHESERVICE:
> + gdth_ioc_cacheservice(ha, &gen, paddr);
> + break;
> + case SCSIRAWSERVICE:
> + gdth_ioc_scsiraw(ha, &gen, paddr);
> + break;
> + default:
> + goto out_free_buf;
>  }
> -}

AFAICT, CACHESERVICE never gets assigned to command.OpCode.

That means your switch() is not equivalent to the original construction --

if (gen.command.OpCode == GDT_IOCTL) {

} else if (gen.command.Service == CACHESERVICE) {

} else if (gen.command.Service == SCSIRAWSERVICE) {

} else {

}

>  
> -rval = __gdth_execute(ha->sdev, &gen.command, cmnd, gen.timeout, 
> &gen.info);
> -if (rval < 0) {
> +execute:
> + rval = __gdth_execute(ha->sdev, &gen.command, cmnd, gen.timeout,
> + &gen.info);
> + if (rval < 0)
> + goto out_free_buf;
> + gen.status = rval;
> +
> + rval = -EFAULT;
> + if (copy_to_user(arg + sizeof(gdth_ioctl_general), buf,
> +  gen.data_len + gen.sense_len))
> + goto out_free_buf;
> + if (copy_to_user(arg, &gen,
> + sizeof(gdth_ioctl_general) - sizeof(gdth_cmd_str)))
> + goto out_free_buf;
> +
> + rval = 0;
> +out_free_buf:
>   gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
> -return rval;
> -}
> -gen.status = rval;
> -
> -if (copy_to_user(arg + sizeof(gdth_ioctl_general), buf, 
> - gen.data_len + gen.sense_len)) {
> -gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
> -return -EFAULT; 
> -} 
> -if (copy_to_user(arg, &gen, 
> -sizeof(gdth_ioctl_general) - sizeof(gdth_cmd_str))) {
> -gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
> -return -EFAULT;
> -}
> -gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
> -return 0;
> + return 0;

This appears to be wrong also. I think you wanted,
return rval;

-- 

>  }
>   
>  static int ioc_hdrlist(void __user *arg, char *cmnd)
> 

Re: [PATCH 4/4] gdth: use generic DMA API

[Finn Thain]

On Thu, 18 Oct 2018, Christoph Hellwig wrote:

> Switch from the legacy PCI DMA API to the generic DMA API.  Also switch
> to dma_map_single from pci_map_page in one case where this makes the code
> simpler.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/scsi/gdth.c  | 111 +++
>  drivers/scsi/gdth_proc.c |   4 +-
>  2 files changed, 56 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index 7274d09b2a6c..3d856554b1b1 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -2518,9 +2518,9 @@ static int gdth_fill_cache_cmd(gdth_ha_str *ha, struct 
> scsi_cmnd *scp,
>  
>  if (scsi_bufflen(scp)) {
>  cmndinfo->dma_dir = (read_write == 1 ?
> -PCI_DMA_TODEVICE : PCI_DMA_FROMDEVICE);   
> -sgcnt = pci_map_sg(ha->pdev, scsi_sglist(scp), 
> scsi_sg_count(scp),
> -   cmndinfo->dma_dir);
> +DMA_TO_DEVICE : DMA_FROM_DEVICE);   
> +sgcnt = dma_map_sg(&ha->pdev->dev, scsi_sglist(scp),
> +scsi_sg_count(scp), cmndinfo->dma_dir);
>  if (mode64) {
>  struct scatterlist *sl;
>  
> @@ -2603,8 +2603,6 @@ static int gdth_fill_raw_cmd(gdth_ha_str *ha, struct 
> scsi_cmnd *scp, u8 b)
>  dma_addr_t sense_paddr;
>  int cmd_index, sgcnt, mode64;
>  u8 t,l;
> -struct page *page;
> -unsigned long offset;
>  struct gdth_cmndinfo *cmndinfo;
>  
>  t = scp->device->id;
> @@ -2649,10 +2647,8 @@ static int gdth_fill_raw_cmd(gdth_ha_str *ha, struct 
> scsi_cmnd *scp, u8 b)
>  }
>  
>  } else {
> -page = virt_to_page(scp->sense_buffer);
> -offset = (unsigned long)scp->sense_buffer & ~PAGE_MASK;
> -sense_paddr = pci_map_page(ha->pdev,page,offset,
> -   16,PCI_DMA_FROMDEVICE);
> +sense_paddr = dma_map_single(&ha->pdev->dev, scp->sense_buffer, 16,
> +  DMA_FROM_DEVICE);
>  
>   cmndinfo->sense_paddr  = sense_paddr;
>  cmdp->OpCode   = GDT_WRITE; /* always */
> @@ -2693,9 +2689,9 @@ static int gdth_fill_raw_cmd(gdth_ha_str *ha, struct 
> scsi_cmnd *scp, u8 b)
>  }
>  
>  if (scsi_bufflen(scp)) {
> -cmndinfo->dma_dir = PCI_DMA_BIDIRECTIONAL;
> -sgcnt = pci_map_sg(ha->pdev, scsi_sglist(scp), 
> scsi_sg_count(scp),
> -   cmndinfo->dma_dir);
> +cmndinfo->dma_dir = DMA_BIDIRECTIONAL;
> +sgcnt = dma_map_sg(&ha->pdev->dev, scsi_sglist(scp),
> +scsi_sg_count(scp), cmndinfo->dma_dir);
>  if (mode64) {
>  struct scatterlist *sl;
>  
> @@ -3313,12 +3309,12 @@ static int gdth_sync_event(gdth_ha_str *ha, int 
> service, u8 index,
>  return 2;
>  }
>  if (scsi_bufflen(scp))
> -pci_unmap_sg(ha->pdev, scsi_sglist(scp), scsi_sg_count(scp),
> +dma_unmap_sg(&ha->pdev->dev, scsi_sglist(scp), 
> scsi_sg_count(scp),
>   cmndinfo->dma_dir);
>  
>  if (cmndinfo->sense_paddr)
> -pci_unmap_page(ha->pdev, cmndinfo->sense_paddr, 16,
> -   
> PCI_DMA_FROMDEVICE);
> +dma_unmap_page(&ha->pdev->dev, cmndinfo->sense_paddr, 16,
> +DMA_FROM_DEVICE);
>  
>  if (ha->status == S_OK) {
>  cmndinfo->status = S_OK;
> @@ -4251,8 +4247,8 @@ static int ioc_general(void __user *arg, char *cmnd)
>   if (gen.data_len + gen.sense_len == 0)
>   goto execute;
>  
> -buf = pci_alloc_consistent(ha->pdev, gen.data_len + gen.sense_len,
> - &paddr);
> +buf = dma_alloc_coherent(&ha->pdev->dev, gen.data_len + 
> gen.sense_len,
> + &paddr, GFP_KERNEL);
>   if (!buf)
>   return -EFAULT;
>  
> @@ -4292,7 +4288,8 @@ static int ioc_general(void __user *arg, char *cmnd)
>  
>   rval = 0;
>  out_free_buf:
> - pci_free_consistent(ha->pdev, gen.data_len + gen.sense_len, buf, paddr);
> + dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len, buf,
> + paddr);
>   return 0;
>  }
>   
> @@ -4749,22 +4746,22 @@ static int __init gdth_isa_probe_one(u32 isa_bios)
>  
>   error = -ENOMEM;
>  
> - ha->pscratch = pci_alloc_consistent(ha->pdev, GDTH_SCRATCH,
> - &scratch_dma_handle);
> + ha->pscratch = dma_alloc_coherent(&ha->pdev->dev, GDTH_SCRATCH,
> + &scratch_dma_handle, GFP_KERNEL);
>   if (!ha->pscratch)
>   goto out_dec_counters;
>   ha->scratch_phys = scratch_dma_handle;
>  
> - ha->pmsg = pci_alloc_consistent(ha->pdev, sizeof(gdth_msg_str),
> - &scratch_dma_h

Re: [PATCH] scsi: ips: fix missing break in switch

[Finn Thain]

On Wed, 17 Oct 2018, Martin K. Petersen wrote:

> 
> >> See the case statements above for another fast exit scenario.
> >> 
> >
> > But that's an error path.
> 
> Look further down. Several other SCSI commands are completed as NOPs the 
> same way.
> 

That's true, but it doesn't indicate a bug to me.

On the contrary, the entire switch (scb->scsi_cmd->cmnd[0]) statement in 
ips_send_cmd() appears to be carefully constructed, just like the matching 
statement in ips_chkstatus().

But none of these indications can decide the question. We just don't have 
enough information. (I'm sure that someone somewhere does have the 
relevant technical information...)

If this really is undecidable, then I think the right patch is the more 
prudent one. That is, add a "fall through" comment, not a "break" 
statement.

Or perhaps a "fall through (TODO: check this)" comment.

> Also, I don't see how the case statement for TUR/INQUIRY would do 
> anything meaningful in terms of preparing a START STOP UNIT for the 
> firmware.
> 

If SSU case doesn't do anything meaningful, then neither does the 
TUR/INQUIRY case, and then you can just delete all of that code:

} else {
scb->cmd.logical_info.op_code = 
IPS_CMD_GET_LD_INFO;
scb->cmd.logical_info.command_id = 
IPS_COMMAND_ID(ha, scb);
scb->cmd.logical_info.reserved = 0;
scb->cmd.logical_info.reserved2 = 0;
scb->data_len = sizeof (IPS_LD_INFO);
scb->data_busaddr = 
ha->logical_drive_info_dma_addr;
scb->flags = 0;
scb->cmd.logical_info.buffer_addr = 
scb->data_busaddr;
ret = IPS_SUCCESS;
}

FWIW, I think this line of reasoning is mistaken.

-- 

Re: [PATCH v2 5/6] esp_scsi: De-duplicate PIO routines

[Finn Thain]

On Wed, 17 Oct 2018, Christoph Hellwig wrote:

> 
> Please leave the possibly unused exports as-is.
> 

I take that to mean "leave the conditional export as-is". Hence, I think 
v3 (posted on the 16th) should address all of the concerns raised by 
reviewers...

-- 

Re: [PATCH] scsi: ips: fix missing break in switch

[Finn Thain]

On Tue, 16 Oct 2018, Martin K. Petersen wrote:

> 
> Finn,
> 
> > This looks wrong to me. I think you've just prevented all START STOP 
> > commands sent to logical volumes from reaching
> >
> > return ((*ha->func.issue) (ha, scb));
> >
> > I think a better patch is to add a "fall though" comment not a "break" 
> > statement. (I no longer have access to a ServeRAID board so I can't 
> > test.)
> 
> When I looked at this a few days ago, it seemed that the fallthrough to 
> the TUR/INQUIRY case statement was accidental and that the intent was to 
> quickly complete START_STOP unit (which probably doesn't make much sense 
> for a RAID device anyway).
> 
> See the case statements above for another fast exit scenario.
> 

But that's an error path.

Also note that START_STOP also appears in ips_chkstatus(), which suggests 
to me that this command may be submitted legitimately.

> Sadly I have no way to test this. It just stuck out like a false 
> positive in Gustavo's fallthrough markup patch.
> 

Without testers, and without another maintainer to review this, I'd 
advocate a more prudent approach. I wonder whether there is another 
maintainer to do a review. The MAINTAINERS file seems to contradict 
itself.

$ grep -C5 drivers/scsi/ips MAINTAINERS 
...
IBM ServeRAID RAID DRIVER
S:  Orphan
F:  drivers/scsi/ips.*
...
IPS SCSI RAID DRIVER
M:  Adaptec OEM Raid Solutions 
L:  linux-scsi@vger.kernel.org
W:  http://www.adaptec.com/
S:  Maintained
F:  drivers/scsi/ips*
...

Note that 'drivers/scsi/ips*' got assigned to Microsemi by a janitorial 
change, as part of commit 679655daffdd ("MAINTAINERS - Add file patterns") 
in 2009. But for ips.c, the last acked-by from the vendor was in 2008.

-- 

Re: [PATCH] scsi: ips: fix missing break in switch

[Finn Thain]

On Tue, 16 Oct 2018, Martin K. Petersen wrote:

> 
> Gustavo,
> 
> > Add missing break statement in order to prevent the code from falling
> > through to case TEST_UNIT_READY.
> 
> Applied to 4.20/scsi-queue, thanks!
> 
> 

This looks wrong to me. I think you've just prevented all START STOP 
commands sent to logical volumes from reaching

return ((*ha->func.issue) (ha, scb));

I think a better patch is to add a "fall though" comment not a "break" 
statement. (I no longer have access to a ServeRAID board so I can't test.)

-- 

[PATCH v3 0/6] mac_esp, zorro_esp, esp_scsi: Various improvements

[Finn Thain]

This series has fixes and cleanup for mac_esp, zorro_esp and the
core esp_scsi driver.

These improvements include elimination of duplicated code temporarily
introduced for zorro_esp.


Finn Thain (6):
  zorro_esp: Limit DMA transfers to 65535 bytes
  esp_scsi: Track residual for PIO transfers
  esp_scsi: Grant disconnect privilege for untagged commands
  esp_scsi: Eliminate ESP_FLAG_DOING_SLOWCMD
  esp_scsi: De-duplicate PIO routines
  esp_scsi: Optimize PIO loops

 drivers/scsi/Kconfig |   5 +
 drivers/scsi/esp_scsi.c  | 197 +---
 drivers/scsi/esp_scsi.h  |  11 +-
 drivers/scsi/mac_esp.c   | 171 +---
 drivers/scsi/zorro_esp.c | 240 ++-
 5 files changed, 208 insertions(+), 416 deletions(-)

-- 
2.18.1

[PATCH v3 1/6] zorro_esp: Limit DMA transfers to 65535 bytes

[Finn Thain]

The core driver, esp_scsi, does not use the ESP_CONFIG2_FENAB bit, so
the chip's Transfer Counter register is only 16 bits wide (not 24).
A larger transfer cannot work and will theoretically result in a failed
command and a "DMA length is zero" error.

Fixes: 3109e5ae0311 ("scsi: zorro_esp: New driver for Amiga Zorro NCR53C9x 
boards")
Signed-off-by: Finn Thain 
Cc: Michael Schmitz 
Tested-by: Michael Schmitz 
Reviewed-by: Michael Schmitz 
---
 drivers/scsi/zorro_esp.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
index bb70882e6b56..be79127db594 100644
--- a/drivers/scsi/zorro_esp.c
+++ b/drivers/scsi/zorro_esp.c
@@ -245,7 +245,7 @@ static int fastlane_esp_irq_pending(struct esp *esp)
 static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
u32 dma_len)
 {
-   return dma_len > 0xFF ? 0xFF : dma_len;
+   return dma_len > 0x ? 0x : dma_len;
 }
 
 static void zorro_esp_reset_dma(struct esp *esp)
@@ -484,7 +484,6 @@ static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, 
u32 addr,
scsi_esp_cmd(esp, ESP_CMD_DMA);
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
scsi_esp_cmd(esp, cmd);
 }
@@ -529,7 +528,6 @@ static void zorro_esp_send_blz1230II_dma_cmd(struct esp 
*esp, u32 addr,
scsi_esp_cmd(esp, ESP_CMD_DMA);
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
scsi_esp_cmd(esp, cmd);
 }
@@ -574,7 +572,6 @@ static void zorro_esp_send_blz2060_dma_cmd(struct esp *esp, 
u32 addr,
scsi_esp_cmd(esp, ESP_CMD_DMA);
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
scsi_esp_cmd(esp, cmd);
 }
@@ -599,7 +596,6 @@ static void zorro_esp_send_cyber_dma_cmd(struct esp *esp, 
u32 addr,
 
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
if (write) {
/* DMA receive */
@@ -649,7 +645,6 @@ static void zorro_esp_send_cyberII_dma_cmd(struct esp *esp, 
u32 addr,
 
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
if (write) {
/* DMA receive */
@@ -691,7 +686,6 @@ static void zorro_esp_send_fastlane_dma_cmd(struct esp 
*esp, u32 addr,
 
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
if (write) {
/* DMA receive */
-- 
2.18.1

[PATCH v3 5/6] esp_scsi: De-duplicate PIO routines

[Finn Thain]

As a temporary measure, the code to implement PIO transfers was
duplicated in zorro_esp and mac_esp. Now that it has stabilized
move the common code into the core driver but don't build it unless
needed.

This replaces the inline assembler with more portable writesb() calls.
Optimizing the m68k writesb() implementation is a separate patch.

Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
Tested-by: Michael Schmitz 
---
Changed since v1:
 - Use shost_printk() instead of pr_err().
 - Add new symbol CONFIG_SCSI_ESP_PIO to Kconfig.
Changed since v2:
 - Omit "default n" from Kconfig patch.
 - Use conventional formatting for ESP sreg values in shost_printk().
 - Collect struct members relating to esp_send_pio_cmd().
---
 drivers/scsi/Kconfig |   5 +
 drivers/scsi/esp_scsi.c  | 128 +
 drivers/scsi/esp_scsi.h  |   6 +
 drivers/scsi/mac_esp.c   | 173 +
 drivers/scsi/zorro_esp.c | 232 ++-
 5 files changed, 179 insertions(+), 365 deletions(-)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 35c909bbf8ba..d2edf0f8beb7 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -42,6 +42,9 @@ config SCSI_DMA
bool
default n
 
+config SCSI_ESP_PIO
+   bool
+
 config SCSI_NETLINK
bool
default n
@@ -1355,6 +1358,7 @@ config SCSI_ZORRO_ESP
tristate "Zorro ESP SCSI support"
depends on ZORRO && SCSI
select SCSI_SPI_ATTRS
+   select SCSI_ESP_PIO
help
  Support for various NCR53C9x (ESP) based SCSI controllers on Zorro
  expansion boards for the Amiga.
@@ -1397,6 +1401,7 @@ config SCSI_MAC_ESP
tristate "Macintosh NCR53c9[46] SCSI"
depends on MAC && SCSI
select SCSI_SPI_ATTRS
+   select SCSI_ESP_PIO
help
  This is the NCR 53c9x SCSI controller found on most of the 68040
  based Macintoshes.
diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 989ed594451d..0f0d486be486 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2771,3 +2771,131 @@ MODULE_PARM_DESC(esp_debug,
 
 module_init(esp_init);
 module_exit(esp_exit);
+
+#ifdef CONFIG_SCSI_ESP_PIO
+static inline unsigned int esp_wait_for_fifo(struct esp *esp)
+{
+   int i = 50;
+
+   do {
+   unsigned int fbytes = esp_read8(ESP_FFLAGS) & ESP_FF_FBYTES;
+
+   if (fbytes)
+   return fbytes;
+
+   udelay(2);
+   } while (--i);
+
+   shost_printk(KERN_ERR, esp->host, "FIFO is empty. sreg [%02x]\n",
+esp_read8(ESP_STATUS));
+   return 0;
+}
+
+static inline int esp_wait_for_intr(struct esp *esp)
+{
+   int i = 50;
+
+   do {
+   esp->sreg = esp_read8(ESP_STATUS);
+   if (esp->sreg & ESP_STAT_INTR)
+   return 0;
+
+   udelay(2);
+   } while (--i);
+
+   shost_printk(KERN_ERR, esp->host, "IRQ timeout. sreg [%02x]\n",
+esp->sreg);
+   return 1;
+}
+
+#define ESP_FIFO_SIZE 16
+
+void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
+ u32 dma_count, int write, u8 cmd)
+{
+   u8 phase = esp->sreg & ESP_STAT_PMASK;
+
+   cmd &= ~ESP_CMD_DMA;
+   esp->send_cmd_error = 0;
+
+   if (write) {
+   u8 *dst = (u8 *)addr;
+   u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : ESP_INTR_BSERV);
+
+   scsi_esp_cmd(esp, cmd);
+
+   while (1) {
+   if (!esp_wait_for_fifo(esp))
+   break;
+
+   *dst++ = esp_read8(ESP_FDATA);
+   --esp_count;
+
+   if (!esp_count)
+   break;
+
+   if (esp_wait_for_intr(esp)) {
+   esp->send_cmd_error = 1;
+   break;
+   }
+
+   if ((esp->sreg & ESP_STAT_PMASK) != phase)
+   break;
+
+   esp->ireg = esp_read8(ESP_INTRPT);
+   if (esp->ireg & mask) {
+   esp->send_cmd_error = 1;
+   break;
+   }
+
+   if (phase == ESP_MIP)
+   scsi_esp_cmd(esp, ESP_CMD_MOK);
+
+   scsi_esp_cmd(esp, ESP_CMD_TI);
+   }
+   } else {
+   unsigned int n = ESP_FIFO_SIZE;
+   u8 *src = (u8 *)addr;
+
+   scsi_esp_cmd(esp, ESP_CMD_FLUSH);
+
+   if (n > esp_count)
+   n = esp_count;
+   writesb(esp->fifo_reg, src, n);
+   src += n;
+

[PATCH v3 3/6] esp_scsi: Grant disconnect privilege for untagged commands

[Finn Thain]

A SCSI device is not granted disconnect privilege by an esp_scsi host
unless that device has its simple_tags flag set. However, a device may
support disconnect/reselect and not support command queueing. Allow
such devices to disconnect and thereby improve bus utilization.

Drop the redundant 'lp' check. The mid-layer invokes .slave_alloc and
.slave_destroy in such a way that we may rely on scmd->device->hostdata
for as long as scmd belongs to the low-level driver.

Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
Tested-by: Michael Schmitz 
---
Changed since v2:
 - Drop redundant 'lp' check.
---
 drivers/scsi/esp_scsi.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 9e5d3f7d29ae..5b24aa21a797 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -717,7 +717,6 @@ static struct esp_cmd_entry 
*find_and_prep_issuable_command(struct esp *esp)
 static void esp_maybe_execute_command(struct esp *esp)
 {
struct esp_target_data *tp;
-   struct esp_lun_data *lp;
struct scsi_device *dev;
struct scsi_cmnd *cmd;
struct esp_cmd_entry *ent;
@@ -743,7 +742,6 @@ static void esp_maybe_execute_command(struct esp *esp)
tgt = dev->id;
lun = dev->lun;
tp = &esp->target[tgt];
-   lp = dev->hostdata;
 
list_move(&ent->list, &esp->active_cmds);
 
@@ -799,14 +797,7 @@ static void esp_maybe_execute_command(struct esp *esp)
}
 
 build_identify:
-   /* If we don't have a lun-data struct yet, we're probing
-* so do not disconnect.  Also, do not disconnect unless
-* we have a tag on this command.
-*/
-   if (lp && (tp->flags & ESP_TGT_DISCONNECT) && ent->tag[0])
-   *p++ = IDENTIFY(1, lun);
-   else
-   *p++ = IDENTIFY(0, lun);
+   *p++ = IDENTIFY(tp->flags & ESP_TGT_DISCONNECT, lun);
 
if (ent->tag[0] && esp->rev == ESP100) {
/* ESP100 lacks select w/atn3 command, use select
-- 
2.18.1

[PATCH v3 2/6] esp_scsi: Track residual for PIO transfers

[Finn Thain]

If a target disconnects during a PIO data transfer the command may
fail when the target reconnects:

scsi host1: DMA length is zero!
scsi host1: cur adr[0438] len[]

The scsi bus is then reset. This happens because the residual reached
zero before the transfer was completed.

The usual residual calculation relies on the Transfer Count registers.
That works for DMA transfers but not for PIO transfers. Fix the problem
by storing the PIO transfer residual and using that to correctly
calculate bytes_sent.

Fixes: 6fe07aaffbf0 ("[SCSI] m68k: new mac_esp scsi driver")
Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
Tested-by: Michael Schmitz 
---
Changed since v2:
 - Declare send_cmd_residual as u32 instead of int.
---
 drivers/scsi/esp_scsi.c | 1 +
 drivers/scsi/esp_scsi.h | 2 ++
 drivers/scsi/mac_esp.c  | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index c3fc34b9964d..9e5d3f7d29ae 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -1338,6 +1338,7 @@ static int esp_data_bytes_sent(struct esp *esp, struct 
esp_cmd_entry *ent,
 
bytes_sent = esp->data_dma_len;
bytes_sent -= ecount;
+   bytes_sent -= esp->send_cmd_residual;
 
/*
 * The am53c974 has a DMA 'pecularity'. The doc states:
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index 8163dca2071b..a2777a30 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -540,6 +540,8 @@ struct esp {
 
void*dma;
int dmarev;
+
+   u32 send_cmd_residual;
 };
 
 /* A front-end driver for the ESP chip should do the following in
diff --git a/drivers/scsi/mac_esp.c b/drivers/scsi/mac_esp.c
index eb551f3cc471..71879f2207e0 100644
--- a/drivers/scsi/mac_esp.c
+++ b/drivers/scsi/mac_esp.c
@@ -427,6 +427,8 @@ static void mac_esp_send_pio_cmd(struct esp *esp, u32 addr, 
u32 esp_count,
scsi_esp_cmd(esp, ESP_CMD_TI);
}
}
+
+   esp->send_cmd_residual = esp_count;
 }
 
 static int mac_esp_irq_pending(struct esp *esp)
-- 
2.18.1

[PATCH v3 6/6] esp_scsi: Optimize PIO loops

[Finn Thain]

Avoid function calls in the inner PIO loops. On a Centris 660av this
improves throughput for sequential read transfers by about 40% and
sequential write by about 10%.

Unfortunately it is not possible to have methods like .esp_write8
placed inline so this is always going to be slow, even with LTO.

Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
Tested-by: Michael Schmitz 
---
Changed since v1:
 - Don't touch the scsi_esp_cmd(esp, ESP_CMD_FLUSH) as it's outside the loop.
---
 drivers/scsi/esp_scsi.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 0f0d486be486..c3077c064522 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2783,7 +2783,7 @@ static inline unsigned int esp_wait_for_fifo(struct esp 
*esp)
if (fbytes)
return fbytes;
 
-   udelay(2);
+   udelay(1);
} while (--i);
 
shost_printk(KERN_ERR, esp->host, "FIFO is empty. sreg [%02x]\n",
@@ -2800,7 +2800,7 @@ static inline int esp_wait_for_intr(struct esp *esp)
if (esp->sreg & ESP_STAT_INTR)
return 0;
 
-   udelay(2);
+   udelay(1);
} while (--i);
 
shost_printk(KERN_ERR, esp->host, "IRQ timeout. sreg [%02x]\n",
@@ -2828,7 +2828,7 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 
esp_count,
if (!esp_wait_for_fifo(esp))
break;
 
-   *dst++ = esp_read8(ESP_FDATA);
+   *dst++ = readb(esp->fifo_reg);
--esp_count;
 
if (!esp_count)
@@ -2849,9 +2849,9 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 
esp_count,
}
 
if (phase == ESP_MIP)
-   scsi_esp_cmd(esp, ESP_CMD_MOK);
+   esp_write8(ESP_CMD_MOK, ESP_CMD);
 
-   scsi_esp_cmd(esp, ESP_CMD_TI);
+   esp_write8(ESP_CMD_TI, ESP_CMD);
}
} else {
unsigned int n = ESP_FIFO_SIZE;
@@ -2891,7 +2891,7 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 
esp_count,
src += n;
esp_count -= n;
 
-   scsi_esp_cmd(esp, ESP_CMD_TI);
+   esp_write8(ESP_CMD_TI, ESP_CMD);
}
}
 
-- 
2.18.1

[PATCH v3 4/6] esp_scsi: Eliminate ESP_FLAG_DOING_SLOWCMD

[Finn Thain]

The concept of a 'slow command' as it appears in esp_scsi is confusing
because it could refer to an ESP command or a SCSI command. It turns out
that it refers to a particular ESP select command which the driver also
tracks as 'ESP_SELECT_MSGOUT'. For readability, it is better to use the
terminology from the datasheets.

The global ESP_FLAG_DOING_SLOWCMD flag is redundant anyway, as it can be
inferred from esp->select_state. Remove the ESP_FLAG_DOING_SLOWCMD cruft
and just use a boolean local variable.

Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
Tested-by: Michael Schmitz 
---
 drivers/scsi/esp_scsi.c | 57 +
 drivers/scsi/esp_scsi.h |  3 +--
 2 files changed, 24 insertions(+), 36 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 5b24aa21a797..989ed594451d 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -478,17 +478,6 @@ static void esp_restore_pointers(struct esp *esp, struct 
esp_cmd_entry *ent)
spriv->tot_residue = ent->saved_tot_residue;
 }
 
-static void esp_check_command_len(struct esp *esp, struct scsi_cmnd *cmd)
-{
-   if (cmd->cmd_len == 6 ||
-   cmd->cmd_len == 10 ||
-   cmd->cmd_len == 12) {
-   esp->flags &= ~ESP_FLAG_DOING_SLOWCMD;
-   } else {
-   esp->flags |= ESP_FLAG_DOING_SLOWCMD;
-   }
-}
-
 static void esp_write_tgt_config3(struct esp *esp, int tgt)
 {
if (esp->rev > ESP100A) {
@@ -720,6 +709,7 @@ static void esp_maybe_execute_command(struct esp *esp)
struct scsi_device *dev;
struct scsi_cmnd *cmd;
struct esp_cmd_entry *ent;
+   bool select_and_stop = false;
int tgt, lun, i;
u32 val, start_cmd;
u8 *p;
@@ -750,7 +740,8 @@ static void esp_maybe_execute_command(struct esp *esp)
esp_map_dma(esp, cmd);
esp_save_pointers(esp, ent);
 
-   esp_check_command_len(esp, cmd);
+   if (!(cmd->cmd_len == 6 || cmd->cmd_len == 10 || cmd->cmd_len == 12))
+   select_and_stop = true;
 
p = esp->command_block;
 
@@ -791,9 +782,9 @@ static void esp_maybe_execute_command(struct esp *esp)
tp->flags &= ~ESP_TGT_CHECK_NEGO;
}
 
-   /* Process it like a slow command.  */
-   if (tp->flags & (ESP_TGT_NEGO_WIDE | ESP_TGT_NEGO_SYNC))
-   esp->flags |= ESP_FLAG_DOING_SLOWCMD;
+   /* If there are multiple message bytes, use Select and Stop */
+   if (esp->msg_out_len)
+   select_and_stop = true;
}
 
 build_identify:
@@ -803,23 +794,10 @@ static void esp_maybe_execute_command(struct esp *esp)
/* ESP100 lacks select w/atn3 command, use select
 * and stop instead.
 */
-   esp->flags |= ESP_FLAG_DOING_SLOWCMD;
+   select_and_stop = true;
}
 
-   if (!(esp->flags & ESP_FLAG_DOING_SLOWCMD)) {
-   start_cmd = ESP_CMD_SELA;
-   if (ent->tag[0]) {
-   *p++ = ent->tag[0];
-   *p++ = ent->tag[1];
-
-   start_cmd = ESP_CMD_SA3;
-   }
-
-   for (i = 0; i < cmd->cmd_len; i++)
-   *p++ = cmd->cmnd[i];
-
-   esp->select_state = ESP_SELECT_BASIC;
-   } else {
+   if (select_and_stop) {
esp->cmd_bytes_left = cmd->cmd_len;
esp->cmd_bytes_ptr = &cmd->cmnd[0];
 
@@ -834,6 +812,19 @@ static void esp_maybe_execute_command(struct esp *esp)
 
start_cmd = ESP_CMD_SELAS;
esp->select_state = ESP_SELECT_MSGOUT;
+   } else {
+   start_cmd = ESP_CMD_SELA;
+   if (ent->tag[0]) {
+   *p++ = ent->tag[0];
+   *p++ = ent->tag[1];
+
+   start_cmd = ESP_CMD_SA3;
+   }
+
+   for (i = 0; i < cmd->cmd_len; i++)
+   *p++ = cmd->cmnd[i];
+
+   esp->select_state = ESP_SELECT_BASIC;
}
val = tgt;
if (esp->rev == FASHME)
@@ -1243,7 +1234,6 @@ static int esp_finish_select(struct esp *esp)
esp_unmap_dma(esp, cmd);
esp_free_lun_tag(ent, cmd->device->hostdata);
tp->flags &= ~(ESP_TGT_NEGO_SYNC | ESP_TGT_NEGO_WIDE);
-   esp->flags &= ~ESP_FLAG_DOING_SLOWCMD;
esp->cmd_bytes_ptr = NULL;
esp->cmd_bytes_left = 0;
} else {
@@ -1294,9 +1284,8 @@ static int esp_finish_select(struct esp *esp)
esp_flush_fifo(esp);
}
 
-   /* If we 

Re: [PATCH v2 5/6] esp_scsi: De-duplicate PIO routines

[Finn Thain]

On Mon, 15 Oct 2018, Hannes Reinecke wrote:

> 
> However, the function declaration really is a worry, as the actual function
> body only exists when the config option is enabled.
> So either add a dummy function or surround the function declaration by
> CONFIG_ESP_PIO.
> Otherwise I think Dan Carpenter and the likes are guaranteed to send you a
> nice mail complaining about this ...
> 

Perhaps I've misunderstood your concern here. Is it a problem that 
esp_scsi.ko may or may not export the function, depending on .config?

For example, if esp_scsi.ko came from a build with CONFIG_SUN3X_ESP=y && 
!CONFIG_SCSI_ZORRO_ESP && !CONFIG_SCSI_MAC_ESP, then it would export no 
esp_send_pio_cmd() symbol.

A dummy function (mentioned above) might then avoid a link error from 
"modprobe zorro_esp" or "modprobe mac_esp" in this scenario. (The modules 
would load but fail to work properly.)

This seems a bit too contrived so I'll post v3 for you to consider.

-- 

Re: [PATCH v2 5/6] esp_scsi: De-duplicate PIO routines

[Finn Thain]

On Mon, 15 Oct 2018, Hannes Reinecke wrote:

> > 
> > In the case of send_cmd_residual, that would mean a second #ifdef 
> > added to esp_data_bytes_sent() where it gets used. I'm happy to comply 
> > but I fear that all these #ifdefs may harm readability...
> > 
> > There are already other variables in struct esp that may go unused, 
> > such as dma_regs, that don't have #ifdefs to elide them. Are these 
> > also problematic in some way?
> > 
> The unused fields in the struct are not so much an issue; in fact, it 
> rather complicated things when having individual fields in the struct 
> surrounded by CONFIG_XXX, as then the order of the fields would change 
> depending on the configuration. Which makes it really hard to debug ..
> 

True enough. We agree that this #ifdef is undesirable. And yet when I 
tried it, I found an unexpected readability benefit to your suggestion:

#ifdef CONFIG_SCSI_ESP_PIO
u8 __iomem  *fifo_reg;
int send_cmd_error;
u32 send_cmd_residual;
#endif

This grouping does help convey the purpose of these struct members, even 
though the #ifdef is meant for the compiler not for the human reader.

So maybe it makes sense to group these definitions (they are all the same 
size):

/* These are used by esp_scsi_send_pio_cmd() */
u8 __iomem  *fifo_reg;
int send_cmd_error;
u32 send_cmd_residual;

> However, the function declaration really is a worry, as the actual 
> function body only exists when the config option is enabled. So either 
> add a dummy function or surround the function declaration by 
> CONFIG_ESP_PIO.
> Otherwise I think Dan Carpenter and the likes are guaranteed to send you 
> a nice mail complaining about this ...
> 

Do static checkers really complain about this? I think the validity of an 
extern can't be known until the final linkage is done.

At that point the checker may complain that no compilation unit references 
a symbol in a header.

But this would lead to false positives where a header file is shared by 
separate programs which share library code but not macros.

-- 

> Cheers,
> 
> Hannes
> 

Re: [PATCH v2 5/6] esp_scsi: De-duplicate PIO routines

[Finn Thain]

On Mon, 15 Oct 2018, Hannes Reinecke wrote:

> On 10/14/18 8:12 AM, Finn Thain wrote:
> > As a temporary measure, the code to implement PIO transfers was
> > duplicated in zorro_esp and mac_esp. Now that it has stabilized
> > move the common code into the core driver but don't build it unless
> > needed.
> > 
> > This replaces the inline assembler with more portable writesb() calls.
> > Optimizing the m68k writesb() implementation is a separate patch.
> > 
> > Tested-by: Stan Johnson 
> > Signed-off-by: Finn Thain 
> > Tested-by: Michael Schmitz 
> > ---
> > Changed since v1:
> >   - Use shost_printk() instead of pr_err().
> >   - Add new symbol CONFIG_SCSI_ESP_PIO to Kconfig.
> > ---
> >   drivers/scsi/Kconfig |   6 +
> >   drivers/scsi/esp_scsi.c  | 128 +
> >   drivers/scsi/esp_scsi.h  |   5 +
> >   drivers/scsi/mac_esp.c   | 173 +
> >   drivers/scsi/zorro_esp.c | 232 ++-
> >   5 files changed, 179 insertions(+), 365 deletions(-)
> > 
> > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> > index 35c909bbf8ba..81c6872f24f8 100644
> > --- a/drivers/scsi/Kconfig
> > +++ b/drivers/scsi/Kconfig
> > @@ -42,6 +42,10 @@ config SCSI_DMA
> > bool
> > default n
> >   +config SCSI_ESP_PIO
> > +   bool
> > +   default n
> > +
> >   config SCSI_NETLINK
> > bool
> > default n
> > @@ -1355,6 +1359,7 @@ config SCSI_ZORRO_ESP
> > tristate "Zorro ESP SCSI support"
> > depends on ZORRO && SCSI
> > select SCSI_SPI_ATTRS
> > +   select SCSI_ESP_PIO
> > help
> >   Support for various NCR53C9x (ESP) based SCSI controllers on Zorro
> >   expansion boards for the Amiga.
> > @@ -1397,6 +1402,7 @@ config SCSI_MAC_ESP
> > tristate "Macintosh NCR53c9[46] SCSI"
> > depends on MAC && SCSI
> > select SCSI_SPI_ATTRS
> > +   select SCSI_ESP_PIO
> > help
> >   This is the NCR 53c9x SCSI controller found on most of the 68040
> >   based Macintoshes.
> > diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> > index 6ccaf818357e..305a322ad13c 100644
> > --- a/drivers/scsi/esp_scsi.c
> > +++ b/drivers/scsi/esp_scsi.c
> > @@ -2776,3 +2776,131 @@ MODULE_PARM_DESC(esp_debug,
> > module_init(esp_init);
> >   module_exit(esp_exit);
> > +
> > +#ifdef CONFIG_SCSI_ESP_PIO
> > +static inline unsigned int esp_wait_for_fifo(struct esp *esp)
> > +{
> > +   int i = 50;
> > +
> > +   do {
> > +   unsigned int fbytes = esp_read8(ESP_FFLAGS) & ESP_FF_FBYTES;
> > +
> > +   if (fbytes)
> > +   return fbytes;
> > +
> > +   udelay(2);
> > +   } while (--i);
> > +
> > +   shost_printk(KERN_ERR, esp->host, "FIFO is empty (sreg %02x)\n",
> > +esp_read8(ESP_STATUS));
> > +   return 0;
> > +}
> > +
> > +static inline int esp_wait_for_intr(struct esp *esp)
> > +{
> > +   int i = 50;
> > +
> > +   do {
> > +   esp->sreg = esp_read8(ESP_STATUS);
> > +   if (esp->sreg & ESP_STAT_INTR)
> > +   return 0;
> > +
> > +   udelay(2);
> > +   } while (--i);
> > +
> > +   shost_printk(KERN_ERR, esp->host, "IRQ timeout (sreg %02x)\n",
> > +esp->sreg);
> > +   return 1;
> > +}
> > +
> > +#define ESP_FIFO_SIZE 16
> > +
> > +void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
> > + u32 dma_count, int write, u8 cmd)
> > +{
> > +   u8 phase = esp->sreg & ESP_STAT_PMASK;
> > +
> > +   cmd &= ~ESP_CMD_DMA;
> > +   esp->send_cmd_error = 0;
> > +
> > +   if (write) {
> > +   u8 *dst = (u8 *)addr;
> > +   u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE :
> > ESP_INTR_BSERV);
> > +
> > +   scsi_esp_cmd(esp, cmd);
> > +
> > +   while (1) {
> > +   if (!esp_wait_for_fifo(esp))
> > +   break;
> > +
> > +   *dst++ = esp_read8(ESP_FDATA);
> > +   --esp_count;
> > +
> > +   if (!esp_count)
> > +   break;
> > +
> > +   if (esp_wait_for_intr(esp)) {
> > +   

Re: [PATCH 5/6] esp_scsi: De-duplicate PIO routines

[Finn Thain]

On Mon, 15 Oct 2018, Hannes Reinecke wrote:

> On 10/13/18 2:51 AM, Finn Thain wrote:
> > As a temporary measure, the code to implement PIO transfers was
> > duplicated in zorro_esp and mac_esp. Now that this code has stabilized,
> > move it into the core driver to eliminate the duplication.
> > 
> > This replaces the inline assembler with more portable writesb() calls.
> > Optimizing the m68k writesb() implementation is a separate patch.
> > 
> > Tested-by: Stan Johnson 
> > Signed-off-by: Finn Thain 
> > ---
> >   drivers/scsi/esp_scsi.c  | 126 +
> >   drivers/scsi/esp_scsi.h  |   5 +
> >   drivers/scsi/mac_esp.c   | 173 ++-
> >   drivers/scsi/zorro_esp.c | 232 
> > +++
> >   4 files changed, 171 insertions(+), 365 deletions(-)
> > 
> > diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> > index 6ccaf818357e..646701fc22a4 100644
> > --- a/drivers/scsi/esp_scsi.c
> > +++ b/drivers/scsi/esp_scsi.c
> > @@ -2776,3 +2776,129 @@ MODULE_PARM_DESC(esp_debug,
> >   
> >   module_init(esp_init);
> >   module_exit(esp_exit);
> > +
> > +#if IS_ENABLED(CONFIG_SCSI_MAC_ESP) || IS_ENABLED(CONFIG_SCSI_ZORRO_ESP)
> > +static inline unsigned int esp_wait_for_fifo(struct esp *esp)
> > +{
> > +   int i = 50;
> > +
> > +   do {
> > +   unsigned int fbytes = esp_read8(ESP_FFLAGS) & ESP_FF_FBYTES;
> > +
> > +   if (fbytes)
> > +   return fbytes;
> > +
> > +   udelay(2);
> > +   } while (--i);
> > +
> > +   pr_err("FIFO is empty (sreg %02x)\n", esp_read8(ESP_STATUS));
> > +   return 0;
> > +}
> > +
> > +static inline int esp_wait_for_intr(struct esp *esp)
> > +{
> > +   int i = 50;
> > +
> > +   do {
> > +   esp->sreg = esp_read8(ESP_STATUS);
> > +   if (esp->sreg & ESP_STAT_INTR)
> > +   return 0;
> > +
> > +   udelay(2);
> > +   } while (--i);
> > +
> > +   pr_err("IRQ timeout (sreg %02x)\n", esp->sreg);
> > +   return 1;
> > +}
> > +
> > +#define ESP_FIFO_SIZE 16
> > +
> > +void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
> > + u32 dma_count, int write, u8 cmd)
> > +{
> > +   u8 phase = esp->sreg & ESP_STAT_PMASK;
> > +
> > +   cmd &= ~ESP_CMD_DMA;
> > +   esp->send_cmd_error = 0;
> > +
> > +   if (write) {
> > +   u8 *dst = (u8 *)addr;
> > +   u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : ESP_INTR_BSERV);
> > +
> > +   scsi_esp_cmd(esp, cmd);
> > +
> > +   while (1) {
> > +   if (!esp_wait_for_fifo(esp))
> > +   break;
> > +
> > +   *dst++ = esp_read8(ESP_FDATA);
> > +   --esp_count;
> > +
> > +   if (!esp_count)
> > +   break;
> > +
> > +   if (esp_wait_for_intr(esp)) {
> > +   esp->send_cmd_error = 1;
> > +   break;
> > +   }
> > +
> > +   if ((esp->sreg & ESP_STAT_PMASK) != phase)
> > +   break;
> > +
> > +   esp->ireg = esp_read8(ESP_INTRPT);
> > +   if (esp->ireg & mask) {
> > +   esp->send_cmd_error = 1;
> > +   break;
> > +   }
> > +
> > +   if (phase == ESP_MIP)
> > +   scsi_esp_cmd(esp, ESP_CMD_MOK);
> > +
> > +   scsi_esp_cmd(esp, ESP_CMD_TI);
> > +   }
> > +   } else {
> > +   unsigned int n = ESP_FIFO_SIZE;
> > +   u8 *src = (u8 *)addr;
> > +
> > +   scsi_esp_cmd(esp, ESP_CMD_FLUSH);
> > +
> > +   if (n > esp_count)
> > +   n = esp_count;
> > +   writesb(esp->fifo_reg, src, n);
> > +   src += n;
> > +   esp_count -= n;
> > +
> > +   scsi_esp_cmd(esp, cmd);
> > +
> > +   while (esp_count) {
> > +   if (esp_wait_for_intr(esp)) {
> > +   esp->send_cmd_error = 1;
> > +   break;
> > +   

Re: [PATCH v2 3/6] esp_scsi: Grant disconnect privilege for untagged commands

[Finn Thain]

On Mon, 15 Oct 2018, Finn Thain wrote:

> On Sun, 14 Oct 2018, Christoph Hellwig wrote:
> 
> > > + *p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);
> > 
> > I think lp should always be non-NULL here.
> > 
> 
> It's not clear from Documentation/scsi/scsi_mid_low_api.txt, but I guess 
> that's true for scanning of targets.
> 
> Is it true in general that queuecommand for a device will not occur 
> before its slave_alloc and not after its slave_destroy invocation?
> 
> (I'm thinking of queuecommand via the other command submission paths, 
> like ioctl.)
> 

Nevermind. From a closer reading of the Documentation, I see that it is 
true in general.

-- 

Re: [PATCH v2 5/6] esp_scsi: De-duplicate PIO routines

[Finn Thain]

On Sun, 14 Oct 2018, Geert Uytterhoeven wrote:

> 
> > --- a/drivers/scsi/Kconfig
> > +++ b/drivers/scsi/Kconfig
> > @@ -42,6 +42,10 @@ config SCSI_DMA
> > bool
> > default n
> >
> > +config SCSI_ESP_PIO
> > +   bool
> > +   default n
> 
> "default n" is the default, so please drop this line.
> 

Done. Thanks.

-- 

Re: [PATCH v2 3/6] esp_scsi: Grant disconnect privilege for untagged commands

[Finn Thain]

On Sun, 14 Oct 2018, Christoph Hellwig wrote:

> > +   *p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);
> 
> I think lp should always be non-NULL here.
> 

It's not clear from Documentation/scsi/scsi_mid_low_api.txt, but I guess 
that's true for scanning of targets.

Is it true in general that queuecommand for a device will not occur before 
its slave_alloc and not after its slave_destroy invocation?

(I'm thinking of queuecommand via the other command submission paths, like 
ioctl.)

-- 

Re: [PATCH v2 3/6] esp_scsi: Grant disconnect privilege for untagged commands

[Finn Thain]

On Mon, 15 Oct 2018, Michael Schmitz wrote:

> 
> Do we really need to make that distinction?
> 

esp_reconnect() dereferences lp, so !lp has to inhibit disconnection.

At least, that's my reading. I can't see any other reason for the lp 
conditional.

-- 

Re: [PATCH v2 2/6] esp_scsi: Track residual for PIO transfers

[Finn Thain]

On Sun, 14 Oct 2018, Geert Uytterhoeven wrote:

> 
> > Fixes: 6fe07aaffbf0
> 
> Fixes: 6fe07aaffbf0 ("[SCSI] m68k: new mac_esp scsi driver")
> 

Fixed.

> > Tested-by: Stan Johnson 
> > Signed-off-by: Finn Thain 
> > Tested-by: Michael Schmitz 
> 
> > --- a/drivers/scsi/esp_scsi.h
> > +++ b/drivers/scsi/esp_scsi.h
> > @@ -540,6 +540,8 @@ struct esp {
> >
> > void*dma;
> > int dmarev;
> > +
> > +   int send_cmd_residual;
> 
> unsigned int?
> 

My first thought was to use u32, same as esp_count. But it turns out that 
the end result really is an int --

static int esp_data_bytes_sent(struct esp *esp, struct esp_cmd_entry *ent,
   struct scsi_cmnd *cmd)
{
int fifo_cnt, ecount, bytes_sent, flush_fifo;

...
bytes_sent = esp->data_dma_len;
bytes_sent -= ecount;
bytes_sent -= esp->send_cmd_residual;
...
return bytes_sent;
}

Apparently over/underflow is a real possibility, because there is a test 
for this in esp_process_event().

-- 

> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> 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
> 

Re: [PATCH v2 1/6] zorro_esp: Limit DMA transfers to 65535 bytes

[Finn Thain]

On Sun, 14 Oct 2018, Geert Uytterhoeven wrote:

> 
> > Fixes: 3109e5ae0311
> 
> Fixes: 3109e5ae0311 ("scsi: zorro_esp: New driver for Amiga Zorro
> NCR53C9x boards")
> 

OK.

> $ git help fixes
> 'fixes' is aliased to 'show --format='Fixes: %h ("%s")' -s'
> 
> BTW, if you use vim, add
> 
> noremap ;gs "zyiw:exe "new \| setlocal buftype=nofile
> bufhidden=hide noswapfile syntax=git \| 0r ! git show ".@z."" \|
> :0
> 
> to .vimrc, and type ";gs" when the cursor is positioned on a commit ID.
> 

Thanks.

> > Signed-off-by: Finn Thain 
> > Cc: Michael Schmitz 
> > Tested-by: Michael Schmitz 
> 
> > --- a/drivers/scsi/zorro_esp.c
> > +++ b/drivers/scsi/zorro_esp.c
> > @@ -245,7 +245,7 @@ static int fastlane_esp_irq_pending(struct esp *esp)
> >  static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
> > u32 dma_len)
> >  {
> > -   return dma_len > 0xFF ? 0xFF : dma_len;
> > +   return dma_len > 0x ? 0x : dma_len;
> >  }
> >
> >  static void zorro_esp_reset_dma(struct esp *esp)
> > @@ -484,7 +484,6 @@ static void zorro_esp_send_blz1230_dma_cmd(struct esp 
> > *esp, u32 addr,
> > scsi_esp_cmd(esp, ESP_CMD_DMA);
> > zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
> > zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
> > -   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
> 
> Since all these sub-drivers seem to support 24-bit transfers, perhaps this is
> something to be fixed in the esp_scsi core?
> 

I don't think there is anything to fix in the esp_scsi core. The fact that 
no-one saw the error indicates that large transfers don't occur in 
practice.

If you said there is an opportunity for an enhancement, I could agree, 
assuming that you also found a way to produce large IO requests. But I 
doubt that such an enhancement would make a measurable improvement.

Even if the benefit was measurable, the fix under review would still be 
needed for stable kernels.

BTW, Michael and I already discussed all this (it probably should have 
happened on the linux-m68k list).

Please see,

$ grep -lr ESP_CONFIG2_FENAB drivers/scsi/
drivers/scsi/am53c974.c
drivers/scsi/esp_scsi.c
drivers/scsi/esp_scsi.h

The authors of am53c974.c obviously decided it wasn't wise to make this 
feature mandatory (which suggests that perhaps it shouldn't go into common 
code).

The comments in esp_scsi.c say that ESP_CONFIG2_FENAB has undesirable 
consequences. There is information in the datasheets on this point but I 
didn't follow it up because I don't see a performance issue.

-- 

[PATCH v2 4/6] esp_scsi: Eliminate ESP_FLAG_DOING_SLOWCMD

[Finn Thain]

The concept of a 'slow command' as it appears in esp_scsi is confusing
because it could refer to an ESP command or a SCSI command. It turns out
that it refers to a particular ESP select command which the driver also
tracks as 'ESP_SELECT_MSGOUT'. For readability, it is better to use the
terminology from the datasheets.

The global ESP_FLAG_DOING_SLOWCMD flag is redundant anyway, as it can be
inferred from esp->select_state. Remove the ESP_FLAG_DOING_SLOWCMD cruft
and just use a boolean local variable.

Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
Tested-by: Michael Schmitz 
---
 drivers/scsi/esp_scsi.c | 57 +
 drivers/scsi/esp_scsi.h |  3 +--
 2 files changed, 24 insertions(+), 36 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index b7c41aa9927c..6ccaf818357e 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -478,17 +478,6 @@ static void esp_restore_pointers(struct esp *esp, struct 
esp_cmd_entry *ent)
spriv->tot_residue = ent->saved_tot_residue;
 }
 
-static void esp_check_command_len(struct esp *esp, struct scsi_cmnd *cmd)
-{
-   if (cmd->cmd_len == 6 ||
-   cmd->cmd_len == 10 ||
-   cmd->cmd_len == 12) {
-   esp->flags &= ~ESP_FLAG_DOING_SLOWCMD;
-   } else {
-   esp->flags |= ESP_FLAG_DOING_SLOWCMD;
-   }
-}
-
 static void esp_write_tgt_config3(struct esp *esp, int tgt)
 {
if (esp->rev > ESP100A) {
@@ -721,6 +710,7 @@ static void esp_maybe_execute_command(struct esp *esp)
struct scsi_device *dev;
struct scsi_cmnd *cmd;
struct esp_cmd_entry *ent;
+   bool select_and_stop = false;
int tgt, lun, i;
u32 val, start_cmd;
u8 *p;
@@ -752,7 +742,8 @@ static void esp_maybe_execute_command(struct esp *esp)
esp_map_dma(esp, cmd);
esp_save_pointers(esp, ent);
 
-   esp_check_command_len(esp, cmd);
+   if (!(cmd->cmd_len == 6 || cmd->cmd_len == 10 || cmd->cmd_len == 12))
+   select_and_stop = true;
 
p = esp->command_block;
 
@@ -793,9 +784,9 @@ static void esp_maybe_execute_command(struct esp *esp)
tp->flags &= ~ESP_TGT_CHECK_NEGO;
}
 
-   /* Process it like a slow command.  */
-   if (tp->flags & (ESP_TGT_NEGO_WIDE | ESP_TGT_NEGO_SYNC))
-   esp->flags |= ESP_FLAG_DOING_SLOWCMD;
+   /* If there are multiple message bytes, use Select and Stop */
+   if (esp->msg_out_len)
+   select_and_stop = true;
}
 
 build_identify:
@@ -808,23 +799,10 @@ static void esp_maybe_execute_command(struct esp *esp)
/* ESP100 lacks select w/atn3 command, use select
 * and stop instead.
 */
-   esp->flags |= ESP_FLAG_DOING_SLOWCMD;
+   select_and_stop = true;
}
 
-   if (!(esp->flags & ESP_FLAG_DOING_SLOWCMD)) {
-   start_cmd = ESP_CMD_SELA;
-   if (ent->tag[0]) {
-   *p++ = ent->tag[0];
-   *p++ = ent->tag[1];
-
-   start_cmd = ESP_CMD_SA3;
-   }
-
-   for (i = 0; i < cmd->cmd_len; i++)
-   *p++ = cmd->cmnd[i];
-
-   esp->select_state = ESP_SELECT_BASIC;
-   } else {
+   if (select_and_stop) {
esp->cmd_bytes_left = cmd->cmd_len;
esp->cmd_bytes_ptr = &cmd->cmnd[0];
 
@@ -839,6 +817,19 @@ static void esp_maybe_execute_command(struct esp *esp)
 
start_cmd = ESP_CMD_SELAS;
esp->select_state = ESP_SELECT_MSGOUT;
+   } else {
+   start_cmd = ESP_CMD_SELA;
+   if (ent->tag[0]) {
+   *p++ = ent->tag[0];
+   *p++ = ent->tag[1];
+
+   start_cmd = ESP_CMD_SA3;
+   }
+
+   for (i = 0; i < cmd->cmd_len; i++)
+   *p++ = cmd->cmnd[i];
+
+   esp->select_state = ESP_SELECT_BASIC;
}
val = tgt;
if (esp->rev == FASHME)
@@ -1248,7 +1239,6 @@ static int esp_finish_select(struct esp *esp)
esp_unmap_dma(esp, cmd);
esp_free_lun_tag(ent, cmd->device->hostdata);
tp->flags &= ~(ESP_TGT_NEGO_SYNC | ESP_TGT_NEGO_WIDE);
-   esp->flags &= ~ESP_FLAG_DOING_SLOWCMD;
esp->cmd_bytes_ptr = NULL;
esp->cmd_bytes_left = 0;
} else {
@@ -1299,9 +1289,8 @@ static int esp_finish_select(struct esp *esp)
esp_flush_fifo(esp);
}
 
-   /* If we 

[PATCH v2 0/6] mac_esp, zorro_esp, esp_scsi: Various improvements

[Finn Thain]

This series has fixes and cleanup for mac_esp, zorro_esp and the
core esp_scsi driver.

These improvements include elimination of duplicated code temporarily
introduced for zorro_esp.


Finn Thain (6):
  zorro_esp: Limit DMA transfers to 65535 bytes
  esp_scsi: Track residual for PIO transfers
  esp_scsi: Grant disconnect privilege for untagged commands
  esp_scsi: Eliminate ESP_FLAG_DOING_SLOWCMD
  esp_scsi: De-duplicate PIO routines
  esp_scsi: Optimize PIO loops

 drivers/scsi/Kconfig |   6 +
 drivers/scsi/esp_scsi.c  | 194 ---
 drivers/scsi/esp_scsi.h  |  10 +-
 drivers/scsi/mac_esp.c   | 171 +---
 drivers/scsi/zorro_esp.c | 240 ++-
 5 files changed, 209 insertions(+), 412 deletions(-)

-- 
2.18.1

[PATCH v2 3/6] esp_scsi: Grant disconnect privilege for untagged commands

[Finn Thain]

A SCSI device is not granted disconnect privilege by an esp_scsi host
unless that device has its simple_tags flag set. However, a device may
support disconnect/reselect and not support command queueing. Allow
these devices to disconnect and improve bus utilization.

Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
Tested-by: Michael Schmitz 
---
 drivers/scsi/esp_scsi.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 9e5d3f7d29ae..b7c41aa9927c 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -800,13 +800,9 @@ static void esp_maybe_execute_command(struct esp *esp)
 
 build_identify:
/* If we don't have a lun-data struct yet, we're probing
-* so do not disconnect.  Also, do not disconnect unless
-* we have a tag on this command.
+* so do not disconnect.
 */
-   if (lp && (tp->flags & ESP_TGT_DISCONNECT) && ent->tag[0])
-   *p++ = IDENTIFY(1, lun);
-   else
-   *p++ = IDENTIFY(0, lun);
+   *p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);
 
if (ent->tag[0] && esp->rev == ESP100) {
/* ESP100 lacks select w/atn3 command, use select
-- 
2.18.1

[PATCH v2 1/6] zorro_esp: Limit DMA transfers to 65535 bytes

[Finn Thain]

The core driver, esp_scsi, does not use the ESP_CONFIG2_FENAB bit, so
the chip's Transfer Counter register is only 16 bits wide (not 24).
A larger transfer cannot work and will theoretically result in a failed
command and a "DMA length is zero" error.

Fixes: 3109e5ae0311
Signed-off-by: Finn Thain 
Cc: Michael Schmitz 
Tested-by: Michael Schmitz 
---
 drivers/scsi/zorro_esp.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
index bb70882e6b56..be79127db594 100644
--- a/drivers/scsi/zorro_esp.c
+++ b/drivers/scsi/zorro_esp.c
@@ -245,7 +245,7 @@ static int fastlane_esp_irq_pending(struct esp *esp)
 static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
u32 dma_len)
 {
-   return dma_len > 0xFF ? 0xFF : dma_len;
+   return dma_len > 0x ? 0x : dma_len;
 }
 
 static void zorro_esp_reset_dma(struct esp *esp)
@@ -484,7 +484,6 @@ static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, 
u32 addr,
scsi_esp_cmd(esp, ESP_CMD_DMA);
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
scsi_esp_cmd(esp, cmd);
 }
@@ -529,7 +528,6 @@ static void zorro_esp_send_blz1230II_dma_cmd(struct esp 
*esp, u32 addr,
scsi_esp_cmd(esp, ESP_CMD_DMA);
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
scsi_esp_cmd(esp, cmd);
 }
@@ -574,7 +572,6 @@ static void zorro_esp_send_blz2060_dma_cmd(struct esp *esp, 
u32 addr,
scsi_esp_cmd(esp, ESP_CMD_DMA);
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
scsi_esp_cmd(esp, cmd);
 }
@@ -599,7 +596,6 @@ static void zorro_esp_send_cyber_dma_cmd(struct esp *esp, 
u32 addr,
 
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
if (write) {
/* DMA receive */
@@ -649,7 +645,6 @@ static void zorro_esp_send_cyberII_dma_cmd(struct esp *esp, 
u32 addr,
 
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
if (write) {
/* DMA receive */
@@ -691,7 +686,6 @@ static void zorro_esp_send_fastlane_dma_cmd(struct esp 
*esp, u32 addr,
 
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
if (write) {
/* DMA receive */
-- 
2.18.1

[PATCH v2 2/6] esp_scsi: Track residual for PIO transfers

[Finn Thain]

If a target disconnects during a PIO data transfer the command may
fail when the target reconnects:

scsi host1: DMA length is zero!
scsi host1: cur adr[0438] len[]

The scsi bus is then reset. This happens because the residual reached
zero before the transfer was completed.

The usual residual calculation relies on the Transfer Count registers
which works for DMA transfers but not for PIO transfers. Fix the problem
by storing the PIO transfer residual and using that to correctly
calculate bytes_sent.

Fixes: 6fe07aaffbf0
Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
Tested-by: Michael Schmitz 
---
 drivers/scsi/esp_scsi.c | 1 +
 drivers/scsi/esp_scsi.h | 2 ++
 drivers/scsi/mac_esp.c  | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index c3fc34b9964d..9e5d3f7d29ae 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -1338,6 +1338,7 @@ static int esp_data_bytes_sent(struct esp *esp, struct 
esp_cmd_entry *ent,
 
bytes_sent = esp->data_dma_len;
bytes_sent -= ecount;
+   bytes_sent -= esp->send_cmd_residual;
 
/*
 * The am53c974 has a DMA 'pecularity'. The doc states:
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index 8163dca2071b..db4b6ea94caa 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -540,6 +540,8 @@ struct esp {
 
void*dma;
int dmarev;
+
+   int send_cmd_residual;
 };
 
 /* A front-end driver for the ESP chip should do the following in
diff --git a/drivers/scsi/mac_esp.c b/drivers/scsi/mac_esp.c
index eb551f3cc471..71879f2207e0 100644
--- a/drivers/scsi/mac_esp.c
+++ b/drivers/scsi/mac_esp.c
@@ -427,6 +427,8 @@ static void mac_esp_send_pio_cmd(struct esp *esp, u32 addr, 
u32 esp_count,
scsi_esp_cmd(esp, ESP_CMD_TI);
}
}
+
+   esp->send_cmd_residual = esp_count;
 }
 
 static int mac_esp_irq_pending(struct esp *esp)
-- 
2.18.1

[PATCH v2 6/6] esp_scsi: Optimize PIO loops

[Finn Thain]

Avoid function calls in the inner PIO loops. On a Centris 660av this
improves throughput for sequential read transfers by about 40% and
sequential write by about 10%.

Unfortunately it is not possible to have method calls like esp_write8()
placed inline so this is always going to be slow (even with LTO).

Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
Tested-by: Michael Schmitz 
---
Changed since v1:
 - Don't touch the scsi_esp_cmd(esp, ESP_CMD_FLUSH) that's outside the loop.
---
 drivers/scsi/esp_scsi.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 305a322ad13c..cdf55bd8562e 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2788,7 +2788,7 @@ static inline unsigned int esp_wait_for_fifo(struct esp 
*esp)
if (fbytes)
return fbytes;
 
-   udelay(2);
+   udelay(1);
} while (--i);
 
shost_printk(KERN_ERR, esp->host, "FIFO is empty (sreg %02x)\n",
@@ -2805,7 +2805,7 @@ static inline int esp_wait_for_intr(struct esp *esp)
if (esp->sreg & ESP_STAT_INTR)
return 0;
 
-   udelay(2);
+   udelay(1);
} while (--i);
 
shost_printk(KERN_ERR, esp->host, "IRQ timeout (sreg %02x)\n",
@@ -2833,7 +2833,7 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 
esp_count,
if (!esp_wait_for_fifo(esp))
break;
 
-   *dst++ = esp_read8(ESP_FDATA);
+   *dst++ = readb(esp->fifo_reg);
--esp_count;
 
if (!esp_count)
@@ -2854,9 +2854,9 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 
esp_count,
}
 
if (phase == ESP_MIP)
-   scsi_esp_cmd(esp, ESP_CMD_MOK);
+   esp_write8(ESP_CMD_MOK, ESP_CMD);
 
-   scsi_esp_cmd(esp, ESP_CMD_TI);
+   esp_write8(ESP_CMD_TI, ESP_CMD);
}
} else {
unsigned int n = ESP_FIFO_SIZE;
@@ -2896,7 +2896,7 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 
esp_count,
src += n;
esp_count -= n;
 
-   scsi_esp_cmd(esp, ESP_CMD_TI);
+   esp_write8(ESP_CMD_TI, ESP_CMD);
}
}
 
-- 
2.18.1

[PATCH v2 5/6] esp_scsi: De-duplicate PIO routines

[Finn Thain]

As a temporary measure, the code to implement PIO transfers was
duplicated in zorro_esp and mac_esp. Now that it has stabilized
move the common code into the core driver but don't build it unless
needed.

This replaces the inline assembler with more portable writesb() calls.
Optimizing the m68k writesb() implementation is a separate patch.

Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
Tested-by: Michael Schmitz 
---
Changed since v1:
 - Use shost_printk() instead of pr_err().
 - Add new symbol CONFIG_SCSI_ESP_PIO to Kconfig.
---
 drivers/scsi/Kconfig |   6 +
 drivers/scsi/esp_scsi.c  | 128 +
 drivers/scsi/esp_scsi.h  |   5 +
 drivers/scsi/mac_esp.c   | 173 +
 drivers/scsi/zorro_esp.c | 232 ++-
 5 files changed, 179 insertions(+), 365 deletions(-)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 35c909bbf8ba..81c6872f24f8 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -42,6 +42,10 @@ config SCSI_DMA
bool
default n
 
+config SCSI_ESP_PIO
+   bool
+   default n
+
 config SCSI_NETLINK
bool
default n
@@ -1355,6 +1359,7 @@ config SCSI_ZORRO_ESP
tristate "Zorro ESP SCSI support"
depends on ZORRO && SCSI
select SCSI_SPI_ATTRS
+   select SCSI_ESP_PIO
help
  Support for various NCR53C9x (ESP) based SCSI controllers on Zorro
  expansion boards for the Amiga.
@@ -1397,6 +1402,7 @@ config SCSI_MAC_ESP
tristate "Macintosh NCR53c9[46] SCSI"
depends on MAC && SCSI
select SCSI_SPI_ATTRS
+   select SCSI_ESP_PIO
help
  This is the NCR 53c9x SCSI controller found on most of the 68040
  based Macintoshes.
diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 6ccaf818357e..305a322ad13c 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2776,3 +2776,131 @@ MODULE_PARM_DESC(esp_debug,
 
 module_init(esp_init);
 module_exit(esp_exit);
+
+#ifdef CONFIG_SCSI_ESP_PIO
+static inline unsigned int esp_wait_for_fifo(struct esp *esp)
+{
+   int i = 50;
+
+   do {
+   unsigned int fbytes = esp_read8(ESP_FFLAGS) & ESP_FF_FBYTES;
+
+   if (fbytes)
+   return fbytes;
+
+   udelay(2);
+   } while (--i);
+
+   shost_printk(KERN_ERR, esp->host, "FIFO is empty (sreg %02x)\n",
+esp_read8(ESP_STATUS));
+   return 0;
+}
+
+static inline int esp_wait_for_intr(struct esp *esp)
+{
+   int i = 50;
+
+   do {
+   esp->sreg = esp_read8(ESP_STATUS);
+   if (esp->sreg & ESP_STAT_INTR)
+   return 0;
+
+   udelay(2);
+   } while (--i);
+
+   shost_printk(KERN_ERR, esp->host, "IRQ timeout (sreg %02x)\n",
+esp->sreg);
+   return 1;
+}
+
+#define ESP_FIFO_SIZE 16
+
+void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
+ u32 dma_count, int write, u8 cmd)
+{
+   u8 phase = esp->sreg & ESP_STAT_PMASK;
+
+   cmd &= ~ESP_CMD_DMA;
+   esp->send_cmd_error = 0;
+
+   if (write) {
+   u8 *dst = (u8 *)addr;
+   u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : ESP_INTR_BSERV);
+
+   scsi_esp_cmd(esp, cmd);
+
+   while (1) {
+   if (!esp_wait_for_fifo(esp))
+   break;
+
+   *dst++ = esp_read8(ESP_FDATA);
+   --esp_count;
+
+   if (!esp_count)
+   break;
+
+   if (esp_wait_for_intr(esp)) {
+   esp->send_cmd_error = 1;
+   break;
+   }
+
+   if ((esp->sreg & ESP_STAT_PMASK) != phase)
+   break;
+
+   esp->ireg = esp_read8(ESP_INTRPT);
+   if (esp->ireg & mask) {
+   esp->send_cmd_error = 1;
+   break;
+   }
+
+   if (phase == ESP_MIP)
+   scsi_esp_cmd(esp, ESP_CMD_MOK);
+
+   scsi_esp_cmd(esp, ESP_CMD_TI);
+   }
+   } else {
+   unsigned int n = ESP_FIFO_SIZE;
+   u8 *src = (u8 *)addr;
+
+   scsi_esp_cmd(esp, ESP_CMD_FLUSH);
+
+   if (n > esp_count)
+   n = esp_count;
+   writesb(esp->fifo_reg, src, n);
+   src += n;
+   esp_count -= n;
+
+   scsi_esp_cmd(esp, cmd);
+
+   while (esp_count) {
+

Re: various esp_scsi cleanups V3

[Finn Thain]

On Sat, 13 Oct 2018, Christoph Hellwig wrote:

> Mostly to avoid methods calls for dma mapping, but also to tidy up
> a few bits found while doing that.
> 
> Changes since v2:
>  - add back required DMA_NONE check
>  - also clear sense_ptr for PIO transfers
>  - remove the pointless union in struct esp_cmd_priv
> 
> Changes since v1:
>  - fix a sun_esp compiler failure in an intermediate patch
>  - drop the dev argument to scsi_esp_register
> 

Tested-by: Finn Thain 

Thanks.

-- 

Re: [PATCH 5/6] esp_scsi: De-duplicate PIO routines

[Finn Thain]

On Sat, 13 Oct 2018, Christoph Hellwig wrote:

> > +#if IS_ENABLED(CONFIG_SCSI_MAC_ESP) || IS_ENABLED(CONFIG_SCSI_ZORRO_ESP)
> 
> Please add a new CONFIG_SCSI_ESP_PIO symbol that is selected by
> the mac and zorro drivers instead.
> 

OK.

> > +   pr_err("FIFO is empty (sreg %02x)\n", esp_read8(ESP_STATUS));
> 
> This should probably use dev_err (at least with my series).
> 

How about shost_printk()? That would be consistent with the rest of 
esp_scsi.c. It also means my series need not depend on yours which would 
complicate things.

-- 

Re: [PATCH 5/5] esp_scsi: move dma mapping into the core code

[Finn Thain]

On Sat, 13 Oct 2018, Christoph Hellwig wrote:

> On Sat, Oct 13, 2018 at 09:24:51AM +1100, Finn Thain wrote:
> > >   struct scatterlist *sg = scsi_sglist(cmd);
> > > - int dir = cmd->sc_data_direction;
> > > - int total, i;
> > > + int total = 0, i;
> > >  
> > > - if (dir == DMA_NONE)
> > > - return;
> > > -
> > 
> > Removing this DMA_NONE test caused an oops in my test. sg becomes a NULL 
> > pointer.
> 
> Does it work if you add the check back?
> 

Yes.

-- 

Re: [PATCH 6/6] esp_scsi: Optimize PIO loops

[Finn Thain]

On Sat, 13 Oct 2018, Michael Schmitz wrote:

> Hi Finn,
> 
> Am 13.10.2018 um 13:51 schrieb Finn Thain:
> > Avoid function calls in the inner PIO loops. On a Centris 660av this
> > improves throughput for sequential read transfers by about 40% and
> > sequential write by about 10%.
> > 
> > Unfortunately it is not possible to have method calls like esp_write8()
> > placed inline so this is always going to be slow (even with LTO).
> > 
> > Tested-by: Stan Johnson 
> > Signed-off-by: Finn Thain 
> > ---
> >  drivers/scsi/esp_scsi.c | 14 +++---
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> > index 646701fc22a4..9f0e68cd0e99 100644
> > --- a/drivers/scsi/esp_scsi.c
> > +++ b/drivers/scsi/esp_scsi.c
> > @@ -2788,7 +2788,7 @@ static inline unsigned int esp_wait_for_fifo(struct
> > esp *esp)
> > if (fbytes)
> > return fbytes;
> > 
> > -   udelay(2);
> > +   udelay(1);
> > } while (--i);
> > 
> > pr_err("FIFO is empty (sreg %02x)\n", esp_read8(ESP_STATUS));
> > @@ -2804,7 +2804,7 @@ static inline int esp_wait_for_intr(struct esp *esp)
> > if (esp->sreg & ESP_STAT_INTR)
> > return 0;
> > 
> > -   udelay(2);
> > +   udelay(1);
> > } while (--i);
> > 
> > pr_err("IRQ timeout (sreg %02x)\n", esp->sreg);
> > @@ -2831,7 +2831,7 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32
> > esp_count,
> > if (!esp_wait_for_fifo(esp))
> > break;
> > 
> > -   *dst++ = esp_read8(ESP_FDATA);
> > +   *dst++ = readb(esp->fifo_reg);
> > --esp_count;
> > 
> > if (!esp_count)
> > @@ -2852,15 +2852,15 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32
> > esp_count,
> > }
> > 
> > if (phase == ESP_MIP)
> > -   scsi_esp_cmd(esp, ESP_CMD_MOK);
> > +   esp_write8(ESP_CMD_MOK, ESP_CMD);
> 
> You're no longer logging this command with this patch. (That'll be the reason
> for the speedup you saw ...)
> 
> > 
> > -   scsi_esp_cmd(esp, ESP_CMD_TI);
> > +   esp_write8(ESP_CMD_TI, ESP_CMD);
> 
> Same here..
> 
> > }
> > } else {
> > unsigned int n = ESP_FIFO_SIZE;
> > u8 *src = (u8 *)addr;
> > 
> > -   scsi_esp_cmd(esp, ESP_CMD_FLUSH);
> > +   esp_write8(ESP_CMD_FLUSH, ESP_CMD);
> 
> here..
> 
> > 
> > if (n > esp_count)
> > n = esp_count;
> > @@ -2894,7 +2894,7 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32
> > esp_count,
> > src += n;
> > esp_count -= n;
> > 
> > -   scsi_esp_cmd(esp, ESP_CMD_TI);
> > +   esp_write8(ESP_CMD_TI, ESP_CMD);
> 
> and here.
> 

Yes, it's deliberate.

> The burst of ESP_CMD_TI's in the log was quite useful to spot what went 
> wrong during PIO.

I don't think it's as useful as you seem to think. Compare 
mac_esp_send_pdma_cmd().

> Maybe mention in the changelog that commands during PIO are no longer 
> logged? Or introduce a new ESP_EVENT_PIO and log that at the start of 
> PIO?
> 

Yes, and I did leave a scsi_esp_cmd(esp, cmd) call at the start of PIO. 

That should be sufficient, right?

-- 

> Cheers,
> 
>   Michael
> 
> 
> 
> > }
> > }
> > 
> > 
> 

[PATCH 1/6] zorro_esp: Limit DMA transfers to 65535 bytes

[Finn Thain]

The core driver, esp_scsi, does not use the ESP_CONFIG2_FENAB bit, so
the chip's Transfer Counter register is only 16 bits wide (not 24).
A larger transfer cannot work and will theoretically result in a failed
command and a "DMA length is zero" error.

Fixes: 3109e5ae0311
Signed-off-by: Finn Thain 
Cc: Michael Schmitz 
---
 drivers/scsi/zorro_esp.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
index bb70882e6b56..be79127db594 100644
--- a/drivers/scsi/zorro_esp.c
+++ b/drivers/scsi/zorro_esp.c
@@ -245,7 +245,7 @@ static int fastlane_esp_irq_pending(struct esp *esp)
 static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
u32 dma_len)
 {
-   return dma_len > 0xFF ? 0xFF : dma_len;
+   return dma_len > 0x ? 0x : dma_len;
 }
 
 static void zorro_esp_reset_dma(struct esp *esp)
@@ -484,7 +484,6 @@ static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, 
u32 addr,
scsi_esp_cmd(esp, ESP_CMD_DMA);
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
scsi_esp_cmd(esp, cmd);
 }
@@ -529,7 +528,6 @@ static void zorro_esp_send_blz1230II_dma_cmd(struct esp 
*esp, u32 addr,
scsi_esp_cmd(esp, ESP_CMD_DMA);
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
scsi_esp_cmd(esp, cmd);
 }
@@ -574,7 +572,6 @@ static void zorro_esp_send_blz2060_dma_cmd(struct esp *esp, 
u32 addr,
scsi_esp_cmd(esp, ESP_CMD_DMA);
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
scsi_esp_cmd(esp, cmd);
 }
@@ -599,7 +596,6 @@ static void zorro_esp_send_cyber_dma_cmd(struct esp *esp, 
u32 addr,
 
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
if (write) {
/* DMA receive */
@@ -649,7 +645,6 @@ static void zorro_esp_send_cyberII_dma_cmd(struct esp *esp, 
u32 addr,
 
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
if (write) {
/* DMA receive */
@@ -691,7 +686,6 @@ static void zorro_esp_send_fastlane_dma_cmd(struct esp 
*esp, u32 addr,
 
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-   zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
if (write) {
/* DMA receive */
-- 
2.16.4

[PATCH 4/6] esp_scsi: Eliminate ESP_FLAG_DOING_SLOWCMD

[Finn Thain]

The concept of a 'slow command' as it appears in esp_scsi is confusing
because it could refer to an ESP command or a SCSI command. It turns out
that it refers to a particular ESP select command which the driver also
tracks as 'ESP_SELECT_MSGOUT'. For readability, it is better to use the
terminology from the datasheets.

The global ESP_FLAG_DOING_SLOWCMD flag is redundant anyway, as it can be
inferred from esp->select_state. Remove the ESP_FLAG_DOING_SLOWCMD cruft
and just use a boolean local variable.

Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
---
 drivers/scsi/esp_scsi.c | 57 -
 drivers/scsi/esp_scsi.h |  3 +--
 2 files changed, 24 insertions(+), 36 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index b7c41aa9927c..6ccaf818357e 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -478,17 +478,6 @@ static void esp_restore_pointers(struct esp *esp, struct 
esp_cmd_entry *ent)
spriv->tot_residue = ent->saved_tot_residue;
 }
 
-static void esp_check_command_len(struct esp *esp, struct scsi_cmnd *cmd)
-{
-   if (cmd->cmd_len == 6 ||
-   cmd->cmd_len == 10 ||
-   cmd->cmd_len == 12) {
-   esp->flags &= ~ESP_FLAG_DOING_SLOWCMD;
-   } else {
-   esp->flags |= ESP_FLAG_DOING_SLOWCMD;
-   }
-}
-
 static void esp_write_tgt_config3(struct esp *esp, int tgt)
 {
if (esp->rev > ESP100A) {
@@ -721,6 +710,7 @@ static void esp_maybe_execute_command(struct esp *esp)
struct scsi_device *dev;
struct scsi_cmnd *cmd;
struct esp_cmd_entry *ent;
+   bool select_and_stop = false;
int tgt, lun, i;
u32 val, start_cmd;
u8 *p;
@@ -752,7 +742,8 @@ static void esp_maybe_execute_command(struct esp *esp)
esp_map_dma(esp, cmd);
esp_save_pointers(esp, ent);
 
-   esp_check_command_len(esp, cmd);
+   if (!(cmd->cmd_len == 6 || cmd->cmd_len == 10 || cmd->cmd_len == 12))
+   select_and_stop = true;
 
p = esp->command_block;
 
@@ -793,9 +784,9 @@ static void esp_maybe_execute_command(struct esp *esp)
tp->flags &= ~ESP_TGT_CHECK_NEGO;
}
 
-   /* Process it like a slow command.  */
-   if (tp->flags & (ESP_TGT_NEGO_WIDE | ESP_TGT_NEGO_SYNC))
-   esp->flags |= ESP_FLAG_DOING_SLOWCMD;
+   /* If there are multiple message bytes, use Select and Stop */
+   if (esp->msg_out_len)
+   select_and_stop = true;
}
 
 build_identify:
@@ -808,23 +799,10 @@ static void esp_maybe_execute_command(struct esp *esp)
/* ESP100 lacks select w/atn3 command, use select
 * and stop instead.
 */
-   esp->flags |= ESP_FLAG_DOING_SLOWCMD;
+   select_and_stop = true;
}
 
-   if (!(esp->flags & ESP_FLAG_DOING_SLOWCMD)) {
-   start_cmd = ESP_CMD_SELA;
-   if (ent->tag[0]) {
-   *p++ = ent->tag[0];
-   *p++ = ent->tag[1];
-
-   start_cmd = ESP_CMD_SA3;
-   }
-
-   for (i = 0; i < cmd->cmd_len; i++)
-   *p++ = cmd->cmnd[i];
-
-   esp->select_state = ESP_SELECT_BASIC;
-   } else {
+   if (select_and_stop) {
esp->cmd_bytes_left = cmd->cmd_len;
esp->cmd_bytes_ptr = &cmd->cmnd[0];
 
@@ -839,6 +817,19 @@ static void esp_maybe_execute_command(struct esp *esp)
 
start_cmd = ESP_CMD_SELAS;
esp->select_state = ESP_SELECT_MSGOUT;
+   } else {
+   start_cmd = ESP_CMD_SELA;
+   if (ent->tag[0]) {
+   *p++ = ent->tag[0];
+   *p++ = ent->tag[1];
+
+   start_cmd = ESP_CMD_SA3;
+   }
+
+   for (i = 0; i < cmd->cmd_len; i++)
+   *p++ = cmd->cmnd[i];
+
+   esp->select_state = ESP_SELECT_BASIC;
}
val = tgt;
if (esp->rev == FASHME)
@@ -1248,7 +1239,6 @@ static int esp_finish_select(struct esp *esp)
esp_unmap_dma(esp, cmd);
esp_free_lun_tag(ent, cmd->device->hostdata);
tp->flags &= ~(ESP_TGT_NEGO_SYNC | ESP_TGT_NEGO_WIDE);
-   esp->flags &= ~ESP_FLAG_DOING_SLOWCMD;
esp->cmd_bytes_ptr = NULL;
esp->cmd_bytes_left = 0;
} else {
@@ -1299,9 +1289,8 @@ static int esp_finish_select(struct esp *esp)
esp_flush_fifo(esp);
}
 
-   /* If we are doing a slow 

[PATCH 2/6] esp_scsi: Track residual for PIO transfers

[Finn Thain]

If a target disconnects during a PIO data transfer the command may
fail when the target reconnects:

scsi host1: DMA length is zero!
scsi host1: cur adr[0438] len[]

The scsi bus is then reset. This happens because the residual reached
zero before the transfer was completed.

The usual residual calculation relies on the Transfer Count registers
which works for DMA transfers but not for PIO transfers. Fix the problem
by storing the PIO transfer residual and using that to correctly
calculate bytes_sent.

Fixes: 6fe07aaffbf0
Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
---
 drivers/scsi/esp_scsi.c | 1 +
 drivers/scsi/esp_scsi.h | 2 ++
 drivers/scsi/mac_esp.c  | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index c3fc34b9964d..9e5d3f7d29ae 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -1338,6 +1338,7 @@ static int esp_data_bytes_sent(struct esp *esp, struct 
esp_cmd_entry *ent,
 
bytes_sent = esp->data_dma_len;
bytes_sent -= ecount;
+   bytes_sent -= esp->send_cmd_residual;
 
/*
 * The am53c974 has a DMA 'pecularity'. The doc states:
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index 8163dca2071b..db4b6ea94caa 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -540,6 +540,8 @@ struct esp {
 
void*dma;
int dmarev;
+
+   int send_cmd_residual;
 };
 
 /* A front-end driver for the ESP chip should do the following in
diff --git a/drivers/scsi/mac_esp.c b/drivers/scsi/mac_esp.c
index eb551f3cc471..71879f2207e0 100644
--- a/drivers/scsi/mac_esp.c
+++ b/drivers/scsi/mac_esp.c
@@ -427,6 +427,8 @@ static void mac_esp_send_pio_cmd(struct esp *esp, u32 addr, 
u32 esp_count,
scsi_esp_cmd(esp, ESP_CMD_TI);
}
}
+
+   esp->send_cmd_residual = esp_count;
 }
 
 static int mac_esp_irq_pending(struct esp *esp)
-- 
2.16.4

[PATCH 6/6] esp_scsi: Optimize PIO loops

[Finn Thain]

Avoid function calls in the inner PIO loops. On a Centris 660av this
improves throughput for sequential read transfers by about 40% and
sequential write by about 10%.

Unfortunately it is not possible to have method calls like esp_write8()
placed inline so this is always going to be slow (even with LTO).

Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
---
 drivers/scsi/esp_scsi.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 646701fc22a4..9f0e68cd0e99 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2788,7 +2788,7 @@ static inline unsigned int esp_wait_for_fifo(struct esp 
*esp)
if (fbytes)
return fbytes;
 
-   udelay(2);
+   udelay(1);
} while (--i);
 
pr_err("FIFO is empty (sreg %02x)\n", esp_read8(ESP_STATUS));
@@ -2804,7 +2804,7 @@ static inline int esp_wait_for_intr(struct esp *esp)
if (esp->sreg & ESP_STAT_INTR)
return 0;
 
-   udelay(2);
+   udelay(1);
} while (--i);
 
pr_err("IRQ timeout (sreg %02x)\n", esp->sreg);
@@ -2831,7 +2831,7 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 
esp_count,
if (!esp_wait_for_fifo(esp))
break;
 
-   *dst++ = esp_read8(ESP_FDATA);
+   *dst++ = readb(esp->fifo_reg);
--esp_count;
 
if (!esp_count)
@@ -2852,15 +2852,15 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 
esp_count,
}
 
if (phase == ESP_MIP)
-   scsi_esp_cmd(esp, ESP_CMD_MOK);
+   esp_write8(ESP_CMD_MOK, ESP_CMD);
 
-   scsi_esp_cmd(esp, ESP_CMD_TI);
+   esp_write8(ESP_CMD_TI, ESP_CMD);
}
} else {
unsigned int n = ESP_FIFO_SIZE;
u8 *src = (u8 *)addr;
 
-   scsi_esp_cmd(esp, ESP_CMD_FLUSH);
+   esp_write8(ESP_CMD_FLUSH, ESP_CMD);
 
if (n > esp_count)
n = esp_count;
@@ -2894,7 +2894,7 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 
esp_count,
src += n;
esp_count -= n;
 
-   scsi_esp_cmd(esp, ESP_CMD_TI);
+   esp_write8(ESP_CMD_TI, ESP_CMD);
}
}
 
-- 
2.16.4

[PATCH 3/6] esp_scsi: Grant disconnect privilege for untagged commands

[Finn Thain]

A SCSI device is not granted disconnect privilege by an esp_scsi host
unless that device has its simple_tags flag set. However, a device may
support disconnect/reselect and not support command queueing. Allow
these devices to disconnect and improve bus utilization.

Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
---
 drivers/scsi/esp_scsi.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 9e5d3f7d29ae..b7c41aa9927c 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -800,13 +800,9 @@ static void esp_maybe_execute_command(struct esp *esp)
 
 build_identify:
/* If we don't have a lun-data struct yet, we're probing
-* so do not disconnect.  Also, do not disconnect unless
-* we have a tag on this command.
+* so do not disconnect.
 */
-   if (lp && (tp->flags & ESP_TGT_DISCONNECT) && ent->tag[0])
-   *p++ = IDENTIFY(1, lun);
-   else
-   *p++ = IDENTIFY(0, lun);
+   *p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);
 
if (ent->tag[0] && esp->rev == ESP100) {
/* ESP100 lacks select w/atn3 command, use select
-- 
2.16.4

[PATCH 5/6] esp_scsi: De-duplicate PIO routines

[Finn Thain]

As a temporary measure, the code to implement PIO transfers was
duplicated in zorro_esp and mac_esp. Now that this code has stabilized,
move it into the core driver to eliminate the duplication.

This replaces the inline assembler with more portable writesb() calls.
Optimizing the m68k writesb() implementation is a separate patch.

Tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
---
 drivers/scsi/esp_scsi.c  | 126 +
 drivers/scsi/esp_scsi.h  |   5 +
 drivers/scsi/mac_esp.c   | 173 ++-
 drivers/scsi/zorro_esp.c | 232 +++
 4 files changed, 171 insertions(+), 365 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 6ccaf818357e..646701fc22a4 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2776,3 +2776,129 @@ MODULE_PARM_DESC(esp_debug,
 
 module_init(esp_init);
 module_exit(esp_exit);
+
+#if IS_ENABLED(CONFIG_SCSI_MAC_ESP) || IS_ENABLED(CONFIG_SCSI_ZORRO_ESP)
+static inline unsigned int esp_wait_for_fifo(struct esp *esp)
+{
+   int i = 50;
+
+   do {
+   unsigned int fbytes = esp_read8(ESP_FFLAGS) & ESP_FF_FBYTES;
+
+   if (fbytes)
+   return fbytes;
+
+   udelay(2);
+   } while (--i);
+
+   pr_err("FIFO is empty (sreg %02x)\n", esp_read8(ESP_STATUS));
+   return 0;
+}
+
+static inline int esp_wait_for_intr(struct esp *esp)
+{
+   int i = 50;
+
+   do {
+   esp->sreg = esp_read8(ESP_STATUS);
+   if (esp->sreg & ESP_STAT_INTR)
+   return 0;
+
+   udelay(2);
+   } while (--i);
+
+   pr_err("IRQ timeout (sreg %02x)\n", esp->sreg);
+   return 1;
+}
+
+#define ESP_FIFO_SIZE 16
+
+void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
+ u32 dma_count, int write, u8 cmd)
+{
+   u8 phase = esp->sreg & ESP_STAT_PMASK;
+
+   cmd &= ~ESP_CMD_DMA;
+   esp->send_cmd_error = 0;
+
+   if (write) {
+   u8 *dst = (u8 *)addr;
+   u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : ESP_INTR_BSERV);
+
+   scsi_esp_cmd(esp, cmd);
+
+   while (1) {
+   if (!esp_wait_for_fifo(esp))
+   break;
+
+   *dst++ = esp_read8(ESP_FDATA);
+   --esp_count;
+
+   if (!esp_count)
+   break;
+
+   if (esp_wait_for_intr(esp)) {
+   esp->send_cmd_error = 1;
+   break;
+   }
+
+   if ((esp->sreg & ESP_STAT_PMASK) != phase)
+   break;
+
+   esp->ireg = esp_read8(ESP_INTRPT);
+   if (esp->ireg & mask) {
+   esp->send_cmd_error = 1;
+   break;
+   }
+
+   if (phase == ESP_MIP)
+   scsi_esp_cmd(esp, ESP_CMD_MOK);
+
+   scsi_esp_cmd(esp, ESP_CMD_TI);
+   }
+   } else {
+   unsigned int n = ESP_FIFO_SIZE;
+   u8 *src = (u8 *)addr;
+
+   scsi_esp_cmd(esp, ESP_CMD_FLUSH);
+
+   if (n > esp_count)
+   n = esp_count;
+   writesb(esp->fifo_reg, src, n);
+   src += n;
+   esp_count -= n;
+
+   scsi_esp_cmd(esp, cmd);
+
+   while (esp_count) {
+   if (esp_wait_for_intr(esp)) {
+   esp->send_cmd_error = 1;
+   break;
+   }
+
+   if ((esp->sreg & ESP_STAT_PMASK) != phase)
+   break;
+
+   esp->ireg = esp_read8(ESP_INTRPT);
+   if (esp->ireg & ~ESP_INTR_BSERV) {
+   esp->send_cmd_error = 1;
+   break;
+   }
+
+   n = ESP_FIFO_SIZE -
+   (esp_read8(ESP_FFLAGS) & ESP_FF_FBYTES);
+
+   if (n > esp_count)
+   n = esp_count;
+   writesb(esp->fifo_reg, src, n);
+   src += n;
+   esp_count -= n;
+
+   scsi_esp_cmd(esp, ESP_CMD_TI);
+   }
+   }
+
+   esp->send_cmd_residual = esp_count;
+}
+EXPORT_SYMBOL(esp_send_pio_cmd);
+#endif
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index d0c032803749..2590e5eda595 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -431,6 +431,7 @@ st

[PATCH 0/6] mac_esp, zorro_esp, esp_scsi: Various improvements

[Finn Thain]

This series has fixes and cleanup for mac_esp, zorro_esp and the
core esp_scsi driver.

The improvements include elimination of duplicated code temporarily
introduced for zorro_esp.

Michael, would you please regression test this series on elgar,
if convenient? So far, only mac_esp has been tested with this code.


Finn Thain (6):
  zorro_esp: Limit DMA transfers to 65535 bytes
  esp_scsi: Track residual for PIO transfers
  esp_scsi: Grant disconnect privilege for untagged commands
  esp_scsi: Eliminate ESP_FLAG_DOING_SLOWCMD
  esp_scsi: De-duplicate PIO routines
  esp_scsi: Optimize PIO loops

 drivers/scsi/esp_scsi.c  | 192 +
 drivers/scsi/esp_scsi.h  |  10 +-
 drivers/scsi/mac_esp.c   | 171 ++---
 drivers/scsi/zorro_esp.c | 240 +++
 4 files changed, 201 insertions(+), 412 deletions(-)

-- 
2.16.4

Re: [PATCH 3/5] esp_scsi: use strong typing for the dev field

[Finn Thain]

> On Fri, 12 Oct 2018, Christoph Hellwig wrote:
> 
> > 
> > > diff --git a/drivers/scsi/mac_esp.c b/drivers/scsi/mac_esp.c
> > > index eb551f3cc471..85d067889a9b 100644
> > > --- a/drivers/scsi/mac_esp.c
> > > +++ b/drivers/scsi/mac_esp.c
> > > @@ -58,8 +58,7 @@ static struct esp *esp_chips[2];
> > >  static DEFINE_SPINLOCK(esp_chips_lock);
> > >
> > >  #define MAC_ESP_GET_PRIV(esp) ((struct mac_esp_priv *) \
> > > -platform_get_drvdata((struct platform_device 
> > > *) \
> > > - (esp->dev)))
> > > +dev_get_drvdata((esp)->dev))
> > >
> > >  static inline void mac_esp_write8(struct esp *esp, u8 val, unsigned long 
> > > reg)
> > >  {
> > > @@ -508,7 +507,7 @@ static int esp_mac_probe(struct platform_device *dev)
> > >   esp = shost_priv(host);
> > >
> > >   esp->host = host;
> > > - esp->dev = dev;
> > > + esp->dev = &dev->dev;
> > >
> > >   esp->command_block = kzalloc(16, GFP_KERNEL);
> > >   if (!esp->command_block)
> > 
> > 
> > Isn't this missing the corresponding dev_set_drvdata() in esp_mac_probe()?
> > Also conversion to dev_get_drvdata() in esp_mac_remove()?
> > 
> 
> I don't think it matters at all, as nothing in the core esp_scsi.c code 
> looks into the driver data to start with, but even if it did, 
> platform_set_drvdata and platform_get_drvdata and just trivial wrappers 
> around dev_set_drvdata and dev_get_drvdata and we rely on that fact all 
> over the kernel.
> 

Sorry, I got confused by the mix of dev_get_drvdata() and 
platform_get_drvdata() but I see now that they all refer to the same 
struct device.

> That being said if you want them converted I can throw in another patch 
> at the end of the series.
> 

No need. Thanks.

-- 

Re: [PATCH 5/5] esp_scsi: move dma mapping into the core code

[Finn Thain]

On Thu, 11 Oct 2018, Christoph Hellwig wrote:

> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index 90604bff8dd2..73fcbd65b9fe 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -369,19 +369,25 @@ static void esp_map_dma(struct esp *esp, struct 
> scsi_cmnd *cmd)
>  {
>   struct esp_cmd_priv *spriv = ESP_CMD_PRIV(cmd);
>   struct scatterlist *sg = scsi_sglist(cmd);
> - int dir = cmd->sc_data_direction;
> - int total, i;
> + int total = 0, i;
>  
> - if (dir == DMA_NONE)
> - return;
> -

Removing this DMA_NONE test caused an oops in my test. sg becomes a NULL 
pointer.

> - spriv->u.num_sg = esp->ops->map_sg(esp, sg, scsi_sg_count(cmd), dir);

Does anyone happen to know whether the union 'u' serves any purpose?

-- 

> + if (esp->flags & ESP_FLAG_NO_DMA_MAP) {
> + /*
> +  * For pseudo DMA and PIO we need the virtual address instead of
> +  * a dma address, so perform an identity mapping.
> +  */
> + spriv->u.num_sg = scsi_sg_count(cmd);
> + for (i = 0; i < spriv->u.num_sg; i++) {
> + sg[i].dma_address = (uintptr_t)sg_virt(&sg[i]);
> + total += sg_dma_len(&sg[i]);
> + }
> + } else {
> + spriv->u.num_sg = scsi_dma_map(cmd);
> + for (i = 0; i < spriv->u.num_sg; i++)
> + total += sg_dma_len(&sg[i]);
> + }
>   spriv->cur_residue = sg_dma_len(sg);
>   spriv->cur_sg = sg;
> -
> - total = 0;
> - for (i = 0; i < spriv->u.num_sg; i++)
> - total += sg_dma_len(&sg[i]);
>   spriv->tot_residue = total;
>  }
>  

Re: [PATCH 3/5] esp_scsi: use strong typing for the dev field

[Finn Thain]

On Thu, 11 Oct 2018, Christoph Hellwig wrote:

> esp->dev is a void pointer that points either to a struct device, or a
> struct platform_device.  As we can easily get from the device to the
> platform_device if needed change it to always point to a struct device
> and properly type the pointer to avoid errors.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/scsi/esp_scsi.h |  2 +-
>  drivers/scsi/mac_esp.c  |  5 ++---
>  drivers/scsi/sun_esp.c  | 37 +
>  3 files changed, 16 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index 8163dca2071b..f98abd0ead57 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -435,7 +435,7 @@ struct esp {
>   const struct esp_driver_ops *ops;
>  
>   struct Scsi_Host*host;
> - void*dev;
> + struct device   *dev;
>  
>   struct esp_cmd_entry*active_cmd;
>  
> diff --git a/drivers/scsi/mac_esp.c b/drivers/scsi/mac_esp.c
> index eb551f3cc471..85d067889a9b 100644
> --- a/drivers/scsi/mac_esp.c
> +++ b/drivers/scsi/mac_esp.c
> @@ -58,8 +58,7 @@ static struct esp *esp_chips[2];
>  static DEFINE_SPINLOCK(esp_chips_lock);
>  
>  #define MAC_ESP_GET_PRIV(esp) ((struct mac_esp_priv *) \
> -platform_get_drvdata((struct platform_device *) \
> - (esp->dev)))
> +dev_get_drvdata((esp)->dev))
>  
>  static inline void mac_esp_write8(struct esp *esp, u8 val, unsigned long reg)
>  {
> @@ -508,7 +507,7 @@ static int esp_mac_probe(struct platform_device *dev)
>   esp = shost_priv(host);
>  
>   esp->host = host;
> - esp->dev = dev;
> + esp->dev = &dev->dev;
>  
>   esp->command_block = kzalloc(16, GFP_KERNEL);
>   if (!esp->command_block)


Isn't this missing the corresponding dev_set_drvdata() in esp_mac_probe()? 
Also conversion to dev_get_drvdata() in esp_mac_remove()?

-- 

> diff --git a/drivers/scsi/sun_esp.c b/drivers/scsi/sun_esp.c
> index c7b60ed61c38..91577ceb4cba 100644
> --- a/drivers/scsi/sun_esp.c
> +++ b/drivers/scsi/sun_esp.c
> @@ -80,7 +80,7 @@ static int esp_sbus_setup_dma(struct esp *esp, struct 
> platform_device *dma_of)
>  
>  static int esp_sbus_map_regs(struct esp *esp, int hme)
>  {
> - struct platform_device *op = esp->dev;
> + struct platform_device *op = to_platform_device(esp->dev);
>   struct resource *res;
>  
>   /* On HME, two reg sets exist, first is DVMA,
> @@ -100,9 +100,7 @@ static int esp_sbus_map_regs(struct esp *esp, int hme)
>  
>  static int esp_sbus_map_command_block(struct esp *esp)
>  {
> - struct platform_device *op = esp->dev;
> -
> - esp->command_block = dma_alloc_coherent(&op->dev, 16,
> + esp->command_block = dma_alloc_coherent(esp->dev, 16,
>   &esp->command_block_dma,
>   GFP_KERNEL);
>   if (!esp->command_block)
> @@ -113,7 +111,7 @@ static int esp_sbus_map_command_block(struct esp *esp)
>  static int esp_sbus_register_irq(struct esp *esp)
>  {
>   struct Scsi_Host *host = esp->host;
> - struct platform_device *op = esp->dev;
> + struct platform_device *op = to_platform_device(esp->dev);
>  
>   host->irq = op->archdata.irqs[0];
>   return request_irq(host->irq, scsi_esp_intr, IRQF_SHARED, "ESP", esp);
> @@ -121,7 +119,7 @@ static int esp_sbus_register_irq(struct esp *esp)
>  
>  static void esp_get_scsi_id(struct esp *esp, struct platform_device *espdma)
>  {
> - struct platform_device *op = esp->dev;
> + struct platform_device *op = to_platform_device(esp->dev);
>   struct device_node *dp;
>  
>   dp = op->dev.of_node;
> @@ -143,7 +141,7 @@ static void esp_get_scsi_id(struct esp *esp, struct 
> platform_device *espdma)
>  
>  static void esp_get_differential(struct esp *esp)
>  {
> - struct platform_device *op = esp->dev;
> + struct platform_device *op = to_platform_device(esp->dev);
>   struct device_node *dp;
>  
>   dp = op->dev.of_node;
> @@ -155,7 +153,7 @@ static void esp_get_differential(struct esp *esp)
>  
>  static void esp_get_clock_params(struct esp *esp)
>  {
> - struct platform_device *op = esp->dev;
> + struct platform_device *op = to_platform_device(esp->dev);
>   struct device_node *bus_dp, *dp;
>   int fmhz;
>  
> @@ -172,7 +170,7 @@ static void esp_get_clock_params(struct esp *esp)
>  static void esp_get_bursts(struct esp *esp, struct platform_device *dma_of)
>  {
>   struct device_node *dma_dp = dma_of->dev.of_node;
> - struct platform_device *op = esp->dev;
> + struct platform_device *op = to_platform_device(esp->dev);
>   struct device_node *dp;
>   u8 bursts, val;
>  
> @@ -215,33 +213,25 @@ static u8 sbus_esp_read8(struct esp *esp, unsigned long 
> reg)
>  static dma_addr_t sbus_esp_map_single(struct esp *esp, v

Re: [PATCH 5/5] esp_scsi: move dma mapping into the core code

[Finn Thain]

On Thu, 11 Oct 2018, Christoph Hellwig wrote:

> index 90604bff8dd2..73fcbd65b9fe 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -369,19 +369,25 @@ static void esp_map_dma(struct esp *esp, struct 
> scsi_cmnd *cmd)
>  {
>   struct esp_cmd_priv *spriv = ESP_CMD_PRIV(cmd);
>   struct scatterlist *sg = scsi_sglist(cmd);
> - int dir = cmd->sc_data_direction;
> - int total, i;
> + int total = 0, i;
>  
> - if (dir == DMA_NONE)
> - return;
> -
> - spriv->u.num_sg = esp->ops->map_sg(esp, sg, scsi_sg_count(cmd), dir);
> + if (esp->flags & ESP_FLAG_NO_DMA_MAP) {
> + /*
> +  * For pseudo DMA and PIO we need the virtual address instead of
> +  * a dma address, so perform an identity mapping.
> +  */
> + spriv->u.num_sg = scsi_sg_count(cmd);
> + for (i = 0; i < spriv->u.num_sg; i++) {
> + sg[i].dma_address = (uintptr_t)sg_virt(&sg[i]);
> + total += sg_dma_len(&sg[i]);
> + }
> + } else {
> + spriv->u.num_sg = scsi_dma_map(cmd);
> + for (i = 0; i < spriv->u.num_sg; i++)
> + total += sg_dma_len(&sg[i]);
> + }
>   spriv->cur_residue = sg_dma_len(sg);
>   spriv->cur_sg = sg;
> -
> - total = 0;
> - for (i = 0; i < spriv->u.num_sg; i++)
> - total += sg_dma_len(&sg[i]);
>   spriv->tot_residue = total;
>  }
>  
> @@ -441,13 +447,8 @@ static void esp_advance_dma(struct esp *esp, struct 
> esp_cmd_entry *ent,
>  
>  static void esp_unmap_dma(struct esp *esp, struct scsi_cmnd *cmd)
>  {
> - struct esp_cmd_priv *spriv = ESP_CMD_PRIV(cmd);
> - int dir = cmd->sc_data_direction;
> -
> - if (dir == DMA_NONE)
> - return;
> -
> - esp->ops->unmap_sg(esp, scsi_sglist(cmd), spriv->u.num_sg, dir);
> + if (!(esp->flags & ESP_FLAG_NO_DMA_MAP))
> + scsi_dma_unmap(cmd);
>  }
>  
>  static void esp_save_pointers(struct esp *esp, struct esp_cmd_entry *ent)
> @@ -624,6 +625,28 @@ static void esp_free_lun_tag(struct esp_cmd_entry *ent,
>   }
>  }
>  
> +static void esp_map_sense(struct esp *esp, struct esp_cmd_entry *ent)
> +{
> + ent->sense_ptr = ent->cmd->sense_buffer;
> + if (esp->flags & ESP_FLAG_NO_DMA_MAP) {
> + ent->sense_dma = (uintptr_t)ent->sense_ptr;
> + return;
> + }
> +
> + ent->sense_dma = dma_map_single(esp->dev, ent->sense_ptr,
> + SCSI_SENSE_BUFFERSIZE, DMA_FROM_DEVICE);
> +}
> +
> +static void esp_unmap_sense(struct esp *esp, struct esp_cmd_entry *ent)
> +{
> + if (esp->flags & ESP_FLAG_NO_DMA_MAP)
> + return;
> +
> + dma_unmap_single(esp->dev, ent->sense_dma, SCSI_SENSE_BUFFERSIZE,
> +  DMA_FROM_DEVICE);
> + ent->sense_ptr = NULL;
> +}
> +

I think it is better to clear ent->sense_ptr unconditionally, as before.

-- 

>  /* When a contingent allegiance conditon is created, we force feed a
>   * REQUEST_SENSE command to the device to fetch the sense data.  I
>   * tried many other schemes, relying on the scsi error handling layer
> @@ -645,12 +668,7 @@ static void esp_autosense(struct esp *esp, struct 
> esp_cmd_entry *ent)
>   if (!ent->sense_ptr) {
>   esp_log_autosense("Doing auto-sense for tgt[%d] lun[%d]\n",
> tgt, lun);
> -
> - ent->sense_ptr = cmd->sense_buffer;
> - ent->sense_dma = esp->ops->map_single(esp,
> -   ent->sense_ptr,
> -   SCSI_SENSE_BUFFERSIZE,
> -   DMA_FROM_DEVICE);
> + esp_map_sense(esp, ent);
>   }
>   ent->saved_sense_ptr = ent->sense_ptr;
>  
> @@ -902,9 +920,7 @@ static void esp_cmd_is_done(struct esp *esp, struct 
> esp_cmd_entry *ent,
>   }
>  
>   if (ent->flags & ESP_CMD_FLAG_AUTOSENSE) {
> - esp->ops->unmap_single(esp, ent->sense_dma,
> -SCSI_SENSE_BUFFERSIZE, DMA_FROM_DEVICE);
> - ent->sense_ptr = NULL;
> + esp_unmap_sense(esp, ent);
>  
>   /* Restore the message/status bytes to what we actually
>* saw originally.  Also, report that we are providing
> @@ -1256,10 +1272,7 @@ static int esp_finish_select(struct esp *esp)
>   esp->cmd_bytes_ptr = NULL;
>   esp->cmd_bytes_left = 0;
>   } else {
> - esp->ops->unmap_single(esp, ent->sense_dma,
> -SCSI_SENSE_BUFFERSIZE,
> -DMA_FROM_DEVICE);
> - ent->sense_ptr = NULL;
> + esp_unmap_sense(esp, ent);
>   }
>  
>   /* Now that the state is unwound properly, pu

Re: [PATCH 3/4] esp_scsi: use strong typing for the dev field

[Finn Thain]

On Wed, 10 Oct 2018, Christoph Hellwig wrote:

> esp->dev is a void pointer that points either to a struct device, or a
> struct platform_device.  As we can easily get from the device to the
> platform_device if needed change it to always point to a struct device
> and properly type the pointer to avoid errors.
> 

If you do so, can you also eliminate the dev argument to 
scsi_esp_register()?


> ...
> diff --git a/drivers/scsi/sun_esp.c b/drivers/scsi/sun_esp.c
> index c7b60ed61c38..c025f7ddaeb0 100644
> --- a/drivers/scsi/sun_esp.c
> +++ b/drivers/scsi/sun_esp.c
> @@ -80,7 +80,7 @@ static int esp_sbus_setup_dma(struct esp *esp, struct 
> platform_device *dma_of)
>  
>  static int esp_sbus_map_regs(struct esp *esp, int hme)
>  {
> - struct platform_device *op = esp->dev;
> + struct platform_device *op = to_platform_device(esp->dev);
>   struct resource *res;
>  
>   /* On HME, two reg sets exist, first is DVMA,
> @@ -100,9 +100,9 @@ static int esp_sbus_map_regs(struct esp *esp, int hme)
>  
>  static int esp_sbus_map_command_block(struct esp *esp)
>  {
> - struct platform_device *op = esp->dev;
> + struct platform_device *op = to_platform_device(esp->dev);
>  
> - esp->command_block = dma_alloc_coherent(&op->dev, 16,
> + esp->command_block = dma_alloc_coherent(dev, 16,

Shouldn't that be esp->dev?

-- 

[PATCH 03/10] NCR5380: Have NCR5380_select() return a bool

[Finn Thain]

The return value is taken to mean "retry" or "don't retry". Change it to
bool to improve readability. Fix related comments. No functional change.

Tested-by: Michael Schmitz 
Signed-off-by: Finn Thain 
---
 drivers/scsi/NCR5380.c | 46 +-
 drivers/scsi/NCR5380.h |  2 +-
 2 files changed, 22 insertions(+), 26 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index c6d10780febe..7be1ba2b9e4e 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -902,20 +902,16 @@ static irqreturn_t __maybe_unused NCR5380_intr(int irq, 
void *dev_id)
return IRQ_RETVAL(handled);
 }
 
-/*
- * Function : int NCR5380_select(struct Scsi_Host *instance,
- * struct scsi_cmnd *cmd)
- *
- * Purpose : establishes I_T_L or I_T_L_Q nexus for new or existing command,
- * including ARBITRATION, SELECTION, and initial message out for
- * IDENTIFY and queue messages.
+/**
+ * NCR5380_select - attempt arbitration and selection for a given command
+ * @instance: the Scsi_Host instance
+ * @cmd: the scsi_cmnd to execute
  *
- * Inputs : instance - instantiation of the 5380 driver on which this
- * target lives, cmd - SCSI command to execute.
+ * This routine establishes an I_T_L nexus for a SCSI command. This involves
+ * ARBITRATION, SELECTION and MESSAGE OUT phases and an IDENTIFY message.
  *
- * Returns cmd if selection failed but should be retried,
- * NULL if selection failed and should not be retried, or
- * NULL if selection succeeded (hostdata->connected == cmd).
+ * Returns true if the operation should be retried.
+ * Returns false if it should not be retried.
  *
  * Side effects :
  * If bus busy, arbitration failed, etc, NCR5380_select() will exit
@@ -923,16 +919,15 @@ static irqreturn_t __maybe_unused NCR5380_intr(int irq, 
void *dev_id)
  * SELECT_ENABLE will be set appropriately, the NCR5380
  * will cease to drive any SCSI bus signals.
  *
- * If successful : I_T_L or I_T_L_Q nexus will be established,
- * instance->connected will be set to cmd.
+ * If successful : the I_T_L nexus will be established, and
+ * hostdata->connected will be set to cmd.
  * SELECT interrupt will be disabled.
  *
  * If failed (no target) : cmd->scsi_done() will be called, and the
  * cmd->result host byte set to DID_BAD_TARGET.
  */
 
-static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
-struct scsi_cmnd *cmd)
+static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
__releases(&hostdata->lock) __acquires(&hostdata->lock)
 {
struct NCR5380_hostdata *hostdata = shost_priv(instance);
@@ -940,6 +935,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host 
*instance,
unsigned char *data;
int len;
int err;
+   bool ret = true;
 
NCR5380_dprint(NDEBUG_ARBITRATION, instance);
dsprintk(NDEBUG_ARBITRATION, instance, "starting arbitration, id = 
%d\n",
@@ -948,7 +944,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host 
*instance,
/*
 * Arbitration and selection phases are slow and involve dropping the
 * lock, so we have to watch out for EH. An exception handler may
-* change 'selecting' to NULL. This function will then return NULL
+* change 'selecting' to NULL. This function will then return false
 * so that the caller will forget about 'cmd'. (During information
 * transfer phases, EH may change 'connected' to NULL.)
 */
@@ -984,7 +980,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host 
*instance,
if (!hostdata->selecting) {
/* Command was aborted */
NCR5380_write(MODE_REG, MR_BASE);
-   return NULL;
+   return false;
}
if (err < 0) {
NCR5380_write(MODE_REG, MR_BASE);
@@ -1033,7 +1029,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host 
*instance,
if (!hostdata->selecting) {
NCR5380_write(MODE_REG, MR_BASE);
NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
-   return NULL;
+   return false;
}
 
dsprintk(NDEBUG_ARBITRATION, instance, "won arbitration\n");
@@ -1119,13 +1115,13 @@ static struct scsi_cmnd *NCR5380_select(struct 
Scsi_Host *instance,
 
/* Can't touch cmd if it has been reclaimed by the scsi ML */
if (!hostdata->selecting)
-   return NULL;
+   return false;
 
cmd->result = DID_BAD_TARGET << 16;
complete_cmd(instance, cmd);
dsprintk(NDEBUG_SELECTION, instance,
"target did not respond within 250ms\n");
-   cmd = NULL;
+   ret = false;

[PATCH 05/10] NCR5380: Use DRIVER_SENSE to indicate valid sense data

[Finn Thain]

When sense data is valid, call set_driver_byte(cmd, DRIVER_SENSE).
Otherwise some callers of scsi_execute() will ignore sense data.
Don't set DID_ERROR or DID_RESET just because sense data is missing.

Tested-by: Michael Schmitz 
Signed-off-by: Finn Thain 
---
 drivers/scsi/NCR5380.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 5226164cfa65..e96a48b9e86c 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -513,11 +513,12 @@ static void complete_cmd(struct Scsi_Host *instance,
 
if (hostdata->sensing == cmd) {
/* Autosense processing ends here */
-   if ((cmd->result & 0xff) != SAM_STAT_GOOD) {
+   if (status_byte(cmd->result) != GOOD) {
scsi_eh_restore_cmnd(cmd, &hostdata->ses);
-   set_host_byte(cmd, DID_ERROR);
-   } else
+   } else {
scsi_eh_restore_cmnd(cmd, &hostdata->ses);
+   set_driver_byte(cmd, DRIVER_SENSE);
+   }
hostdata->sensing = NULL;
}
 
@@ -2273,7 +2274,6 @@ static int NCR5380_abort(struct scsi_cmnd *cmd)
if (list_del_cmd(&hostdata->autosense, cmd)) {
dsprintk(NDEBUG_ABORT, instance,
 "abort: removed %p from sense queue\n", cmd);
-   set_host_byte(cmd, DID_ERROR);
complete_cmd(instance, cmd);
}
 
@@ -2352,7 +2352,6 @@ static int NCR5380_host_reset(struct scsi_cmnd *cmd)
list_for_each_entry(ncmd, &hostdata->autosense, list) {
struct scsi_cmnd *cmd = NCR5380_to_scmd(ncmd);
 
-   set_host_byte(cmd, DID_RESET);
cmd->scsi_done(cmd);
}
INIT_LIST_HEAD(&hostdata->autosense);
-- 
2.16.4

[PATCH 00/10] NCR5380: Various improvements

[Finn Thain]

[This patch series is being re-sent unchanged due to email problems.]

This series addresses issues which became apparent when Michael Schmitz
tried to use an AztecMonster II SATA/SCSI adapter with a 5380 host.

That target seems to have some bugs which thoroughly exercise driver
error paths and EH.

The patch by Hannes Reinecke was cherry-picked from his 'eh-reset.v5'
branch.


Finn Thain (9):
  NCR5380: Reduce goto statements in NCR5380_select()
  NCR5380: Have NCR5380_select() return a bool
  NCR5380: Withhold disconnect privilege for REQUEST SENSE
  NCR5380: Use DRIVER_SENSE to indicate valid sense data
  NCR5380: Check for invalid reselection target
  NCR5380: Don't clear busy flag when abort fails
  NCR5380: Don't call dsprintk() following reselection interrupt
  NCR5380: Handle BUS FREE during reselection
  NCR5380: Check for bus reset

Hannes Reinecke (1):
  NCR5380: Clear all unissued commands on host reset

 drivers/scsi/NCR5380.c | 167 +
 drivers/scsi/NCR5380.h |   2 +-
 2 files changed, 101 insertions(+), 68 deletions(-)

-- 
2.16.4

[PATCH 09/10] NCR5380: Handle BUS FREE during reselection

[Finn Thain]

The X3T9.2 specification (draft) says, under "6.1.4.2 RESELECTION
time-out procedure", that a target may assert RST or go to BUS FREE
phase if the initiator does not respond within 200 us. Something
like this has been observed with AztecMonster II target. When it
happens, all we can do is wait for the target to try again.

Tested-by: Michael Schmitz 
Signed-off-by: Finn Thain 
---
 drivers/scsi/NCR5380.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 419033643015..b9a3eb0647e4 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -2047,6 +2047,9 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
 
if (NCR5380_poll_politely(hostdata,
  STATUS_REG, SR_REQ, SR_REQ, 2 * HZ) < 0) {
+   if ((NCR5380_read(STATUS_REG) & (SR_BSY | SR_SEL)) == 0)
+   /* BUS FREE phase */
+   return;
shost_printk(KERN_ERR, instance, "reselect: REQ timeout\n");
do_abort(instance);
return;
-- 
2.16.4

[PATCH 08/10] NCR5380: Don't call dsprintk() following reselection interrupt

[Finn Thain]

The X3T9.2 specification (draft) says, under "6.1.4.1 RESELECTION",

... The reselected initiator shall then assert the BSY signal
within a selection abort time of its most recent detection of being
reselected; this is required for correct operation of the time-out
procedure.

The selection abort time is only 200 us which may be insufficient time
for a printk() call. Move the diagnostics to the error paths.

Tested-by: Michael Schmitz 
Signed-off-by: Finn Thain 
---
 drivers/scsi/NCR5380.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 5826421146ba..419033643015 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -2023,8 +2023,6 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
return;
}
 
-   dsprintk(NDEBUG_RESELECTION, instance, "reselect\n");
-
/*
 * At this point, we have detected that our SCSI ID is on the bus,
 * SEL is true and BSY was false for at least one bus settle delay
@@ -2037,6 +2035,7 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE | ICR_ASSERT_BSY);
if (NCR5380_poll_politely(hostdata,
  STATUS_REG, SR_SEL, 0, 2 * HZ) < 0) {
+   shost_printk(KERN_ERR, instance, "reselect: !SEL timeout\n");
NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
return;
}
@@ -2048,6 +2047,7 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
 
if (NCR5380_poll_politely(hostdata,
  STATUS_REG, SR_REQ, SR_REQ, 2 * HZ) < 0) {
+   shost_printk(KERN_ERR, instance, "reselect: REQ timeout\n");
do_abort(instance);
return;
}
-- 
2.16.4

[PATCH 10/10] NCR5380: Check for bus reset

[Finn Thain]

The SR_RST bit isn't latched. Hence, detecting a bus reset isn't reliable.
When it is detected, the right thing to do is to drop all connected and
disconnected commands. The code for that is already present so refactor
it and call it when SR_RST is set.

Tested-by: Michael Schmitz 
Signed-off-by: Finn Thain 
---
 drivers/scsi/NCR5380.c | 74 ++
 1 file changed, 45 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index b9a3eb0647e4..8429c855701f 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -131,6 +131,7 @@
 
 static int do_abort(struct Scsi_Host *);
 static void do_reset(struct Scsi_Host *);
+static void bus_reset_cleanup(struct Scsi_Host *);
 
 /**
  * initialize_SCp - init the scsi pointer field
@@ -883,7 +884,14 @@ static irqreturn_t __maybe_unused NCR5380_intr(int irq, 
void *dev_id)
/* Probably Bus Reset */
NCR5380_read(RESET_PARITY_INTERRUPT_REG);
 
-   dsprintk(NDEBUG_INTR, instance, "unknown interrupt\n");
+   if (sr & SR_RST) {
+   /* Certainly Bus Reset */
+   shost_printk(KERN_WARNING, instance,
+"bus reset interrupt\n");
+   bus_reset_cleanup(instance);
+   } else {
+   dsprintk(NDEBUG_INTR, instance, "unknown 
interrupt\n");
+   }
 #ifdef SUN3_SCSI_VME
dregs->csr |= CSR_DMA_ENABLE;
 #endif
@@ -2305,31 +2313,12 @@ static int NCR5380_abort(struct scsi_cmnd *cmd)
 }
 
 
-/**
- * NCR5380_host_reset - reset the SCSI host
- * @cmd: SCSI command undergoing EH
- *
- * Returns SUCCESS
- */
-
-static int NCR5380_host_reset(struct scsi_cmnd *cmd)
+static void bus_reset_cleanup(struct Scsi_Host *instance)
 {
-   struct Scsi_Host *instance = cmd->device->host;
struct NCR5380_hostdata *hostdata = shost_priv(instance);
int i;
-   unsigned long flags;
struct NCR5380_cmd *ncmd;
 
-   spin_lock_irqsave(&hostdata->lock, flags);
-
-#if (NDEBUG & NDEBUG_ANY)
-   shost_printk(KERN_INFO, instance, __func__);
-#endif
-   NCR5380_dprint(NDEBUG_ANY, instance);
-   NCR5380_dprint_phase(NDEBUG_ANY, instance);
-
-   do_reset(instance);
-
/* reset NCR registers */
NCR5380_write(MODE_REG, MR_BASE);
NCR5380_write(TARGET_COMMAND_REG, 0);
@@ -2341,14 +2330,6 @@ static int NCR5380_host_reset(struct scsi_cmnd *cmd)
 * commands!
 */
 
-   list_for_each_entry(ncmd, &hostdata->unissued, list) {
-   struct scsi_cmnd *cmd = NCR5380_to_scmd(ncmd);
-
-   cmd->result = DID_RESET << 16;
-   cmd->scsi_done(cmd);
-   }
-   INIT_LIST_HEAD(&hostdata->unissued);
-
if (hostdata->selecting) {
hostdata->selecting->result = DID_RESET << 16;
complete_cmd(instance, hostdata->selecting);
@@ -2382,6 +2363,41 @@ static int NCR5380_host_reset(struct scsi_cmnd *cmd)
 
queue_work(hostdata->work_q, &hostdata->main_task);
maybe_release_dma_irq(instance);
+}
+
+/**
+ * NCR5380_host_reset - reset the SCSI host
+ * @cmd: SCSI command undergoing EH
+ *
+ * Returns SUCCESS
+ */
+
+static int NCR5380_host_reset(struct scsi_cmnd *cmd)
+{
+   struct Scsi_Host *instance = cmd->device->host;
+   struct NCR5380_hostdata *hostdata = shost_priv(instance);
+   unsigned long flags;
+   struct NCR5380_cmd *ncmd;
+
+   spin_lock_irqsave(&hostdata->lock, flags);
+
+#if (NDEBUG & NDEBUG_ANY)
+   shost_printk(KERN_INFO, instance, __func__);
+#endif
+   NCR5380_dprint(NDEBUG_ANY, instance);
+   NCR5380_dprint_phase(NDEBUG_ANY, instance);
+
+   list_for_each_entry(ncmd, &hostdata->unissued, list) {
+   struct scsi_cmnd *scmd = NCR5380_to_scmd(ncmd);
+
+   scmd->result = DID_RESET << 16;
+   scmd->scsi_done(scmd);
+   }
+   INIT_LIST_HEAD(&hostdata->unissued);
+
+   do_reset(instance);
+   bus_reset_cleanup(instance);
+
spin_unlock_irqrestore(&hostdata->lock, flags);
 
return SUCCESS;
-- 
2.16.4

[PATCH 06/10] NCR5380: Check for invalid reselection target

[Finn Thain]

The X3T9.2 specification (draft) says, under "6.1.4.1 RESELECTION", that
"the initiator shall not respond to a RESELECTION phase if other than two
SCSI ID bits are on the DATA BUS." This issue (too many bits set) has
been observed in the wild, so add a check.

Tested-by: Michael Schmitz 
Signed-off-by: Finn Thain 
---
 drivers/scsi/NCR5380.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index e96a48b9e86c..3058b68b6740 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -2016,6 +2016,11 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
NCR5380_write(MODE_REG, MR_BASE);
 
target_mask = NCR5380_read(CURRENT_SCSI_DATA_REG) & 
~(hostdata->id_mask);
+   if (!target_mask || target_mask & (target_mask - 1)) {
+   shost_printk(KERN_WARNING, instance,
+"reselect: bad target_mask 0x%02x\n", target_mask);
+   return;
+   }
 
dsprintk(NDEBUG_RESELECTION, instance, "reselect\n");
 
-- 
2.16.4

[PATCH 02/10] NCR5380: Reduce goto statements in NCR5380_select()

[Finn Thain]

Replace a 'goto' statement with a simple 'return' where possible.
This improves readability. No functional change.

Tested-by: Michael Schmitz 
Signed-off-by: Finn Thain 
---
 drivers/scsi/NCR5380.c | 21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 80c20ab4fc53..c6d10780febe 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -984,7 +984,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host 
*instance,
if (!hostdata->selecting) {
/* Command was aborted */
NCR5380_write(MODE_REG, MR_BASE);
-   goto out;
+   return NULL;
}
if (err < 0) {
NCR5380_write(MODE_REG, MR_BASE);
@@ -1033,7 +1033,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host 
*instance,
if (!hostdata->selecting) {
NCR5380_write(MODE_REG, MR_BASE);
NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
-   goto out;
+   return NULL;
}
 
dsprintk(NDEBUG_ARBITRATION, instance, "won arbitration\n");
@@ -1116,13 +1116,16 @@ static struct scsi_cmnd *NCR5380_select(struct 
Scsi_Host *instance,
spin_lock_irq(&hostdata->lock);
NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
+
/* Can't touch cmd if it has been reclaimed by the scsi ML */
-   if (hostdata->selecting) {
-   cmd->result = DID_BAD_TARGET << 16;
-   complete_cmd(instance, cmd);
-   dsprintk(NDEBUG_SELECTION, instance, "target did not 
respond within 250ms\n");
-   cmd = NULL;
-   }
+   if (!hostdata->selecting)
+   return NULL;
+
+   cmd->result = DID_BAD_TARGET << 16;
+   complete_cmd(instance, cmd);
+   dsprintk(NDEBUG_SELECTION, instance,
+   "target did not respond within 250ms\n");
+   cmd = NULL;
goto out;
}
 
@@ -1155,7 +1158,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host 
*instance,
}
if (!hostdata->selecting) {
do_abort(instance);
-   goto out;
+   return NULL;
}
 
dsprintk(NDEBUG_SELECTION, instance, "target %d selected, going into 
MESSAGE OUT phase.\n",
-- 
2.16.4

[PATCH 07/10] NCR5380: Don't clear busy flag when abort fails

[Finn Thain]

When NCR5380_abort() returns FAILED, the driver forgets that the target
is still busy. Hence, further commands may be sent to the target, which
may fail during selection and produce the error message, "reselection
after won arbitration?". Prevent this by leaving the busy flag set when
NCR5380_abort() fails.

Tested-by: Michael Schmitz 
Signed-off-by: Finn Thain 
---
Consistent with the rest of NCR5380.c, this patch triggers some
"line over 80 characters" messages from checkpatch.pl. I haven't
addressed those complaints because IMHO the cure is worse than
the disease. Refactoring to reduce indentation levels would be
a lot of churn.
---
 drivers/scsi/NCR5380.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 3058b68b6740..5826421146ba 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -522,8 +522,6 @@ static void complete_cmd(struct Scsi_Host *instance,
hostdata->sensing = NULL;
}
 
-   hostdata->busy[scmd_id(cmd)] &= ~(1 << cmd->device->lun);
-
cmd->scsi_done(cmd);
 }
 
@@ -1713,6 +1711,7 @@ static void NCR5380_information_transfer(struct Scsi_Host 
*instance)
cmd->result = DID_ERROR << 16;
complete_cmd(instance, cmd);
hostdata->connected = NULL;
+   hostdata->busy[scmd_id(cmd)] &= ~(1 << 
cmd->device->lun);
return;
 #endif
case PHASE_DATAIN:
@@ -1795,6 +1794,7 @@ static void NCR5380_information_transfer(struct Scsi_Host 
*instance)
 cmd, scmd_id(cmd), 
cmd->device->lun);
 
hostdata->connected = NULL;
+   hostdata->busy[scmd_id(cmd)] &= ~(1 << 
cmd->device->lun);
 
cmd->result &= ~0x;
cmd->result |= cmd->SCp.Status;
@@ -1953,6 +1953,7 @@ static void NCR5380_information_transfer(struct Scsi_Host 
*instance)
NCR5380_transfer_pio(instance, &phase, &len, 
&data);
if (msgout == ABORT) {
hostdata->connected = NULL;
+   hostdata->busy[scmd_id(cmd)] &= ~(1 << 
cmd->device->lun);
cmd->result = DID_ERROR << 16;
complete_cmd(instance, cmd);
maybe_release_dma_irq(instance);
@@ -2108,13 +2109,16 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
dsprintk(NDEBUG_RESELECTION | NDEBUG_QUEUES, instance,
 "reselect: removed %p from disconnected queue\n", tmp);
} else {
+   int target = ffs(target_mask) - 1;
+
shost_printk(KERN_ERR, instance, "target bitmask 0x%02x lun %d 
not in disconnected queue.\n",
 target_mask, lun);
/*
 * Since we have an established nexus that we can't do anything
 * with, we must abort it.
 */
-   do_abort(instance);
+   if (do_abort(instance) == 0)
+   hostdata->busy[target] &= ~(1 << lun);
return;
}
 
@@ -2285,8 +2289,10 @@ static int NCR5380_abort(struct scsi_cmnd *cmd)
 out:
if (result == FAILED)
dsprintk(NDEBUG_ABORT, instance, "abort: failed to abort %p\n", 
cmd);
-   else
+   else {
+   hostdata->busy[scmd_id(cmd)] &= ~(1 << cmd->device->lun);
dsprintk(NDEBUG_ABORT, instance, "abort: successfully aborted 
%p\n", cmd);
+   }
 
queue_work(hostdata->work_q, &hostdata->main_task);
maybe_release_dma_irq(instance);
-- 
2.16.4