[PATCH 0/9] scsi: sd/sd_zbc cleanup and improvements

[PATCH 0/9] scsi: sd/sd_zbc cleanup and improvements

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

This series of small patches cleanup code mainly in sd.c and sd_zbc.c, with
another small improvement in scsi_error.c.

The first 2 patches are not stricktly necessary but would be great to apply to
make emacs pretty printer happy and reduce its overhead (if white space
highlighting is enabled).

The last patch is the only one that introduces a functional change. All other
patches do not change any functionality.

Comments and reviews are welcome.

Bart Van Assche (1):
  sd_zbc: Remove superfluous assignments

Damien Le Moal (8):
  sd: Remove white space before EOL
  sd: Fix functions description
  sd: Remove unecessary variable in sd_done()
  sd: Improve sd_completed_bytes
  sd: Cleanup sd_done sense data handling
  scsi: Improve scsi_get_sense_info_fld
  sd_zbc: Rename sd_zbc_setup_write_cmnd
  sd_zbc: Do not write lock zones for reset

 drivers/scsi/scsi_error.c |  36 ++---
 drivers/scsi/sd.c         | 402 +++++++++++++++++++++++-----------------------
 drivers/scsi/sd.h         |   8 +-
 drivers/scsi/sd_zbc.c     |  60 +++----
 include/scsi/scsi_eh.h    |   4 +-
 5 files changed, 243 insertions(+), 267 deletions(-)

-- 
2.9.3

[PATCH 1/9] sd: Remove white space before EOL

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

No functional change is introduced by this patch.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/sd.c | 60 +++++++++++++++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 00b168b..db362da 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -7,20 +7,20 @@
  *              Subsequent revisions: Eric Youngdale
  *	Modification history:
  *       - Drew Eckhardt <drew@colorado.edu> original
- *       - Eric Youngdale <eric@andante.org> add scatter-gather, multiple 
+ *       - Eric Youngdale <eric@andante.org> add scatter-gather, multiple
  *         outstanding request, and other enhancements.
  *         Support loadable low-level scsi drivers.
- *       - Jirka Hanika <geo@ff.cuni.cz> support more scsi disks using 
+ *       - Jirka Hanika <geo@ff.cuni.cz> support more scsi disks using
  *         eight major numbers.
  *       - Richard Gooch <rgooch@atnf.csiro.au> support devfs.
- *	 - Torben Mathiasen <tmm@image.dk> Resource allocation fixes in 
+ *	 - Torben Mathiasen <tmm@image.dk> Resource allocation fixes in
  *	   sd_init and cleanups.
  *	 - Alex Davis <letmein@erols.com> Fix problem where partition info
- *	   not being read in sd_open. Fix problem where removable media 
+ *	   not being read in sd_open. Fix problem where removable media
  *	   could be ejected after sd_open.
  *	 - Douglas Gilbert <dgilbert@interlog.com> cleanup for lk 2.5.x
- *	 - Badari Pulavarty <pbadari@us.ibm.com>, Matthew Wilcox 
- *	   <willy@debian.org>, Kurt Garloff <garloff@suse.de>: 
+ *	 - Badari Pulavarty <pbadari@us.ibm.com>, Matthew Wilcox
+ *	   <willy@debian.org>, Kurt Garloff <garloff@suse.de>:
  *	   Support 32k/1M disks.
  *
  *	Logging policy (needs CONFIG_SCSI_LOGGING defined):
@@ -29,7 +29,7 @@
  *	 - entering sd_ioctl: SCSI_LOG_IOCTL level 1
  *	 - entering other commands: SCSI_LOG_HLQUEUE level 3
  *	Note: when the logging level is set by the user, it must be greater
- *	than the level indicated above to trigger output.	
+ *	than the level indicated above to trigger output.
  */
 
 #include <linux/module.h>
@@ -548,16 +548,16 @@ static struct kobject *sd_default_probe(dev_t devt, int *partno, void *data)
 
 /*
  * Device no to disk mapping:
- * 
+ *
  *       major         disc2     disc  p1
  *   |............|.............|....|....| <- dev_t
  *    31        20 19          8 7  4 3  0
- * 
+ *
  * Inside a major, we have 16k disks, however mapped non-
  * contiguously. The first 16 disks are for major0, the next
- * ones with major1, ... Disk 256 is for major0 again, disk 272 
- * for major1, ... 
- * As we stay compatible with our numbering scheme, we can reuse 
+ * ones with major1, ... Disk 256 is for major0 again, disk 272
+ * for major1, ...
+ * As we stay compatible with our numbering scheme, we can reuse
  * the well-know SCSI majors 8, 65--71, 136--143.
  */
 static int sd_major(int major_idx)
@@ -950,7 +950,7 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
 
 	if (sdp->changed) {
 		/*
-		 * quietly refuse to do anything to a changed disc until 
+		 * quietly refuse to do anything to a changed disc until
 		 * the changed bit has been reset
 		 */
 		/* printk("SCSI disk has been changed or is not present. Prohibiting further I/O.\n"); */
@@ -1193,7 +1193,7 @@ static void sd_uninit_command(struct scsi_cmnd *SCpnt)
  *	@inode: only i_rdev member may be used
  *	@filp: only f_mode and f_flags may be used
  *
- *	Returns 0 if successful. Returns a negated errno value in case 
+ *	Returns 0 if successful. Returns a negated errno value in case
  *	of error.
  *
  *	Note: This can be called from a user context (e.g. fsck(1) )
@@ -1261,7 +1261,7 @@ static int sd_open(struct block_device *bdev, fmode_t mode)
 
 error_out:
 	scsi_disk_put(sdkp);
-	return retval;	
+	return retval;
 }
 
 /**
@@ -1270,7 +1270,7 @@ static int sd_open(struct block_device *bdev, fmode_t mode)
  *	@inode: only i_rdev member may be used
  *	@filp: only f_mode and f_flags may be used
  *
- *	Returns 0. 
+ *	Returns 0.
  *
  *	Note: may block (uninterruptible) if error recovery is underway
  *	on this disk.
@@ -1344,7 +1344,7 @@ static int sd_ioctl(struct block_device *bdev, fmode_t mode,
 	struct scsi_device *sdp = sdkp->device;
 	void __user *p = (void __user *)arg;
 	int error;
-    
+
 	SCSI_LOG_IOCTL(1, sd_printk(KERN_INFO, sdkp, "sd_ioctl: disk=%s, "
 				    "cmd=0x%x\n", disk->disk_name, cmd));
 
@@ -1555,9 +1555,9 @@ static void sd_rescan(struct device *dev)
 
 
 #ifdef CONFIG_COMPAT
-/* 
- * This gets directly called from VFS. When the ioctl 
- * is not recognized we go back to the other translation paths. 
+/*
+ * This gets directly called from VFS. When the ioctl
+ * is not recognized we go back to the other translation paths.
  */
 static int sd_compat_ioctl(struct block_device *bdev, fmode_t mode,
 			   unsigned int cmd, unsigned long arg)
@@ -1569,12 +1569,12 @@ static int sd_compat_ioctl(struct block_device *bdev, fmode_t mode,
 			(mode & FMODE_NDELAY) != 0);
 	if (error)
 		return error;
-	       
-	/* 
+
+	/*
 	 * Let the static ioctl translation table take care of it.
 	 */
 	if (!sdev->host->hostt->compat_ioctl)
-		return -ENOIOCTLCMD; 
+		return -ENOIOCTLCMD;
 	return sdev->host->hostt->compat_ioctl(sdev, cmd, (void __user *)arg);
 }
 #endif
@@ -1953,7 +1953,7 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 			if (the_result)
 				sense_valid = scsi_sense_valid(&sshdr);
 			retries++;
