[ndctl PATCH] libndctl: Fix probe of non-nfit nvdimms

[ndctl PATCH] libndctl: Fix probe of non-nfit nvdimms

[Vaibhav Jain]

commit 107a24ff429f ("ndctl/list: Add firmware activation
enumeration") introduced changes in add_dimm() to enumerate the status
of firmware activation. However a branch added in that commit broke
the probe for non-nfit nvdimms like one provided by papr-scm. This
cause an error reported when listing namespaces like below:

$ sudo ndctl list
libndctl: add_dimm: nmem0: probe failed: No such device
libndctl: __sysfs_device_parse: nmem0: add_dev() failed

Do a fix for this by removing the offending branch in the add_dimm()
patch. This continues the flow of add_dimm() probe even if the nfit is
not detected on the associated bus.

Fixes: 107a24ff429fa("ndctl/list: Add firmware activation enumeration")
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
 ndctl/lib/libndctl.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 554696386f48..ad521d365ee4 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -1875,9 +1875,6 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
 	else
 		dimm->fwa_result = fwa_result_to_result(buf);
 
-	if (!ndctl_bus_has_nfit(bus))
-		goto out;
-
 	/* Check if the given dimm supports nfit */
 	if (ndctl_bus_has_nfit(bus)) {
 		dimm->formats = formats;
-- 
2.26.2
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH] libndctl: Fix probe of non-nfit nvdimms

[Verma, Vishal L]

On Fri, 2020-10-09 at 17:30 +0530, Vaibhav Jain wrote:
> commit 107a24ff429f ("ndctl/list: Add firmware activation
> enumeration") introduced changes in add_dimm() to enumerate the status
> of firmware activation. However a branch added in that commit broke
> the probe for non-nfit nvdimms like one provided by papr-scm. This
> cause an error reported when listing namespaces like below:
> 
> $ sudo ndctl list
> libndctl: add_dimm: nmem0: probe failed: No such device
> libndctl: __sysfs_device_parse: nmem0: add_dev() failed
> 
> Do a fix for this by removing the offending branch in the add_dimm()
> patch. This continues the flow of add_dimm() probe even if the nfit is
> not detected on the associated bus.
> 
> Fixes: 107a24ff429fa("ndctl/list: Add firmware activation enumeration")
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
>  ndctl/lib/libndctl.c | 3 ---
>  1 file changed, 3 deletions(-)

Ah apologies - this snuck in when I reflowed Dan's patches on top of the
papr work for v70.

I expect you'd like a point release with this fix asap?

Is there a way for me to incorporate some papr unit tests into my
release workflow so I can avoid breaking things like this again?

I'll also try to do a better job of pushing things out to the pending
branch more frequently so if you're monitoring that branch, hopefully
things like this will get caught before a release happens :)

> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index 554696386f48..ad521d365ee4 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -1875,9 +1875,6 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
>  	else
>  		dimm->fwa_result = fwa_result_to_result(buf);
>  
> -	if (!ndctl_bus_has_nfit(bus))
> -		goto out;
> -
>  	/* Check if the given dimm supports nfit */
>  	if (ndctl_bus_has_nfit(bus)) {
>  		dimm->formats = formats;

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH] libndctl: Fix probe of non-nfit nvdimms

[Dan Williams]

On Fri, Oct 9, 2020 at 11:36 AM Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
>
> On Fri, 2020-10-09 at 17:30 +0530, Vaibhav Jain wrote:
> > commit 107a24ff429f ("ndctl/list: Add firmware activation
> > enumeration") introduced changes in add_dimm() to enumerate the status
> > of firmware activation. However a branch added in that commit broke
> > the probe for non-nfit nvdimms like one provided by papr-scm. This
> > cause an error reported when listing namespaces like below:
> >
> > $ sudo ndctl list
> > libndctl: add_dimm: nmem0: probe failed: No such device
> > libndctl: __sysfs_device_parse: nmem0: add_dev() failed
> >
> > Do a fix for this by removing the offending branch in the add_dimm()
> > patch. This continues the flow of add_dimm() probe even if the nfit is
> > not detected on the associated bus.
> >
> > Fixes: 107a24ff429fa("ndctl/list: Add firmware activation enumeration")
> > Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> > ---
> >  ndctl/lib/libndctl.c | 3 ---
> >  1 file changed, 3 deletions(-)
>
> Ah apologies - this snuck in when I reflowed Dan's patches on top of the
> papr work for v70.
>
> I expect you'd like a point release with this fix asap?
>
> Is there a way for me to incorporate some papr unit tests into my
> release workflow so I can avoid breaking things like this again?
>
> I'll also try to do a better job of pushing things out to the pending
> branch more frequently so if you're monitoring that branch, hopefully
> things like this will get caught before a release happens :)

Would be nice to have something like a papr_test next to nfit_test for
such regression testing. These kinds of mistakes are really only
avoidable with regression tests.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH] libndctl: Fix probe of non-nfit nvdimms

[Vaibhav Jain]

Hi Dan and Vishal,

Thanks so much for quick turnaround on this.

Dan Williams <dan.j.williams@intel.com> writes:

