misc scsi ioctl updates V2

misc scsi ioctl updates V2

[Christoph Hellwig]

This series (against the core-for-3.19 branch) deals with various
issues realated to scsi-ioctls, mostly with the SG_SCSI_RESET ioctl.

Changes since V1:
  - fix scsi_ioctl_reset return value, pointed out by Bart Van Assche

[PATCH 1/6] scsi: refactor scsi_reset_provider handling

[Christoph Hellwig]

Pull the common code from the two callers into the function,
and renamed it to scsi_ioctl_reset.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_error.c | 76 ++++++++++++++++++++++-------------------------
 drivers/scsi/scsi_ioctl.c | 33 +-------------------
 drivers/scsi/sg.c         | 34 ++-------------------
 include/scsi/scsi_eh.h    | 15 +---------
 4 files changed, 41 insertions(+), 117 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index fa7b5ec..ba19687 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -36,6 +36,7 @@
 #include <scsi/scsi_transport.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>
+#include <scsi/sg.h>
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
@@ -2309,39 +2310,36 @@ scsi_reset_provider_done_command(struct scsi_cmnd *scmd)
 {
 }
 
-/*
- * Function:	scsi_reset_provider
- *
- * Purpose:	Send requested reset to a bus or device at any phase.
- *
- * Arguments:	device	- device to send reset to
- *		flag - reset type (see scsi.h)
- *
- * Returns:	SUCCESS/FAILURE.
- *
- * Notes:	This is used by the SCSI Generic driver to provide
- *		Bus/Device reset capability.
+/**
+ * scsi_ioctl_reset: explicitly reset a host/bus/target/device
+ * @dev:	scsi_device to operate on
+ * @val:	reset type (see sg.h)
  */
 int
-scsi_reset_provider(struct scsi_device *dev, int flag)
+scsi_ioctl_reset(struct scsi_device *dev, int __user *arg)
 {
 	struct scsi_cmnd *scmd;
 	struct Scsi_Host *shost = dev->host;
 	struct request req;
 	unsigned long flags;
-	int rtn;
+	int error = 0, rtn, val;
+
+	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
+		return -EACCES;
+
+	error = get_user(val, arg);
+	if (error)
+		return error;
 
 	if (scsi_autopm_get_host(shost) < 0)
-		return FAILED;
+		return -EIO;
 
-	if (!get_device(&dev->sdev_gendev)) {
-		rtn = FAILED;
+	error = -EIO;
+	if (!get_device(&dev->sdev_gendev))
 		goto out_put_autopm_host;
-	}
 
 	scmd = scsi_get_command(dev, GFP_KERNEL);
 	if (!scmd) {
-		rtn = FAILED;
 		put_device(&dev->sdev_gendev);
 		goto out_put_autopm_host;
 	}
@@ -2362,39 +2360,37 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
 	shost->tmf_in_progress = 1;
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
-	switch (flag) {
-	case SCSI_TRY_RESET_DEVICE:
+	switch (val & ~SG_SCSI_RESET_NO_ESCALATE) {
+	case SG_SCSI_RESET_NOTHING:
+		rtn = SUCCESS;
+		break;
+	case SG_SCSI_RESET_DEVICE:
 		rtn = scsi_try_bus_device_reset(scmd);
-		if (rtn == SUCCESS)
+		if (rtn == SUCCESS || (val & SG_SCSI_RESET_NO_ESCALATE))
 			break;
 		/* FALLTHROUGH */
-	case SCSI_TRY_RESET_TARGET:
+	case SG_SCSI_RESET_TARGET:
 		rtn = scsi_try_target_reset(scmd);
-		if (rtn == SUCCESS)
+		if (rtn == SUCCESS || (val & SG_SCSI_RESET_NO_ESCALATE))
 			break;
 		/* FALLTHROUGH */
-	case SCSI_TRY_RESET_BUS:
+	case SG_SCSI_RESET_BUS:
 		rtn = scsi_try_bus_reset(scmd);
-		if (rtn == SUCCESS)
+		if (rtn == SUCCESS || (val & SG_SCSI_RESET_NO_ESCALATE))
 			break;
 		/* FALLTHROUGH */
-	case SCSI_TRY_RESET_HOST:
-	case SCSI_TRY_RESET_HOST | SCSI_TRY_RESET_NO_ESCALATE:
+	case SG_SCSI_RESET_HOST:
 		rtn = scsi_try_host_reset(scmd);
-		break;
-	case SCSI_TRY_RESET_DEVICE | SCSI_TRY_RESET_NO_ESCALATE:
-		rtn = scsi_try_bus_device_reset(scmd);
-		break;
-	case SCSI_TRY_RESET_TARGET | SCSI_TRY_RESET_NO_ESCALATE:
-		rtn = scsi_try_target_reset(scmd);
-		break;
-	case SCSI_TRY_RESET_BUS | SCSI_TRY_RESET_NO_ESCALATE:
-		rtn = scsi_try_bus_reset(scmd);
-		break;
+		if (rtn == SUCCESS)
+			break;
 	default:
+		/* FALLTHROUGH */
 		rtn = FAILED;
+		break;
 	}
 
+	error = (rtn == SUCCESS) ? 0 : -EIO;
+
 	spin_lock_irqsave(shost->host_lock, flags);
 	shost->tmf_in_progress = 0;
 	spin_unlock_irqrestore(shost->host_lock, flags);
@@ -2414,9 +2410,9 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
 	scsi_next_command(scmd);
 out_put_autopm_host:
 	scsi_autopm_put_host(shost);
-	return rtn;
+	return error;
 }
-EXPORT_SYMBOL(scsi_reset_provider);
+EXPORT_SYMBOL(scsi_ioctl_reset);
 
 /**
  * scsi_normalize_sense - normalize main elements from either fixed or
diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index 5207274..5ddc08f 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -292,8 +292,6 @@ EXPORT_SYMBOL(scsi_ioctl);
 int scsi_nonblockable_ioctl(struct scsi_device *sdev, int cmd,
 			    void __user *arg, int ndelay)
 {
-	int val, val2, result;
-
 	/* The first set of iocts may be executed even if we're doing
 	 * error processing, as long as the device was opened
 	 * non-blocking */
@@ -305,36 +303,7 @@ int scsi_nonblockable_ioctl(struct scsi_device *sdev, int cmd,
 
 	switch (cmd) {
 	case SG_SCSI_RESET:
-		result = get_user(val, (int __user *)arg);
-		if (result)
-			return result;
-		if (val & SG_SCSI_RESET_NO_ESCALATE) {
-			val &= ~SG_SCSI_RESET_NO_ESCALATE;
-			val2 = SCSI_TRY_RESET_NO_ESCALATE;
-		} else
-			val2 = 0;
-		if (val == SG_SCSI_RESET_NOTHING)
-			return 0;
-		switch (val) {
-		case SG_SCSI_RESET_DEVICE:
-			val2 |= SCSI_TRY_RESET_DEVICE;
-			break;
-		case SG_SCSI_RESET_TARGET:
-			val2 |= SCSI_TRY_RESET_TARGET;
-			break;
-		case SG_SCSI_RESET_BUS:
-			val2 |= SCSI_TRY_RESET_BUS;
-			break;
-		case SG_SCSI_RESET_HOST:
-			val2 |= SCSI_TRY_RESET_HOST;
-			break;
-		default:
-			return -EINVAL;
-		}
-		if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
-			return -EACCES;
-		return (scsi_reset_provider(sdev, val2) ==
-			SUCCESS) ? 0 : -EIO;
+		return scsi_ioctl_reset(sdev, arg);
 	}
 	return -ENODEV;
 }
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 2fe2701..7c55cac 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -847,7 +847,7 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 {
 	void __user *p = (void __user *)arg;
 	int __user *ip = p;
-	int result, val, val2, read_only;
+	int result, val, read_only;
 	Sg_device *sdp;
 	Sg_fd *sfp;
 	Sg_request *srp;
@@ -1079,36 +1079,8 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 				return -EBUSY;
 		} else if (!scsi_block_when_processing_errors(sdp->device))
 			return -EBUSY;
-		result = get_user(val, ip);
-		if (result)
-			return result;
-		if (val & SG_SCSI_RESET_NO_ESCALATE) {
-			val &= ~SG_SCSI_RESET_NO_ESCALATE;
-			val2 = SCSI_TRY_RESET_NO_ESCALATE;
-		} else
-			val2 = 0;
-		if (SG_SCSI_RESET_NOTHING == val)
-			return 0;
-		switch (val) {
-		case SG_SCSI_RESET_DEVICE:
-			val2 |= SCSI_TRY_RESET_DEVICE;
-			break;
-		case SG_SCSI_RESET_TARGET:
-			val2 |= SCSI_TRY_RESET_TARGET;
-			break;
-		case SG_SCSI_RESET_BUS:
-			val2 |= SCSI_TRY_RESET_BUS;
-			break;
-		case SG_SCSI_RESET_HOST:
-			val2 |= SCSI_TRY_RESET_HOST;
-			break;
-		default:
-			return -EINVAL;
-		}
-		if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
-			return -EACCES;
-		return (scsi_reset_provider(sdp->device, val2) ==
-			SUCCESS) ? 0 : -EIO;
+
+		return scsi_ioctl_reset(sdp->device, ip);
 	case SCSI_IOCTL_SEND_COMMAND:
 		if (atomic_read(&sdp->detaching))
 			return -ENODEV;
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index 2562481..1e1421b 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -60,20 +60,7 @@ extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
 
 extern void scsi_build_sense_buffer(int desc, u8 *buf, u8 key, u8 asc, u8 ascq);
 
-/*
- * Reset request from external source
- * Note: if SCSI_TRY_RESET_DEVICE fails then it will escalate to
- * SCSI_TRY_RESET_TARGET which if it fails will escalate to
- * SCSI_TRY_RESET_BUS which if it fails will escalate to SCSI_TRY_RESET_HOST.
- * To prevent escalation OR with SCSI_TRY_RESET_NO_ESCALATE.
- */
-#define SCSI_TRY_RESET_DEVICE	1
-#define SCSI_TRY_RESET_BUS	2
-#define SCSI_TRY_RESET_HOST	3
-#define SCSI_TRY_RESET_TARGET	4
-#define SCSI_TRY_RESET_NO_ESCALATE	0x100	/* OR-ed to prior defines */
-
-extern int scsi_reset_provider(struct scsi_device *, int);
+extern int scsi_ioctl_reset(struct scsi_device *, int __user *);
 
 struct scsi_eh_save {
 	/* saved state */
-- 
1.9.1

Re: [PATCH 1/6] scsi: refactor scsi_reset_provider handling

[Hannes Reinecke]

On 10/30/2014 10:27 AM, Christoph Hellwig wrote:
> Pull the common code from the two callers into the function,
> and renamed it to scsi_ioctl_reset.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] scsi: refactor scsi_reset_provider handling

[Hannes Reinecke]

On 10/30/2014 10:27 AM, Christoph Hellwig wrote:
> Pull the common code from the two callers into the function,
> and renamed it to scsi_ioctl_reset.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] scsi: refactor scsi_reset_provider handling

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> Pull the common code from the two callers into the function,
Christoph> and renamed it to scsi_ioctl_reset.