-		} while (retries < 3 && 
+		} while (retries < 3 &&
 			 (!scsi_status_is_good(the_result) ||
 			  ((driver_byte(the_result) & DRIVER_SENSE) &&
 			  sense_valid && sshdr.sense_key == UNIT_ATTENTION)));
@@ -2026,7 +2026,7 @@ sd_spinup_disk(struct scsi_disk *sdkp)
 			}
 			break;
 		}
-				
+
 	} while (spintime && time_before_eq(jiffies, spintime_expire));
 
 	if (spintime) {
@@ -3106,13 +3106,13 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
  *	for each scsi device (not just disks) present.
  *	@dev: pointer to device object
  *
- *	Returns 0 if successful (or not interested in this scsi device 
+ *	Returns 0 if successful (or not interested in this scsi device
  *	(e.g. scanner)); 1 when there is an error.
  *
  *	Note: this function is invoked from the scsi mid-level.
- *	This function sets up the mapping between a given 
- *	<host,channel,id,lun> (found in sdp) and new device name 
- *	(e.g. /dev/sda). More precisely it is the block device major 
+ *	This function sets up the mapping between a given
+ *	<host,channel,id,lun> (found in sdp) and new device name
+ *	(e.g. /dev/sda). More precisely it is the block device major
  *	and minor number that is chosen here.
  *
  *	Assume sd_probe is not re-entrant (for time being)
@@ -3267,7 +3267,7 @@ static void scsi_disk_release(struct device *dev)
 {
 	struct scsi_disk *sdkp = to_scsi_disk(dev);
 	struct gendisk *disk = sdkp->disk;
-	
+
 	spin_lock(&sd_index_lock);
 	ida_remove(&sd_index_ida, sdkp->index);
 	spin_unlock(&sd_index_lock);
-- 
2.9.3

Re: [PATCH 1/9] sd: Remove white space before EOL

[Bart Van Assche]

On Fri, 2017-04-21 at 18:16 +0900, damien.lemoal@wdc.com wrote:
> No functional change is introduced by this patch.

Hello Damien,

The only tree for which I know that this kind of whitespace / coding style
patches are welcome is the staging tree. In general such patches are not
considered welcome because:
- Such patches pollute the changelog and make it harder to use tools like
  git annotate / git blame.
- Such patches make it harder for distro maintainers to backport patches
  from upstream kernels to distro (enterprise) kernels.
- If any kernel developer has developed kernel patches for the sd driver
  against kernel v4.11 and wants to submit these after the v4.12 merge
  window then that developer will have to rebase (and retest) these
  patches on top of kernel v4.12-rc1. Whitespace patches trigger really
  ugly rebase conflicts and hence are painful for other kernel developers.

Hence my request to drop this patch.

Thanks,

Bart.

Re: [PATCH 1/9] sd: Remove white space before EOL

[Damien Le Moal]

Bart,

On 4/22/17 02:22, Bart Van Assche wrote:
> On Fri, 2017-04-21 at 18:16 +0900, damien.lemoal@wdc.com wrote:
>> No functional change is introduced by this patch.
> 
> Hello Damien,
> 
> The only tree for which I know that this kind of whitespace / coding style
> patches are welcome is the staging tree. In general such patches are not
> considered welcome because:
> - Such patches pollute the changelog and make it harder to use tools like
>   git annotate / git blame.
> - Such patches make it harder for distro maintainers to backport patches
>   from upstream kernels to distro (enterprise) kernels.
> - If any kernel developer has developed kernel patches for the sd driver
>   against kernel v4.11 and wants to submit these after the v4.12 merge
>   window then that developer will have to rebase (and retest) these
>   patches on top of kernel v4.12-rc1. Whitespace patches trigger really
>   ugly rebase conflicts and hence are painful for other kernel developers.
> 
> Hence my request to drop this patch.

OK. As I said, this patch was "optional".
Just wondering though how we are supposed to do code maintenance if that
kind of trivial cleanup cannot go in... (not to mention that checkpatch
keep warning about white spaces in sd.c if a patch touches something
near one of those multiple lines with white spaces).

But fine, let's drop it.

-- 
Damien Le Moal,
Western Digital

[PATCH 2/9] sd: Fix functions description

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

Use regular format comments documenting functions, fix argument names,
etc

No functional change is introduced by this patch.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/sd.c | 272 +++++++++++++++++++++++++++---------------------------
 1 file changed, 136 insertions(+), 136 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index db362da..935ce34 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -705,11 +705,10 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
 
 /**
  * sd_setup_discard_cmnd - unmap blocks on thinly provisioned device
- * @sdp: scsi device to operate on
- * @rq: Request to prepare
+ * @cmd: command to prepare
  *
- * Will issue either UNMAP or WRITE SAME(16) depending on preference
- * indicated by target device.
+ * Will set either UNMAP, WRITE SAME(10) or WRITE SAME(16) depending
+ * on preference indicated by target device.
  **/
 static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd)
 {
@@ -827,7 +826,7 @@ static void sd_config_write_same(struct scsi_disk *sdkp)
  * sd_setup_write_same_cmnd - write the same data to multiple blocks
  * @cmd: command to prepare
  *
- * Will issue either WRITE SAME(10) or WRITE SAME(16) depending on
+ * Will setup either WRITE SAME(10) or WRITE SAME(16) depending on
  * preference indicated by target device.
  **/
 static int sd_setup_write_same_cmnd(struct scsi_cmnd *cmd)
@@ -1189,20 +1188,18 @@ static void sd_uninit_command(struct scsi_cmnd *SCpnt)
 }
 
 /**
- *	sd_open - open a scsi disk device
- *	@inode: only i_rdev member may be used
- *	@filp: only f_mode and f_flags may be used
+ * sd_open - open a scsi disk device
+ * @bdev: Block device of the scsi disk to open
+ * @mode: FMODE_* mask
  *
- *	Returns 0 if successful. Returns a negated errno value in case
- *	of error.
+ * Returns 0 if successful. Returns a negated errno value in case
+ * of error.
  *
- *	Note: This can be called from a user context (e.g. fsck(1) )
- *	or from within the kernel (e.g. as a result of a mount(1) ).
- *	In the latter case @inode and @filp carry an abridged amount
- *	of information as noted above.
+ * Note: This can be called from a user context (e.g. fsck(1) )
+ * or from within the kernel (e.g. as a result of a mount(1) ).
  *
- *	Locking: called with bdev->bd_mutex held.
- **/
+ * Locking: called with bdev->bd_mutex held.
+ */
 static int sd_open(struct block_device *bdev, fmode_t mode)
 {
 	struct scsi_disk *sdkp = scsi_disk_get(bdev->bd_disk);
@@ -1265,18 +1262,16 @@ static int sd_open(struct block_device *bdev, fmode_t mode)
 }
 
 /**
- *	sd_release - invoked when the (last) close(2) is called on this
- *	scsi disk.
- *	@inode: only i_rdev member may be used
- *	@filp: only f_mode and f_flags may be used
- *
- *	Returns 0.
+ * sd_release - invoked when the (last) close(2) is called on this
+ * scsi disk.
+ * @disk: disk to release
+ * @mode: FMODE_* mask
  *
- *	Note: may block (uninterruptible) if error recovery is underway
- *	on this disk.
+ * Note: may block (uninterruptible) if error recovery is underway
+ * on this disk.
  *
- *	Locking: called with bdev->bd_mutex held.
- **/
+ * Locking: called with bdev->bd_mutex held.
+ */
 static void sd_release(struct gendisk *disk, fmode_t mode)
 {
 	struct scsi_disk *sdkp = scsi_disk(disk);
@@ -1323,19 +1318,19 @@ static int sd_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 }
 
 /**
- *	sd_ioctl - process an ioctl
- *	@inode: only i_rdev/i_bdev members may be used
- *	@filp: only f_mode and f_flags may be used
- *	@cmd: ioctl command number
- *	@arg: this is third argument given to ioctl(2) system call.
- *	Often contains a pointer.
+ * sd_ioctl - process an ioctl
+ * @bdev: target block device
+ * @mode: FMODE_* mask
+ * @cmd: ioctl command number
+ * @arg: this is third argument given to ioctl(2) system call.
+ *       Often contains a pointer.
  *
- *	Returns 0 if successful (some ioctls return positive numbers on
- *	success as well). Returns a negated errno value in case of error.
+ * Returns 0 if successful (some ioctls return positive numbers on
+ * success as well). Returns a negated errno value in case of error.
  *
- *	Note: most ioctls are forward onto the block subsystem or further
- *	down in the scsi subsystem.
- **/
+ * Note: most ioctls are forward onto the block subsystem or further
+ * down in the scsi subsystem.
+ */
 static int sd_ioctl(struct block_device *bdev, fmode_t mode,
 		    unsigned int cmd, unsigned long arg)
 {
@@ -1415,14 +1410,14 @@ static int media_not_present(struct scsi_disk *sdkp,
 }
 
 /**
- *	sd_check_events - check media events
- *	@disk: kernel device descriptor
- *	@clearing: disk events currently being cleared
+ * sd_check_events - check media events
+ * @disk: kernel device descriptor
+ * @clearing: disk events currently being cleared
  *
- *	Returns mask of DISK_EVENT_*.
+ * Returns mask of DISK_EVENT_*.
  *
- *	Note: this function is invoked from the block subsystem.
- **/
+ * Note: this function is invoked from the block subsystem.
+ */
 static unsigned int sd_check_events(struct gendisk *disk, unsigned int clearing)
 {
 	struct scsi_disk *sdkp = scsi_disk_get(disk);
@@ -1688,17 +1683,17 @@ static const struct block_device_operations sd_fops = {
 };
 
 /**
- *	sd_eh_reset - reset error handling callback
- *	@scmd:		sd-issued command that has failed
+ * sd_eh_reset - reset error handling callback
+ * @scmd: sd-issued command that has failed
  *
- *	This function is called by the SCSI midlayer before starting
- *	SCSI EH. When counting medium access failures we have to be
- *	careful to register it only only once per device and SCSI EH run;
- *	there might be several timed out commands which will cause the
- *	'max_medium_access_timeouts' counter to trigger after the first
- *	SCSI EH run already and set the device to offline.
- *	So this function resets the internal counter before starting SCSI EH.
- **/
+ * This function is called by the SCSI midlayer before starting
+ * SCSI EH. When counting medium access failures we have to be
+ * careful to register it only only once per device and SCSI EH run;
+ * there might be several timed out commands which will cause the
+ * 'max_medium_access_timeouts' counter to trigger after the first
+ * SCSI EH run already and set the device to offline.
+ * So this function resets the internal counter before starting SCSI EH.
+ */
 static void sd_eh_reset(struct scsi_cmnd *scmd)
 {
 	struct scsi_disk *sdkp = scsi_disk(scmd->request->rq_disk);
@@ -1708,17 +1703,17 @@ static void sd_eh_reset(struct scsi_cmnd *scmd)
 }
 
 /**
- *	sd_eh_action - error handling callback
- *	@scmd:		sd-issued command that has failed
- *	@eh_disp:	The recovery disposition suggested by the midlayer
+ * sd_eh_action - error handling callback
+ * @scmd: sd-issued command that has failed
+ * @eh_disp: The recovery disposition suggested by the midlayer
  *
- *	This function is called by the SCSI midlayer upon completion of an
- *	error test command (currently TEST UNIT READY). The result of sending
- *	the eh command is passed in eh_disp.  We're looking for devices that
- *	fail medium access commands but are OK with non access commands like
- *	test unit ready (so wrongly see the device as having a successful
- *	recovery)
- **/
+ * This function is called by the SCSI midlayer upon completion of an
+ * error test command (currently TEST UNIT READY). The result of sending
+ * the eh command is passed in eh_disp.  We're looking for devices that
+ * fail medium access commands but are OK with non access commands like
+ * test unit ready (so wrongly see the device as having a successful
+ * recovery)
+ */
 static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
 {
 	struct scsi_disk *sdkp = scsi_disk(scmd->request->rq_disk);
@@ -1798,12 +1793,14 @@ static unsigned int sd_completed_bytes(struct scsi_cmnd *scmd)
 }
 
 /**
- *	sd_done - bottom half handler: called when the lower level
- *	driver has completed (successfully or otherwise) a scsi command.
- *	@SCpnt: mid-level's per command structure.
+ * sd_done - bottom half command completion handler
+ * @SCpnt: mid-level's per command structure.
  *
- *	Note: potentially run from within an ISR. Must not block.
- **/
+ * Called when the lower level driver has completed (successfully
+ * or otherwise) a scsi command.
+ *
+ * Note: potentially run from within an ISR. Must not block.
+ */
 static int sd_done(struct scsi_cmnd *SCpnt)
 {
 	int result = SCpnt->result;
@@ -2713,7 +2710,7 @@ static void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)
 
 /**
  * sd_read_block_limits - Query disk device for preferred I/O sizes.
- * @disk: disk to query
+ * @sdkp: disk to query
  */
 static void sd_read_block_limits(struct scsi_disk *sdkp)
 {
@@ -2779,7 +2776,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 
 /**
  * sd_read_block_characteristics - Query block dev. characteristics
- * @disk: disk to query
+ * @sdkp: disk to query
  */
 static void sd_read_block_characteristics(struct scsi_disk *sdkp)
 {
@@ -2827,7 +2824,7 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
 
 /**
  * sd_read_block_provisioning - Query provisioning VPD page
- * @disk: disk to query
+ * @sdkp: disk to query
  */
 static void sd_read_block_provisioning(struct scsi_disk *sdkp)
 {
@@ -2883,10 +2880,11 @@ static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer)
 }
 
 /**
- *	sd_revalidate_disk - called the first time a new disk is seen,
- *	performs disk spin up, read_capacity, etc.
- *	@disk: struct gendisk we care about
- **/
+ * sd_revalidate_disk - called the first time a new disk is seen
+ * @disk: struct gendisk we care about
+ *
+ * Performs disk spin up, read_capacity, etc.
+ */
 static int sd_revalidate_disk(struct gendisk *disk)
 {
 	struct scsi_disk *sdkp = scsi_disk(disk);
@@ -2978,16 +2976,16 @@ static int sd_revalidate_disk(struct gendisk *disk)
 }
 
 /**
- *	sd_unlock_native_capacity - unlock native capacity
- *	@disk: struct gendisk to set capacity for
+ * sd_unlock_native_capacity - unlock native capacity
+ * @disk: struct gendisk to set capacity for
  *
- *	Block layer calls this function if it detects that partitions
- *	on @disk reach beyond the end of the device.  If the SCSI host
- *	implements ->unlock_native_capacity() method, it's invoked to
- *	give it a chance to adjust the device capacity.
+ * Block layer calls this function if it detects that partitions
+ * on @disk reach beyond the end of the device.  If the SCSI host
+ * implements ->unlock_native_capacity() method, it's invoked to
+ * give it a chance to adjust the device capacity.
  *
- *	CONTEXT:
- *	Defined by block layer.  Might sleep.
+ * CONTEXT:
+ * Defined by block layer.  Might sleep.
  */
 static void sd_unlock_native_capacity(struct gendisk *disk)
 {
@@ -2998,26 +2996,26 @@ static void sd_unlock_native_capacity(struct gendisk *disk)
 }
 
 /**
- *	sd_format_disk_name - format disk name
- *	@prefix: name prefix - ie. "sd" for SCSI disks
- *	@index: index of the disk to format name for
- *	@buf: output buffer
- *	@buflen: length of the output buffer
+ * sd_format_disk_name - format disk name
+ * @prefix: name prefix - ie. "sd" for SCSI disks
+ * @index: index of the disk to format name for
+ * @buf: output buffer
+ * @buflen: length of the output buffer
  *
- *	SCSI disk names starts at sda.  The 26th device is sdz and the
- *	27th is sdaa.  The last one for two lettered suffix is sdzz
- *	which is followed by sdaaa.
+ * SCSI disk names starts at sda.  The 26th device is sdz and the
+ * 27th is sdaa.  The last one for two lettered suffix is sdzz
+ * which is followed by sdaaa.
  *
- *	This is basically 26 base counting with one extra 'nil' entry
- *	at the beginning from the second digit on and can be
- *	determined using similar method as 26 base conversion with the
- *	index shifted -1 after each digit is computed.
+ * This is basically 26 base counting with one extra 'nil' entry
+ * at the beginning from the second digit on and can be
+ * determined using similar method as 26 base conversion with the
+ * index shifted -1 after each digit is computed.
  *
- *	CONTEXT:
- *	Don't care.
+ * CONTEXT:
+ * Don't care.
  *
- *	RETURNS:
- *	0 on success, -errno on failure.
+ * RETURNS:
+ * 0 on success, -errno on failure.
  */
 static int sd_format_disk_name(char *prefix, int index, char *buf, int buflen)
 {
@@ -3101,23 +3099,23 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
 }
 
 /**
- *	sd_probe - called during driver initialization and whenever a
- *	new scsi device is attached to the system. It is called once
- *	for each scsi device (not just disks) present.
- *	@dev: pointer to device object
+ * sd_probe - called during driver initialization and whenever a
+ * new scsi device is attached to the system. It is called once
+ * for each scsi device (not just disks) present.
+ * @dev: pointer to device object
  *
- *	Returns 0 if successful (or not interested in this scsi device
- *	(e.g. scanner)); 1 when there is an error.
+ * Returns 0 if successful (or not interested in this scsi device
+ * (e.g. scanner)); 1 when there is an error.
  *
- *	Note: this function is invoked from the scsi mid-level.
- *	This function sets up the mapping between a given
- *	<host,channel,id,lun> (found in sdp) and new device name
- *	(e.g. /dev/sda). More precisely it is the block device major
- *	and minor number that is chosen here.
+ * Note: this function is invoked from the scsi mid-level.
+ * This function sets up the mapping between a given
+ * <host,channel,id,lun> (found in sdp) and new device name
+ * (e.g. /dev/sda). More precisely it is the block device major
+ * and minor number that is chosen here.
  *
- *	Assume sd_probe is not re-entrant (for time being)
- *	Also think about sd_probe() and sd_remove() running coincidentally.
- **/
+ * Assume sd_probe is not re-entrant (for time being)
+ * Also think about sd_probe() and sd_remove() running coincidentally.
+ */
 static int sd_probe(struct device *dev)
 {
 	struct scsi_device *sdp = to_scsi_device(dev);
@@ -3216,16 +3214,18 @@ static int sd_probe(struct device *dev)
 }
 
 /**
- *	sd_remove - called whenever a scsi disk (previously recognized by
- *	sd_probe) is detached from the system. It is called (potentially
- *	multiple times) during sd module unload.
- *	@dev: pointer to device object
+ * sd_remove - called whenever a scsi disk is detached from the system
+ * @dev: pointer to device object
  *
- *	Note: this function is invoked from the scsi mid-level.
- *	This function potentially frees up a device name (e.g. /dev/sdc)
- *	that could be re-used by a subsequent sd_probe().
- *	This function is not called when the built-in sd driver is "exit-ed".
- **/
+ * Called whenever a scsi disk (previously recognized by sd_probe) is
+ * detached from the system. It is called (potentially multiple times)
+ * during sd module unload.
+ *
+ * Note: this function is invoked from the scsi mid-level.
+ * This function potentially frees up a device name (e.g. /dev/sdc)
+ * that could be re-used by a subsequent sd_probe().
+ * This function is not called when the built-in sd driver is "exit-ed".
+ */
 static int sd_remove(struct device *dev)
 {
 	struct scsi_disk *sdkp;
@@ -3255,14 +3255,14 @@ static int sd_remove(struct device *dev)
 }
 
 /**
- *	scsi_disk_release - Called to free the scsi_disk structure
- *	@dev: pointer to embedded class device
+ * scsi_disk_release - Called to free the scsi_disk structure
+ * @dev: pointer to embedded class device
  *
- *	sd_ref_mutex must be held entering this routine.  Because it is
- *	called on last put, you should always use the scsi_disk_get()
- *	scsi_disk_put() helpers which manipulate the semaphore directly
- *	and never do a direct put_device.
- **/
+ * sd_ref_mutex must be held entering this routine.  Because it is
+ * called on last put, you should always use the scsi_disk_get()
+ * scsi_disk_put() helpers which manipulate the semaphore directly
+ * and never do a direct put_device.
+ */
 static void scsi_disk_release(struct device *dev)
 {
 	struct scsi_disk *sdkp = to_scsi_disk(dev);
@@ -3396,11 +3396,11 @@ static int sd_resume(struct device *dev)
 }
 
 /**
- *	init_sd - entry point for this driver (both when built in or when
- *	a module).
+ * init_sd - entry point for this driver (both when built in or when
+ * a module).
  *
- *	Note: this function registers this driver with the scsi mid-level.
- **/
+ * Note: this function registers this driver with the scsi mid-level.
+ */
 static int __init init_sd(void)
 {
 	int majors = 0, i, err;
@@ -3458,10 +3458,10 @@ static int __init init_sd(void)
 }
 
 /**
- *	exit_sd - exit point for this driver (when it is a module).
+ * exit_sd - exit point for this driver (when it is a module).
  *
- *	Note: this function unregisters this driver from the scsi mid-level.
- **/
+ * Note: this function unregisters this driver from the scsi mid-level.
+ */
 static void __exit exit_sd(void)
 {
 	int i;
-- 
2.9.3

Re: [PATCH 2/9] sd: Fix functions description

[Bart Van Assche]

On Fri, 2017-04-21 at 18:16 +0900, damien.lemoal@wdc.com wrote:
> - *	Returns 0 if successful. Returns a negated errno value in case
> - *	of error.
> + * Returns 0 if successful. Returns a negated errno value in case
> + * of error.

Hello Damien,

The argument name fixes are definitely welcome in my opinion. But I see
that this patch not only fixes argument names and function descriptions
but also includes whitespace-only changes like the above. Sorry but I
don't like whitespace-only changes ...

Bart.

Re: [PATCH 2/9] sd: Fix functions description

[Damien Le Moal]

Bart,

On 4/22/17 02:28, Bart Van Assche wrote:
> On Fri, 2017-04-21 at 18:16 +0900, damien.lemoal@wdc.com wrote:
>> - *   Returns 0 if successful. Returns a negated errno value in case
>> - *   of error.
>> + * Returns 0 if successful. Returns a negated errno value in case
>> + * of error.
> 
> Hello Damien,
> 
> The argument name fixes are definitely welcome in my opinion. But I see
> that this patch not only fixes argument names and function descriptions
> but also includes whitespace-only changes like the above. Sorry but I
> don't like whitespace-only changes ...

Yes, I understand, you were clear about that.
The "white space" changes intent are to match the preferred format for
functions documentation (From Documentation/doc-guide/kerne-doc.rst)

  /**
   * foobar() - Brief description of foobar.
   * @arg: Description of argument of foobar.
   *
   * Longer description of foobar.
   *
   * Return: Description of return value of foobar.
   */

If that is wrong/unnecessary, then I will rewrite that patch to only
limit its scope to the argument name fixes.

Best regards.

-- 
Damien Le Moal,
Western Digital

[PATCH 3/9] sd: Remove unecessary variable in sd_done()

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

The 'sense_deferred' variable in sd_done() is set only if
'sense_valid' is true. So it is not necessary and the result of
scsi_sense_is_deferred() and of scsi_command_normalize_sense() can
be combined together in 'sense_valid'.

Also, since scsi_command_normalize_sense() returns a bool, use that
type for 'sense_valid' declaration.

No functional change is introduced by this patch.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/sd.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 935ce34..c57084e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1808,8 +1808,7 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 	struct scsi_sense_hdr sshdr;
 	struct scsi_disk *sdkp = scsi_disk(SCpnt->request->rq_disk);
 	struct request *req = SCpnt->request;
-	int sense_valid = 0;
-	int sense_deferred = 0;
+	bool sense_valid = false;
 	unsigned char op = SCpnt->cmnd[0];
 	unsigned char unmap = SCpnt->cmnd[1] & 8;
 
@@ -1837,15 +1836,12 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 		break;
 	}
 
-	if (result) {
-		sense_valid = scsi_command_normalize_sense(SCpnt, &sshdr);
-		if (sense_valid)
-			sense_deferred = scsi_sense_is_deferred(&sshdr);
-	}
+	if (result)
+		sense_valid = scsi_command_normalize_sense(SCpnt, &sshdr) &&
+			!scsi_sense_is_deferred(&sshdr);
 	sdkp->medium_access_timed_out = 0;
 
-	if (driver_byte(result) != DRIVER_SENSE &&
-	    (!sense_valid || sense_deferred))
+	if (driver_byte(result) != DRIVER_SENSE && !sense_valid)
 		goto out;
 
 	switch (sshdr.sense_key) {
-- 
2.9.3

Re: [PATCH 3/9] sd: Remove unecessary variable in sd_done()

[James Bottomley]

On Fri, 2017-04-21 at 18:16 +0900, damien.lemoal@wdc.com wrote:
> From: Damien Le Moal <damien.lemoal@wdc.com>
> 
> The 'sense_deferred' variable in sd_done() is set only if
> 'sense_valid' is true. So it is not necessary and the result of
> scsi_sense_is_deferred() and of scsi_command_normalize_sense() can
> be combined together in 'sense_valid'.
> 
> Also, since scsi_command_normalize_sense() returns a bool, use that
> type for 'sense_valid' declaration.
> 
> No functional change is introduced by this patch.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/scsi/sd.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 935ce34..c57084e 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1808,8 +1808,7 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>  	struct scsi_sense_hdr sshdr;
>  	struct scsi_disk *sdkp = scsi_disk(SCpnt->request->rq_disk);
>  	struct request *req = SCpnt->request;
> -	int sense_valid = 0;
> -	int sense_deferred = 0;
> +	bool sense_valid = false;
>  	unsigned char op = SCpnt->cmnd[0];
>  	unsigned char unmap = SCpnt->cmnd[1] & 8;
>  
> @@ -1837,15 +1836,12 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>  		break;
>  	}
>  
> -	if (result) {
> -		sense_valid = scsi_command_normalize_sense(SCpnt,
> &sshdr);
> -		if (sense_valid)
> -			sense_deferred =
> scsi_sense_is_deferred(&sshdr);
> -	}
> +	if (result)
> +		sense_valid = scsi_command_normalize_sense(SCpnt,
> &sshdr) &&
> +			!scsi_sense_is_deferred(&sshdr);
>  	sdkp->medium_access_timed_out = 0;
>  
> -	if (driver_byte(result) != DRIVER_SENSE &&
> -	    (!sense_valid || sense_deferred))
> +	if (driver_byte(result) != DRIVER_SENSE && !sense_valid)
>  		goto out;
>  
>  	switch (sshdr.sense_key) {

Really, no, you're making the code less clear for no gain.  I'm fairly
certain the compiler can optimise this without any help and when you're
skimming the code you can easily see that the out jump is taken if you
have a sense code that's either invalid or deferred.  After your change
you have to glance one level deeper to come to the same conclusion.

To be clear: I wouldn't object if the original function were written
the way you propose because we all get used to scanning code looking
for things like this.  However, a patch to change existing code fails
the net benefit test.

James

Re: [PATCH 3/9] sd: Remove unecessary variable in sd_done()

[Damien Le Moal]

James,

On 4/22/17 03:42, James Bottomley wrote:
> Really, no, you're making the code less clear for no gain.  I'm fairly
> certain the compiler can optimise this without any help and when you're
> skimming the code you can easily see that the out jump is taken if you
> have a sense code that's either invalid or deferred.  After your change
> you have to glance one level deeper to come to the same conclusion.
> 
> To be clear: I wouldn't object if the original function were written
> the way you propose because we all get used to scanning code looking
> for things like this.  However, a patch to change existing code fails
> the net benefit test.

OK. Let's drop this patch then.
Thank you for the review.

-- 
Damien Le Moal,
Western Digital

[PATCH 4/9] sd: Improve sd_completed_bytes

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

Re-shuffle the code to be more efficient by not initializing
variables upfront (i.e. do it only when necessary).
Also replace the do_div calls with calls to sectors_to_logical().

No functional change is introduced by this patch.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/sd.c | 48 ++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index c57084e..5ff0082 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1754,41 +1754,45 @@ static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
 
 static unsigned int sd_completed_bytes(struct scsi_cmnd *scmd)
 {
-	u64 start_lba = blk_rq_pos(scmd->request);
-	u64 end_lba = blk_rq_pos(scmd->request) + (scsi_bufflen(scmd) / 512);
-	u64 factor = scmd->device->sector_size / 512;
-	u64 bad_lba;
-	int info_valid;
+	struct request *req = scmd->request;
+	struct scsi_device *sdev = scmd->device;
+	unsigned int transferred, good_bytes;
+	u64 start_lba, end_lba, bad_lba;
+
 	/*
-	 * resid is optional but mostly filled in.  When it's unused,
-	 * its value is zero, so we assume the whole buffer transferred
+	 * Some commands have a payload smaller than the device logical
+	 * block size (e.g. INQUIRY on a 4K disk).
 	 */
-	unsigned int transferred = scsi_bufflen(scmd) - scsi_get_resid(scmd);
-	unsigned int good_bytes;
-
-	info_valid = scsi_get_sense_info_fld(scmd->sense_buffer,
-					     SCSI_SENSE_BUFFERSIZE,
-					     &bad_lba);
-	if (!info_valid)
+	if (scsi_bufflen(scmd) <= sdev->sector_size)
 		return 0;
 
-	if (scsi_bufflen(scmd) <= scmd->device->sector_size)
+	/* Check if we have a 'bad_lba' information */
+	if (!scsi_get_sense_info_fld(scmd->sense_buffer,
+				     SCSI_SENSE_BUFFERSIZE,
+				     &bad_lba))
 		return 0;
 
-	/* be careful ... don't want any overflows */
-	do_div(start_lba, factor);
-	do_div(end_lba, factor);
-
-	/* The bad lba was reported incorrectly, we have no idea where
+	/*
+	 * If the bad lba was reported incorrectly, we have no idea where
 	 * the error is.
 	 */
-	if (bad_lba < start_lba  || bad_lba >= end_lba)
+	start_lba = sectors_to_logical(sdev, blk_rq_pos(req));
+	end_lba = sectors_to_logical(sdev,
+				blk_rq_pos(req) + (scsi_bufflen(scmd) >> 9));
+	if (bad_lba < start_lba || bad_lba >= end_lba)
 		return 0;
 
+	/*
+	 * resid is optional but mostly filled in.  When it's unused,
+	 * its value is zero, so we assume the whole buffer transferred
+	 */
+	transferred = scsi_bufflen(scmd) - scsi_get_resid(scmd);
+
 	/* This computation should always be done in terms of
 	 * the resolution of the device's medium.
 	 */
-	good_bytes = (bad_lba - start_lba) * scmd->device->sector_size;
+	good_bytes = logical_to_bytes(sdev, bad_lba - start_lba);
+
 	return min(good_bytes, transferred);
 }
 
-- 
2.9.3

[PATCH 5/9] sd: Cleanup sd_done sense data handling

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

In sd_done(), for the ILLEGAL REQUEST sense key case, add an 'else'
after the first 'if (sshdr.asc == 0x10)' test to avoid the second test
(the values tested are different).

Still for the same ILLEGAL REQUEST case, move the declarations of the
variables 'op' and 'unmap' within the scope of the second if since
these variables are only used there.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/sd.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5ff0082..a59a107 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1813,8 +1813,6 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 	struct scsi_disk *sdkp = scsi_disk(SCpnt->request->rq_disk);
 	struct request *req = SCpnt->request;
 	bool sense_valid = false;
-	unsigned char op = SCpnt->cmnd[0];
-	unsigned char unmap = SCpnt->cmnd[1] & 8;
 
 	switch (req_op(req)) {
 	case REQ_OP_DISCARD:
@@ -1869,10 +1867,14 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 			good_bytes = sd_completed_bytes(SCpnt);
 		break;
 	case ILLEGAL_REQUEST:
-		if (sshdr.asc == 0x10)  /* DIX: Host detected corruption */
+		if (sshdr.asc == 0x10) {
+			/* DIX: Host detected corruption */
 			good_bytes = sd_completed_bytes(SCpnt);
-		/* INVALID COMMAND OPCODE or INVALID FIELD IN CDB */
-		if (sshdr.asc == 0x20 || sshdr.asc == 0x24) {
+		} else if (sshdr.asc == 0x20 || sshdr.asc == 0x24) {
+			/* INVALID COMMAND OPCODE or INVALID FIELD IN CDB */
+			unsigned char op = SCpnt->cmnd[0];
+			unsigned char unmap = SCpnt->cmnd[1] & 8;
+
 			switch (op) {
 			case UNMAP:
 				sd_config_discard(sdkp, SD_LBP_DISABLE);
@@ -1884,8 +1886,6 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 				else {
 					sdkp->device->no_write_same = 1;
 					sd_config_write_same(sdkp);
-
-					good_bytes = 0;
 					req->__data_len = blk_rq_bytes(req);
 					req->rq_flags |= RQF_QUIET;
 				}
-- 
2.9.3

Re: [PATCH 5/9] sd: Cleanup sd_done sense data handling

[Bart Van Assche]

On Fri, 2017-04-21 at 18:16 +0900, damien.lemoal@wdc.com wrote:
> @@ -1884,8 +1886,6 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>  				else {
>  					sdkp->device->no_write_same = 1;
>  					sd_config_write_same(sdkp);
> -
> -					good_bytes = 0;
>  					req->__data_len = blk_rq_bytes(req);
>  					req->rq_flags |= RQF_QUIET;
>  				}

This change looks fine to me but has not been described in the patch
description? Anyway:

Reviewed-by: Bart Van Assche <Bart.VanAssche@sandisk.com>

Re: [PATCH 5/9] sd: Cleanup sd_done sense data handling

[Damien Le Moal]

Bart,

On 4/22/17 03:41, Bart Van Assche wrote:
> On Fri, 2017-04-21 at 18:16 +0900, damien.lemoal@wdc.com wrote:
>> @@ -1884,8 +1886,6 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>>                                else {
>>                                        sdkp->device->no_write_same = 1;
>>                                        sd_config_write_same(sdkp);
>> -
>> -                                     good_bytes = 0;
>>                                        req->__data_len = blk_rq_bytes(req);
>>                                        req->rq_flags |= RQF_QUIET;
>>                                }
> 
> This change looks fine to me but has not been described in the patch
> description? Anyway:
> 
> Reviewed-by: Bart Van Assche <Bart.VanAssche@sandisk.com>

Oops. Yes, I forgot to mention that in the commit message.
(for result != 0, good_bytes is already set to 0, so that assignment is
not necessary).
Will resend.

-- 
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com

[PATCH 6/9] scsi: Improve scsi_get_sense_info_fld

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

Use get_unaligned_be32 and get_unaligned_be64 to obtain values from
the sense buffer instead of open coding the operations.
Also change the function return value to a bool.

No functional change is introduced by this patch.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/scsi_error.c | 36 ++++++++++++++----------------------
 include/scsi/scsi_eh.h    |  4 ++--
 2 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 53e3343..9a966ba 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -46,6 +46,8 @@
 
 #include <trace/events/scsi.h>
 
+#include <asm/unaligned.h>
+
 static void scsi_eh_done(struct scsi_cmnd *scmd);
 
 /*
@@ -2363,42 +2365,32 @@ EXPORT_SYMBOL(scsi_command_normalize_sense);
  * Return value:
  *	1 if information field found, 0 if not found.
  */
-int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
-			    u64 * info_out)
+bool scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
+			     u64 * info_out)
 {
-	int j;
 	const u8 * ucp;
-	u64 ull;
 
 	if (sb_len < 7)
-		return 0;
+		return false;
 	switch (sense_buffer[0] & 0x7f) {
 	case 0x70:
 	case 0x71:
 		if (sense_buffer[0] & 0x80) {
-			*info_out = (sense_buffer[3] << 24) +
-				    (sense_buffer[4] << 16) +
-				    (sense_buffer[5] << 8) + sense_buffer[6];
-			return 1;
-		} else
-			return 0;
+			*info_out = get_unaligned_be32(&sense_buffer[3]);
+			return true;
+		}
+		return false;
 	case 0x72:
 	case 0x73:
 		ucp = scsi_sense_desc_find(sense_buffer, sb_len,
 					   0 /* info desc */);
 		if (ucp && (0xa == ucp[1])) {
-			ull = 0;
-			for (j = 0; j < 8; ++j) {
-				if (j > 0)
-					ull <<= 8;
-				ull |= ucp[4 + j];
-			}
-			*info_out = ull;
-			return 1;
-		} else
-			return 0;
+			*info_out = get_unaligned_be64(&ucp[4]);
+			return true;
+		}
+		return false;
 	default:
-		return 0;
+		return false;
 	}
 }
 EXPORT_SYMBOL(scsi_get_sense_info_fld);
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index a25b328..cf9e61d 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -23,8 +23,8 @@ static inline bool scsi_sense_is_deferred(const struct scsi_sense_hdr *sshdr)
 	return ((sshdr->response_code >= 0x70) && (sshdr->response_code & 1));
 }
 
-extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
-				   u64 * info_out);
+extern bool scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
+				    u64 * info_out);
 
 extern int scsi_ioctl_reset(struct scsi_device *, int __user *);
 
-- 
2.9.3

Re: [PATCH 6/9] scsi: Improve scsi_get_sense_info_fld

[Bart Van Assche]

On Fri, 2017-04-21 at 18:16 +0900, damien.lemoal@wdc.com wrote:
> @@ -2363,42 +2365,32 @@ EXPORT_SYMBOL(scsi_command_normalize_sense);
>   * Return value:
>   *	1 if information field found, 0 if not found.
>   */
> -int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
> -			    u64 * info_out)
> +bool scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
> +			     u64 * info_out)

Hello Damien,

If you have to repost this patch please address the checkpatch complaints
of the following category triggered by the above declaration:

ERROR: "foo * bar" should be "foo *bar"

Otherwise this patch looks fine to me. Hence:

Reviewed-by: Bart Van Assche <Bart.VanAssche@sandisk.com>

Re: [PATCH 6/9] scsi: Improve scsi_get_sense_info_fld

[Damien Le Moal]

Bart,

On 4/22/17 01:58, Bart Van Assche wrote:
> On Fri, 2017-04-21 at 18:16 +0900, damien.lemoal@wdc.com wrote:
>> @@ -2363,42 +2365,32 @@ EXPORT_SYMBOL(scsi_command_normalize_sense);
>>   * Return value:
>>   *   1 if information field found, 0 if not found.
>>   */
>> -int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
>> -                         u64 * info_out)
>> +bool scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
>> +                          u64 * info_out)
> 
> Hello Damien,
> 
> If you have to repost this patch please address the checkpatch complaints
> of the following category triggered by the above declaration:
> 
> ERROR: "foo * bar" should be "foo *bar"
> 
> Otherwise this patch looks fine to me. Hence:
> 
> Reviewed-by: Bart Van Assche <Bart.VanAssche@sandisk.com>

