| Message ID | 20200817160734.12402-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,01/10] media: ipu3-cio2: Simplify cleanup code | expand |
Hi Andy, Thank you for the patch. Glad to see interest in the CIO2 driver :-) On Mon, Aug 17, 2020 at 07:07:24PM +0300, Andy Shevchenko wrote: > The code looks more nicer if we use: > while (i--) > instead: > for (i = i - 1; i >= 0; i--) > > This would also allow making 'i' unsigned again. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > v2: converted i to unsigned (Sakari) > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > index 92f5eadf2c99..cb74d49934f1 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > @@ -847,7 +847,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb) > unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page); > struct sg_table *sg; > struct sg_dma_page_iter sg_iter; > - int i, j; > + unsigned int i, j; > > if (lops <= 0 || lops > CIO2_MAX_LOPS) { > dev_err(dev, "%s: bad buffer size (%i)\n", __func__, > @@ -887,7 +887,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb) > b->lop[i][j] = cio2->dummy_page_bus_addr >> PAGE_SHIFT; > return 0; > fail: > - for (i--; i >= 0; i--) > + while (i--) > dma_free_coherent(dev, CIO2_PAGE_SIZE, > b->lop[i], b->lop_bus_addr[i]); Looks good to me. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> There's an additional issue though, if vb2_dma_sg_plane_desc() fails, we should free all the allocated memory. > return -ENOMEM;
Hi Laurent and Andy, On Fri, Oct 09, 2020 at 03:57:16AM +0300, Laurent Pinchart wrote: > Hi Andy, > > Thank you for the patch. Glad to see interest in the CIO2 driver :-) > > On Mon, Aug 17, 2020 at 07:07:24PM +0300, Andy Shevchenko wrote: > > The code looks more nicer if we use: > > while (i--) > > instead: > > for (i = i - 1; i >= 0; i--) > > > > This would also allow making 'i' unsigned again. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > v2: converted i to unsigned (Sakari) > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > index 92f5eadf2c99..cb74d49934f1 100644 > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > @@ -847,7 +847,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb) > > unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page); > > struct sg_table *sg; > > struct sg_dma_page_iter sg_iter; > > - int i, j; > > + unsigned int i, j; > > > > if (lops <= 0 || lops > CIO2_MAX_LOPS) { > > dev_err(dev, "%s: bad buffer size (%i)\n", __func__, > > @@ -887,7 +887,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb) > > b->lop[i][j] = cio2->dummy_page_bus_addr >> PAGE_SHIFT; > > return 0; > > fail: > > - for (i--; i >= 0; i--) > > + while (i--) > > dma_free_coherent(dev, CIO2_PAGE_SIZE, > > b->lop[i], b->lop_bus_addr[i]); > > Looks good to me. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > There's an additional issue though, if vb2_dma_sg_plane_desc() fails, we > should free all the allocated memory. These patches have been merged some time ago. Further changes should be on top of this set.
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index 92f5eadf2c99..cb74d49934f1 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c @@ -847,7 +847,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb) unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page); struct sg_table *sg; struct sg_dma_page_iter sg_iter; - int i, j; + unsigned int i, j; if (lops <= 0 || lops > CIO2_MAX_LOPS) { dev_err(dev, "%s: bad buffer size (%i)\n", __func__, @@ -887,7 +887,7 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb) b->lop[i][j] = cio2->dummy_page_bus_addr >> PAGE_SHIFT; return 0; fail: - for (i--; i >= 0; i--) + while (i--) dma_free_coherent(dev, CIO2_PAGE_SIZE, b->lop[i], b->lop_bus_addr[i]); return -ENOMEM;
The code looks more nicer if we use: while (i--) instead: for (i = i - 1; i >= 0; i--) This would also allow making 'i' unsigned again. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- v2: converted i to unsigned (Sakari) drivers/media/pci/intel/ipu3/ipu3-cio2.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)