Re: [PATCH 1/2 v3] Staging: comedi: fix printk() issue in adv_pci1710.c

[Ryan Mallon]

On 19/07/11 16:07, Ryan Mallon wrote:
> On 19/07/11 15:50, Ravishankar wrote:
> > From: Ravishankar<ravi.shankar@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/