[PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Aneesh Kumar K.V]

ndctl utility requires the ndbus to have unique names. If not while
enumerating the bus in userspace it drops bus with similar names.
This results in us not listing devices beneath the bus.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 drivers/nvdimm/of_pmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
index a0c8dcfa0bf9..97187d6c0bdb 100644
--- a/drivers/nvdimm/of_pmem.c
+++ b/drivers/nvdimm/of_pmem.c
@@ -42,7 +42,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv->bus_desc.attr_groups = bus_attr_groups;
-	priv->bus_desc.provider_name = "of_pmem";
+	priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
 	priv->bus_desc.module = THIS_MODULE;
 	priv->bus_desc.of_node = np;
 
-- 
2.21.0

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Aneesh Kumar K.V]

ndctl utility requires the ndbus to have unique names. If not while
enumerating the bus in userspace it drops bus with similar names.
This results in us not listing devices beneath the bus.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 drivers/nvdimm/of_pmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
index a0c8dcfa0bf9..97187d6c0bdb 100644
--- a/drivers/nvdimm/of_pmem.c
+++ b/drivers/nvdimm/of_pmem.c
@@ -42,7 +42,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv->bus_desc.attr_groups = bus_attr_groups;
-	priv->bus_desc.provider_name = "of_pmem";
+	priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
 	priv->bus_desc.module = THIS_MODULE;
 	priv->bus_desc.of_node = np;
 
-- 
2.21.0

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Dan Williams]

On Tue, Aug 6, 2019 at 9:00 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> ndctl utility requires the ndbus to have unique names. If not while
> enumerating the bus in userspace it drops bus with similar names.
> This results in us not listing devices beneath the bus.

It does?

>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  drivers/nvdimm/of_pmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> index a0c8dcfa0bf9..97187d6c0bdb 100644
> --- a/drivers/nvdimm/of_pmem.c
> +++ b/drivers/nvdimm/of_pmem.c
> @@ -42,7 +42,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
>                 return -ENOMEM;
>
>         priv->bus_desc.attr_groups = bus_attr_groups;
> -       priv->bus_desc.provider_name = "of_pmem";
> +       priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);

This looks ok to me to address support for older ndctl binaries, but
I'd like to also fix the ndctl bug that makes non-unique provider
names fail.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Dan Williams]

On Tue, Aug 6, 2019 at 9:00 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> ndctl utility requires the ndbus to have unique names. If not while
> enumerating the bus in userspace it drops bus with similar names.
> This results in us not listing devices beneath the bus.

It does?

>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  drivers/nvdimm/of_pmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> index a0c8dcfa0bf9..97187d6c0bdb 100644
> --- a/drivers/nvdimm/of_pmem.c
> +++ b/drivers/nvdimm/of_pmem.c
> @@ -42,7 +42,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
>                 return -ENOMEM;
>
>         priv->bus_desc.attr_groups = bus_attr_groups;
> -       priv->bus_desc.provider_name = "of_pmem";
> +       priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);

This looks ok to me to address support for older ndctl binaries, but
I'd like to also fix the ndctl bug that makes non-unique provider
names fail.

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Aneesh Kumar K.V]

On 8/7/19 9:43 AM, Dan Williams wrote:
> On Tue, Aug 6, 2019 at 9:00 PM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
>>
>> ndctl utility requires the ndbus to have unique names. If not while
>> enumerating the bus in userspace it drops bus with similar names.
>> This results in us not listing devices beneath the bus.
> 
> It does?
> 
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   drivers/nvdimm/of_pmem.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
>> index a0c8dcfa0bf9..97187d6c0bdb 100644
>> --- a/drivers/nvdimm/of_pmem.c
>> +++ b/drivers/nvdimm/of_pmem.c
>> @@ -42,7 +42,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
>>                  return -ENOMEM;
>>
>>          priv->bus_desc.attr_groups = bus_attr_groups;
>> -       priv->bus_desc.provider_name = "of_pmem";
>> +       priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
> 
> This looks ok to me to address support for older ndctl binaries, but
> I'd like to also fix the ndctl bug that makes non-unique provider
> names fail.
> 