OK. Will do.

-- 
Damien Le Moal,
Western Digital

[PATCH 7/9] sd_zbc: Rename sd_zbc_setup_write_cmnd

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

Rename sd_zbc_setup_write_cmnd() to sd_zbc_write_lock_zone() to be
clear about what the function actually does. To be consistent, also
rename sd_zbc_cancel_write_cmnd() to sd_zbc_write_unlock_zone().

No functional change is introduced by this patch.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/sd.c     |  8 ++++----
 drivers/scsi/sd.h     |  8 ++++----
 drivers/scsi/sd_zbc.c | 14 +++++---------
 3 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index a59a107..ce37d52 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -846,7 +846,7 @@ static int sd_setup_write_same_cmnd(struct scsi_cmnd *cmd)
 	BUG_ON(bio_offset(bio) || bio_iovec(bio).bv_len != sdp->sector_size);
 
 	if (sd_is_zoned(sdkp)) {
-		ret = sd_zbc_setup_write_cmnd(cmd);
+		ret = sd_zbc_write_lock_zone(cmd);
 		if (ret != BLKPREP_OK)
 			return ret;
 	}
@@ -918,7 +918,7 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
 	unsigned char protect;
 
 	if (zoned_write) {
-		ret = sd_zbc_setup_write_cmnd(SCpnt);
+		ret = sd_zbc_write_lock_zone(SCpnt);
 		if (ret != BLKPREP_OK)
 			return ret;
 	}
