[PATCH] classmate-laptop: Add RFKILL support.

[PATCH] classmate-laptop: Add RFKILL support.

[Thadeu Lima de Souza Cascardo]

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>
---
 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
@@ -24,6 +24,7 @@
 #include <acpi/acpi_drivers.h>
 #include <linux/backlight.h>
 #include <linux/input.h>
+#include <linux/rfkill.h>
 
 MODULE_LICENSE("GPL");
 
@@ -37,7 +38,7 @@ struct cmpc_accel {
 
 #define CMPC_ACCEL_HID		"ACCE0000"
 #define CMPC_TABLET_HID		"TBLT0000"
-#define CMPC_BL_HID		"IPML200"
+#define CMPC_IPML_HID	"IPML200"
 #define CMPC_KEYS_HID		"FnBT0000"
 
 /*
@@ -461,43 +462,168 @@ static const struct backlight_ops cmpc_bl_ops = {
 	.update_status = cmpc_bl_update_status
 };
 
-static int cmpc_bl_add(struct acpi_device *acpi)
+/*
+ * RFKILL code.
+ */
+
+static acpi_status cmpc_get_rfkill_wlan(acpi_handle handle,
+					unsigned long long *value)
 {
-	struct backlight_properties props;
+	union acpi_object param;
+	struct acpi_object_list input;
+	unsigned long long output;
+	acpi_status status;
+
+	param.type = ACPI_TYPE_INTEGER;
+	param.integer.value = 0xC1;
+	input.count = 1;
+	input.pointer = &param;
+	status = acpi_evaluate_integer(handle, "GRDI", &input, &output);
+	if (ACPI_SUCCESS(status))
+		*value = output;
+	return status;
+}
+
+static acpi_status cmpc_set_rfkill_wlan(acpi_handle handle,
+					unsigned long long value)
+{
+	union acpi_object param[2];
+	struct acpi_object_list input;
+	acpi_status status;
+	unsigned long long output;
+
+	param[0].type = ACPI_TYPE_INTEGER;
+	param[0].integer.value = 0xC1;
+	param[1].type = ACPI_TYPE_INTEGER;
+	param[1].integer.value = value;
+	input.count = 2;
+	input.pointer = param;
+	status = acpi_evaluate_integer(handle, "GWRI", &input, &output);
+	return status;
+}
+
+static void cmpc_rfkill_query(struct rfkill *rfkill, void *data)
+{
+	acpi_status status;
+	acpi_handle handle;
+	unsigned long long state;
+	bool blocked;
+
+	handle = data;
+	status = cmpc_get_rfkill_wlan(handle, &state);
+	if (ACPI_SUCCESS(status)) {
+		blocked = state & 1 ? false : true;
+		rfkill_set_sw_state(rfkill, blocked);
+	}
+}
+
+static int cmpc_rfkill_block(void *data, bool blocked)
+{
+	acpi_status status;
+	acpi_handle handle;
+	unsigned long long state;
+
+	handle = data;
+	status = cmpc_get_rfkill_wlan(handle, &state);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+	if (blocked)
+		state &= ~1;
+	else
+		state |= 1;
+	status = cmpc_set_rfkill_wlan(handle, state);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+	return 0;
+}
+
+static const struct rfkill_ops cmpc_rfkill_ops = {
+	.query = cmpc_rfkill_query,
+	.set_block = cmpc_rfkill_block,
+};
+
+/*
+ * Common backlight and rfkill code.
+ */
+
+struct ipml200_dev {
 	struct backlight_device *bd;
+	struct rfkill *rf;
+};
+
+static int cmpc_ipml_add(struct acpi_device *acpi)
+{
+	int retval;
+	struct ipml200_dev *ipml;
+	struct backlight_properties props;
+
+	ipml = kmalloc(sizeof(*ipml), GFP_KERNEL);
+	if (ipml == NULL)
+		return -ENOMEM;
 
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.max_brightness = 7;
-	bd = backlight_device_register("cmpc_bl", &acpi->dev, acpi->handle,
-				       &cmpc_bl_ops, &props);
-	if (IS_ERR(bd))
-		return PTR_ERR(bd);
-	dev_set_drvdata(&acpi->dev, bd);
+	ipml->bd = backlight_device_register("cmpc_bl", &acpi->dev,
+					     acpi->handle, &cmpc_bl_ops,
+					     &props);
+	if (IS_ERR(ipml->bd)) {
+		retval = PTR_ERR(ipml->bd);
+		goto out_bd;
+	}
+
+	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;
+	}
+
+	dev_set_drvdata(&acpi->dev, ipml);
 	return 0;
+
+out_bd:
+	kfree(ipml);
+	return retval;
 }
 
