linux-kernel - Re: [PATCH 1 of 5 ] /drivers/char ioremap balancing/ returncode check

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]

Message-ID: <46C355AC.3040604@gmail.com>
Date:	Wed, 15 Aug 2007 21:36:12 +0200
From:	Jiri Slaby <jirislaby@...il.com>
To:	postfail@...hmail.com
CC:	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 1 of 5 ] /drivers/char ioremap balancing/ returncode check

Scott Thompson napsal(a):
> On Mon, 13 Aug 2007 16:17:29 -0400 Jiri Slaby <jirislaby@...il.com> 
> wrote:
>>> diff --git a/drivers/char/sx.c b/drivers/char/sx.c
>>> index 85a2328..30829ed 100644
>>> --- a/drivers/char/sx.c
>>> +++ b/drivers/char/sx.c
>>> @@ -2627,6 +2627,12 @@ static void __devinit fix_sx_pci(struct 
>>> pci_dev *pdev, struct sx_board *board)
>>>  	pci_read_config_dword(pdev, PCI_BASE_ADDRESS_0, &hwbase);
>>>  	hwbase &= PCI_BASE_ADDRESS_MEM_MASK;
>>>  	rebase = ioremap(hwbase, 0x80);
>>> +	if (!rebase) {
>>> +		printk(KERN_DEBUG 
>>> +			"sx:ioremap fail, unable to perform cntrl reg fix\n");
>>> +		return;
>>> +	}
>>> +		
>> 1) this should be pci_iomap(pdev, 0, 0x80)
> The ioremap call was there before my patch and I'm just trying to 
> audit ioremap calls to make sure that they are cleaned up and have 
> their return codes checked.   If you want me to take care of that 
> "while I'm in the neighborhood", let me know, but trying to avoid 
> too much scope creep on my audit to increase chances it actually 
> gets included...

Yup, while you are at it, make yet another patch, which will convert this to
pci_iomap/unmap.

> 
>> 2) KERN_DEBUG is inappropriate and wrap the string in the middle 
>> and not in the
>> beginning
> 
> Agree on the wordwrap, several artifacts were created on this 
> patchset and this whole thing will be resubmitted through a 
> different mail client. 

Aha. Anyways, if it's more that 80 columns in width, split it into more lines.

> As for KERN_DEBUG, KERN_DEBUG is used in the function following 
> this call as notification that this workaround was required.  When 
> I audit code as I am here I generally try to blend the patch in 
> with the style and conventions of the code around it.  If 
> KERN_DEBUG is inappropriate on the subsequent call, both values 
> should be changed.  

Only a string, where you warn an user, that something went wrong and this and
over that won't work... KERN_DEBUG messages are usually NOT logged. I suggest
KERN_ERR for mapping failure and KERN_INFO for the message about the workaround
is being used.

-- 
Jiri Slaby (jirislaby@...il.com)
Faculty of Informatics, Masaryk University
-
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/