nvme-fabrics: correct some printk information

drivers/nvme/host/fc.c

Quoted 6 lines · Dan Carpenter

We really don't care where "ctrl" is on the stack since we're just returning soon what we want is the actual ctrl pointer itself.

Signed-off-by Dan Carpenter <[email protected]>

drivers/nvme/host/fc.c +0 -0

[]

drivers/nvme/host/fc.c enum blk_eh_timer_return · line 2405

Quoted 6 lines

drivers/nvme/host/fc.c +1 -1

@@ -2402,7 +2402,7 @@ enum blk_eh_timer_return
 
 	dev_info(ctrl->ctrl.device,
 		"NVME-FC{%d}: new ctrl: NQN \"%s\" (%p)\n",
-		ctrl->cnum, ctrl->ctrl.opts->subsysnqn, &ctrl);
+		ctrl->cnum, ctrl->ctrl.opts->subsysnqn, ctrl);

Found by script or inspection?

If by script, it seems unlikely there's only 1 instance where an address of an automatic pointer type is used incorrectly.

Quoted 17 lines · Joe Perches

drivers/nvme/host/fc.c

Quoted 6 lines · Dan Carpenter

We really don't care where "ctrl" is on the stack since we're just returning soon what we want is the actual ctrl pointer itself.

Signed-off-by Dan Carpenter <[email protected]>

drivers/nvme/host/fc.c +0 -0

[]

drivers/nvme/host/fc.c enum blk_eh_timer_return · line 2405

Quoted 6 lines

drivers/nvme/host/fc.c +1 -1

@@ -2402,7 +2402,7 @@ enum blk_eh_timer_return
 
 	dev_info(ctrl->ctrl.device,
 		"NVME-FC{%d}: new ctrl: NQN \"%s\" (%p)\n",
-		ctrl->cnum, ctrl->ctrl.opts->subsysnqn, &ctrl);
+		ctrl->cnum, ctrl->ctrl.opts->subsysnqn, ctrl);

Found by script or inspection?

Quoted 3 lines

If by script, it seems unlikely there's only 1 instance where an address of an automatic pointer type is used incorrectly.

Script. But it's using a pretty specific heuristic where we kmalloc a pointer and then pass the address. It prints few warnings. Probably 40% false positives, but the remaining examples of course are 100% false positives.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 19 lines · Dan Carpenter

Quoted 18 lines · Joe Perches

drivers/nvme/host/fc.c

Quoted 6 lines · Dan Carpenter

We really don't care where "ctrl" is on the stack since we're just returning soon what we want is the actual ctrl pointer itself.

Signed-off-by Dan Carpenter <[email protected]>

drivers/nvme/host/fc.c +0 -0

[]

drivers/nvme/host/fc.c enum blk_eh_timer_return · line 2405

Quoted 6 lines

drivers/nvme/host/fc.c +1 -1

@@ -2402,7 +2402,7 @@ enum blk_eh_timer_return
 
 	dev_info(ctrl->ctrl.device,
 		"NVME-FC{%d}: new ctrl: NQN \"%s\" (%p)\n",
-		ctrl->cnum, ctrl->ctrl.opts->subsysnqn, &ctrl);
+		ctrl->cnum, ctrl->ctrl.opts->subsysnqn, ctrl);

Found by script or inspection?

Quoted 8 lines

Quoted 3 lines

If by script, it seems unlikely there's only 1 instance where an address of an automatic pointer type is used incorrectly.

Script. But it's using a pretty specific heuristic where we kmalloc a pointer and then pass the address. It prints few warnings. Probably 40% false positives, but the remaining examples of course are 100% false positives.

I tried anything that looks like a print, ie has a format string argument, and was taking the address of a local variable as another argument. But there are lots of weird format designators in the kernel that Coccinelle doesn't know about for which passing the address of a local variable is reasonable. So for the moment, there are, as far as I can see, just a lot of false positives. I did add improving the support for format strings to my TODO list.

tmp/nothing//dma/pxa_dma.c static unsigned int clear_chan_irq(struc · line 645

Quoted 10 lines · Joe Perches

diff -u -p drivers//dma/pxa_dma.c /tmp/nothing//dma/pxa_dma.c

tmp/nothing//dma/pxa_dma.c renamed from /dma/pxa_dma.c +0 -3

