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

[Johannes Berg <johannes@sipsolutions.net>]

On Wed, 2010-06-09 at 15:19 -0300, Thadeu Lima de Souza Cascardo wrote:

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

No, it's perfectly consistent. RFKILL=n returns non-NULL on success,
just as RFKILL=y/m. It's just defined to be always successful for
RFKILL=n, with the special case that it's returning an ERR_PTR for its
own checking in rfkill_register.

All the drivers should do is test for NULL, and if non-NULL proceed as
normal.

> 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"?

That's the intended outcome, yes, so that drivers need NOT worry about
whether RFKILL is built into the system or not at all.

johannes