linux-kernel - Re: [PATCH 1/3] platform: add common resource requesting and mapping helper

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120131113358.GM2471@pengutronix.de>
Date:	Tue, 31 Jan 2012 12:33:58 +0100
From:	Wolfram Sang <w.sang@...gutronix.de>
To:	Barry Song <21cnbao@...il.com>
Cc:	Barry Song <Barry.Song@....com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	linux-kernel@...r.kernel.org, workgroup.linux@....com,
	Erik Gilling <konkers@...gle.com>,
	Grant Likely <grant.likely@...retlab.ca>,
	linux-mtd@...ts.infradead.org, Barry Song <Baohua.Song@....com>,
	David Woodhouse <dwmw2@...radead.org>,
	Atsushi Nemoto <anemo@....ocn.ne.jp>
Subject: Re: [PATCH 1/3] platform: add common resource requesting and
 mapping helper

Barry,

> > You don't need to do the error checking for 'res'. You can simply do
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > base = devm_request_and_ioremap(&dev->dev, res);
> 
> i do know  devm_request_and_ioremap() does res checking. but that is
> implicit, confused and not a smart way actually.

I agree about the implicit thing (keep in mind the function is new). But
calling a function "not smart" because it checks its arguments? I do
like the NULL check of kfree() for example.

> actually, no people by now really use the implicit checking. that
> shows people don't really think that is a good programming way.

I'd think most people just copy&paste and don't have an opinion either
way, so the quantity doesn't show much.

> > devm_request_and_ioremap() will check res. Given that, I don't think
> > we can save a lot with another wrapper.
> 
> i think we can save some.
> The story begins from Grant's feedback in:
> http://www.spinics.net/lists/arm-kernel/msg157644.html

I am not sure using 'platform_devm_request_and_ioremap' and later using
plain 'devm_*' functions (without platform_-prefix) is less confusing.
The alternative would be to check which helper functions also use
'struct resource' and if they do checks on that. If all do that, you
would have the simple rule, that you only need to check yourself if you
access it yourself.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)