nvdimm/of_pmem: Provide a unique name for bus provider
diff mbox series

Message ID 20190807040029.11344-1-aneesh.kumar@linux.ibm.com
State New
Headers show
Series
  • nvdimm/of_pmem: Provide a unique name for bus provider
Related show

Commit Message

Aneesh Kumar K.V Aug. 7, 2019, 4 a.m. UTC
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(-)

Comments

Dan Williams Aug. 7, 2019, 4:13 a.m. UTC | #1
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.
Aneesh Kumar K.V Aug. 7, 2019, 4:17 a.m. UTC | #2
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;
Dan Williams Aug. 7, 2019, 4:52 a.m. UTC | #3
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;
Vaibhav Jain Aug. 7, 2019, 5:42 a.m. UTC | #4
"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>
Aneesh Kumar K.V Aug. 7, 2019, 6 a.m. UTC | #5
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
Dan Williams Aug. 7, 2019, 3:34 p.m. UTC | #6
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

Patch
diff mbox series

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;