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

[Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>]

[-- Attachment #1: Type: text/plain, Size: 4372 bytes --]

On Wed, Jun 09, 2010 at 11:28:47AM +0100, Alan Jenkins wrote:
> 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.
> 
> 

Hello, Alan.

Thanks for your review.

> > 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().
> 
> 

I really don't know of anything that may change the rfkill state behind
our back. However, would this work in the case a new version comes out
with this same ACPI interface that does it? And keeping the query method
would only be a performance issue? If that's the case, and there are no
strong opinions about using the cache, I think we could keep the method.
The hardware interface allow us to query and I think it's better to
point it out that the hardware has this capability. What do you think
about it?

> >+	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.
> 

Gee! At the time, I only read the RFKILL=n implementation in the header.
It returns ERR_PTR(-ENODEV), while the RFKILL=y implementation does
indeed return NULL. This is inconsistent interface, and we'd better fix
it, in my opinion. But for the time, we must fix the users here.

> 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).
> 

I see why eeepc_laptop would still work right now. But I think it's
risky for the casual driver writer to trust the rfkill device is there
while it isn't. If this is by design, to bite those driver writers that
do "stupid" things (like not using the right interface) and that they
should still manipulate hardware as if the rfkill device was there,
that's OK. But is this really by design or should we fix it? For
example, eeepc_laptop does all the wlan pci device hotplug stuff and
also write some ACPI values. And it will do that if RFKILL is disabled
but will not do it if RFKILL is enabled but device allocation fails. So,
should we document that ERR_PTR(-ENODEV) is not a failure, but a
"dummy"?

In any case, classmate-laptop does need a fix! Care to make a patch?

Thanks again for the review and my best regards.
Cascardo.

> 
> Regards
> Alan

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]