misc scsi ioctl updates

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 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, 40 insertions(+), 118 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 263d907..c4df3cd 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"
@@ -2294,39 +2295,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;
 	}
@@ -2347,37 +2345,33 @@ 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:
+	error = 0;
+	switch (val & ~SG_SCSI_RESET_NO_ESCALATE) {
+	case SG_SCSI_RESET_NOTHING:
+		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:
-		rtn = FAILED;
+		/* FALLTHROUGH */
+		error = -EIO;
+		break;
 	}
 
 	spin_lock_irqsave(shost->host_lock, flags);
@@ -2399,9 +2393,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 12fe676..6dc61b0 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 fe44c14..bf8b14b 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 49af14a..ff4561f 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

[Bart Van Assche]

On 10/27/14 18:59, Christoph Hellwig wrote:
> -/*
> - * 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;
>   	}
> @@ -2347,37 +2345,33 @@ 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:
> +	error = 0;
> +	switch (val & ~SG_SCSI_RESET_NO_ESCALATE) {
> +	case SG_SCSI_RESET_NOTHING:
> +		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:
> -		rtn = FAILED;
> +		/* FALLTHROUGH */
> +		error = -EIO;
> +		break;
>   	}
>
>   	spin_lock_irqsave(shost->host_lock, flags);
> @@ -2399,9 +2393,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);

This function returns 0 even if an action like SG_SCSI_RESET_BUS fails 
(rtn != SUCCESS) with the flag SCSI_TRY_RESET_NO_ESCALATE set. I think 
the current behavior is to return -EIO in that case. If this change was 
intended, please mention it in the patch description.

Bart.

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

[Christoph Hellwig]

On Tue, Oct 28, 2014 at 09:51:12AM +0100, Bart Van Assche wrote:
> This function returns 0 even if an action like SG_SCSI_RESET_BUS fails (rtn 
> != SUCCESS) with the flag SCSI_TRY_RESET_NO_ESCALATE set. I think the 
> current behavior is to return -EIO in that case. If this change was 
> intended, please mention it in the patch description.

I will fix this up for the next version, thanks for catching it!

[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 ef5ae0d..fcd9327 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 dff37a25..8c28b49 100644
--- a/drivers/scsi/osst.c
+++ b/drivers/scsi/osst.c
@@ -4967,10 +4967,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 6dc61b0..d0cb33d 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 cfba74c..070b4b7 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 bf8b14b..2da32ac 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 8d5f8b4..7270e3f 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -3375,11 +3375,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

[Bart Van Assche]

On 10/27/14 18:59, 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.

Hello Christoph,

I might be missing some background information here, but it's not clear 
to me why the function like scsi_block_when_processing_errors() was 
introduced some time ago. What if immediately after error handling has 
finished a new request is queued that kicks the error handler again 
before the caller of scsi_block_when_processing_errors() has finished 
the actions that should not occur concurrently with error handling ? Has 
it already been considered to introduce a mutex to serialize error 
handling and activity that should not occur concurrently with error 
handling ?

Thanks,

Bart.

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

[Christoph Hellwig]

On Tue, Oct 28, 2014 at 10:05:32AM +0100, Bart Van Assche wrote:
> I might be missing some background information here, but it's not clear to 
> me why the function like scsi_block_when_processing_errors() was introduced 
> some time ago. What if immediately after error handling has finished a new 
> request is queued that kicks the error handler again before the caller of 
> scsi_block_when_processing_errors() has finished the actions that should 
> not occur concurrently with error handling ? Has it already been considered 
> to introduce a mutex to serialize error handling and activity that should 
> not occur concurrently with error handling ?

I've not looked into changing the conditions, but your questions are
good ones.  I'd like to go even further and ask the question of what
we try to protect here.  For SG_SCSI_RESET the answer is that we want
to enforce a single outstanding TMF, as require by old parallel SCSI HBAs,
and possible by various drivers.  I don't quite understand why we check
the EH state before executing other ioctl, though.  Even more confusing
is that some driver like st.c and sg.c only enforce this on some ioctls.

Changing the exclusion rules isn't in the scope of this series, but if
you interested in looking into this series I'd be happy if we get the
ball on it rolling.

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

[Bart Van Assche]

On 10/28/14 18:39, Christoph Hellwig wrote:
> On Tue, Oct 28, 2014 at 10:05:32AM +0100, Bart Van Assche wrote:
>> I might be missing some background information here, but it's not clear to
>> me why the function like scsi_block_when_processing_errors() was introduced
>> some time ago. What if immediately after error handling has finished a new
>> request is queued that kicks the error handler again before the caller of
>> scsi_block_when_processing_errors() has finished the actions that should
>> not occur concurrently with error handling ? Has it already been considered
>> to introduce a mutex to serialize error handling and activity that should
>> not occur concurrently with error handling ?
>
> I've not looked into changing the conditions, but your questions are
> good ones.  I'd like to go even further and ask the question of what
> we try to protect here.  For SG_SCSI_RESET the answer is that we want
> to enforce a single outstanding TMF, as require by old parallel SCSI HBAs,
> and possible by various drivers.  I don't quite understand why we check
> the EH state before executing other ioctl, though.  Even more confusing
> is that some driver like st.c and sg.c only enforce this on some ioctls.
>
> Changing the exclusion rules isn't in the scope of this series, but if
> you interested in looking into this series I'd be happy if we get the
> ball on it rolling.

Hello Christoph,

As you know I'm working on several different projects simultaneously so 
I'm not sure I will be able to free up some time soon to work on this. I 
will add this on my to-do list but if anyone else would like to start 
working on this that would be welcome.

Bart.

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

[Christoph Hellwig]

On Wed, Oct 29, 2014 at 11:55:38AM +0100, Bart Van Assche wrote:
> As you know I'm working on several different projects simultaneously so I'm 
> not sure I will be able to free up some time soon to work on this. I will 
> add this on my to-do list but if anyone else would like to start working on 
> this that would be welcome.

I don't think this is an urgent one - so far no one ran into actual issue
due to it.  I'd still love to see it addressed eventually by someone.

[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 070b4b7..467563e 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

[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 7270e3f..f501e5a 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -860,17 +860,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

[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 8c28b49..9214e5d 100644
--- a/drivers/scsi/osst.c
+++ b/drivers/scsi/osst.c
@@ -3325,19 +3325,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

[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

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