Messages in this thread | | | From | Logan Gunthorpe <> | Date | Thu, 11 Jun 2020 17:09:03 -0600 | Subject | Re: [PATCH v13 2/9] nvme: Create helper function to obtain command effects |
| |
On 2020-06-11 4:56 p.m., Chaitanya Kulkarni wrote: > On 5/14/20 10:23 AM, Logan Gunthorpe wrote: >> Separate the code to obtain command effects from the code >> to start a passthru request and open code nvme_known_admin_effects() >> in the new helper. >> >> The new helper function will be necessary for nvmet passthru >> code to determine if we need to change out of interrupt context >> to handle the effects. >> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >> Reviewed-by: Sagi Grimberg <sagi@grimberg.me> >> --- >> drivers/nvme/host/core.c | 39 ++++++++++++++++++++++----------------- >> 1 file changed, 22 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >> index d22859543e4b..5062a83c3634 100644 >> --- a/drivers/nvme/host/core.c >> +++ b/drivers/nvme/host/core.c >> @@ -1317,22 +1317,8 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio) >> metadata, meta_len, lower_32_bits(io.slba), NULL, 0); >> } >> >> -static u32 nvme_known_admin_effects(u8 opcode) >> -{ >> - switch (opcode) { >> - case nvme_admin_format_nvm: >> - return NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC | >> - NVME_CMD_EFFECTS_CSE_MASK; >> - case nvme_admin_sanitize_nvm: >> - return NVME_CMD_EFFECTS_CSE_MASK; >> - default: >> - break; >> - } >> - return 0; >> -} >> - >> -static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns, >> - u8 opcode) >> +static u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, >> + u8 opcode) >> { >> u32 effects = 0; >> >> @@ -1348,7 +1334,26 @@ static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns, >> >> if (ctrl->effects) >> effects = le32_to_cpu(ctrl->effects->acs[opcode]); >> - effects |= nvme_known_admin_effects(opcode); >> + >> + switch (opcode) { >> + case nvme_admin_format_nvm: >> + effects |= NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC | >> + NVME_CMD_EFFECTS_CSE_MASK; >> + break; >> + case nvme_admin_sanitize_nvm: >> + effects |= NVME_CMD_EFFECTS_CSE_MASK; >> + break; >> + default: >> + break; >> + } >> + >> + return effects; >> +} >> + >> +static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns, >> + u8 opcode) >> +{ >> + u32 effects = nvme_command_effects(ctrl, ns, opcode); >> >> /* >> * For simplicity, IO to all namespaces is quiesced even if the command >> > > Seems like you have changed the existing function body from > returning from switch to returning at the end of the function along with > the name that is why diff is large, which also adds an extra variable > named "effect". > > How about following ? which keeps the diff small and removes the extra > variable and keeps the existing code as it.
That doesn't really get what's needed for the new helper. We need a new helper that gets the effects as nvme_passthru_start() sees them (including ctrl->effects->acs[opcode]). Changing the name of nvme_known_admin_effects() is not equivalent.
This patch would be possible to do without open coding nvme_known_admin_effects() in the new helper, but doing so helps to clean things up a bit as nvme_known_admin_effects() isn't used anywhere else.
Logan
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 78fc38b8356f..f47013fdc5ee 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -1326,7 +1326,8 @@ static int nvme_submit_io(struct nvme_ns *ns, > struct nvme_user_io __user *uio) > metadata, meta_len, lower_32_bits(io.slba), > NULL, 0); > } > > -static u32 nvme_known_admin_effects(u8 opcode) > +static u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, > + u8 opcode) > { > switch (opcode) { > case nvme_admin_format_nvm: > @@ -1343,7 +1344,7 @@ static u32 nvme_known_admin_effects(u8 opcode) > static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns, > u8 opcode) > { > - u32 effects = 0; > + u32 effects = nvme_command_effects(ctrl, ns, opcode); > > if (ns) { > if (ctrl->effects) > @@ -1357,7 +1358,6 @@ static u32 nvme_passthru_start(struct nvme_ctrl > *ctrl, struct nvme_ns *ns, > > if (ctrl->effects) > effects = le32_to_cpu(ctrl->effects->acs[opcode]); > - effects |= nvme_known_admin_effects(opcode); > > /* > * For simplicity, IO to all namespaces is quiesced even if the > command >
| |