Re: [PATCH v13 2/9] nvme: Create helper function to obtain command effects

[Logan Gunthorpe]

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@xxxxxxxxxxxx>
>> Reviewed-by: Sagi Grimberg <sagi@xxxxxxxxxxx>
>> ---
>>   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
>