[PATCH] 6fire: fix URB transfer buffer for midi output

[PATCH] 6fire: fix URB transfer buffer for midi output

[Torsten Schenk]

Patch fixes URB transfer buffer allocation for midi output to be DMA-able.

Signed-off-by: Torsten Schenk <torsten.schenk@zoho.com>
---
diff -Nur a/sound/usb/6fire/midi.c b/sound/usb/6fire/midi.c
--- a/sound/usb/6fire/midi.c	2013-08-07 16:32:10.579639391 +0200
+++ b/sound/usb/6fire/midi.c	2013-08-07 16:32:31.363378104 +0200
@@ -19,6 +19,10 @@
 #include "chip.h"
 #include "comm.h"
 
+enum {
+	MIDI_BUFSIZE = 64
+};
+
 static void usb6fire_midi_out_handler(struct urb *urb)
 {
 	struct midi_runtime *rt = urb->context;
@@ -156,6 +160,12 @@
 	if (!rt)
 		return -ENOMEM;
 
+	rt->out_buffer = kzalloc(MIDI_BUFSIZE, GFP_KERNEL);
+	if (!rt->out_buffer) {
+		kfree(rt);
+		return -ENOMEM;
+	}
+
 	rt->chip = chip;
 	rt->in_received = usb6fire_midi_in_received;
 	rt->out_buffer[0] = 0x80; /* 'send midi' command */
@@ -169,6 +179,7 @@
 
 	ret = snd_rawmidi_new(chip->card, "6FireUSB", 0, 1, 1, &rt->instance);
 	if (ret < 0) {
+		kfree(rt->out_buffer);
 		kfree(rt);
 		snd_printk(KERN_ERR PREFIX "unable to create midi.\n");
 		return ret;
@@ -197,6 +208,9 @@
 
 void usb6fire_midi_destroy(struct sfire_chip *chip)
 {
-	kfree(chip->midi);
+	struct midi_runtime *rt = chip->midi;
+
+	kfree(rt->out_buffer);
+	kfree(rt);
 	chip->midi = NULL;
 }
diff -Nur a/sound/usb/6fire/midi.h b/sound/usb/6fire/midi.h
--- a/sound/usb/6fire/midi.h	2013-08-07 16:32:10.579639391 +0200
+++ b/sound/usb/6fire/midi.h	2013-08-07 16:32:31.363378104 +0200
@@ -16,10 +16,6 @@
 
 #include "common.h"
 
-enum {
-	MIDI_BUFSIZE = 64
-};
-
 struct midi_runtime {
 	struct sfire_chip *chip;
 	struct snd_rawmidi *instance;
@@ -32,7 +28,7 @@
 	struct snd_rawmidi_substream *out;
 	struct urb out_urb;
 	u8 out_serial; /* serial number of out packet */
-	u8 out_buffer[MIDI_BUFSIZE];
+	u8 *out_buffer;
 	int buffer_offset;
 
 	void (*in_received)(struct midi_runtime *rt, u8 *data, int length);

Re: [PATCH] 6fire: fix URB transfer buffer for midi output

[Takashi Iwai]

[Cc'ed to linux-usb ML]

At Wed, 7 Aug 2013 16:51:49 +0200,
Torsten Schenk wrote:
> 
> Patch fixes URB transfer buffer allocation for midi output to be DMA-able.

Is this really needed?
That is, can't a transfer buffer be at middle of kmalloc'ed space, but
must be always the head of the kmalloc'ed space?


Takashi

> 
> Signed-off-by: Torsten Schenk <torsten.schenk-ytc+IHgoah0@public.gmane.org>
> ---
> diff -Nur a/sound/usb/6fire/midi.c b/sound/usb/6fire/midi.c
> --- a/sound/usb/6fire/midi.c	2013-08-07 16:32:10.579639391 +0200
> +++ b/sound/usb/6fire/midi.c	2013-08-07 16:32:31.363378104 +0200
> @@ -19,6 +19,10 @@
>  #include "chip.h"
>  #include "comm.h"
>  
> +enum {
> +	MIDI_BUFSIZE = 64
> +};
> +
>  static void usb6fire_midi_out_handler(struct urb *urb)
>  {
>  	struct midi_runtime *rt = urb->context;
> @@ -156,6 +160,12 @@
>  	if (!rt)
>  		return -ENOMEM;
>  
> +	rt->out_buffer = kzalloc(MIDI_BUFSIZE, GFP_KERNEL);
> +	if (!rt->out_buffer) {
> +		kfree(rt);
> +		return -ENOMEM;
> +	}
> +
>  	rt->chip = chip;
>  	rt->in_received = usb6fire_midi_in_received;
>  	rt->out_buffer[0] = 0x80; /* 'send midi' command */
> @@ -169,6 +179,7 @@
>  
>  	ret = snd_rawmidi_new(chip->card, "6FireUSB", 0, 1, 1, &rt->instance);
>  	if (ret < 0) {
> +		kfree(rt->out_buffer);
>  		kfree(rt);
>  		snd_printk(KERN_ERR PREFIX "unable to create midi.\n");
>  		return ret;
> @@ -197,6 +208,9 @@
>  
>  void usb6fire_midi_destroy(struct sfire_chip *chip)
>  {
> -	kfree(chip->midi);
> +	struct midi_runtime *rt = chip->midi;
> +
> +	kfree(rt->out_buffer);
> +	kfree(rt);
>  	chip->midi = NULL;
>  }
> diff -Nur a/sound/usb/6fire/midi.h b/sound/usb/6fire/midi.h
> --- a/sound/usb/6fire/midi.h	2013-08-07 16:32:10.579639391 +0200
> +++ b/sound/usb/6fire/midi.h	2013-08-07 16:32:31.363378104 +0200
> @@ -16,10 +16,6 @@
>  
>  #include "common.h"
>  
> -enum {
> -	MIDI_BUFSIZE = 64
> -};
> -
>  struct midi_runtime {
>  	struct sfire_chip *chip;
>  	struct snd_rawmidi *instance;
> @@ -32,7 +28,7 @@
>  	struct snd_rawmidi_substream *out;
>  	struct urb out_urb;
>  	u8 out_serial; /* serial number of out packet */
> -	u8 out_buffer[MIDI_BUFSIZE];
> +	u8 *out_buffer;
>  	int buffer_offset;
>  
>  	void (*in_received)(struct midi_runtime *rt, u8 *data, int length);
> 
--
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

Re: [PATCH] 6fire: fix URB transfer buffer for midi output

[Alan Stern]

On Wed, 7 Aug 2013, Takashi Iwai wrote:

> [Cc'ed to linux-usb ML]
> 
> At Wed, 7 Aug 2013 16:51:49 +0200,
> Torsten Schenk wrote:
> > 
> > Patch fixes URB transfer buffer allocation for midi output to be DMA-able.
> 
> Is this really needed?
> That is, can't a transfer buffer be at middle of kmalloc'ed space, but
> must be always the head of the kmalloc'ed space?

A buffer _can_ be in the middle of a kmalloc'ed space, but the CPU must
not access any of the fields around it that might occupy the same cache
line while the buffer is being used for DMA.  In general, it's safest
not to put any other data in the same kmalloc'ed region with a DMA
buffer.

> > @@ -32,7 +28,7 @@
> >  	struct snd_rawmidi_substream *out;
> >  	struct urb out_urb;
> >  	u8 out_serial; /* serial number of out packet */
> > -	u8 out_buffer[MIDI_BUFSIZE];
> > +	u8 *out_buffer;
> >  	int buffer_offset;

In this case, the CPU would access out_urb while out_buffer was in use.

Alan Stern

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

Re: [PATCH] 6fire: fix URB transfer buffer for midi output

[Takashi Iwai]

At Wed, 7 Aug 2013 13:38:20 -0400 (EDT),
Alan Stern wrote:
> 
> On Wed, 7 Aug 2013, Takashi Iwai wrote:
> 
> > [Cc'ed to linux-usb ML]
> > 
> > At Wed, 7 Aug 2013 16:51:49 +0200,
> > Torsten Schenk wrote:
> > > 
> > > Patch fixes URB transfer buffer allocation for midi output to be DMA-able.
> > 
> > Is this really needed?
> > That is, can't a transfer buffer be at middle of kmalloc'ed space, but
> > must be always the head of the kmalloc'ed space?
> 
> A buffer _can_ be in the middle of a kmalloc'ed space, but the CPU must
> not access any of the fields around it that might occupy the same cache
> line while the buffer is being used for DMA.  In general, it's safest
> not to put any other data in the same kmalloc'ed region with a DMA
> buffer.

Hrm, but does the kmalloc buffer always guarantee such cache line
exclusiveness...?  I thought a simple one like SLOB doesn't care.


> > > @@ -32,7 +28,7 @@
> > >  	struct snd_rawmidi_substream *out;
> > >  	struct urb out_urb;
> > >  	u8 out_serial; /* serial number of out packet */
> > > -	u8 out_buffer[MIDI_BUFSIZE];
> > > +	u8 *out_buffer;
> > >  	int buffer_offset;
> 
> In this case, the CPU would access out_urb while out_buffer was in use.

OK, then we need to fix sound/usb/6fire/pcm.c, too.
Torsten, care to respin the patch?


thanks,

Takashi
--
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

Re: [PATCH] 6fire: fix URB transfer buffer for midi output

[Clemens Ladisch]

Takashi Iwai wrote:
> Alan Stern wrote:
>> A buffer _can_ be in the middle of a kmalloc'ed space, but the CPU must
>> not access any of the fields around it that might occupy the same cache
>> line while the buffer is being used for DMA.
>
> Hrm, but does the kmalloc buffer always guarantee such cache line
> exclusiveness...?  I thought a simple one like SLOB doesn't care.

Documentation/DMA-API-HOWTO.txt says:
|  Architectures must ensure that kmalloc'ed buffer is
|  DMA-safe. Drivers and subsystems depend on it. If an architecture
|  isn't fully DMA-coherent (i.e. hardware doesn't ensure that data in
|  the CPU cache is identical to data in main memory),
|  ARCH_DMA_MINALIGN must be set so that the memory allocator
|  makes sure that kmalloc'ed buffer doesn't share a cache line with
|  the others.


Regards,
Clemens

Re: [alsa-devel] [PATCH] 6fire: fix URB transfer buffer for midi output

[Takashi Iwai]

At Thu, 08 Aug 2013 09:16:27 +0200,
Clemens Ladisch wrote:
> 
> Takashi Iwai wrote:
> > Alan Stern wrote:
> >> A buffer _can_ be in the middle of a kmalloc'ed space, but the CPU must
> >> not access any of the fields around it that might occupy the same cache
> >> line while the buffer is being used for DMA.
> >
> > Hrm, but does the kmalloc buffer always guarantee such cache line
> > exclusiveness...?  I thought a simple one like SLOB doesn't care.
> 
> Documentation/DMA-API-HOWTO.txt says:
> |  Architectures must ensure that kmalloc'ed buffer is
> |  DMA-safe. Drivers and subsystems depend on it. If an architecture
> |  isn't fully DMA-coherent (i.e. hardware doesn't ensure that data in
> |  the CPU cache is identical to data in main memory),
> |  ARCH_DMA_MINALIGN must be set so that the memory allocator
> |  makes sure that kmalloc'ed buffer doesn't share a cache line with
> |  the others.

Ah, good, thanks!


Takashi
--
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

[PATCH 1/2] 6fire: make buffers DMA-able (pcm)

[Torsten Schenk]

Patch makes pcm buffers DMA-able by allocating each one separately.

Signed-off-by: Torsten Schenk <torsten.schenk@zoho.com>
---
diff -Nur a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c
--- a/sound/usb/6fire/pcm.c	2013-08-04 22:46:46.000000000 +0200
+++ b/sound/usb/6fire/pcm.c	2013-08-11 11:02:38.305774934 +0200
@@ -582,6 +582,33 @@
 	urb->instance.number_of_packets = PCM_N_PACKETS_PER_URB;
 }
 
+static int usb6fire_pcm_buffers_init(struct pcm_runtime *rt)
+{
+	int i;
+
+	for (i = 0; i < PCM_N_URBS; i++) {
+		rt->out_urbs[i].buffer = kzalloc(PCM_N_PACKETS_PER_URB
+				* PCM_MAX_PACKET_SIZE, GFP_KERNEL);
+		if (!rt->out_urbs[i].buffer)
+			return -ENOMEM;
+		rt->in_urbs[i].buffer = kzalloc(PCM_N_PACKETS_PER_URB
+				* PCM_MAX_PACKET_SIZE, GFP_KERNEL);
+		if (!rt->in_urbs[i].buffer)
+			return -ENOMEM;
+	}
+	return 0;
+}
+
+static void usb6fire_pcm_buffers_destroy(struct pcm_runtime *rt)
+{
+	int i;
+
+	for (i = 0; i < PCM_N_URBS; i++) {
+		kfree(rt->out_urbs[i].buffer);
+		kfree(rt->in_urbs[i].buffer);
+	}
+}
+
 int usb6fire_pcm_init(struct sfire_chip *chip)
 {
 	int i;
@@ -593,6 +620,13 @@
 	if (!rt)
 		return -ENOMEM;
 
+	ret = usb6fire_pcm_buffers_init(rt);
+	if (ret) {
+		usb6fire_pcm_buffers_destroy(rt);
+		kfree(rt);
+		return ret;
+	}
+
 	rt->chip = chip;
 	rt->stream_state = STREAM_DISABLED;
 	rt->rate = ARRAY_SIZE(rates);
@@ -614,6 +648,7 @@
 
 	ret = snd_pcm_new(chip->card, "DMX6FireUSB", 0, 1, 1, &pcm);
 	if (ret < 0) {
+		usb6fire_pcm_buffers_destroy(rt);
 		kfree(rt);
 		snd_printk(KERN_ERR PREFIX "cannot create pcm instance.\n");
 		return ret;
@@ -625,6 +660,7 @@
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &pcm_ops);
 
 	if (ret) {
+		usb6fire_pcm_buffers_destroy(rt);
 		kfree(rt);
 		snd_printk(KERN_ERR PREFIX
 				"error preallocating pcm buffers.\n");
@@ -669,6 +705,9 @@
 
 void usb6fire_pcm_destroy(struct sfire_chip *chip)
 {
-	kfree(chip->pcm);
+	struct pcm_runtime *rt = chip->pcm;
+
+	usb6fire_pcm_buffers_destroy(rt);
+	kfree(rt);
 	chip->pcm = NULL;
 }
diff -Nur a/sound/usb/6fire/pcm.h b/sound/usb/6fire/pcm.h
--- a/sound/usb/6fire/pcm.h	2013-08-04 22:46:46.000000000 +0200
+++ b/sound/usb/6fire/pcm.h	2013-08-11 10:49:28.779700620 +0200
@@ -32,7 +32,7 @@
 	struct urb instance;
 	struct usb_iso_packet_descriptor packets[PCM_N_PACKETS_PER_URB];
 	/* END DO NOT SEPARATE */
-	u8 buffer[PCM_N_PACKETS_PER_URB * PCM_MAX_PACKET_SIZE];
+	u8 *buffer;
 
 	struct pcm_urb *peer;
 };

Re: [PATCH 1/2] 6fire: make buffers DMA-able (pcm)

[Takashi Iwai]

At Sun, 11 Aug 2013 11:11:19 +0200,
Torsten Schenk wrote:
> 
> Patch makes pcm buffers DMA-able by allocating each one separately.
> 
> Signed-off-by: Torsten Schenk <torsten.schenk@zoho.com>

Thanks, applied.


Takashi

> ---
> diff -Nur a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c
> --- a/sound/usb/6fire/pcm.c	2013-08-04 22:46:46.000000000 +0200
> +++ b/sound/usb/6fire/pcm.c	2013-08-11 11:02:38.305774934 +0200
> @@ -582,6 +582,33 @@
>  	urb->instance.number_of_packets = PCM_N_PACKETS_PER_URB;
>  }
>  
> +static int usb6fire_pcm_buffers_init(struct pcm_runtime *rt)
> +{
> +	int i;
> +
> +	for (i = 0; i < PCM_N_URBS; i++) {
> +		rt->out_urbs[i].buffer = kzalloc(PCM_N_PACKETS_PER_URB
> +				* PCM_MAX_PACKET_SIZE, GFP_KERNEL);
> +		if (!rt->out_urbs[i].buffer)
> +			return -ENOMEM;
> +		rt->in_urbs[i].buffer = kzalloc(PCM_N_PACKETS_PER_URB
> +				* PCM_MAX_PACKET_SIZE, GFP_KERNEL);
> +		if (!rt->in_urbs[i].buffer)
> +			return -ENOMEM;
> +	}
> +	return 0;
> +}
> +
> +static void usb6fire_pcm_buffers_destroy(struct pcm_runtime *rt)
> +{
> +	int i;
> +
> +	for (i = 0; i < PCM_N_URBS; i++) {
> +		kfree(rt->out_urbs[i].buffer);
> +		kfree(rt->in_urbs[i].buffer);
> +	}
> +}
> +
>  int usb6fire_pcm_init(struct sfire_chip *chip)
>  {
>  	int i;
> @@ -593,6 +620,13 @@
>  	if (!rt)
>  		return -ENOMEM;
>  
> +	ret = usb6fire_pcm_buffers_init(rt);
> +	if (ret) {
> +		usb6fire_pcm_buffers_destroy(rt);
> +		kfree(rt);
> +		return ret;
> +	}
> +
>  	rt->chip = chip;
>  	rt->stream_state = STREAM_DISABLED;
>  	rt->rate = ARRAY_SIZE(rates);
> @@ -614,6 +648,7 @@
>  
>  	ret = snd_pcm_new(chip->card, "DMX6FireUSB", 0, 1, 1, &pcm);
>  	if (ret < 0) {
> +		usb6fire_pcm_buffers_destroy(rt);
>  		kfree(rt);
>  		snd_printk(KERN_ERR PREFIX "cannot create pcm instance.\n");
>  		return ret;
> @@ -625,6 +660,7 @@
>  	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &pcm_ops);
>  
>  	if (ret) {
> +		usb6fire_pcm_buffers_destroy(rt);
>  		kfree(rt);
>  		snd_printk(KERN_ERR PREFIX
>  				"error preallocating pcm buffers.\n");
> @@ -669,6 +705,9 @@
>  
>  void usb6fire_pcm_destroy(struct sfire_chip *chip)
>  {
> -	kfree(chip->pcm);
> +	struct pcm_runtime *rt = chip->pcm;
> +
> +	usb6fire_pcm_buffers_destroy(rt);
> +	kfree(rt);
>  	chip->pcm = NULL;
>  }
> diff -Nur a/sound/usb/6fire/pcm.h b/sound/usb/6fire/pcm.h
> --- a/sound/usb/6fire/pcm.h	2013-08-04 22:46:46.000000000 +0200
> +++ b/sound/usb/6fire/pcm.h	2013-08-11 10:49:28.779700620 +0200
> @@ -32,7 +32,7 @@
>  	struct urb instance;
>  	struct usb_iso_packet_descriptor packets[PCM_N_PACKETS_PER_URB];
>  	/* END DO NOT SEPARATE */
> -	u8 buffer[PCM_N_PACKETS_PER_URB * PCM_MAX_PACKET_SIZE];
> +	u8 *buffer;
>  
>  	struct pcm_urb *peer;
>  };
>