Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>
Cc:     Christoph Hellwig <hch@lst.de>, Sagi Grimberg <sagi@grimberg.me>,
        Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@fb.com>,
        Max Gurtovoy <maxg@mellanox.com>,
        Stephen Bates <sbates@raithlin.com>
References: <20200514172253.6445-1-logang@deltatee.com>
 <20200514172253.6445-3-logang@deltatee.com>
 <BYAPR04MB49658F968DC104E99B5C923286800@BYAPR04MB4965.namprd04.prod.outlook.com>
From:   Logan Gunthorpe <logang@deltatee.com>
Message-ID: <09f1de95-533e-6707-9c86-665f32eb53d7@deltatee.com>
Date:   Thu, 11 Jun 2020 17:09:03 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <BYAPR04MB49658F968DC104E99B5C923286800@BYAPR04MB4965.namprd04.prod.outlook.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [PATCH v13 2/9] nvme: Create helper function to obtain command
 effects
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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
>