linux-kernel - Re: gpio patches in mmotm

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 for Android: free password hash cracker in your pocket

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

Message-ID: <Pine.LNX.4.64.0804080812420.3556@axis700.grange>
Date:	Tue, 8 Apr 2008 08:28:37 +0200 (CEST)
From:	Guennadi Liakhovetski <g.liakhovetski@...gutronix.de>
To:	Uwe Kleine-König <Uwe.Kleine-Koenig@...i.com>
cc:	David Brownell <david-b@...bell.net>, linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: gpio patches in mmotm

On Tue, 8 Apr 2008, Uwe Kleine-König wrote:

> > I'm storing the GPIO number locally, and if the system doesn't have a 
> > valid GPIO for me, I'm storing an invalid GPIO number. Then at any time if 
> > the GPIO has to be used, I just verify if gpio_is_valid(), and if not, 
> > return an error code for this request, but the driver remains otherwise 
> > functional.
> OK, so in your driver you have:
> 
> 	if (gpio_is_valid(gpio)) {
> 		/* We have a data bus switch. */
> 		ret = gpio_request(gpio, "mt9m001");
> 		if (ret < 0) {
> 			dev_err(&mt9m001->client->dev, "Cannot get GPIO %u\n",
> 				gpio);
> 			return ret;
> 		}
> 		ret = gpio_direction_output(gpio, 0);
> 		if (ret < 0) {
> 			...
> 
> 
> In my eyes the following is better:
> 
> 	/* Do we have a data bus switch? */
> 	ret = gpio_request(gpio, "mt9m001");
> 	if (ret < 0) {
> 		if (ret != -EINVAL) {
> 			dev_err(...);
> 			return ret;
> 		}
> 	} else {
> 		ret = gpio_direction_output(gpio, 0);
> 		if (ret < 0) {
> 			...

Yes, you could do that. But then you have to test either before calling 
gpio_set_value_cansleep() or inside it. And the test you have to perform 
_is_ the validity check, so, you need it anyway.

> Then you don't need to extend the API.  Moreover with your variant the
> check that gpio is valid must be done twice[1].

Actually three times. The one before gpio_free() is not actually needed, 
right, it is anyway checked inside. But gpio_set_value_cansleep() doesn't 
check, so, it would be rude to call it with an invalid value.

> [1] OK, gpio_is_valid and gpio_request might be inline functions, but
> for "my" architecture it is not.

Which arch is it? As I said, you could simplify the two specific camera 
drivers by removing the checks where they are redundant. But on other 
occasions the checks have to be done anyway, so, it is not a question of 
runtime performance (apart from maybe the difference between calling a 
function and executing inline), but just of an extra API member, which you 
can have different opinions about:-)

Thanks
Guennadi
---
Guennadi Liakhovetski
--
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/