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

[Laurence Oberman <loberman@redhat.com>]

On Tue, 2025-11-04 at 10:46 -0500, 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;


For the series:
Reviewed by: Laurence Oberman <loberman@redhat.com>

Many hours of testing done here folks, this looks to be solid and fixes
all our prior issues.