0462269ab121d323a016874ebdd42217f2911ee7 (ndctl: provide a method to 
invalidate the bus list)

This hunk does the filtering.

@@ -928,6 +929,14 @@ static int add_bus(void *parent, int id, const char 
*ctl_base)
  		goto err_read;
  	bus->buf_len = strlen(bus->bus_path) + 50;

+	ndctl_bus_foreach(ctx, bus_dup)
+		if (strcmp(ndctl_bus_get_provider(bus_dup),
+					ndctl_bus_get_provider(bus)) == 0) {
+			free_bus(bus, NULL);
+			free(path);
+			return 1;
+		}
+
  	list_add(&ctx->busses, &bus->list);

  	rc = 0;

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Aneesh Kumar K.V]

On 8/7/19 9:43 AM, Dan Williams wrote:
> On Tue, Aug 6, 2019 at 9:00 PM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
>>
>> ndctl utility requires the ndbus to have unique names. If not while
>> enumerating the bus in userspace it drops bus with similar names.
>> This results in us not listing devices beneath the bus.
> 
> It does?
> 
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   drivers/nvdimm/of_pmem.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
>> index a0c8dcfa0bf9..97187d6c0bdb 100644
>> --- a/drivers/nvdimm/of_pmem.c
>> +++ b/drivers/nvdimm/of_pmem.c
>> @@ -42,7 +42,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
>>                  return -ENOMEM;
>>
>>          priv->bus_desc.attr_groups = bus_attr_groups;
>> -       priv->bus_desc.provider_name = "of_pmem";
>> +       priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
> 
> This looks ok to me to address support for older ndctl binaries, but
> I'd like to also fix the ndctl bug that makes non-unique provider
> names fail.
> 

0462269ab121d323a016874ebdd42217f2911ee7 (ndctl: provide a method to 
invalidate the bus list)

This hunk does the filtering.

@@ -928,6 +929,14 @@ static int add_bus(void *parent, int id, const char 
*ctl_base)
  		goto err_read;
  	bus->buf_len = strlen(bus->bus_path) + 50;

+	ndctl_bus_foreach(ctx, bus_dup)
+		if (strcmp(ndctl_bus_get_provider(bus_dup),
+					ndctl_bus_get_provider(bus)) == 0) {
+			free_bus(bus, NULL);
+			free(path);
+			return 1;
+		}
+
  	list_add(&ctx->busses, &bus->list);

  	rc = 0;

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Dan Williams]

On Tue, Aug 6, 2019 at 9:17 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> On 8/7/19 9:43 AM, Dan Williams wrote:
> > On Tue, Aug 6, 2019 at 9:00 PM Aneesh Kumar K.V
> > <aneesh.kumar@linux.ibm.com> wrote:
> >>
> >> ndctl utility requires the ndbus to have unique names. If not while
> >> enumerating the bus in userspace it drops bus with similar names.
> >> This results in us not listing devices beneath the bus.
> >
> > It does?
> >
> >>
> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> >> ---
> >>   drivers/nvdimm/of_pmem.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> >> index a0c8dcfa0bf9..97187d6c0bdb 100644
> >> --- a/drivers/nvdimm/of_pmem.c
> >> +++ b/drivers/nvdimm/of_pmem.c
> >> @@ -42,7 +42,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
> >>                  return -ENOMEM;
> >>
> >>          priv->bus_desc.attr_groups = bus_attr_groups;
> >> -       priv->bus_desc.provider_name = "of_pmem";
> >> +       priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
> >
> > This looks ok to me to address support for older ndctl binaries, but
> > I'd like to also fix the ndctl bug that makes non-unique provider
> > names fail.
> >
>
> 0462269ab121d323a016874ebdd42217f2911ee7 (ndctl: provide a method to
> invalidate the bus list)
>
> This hunk does the filtering.
>
> @@ -928,6 +929,14 @@ static int add_bus(void *parent, int id, const char
> *ctl_base)
>                 goto err_read;
>         bus->buf_len = strlen(bus->bus_path) + 50;
>
> +       ndctl_bus_foreach(ctx, bus_dup)
> +               if (strcmp(ndctl_bus_get_provider(bus_dup),
> +                                       ndctl_bus_get_provider(bus)) == 0) {
> +                       free_bus(bus, NULL);
> +                       free(path);
> +                       return 1;
> +               }
> +

