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;
IronPort-SDR: ccP+5hwTWFsCo9sCoDjZBP+SjICszFTMuWevMBd8uPr6bdS0PFdyKfEjBLMP7Ci3zmee1mIaJ5
 k6viuFGp8LB2FZqtB8gh9pzHh34fLBvFVDLSaWypEWtowMv7+UZbhv2/sjkYMzfYN3XSwXIIhk
 iv62uk7gOjLG8FzBio24+NMECAdsfHCSWV1GgcWJJdymTCXdfsVQMIfXIOmMWULg33E0cghaPP
 DLmGl9uzZKp4e/tp1d3WRo3u5CGOLjEBTHIVEMfD4dj9GTYNH/o5Ak6X5ZRySkycxwTXRp+e92
 QAw=
From:   Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
To:     Logan Gunthorpe <logang@deltatee.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>
Subject: Re: [PATCH v13 2/9] nvme: Create helper function to obtain command
 effects
Thread-Topic: [PATCH v13 2/9] nvme: Create helper function to obtain command
 effects
Thread-Index: AQHWKhRkkBYMhr4OK0y6kDVy5Q4NnQ==
Date:   Thu, 11 Jun 2020 22:56:42 +0000
Message-ID: <BYAPR04MB49658F968DC104E99B5C923286800@BYAPR04MB4965.namprd04.prod.outlook.com>
References: <20200514172253.6445-1-logang@deltatee.com>
 <20200514172253.6445-3-logang@deltatee.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: c9f43330-309f-4889-c8c2-08d80e5ab55e
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Jun 2020 22:56:42.4639
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: hQGZKOfuTgV4V9TZuaJBKUjtjSoKkVQI4PYZLthydxLQvOoHz4IeEjrlwBN0XCNZrUmZz4OHK0KUX3j/448w8KQTMAaXJdRh57Wzzedgdkw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR04MB6884
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 5/14/20 10:23 AM, Logan Gunthorpe wrote:=0A=
> Separate the code to obtain command effects from the code=0A=
> to start a passthru request and open code nvme_known_admin_effects()=0A=
> in the new helper.=0A=
> =0A=
> The new helper function will be necessary for nvmet passthru=0A=
> code to determine if we need to change out of interrupt context=0A=
> to handle the effects.=0A=
> =0A=
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>=0A=
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>=0A=
> ---=0A=
>   drivers/nvme/host/core.c | 39 ++++++++++++++++++++++-----------------=
=0A=
>   1 file changed, 22 insertions(+), 17 deletions(-)=0A=
> =0A=
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c=0A=
> index d22859543e4b..5062a83c3634 100644=0A=
> --- a/drivers/nvme/host/core.c=0A=
> +++ b/drivers/nvme/host/core.c=0A=
> @@ -1317,22 +1317,8 @@ static int nvme_submit_io(struct nvme_ns *ns, stru=
ct nvme_user_io __user *uio)=0A=
>   			metadata, meta_len, lower_32_bits(io.slba), NULL, 0);=0A=
>   }=0A=
>   =0A=
> -static u32 nvme_known_admin_effects(u8 opcode)=0A=
> -{=0A=
> -	switch (opcode) {=0A=
> -	case nvme_admin_format_nvm:=0A=
> -		return NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC |=0A=
> -					NVME_CMD_EFFECTS_CSE_MASK;=0A=
> -	case nvme_admin_sanitize_nvm:=0A=
> -		return NVME_CMD_EFFECTS_CSE_MASK;=0A=
> -	default:=0A=
> -		break;=0A=
> -	}=0A=
> -	return 0;=0A=
> -}=0A=
> -=0A=
> -static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *n=
s,=0A=
> -								u8 opcode)=0A=
> +static u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *=
ns,=0A=
> +				u8 opcode)=0A=
>   {=0A=
>   	u32 effects =3D 0;=0A=
>   =0A=
> @@ -1348,7 +1334,26 @@ static u32 nvme_passthru_start(struct nvme_ctrl *c=
trl, struct nvme_ns *ns,=0A=
>   =0A=
>   	if (ctrl->effects)=0A=
>   		effects =3D le32_to_cpu(ctrl->effects->acs[opcode]);=0A=
> -	effects |=3D nvme_known_admin_effects(opcode);=0A=
> +=0A=
> +	switch (opcode) {=0A=
> +	case nvme_admin_format_nvm:=0A=
> +		effects |=3D NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC |=0A=
> +			NVME_CMD_EFFECTS_CSE_MASK;=0A=
> +		break;=0A=
> +	case nvme_admin_sanitize_nvm:=0A=
> +		effects |=3D NVME_CMD_EFFECTS_CSE_MASK;=0A=
> +		break;=0A=
> +	default:=0A=
> +		break;=0A=
> +	}=0A=
> +=0A=
> +	return effects;=0A=
> +}=0A=
> +=0A=
> +static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *n=
s,=0A=
> +			       u8 opcode)=0A=
> +{=0A=
> +	u32 effects =3D nvme_command_effects(ctrl, ns, opcode);=0A=
>   =0A=
>   	/*=0A=
>   	 * For simplicity, IO to all namespaces is quiesced even if the comman=
d=0A=
> =0A=
=0A=
Seems like you have changed the existing function body from=0A=
returning from switch to returning at the end of the function along with=0A=
the name that is why diff is large, which also adds an extra variable =0A=
named "effect".=0A=
=0A=
How about following ? which keeps the diff small and removes the extra =0A=
variable and keeps the existing code as it.=0A=
=0A=
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c=0A=
index 78fc38b8356f..f47013fdc5ee 100644=0A=
--- a/drivers/nvme/host/core.c=0A=
+++ b/drivers/nvme/host/core.c=0A=
@@ -1326,7 +1326,8 @@ static int nvme_submit_io(struct nvme_ns *ns, =0A=
struct nvme_user_io __user *uio)=0A=
                         metadata, meta_len, lower_32_bits(io.slba), =0A=
NULL, 0);=0A=
  }=0A=
=0A=
-static u32 nvme_known_admin_effects(u8 opcode)=0A=
+static u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns=
,=0A=
+                               u8 opcode)=0A=
  {=0A=
         switch (opcode) {=0A=
         case nvme_admin_format_nvm:=0A=
@@ -1343,7 +1344,7 @@ static u32 nvme_known_admin_effects(u8 opcode)=0A=
  static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns=
,=0A=
                                                                 u8 opcode)=
=0A=
  {=0A=
-       u32 effects =3D 0;=0A=
+       u32 effects =3D nvme_command_effects(ctrl, ns, opcode);=0A=
=0A=
         if (ns) {=0A=
                 if (ctrl->effects)=0A=
@@ -1357,7 +1358,6 @@ static u32 nvme_passthru_start(struct nvme_ctrl =0A=
*ctrl, struct nvme_ns *ns,=0A=
=0A=
         if (ctrl->effects)=0A=
                 effects =3D le32_to_cpu(ctrl->effects->acs[opcode]);=0A=
-       effects |=3D nvme_known_admin_effects(opcode);=0A=
=0A=
         /*=0A=
          * For simplicity, IO to all namespaces is quiesced even if the =
=0A=
command=0A=
-- =0A=
2.22.1=0A=