[PATCH] nvme: module parameter to disable pi checks

[Kanchan Joshi]

On 10/23/2024 9:20 PM, Keith Busch wrote:
> @@ -1763,6 +1767,7 @@ static bool nvme_init_integrity(struct nvme_ns_head *head,
>   		struct queue_limits *lim, struct nvme_ns_info *info)
>   {
>   	struct blk_integrity *bi = &lim->integrity;
> +	int pi_type = head->pi_type;
>   
>   	memset(bi, 0, sizeof(*bi));
>   
> @@ -1777,7 +1782,10 @@ static bool nvme_init_integrity(struct nvme_ns_head *head,
>   	    !(head->features & NVME_NS_METADATA_SUPPORTED))
>   		return nvme_ns_has_pi(head);
>   
> -	switch (head->pi_type) {
> +	if (disable_pi)
> +		pi_type = 0;

I think this will not give the desired outcome. Since, regardless of the 
nop profile, driver continues to send PRCHK_GUARD/REF to the device 
during nvme_setup_rw. And you will continue to see read-failures.

Instead, you probably want this patch [1]?

Also in current boolean form, "disable_pi" will do its thing on all 
connected nvme devices. Should we make this fine granular. Like this 
patch [2]?


[1]
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 84cb859a911d..c93557621bcb 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -91,6 +91,10 @@ module_param(apst_secondary_latency_tol_us, ulong, 0644);
  MODULE_PARM_DESC(apst_secondary_latency_tol_us,
         "secondary APST latency tolerance in us");

+static bool disable_pi = false;
+module_param(disable_pi, bool, 0444);
+MODULE_PARM_DESC(disable_pi, "disable protection-information if used");
+
  /*
   * nvme_wq - hosts nvme related works that are not reset or delete
   * nvme_reset_wq - hosts nvme reset works
@@ -1911,7 +1915,7 @@ static void nvme_configure_metadata(struct 
nvme_ctrl *ctrl,
                 head->guard_type = NVME_NVM_NS_16B_GUARD;
         }

-       if (head->pi_size && head->ms >= head->pi_size)
+       if (!disable_pi && head->pi_size && head->ms >= head->pi_size)
                 head->pi_type = id->dps & NVME_NS_DPS_PI_MASK;
         if (!(id->dps & NVME_NS_DPS_PI_FIRST))
                 info->pi_offset = head->ms - head->pi_size;

[2]
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 84cb859a911d..0876acadb81d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -91,6 +91,20 @@ module_param(apst_secondary_latency_tol_us, ulong, 0644);
  MODULE_PARM_DESC(apst_secondary_latency_tol_us,
         "secondary APST latency tolerance in us");

+static u8 disable_pi;
+module_param(disable_pi, byte, 0444);
+MODULE_PARM_DESC(disable_pi, "disable protection-information if used");
+
+/*
+ * disable_pi = 0, do nothing.
+ * disable_pi = 1, disables pi only if it is in the last bytes.
+ * disable_pi > 1, always disables pi.
+ */
+enum {
+       NVME_DISABLE_PI_NONE,
+       NVME_DISABLE_PI_LAST_BYTE,
+};
+
  /*
   * nvme_wq - hosts nvme related works that are not reset or delete
   * nvme_reset_wq - hosts nvme reset works
@@ -1897,6 +1911,8 @@ static void nvme_configure_metadata(struct 
nvme_ctrl *ctrl,
                 struct nvme_ns_head *head, struct nvme_id_ns *id,
                 struct nvme_id_ns_nvm *nvm, struct nvme_ns_info *info)
  {
+       bool last = !(id->dps & NVME_NS_DPS_PI_FIRST);
+
         head->features &= ~(NVME_NS_METADATA_SUPPORTED | NVME_NS_EXT_LBAS);
         head->pi_type = 0;
         head->pi_size = 0;
@@ -1913,9 +1929,20 @@ static void nvme_configure_metadata(struct 
nvme_ctrl *ctrl,

         if (head->pi_size && head->ms >= head->pi_size)
                 head->pi_type = id->dps & NVME_NS_DPS_PI_MASK;
-       if (!(id->dps & NVME_NS_DPS_PI_FIRST))
+       if (last)
                 info->pi_offset = head->ms - head->pi_size;

+       switch (disable_pi) {
+       case NVME_DISABLE_PI_NONE:
+               break;
+       case NVME_DISABLE_PI_LAST_BYTE:
+               if (last)
+                       head->pi_type = 0;
+               break;
+       default:
+               head->pi_type = 0;
+       }
+
         if (ctrl->ops->flags & NVME_F_FABRICS) {
                 /*
                  * The NVMe over Fabrics specification only supports 
metadata as