* [PATCH 1/2 v2] Staging: comedi: fix printk() issue in adv_pci1710.c
[not found] <[PATCH]Staging: comedi: fix printk() issue in adv_pci1710.c>
@ 2011-07-19 3:52 ` Ravishankar
2011-07-19 3:55 ` Joe Perches
2011-07-19 4:55 ` [PATCH 1/2 v3] " Ravishankar
2011-07-19 5:50 ` Ravishankar
2 siblings, 1 reply; 10+ messages in thread
From: Ravishankar @ 2011-07-19 3:52 UTC (permalink / raw)
To: gregkh, wfp5p; +Cc: devel, linux-kernel, Ravishankar, Ravishankar
From: Ravishankar <ravi.shankar@greenturtles.in>
This is a patch to the adv_pci1710.c file that fixes up a printk() warning found by the checkpatch.pl tool
Signed-off-by: Ravishankar <ravishankarkm32@gmail.com>
---
KERN_INFO is replaced by KERN_ERR.
drivers/staging/comedi/drivers/adv_pci1710.c | 20 ++++++++++----------
1 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/comedi/drivers/adv_pci1710.c b/drivers/staging/comedi/drivers/adv_pci1710.c
index fd71cc6..093b9e6 100644
--- a/drivers/staging/comedi/drivers/adv_pci1710.c
+++ b/drivers/staging/comedi/drivers/adv_pci1710.c
@@ -1396,14 +1396,14 @@ static int pci1710_attach(struct comedi_device *dev,
int i;
int board_index;
- printk("comedi%d: adv_pci1710: ", dev->minor);
+ printk(KERN_INFO "comedi%d: adv_pci1710: ", dev->minor);
opt_bus = it->options[0];
opt_slot = it->options[1];
ret = alloc_private(dev, sizeof(struct pci1710_private));
if (ret < 0) {
- printk(" - Allocation failed!\n");
+ printk(KERN_ERR " - Allocation failed!\n");
return -ENOMEM;
}
@@ -1451,10 +1451,10 @@ static int pci1710_attach(struct comedi_device *dev,
if (!pcidev) {
if (opt_bus || opt_slot) {
- printk(" - Card at b:s %d:%d %s\n",
+ printk(KERN_ERR " - Card at b:s %d:%d %s\n",
opt_bus, opt_slot, errstr);
} else {
- printk(" - Card %s\n", errstr);
+ printk(KERN_ERR " - Card %s\n", errstr);
}
return -EIO;
}
@@ -1465,7 +1465,7 @@ static int pci1710_attach(struct comedi_device *dev,
irq = pcidev->irq;
iobase = pci_resource_start(pcidev, 2);
- printk(", b:s:f=%d:%d:%d, io=0x%4lx", pci_bus, pci_slot, pci_func,
+ printk(KERN_INFO ", b:s:f=%d:%d:%d, io=0x%4lx", pci_bus, pci_slot,
+ pci_func, iobase);
dev->iobase = iobase;
@@ -1487,7 +1487,7 @@ static int pci1710_attach(struct comedi_device *dev,
ret = alloc_subdevices(dev, n_subdevices);
if (ret < 0) {
- printk(" - Allocation failed!\n");
+ printk(KERN_ERR " - Allocation failed!\n");
return ret;
}
@@ -1499,14 +1499,14 @@ static int pci1710_attach(struct comedi_device *dev,
IRQF_SHARED, "Advantech PCI-1710",
dev)) {
printk
- (", unable to allocate IRQ %d, DISABLING IT",
+ (KERN_CONT ", unable to allocate IRQ %d, DISABLING IT",
irq);
irq = 0; /* Can't use IRQ */
} else {
- printk(", irq=%u", irq);
+ printk(KERN_CONT ", irq=%u", irq);
}
} else {
- printk(", IRQ disabled");
+ printk(KERN_CONT ", IRQ disabled");
}
} else {
irq = 0;
@@ -1514,7 +1514,7 @@ static int pci1710_attach(struct comedi_device *dev,
dev->irq = irq;
- printk(".\n");
+ printk(KERN_CONT ".\n");
subdev = 0;
--
1.6.5.2
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2 v2] Staging: comedi: fix printk() issue in adv_pci1710.c
2011-07-19 3:52 ` [PATCH 1/2 v2] Staging: comedi: fix printk() issue in adv_pci1710.c Ravishankar
@ 2011-07-19 3:55 ` Joe Perches
0 siblings, 0 replies; 10+ messages in thread
From: Joe Perches @ 2011-07-19 3:55 UTC (permalink / raw)
To: Ravishankar; +Cc: gregkh, wfp5p, devel, linux-kernel, Ravishankar
On Tue, 2011-07-19 at 09:22 +0530, Ravishankar wrote:
> From: Ravishankar <ravi.shankar@greenturtles.in>
> This is a patch to the adv_pci1710.c file that fixes up a printk() warning found by the checkpatch.pl tool
> Signed-off-by: Ravishankar <ravishankarkm32@gmail.com>
> ---
> KERN_INFO is replaced by KERN_ERR.
> drivers/staging/comedi/drivers/adv_pci1710.c | 20 ++++++++++----------
> 1 files changed, 10 insertions(+), 10 deletions(-)
> diff --git a/drivers/staging/comedi/drivers/adv_pci1710.c b/drivers/staging/comedi/drivers/adv_pci1710.c
> index fd71cc6..093b9e6 100644
> --- a/drivers/staging/comedi/drivers/adv_pci1710.c
> +++ b/drivers/staging/comedi/drivers/adv_pci1710.c
> @@ -1396,14 +1396,14 @@ static int pci1710_attach(struct comedi_device *dev,\
[]
> - printk("comedi%d: adv_pci1710: ", dev->minor);
> + printk(KERN_INFO "comedi%d: adv_pci1710: ", dev->minor);
[]
> if (ret < 0) {
> - printk(" - Allocation failed!\n");
> + printk(KERN_ERR " - Allocation failed!\n");
> return -ENOMEM;
> }
Ravi, (may I call you Ravi?)
you've done this several times now.
Please understand that a printk without a "\n" termination
can be continued by subsequent printks.
When that happens, you should use printk(KERN_CONT... or
pr_cont(... to continue the previous message, not give a
new error level to the continuation.
cheers, Joe
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2 v3] Staging: comedi: fix printk() issue in adv_pci1710.c
[not found] <[PATCH]Staging: comedi: fix printk() issue in adv_pci1710.c>
2011-07-19 3:52 ` [PATCH 1/2 v2] Staging: comedi: fix printk() issue in adv_pci1710.c Ravishankar
@ 2011-07-19 4:55 ` Ravishankar
2011-07-19 4:52 ` Joe Perches
2011-07-19 5:50 ` Ravishankar
2 siblings, 1 reply; 10+ messages in thread
From: Ravishankar @ 2011-07-19 4:55 UTC (permalink / raw)
To: gregkh, wfp5p; +Cc: devel, linux-kernel, Ravishankar, Ravishankar
From: Ravishankar <ravi.shankar@greenturtles.in>
This is a patch to the adv_pci1710.c file that fixes up a printk() warning found by the checkpatch.pl tool
Signed-off-by: Ravishankar <ravishankarkm32@gmail.com>
---
KERN_CONT issue is fixed
drivers/staging/comedi/drivers/adv_pci1710.c | 20 ++++++++++----------
1 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/comedi/drivers/adv_pci1710.c b/drivers/staging/comedi/drivers/adv_pci1710.c
index fd71cc6..093b9e6 100644
--- a/drivers/staging/comedi/drivers/adv_pci1710.c
+++ b/drivers/staging/comedi/drivers/adv_pci1710.c
@@ -1396,14 +1396,14 @@ static int pci1710_attach(struct comedi_device *dev,
int i;
int board_index;
- printk("comedi%d: adv_pci1710: ", dev->minor);
+ printk(KERN_CONT "comedi%d: adv_pci1710: ", dev->minor);
opt_bus = it->options[0];
opt_slot = it->options[1];
ret = alloc_private(dev, sizeof(struct pci1710_private));
if (ret < 0) {
- printk(" - Allocation failed!\n");
+ printk(KERN_CONT " - Allocation failed!\n");
return -ENOMEM;
}
@@ -1451,10 +1451,10 @@ static int pci1710_attach(struct comedi_device *dev,
if (!pcidev) {
if (opt_bus || opt_slot) {
- printk(" - Card at b:s %d:%d %s\n",
+ printk(KERN_ERR " - Card at b:s %d:%d %s\n",
opt_bus, opt_slot, errstr);
} else {
- printk(" - Card %s\n", errstr);
+ printk(KERN_ERR " - Card %s\n", errstr);
}
return -EIO;
}
@@ -1465,7 +1465,7 @@ static int pci1710_attach(struct comedi_device *dev,
irq = pcidev->irq;
iobase = pci_resource_start(pcidev, 2);
- printk(", b:s:f=%d:%d:%d, io=0x%4lx", pci_bus, pci_slot, pci_func,
+ printk(KERN_CONT ", b:s:f=%d:%d:%d, io=0x%4lx", pci_bus, pci_slot,
+ pci_func, iobase);
dev->iobase = iobase;
@@ -1487,7 +1487,7 @@ static int pci1710_attach(struct comedi_device *dev,
ret = alloc_subdevices(dev, n_subdevices);
if (ret < 0) {
- printk(" - Allocation failed!\n");
+ printk(KERN_CONT " - Allocation failed!\n");
return ret;
}
@@ -1499,14 +1499,14 @@ static int pci1710_attach(struct comedi_device *dev,
IRQF_SHARED, "Advantech PCI-1710",
dev)) {
printk
- (", unable to allocate IRQ %d, DISABLING IT",
+ (KERN_CONT ", unable to allocate IRQ %d, DISABLING IT",
irq);
irq = 0; /* Can't use IRQ */
} else {
- printk(", irq=%u", irq);
+ printk(KERN_CONT ", irq=%u", irq);
}
} else {
- printk(", IRQ disabled");
+ printk(KERN_CONT ", IRQ disabled");
}
} else {
irq = 0;
@@ -1514,7 +1514,7 @@ static int pci1710_attach(struct comedi_device *dev,
dev->irq = irq;
- printk(".\n");
+ printk(KERN_CONT ".\n");
subdev = 0;
--
1.6.5.2
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2 v3] Staging: comedi: fix printk() issue in adv_pci1710.c
2011-07-19 4:55 ` [PATCH 1/2 v3] " Ravishankar
@ 2011-07-19 4:52 ` Joe Perches
0 siblings, 0 replies; 10+ messages in thread
From: Joe Perches @ 2011-07-19 4:52 UTC (permalink / raw)
To: Ravishankar; +Cc: gregkh, wfp5p, devel, linux-kernel, Ravishankar
On Tue, 2011-07-19 at 10:25 +0530, Ravishankar wrote:
> From: Ravishankar <ravi.shankar@greenturtles.in>
> This is a patch to the adv_pci1710.c file that fixes up a printk() warning found by the checkpatch.pl tool
> Signed-off-by: Ravishankar <ravishankarkm32@gmail.com>
> ---
> KERN_CONT issue is fixed
Nope, still broken, but in a different way.
> drivers/staging/comedi/drivers/adv_pci1710.c | 20 ++++++++++----------
> 1 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/adv_pci1710.c b/drivers/staging/comedi/drivers/adv_pci1710.c
> index fd71cc6..093b9e6 100644
> --- a/drivers/staging/comedi/drivers/adv_pci1710.c
> +++ b/drivers/staging/comedi/drivers/adv_pci1710.c
> @@ -1396,14 +1396,14 @@ static int pci1710_attach(struct comedi_device *dev,
> int i;
> int board_index;
>
> - printk("comedi%d: adv_pci1710: ", dev->minor);
> + printk(KERN_CONT "comedi%d: adv_pci1710: ", dev->minor);
This should be KERN_INFO.
_All_ initial printks should have some appropriate KERN_<level>
that is not KERN_CONT (which stands for continuation, btw)
> opt_bus = it->options[0];
> opt_slot = it->options[1];
>
> ret = alloc_private(dev, sizeof(struct pci1710_private));
> if (ret < 0) {
> - printk(" - Allocation failed!\n");
> + printk(KERN_CONT " - Allocation failed!\n");
This could be:
printk(KERN_CONT "\n");
printk(KERN_ERR "Comedi%d: adv_pci1710: Allocation failed\n",
dev->minor);
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2 v3] Staging: comedi: fix printk() issue in adv_pci1710.c
[not found] <[PATCH]Staging: comedi: fix printk() issue in adv_pci1710.c>
2011-07-19 3:52 ` [PATCH 1/2 v2] Staging: comedi: fix printk() issue in adv_pci1710.c Ravishankar
2011-07-19 4:55 ` [PATCH 1/2 v3] " Ravishankar
@ 2011-07-19 5:50 ` Ravishankar
2011-07-19 6:07 ` Ryan Mallon
2 siblings, 1 reply; 10+ messages in thread
From: Ravishankar @ 2011-07-19 5:50 UTC (permalink / raw)
To: gregkh, wfp5p; +Cc: devel, linux-kernel, Ravishankar, Ravishankar
From: Ravishankar <ravi.shankar@greenturtles.in>
This is a patch to the adv_pci1710.c file that fixes up a printk() warning found by the checkpatch.pl tool
Signed-off-by: Ravishankar <ravishankarkm32@gmail.com>
---
KERN_CONT issue is fixed
drivers/staging/comedi/drivers/adv_pci1710.c | 20 ++++++++++----------
1 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/comedi/drivers/adv_pci1710.c b/drivers/staging/comedi/drivers/adv_pci1710.c
index fd71cc6..093b9e6 100644
--- a/drivers/staging/comedi/drivers/adv_pci1710.c
+++ b/drivers/staging/comedi/drivers/adv_pci1710.c
@@ -1396,14 +1396,14 @@ static int pci1710_attach(struct comedi_device *dev,
int i;
int board_index;
- printk("comedi%d: adv_pci1710: ", dev->minor);
+ printk(KERN_INFO "comedi%d: adv_pci1710: ", dev->minor);
opt_bus = it->options[0];
opt_slot = it->options[1];
ret = alloc_private(dev, sizeof(struct pci1710_private));
if (ret < 0) {
- printk(" - Allocation failed!\n");
+ printk(KERN_CONT "\n");
+ printk(KERN_ERR "Comedi%d: adv_pci1710: Allocation failed\n",
+ dev->minor);
return -ENOMEM;
}
@@ -1451,10 +1451,10 @@ static int pci1710_attach(struct comedi_device *dev,
if (!pcidev) {
if (opt_bus || opt_slot) {
- printk(" - Card at b:s %d:%d %s\n",
+ printk(KERN_ERR " - Card at b:s %d:%d %s\n",
opt_bus, opt_slot, errstr);
} else {
- printk(" - Card %s\n", errstr);
+ printk(KERN_ERR " - Card %s\n", errstr);
}
return -EIO;
}
@@ -1465,7 +1465,7 @@ static int pci1710_attach(struct comedi_device *dev,
irq = pcidev->irq;
iobase = pci_resource_start(pcidev, 2);
- printk(", b:s:f=%d:%d:%d, io=0x%4lx", pci_bus, pci_slot, pci_func,
+ printk(KERN_INFO ", b:s:f=%d:%d:%d, io=0x%4lx", pci_bus, pci_slot,
+ pci_func, iobase);
dev->iobase = iobase;
@@ -1487,7 +1487,7 @@ static int pci1710_attach(struct comedi_device *dev,
ret = alloc_subdevices(dev, n_subdevices);
if (ret < 0) {
- printk(" - Allocation failed!\n");
+ printk(KERN_CONT " - Allocation failed!\n");
return ret;
}
@@ -1499,14 +1499,14 @@ static int pci1710_attach(struct comedi_device *dev,
IRQF_SHARED, "Advantech PCI-1710",
dev)) {
printk
- (", unable to allocate IRQ %d, DISABLING IT",
+ (KERN_INFO ", unable to allocate IRQ %d, DISABLING IT",
irq);
irq = 0; /* Can't use IRQ */
} else {
- printk(", irq=%u", irq);
+ printk(KERN_CONT ", irq=%u", irq);
}
} else {
- printk(", IRQ disabled");
+ printk(KERN_CONT ", IRQ disabled");
}
} else {
irq = 0;
@@ -1514,7 +1514,7 @@ static int pci1710_attach(struct comedi_device *dev,
dev->irq = irq;
- printk(".\n");
+ printk(KERN_CONT ".\n");
subdev = 0;
--
1.6.5.2
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2 v3] Staging: comedi: fix printk() issue in adv_pci1710.c
2011-07-19 5:50 ` Ravishankar
@ 2011-07-19 6:07 ` Ryan Mallon
2011-07-19 6:12 ` Dan Carpenter
2011-07-19 23:44 ` Ryan Mallon
0 siblings, 2 replies; 10+ messages in thread
From: Ryan Mallon @ 2011-07-19 6:07 UTC (permalink / raw)
To: Ravishankar; +Cc: gregkh, wfp5p, devel, linux-kernel, Ravishankar
On 19/07/11 15:50, Ravishankar wrote:
> From: Ravishankar<ravi.shankar@greenturtles.in>
>
> This is a patch to the adv_pci1710.c file that fixes up a printk() warning found by the checkpatch.pl tool
>
> Signed-off-by: Ravishankar<ravishankarkm32@gmail.com>
> ---
> KERN_CONT issue is fixed
>
> drivers/staging/comedi/drivers/adv_pci1710.c | 20 ++++++++++----------
> 1 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/adv_pci1710.c b/drivers/staging/comedi/drivers/adv_pci1710.c
> index fd71cc6..093b9e6 100644
> --- a/drivers/staging/comedi/drivers/adv_pci1710.c
> +++ b/drivers/staging/comedi/drivers/adv_pci1710.c
> @@ -1396,14 +1396,14 @@ static int pci1710_attach(struct comedi_device *dev,
> int i;
> int board_index;
>
> - printk("comedi%d: adv_pci1710: ", dev->minor);
> + printk(KERN_INFO "comedi%d: adv_pci1710: ", dev->minor);
>
> opt_bus = it->options[0];
> opt_slot = it->options[1];
>
> ret = alloc_private(dev, sizeof(struct pci1710_private));
> if (ret< 0) {
> - printk(" - Allocation failed!\n");
> + printk(KERN_CONT "\n");
> + printk(KERN_ERR "Comedi%d: adv_pci1710: Allocation failed\n",
> + dev->minor);
This still isn't correct. The initial printk (KERN_INFO above) has no
trailing newline. Here you are almost doing it correctly, except that
you are adding an extra newline. This error message will look like this:
comedi0: adv_pci1710:
comedi0: adv_pci1710: Allocation failed
Which is just silly. The printk's below are broken in that you are using
KERN_ERR where you should be using KERN_CONT.
As it stands the code is a little tricky to follow since the trailing
newline gets added at the end unless there is an error when it gets
added in place. It might be better just to print the full message,
including the "comedi%d: adv_pci1710:" bit, at each printk call site and
get rid of the whole KERN_CONT nonsense altogether. Alternatively you
could create a comedi_printk function (or use pr_fmt) which wraps this up.
Also, do we really need to print out things like allocation failures? We
have the errno value already and allocation failures for devices drivers
aren't so common that we error messages for them?
~Ryan
> return -ENOMEM;
> }
>
> @@ -1451,10 +1451,10 @@ static int pci1710_attach(struct comedi_device *dev,
>
> if (!pcidev) {
> if (opt_bus || opt_slot) {
> - printk(" - Card at b:s %d:%d %s\n",
> + printk(KERN_ERR " - Card at b:s %d:%d %s\n",
> opt_bus, opt_slot, errstr);
> } else {
> - printk(" - Card %s\n", errstr);
> + printk(KERN_ERR " - Card %s\n", errstr);
> }
> return -EIO;
> }
> @@ -1465,7 +1465,7 @@ static int pci1710_attach(struct comedi_device *dev,
> irq = pcidev->irq;
> iobase = pci_resource_start(pcidev, 2);
>
> - printk(", b:s:f=%d:%d:%d, io=0x%4lx", pci_bus, pci_slot, pci_func,
> + printk(KERN_INFO ", b:s:f=%d:%d:%d, io=0x%4lx", pci_bus, pci_slot,
> + pci_func, iobase);
>
> dev->iobase = iobase;
> @@ -1487,7 +1487,7 @@ static int pci1710_attach(struct comedi_device *dev,
>
> ret = alloc_subdevices(dev, n_subdevices);
> if (ret< 0) {
> - printk(" - Allocation failed!\n");
> + printk(KERN_CONT " - Allocation failed!\n");
> return ret;
> }
>
> @@ -1499,14 +1499,14 @@ static int pci1710_attach(struct comedi_device *dev,
> IRQF_SHARED, "Advantech PCI-1710",
> dev)) {
> printk
> - (", unable to allocate IRQ %d, DISABLING IT",
> + (KERN_INFO ", unable to allocate IRQ %d, DISABLING IT",
> irq);
> irq = 0; /* Can't use IRQ */
> } else {
> - printk(", irq=%u", irq);
> + printk(KERN_CONT ", irq=%u", irq);
> }
> } else {
> - printk(", IRQ disabled");
> + printk(KERN_CONT ", IRQ disabled");
> }
> } else {
> irq = 0;
> @@ -1514,7 +1514,7 @@ static int pci1710_attach(struct comedi_device *dev,
>
> dev->irq = irq;
>
> - printk(".\n");
> + printk(KERN_CONT ".\n");
>
> subdev = 0;
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2 v3] Staging: comedi: fix printk() issue in adv_pci1710.c
2011-07-19 6:07 ` Ryan Mallon
@ 2011-07-19 6:12 ` Dan Carpenter
2011-07-19 6:15 ` Ryan Mallon
2011-07-19 23:44 ` Ryan Mallon
1 sibling, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2011-07-19 6:12 UTC (permalink / raw)
To: Ryan Mallon; +Cc: Ravishankar, devel, gregkh, linux-kernel, Ravishankar
On Tue, Jul 19, 2011 at 04:07:27PM +1000, Ryan Mallon wrote:
> This still isn't correct. The initial printk (KERN_INFO above) has
> no trailing newline. Here you are almost doing it correctly, except
> that you are adding an extra newline. This error message will look
> like this:
>
> comedi0: adv_pci1710:
> comedi0: adv_pci1710: Allocation failed
>
Actually kmalloc() prints an message with a stack trace on failure as
well so that would go in the middle, just before the first newline.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2 v3] Staging: comedi: fix printk() issue in adv_pci1710.c
2011-07-19 6:12 ` Dan Carpenter
@ 2011-07-19 6:15 ` Ryan Mallon
2011-07-19 6:25 ` Dan Carpenter
0 siblings, 1 reply; 10+ messages in thread
From: Ryan Mallon @ 2011-07-19 6:15 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Ravishankar, devel, gregkh, linux-kernel, Ravishankar
On 19/07/11 16:12, Dan Carpenter wrote:
> On Tue, Jul 19, 2011 at 04:07:27PM +1000, Ryan Mallon wrote:
>> This still isn't correct. The initial printk (KERN_INFO above) has
>> no trailing newline. Here you are almost doing it correctly, except
>> that you are adding an extra newline. This error message will look
>> like this:
>>
>> comedi0: adv_pci1710:
>> comedi0: adv_pci1710: Allocation failed
>>
> Actually kmalloc() prints an message with a stack trace on failure as
> well so that would go in the middle, just before the first newline.
>
Thanks. Didn't know that. Is that a KConfig option or by default?
Either way, it's another good reason to get rid of all the KERN_CONT
stuff in the driver.
~Ryan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2 v3] Staging: comedi: fix printk() issue in adv_pci1710.c
2011-07-19 6:15 ` Ryan Mallon
@ 2011-07-19 6:25 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2011-07-19 6:25 UTC (permalink / raw)
To: Ryan Mallon; +Cc: Ravishankar, devel, gregkh, linux-kernel, Ravishankar
On Tue, Jul 19, 2011 at 04:15:51PM +1000, Ryan Mallon wrote:
> On 19/07/11 16:12, Dan Carpenter wrote:
> >On Tue, Jul 19, 2011 at 04:07:27PM +1000, Ryan Mallon wrote:
> >>This still isn't correct. The initial printk (KERN_INFO above) has
> >>no trailing newline. Here you are almost doing it correctly, except
> >>that you are adding an extra newline. This error message will look
> >>like this:
> >>
> >>comedi0: adv_pci1710:
> >>comedi0: adv_pci1710: Allocation failed
> >>
> >Actually kmalloc() prints an message with a stack trace on failure as
> >well so that would go in the middle, just before the first newline.
> >
>
> Thanks. Didn't know that. Is that a KConfig option or by default?
By default, unless you call kmalloc() with __GFP_NOWARN. The warning
is in warn_alloc_failed().
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2 v3] Staging: comedi: fix printk() issue in adv_pci1710.c
2011-07-19 6:07 ` Ryan Mallon
2011-07-19 6:12 ` Dan Carpenter
@ 2011-07-19 23:44 ` Ryan Mallon
1 sibling, 0 replies; 10+ messages in thread
From: Ryan Mallon @ 2011-07-19 23:44 UTC (permalink / raw)
To: Ravishankar; +Cc: gregkh, wfp5p, devel, linux-kernel, Ravishankar
On 19/07/11 16:07, Ryan Mallon wrote:
> On 19/07/11 15:50, Ravishankar wrote:
>> From: Ravishankar<ravi.shankar@greenturtles.in>
>>
>> This is a patch to the adv_pci1710.c file that fixes up a printk()
>> warning found by the checkpatch.pl tool
>>
>> Signed-off-by: Ravishankar<ravishankarkm32@gmail.com>
>> ---
>> KERN_CONT issue is fixed
>>
>> drivers/staging/comedi/drivers/adv_pci1710.c | 20
>> ++++++++++----------
>> 1 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/adv_pci1710.c
>> b/drivers/staging/comedi/drivers/adv_pci1710.c
>> index fd71cc6..093b9e6 100644
>> --- a/drivers/staging/comedi/drivers/adv_pci1710.c
>> +++ b/drivers/staging/comedi/drivers/adv_pci1710.c
>> @@ -1396,14 +1396,14 @@ static int pci1710_attach(struct
>> comedi_device *dev,
>> int i;
>> int board_index;
>>
>> - printk("comedi%d: adv_pci1710: ", dev->minor);
>> + printk(KERN_INFO "comedi%d: adv_pci1710: ", dev->minor);
>>
>> opt_bus = it->options[0];
>> opt_slot = it->options[1];
>>
>> ret = alloc_private(dev, sizeof(struct pci1710_private));
>> if (ret< 0) {
>> - printk(" - Allocation failed!\n");
>> + printk(KERN_CONT "\n");
>> + printk(KERN_ERR "Comedi%d: adv_pci1710: Allocation failed\n",
>> + dev->minor);
>
> This still isn't correct. The initial printk (KERN_INFO above) has no
> trailing newline. Here you are almost doing it correctly, except that
> you are adding an extra newline. This error message will look like this:
>
> comedi0: adv_pci1710:
> comedi0: adv_pci1710: Allocation failed
>
> Which is just silly. The printk's below are broken in that you are
> using KERN_ERR where you should be using KERN_CONT.
>
> As it stands the code is a little tricky to follow since the trailing
> newline gets added at the end unless there is an error when it gets
> added in place. It might be better just to print the full message,
> including the "comedi%d: adv_pci1710:" bit, at each printk call site
> and get rid of the whole KERN_CONT nonsense altogether. Alternatively
> you could create a comedi_printk function (or use pr_fmt) which wraps
> this up.
In fact, looking at the code again, the printks should probably just be
replaced with the dev_printk functions which should print the necessary
device name information.
~Ryan
^ permalink raw reply [flat|nested] 10+ messages in thread