s/renamed/rename/

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH 1/6] scsi: refactor scsi_reset_provider handling

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Christoph Hellwig
> Sent: Thursday, 30 October, 2014 4:27 AM
> To: linux-scsi@vger.kernel.org
> Cc: Douglas Gilbert; Elliott, Robert (Server Storage)
> Subject: [PATCH 1/6] scsi: refactor scsi_reset_provider handling
> 
> Pull the common code from the two callers into the function,
> and renamed it to scsi_ioctl_reset.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/scsi_error.c | 76 ++++++++++++++++++++++---------------
> ----------
>  drivers/scsi/scsi_ioctl.c | 33 +-------------------
>  drivers/scsi/sg.c         | 34 ++-------------------
>  include/scsi/scsi_eh.h    | 15 +---------
>  4 files changed, 41 insertions(+), 117 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index fa7b5ec..ba19687 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -36,6 +36,7 @@
>  #include <scsi/scsi_transport.h>
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsi_ioctl.h>
> +#include <scsi/sg.h>
> 
>  #include "scsi_priv.h"
>  #include "scsi_logging.h"
> @@ -2309,39 +2310,36 @@ scsi_reset_provider_done_command(struct
> scsi_cmnd *scmd)
>  {
>  }
> 
> -/*
> - * Function:	scsi_reset_provider
> - *
> - * Purpose:	Send requested reset to a bus or device at any phase.
> - *
> - * Arguments:	device	- device to send reset to
> - *		flag - reset type (see scsi.h)
> - *
> - * Returns:	SUCCESS/FAILURE.
> - *
> - * Notes:	This is used by the SCSI Generic driver to provide
> - *		Bus/Device reset capability.
> +/**
> + * scsi_ioctl_reset: explicitly reset a host/bus/target/device
> + * @dev:	scsi_device to operate on
> + * @val:	reset type (see sg.h)
>   */
>  int
> -scsi_reset_provider(struct scsi_device *dev, int flag)
> +scsi_ioctl_reset(struct scsi_device *dev, int __user *arg)
>  {
>  	struct scsi_cmnd *scmd;
>  	struct Scsi_Host *shost = dev->host;
>  	struct request req;

Is this 368 byte structure too big to place on the stack?  
Most other code using struct request uses an alloc() 
function.  Also, on the stack, it is unlikely to be
cacheline aligned.

>  	unsigned long flags;
> -	int rtn;
> +	int error = 0, rtn, val;
> +

No need to initialize error, as it is unilaterally set 
4 lines below.

> +	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> +		return -EACCES;
> +
> +	error = get_user(val, arg);
> +	if (error)
> +		return error;
> 
>  	if (scsi_autopm_get_host(shost) < 0)
> -		return FAILED;
> +		return -EIO;
>

This could propagate the return value from scsi_autopm_get_host,
which is the value from pm_runtime_get_sync, 
which is the value from __pm_runtime_resume,
which is the value from rpm_resume,
which could be -EINVAL, -EACCESS, -EINPROGRESS, -EBUSY, or
the value from rpm_callback, 
which could be -ENOSYS, etc.

> -	if (!get_device(&dev->sdev_gendev)) {
> -		rtn = FAILED;
> +	error = -EIO;
> +	if (!get_device(&dev->sdev_gendev))
>  		goto out_put_autopm_host;
> -	}
> 
>  	scmd = scsi_get_command(dev, GFP_KERNEL);
>  	if (!scmd) {
> -		rtn = FAILED;
>  		put_device(&dev->sdev_gendev);
>  		goto out_put_autopm_host;
>  	}
> @@ -2362,39 +2360,37 @@ scsi_reset_provider(struct scsi_device *dev,
> int flag)
>  	shost->tmf_in_progress = 1;
>  	spin_unlock_irqrestore(shost->host_lock, flags);
> 
> -	switch (flag) {
> -	case SCSI_TRY_RESET_DEVICE:
> +	switch (val & ~SG_SCSI_RESET_NO_ESCALATE) {
> +	case SG_SCSI_RESET_NOTHING:
> +		rtn = SUCCESS;
> +		break;
> +	case SG_SCSI_RESET_DEVICE:
>  		rtn = scsi_try_bus_device_reset(scmd);
> -		if (rtn == SUCCESS)
> +		if (rtn == SUCCESS || (val & SG_SCSI_RESET_NO_ESCALATE))
>  			break;
>  		/* FALLTHROUGH */
> -	case SCSI_TRY_RESET_TARGET:
> +	case SG_SCSI_RESET_TARGET:
>  		rtn = scsi_try_target_reset(scmd);
> -		if (rtn == SUCCESS)
> +		if (rtn == SUCCESS || (val & SG_SCSI_RESET_NO_ESCALATE))
>  			break;
>  		/* FALLTHROUGH */
> -	case SCSI_TRY_RESET_BUS:
> +	case SG_SCSI_RESET_BUS:
>  		rtn = scsi_try_bus_reset(scmd);
> -		if (rtn == SUCCESS)
> +		if (rtn == SUCCESS || (val & SG_SCSI_RESET_NO_ESCALATE))
>  			break;
>  		/* FALLTHROUGH */
> -	case SCSI_TRY_RESET_HOST:
> -	case SCSI_TRY_RESET_HOST | SCSI_TRY_RESET_NO_ESCALATE:
> +	case SG_SCSI_RESET_HOST:
>  		rtn = scsi_try_host_reset(scmd);
> -		break;
> -	case SCSI_TRY_RESET_DEVICE | SCSI_TRY_RESET_NO_ESCALATE:
> -		rtn = scsi_try_bus_device_reset(scmd);
> -		break;
> -	case SCSI_TRY_RESET_TARGET | SCSI_TRY_RESET_NO_ESCALATE:
> -		rtn = scsi_try_target_reset(scmd);
> -		break;
> -	case SCSI_TRY_RESET_BUS | SCSI_TRY_RESET_NO_ESCALATE:
> -		rtn = scsi_try_bus_reset(scmd);
> -		break;
> +		if (rtn == SUCCESS)
> +			break;
>  	default:
> +		/* FALLTHROUGH */
>  		rtn = FAILED;
> +		break;

The FALLTHROUGH comment belongs above the default: line
to match the others.

>  	}
> 
> +	error = (rtn == SUCCESS) ? 0 : -EIO;
> +
>  	spin_lock_irqsave(shost->host_lock, flags);
>  	shost->tmf_in_progress = 0;
>  	spin_unlock_irqrestore(shost->host_lock, flags);


Reviewed-by: Robert Elliott <elliott@hp.com>

---
Rob Elliott    HP Server Storage

[PATCH 2/6] scsi: split scsi_nonblockable_ioctl

[Christoph Hellwig]

The calling conventions for this function where bad as it could return
-ENODEV both for a device not currently online and a not recognized ioctl.

Add a new scsi_ioctl_block_when_processing_errors function that wraps
scsi_block_when_processing_errors with the a special case for the
SG_SCSI_RESET ioctl command, and handle the SG_SCSI_RESET case itself
in scsi_ioctl.  All callers of scsi_ioctl now must call the above helper
to check for the EH state, so that the ioctl handler itself doesn't
have to.

Reported-by: Robert Elliott <Elliott@hp.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/ch.c         |  5 +++++
 drivers/scsi/osst.c       |  6 +++---
 drivers/scsi/scsi_ioctl.c | 47 +++++++++++++----------------------------------
 drivers/scsi/sd.c         |  6 +++---
 drivers/scsi/sg.c         | 33 +++++++++++++++------------------
 drivers/scsi/sr.c         | 15 +++++----------
 drivers/scsi/st.c         |  7 +++----
 include/scsi/scsi_ioctl.h |  4 ++--
 8 files changed, 49 insertions(+), 74 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 226ef77..4f502f9 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -616,6 +616,11 @@ static long ch_ioctl(struct file *file,
 	int retval;
 	void __user *argp = (void __user *)arg;
 
+	retval = scsi_ioctl_block_when_processing_errors(ch->device, cmd,
+			file->f_flags & O_NDELAY);
+	if (retval)
+		return retval;
+
 	switch (cmd) {
 	case CHIOGPARAMS:
 	{
diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c
index 3d0d13c..b6d63d6 100644
--- a/drivers/scsi/osst.c
+++ b/drivers/scsi/osst.c
@@ -4969,10 +4969,10 @@ static long osst_ioctl(struct file * file,
 	 * may try and take the device offline, in which case all further
 	 * access to the device is prohibited.
 	 */
-	if( !scsi_block_when_processing_errors(STp->device) ) {
-		retval = (-ENXIO);
+	retval = scsi_ioctl_block_when_processing_errors(STp->device, cmd_in,
+			file->f_flags & O_NDELAY);
+	if (retval)
 		goto out;
-	}
 
 	cmd_type = _IOC_TYPE(cmd_in);
 	cmd_nr   = _IOC_NR(cmd_in);
diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index 5ddc08f..712f159 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -200,19 +200,6 @@ int scsi_ioctl(struct scsi_device *sdev, int cmd, void __user *arg)
 {
 	char scsi_cmd[MAX_COMMAND_SIZE];
 
-	/* No idea how this happens.... */
-	if (!sdev)
-		return -ENXIO;
-
-	/*
-	 * If we are in the middle of error recovery, don't let anyone
-	 * else try and use this device.  Also, if error recovery fails, it
-	 * may try and take the device offline, in which case all further
-	 * access to the device is prohibited.
-	 */
-	if (!scsi_block_when_processing_errors(sdev))
-		return -ENODEV;
-
 	/* Check for deprecated ioctls ... all the ioctls which don't
 	 * follow the new unique numbering scheme are deprecated */
 	switch (cmd) {
@@ -273,6 +260,8 @@ int scsi_ioctl(struct scsi_device *sdev, int cmd, void __user *arg)
 				     START_STOP_TIMEOUT, NORMAL_RETRIES);
         case SCSI_IOCTL_GET_PCI:
                 return scsi_ioctl_get_pci(sdev, arg);
+	case SG_SCSI_RESET:
+		return scsi_ioctl_reset(sdev, arg);
 	default:
 		if (sdev->host->hostt->ioctl)
 			return sdev->host->hostt->ioctl(sdev, cmd, arg);
@@ -281,30 +270,20 @@ int scsi_ioctl(struct scsi_device *sdev, int cmd, void __user *arg)
 }
 EXPORT_SYMBOL(scsi_ioctl);
 
-/**
- * scsi_nonblockable_ioctl() - Handle SG_SCSI_RESET
- * @sdev: scsi device receiving ioctl
- * @cmd: Must be SC_SCSI_RESET
- * @arg: pointer to int containing SG_SCSI_RESET_{DEVICE,TARGET,BUS,HOST}
- *       possibly OR-ed with SG_SCSI_RESET_NO_ESCALATE
- * @ndelay: file mode O_NDELAY flag
+/*
+ * We can process a reset even when a device isn't fully operable.
  */
-int scsi_nonblockable_ioctl(struct scsi_device *sdev, int cmd,
-			    void __user *arg, int ndelay)
+int scsi_ioctl_block_when_processing_errors(struct scsi_device *sdev, int cmd,
+		bool ndelay)
 {
-	/* The first set of iocts may be executed even if we're doing
-	 * error processing, as long as the device was opened
-	 * non-blocking */
-	if (ndelay) {
+	if (cmd == SG_SCSI_RESET && ndelay) {
 		if (scsi_host_in_recovery(sdev->host))
 			return -ENODEV;
-	} else if (!scsi_block_when_processing_errors(sdev))
-		return -ENODEV;
-
-	switch (cmd) {
-	case SG_SCSI_RESET:
-		return scsi_ioctl_reset(sdev, arg);
+	} else {
+		if (!scsi_block_when_processing_errors(sdev))
+			return -ENODEV;
 	}
-	return -ENODEV;
+
+	return 0;
 }
-EXPORT_SYMBOL(scsi_nonblockable_ioctl);
+EXPORT_SYMBOL_GPL(scsi_ioctl_block_when_processing_errors);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 242f9b1..61e50ae 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1334,9 +1334,9 @@ static int sd_ioctl(struct block_device *bdev, fmode_t mode,
 	 * may try and take the device offline, in which case all further
 	 * access to the device is prohibited.
 	 */
-	error = scsi_nonblockable_ioctl(sdp, cmd, p,
-					(mode & FMODE_NDELAY) != 0);
-	if (!scsi_block_when_processing_errors(sdp) || !error)
+	error = scsi_ioctl_block_when_processing_errors(sdp, cmd,
+			mode & FMODE_NDELAY);
+	if (error)
 		goto out;
 
 	/*
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 7c55cac..b14f64c 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1071,16 +1071,6 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 		if (atomic_read(&sdp->detaching))
 			return -ENODEV;
 		return put_user(sdp->device->host->hostt->emulated, ip);
-	case SG_SCSI_RESET:
-		if (atomic_read(&sdp->detaching))
-			return -ENODEV;
-		if (filp->f_flags & O_NONBLOCK) {
-			if (scsi_host_in_recovery(sdp->device->host))
-				return -EBUSY;
-		} else if (!scsi_block_when_processing_errors(sdp->device))
-			return -EBUSY;
-
-		return scsi_ioctl_reset(sdp->device, ip);
 	case SCSI_IOCTL_SEND_COMMAND:
 		if (atomic_read(&sdp->detaching))
 			return -ENODEV;
@@ -1100,13 +1090,6 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 			return result;
 		sdp->sgdebug = (char) val;
 		return 0;
-	case SCSI_IOCTL_GET_IDLUN:
-	case SCSI_IOCTL_GET_BUS_NUMBER:
-	case SCSI_IOCTL_PROBE_HOST:
-	case SG_GET_TRANSFORM:
-		if (atomic_read(&sdp->detaching))
-			return -ENODEV;
-		return scsi_ioctl(sdp->device, cmd_in, p);
 	case BLKSECTGET:
 		return put_user(max_sectors_bytes(sdp->device->request_queue),
 				ip);
@@ -1122,11 +1105,25 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 		return blk_trace_startstop(sdp->device->request_queue, 0);
 	case BLKTRACETEARDOWN:
 		return blk_trace_remove(sdp->device->request_queue);
+	case SCSI_IOCTL_GET_IDLUN:
+	case SCSI_IOCTL_GET_BUS_NUMBER:
+	case SCSI_IOCTL_PROBE_HOST:
+	case SG_GET_TRANSFORM:
+	case SG_SCSI_RESET:
+		if (atomic_read(&sdp->detaching))
+			return -ENODEV;
+		break;
 	default:
 		if (read_only)
 			return -EPERM;	/* don't know so take safe approach */
-		return scsi_ioctl(sdp->device, cmd_in, p);
+		break;
 	}
+
+	result = scsi_ioctl_block_when_processing_errors(sdp->device,
+			cmd_in, filp->f_flags & O_NDELAY);
+	if (result)
+		return result;
+	return scsi_ioctl(sdp->device, cmd_in, p);
 }
 
 #ifdef CONFIG_COMPAT
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 2de44cc..1d9eabf 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -549,6 +549,11 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 
 	mutex_lock(&sr_mutex);
 
+	ret = scsi_ioctl_block_when_processing_errors(sdev, cmd,
+			mode & FMODE_NDELAY);
+	if (ret)
+		goto out;
+
 	/*
 	 * Send SCSI addressing ioctls directly to mid level, send other
 	 * ioctls to cdrom/block level.
@@ -564,16 +569,6 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 	if (ret != -ENOSYS)
 		goto out;
 
-	/*
-	 * ENODEV means that we didn't recognise the ioctl, or that we
-	 * cannot execute it in the current device state.  In either
-	 * case fall through to scsi_ioctl, which will return ENDOEV again
-	 * if it doesn't recognise the ioctl
-	 */
-	ret = scsi_nonblockable_ioctl(sdev, cmd, argp,
-					(mode & FMODE_NDELAY) != 0);
-	if (ret != -ENODEV)
-		goto out;
 	ret = scsi_ioctl(sdev, cmd, argp);
 
 out:
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 63c35ed..7d2e036 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -3376,11 +3376,10 @@ static long st_ioctl(struct file *file, unsigned int cmd_in, unsigned long arg)
 	 * may try and take the device offline, in which case all further
 	 * access to the device is prohibited.
 	 */
-	retval = scsi_nonblockable_ioctl(STp->device, cmd_in, p,
-					file->f_flags & O_NDELAY);
-	if (!scsi_block_when_processing_errors(STp->device) || retval != -ENODEV)
+	retval = scsi_ioctl_block_when_processing_errors(STp->device, cmd_in,
+			file->f_flags & O_NDELAY);
+	if (retval)
 		goto out;
-	retval = 0;
 
 	cmd_type = _IOC_TYPE(cmd_in);
 	cmd_nr = _IOC_NR(cmd_in);
diff --git a/include/scsi/scsi_ioctl.h b/include/scsi/scsi_ioctl.h
index b900684..8d19d1d 100644
--- a/include/scsi/scsi_ioctl.h
+++ b/include/scsi/scsi_ioctl.h
@@ -40,9 +40,9 @@ typedef struct scsi_fctargaddress {
 	unsigned char host_wwn[8]; // include NULL term.
 } Scsi_FCTargAddress;
 
+int scsi_ioctl_block_when_processing_errors(struct scsi_device *sdev,
+		int cmd, bool ndelay);
 extern int scsi_ioctl(struct scsi_device *, int, void __user *);
-extern int scsi_nonblockable_ioctl(struct scsi_device *sdev, int cmd,
-				   void __user *arg, int ndelay);
 
 #endif /* __KERNEL__ */
 #endif /* _SCSI_IOCTL_H */
-- 
1.9.1

Re: [PATCH 2/6] scsi: split scsi_nonblockable_ioctl

[Hannes Reinecke]

On 10/30/2014 10:27 AM, Christoph Hellwig wrote:
> The calling conventions for this function where bad as it could return
> -ENODEV both for a device not currently online and a not recognized ioctl.
> 
> Add a new scsi_ioctl_block_when_processing_errors function that wraps
> scsi_block_when_processing_errors with the a special case for the
> SG_SCSI_RESET ioctl command, and handle the SG_SCSI_RESET case itself
> in scsi_ioctl.  All callers of scsi_ioctl now must call the above helper
> to check for the EH state, so that the ioctl handler itself doesn't
> have to.
> 
> Reported-by: Robert Elliott <Elliott@hp.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] scsi: split scsi_nonblockable_ioctl

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> The calling conventions for this function where bad as it
Christoph> could return -ENODEV both for a device not currently online
Christoph> and a not recognized ioctl.

s/where/were/

Christoph> Add a new scsi_ioctl_block_when_processing_errors function
Christoph> that wraps scsi_block_when_processing_errors with the a
Christoph> special case for the SG_SCSI_RESET ioctl command, and handle
Christoph> the SG_SCSI_RESET case itself in scsi_ioctl.  All callers of
Christoph> scsi_ioctl now must call the above helper to check for the EH
Christoph> state, so that the ioctl handler itself doesn't have to.

Nice cleanup!

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH 2/6] scsi: split scsi_nonblockable_ioctl

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@lst.de]
> Sent: Thursday, 30 October, 2014 4:27 AM
> To: linux-scsi@vger.kernel.org
> Cc: Douglas Gilbert; Elliott, Robert (Server Storage)
> Subject: [PATCH 2/6] scsi: split scsi_nonblockable_ioctl
> 
> The calling conventions for this function where bad as it could
> return
> -ENODEV both for a device not currently online and a not recognized
> ioctl.
> 
> Add a new scsi_ioctl_block_when_processing_errors function that wraps
> scsi_block_when_processing_errors with the a special case for the
> SG_SCSI_RESET ioctl command, and handle the SG_SCSI_RESET case itself
> in scsi_ioctl.  All callers of scsi_ioctl now must call the above
> helper to check for the EH state, so that the ioctl handler itself doesn't
> have to.
> 
> Reported-by: Robert Elliott <Elliott@hp.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/ch.c         |  5 +++++
>  drivers/scsi/osst.c       |  6 +++---
>  drivers/scsi/scsi_ioctl.c | 47 +++++++++++++------------------------
> ----------
>  drivers/scsi/sd.c         |  6 +++---
>  drivers/scsi/sg.c         | 33 +++++++++++++++------------------
>  drivers/scsi/sr.c         | 15 +++++----------
>  drivers/scsi/st.c         |  7 +++----
>  include/scsi/scsi_ioctl.h |  4 ++--
>  8 files changed, 49 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
> index 5ddc08f..712f159 100644
> --- a/drivers/scsi/scsi_ioctl.c
> +++ b/drivers/scsi/scsi_ioctl.c
...

> @@ -281,30 +270,20 @@ int scsi_ioctl(struct scsi_device *sdev, int
> cmd, void __user *arg)
>  }
>  EXPORT_SYMBOL(scsi_ioctl);
> 
> -/**
> - * scsi_nonblockable_ioctl() - Handle SG_SCSI_RESET
> - * @sdev: scsi device receiving ioctl
> - * @cmd: Must be SC_SCSI_RESET
> - * @arg: pointer to int containing
> SG_SCSI_RESET_{DEVICE,TARGET,BUS,HOST}
> - *       possibly OR-ed with SG_SCSI_RESET_NO_ESCALATE
> - * @ndelay: file mode O_NDELAY flag
> +/*
> + * We can process a reset even when a device isn't fully operable.
>   */
> -int scsi_nonblockable_ioctl(struct scsi_device *sdev, int cmd,
> -			    void __user *arg, int ndelay)
> +int scsi_ioctl_block_when_processing_errors(struct scsi_device
> *sdev, int cmd,
> +		bool ndelay)
>  {
> -	/* The first set of iocts may be executed even if we're doing
> -	 * error processing, as long as the device was opened
> -	 * non-blocking */
> -	if (ndelay) {
> +	if (cmd == SG_SCSI_RESET && ndelay) {
>  		if (scsi_host_in_recovery(sdev->host))
>  			return -ENODEV;
> -	} else if (!scsi_block_when_processing_errors(sdev))
> -		return -ENODEV;
> -
> -	switch (cmd) {
> -	case SG_SCSI_RESET:
> -		return scsi_ioctl_reset(sdev, arg);
> +	} else {
> +		if (!scsi_block_when_processing_errors(sdev))
> +			return -ENODEV;
>  	}
> -	return -ENODEV;
> +
> +	return 0;
>  }
> -EXPORT_SYMBOL(scsi_nonblockable_ioctl);
> +EXPORT_SYMBOL_GPL(scsi_ioctl_block_when_processing_errors);

Most of the SCSI midlayer functions are exported as non-GPL; 
is it really necessary to prevent closed source drivers from 
using this new function, especially since it's just a
refactoring of some previously non-GPL exported functions?

...
> diff --git a/include/scsi/scsi_ioctl.h b/include/scsi/scsi_ioctl.h
> index b900684..8d19d1d 100644
> --- a/include/scsi/scsi_ioctl.h
> +++ b/include/scsi/scsi_ioctl.h
> @@ -40,9 +40,9 @@ typedef struct scsi_fctargaddress {
>  	unsigned char host_wwn[8]; // include NULL term.
>  } Scsi_FCTargAddress;
> 
> +int scsi_ioctl_block_when_processing_errors(struct scsi_device
> *sdev,
> +		int cmd, bool ndelay);
>  extern int scsi_ioctl(struct scsi_device *, int, void __user *);
> -extern int scsi_nonblockable_ioctl(struct scsi_device *sdev, int
> cmd,
> -				   void __user *arg, int ndelay);
> 
>  #endif /* __KERNEL__ */
>  #endif /* _SCSI_IOCTL_H */
> --
> 1.9.1

Beyond this patch, the existing scsi_block_when_processing_errors
description should be expanded to mention that it is used to
prevent ioctls and commands, not just commands:

scsi_error.c:
/**
 * scsi_block_when_processing_errors - Prevent cmds from being queued.
 * @sdev:       Device on which we are performing recovery.
 *
 * Description:
 *     We block until the host is out of error recovery, and then check to
 *     see whether the host or the device is offline.
 *
 * Return value:
 *     0 when dev was taken offline by error recovery. 1 OK to proceed.
 */
int scsi_block_when_processing_errors(struct scsi_device *sdev)
...


Reviewed-by: Robert Elliott <elliott@hp.com>


---
Rob Elliott    HP Server Storage

[PATCH 3/6] sd: fix up ->compat_ioctl

[Christoph Hellwig]

No need to verify the passthrough ioctls, the real handler will
take care of that.  Also make sure not to block for resets on
O_NONBLOCK fds.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 61e50ae..baf5cf4 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1541,31 +1541,19 @@ static int sd_compat_ioctl(struct block_device *bdev, fmode_t mode,
 			   unsigned int cmd, unsigned long arg)
 {
 	struct scsi_device *sdev = scsi_disk(bdev->bd_disk)->device;
-	int ret;
-
-	ret = scsi_verify_blk_ioctl(bdev, cmd);
-	if (ret < 0)
-		return ret;
+	int error;
 
-	/*
-	 * If we are in the middle of error recovery, don't let anyone
-	 * else try and use this device.  Also, if error recovery fails, it
-	 * may try and take the device offline, in which case all further
-	 * access to the device is prohibited.
-	 */
-	if (!scsi_block_when_processing_errors(sdev))
-		return -ENODEV;
+	error = scsi_ioctl_block_when_processing_errors(sdev, cmd,
+			mode & FMODE_NDELAY);
+	if (error)
+		return error;
 	       
-	if (sdev->host->hostt->compat_ioctl) {
-		ret = sdev->host->hostt->compat_ioctl(sdev, cmd, (void __user *)arg);
-
-		return ret;
-	}
-
 	/* 
 	 * Let the static ioctl translation table take care of it.
 	 */
-	return -ENOIOCTLCMD; 
+	if (!sdev->host->hostt->compat_ioctl)
+		return -ENOIOCTLCMD; 
+	return sdev->host->hostt->compat_ioctl(sdev, cmd, (void __user *)arg);
 }
 #endif
 
-- 
1.9.1

Re: [PATCH 3/6] sd: fix up ->compat_ioctl

[Hannes Reinecke]

On 10/30/2014 10:27 AM, Christoph Hellwig wrote:
> No need to verify the passthrough ioctls, the real handler will
> take care of that.  Also make sure not to block for resets on
> O_NONBLOCK fds.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] sd: fix up ->compat_ioctl

[Hannes Reinecke]

On 10/30/2014 10:27 AM, Christoph Hellwig wrote:
> No need to verify the passthrough ioctls, the real handler will
> take care of that.  Also make sure not to block for resets on
> O_NONBLOCK fds.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] sd: fix up ->compat_ioctl

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> No need to verify the passthrough ioctls, the real handler
Christoph> will take care of that.  Also make sure not to block for
Christoph> resets on O_NONBLOCK fds.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH 3/6] sd: fix up ->compat_ioctl

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@lst.de]
> Sent: Thursday, 30 October, 2014 4:27 AM
> To: linux-scsi@vger.kernel.org
> Cc: Douglas Gilbert; Elliott, Robert (Server Storage)
> Subject: [PATCH 3/6] sd: fix up ->compat_ioctl
> 
> No need to verify the passthrough ioctls, the real handler will
> take care of that.  Also make sure not to block for resets on
> O_NONBLOCK fds.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/sd.c | 28 ++++++++--------------------
>  1 file changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 61e50ae..baf5cf4 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1541,31 +1541,19 @@ static int sd_compat_ioctl(struct
> block_device *bdev, fmode_t mode,
>  			   unsigned int cmd, unsigned long arg)
...
>  	/*
>  	 * Let the static ioctl translation table take care of it.
>  	 */
> -	return -ENOIOCTLCMD;
> +	if (!sdev->host->hostt->compat_ioctl)
> +		return -ENOIOCTLCMD;

