Re: [PATCHv2] nvme: always initialize known command effects

[Kanchan Joshi <joshi.k@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 3423 bytes --]

On Thu, Jan 19, 2023 at 08:41:28AM -0800, Keith Busch wrote:
>From: Keith Busch <kbusch@kernel.org>
>
>Instead of appending command effects flags per IO, set the known effects
>flags the driver needs to react to just once during initial setup.
>
>Signed-off-by: Keith Busch <kbusch@kernel.org>
>---
> drivers/nvme/host/core.c | 83 +++++++++++++++++++++-------------------
> 1 file changed, 44 insertions(+), 39 deletions(-)
>
>diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>index 7be562a4e1aa7..d7d2c2b342ba4 100644
>--- a/drivers/nvme/host/core.c
>+++ b/drivers/nvme/host/core.c
>@@ -1060,41 +1060,12 @@ 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;
>-}
>-
>-static u32 nvme_known_nvm_effects(u8 opcode)
>-{
>-	switch (opcode) {
>-	case nvme_cmd_write:
>-	case nvme_cmd_write_zeroes:
>-	case nvme_cmd_write_uncor:
>-		 return NVME_CMD_EFFECTS_LBCC;
>-	default:
>-		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]);
>-		if (ns->head->ids.csi == NVME_CAP_CSS_NVM)
>-			effects |= nvme_known_nvm_effects(opcode);
>+		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 unusual effects:%08x\n",
>@@ -1107,9 +1078,7 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
> 		 */
> 		effects &= ~NVME_CMD_EFFECTS_CSE_MASK;
> 	} else {
>-		if (ctrl->effects)
>-			effects = le32_to_cpu(ctrl->effects->acs[opcode]);
>-		effects |= nvme_known_admin_effects(opcode);
>+		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
> 	}
>
> 	return effects;
>@@ -3122,6 +3091,44 @@ 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] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC |
>+						NVME_CMD_EFFECTS_NCC |
>+						NVME_CMD_EFFECTS_CSE_MASK);
>+	log->acs[nvme_admin_sanitize_nvm] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC |
>+						NVME_CMD_EFFECTS_CSE_MASK);
>+
>+	log->iocs[nvme_cmd_write] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
>+	log->iocs[nvme_cmd_write_zeroes] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
>+	log->iocs[nvme_cmd_write_uncor] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
>+}
>+
>+static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>+{
>+	int ret = 0;
>+
>+	if (ctrl->effects)
>+		return 0;
>+
>+	if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
>+		ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
>+		if (ret < 0)
>+			return ret;
>+	}
>+
>+	if (!ctrl->effects) {
>+		ctrl->effects = kzalloc(sizeof(*ctrl->effects), GFP_KERNEL);

ctrl->effects is not getting freed if controller does not support the
commands-supported-and-effects log page?


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]