linux-kernel - Re: [PATCH v2] Staging: comedi: fix printk issue in adv_pci

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 27 Jul 2011 14:35:52 +0200 (CEST)
From:	Jesper Juhl <jj@...osbits.net>
To:	Ravishankar <ravishankarkm32@...il.com>
cc:	gregkh@...e.de, wfp5p@...ginia.edu, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org,
	Ravishankar <ravi.shankar@...enturtles.in>
Subject: Re: [PATCH v2] Staging: comedi: fix printk issue in adv_pci_dio.c

On Wed, 27 Jul 2011, Ravishankar wrote:

> From: Ravishankar <ravi.shankar@...enturtles.in>
> 
> This is a patch to the adv_pci_dio.c file that fixes up a printk warning found by the checkpatch.pl tool
> 
> Signed-off-by: Ravishankar <ravishankarkm32@...il.com>
> ---
>  drivers/staging/comedi/drivers/adv_pci_dio.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/adv_pci_dio.c b/drivers/staging/comedi/drivers/adv_pci_dio.c
> index d23799b..1d2261f 100644
> --- a/drivers/staging/comedi/drivers/adv_pci_dio.c
> +++ b/drivers/staging/comedi/drivers/adv_pci_dio.c
> @@ -1106,11 +1106,11 @@ static int pci_dio_attach(struct comedi_device *dev,
>  	unsigned long iobase;
>  	struct pci_dev *pcidev = NULL;
>  
> -	printk("comedi%d: adv_pci_dio: ", dev->minor);
> +	printk(KERN_INFO "comedi%d: adv_pci_dio: ", dev->minor);
>  

This printk() is used both for printing out information in the case of 
success, in which case the KERN_INFO level is fine. But it is also used to 
print out error messages in case of failure, in which case KERN_WARNING 
would probably be better. So I'm wondering if it wouldn't be better to 
restructure the code so that the printing of error messages and success 
info becomes two seperate printk()'s each with the apropriate level.


>  	ret = alloc_private(dev, sizeof(struct pci_dio_private));
>  	if (ret < 0) {
> -		printk(", Error: Cann't allocate private memory!\n");
> +		printk(KERN_CONT ", Error: Cann't allocate private memory!\n");

Might as well fix the spelling error ( s/Cann't/Can't/ ) while you are 
changing the line anyway.


>  		return -ENOMEM;
>  	}
>  
> @@ -1140,19 +1140,19 @@ static int pci_dio_attach(struct comedi_device *dev,
>  	}
>  
>  	if (!dev->board_ptr) {
> -		printk(", Error: Requested type of the card was not found!\n");
> +		printk(KERN_WARNING ", Error: Requested type of the card was not "
> +		"found!\n");

As Joe Perches already pointed out to you, this is a continuation of the 
first printk() and should be using KERN_CONT.


>  		return -EIO;
>  	}
>  
>  	if (comedi_pci_enable(pcidev, driver_pci_dio.driver_name)) {
>  		printk
> -		    (", Error: Can't enable PCI device and request regions!\n");
> +		(KERN_WARNING ", Error: Can't enable PCI device and request "
> +		"regions!\n");

This one as well. And it has got to be possible to find a less hideous way 
to break that line..

what about:

  printk(KERN_CONT
         ", Error: Can't enable PCI device and request regions!\n");

? If it even needs to be broken at all (the 80 column rule is not fixed in 
stone).


>  		return -EIO;
>  	}
>  	iobase = pci_resource_start(pcidev, this_board->main_pci_region);
> -	printk(", b:s:f=%d:%d:%d, io=0x%4lx",
> -	       pcidev->bus->number, PCI_SLOT(pcidev->devfn),
> -	       PCI_FUNC(pcidev->devfn), iobase);
> +	printk(KERN_CONT ", b:s:f=%d:%d:%d, io=0x%4lx",
> +	       pcidev->bus->number, PCI_SLOT(pcidev->devfn),
> +	       PCI_FUNC(pcidev->devfn), iobase);
> 
>  	dev->iobase = iobase;
>  	dev->board_name = this_board->name;
> @@ -1178,11 +1178,11 @@ static int pci_dio_attach(struct comedi_device *dev,
>  
>  	ret = alloc_subdevices(dev, n_subdevices);
>  	if (ret < 0) {
> -		printk(", Error: Cann't allocate subdevice memory!\n");
> +		printk(KERN_CONT ", Error: Cann't allocate subdevice memory!\n");
>  		return ret;
>  	}
>  
> -	printk(".\n");
> +	printk(KERN_CONT ".\n");
>  
>  	subdev = 0;
>  
 
Please take the feedback people give you serious. Joe kindly pointed out 
your mistakes the last time you posted this and yet your next patch has 
the exact same mistakes.

Read through your patches before you send them and double check each and 
every change - then check it again.

-- 
Jesper Juhl <jj@...osbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

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