Prev: KVM: SVM: Coding style cleanup
Next: Email Back Shortly
From: Oliver Neukum on 24 Feb 2010 13:30
Am Mittwoch, 24. Februar 2010 17:00:49 schrieb Bruno Prémont:
> +static int picolcd_raw_event(struct hid_device *hdev,
> + struct hid_report *report, u8 *raw_data, int size)
> +{
> + struct picolcd_data *data = hid_get_drvdata(hdev);
> + char hexdata[25];
> + int i;
> +
> + if (data == NULL)
> + return 1;
> +
> + for (i = 0; i < sizeof(hexdata) / 2; i++)
> + sprintf(hexdata+2*i, "%02hhx", raw_data[i]);
> + if (size >= sizeof(hexdata)/2) {
> + hexdata[sizeof(hexdata)-4] = '.';
> + hexdata[sizeof(hexdata)-3] = '.';
> + hexdata[sizeof(hexdata)-2] = '.';
> + hexdata[sizeof(hexdata)-1] = '\0';
> + } else
> + hexdata[size*2] = '\0';
> +
> + switch (report->id) {
> + case REPORT_KEYPAD:
> + if (size == 3 && raw_data[0] == 0x11 && data->input_keys) {
> + return picolcd_raw_keypad(hdev, report, raw_data+1, size-1);
> + } else {
> + dbg_hid(PICOLCD_NAME " unsupported key event (%d bytes): 0x%s\n", size, hexdata);
> + break;
> + }
> + break;
> + case REPORT_VERSION:
> + if (size == 3)
> + dev_info(&hdev->dev, "Firmware version is %hd.%hd\n", raw_data[1], raw_data[2]);
> +
> + spin_lock(&data->lock);

If I recall correctly raw_event is called in interrupt. As you take a spinlock here,
the lock in code not called in interrupt must disable interrupts, or you may
deadlock.

Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Rick L. Vinyard, Jr. on 24 Feb 2010 16:50

Oliver Neukum wrote:
> Am Mittwoch, 24. Februar 2010 17:00:49 schrieb Bruno Prémont:
>> +static int picolcd_raw_event(struct hid_device *hdev,
>> + struct hid_report *report, u8 *raw_data, int size)
>> +{
>> + struct picolcd_data *data = hid_get_drvdata(hdev);
>> + char hexdata[25];
>> + int i;
>> +
>> + if (data == NULL)
>> + return 1;
>> +
>> + for (i = 0; i < sizeof(hexdata) / 2; i++)
>> + sprintf(hexdata+2*i, "%02hhx", raw_data[i]);
>> + if (size >= sizeof(hexdata)/2) {
>> + hexdata[sizeof(hexdata)-4] = '.';
>> + hexdata[sizeof(hexdata)-3] = '.';
>> + hexdata[sizeof(hexdata)-2] = '.';
>> + hexdata[sizeof(hexdata)-1] = '\0';
>> + } else
>> + hexdata[size*2] = '\0';
>> +
>> + switch (report->id) {
>> + case REPORT_KEYPAD:
>> + if (size == 3 && raw_data[0] == 0x11 &&
>> data->input_keys) {
>> + return picolcd_raw_keypad(hdev, report,
>> raw_data+1, size-1);
>> + } else {
>> + dbg_hid(PICOLCD_NAME " unsupported key event (%d
>> bytes): 0x%s\n", size, hexdata);
>> + break;
>> + }
>> + break;
>> + case REPORT_VERSION:
>> + if (size == 3)
>> + dev_info(&hdev->dev, "Firmware version is
>> %hd.%hd\n", raw_data[1], raw_data[2]);
>> +
>> + spin_lock(&data->lock);
>
> If I recall correctly raw_event is called in interrupt. As you take a
> spinlock here,
> the lock in code not called in interrupt must disable interrupts, or you
> may
> deadlock.
>

The issue, as I understand it is that non-interrupt code may obtain the
lock and then the interrupt code is executed... hence the deadlock and the
need to use spin_lock_irqsave() and spin_unlock_irqrestore().

If that is correct, is there any problem with the following approach?

The key difference is the replacement of spin_lock() with spin_trylock()
such that if the non-interrupt code has already obtained the lock, the
interrupt will not deadlock but instead take the else path and schedule a
framebuffer update at the next interval.

static void g13_fb_urb_completion(struct urb *urb)
{
struct g13_data *data = urb->context;
spin_unlock(&data->fb_urb_lock);
}

static int g13_fb_send(struct hid_device *hdev)
{
struct usb_interface *intf;
struct usb_device *usb_dev;
struct g13_data *data = hid_get_g13data(hdev);

struct usb_host_endpoint *ep;
unsigned int pipe;
int retval = 0;

/*
* Try and lock the framebuffer urb to prevent access if we have
* submitted it. If we can't lock it we'll have to delay this update
* until the next framebuffer interval.
*
* Fortunately, we already have the infrastructure in place with the
* framebuffer deferred I/O driver to schedule the delayed update.
*/
if (likely(spin_trylock(&data->fb_urb_lock))) {
/* Get the usb device to send the image on */
intf = to_usb_interface(hdev->dev.parent);
usb_dev = interface_to_usbdev(intf);

pipe = usb_sndintpipe(usb_dev, 0x02);

ep = usb_dev->ep_out[usb_pipeendpoint(pipe)];

if (unlikely(!ep)) {
spin_unlock(&data->fb_urb_lock);
return -EINVAL;
}

pipe = (pipe & ~(3 << 30)) | (PIPE_INTERRUPT << 30);
usb_fill_int_urb(data->fb_urb, usb_dev, pipe, data->fb_vbitmap,
G13_VBITMAP_SIZE, g13_fb_urb_completion, NULL,
ep->desc.bInterval);
data->fb_urb->context = data;
data->fb_urb->actual_length = 0;

retval = usb_submit_urb(data->fb_urb, GFP_NOIO);
if (unlikely(retval < 0)) {
/*
* We need to unlock the framebuffer urb lock since
* the urb submission failed and therefore
* g13_fb_urb_completion() won't be called.
*/
spin_unlock(&data->fb_urb_lock);
return retval;
}
} else {
schedule_delayed_work(&data->fb_info->deferred_work,
data->fb_defio.delay);
}

return retval;
}

Thanks,

--

Rick

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Oliver Neukum on 24 Feb 2010 23:20
Am Mittwoch, 24. Februar 2010 22:44:53 schrieb Rick L. Vinyard, Jr.:
> The issue, as I understand it is that non-interrupt code may obtain the
> lock and then the interrupt code is executed... hence the deadlock and the
> need to use spin_lock_irqsave() and spin_unlock_irqrestore().

Yes.

> If that is correct, is there any problem with the following approach?

Why not always a workqueue or alternatively spin_lock_irq() when
you are not in interrupt? This approach seems needlessly complicated.

Secondly, when you hold a spinlock, you must use GFP_ATOMIC.
GFP_NOIO is insufficient.

Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Kosina on 25 Feb 2010 06:10
On Wed, 24 Feb 2010, Rick L. Vinyard, Jr. wrote:

> >> +static int picolcd_raw_event(struct hid_device *hdev,
> >> + struct hid_report *report, u8 *raw_data, int size)
> >> +{
> >> + struct picolcd_data *data = hid_get_drvdata(hdev);
> >> + char hexdata[25];
> >> + int i;
> >> +
> >> + if (data == NULL)
> >> + return 1;
> >> +
> >> + for (i = 0; i < sizeof(hexdata) / 2; i++)
> >> + sprintf(hexdata+2*i, "%02hhx", raw_data[i]);
> >> + if (size >= sizeof(hexdata)/2) {
> >> + hexdata[sizeof(hexdata)-4] = '.';
> >> + hexdata[sizeof(hexdata)-3] = '.';
> >> + hexdata[sizeof(hexdata)-2] = '.';
> >> + hexdata[sizeof(hexdata)-1] = '\0';
> >> + } else
> >> + hexdata[size*2] = '\0';
> >> +
> >> + switch (report->id) {
> >> + case REPORT_KEYPAD:
> >> + if (size == 3 && raw_data[0] == 0x11 &&
> >> data->input_keys) {
> >> + return picolcd_raw_keypad(hdev, report,
> >> raw_data+1, size-1);
> >> + } else {
> >> + dbg_hid(PICOLCD_NAME " unsupported key event (%d
> >> bytes): 0x%s\n", size, hexdata);
> >> + break;
> >> + }
> >> + break;
> >> + case REPORT_VERSION:
> >> + if (size == 3)
> >> + dev_info(&hdev->dev, "Firmware version is
> >> %hd.%hd\n", raw_data[1], raw_data[2]);
> >> +
> >> + spin_lock(&data->lock);
> >
> > If I recall correctly raw_event is called in interrupt.

Yes, that is correct.

> The issue, as I understand it is that non-interrupt code may obtain the
> lock and then the interrupt code is executed... hence the deadlock and
> the need to use spin_lock_irqsave() and spin_unlock_irqrestore().

Exactly. All the spinlocks that are aquired in interrupt code-paths must
be acquired with _irqsave()/_irqrestore() from the non-interrupt code to
prevent exactly this kind of deadlock.
>
> The key difference is the replacement of spin_lock() with spin_trylock()
> such that if the non-interrupt code has already obtained the lock, the
> interrupt will not deadlock but instead take the else path and schedule a
> framebuffer update at the next interval.

Why is _irqsave() and/or deferred work not enough? The aproach with
_trylock() seems to be overly complicated for no good reason (I personally
become very suspicious every time I see code that is using _trylock()).

[ by the way, Rick, are you planning to resubmit the G13 driver with
incorporated feedback from the last review round? ]

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Kosina on 25 Feb 2010 06:10
On Wed, 24 Feb 2010, Bruno Prémont wrote:

> +config HID_PICOLCD
> + tristate "Minibox PicoLCD (graphic version)"
> + depends on FB
> + select FB_DEFERRED_IO
> + select FB_SYS_FILLRECT
> + select FB_SYS_COPYAREA
> + select FB_SYS_IMAGEBLIT
> + select FB_SYS_FOPS
> + select BACKLIGHT_LCD_SUPPORT
> + select BACKLIGHT_CLASS_DEVICE
> + select LCD_CLASS_DEVICE

I guess you need dependency on USB_HID as well, right?

> --- a/drivers/hid/hid-picolcd.c
> +++ b/drivers/hid/hid-picolcd.c
> @@ -0,0 +1,1075 @@
> +/***************************************************************************
> + * Copyright (C) 2010 by Bruno Prémont <bonbons(a)linux-vserver.org> *
> + * *
> + * Based on Logitech G13 driver (v0.4) *
> + * Copyright (C) 2009 by Rick L. Vinyard, Jr. <rvinyard(a)cs.nmsu.edu> *
> + * *
> + * This program is free software: you can redistribute it and/or modify *
> + * it under the terms of the GNU General Public License as published by *
> + * the Free Software Foundation, either version 2 of the License. *

I support removing the 'or any later' clause, but I think your version has
the grammar wrong :)


> +/* Update fb_vbitmap from the screen_base and send changed tiles to device */
> +static void picolcd_fb_update(struct picolcd_data *data)
> +{
> + int chip, tile;
> +
> + spin_lock(&data->lock);
> + if (!(data->status & PICOLCD_READY_FB)) {
> + spin_unlock(&data->lock);
> + picolcd_fb_reset(data->hdev, 0);
> + } else
> + spin_unlock(&data->lock);

Please put the brackets to the else branch as well.

Also, it'd be great if the framebuffer part would get Ack from someone
more familiar with framebuffer code than me.

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2 3
Prev: KVM: SVM: Coding style cleanup
Next: Email Back Shortly