Re: [PATCH 1/2] st: separate st-unique ioctl handling from scsi common ioctl handling

[John Meneghini <jmeneghi@redhat.com>]

This diff is difficult to read.  However, in reviewing this code manually by reading the results of this patch and comparing it with
the previous file I believe this change is correct and functionally equivalent to the previous code.

I've also tested this patch with my new and improved tape_tests and reproduced the failing scsi ioctl(idlun) problem with this patch.
So this confirms there is no functional change with this patch. It isn't pretty but it should provide a safe, bisect-able change that
prepares for your next patch.

Reviewed-by: John Meneghini <jmeneghi@redhat.com>
Tested-by: John Meneghini <jmeneghi@redhat.com>

/John

https://github.com/johnmeneghini/tape_tests

[root@estor-t560 ~]# grep -E "TEST.FAILED|TEST.WARN|sg_reset|Power.on/reset.recognized|Unit.Attention" /root/tape_reset_tests.log
--- sg_reset --target /dev/sg2
--- sg_map -st -x -i TEST WARN : device /dev/nst0 failed on scsi ioctl(idlun), skip: Input/output error
[  247.493571] st 1:0:0:0: [st0] Power on/reset recognized.
--- sg_map -st -x -i TEST WARN : device /dev/nst0 failed on scsi ioctl(idlun), skip: Input/output error
--- stinit -f /home/jmeneghi/tape_tests/stinit.conf -v /dev/nst0 TEST WARN : The SCSI INQUIRY for device '/dev/nst0' failed (power off?): Input/output error
--- sg_reset --target /dev/sg2
[  265.727311] st 1:0:0:0: [st0] Power on/reset recognized.
[  265.727357] st 1:0:0:0: [st0] Sense Key : Unit Attention [current]
--- sg_map -st -x -i TEST WARN : device /dev/nst0 failed on scsi ioctl(idlun), skip: Input/output error
--- stinit -f /home/jmeneghi/tape_tests/stinit.conf -v /dev/nst0 TEST WARN : The SCSI INQUIRY for device '/dev/nst0' failed (power off?): Input/output error
--- sg_reset --target /dev/sg2
[  285.860196] st 1:0:0:0: [st0] Power on/reset recognized.
[  285.860241] st 1:0:0:0: [st0] Sense Key : Unit Attention [current]
--- sg_reset --target /dev/sg2
[  310.125953] st 1:0:0:0: [st0] Power on/reset recognized.
--- sg_reset --target /dev/sg2
[  351.444178] st 1:0:0:0: [st0] Power on/reset recognized.
--- sg_reset --target /dev/sg2
[  992.210217] st 1:0:0:0: [st0] Power on/reset recognized.
--- sg_map -st -x -i TEST WARN : device /dev/nst0 failed on scsi ioctl(idlun), skip: Input/output error
--- stinit -f /home/jmeneghi/tape_tests/stinit.conf -v /dev/nst0 TEST WARN : The SCSI INQUIRY for device '/dev/nst0' failed (power off?): Input/output error
--- sg_reset --target /dev/sg2
[ 1002.023106] st 1:0:0:0: [st0] Power on/reset recognized.
[root@estor-t560 ~]#






On 11/4/25 10:46 AM, David Jeffery wrote:
> The st ioctl function currently interleaves code for handling various st
> specific ioctls with parts of code needed for handling ioctls common to
> all scsi devices. Separate out st's code for the common ioctls into a more
> manageable, separate function.
> 
> Signed-off-by: David Jeffery <djeffery@redhat.com>
> Tested-by:     Laurence Oberman <loberman@redhat.com>
> ---
> 
>   drivers/scsi/st.c | 85 ++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 62 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index 74a6830b7ed8..87f0e303fdd6 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -3526,8 +3526,60 @@ static int partition_tape(struct scsi_tape *STp, int size)
>   out:
>   	return result;
>   }
> -\f
>   
> +/*
> + * Handles any extra state needed for ioctls which are not st-specific.
> + * Called with the scsi_tape lock held, released before return
> + */
> +static long st_common_ioctl(struct scsi_tape *STp, struct st_modedef *STm,
> +			    struct file *file, unsigned int cmd_in,
> +			    unsigned long arg)
> +{
> +	int i, retval = 0;
> +
> +	if (!STm->defined) {
> +		retval = -ENXIO;
> +		goto out;
> +	}
> +
> +	if ((i = flush_buffer(STp, 0)) < 0) {
> +		retval = i;
> +		goto out;
> +	} else { /* flush_buffer succeeds */
> +		if (STp->can_partitions) {
> +			i = switch_partition(STp);
> +			if (i < 0) {
> +				retval = i;
> +				goto out;
> +			}
> +		}
> +	}
> +	mutex_unlock(&STp->lock);
> +
> +	switch (cmd_in) {
> +	case SG_IO:
> +	case SCSI_IOCTL_SEND_COMMAND:
> +	case CDROM_SEND_PACKET:
> +		if (!capable(CAP_SYS_RAWIO))
> +			return -EPERM;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	retval = scsi_ioctl(STp->device, file->f_mode & FMODE_WRITE,
> +			    cmd_in, (void __user *)arg);
> +	if (!retval && cmd_in == SCSI_IOCTL_STOP_UNIT) {
> +		/* unload */
> +		STp->rew_at_close = 0;
> +		STp->ready = ST_NO_TAPE;
> +	}
> +
> +	return retval;
> +out:
> +	mutex_unlock(&STp->lock);
> +	return retval;
> +}
>   
>   /* The ioctl command */
>   static long st_ioctl(struct file *file, unsigned int cmd_in, unsigned long arg)
> @@ -3565,6 +3617,15 @@ static long st_ioctl(struct file *file, unsigned int cmd_in, unsigned long arg)
>   	if (retval)
>   		goto out;
>   
> +	switch(cmd_in) {
> +	case MTIOCPOS:
> +	case MTIOCGET:
> +	case MTIOCTOP:
> +		break;
> +	default:
> +		return st_common_ioctl(STp, STm, file, cmd_in, arg);
> +	}
> +
>   	cmd_type = _IOC_TYPE(cmd_in);
>   	cmd_nr = _IOC_NR(cmd_in);
>   
> @@ -3876,29 +3937,7 @@ static long st_ioctl(struct file *file, unsigned int cmd_in, unsigned long arg)
>   		}
>   		mt_pos.mt_blkno = blk;
>   		retval = put_user_mtpos(p, &mt_pos);
> -		goto out;
> -	}
> -	mutex_unlock(&STp->lock);
> -
> -	switch (cmd_in) {
> -	case SG_IO:
> -	case SCSI_IOCTL_SEND_COMMAND:
> -	case CDROM_SEND_PACKET:
> -		if (!capable(CAP_SYS_RAWIO))
> -			return -EPERM;
> -		break;
> -	default:
> -		break;
>   	}
> -
> -	retval = scsi_ioctl(STp->device, file->f_mode & FMODE_WRITE, cmd_in, p);
> -	if (!retval && cmd_in == SCSI_IOCTL_STOP_UNIT) {
> -		/* unload */
> -		STp->rew_at_close = 0;
> -		STp->ready = ST_NO_TAPE;
> -	}
> -	return retval;
> -
>    out:
>   	mutex_unlock(&STp->lock);
>   	return retval;