[alsa-devel] [PATCH] 6fire: fix DMA issues with URB transfer_buffer usage

[Torsten Schenk]

On Wed, 07 Aug 2013 14:43:43 +0200
Takashi Iwai <tiwai at suse.de> wrote:

> At Tue, 06 Aug 2013 14:53:24 +0300,
> Jussi Kivilinna wrote:
> > 
> > Patch fixes 6fire not to use stack as URB transfer_buffer. URB
> > buffers need to be DMA-able, which stack is not. Furthermore,
> > transfer_buffer should not be allocated as part of larger device
> > structure because DMA coherency issues and patch fixes this issue
> > too.

Thanks for the information. There is another section where this applies
in midi.c/midi.h. I can post a patch later.

> > 
> > Patch is only compile tested.
> 
> The changes look OK, but I'd like to let it checked with a real
> hardware before putting to stable kernel.
> 
> Torsten, could you check this?

The patch works nicely.

I'm just wondering if instead of calling kmalloc() every time it'd be
better to put a global sender_buffer and a mutex into the comm_runtime
struct and lock it every time a writeX is performed. This also would
have the advantage that message blocks would never overlap which
currently is possible.

I can prepare a patch on top of this one and send it later, if you
agree.

Thanks,
Torsten

> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > Cc: stable at vger.kernel.org
> > Signed-off-by: Jussi Kivilinna <jussi.kivilinna at iki.fi>
> > ---
> >  sound/usb/6fire/comm.c |   38 ++++++++++++++++++++++++++++++++
> > +----- sound/usb/6fire/comm.h |    2 +-
> >  2 files changed, 34 insertions(+), 6 deletions(-)
> > 
> > diff --git a/sound/usb/6fire/comm.c b/sound/usb/6fire/comm.c
> > index 9e6e3ff..23452ee 100644
> > --- a/sound/usb/6fire/comm.c
> > +++ b/sound/usb/6fire/comm.c
> > @@ -110,19 +110,37 @@ static int usb6fire_comm_send_buffer(u8
> > *buffer, struct usb_device *dev) static int usb6fire_comm_write8
> > (struct comm_runtime *rt, u8 request, u8 reg, u8 value)
> >  {
> > -	u8 buffer[13]; /* 13: maximum length of message */
> > +	u8 *buffer;
> > +	int ret;
> > +
> > +	/* 13: maximum length of message */
> > +	buffer = kmalloc(13, GFP_KERNEL);
> > +	if (!buffer)
> > +		return -ENOMEM;
> >  
> >  	usb6fire_comm_init_buffer(buffer, 0x00, request, reg,
> > value, 0x00);
> > -	return usb6fire_comm_send_buffer(buffer, rt->chip->dev);
> > +	ret = usb6fire_comm_send_buffer(buffer, rt->chip->dev);
> > +
> > +	kfree(buffer);
> > +	return ret;
> >  }
> >  
> >  static int usb6fire_comm_write16(struct comm_runtime *rt, u8
> > request, u8 reg, u8 vl, u8 vh)
> >  {
> > -	u8 buffer[13]; /* 13: maximum length of message */
> > +	u8 *buffer;
> > +	int ret;
> > +
> > +	/* 13: maximum length of message */
> > +	buffer = kmalloc(13, GFP_KERNEL);
> > +	if (!buffer)
> > +		return -ENOMEM;
> >  
> >  	usb6fire_comm_init_buffer(buffer, 0x00, request, reg, vl,
> > vh);
> > -	return usb6fire_comm_send_buffer(buffer, rt->chip->dev);
> > +	ret = usb6fire_comm_send_buffer(buffer, rt->chip->dev);
> > +
> > +	kfree(buffer);
> > +	return ret;
> >  }
> >  
> >  int usb6fire_comm_init(struct sfire_chip *chip)
> > @@ -135,6 +153,12 @@ int usb6fire_comm_init(struct sfire_chip *chip)
> >  	if (!rt)
> >  		return -ENOMEM;
> >  
> > +	rt->receiver_buffer = kzalloc(COMM_RECEIVER_BUFSIZE,
> > GFP_KERNEL);
> > +	if (!rt->receiver_buffer) {
> > +		kfree(rt);
> > +		return -ENOMEM;
> > +	}
> > +
> >  	urb = &rt->receiver;
> >  	rt->serial = 1;
> >  	rt->chip = chip;
> > @@ -153,6 +177,7 @@ int usb6fire_comm_init(struct sfire_chip *chip)
> >  	urb->interval = 1;
> >  	ret = usb_submit_urb(urb, GFP_KERNEL);
> >  	if (ret < 0) {
> > +		kfree(rt->receiver_buffer);
> >  		kfree(rt);
> >  		snd_printk(KERN_ERR PREFIX "cannot create comm
> > data receiver."); return ret;
> > @@ -171,6 +196,9 @@ void usb6fire_comm_abort(struct sfire_chip
> > *chip) 
> >  void usb6fire_comm_destroy(struct sfire_chip *chip)
> >  {
> > -	kfree(chip->comm);
> > +	struct comm_runtime *rt = chip->comm;
> > +
> > +	kfree(rt->receiver_buffer);
> > +	kfree(rt);
> >  	chip->comm = NULL;
> >  }
> > diff --git a/sound/usb/6fire/comm.h b/sound/usb/6fire/comm.h
> > index 6a0840b..780d5ed 100644
> > --- a/sound/usb/6fire/comm.h
> > +++ b/sound/usb/6fire/comm.h
> > @@ -24,7 +24,7 @@ struct comm_runtime {
> >  	struct sfire_chip *chip;
> >  
> >  	struct urb receiver;
> > -	u8 receiver_buffer[COMM_RECEIVER_BUFSIZE];
> > +	u8 *receiver_buffer;
> >  
> >  	u8 serial; /* urb serial */
> >  
> > 
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel at alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> > 
>