Although I don't see it in the quoted email, git complains that 
this line (and the original being deleted) have a space 
after -ENOIOCTLCMD;

> +	return sdev->host->hostt->compat_ioctl(sdev, cmd, (void __user
> *)arg);
>  }
>  #endif
> 
> --
> 1.9.1

Reviewed-by: Robert Elliott <elliott@hp.com>

[PATCH 4/6] st: call scsi_set_medium_removal directly

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/st.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 7d2e036..e46e02b2 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -861,17 +861,16 @@ static int set_mode_densblk(struct scsi_tape * STp, struct st_modedef * STm)
 /* Lock or unlock the drive door. Don't use when st_request allocated. */
 static int do_door_lock(struct scsi_tape * STp, int do_lock)
 {
-	int retval, cmd;
+	int retval;
 
-	cmd = do_lock ? SCSI_IOCTL_DOORLOCK : SCSI_IOCTL_DOORUNLOCK;
 	DEBC_printk(STp, "%socking drive door.\n", do_lock ? "L" : "Unl");
-	retval = scsi_ioctl(STp->device, cmd, NULL);
-	if (!retval) {
+
+	retval = scsi_set_medium_removal(STp->device,
+			do_lock ? SCSI_REMOVAL_PREVENT : SCSI_REMOVAL_ALLOW);
+	if (!retval)
 		STp->door_locked = do_lock ? ST_LOCKED_EXPLICIT : ST_UNLOCKED;
-	}
-	else {
+	else
 		STp->door_locked = ST_LOCK_FAILS;
-	}
 	return retval;
 }
 
