[PATCH 0/2] nvme-cli: fix disconnect-all for pcie tp

[PATCH 0/2] nvme-cli: fix disconnect-all for pcie tp

[Minwoo Im]

The first patch fixes that it tries to delete_controller even it's a
controller with PCIe transport type.  If so, the rest of the controller
with fabrics type might not be disconnected properly.

The second one is just a trivial clean-up patch.

Minwoo Im (2):
  nvme-cli: don't try to disconnect for ctrl with pcie transport
  nvme-cli: remove unnecessary initialize of local var

 fabrics.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

-- 
2.21.0

[PATCH 1/2] nvme-cli: don't try to disconnect for ctrl with pcie transport

[Minwoo Im]

If a host system has both pcie and fabrics controllers, the following
nvme-cli disconnect-all command will might not be working because
contrlller with pcie transport does not have delete_controller in sysfs.

root at target:~# nvme list
Node             SN                   Model                                    Namespace Usage                      Format           FW Rev
---------------- -------------------- ---------------------------------------- --------- -------------------------- ---------------- --------
/dev/nvme0n1     foo                  QEMU NVMe Ctrl                           1         536.87  MB / 536.87  MB    512   B +  0 B   1.0
/dev/nvme1n1     b92326b9b2323bf0     Linux                                    1         134.22  MB / 134.22  MB    512   B +  0 B   5.1.0-rc
/dev/nvme2n1     65fa04eddd9bbac0     Linux                                    1         134.22  MB / 134.22  MB    512   B +  0 B   5.1.0-rc

root at target:~/nvme-cli.git# nvme disconnect-all
Failed to open /sys/class/nvme/nvme0/delete_controller: No such file or directory

