lkml.org 
[lkml]   [2018]   [Jan]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RESEND PATCH v2 07/15] ASoC: qcom: q6asm: Add support to memory map and unmap
On Wed 03 Jan 08:26 PST 2018, Srinivas Kandagatla wrote:

> Thanks for your review comments.
>
> On 02/01/18 05:48, Bjorn Andersson wrote:
> > On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@linaro.org wrote:
[..]
> > > +int q6asm_unmap_memory_regions(unsigned int dir, struct audio_client *ac)
> > > +{
> > > + struct audio_port_data *port;
> > > + int cnt = 0;
> > > + int rc = 0;
> > > +
> > > + mutex_lock(&ac->cmd_lock);
> > > + port = &ac->port[dir];
> > > + if (!port->buf) {
> > > + mutex_unlock(&ac->cmd_lock);
> > > + return 0;
> >
> > Put a label right before the mutex_unlock below and return rc instead of
> > 0, then you can replace these two lines with "goto unlock"
> >
> > > + }
> > > + cnt = port->max_buf_cnt - 1;
> >
> > What if we mapped 1 period? Why shouldn't we enter the unmap path?
> >
> It would enter into unmap path, as cnt would be 0 for 1 period.
>

You're right, I missed the = in the comparison, but I don't see a reason
to subtract 1. It seems like the max_buf_cnt might have been used
differently in the past?

I suggest that you drop the - 1 and change the comparison to cnt > 0, if
nothing else to not confuse me if I read this code again ;)

> > > + if (cnt >= 0) {
[..]
> > > +int q6asm_map_memory_regions(unsigned int dir, struct audio_client *ac,
> > > + dma_addr_t phys,
> > > + unsigned int period_sz, unsigned int periods)
[..]
> > > + ac->port[dir].max_buf_cnt = 0;
> > > + kfree(buf);
> > > + ac->port[dir].buf = NULL;
> >
> > These operations are done without holding cmd_lock.
> >
> I will revisit such instances where the buf is not protected.
>

NB. I got the impression that cmd_lock was actually the port_lock in
most places.

Regards,
Bjorn

\
 
 \ /
  Last update: 2018-01-03 20:39    [W:0.088 / U:1.376 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site