-- 
1.9.1

Re: [PATCH 4/6] st: call scsi_set_medium_removal directly

[Hannes Reinecke]

On 10/30/2014 10:27 AM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/st.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index 7d2e036..e46e02b2 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -861,17 +861,16 @@ static int set_mode_densblk(struct scsi_tape * STp, struct st_modedef * STm)
>  /* Lock or unlock the drive door. Don't use when st_request allocated. */
>  static int do_door_lock(struct scsi_tape * STp, int do_lock)
>  {
> -	int retval, cmd;
> +	int retval;
>  
> -	cmd = do_lock ? SCSI_IOCTL_DOORLOCK : SCSI_IOCTL_DOORUNLOCK;
>  	DEBC_printk(STp, "%socking drive door.\n", do_lock ? "L" : "Unl");
> -	retval = scsi_ioctl(STp->device, cmd, NULL);
> -	if (!retval) {
> +
> +	retval = scsi_set_medium_removal(STp->device,
> +			do_lock ? SCSI_REMOVAL_PREVENT : SCSI_REMOVAL_ALLOW);
> +	if (!retval)
>  		STp->door_locked = do_lock ? ST_LOCKED_EXPLICIT : ST_UNLOCKED;
> -	}
> -	else {
> +	else
>  		STp->door_locked = ST_LOCK_FAILS;
> -	}
>  	return retval;
>  }
>  
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] st: call scsi_set_medium_removal directly

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH 4/6] st: call scsi_set_medium_removal directly

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@lst.de]
> Sent: Thursday, 30 October, 2014 4:27 AM
> To: linux-scsi@vger.kernel.org
> Cc: Douglas Gilbert; Elliott, Robert (Server Storage)
> Subject: [PATCH 4/6] st: call scsi_set_medium_removal directly
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/st.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index 7d2e036..e46e02b2 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -861,17 +861,16 @@ static int set_mode_densblk(struct scsi_tape *
> STp, struct st_modedef * STm)
>  /* Lock or unlock the drive door. Don't use when st_request
> allocated. */
>  static int do_door_lock(struct scsi_tape * STp, int do_lock)
>  {
> -	int retval, cmd;
> +	int retval;
> 
> -	cmd = do_lock ? SCSI_IOCTL_DOORLOCK : SCSI_IOCTL_DOORUNLOCK;
>  	DEBC_printk(STp, "%socking drive door.\n", do_lock ? "L" :
> "Unl");
> -	retval = scsi_ioctl(STp->device, cmd, NULL);
> -	if (!retval) {
> +
> +	retval = scsi_set_medium_removal(STp->device,
> +			do_lock ? SCSI_REMOVAL_PREVENT :
> SCSI_REMOVAL_ALLOW);
> +	if (!retval)
>  		STp->door_locked = do_lock ? ST_LOCKED_EXPLICIT :
> ST_UNLOCKED;
> -	}
> -	else {
> +	else
>  		STp->door_locked = ST_LOCK_FAILS;
> -	}
>  	return retval;
>  }

