All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/15] Subject: Protection information and block size cleanup
@ 2021-06-09  3:39 Martin K. Petersen
  2021-06-09  3:39 ` [PATCH 01/15] scsi: core: Add scsi_prot_ref_tag() helper Martin K. Petersen
                   ` (16 more replies)
  0 siblings, 17 replies; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi; +Cc: Martin K. Petersen

This series cleans up how low-level device drivers go about handling
protection information. The SCSI midlayer provides a set of flags and
helpers but not all drivers currently use them. This series updates
the drivers to use the proper calls to query things like the
protection interval, reference tag seed value, etc.

In addition scsi_debug is enhanced to validate and store protection
information the same way as a regular PI-capable disk drive or
SSD. With these changes scsi_debug is now able to pass the same PI
qualification tests as physical hardware.

And finally: Bart's recent series [1] attempted to clean up some of
the confusion between logical block addresses and block layer sector
addresses. Part of Bart's work is included in this series as well as
some fixes to the remainder of the scsi_get_lba() call sites.
Sometimes it is appropriate use sectors, in other cases logical blocks
are a better choice. For instance when printing something with a
reference tag given that the latter typically contains the lower 32
bits of the LBA.

The changes done in this series are also meant to facilitate removing
the request pointer from struct scsi_cmnd [2]. By removing several
references to struct request in the various drivers that transition
will now be easier.

[1] https://lore.kernel.org/linux-scsi/20210513223757.3938-1-bvanassche@acm.org/
[2] https://lore.kernel.org/linux-scsi/20210524030856.2824-1-bvanassche@acm.org/

--
Martin K. Petersen	Oracle Linux Engineering

Bart Van Assche (2):
  scsi: core: Introduce scsi_get_sector()
  scsi: iser: Use scsi_get_sector() instead of scsi_get_lba()

Martin K. Petersen (13):
  scsi: core: Add scsi_prot_ref_tag() helper
  scsi: lpfc: Use the proper SCSI midlayer interfaces for PI
  scsi: qla2xxx: Use the proper SCSI midlayer interfaces for PI
  scsi: mpt3sas: Use the proper SCSI midlayer interfaces for PI
  scsi: mpi3mr: Use the proper SCSI midlayer interfaces for PI
  scsi: zfcp: Use the proper SCSI midlayer interfaces for PI
  scsi: isci: Use the proper SCSI midlayer interfaces for PI
  scsi: scsi_debug: Remove dump_sector()
  scsi: scsi_debug: Improve RDPROTECT/WRPROTECT handling
  scsi: core: Make scsi_get_lba() return the LBA
  scsi: core: Add helper to return number of logical blocks in a request
  scsi: lpfc: Switch to scsi_get_block_count()
  scsi: ufs: core: Use scsi_get_lba() to get LBA

 drivers/infiniband/ulp/iser/iser_verbs.c |   2 +-
 drivers/s390/scsi/zfcp_fsf.c             |   4 +-
 drivers/scsi/isci/request.c              |   4 +-
 drivers/scsi/lpfc/lpfc_scsi.c            | 116 +++++++++--------------
 drivers/scsi/mpi3mr/mpi3mr_os.c          |  59 ++++--------
 drivers/scsi/mpt3sas/mpt3sas_scsih.c     |  28 ++----
 drivers/scsi/qla2xxx/qla_iocb.c          |  84 +++-------------
 drivers/scsi/scsi_debug.c                | 112 +++++++++++++---------
 drivers/scsi/ufs/ufshcd.c                |   3 +-
 include/scsi/scsi_cmnd.h                 |  25 ++++-
 10 files changed, 182 insertions(+), 255 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 01/15] scsi: core: Add scsi_prot_ref_tag() helper
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-09 19:55   ` Bart Van Assche
  2021-06-14 14:33   ` Benjamin Block
  2021-06-09  3:39 ` [PATCH 02/15] scsi: lpfc: Use the proper SCSI midlayer interfaces for PI Martin K. Petersen
                   ` (15 subsequent siblings)
  16 siblings, 2 replies; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi; +Cc: Martin K. Petersen

We are about to remove the request pointer from struct scsi_cmnd and
that will complicate getting to the ref_tag via t10_pi_ref_tag() in
the various drivers. Introduce a helper function to retrieve the
reference tag so drivers will not have to worry about the details.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 include/scsi/scsi_cmnd.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 779a59fe8676..301b9cd4ddd0 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -287,6 +287,13 @@ static inline sector_t scsi_get_lba(struct scsi_cmnd *scmd)
 	return blk_rq_pos(scmd->request);
 }
 
