linux-kernel - [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-next>] [day] [month] [year] [list]
Message-ID: <CAEP+3OD50SqDyE7kRVQ54WgDMeG0j+Qs8X3CD3i2E8bMjSD7Ow@mail.gmail.com>
Date:	Thu, 28 Jul 2011 11:15:09 +0530
From:	ravi shankar <ravishankarkm32@...il.com>
To:	Jesper Juhl <jj@...osbits.net>
Cc:	gregkh@...e.de, wfp5p@...ginia.edu, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: [PATCH v2] Staging: comedi: fix printk issue in adv_pci_dio.c

Hi,

      Jesper as you told it would be better to make one
printk(KERN_ERR...) for all levels instead of two. What is your
opinion?

Thanks & Regards
 Ravishankar

On Wed, Jul 27, 2011 at 6:05 PM, Jesper Juhl <jj@...osbits.net> wrote:
> 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/