Reviewed-by: Robert Elliott <elliott@hp.com>

A few comments spawned by this (and patch 5/6):

1. Although PREVENT ALLOW MEDIUM REMOVAL was a generic
command defined in SPC-2, In 2006 T10 proposal
06-248r1 changed it to be a command-set specific 
command for SPC-3.  MMC, SSC, SBC, and SMC had slight
differences and the groups wouldn't agree on a converged
definition.

For example:
* SSC and SBC only define two of the bit combinations 
  in byte 4 bits 1:0 - allowed and prevented
* MMC defines all four combinations of byte 4 bits 1:0 -
  unlocked, locked, persistent allow, and persistent prevent
* SMC follows SSC/SBC but adds a PREEMPT bit in 
  byte 4 bit 7

So, relying on one SCSI-wide scsi_set_medium_removal
call could theoretically be a problem (though not yet,
since none of those extra features are used).


2. Reviewing the callers for scsi_set_medium_removal,
I noticed sd.c sd_release ignores the return value 
from scsi_set_medium_removal (like many others).

Its comment says:
*      Returns 0.
but it is a void function so doesn't really return anything:
static void sd_release(struct gendisk *disk, fmode_t mode)


3. Reviewing the callers, st_release has an initialized
result variable but does nothing else with it but return it:
	int result = 0;
	...
	return result;