-static int cmpc_bl_remove(struct acpi_device *acpi, int type)
+static int cmpc_ipml_remove(struct acpi_device *acpi, int type)
 {
-	struct backlight_device *bd;
+	struct ipml200_dev *ipml;
+
+	ipml = dev_get_drvdata(&acpi->dev);
+
+	backlight_device_unregister(ipml->bd);
+
+	if (ipml->rf) {
+		rfkill_unregister(ipml->rf);
+		rfkill_destroy(ipml->rf);
+	}
+
+	kfree(ipml);
 
-	bd = dev_get_drvdata(&acpi->dev);
-	backlight_device_unregister(bd);
 	return 0;
 }
 
-static const struct acpi_device_id cmpc_bl_device_ids[] = {
-	{CMPC_BL_HID, 0},
+static const struct acpi_device_id cmpc_ipml_device_ids[] = {
+	{CMPC_IPML_HID, 0},
 	{"", 0}
 };
 
-static struct acpi_driver cmpc_bl_acpi_driver = {
+static struct acpi_driver cmpc_ipml_acpi_driver = {
 	.owner = THIS_MODULE,
 	.name = "cmpc",
 	.class = "cmpc",
-	.ids = cmpc_bl_device_ids,
+	.ids = cmpc_ipml_device_ids,
 	.ops = {
-		.add = cmpc_bl_add,
-		.remove = cmpc_bl_remove
+		.add = cmpc_ipml_add,
+		.remove = cmpc_ipml_remove
 	}
 };
 
@@ -580,7 +706,7 @@ static int cmpc_init(void)
 	if (r)
 		goto failed_keys;
 
-	r = acpi_bus_register_driver(&cmpc_bl_acpi_driver);
+	r = acpi_bus_register_driver(&cmpc_ipml_acpi_driver);
 	if (r)
 		goto failed_bl;
 
@@ -598,7 +724,7 @@ failed_accel:
 	acpi_bus_unregister_driver(&cmpc_tablet_acpi_driver);
 
 failed_tablet:
-	acpi_bus_unregister_driver(&cmpc_bl_acpi_driver);
+	acpi_bus_unregister_driver(&cmpc_ipml_acpi_driver);
 
 failed_bl:
 	acpi_bus_unregister_driver(&cmpc_keys_acpi_driver);
@@ -611,7 +737,7 @@ static void cmpc_exit(void)
 {
 	acpi_bus_unregister_driver(&cmpc_accel_acpi_driver);
 	acpi_bus_unregister_driver(&cmpc_tablet_acpi_driver);
-	acpi_bus_unregister_driver(&cmpc_bl_acpi_driver);
+	acpi_bus_unregister_driver(&cmpc_ipml_acpi_driver);
 	acpi_bus_unregister_driver(&cmpc_keys_acpi_driver);
 }
 
@@ -621,7 +747,7 @@ module_exit(cmpc_exit);
 static const struct acpi_device_id cmpc_device_ids[] = {
 	{CMPC_ACCEL_HID, 0},
 	{CMPC_TABLET_HID, 0},
-	{CMPC_BL_HID, 0},
+	{CMPC_IPML_HID, 0},
 	{CMPC_KEYS_HID, 0},
 	{"", 0}
 };
-- 
1.6.6.1

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

[Matthew Garrett]

On Tue, Apr 27, 2010 at 01:15:00PM -0300, 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.

Is this purely a software-controlled toggle, or can the firmware change 
the state as well? If the latter, how is synchronisation between 
hardware and OS state handled?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Thadeu Lima de Souza Cascardo]

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

On Tue, Apr 27, 2010 at 05:22:27PM +0100, Matthew Garrett wrote:
> On Tue, Apr 27, 2010 at 01:15:00PM -0300, 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.
> 
> Is this purely a software-controlled toggle, or can the firmware change 
> the state as well? If the latter, how is synchronisation between 
> hardware and OS state handled?
> 
> -- 
> Matthew Garrett | mjg59@srcf.ucam.org

It's purely a soft block device. I'll add that in the commit message
with any other changes suggested or requested.

I'd really like some feedback about the implementation. I've done tests
around here, but I may still have gotten something wrong. And I sure
hope I haven't messed with the backlight. But nothing points that I've
done so.

Anyway, I'd like some review.

Thanks,
Cascardo.

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

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

[Matthew Garrett]

Ok, looks fine - I've merged this for -next.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Alan Jenkins]

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

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

[Thadeu Lima de Souza Cascardo]

[-- 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 --]

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

[Johannes Berg]

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

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

[Thadeu Lima de Souza Cascardo]

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

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] classmate-laptop: should check for NULL as retval for rfkill_alloc

