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:

quoted

On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:

[..]

quoted

quoted

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

quoted

+	}
+	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 ;)

quoted

quoted

+	if (cnt >= 0) {

[..]

quoted

quoted

+int q6asm_map_memory_regions(unsigned int dir, struct audio_client *ac,
+			     dma_addr_t phys,
+			     unsigned int period_sz, unsigned int periods)

[..]

quoted

quoted

+		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