* Re: [PATCH 08/50] USB: legousbtower: spin_lock in complete() cleanup
From: Oliver Neukum @ 2013-07-11 12:36 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Ming Lei, Greg Kroah-Hartman, linux-usb, Alan Stern, linux-input,
linux-bluetooth, netdev, linux-wireless, linux-media, alsa-devel,
Juergen Stuber
In-Reply-To: <51DEA289.5050509@cogentembedded.com>
On Thursday 11 July 2013 16:18:17 Sergei Shtylyov wrote:
> I don't think this patch passes checkpatch.pl.
This series is a mechanical replacement in dozens of drivers.
We cannot demand nice formatting. If you want to do something
productive, check the locking in the driver.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH 08/50] USB: legousbtower: spin_lock in complete() cleanup
From: Ming Lei @ 2013-07-11 12:42 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Oliver Neukum, Alan Stern, linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Juergen Stuber
In-Reply-To: <51DEA289.5050509-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
On Thu, Jul 11, 2013 at 8:18 PM, Sergei Shtylyov
<sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org> wrote:
> Hello.
>
>
> On 11-07-2013 13:05, Ming Lei wrote:
>
>> Complete() will be run with interrupt enabled, so change to
>> spin_lock_irqsave().
>
>
>> Cc: Juergen Stuber <starblue-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
>> Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> ---
>> drivers/usb/misc/legousbtower.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>
>> diff --git a/drivers/usb/misc/legousbtower.c
>> b/drivers/usb/misc/legousbtower.c
>> index 8089479..4044989 100644
>> --- a/drivers/usb/misc/legousbtower.c
>> +++ b/drivers/usb/misc/legousbtower.c
>> @@ -771,6 +771,7 @@ static void tower_interrupt_in_callback (struct urb
>> *urb)
>> struct lego_usb_tower *dev = urb->context;
>> int status = urb->status;
>> int retval;
>> + unsigned long flags;
>>
>> dbg(4, "%s: enter, status %d", __func__, status);
>>
>> @@ -788,7 +789,7 @@ static void tower_interrupt_in_callback (struct urb
>> *urb)
>> }
>>
>> if (urb->actual_length > 0) {
>> - spin_lock (&dev->read_buffer_lock);
>> + spin_lock_irqsave (&dev->read_buffer_lock, flags);
>> if (dev->read_buffer_length + urb->actual_length <
>> read_buffer_size) {
>> memcpy (dev->read_buffer +
>> dev->read_buffer_length,
>> dev->interrupt_in_buffer,
>> @@ -799,7 +800,7 @@ static void tower_interrupt_in_callback (struct urb
>> *urb)
>> } else {
>> printk(KERN_WARNING "%s: read_buffer overflow, %d
>> bytes dropped", __func__, urb->actual_length);
>> }
>> - spin_unlock (&dev->read_buffer_lock);
>> + spin_unlock_irqrestore (&dev->read_buffer_lock, flags);
>> }
>
>
> I don't think this patch passes checkpatch.pl.
No errors reported from checkpatch.pl, only warnings which isn't introduced
by this patch.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 36/50] media: usb: em28xx: spin_lock in complete() cleanup
From: Devin Heitmueller @ 2013-07-11 12:42 UTC (permalink / raw)
To: Ming Lei
Cc: Greg Kroah-Hartman, linux-usb, Oliver Neukum, Alan Stern,
linux-input, linux-bluetooth, netdev, linux-wireless, linux-media,
alsa-devel, Mauro Carvalho Chehab
In-Reply-To: <1373533573-12272-37-git-send-email-ming.lei@canonical.com>
On Thu, Jul 11, 2013 at 5:05 AM, Ming Lei <ming.lei@canonical.com> wrote:
> Complete() will be run with interrupt enabled, so change to
> spin_lock_irqsave().
>
> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
> drivers/media/usb/em28xx/em28xx-core.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
> index fc157af..0d698f9 100644
> --- a/drivers/media/usb/em28xx/em28xx-core.c
> +++ b/drivers/media/usb/em28xx/em28xx-core.c
> @@ -941,6 +941,7 @@ static void em28xx_irq_callback(struct urb *urb)
> {
> struct em28xx *dev = urb->context;
> int i;
> + unsigned long flags;
>
> switch (urb->status) {
> case 0: /* success */
> @@ -956,9 +957,9 @@ static void em28xx_irq_callback(struct urb *urb)
> }
>
> /* Copy data from URB */
> - spin_lock(&dev->slock);
> + spin_lock_irqsave(&dev->slock, flags);
> dev->usb_ctl.urb_data_copy(dev, urb);
> - spin_unlock(&dev->slock);
> + spin_unlock_irqrestore(&dev->slock, flags);
>
> /* Reset urb buffers */
> for (i = 0; i < urb->number_of_packets; i++) {
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
I actually stumbled across this a couple of weeks ago, and have had an
identical patch running in a local dev tree.
Reviewed-by: Devin Heitmueller <dheitmueller@kernellabs.com>
Tested-by: Devin Heitmueller <dheitmueller@kernellabs.com>
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
^ permalink raw reply
* Re: [PATCH 17/50] USB: serial: sierra: spin_lock in complete() cleanup
From: Sergei Shtylyov @ 2013-07-11 13:02 UTC (permalink / raw)
To: Ming Lei
Cc: Greg Kroah-Hartman, linux-usb, Oliver Neukum, Alan Stern,
linux-input, linux-bluetooth, netdev, linux-wireless, linux-media,
alsa-devel, Johan Hovold
In-Reply-To: <1373533573-12272-18-git-send-email-ming.lei@canonical.com>
Hello.
On 11-07-2013 13:05, Ming Lei wrote:
> Complete() will be run with interrupt enabled, so change to
> spin_lock_irqsave().
> Cc: Johan Hovold <jhovold@gmail.com>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
> drivers/usb/serial/sierra.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
> diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c
> index de958c5..e79b6ad 100644
> --- a/drivers/usb/serial/sierra.c
> +++ b/drivers/usb/serial/sierra.c
> @@ -433,6 +433,7 @@ static void sierra_outdat_callback(struct urb *urb)
> struct sierra_port_private *portdata = usb_get_serial_port_data(port);
> struct sierra_intf_private *intfdata;
> int status = urb->status;
> + unsigned long flags;
>
> intfdata = port->serial->private;
>
> @@ -443,12 +444,12 @@ static void sierra_outdat_callback(struct urb *urb)
> dev_dbg(&port->dev, "%s - nonzero write bulk status "
> "received: %d\n", __func__, status);
>
> - spin_lock(&portdata->lock);
> + spin_lock_irqsave(&portdata->lock, flags);
> --portdata->outstanding_urbs;
> - spin_unlock(&portdata->lock);
> - spin_lock(&intfdata->susp_lock);
> + spin_unlock_irqrestore(&portdata->lock, flags);
> + spin_lock_irqsave(&intfdata->susp_lock, flags);
You are allowing an interrupt enabled window where previously it
wasn't possible. Why notleave these 2 lines as is?
> --intfdata->in_flight;
> - spin_unlock(&intfdata->susp_lock);
> + spin_unlock_irqrestore(&intfdata->susp_lock, flags);
>
> usb_serial_port_softint(port);
> }
WBR, Sergei
^ permalink raw reply
* Re: [PATCH 45/50] sound: usb: usx2y: spin_lock in complete() cleanup
From: Sergei Shtylyov @ 2013-07-11 13:08 UTC (permalink / raw)
To: Ming Lei
Cc: Greg Kroah-Hartman, linux-usb, Oliver Neukum, Alan Stern,
linux-input, linux-bluetooth, netdev, linux-wireless, linux-media,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <1373533573-12272-46-git-send-email-ming.lei@canonical.com>
On 11-07-2013 13:06, Ming Lei wrote:
> Complete() will be run with interrupt enabled, so change to
> spin_lock_irqsave().
Changelog doesn't match the patch.
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: alsa-devel@alsa-project.org
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
> sound/usb/usx2y/usbusx2yaudio.c | 4 ++++
> 1 file changed, 4 insertions(+)
> diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
> index 4967fe9..e2ee893 100644
> --- a/sound/usb/usx2y/usbusx2yaudio.c
> +++ b/sound/usb/usx2y/usbusx2yaudio.c
> @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y)
> struct snd_usX2Y_substream *subs = usX2Y->subs[s];
> if (subs) {
> if (atomic_read(&subs->state) >= state_PRERUNNING) {
> + unsigned long flags;
> +
> + local_irq_save(flags);
> snd_pcm_stop(subs->pcm_substream, SNDRV_PCM_STATE_XRUN);
> + local_irq_restore(flags);
> }
WBR, Sergei
^ permalink raw reply
* Re: [PATCH 46/50] Sound: usb: ua101: spin_lock in complete() cleanup
From: Sergei Shtylyov @ 2013-07-11 13:10 UTC (permalink / raw)
To: Ming Lei
Cc: Greg Kroah-Hartman, linux-usb, Oliver Neukum, Alan Stern,
linux-input, linux-bluetooth, netdev, linux-wireless, linux-media,
alsa-devel, Clemens Ladisch, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <1373533573-12272-47-git-send-email-ming.lei@canonical.com>
On 11-07-2013 13:06, Ming Lei wrote:
Here the subject doesn't match the patch.
> Complete() will be run with interrupt enabled, so disable local
> interrupt before holding a global lock which is held without irqsave.
> Cc: Clemens Ladisch <clemens@ladisch.de>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: alsa-devel@alsa-project.org
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
> sound/usb/misc/ua101.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
> diff --git a/sound/usb/misc/ua101.c b/sound/usb/misc/ua101.c
> index 8b5d2c5..52a60c6 100644
> --- a/sound/usb/misc/ua101.c
> +++ b/sound/usb/misc/ua101.c
> @@ -613,14 +613,24 @@ static int start_usb_playback(struct ua101 *ua)
>
> static void abort_alsa_capture(struct ua101 *ua)
> {
> - if (test_bit(ALSA_CAPTURE_RUNNING, &ua->states))
> + if (test_bit(ALSA_CAPTURE_RUNNING, &ua->states)) {
> + unsigned long flags;
> +
> + local_irq_save(flags);
> snd_pcm_stop(ua->capture.substream, SNDRV_PCM_STATE_XRUN);
> + local_irq_restore(flags);
> + }
> }
>
> static void abort_alsa_playback(struct ua101 *ua)
> {
> - if (test_bit(ALSA_PLAYBACK_RUNNING, &ua->states))
> + if (test_bit(ALSA_PLAYBACK_RUNNING, &ua->states)) {
> + unsigned long flags;
> +
> + local_irq_save(flags);
> snd_pcm_stop(ua->playback.substream, SNDRV_PCM_STATE_XRUN);
> + local_irq_restore(flags);
> + }
> }
WBR, Sergei
^ permalink raw reply
* Re: [PATCH 44/50] sound: usb: caiaq: spin_lock in complete() cleanup
From: Sergei Shtylyov @ 2013-07-11 13:12 UTC (permalink / raw)
To: Ming Lei
Cc: Greg Kroah-Hartman, linux-usb, Oliver Neukum, Alan Stern,
linux-input, linux-bluetooth, netdev, linux-wireless, linux-media,
alsa-devel, Daniel Mack, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <1373533573-12272-45-git-send-email-ming.lei@canonical.com>
On 11-07-2013 13:06, Ming Lei wrote:
> Complete() will be run with interrupt enabled, so change to
> spin_lock_irqsave().
> Cc: Daniel Mack <zonque@gmail.com>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: alsa-devel@alsa-project.org
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
> sound/usb/caiaq/audio.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
> diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
> index 7103b09..e5675ab 100644
> --- a/sound/usb/caiaq/audio.c
> +++ b/sound/usb/caiaq/audio.c
> @@ -672,10 +672,11 @@ static void read_completed(struct urb *urb)
> offset += len;
>
> if (len > 0) {
> - spin_lock(&cdev->spinlock);
> + unsigned long flags;
Emoty line wouldn't hurt here, after declaration.
> + spin_lock_irqsave(&cdev->spinlock, flags);
> fill_out_urb(cdev, out, &out->iso_frame_desc[outframe]);
> read_in_urb(cdev, urb, &urb->iso_frame_desc[frame]);
> - spin_unlock(&cdev->spinlock);
> + spin_unlock_irqrestore(&cdev->spinlock, flags);
WBR, Sergei
^ permalink raw reply
* Re: [PATCH 42/50] media: usb: tlg2300: spin_lock in complete() cleanup
From: Sergei Shtylyov @ 2013-07-11 13:13 UTC (permalink / raw)
To: Ming Lei
Cc: Greg Kroah-Hartman, linux-usb, Oliver Neukum, Alan Stern,
linux-input, linux-bluetooth, netdev, linux-wireless, linux-media,
alsa-devel, Mauro Carvalho Chehab
In-Reply-To: <1373533573-12272-43-git-send-email-ming.lei@canonical.com>
On 11-07-2013 13:06, Ming Lei wrote:
Subject doesn't match the patch.
> Complete() will be run with interrupt enabled, so disable local
> interrupt before holding a global lock which is held without
> irqsave.
>
> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
> drivers/media/usb/tlg2300/pd-alsa.c | 3 +++
> 1 file changed, 3 insertions(+)
> diff --git a/drivers/media/usb/tlg2300/pd-alsa.c b/drivers/media/usb/tlg2300/pd-alsa.c
> index 3f3e141..cbccc96 100644
> --- a/drivers/media/usb/tlg2300/pd-alsa.c
> +++ b/drivers/media/usb/tlg2300/pd-alsa.c
[...]
> @@ -156,6 +157,7 @@ static inline void handle_audio_data(struct urb *urb, int *period_elapsed)
> memcpy(runtime->dma_area + oldptr * stride, cp, len * stride);
>
> /* update the statas */
> + local_irq_save(flags);
> snd_pcm_stream_lock(pa->capture_pcm_substream);
> pa->rcv_position += len;
> if (pa->rcv_position >= runtime->buffer_size)
> @@ -167,6 +169,7 @@ static inline void handle_audio_data(struct urb *urb, int *period_elapsed)
> *period_elapsed = 1;
> }
> snd_pcm_stream_unlock(pa->capture_pcm_substream);
> + local_irq_restore(flags);
> }
WBR, Sergei
^ permalink raw reply
* [PATCH 0/2] Bluetooth: hidp: cleanup HID init and re-enable hidinput_input_event callback
From: Benjamin Tissoires @ 2013-07-11 13:41 UTC (permalink / raw)
To: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
David Herrmann, Jiri Kosina, linux-bluetooth, linux-input,
linux-kernel
Hi guys,
These two patches enhance a little bit the HIDp driver:
- the first one re-implement in a safer way the callback hidinput_input_event,
allowing keyboards leds to be set over bluetooth
- the second one disable the send_report commands called during init, which
are not part of the spec and may fail with some devices (the Wiimote is in
this case, but David guarded it against this problem).
Cheers,
Benjamin
Benjamin Tissoires (2):
Bluetooth: hidp: implement hidinput_input_event callback
Bluetooth: hidp: remove wrong send_report at init
net/bluetooth/hidp/core.c | 40 ++++++++++++++++++++++++++--------------
1 file changed, 26 insertions(+), 14 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback
From: Benjamin Tissoires @ 2013-07-11 13:41 UTC (permalink / raw)
To: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
David Herrmann, Jiri Kosina,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1373550090-30379-1-git-send-email-benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
We can re-enable hidinput_input_event to allow the leds of bluetooth
keyboards to be set.
Now the callbacks uses hid core to retrieve the right HID report to
send, so this version is safer.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
net/bluetooth/hidp/core.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index f13a8da..9c8b50d 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -238,6 +238,31 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
return hidp_send_intr_message(session, hdr, buf, rsize);
}
+static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
+ unsigned int code, int value)
+{
+ struct hid_device *hid = input_get_drvdata(dev);
+ struct hidp_session *session = hid->driver_data;
+ struct hid_field *field;
+ int offset;
+
+ BT_DBG("session %p type %d code %d value %d",
+ session, type, code, value);
+
+ if (type != EV_LED)
+ return -1;
+
+ offset = hidinput_find_field(hid, type, code, &field);
+ if (offset == -1) {
+ hid_warn(dev, "event field not found\n");
+ return -1;
+ }
+
+ hid_set_field(field, offset, value);
+
+ return hidp_send_report(session, field->report);
+}
+
static int hidp_get_raw_report(struct hid_device *hid,
unsigned char report_number,
unsigned char *data, size_t count,
@@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
.stop = hidp_stop,
.open = hidp_open,
.close = hidp_close,
+ .hidinput_input_event = hidp_hidinput_event,
};
/* This function sets up the hid device. It does not add it
--
1.8.3.1
^ permalink raw reply related
* [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init
From: Benjamin Tissoires @ 2013-07-11 13:41 UTC (permalink / raw)
To: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
David Herrmann, Jiri Kosina, linux-bluetooth, linux-input,
linux-kernel
In-Reply-To: <1373550090-30379-1-git-send-email-benjamin.tissoires@redhat.com>
The USB hid implementation does retrieve the reports during the start.
However, this implementation does not call the HID command GET_REPORT
(which would fetch the current status of each report), but use the
DATA command, which is an Output Report (so transmitting data from the
host to the device).
The Wiimote controller is already guarded against this problem in the
protocol, but it is not conformant to the specification to set all the
reports to 0 on start.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
net/bluetooth/hidp/core.c | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 9c8b50d..59d132a 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -703,20 +703,6 @@ static int hidp_parse(struct hid_device *hid)
static int hidp_start(struct hid_device *hid)
{
- struct hidp_session *session = hid->driver_data;
- struct hid_report *report;
-
- if (hid->quirks & HID_QUIRK_NO_INIT_REPORTS)
- return 0;
-
- list_for_each_entry(report, &hid->report_enum[HID_INPUT_REPORT].
- report_list, list)
- hidp_send_report(session, report);
-
- list_for_each_entry(report, &hid->report_enum[HID_FEATURE_REPORT].
- report_list, list)
- hidp_send_report(session, report);
-
return 0;
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH 08/50] USB: legousbtower: spin_lock in complete() cleanup
From: Sergei Shtylyov @ 2013-07-11 13:47 UTC (permalink / raw)
To: Oliver Neukum
Cc: Ming Lei, Greg Kroah-Hartman, linux-usb, Alan Stern, linux-input,
linux-bluetooth, netdev, linux-wireless, linux-media, alsa-devel,
Juergen Stuber
In-Reply-To: <3187374.nPY1jDDWKm@linux-5eaq.site>
Hello.
On 11-07-2013 16:36, Oliver Neukum wrote:
>> I don't think this patch passes checkpatch.pl.
> This series is a mechanical replacement in dozens of drivers.
That mechanicity shows too much in some patches.
> We cannot demand nice formatting. If you want to do something
> productive, check the locking in the driver.
I'm not paid for it and don't have time to do it for free.
> Regards
> Oliver
WBR, Sergei
^ permalink raw reply
* Re: [PATCH 45/50] sound: usb: usx2y: spin_lock in complete() cleanup
From: Takashi Iwai @ 2013-07-11 13:50 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Ming Lei, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Oliver Neukum, Alan Stern, linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Jaroslav Kysela
In-Reply-To: <51DEAE4E.90204-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
At Thu, 11 Jul 2013 17:08:30 +0400,
Sergei Shtylyov wrote:
>
> On 11-07-2013 13:06, Ming Lei wrote:
>
> > Complete() will be run with interrupt enabled, so change to
> > spin_lock_irqsave().
>
> Changelog doesn't match the patch.
Yep, but moreover...
> > Cc: Jaroslav Kysela <perex-/Fr2/VpizcU@public.gmane.org>
> > Cc: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
> > Cc: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org
> > Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> > ---
> > sound/usb/usx2y/usbusx2yaudio.c | 4 ++++
> > 1 file changed, 4 insertions(+)
>
> > diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
> > index 4967fe9..e2ee893 100644
> > --- a/sound/usb/usx2y/usbusx2yaudio.c
> > +++ b/sound/usb/usx2y/usbusx2yaudio.c
> > @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y)
> > struct snd_usX2Y_substream *subs = usX2Y->subs[s];
> > if (subs) {
> > if (atomic_read(&subs->state) >= state_PRERUNNING) {
> > + unsigned long flags;
> > +
> > + local_irq_save(flags);
> > snd_pcm_stop(subs->pcm_substream, SNDRV_PCM_STATE_XRUN);
> > + local_irq_restore(flags);
> > }
... actually this snd_pcm_stop() call should have been covered by
snd_pcm_stream_lock(). Maybe it'd be enough to have a single patch
together with the change, i.e. wrapping with
snd_pcm_stream_lock_irqsave().
I'll prepare the patch for 3.11 independently from your patch series,
so please drop this one.
BTW, the word "cleanup" in the subject is inappropriate. This is
rather a fix together with the core change.
And, I wonder whether we can take a safer approach. When the caller
condition changed, we often introduced either a different ops
(e.g. ioctl case) or a flag for the new condition, at least during the
transition period.
Last but not least, is a conversion to tasklet really preferred?
tasklet is rather an obsoleted infrastructure nowadays, and people
don't recommend to use it any longer, AFAIK.
thanks,
Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init
From: David Herrmann @ 2013-07-11 13:51 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan, Jiri Kosina,
linux-bluetooth@vger.kernel.org, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1373550090-30379-3-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Thu, Jul 11, 2013 at 3:41 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> The USB hid implementation does retrieve the reports during the start.
> However, this implementation does not call the HID command GET_REPORT
> (which would fetch the current status of each report), but use the
> DATA command, which is an Output Report (so transmitting data from the
> host to the device).
> The Wiimote controller is already guarded against this problem in the
> protocol, but it is not conformant to the specification to set all the
> reports to 0 on start.
I always wondered whether report-setup is really needed for BT-HIDP.
The BT profile doesn't mention it but I thought it was part of the
USBHID core specification.
During hid-wiimote development I added support for
HID_QUIRK_NO_INIT_REPORTS to HIDP to silence the wiimote errors. But
if you say that it's specific to USBHID, I am fine with this. I never
read the USBHID specs, though, I rely on your comment here.
Anyway, code looks good:
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks!
David
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> net/bluetooth/hidp/core.c | 14 --------------
> 1 file changed, 14 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 9c8b50d..59d132a 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -703,20 +703,6 @@ static int hidp_parse(struct hid_device *hid)
>
> static int hidp_start(struct hid_device *hid)
> {
> - struct hidp_session *session = hid->driver_data;
> - struct hid_report *report;
> -
> - if (hid->quirks & HID_QUIRK_NO_INIT_REPORTS)
> - return 0;
> -
> - list_for_each_entry(report, &hid->report_enum[HID_INPUT_REPORT].
> - report_list, list)
> - hidp_send_report(session, report);
> -
> - list_for_each_entry(report, &hid->report_enum[HID_FEATURE_REPORT].
> - report_list, list)
> - hidp_send_report(session, report);
> -
> return 0;
> }
>
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback
From: David Herrmann @ 2013-07-11 14:02 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan, Jiri Kosina,
linux-bluetooth@vger.kernel.org, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1373550090-30379-2-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Thu, Jul 11, 2013 at 3:41 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> We can re-enable hidinput_input_event to allow the leds of bluetooth
> keyboards to be set.
> Now the callbacks uses hid core to retrieve the right HID report to
> send, so this version is safer.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> net/bluetooth/hidp/core.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index f13a8da..9c8b50d 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -238,6 +238,31 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
> return hidp_send_intr_message(session, hdr, buf, rsize);
> }
>
> +static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
> + unsigned int code, int value)
> +{
> + struct hid_device *hid = input_get_drvdata(dev);
I dislike that we have to deal with input_get_drvdata() in a transport
driver. It is not obvious from the transport driver that this gives us
an hid_device. But ok, that's how we do it in usbhid and hid-input so
it's another issue..
> + struct hidp_session *session = hid->driver_data;
> + struct hid_field *field;
> + int offset;
> +
> + BT_DBG("session %p type %d code %d value %d",
> + session, type, code, value);
> +
> + if (type != EV_LED)
> + return -1;
> +
> + offset = hidinput_find_field(hid, type, code, &field);
> + if (offset == -1) {
> + hid_warn(dev, "event field not found\n");
> + return -1;
> + }
> +
> + hid_set_field(field, offset, value);
> +
> + return hidp_send_report(session, field->report);
> +}
> +
We had this discussion before (regarding uhid and hidpinput_event), it
would be nice to have a helper in hid-input.c which does this. We copy
the code into every transport driver, which bugs me. But no-one
stepped up to do this, so I am fine.
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks!
David
> static int hidp_get_raw_report(struct hid_device *hid,
> unsigned char report_number,
> unsigned char *data, size_t count,
> @@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
> .stop = hidp_stop,
> .open = hidp_open,
> .close = hidp_close,
> + .hidinput_input_event = hidp_hidinput_event,
> };
>
> /* This function sets up the hid device. It does not add it
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 44/50] sound: usb: caiaq: spin_lock in complete() cleanup
From: Daniel Mack @ 2013-07-11 14:06 UTC (permalink / raw)
To: Ming Lei
Cc: Greg Kroah-Hartman, linux-usb, Oliver Neukum, Alan Stern,
linux-input, linux-bluetooth, netdev, linux-wireless, linux-media,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <1373533573-12272-45-git-send-email-ming.lei@canonical.com>
On 11.07.2013 11:06, Ming Lei wrote:
> Complete() will be run with interrupt enabled, so change to
> spin_lock_irqsave().
>
> Cc: Daniel Mack <zonque@gmail.com>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: alsa-devel@alsa-project.org
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
Sound right to me, thanks.
Acked-by: Daniel Mack <zonque@gmail.com>
> ---
> sound/usb/caiaq/audio.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
> index 7103b09..e5675ab 100644
> --- a/sound/usb/caiaq/audio.c
> +++ b/sound/usb/caiaq/audio.c
> @@ -672,10 +672,11 @@ static void read_completed(struct urb *urb)
> offset += len;
>
> if (len > 0) {
> - spin_lock(&cdev->spinlock);
> + unsigned long flags;
> + spin_lock_irqsave(&cdev->spinlock, flags);
> fill_out_urb(cdev, out, &out->iso_frame_desc[outframe]);
> read_in_urb(cdev, urb, &urb->iso_frame_desc[frame]);
> - spin_unlock(&cdev->spinlock);
> + spin_unlock_irqrestore(&cdev->spinlock, flags);
> check_for_elapsed_periods(cdev, cdev->sub_playback);
> check_for_elapsed_periods(cdev, cdev->sub_capture);
> send_it = 1;
>
^ permalink raw reply
* Re: [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback
From: Benjamin Tissoires @ 2013-07-11 14:10 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan, Jiri Kosina,
linux-bluetooth@vger.kernel.org, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <CANq1E4Tx7cVkWmZHtLvtW4U78uedEUe4Qs40aeyzRORmEBTPBg@mail.gmail.com>
Hi David,
On Thu, Jul 11, 2013 at 4:02 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
>> +static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
>> + unsigned int code, int value)
>> +{
>> + struct hid_device *hid = input_get_drvdata(dev);
>
> I dislike that we have to deal with input_get_drvdata() in a transport
> driver. It is not obvious from the transport driver that this gives us
> an hid_device. But ok, that's how we do it in usbhid and hid-input so
> it's another issue..
right. So we should definitively change the callback signature here.
>
>> + struct hidp_session *session = hid->driver_data;
>> + struct hid_field *field;
>> + int offset;
>> +
>> + BT_DBG("session %p type %d code %d value %d",
>> + session, type, code, value);
>> +
>> + if (type != EV_LED)
>> + return -1;
>> +
>> + offset = hidinput_find_field(hid, type, code, &field);
>> + if (offset == -1) {
>> + hid_warn(dev, "event field not found\n");
>> + return -1;
>> + }
>> +
>> + hid_set_field(field, offset, value);
>> +
>> + return hidp_send_report(session, field->report);
>> +}
>> +
>
> We had this discussion before (regarding uhid and hidpinput_event), it
> would be nice to have a helper in hid-input.c which does this. We copy
> the code into every transport driver, which bugs me. But no-one
> stepped up to do this, so I am fine.
oh, yes, you are right. I'll add it to my todo list (now that I am
responsible of 2/4 code duplication :-S )
>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks for the review!
Cheers,
Benjamin
>
> Thanks!
> David
>
>> static int hidp_get_raw_report(struct hid_device *hid,
>> unsigned char report_number,
>> unsigned char *data, size_t count,
>> @@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
>> .stop = hidp_stop,
>> .open = hidp_open,
>> .close = hidp_close,
>> + .hidinput_input_event = hidp_hidinput_event,
>> };
>>
>> /* This function sets up the hid device. It does not add it
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 45/50] sound: usb: usx2y: spin_lock in complete() cleanup
From: Ming Lei @ 2013-07-11 14:13 UTC (permalink / raw)
To: Takashi Iwai
Cc: Sergei Shtylyov, Greg Kroah-Hartman, linux-usb, Oliver Neukum,
Alan Stern, linux-input, linux-bluetooth, netdev, linux-wireless,
linux-media, alsa-devel, Jaroslav Kysela
In-Reply-To: <s5hip0hb3y6.wl%tiwai@suse.de>
On Thu, Jul 11, 2013 at 9:50 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Thu, 11 Jul 2013 17:08:30 +0400,
> Sergei Shtylyov wrote:
>>
>> On 11-07-2013 13:06, Ming Lei wrote:
>>
>> > Complete() will be run with interrupt enabled, so change to
>> > spin_lock_irqsave().
>>
>> Changelog doesn't match the patch.
>
> Yep, but moreover...
>
>> > Cc: Jaroslav Kysela <perex@perex.cz>
>> > Cc: Takashi Iwai <tiwai@suse.de>
>> > Cc: alsa-devel@alsa-project.org
>> > Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> > ---
>> > sound/usb/usx2y/usbusx2yaudio.c | 4 ++++
>> > 1 file changed, 4 insertions(+)
>>
>> > diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
>> > index 4967fe9..e2ee893 100644
>> > --- a/sound/usb/usx2y/usbusx2yaudio.c
>> > +++ b/sound/usb/usx2y/usbusx2yaudio.c
>> > @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y)
>> > struct snd_usX2Y_substream *subs = usX2Y->subs[s];
>> > if (subs) {
>> > if (atomic_read(&subs->state) >= state_PRERUNNING) {
>> > + unsigned long flags;
>> > +
>> > + local_irq_save(flags);
>> > snd_pcm_stop(subs->pcm_substream, SNDRV_PCM_STATE_XRUN);
>> > + local_irq_restore(flags);
>> > }
>
> ... actually this snd_pcm_stop() call should have been covered by
> snd_pcm_stream_lock(). Maybe it'd be enough to have a single patch
> together with the change, i.e. wrapping with
> snd_pcm_stream_lock_irqsave().
Please use snd_pcm_stream_lock_irqsave() so that I can avoid sending
out similar patch later, :-)
>
> I'll prepare the patch for 3.11 independently from your patch series,
> so please drop this one.
OK, thanks for dealing with that.
>
>
> BTW, the word "cleanup" in the subject is inappropriate. This is
> rather a fix together with the core change.
It is a cleanup since the patchset only addresses lock problem which
is caused by the tasklet conversion.
>
> And, I wonder whether we can take a safer approach. When the caller
> condition changed, we often introduced either a different ops
> (e.g. ioctl case) or a flag for the new condition, at least during the
> transition period.
Interrupt is't enabled until all current drivers are cleaned up, so it is really
safe, please see patch [2].
>
> Last but not least, is a conversion to tasklet really preferred?
> tasklet is rather an obsoleted infrastructure nowadays, and people
> don't recommend to use it any longer, AFAIK.
We discussed the problem in the below link previously[1], Steven
and Thomas suggested to use threaded irq handler, but which
may degrade USB mass storage performance, so we have to
take tasklet now until we rewrite transport part of USB mass storage
driver.
Also the conversion[2] has avoided the tasklet spin lock problem
already.
[1], http://marc.info/?t=137079119200001&r=1&w=2
[2], http://marc.info/?l=linux-usb&m=137286326726326&w=2
Thanks,
--
Ming Lei
^ permalink raw reply
* Re: [PATCH 45/50] sound: usb: usx2y: spin_lock in complete() cleanup
From: Takashi Iwai @ 2013-07-11 14:34 UTC (permalink / raw)
To: Ming Lei
Cc: Sergei Shtylyov, Greg Kroah-Hartman,
linux-usb-u79uwXL29TY76Z2rM5mHXA, Oliver Neukum, Alan Stern,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Jaroslav Kysela
In-Reply-To: <CACVXFVPuv4VD_xCocP2QVOqUqAXm9gjwgwcRn6y=3qXPntOxCQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
At Thu, 11 Jul 2013 22:13:35 +0800,
Ming Lei wrote:
>
> On Thu, Jul 11, 2013 at 9:50 PM, Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org> wrote:
> > At Thu, 11 Jul 2013 17:08:30 +0400,
> > Sergei Shtylyov wrote:
> >>
> >> On 11-07-2013 13:06, Ming Lei wrote:
> >>
> >> > Complete() will be run with interrupt enabled, so change to
> >> > spin_lock_irqsave().
> >>
> >> Changelog doesn't match the patch.
> >
> > Yep, but moreover...
> >
> >> > Cc: Jaroslav Kysela <perex-/Fr2/VpizcU@public.gmane.org>
> >> > Cc: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
> >> > Cc: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org
> >> > Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> >> > ---
> >> > sound/usb/usx2y/usbusx2yaudio.c | 4 ++++
> >> > 1 file changed, 4 insertions(+)
> >>
> >> > diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
> >> > index 4967fe9..e2ee893 100644
> >> > --- a/sound/usb/usx2y/usbusx2yaudio.c
> >> > +++ b/sound/usb/usx2y/usbusx2yaudio.c
> >> > @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y)
> >> > struct snd_usX2Y_substream *subs = usX2Y->subs[s];
> >> > if (subs) {
> >> > if (atomic_read(&subs->state) >= state_PRERUNNING) {
> >> > + unsigned long flags;
> >> > +
> >> > + local_irq_save(flags);
> >> > snd_pcm_stop(subs->pcm_substream, SNDRV_PCM_STATE_XRUN);
> >> > + local_irq_restore(flags);
> >> > }
> >
> > ... actually this snd_pcm_stop() call should have been covered by
> > snd_pcm_stream_lock(). Maybe it'd be enough to have a single patch
> > together with the change, i.e. wrapping with
> > snd_pcm_stream_lock_irqsave().
>
> Please use snd_pcm_stream_lock_irqsave() so that I can avoid sending
> out similar patch later, :-)
>
> >
> > I'll prepare the patch for 3.11 independently from your patch series,
> > so please drop this one.
>
> OK, thanks for dealing with that.
>
> >
> >
> > BTW, the word "cleanup" in the subject is inappropriate. This is
> > rather a fix together with the core change.
>
> It is a cleanup since the patchset only addresses lock problem which
> is caused by the tasklet conversion.
Well, the conversion to irqsave() is needed for the future drop of
irq_save() in the caller, right? Then this isn't a cleanup but a
preparation for movement ahead.
> > And, I wonder whether we can take a safer approach. When the caller
> > condition changed, we often introduced either a different ops
> > (e.g. ioctl case) or a flag for the new condition, at least during the
> > transition period.
>
> Interrupt is't enabled until all current drivers are cleaned up, so it is really
> safe, please see patch [2].
OK.
> > Last but not least, is a conversion to tasklet really preferred?
> > tasklet is rather an obsoleted infrastructure nowadays, and people
> > don't recommend to use it any longer, AFAIK.
>
> We discussed the problem in the below link previously[1], Steven
> and Thomas suggested to use threaded irq handler, but which
> may degrade USB mass storage performance, so we have to
> take tasklet now until we rewrite transport part of USB mass storage
> driver.
>
> Also the conversion[2] has avoided the tasklet spin lock problem
> already.
OK, good to know.
thanks,
Takashi
> [1], http://marc.info/?t=137079119200001&r=1&w=2
> [2], http://marc.info/?l=linux-usb&m=137286326726326&w=2
>
> Thanks,
> --
> Ming Lei
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 45/50] sound: usb: usx2y: spin_lock in complete() cleanup
From: Ming Lei @ 2013-07-11 14:52 UTC (permalink / raw)
To: Takashi Iwai
Cc: Sergei Shtylyov, Greg Kroah-Hartman, linux-usb, Oliver Neukum,
Alan Stern, linux-input, linux-bluetooth, netdev, linux-wireless,
linux-media, alsa-devel, Jaroslav Kysela
In-Reply-To: <s5h7ggx413u.wl%tiwai@suse.de>
On Thu, Jul 11, 2013 at 10:34 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Thu, 11 Jul 2013 22:13:35 +0800,
> Ming Lei wrote:
>>
>> On Thu, Jul 11, 2013 at 9:50 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> > At Thu, 11 Jul 2013 17:08:30 +0400,
>> > Sergei Shtylyov wrote:
>> >>
>> >> On 11-07-2013 13:06, Ming Lei wrote:
>> >>
>> >> > Complete() will be run with interrupt enabled, so change to
>> >> > spin_lock_irqsave().
>> >>
>> >> Changelog doesn't match the patch.
>> >
>> > Yep, but moreover...
>> >
>> >> > Cc: Jaroslav Kysela <perex@perex.cz>
>> >> > Cc: Takashi Iwai <tiwai@suse.de>
>> >> > Cc: alsa-devel@alsa-project.org
>> >> > Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> >> > ---
>> >> > sound/usb/usx2y/usbusx2yaudio.c | 4 ++++
>> >> > 1 file changed, 4 insertions(+)
>> >>
>> >> > diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
>> >> > index 4967fe9..e2ee893 100644
>> >> > --- a/sound/usb/usx2y/usbusx2yaudio.c
>> >> > +++ b/sound/usb/usx2y/usbusx2yaudio.c
>> >> > @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y)
>> >> > struct snd_usX2Y_substream *subs = usX2Y->subs[s];
>> >> > if (subs) {
>> >> > if (atomic_read(&subs->state) >= state_PRERUNNING) {
>> >> > + unsigned long flags;
>> >> > +
>> >> > + local_irq_save(flags);
>> >> > snd_pcm_stop(subs->pcm_substream, SNDRV_PCM_STATE_XRUN);
>> >> > + local_irq_restore(flags);
>> >> > }
>> >
>> > ... actually this snd_pcm_stop() call should have been covered by
>> > snd_pcm_stream_lock(). Maybe it'd be enough to have a single patch
>> > together with the change, i.e. wrapping with
>> > snd_pcm_stream_lock_irqsave().
>>
>> Please use snd_pcm_stream_lock_irqsave() so that I can avoid sending
>> out similar patch later, :-)
>>
>> >
>> > I'll prepare the patch for 3.11 independently from your patch series,
>> > so please drop this one.
>>
>> OK, thanks for dealing with that.
>>
>> >
>> >
>> > BTW, the word "cleanup" in the subject is inappropriate. This is
>> > rather a fix together with the core change.
>>
>> It is a cleanup since the patchset only addresses lock problem which
>> is caused by the tasklet conversion.
>
> Well, the conversion to irqsave() is needed for the future drop of
> irq_save() in the caller, right? Then this isn't a cleanup but a
> preparation for movement ahead.
Sounds more accurate, and I will change the title in next round, :-)
Thanks,
--
Ming Lei
^ permalink raw reply
* Re: [PATCH] usb: USB host support should depend on HAS_DMA
From: Alan Stern @ 2013-07-11 15:03 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-input@vger.kernel.org,
Linux Media Mailing List, USB list, linux-kernel@vger.kernel.org
In-Reply-To: <CAMuHMdWF8pHQ7RfojiJK7NxULZZjecvzSkP6R+Q-5ib4A516Bw@mail.gmail.com>
On Thu, 11 Jul 2013, Geert Uytterhoeven wrote:
> On Thu, Jul 11, 2013 at 3:01 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Thu, 11 Jul 2013, Arnd Bergmann wrote:
> >
> >> On Wednesday 10 July 2013, Alan Stern wrote:
> >> > This isn't right. There are USB host controllers that use PIO, not
> >> > DMA. The HAS_DMA dependency should go with the controller driver, not
> >> > the USB core.
> >> >
> >> > On the other hand, the USB core does call various routines like
> >> > dma_unmap_single. It ought to be possible to compile these calls even
> >> > when DMA isn't enabled. That is, they should be defined as do-nothing
> >> > stubs.
> >>
> >> The asm-generic/dma-mapping-broken.h file intentionally causes link
> >> errors, but that could be changed.
> >>
> >> The better approach in my mind would be to replace code like
> >>
> >>
> >> if (hcd->self.uses_dma)
> >>
> >> with
> >>
> >> if (IS_ENABLED(CONFIG_HAS_DMA) && hcd->self.uses_dma) {
> >>
> >> which will reliably cause that reference to be omitted from object code,
> >> but not stop giving link errors for drivers that actually require
> >> DMA.
> >
> > How will it give link errors for drivers that require DMA?
>
> It won't. Unless the host driver itself calls into the DMA API, too
> (are there any that don't?).
To my knowledge, all the host controller drivers which use DMA _do_
call functions in the DMA API. So they would still get link errors,
even though the USB core wouldn't.
Therefore adding the appropriate HAS_DMA dependencies should be
straightforward: Try to build all the drivers and see which ones fail
to link.
Alan Stern
^ permalink raw reply
* Re: [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback
From: David Herrmann @ 2013-07-11 16:07 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan, Jiri Kosina,
linux-bluetooth@vger.kernel.org, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <CAN+gG=HsR3uQDF-U+jAd5Ap_+GQqLG2xOe-waLBNq3D6Righnw@mail.gmail.com>
Hi
On Thu, Jul 11, 2013 at 4:10 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> Hi David,
>
> On Thu, Jul 11, 2013 at 4:02 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> Hi
>>
>>> +static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
>>> + unsigned int code, int value)
>>> +{
>>> + struct hid_device *hid = input_get_drvdata(dev);
>>
>> I dislike that we have to deal with input_get_drvdata() in a transport
>> driver. It is not obvious from the transport driver that this gives us
>> an hid_device. But ok, that's how we do it in usbhid and hid-input so
>> it's another issue..
>
> right. So we should definitively change the callback signature here.
>
>>
>>> + struct hidp_session *session = hid->driver_data;
>>> + struct hid_field *field;
>>> + int offset;
>>> +
>>> + BT_DBG("session %p type %d code %d value %d",
>>> + session, type, code, value);
>>> +
>>> + if (type != EV_LED)
>>> + return -1;
>>> +
>>> + offset = hidinput_find_field(hid, type, code, &field);
>>> + if (offset == -1) {
>>> + hid_warn(dev, "event field not found\n");
>>> + return -1;
>>> + }
>>> +
>>> + hid_set_field(field, offset, value);
>>> +
>>> + return hidp_send_report(session, field->report);
>>> +}
>>> +
>>
>> We had this discussion before (regarding uhid and hidpinput_event), it
>> would be nice to have a helper in hid-input.c which does this. We copy
>> the code into every transport driver, which bugs me. But no-one
>> stepped up to do this, so I am fine.
>
> oh, yes, you are right. I'll add it to my todo list (now that I am
> responsible of 2/4 code duplication :-S )
Feel free to push this series forward unchanged, if it is urgent. I
have an experimental series here which implements a generic
hidinput_input_event() handler (including the scheduled worker) that I
will send later today or tomorrow.
Cheers
David
>>
>> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>
> Thanks for the review!
>
> Cheers,
> Benjamin
>
>>
>> Thanks!
>> David
>>
>>> static int hidp_get_raw_report(struct hid_device *hid,
>>> unsigned char report_number,
>>> unsigned char *data, size_t count,
>>> @@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
>>> .stop = hidp_stop,
>>> .open = hidp_open,
>>> .close = hidp_close,
>>> + .hidinput_input_event = hidp_hidinput_event,
>>> };
>>>
>>> /* This function sets up the hid device. It does not add it
>>> --
>>> 1.8.3.1
>>>
^ permalink raw reply
* Re: [PATCH 1/2] Bluetooth: hidp: implement hidinput_input_event callback
From: Jiri Kosina @ 2013-07-11 22:50 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
David Herrmann, linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1373550090-30379-2-git-send-email-benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Thu, 11 Jul 2013, Benjamin Tissoires wrote:
> We can re-enable hidinput_input_event to allow the leds of bluetooth
> keyboards to be set.
> Now the callbacks uses hid core to retrieve the right HID report to
> send, so this version is safer.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Jiri Kosina <jkosina-AlSwsSmVLrQ@public.gmane.org>
> ---
> net/bluetooth/hidp/core.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index f13a8da..9c8b50d 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -238,6 +238,31 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
> return hidp_send_intr_message(session, hdr, buf, rsize);
> }
>
> +static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
> + unsigned int code, int value)
> +{
> + struct hid_device *hid = input_get_drvdata(dev);
> + struct hidp_session *session = hid->driver_data;
> + struct hid_field *field;
> + int offset;
> +
> + BT_DBG("session %p type %d code %d value %d",
> + session, type, code, value);
> +
> + if (type != EV_LED)
> + return -1;
> +
> + offset = hidinput_find_field(hid, type, code, &field);
> + if (offset == -1) {
> + hid_warn(dev, "event field not found\n");
> + return -1;
> + }
> +
> + hid_set_field(field, offset, value);
> +
> + return hidp_send_report(session, field->report);
> +}
> +
> static int hidp_get_raw_report(struct hid_device *hid,
> unsigned char report_number,
> unsigned char *data, size_t count,
> @@ -711,6 +736,7 @@ static struct hid_ll_driver hidp_hid_driver = {
> .stop = hidp_stop,
> .open = hidp_open,
> .close = hidp_close,
> + .hidinput_input_event = hidp_hidinput_event,
> };
>
> /* This function sets up the hid device. It does not add it
> --
> 1.8.3.1
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 2/2] Bluetooth: hidp: remove wrong send_report at init
From: Jiri Kosina @ 2013-07-11 22:51 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Marcel Holtmann, Gustavo Padovan,
David Herrmann, linux-bluetooth, linux-input, linux-kernel
In-Reply-To: <1373550090-30379-3-git-send-email-benjamin.tissoires@redhat.com>
On Thu, 11 Jul 2013, Benjamin Tissoires wrote:
> The USB hid implementation does retrieve the reports during the start.
> However, this implementation does not call the HID command GET_REPORT
> (which would fetch the current status of each report), but use the
> DATA command, which is an Output Report (so transmitting data from the
> host to the device).
> The Wiimote controller is already guarded against this problem in the
> protocol, but it is not conformant to the specification to set all the
> reports to 0 on start.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Acked-by: Jiri Kosina <jkosina@suse.cz>
Gustavo, could you please take it through your tree?
Thanks Benjamin, thanks David.
> ---
> net/bluetooth/hidp/core.c | 14 --------------
> 1 file changed, 14 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 9c8b50d..59d132a 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -703,20 +703,6 @@ static int hidp_parse(struct hid_device *hid)
>
> static int hidp_start(struct hid_device *hid)
> {
> - struct hidp_session *session = hid->driver_data;
> - struct hid_report *report;
> -
> - if (hid->quirks & HID_QUIRK_NO_INIT_REPORTS)
> - return 0;
> -
> - list_for_each_entry(report, &hid->report_enum[HID_INPUT_REPORT].
> - report_list, list)
> - hidp_send_report(session, report);
> -
> - list_for_each_entry(report, &hid->report_enum[HID_FEATURE_REPORT].
> - report_list, list)
> - hidp_send_report(session, report);
> -
> return 0;
> }
>
> --
> 1.8.3.1
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] input: don't call input_dev_release_keys() in resume
From: Oskar Andero @ 2013-07-12 7:44 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
Makarov, Aleksej
In-Reply-To: <20130705074632.GF11422@caracas.corpusers.net>
On 09:46 Fri 05 Jul , Oskar Andero wrote:
> Hi Dmitry,
>
> On 18:33 Thu 04 Apr , Dmitry Torokhov wrote:
> > Hi Oskar,
> >
> > On Thu, Mar 07, 2013 at 03:01:22PM +0100, oskar.andero@sonymobile.com wrote:
> > > From: Aleksej Makarov <aleksej.makarov@sonymobile.com>
> > >
> > > When waking up the platform by pressing a specific key, sending a
> > > release on that key makes it impossible to react on the event in
> > > user-space.
> > >
> >
> > No, we can not simply not release keys after resume from suspend, as
> > this leads to keys being stuck. Consider you are holding an 'I' key on
> > your external USB keyboard and close your laptop's lid. Then you release
> > the key and leave. Later you come back, open the lid waking the laptop
> > and observe endless stream of 'I' in your open terminal.
> >
> > Maybe we should release the keys during suspend time? I am not sure how
> > Android infrastructure will react to this though...
>
> I finally got the time to try this out. Releasing the keys in suspend
> also solves our problem. Would such patch work for the USB keyboard
> case you described? Theoretically, I think it should, right?
>
> So, basically:
>
> static int input_dev_suspend(struct device *dev)
> {
> struct input_dev *input_dev = to_input_dev(dev);
>
> - mutex_lock(&input_dev->mutex);
> -
> - if (input_dev->users)
> - input_dev_toggle(input_dev, false);
> -
> - mutex_unlock(&input_dev->mutex);
> + input_reset_device(input_dev);
>
> return 0;
> }
>
> static int input_dev_resume(struct device *dev)
> {
> - struct input_dev *input_dev = to_input_dev(dev);
> -
> - input_reset_device(input_dev);
> -
> return 0;
> }
>
> Should I send the patch?
Ping. Any thoughts on this?
Thanks!
-Oskar
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox