Re: [bug report] drm/prime: replace NULL with error value in drm_prime_pages_to_sg

2018-06-15 Thread Oleksandr Andrushchenko 

On 06/15/2018 12:08 PM, Dan Carpenter wrote:

On Fri, Jun 15, 2018 at 08:39:17AM +0300, Oleksandr Andrushchenko wrote:

On 06/15/2018 06:22 AM, YoungJun Cho wrote:

Dear Dan,

Your mail flashes back to my memory 5 years ago.
Back then, I had cleaned up the exynos driver.

And the replacement IS_ERR was one of items.

IMHO it is still better to modify those two functions,
drm_gem_cma_prime_get_sg_table and xen_drm_front_gem_get_sg_table.

Thank you.
Best regards YJ


On Thu, 14 Jun 2018, 23:42 Dan Carpenter, mailto:dan.carpen...@oracle.com>> wrote:

 Hello YoungJun Cho,

 The patch 7e3d88f9cce3: "drm/prime: replace NULL with error value in
 drm_prime_pages_to_sg" from Jun 24, 2013, leads to the following
 static checker warning:

         drivers/gpu/drm/drm_prime.c:317 drm_gem_map_dma_buf()
         warn: 'sgt' can also be NULL

 drivers/gpu/drm/drm_prime.c
    307          /*
    308           * two mappings with different directions for the
 same attachment are
    309           * not allowed
    310           */
    311          if (WARN_ON(prime_attach->dir != DMA_NONE))
    312                  return ERR_PTR(-EBUSY);
    313
    314          sgt = obj->dev->driver->gem_prime_get_sg_table(obj);

 The problematic functions here are
 drm_gem_cma_prime_get_sg_table() and
 xen_drm_front_gem_get_sg_table().  My preference would be to update
 those functions to return error pointers, but I'm not familiar
 with the
 code or able to test it.


I believe it is safe to change the return value to error pointer:
the code below (which is the only caller of .gem_prime_get_sg_table
callback) does not check for NULL anyways

Dan, do you want me to send the patch for Xen or you prefer to post both
Xen and CMA helpers fix yourself?

Can you do it and give me a Reported-by tag?

Sure

regards,
dan carpenter



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [bug report] drm/prime: replace NULL with error value in drm_prime_pages_to_sg

2018-06-15 Thread Dan Carpenter 
On Fri, Jun 15, 2018 at 08:39:17AM +0300, Oleksandr Andrushchenko wrote:
> On 06/15/2018 06:22 AM, YoungJun Cho wrote:
> > Dear Dan,
> > 
> > Your mail flashes back to my memory 5 years ago.
> > Back then, I had cleaned up the exynos driver.
> > 
> > And the replacement IS_ERR was one of items.
> > 
> > IMHO it is still better to modify those two functions,
> > drm_gem_cma_prime_get_sg_table and xen_drm_front_gem_get_sg_table.
> > 
> > Thank you.
> > Best regards YJ
> > 
> > 
> > On Thu, 14 Jun 2018, 23:42 Dan Carpenter,  > > wrote:
> > 
> > Hello YoungJun Cho,
> > 
> > The patch 7e3d88f9cce3: "drm/prime: replace NULL with error value in
> > drm_prime_pages_to_sg" from Jun 24, 2013, leads to the following
> > static checker warning:
> > 
> >         drivers/gpu/drm/drm_prime.c:317 drm_gem_map_dma_buf()
> >         warn: 'sgt' can also be NULL
> > 
> > drivers/gpu/drm/drm_prime.c
> >    307          /*
> >    308           * two mappings with different directions for the
> > same attachment are
> >    309           * not allowed
> >    310           */
> >    311          if (WARN_ON(prime_attach->dir != DMA_NONE))
> >    312                  return ERR_PTR(-EBUSY);
> >    313
> >    314          sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
> > 
> > The problematic functions here are
> > drm_gem_cma_prime_get_sg_table() and
> > xen_drm_front_gem_get_sg_table().  My preference would be to update
> > those functions to return error pointers, but I'm not familiar
> > with the
> > code or able to test it.
> > 
> I believe it is safe to change the return value to error pointer:
> the code below (which is the only caller of .gem_prime_get_sg_table
> callback) does not check for NULL anyways
> 
> Dan, do you want me to send the patch for Xen or you prefer to post both
> Xen and CMA helpers fix yourself?

Can you do it and give me a Reported-by tag?

regards,
dan carpenter

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [bug report] drm/prime: replace NULL with error value in drm_prime_pages_to_sg

2018-06-14 Thread Oleksandr Andrushchenko 

On 06/15/2018 06:22 AM, YoungJun Cho wrote:

Dear Dan,

Your mail flashes back to my memory 5 years ago.
Back then, I had cleaned up the exynos driver.

And the replacement IS_ERR was one of items.

IMHO it is still better to modify those two functions, 
drm_gem_cma_prime_get_sg_table and xen_drm_front_gem_get_sg_table.


Thank you.
Best regards YJ


On Thu, 14 Jun 2018, 23:42 Dan Carpenter, > wrote:


Hello YoungJun Cho,

The patch 7e3d88f9cce3: "drm/prime: replace NULL with error value in
drm_prime_pages_to_sg" from Jun 24, 2013, leads to the following
static checker warning:

        drivers/gpu/drm/drm_prime.c:317 drm_gem_map_dma_buf()
        warn: 'sgt' can also be NULL

