| Message ID | 20200423142627.1820-1-zhengdejin5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] console: console: Complete exception handling in newport_probe() | expand |
On Thu, Apr 23, 2020 at 5:26 PM Dejin Zheng <zhengdejin5@gmail.com> wrote: > > A call of the function ¡°do_take_over_console¡± can fail here. > The corresponding system resources were not released then. > Thus add a call of the function ¡°iounmap¡± together with the check > of a failure predicate. ... > CC: Andy Shevchenko <andy.shevchenko@gmail.com> Use Cc: Better to read. ... > v1 -> v2: > - modify the commit comments by Markus'suggestion. What suggestion? You need to be clear in changelog what exactly has been done without looking to any previous mail. ... > console_lock(); > err = do_take_over_console(&newport_con, 0, MAX_NR_CONSOLES - 1, 1); > console_unlock(); > + > + if (err) > + iounmap((void *)npregs); > return err; > } I have briefly looked at the code (it is actually quite old one!), and I think this is half-baked solution, besides the fact of missed __iomem annotation and useless explicit casting. The proper one seems to switch to memremap() and do memunmap() here.
Hi, I believe that the patch summary line should be: "[PATCH v2] console: newport_con: ..." On 4/23/20 4:26 PM, Dejin Zheng wrote: > A call of the function ¡°do_take_over_console¡± can fail here. > The corresponding system resources were not released then. > Thus add a call of the function ¡°iounmap¡± together with the check > of a failure predicate. > > Fixes: e84de0c6190503 ("MIPS: GIO bus support for SGI IP22/28") I cannot see how this patch fixes commit e84de0c6190503 (AFAICS npregs has also been leaked on error before)? > CC: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> > --- > v1 -> v2: > - modify the commit comments by Markus'suggestion. > > drivers/video/console/newport_con.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/video/console/newport_con.c b/drivers/video/console/newport_con.c > index 00dddf6e08b0..6bfc8e3ffd4a 100644 > --- a/drivers/video/console/newport_con.c > +++ b/drivers/video/console/newport_con.c > @@ -720,6 +720,9 @@ static int newport_probe(struct gio_device *dev, > console_lock(); > err = do_take_over_console(&newport_con, 0, MAX_NR_CONSOLES - 1, 1); > console_unlock(); > + > + if (err) > + iounmap((void *)npregs); Looks OK but while you are at it, could you please also add missing release_mem_region() on error and on device removal: newport_addr = dev->resource.start + 0xF0000; if (!request_mem_region(newport_addr, 0x10000, "Newport")) return -ENODEV; npregs = (struct newport_regs *)/* ioremap cannot fail */ ioremap(newport_addr, sizeof(struct newport_regs)); console_lock(); err = do_take_over_console(&newport_con, 0, MAX_NR_CONSOLES - 1, 1); console_unlock(); return err; } static void newport_remove(struct gio_device *dev) { give_up_console(&newport_con); iounmap((void *)npregs); } ? > return err; > } > Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On Thu, Apr 23, 2020 at 5:55 PM Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> wrote: > > + if (err) > > + iounmap((void *)npregs); > > Looks OK but while you are at it, could you please also add missing > release_mem_region() on error and on device removal: > > newport_addr = dev->resource.start + 0xF0000; > if (!request_mem_region(newport_addr, 0x10000, "Newport")) > return -ENODEV; > > npregs = (struct newport_regs *)/* ioremap cannot fail */ > ioremap(newport_addr, sizeof(struct newport_regs)); > console_lock(); > err = do_take_over_console(&newport_con, 0, MAX_NR_CONSOLES - 1, 1); > console_unlock(); > return err; > } > > static void newport_remove(struct gio_device *dev) > { > give_up_console(&newport_con); > iounmap((void *)npregs); > } > > ? Don't you think that proper solution is rather switch to memremap()? > > return err; > > }
On 4/23/20 5:05 PM, Andy Shevchenko wrote: > On Thu, Apr 23, 2020 at 5:55 PM Bartlomiej Zolnierkiewicz > <b.zolnierkie@samsung.com> wrote: > >>> + if (err) >>> + iounmap((void *)npregs); >> >> Looks OK but while you are at it, could you please also add missing >> release_mem_region() on error and on device removal: >> >> newport_addr = dev->resource.start + 0xF0000; >> if (!request_mem_region(newport_addr, 0x10000, "Newport")) >> return -ENODEV; >> >> npregs = (struct newport_regs *)/* ioremap cannot fail */ >> ioremap(newport_addr, sizeof(struct newport_regs)); >> console_lock(); >> err = do_take_over_console(&newport_con, 0, MAX_NR_CONSOLES - 1, 1); >> console_unlock(); >> return err; >> } >> >> static void newport_remove(struct gio_device *dev) >> { >> give_up_console(&newport_con); >> iounmap((void *)npregs); >> } >> >> ? > > Don't you think that proper solution is rather switch to memremap()? Doesn't seem to be a case here (used memory region in uncached). On MIPS (this is MIPS-only driver): ... #define ioremap(offset, size) \ __ioremap_mode((offset), (size), _CACHE_UNCACHED) #define ioremap_uc ioremap ... While memremap() is only for cacheable memory: ... * memremap() - remap an iomem_resource as cacheable memory * @offset: iomem resource start address * @size: size of remap * @flags: any of MEMREMAP_WB, MEMREMAP_WT, MEMREMAP_WC, * MEMREMAP_ENC, MEMREMAP_DEC ... >>> return err; >>> } > Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On Thu, Apr 23, 2020 at 04:55:35PM +0200, Bartlomiej Zolnierkiewicz wrote: > > Hi, > > I believe that the patch summary line should be: > > "[PATCH v2] console: newport_con: ..." > OK, thanks! > On 4/23/20 4:26 PM, Dejin Zheng wrote: > > A call of the function ¡°do_take_over_console¡± can fail here. > > The corresponding system resources were not released then. > > Thus add a call of the function ¡°iounmap¡± together with the check > > of a failure predicate. > > > > Fixes: e84de0c6190503 ("MIPS: GIO bus support for SGI IP22/28") > > I cannot see how this patch fixes commit e84de0c6190503 > (AFAICS npregs has also been leaked on error before)? > yes, you are right, the commit should be e86bb8acc0fdca82d2 ("[PATCH] VT binding: Make newport_con support binding") - move register ioremap from newport_startup() to newport_console_init() > > CC: Andy Shevchenko <andy.shevchenko@gmail.com> > > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> > > --- > > v1 -> v2: > > - modify the commit comments by Markus'suggestion. > > > > drivers/video/console/newport_con.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/video/console/newport_con.c b/drivers/video/console/newport_con.c > > index 00dddf6e08b0..6bfc8e3ffd4a 100644 > > --- a/drivers/video/console/newport_con.c > > +++ b/drivers/video/console/newport_con.c > > @@ -720,6 +720,9 @@ static int newport_probe(struct gio_device *dev, > > console_lock(); > > err = do_take_over_console(&newport_con, 0, MAX_NR_CONSOLES - 1, 1); > > console_unlock(); > > + > > + if (err) > > + iounmap((void *)npregs); > > Looks OK but while you are at it, could you please also add missing > release_mem_region() on error and on device removal: > Ok, Thanks, I will send the patch v3 for it. > newport_addr = dev->resource.start + 0xF0000; > if (!request_mem_region(newport_addr, 0x10000, "Newport")) > return -ENODEV; > > npregs = (struct newport_regs *)/* ioremap cannot fail */ > ioremap(newport_addr, sizeof(struct newport_regs)); > console_lock(); > err = do_take_over_console(&newport_con, 0, MAX_NR_CONSOLES - 1, 1); > console_unlock(); > return err; > } > > static void newport_remove(struct gio_device *dev) > { > give_up_console(&newport_con); > iounmap((void *)npregs); > } > > ? > > > return err; > > } > > > > Best regards, > -- > Bartlomiej Zolnierkiewicz > Samsung R&D Institute Poland > Samsung Electronics
On Thu, Apr 23, 2020 at 05:52:09PM +0300, Andy Shevchenko wrote: > On Thu, Apr 23, 2020 at 5:26 PM Dejin Zheng <zhengdejin5@gmail.com> wrote: > > > > A call of the function do_take_over_console() can fail here. > > The corresponding system resources were not released then. > > Thus add a call of the function iounmap() together with the check > > of a failure predicate. > > ... > > > CC: Andy Shevchenko <andy.shevchenko@gmail.com> > > Use Cc: Better to read. > I will pay attention to the next submission, thanks. > ... > > > v1 -> v2: > > - modify the commit comments by Markus'suggestion. > > What suggestion? You need to be clear in changelog what exactly has > been done without looking to any previous mail. > The commit comments have some more appropriate instructions by Markus'suggestion. here is my first version commit comments: if do_take_over_console() return an error in the newport_probe(), due to the io virtual address is not released, it will cause a leak. Thnaks! > ... > > > console_lock(); > > err = do_take_over_console(&newport_con, 0, MAX_NR_CONSOLES - 1, 1); > > console_unlock(); > > + > > + if (err) > > + iounmap((void *)npregs); > > return err; > > } > > I have briefly looked at the code (it is actually quite old one!), and > I think this is half-baked solution, besides the fact of missed > __iomem annotation and useless explicit casting. > The proper one seems to switch to memremap() and do memunmap() here. > > -- > With Best Regards, > Andy Shevchenko
diff --git a/drivers/video/console/newport_con.c b/drivers/video/console/newport_con.c index 00dddf6e08b0..6bfc8e3ffd4a 100644 --- a/drivers/video/console/newport_con.c +++ b/drivers/video/console/newport_con.c @@ -720,6 +720,9 @@ static int newport_probe(struct gio_device *dev, console_lock(); err = do_take_over_console(&newport_con, 0, MAX_NR_CONSOLES - 1, 1); console_unlock(); + + if (err) + iounmap((void *)npregs); return err; }
A call of the function ¡°do_take_over_console¡± can fail here. The corresponding system resources were not released then. Thus add a call of the function ¡°iounmap¡± together with the check of a failure predicate. Fixes: e84de0c6190503 ("MIPS: GIO bus support for SGI IP22/28") CC: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> --- v1 -> v2: - modify the commit comments by Markus'suggestion. drivers/video/console/newport_con.c | 3 +++ 1 file changed, 3 insertions(+)