Re: [PATCH] scsi: use set_host_byte instead of open-coding it

[Johannes Thumshirn]

On Wed, Oct 11, 2017 at 03:05:40PM +0200, Steffen Maier wrote:
> Maybe I misunderstand, but doesn't set_host_byte only set the host byte but
> leave the other 3 parts untouched in c->result?
> 
> static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
> {
>   cmd->result = (cmd->result & 0xff00) | (status << 16);
> }
> 
> In contrast, assigning something to c->result resets all parts.
> If so, the semantic patch would introduce a subtle semantic change.
> Unless it's guaranteed that in all the touched cases, c->result always has 0
> for status, message, and driver byte before calling set_host_byte().

Yes it does, another case I'll have to add to my coccinelle spatch, thanks for
reminding me (and I'm also doing it for set_msg_byte() and set_driver_byte(),
the status byte will be done manually probably).

> 
> Bart's suggestion also sounds nice.

Yes I'm working on implementing it currently.

> 
> FYI: Originally, I only thought about using set_host_byte in that one place
> fix of yours; I did not expect a full framework rework.

Oh well:
$ git diff --stat master..
[..]
 80 files changed, 589 insertions(+), 540 deletions(-)

Byte,
Johannes

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] scsi: use set_host_byte instead of open-coding it

[Steffen Maier]

On 10/10/2017 05:29 PM, Johannes Thumshirn wrote:

Call set_host_byte() instead of open-coding it.

Converted using this simple Coccinelle spatch


@@
local idexpression struct scsi_cmnd *c;
expression E1;
@@

- c->result = E1 << 16;
+ set_host_byte(c, E1);


Maybe I misunderstand, but doesn't set_host_byte only set the host byte 
but leave the other 3 parts untouched in c->result?


static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
{
cmd->result = (cmd->result & 0xff00) | (status << 16);
}

In contrast, assigning something to c->result resets all parts.
If so, the semantic patch would introduce a subtle semantic change.
Unless it's guaranteed that in all the touched cases, c->result always 
has 0 for status, message, and driver byte before calling set_host_byte().


Bart's suggestion also sounds nice.

FYI: Originally, I only thought about using set_host_byte in that one 
place fix of yours; I did not expect a full framework rework.


--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] scsi: use set_host_byte instead of open-coding it

[Bart Van Assche]

On Tue, 2017-10-10 at 17:29 +0200, Johannes Thumshirn wrote:
> Call set_host_byte() instead of open-coding it.
> 
> Converted using this simple Coccinelle spatch
> 
> 
> @@
> local idexpression struct scsi_cmnd *c;
> expression E1;
> @@
> 
> - c->result = E1 << 16;
> + set_host_byte(c, E1);
> 

This is useful but I think we should take this a step further. How
about the following approach:
- Introduce four enumeration types - one for the msg byte codes, one
  for the host byte codes, one for the driver byte codes and one for
  SCSI sense codes.
- Update the signatures of functions that store or extract these four
  bytes.
- Change the data type of scsi_cmnd.result into something that makes
  regular assignments trigger a compiler error.

I'm proposing this because there are several drivers that do not assign the
SCSI host byte correctly:
$ git grep -nH '>result = DID_' | grep -v '<< *16' | grep -v ': \*'
drivers/scsi/ips.c:953: scsi_cmd->result = DID_ERROR;
drivers/scsi/ips.c:2657:SC->result = DID_OK;
drivers/scsi/libiscsi.c:1731:   sc->result = DID_REQUEUE;
drivers/scsi/qla2xxx/qla_bsg.c:152: bsg_reply->result = 
DID_OK;
drivers/scsi/qla2xxx/qla_bsg.c:167: 
bsg_reply->result = DID_OK;
drivers/scsi/qla2xxx/qla_bsg.c:184: bsg_reply->result = DID_OK;
drivers/scsi/qla2xxx/qla_bsg.c:236: bsg_reply->result = DID_OK;
drivers/scsi/qla2xxx/qla_bsg.c:976: bsg_reply->result = DID_OK;
drivers/scsi/qla2xxx/qla_bsg.c:1078:bsg_reply->result = DID_OK;
drivers/scsi/qla2xxx/qla_bsg.c:1261:bsg_reply->result = DID_OK;
drivers/scsi/qla2xxx/qla_bsg.c:1375:bsg_reply->result = DID_OK;
drivers/scsi/qla2xxx/qla_bsg.c:1480:bsg_reply->result = DID_OK;
drivers/scsi/qla2xxx/qla_bsg.c:1516:bsg_reply->result = DID_OK;
drivers/scsi/qlogicpti.c:1049:  Cmnd->result = DID_BUS_BUSY;
drivers/scsi/stex.c:615:cmd->result = DID_NO_CONNECT;

Bart.

[PATCH] scsi: use set_host_byte instead of open-coding it

[Johannes Thumshirn]

Call set_host_byte() instead of open-coding it.

Converted using this simple Coccinelle spatch


@@
local idexpression struct scsi_cmnd *c;
expression E1;
@@

- c->result = E1 << 16;
+ set_host_byte(c, E1);


