Messages in this thread | | | Date | Wed, 20 Jul 2011 09:44:33 +1000 | From | Ryan Mallon <> | Subject | Re: [PATCH 1/2 v3] Staging: comedi: fix printk() issue in adv_pci1710.c |
| |
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
| |