| Message ID | 20170630120446.13994-4-romain.perier@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, 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. Lothar Waßmann
On Fri, Jun 30, 2017 at 02:13:29PM +0200, Lothar Waßmann wrote: > Hi, > > 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
Hello, 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: >> Hi, >> >> 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 Regards, Romain
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
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; }