@@ -640,9 +640,6 @@ static unsigned int clear_chan_irq(struc
 	dcsr = phy_readl_relaxed(phy, DCSR);
 	phy_writel(phy, dcsr, DCSR);
 	if ((dcsr & PXA_DCSR_BUSERR) && (phy->vchan))
-		dev_warn(&phy->vchan->vc.chan.dev->device,
-			 "%s(chan=%p): PXA_DCSR_BUSERR\n",
-			 __func__, &phy->vchan);

That's not a defect. We're getting the address of vchan. I don't get it?

Quoted 14 lines · Dan Carpenter

tmp/nothing//dma/pxa_dma.c static unsigned int clear_chan_irq(struc · line 645

Quoted 10 lines · Joe Perches

diff -u -p drivers//dma/pxa_dma.c /tmp/nothing//dma/pxa_dma.c

tmp/nothing//dma/pxa_dma.c renamed from /dma/pxa_dma.c +0 -3

@@ -640,9 +640,6 @@ static unsigned int clear_chan_irq(struc
 	dcsr = phy_readl_relaxed(phy, DCSR);
 	phy_writel(phy, dcsr, DCSR);
 	if ((dcsr & PXA_DCSR_BUSERR) && (phy->vchan))
-		dev_warn(&phy->vchan->vc.chan.dev->device,
-			 "%s(chan=%p): PXA_DCSR_BUSERR\n",
-			 __func__, &phy->vchan);

That's not a defect. We're getting the address of vchan. I don't get it?

$ git grep -n -w vchan drivers/dma/pxa*
drivers/dma/pxa_dma.c:103:      struct pxad_chan        *vchan;

Quoted 16 lines · Joe Perches

Quoted 14 lines · Dan Carpenter

tmp/nothing//dma/pxa_dma.c static unsigned int clear_chan_irq(struc · line 645

Quoted 10 lines · Joe Perches

diff -u -p drivers//dma/pxa_dma.c /tmp/nothing//dma/pxa_dma.c

tmp/nothing//dma/pxa_dma.c renamed from /dma/pxa_dma.c +0 -3

@@ -640,9 +640,6 @@ static unsigned int clear_chan_irq(struc
 	dcsr = phy_readl_relaxed(phy, DCSR);
 	phy_writel(phy, dcsr, DCSR);
 	if ((dcsr & PXA_DCSR_BUSERR) && (phy->vchan))
-		dev_warn(&phy->vchan->vc.chan.dev->device,
-			 "%s(chan=%p): PXA_DCSR_BUSERR\n",
-			 __func__, &phy->vchan);

That's not a defect. We're getting the address of vchan. I don't get it?

Quoted 2 lines

$ git grep -n -w vchan drivers/dma/pxa*
drivers/dma/pxa_dma.c:103:      struct pxad_chan        *vchan;

I'm not sure what you're saying here still. This code works as intended. We're not printing a stack address.

Quoted 17 lines · Dan Carpenter

Quoted 16 lines · Joe Perches

Quoted 14 lines · Dan Carpenter

tmp/nothing//dma/pxa_dma.c static unsigned int clear_chan_irq(struc · line 645

Quoted 10 lines · Joe Perches

diff -u -p drivers//dma/pxa_dma.c /tmp/nothing//dma/pxa_dma.c

tmp/nothing//dma/pxa_dma.c renamed from /dma/pxa_dma.c +0 -3

@@ -640,9 +640,6 @@ static unsigned int clear_chan_irq(struc
          dcsr = phy_readl_relaxed(phy, DCSR);
          phy_writel(phy, dcsr, DCSR);
          if ((dcsr & PXA_DCSR_BUSERR) && (phy->vchan))
-               dev_warn(&phy->vchan->vc.chan.dev->device,
-                        "%s(chan=%p): PXA_DCSR_BUSERR\n",
-                        __func__, &phy->vchan);

That's not a defect.  We're getting the address of vchan.  I don't get it?

Quoted 5 lines

Quoted 2 lines

$ git grep -n -w vchan drivers/dma/pxa*
drivers/dma/pxa_dma.c:103:      struct pxad_chan        *vchan;

I'm not sure what you're saying here still.  This code works as intended.  We're not printing a stack address.

I guess that the point is that one would like to print the channel, not the address of the channel?

Quoted 31 lines · Julia Lawall

Quoted 19 lines · Dan Carpenter

Quoted 18 lines · Joe Perches

drivers/nvme/host/fc.c

Quoted 6 lines · Dan Carpenter

We really don't care where "ctrl" is on the stack since we're just returning soon what we want is the actual ctrl pointer itself.

Signed-off-by Dan Carpenter <[email protected]>

drivers/nvme/host/fc.c +0 -0

[]

drivers/nvme/host/fc.c enum blk_eh_timer_return · line 2405

Quoted 6 lines

drivers/nvme/host/fc.c +1 -1

@@ -2402,7 +2402,7 @@ enum blk_eh_timer_return
 
 	dev_info(ctrl->ctrl.device,
 		"NVME-FC{%d}: new ctrl: NQN \"%s\" (%p)\n",
-		ctrl->cnum, ctrl->ctrl.opts->subsysnqn, &ctrl);
+		ctrl->cnum, ctrl->ctrl.opts->subsysnqn, ctrl);

Found by script or inspection?

Quoted 8 lines

Quoted 3 lines

If by script, it seems unlikely there's only 1 instance where an address of an automatic pointer type is used incorrectly.

Script. But it's using a pretty specific heuristic where we kmalloc a pointer and then pass the address. It prints few warnings. Probably 40% false positives, but the remaining examples of course are 100% false positives.

Quoted 7 lines

I tried anything that looks like a print, ie has a format string argument, and was taking the address of a local variable as another argument. But there are lots of weird format designators in the kernel that Coccinelle doesn't know about for which passing the address of a local variable is reasonable. So for the moment, there are, as far as I can see, just a lot of false positives. I did add improving the support for format strings to my TODO list.

I think there's probably a class of defects that could be found something like this in coccinelle:

@@
type T;
T *t;
@@

* \(netdev_emerg\|netdev_crit\|netdev_alert\|netdev_err\|netdev_notice\|netdev_warn\|netdev_warn\|netdev_info\|netdev_dbg\|dev_emerg\|dev_crit\|dev_alert\|dev_err\|dev_notice\|dev_warn\|dev_warn\|dev_info\|dev_dbg\|pr_emerg\|pr_crit\|pr_alert\|pr_err\|pr_notice\|pr_warn\|pr_warning\|pr_warn\|pr_info\|pr_debug\|printk\|vsprintf\|vscnprintf\|vsprintf\)(..., &t, ...);

This finds a few like:

diff -u -p drivers//dma/pxa_dma.c /tmp/nothing//dma/pxa_dma.c

tmp/nothing//dma/pxa_dma.c renamed from /dma/pxa_dma.c +0 -3

@@ -640,9 +640,6 @@ static unsigned int clear_chan_irq(struc
 	dcsr = phy_readl_relaxed(phy, DCSR);
 	phy_writel(phy, dcsr, DCSR);
 	if ((dcsr & PXA_DCSR_BUSERR) && (phy->vchan))
-		dev_warn(&phy->vchan->vc.chan.dev->device,
-			 "%s(chan=%p): PXA_DCSR_BUSERR\n",
-			 __func__, &phy->vchan);
 
 	return dcsr & ~PXA_DCSR_RUN;
 }

btw: It'd be nice if coccinelle could use multiple nested "\("

Quoted 66 lines · Joe Perches

Quoted 31 lines · Julia Lawall

Quoted 19 lines · Dan Carpenter

Quoted 18 lines · Joe Perches

drivers/nvme/host/fc.c

Quoted 6 lines · Dan Carpenter

We really don't care where "ctrl" is on the stack since we're just returning soon what we want is the actual ctrl pointer itself.

Signed-off-by Dan Carpenter <[email protected]>

drivers/nvme/host/fc.c +0 -0

[]

drivers/nvme/host/fc.c enum blk_eh_timer_return · line 2405

Quoted 6 lines

drivers/nvme/host/fc.c +1 -1

@@ -2402,7 +2402,7 @@ enum blk_eh_timer_return
 
 	dev_info(ctrl->ctrl.device,
 		"NVME-FC{%d}: new ctrl: NQN \"%s\" (%p)\n",
-		ctrl->cnum, ctrl->ctrl.opts->subsysnqn, &ctrl);
+		ctrl->cnum, ctrl->ctrl.opts->subsysnqn, ctrl);

Found by script or inspection?

Quoted 8 lines

Quoted 3 lines

If by script, it seems unlikely there's only 1 instance where an address of an automatic pointer type is used incorrectly.

Script. But it's using a pretty specific heuristic where we kmalloc a pointer and then pass the address. It prints few warnings. Probably 40% false positives, but the remaining examples of course are 100% false positives.

Quoted 7 lines

I tried anything that looks like a print, ie has a format string argument, and was taking the address of a local variable as another argument. But there are lots of weird format designators in the kernel that Coccinelle doesn't know about for which passing the address of a local variable is reasonable. So for the moment, there are, as far as I can see, just a lot of false positives. I did add improving the support for format strings to my TODO list.

I think there's probably a class of defects that could be found something like this in coccinelle:

@@
type T;
T *t;
@@

* \(netdev_emerg\|netdev_crit\|netdev_alert\|netdev_err\|netdev_notice\|netdev_warn\|netdev_warn\|netdev_info\|netdev_dbg\|dev_emerg\|dev_crit\|dev_alert\|dev_err\|dev_notice\|dev_warn\|dev_warn\|dev_info\|dev_dbg\|pr_emerg\|pr_crit\|pr_alert\|pr_err\|pr_notice\|pr_warn\|pr_warning\|pr_warn\|pr_info\|pr_debug\|printk\|vsprintf\|vscnprintf\|vsprintf\)(..., &t, ...);

This finds a few like:

diff -u -p drivers//dma/pxa_dma.c /tmp/nothing//dma/pxa_dma.c

tmp/nothing//dma/pxa_dma.c renamed from /dma/pxa_dma.c +0 -3

@@ -640,9 +640,6 @@ static unsigned int clear_chan_irq(struc
 	dcsr = phy_readl_relaxed(phy, DCSR);
 	phy_writel(phy, dcsr, DCSR);
 	if ((dcsr & PXA_DCSR_BUSERR) && (phy->vchan))
-		dev_warn(&phy->vchan->vc.chan.dev->device,
-			 "%s(chan=%p): PXA_DCSR_BUSERR\n",
-			 __func__, &phy->vchan);
 
 	return dcsr & ~PXA_DCSR_RUN;
 }

Quoted 1 line

btw: It'd be nice if coccinelle could use multiple nested "\("

What exactly didn't work? It should be possible.

My rule was:

@@
format d;
local idexpression l;
identifier f != {sscanf,fscanf};
@@

f(...,"...%@d@...",...,
*&l
,...)

But there are many false positives, with things like %pV.

julia

Quoted 19 lines · Dan Carpenter

Quoted 18 lines · Joe Perches

drivers/nvme/host/fc.c

Quoted 6 lines · Dan Carpenter

We really don't care where "ctrl" is on the stack since we're just returning soon what we want is the actual ctrl pointer itself.

Signed-off-by Dan Carpenter <[email protected]>

drivers/nvme/host/fc.c +0 -0

[]

drivers/nvme/host/fc.c enum blk_eh_timer_return · line 2405

Quoted 6 lines

drivers/nvme/host/fc.c +1 -1

@@ -2402,7 +2402,7 @@ enum blk_eh_timer_return
 
 	dev_info(ctrl->ctrl.device,
 		"NVME-FC{%d}: new ctrl: NQN \"%s\" (%p)\n",
-		ctrl->cnum, ctrl->ctrl.opts->subsysnqn, &ctrl);
+		ctrl->cnum, ctrl->ctrl.opts->subsysnqn, ctrl);

Found by script or inspection?

Quoted 8 lines

Quoted 3 lines

If by script, it seems unlikely there's only 1 instance where an address of an automatic pointer type is used incorrectly.

Script. But it's using a pretty specific heuristic where we kmalloc a pointer and then pass the address. It prints few warnings. Probably 40% false positives, but the remaining examples of course are 100% false positives.

I tried anything that looks like a print, ie has a format string argument, and was taking the address of a local variable as another argument. But there are lots of weird format designators in the kernel that Coccinelle doesn't know about for which passing the address of a local variable is reasonable. So for the moment, there are, as far as I can see, just a lot of false positives. I did add improving the support for format strings to my TODO list.

Hi,

As mentioned, I've been working on getting smatch to do type checking of the various %p format extensions. The code is now on github
(https://github.com/Villemoes/smatch).

Note that this work revealed a bug in sparse's handling of string literals coming from macro expansions
(http://thread.gmane.org/gmane.comp.parsers.sparse/4080). I've applied one of the suggested fixes, but it's still not clear to me what the final fix will be in sparse upstream. Anyway, this was good enough to get the ball rolling.

While developing this, I found it useful to only enable that specific check (both to get smatch run faster and to get less noise in the output), so there's also a few unrelated patches in the printf branch implementing that feature.

sparse currently ignores attribute((format)), so the list of printf functions has been extracted with a perl script and hard-coded. Even if sparse understood attribute((format)), I wouldn't know how to set up a hook for 'call of function with this or that attribute'.

I don't think it's ready to be merged upstream (and whether that will even happen is of course entirely up to Dan), but now it's out there for people to play with. I have already sent patches for the four %p bugs found, but there may be a few more lurking in arch/<not x86>/ - I don't know how to pursuade the build system to go there.

Rasmus

We really don't care where "ctrl" is on the stack since we're just returning soon what we want is the actual ctrl pointer itself.

Signed-off-by Dan Carpenter <[email protected]>

drivers/nvme/host/fc.c +1 -1

index 771e2e761872..e6395ed2f562 100644
@@ -2402,7 +2402,7 @@ enum blk_eh_timer_return
 
  	dev_info(ctrl->ctrl.device,
  		"NVME-FC{%d}: new ctrl: NQN \"%s\" (%p)\n",
-		ctrl->cnum, ctrl->ctrl.opts->subsysnqn, &ctrl);
+		ctrl->cnum, ctrl->ctrl.opts->subsysnqn, ctrl);
 
  	kref_get(&ctrl->ctrl.kref);
 

Dan,

Mind if I solve this a different way ?