Re: [PATCH] classmate-laptop: Add RFKILL support.

[Alan Jenkins <alan-jenkins@tuffmail.co.uk>]

Thadeu Lima de Souza Cascardo wrote:
> The RFKILL device shares the same ACPI device used for backlight. So, it
> required a new struct sharing both a backlight_device and a rfkill
> device.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
> ---

Apologies for tardiness.  You did ask for review, so I've scanned it for some common rfkill pitfalls.


>  drivers/platform/x86/classmate-laptop.c |  170 +++++++++++++++++++++++++++----
>  1 files changed, 148 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/platform/x86/classmate-laptop.c b/drivers/platform/x86/classmate-laptop.c
> index 7f9e5dd..3bf399f 100644
> --- a/drivers/platform/x86/classmate-laptop.c
> +++ b/drivers/platform/x86/classmate-laptop.c

> +static const struct rfkill_ops cmpc_rfkill_ops = {
> +	.query = cmpc_rfkill_query,
> +	.set_block = cmpc_rfkill_block,
> +};

You said down-thread that firmware does not change the state.  In that case, I believe the query method is unnecessary


 * @query: query the rfkill block state(s) and call exactly one of the
 *	rfkill_set{,_hw,_sw}_state family of functions. Assign this
 *	method if input events can cause hardware state changes to make
 *	the rfkill core query your driver before setting a requested
 *	block.


Generally, the rfkill core caches the soft blocked state.  It doesn't call query() during registration either - it calls set_block() with a default value (usually "unblocked").  query() is only used to minimize a race with the firmware during set_block().


> +	ipml->rf = rfkill_alloc("cmpc_rfkill", &acpi->dev, RFKILL_TYPE_WLAN,
> +				&cmpc_rfkill_ops, acpi->handle);
> +	/* rfkill_alloc may fail if RFKILL is disabled. We should still work
> +	 * anyway. */
> +	if (!IS_ERR(ipml->rf)) {
> +		retval = rfkill_register(ipml->rf);
> +		if (retval) {
> +			rfkill_destroy(ipml->rf);
> +			ipml->rf = NULL;
> +		}
> +	} else {
> +		ipml->rf = NULL;
> +	}

I think the comment is wrong, and so is the code it references.

rfkill_alloc() is documented as returning NULL on failure, not an ERR_PTR.  So you're going to pass NULL into rfkill_register() on allocation failure, which will BUG out.

eeepc_laptop tests for NULL here, not ERR_PTR.

If you look at the implementation when RFKILL is disabled, rfkill_register() is designed to accept  ERR_PTR(-ENODEV) without complaint.  (And the other rfkill functions won't care either; they'll just be completely empty stubs).


Regards
Alan