[PATCH] ptp: fix an IS_ERR() vs NULL check

[PATCH] ptp: fix an IS_ERR() vs NULL check

[Dan Carpenter]

The pps_register_source() function doesn't return NULL, it returns
error pointers.

Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/ptp/ptp_clock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 8a81eecc0ecd..48f3594a7458 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -265,8 +265,8 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 		pps.mode = PTP_PPS_MODE;
 		pps.owner = info->owner;
 		ptp->pps_source = pps_register_source(&pps, PTP_PPS_DEFAULTS);
-		if (!ptp->pps_source) {
-			err = -EINVAL;
+		if (IS_ERR(ptp->pps_source)) {
+			err = PTR_ERR(ptp->pps_source);
 			pr_err("failed to register pps source\n");
 			goto no_pps;
 		}
-- 
2.11.0

Re: [PATCH] ptp: fix an IS_ERR() vs NULL check

[Richard Cochran]

On Fri, Nov 30, 2018 at 03:58:34PM +0300, Dan Carpenter wrote:
> The pps_register_source() function doesn't return NULL, it returns
> error pointers.

It keeps a local variable for errno, but then it returns NULL.
But this is about to change with this recent patch:

   26.Nov'18 YueHaibing [PATCH -next] pps: using ERR_PTR instead of NULL while pps_register_source fails

It hasn't been merged yet AFAICT, but gregkh said he take it.

Thanks,
Richard

Re: [PATCH] ptp: fix an IS_ERR() vs NULL check

[Dan Carpenter]

On Fri, Nov 30, 2018 at 08:32:52AM -0800, Richard Cochran wrote:
> On Fri, Nov 30, 2018 at 03:58:34PM +0300, Dan Carpenter wrote:
> > The pps_register_source() function doesn't return NULL, it returns
> > error pointers.
> 
> It keeps a local variable for errno, but then it returns NULL.
> But this is about to change with this recent patch:
> 
>    26.Nov'18 YueHaibing [PATCH -next] pps: using ERR_PTR instead of NULL while pps_register_source fails
> 
> It hasn't been merged yet AFAICT, but gregkh said he take it.

Oh, yeah.  That patch has actually been applied, but we didn't update
the caller.  I should have made the Fixes tag point to that YueHaibing's
patch.

regards,
dan carpenter

Re: [PATCH v2] ptp: fix an IS_ERR() vs NULL check

[Richard Cochran]

On Mon, Dec 03, 2018 at 01:55:06PM +0300, Dan Carpenter wrote:
> We recently modified pps_register_source() to return error pointers
> instead of NULL but this check wasn't updated.

But it *was* updated!
 
> Fixes: 3b1ad360acad ("pps: using ERR_PTR instead of NULL while pps_register_source fails")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2:  Use the correct Fixes tag.  Add Greg to the CC list, because he
> is maintaining this driver.

What driver?  (I am maintaining drivers/ptp/ptp_clock.c)
 
>  drivers/ptp/ptp_clock.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index 8a81eecc0ecd..48f3594a7458 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -265,8 +265,8 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
>  		pps.mode = PTP_PPS_MODE;
>  		pps.owner = info->owner;
>  		ptp->pps_source = pps_register_source(&pps, PTP_PPS_DEFAULTS);
> -		if (!ptp->pps_source) {
> -			err = -EINVAL;
> +		if (IS_ERR(ptp->pps_source)) {
> +			err = PTR_ERR(ptp->pps_source);
>  			pr_err("failed to register pps source\n");
>  			goto no_pps;
>  		}

YueHaibing's patch has the following hunk.  It really is already
there.  I don't see it yet on Linus' master branch, but the patch
should update drivers/pps/kapi.c and the callers all in one commit.

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 8a81eec..48f3594 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -265,8 +265,8 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 		pps.mode = PTP_PPS_MODE;
 		pps.owner = info->owner;
 		ptp->pps_source = pps_register_source(&pps, PTP_PPS_DEFAULTS);
-		if (!ptp->pps_source) {
-			err = -EINVAL;
+		if (IS_ERR(ptp->pps_source)) {
+			err = PTR_ERR(ptp->pps_source);
 			pr_err("failed to register pps source\n");
 			goto no_pps;
 		}

Thanks,
Richard

Re: [PATCH v2] ptp: fix an IS_ERR() vs NULL check

[Dan Carpenter]

Hi Greg,

I meant to CC you but I screwed up and added you to the From header
instead... :(

Why did you commit 3b1ad360acad ("pps: using ERR_PTR instead of NULL
while pps_register_source fails")?  You're not listed as a maintainer so
I wouldn't have known to CC you.

The back story is that the last chunk which changes drivers/ptp/ptp_clock.c
was dropped by mistake and it's not there in linux-next.  I wrote a
patch which adds it, but everyone is super confused now...  Here is
YueHaibing's original patch btw, for reference.
https://www.spinics.net/lists/netdev/msg535929.html

regards,
dan carpenter


On Mon, Dec 03, 2018 at 08:42:57PM -0800, Richard Cochran wrote:
> On Mon, Dec 03, 2018 at 01:55:06PM +0300, Dan Carpenter wrote:
> > We recently modified pps_register_source() to return error pointers
> > instead of NULL but this check wasn't updated.
> 
> But it *was* updated!
>  
> > Fixes: 3b1ad360acad ("pps: using ERR_PTR instead of NULL while pps_register_source fails")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > v2:  Use the correct Fixes tag.  Add Greg to the CC list, because he
> > is maintaining this driver.
> 
> What driver?  (I am maintaining drivers/ptp/ptp_clock.c)
>  
> >  drivers/ptp/ptp_clock.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> > index 8a81eecc0ecd..48f3594a7458 100644
> > --- a/drivers/ptp/ptp_clock.c
> > +++ b/drivers/ptp/ptp_clock.c
> > @@ -265,8 +265,8 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
> >  		pps.mode = PTP_PPS_MODE;
> >  		pps.owner = info->owner;
> >  		ptp->pps_source = pps_register_source(&pps, PTP_PPS_DEFAULTS);
> > -		if (!ptp->pps_source) {
> > -			err = -EINVAL;
> > +		if (IS_ERR(ptp->pps_source)) {
> > +			err = PTR_ERR(ptp->pps_source);
> >  			pr_err("failed to register pps source\n");
> >  			goto no_pps;
> >  		}
> 
> YueHaibing's patch has the following hunk.  It really is already
> there.  I don't see it yet on Linus' master branch, but the patch
> should update drivers/pps/kapi.c and the callers all in one commit.
> 
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index 8a81eec..48f3594 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -265,8 +265,8 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
>  		pps.mode = PTP_PPS_MODE;
>  		pps.owner = info->owner;
>  		ptp->pps_source = pps_register_source(&pps, PTP_PPS_DEFAULTS);
> -		if (!ptp->pps_source) {
> -			err = -EINVAL;
> +		if (IS_ERR(ptp->pps_source)) {
> +			err = PTR_ERR(ptp->pps_source);
>  			pr_err("failed to register pps source\n");
>  			goto no_pps;
>  		}
> 
> Thanks,
> Richard

Re: [PATCH v2] ptp: fix an IS_ERR() vs NULL check

[Greg Kroah-Hartman]

On Tue, Dec 04, 2018 at 10:00:33AM +0300, Dan Carpenter wrote:
> Hi Greg,
> 
> I meant to CC you but I screwed up and added you to the From header
> instead... :(
> 
> Why did you commit 3b1ad360acad ("pps: using ERR_PTR instead of NULL
> while pps_register_source fails")?  You're not listed as a maintainer so
> I wouldn't have known to CC you.

The maintainer told me to do so, see this email thread:
	https://lore.kernel.org/lkml/44edc27b-e679-24e8-aef1-fe5b7570f982@enneenne.com/

> The back story is that the last chunk which changes drivers/ptp/ptp_clock.c
> was dropped by mistake and it's not there in linux-next.  I wrote a
> patch which adds it, but everyone is super confused now...  Here is
> YueHaibing's original patch btw, for reference.
> https://www.spinics.net/lists/netdev/msg535929.html

If you want me to take a fixup patch for this, I will be glad to.  I can
also drop it from my tree as well, just let me know.

Sorry for the confusion.

greg k-h

Re: [PATCH v2] ptp: fix an IS_ERR() vs NULL check

[Richard Cochran]

On Tue, Dec 04, 2018 at 10:00:33AM +0300, Dan Carpenter wrote:
> 
> Why did you commit 3b1ad360acad ("pps: using ERR_PTR instead of NULL
> while pps_register_source fails")?  You're not listed as a maintainer so
> I wouldn't have known to CC you.

I'm confused.  Where is that commit?

   $ git show 3b1ad360acad
   fatal: ambiguous argument '3b1ad360acad': unknown revision or path not in the working tree.

I'm pulling from:

   torvalds git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git (fetch)
   stable   git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git (fetch)

(and also from net and net-next)

Thanks,
Richard

Re: [PATCH v2] ptp: fix an IS_ERR() vs NULL check

[Dan Carpenter]

On Tue, Dec 04, 2018 at 06:55:38AM -0800, Richard Cochran wrote:
> On Tue, Dec 04, 2018 at 10:00:33AM +0300, Dan Carpenter wrote:
> > 
> > Why did you commit 3b1ad360acad ("pps: using ERR_PTR instead of NULL
> > while pps_register_source fails")?  You're not listed as a maintainer so
> > I wouldn't have known to CC you.
> 
> I'm confused.  Where is that commit?
> 
>    $ git show 3b1ad360acad
>    fatal: ambiguous argument '3b1ad360acad': unknown revision or path not in the working tree.
> 
> I'm pulling from:
> 
>    torvalds git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git (fetch)
>    stable   git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git (fetch)
> 
> (and also from net and net-next)
> 

I'm on linux-next but originally it came from the char-misc-next tree.

git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git char-misc-next

regards,
dan carpenter

Re: [PATCH v2] ptp: fix an IS_ERR() vs NULL check

[Richard Cochran]

Greg,

On Tue, Dec 04, 2018 at 06:10:49PM +0300, Dan Carpenter wrote:
> On Tue, Dec 04, 2018 at 06:55:38AM -0800, Richard Cochran wrote:
> > On Tue, Dec 04, 2018 at 10:00:33AM +0300, Dan Carpenter wrote:
> > > 
> > > Why did you commit 3b1ad360acad ("pps: using ERR_PTR instead of NULL
> > > while pps_register_source fails")?  You're not listed as a maintainer so
> > > I wouldn't have known to CC you.
> > 
> > I'm confused.  Where is that commit?

...

> I'm on linux-next but originally it came from the char-misc-next tree.
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git char-misc-next

Commit 3b1ad360acad changes a return value in the PPS core, but it is
missing the hunk for the caller in drivers/ptp/ptp_clock.c.  Can that
be amended?

Thanks,
Richard

Re: [PATCH v2] ptp: fix an IS_ERR() vs NULL check

[Greg Kroah-Hartman]

On Thu, Dec 06, 2018 at 04:38:43AM -0800, Richard Cochran wrote:
> Greg,
> 
> On Tue, Dec 04, 2018 at 06:10:49PM +0300, Dan Carpenter wrote:
> > On Tue, Dec 04, 2018 at 06:55:38AM -0800, Richard Cochran wrote:
> > > On Tue, Dec 04, 2018 at 10:00:33AM +0300, Dan Carpenter wrote:
> > > > 
> > > > Why did you commit 3b1ad360acad ("pps: using ERR_PTR instead of NULL
> > > > while pps_register_source fails")?  You're not listed as a maintainer so
> > > > I wouldn't have known to CC you.
> > > 
> > > I'm confused.  Where is that commit?
> 
> ...
> 
> > I'm on linux-next but originally it came from the char-misc-next tree.
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git char-misc-next
> 
> Commit 3b1ad360acad changes a return value in the PPS core, but it is
> missing the hunk for the caller in drivers/ptp/ptp_clock.c.  Can that
> be amended?

Send me a patch or tell me what I need to revert and I will be glad to
do so :)

thanks,

greg k-h

[PATCH v3] ptp: fix an IS_ERR() vs NULL check

[Dan Carpenter]

We recently modified pps_register_source() to return error pointers
instead of NULL but it seems like there was a merge issue and part of
the commit was lost.  Anyway, the ptp_clock_register() function needs to
be updated to check for IS_ERR() as well.

Fixes: 3b1ad360acad ("pps: using ERR_PTR instead of NULL while pps_register_source fails")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2:  Use the correct Fixes tag.
v3:  Add Greg to the CC list for real this time.

 drivers/ptp/ptp_clock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 8a81eecc0ecd..48f3594a7458 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -265,8 +265,8 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 		pps.mode = PTP_PPS_MODE;
 		pps.owner = info->owner;
 		ptp->pps_source = pps_register_source(&pps, PTP_PPS_DEFAULTS);
-		if (!ptp->pps_source) {
-			err = -EINVAL;
+		if (IS_ERR(ptp->pps_source)) {
+			err = PTR_ERR(ptp->pps_source);
 			pr_err("failed to register pps source\n");
 			goto no_pps;
 		}
-- 
2.11.0

Re: [PATCH v3] ptp: fix an IS_ERR() vs NULL check

[Richard Cochran]

On Fri, Dec 07, 2018 at 09:00:46AM +0300, Dan Carpenter wrote:
> We recently modified pps_register_source() to return error pointers
> instead of NULL but it seems like there was a merge issue and part of
> the commit was lost.  Anyway, the ptp_clock_register() function needs to
> be updated to check for IS_ERR() as well.
> 
> Fixes: 3b1ad360acad ("pps: using ERR_PTR instead of NULL while pps_register_source fails")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Greg,

This patch provides the missing hunk.  Please apply it.

Thanks,
Richard