On Fri, Mar 19, 2010 at 11:51:31AM -0700, Dmitry Torokhov wrote:
> Hi Yong,
>
> On Fri, Mar 19, 2010 at 09:39:24PM +0800, Yong Wang wrote:
> > +
> > +struct key_entry {
> > + char type;
> > + u8 code;
> > + u16 keycode;
> > +};
> > +
>
> Like Matthew mentioned, please switch to sparse-keymap library.
>
> > +
> > +static void eeepc_wmi_notify(u32 value, void *context)
> > +{
> > + struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> > + static struct key_entry *key;
> > + union acpi_object *obj;
> > + int code;
> > +
> > + wmi_get_event_data(value, &response);
> > +
> > + obj = (union acpi_object *)response.pointer;
> > +
> > + if (obj && obj->type == ACPI_TYPE_INTEGER) {
> > + code = obj->integer.value;
> > +
> > + if (code >= NOTIFY_BRNUP_MIN && code <= NOTIFY_BRNUP_MAX)
> > + code = NOTIFY_BRNUP_MIN;
> > + else if (code >= NOTIFY_BRNDOWN_MIN && code <= NOTIFY_BRNDOWN_MAX)
> > + code = NOTIFY_BRNDOWN_MIN;
> > +
> > + key = eeepc_wmi_get_entry_by_scancode(code);
> > +
> > + if (key) {
> > + input_report_key(eeepc_wmi_input_dev, key->keycode, 1);
> > + input_sync(eeepc_wmi_input_dev);
> > + input_report_key(eeepc_wmi_input_dev, key->keycode, 0);
> > + input_sync(eeepc_wmi_input_dev);
> > + } else
> > + printk(KERN_INFO "EEEPC WMI: Unknown key %x pressed\n", code);
> > + }
>
> You are leaking memory pointed to by obj.
>
> > +
> > +static int __init eeepc_wmi_init(void)
> > +{
> > + int err;
> > +
> > + if (wmi_has_guid(EEEPC_WMI_EVENT_GUID)) {
> > + err = wmi_install_notify_handler(EEEPC_WMI_EVENT_GUID,
> > + eeepc_wmi_notify, NULL);
> > + if (ACPI_FAILURE(err))
> > + return -EINVAL;
> > +
>
> It is not a good idea to install notify handler before registering input
> devive. If event comes too early you will oops. In fact this is a common
> problem in many platfrom drivers.
>
> > + err = eeepc_wmi_input_setup();
> > + if (err) {
> > + wmi_remove_notify_handler(EEEPC_WMI_EVENT_GUID);
> > + return err;
> > + }
> > + }
> > +
> > + return 0;
>
> Do not return success if GUID is not found - do
>
> if (!wmi_has_guid(EEEPC_WMI_EVENT_GUID))
> return -ENODEV;
>
> instead.
>
> > +}
> > +
> > +static void __exit eeepc_wmi_exit(void)
> > +{
> > + if (wmi_has_guid(EEEPC_WMI_EVENT_GUID)) {
>
> ... and you won't have to check guid here.
>
Great! Thank you very much for all the valuable comments. I will review and repost.
-Yong
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