+static inline u32 scsi_prot_ref_tag(struct scsi_cmnd *scmd)
+{
+	struct request *rq = blk_mq_rq_from_pdu(scmd);
+
+	return t10_pi_ref_tag(rq);
+}
+
 static inline unsigned int scsi_prot_interval(struct scsi_cmnd *scmd)
 {
 	return scmd->device->sector_size;
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 02/15] scsi: lpfc: Use the proper SCSI midlayer interfaces for PI
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
  2021-06-09  3:39 ` [PATCH 01/15] scsi: core: Add scsi_prot_ref_tag() helper Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-09  3:39 ` [PATCH 03/15] scsi: qla2xxx: " Martin K. Petersen
                   ` (14 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi; +Cc: Martin K. Petersen

Use the SCSI midlayer interfaces to query protection interval,
reference tag, and per-command DIX flags.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/lpfc/lpfc_scsi.c | 92 +++++++++++++----------------------
 1 file changed, 34 insertions(+), 58 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
index 503919b10a6d..ca2e00e512f4 100644
--- a/drivers/scsi/lpfc/lpfc_scsi.c
+++ b/drivers/scsi/lpfc/lpfc_scsi.c
@@ -87,30 +87,6 @@ lpfc_release_scsi_buf_s3(struct lpfc_hba *phba, struct lpfc_io_buf *psb);
 static int
 lpfc_prot_group_type(struct lpfc_hba *phba, struct scsi_cmnd *sc);
 
-static inline unsigned
-lpfc_cmd_blksize(struct scsi_cmnd *sc)
-{
-	return sc->device->sector_size;
-}
-
-#define LPFC_CHECK_PROTECT_GUARD	1
-#define LPFC_CHECK_PROTECT_REF		2
-static inline unsigned
-lpfc_cmd_protect(struct scsi_cmnd *sc, int flag)
-{
-	return 1;
-}
-
-static inline unsigned
-lpfc_cmd_guard_csum(struct scsi_cmnd *sc)
-{
-	if (lpfc_prot_group_type(NULL, sc) == LPFC_PG_TYPE_NO_DIF)
-		return 0;
-	if (scsi_host_get_guard(sc->device->host) == SHOST_DIX_GUARD_IP)
-		return 1;
-	return 0;
-}
-
 /**
  * lpfc_sli4_set_rsp_sgl_last - Set the last bit in the response sge.
  * @phba: Pointer to HBA object.
@@ -1037,13 +1013,13 @@ lpfc_bg_err_inject(struct lpfc_hba *phba, struct scsi_cmnd *sc,
 		return 0;
 
 	sgpe = scsi_prot_sglist(sc);
-	lba = t10_pi_ref_tag(sc->request);
+	lba = scsi_prot_ref_tag(sc);
 	if (lba == LPFC_INVALID_REFTAG)
 		return 0;
 
 	/* First check if we need to match the LBA */
 	if (phba->lpfc_injerr_lba != LPFC_INJERR_LBA_OFF) {
-		blksize = lpfc_cmd_blksize(sc);
+		blksize = scsi_prot_interval(sc);
 		numblks = (scsi_bufflen(sc) + blksize - 1) / blksize;
 
 		/* Make sure we have the right LBA if one is specified */
@@ -1432,7 +1408,7 @@ lpfc_sc_to_bg_opcodes(struct lpfc_hba *phba, struct scsi_cmnd *sc,
 {
 	uint8_t ret = 0;
 
-	if (lpfc_cmd_guard_csum(sc)) {
+	if (sc->prot_flags & SCSI_PROT_IP_CHECKSUM) {
 		switch (scsi_get_prot_op(sc)) {
 		case SCSI_PROT_READ_INSERT:
 		case SCSI_PROT_WRITE_STRIP:
@@ -1512,7 +1488,7 @@ lpfc_bg_err_opcodes(struct lpfc_hba *phba, struct scsi_cmnd *sc,
 {
 	uint8_t ret = 0;
 
-	if (lpfc_cmd_guard_csum(sc)) {
+	if (sc->prot_flags & SCSI_PROT_IP_CHECKSUM) {
 		switch (scsi_get_prot_op(sc)) {
 		case SCSI_PROT_READ_INSERT:
 		case SCSI_PROT_WRITE_STRIP:
@@ -1620,7 +1596,7 @@ lpfc_bg_setup_bpl(struct lpfc_hba *phba, struct scsi_cmnd *sc,
 		goto out;
 
 	/* extract some info from the scsi command for pde*/
-	reftag = t10_pi_ref_tag(sc->request);
+	reftag = scsi_prot_ref_tag(sc);
 	if (reftag == LPFC_INVALID_REFTAG)
 		goto out;
 
@@ -1659,12 +1635,12 @@ lpfc_bg_setup_bpl(struct lpfc_hba *phba, struct scsi_cmnd *sc,
 	 * protection data is automatically generated, not checked.
 	 */
 	if (datadir == DMA_FROM_DEVICE) {
-		if (lpfc_cmd_protect(sc, LPFC_CHECK_PROTECT_GUARD))
+		if (sc->prot_flags & SCSI_PROT_GUARD_CHECK)
 			bf_set(pde6_ce, pde6, checking);
 		else
 			bf_set(pde6_ce, pde6, 0);
 
-		if (lpfc_cmd_protect(sc, LPFC_CHECK_PROTECT_REF))
+		if (sc->prot_flags & SCSI_PROT_REF_CHECK)
 			bf_set(pde6_re, pde6, checking);
 		else
 			bf_set(pde6_re, pde6, 0);
@@ -1782,8 +1758,8 @@ lpfc_bg_setup_bpl_prot(struct lpfc_hba *phba, struct scsi_cmnd *sc,
 		goto out;
 
 	/* extract some info from the scsi command */
-	blksize = lpfc_cmd_blksize(sc);
-	reftag = t10_pi_ref_tag(sc->request);
+	blksize = scsi_prot_interval(sc);
+	reftag = scsi_prot_ref_tag(sc);
 	if (reftag == LPFC_INVALID_REFTAG)
 		goto out;
 
@@ -1823,12 +1799,12 @@ lpfc_bg_setup_bpl_prot(struct lpfc_hba *phba, struct scsi_cmnd *sc,
 		bf_set(pde6_optx, pde6, txop);
 		bf_set(pde6_oprx, pde6, rxop);
 
-		if (lpfc_cmd_protect(sc, LPFC_CHECK_PROTECT_GUARD))
+		if (sc->prot_flags & SCSI_PROT_GUARD_CHECK)
 			bf_set(pde6_ce, pde6, checking);
 		else
 			bf_set(pde6_ce, pde6, 0);
 
-		if (lpfc_cmd_protect(sc, LPFC_CHECK_PROTECT_REF))
+		if (sc->prot_flags & SCSI_PROT_REF_CHECK)
 			bf_set(pde6_re, pde6, checking);
 		else
 			bf_set(pde6_re, pde6, 0);
@@ -2014,7 +1990,7 @@ lpfc_bg_setup_sgl(struct lpfc_hba *phba, struct scsi_cmnd *sc,
 		goto out;
 
 	/* extract some info from the scsi command for pde*/
-	reftag = t10_pi_ref_tag(sc->request);
+	reftag = scsi_prot_ref_tag(sc);
 	if (reftag == LPFC_INVALID_REFTAG)
 		goto out;
 
@@ -2042,12 +2018,12 @@ lpfc_bg_setup_sgl(struct lpfc_hba *phba, struct scsi_cmnd *sc,
 	 * protection data is automatically generated, not checked.
 	 */
 	if (sc->sc_data_direction == DMA_FROM_DEVICE) {
-		if (lpfc_cmd_protect(sc, LPFC_CHECK_PROTECT_GUARD))
+		if (sc->prot_flags & SCSI_PROT_GUARD_CHECK)
 			bf_set(lpfc_sli4_sge_dif_ce, diseed, checking);
 		else
 			bf_set(lpfc_sli4_sge_dif_ce, diseed, 0);
 
-		if (lpfc_cmd_protect(sc, LPFC_CHECK_PROTECT_REF))
+		if (sc->prot_flags & SCSI_PROT_REF_CHECK)
 			bf_set(lpfc_sli4_sge_dif_re, diseed, checking);
 		else
 			bf_set(lpfc_sli4_sge_dif_re, diseed, 0);
@@ -2214,8 +2190,8 @@ lpfc_bg_setup_sgl_prot(struct lpfc_hba *phba, struct scsi_cmnd *sc,
 		goto out;
 
 	/* extract some info from the scsi command */
-	blksize = lpfc_cmd_blksize(sc);
-	reftag = t10_pi_ref_tag(sc->request);
+	blksize = scsi_prot_interval(sc);
+	reftag = scsi_prot_ref_tag(sc);
 	if (reftag == LPFC_INVALID_REFTAG)
 		goto out;
 
@@ -2272,9 +2248,8 @@ lpfc_bg_setup_sgl_prot(struct lpfc_hba *phba, struct scsi_cmnd *sc,
 		diseed->ref_tag = cpu_to_le32(reftag);
 		diseed->ref_tag_tran = diseed->ref_tag;
 
-		if (lpfc_cmd_protect(sc, LPFC_CHECK_PROTECT_GUARD)) {
+		if (sc->prot_flags & SCSI_PROT_GUARD_CHECK) {
 			bf_set(lpfc_sli4_sge_dif_ce, diseed, checking);
-
 		} else {
 			bf_set(lpfc_sli4_sge_dif_ce, diseed, 0);
 			/*
@@ -2291,7 +2266,7 @@ lpfc_bg_setup_sgl_prot(struct lpfc_hba *phba, struct scsi_cmnd *sc,
 		}
 
 
-		if (lpfc_cmd_protect(sc, LPFC_CHECK_PROTECT_REF))
+		if (sc->prot_flags & SCSI_PROT_REF_CHECK)
 			bf_set(lpfc_sli4_sge_dif_re, diseed, checking);
 		else
 			bf_set(lpfc_sli4_sge_dif_re, diseed, 0);
@@ -2548,7 +2523,7 @@ lpfc_bg_scsi_adjust_dl(struct lpfc_hba *phba,
 	 * DIF (trailer) attached to it. Must ajust FCP data length
 	 * to account for the protection data.
 	 */
-	fcpdl += (fcpdl / lpfc_cmd_blksize(sc)) * 8;
+	fcpdl += (fcpdl / scsi_prot_interval(sc)) * 8;
 
 	return fcpdl;
 }
@@ -2802,14 +2777,14 @@ lpfc_calc_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd)
 		 * data length is a multiple of the blksize.
 		 */
 		sgde = scsi_sglist(cmd);
-		blksize = lpfc_cmd_blksize(cmd);
+		blksize = scsi_prot_interval(cmd);
 		data_src = (uint8_t *)sg_virt(sgde);
 		data_len = sgde->length;
 		if ((data_len & (blksize - 1)) == 0)
 			chk_guard = 1;
 
 		src = (struct scsi_dif_tuple *)sg_virt(sgpe);
-		start_ref_tag = t10_pi_ref_tag(cmd->request);
+		start_ref_tag = scsi_prot_ref_tag(cmd);
 		if (start_ref_tag == LPFC_INVALID_REFTAG)
 			goto out;
 		start_app_tag = src->app_tag;
@@ -2830,7 +2805,8 @@ lpfc_calc_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd)
 				/* First Guard Tag checking */
 				if (chk_guard) {
 					guard_tag = src->guard_tag;
-					if (lpfc_cmd_guard_csum(cmd))
+					if (cmd->prot_flags
+					    & SCSI_PROT_IP_CHECKSUM)
 						sum = lpfc_bg_csum(data_src,
 								   blksize);
 					else
@@ -2901,7 +2877,7 @@ lpfc_calc_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd)
 		phba->bg_guard_err_cnt++;
 		lpfc_printf_log(phba, KERN_WARNING, LOG_FCP | LOG_BG,
 				"9069 BLKGRD: reftag %x grd_tag err %x != %x\n",
-				t10_pi_ref_tag(cmd->request),
+				scsi_prot_ref_tag(cmd),
 				sum, guard_tag);
 
 	} else if (err_type == BGS_REFTAG_ERR_MASK) {
@@ -2911,7 +2887,7 @@ lpfc_calc_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd)
 		phba->bg_reftag_err_cnt++;
 		lpfc_printf_log(phba, KERN_WARNING, LOG_FCP | LOG_BG,
 				"9066 BLKGRD: reftag %x ref_tag err %x != %x\n",
-				t10_pi_ref_tag(cmd->request),
+				scsi_prot_ref_tag(cmd),
 				ref_tag, start_ref_tag);
 
 	} else if (err_type == BGS_APPTAG_ERR_MASK) {
@@ -2921,7 +2897,7 @@ lpfc_calc_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd)
 		phba->bg_apptag_err_cnt++;
 		lpfc_printf_log(phba, KERN_WARNING, LOG_FCP | LOG_BG,
 				"9041 BLKGRD: reftag %x app_tag err %x != %x\n",
-				t10_pi_ref_tag(cmd->request),
+				scsi_prot_ref_tag(cmd),
 				app_tag, start_app_tag);
 	}
 }
@@ -3094,7 +3070,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"9072 BLKGRD: Invalid BG Profile in cmd "
 				"0x%x reftag 0x%x blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
-				t10_pi_ref_tag(cmd->request),
+				scsi_prot_ref_tag(cmd),
 				blk_rq_sectors(cmd->request), bgstat, bghm);
 		ret = (-1);
 		goto out;
@@ -3106,7 +3082,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"9073 BLKGRD: Invalid BG PDIF Block in cmd "
 				"0x%x reftag 0x%x blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
-				t10_pi_ref_tag(cmd->request),
+				scsi_prot_ref_tag(cmd),
 				blk_rq_sectors(cmd->request), bgstat, bghm);
 		ret = (-1);
 		goto out;
@@ -3122,7 +3098,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"9055 BLKGRD: Guard Tag error in cmd "
 				"0x%x reftag 0x%x blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
-				t10_pi_ref_tag(cmd->request),
+				scsi_prot_ref_tag(cmd),
 				blk_rq_sectors(cmd->request), bgstat, bghm);
 	}
 
@@ -3137,7 +3113,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"9056 BLKGRD: Ref Tag error in cmd "
 				"0x%x reftag 0x%x blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
-				t10_pi_ref_tag(cmd->request),
+				scsi_prot_ref_tag(cmd),
 				blk_rq_sectors(cmd->request), bgstat, bghm);
 	}
 
@@ -3152,7 +3128,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"9061 BLKGRD: App Tag error in cmd "
 				"0x%x reftag 0x%x blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
-				t10_pi_ref_tag(cmd->request),
+				scsi_prot_ref_tag(cmd),
 				blk_rq_sectors(cmd->request), bgstat, bghm);
 	}
 
@@ -3196,7 +3172,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"9057 BLKGRD: Unknown error in cmd "
 				"0x%x reftag 0x%x blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
-				t10_pi_ref_tag(cmd->request),
+				scsi_prot_ref_tag(cmd),
 				blk_rq_sectors(cmd->request), bgstat, bghm);
 
 		/* Calcuate what type of error it was */
@@ -5284,7 +5260,7 @@ lpfc_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmnd)
 					 "reftag x%x cnt %u pt %x\n",
 					 dif_op_str[scsi_get_prot_op(cmnd)],
 					 cmnd->cmnd[0],
-					 t10_pi_ref_tag(cmnd->request),
+					 scsi_prot_ref_tag(cmnd),
 					 blk_rq_sectors(cmnd->request),
 					 (cmnd->cmnd[1]>>5));
 		}
@@ -5296,7 +5272,7 @@ lpfc_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmnd)
 					 "9038 BLKGRD: rcvd PROT_NORMAL cmd: "
 					 "x%x reftag x%x cnt %u pt %x\n",
 					 cmnd->cmnd[0],
-					 t10_pi_ref_tag(cmnd->request),
+					 scsi_prot_ref_tag(cmnd),
 					 blk_rq_sectors(cmnd->request),
 					 (cmnd->cmnd[1]>>5));
 		}
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 03/15] scsi: qla2xxx: Use the proper SCSI midlayer interfaces for PI
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
  2021-06-09  3:39 ` [PATCH 01/15] scsi: core: Add scsi_prot_ref_tag() helper Martin K. Petersen
  2021-06-09  3:39 ` [PATCH 02/15] scsi: lpfc: Use the proper SCSI midlayer interfaces for PI Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-17  0:15   ` Arun Easi
  2021-06-09  3:39 ` [PATCH 04/15] scsi: mpt3sas: " Martin K. Petersen
                   ` (13 subsequent siblings)
  16 siblings, 1 reply; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi; +Cc: Martin K. Petersen

Use the SCSI midlayer interfaces to query protection interval,
reference tag, and per-command DIX flags.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/qla2xxx/qla_iocb.c | 84 ++++++---------------------------
 1 file changed, 15 insertions(+), 69 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index 38b5bdde2405..42a6fbba7529 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -145,7 +145,6 @@ inline int
 qla24xx_configure_prot_mode(srb_t *sp, uint16_t *fw_prot_opts)
 {
 	struct scsi_cmnd *cmd = GET_CMD_SP(sp);
-	uint8_t	guard = scsi_host_get_guard(cmd->device->host);
 
 	/* We always use DIFF Bundling for best performance */
 	*fw_prot_opts = 0;
@@ -166,7 +165,7 @@ qla24xx_configure_prot_mode(srb_t *sp, uint16_t *fw_prot_opts)
 		break;
 	case SCSI_PROT_READ_PASS:
 	case SCSI_PROT_WRITE_PASS:
-		if (guard & SHOST_DIX_GUARD_IP)
+		if (cmd->prot_flags & SCSI_PROT_IP_CHECKSUM)
 			*fw_prot_opts |= PO_MODE_DIF_TCP_CKSUM;
 		else
 			*fw_prot_opts |= PO_MODE_DIF_PASS;
@@ -176,6 +175,9 @@ qla24xx_configure_prot_mode(srb_t *sp, uint16_t *fw_prot_opts)
 		break;
 	}
 
+	if (!(cmd->prot_flags & SCSI_PROT_GUARD_CHECK))
+		*fw_prot_opts |= PO_DISABLE_GUARD_CHECK;
+
 	return scsi_prot_sg_count(cmd);
 }
 