Yup, that's broken, does this incremental fix work?

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 4d9cc7e29c6b..6596f94edef8 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -889,7 +889,9 @@ static void *add_bus(void *parent, int id, const
char *ctl_base)

        ndctl_bus_foreach(ctx, bus_dup)
                if (strcmp(ndctl_bus_get_provider(bus_dup),
-                                       ndctl_bus_get_provider(bus)) == 0) {
+                                       ndctl_bus_get_provider(bus)) == 0
+                               && strcmp(ndctl_bus_get_devname(bus_dup),
+                                       ndctl_bus_get_devname(bus)) == 0) {
                        free_bus(bus, NULL);
                        free(path);
                        return bus_dup;
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Dan Williams]

On Tue, Aug 6, 2019 at 9:17 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> On 8/7/19 9:43 AM, Dan Williams wrote:
> > On Tue, Aug 6, 2019 at 9:00 PM Aneesh Kumar K.V
> > <aneesh.kumar@linux.ibm.com> wrote:
> >>
> >> ndctl utility requires the ndbus to have unique names. If not while
> >> enumerating the bus in userspace it drops bus with similar names.
> >> This results in us not listing devices beneath the bus.
> >
> > It does?
> >
> >>
> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> >> ---
> >>   drivers/nvdimm/of_pmem.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> >> index a0c8dcfa0bf9..97187d6c0bdb 100644
> >> --- a/drivers/nvdimm/of_pmem.c
> >> +++ b/drivers/nvdimm/of_pmem.c
> >> @@ -42,7 +42,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
> >>                  return -ENOMEM;
> >>
> >>          priv->bus_desc.attr_groups = bus_attr_groups;
> >> -       priv->bus_desc.provider_name = "of_pmem";
> >> +       priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
> >
> > This looks ok to me to address support for older ndctl binaries, but
> > I'd like to also fix the ndctl bug that makes non-unique provider
> > names fail.
> >
>
> 0462269ab121d323a016874ebdd42217f2911ee7 (ndctl: provide a method to
> invalidate the bus list)
>
> This hunk does the filtering.
>
> @@ -928,6 +929,14 @@ static int add_bus(void *parent, int id, const char
> *ctl_base)
>                 goto err_read;
>         bus->buf_len = strlen(bus->bus_path) + 50;
>
> +       ndctl_bus_foreach(ctx, bus_dup)
> +               if (strcmp(ndctl_bus_get_provider(bus_dup),
> +                                       ndctl_bus_get_provider(bus)) == 0) {
> +                       free_bus(bus, NULL);
> +                       free(path);
> +                       return 1;
> +               }
> +

Yup, that's broken, does this incremental fix work?

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 4d9cc7e29c6b..6596f94edef8 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -889,7 +889,9 @@ static void *add_bus(void *parent, int id, const
char *ctl_base)

        ndctl_bus_foreach(ctx, bus_dup)
                if (strcmp(ndctl_bus_get_provider(bus_dup),
-                                       ndctl_bus_get_provider(bus)) == 0) {
+                                       ndctl_bus_get_provider(bus)) == 0
+                               && strcmp(ndctl_bus_get_devname(bus_dup),
+                                       ndctl_bus_get_devname(bus)) == 0) {
                        free_bus(bus, NULL);
                        free(path);
                        return bus_dup;

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Aneesh Kumar K.V]