> On Fri, Oct 9, 2020 at 11:36 AM Verma, Vishal L
> <vishal.l.verma@intel.com> wrote:
>>
>> On Fri, 2020-10-09 at 17:30 +0530, Vaibhav Jain wrote:
>> > commit 107a24ff429f ("ndctl/list: Add firmware activation
>> > enumeration") introduced changes in add_dimm() to enumerate the status
>> > of firmware activation. However a branch added in that commit broke
>> > the probe for non-nfit nvdimms like one provided by papr-scm. This
>> > cause an error reported when listing namespaces like below:
>> >
>> > $ sudo ndctl list
>> > libndctl: add_dimm: nmem0: probe failed: No such device
>> > libndctl: __sysfs_device_parse: nmem0: add_dev() failed
>> >
>> > Do a fix for this by removing the offending branch in the add_dimm()
>> > patch. This continues the flow of add_dimm() probe even if the nfit is
>> > not detected on the associated bus.
>> >
>> > Fixes: 107a24ff429fa("ndctl/list: Add firmware activation enumeration")
>> > Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> > ---
>> >  ndctl/lib/libndctl.c | 3 ---
>> >  1 file changed, 3 deletions(-)
>>
>> Ah apologies - this snuck in when I reflowed Dan's patches on top of the
>> papr work for v70.
No worries :-)

>>
>> I expect you'd like a point release with this fix asap?
Yes, that will be great. Thanks

>>
>> Is there a way for me to incorporate some papr unit tests into my
>> release workflow so I can avoid breaking things like this again?
>>
>> I'll also try to do a better job of pushing things out to the pending
>> branch more frequently so if you're monitoring that branch, hopefully
>> things like this will get caught before a release happens :)

Fully agree, if that happens we can incorporate it into our CI system to
ensure that such regressions are caught early on before any release is
tagged.

>
> Would be nice to have something like a papr_test next to nfit_test for
> such regression testing. These kinds of mistakes are really only
> avoidable with regression tests.
Yes Agree, fortunatly Santosh  has recently posted an RFC patchset
implementing such tests at [1]. Once that gets merged, can used to
perform regression testing.

[1] "testing/nvdimm: Add test module for non-nfit platforms"
https://lore.kernel.org/linux-nvdimm/20201006010013.848302-1-santosh@fossix.org/

-- 
Cheers
~ Vaibhav
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [ndctl PATCH] libndctl: Fix probe of non-nfit nvdimms

[Santosh Sivaraj]

Vaibhav Jain <vaibhav@linux.ibm.com> writes:

> Hi Dan and Vishal,
>
> Thanks so much for quick turnaround on this.
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
>> On Fri, Oct 9, 2020 at 11:36 AM Verma, Vishal L
>> <vishal.l.verma@intel.com> wrote:
>>>
>>> On Fri, 2020-10-09 at 17:30 +0530, Vaibhav Jain wrote:
>>> > commit 107a24ff429f ("ndctl/list: Add firmware activation
>>> > enumeration") introduced changes in add_dimm() to enumerate the status
>>> > of firmware activation. However a branch added in that commit broke
>>> > the probe for non-nfit nvdimms like one provided by papr-scm. This
>>> > cause an error reported when listing namespaces like below:
>>> >
>>> > $ sudo ndctl list
>>> > libndctl: add_dimm: nmem0: probe failed: No such device
>>> > libndctl: __sysfs_device_parse: nmem0: add_dev() failed
>>> >
>>> > Do a fix for this by removing the offending branch in the add_dimm()
>>> > patch. This continues the flow of add_dimm() probe even if the nfit is
>>> > not detected on the associated bus.
>>> >
>>> > Fixes: 107a24ff429fa("ndctl/list: Add firmware activation enumeration")
>>> > Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>>> > ---
>>> >  ndctl/lib/libndctl.c | 3 ---
>>> >  1 file changed, 3 deletions(-)
>>>
>>> Ah apologies - this snuck in when I reflowed Dan's patches on top of the
>>> papr work for v70.
> No worries :-)
>
>>>
>>> I expect you'd like a point release with this fix asap?
> Yes, that will be great. Thanks
>
>>>
>>> Is there a way for me to incorporate some papr unit tests into my
>>> release workflow so I can avoid breaking things like this again?
>>>
>>> I'll also try to do a better job of pushing things out to the pending
>>> branch more frequently so if you're monitoring that branch, hopefully
>>> things like this will get caught before a release happens :)
>
> Fully agree, if that happens we can incorporate it into our CI system to
> ensure that such regressions are caught early on before any release is
> tagged.
>
>>
>> Would be nice to have something like a papr_test next to nfit_test for
>> such regression testing. These kinds of mistakes are really only
>> avoidable with regression tests.
> Yes Agree, fortunatly Santosh  has recently posted an RFC patchset
> implementing such tests at [1]. Once that gets merged, can used to
> perform regression testing.
>
> [1] "testing/nvdimm: Add test module for non-nfit platforms"
> https://lore.kernel.org/linux-nvdimm/20201006010013.848302-1-santosh@fossix.org/
>

Thanks Vaibhav to point that out.

Dan/Vishal/Ira,

If you could provide your comments on the above RFC we could move forward on
this.

Thanks,
Santosh
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org