@@ -1145,7 +1145,7 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
 	ret = BLKPREP_OK;
  out:
 	if (zoned_write && ret != BLKPREP_OK)
-		sd_zbc_cancel_write_cmnd(SCpnt);
+		sd_zbc_write_unlock_zone(SCpnt);
 
 	return ret;
 }
@@ -1783,7 +1783,7 @@ static unsigned int sd_completed_bytes(struct scsi_cmnd *scmd)
 		return 0;
 
 	/*
-	 * resid is optional but mostly filled in.  When it's unused,
+	 * resid is optional but mostly filled in. When it's unused,
 	 * its value is zero, so we assume the whole buffer transferred
 	 */
 	transferred = scsi_bufflen(scmd) - scsi_get_resid(scmd);
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 0cf9680..2044e07a 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -267,8 +267,8 @@ static inline int sd_is_zoned(struct scsi_disk *sdkp)
 extern int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buffer);
 extern void sd_zbc_remove(struct scsi_disk *sdkp);
 extern void sd_zbc_print_zones(struct scsi_disk *sdkp);
-extern int sd_zbc_setup_write_cmnd(struct scsi_cmnd *cmd);
-extern void sd_zbc_cancel_write_cmnd(struct scsi_cmnd *cmd);
+extern int sd_zbc_write_lock_zone(struct scsi_cmnd *cmd);
+extern void sd_zbc_write_unlock_zone(struct scsi_cmnd *cmd);
 extern int sd_zbc_setup_report_cmnd(struct scsi_cmnd *cmd);
 extern int sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd);
 extern void sd_zbc_complete(struct scsi_cmnd *cmd, unsigned int good_bytes,
@@ -286,13 +286,13 @@ static inline void sd_zbc_remove(struct scsi_disk *sdkp) {}
 
 static inline void sd_zbc_print_zones(struct scsi_disk *sdkp) {}
 
-static inline int sd_zbc_setup_write_cmnd(struct scsi_cmnd *cmd)
+static inline int sd_zbc_write_lock_zone(struct scsi_cmnd *cmd)
 {
 	/* Let the drive fail requests */
 	return BLKPREP_OK;
 }
 
-static inline void sd_zbc_cancel_write_cmnd(struct scsi_cmnd *cmd) {}
+static inline void sd_zbc_write_unlock_zone(struct scsi_cmnd *cmd) {}
 
 static inline int sd_zbc_setup_report_cmnd(struct scsi_cmnd *cmd)
 {
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 92620c8..13a10ea 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -269,7 +269,7 @@ int sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd)
 	return BLKPREP_OK;
 }
 