@@ -772,74 +774,18 @@ qla24xx_set_t10dif_tags(srb_t *sp, struct fw_dif_context *pkt,
 {
 	struct scsi_cmnd *cmd = GET_CMD_SP(sp);
 
-	switch (scsi_get_prot_type(cmd)) {
-	case SCSI_PROT_DIF_TYPE0:
-		/*
-		 * No check for ql2xenablehba_err_chk, as it would be an
-		 * I/O error if hba tag generation is not done.
-		 */
-		pkt->ref_tag = cpu_to_le32((uint32_t)
-		    (0xffffffff & scsi_get_lba(cmd)));
-
-		if (!qla2x00_hba_err_chk_enabled(sp))
-			break;
-
-		pkt->ref_tag_mask[0] = 0xff;
-		pkt->ref_tag_mask[1] = 0xff;
-		pkt->ref_tag_mask[2] = 0xff;
-		pkt->ref_tag_mask[3] = 0xff;
-		break;
-
-	/*
-	 * For TYPE 2 protection: 16 bit GUARD + 32 bit REF tag has to
-	 * match LBA in CDB + N
-	 */
-	case SCSI_PROT_DIF_TYPE2:
-		pkt->app_tag = cpu_to_le16(0);
-		pkt->app_tag_mask[0] = 0x0;
-		pkt->app_tag_mask[1] = 0x0;
-
-		pkt->ref_tag = cpu_to_le32((uint32_t)
-		    (0xffffffff & scsi_get_lba(cmd)));
+	pkt->ref_tag = cpu_to_le32(scsi_prot_ref_tag(cmd));
 
-		if (!qla2x00_hba_err_chk_enabled(sp))
-			break;
-
-		/* enable ALL bytes of the ref tag */
-		pkt->ref_tag_mask[0] = 0xff;
-		pkt->ref_tag_mask[1] = 0xff;
-		pkt->ref_tag_mask[2] = 0xff;
-		pkt->ref_tag_mask[3] = 0xff;
-		break;
-
-	/* For Type 3 protection: 16 bit GUARD only */
-	case SCSI_PROT_DIF_TYPE3:
-		pkt->ref_tag_mask[0] = pkt->ref_tag_mask[1] =
-			pkt->ref_tag_mask[2] = pkt->ref_tag_mask[3] =
-								0x00;
-		break;
-
-	/*
-	 * For TYpe 1 protection: 16 bit GUARD tag, 32 bit REF tag, and
-	 * 16 bit app tag.
-	 */
-	case SCSI_PROT_DIF_TYPE1:
-		pkt->ref_tag = cpu_to_le32((uint32_t)
-		    (0xffffffff & scsi_get_lba(cmd)));
-		pkt->app_tag = cpu_to_le16(0);
-		pkt->app_tag_mask[0] = 0x0;
-		pkt->app_tag_mask[1] = 0x0;
-
-		if (!qla2x00_hba_err_chk_enabled(sp))
-			break;
-
-		/* enable ALL bytes of the ref tag */
-		pkt->ref_tag_mask[0] = 0xff;
-		pkt->ref_tag_mask[1] = 0xff;
-		pkt->ref_tag_mask[2] = 0xff;
-		pkt->ref_tag_mask[3] = 0xff;
-		break;
+	if (cmd->prot_flags & SCSI_PROT_REF_CHECK) {
+                pkt->ref_tag_mask[0] = 0xff;
+                pkt->ref_tag_mask[1] = 0xff;
+                pkt->ref_tag_mask[2] = 0xff;
+                pkt->ref_tag_mask[3] = 0xff;
 	}
+
+	pkt->app_tag = __constant_cpu_to_le16(0);
+	pkt->app_tag_mask[0] = 0x0;
+	pkt->app_tag_mask[1] = 0x0;
 }
 
 int
@@ -905,7 +851,7 @@ qla24xx_walk_and_build_sglist_no_difb(struct qla_hw_data *ha, srb_t *sp,
 	memset(&sgx, 0, sizeof(struct qla2_sgx));
 	if (sp) {
 		cmd = GET_CMD_SP(sp);
-		prot_int = cmd->device->sector_size;
+		prot_int = scsi_prot_interval(cmd);
 
 		sgx.tot_bytes = scsi_bufflen(cmd);
 		sgx.cur_sg = scsi_sglist(cmd);
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 04/15] scsi: mpt3sas: Use the proper SCSI midlayer interfaces for PI
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
                   ` (2 preceding siblings ...)
  2021-06-09  3:39 ` [PATCH 03/15] scsi: qla2xxx: " Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-09  3:39 ` [PATCH 05/15] scsi: mpi3mr: " Martin K. Petersen
                   ` (12 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi; +Cc: Martin K. Petersen

Use the SCSI midlayer interfaces to query protection interval,
reference tag, and per-command DIX flags.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index dc2aaaf645d3..322450800056 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -5062,33 +5062,17 @@ _scsih_setup_eedp(struct MPT3SAS_ADAPTER *ioc, struct scsi_cmnd *scmd,
 	else
 		return;
 
-	switch (prot_type) {
-	case SCSI_PROT_DIF_TYPE1:
-	case SCSI_PROT_DIF_TYPE2:
+	if (scmd->prot_op & SCSI_PROT_GUARD_CHECK)
+		eedp_flags |= MPI2_SCSIIO_EEDPFLAGS_CHECK_GUARD;
 
-		/*
-		* enable ref/guard checking
-		* auto increment ref tag
-		*/
+	if (scmd->prot_op & SCSI_PROT_REF_CHECK) {
 		eedp_flags |= MPI2_SCSIIO_EEDPFLAGS_INC_PRI_REFTAG |
-		    MPI2_SCSIIO_EEDPFLAGS_CHECK_REFTAG |
-		    MPI2_SCSIIO_EEDPFLAGS_CHECK_GUARD;
+			MPI2_SCSIIO_EEDPFLAGS_CHECK_REFTAG;
 		mpi_request->CDB.EEDP32.PrimaryReferenceTag =
-		    cpu_to_be32(t10_pi_ref_tag(scmd->request));
-		break;
-
-	case SCSI_PROT_DIF_TYPE3:
-
-		/*
-		* enable guard checking
-		*/
-		eedp_flags |= MPI2_SCSIIO_EEDPFLAGS_CHECK_GUARD;
-
-		break;
+			cpu_to_be32(scsi_prot_ref_tag(scmd));
 	}
 
-	mpi_request_3v->EEDPBlockSize =
-	    cpu_to_le16(scmd->device->sector_size);
+	mpi_request_3v->EEDPBlockSize = scsi_prot_interval(scmd);
 
 	if (ioc->is_gen35_ioc)
 		eedp_flags |= MPI25_SCSIIO_EEDPFLAGS_APPTAG_DISABLE_MODE;
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 05/15] scsi: mpi3mr: Use the proper SCSI midlayer interfaces for PI
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
                   ` (3 preceding siblings ...)
  2021-06-09  3:39 ` [PATCH 04/15] scsi: mpt3sas: " Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-09  3:39 ` [PATCH 06/15] scsi: zfcp: " Martin K. Petersen
                   ` (11 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi; +Cc: Martin K. Petersen

Use the SCSI midlayer interfaces to query protection interval,
reference tag, and per-command DIX flags

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/mpi3mr/mpi3mr_os.c | 59 +++++++++++----------------------
 1 file changed, 20 insertions(+), 39 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 4ab0609a1b94..11dcd6930215 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -1962,7 +1962,6 @@ static void mpi3mr_setup_eedp(struct mpi3mr_ioc *mrioc,
 {
 	u16 eedp_flags = 0;
 	unsigned char prot_op = scsi_get_prot_op(scmd);
-	unsigned char prot_type = scsi_get_prot_type(scmd);
 
 	switch (prot_op) {
 	case SCSI_PROT_NORMAL:
@@ -1982,60 +1981,42 @@ static void mpi3mr_setup_eedp(struct mpi3mr_ioc *mrioc,
 		scsiio_req->msg_flags |= MPI3_SCSIIO_MSGFLAGS_METASGL_VALID;
 		break;
 	case SCSI_PROT_READ_PASS:
-		eedp_flags = MPI3_EEDPFLAGS_EEDP_OP_CHECK |
-		    MPI3_EEDPFLAGS_CHK_REF_TAG | MPI3_EEDPFLAGS_CHK_APP_TAG |
-		    MPI3_EEDPFLAGS_CHK_GUARD;
+		eedp_flags = MPI3_EEDPFLAGS_EEDP_OP_CHECK;
 		scsiio_req->msg_flags |= MPI3_SCSIIO_MSGFLAGS_METASGL_VALID;
 		break;
 	case SCSI_PROT_WRITE_PASS:
-		if (scsi_host_get_guard(scmd->device->host)
-		    & SHOST_DIX_GUARD_IP) {
-			eedp_flags = MPI3_EEDPFLAGS_EEDP_OP_CHECK_REGEN |
-			    MPI3_EEDPFLAGS_CHK_APP_TAG |
-			    MPI3_EEDPFLAGS_CHK_GUARD |
-			    MPI3_EEDPFLAGS_INCR_PRI_REF_TAG;
+		if (scmd->prot_flags & SCSI_PROT_IP_CHECKSUM) {
+			eedp_flags = MPI3_EEDPFLAGS_EEDP_OP_CHECK_REGEN;
 			scsiio_req->sgl[0].eedp.application_tag_translation_mask =
 			    0xffff;
-		} else {
-			eedp_flags = MPI3_EEDPFLAGS_EEDP_OP_CHECK |
-			    MPI3_EEDPFLAGS_CHK_REF_TAG |
-			    MPI3_EEDPFLAGS_CHK_APP_TAG |
-			    MPI3_EEDPFLAGS_CHK_GUARD;
-		}
+		} else
+			eedp_flags = MPI3_EEDPFLAGS_EEDP_OP_CHECK;
+
 		scsiio_req->msg_flags |= MPI3_SCSIIO_MSGFLAGS_METASGL_VALID;
 		break;
 	default:
 		return;
 	}
 
-	if (scsi_host_get_guard(scmd->device->host) & SHOST_DIX_GUARD_IP)
+	if (scmd->prot_flags & SCSI_PROT_GUARD_CHECK)
+		eedp_flags |= MPI3_EEDPFLAGS_CHK_GUARD;
+
+	if (scmd->prot_flags & SCSI_PROT_IP_CHECKSUM)
 		eedp_flags |= MPI3_EEDPFLAGS_HOST_GUARD_IP_CHKSUM;
 
-	switch (prot_type) {
-	case SCSI_PROT_DIF_TYPE0:
-		eedp_flags |= MPI3_EEDPFLAGS_INCR_PRI_REF_TAG;
+	if (scmd->prot_flags & SCSI_PROT_REF_CHECK) {
+		eedp_flags |= MPI3_EEDPFLAGS_CHK_REF_TAG |
+			MPI3_EEDPFLAGS_INCR_PRI_REF_TAG;
 		scsiio_req->cdb.eedp32.primary_reference_tag =
-		    cpu_to_be32(t10_pi_ref_tag(scmd->request));
-		break;
-	case SCSI_PROT_DIF_TYPE1:
-	case SCSI_PROT_DIF_TYPE2:
-		eedp_flags |= MPI3_EEDPFLAGS_INCR_PRI_REF_TAG |
-		    MPI3_EEDPFLAGS_ESC_MODE_APPTAG_DISABLE |
-		    MPI3_EEDPFLAGS_CHK_GUARD;
-		scsiio_req->cdb.eedp32.primary_reference_tag =
-		    cpu_to_be32(t10_pi_ref_tag(scmd->request));
-		break;
-	case SCSI_PROT_DIF_TYPE3:
-		eedp_flags |= MPI3_EEDPFLAGS_CHK_GUARD |
-		    MPI3_EEDPFLAGS_ESC_MODE_APPTAG_DISABLE;
-		break;
-
-	default:
-		scsiio_req->msg_flags &= ~(MPI3_SCSIIO_MSGFLAGS_METASGL_VALID);
-		return;
+			cpu_to_be32(scsi_prot_ref_tag(scmd));
 	}
 
-	switch (scmd->device->sector_size) {
+	if (scmd->prot_flags & SCSI_PROT_REF_INCREMENT)
+		eedp_flags |= MPI3_EEDPFLAGS_INCR_PRI_REF_TAG;
+
+	eedp_flags |= MPI3_EEDPFLAGS_ESC_MODE_APPTAG_DISABLE;
+
+	switch (scsi_prot_interval(scmd)) {
 	case 512:
 		scsiio_req->sgl[0].eedp.user_data_size = MPI3_EEDP_UDS_512;
 		break;
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 06/15] scsi: zfcp: Use the proper SCSI midlayer interfaces for PI
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
                   ` (4 preceding siblings ...)
  2021-06-09  3:39 ` [PATCH 05/15] scsi: mpi3mr: " Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-14 14:33   ` Benjamin Block
  2021-06-09  3:39 ` [PATCH 07/15] scsi: isci: " Martin K. Petersen
                   ` (10 subsequent siblings)
  16 siblings, 1 reply; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi; +Cc: Martin K. Petersen

Use scsi_prot_ref_tag() and scsi_prot_interval() instead
scsi_get_lba() and sector_size.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/s390/scsi/zfcp_fsf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index 2e4804ef2fb9..1990216cf289 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -2599,8 +2599,8 @@ int zfcp_fsf_fcp_cmnd(struct scsi_cmnd *scsi_cmnd)
 	io->fcp_cmnd_length = FCP_CMND_LEN;
 
 	if (scsi_get_prot_op(scsi_cmnd) != SCSI_PROT_NORMAL) {
-		io->data_block_length = scsi_cmnd->device->sector_size;
-		io->ref_tag_value = scsi_get_lba(scsi_cmnd) & 0xFFFFFFFF;
+		io->data_block_length = scsi_prot_interval(scsi_cmnd);
+		io->ref_tag_value = scsi_prot_ref_tag(scsi_cmnd);
 	}
 
 	if (zfcp_fsf_set_data_dir(scsi_cmnd, &io->data_direction))
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 07/15] scsi: isci: Use the proper SCSI midlayer interfaces for PI
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
                   ` (5 preceding siblings ...)
  2021-06-09  3:39 ` [PATCH 06/15] scsi: zfcp: " Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-09  3:39 ` [PATCH 08/15] scsi: scsi_debug: Remove dump_sector() Martin K. Petersen
                   ` (9 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi; +Cc: Martin K. Petersen

Use scsi_prot_ref_tag() instead of scsi_get_lba() to get the reference
tag for a given I/O.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/isci/request.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/isci/request.c b/drivers/scsi/isci/request.c
index e1ff79464131..fcaa84a3c210 100644
--- a/drivers/scsi/isci/request.c
+++ b/drivers/scsi/isci/request.c
@@ -341,7 +341,7 @@ static void scu_ssp_ireq_dif_insert(struct isci_request *ireq, u8 type, u8 op)
 	tc->reserved_E8_0 = 0;
 
 	if ((type & SCSI_PROT_DIF_TYPE1) || (type & SCSI_PROT_DIF_TYPE2))
-		tc->ref_tag_seed_gen = scsi_get_lba(scmd) & 0xffffffff;
+		tc->ref_tag_seed_gen = scsi_prot_ref_tag(scmd);
 	else if (type & SCSI_PROT_DIF_TYPE3)
 		tc->ref_tag_seed_gen = 0;
 }
@@ -369,7 +369,7 @@ static void scu_ssp_ireq_dif_strip(struct isci_request *ireq, u8 type, u8 op)
 	tc->app_tag_gen = 0;
 
 	if ((type & SCSI_PROT_DIF_TYPE1) || (type & SCSI_PROT_DIF_TYPE2))
-		tc->ref_tag_seed_verify = scsi_get_lba(scmd) & 0xffffffff;
+		tc->ref_tag_seed_verify = scsi_prot_ref_tag(scmd);
 	else if (type & SCSI_PROT_DIF_TYPE3)
 		tc->ref_tag_seed_verify = 0;
 
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 08/15] scsi: scsi_debug: Remove dump_sector()
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
                   ` (6 preceding siblings ...)
  2021-06-09  3:39 ` [PATCH 07/15] scsi: isci: " Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-09 19:55   ` Bart Van Assche
  2021-06-14 22:06   ` Douglas Gilbert
  2021-06-09  3:39 ` [PATCH 09/15] scsi: scsi_debug: Improve RDPROTECT/WRPROTECT handling Martin K. Petersen
                   ` (8 subsequent siblings)
  16 siblings, 2 replies; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi; +Cc: Martin K. Petersen

The function used to dump sectors containing protection information
errors was useful during initial development over a decade ago.
However, dump_sector() substantially slows down the system during
testing due to writing an entire sector's worth of data to syslog on
every error.

We now log plenty of information about the nature of detected
protection information errors throughout the stack. Dumping the entire
contents of an offending sector is no longer needed.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi_debug.c | 26 +-------------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 5b3a20a140f9..9033ab4911ba 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3232,28 +3232,6 @@ static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	return 0;
 }
 
-static void dump_sector(unsigned char *buf, int len)
-{
-	int i, j, n;
-
-	pr_err(">>> Sector Dump <<<\n");
-	for (i = 0 ; i < len ; i += 16) {
-		char b[128];
-
-		for (j = 0, n = 0; j < 16; j++) {
-			unsigned char c = buf[i+j];
-
-			if (c >= 0x20 && c < 0x7e)
-				n += scnprintf(b + n, sizeof(b) - n,
-					       " %c ", buf[i+j]);
-			else
-				n += scnprintf(b + n, sizeof(b) - n,
-					       "%02x ", buf[i+j]);
-		}
-		pr_err("%04d: %s\n", i, b);
-	}
-}
-
 static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			     unsigned int sectors, u32 ei_lba)
 {
@@ -3300,10 +3278,8 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			daddr = diter.addr + dpage_offset;
 
 			ret = dif_verify(sdt, daddr, sector, ei_lba);
-			if (ret) {
-				dump_sector(daddr, sdebug_sector_size);
+			if (ret)
 				goto out;
-			}
 
 			sector++;
 			ei_lba++;
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 09/15] scsi: scsi_debug: Improve RDPROTECT/WRPROTECT handling
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
                   ` (7 preceding siblings ...)
  2021-06-09  3:39 ` [PATCH 08/15] scsi: scsi_debug: Remove dump_sector() Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-14 22:07   ` Douglas Gilbert
  2021-06-09  3:39 ` [PATCH 10/15] scsi: core: Introduce scsi_get_sector() Martin K. Petersen
                   ` (7 subsequent siblings)
  16 siblings, 1 reply; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi; +Cc: Martin K. Petersen

It is useful for testing purposes to be able to inject errors by
writing bad protection information to media with checking disabled and
then attempting to read it back. Extend scsi_debug's PI verification
logic to give the driver feature parity with commercially available
drives. Almost all devices with PI capability support RDPROTECT and
WRPROTECT values of 0, 1, and 3.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi_debug.c | 90 +++++++++++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 9033ab4911ba..25112b15ab14 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3076,6 +3076,7 @@ static void dif_copy_prot(struct scsi_cmnd *scp, sector_t sector,
 static int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec,
 			    unsigned int sectors, u32 ei_lba)
 {
+	int ret = 0;
 	unsigned int i;
 	sector_t sector;
 	struct sdeb_store_info *sip = devip2sip((struct sdebug_dev_info *)
@@ -3083,26 +3084,33 @@ static int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec,
 	struct t10_pi_tuple *sdt;
 
 	for (i = 0; i < sectors; i++, ei_lba++) {
-		int ret;
-
 		sector = start_sec + i;
 		sdt = dif_store(sip, sector);
 
 		if (sdt->app_tag == cpu_to_be16(0xffff))
 			continue;
 
-		ret = dif_verify(sdt, lba2fake_store(sip, sector), sector,
-				 ei_lba);
-		if (ret) {
-			dif_errors++;
-			return ret;
+		/*
+		 * Because scsi_debug acts as both initiator and
+		 * target we proceed to verify the PI even if
+		 * RDPROTECT=3. This is done so the "initiator" knows
+		 * which type of error to return. Otherwise we would
+		 * have to iterate over the PI twice.
+		 */
+		if (scp->cmnd[1] >> 5) { /* RDPROTECT */
+			ret = dif_verify(sdt, lba2fake_store(sip, sector),
+					 sector, ei_lba);
+			if (ret) {
+				dif_errors++;
+				break;
+			}
 		}
 	}
 
 	dif_copy_prot(scp, start_sec, sectors, true);
 	dix_reads++;
 
-	return 0;
+	return ret;
 }
 
 static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
@@ -3196,12 +3204,29 @@ static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 
 	/* DIX + T10 DIF */
 	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
-		int prot_ret = prot_verify_read(scp, lba, num, ei_lba);
-
-		if (prot_ret) {
-			read_unlock(macc_lckp);
-			mk_sense_buffer(scp, ABORTED_COMMAND, 0x10, prot_ret);
-			return illegal_condition_result;
+		switch (prot_verify_read(scp, lba, num, ei_lba)) {
+		case 1: /* Guard tag error */
+			if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
+				read_unlock(macc_lckp);
+				mk_sense_buffer(scp, ABORTED_COMMAND, 0x10, 1);
+				return check_condition_result;
+			} else if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
+				read_unlock(macc_lckp);
+				mk_sense_buffer(scp, ILLEGAL_REQUEST, 0x10, 1);
+				return illegal_condition_result;
+			}
+			break;
+		case 3: /* Reference tag error */
+			if (cmd[1] >> 5 != 3) { /* RDPROTECT != 3 */
+				read_unlock(macc_lckp);
+				mk_sense_buffer(scp, ABORTED_COMMAND, 0x10, 3);
+				return check_condition_result;
+			} else if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
+				read_unlock(macc_lckp);
+				mk_sense_buffer(scp, ILLEGAL_REQUEST, 0x10, 3);
+				return illegal_condition_result;
+			}
+			break;
 		}
 	}
 
@@ -3277,9 +3302,11 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			sdt = piter.addr + ppage_offset;
 			daddr = diter.addr + dpage_offset;
 
-			ret = dif_verify(sdt, daddr, sector, ei_lba);
-			if (ret)
-				goto out;
+			if (SCpnt->cmnd[1] >> 5 != 3) { /* WRPROTECT */
+				ret = dif_verify(sdt, daddr, sector, ei_lba);
+				if (ret)
+					goto out;
+			}
 
 			sector++;
 			ei_lba++;
@@ -3456,12 +3483,29 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 
 	/* DIX + T10 DIF */
 	if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
-		int prot_ret = prot_verify_write(scp, lba, num, ei_lba);
-
-		if (prot_ret) {
-			write_unlock(macc_lckp);
-			mk_sense_buffer(scp, ILLEGAL_REQUEST, 0x10, prot_ret);
-			return illegal_condition_result;
+		switch (prot_verify_write(scp, lba, num, ei_lba)) {
+		case 1: /* Guard tag error */
+			if (scp->prot_flags & SCSI_PROT_GUARD_CHECK) {
+				write_unlock(macc_lckp);
+				mk_sense_buffer(scp, ILLEGAL_REQUEST, 0x10, 1);
+				return illegal_condition_result;
+			} else if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
+				write_unlock(macc_lckp);
+				mk_sense_buffer(scp, ABORTED_COMMAND, 0x10, 1);
+				return check_condition_result;
+			}
+			break;
+		case 3: /* Reference tag error */
+			if (scp->prot_flags & SCSI_PROT_REF_CHECK) {
+				write_unlock(macc_lckp);
+				mk_sense_buffer(scp, ILLEGAL_REQUEST, 0x10, 3);
+				return illegal_condition_result;
+			} else if (scp->cmnd[1] >> 5 != 3) { /* WRPROTECT != 3 */
+				write_unlock(macc_lckp);
+				mk_sense_buffer(scp, ABORTED_COMMAND, 0x10, 3);
+				return check_condition_result;
+			}
+			break;
 		}
 	}
 
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 10/15] scsi: core: Introduce scsi_get_sector()
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
                   ` (8 preceding siblings ...)
  2021-06-09  3:39 ` [PATCH 09/15] scsi: scsi_debug: Improve RDPROTECT/WRPROTECT handling Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-14 14:48   ` Benjamin Block
  2021-06-09  3:39 ` [PATCH 11/15] scsi: iser: Use scsi_get_sector() instead of scsi_get_lba() Martin K. Petersen
                   ` (6 subsequent siblings)
  16 siblings, 1 reply; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi
  Cc: Bart Van Assche, Christoph Hellwig, Ming Lei, Hannes Reinecke,
	Damien Le Moal, Martin K . Petersen

From: Bart Van Assche <bvanassche@acm.org>

Since scsi_get_lba() returns a sector_t value instead of the LBA, the name
of that function is confusing. Introduce an identical function
scsi_get_sector().

Link: https://lore.kernel.org/r/20210513223757.3938-2-bvanassche@acm.org
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 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 301b9cd4ddd0..cba63377d46a 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -220,6 +220,11 @@ static inline int scsi_sg_copy_to_buffer(struct scsi_cmnd *cmd,
 				 buf, buflen);
 }
 
+static inline sector_t scsi_get_sector(struct scsi_cmnd *scmd)
+{
+	return blk_rq_pos(scmd->request);
+}
+
 /*
  * The operations below are hints that tell the controller driver how
  * to handle I/Os with DIF or similar types of protection information.
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 11/15] scsi: iser: Use scsi_get_sector() instead of scsi_get_lba()
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
                   ` (9 preceding siblings ...)
  2021-06-09  3:39 ` [PATCH 10/15] scsi: core: Introduce scsi_get_sector() Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-09  3:39 ` [PATCH 12/15] scsi: core: Make scsi_get_lba() return the LBA Martin K. Petersen
                   ` (5 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi
  Cc: Bart Van Assche, Sagi Grimberg, Damien Le Moal, Martin K . Petersen

From: Bart Van Assche <bvanassche@acm.org>

Use scsi_get_sector() instead of scsi_get_lba() since the name of the
latter is confusing. This patch does not change any functionality.

Link: https://lore.kernel.org/r/20210513223757.3938-3-bvanassche@acm.org
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/infiniband/ulp/iser/iser_verbs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 136f6c4492e0..d6bbf1bf428c 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -949,7 +949,7 @@ u8 iser_check_task_pi_status(struct iscsi_iser_task *iser_task,
 			sector_t sector_off = mr_status.sig_err.sig_err_offset;
 
 			sector_div(sector_off, sector_size + 8);
-			*sector = scsi_get_lba(iser_task->sc) + sector_off;
+			*sector = scsi_get_sector(iser_task->sc) + sector_off;
 
 			iser_err("PI error found type %d at sector %llx "
 			       "expected %x vs actual %x\n",
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 12/15] scsi: core: Make scsi_get_lba() return the LBA
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
                   ` (10 preceding siblings ...)
  2021-06-09  3:39 ` [PATCH 11/15] scsi: iser: Use scsi_get_sector() instead of scsi_get_lba() Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-09 19:57   ` Bart Van Assche
  2021-06-14 14:50   ` Benjamin Block
  2021-06-09  3:39 ` [PATCH 13/15] scsi: core: Add helper to return number of logical blocks in a request Martin K. Petersen
                   ` (4 subsequent siblings)
  16 siblings, 2 replies; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi; +Cc: Martin K. Petersen

scsi_get_lba() confusingly returned the block layer sector number
expressed in units of 512 bytes. Now that we have a more aptly named
scsi_get_sector() function, make scsi_get_lba() return the actual LBA.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 include/scsi/scsi_cmnd.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index cba63377d46a..90da9617d28a 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -225,6 +225,13 @@ static inline sector_t scsi_get_sector(struct scsi_cmnd *scmd)
 	return blk_rq_pos(scmd->request);
 }
 
+static inline sector_t scsi_get_lba(struct scsi_cmnd *scmd)
+{
+	unsigned int shift = ilog2(scmd->device->sector_size) - SECTOR_SHIFT;
+
+	return blk_rq_pos(scmd->request) >> shift;
+}
+
 /*
  * The operations below are hints that tell the controller driver how
  * to handle I/Os with DIF or similar types of protection information.
@@ -287,11 +294,6 @@ static inline unsigned char scsi_get_prot_type(struct scsi_cmnd *scmd)
 	return scmd->prot_type;
 }
 
-static inline sector_t scsi_get_lba(struct scsi_cmnd *scmd)
-{
-	return blk_rq_pos(scmd->request);
-}
-
 static inline u32 scsi_prot_ref_tag(struct scsi_cmnd *scmd)
 {
 	struct request *rq = blk_mq_rq_from_pdu(scmd);
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 13/15] scsi: core: Add helper to return number of logical blocks in a request
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
                   ` (11 preceding siblings ...)
  2021-06-09  3:39 ` [PATCH 12/15] scsi: core: Make scsi_get_lba() return the LBA Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-09 19:58   ` Bart Van Assche
  2021-06-09  3:39 ` [PATCH 14/15] scsi: lpfc: Switch to scsi_get_block_count() Martin K. Petersen
                   ` (3 subsequent siblings)
  16 siblings, 1 reply; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi; +Cc: Martin K. Petersen

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 include/scsi/scsi_cmnd.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 90da9617d28a..570719237f23 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -232,6 +232,13 @@ static inline sector_t scsi_get_lba(struct scsi_cmnd *scmd)
 	return blk_rq_pos(scmd->request) >> shift;
 }
 
