Re: [PATCH v8 5/9] dmaengine: dw: dmamux: Introduce RZN1 DMA router support

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 1804 bytes --]

On Fri, 8 Apr 2022, Andy Shevchenko wrote:

> On Fri, Apr 08, 2022 at 12:55:47PM +0300, Ilpo Järvinen wrote:
> > On Wed, 6 Apr 2022, Miquel Raynal wrote:
> > 
> > > The Renesas RZN1 DMA IP is based on a DW core, with eg. an additional
> > > dmamux register located in the system control area which can take up to
> > > 32 requests (16 per DMA controller). Each DMA channel can be wired to
> > > two different peripherals.
> > > 
> > > We need two additional information from the 'dmas' property: the channel
> > > (bit in the dmamux register) that must be accessed and the value of the
> > > mux for this channel.
> 
> > > +	mask = BIT(map->req_idx);
> > > +	mutex_lock(&dmamux->lock);
> > > +	dmamux->used_chans |= mask;
> > > +	ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0);
> > > +	if (ret)
> > > +		goto release_chan_and_unlock;
> > > +
> > > +	mutex_unlock(&dmamux->lock);
> > > +
> > > +	return map;
> > > +
> > > +release_chan_and_unlock:
> > > +	dmamux->used_chans &= ~mask;
> > 
> > Now that I check this again, I'm not sure why dmamux->used_chans |= mask; 
> > couldn't be done after r9a06g032_sysctrl_set_dmamux() call so this 
> > rollback of it wouldn't be necessary.
> 
> I would still need the mutex unlock which I believe is down path there under
> some other label. Hence you are proposing something like
> 
> 	mask = BIT(map->req_idx);
> 
> 	mutex_lock(&dmamux->lock);
> 	ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0);
> 	if (ret)
> 		goto err_unlock; // or whatever label is
> 
> 	dmamux->used_chans |= mask;
> 	mutex_unlock(&dmamux->lock);
> 
> 	return map;
> 
> Is that correct? If so, I don't see impediments either.

Yes, and yes, the mutex still has to be unlocked on that error path.

> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.