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 <[email protected]>
>> Reviewed-by: Sagi Grimberg <[email protected]>
>> ---
>> 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
>