-int sd_zbc_setup_write_cmnd(struct scsi_cmnd *cmd)
+int sd_zbc_write_lock_zone(struct scsi_cmnd *cmd)
 {
 	struct request *rq = cmd->request;
 	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
@@ -278,7 +278,7 @@ int sd_zbc_setup_write_cmnd(struct scsi_cmnd *cmd)
 	unsigned int zno = sd_zbc_zone_no(sdkp, sector);
 
 	/*
-	 * Note: Checks of the alignment of the write command on
+	 * Note: Checks of the alignment of the command on
 	 * logical blocks is done in sd.c
 	 */
 
@@ -303,8 +303,9 @@ int sd_zbc_setup_write_cmnd(struct scsi_cmnd *cmd)
 	return BLKPREP_OK;
 }
 
-static void sd_zbc_unlock_zone(struct request *rq)
+void sd_zbc_write_unlock_zone(struct scsi_cmnd *cmd)
 {
+	struct request *rq = cmd->request;
 	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
 
 	if (sdkp->zones_wlock) {
@@ -315,11 +316,6 @@ static void sd_zbc_unlock_zone(struct request *rq)
 	}
 }
 
-void sd_zbc_cancel_write_cmnd(struct scsi_cmnd *cmd)
-{
-	sd_zbc_unlock_zone(cmd->request);
-}
-
 void sd_zbc_complete(struct scsi_cmnd *cmd,
 		     unsigned int good_bytes,
 		     struct scsi_sense_hdr *sshdr)
