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

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

 > commit c08d50f3400ecf3dbc032455be7a589baf899258
 > Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
 > 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)?