On Wed, Jun 09, 2010 at 08:32:59PM +0200, Johannes Berg wrote:
> 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
>
Thanks, Johannes.
I think this is a fix for 2.6.35, then. Otherwise, we get a BUG_ON in
case rfkill_alloc fails. If there was not a BUG_ON at rfkill_register,
there would be a NULL pointer dereference.
I will check the driver for the RFKILL=n case. If it's sane, I'll just
change the check for the NULL check.
Best regards,
Cascardo.
Attachment:
signature.asc
Description: Digital signature
