diff mbox series

[v3,03/19] scsi: Move sd_pr_type to header to share

Message ID 20221026231945.6609-4-michael.christie@oracle.com (mailing list archive)
State New, archived
Headers show
Series Use block pr_ops in LIO | expand

Commit Message

Mike Christie Oct. 26, 2022, 11:19 p.m. UTC 
LIO is going to want to do the same block to/from SCSI pr types as sd.c
so this moves the sd_pr_type helper to a new file. The next patch will
then also add a helper to go from the SCSI value to the block one for use
with PERSISTENT_RESERVE_IN commands.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c            | 31 +++++++------------------------
 include/scsi/scsi_block_pr.h | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 24 deletions(-)
 create mode 100644 include/scsi/scsi_block_pr.h

Comments

Chaitanya Kulkarni Nov. 1, 2022, 5:43 a.m. UTC | #1 
On 10/26/22 16:19, Mike Christie wrote:
> LIO is going to want to do the same block to/from SCSI pr types as sd.c
> so this moves the sd_pr_type helper to a new file. The next patch will
> then also add a helper to go from the SCSI value to the block one for use
> with PERSISTENT_RESERVE_IN commands.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/sd.c            | 31 +++++++------------------------
>   include/scsi/scsi_block_pr.h | 36 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 43 insertions(+), 24 deletions(-)
>   create mode 100644 include/scsi/scsi_block_pr.h
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 4dc5c932fbd3..ad9374b07585 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -67,6 +67,7 @@
>   #include <scsi/scsi_host.h>
>   #include <scsi/scsi_ioctl.h>
>   #include <scsi/scsicam.h>
> +#include <scsi/scsi_block_pr.h>
>   
>   #include "sd.h"
>   #include "scsi_priv.h"
> @@ -1694,28 +1695,8 @@ static int sd_get_unique_id(struct gendisk *disk, u8 id[16],
>   	return ret;
>   }
>   
> -static char sd_pr_type(enum pr_type type)
> -{
> -	switch (type) {
> -	case PR_WRITE_EXCLUSIVE:
> -		return 0x01;
> -	case PR_EXCLUSIVE_ACCESS:
> -		return 0x03;
> -	case PR_WRITE_EXCLUSIVE_REG_ONLY:
> -		return 0x05;
> -	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
> -		return 0x06;
> -	case PR_WRITE_EXCLUSIVE_ALL_REGS:
> -		return 0x07;
> -	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
> -		return 0x08;
> -	default:
> -		return 0;
> -	}
> -};
> -
>   static int sd_pr_out_command(struct block_device *bdev, u8 sa,
> -		u64 key, u64 sa_key, u8 type, u8 flags)
> +		u64 key, u64 sa_key, enum scsi_pr_type type, u8 flags)
>   {
>   	struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);
>   	struct scsi_device *sdev = sdkp->device;
> @@ -1778,19 +1759,21 @@ static int sd_pr_reserve(struct block_device *bdev, u64 key, enum pr_type type,
>   {
>   	if (flags)
>   		return -EOPNOTSUPP;
> -	return sd_pr_out_command(bdev, 0x01, key, 0, sd_pr_type(type), 0);
> +	return sd_pr_out_command(bdev, 0x01, key, 0,
> +				 block_pr_type_to_scsi(type), 0);
>   }
>   
>   static int sd_pr_release(struct block_device *bdev, u64 key, enum pr_type type)
>   {
> -	return sd_pr_out_command(bdev, 0x02, key, 0, sd_pr_type(type), 0);
> +	return sd_pr_out_command(bdev, 0x02, key, 0,
> +				 block_pr_type_to_scsi(type), 0);
>   }
>   
>   static int sd_pr_preempt(struct block_device *bdev, u64 old_key, u64 new_key,
>   		enum pr_type type, bool abort)
>   {
>   	return sd_pr_out_command(bdev, abort ? 0x05 : 0x04, old_key, new_key,
> -			     sd_pr_type(type), 0);
> +				 block_pr_type_to_scsi(type), 0);
>   }
>   
>   static int sd_pr_clear(struct block_device *bdev, u64 key)
> diff --git a/include/scsi/scsi_block_pr.h b/include/scsi/scsi_block_pr.h
> new file mode 100644
> index 000000000000..6e99f844330d
> --- /dev/null
> +++ b/include/scsi/scsi_block_pr.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _SCSI_BLOCK_PR_H
> +#define _SCSI_BLOCK_PR_H
> +
> +#include <uapi/linux/pr.h>
> +
> +enum scsi_pr_type {
> +	SCSI_PR_WRITE_EXCLUSIVE			= 0x01,
> +	SCSI_PR_EXCLUSIVE_ACCESS		= 0x03,
> +	SCSI_PR_WRITE_EXCLUSIVE_REG_ONLY	= 0x05,
> +	SCSI_PR_EXCLUSIVE_ACCESS_REG_ONLY	= 0x06,
> +	SCSI_PR_WRITE_EXCLUSIVE_ALL_REGS	= 0x07,
> +	SCSI_PR_EXCLUSIVE_ACCESS_ALL_REGS	= 0x08,
> +};
> +
> +static inline enum scsi_pr_type block_pr_type_to_scsi(enum pr_type type)
> +{
> +	switch (type) {
> +	case PR_WRITE_EXCLUSIVE:
> +		return SCSI_PR_WRITE_EXCLUSIVE;
> +	case PR_EXCLUSIVE_ACCESS:
> +		return SCSI_PR_EXCLUSIVE_ACCESS;
> +	case PR_WRITE_EXCLUSIVE_REG_ONLY:
> +		return SCSI_PR_WRITE_EXCLUSIVE_REG_ONLY;
> +	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
> +		return SCSI_PR_EXCLUSIVE_ACCESS_REG_ONLY;
> +	case PR_WRITE_EXCLUSIVE_ALL_REGS:
> +		return SCSI_PR_WRITE_EXCLUSIVE_ALL_REGS;
> +	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
> +		return SCSI_PR_EXCLUSIVE_ACCESS_ALL_REGS;
> +	default:
> +		return 0;
> +	}
> +};