+static inline unsigned int scsi_get_block_count(struct scsi_cmnd *scmd)
+{
+	unsigned int shift = ilog2(scmd->device->sector_size) - SECTOR_SHIFT;
+
+	return blk_rq_bytes(scmd->request) >> shift;
+}
+
 /*
  * The operations below are hints that tell the controller driver how
  * to handle I/Os with DIF or similar types of protection information.
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 14/15] scsi: lpfc: Switch to scsi_get_block_count()
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
                   ` (12 preceding siblings ...)
  2021-06-09  3:39 ` [PATCH 13/15] scsi: core: Add helper to return number of logical blocks in a request Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-09  3:39 ` [PATCH 15/15] scsi: ufs: core: Use scsi_get_lba() to get LBA Martin K. Petersen
                   ` (2 subsequent siblings)
  16 siblings, 0 replies; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi; +Cc: Martin K. Petersen

Use scsi_get_block_count() for places where the I/O size needs to be
expressed in units of the logical block size.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/lpfc/lpfc_scsi.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
index ca2e00e512f4..5f19eccaad24 100644
--- a/drivers/scsi/lpfc/lpfc_scsi.c
+++ b/drivers/scsi/lpfc/lpfc_scsi.c
@@ -2959,7 +2959,7 @@ lpfc_sli4_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				" 0x%x lba 0x%llx blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
 				(unsigned long long)scsi_get_lba(cmd),
-				blk_rq_sectors(cmd->request), bgstat, bghm);
+				scsi_get_block_count(cmd), bgstat, bghm);
 	}
 
 	if (lpfc_bgs_get_reftag_err(bgstat)) {
@@ -2974,7 +2974,7 @@ lpfc_sli4_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				" 0x%x lba 0x%llx blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
 				(unsigned long long)scsi_get_lba(cmd),
-				blk_rq_sectors(cmd->request), bgstat, bghm);
+				scsi_get_block_count(cmd), bgstat, bghm);
 	}
 
 	if (lpfc_bgs_get_apptag_err(bgstat)) {
@@ -2989,7 +2989,7 @@ lpfc_sli4_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				" 0x%x lba 0x%llx blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
 				(unsigned long long)scsi_get_lba(cmd),
-				blk_rq_sectors(cmd->request), bgstat, bghm);
+				scsi_get_block_count(cmd), bgstat, bghm);
 	}
 
 	if (lpfc_bgs_get_hi_water_mark_present(bgstat)) {
@@ -3033,7 +3033,7 @@ lpfc_sli4_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				" 0x%x lba 0x%llx blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
 				(unsigned long long)scsi_get_lba(cmd),
-				blk_rq_sectors(cmd->request), bgstat, bghm);
+				scsi_get_block_count(cmd), bgstat, bghm);
 
 		/* Calcuate what type of error it was */
 		lpfc_calc_bg_err(phba, lpfc_cmd);
