Hi, Thierry,
I have some comments by reading imx-sdma code in linux kernel only.
FYI.
On Fri, 2018-02-16 at 17:16 +0000, Thierry Bultel wrote:
> Add a spinlock and an 'enabled' boolean on channel descriptor, to
> avoid
> using buffer descriptors in the interrupt context,
> when sdma_disable_channel is called meanwhile.
>
> Signed-off-by: Thierry Bultel <tbultel@xxxxxxxxxxxxxxx>
> ---
>
> Changes since V1:
> * Applied Shunyong Yang's review advice:
> * Use a bool type for the enabled field
> * Fixed the race condition
>
> drivers/dma/imx-sdma.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index d1651a5..0df6d54 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -330,6 +330,7 @@ struct sdma_channel {
> unsigned int chn_real_count;
> struct tasklet_struct tasklet;
> struct imx_dma_data data;
> + bool enabled;
> };
>
> #define IMX_DMA_SG_LOOP BIT(0)
> @@ -588,7 +589,13 @@ static int sdma_config_ownership(struct
> sdma_channel *sdmac,
>
> static void sdma_enable_channel(struct sdma_engine *sdma, int
> channel)
> {
> + unsigned long flags;
> + struct sdma_channel *sdmac = &sdma->channel[channel];
> writel(BIT(channel), sdma->regs + SDMA_H_START);
> +
> + spin_lock_irqsave(&sdmac->lock, flags);
> + sdmac->enabled = 1;
> + spin_unlock_irqrestore(&sdmac->lock, flags);
Should writel(BIT(channel), sdma->regs + SDMA_H_START); be protected
also? Is there possibility the channel disabled by
by sdma_disable_channel() but sdmac->enabled is set to 1?
> }
>
> /*
> @@ -677,6 +684,14 @@ static void sdma_update_channel_loop(struct
> sdma_channel *sdmac)
> struct sdma_buffer_descriptor *bd;
> int error = 0;
> enum dma_status old_status = sdmac->status;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&sdmac->lock, flags);
> + if (!sdmac->enabled) {
> + spin_unlock_irqrestore(&sdmac->lock, flags);
> + return;
> + }
> + spin_unlock_irqrestore(&sdmac->lock, flags);
>
Is there possibility that check above passed but the channel is
disabled by sdma_disable_channel() when code is running in while (1)
loop just after the check?
while (1) {
bd = &sdmac->bd[sdmac->buf_tail];
...
}
If this case occurs, is it a problem? If it is, maybe there needs some
changes in current code as protecting loop via spinlock is somewhat
overhead.
> /*
> * loop mode. Iterate over descriptors, re-setup them and
> @@ -930,10 +945,15 @@ static int sdma_disable_channel(struct dma_chan
> *chan)
> struct sdma_channel *sdmac = to_sdma_chan(chan);
> struct sdma_engine *sdma = sdmac->sdma;
> int channel = sdmac->channel;
> + unsigned long flags;
>
> writel_relaxed(BIT(channel), sdma->regs + SDMA_H_STATSTOP);
> sdmac->status = DMA_ERROR;
>
> + spin_lock_irqsave(&sdmac->lock, flags);
> + sdmac->enabled = 0;
> + spin_unlock_irqrestore(&sdmac->lock, flags);
> +
similar to sdma_enable_channel() for writel_relaxed() protection.
Thanks.
Shunyong.
> return 0;
> }
> --
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