do we need above semicolon ?

how about not using switch case pattern totally untested below ?

static inline enum scsi_pr_type block_pr_type_to_scsi(enum pr_type type)
{
         enum pr_type pr_to_scsi_pr[] = {
                 [PR_WRITE_EXCLUSIVE] = SCSI_PR_WRITE_EXCLUSIVE,
                 [PR_EXCLUSIVE_ACCESS] = SCSI_PR_EXCLUSIVE_ACCESS,
                 [PR_WRITE_EXCLUSIVE_REG_ONLY] = 
SCSI_PR_WRITE_EXCLUSIVE_REG_ONLY,
                 [PR_EXCLUSIVE_ACCESS_REG_ONLY] = 
SCSI_PR_EXCLUSIVE_ACCESS_REG_ONLY,
                 [PR_WRITE_EXCLUSIVE_ALL_REGS] = 
SCSI_PR_WRITE_EXCLUSIVE_ALL_REGS,
                 [PR_EXCLUSIVE_ACCESS_ALL_REGS] = 
SCSI_PR_EXCLUSIVE_ACCESS_ALL_REGS,
         };

         if (type > ARRAY_SIZE(pr_to_scsi_pr))
                 return 0;
         return pr_to_scsi_pr[type];
}


-ck
Mike Christie Nov. 1, 2022, 4:43 p.m. UTC | #2 
On 11/1/22 12:43 AM, Chaitanya Kulkarni wrote:
>> +static inline enum scsi_pr_type block_pr_type_to_scsi(enum pr_type type)
>> +{
>> +	switch (type) {
>> +	case PR_WRITE_EXCLUSIVE:
>> +		return SCSI_PR_WRITE_EXCLUSIVE;
>> +	case PR_EXCLUSIVE_ACCESS:
>> +		return SCSI_PR_EXCLUSIVE_ACCESS;
>> +	case PR_WRITE_EXCLUSIVE_REG_ONLY:
>> +		return SCSI_PR_WRITE_EXCLUSIVE_REG_ONLY;
>> +	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
>> +		return SCSI_PR_EXCLUSIVE_ACCESS_REG_ONLY;
>> +	case PR_WRITE_EXCLUSIVE_ALL_REGS:
>> +		return SCSI_PR_WRITE_EXCLUSIVE_ALL_REGS;
>> +	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
>> +		return SCSI_PR_EXCLUSIVE_ACCESS_ALL_REGS;
>> +	default:
>> +		return 0;
>> +	}
>> +};
> 
> 
> do we need above semicolon ?