@@ -3071,7 +3071,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"0x%x reftag 0x%x blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
 				scsi_prot_ref_tag(cmd),
-				blk_rq_sectors(cmd->request), bgstat, bghm);
+				scsi_get_block_count(cmd), bgstat, bghm);
 		ret = (-1);
 		goto out;
 	}
@@ -3083,7 +3083,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"0x%x reftag 0x%x blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
 				scsi_prot_ref_tag(cmd),
-				blk_rq_sectors(cmd->request), bgstat, bghm);
+				scsi_get_block_count(cmd), bgstat, bghm);
 		ret = (-1);
 		goto out;
 	}
@@ -3099,7 +3099,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"0x%x reftag 0x%x blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
 				scsi_prot_ref_tag(cmd),
-				blk_rq_sectors(cmd->request), bgstat, bghm);
+				scsi_get_block_count(cmd), bgstat, bghm);
 	}
 
 	if (lpfc_bgs_get_reftag_err(bgstat)) {
@@ -3114,7 +3114,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"0x%x reftag 0x%x blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
 				scsi_prot_ref_tag(cmd),
-				blk_rq_sectors(cmd->request), bgstat, bghm);
+				scsi_get_block_count(cmd), bgstat, bghm);
 	}
 
 	if (lpfc_bgs_get_apptag_err(bgstat)) {
@@ -3129,7 +3129,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"0x%x reftag 0x%x blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
 				scsi_prot_ref_tag(cmd),
-				blk_rq_sectors(cmd->request), bgstat, bghm);
+				scsi_get_block_count(cmd), bgstat, bghm);
 	}
 
 	if (lpfc_bgs_get_hi_water_mark_present(bgstat)) {
@@ -3173,7 +3173,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"0x%x reftag 0x%x blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
 				scsi_prot_ref_tag(cmd),
-				blk_rq_sectors(cmd->request), bgstat, bghm);
+				scsi_get_block_count(cmd), bgstat, bghm);
 
 		/* Calcuate what type of error it was */
 		lpfc_calc_bg_err(phba, lpfc_cmd);
@@ -5261,7 +5261,7 @@ lpfc_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmnd)
 					 dif_op_str[scsi_get_prot_op(cmnd)],
 					 cmnd->cmnd[0],
 					 scsi_prot_ref_tag(cmnd),
-					 blk_rq_sectors(cmnd->request),
+					 scsi_get_block_count(cmnd),
 					 (cmnd->cmnd[1]>>5));
 		}
 		err = lpfc_bg_scsi_prep_dma_buf(phba, lpfc_cmd);
@@ -5273,7 +5273,7 @@ lpfc_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmnd)
 					 "x%x reftag x%x cnt %u pt %x\n",
 					 cmnd->cmnd[0],
 					 scsi_prot_ref_tag(cmnd),
-					 blk_rq_sectors(cmnd->request),
+					 scsi_get_block_count(cmnd),
 					 (cmnd->cmnd[1]>>5));
 		}
 		err = lpfc_scsi_prep_dma_buf(phba, lpfc_cmd);
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 15/15] scsi: ufs: core: Use scsi_get_lba() to get LBA
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
                   ` (13 preceding siblings ...)
  2021-06-09  3:39 ` [PATCH 14/15] scsi: lpfc: Switch to scsi_get_block_count() Martin K. Petersen
@ 2021-06-09  3:39 ` Martin K. Petersen
  2021-06-09 19:59   ` Bart Van Assche
  2021-07-16 21:27 ` [PATCH 00/15] Subject: Protection information and block size cleanup Bart Van Assche
  2021-07-24  2:13 ` Martin K. Petersen
  16 siblings, 1 reply; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-09  3:39 UTC (permalink / raw)
  To: linux-scsi; +Cc: Martin K. Petersen

Use the scsi_get_lba() helper instead of a function internal to the
SCSI disk driver. Remove #include "sd.h".

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/ufs/ufshcd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index fe1b5f4b586a..beb52245554f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -25,7 +25,6 @@
 #include "ufs_bsg.h"
 #include "ufshcd-crypto.h"
 #include <asm/unaligned.h>
-#include "../sd.h"
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/ufs.h>
@@ -390,7 +389,7 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
 	/* trace UPIU also */
 	ufshcd_add_cmd_upiu_trace(hba, tag, str_t);
 	opcode = cmd->cmnd[0];