This patch checks the transport type of the controller in iteration.

Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 fabrics.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fabrics.c b/fabrics.c
index 511de06..733a24b 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -1196,6 +1196,8 @@ int disconnect_all(const char *desc, int argc, char **argv)
 
 		for (j = 0; j < subsys->nctrls; j++) {
 			struct ctrl_list_item *ctrl = &subsys->ctrls[j];
+			if (!strcmp(ctrl->transport, "pcie"))
+				continue;
 
 			ret = disconnect_by_device(ctrl->name);
 			if (ret)
-- 
2.21.0

[PATCH 2/2] nvme-cli: remove unnecessary initialize of local var

[Minwoo Im]

This patch does not have any functional changes here.  Just clean-up the
unnecessary initialize.

Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 fabrics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fabrics.c b/fabrics.c
index 733a24b..573a6ef 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -1180,7 +1180,7 @@ int disconnect(const char *desc, int argc, char **argv)
 int disconnect_all(const char *desc, int argc, char **argv)
 {
 	struct subsys_list_item *slist;
-	int i, j, ret = 0, subcnt = 0;
+	int i, j, ret, subcnt = 0;
 	const struct argconfig_commandline_options command_line_options[] = {
 		{NULL},
 	};
-- 
2.21.0

[PATCH 2/2] nvme-cli: remove unnecessary initialize of local var

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
On 5/16/19 10:24 AM, Minwoo Im wrote:
> This patch does not have any functional changes here.  Just clean-up the
> unnecessary initialize.
>
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
>  fabrics.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fabrics.c b/fabrics.c
> index 733a24b..573a6ef 100644
> --- a/fabrics.c
> +++ b/fabrics.c
> @@ -1180,7 +1180,7 @@ int disconnect(const char *desc, int argc, char **argv)
>  int disconnect_all(const char *desc, int argc, char **argv)
>  {
>  	struct subsys_list_item *slist;
> -	int i, j, ret = 0, subcnt = 0;
> +	int i, j, ret, subcnt = 0;
>  	const struct argconfig_commandline_options command_line_options[] = {
>  		{NULL},
>  	};

[PATCH 1/2] nvme-cli: don't try to disconnect for ctrl with pcie transport

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>

On 5/16/19 10:24 AM, Minwoo Im wrote:
> If a host system has both pcie and fabrics controllers, the following
> nvme-cli disconnect-all command will might not be working because
> contrlller with pcie transport does not have delete_controller in sysfs.

Can we please move following block of the output into the comment(not
sure if that is the right name)

of the patch section ? i.e. after signoff... line. This can be done at
the merge time also.

---

output

---

fabrics.c | 2 ++

>
> root at target:~# nvme list
> Node             SN                   Model                                    Namespace Usage                      Format           FW Rev
> ---------------- -------------------- ---------------------------------------- --------- -------------------------- ---------------- --------
> /dev/nvme0n1     foo                  QEMU NVMe Ctrl                           1         536.87  MB / 536.87  MB    512   B +  0 B   1.0
> /dev/nvme1n1     b92326b9b2323bf0     Linux                                    1         134.22  MB / 134.22  MB    512   B +  0 B   5.1.0-rc
> /dev/nvme2n1     65fa04eddd9bbac0     Linux                                    1         134.22  MB / 134.22  MB    512   B +  0 B   5.1.0-rc
>
> root at target:~/nvme-cli.git# nvme disconnect-all
> Failed to open /sys/class/nvme/nvme0/delete_controller: No such file or directory
>
> This patch checks the transport type of the controller in iteration.
>
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
>  fabrics.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fabrics.c b/fabrics.c
> index 511de06..733a24b 100644
> --- a/fabrics.c
> +++ b/fabrics.c
> @@ -1196,6 +1196,8 @@ int disconnect_all(const char *desc, int argc, char **argv)
>  
>  		for (j = 0; j < subsys->nctrls; j++) {
>  			struct ctrl_list_item *ctrl = &subsys->ctrls[j];
> +			if (!strcmp(ctrl->transport, "pcie"))

I didn't looked into the case but it is safe to use strcasecmp()? if you
are sure strcmp()

will do the job, please disregard this comment.

> +				continue;
>  
>  			ret = disconnect_by_device(ctrl->name);
>  			if (ret)

[PATCH 0/2] nvme-cli: fix disconnect-all for pcie tp

[Minwoo Im]

Gentle ping here.

[PATCH 0/2] nvme-cli: fix disconnect-all for pcie tp

[Sagi Grimberg]

For both patches:

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 0/2] nvme-cli: fix disconnect-all for pcie tp

[Keith Busch]

On Thu, May 16, 2019@10:23:48AM -0700, Minwoo Im wrote:
> The first patch fixes that it tries to delete_controller even it's a
> controller with PCIe transport type.  If so, the rest of the controller
> with fabrics type might not be disconnected properly.
> 
> The second one is just a trivial clean-up patch.

Thanks, added with Sagi and Chaitanya's reviews.

[PATCH 1/2] nvme-cli: don't try to disconnect for ctrl with pcie transport

[Minwoo Im]

On 19-05-16 23:08:31, Chaitanya Kulkarni wrote:
> Looks good.
> 
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
> 
> On 5/16/19 10:24 AM, Minwoo Im wrote:
> > If a host system has both pcie and fabrics controllers, the following
> > nvme-cli disconnect-all command will might not be working because
> > contrlller with pcie transport does not have delete_controller in sysfs.
> 
> Can we please move following block of the output into the comment(not
> sure if that is the right name)
> 
> of the patch section ? i.e. after signoff... line. This can be done at
> the merge time also.

Chaitanya,

Sorry for lately response here.  I don't get what you really mean here.
Could you please explain why should we move the following output lines
to after --- blocks ?

> 
> ---
> 
> output
> 
> ---
> 
> fabrics.c | 2 ++
> 
> >
> > root at target:~# nvme list
> > Node             SN                   Model                                    Namespace Usage                      Format           FW Rev
> > ---------------- -------------------- ---------------------------------------- --------- -------------------------- ---------------- --------
> > /dev/nvme0n1     foo                  QEMU NVMe Ctrl                           1         536.87  MB / 536.87  MB    512   B +  0 B   1.0
> > /dev/nvme1n1     b92326b9b2323bf0     Linux                                    1         134.22  MB / 134.22  MB    512   B +  0 B   5.1.0-rc
> > /dev/nvme2n1     65fa04eddd9bbac0     Linux                                    1         134.22  MB / 134.22  MB    512   B +  0 B   5.1.0-rc
> >
> > root at target:~/nvme-cli.git# nvme disconnect-all
> > Failed to open /sys/class/nvme/nvme0/delete_controller: No such file or directory
> >
> > This patch checks the transport type of the controller in iteration.
> >
> > Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> > ---
> >  fabrics.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/fabrics.c b/fabrics.c
> > index 511de06..733a24b 100644
> > --- a/fabrics.c
> > +++ b/fabrics.c
> > @@ -1196,6 +1196,8 @@ int disconnect_all(const char *desc, int argc, char **argv)
> >  
> >  		for (j = 0; j < subsys->nctrls; j++) {
> >  			struct ctrl_list_item *ctrl = &subsys->ctrls[j];
> > +			if (!strcmp(ctrl->transport, "pcie"))
> 
> I didn't looked into the case but it is safe to use strcasecmp()? if you
> are sure strcmp()
> 
> will do the job, please disregard this comment.

strcmp() is enough to go here :)