No. It was a mistake.

> 
> how about not using switch case pattern totally untested below ?
> 
> static inline enum scsi_pr_type block_pr_type_to_scsi(enum pr_type type)
> {
>          enum pr_type pr_to_scsi_pr[] = {
>                  [PR_WRITE_EXCLUSIVE] = SCSI_PR_WRITE_EXCLUSIVE,
>                  [PR_EXCLUSIVE_ACCESS] = SCSI_PR_EXCLUSIVE_ACCESS,
>                  [PR_WRITE_EXCLUSIVE_REG_ONLY] = 
> SCSI_PR_WRITE_EXCLUSIVE_REG_ONLY,
>                  [PR_EXCLUSIVE_ACCESS_REG_ONLY] = 
> SCSI_PR_EXCLUSIVE_ACCESS_REG_ONLY,
>                  [PR_WRITE_EXCLUSIVE_ALL_REGS] = 
> SCSI_PR_WRITE_EXCLUSIVE_ALL_REGS,
>                  [PR_EXCLUSIVE_ACCESS_ALL_REGS] = 
> SCSI_PR_EXCLUSIVE_ACCESS_ALL_REGS,
>          };
> 
>          if (type > ARRAY_SIZE(pr_to_scsi_pr))
>                  return 0;
>          return pr_to_scsi_pr[type];
> }
> 

Keith also wanted something like this for nvme so will fix up
the scsi and nvme code.
Bart Van Assche Nov. 2, 2022, 10:47 p.m. UTC | #3 
On 10/26/22 16:19, Mike Christie wrote:
> +static inline enum scsi_pr_type block_pr_type_to_scsi(enum pr_type type)
> +{
> +	switch (type) {
> +	case PR_WRITE_EXCLUSIVE:
> +		return SCSI_PR_WRITE_EXCLUSIVE;
> +	case PR_EXCLUSIVE_ACCESS:
> +		return SCSI_PR_EXCLUSIVE_ACCESS;
> +	case PR_WRITE_EXCLUSIVE_REG_ONLY:
> +		return SCSI_PR_WRITE_EXCLUSIVE_REG_ONLY;
> +	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
> +		return SCSI_PR_EXCLUSIVE_ACCESS_REG_ONLY;
> +	case PR_WRITE_EXCLUSIVE_ALL_REGS:
> +		return SCSI_PR_WRITE_EXCLUSIVE_ALL_REGS;
> +	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
> +		return SCSI_PR_EXCLUSIVE_ACCESS_ALL_REGS;
> +	default:
> +		return 0;
> +	}
> +};

Please leave out "default: return 0;" from the switch statement and add 
"return 0;" after the switch statement. That will make the compiler emit 
a warning if a value is added in enum pr_type but not in the above function.

Thanks,

Bart.
Mike Christie Nov. 3, 2022, 2:13 a.m. UTC | #4 
On 11/2/22 5:47 PM, Bart Van Assche wrote:
> On 10/26/22 16:19, Mike Christie wrote:
>> +static inline enum scsi_pr_type block_pr_type_to_scsi(enum pr_type type)
>> +{
>> +    switch (type) {
>> +    case PR_WRITE_EXCLUSIVE:
>> +        return SCSI_PR_WRITE_EXCLUSIVE;
>> +    case PR_EXCLUSIVE_ACCESS:
>> +        return SCSI_PR_EXCLUSIVE_ACCESS;
>> +    case PR_WRITE_EXCLUSIVE_REG_ONLY:
>> +        return SCSI_PR_WRITE_EXCLUSIVE_REG_ONLY;
>> +    case PR_EXCLUSIVE_ACCESS_REG_ONLY:
>> +        return SCSI_PR_EXCLUSIVE_ACCESS_REG_ONLY;
>> +    case PR_WRITE_EXCLUSIVE_ALL_REGS:
>> +        return SCSI_PR_WRITE_EXCLUSIVE_ALL_REGS;
>> +    case PR_EXCLUSIVE_ACCESS_ALL_REGS:
>> +        return SCSI_PR_EXCLUSIVE_ACCESS_ALL_REGS;
>> +    default:
>> +        return 0;
>> +    }
>> +};
> 
> Please leave out "default: return 0;" from the switch statement and add "return 0;" after the switch statement. That will make the compiler emit a warning if a value is added in enum pr_type but not in the above function.
Hey Bart,

