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

Subsystems: networking drivers, ptp hardware clock support, the rest

9 messages, 3 authors, 2018-12-12 · open the first message on its own page

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

From: Richard Cochran <richardcochran@gmail.com>
Date: 2018-12-04 04:43:03

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 <redacted>
---
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)

quoted hunk

 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

From: Dan Carpenter <hidden>
Date: 2018-12-04 07:00:54

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:

quoted hunk

On Mon, Dec 03, 2018 at 01:55:06PM +0300, Dan Carpenter wrote:

quoted

We recently modified pps_register_source() to return error pointers
instead of NULL but this check wasn't updated.

But it *was* updated!

quoted

Fixes: 3b1ad360acad ("pps: using ERR_PTR instead of NULL while pps_register_source fails")
Signed-off-by: Dan Carpenter <redacted>
---
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)

quoted

 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

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: 2018-12-04 11:09:56

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

From: Richard Cochran <richardcochran@gmail.com>
Date: 2018-12-04 14:55:44

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

From: Dan Carpenter <hidden>
Date: 2018-12-04 15:11:15

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:

quoted

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

From: Richard Cochran <richardcochran@gmail.com>
Date: 2018-12-06 12:38:48

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:

quoted

On Tue, Dec 04, 2018 at 10:00:33AM +0300, Dan Carpenter wrote:

quoted

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

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: 2018-12-06 13:57:23

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:

quoted

On Tue, Dec 04, 2018 at 06:55:38AM -0800, Richard Cochran wrote:

quoted

On Tue, Dec 04, 2018 at 10:00:33AM +0300, Dan Carpenter wrote:

quoted

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?

...

quoted

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

From: Dan Carpenter <hidden>
Date: 2018-12-07 06:01:03

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 <redacted>
---
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

From: Richard Cochran <richardcochran@gmail.com>
Date: 2018-12-12 14:17:08

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 <redacted>

Greg,

This patch provides the missing hunk.  Please apply it.

Thanks,
Richard

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help