| Message ID | 4A89E768.7010207@dell.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
Hi Mario, the general rule for patches on LKML is that you send them inline and not as attachment. A successful workaround for the ones with Exchange only mail account is to use git send-email. > --- drivers/platform/x86/compal-laptop.c.old 2009-08-17 > 07:01:36.244874430 -0500 > +++ drivers/platform/x86/compal-laptop.c 2009-08-17 > 07:02:15.012836625 -0500 > @@ -52,6 +52,7 @@ > #include <linux/backlight.h> > #include <linux/platform_device.h> > #include <linux/autoconf.h> > +#include <linux/rfkill.h> > > #define COMPAL_DRIVER_VERSION "0.2.6" > > @@ -64,6 +65,9 @@ > #define WLAN_MASK 0x01 > #define BT_MASK 0x02 > > +static struct rfkill *wifi_rfkill; > +static struct rfkill *bluetooth_rfkill; > + > static int force; > module_param(force, bool, 0); > MODULE_PARM_DESC(force, "Force driver load, ignore DMI data"); > @@ -89,6 +93,87 @@ > return (int) result; > } > > +static void compal_rfkill_query(struct rfkill *rfkill, void *data) > +{ > + unsigned long radio = (unsigned long) data; > + u8 result; > + bool blocked; > + > + ec_read(COMPAL_EC_COMMAND_WIRELESS, &result); > + > + if ((result & KILLSWITCH_MASK) == 0) > + blocked = 1; > + else if (radio == WLAN_MASK) > + blocked = !(result & WLAN_MASK); You are using spaces instead of tabs here. > + else > + blocked = !((result & BT_MASK) >> 1); > + > + rfkill_set_sw_state(rfkill,blocked); > + rfkill_set_hw_state(rfkill,0); > +} > + > +static int compal_rfkill_set(void *data, bool blocked) > +{ > + unsigned long radio = (unsigned long) data; > + u8 result, value; > + > + ec_read(COMPAL_EC_COMMAND_WIRELESS, &result); > + > + if ((result & KILLSWITCH_MASK) == 0) > + return -EINVAL; > + else { > + if (!blocked) > + value = (u8) (result | radio); > + else > + value = (u8) (result & ~radio); > + ec_write(COMPAL_EC_COMMAND_WIRELESS, value); > + } This else part is utterly stupid. There is no need to have an else statement. You already left the function. So get rid of it. And at the same time this code becomes readable. > + return 0; > +} > + > +static const struct rfkill_ops compal_rfkill_ops = { > + .set_block = compal_rfkill_set, > + .query = compal_rfkill_query, > +}; > + > +static int setup_rfkill(void) > +{ > + int ret; > + > + wifi_rfkill = rfkill_alloc("compal-wifi", NULL, > RFKILL_TYPE_WLAN, > + &compal_rfkill_ops, (void *) > WLAN_MASK); > + if (!wifi_rfkill) { > + ret = -ENOMEM; > + goto err_wifi; > + } > + ret = rfkill_register(wifi_rfkill); > + if (ret) > + goto err_wifi; > + > + bluetooth_rfkill = rfkill_alloc("compal-bluetooth", NULL, > RFKILL_TYPE_BLUETOOTH, > + &compal_rfkill_ops, (void *) > BT_MASK); > + if (!bluetooth_rfkill) { > + ret = -ENOMEM; > + goto err_bt; > + } > + ret = rfkill_register(bluetooth_rfkill); > + if (ret) > + goto err_bt; > + > + return 0; > +err_bt: > + rfkill_destroy(bluetooth_rfkill); > + if (bluetooth_rfkill) > + rfkill_unregister(bluetooth_rfkill); > +err_wifi: > + rfkill_destroy(wifi_rfkill); > + if (wifi_rfkill) > + rfkill_unregister(wifi_rfkill); I don't understand how this is not a potential NULL pointer dereference. There might some good luck that the pointer is still valid at that time, but I highly doubt it. So please unregister before destory. > + > + return ret; > +} > + > static int set_wlan_state(int state) > { > u8 result, value; > @@ -357,6 +442,12 @@ > if (!force && !dmi_check_system(compal_dmi_table)) > return -ENODEV; > > + ret = setup_rfkill(); > + if (ret) { > + printk(KERN_WARNING "compal-laptop: Unable to setup > rfkill\n"); > + goto fail_rfkill; > + } > + > /* Register backlight stuff */ > > if (!acpi_video_backlight_support()) { > @@ -410,6 +501,13 @@ > > backlight_device_unregister(compalbl_device); > > +fail_rfkill: > + > + if (wifi_rfkill) > + rfkill_unregister(wifi_rfkill); > + if (bluetooth_rfkill) > + rfkill_unregister(bluetooth_rfkill); > + What non-sense is this. If setup_rfkill() fails, then both RFKILL switches got unregistered if they ever have been registered. > return ret; > } > > @@ -420,6 +518,10 @@ > platform_device_unregister(compal_device); > platform_driver_unregister(&compal_driver); > backlight_device_unregister(compalbl_device); > + if (wifi_rfkill) > + rfkill_unregister(wifi_rfkill); > + if (bluetooth_rfkill) > + rfkill_unregister(bluetooth_rfkill); Same here. It should never ever succeeded in the first place. You can call it conditionally. > > printk(KERN_INFO "compal-laptop: driver unloaded.\n"); > } > Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/18/09, Marcel Holtmann <marcel@holtmann.org> wrote: > Hi Mario, ... >> +static int setup_rfkill(void) >> +{ >> + int ret; >> + >> + wifi_rfkill = rfkill_alloc("compal-wifi", NULL, >> RFKILL_TYPE_WLAN, >> + &compal_rfkill_ops, (void *) >> WLAN_MASK); >> + if (!wifi_rfkill) { >> + ret = -ENOMEM; >> + goto err_wifi; >> + } >> + ret = rfkill_register(wifi_rfkill); >> + if (ret) >> + goto err_wifi; >> + >> + bluetooth_rfkill = rfkill_alloc("compal-bluetooth", NULL, >> RFKILL_TYPE_BLUETOOTH, >> + &compal_rfkill_ops, (void *) >> BT_MASK); >> + if (!bluetooth_rfkill) { >> + ret = -ENOMEM; >> + goto err_bt; >> + } >> + ret = rfkill_register(bluetooth_rfkill); >> + if (ret) >> + goto err_bt; >> + >> + return 0; >> +err_bt: >> + rfkill_destroy(bluetooth_rfkill); >> + if (bluetooth_rfkill) >> + rfkill_unregister(bluetooth_rfkill); >> +err_wifi: >> + rfkill_destroy(wifi_rfkill); >> + if (wifi_rfkill) >> + rfkill_unregister(wifi_rfkill); > > I don't understand how this is not a potential NULL pointer dereference. > There might some good luck that the pointer is still valid at that time, > but I highly doubt it. So please unregister before destory. Wrong as well :-). If you fail to register wifi_rfkill, you should *only* call rfkill_destroy(). So I think it should look like this: + if (wifi_rfkill) + rfkill_unregister(wifi_rfkill); +err_wifi: + rfkill_destroy(wifi_rfkill); ... >> @@ -420,6 +518,10 @@ >> platform_device_unregister(compal_device); >> platform_driver_unregister(&compal_driver); >> backlight_device_unregister(compalbl_device); >> + if (wifi_rfkill) >> + rfkill_unregister(wifi_rfkill); >> + if (bluetooth_rfkill) >> + rfkill_unregister(bluetooth_rfkill); > > Same here. It should never ever succeeded in the first place. You can > call it conditionally. They're already called conditionally. I assume you mean unconditionally here. I agree with all your other comments. Although I wouldn't call the return/else style issue stupid, I'd just say it was confused :-). Regards Alan -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/18/09, Mario Limonciello <mario_limonciello@dell.com> wrote: > In order to be useful in modern kernels and standard interfaces, > compal-laptop should have rfkill support. This patch adds it. > -- > Mario Limonciello > *Dell | Linux Engineering* > mario_limonciello@dell.com > I have some comments of my own as well, so here goes. > +static void compal_rfkill_query(struct rfkill *rfkill, void *data) > +{ > + unsigned long radio = (unsigned long) data; > + u8 result; > + bool blocked; > + > + ec_read(COMPAL_EC_COMMAND_WIRELESS, &result); > + > + if ((result & KILLSWITCH_MASK) == 0) > + blocked = 1; > + else if (radio == WLAN_MASK) > + blocked = !(result & WLAN_MASK); > + else > + blocked = !((result & BT_MASK) >> 1); Ick, this is overdone. You should just be able to do + else + blocked = !(result & radio); > + rfkill_set_sw_state(rfkill,blocked); > + rfkill_set_hw_state(rfkill,0); This last line is redundant. The HW state will default to "unblocked" if you never set it. > +static int compal_rfkill_set(void *data, bool blocked) > +{ > + unsigned long radio = (unsigned long) data; > + u8 result, value; > + > + ec_read(COMPAL_EC_COMMAND_WIRELESS, &result); > + > + if ((result & KILLSWITCH_MASK) == 0) > + return -EINVAL; > + else { > + if (!blocked) > + value = (u8) (result | radio); > + else > + value = (u8) (result & ~radio); > + ec_write(COMPAL_EC_COMMAND_WIRELESS, value); > + } > + > + return 0; > +} Note that the return value of rfkill_set is not propagated to userspace through the /dev/rfkill interface. The way you use "KILLSWITCH_MASK", it looks like a hard rfkill bit that covers all radios. Why don't you report it as a hard block on each rfkill device? Note that strictly speaking, you should only call one rfkill_set function within the query() method. "@query: query the rfkill block state(s) and call exactly one of the rfkill_set{,_hw,_sw}_state family of functions." (I'd recommend reading the comments in include/linux/rfkill.h if you haven't already). So If you do want to set both hw and sw states in query(), you should use rfkill_set_states(). > +static int setup_rfkill(void) > +{ > + int ret; > + > + wifi_rfkill = rfkill_alloc("compal-wifi", NULL, RFKILL_TYPE_WLAN, > + &compal_rfkill_ops, (void *) WLAN_MASK); I think you should use "compal_device->dev" in place of NULL here. Otherwise the rfkill devices will appear in sysfs as "virtual devices", belonging to no "physical" device. > + bluetooth_rfkill = rfkill_alloc("compal-bluetooth", NULL, RFKILL_TYPE_BLUETOOTH, > + &compal_rfkill_ops, (void *) BT_MASK); Same here. Regards Alan -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/18/09, Marcel Holtmann <marcel@holtmann.org> wrote: > Hi Mario, ... >> +static void compal_rfkill_query(struct rfkill *rfkill, void *data) >> +{ >> + unsigned long radio = (unsigned long) data; >> + u8 result; >> + bool blocked; >> + >> + ec_read(COMPAL_EC_COMMAND_WIRELESS, &result); >> + >> + if ((result & KILLSWITCH_MASK) == 0) >> + blocked = 1; >> + else if (radio == WLAN_MASK) >> + blocked = !(result & WLAN_MASK); > > You are using spaces instead of tabs here. Btw Mario, scripts/checkpatch.pl is a good way to check for whitespace errors and a bunch of other trivia. So human patch review can concentrate on the important stuff :-). Thanks Alan -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/18/09, Alan Jenkins <sourcejedi.lkml@googlemail.com> wrote: > On 8/18/09, Mario Limonciello <mario_limonciello@dell.com> wrote: >> In order to be useful in modern kernels and standard interfaces, >> compal-laptop should have rfkill support. This patch adds it. >> -- >> Mario Limonciello >> *Dell | Linux Engineering* >> mario_limonciello@dell.com >> > > I have some comments of my own as well, so here goes. > >> +static void compal_rfkill_query(struct rfkill *rfkill, void *data) >> +{ >> + unsigned long radio = (unsigned long) data; >> + u8 result; >> + bool blocked; >> + >> + ec_read(COMPAL_EC_COMMAND_WIRELESS, &result); >> + >> + if ((result & KILLSWITCH_MASK) == 0) >> + blocked = 1; > >> + else if (radio == WLAN_MASK) >> + blocked = !(result & WLAN_MASK); >> + else >> + blocked = !((result & BT_MASK) >> 1); > > Ick, this is overdone. You should just be able to do > > + else > + blocked = !(result & radio); > >> + rfkill_set_sw_state(rfkill,blocked); >> + rfkill_set_hw_state(rfkill,0); > > This last line is redundant. The HW state will default to "unblocked" > if you never set it. > >> +static int compal_rfkill_set(void *data, bool blocked) >> +{ >> + unsigned long radio = (unsigned long) data; >> + u8 result, value; >> + >> + ec_read(COMPAL_EC_COMMAND_WIRELESS, &result); >> + >> + if ((result & KILLSWITCH_MASK) == 0) >> + return -EINVAL; >> + else { >> + if (!blocked) >> + value = (u8) (result | radio); >> + else >> + value = (u8) (result & ~radio); >> + ec_write(COMPAL_EC_COMMAND_WIRELESS, value); >> + } >> + >> + return 0; >> +} > > Note that the return value of rfkill_set is not propagated to > userspace through the /dev/rfkill interface. > > The way you use "KILLSWITCH_MASK", it looks like a hard rfkill bit > that covers all radios. Why don't you report it as a hard block on > each rfkill device? P.S. If you do this, you probably also want to set up polling. I think you should be able to reuse the query() function for the poll() method (but you should check yourself whether that makes sense). Thanks again Alan -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/18/09, Alan Jenkins <sourcejedi.lkml@googlemail.com> wrote: > On 8/18/09, Marcel Holtmann <marcel@holtmann.org> wrote: >> Hi Mario, > > ... > >>> +static int setup_rfkill(void) >>> +{ >>> + int ret; >>> + >>> + wifi_rfkill = rfkill_alloc("compal-wifi", NULL, >>> RFKILL_TYPE_WLAN, >>> + &compal_rfkill_ops, (void *) >>> WLAN_MASK); >>> + if (!wifi_rfkill) { >>> + ret = -ENOMEM; >>> + goto err_wifi; >>> + } >>> + ret = rfkill_register(wifi_rfkill); >>> + if (ret) >>> + goto err_wifi; >>> + >>> + bluetooth_rfkill = rfkill_alloc("compal-bluetooth", NULL, >>> RFKILL_TYPE_BLUETOOTH, >>> + &compal_rfkill_ops, (void *) >>> BT_MASK); >>> + if (!bluetooth_rfkill) { >>> + ret = -ENOMEM; >>> + goto err_bt; >>> + } >>> + ret = rfkill_register(bluetooth_rfkill); >>> + if (ret) >>> + goto err_bt; >>> + >>> + return 0; >>> +err_bt: >>> + rfkill_destroy(bluetooth_rfkill); >>> + if (bluetooth_rfkill) >>> + rfkill_unregister(bluetooth_rfkill); >>> +err_wifi: >>> + rfkill_destroy(wifi_rfkill); >>> + if (wifi_rfkill) >>> + rfkill_unregister(wifi_rfkill); >> >> I don't understand how this is not a potential NULL pointer dereference. >> There might some good luck that the pointer is still valid at that time, >> but I highly doubt it. So please unregister before destory. > > Wrong as well :-). > > If you fail to register wifi_rfkill, you should *only* call > rfkill_destroy(). So I think it should look like this: > > + if (wifi_rfkill) > + rfkill_unregister(wifi_rfkill); > +err_wifi: > + rfkill_destroy(wifi_rfkill); > > ... > >>> @@ -420,6 +518,10 @@ >>> platform_device_unregister(compal_device); >>> platform_driver_unregister(&compal_driver); >>> backlight_device_unregister(compalbl_device); >>> + if (wifi_rfkill) >>> + rfkill_unregister(wifi_rfkill); >>> + if (bluetooth_rfkill) >>> + rfkill_unregister(bluetooth_rfkill); >> >> Same here. It should never ever succeeded in the first place. You can >> call it conditionally. > > They're already called conditionally. I assume you mean unconditionally > here. Also, you're missing the calls to rfkill_destroy() here. Whew, I think that's everything. I hope you find the feedback useful, despite it being a little fragmented. Regards Alan -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- drivers/platform/x86/compal-laptop.c.old 2009-08-17 07:01:36.244874430 -0500 +++ drivers/platform/x86/compal-laptop.c 2009-08-17 07:02:15.012836625 -0500 @@ -52,6 +52,7 @@ #include <linux/backlight.h> #include <linux/platform_device.h> #include <linux/autoconf.h> +#include <linux/rfkill.h> #define COMPAL_DRIVER_VERSION "0.2.6" @@ -64,6 +65,9 @@ #define WLAN_MASK 0x01 #define BT_MASK 0x02 +static struct rfkill *wifi_rfkill; +static struct rfkill *bluetooth_rfkill; + static int force; module_param(force, bool, 0); MODULE_PARM_DESC(force, "Force driver load, ignore DMI data"); @@ -89,6 +93,87 @@ return (int) result; } +static void compal_rfkill_query(struct rfkill *rfkill, void *data) +{ + unsigned long radio = (unsigned long) data; + u8 result; + bool blocked; + + ec_read(COMPAL_EC_COMMAND_WIRELESS, &result); + + if ((result & KILLSWITCH_MASK) == 0) + blocked = 1; + else if (radio == WLAN_MASK) + blocked = !(result & WLAN_MASK); + else + blocked = !((result & BT_MASK) >> 1); + + rfkill_set_sw_state(rfkill,blocked); + rfkill_set_hw_state(rfkill,0); +} + +static int compal_rfkill_set(void *data, bool blocked) +{ + unsigned long radio = (unsigned long) data; + u8 result, value; + + ec_read(COMPAL_EC_COMMAND_WIRELESS, &result); + + if ((result & KILLSWITCH_MASK) == 0) + return -EINVAL; + else { + if (!blocked) + value = (u8) (result | radio); + else + value = (u8) (result & ~radio); + ec_write(COMPAL_EC_COMMAND_WIRELESS, value); + } + + return 0; +} + +static const struct rfkill_ops compal_rfkill_ops = { + .set_block = compal_rfkill_set, + .query = compal_rfkill_query, +}; + +static int setup_rfkill(void) +{ + int ret; + + wifi_rfkill = rfkill_alloc("compal-wifi", NULL, RFKILL_TYPE_WLAN, + &compal_rfkill_ops, (void *) WLAN_MASK); + if (!wifi_rfkill) { + ret = -ENOMEM; + goto err_wifi; + } + ret = rfkill_register(wifi_rfkill); + if (ret) + goto err_wifi; + + bluetooth_rfkill = rfkill_alloc("compal-bluetooth", NULL, RFKILL_TYPE_BLUETOOTH, + &compal_rfkill_ops, (void *) BT_MASK); + if (!bluetooth_rfkill) { + ret = -ENOMEM; + goto err_bt; + } + ret = rfkill_register(bluetooth_rfkill); + if (ret) + goto err_bt; + + return 0; +err_bt: + rfkill_destroy(bluetooth_rfkill); + if (bluetooth_rfkill) + rfkill_unregister(bluetooth_rfkill); +err_wifi: + rfkill_destroy(wifi_rfkill); + if (wifi_rfkill) + rfkill_unregister(wifi_rfkill); + + return ret; +} + static int set_wlan_state(int state) { u8 result, value; @@ -357,6 +442,12 @@ if (!force && !dmi_check_system(compal_dmi_table)) return -ENODEV; + ret = setup_rfkill(); + if (ret) { + printk(KERN_WARNING "compal-laptop: Unable to setup rfkill\n"); + goto fail_rfkill; + } + /* Register backlight stuff */ if (!acpi_video_backlight_support()) { @@ -410,6 +501,13 @@ backlight_device_unregister(compalbl_device); +fail_rfkill: + + if (wifi_rfkill) + rfkill_unregister(wifi_rfkill); + if (bluetooth_rfkill) + rfkill_unregister(bluetooth_rfkill); + return ret; } @@ -420,6 +518,10 @@ platform_device_unregister(compal_device); platform_driver_unregister(&compal_driver); backlight_device_unregister(compalbl_device); + if (wifi_rfkill) + rfkill_unregister(wifi_rfkill); + if (bluetooth_rfkill) + rfkill_unregister(bluetooth_rfkill); printk(KERN_INFO "compal-laptop: driver unloaded.\n"); }