@@ -333,7 +329,7 @@ void sd_zbc_complete(struct scsi_cmnd *cmd,
 	case REQ_OP_ZONE_RESET:
 
 		/* Unlock the zone */
-		sd_zbc_unlock_zone(rq);
+		sd_zbc_write_unlock_zone(cmd);
 
 		if (!result ||
 		    sshdr->sense_key != ILLEGAL_REQUEST)
-- 
2.9.3

Re: [PATCH 7/9] sd_zbc: Rename sd_zbc_setup_write_cmnd

[Bart Van Assche]

On Fri, 2017-04-21 at 18:16 +0900, damien.lemoal@wdc.com wrote:
> Rename sd_zbc_setup_write_cmnd() to sd_zbc_write_lock_zone() to be
> clear about what the function actually does. To be consistent, also
> rename sd_zbc_cancel_write_cmnd() to sd_zbc_write_unlock_zone().
> 
> [ ... ]
>  	/*
> -	 * resid is optional but mostly filled in.  When it's unused,
> +	 * resid is optional but mostly filled in. When it's unused,
>  	 * its value is zero, so we assume the whole buffer transferred
>  	 */

Is this whitespace-only change useful? Otherwise this patch looks fine
to me. Hence:

Reviewed-by: Bart Van Assche <Bart.VanAssche@sandisk.com>

Re: [PATCH 7/9] sd_zbc: Rename sd_zbc_setup_write_cmnd

[Damien Le Moal]

Bart,

On 4/22/17 02:07, Bart Van Assche wrote:
> On Fri, 2017-04-21 at 18:16 +0900, damien.lemoal@wdc.com wrote:
>> Rename sd_zbc_setup_write_cmnd() to sd_zbc_write_lock_zone() to be
>> clear about what the function actually does. To be consistent, also
>> rename sd_zbc_cancel_write_cmnd() to sd_zbc_write_unlock_zone().
>> 
>> [ ... ]
>>        /*
>> -      * resid is optional but mostly filled in.  When it's unused,
>> +      * resid is optional but mostly filled in. When it's unused,
>>         * its value is zero, so we assume the whole buffer transferred
>>         */
> 
> Is this whitespace-only change useful? Otherwise this patch looks fine
> to me. Hence:
> 
> Reviewed-by: Bart Van Assche <Bart.VanAssche@sandisk.com>

Ooops... No it is not. I will resend a v2.
Thanks.


-- 
Damien Le Moal,
Western Digital

[PATCH 8/9] sd_zbc: Remove superfluous assignments

[damien.lemoal]

From: Bart Van Assche <bart.vanassche@sandisk.com>

A value is assigned to the variable 'capacity' in sd_zbc_read_zones()
but that value is never used. Hence remove the variable 'capacity'.

[Damien: There is no need to initialize to 0 the variable 'ret'
in sd_zbc_read_zones()]

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/sd_zbc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 13a10ea..1cf8897 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -560,8 +560,7 @@ static int sd_zbc_setup(struct scsi_disk *sdkp)
 int sd_zbc_read_zones(struct scsi_disk *sdkp,
 		      unsigned char *buf)
 {
-	sector_t capacity;
-	int ret = 0;
+	int ret;
 
 	if (!sd_is_zoned(sdkp))
 		/*
@@ -593,7 +592,6 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp,
 	ret = sd_zbc_check_capacity(sdkp, buf);
 	if (ret)
 		goto err;
-	capacity = logical_to_sectors(sdkp->device, sdkp->capacity);
 
 	/*
 	 * Check zone size: only devices with a constant zone size (except
-- 
2.9.3

[PATCH 9/9] sd_zbc: Do not write lock zones for reset

[damien.lemoal]

From: Damien Le Moal <damien.lemoal@wdc.com>

Resetting a zone write pointer is equivalent to discarding sectors:
after a reset, the zone sectors will contain zeros (or the format
pattern). So there is no need for mutual exclusion between a zone reset
and write. Similarly to discard, make it the responsability of the user
to properly synchronize between reset and write (as is done now for
discard and write).

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/sd_zbc.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 1cf8897..b47251a 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -237,7 +237,6 @@ int sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd)
 	struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
 	sector_t sector = blk_rq_pos(rq);
 	sector_t block = sectors_to_logical(sdkp->device, sector);
-	unsigned int zno = block >> sdkp->zone_shift;
 
 	if (!sd_is_zoned(sdkp))
 		/* Not a zoned device */
@@ -250,11 +249,6 @@ int sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd)
 		/* Unaligned request */
 		return BLKPREP_KILL;
 
-	/* Do not allow concurrent reset and writes */
-	if (sdkp->zones_wlock &&
-	    test_and_set_bit(zno, sdkp->zones_wlock))
-		return BLKPREP_DEFER;
-
 	cmd->cmd_len = 16;
 	memset(cmd->cmnd, 0, cmd->cmd_len);
 	cmd->cmnd[0] = ZBC_OUT;
@@ -324,38 +318,34 @@ void sd_zbc_complete(struct scsi_cmnd *cmd,
 	struct request *rq = cmd->request;
 
 	switch (req_op(rq)) {
+	case REQ_OP_ZONE_RESET:
+
+		if (result &&
+		    sshdr->sense_key == ILLEGAL_REQUEST &&
+		    sshdr->asc == 0x24)
+			/*
+			 * INVALID FIELD IN CDB error: reset of a conventional
+			 * zone was attempted. Nothing to worry about, so be
+			 * quiet about the error.
+			 */
+			rq->rq_flags |= RQF_QUIET;
+		break;
+
 	case REQ_OP_WRITE:
 	case REQ_OP_WRITE_SAME:
-	case REQ_OP_ZONE_RESET:
 
 		/* Unlock the zone */
 		sd_zbc_write_unlock_zone(cmd);
 
-		if (!result ||
-		    sshdr->sense_key != ILLEGAL_REQUEST)
-			break;
-
-		switch (sshdr->asc) {
-		case 0x24:
-			/*
-			 * INVALID FIELD IN CDB error: For a zone reset,
-			 * this means that a reset of a conventional
-			 * zone was attempted. Nothing to worry about in
-			 * this case, so be quiet about the error.
-			 */
-			if (req_op(rq) == REQ_OP_ZONE_RESET)
-				rq->rq_flags |= RQF_QUIET;
-			break;
-		case 0x21:
+		if (result &&
+		    sshdr->sense_key == ILLEGAL_REQUEST &&
+		    sshdr->asc == 0x21)
 			/*
 			 * INVALID ADDRESS FOR WRITE error: It is unlikely that
 			 * retrying write requests failed with any kind of
 			 * alignement error will result in success. So don't.
 			 */
 			cmd->allowed = 0;
-			break;
-		}
-
 		break;
 
 	case REQ_OP_ZONE_REPORT:
-- 
2.9.3

Re: [PATCH 9/9] sd_zbc: Do not write lock zones for reset

[Bart Van Assche]

On Fri, 2017-04-21 at 18:16 +0900, damien.lemoal@wdc.com wrote:
> @@ -250,11 +249,6 @@ int sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd)
>  		/* Unaligned request */
>  		return BLKPREP_KILL;
>  
> -	/* Do not allow concurrent reset and writes */
> -	if (sdkp->zones_wlock &&
> -	    test_and_set_bit(zno, sdkp->zones_wlock))
> -		return BLKPREP_DEFER;
> -

Hello Damien,

Since concurrent zone resets and writes are easy to detect and since these
indicate a bug in the application that submits these, should an error message
be logged if that happens? Otherwise this patch looks fine to me.

Bart.

Re: [PATCH 9/9] sd_zbc: Do not write lock zones for reset

[Damien Le Moal]

Bart,

On 4/22/17 02:15, Bart Van Assche wrote:
> On Fri, 2017-04-21 at 18:16 +0900, damien.lemoal@wdc.com wrote:
>> @@ -250,11 +249,6 @@ int sd_zbc_setup_reset_cmnd(struct scsi_cmnd *cmd)
>>                /* Unaligned request */
>>                return BLKPREP_KILL;
>>  
>> -     /* Do not allow concurrent reset and writes */
>> -     if (sdkp->zones_wlock &&
>> -         test_and_set_bit(zno, sdkp->zones_wlock))
>> -             return BLKPREP_DEFER;
>> -
> 
> Hello Damien,
> 
> Since concurrent zone resets and writes are easy to detect and since these
> indicate a bug in the application that submits these, should an error
> message
> be logged if that happens? Otherwise this patch looks fine to me.

Yes, we could easily add a warning, but if we consider the 3 possible
cases of reset+write combinations:
1) Reset comes in while a write is already in the queue: The write will
succeed (assuming it is directed at the zone write pointer) and the
reset will also succeed.
2) A write directed at the current write pointer comes in while a reset
is in the queue: the write will fail.
3) A write directed at the beginning of the zone comes in while a reset
is in the queue: the write will succeed.

The warning would trigger only in case (1), but that case may be
actually intentional by the user. Case (2) will get an "indirect"
warning through the write align error, which clearly signals a bug to
the user. For case (3), that may also be intentional by the user (not
recommended though).

So I am afraid that a warning may trigger false-positives and may not be
that useful in the end.

Best regards.

-- 
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com