[Thadeu Lima de Souza Cascardo]

rfkill_alloc returns NULL when it fails if RFKILL is enabled. When RFKILL is
disabled, its return value of ERR_PTR(-ENODEV) is OK to use as all rfkill
functions will work with it, as they are simply empty stubs.

Reported-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: platform-driver-x86@vger.kernel.org
Cc: mjg@redhat.com
Cc: don@syst.com.br
Cc: rpurdie@rpsys.net
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
---
 drivers/platform/x86/classmate-laptop.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/classmate-laptop.c b/drivers/platform/x86/classmate-laptop.c
index 3bf399f..a0aaf5b 100644
--- a/drivers/platform/x86/classmate-laptop.c
+++ b/drivers/platform/x86/classmate-laptop.c
@@ -573,9 +573,12 @@ static int cmpc_ipml_add(struct acpi_device *acpi)
 
 	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)) {
+	/*
+	 * If RFKILL is disabled, rfkill_alloc will return ERR_PTR(-ENODEV).
+	 * This is OK, however, since all other uses of the device will not
+	 * derefence it.
+	 */
+	if (!ipml->rf) {
 		retval = rfkill_register(ipml->rf);
 		if (retval) {
 			rfkill_destroy(ipml->rf);
-- 
1.7.1

Re: [PATCH] classmate-laptop: should check for NULL as retval for rfkill_alloc

[Johannes Berg]

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

> +	/*
> +	 * If RFKILL is disabled, rfkill_alloc will return ERR_PTR(-ENODEV).
> +	 * This is OK, however, since all other uses of the device will not
> +	 * derefence it.
> +	 */
> +	if (!ipml->rf) {

the ! shouldn't be there

johannes

[PATCH] classmate-laptop: should check for NULL as retval for rfkill_alloc

[Thadeu Lima de Souza Cascardo]

rfkill_alloc returns NULL when it fails if RFKILL is enabled. When RFKILL is
disabled, its return value of ERR_PTR(-ENODEV) is OK to use as all rfkill
functions will work with it, as they are simply empty stubs.

Reported-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: platform-driver-x86@vger.kernel.org
Cc: mjg@redhat.com
Cc: don@syst.com.br
Cc: rpurdie@rpsys.net
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
---
 drivers/platform/x86/classmate-laptop.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/classmate-laptop.c b/drivers/platform/x86/classmate-laptop.c
index 3bf399f..73ea76e 100644
--- a/drivers/platform/x86/classmate-laptop.c
+++ b/drivers/platform/x86/classmate-laptop.c
@@ -573,16 +573,17 @@ static int cmpc_ipml_add(struct acpi_device *acpi)
 
 	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)) {
+	/*
+	 * If RFKILL is disabled, rfkill_alloc will return ERR_PTR(-ENODEV).
+	 * This is OK, however, since all other uses of the device will not
+	 * derefence it.
+	 */
+	if (ipml->rf) {
 		retval = rfkill_register(ipml->rf);
 		if (retval) {
 			rfkill_destroy(ipml->rf);
 			ipml->rf = NULL;
 		}
-	} else {
-		ipml->rf = NULL;
 	}
 
 	dev_set_drvdata(&acpi->dev, ipml);
-- 
1.7.1

Re: [PATCH] classmate-laptop: should check for NULL as retval for rfkill_alloc

[Thadeu Lima de Souza Cascardo]

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

On Wed, Jun 09, 2010 at 09:46:17PM +0200, Johannes Berg wrote:
> On Wed, 2010-06-09 at 16:39 -0300, Thadeu Lima de Souza Cascardo wrote:
> 
> > +	/*
> > +	 * If RFKILL is disabled, rfkill_alloc will return ERR_PTR(-ENODEV).
> > +	 * This is OK, however, since all other uses of the device will not
> > +	 * derefence it.
> > +	 */
> > +	if (!ipml->rf) {
> 
> the ! shouldn't be there
> 
> johannes
> 

Sorry for that. The else case is unneeded too. Removed in the next
patch.

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