linux-kernel - Re: [PATCH] net: rfkill: add generic gpio rfkill driver

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: <1304762503.3574.5.camel@jlt3.sipsolutions.net>
Date:	Sat, 07 May 2011 12:01:43 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	Rhyland Klein <rklein@...dia.com>
Cc:	olof@...om.net, linux-wireless@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: rfkill: add generic gpio rfkill driver

On Fri, 2011-05-06 at 17:40 -0700, Rhyland Klein wrote:

[snip]

Looks nice! A few comments below:

> + * @blocked:           block rf enabled (default is true)

(see below)

> +struct rfkill_gpio_platform_data {
> +	char			*name;

> +	char			*power_clk_name;

const?

> +	bool			blocked;

The comments say this is default "true", but I don't think that makes
sense since this struct would be defined somewhere else and be static
const so zero-initialised. Also, are you sure that even having this
makes sense? (see below)

> +config RFKILL_GPIO
> +	bool "GPIO RFKILL driver"
> +	depends on RFKILL && GPIOLIB
> +	help
> +	  If you say yes here you get support of a generic gpio RFKILL
> +	  driver. Platform needs to define the resources required

That appears to be cut short?

> +#define GPIO_NAME_LEN  40

Is there really no definition yet? Maybe it would make sense to
dynamically allocate?

> +struct rfkill_gpio_data {
> +	struct rfkill_gpio_platform_data	*pdata;
> +	struct rfkill				*rfkill_dev;
> +	char					reset_name[GPIO_NAME_LEN];
> +	char					shutdown_name[GPIO_NAME_LEN];
> +	struct					clk *pwr_clk;

That last line is a little oddly formatted.

> +	/* setup initial state */
> +	rfkill_gpio_set_power(rfkill, pdata->blocked);
> +	rfkill_set_states(rfkill->rfkill_dev, pdata->blocked, false);
> +
> +	ret = rfkill_register(rfkill->rfkill_dev);

So if you do this, rfkill core will set the persistent thing. We
discovered a while ago (and I mentioned that in the original thread)
there will be quirky behaviour. I would recommend leaving this out and
just relying on the rfkill core to call set_block() a little after
rfkill_register() to sync the state of this device with the desired
system state.

johannes

--
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/