It ignores the do_door_lock -> scsi_set_medium_removal
result (like many others).

---
Rob Elliott    HP Server Storage

Re: [PATCH 4/6] st: call scsi_set_medium_removal directly

[Christoph Hellwig]

On Thu, Nov 06, 2014 at 11:54:41PM +0000, Elliott, Robert (Server Storage) wrote:
> A few comments spawned by this (and patch 5/6):
> 
> 1. Although PREVENT ALLOW MEDIUM REMOVAL was a generic
> command defined in SPC-2, In 2006 T10 proposal
> 06-248r1 changed it to be a command-set specific 
> command for SPC-3.  MMC, SSC, SBC, and SMC had slight
> differences and the groups wouldn't agree on a converged
> definition.
> 
> For example:
> * SSC and SBC only define two of the bit combinations 
>   in byte 4 bits 1:0 - allowed and prevented
> * MMC defines all four combinations of byte 4 bits 1:0 -
>   unlocked, locked, persistent allow, and persistent prevent
> * SMC follows SSC/SBC but adds a PREEMPT bit in 
>   byte 4 bit 7
> 
> So, relying on one SCSI-wide scsi_set_medium_removal
> call could theoretically be a problem (though not yet,
> since none of those extra features are used).

The scsi_set_medium_removal is fairly trivial.  For the unlikely case
that we are going to make use of these additional features we'll
need to split it.

