lkml.org 
[lkml]   [2017]   [Jul]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/7] serial: imx: init dma_is_{rx|tx}ing variables
    Hello Romain,

    On Wed, Jul 05, 2017 at 12:14:57PM +0200, Romain Perier wrote:
    > Le 03/07/2017 à 08:52, Uwe Kleine-König a écrit :
    > > On Fri, Jun 30, 2017 at 02:13:29PM +0200, Lothar Waßmann wrote:
    > >> On Fri, 30 Jun 2017 14:04:42 +0200 Romain Perier wrote:
    > >>> From: Nandor Han <nandor.han@ge.com>
    > >>>
    > >>> Initialize both dma_is_{rx|tx}ing variables when DMA is enabled to avoid
    > >>> checking uninitialized variables if port shutdown is requested before
    > >>> DMA channels get a chance to start.
    > >>>
    > >>> Signed-off-by: Nandor Han <nandor.han@ge.com>
    > >>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
    > >>> ---
    > >>> drivers/tty/serial/imx.c | 3 +++
    > >>> 1 file changed, 3 insertions(+)
    > >>>
    > >>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
    > >>> index 188063d..81fb413 100644
    > >>> --- a/drivers/tty/serial/imx.c
    > >>> +++ b/drivers/tty/serial/imx.c
    > >>> @@ -1225,6 +1225,9 @@ static void imx_enable_dma(struct imx_port *sport)
    > >>>
    > >>> imx_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
    > >>>
    > >>> + sport->dma_is_rxing = 0;
    > >>> + sport->dma_is_txing = 0;
    > >>> +
    > >>> sport->dma_is_enabled = 1;
    > >>> }
    > >>>
    > >> sport is devm_kzalloc()ed, so the variables are initialized to 0 anyway.
    > > I'd agree to Lothar's statement. Did you find this issue by inspection,
    > > or does it fix a compiler warning? Do you think there is an actual
    > > problem?
    > >
    > > Best regards
    > > Uwe
    > >
    > What does happen if the UART port is shutdown and then re-enabled ? I
    > don't think that kzalloc will work in this case

    imx_shutdown has:

    if (sport->dma_is_enabled) {
    sport->dma_is_rxing = 0;
    sport->dma_is_txing = 0;

    which might be good enough. Can dma_is_[rt]xing be != 0 if
    dma_is_enabled is false? It seems it cannot, the only place where
    dma_is_enabled is set to 0 (apart from the kzalloc where dma_is_[rt]xing
    is set to zero) is imx_disable_dma(). The only caller sets both
    dma_is_[rt]xing to zero before.

    So this patch should be dropped or its commit log improved to point out
    the actual problem.

    Best regards
    Uwe

    --
    Pengutronix e.K. | Uwe Kleine-König |
    Industrial Linux Solutions | http://www.pengutronix.de/ |

    \
     
     \ /
      Last update: 2017-07-05 13:59    [W:2.819 / U:1.376 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site