Signed-off-by: Johannes Thumshirn 
Cc: Steffen Maier 
Cc: Tejun Heo 
Cc: Bart Van Assche 
Cc: Oliver Neukum 
Cc: Alan Stern 
---
Martin, this path should be applied on top of
http://lkml.kernel.org/r/20171009113319.9505-1-jthumsh...@suse.de
the above is a bug fix and possibly stable material.
---
 drivers/ata/libata-scsi.c   |  2 +-
 drivers/infiniband/ulp/srp/ib_srp.c |  2 +-
 drivers/message/fusion/mptfc.c  |  4 +-
 drivers/message/fusion/mptsas.c |  2 +-
 drivers/message/fusion/mptscsih.c   | 53 +++---
 drivers/message/fusion/mptspi.c |  4 +-
 drivers/scsi/53c700.c   |  2 +-
 drivers/scsi/BusLogic.c | 12 ++---
 drivers/scsi/NCR5380.c  | 16 +++
 drivers/scsi/aacraid/aachba.c   | 12 ++---
 drivers/scsi/aacraid/linit.c|  4 +-
 drivers/scsi/aic7xxx/aic79xx_osm.c  |  2 +-
 drivers/scsi/aic7xxx/aic7xxx_osm.c  |  2 +-
 drivers/scsi/arcmsr/arcmsr_hba.c|  2 +-
 drivers/scsi/arm/acornscsi.c|  4 +-
 drivers/scsi/bfa/bfad_im.c  |  4 +-
 drivers/scsi/bnx2fc/bnx2fc_io.c |  6 +--
 drivers/scsi/dc395x.c   |  8 ++--
 drivers/scsi/eata.c |  4 +-
 drivers/scsi/eata_pio.c |  4 +-
 drivers/scsi/esas2r/esas2r_main.c   |  8 ++--
 drivers/scsi/esp_scsi.c |  6 +--
 drivers/scsi/fnic/fnic_scsi.c   | 12 ++---
 drivers/scsi/hpsa.c | 46 ++--
 drivers/scsi/hptiop.c   |  2 +-
 drivers/scsi/ibmvscsi/ibmvfc.c  |  2 +-
 drivers/scsi/imm.c  |  2 +-
 drivers/scsi/ips.c  | 16 +++
 drivers/scsi/libfc/fc_fcp.c | 10 ++---
 drivers/scsi/libiscsi.c | 22 +-
 drivers/scsi/libsas/sas_scsi_host.c |  6 +--
 drivers/scsi/megaraid/megaraid_sas_base.c   | 12 ++---
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 12 ++---
 drivers/scsi/mesh.c |  4 +-
 drivers/scsi/mpt3sas/mpt3sas_scsih.c| 58 -
 drivers/scsi/nsp32.c| 32 +++---
 drivers/scsi/pcmcia/nsp_cs.c| 10 ++---
 drivers/scsi/pcmcia/sym53c500_cs.c  | 10 ++---
 drivers/scsi/ppa.c  |  4 +-
 drivers/scsi/ps3rom.c   |  4 +-
 drivers/scsi/qedf/qedf_io.c |  8 ++--
 drivers/scsi/qla2xxx/qla_iocb.c |  4 +-
 drivers/scsi/qla2xxx/qla_isr.c  |  2 +-
 drivers/scsi/qla2xxx/qla_os.c   | 16 +++
 drivers/scsi/qla4xxx/ql4_isr.c  | 22 +-
 drivers/scsi/qla4xxx/ql4_os.c   |  6 +--
 drivers/scsi/qlogicfas408.c |  2 +-
 drivers/scsi/qlogicpti.c|  2 +-
 drivers/scsi/scsi_lib.c |  4 +-
 drivers/scsi/snic/snic_scsi.c   |  2 +-
 drivers/scsi/stex.c |  2 +-
 drivers/scsi/storvsc_drv.c  |  2 +-
 drivers/scsi/wd33c93.c  |  8 ++--
 drivers/scsi/wd719x.c   |  2 +-
 drivers/scsi/xen-scsifront.c|  2 +-
 drivers/staging/rts5208/rtsx.c  |  2 +-
 drivers/staging/rts5208/rtsx_transport.c|  4 +-
 drivers/staging/unisys/visorhba/visorhba_main.c |  8 ++--
 drivers/usb/image/microtek.c|  4 +-
 drivers/usb/storage/isd200.c| 12 ++---
 drivers/usb/storage/scsiglue.c  |  2 +-
 drivers/usb/storage/transport.c | 14 +++---
 drivers/usb/storage/uas.c   | 12 ++---
 63 files changed, 285 insertions(+), 284 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 44ba292f2cd7..37253d5f1502 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4331,7 +4331,7 @@ static inline int __ata_scsi_queuecmd(struct scsi_cmnd 
*scmd,
  bad_cdb_len:
DPRINTK("bad CDB len=%u, scsi_op=0x%02x, max=%u\n",
scmd->cmd_len, scsi_op, dev->cdb_len);
-   scmd->result = DID_ERROR << 16;
+   set_host_byte(scmd, DID_ERROR);
scmd->scsi_done(scmd);
return 0;
 }
diff --git