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

[Ryan Mallon <rmallon@gmail.com>]

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