Did you want that compiler warning functionality in the future code
or are you asking me to do the above only if we go the switch based
route?

Chaitanya requested me to make this array based in nvme/scsi. For this
approach, I can add a WARN or printk warning if the pr_type passed in
does not match a value in the array. I don't think I can do a compiler
warning though.

I didn't care, but I think the compiler warning route might be better
though.
Bart Van Assche Nov. 3, 2022, 6:14 p.m. UTC | #5 
On 11/2/22 19:13, Mike Christie wrote:
> On 11/2/22 5:47 PM, Bart Van Assche wrote:
>> On 10/26/22 16:19, Mike Christie wrote:
>>> +static inline enum scsi_pr_type block_pr_type_to_scsi(enum pr_type type)
>>> +{
>>> +    switch (type) {
>>> +    case PR_WRITE_EXCLUSIVE:
>>> +        return SCSI_PR_WRITE_EXCLUSIVE;
>>> +    case PR_EXCLUSIVE_ACCESS:
>>> +        return SCSI_PR_EXCLUSIVE_ACCESS;
>>> +    case PR_WRITE_EXCLUSIVE_REG_ONLY:
>>> +        return SCSI_PR_WRITE_EXCLUSIVE_REG_ONLY;
>>> +    case PR_EXCLUSIVE_ACCESS_REG_ONLY:
>>> +        return SCSI_PR_EXCLUSIVE_ACCESS_REG_ONLY;
>>> +    case PR_WRITE_EXCLUSIVE_ALL_REGS:
>>> +        return SCSI_PR_WRITE_EXCLUSIVE_ALL_REGS;
>>> +    case PR_EXCLUSIVE_ACCESS_ALL_REGS:
>>> +        return SCSI_PR_EXCLUSIVE_ACCESS_ALL_REGS;
>>> +    default:
>>> +        return 0;
>>> +    }
>>> +};
>>
>> Please leave out "default: return 0;" from the switch statement and add "return 0;" after the switch statement. That will make the compiler emit a warning if a value is added in enum pr_type but not in the above function.
> 
> Did you want that compiler warning functionality in the future code
> or are you asking me to do the above only if we go the switch based
> route?
> 
> Chaitanya requested me to make this array based in nvme/scsi. For this
> approach, I can add a WARN or printk warning if the pr_type passed in
> does not match a value in the array. I don't think I can do a compiler
> warning though.
> 
> I didn't care, but I think the compiler warning route might be better
> though.

Hi Mike,

Although I do not have a strong opinion about this, keeping the switch 
statement and moving the return statement out of the switch statement 
has the advantage that the compiler will warn if the switch statement is 
incomplete. I don't think that a similar warning would be emitted if the 
switch statement would be converted into an array lookup.

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4dc5c932fbd3..ad9374b07585 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -67,6 +67,7 @@ 
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>
 #include <scsi/scsicam.h>
+#include <scsi/scsi_block_pr.h>
 
 #include "sd.h"
 #include "scsi_priv.h"
@@ -1694,28 +1695,8 @@  static int sd_get_unique_id(struct gendisk *disk, u8 id[16],
 	return ret;
 }
 