On 8/7/19 10:22 AM, Dan Williams wrote:
> On Tue, Aug 6, 2019 at 9:17 PM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
>>
>> On 8/7/19 9:43 AM, Dan Williams wrote:
>>> On Tue, Aug 6, 2019 at 9:00 PM Aneesh Kumar K.V
>>> <aneesh.kumar@linux.ibm.com> wrote:
>>>>
>>>> ndctl utility requires the ndbus to have unique names. If not while
>>>> enumerating the bus in userspace it drops bus with similar names.
>>>> This results in us not listing devices beneath the bus.
>>>
>>> It does?
>>>
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>> ---
>>>>    drivers/nvdimm/of_pmem.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
>>>> index a0c8dcfa0bf9..97187d6c0bdb 100644
>>>> --- a/drivers/nvdimm/of_pmem.c
>>>> +++ b/drivers/nvdimm/of_pmem.c
>>>> @@ -42,7 +42,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
>>>>                   return -ENOMEM;
>>>>
>>>>           priv->bus_desc.attr_groups = bus_attr_groups;
>>>> -       priv->bus_desc.provider_name = "of_pmem";
>>>> +       priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
>>>
>>> This looks ok to me to address support for older ndctl binaries, but
>>> I'd like to also fix the ndctl bug that makes non-unique provider
>>> names fail.
>>>
>>
>> 0462269ab121d323a016874ebdd42217f2911ee7 (ndctl: provide a method to
>> invalidate the bus list)
>>
>> This hunk does the filtering.
>>
>> @@ -928,6 +929,14 @@ static int add_bus(void *parent, int id, const char
>> *ctl_base)
>>                  goto err_read;
>>          bus->buf_len = strlen(bus->bus_path) + 50;
>>
>> +       ndctl_bus_foreach(ctx, bus_dup)
>> +               if (strcmp(ndctl_bus_get_provider(bus_dup),
>> +                                       ndctl_bus_get_provider(bus)) == 0) {
>> +                       free_bus(bus, NULL);
>> +                       free(path);
>> +                       return 1;
>> +               }
>> +
> 
> Yup, that's broken, does this incremental fix work?
> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index 4d9cc7e29c6b..6596f94edef8 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -889,7 +889,9 @@ static void *add_bus(void *parent, int id, const
> char *ctl_base)
> 
>          ndctl_bus_foreach(ctx, bus_dup)
>                  if (strcmp(ndctl_bus_get_provider(bus_dup),
> -                                       ndctl_bus_get_provider(bus)) == 0) {
> +                                       ndctl_bus_get_provider(bus)) == 0
> +                               && strcmp(ndctl_bus_get_devname(bus_dup),
> +                                       ndctl_bus_get_devname(bus)) == 0) {
>                          free_bus(bus, NULL);
>                          free(path);
>                          return bus_dup;
> 

That worked.

-aneesh
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Aneesh Kumar K.V]

On 8/7/19 10:22 AM, Dan Williams wrote:
> On Tue, Aug 6, 2019 at 9:17 PM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
>>
>> On 8/7/19 9:43 AM, Dan Williams wrote:
>>> On Tue, Aug 6, 2019 at 9:00 PM Aneesh Kumar K.V
>>> <aneesh.kumar@linux.ibm.com> wrote:
>>>>
>>>> ndctl utility requires the ndbus to have unique names. If not while
>>>> enumerating the bus in userspace it drops bus with similar names.
>>>> This results in us not listing devices beneath the bus.
>>>
>>> It does?
>>>
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>> ---
>>>>    drivers/nvdimm/of_pmem.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
>>>> index a0c8dcfa0bf9..97187d6c0bdb 100644
>>>> --- a/drivers/nvdimm/of_pmem.c
>>>> +++ b/drivers/nvdimm/of_pmem.c
>>>> @@ -42,7 +42,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
>>>>                   return -ENOMEM;
>>>>
>>>>           priv->bus_desc.attr_groups = bus_attr_groups;
>>>> -       priv->bus_desc.provider_name = "of_pmem";
>>>> +       priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
>>>
>>> This looks ok to me to address support for older ndctl binaries, but
>>> I'd like to also fix the ndctl bug that makes non-unique provider
>>> names fail.
>>>
>>
>> 0462269ab121d323a016874ebdd42217f2911ee7 (ndctl: provide a method to
>> invalidate the bus list)
>>
>> This hunk does the filtering.
>>
>> @@ -928,6 +929,14 @@ static int add_bus(void *parent, int id, const char
>> *ctl_base)
>>                  goto err_read;
>>          bus->buf_len = strlen(bus->bus_path) + 50;
>>
>> +       ndctl_bus_foreach(ctx, bus_dup)
>> +               if (strcmp(ndctl_bus_get_provider(bus_dup),
>> +                                       ndctl_bus_get_provider(bus)) == 0) {
>> +                       free_bus(bus, NULL);
>> +                       free(path);
>> +                       return 1;
>> +               }
>> +
> 
> Yup, that's broken, does this incremental fix work?
> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index 4d9cc7e29c6b..6596f94edef8 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -889,7 +889,9 @@ static void *add_bus(void *parent, int id, const
> char *ctl_base)
> 
>          ndctl_bus_foreach(ctx, bus_dup)
>                  if (strcmp(ndctl_bus_get_provider(bus_dup),
> -                                       ndctl_bus_get_provider(bus)) == 0) {
> +                                       ndctl_bus_get_provider(bus)) == 0
> +                               && strcmp(ndctl_bus_get_devname(bus_dup),
> +                                       ndctl_bus_get_devname(bus)) == 0) {
>                          free_bus(bus, NULL);
>                          free(path);
>                          return bus_dup;
> 

That worked.

-aneesh

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Dan Williams]

