LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH] ptp: fix an IS_ERR() vs NULL check
@ 2018-11-30 12:58 Dan Carpenter
  2018-11-30 16:32 ` Richard Cochran
  0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2018-11-30 12:58 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev, linux-kernel, kernel-janitors

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ptp: fix an IS_ERR() vs NULL check
  2018-11-30 12:58 [PATCH] ptp: fix an IS_ERR() vs NULL check Dan Carpenter
@ 2018-11-30 16:32 ` Richard Cochran
  2018-12-03 10:04   ` Dan Carpenter
       [not found]   ` <20181203105506.GA21127@unbuntlaptop>
  0 siblings, 2 replies; 12+ messages in thread
From: Richard Cochran @ 2018-11-30 16:32 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: netdev, linux-kernel, kernel-janitors

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ptp: fix an IS_ERR() vs NULL check
  2018-11-30 16:32 ` Richard Cochran
@ 2018-12-03 10:04   ` Dan Carpenter
       [not found]   ` <20181203105506.GA21127@unbuntlaptop>
  1 sibling, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2018-12-03 10:04 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev, linux-kernel, kernel-janitors

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




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] ptp: fix an IS_ERR() vs NULL check
       [not found]   ` <20181203105506.GA21127@unbuntlaptop>
@ 2018-12-04  4:42     ` " Richard Cochran
  2018-12-04  7:00       ` Dan Carpenter
  0 siblings, 1 reply; 12+ messages in thread
From: Richard Cochran @ 2018-12-04  4:42 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: netdev, linux-kernel, kernel-janitors

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] ptp: fix an IS_ERR() vs NULL check
  2018-12-04  4:42     ` [PATCH v2] " Richard Cochran
@ 2018-12-04  7:00       ` Dan Carpenter
  2018-12-04 10:54         ` Greg Kroah-Hartman
  2018-12-04 14:55         ` Richard Cochran
  0 siblings, 2 replies; 12+ messages in thread
From: Dan Carpenter @ 2018-12-04  7:00 UTC (permalink / raw)
  To: Richard Cochran, Greg Kroah-Hartman
  Cc: netdev, linux-kernel, kernel-janitors, YueHaibing

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] ptp: fix an IS_ERR() vs NULL check
  2018-12-04  7:00       ` Dan Carpenter
@ 2018-12-04 10:54         ` Greg Kroah-Hartman
  2018-12-04 14:55         ` Richard Cochran
  1 sibling, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2018-12-04 10:54 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Richard Cochran, netdev, linux-kernel, kernel-janitors, YueHaibing

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] ptp: fix an IS_ERR() vs NULL check
  2018-12-04  7:00       ` Dan Carpenter
  2018-12-04 10:54         ` Greg Kroah-Hartman
@ 2018-12-04 14:55         ` Richard Cochran
  2018-12-04 15:10           ` Dan Carpenter
  1 sibling, 1 reply; 12+ messages in thread
From: Richard Cochran @ 2018-12-04 14:55 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Greg Kroah-Hartman, netdev, linux-kernel, kernel-janitors, YueHaibing

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] ptp: fix an IS_ERR() vs NULL check
  2018-12-04 14:55         ` Richard Cochran
@ 2018-12-04 15:10           ` Dan Carpenter
  2018-12-06 12:38             ` Richard Cochran
  0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2018-12-04 15:10 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Greg Kroah-Hartman, netdev, linux-kernel, kernel-janitors, YueHaibing

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] ptp: fix an IS_ERR() vs NULL check
  2018-12-04 15:10           ` Dan Carpenter
@ 2018-12-06 12:38             ` Richard Cochran
  2018-12-06 13:57               ` Greg Kroah-Hartman
  0 siblings, 1 reply; 12+ messages in thread
From: Richard Cochran @ 2018-12-06 12:38 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Greg Kroah-Hartman, netdev, linux-kernel, kernel-janitors, YueHaibing

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] ptp: fix an IS_ERR() vs NULL check
  2018-12-06 12:38             ` Richard Cochran
@ 2018-12-06 13:57               ` Greg Kroah-Hartman
  2018-12-07  6:00                 ` [PATCH v3] " Dan Carpenter
  0 siblings, 1 reply; 12+ messages in thread
From: Greg Kroah-Hartman @ 2018-12-06 13:57 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Dan Carpenter, netdev, linux-kernel, kernel-janitors, YueHaibing

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3] ptp: fix an IS_ERR() vs NULL check
  2018-12-06 13:57               ` Greg Kroah-Hartman
@ 2018-12-07  6:00                 ` " Dan Carpenter
  2018-12-12 14:17                   ` Richard Cochran
  0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2018-12-07  6:00 UTC (permalink / raw)
  To: Richard Cochran
  Cc: netdev, linux-kernel, kernel-janitors, YueHaibing, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] ptp: fix an IS_ERR() vs NULL check
  2018-12-07  6:00                 ` [PATCH v3] " Dan Carpenter
@ 2018-12-12 14:17                   ` Richard Cochran
  0 siblings, 0 replies; 12+ messages in thread
From: Richard Cochran @ 2018-12-12 14:17 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: netdev, linux-kernel, kernel-janitors, YueHaibing, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-30 12:58 [PATCH] ptp: fix an IS_ERR() vs NULL check Dan Carpenter
2018-11-30 16:32 ` Richard Cochran
2018-12-03 10:04   ` Dan Carpenter
     [not found]   ` <20181203105506.GA21127@unbuntlaptop>
2018-12-04  4:42     ` [PATCH v2] " Richard Cochran
2018-12-04  7:00       ` Dan Carpenter
2018-12-04 10:54         ` Greg Kroah-Hartman
2018-12-04 14:55         ` Richard Cochran
2018-12-04 15:10           ` Dan Carpenter
2018-12-06 12:38             ` Richard Cochran
2018-12-06 13:57               ` Greg Kroah-Hartman
2018-12-07  6:00                 ` [PATCH v3] " Dan Carpenter
2018-12-12 14:17                   ` Richard Cochran

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox