Hi Lucas, I have quick test for UART, seems this patch set broke uart function on i.mx7d, I believe it should be same on other i.mx6 family. Could you double check it? > -----Original Message----- > From: Lucas Stach <l.stach@xxxxxxxxxxxxxx> > Sent: 2018年9月10日 17:59 > To: Robin Gong <yibin.gong@xxxxxxx>; Vinod Koul <vkoul@xxxxxxxxxx> > Cc: dmaengine@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; > kernel@xxxxxxxxxxxxxx; patchwork-lst@xxxxxxxxxxxxxx > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > termination via worker > > Am Dienstag, den 04.09.2018, 02:36 +0000 schrieb Robin Gong: > > > -----Original Message----- > > > From: Lucas Stach <l.stach@xxxxxxxxxxxxxx> > > > Sent: 2018年9月3日 21:12 > > > To: Robin Gong <yibin.gong@xxxxxxx>; Vinod Koul <vkoul@xxxxxxxxxx> > > > Cc: dmaengine@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; > > > kernel@xxxxxxxxxxxxxx; patchwork-lst@xxxxxxxxxxxxxx > > > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > > > termination via worker > > > > > > Am Montag, den 03.09.2018, 08:59 +0000 schrieb Robin Gong: > > > > > -----Original Message----- > > > > > From: Lucas Stach <l.stach@xxxxxxxxxxxxxx> > > > > > Sent: 2018年9月3日 16:41 > > > > > To: Robin Gong <yibin.gong@xxxxxxx>; Vinod Koul <vkoul@kernel.o > > > > > rg> > > > > > Cc: dmaengine@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > > > > > ; kernel@xxxxxxxxxxxxxx; patchwork-lst@xxxxxxxxxxxxxx > > > > > Subject: Re: [PATCH 3/4] dmaengine: imx-sdma: implement channel > > > > > termination via worker > > > > > > > > > > Hi Robin, > > > > > > > > > > Am Freitag, den 31.08.2018, 09:49 +0000 schrieb Robin Gong: > > > > > > Hi Lucas, > > > > > > Seems I miss your previous mail. Thanks for your patch, but > > > > > > if move most jobs of sdma_disable_channel_with_delay() into > > > > > > worker, that will bring another race condition that upper > > > > > > driver such as Audio terminate channel and free resource of > > > > > > dma channel without really channel stop, if dma transfer done > > > > > > interrupt come after that, oops or kernel cash may be caught. > > > > > > Leave 'sdmac->desc = NULL' in the > > > > > > > > > > sdma_disable_channel_with_delay() may fix such potential issue. > > > > > > > > > > No, there is no such issue. The audio channel terminate will > > > > > call dmaengine_terminate_sync(), which internally calls > > > > > dmaengine_terminate_async() and then does a > > > > > dmaengine_synchronize(). > > > > > As this patchset implements the device_synchronize function in > > > > > the sdma driver, this will wait for the worker to finish its > > > > > execution, so there is no race condition to worry about here. > > > > > > > > > > Regards, > > > > > Lucas > > > > > > > > Yes, but how about other drivers which not call > > > > dmaengine_terminate_sync()? > > > > > > Please read the dmaengine documentation. device_terminate_all has no > > > requirement that the transfer is actually canceled when the call > > > returns. If the caller needs a guarantee that the channel is stopped > > > it _must_ call device_synchronize. > > > > I know that, but the fact is some driver still use > > dmaengine_terminate_all() such as > > Spi/uart driver. My concern is how to avoid to break their function. > > They should simply be fixed to not use a deprecated function. Both of those are > only using device_terminate_all in error or shutdown paths, so the risk of races > is pretty minimal even with the current code. And I think the SPI driver is trivial > to fix, as we can just use the terminate_sync variant there. The UART driver > is a bit more tricky. > > Regards, > Lucas