-	lba = sectors_to_logical(cmd->device, blk_rq_pos(cmd->request));
+	lba = scsi_get_lba(cmd);
 
 	if (opcode == READ_10 || opcode == WRITE_10) {
 		/*
-- 
2.31.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 01/15] scsi: core: Add scsi_prot_ref_tag() helper
  2021-06-09  3:39 ` [PATCH 01/15] scsi: core: Add scsi_prot_ref_tag() helper Martin K. Petersen
@ 2021-06-09 19:55   ` Bart Van Assche
  2021-06-14 14:33   ` Benjamin Block
  1 sibling, 0 replies; 39+ messages in thread
From: Bart Van Assche @ 2021-06-09 19:55 UTC (permalink / raw)
  To: Martin K. Petersen, linux-scsi

On 6/8/21 8:39 PM, Martin K. Petersen wrote:
> We are about to remove the request pointer from struct scsi_cmnd and
> that will complicate getting to the ref_tag via t10_pi_ref_tag() in
> the various drivers. Introduce a helper function to retrieve the
> reference tag so drivers will not have to worry about the details.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  include/scsi/scsi_cmnd.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 779a59fe8676..301b9cd4ddd0 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -287,6 +287,13 @@ static inline sector_t scsi_get_lba(struct scsi_cmnd *scmd)
>  	return blk_rq_pos(scmd->request);
>  }
>  
> +static inline u32 scsi_prot_ref_tag(struct scsi_cmnd *scmd)
> +{
> +	struct request *rq = blk_mq_rq_from_pdu(scmd);
> +
> +	return t10_pi_ref_tag(rq);
> +}
> +
>  static inline unsigned int scsi_prot_interval(struct scsi_cmnd *scmd)
>  {
>  	return scmd->device->sector_size;

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 08/15] scsi: scsi_debug: Remove dump_sector()
  2021-06-09  3:39 ` [PATCH 08/15] scsi: scsi_debug: Remove dump_sector() Martin K. Petersen
@ 2021-06-09 19:55   ` Bart Van Assche
  2021-06-14 22:06   ` Douglas Gilbert
  1 sibling, 0 replies; 39+ messages in thread
From: Bart Van Assche @ 2021-06-09 19:55 UTC (permalink / raw)
  To: Martin K. Petersen, linux-scsi

On 6/8/21 8:39 PM, Martin K. Petersen wrote:
> The function used to dump sectors containing protection information
> errors was useful during initial development over a decade ago.
> However, dump_sector() substantially slows down the system during
> testing due to writing an entire sector's worth of data to syslog on
> every error.
> 
> We now log plenty of information about the nature of detected
> protection information errors throughout the stack. Dumping the entire
> contents of an offending sector is no longer needed.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 12/15] scsi: core: Make scsi_get_lba() return the LBA
  2021-06-09  3:39 ` [PATCH 12/15] scsi: core: Make scsi_get_lba() return the LBA Martin K. Petersen
@ 2021-06-09 19:57   ` Bart Van Assche
  2021-06-14 14:50   ` Benjamin Block
  1 sibling, 0 replies; 39+ messages in thread
From: Bart Van Assche @ 2021-06-09 19:57 UTC (permalink / raw)
  To: Martin K. Petersen, linux-scsi

On 6/8/21 8:39 PM, Martin K. Petersen wrote:
> scsi_get_lba() confusingly returned the block layer sector number
> expressed in units of 512 bytes. Now that we have a more aptly named
> scsi_get_sector() function, make scsi_get_lba() return the actual LBA.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 13/15] scsi: core: Add helper to return number of logical blocks in a request
  2021-06-09  3:39 ` [PATCH 13/15] scsi: core: Add helper to return number of logical blocks in a request Martin K. Petersen
@ 2021-06-09 19:58   ` Bart Van Assche
  2021-06-10  3:19     ` Martin K. Petersen
  0 siblings, 1 reply; 39+ messages in thread
From: Bart Van Assche @ 2021-06-09 19:58 UTC (permalink / raw)
  To: Martin K. Petersen, linux-scsi

On 6/8/21 8:39 PM, Martin K. Petersen wrote:
> +static inline unsigned int scsi_get_block_count(struct scsi_cmnd *scmd)
> +{
> +	unsigned int shift = ilog2(scmd->device->sector_size) - SECTOR_SHIFT;
> +
> +	return blk_rq_bytes(scmd->request) >> shift;
> +}

I think we either need a comment above this function that explains that
the return value is a number of logical blocks or to change the function
name to make the meaning of the return value clear.

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 15/15] scsi: ufs: core: Use scsi_get_lba() to get LBA
  2021-06-09  3:39 ` [PATCH 15/15] scsi: ufs: core: Use scsi_get_lba() to get LBA Martin K. Petersen
@ 2021-06-09 19:59   ` Bart Van Assche
  0 siblings, 0 replies; 39+ messages in thread
From: Bart Van Assche @ 2021-06-09 19:59 UTC (permalink / raw)
  To: Martin K. Petersen, linux-scsi

On 6/8/21 8:39 PM, Martin K. Petersen wrote:
> Use the scsi_get_lba() helper instead of a function internal to the
> SCSI disk driver. Remove #include "sd.h".

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 13/15] scsi: core: Add helper to return number of logical blocks in a request
  2021-06-09 19:58   ` Bart Van Assche
@ 2021-06-10  3:19     ` Martin K. Petersen
  2021-06-10  3:40       ` Bart Van Assche
  0 siblings, 1 reply; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-10  3:19 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: Martin K. Petersen, linux-scsi


Hi Bart!

> On 6/8/21 8:39 PM, Martin K. Petersen wrote:
>> +static inline unsigned int scsi_get_block_count(struct scsi_cmnd *scmd)
>> +{
>> +	unsigned int shift = ilog2(scmd->device->sector_size) - SECTOR_SHIFT;
>> +
>> +	return blk_rq_bytes(scmd->request) >> shift;
>> +}
>
> I think we either need a comment above this function that explains that
> the return value is a number of logical blocks or to change the function
> name to make the meaning of the return value clear.

I went with the scsi_get_ prefix to match the scsi_get_sectors() and
scsi_get_lba() calls. Felt that "block" would suffice but I could make
it scsi_logical_block_count() if you prefer?

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 13/15] scsi: core: Add helper to return number of logical blocks in a request
  2021-06-10  3:19     ` Martin K. Petersen
@ 2021-06-10  3:40       ` Bart Van Assche
  0 siblings, 0 replies; 39+ messages in thread
From: Bart Van Assche @ 2021-06-10  3:40 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: linux-scsi

On 6/9/21 8:19 PM, Martin K. Petersen wrote:
>> On 6/8/21 8:39 PM, Martin K. Petersen wrote:
>>> +static inline unsigned int scsi_get_block_count(struct scsi_cmnd *scmd)
>>> +{
>>> +	unsigned int shift = ilog2(scmd->device->sector_size) - SECTOR_SHIFT;
>>> +
>>> +	return blk_rq_bytes(scmd->request) >> shift;
>>> +}
>>
>> I think we either need a comment above this function that explains that
>> the return value is a number of logical blocks or to change the function
>> name to make the meaning of the return value clear.
> 
> I went with the scsi_get_ prefix to match the scsi_get_sectors() and
> scsi_get_lba() calls. Felt that "block" would suffice but I could make
> it scsi_logical_block_count() if you prefer?

The name scsi_logical_block_count() sounds fine to me :-)

Thanks,

Bart.



^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 06/15] scsi: zfcp: Use the proper SCSI midlayer interfaces for PI
  2021-06-09  3:39 ` [PATCH 06/15] scsi: zfcp: " Martin K. Petersen
@ 2021-06-14 14:33   ` Benjamin Block
  2021-06-15  2:27     ` Martin K. Petersen
  0 siblings, 1 reply; 39+ messages in thread
From: Benjamin Block @ 2021-06-14 14:33 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: linux-scsi, Steffen Maier, Fedor Loshakov

Hey Martin,

please set me and Steffen on CC next time for zfcp.

On Tue, Jun 08, 2021 at 11:39:20PM -0400, Martin K. Petersen wrote:
> Use scsi_prot_ref_tag() and scsi_prot_interval() instead
> scsi_get_lba() and sector_size.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/s390/scsi/zfcp_fsf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
> index 2e4804ef2fb9..1990216cf289 100644
> --- a/drivers/s390/scsi/zfcp_fsf.c
> +++ b/drivers/s390/scsi/zfcp_fsf.c
> @@ -2599,8 +2599,8 @@ int zfcp_fsf_fcp_cmnd(struct scsi_cmnd *scsi_cmnd)
>  	io->fcp_cmnd_length = FCP_CMND_LEN;
>  
>  	if (scsi_get_prot_op(scsi_cmnd) != SCSI_PROT_NORMAL) {
> -		io->data_block_length = scsi_cmnd->device->sector_size;
> -		io->ref_tag_value = scsi_get_lba(scsi_cmnd) & 0xFFFFFFFF;
> +		io->data_block_length = scsi_prot_interval(scsi_cmnd);
> +		io->ref_tag_value = scsi_prot_ref_tag(scsi_cmnd);

Reviewed-by: Benjamin Block <bblock@linux.ibm.com>


Out of curiosity, do you have any idea whether there is any storage that
offers DIF with a different Logical Block Size than 512 (I haven't see
any, although, that doesn't say much)? Just re-read some parts of our
HBA specs and we probably would be in trouble, if it does, with how we do
things here.

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 01/15] scsi: core: Add scsi_prot_ref_tag() helper
  2021-06-09  3:39 ` [PATCH 01/15] scsi: core: Add scsi_prot_ref_tag() helper Martin K. Petersen
  2021-06-09 19:55   ` Bart Van Assche
@ 2021-06-14 14:33   ` Benjamin Block
  1 sibling, 0 replies; 39+ messages in thread
From: Benjamin Block @ 2021-06-14 14:33 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: linux-scsi

On Tue, Jun 08, 2021 at 11:39:15PM -0400, Martin K. Petersen wrote:
> We are about to remove the request pointer from struct scsi_cmnd and
> that will complicate getting to the ref_tag via t10_pi_ref_tag() in
> the various drivers. Introduce a helper function to retrieve the
> reference tag so drivers will not have to worry about the details.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  include/scsi/scsi_cmnd.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 779a59fe8676..301b9cd4ddd0 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -287,6 +287,13 @@ static inline sector_t scsi_get_lba(struct scsi_cmnd *scmd)
>  	return blk_rq_pos(scmd->request);
>  }
>  
> +static inline u32 scsi_prot_ref_tag(struct scsi_cmnd *scmd)
> +{
> +	struct request *rq = blk_mq_rq_from_pdu(scmd);
> +
> +	return t10_pi_ref_tag(rq);
> +}
> +
>  static inline unsigned int scsi_prot_interval(struct scsi_cmnd *scmd)
>  {
>  	return scmd->device->sector_size;

Reviewed-by: Benjamin Block <bblock@linux.ibm.com>

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 10/15] scsi: core: Introduce scsi_get_sector()
  2021-06-09  3:39 ` [PATCH 10/15] scsi: core: Introduce scsi_get_sector() Martin K. Petersen
@ 2021-06-14 14:48   ` Benjamin Block
  2021-06-15  2:28     ` Martin K. Petersen
  0 siblings, 1 reply; 39+ messages in thread
From: Benjamin Block @ 2021-06-14 14:48 UTC (permalink / raw)
  To: Martin K. Petersen
  Cc: linux-scsi, Bart Van Assche, Christoph Hellwig, Ming Lei,
	Hannes Reinecke, Damien Le Moal

On Tue, Jun 08, 2021 at 11:39:24PM -0400, Martin K. Petersen wrote:
> From: Bart Van Assche <bvanassche@acm.org>
> 
> Since scsi_get_lba() returns a sector_t value instead of the LBA, the name
> of that function is confusing. Introduce an identical function
> scsi_get_sector().
> 
> Link: https://lore.kernel.org/r/20210513223757.3938-2-bvanassche@acm.org
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  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 301b9cd4ddd0..cba63377d46a 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -220,6 +220,11 @@ static inline int scsi_sg_copy_to_buffer(struct scsi_cmnd *cmd,
>  				 buf, buflen);
>  }
>  
> +static inline sector_t scsi_get_sector(struct scsi_cmnd *scmd)
> +{
> +	return blk_rq_pos(scmd->request);

Wondering a bit why this still uses the request pointer, after you say
in patch 01 that it goes away. So it should probably use
`blk_mq_rq_from_pdu()`?


-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 12/15] scsi: core: Make scsi_get_lba() return the LBA
  2021-06-09  3:39 ` [PATCH 12/15] scsi: core: Make scsi_get_lba() return the LBA Martin K. Petersen
  2021-06-09 19:57   ` Bart Van Assche
@ 2021-06-14 14:50   ` Benjamin Block
  2021-06-15  9:34     ` Benjamin Block
  1 sibling, 1 reply; 39+ messages in thread
From: Benjamin Block @ 2021-06-14 14:50 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: linux-scsi

On Tue, Jun 08, 2021 at 11:39:26PM -0400, Martin K. Petersen wrote:
> scsi_get_lba() confusingly returned the block layer sector number
> expressed in units of 512 bytes. Now that we have a more aptly named
> scsi_get_sector() function, make scsi_get_lba() return the actual LBA.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  include/scsi/scsi_cmnd.h | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index cba63377d46a..90da9617d28a 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -225,6 +225,13 @@ static inline sector_t scsi_get_sector(struct scsi_cmnd *scmd)
>  	return blk_rq_pos(scmd->request);
>  }
>  
> +static inline sector_t scsi_get_lba(struct scsi_cmnd *scmd)
> +{
> +	unsigned int shift = ilog2(scmd->device->sector_size) - SECTOR_SHIFT;
> +
> +	return blk_rq_pos(scmd->request) >> shift;

Hmm again, should it use `blk_mq_rq_from_pdu()`?

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 08/15] scsi: scsi_debug: Remove dump_sector()
  2021-06-09  3:39 ` [PATCH 08/15] scsi: scsi_debug: Remove dump_sector() Martin K. Petersen
  2021-06-09 19:55   ` Bart Van Assche
@ 2021-06-14 22:06   ` Douglas Gilbert
  1 sibling, 0 replies; 39+ messages in thread
From: Douglas Gilbert @ 2021-06-14 22:06 UTC (permalink / raw)
  To: Martin K. Petersen, linux-scsi

On 2021-06-08 11:39 p.m., Martin K. Petersen wrote:
> The function used to dump sectors containing protection information
> errors was useful during initial development over a decade ago.
> However, dump_sector() substantially slows down the system during
> testing due to writing an entire sector's worth of data to syslog on
> every error.
> 
> We now log plenty of information about the nature of detected
> protection information errors throughout the stack. Dumping the entire
> contents of an offending sector is no longer needed.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

Reviewed-by: Douglas Gilbert <dgilbert@interlog.com>


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 09/15] scsi: scsi_debug: Improve RDPROTECT/WRPROTECT handling
  2021-06-09  3:39 ` [PATCH 09/15] scsi: scsi_debug: Improve RDPROTECT/WRPROTECT handling Martin K. Petersen
@ 2021-06-14 22:07   ` Douglas Gilbert
  0 siblings, 0 replies; 39+ messages in thread
From: Douglas Gilbert @ 2021-06-14 22:07 UTC (permalink / raw)
  To: Martin K. Petersen, linux-scsi

On 2021-06-08 11:39 p.m., Martin K. Petersen wrote:
> It is useful for testing purposes to be able to inject errors by
> writing bad protection information to media with checking disabled and
> then attempting to read it back. Extend scsi_debug's PI verification
> logic to give the driver feature parity with commercially available
> drives. Almost all devices with PI capability support RDPROTECT and
> WRPROTECT values of 0, 1, and 3.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

Reviewed-by: Douglas Gilbert <dgilbert@interlog.com>



^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 06/15] scsi: zfcp: Use the proper SCSI midlayer interfaces for PI
  2021-06-14 14:33   ` Benjamin Block
@ 2021-06-15  2:27     ` Martin K. Petersen
  2021-06-15  9:31       ` Benjamin Block
  0 siblings, 1 reply; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-15  2:27 UTC (permalink / raw)
  To: Benjamin Block
  Cc: Martin K. Petersen, linux-scsi, Steffen Maier, Fedor Loshakov


Benjamin,

> please set me and Steffen on CC next time for zfcp.

Sure.

> Out of curiosity, do you have any idea whether there is any storage
> that offers DIF with a different Logical Block Size than 512 (I
> haven't see any, although, that doesn't say much)? Just re-read some
> parts of our HBA specs and we probably would be in trouble, if it
> does, with how we do things here.

I have a few that are 4Kn+8. It wouldn't say these are very common, the
16-bit CRC isn't that great with 4K blocks.

To address the block size issue we defined a couple of new protection
formats in NVMe. These allow for 32 and 64-bit CRCs, larger reference
tags, etc. However, these enhancements have yet to percolate down into
SCSI/SBC.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 10/15] scsi: core: Introduce scsi_get_sector()
  2021-06-14 14:48   ` Benjamin Block
@ 2021-06-15  2:28     ` Martin K. Petersen
  2021-06-15  9:33       ` Benjamin Block
  0 siblings, 1 reply; 39+ messages in thread
From: Martin K. Petersen @ 2021-06-15  2:28 UTC (permalink / raw)
  To: Benjamin Block
  Cc: Martin K. Petersen, linux-scsi, Bart Van Assche,
	Christoph Hellwig, Ming Lei, Hannes Reinecke, Damien Le Moal


Benjamin,

> Wondering a bit why this still uses the request pointer, after you say
> in patch 01 that it goes away. So it should probably use
> `blk_mq_rq_from_pdu()`?

I was just getting everything ready for Bart's series that makes this
change throughout the tree.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 06/15] scsi: zfcp: Use the proper SCSI midlayer interfaces for PI
  2021-06-15  2:27     ` Martin K. Petersen
@ 2021-06-15  9:31       ` Benjamin Block
  0 siblings, 0 replies; 39+ messages in thread
From: Benjamin Block @ 2021-06-15  9:31 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: linux-scsi, Steffen Maier, Fedor Loshakov

On Mon, Jun 14, 2021 at 10:27:57PM -0400, Martin K. Petersen wrote:
> > Out of curiosity, do you have any idea whether there is any storage
> > that offers DIF with a different Logical Block Size than 512 (I
> > haven't see any, although, that doesn't say much)? Just re-read some
> > parts of our HBA specs and we probably would be in trouble, if it
> > does, with how we do things here.
> 
> I have a few that are 4Kn+8. It wouldn't say these are very common, the
> 16-bit CRC isn't that great with 4K blocks.
> 
> To address the block size issue we defined a couple of new protection
> formats in NVMe. These allow for 32 and 64-bit CRCs, larger reference
> tags, etc. However, these enhancements have yet to percolate down into
> SCSI/SBC.

Oh, interesting. Thanks for sharing.

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 10/15] scsi: core: Introduce scsi_get_sector()
  2021-06-15  2:28     ` Martin K. Petersen
@ 2021-06-15  9:33       ` Benjamin Block
  0 siblings, 0 replies; 39+ messages in thread
From: Benjamin Block @ 2021-06-15  9:33 UTC (permalink / raw)
  To: Martin K. Petersen
  Cc: linux-scsi, Bart Van Assche, Christoph Hellwig, Ming Lei,
	Hannes Reinecke, Damien Le Moal

On Mon, Jun 14, 2021 at 10:28:52PM -0400, Martin K. Petersen wrote:
> > Wondering a bit why this still uses the request pointer, after you say
> > in patch 01 that it goes away. So it should probably use
> > `blk_mq_rq_from_pdu()`?
> 
> I was just getting everything ready for Bart's series that makes this
> change throughout the tree.
> 

Ah ok, didn't realize that.


Reviewed-by: Benjamin Block <bblock@linux.ibm.com>

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 12/15] scsi: core: Make scsi_get_lba() return the LBA
  2021-06-14 14:50   ` Benjamin Block
@ 2021-06-15  9:34     ` Benjamin Block
  0 siblings, 0 replies; 39+ messages in thread
From: Benjamin Block @ 2021-06-15  9:34 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: linux-scsi

On Mon, Jun 14, 2021 at 04:50:12PM +0200, Benjamin Block wrote:
> On Tue, Jun 08, 2021 at 11:39:26PM -0400, Martin K. Petersen wrote:
> > scsi_get_lba() confusingly returned the block layer sector number
> > expressed in units of 512 bytes. Now that we have a more aptly named
> > scsi_get_sector() function, make scsi_get_lba() return the actual LBA.
> > 
> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> > ---
> >  include/scsi/scsi_cmnd.h | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> > index cba63377d46a..90da9617d28a 100644
> > --- a/include/scsi/scsi_cmnd.h
> > +++ b/include/scsi/scsi_cmnd.h
> > @@ -225,6 +225,13 @@ static inline sector_t scsi_get_sector(struct scsi_cmnd *scmd)
> >  	return blk_rq_pos(scmd->request);
> >  }
> >  
> > +static inline sector_t scsi_get_lba(struct scsi_cmnd *scmd)
> > +{
> > +	unsigned int shift = ilog2(scmd->device->sector_size) - SECTOR_SHIFT;
> > +
> > +	return blk_rq_pos(scmd->request) >> shift;
> 
> Hmm again, should it use `blk_mq_rq_from_pdu()`?
> 

I'm taking this is the same as with the other patch, Bart's series will
replace that as well.


Reviewed-by: Benjamin Block <bblock@linux.ibm.com>

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 03/15] scsi: qla2xxx: Use the proper SCSI midlayer interfaces for PI
  2021-06-09  3:39 ` [PATCH 03/15] scsi: qla2xxx: " Martin K. Petersen
@ 2021-06-17  0:15   ` Arun Easi
  0 siblings, 0 replies; 39+ messages in thread
From: Arun Easi @ 2021-06-17  0:15 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: linux-scsi

On Tue, 8 Jun 2021, 8:39pm, Martin K. Petersen wrote:

> Use the SCSI midlayer interfaces to query protection interval,
> reference tag, and per-command DIX flags.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/qla2xxx/qla_iocb.c | 84 ++++++---------------------------
>  1 file changed, 15 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> index 38b5bdde2405..42a6fbba7529 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> @@ -145,7 +145,6 @@ inline int
>  qla24xx_configure_prot_mode(srb_t *sp, uint16_t *fw_prot_opts)
>  {
>  	struct scsi_cmnd *cmd = GET_CMD_SP(sp);
> -	uint8_t	guard = scsi_host_get_guard(cmd->device->host);
>  
>  	/* We always use DIFF Bundling for best performance */
>  	*fw_prot_opts = 0;
> @@ -166,7 +165,7 @@ qla24xx_configure_prot_mode(srb_t *sp, uint16_t *fw_prot_opts)
>  		break;
>  	case SCSI_PROT_READ_PASS:
>  	case SCSI_PROT_WRITE_PASS:
> -		if (guard & SHOST_DIX_GUARD_IP)
> +		if (cmd->prot_flags & SCSI_PROT_IP_CHECKSUM)
>  			*fw_prot_opts |= PO_MODE_DIF_TCP_CKSUM;
>  		else
>  			*fw_prot_opts |= PO_MODE_DIF_PASS;
> @@ -176,6 +175,9 @@ qla24xx_configure_prot_mode(srb_t *sp, uint16_t *fw_prot_opts)
>  		break;
>  	}
>  
> +	if (!(cmd->prot_flags & SCSI_PROT_GUARD_CHECK))
> +		*fw_prot_opts |= PO_DISABLE_GUARD_CHECK;
> +
>  	return scsi_prot_sg_count(cmd);
>  }
>  
> @@ -772,74 +774,18 @@ qla24xx_set_t10dif_tags(srb_t *sp, struct fw_dif_context *pkt,
>  {
>  	struct scsi_cmnd *cmd = GET_CMD_SP(sp);
>  
> -	switch (scsi_get_prot_type(cmd)) {
> -	case SCSI_PROT_DIF_TYPE0:
> -		/*
> -		 * No check for ql2xenablehba_err_chk, as it would be an
> -		 * I/O error if hba tag generation is not done.
> -		 */
> -		pkt->ref_tag = cpu_to_le32((uint32_t)
> -		    (0xffffffff & scsi_get_lba(cmd)));
> -
> -		if (!qla2x00_hba_err_chk_enabled(sp))
> -			break;
> -
> -		pkt->ref_tag_mask[0] = 0xff;
> -		pkt->ref_tag_mask[1] = 0xff;
> -		pkt->ref_tag_mask[2] = 0xff;
> -		pkt->ref_tag_mask[3] = 0xff;
> -		break;
> -
> -	/*
> -	 * For TYPE 2 protection: 16 bit GUARD + 32 bit REF tag has to
> -	 * match LBA in CDB + N
> -	 */
> -	case SCSI_PROT_DIF_TYPE2:
> -		pkt->app_tag = cpu_to_le16(0);
> -		pkt->app_tag_mask[0] = 0x0;
> -		pkt->app_tag_mask[1] = 0x0;
> -
> -		pkt->ref_tag = cpu_to_le32((uint32_t)
> -		    (0xffffffff & scsi_get_lba(cmd)));
> +	pkt->ref_tag = cpu_to_le32(scsi_prot_ref_tag(cmd));
>  
> -		if (!qla2x00_hba_err_chk_enabled(sp))
> -			break;
> -
> -		/* enable ALL bytes of the ref tag */
> -		pkt->ref_tag_mask[0] = 0xff;
> -		pkt->ref_tag_mask[1] = 0xff;
> -		pkt->ref_tag_mask[2] = 0xff;
> -		pkt->ref_tag_mask[3] = 0xff;
> -		break;
> -
> -	/* For Type 3 protection: 16 bit GUARD only */
> -	case SCSI_PROT_DIF_TYPE3:
> -		pkt->ref_tag_mask[0] = pkt->ref_tag_mask[1] =
> -			pkt->ref_tag_mask[2] = pkt->ref_tag_mask[3] =
> -								0x00;
> -		break;
> -
> -	/*
> -	 * For TYpe 1 protection: 16 bit GUARD tag, 32 bit REF tag, and
> -	 * 16 bit app tag.
> -	 */
> -	case SCSI_PROT_DIF_TYPE1:
> -		pkt->ref_tag = cpu_to_le32((uint32_t)
> -		    (0xffffffff & scsi_get_lba(cmd)));
> -		pkt->app_tag = cpu_to_le16(0);
> -		pkt->app_tag_mask[0] = 0x0;
> -		pkt->app_tag_mask[1] = 0x0;
> -
> -		if (!qla2x00_hba_err_chk_enabled(sp))
> -			break;
> -
> -		/* enable ALL bytes of the ref tag */
> -		pkt->ref_tag_mask[0] = 0xff;
> -		pkt->ref_tag_mask[1] = 0xff;
> -		pkt->ref_tag_mask[2] = 0xff;
> -		pkt->ref_tag_mask[3] = 0xff;
> -		break;
> +	if (cmd->prot_flags & SCSI_PROT_REF_CHECK) {

I would also add a "&& qla2x00_hba_err_chk_enabled(sp)" to the above check 
to preserve the old semantics (which was helpful during testing).

With that:

Reviewed-by: Arun Easi <aeasi@marvell.com>

Regards,
-Arun


> +                pkt->ref_tag_mask[0] = 0xff;
> +                pkt->ref_tag_mask[1] = 0xff;
> +                pkt->ref_tag_mask[2] = 0xff;
> +                pkt->ref_tag_mask[3] = 0xff;
>  	}
> +
> +	pkt->app_tag = __constant_cpu_to_le16(0);
> +	pkt->app_tag_mask[0] = 0x0;
> +	pkt->app_tag_mask[1] = 0x0;
>  }
>  
>  int
> @@ -905,7 +851,7 @@ qla24xx_walk_and_build_sglist_no_difb(struct qla_hw_data *ha, srb_t *sp,
>  	memset(&sgx, 0, sizeof(struct qla2_sgx));
>  	if (sp) {
>  		cmd = GET_CMD_SP(sp);
> -		prot_int = cmd->device->sector_size;
> +		prot_int = scsi_prot_interval(cmd);
>  
>  		sgx.tot_bytes = scsi_bufflen(cmd);
>  		sgx.cur_sg = scsi_sglist(cmd);
> 

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/15] Subject: Protection information and block size cleanup
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
                   ` (14 preceding siblings ...)
  2021-06-09  3:39 ` [PATCH 15/15] scsi: ufs: core: Use scsi_get_lba() to get LBA Martin K. Petersen
@ 2021-07-16 21:27 ` Bart Van Assche
  2021-07-19  1:43   ` Martin K. Petersen
  2021-07-24  2:13 ` Martin K. Petersen
  16 siblings, 1 reply; 39+ messages in thread
From: Bart Van Assche @ 2021-07-16 21:27 UTC (permalink / raw)
  To: Martin K. Petersen, linux-scsi

On 6/8/21 8:39 PM, Martin K. Petersen wrote:
> This series cleans up how low-level device drivers go about handling
> protection information. The SCSI midlayer provides a set of flags and
> helpers but not all drivers currently use them. This series updates
> the drivers to use the proper calls to query things like the
> protection interval, reference tag seed value, etc.

Hi Martin,

Do you plan to queue or repost this patch series?

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/15] Subject: Protection information and block size cleanup
  2021-07-16 21:27 ` [PATCH 00/15] Subject: Protection information and block size cleanup Bart Van Assche
@ 2021-07-19  1:43   ` Martin K. Petersen
  2021-07-21  3:53     ` Bart Van Assche
  0 siblings, 1 reply; 39+ messages in thread
From: Martin K. Petersen @ 2021-07-19  1:43 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: Martin K. Petersen, linux-scsi


Bart,

> Do you plan to queue or repost this patch series?

Yes. Still haven't pushed my 5.15 tree because I had several issues with
-rc1, including the configfs bug which broke most of my test setup. I am
rebasing to -rc2.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/15] Subject: Protection information and block size cleanup
  2021-07-19  1:43   ` Martin K. Petersen
@ 2021-07-21  3:53     ` Bart Van Assche
  0 siblings, 0 replies; 39+ messages in thread
From: Bart Van Assche @ 2021-07-21  3:53 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: linux-scsi

On 7/18/21 6:43 PM, Martin K. Petersen wrote:
> Yes. Still haven't pushed my 5.15 tree because I had several issues with
> -rc1, including the configfs bug which broke most of my test setup.
Hi Martin,

To test my configfs patch I used LIO scripts that I wrote many years
ago. It was only after I received a bug report that I realized that
these scripts only write configfs attributes but do not read any
configfs attribute. The configfs breakage shouldn't have happened but
this explains how read testing got overlooked.

Bart.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 00/15] Subject: Protection information and block size cleanup
  2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
                   ` (15 preceding siblings ...)
  2021-07-16 21:27 ` [PATCH 00/15] Subject: Protection information and block size cleanup Bart Van Assche
@ 2021-07-24  2:13 ` Martin K. Petersen
  16 siblings, 0 replies; 39+ messages in thread
From: Martin K. Petersen @ 2021-07-24  2:13 UTC (permalink / raw)
  To: linux-scsi, Martin K. Petersen

On Tue, 8 Jun 2021 23:39:14 -0400, Martin K. Petersen wrote:

> This series cleans up how low-level device drivers go about handling
> protection information. The SCSI midlayer provides a set of flags and
> helpers but not all drivers currently use them. This series updates
> the drivers to use the proper calls to query things like the
> protection interval, reference tag seed value, etc.
> 
> In addition scsi_debug is enhanced to validate and store protection
> information the same way as a regular PI-capable disk drive or
> SSD. With these changes scsi_debug is now able to pass the same PI
> qualification tests as physical hardware.
> 
> [...]

Applied to 5.15/scsi-queue, thanks!

[01/15] scsi: core: Add scsi_prot_ref_tag() helper
        https://git.kernel.org/mkp/scsi/c/7ba46799d346
[03/15] scsi: qla2xxx: Use the proper SCSI midlayer interfaces for PI
        https://git.kernel.org/mkp/scsi/c/e2e9cd68fb3c
[06/15] scsi: zfcp: Use the proper SCSI midlayer interfaces for PI
        https://git.kernel.org/mkp/scsi/c/73e61d5c22bf
[08/15] scsi: scsi_debug: Remove dump_sector()
        https://git.kernel.org/mkp/scsi/c/c78be80d20cd
[09/15] scsi: scsi_debug: Improve RDPROTECT/WRPROTECT handling
        https://git.kernel.org/mkp/scsi/c/f7be677227a5
[10/15] scsi: core: Introduce scsi_get_sector()
        https://git.kernel.org/mkp/scsi/c/f0f214fe8cd3
[11/15] scsi: iser: Use scsi_get_sector() instead of scsi_get_lba()
        https://git.kernel.org/mkp/scsi/c/87662a472a9d
[12/15] scsi: core: Make scsi_get_lba() return the LBA
        https://git.kernel.org/mkp/scsi/c/d2c945f01d23
[15/15] scsi: ufs: core: Use scsi_get_lba() to get LBA
        https://git.kernel.org/mkp/scsi/c/54815088859f

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, other threads:[~2021-07-24  2:15 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-09  3:39 [PATCH 00/15] Subject: Protection information and block size cleanup Martin K. Petersen
2021-06-09  3:39 ` [PATCH 01/15] scsi: core: Add scsi_prot_ref_tag() helper Martin K. Petersen
2021-06-09 19:55   ` Bart Van Assche
2021-06-14 14:33   ` Benjamin Block
2021-06-09  3:39 ` [PATCH 02/15] scsi: lpfc: Use the proper SCSI midlayer interfaces for PI Martin K. Petersen
2021-06-09  3:39 ` [PATCH 03/15] scsi: qla2xxx: " Martin K. Petersen
2021-06-17  0:15   ` Arun Easi
2021-06-09  3:39 ` [PATCH 04/15] scsi: mpt3sas: " Martin K. Petersen
2021-06-09  3:39 ` [PATCH 05/15] scsi: mpi3mr: " Martin K. Petersen
2021-06-09  3:39 ` [PATCH 06/15] scsi: zfcp: " Martin K. Petersen
2021-06-14 14:33   ` Benjamin Block
2021-06-15  2:27     ` Martin K. Petersen
2021-06-15  9:31       ` Benjamin Block
2021-06-09  3:39 ` [PATCH 07/15] scsi: isci: " Martin K. Petersen
2021-06-09  3:39 ` [PATCH 08/15] scsi: scsi_debug: Remove dump_sector() Martin K. Petersen
2021-06-09 19:55   ` Bart Van Assche
2021-06-14 22:06   ` Douglas Gilbert
2021-06-09  3:39 ` [PATCH 09/15] scsi: scsi_debug: Improve RDPROTECT/WRPROTECT handling Martin K. Petersen
2021-06-14 22:07   ` Douglas Gilbert
2021-06-09  3:39 ` [PATCH 10/15] scsi: core: Introduce scsi_get_sector() Martin K. Petersen
2021-06-14 14:48   ` Benjamin Block
2021-06-15  2:28     ` Martin K. Petersen
2021-06-15  9:33       ` Benjamin Block
2021-06-09  3:39 ` [PATCH 11/15] scsi: iser: Use scsi_get_sector() instead of scsi_get_lba() Martin K. Petersen
2021-06-09  3:39 ` [PATCH 12/15] scsi: core: Make scsi_get_lba() return the LBA Martin K. Petersen
2021-06-09 19:57   ` Bart Van Assche
2021-06-14 14:50   ` Benjamin Block
2021-06-15  9:34     ` Benjamin Block
2021-06-09  3:39 ` [PATCH 13/15] scsi: core: Add helper to return number of logical blocks in a request Martin K. Petersen
2021-06-09 19:58   ` Bart Van Assche
2021-06-10  3:19     ` Martin K. Petersen
2021-06-10  3:40       ` Bart Van Assche
2021-06-09  3:39 ` [PATCH 14/15] scsi: lpfc: Switch to scsi_get_block_count() Martin K. Petersen
2021-06-09  3:39 ` [PATCH 15/15] scsi: ufs: core: Use scsi_get_lba() to get LBA Martin K. Petersen
2021-06-09 19:59   ` Bart Van Assche
2021-07-16 21:27 ` [PATCH 00/15] Subject: Protection information and block size cleanup Bart Van Assche
2021-07-19  1:43   ` Martin K. Petersen
2021-07-21  3:53     ` Bart Van Assche
2021-07-24  2:13 ` Martin K. Petersen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.