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

[Takashi Iwai]

At Wed, 7 Aug 2013 15:59:07 +0200,
Torsten Schenk wrote:
> 
> 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.

Yes, please.

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

OK, I queued the patch to sound git tree now.

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

Yes, it'd be much better.


thanks,

Takashi

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