> 2. Reviewing the callers for scsi_set_medium_removal,
> I noticed sd.c sd_release ignores the return value 
> from scsi_set_medium_removal (like many others).
> 
> Its comment says:
> *      Returns 0.
> but it is a void function so doesn't really return anything:
> static void sd_release(struct gendisk *disk, fmode_t mode)

Right.  That's actually a fairly recent change from Al, before
that the ->release method for block devices had a return value,
which we then always iŋnored higher up.

> 3. Reviewing the callers, st_release has an initialized
> result variable but does nothing else with it but return it:
> 	int result = 0;
> 	...
> 	return result;
> 
> It ignores the do_door_lock -> scsi_set_medium_removal
> result (like many others).

The st driver defintively could use an audit for error code propagation.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] st: call scsi_set_medium_removal directly

["Kai Mäkisara (Kolumbus)"]

> On 7.11.2014, at 1.54, Elliott, Robert (Server Storage) <Elliott@hp.com> wrote:
> 
...
> 3. Reviewing the callers, st_release has an initialized
> result variable but does nothing else with it but return it:
> 	int result = 0;
> 	...
> 	return result;
> 
> It ignores the do_door_lock -> scsi_set_medium_removal
> result (like many others).
> 
The return code for automatic door locking/unlocking has been ignored on purpose. It is returned to caller for explicit door locking/unlocking. One can, of course, revise this decision.

The return value of release() is ignored by VFS. The proper fix would probably be to define release() void.

Thanks,
Kai

