sashiko.dev review (Re: [PATCH v4 11/17] dmaengine: sh: rz-dmac: Refactor pause/resume code)

[Claudiu Beznea]

> commit c08d50f3400ecf3dbc032455be7a589baf899258
> Author: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> dmaengine: sh: rz-dmac: Refactor pause/resume code
> This commit refactors the pause/resume support to use a bitmask for the
> channel status. This prepares the driver for upcoming system suspend and
> cyclic DMA support where multiple independent pause states might be active.
> > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> > index 04eb1a7f1e627..d009b7607d447 100644
> > --- a/drivers/dma/sh/rz-dmac.c
> > +++ b/drivers/dma/sh/rz-dmac.c
> [ ... ]
> > +static int rz_dmac_device_resume_set(struct rz_dmac_chan *channel,
> > +                                unsigned long clear_bitmask)
> > +{
> > +   int ret = 0;
> > +   u32 val;
> > +
> > +   lockdep_assert_held(&channel->vc.lock);
> > +
> > +   /* Do not check CHSTAT_SUS but rely on HW capabilities. */
> > +
> > +   rz_dmac_ch_writel(channel, CHCTRL_CLRSUS, CHCTRL, 1);
> > +   ret = read_poll_timeout_atomic(rz_dmac_ch_readl, val,
> > +                                  !(val & CHSTAT_SUS), 1, 1024, false,
> > +                                  channel, CHSTAT, 1);
> > +
> > +   channel->status &= ~clear_bitmask;
> > +
> > +   return ret;
> > +}
> If multiple pause bits are set in channel->status, will this unconditionally
> resume the hardware?
> If a channel is paused explicitly by a dmaengine client using
> RZ_DMAC_CHAN_STATUS_PAUSED, and is subsequently paused by the upcoming system
> suspend functionality,

The upcomming system suspend functionality don't call rz_dmac_device_pause_set() 
if RZ_DMAC_CHAN_STATUS_PAUSED is set or the channel is not enabled(), it just 
return. The following could be checked:

static int rz_dmac_device_pause_internal(struct rz_dmac_chan *channel)
{
	lockdep_assert_held(&channel->vc.lock);

	/* Skip channels explicitly paused by consummers or disabled. */
	if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_PAUSED) ||
	    !rz_dmac_chan_is_enabled(channel))
		return 0;

	return rz_dmac_device_pause_set(channel, BIT(RZ_DMAC_CHAN_STATUS_PAUSED_INTERNAL));
}


> it appears this code will write CHCTRL_CLRSUS to the
> hardware as soon as the first of those conditions is cleared.
> This would silently override the other pause request, causing the hardware to
> resume prematurely while another client expects it to be paused.
> Should this update channel->status first, and only write CHCTRL_CLRSUS
> to the hardware if no other pause bits remain active (channel->status == 0)?