On Tue, Aug 6, 2019 at 11:00 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> On 8/7/19 10:22 AM, Dan Williams wrote:
> > On Tue, Aug 6, 2019 at 9:17 PM Aneesh Kumar K.V
> > <aneesh.kumar@linux.ibm.com> wrote:
> >>
> >> On 8/7/19 9:43 AM, Dan Williams wrote:
> >>> On Tue, Aug 6, 2019 at 9:00 PM Aneesh Kumar K.V
> >>> <aneesh.kumar@linux.ibm.com> wrote:
> >>>>
> >>>> ndctl utility requires the ndbus to have unique names. If not while
> >>>> enumerating the bus in userspace it drops bus with similar names.
> >>>> This results in us not listing devices beneath the bus.
> >>>
> >>> It does?
> >>>
> >>>>
> >>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> >>>> ---
> >>>>    drivers/nvdimm/of_pmem.c | 2 +-
> >>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> >>>> index a0c8dcfa0bf9..97187d6c0bdb 100644
> >>>> --- a/drivers/nvdimm/of_pmem.c
> >>>> +++ b/drivers/nvdimm/of_pmem.c
> >>>> @@ -42,7 +42,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
> >>>>                   return -ENOMEM;
> >>>>
> >>>>           priv->bus_desc.attr_groups = bus_attr_groups;
> >>>> -       priv->bus_desc.provider_name = "of_pmem";
> >>>> +       priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
> >>>
> >>> This looks ok to me to address support for older ndctl binaries, but
> >>> I'd like to also fix the ndctl bug that makes non-unique provider
> >>> names fail.
> >>>
> >>
> >> 0462269ab121d323a016874ebdd42217f2911ee7 (ndctl: provide a method to
> >> invalidate the bus list)
> >>
> >> This hunk does the filtering.
> >>
> >> @@ -928,6 +929,14 @@ static int add_bus(void *parent, int id, const char
> >> *ctl_base)
> >>                  goto err_read;
> >>          bus->buf_len = strlen(bus->bus_path) + 50;
> >>
> >> +       ndctl_bus_foreach(ctx, bus_dup)
> >> +               if (strcmp(ndctl_bus_get_provider(bus_dup),
> >> +                                       ndctl_bus_get_provider(bus)) == 0) {
> >> +                       free_bus(bus, NULL);
> >> +                       free(path);
> >> +                       return 1;
> >> +               }
> >> +
> >
> > Yup, that's broken, does this incremental fix work?
> >
> > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> > index 4d9cc7e29c6b..6596f94edef8 100644
> > --- a/ndctl/lib/libndctl.c
> > +++ b/ndctl/lib/libndctl.c
> > @@ -889,7 +889,9 @@ static void *add_bus(void *parent, int id, const
> > char *ctl_base)
> >
> >          ndctl_bus_foreach(ctx, bus_dup)
> >                  if (strcmp(ndctl_bus_get_provider(bus_dup),
> > -                                       ndctl_bus_get_provider(bus)) == 0) {
> > +                                       ndctl_bus_get_provider(bus)) == 0
> > +                               && strcmp(ndctl_bus_get_devname(bus_dup),
> > +                                       ndctl_bus_get_devname(bus)) == 0) {
> >                          free_bus(bus, NULL);
> >                          free(path);
> >                          return bus_dup;
> >
>
> That worked.

Great. I'll make a formal patch, and I'll amend the changelog of the
proposed kernel change to say "older ndctl binaries mistakenly
require"
>
> -aneesh
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Dan Williams]