drivers/gpu/drm/drm_prime.c
   307          /*
   308           * two mappings with different directions for the
same attachment are
   309           * not allowed
   310           */
   311          if (WARN_ON(prime_attach->dir != DMA_NONE))
   312                  return ERR_PTR(-EBUSY);
   313
   314          sgt = obj->dev->driver->gem_prime_get_sg_table(obj);

The problematic functions here are
drm_gem_cma_prime_get_sg_table() and
xen_drm_front_gem_get_sg_table().  My preference would be to update
those functions to return error pointers, but I'm not familiar
with the
code or able to test it.


I believe it is safe to change the return value to error pointer:
the code below (which is the only caller of .gem_prime_get_sg_table
callback) does not check for NULL anyways

Dan, do you want me to send the patch for Xen or you prefer to post both
Xen and CMA helpers fix yourself?


But this static checker test seems pretty good so I'm likely to
publish
it soon and then this sort of bug will normally be caught quickly.

   315
   316          if (!IS_ERR(sgt)) {
   317                  if (!dma_map_sg_attrs(attach->dev,
sgt->sgl, sgt->nents, dir,
   318 DMA_ATTR_SKIP_CPU_SYNC)) {
   319                          sg_free_table(sgt);
   320                          kfree(sgt);
   321                          sgt = ERR_PTR(-ENOMEM);
   322                  } else {
   323                          prime_attach->sgt = sgt;
   324                          prime_attach->dir = dir;
   325                  }
   326          }
   327
   328          return sgt;

regards,
dan carpenter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org

https://lists.freedesktop.org/mailman/listinfo/dri-devel



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [bug report] drm/prime: replace NULL with error value in drm_prime_pages_to_sg

2018-06-14 Thread YoungJun Cho 
Dear Dan,

Your mail flashes back to my memory 5 years ago.
Back then, I had cleaned up the exynos driver.

And the replacement IS_ERR was one of items.

IMHO it is still better to modify those two functions,
drm_gem_cma_prime_get_sg_table and xen_drm_front_gem_get_sg_table.

Thank you.
Best regards YJ


On Thu, 14 Jun 2018, 23:42 Dan Carpenter,  wrote:

> Hello YoungJun Cho,
>
> The patch 7e3d88f9cce3: "drm/prime: replace NULL with error value in
> drm_prime_pages_to_sg" from Jun 24, 2013, leads to the following
> static checker warning:
>
> drivers/gpu/drm/drm_prime.c:317 drm_gem_map_dma_buf()
> warn: 'sgt' can also be NULL
>
> drivers/gpu/drm/drm_prime.c
>307  /*
>308   * two mappings with different directions for the same
> attachment are
>309   * not allowed
>310   */
>311  if (WARN_ON(prime_attach->dir != DMA_NONE))
>312  return ERR_PTR(-EBUSY);
>313
>314  sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
>
> The problematic functions here are drm_gem_cma_prime_get_sg_table() and
> xen_drm_front_gem_get_sg_table().  My preference would be to update
> those functions to return error pointers, but I'm not familiar with the
> code or able to test it.
>
> But this static checker test seems pretty good so I'm likely to publish
> it soon and then this sort of bug will normally be caught quickly.
>
>315
>316  if (!IS_ERR(sgt)) {
>317  if (!dma_map_sg_attrs(attach->dev, sgt->sgl,
> sgt->nents, dir,
>318DMA_ATTR_SKIP_CPU_SYNC)) {
>319  sg_free_table(sgt);
>320  kfree(sgt);
>321  sgt = ERR_PTR(-ENOMEM);
>322  } else {
>323  prime_attach->sgt = sgt;
>324  prime_attach->dir = dir;
>325  }
>326  }
>327
>328  return sgt;
>
> regards,
> dan carpenter
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[bug report] drm/prime: replace NULL with error value in drm_prime_pages_to_sg

2018-06-14 Thread Dan Carpenter 
Hello YoungJun Cho,

The patch 7e3d88f9cce3: "drm/prime: replace NULL with error value in
drm_prime_pages_to_sg" from Jun 24, 2013, leads to the following
static checker warning:

drivers/gpu/drm/drm_prime.c:317 drm_gem_map_dma_buf()
warn: 'sgt' can also be NULL

drivers/gpu/drm/drm_prime.c
   307  /*
   308   * two mappings with different directions for the same 
attachment are
   309   * not allowed
   310   */
   311  if (WARN_ON(prime_attach->dir != DMA_NONE))
   312  return ERR_PTR(-EBUSY);
   313  
   314  sgt = obj->dev->driver->gem_prime_get_sg_table(obj);

The problematic functions here are drm_gem_cma_prime_get_sg_table() and
xen_drm_front_gem_get_sg_table().  My preference would be to update
those functions to return error pointers, but I'm not familiar with the
code or able to test it.

But this static checker test seems pretty good so I'm likely to publish
it soon and then this sort of bug will normally be caught quickly.

   315  
   316  if (!IS_ERR(sgt)) {
   317  if (!dma_map_sg_attrs(attach->dev, sgt->sgl, 
sgt->nents, dir,
   318DMA_ATTR_SKIP_CPU_SYNC)) {
   319  sg_free_table(sgt);
   320  kfree(sgt);
   321  sgt = ERR_PTR(-ENOMEM);
   322  } else {
   323  prime_attach->sgt = sgt;
   324  prime_attach->dir = dir;
   325  }
   326  }
   327  
   328  return sgt;

regards,
dan carpenter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel