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
--
2.20.1
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.
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
--
2.22.1
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
>