[PATCH 5/6] osst: call scsi_set_medium_removal directly

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/osst.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c
index b6d63d6..8c38464 100644
--- a/drivers/scsi/osst.c
+++ b/drivers/scsi/osst.c
@@ -3327,19 +3327,18 @@ static int osst_write_frame(struct osst_tape * STp, struct osst_request ** aSRpn
 /* Lock or unlock the drive door. Don't use when struct osst_request allocated. */
 static int do_door_lock(struct osst_tape * STp, int do_lock)
 {
-	int retval, cmd;
+	int retval;
 
-	cmd = do_lock ? SCSI_IOCTL_DOORLOCK : SCSI_IOCTL_DOORUNLOCK;
 #if DEBUG
 	printk(OSST_DEB_MSG "%s:D: %socking drive door.\n", tape_name(STp), do_lock ? "L" : "Unl");
 #endif
-	retval = scsi_ioctl(STp->device, cmd, NULL);
-	if (!retval) {
+
+	retval = scsi_set_medium_removal(STp->device,
+			do_lock ? SCSI_REMOVAL_PREVENT : SCSI_REMOVAL_ALLOW);
+	if (!retval)
 		STp->door_locked = do_lock ? ST_LOCKED_EXPLICIT : ST_UNLOCKED;
-	}
-	else {
+	else
 		STp->door_locked = ST_LOCK_FAILS;
-	}
 	return retval;
 }
 
-- 
1.9.1

Re: [PATCH 5/6] osst: call scsi_set_medium_removal directly

[Hannes Reinecke]

On 10/30/2014 10:27 AM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/osst.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c
> index b6d63d6..8c38464 100644
> --- a/drivers/scsi/osst.c
> +++ b/drivers/scsi/osst.c
> @@ -3327,19 +3327,18 @@ static int osst_write_frame(struct osst_tape * STp, struct osst_request ** aSRpn
>  /* Lock or unlock the drive door. Don't use when struct osst_request allocated. */
>  static int do_door_lock(struct osst_tape * STp, int do_lock)
>  {
> -	int retval, cmd;
> +	int retval;
>  
> -	cmd = do_lock ? SCSI_IOCTL_DOORLOCK : SCSI_IOCTL_DOORUNLOCK;
>  #if DEBUG
>  	printk(OSST_DEB_MSG "%s:D: %socking drive door.\n", tape_name(STp), do_lock ? "L" : "Unl");
>  #endif
> -	retval = scsi_ioctl(STp->device, cmd, NULL);
> -	if (!retval) {
> +
> +	retval = scsi_set_medium_removal(STp->device,
> +			do_lock ? SCSI_REMOVAL_PREVENT : SCSI_REMOVAL_ALLOW);
> +	if (!retval)
>  		STp->door_locked = do_lock ? ST_LOCKED_EXPLICIT : ST_UNLOCKED;
> -	}
> -	else {
> +	else
>  		STp->door_locked = ST_LOCK_FAILS;
> -	}
>  	return retval;
>  }
>  
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] osst: call scsi_set_medium_removal directly

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH 5/6] osst: call scsi_set_medium_removal directly

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@lst.de]
> Sent: Thursday, 30 October, 2014 4:27 AM
> To: linux-scsi@vger.kernel.org
> Cc: Douglas Gilbert; Elliott, Robert (Server Storage)
> Subject: [PATCH 5/6] osst: call scsi_set_medium_removal directly
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/osst.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c
> index b6d63d6..8c38464 100644
> --- a/drivers/scsi/osst.c
> +++ b/drivers/scsi/osst.c
> @@ -3327,19 +3327,18 @@ static int osst_write_frame(struct osst_tape
> * STp, struct osst_request ** aSRpn
>  /* Lock or unlock the drive door. Don't use when struct osst_request
> allocated. */
>  static int do_door_lock(struct osst_tape * STp, int do_lock)
>  {
> -	int retval, cmd;
> +	int retval;
> 
> -	cmd = do_lock ? SCSI_IOCTL_DOORLOCK : SCSI_IOCTL_DOORUNLOCK;
>  #if DEBUG
>  	printk(OSST_DEB_MSG "%s:D: %socking drive door.\n",
> tape_name(STp), do_lock ? "L" : "Unl");
>  #endif
> -	retval = scsi_ioctl(STp->device, cmd, NULL);
> -	if (!retval) {
> +
> +	retval = scsi_set_medium_removal(STp->device,
> +			do_lock ? SCSI_REMOVAL_PREVENT :
> SCSI_REMOVAL_ALLOW);
> +	if (!retval)
>  		STp->door_locked = do_lock ? ST_LOCKED_EXPLICIT :
> ST_UNLOCKED;
> -	}
> -	else {
> +	else
>  		STp->door_locked = ST_LOCK_FAILS;
> -	}
>  	return retval;
>  }
> 

Reviewed-by: Robert Elliott <elliott@hp.com>

One additional nit (not necessary):
The do_lock argument could be bool rather than int both 
here and in st.c.


---
Rob Elliott    HP Server Storage

[PATCH 6/6] scsi: return EAGAIN when resetting a device under EH

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index 712f159..c4f7b56 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -278,7 +278,7 @@ int scsi_ioctl_block_when_processing_errors(struct scsi_device *sdev, int cmd,
 {
 	if (cmd == SG_SCSI_RESET && ndelay) {
 		if (scsi_host_in_recovery(sdev->host))
-			return -ENODEV;
+			return -EAGAIN;
 	} else {
 		if (!scsi_block_when_processing_errors(sdev))
 			return -ENODEV;
-- 
1.9.1

Re: [PATCH 6/6] scsi: return EAGAIN when resetting a device under EH

[Hannes Reinecke]

On 10/30/2014 10:27 AM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/scsi_ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
> index 712f159..c4f7b56 100644
> --- a/drivers/scsi/scsi_ioctl.c
> +++ b/drivers/scsi/scsi_ioctl.c
> @@ -278,7 +278,7 @@ int scsi_ioctl_block_when_processing_errors(struct scsi_device *sdev, int cmd,
>  {
>  	if (cmd == SG_SCSI_RESET && ndelay) {
>  		if (scsi_host_in_recovery(sdev->host))
> -			return -ENODEV;
> +			return -EAGAIN;
>  	} else {
>  		if (!scsi_block_when_processing_errors(sdev))
>  			return -ENODEV;
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] scsi: return EAGAIN when resetting a device under EH

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> - return -ENODEV;
Christoph> + return -EAGAIN;

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH 6/6] scsi: return EAGAIN when resetting a device under EH

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@lst.de]
> Sent: Thursday, 30 October, 2014 4:27 AM
> To: linux-scsi@vger.kernel.org
> Cc: Douglas Gilbert; Elliott, Robert (Server Storage)
> Subject: [PATCH 6/6] scsi: return EAGAIN when resetting a device
> under EH
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/scsi_ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
> index 712f159..c4f7b56 100644
> --- a/drivers/scsi/scsi_ioctl.c
> +++ b/drivers/scsi/scsi_ioctl.c
> @@ -278,7 +278,7 @@ int
> scsi_ioctl_block_when_processing_errors(struct scsi_device *sdev, int
> cmd,
>  {
>  	if (cmd == SG_SCSI_RESET && ndelay) {
>  		if (scsi_host_in_recovery(sdev->host))
> -			return -ENODEV;
> +			return -EAGAIN;
>  	} else {
>  		if (!scsi_block_when_processing_errors(sdev))
>  			return -ENODEV;
> --
> 1.9.1

This is how sg_reset responds to -EAGAIN, which seems reasonable:
	sg_reset: starting device reset
	sg_reset: SG_SCSI_RESET failed: Resource temporarily unavailable

That is easy to trigger when running sg_reset in parallel to
multiple devices presented by the same controller.  In one
example to 16 devices, 10 got that result while 6 succeeded.

Tested-by: Robert Elliott <elliott@hp.com>
Reviewed-by: Robert Elliott <elliott@hp.com>

Re: [PATCH 6/6] scsi: return EAGAIN when resetting a device under EH

[Douglas Gilbert]

On 14-11-06 04:46 PM, Elliott, Robert (Server Storage) wrote:
>
>
>> -----Original Message-----
>> From: Christoph Hellwig [mailto:hch@lst.de]
>> Sent: Thursday, 30 October, 2014 4:27 AM
>> To: linux-scsi@vger.kernel.org
>> Cc: Douglas Gilbert; Elliott, Robert (Server Storage)
>> Subject: [PATCH 6/6] scsi: return EAGAIN when resetting a device
>> under EH
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>   drivers/scsi/scsi_ioctl.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
>> index 712f159..c4f7b56 100644
>> --- a/drivers/scsi/scsi_ioctl.c
>> +++ b/drivers/scsi/scsi_ioctl.c
>> @@ -278,7 +278,7 @@ int
>> scsi_ioctl_block_when_processing_errors(struct scsi_device *sdev, int
>> cmd,
>>   {
>>   	if (cmd == SG_SCSI_RESET && ndelay) {
>>   		if (scsi_host_in_recovery(sdev->host))
>> -			return -ENODEV;
>> +			return -EAGAIN;
>>   	} else {
>>   		if (!scsi_block_when_processing_errors(sdev))
>>   			return -ENODEV;
>> --
>> 1.9.1
>
> This is how sg_reset responds to -EAGAIN, which seems reasonable:
> 	sg_reset: starting device reset
> 	sg_reset: SG_SCSI_RESET failed: Resource temporarily unavailable

That message has been tweaked so in sg3_utils version 1.40
that will read:
    sg_reset: try again later, may be resetting now

> That is easy to trigger when running sg_reset in parallel to
> multiple devices presented by the same controller.  In one
> example to 16 devices, 10 got that result while 6 succeeded.
>
> Tested-by: Robert Elliott <elliott@hp.com>
> Reviewed-by: Robert Elliott <elliott@hp.com>

misc scsi ioctl updates

[Christoph Hellwig]

This series (against the core-for-3.19 branch) deals with various
issues realated to scsi-ioctls, mostly with the SG_SCSI_RESET ioctl.

[PATCH 6/6] scsi: return EAGAIN when resetting a device under EH

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index d0cb33d..53e8fea 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -278,7 +278,7 @@ int scsi_ioctl_block_when_processing_errors(struct scsi_device *sdev, int cmd,
 {
 	if (cmd == SG_SCSI_RESET && ndelay) {
 		if (scsi_host_in_recovery(sdev->host))
-			return -ENODEV;
+			return -EAGAIN;
 	} else {
 		if (!scsi_block_when_processing_errors(sdev))
 			return -ENODEV;
-- 
1.9.1