| Message ID | 1b3ce6819beec9779468d1b661889d5b2dfa0c3e.1551352503.git.jsarha@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/tilcdc: a cleanup and a fix | expand |
Hi Jyri, Thank you for the patch. On Thu, Feb 28, 2019 at 01:18:50PM +0200, Jyri Sarha wrote: > drm_fb_cma_get_gem_obj() may return NULL. The return value needs to be > checked before dereferencing the returned pointer. > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > --- > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > index 1067e702c22c..a8ae064f52bb 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > @@ -75,6 +75,8 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) > u64 dma_base_and_ceiling; > > gem = drm_fb_cma_get_gem_obj(fb, 0); > + if (WARN_ON(!gem)) > + return; But this should not happen, right ? Don't we have the required checks in place to ensure there will always be a valid GEM object available here ? > start = gem->paddr + fb->offsets[0] + > crtc->y * fb->pitches[0] +
On 05/03/2019 17:43, Laurent Pinchart wrote: > Hi Jyri, > > Thank you for the patch. > > On Thu, Feb 28, 2019 at 01:18:50PM +0200, Jyri Sarha wrote: >> drm_fb_cma_get_gem_obj() may return NULL. The return value needs to be >> checked before dereferencing the returned pointer. >> >> Signed-off-by: Jyri Sarha <jsarha@ti.com> >> --- >> drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c >> index 1067e702c22c..a8ae064f52bb 100644 >> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c >> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c >> @@ -75,6 +75,8 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) >> u64 dma_base_and_ceiling; >> >> gem = drm_fb_cma_get_gem_obj(fb, 0); >> + if (WARN_ON(!gem)) >> + return; > > But this should not happen, right ? Don't we have the required checks in > place to ensure there will always be a valid GEM object available here ? > The patch is trying to make clockwork - a static analysis tool - happy. Should have mentioned that in the patch. But the fact that static clockwork complains about this suggests that either there is no such check elsewhere, or clockwork does not understand it. >> start = gem->paddr + fb->offsets[0] + >> crtc->y * fb->pitches[0] + > Best regards, Jyri
On Tue, Mar 05, 2019 at 09:42:50PM +0200, Jyri Sarha wrote: > On 05/03/2019 17:43, Laurent Pinchart wrote: > > Hi Jyri, > > > > Thank you for the patch. > > > > On Thu, Feb 28, 2019 at 01:18:50PM +0200, Jyri Sarha wrote: > >> drm_fb_cma_get_gem_obj() may return NULL. The return value needs to be > >> checked before dereferencing the returned pointer. > >> > >> Signed-off-by: Jyri Sarha <jsarha@ti.com> > >> --- > >> drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > >> index 1067e702c22c..a8ae064f52bb 100644 > >> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > >> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > >> @@ -75,6 +75,8 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) > >> u64 dma_base_and_ceiling; > >> > >> gem = drm_fb_cma_get_gem_obj(fb, 0); > >> + if (WARN_ON(!gem)) > >> + return; > > > > But this should not happen, right ? Don't we have the required checks in > > place to ensure there will always be a valid GEM object available here ? > > > > The patch is trying to make clockwork - a static analysis tool - happy. > Should have mentioned that in the patch. But the fact that static > clockwork complains about this suggests that either there is no such > check elsewhere, or clockwork does not understand it. I'd go for the latter. drm_fb_cma_get_gem_obj() will return NULL if the plane index is >= 4 (which is clearly not the case here), and will otherwise return the GEM object pointer from an internal array. That array is populated when the framebuffer is allocated created, by drm_gem_fb_create() called from tilcdc_fb_create() in this case. The check here is not needed, but clockwork can't easily know about that. > >> start = gem->paddr + fb->offsets[0] + > >> crtc->y * fb->pitches[0] +
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 1067e702c22c..a8ae064f52bb 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -75,6 +75,8 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) u64 dma_base_and_ceiling; gem = drm_fb_cma_get_gem_obj(fb, 0); + if (WARN_ON(!gem)) + return; start = gem->paddr + fb->offsets[0] + crtc->y * fb->pitches[0] +
drm_fb_cma_get_gem_obj() may return NULL. The return value needs to be checked before dereferencing the returned pointer. Signed-off-by: Jyri Sarha <jsarha@ti.com> --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++ 1 file changed, 2 insertions(+)