[PATCH 6/7] nvme: also return I/O command effects from nvme_command_effects

[Keith Busch]

On Tue, Dec 13, 2022 at 07:54:15PM +0100, Christoph Hellwig wrote:
> On Tue, Dec 13, 2022 at 10:53:07AM -0700, Keith Busch wrote:
> > Also nvme_cmd_write_uncorr.
> 
> Indeed.
> 
> > I think you'd need to know the namespace's command set to assume these
> > opcodes, though.
> 
> ctrl->effects always is NVM command set if supported, or otherwise
> empty for I/O commands.
> 
> > I was thinking instead of appending effects per-IO, we could always set
> > ns->head->effects even if the device doesn't support the log page, then
> > overwrite the log page's opcodes with known LBCC effects during
> > initialization. That way we don't need a per-io check. Does that sound
> > reasonable?
> 
> I'd prefer to keep OR ingin the known worst cases.  But we can do
> that at setup time.

This is what I was thinking (but please ignore that I missed the
endian conversion in this example):

---
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d11e0d5b231df..b8672dc28beee 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1060,39 +1060,19 @@ int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 }
 EXPORT_SYMBOL_GPL(nvme_submit_sync_cmd);
 
-static u32 nvme_known_admin_effects(u8 opcode)
-{
-	switch (opcode) {
-	case nvme_admin_format_nvm:
-		return NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_NCC |
-			NVME_CMD_EFFECTS_CSE_MASK;
-	case nvme_admin_sanitize_nvm:
-		return NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK;
-	default:
-		break;
-	}
-	return 0;
-}
-
 u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 {
-	u32 effects = 0;
-
 	if (ns) {
-		if (ns->head->effects)
-			effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
+		u32 effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
+
 		if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
 			dev_warn_once(ctrl->device,
 				"IO command:%02x has unhandled effects:%08x\n",
 				opcode, effects);
-		return 0;
+		return effects;
 	}
 
-	if (ctrl->effects)
-		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
-	effects |= nvme_known_admin_effects(opcode);
-
-	return effects;
+	return le32_to_cpu(ctrl->effects->acs[opcode]);
 }
 EXPORT_SYMBOL_NS_GPL(nvme_command_effects, NVME_TARGET_PASSTHRU);
 
@@ -3100,6 +3080,21 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
 	return ret;
 }
 
+static void nvme_init_known_nvm_effects(struct nvme_ctrl *ctrl)
+{
+	struct nvme_effects_log	*log = ctrl->effects;
+
+	log->acs[nvme_admin_format_nvm] |= NVME_CMD_EFFECTS_LBCC |
+						NVME_CMD_EFFECTS_NCC |
+						NVME_CMD_EFFECTS_CSE_MASK;
+	log->acs[nvme_admin_sanitize_nvm] |= NVME_CMD_EFFECTS_LBCC |
+						NVME_CMD_EFFECTS_CSE_MASK;
+
+	log->iocs[nvme_cmd_write] |= NVME_CMD_EFFECTS_LBCC;
+	log->iocs[nvme_cmd_write_zeroes] |= NVME_CMD_EFFECTS_LBCC;
+	log->iocs[nvme_cmd_write_uncor] |= NVME_CMD_EFFECTS_LBCC;
+}
+
 static int nvme_init_identify(struct nvme_ctrl *ctrl)
 {
 	struct nvme_id_ctrl *id;
@@ -3113,10 +3108,21 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 		return -EIO;
 	}
 
-	if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
-		ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
-		if (ret < 0)
-			goto out_free;
+	if (!ctrl->effects) {
+		if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
+			ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM,
+						   &ctrl->effects);
+			if (ret < 0)
+				goto out_free;
+		} else {
+			ctrl->effects = kzalloc(sizeof(*ctrl->effects),
+						GFP_KERNEL);
+			if (!ctrl->effects) {
+				ret = -ENOMEM;
+				goto out_free;
+			}
+		}
+		nvme_init_known_nvm_effects(ctrl);
 	}
 
 	if (!(ctrl->ops->flags & NVME_F_FABRICS))
--