-static char sd_pr_type(enum pr_type type)
-{
-	switch (type) {
-	case PR_WRITE_EXCLUSIVE:
-		return 0x01;
-	case PR_EXCLUSIVE_ACCESS:
-		return 0x03;
-	case PR_WRITE_EXCLUSIVE_REG_ONLY:
-		return 0x05;
-	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
-		return 0x06;
-	case PR_WRITE_EXCLUSIVE_ALL_REGS:
-		return 0x07;
-	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
-		return 0x08;
-	default:
-		return 0;
-	}
-};
-
 static int sd_pr_out_command(struct block_device *bdev, u8 sa,
-		u64 key, u64 sa_key, u8 type, u8 flags)
+		u64 key, u64 sa_key, enum scsi_pr_type type, u8 flags)
 {
 	struct scsi_disk *sdkp = scsi_disk(bdev->bd_disk);
 	struct scsi_device *sdev = sdkp->device;
@@ -1778,19 +1759,21 @@  static int sd_pr_reserve(struct block_device *bdev, u64 key, enum pr_type type,
 {
 	if (flags)
 		return -EOPNOTSUPP;
-	return sd_pr_out_command(bdev, 0x01, key, 0, sd_pr_type(type), 0);
+	return sd_pr_out_command(bdev, 0x01, key, 0,
+				 block_pr_type_to_scsi(type), 0);
 }
 
 static int sd_pr_release(struct block_device *bdev, u64 key, enum pr_type type)
 {
-	return sd_pr_out_command(bdev, 0x02, key, 0, sd_pr_type(type), 0);
+	return sd_pr_out_command(bdev, 0x02, key, 0,
+				 block_pr_type_to_scsi(type), 0);
 }
 
 static int sd_pr_preempt(struct block_device *bdev, u64 old_key, u64 new_key,
 		enum pr_type type, bool abort)
 {
 	return sd_pr_out_command(bdev, abort ? 0x05 : 0x04, old_key, new_key,
-			     sd_pr_type(type), 0);
+				 block_pr_type_to_scsi(type), 0);
 }
 
 static int sd_pr_clear(struct block_device *bdev, u64 key)
diff --git a/include/scsi/scsi_block_pr.h b/include/scsi/scsi_block_pr.h
new file mode 100644
index 000000000000..6e99f844330d
--- /dev/null
+++ b/include/scsi/scsi_block_pr.h
@@ -0,0 +1,36 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _SCSI_BLOCK_PR_H
+#define _SCSI_BLOCK_PR_H
+
+#include <uapi/linux/pr.h>
+
+enum scsi_pr_type {
+	SCSI_PR_WRITE_EXCLUSIVE			= 0x01,
+	SCSI_PR_EXCLUSIVE_ACCESS		= 0x03,
+	SCSI_PR_WRITE_EXCLUSIVE_REG_ONLY	= 0x05,
+	SCSI_PR_EXCLUSIVE_ACCESS_REG_ONLY	= 0x06,
+	SCSI_PR_WRITE_EXCLUSIVE_ALL_REGS	= 0x07,
+	SCSI_PR_EXCLUSIVE_ACCESS_ALL_REGS	= 0x08,
+};
+
+static inline enum scsi_pr_type block_pr_type_to_scsi(enum pr_type type)
+{
+	switch (type) {
+	case PR_WRITE_EXCLUSIVE:
+		return SCSI_PR_WRITE_EXCLUSIVE;
+	case PR_EXCLUSIVE_ACCESS:
+		return SCSI_PR_EXCLUSIVE_ACCESS;
+	case PR_WRITE_EXCLUSIVE_REG_ONLY:
+		return SCSI_PR_WRITE_EXCLUSIVE_REG_ONLY;
+	case PR_EXCLUSIVE_ACCESS_REG_ONLY:
+		return SCSI_PR_EXCLUSIVE_ACCESS_REG_ONLY;
+	case PR_WRITE_EXCLUSIVE_ALL_REGS:
+		return SCSI_PR_WRITE_EXCLUSIVE_ALL_REGS;
+	case PR_EXCLUSIVE_ACCESS_ALL_REGS:
+		return SCSI_PR_EXCLUSIVE_ACCESS_ALL_REGS;
+	default:
+		return 0;
+	}
+};
+
+#endif