On Tue, Aug 6, 2019 at 11:00 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> On 8/7/19 10:22 AM, Dan Williams wrote:
> > On Tue, Aug 6, 2019 at 9:17 PM Aneesh Kumar K.V
> > <aneesh.kumar@linux.ibm.com> wrote:
> >>
> >> On 8/7/19 9:43 AM, Dan Williams wrote:
> >>> On Tue, Aug 6, 2019 at 9:00 PM Aneesh Kumar K.V
> >>> <aneesh.kumar@linux.ibm.com> wrote:
> >>>>
> >>>> ndctl utility requires the ndbus to have unique names. If not while
> >>>> enumerating the bus in userspace it drops bus with similar names.
> >>>> This results in us not listing devices beneath the bus.
> >>>
> >>> It does?
> >>>
> >>>>
> >>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> >>>> ---
> >>>>    drivers/nvdimm/of_pmem.c | 2 +-
> >>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> >>>> index a0c8dcfa0bf9..97187d6c0bdb 100644
> >>>> --- a/drivers/nvdimm/of_pmem.c
> >>>> +++ b/drivers/nvdimm/of_pmem.c
> >>>> @@ -42,7 +42,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
> >>>>                   return -ENOMEM;
> >>>>
> >>>>           priv->bus_desc.attr_groups = bus_attr_groups;
> >>>> -       priv->bus_desc.provider_name = "of_pmem";
> >>>> +       priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
> >>>
> >>> This looks ok to me to address support for older ndctl binaries, but
> >>> I'd like to also fix the ndctl bug that makes non-unique provider
> >>> names fail.
> >>>
> >>
> >> 0462269ab121d323a016874ebdd42217f2911ee7 (ndctl: provide a method to
> >> invalidate the bus list)
> >>
> >> This hunk does the filtering.
> >>
> >> @@ -928,6 +929,14 @@ static int add_bus(void *parent, int id, const char
> >> *ctl_base)
> >>                  goto err_read;
> >>          bus->buf_len = strlen(bus->bus_path) + 50;
> >>
> >> +       ndctl_bus_foreach(ctx, bus_dup)
> >> +               if (strcmp(ndctl_bus_get_provider(bus_dup),
> >> +                                       ndctl_bus_get_provider(bus)) == 0) {
> >> +                       free_bus(bus, NULL);
> >> +                       free(path);
> >> +                       return 1;
> >> +               }
> >> +
> >
> > Yup, that's broken, does this incremental fix work?
> >
> > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> > index 4d9cc7e29c6b..6596f94edef8 100644
> > --- a/ndctl/lib/libndctl.c
> > +++ b/ndctl/lib/libndctl.c
> > @@ -889,7 +889,9 @@ static void *add_bus(void *parent, int id, const
> > char *ctl_base)
> >
> >          ndctl_bus_foreach(ctx, bus_dup)
> >                  if (strcmp(ndctl_bus_get_provider(bus_dup),
> > -                                       ndctl_bus_get_provider(bus)) == 0) {
> > +                                       ndctl_bus_get_provider(bus)) == 0
> > +                               && strcmp(ndctl_bus_get_devname(bus_dup),
> > +                                       ndctl_bus_get_devname(bus)) == 0) {
> >                          free_bus(bus, NULL);
> >                          free(path);
> >                          return bus_dup;
> >
>
> That worked.

Great. I'll make a formal patch, and I'll amend the changelog of the
proposed kernel change to say "older ndctl binaries mistakenly
require"
>
> -aneesh

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Vaibhav Jain]

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:

> ndctl utility requires the ndbus to have unique names. If not while
> enumerating the bus in userspace it drops bus with similar names.
> This results in us not listing devices beneath the bus.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  drivers/nvdimm/of_pmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> index a0c8dcfa0bf9..97187d6c0bdb 100644
> --- a/drivers/nvdimm/of_pmem.c
> +++ b/drivers/nvdimm/of_pmem.c
> @@ -42,7 +42,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	priv->bus_desc.attr_groups = bus_attr_groups;
> -	priv->bus_desc.provider_name = "of_pmem";
> +	priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
>  	priv->bus_desc.module = THIS_MODULE;
>  	priv->bus_desc.of_node = np;
>  
> -- 
> 2.21.0
>

Tested-by: Vaibhav Jain <vaibhav@linux.ibm.com>
-- 
Vaibhav Jain <vaibhav@linux.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Vaibhav Jain]

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:

> ndctl utility requires the ndbus to have unique names. If not while
> enumerating the bus in userspace it drops bus with similar names.
> This results in us not listing devices beneath the bus.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  drivers/nvdimm/of_pmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> index a0c8dcfa0bf9..97187d6c0bdb 100644
> --- a/drivers/nvdimm/of_pmem.c
> +++ b/drivers/nvdimm/of_pmem.c
> @@ -42,7 +42,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	priv->bus_desc.attr_groups = bus_attr_groups;
> -	priv->bus_desc.provider_name = "of_pmem";
> +	priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
>  	priv->bus_desc.module = THIS_MODULE;
>  	priv->bus_desc.of_node = np;
>  
> -- 
> 2.21.0
>

Tested-by: Vaibhav Jain <vaibhav@linux.ibm.com>
-- 
Vaibhav Jain <vaibhav@linux.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Aneesh Kumar K.V]

patchwork-bot+linux-nvdimm@kernel.org writes:

> Hello:
>
> This patch was applied to nvdimm/nvdimm.git (refs/heads/libnvdimm-for-next).
>
> On Wed,  7 Aug 2019 09:30:29 +0530 you wrote:
>> ndctl utility requires the ndbus to have unique names. If not while
>> enumerating the bus in userspace it drops bus with similar names.
>> This results in us not listing devices beneath the bus.
>> 
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>  drivers/nvdimm/of_pmem.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
>
> Here is a summary with links:
>   - nvdimm/of_pmem: Provide a unique name for bus provider
>     https://git.kernel.org/nvdimm/nvdimm/c/49bddc73d15c25a68e4294d76fc74519fda984cd
>
> You are awesome, thank you!

We decided to fix this in ndctl tool? If we go with ndctl fix, we
can drop the kernel change. Parallely I am planning to do a  fix
for papr_scm driver that will update the provider name as "papr_scm". That
way we can find out the backend driver using

:~> ndctl list -B
[
  {
    "provider":"papr_scm",
    "dev":"ndbus1"
  },
  {
    "provider":"of_pmem",
    "dev":"ndbus0"
  }
]


-aneesh

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Dan Williams]

On Thu, Aug 29, 2019 at 9:31 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> patchwork-bot+linux-nvdimm@kernel.org writes:
>
> > Hello:
> >
> > This patch was applied to nvdimm/nvdimm.git (refs/heads/libnvdimm-for-next).
> >
> > On Wed,  7 Aug 2019 09:30:29 +0530 you wrote:
> >> ndctl utility requires the ndbus to have unique names. If not while
> >> enumerating the bus in userspace it drops bus with similar names.
> >> This results in us not listing devices beneath the bus.
> >>
> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> >> ---
> >>  drivers/nvdimm/of_pmem.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >
> > Here is a summary with links:
> >   - nvdimm/of_pmem: Provide a unique name for bus provider
> >     https://git.kernel.org/nvdimm/nvdimm/c/49bddc73d15c25a68e4294d76fc74519fda984cd
> >
> > You are awesome, thank you!
>
> We decided to fix this in ndctl tool? If we go with ndctl fix, we
> can drop the kernel change.

Oh, I was planning to do both any concerns if I keep the kernel
change, otherwise I'll need to rebase the branch.

> Parallely I am planning to do a  fix
> for papr_scm driver that will update the provider name as "papr_scm". That
> way we can find out the backend driver using
>
> :~> ndctl list -B
> [
>   {
>     "provider":"papr_scm",
>     "dev":"ndbus1"
>   },
>   {
>     "provider":"of_pmem",
>     "dev":"ndbus0"
>   }
> ]
>
>
> -aneesh
>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Aneesh Kumar K.V]

On 8/30/19 10:10 AM, Dan Williams wrote:
> On Thu, Aug 29, 2019 at 9:31 PM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
>>
>> patchwork-bot+linux-nvdimm@kernel.org writes:
>>
>>> Hello:
>>>
>>> This patch was applied to nvdimm/nvdimm.git (refs/heads/libnvdimm-for-next).
>>>
>>> On Wed,  7 Aug 2019 09:30:29 +0530 you wrote:
>>>> ndctl utility requires the ndbus to have unique names. If not while
>>>> enumerating the bus in userspace it drops bus with similar names.
>>>> This results in us not listing devices beneath the bus.
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>> ---
>>>>   drivers/nvdimm/of_pmem.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>>
>>> Here is a summary with links:
>>>    - nvdimm/of_pmem: Provide a unique name for bus provider
>>>      https://git.kernel.org/nvdimm/nvdimm/c/49bddc73d15c25a68e4294d76fc74519fda984cd
>>>
>>> You are awesome, thank you!
>>
>> We decided to fix this in ndctl tool? If we go with ndctl fix, we
>> can drop the kernel change.
> 
> Oh, I was planning to do both any concerns if I keep the kernel
> change, otherwise I'll need to rebase the branch.
> 

I guess rebasing is not going to be nice. So we can keep the patch and 
if we are really need to move the provider name to indicate backend 
driver, I will fixup both of_pmem and papr_scm together.

Thanks.
-aneesh
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm/of_pmem: Provide a unique name for bus provider

[Dan Williams]

On Thu, Aug 29, 2019 at 9:45 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> On 8/30/19 10:10 AM, Dan Williams wrote:
> > On Thu, Aug 29, 2019 at 9:31 PM Aneesh Kumar K.V
> > <aneesh.kumar@linux.ibm.com> wrote:
> >>
> >> patchwork-bot+linux-nvdimm@kernel.org writes:
> >>
> >>> Hello:
> >>>
> >>> This patch was applied to nvdimm/nvdimm.git (refs/heads/libnvdimm-for-next).
> >>>
> >>> On Wed,  7 Aug 2019 09:30:29 +0530 you wrote:
> >>>> ndctl utility requires the ndbus to have unique names. If not while
> >>>> enumerating the bus in userspace it drops bus with similar names.
> >>>> This results in us not listing devices beneath the bus.
> >>>>
> >>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> >>>> ---
> >>>>   drivers/nvdimm/of_pmem.c | 2 +-
> >>>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>>
> >>> Here is a summary with links:
> >>>    - nvdimm/of_pmem: Provide a unique name for bus provider
> >>>      https://git.kernel.org/nvdimm/nvdimm/c/49bddc73d15c25a68e4294d76fc74519fda984cd
> >>>
> >>> You are awesome, thank you!
> >>
> >> We decided to fix this in ndctl tool? If we go with ndctl fix, we
> >> can drop the kernel change.
> >
> > Oh, I was planning to do both any concerns if I keep the kernel
> > change, otherwise I'll need to rebase the branch.
> >
>
> I guess rebasing is not going to be nice. So we can keep the patch and
> if we are really need to move the provider name to indicate backend
> driver, I will fixup both of_pmem and papr_scm together.

Another reason to make the kernel change is to improve compatibility
with older ndctl releases.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm