Re: [PATCH] drm: Add checks for NULL drm_*_helper_funcs

[Daniel Vetter]

On Tue, May 29, 2018 at 12:03:24PM +0300, Haneen Mohammed wrote:
> On Tue, May 29, 2018 at 10:03:53AM +0200, Daniel Vetter wrote:
> > On Fri, May 25, 2018 at 05:20:08AM +0300, Haneen Mohammed wrote:
> > > This patch add checks for NULL drm_[connector/crtc/plane]_helper_funcs
> > > pointers before derefrencing the variable to avoid NULL pointer
> > > dereference and make the helper functions as optional as possible.
> > > 
> > > Signed-off-by: Haneen Mohammed 
> > 
> > I started reviewing this, and then realized it's a bit a can of worms.
> > E.g. connector->funcs->detect shouldn't be there, it's a helper callback
> > really (but placed in the wrong function table). So connector->funcs isn't
> > optional, but connector->funcs->detect maybe should be optional.
> > 
> > So I'm not sure anymore whether doing this holesale is a good idea. Do we
> > still need this for vkms? If yes, then a more focused patch to just make
> > the things optional that vkms does not (yet) provide might be better.
> > Including an explanation of what exactly blows up in vkms.
> > 
> 
> hm only for running the igt tests & modetest in libdrm. 
> 
> It was easier to add check for all the funcs, but I will check again the
> least amount of checks needed for vkms.

To clarify what I'm looking for: I think a more specific approach like
"vkms doesn have funcs for $object, and that makes sense because $reason."
I think making some of the funcs optional might not make any sense at all,
since any working driver needs them. vkms atm is special, since it just
absolutely nothing right now (but that will change).

The goal here is to make driver writing easier by only requiring what's
really needed, not mislead them by not requiring stuff that really should
be there for a working driver :-)
-Daniel

> 
> Thank you!
> Haneen
> 
> > Thanks, Daniel
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 42 +++--
> > >  drivers/gpu/drm/drm_probe_helper.c  | 11 
> > >  2 files changed, 28 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > index c35654591c12..52092deb741d 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -112,9 +112,9 @@ static int handle_conflicting_encoders(struct 
> > > drm_atomic_state *state,
> > >   if (!new_conn_state->crtc)
> > >   continue;
> > >  
> > > - if (funcs->atomic_best_encoder)
> > > + if (funcs && funcs->atomic_best_encoder)
> > >   new_encoder = funcs->atomic_best_encoder(connector, 
> > > new_conn_state);
> > > - else if (funcs->best_encoder)
> > > + else if (funcs && funcs->best_encoder)
> > >   new_encoder = funcs->best_encoder(connector);
> > >   else
> > >   new_encoder = drm_atomic_helper_best_encoder(connector);
> > > @@ -308,10 +308,10 @@ update_connector_routing(struct drm_atomic_state 
> > > *state,
> > >  
> > >   funcs = connector->helper_private;
> > >  
> > > - if (funcs->atomic_best_encoder)
> > > + if (funcs && funcs->atomic_best_encoder)
> > >   new_encoder = funcs->atomic_best_encoder(connector,
> > >new_connector_state);
> > > - else if (funcs->best_encoder)
> > > + else if (funcs && funcs->best_encoder)
> > >   new_encoder = funcs->best_encoder(connector);
> > >   else
> > >   new_encoder = drm_atomic_helper_best_encoder(connector);
> > > @@ -438,7 +438,7 @@ mode_fixup(struct drm_atomic_state *state)
> > >   continue;
> > >  
> > >   funcs = crtc->helper_private;
> > > - if (!funcs->mode_fixup)
> > > + if (!funcs || !funcs->mode_fixup)
> > >   continue;
> > >  
> > >   ret = funcs->mode_fixup(crtc, _crtc_state->mode,
> > > @@ -639,7 +639,7 @@ drm_atomic_helper_check_modeset(struct drm_device 
> > > *dev,
> > >   new_crtc_state->connectors_changed = true;
> > >   }
> > >  
> > > - if (funcs->atomic_check)
> > > + if (funcs && funcs->atomic_check)
> > >   ret = funcs->atomic_check(connector, 
> > > new_connector_state);
> > >   if (ret)
> > >   return ret;
> > > @@ -681,7 +681,7 @@ drm_atomic_helper_check_modeset(struct drm_device 
> > > *dev,
> > >   if (connectors_mask & BIT(i))
> > >   continue;
> > >  
> > > - if (funcs->atomic_check)
> > > + if (funcs && funcs->atomic_check)
> > >   ret = funcs->atomic_check(connector, 
> > > new_connector_state);
> > >   if (ret)
> > >   return ret;
> > > @@ -972,14 +972,16 @@ disable_outputs(struct drm_device *dev, struct 
> > > drm_atomic_state *old_state)
> > >  
> > >  
> > >   /* Right function depends upon target state. */
> > > - if 

Re: [PATCH] drm: Add checks for NULL drm_*_helper_funcs

[Haneen Mohammed]

On Tue, May 29, 2018 at 10:03:53AM +0200, Daniel Vetter wrote:
> On Fri, May 25, 2018 at 05:20:08AM +0300, Haneen Mohammed wrote:
> > This patch add checks for NULL drm_[connector/crtc/plane]_helper_funcs
> > pointers before derefrencing the variable to avoid NULL pointer
> > dereference and make the helper functions as optional as possible.
> > 
> > Signed-off-by: Haneen Mohammed 
> 
> I started reviewing this, and then realized it's a bit a can of worms.
> E.g. connector->funcs->detect shouldn't be there, it's a helper callback
> really (but placed in the wrong function table). So connector->funcs isn't
> optional, but connector->funcs->detect maybe should be optional.
> 
> So I'm not sure anymore whether doing this holesale is a good idea. Do we
> still need this for vkms? If yes, then a more focused patch to just make
> the things optional that vkms does not (yet) provide might be better.
> Including an explanation of what exactly blows up in vkms.
> 

hm only for running the igt tests & modetest in libdrm. 

It was easier to add check for all the funcs, but I will check again the
least amount of checks needed for vkms.

Thank you!
Haneen

> Thanks, Daniel
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 42 +++--
> >  drivers/gpu/drm/drm_probe_helper.c  | 11 
> >  2 files changed, 28 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index c35654591c12..52092deb741d 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -112,9 +112,9 @@ static int handle_conflicting_encoders(struct 
> > drm_atomic_state *state,
> > if (!new_conn_state->crtc)
> > continue;
> >  
> > -   if (funcs->atomic_best_encoder)
> > +   if (funcs && funcs->atomic_best_encoder)
> > new_encoder = funcs->atomic_best_encoder(connector, 
> > new_conn_state);
> > -   else if (funcs->best_encoder)
> > +   else if (funcs && funcs->best_encoder)
> > new_encoder = funcs->best_encoder(connector);
> > else
> > new_encoder = drm_atomic_helper_best_encoder(connector);
> > @@ -308,10 +308,10 @@ update_connector_routing(struct drm_atomic_state 
> > *state,
> >  
> > funcs = connector->helper_private;
> >  
> > -   if (funcs->atomic_best_encoder)
> > +   if (funcs && funcs->atomic_best_encoder)
> > new_encoder = funcs->atomic_best_encoder(connector,
> >  new_connector_state);
> > -   else if (funcs->best_encoder)
> > +   else if (funcs && funcs->best_encoder)
> > new_encoder = funcs->best_encoder(connector);
> > else
> > new_encoder = drm_atomic_helper_best_encoder(connector);
> > @@ -438,7 +438,7 @@ mode_fixup(struct drm_atomic_state *state)
> > continue;
> >  
> > funcs = crtc->helper_private;
> > -   if (!funcs->mode_fixup)
> > +   if (!funcs || !funcs->mode_fixup)
> > continue;
> >  
> > ret = funcs->mode_fixup(crtc, _crtc_state->mode,
> > @@ -639,7 +639,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > new_crtc_state->connectors_changed = true;
> > }
> >  
> > -   if (funcs->atomic_check)
> > +   if (funcs && funcs->atomic_check)
> > ret = funcs->atomic_check(connector, 
> > new_connector_state);
> > if (ret)
> > return ret;
> > @@ -681,7 +681,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > if (connectors_mask & BIT(i))
> > continue;
> >  
> > -   if (funcs->atomic_check)
> > +   if (funcs && funcs->atomic_check)
> > ret = funcs->atomic_check(connector, 
> > new_connector_state);
> > if (ret)
> > return ret;
> > @@ -972,14 +972,16 @@ disable_outputs(struct drm_device *dev, struct 
> > drm_atomic_state *old_state)
> >  
> >  
> > /* Right function depends upon target state. */
> > -   if (new_crtc_state->enable && funcs->prepare)
> > -   funcs->prepare(crtc);
> > -   else if (funcs->atomic_disable)
> > -   funcs->atomic_disable(crtc, old_crtc_state);
> > -   else if (funcs->disable)
> > -   funcs->disable(crtc);
> > -   else
> > -   funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
> > +   if (funcs) {
> > +   if (new_crtc_state->enable && funcs->prepare)
> > +   funcs->prepare(crtc);
> > +   else if (funcs->atomic_disable)
> > +   funcs->atomic_disable(crtc, old_crtc_state);
> > +   else if (funcs->disable)
> > +  

Re: [PATCH] drm: Add checks for NULL drm_*_helper_funcs

[Daniel Vetter]

On Fri, May 25, 2018 at 05:20:08AM +0300, Haneen Mohammed wrote:
> This patch add checks for NULL drm_[connector/crtc/plane]_helper_funcs
> pointers before derefrencing the variable to avoid NULL pointer
> dereference and make the helper functions as optional as possible.
> 
> Signed-off-by: Haneen Mohammed 

I started reviewing this, and then realized it's a bit a can of worms.
E.g. connector->funcs->detect shouldn't be there, it's a helper callback
really (but placed in the wrong function table). So connector->funcs isn't
optional, but connector->funcs->detect maybe should be optional.

So I'm not sure anymore whether doing this holesale is a good idea. Do we
still need this for vkms? If yes, then a more focused patch to just make
the things optional that vkms does not (yet) provide might be better.
Including an explanation of what exactly blows up in vkms.

Thanks, Daniel
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 42 +++--
>  drivers/gpu/drm/drm_probe_helper.c  | 11 
>  2 files changed, 28 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index c35654591c12..52092deb741d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -112,9 +112,9 @@ static int handle_conflicting_encoders(struct 
> drm_atomic_state *state,
>   if (!new_conn_state->crtc)
>   continue;
>  
> - if (funcs->atomic_best_encoder)
> + if (funcs && funcs->atomic_best_encoder)
>   new_encoder = funcs->atomic_best_encoder(connector, 
> new_conn_state);
> - else if (funcs->best_encoder)
> + else if (funcs && funcs->best_encoder)
>   new_encoder = funcs->best_encoder(connector);
>   else
>   new_encoder = drm_atomic_helper_best_encoder(connector);
> @@ -308,10 +308,10 @@ update_connector_routing(struct drm_atomic_state *state,
>  
>   funcs = connector->helper_private;
>  
> - if (funcs->atomic_best_encoder)
> + if (funcs && funcs->atomic_best_encoder)
>   new_encoder = funcs->atomic_best_encoder(connector,
>new_connector_state);
> - else if (funcs->best_encoder)
> + else if (funcs && funcs->best_encoder)
>   new_encoder = funcs->best_encoder(connector);
>   else
>   new_encoder = drm_atomic_helper_best_encoder(connector);
> @@ -438,7 +438,7 @@ mode_fixup(struct drm_atomic_state *state)
>   continue;
>  
>   funcs = crtc->helper_private;
> - if (!funcs->mode_fixup)
> + if (!funcs || !funcs->mode_fixup)
>   continue;
>  
>   ret = funcs->mode_fixup(crtc, _crtc_state->mode,
> @@ -639,7 +639,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>   new_crtc_state->connectors_changed = true;
>   }
>  
> - if (funcs->atomic_check)
> + if (funcs && funcs->atomic_check)
>   ret = funcs->atomic_check(connector, 
> new_connector_state);
>   if (ret)
>   return ret;
> @@ -681,7 +681,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>   if (connectors_mask & BIT(i))
>   continue;
>  
> - if (funcs->atomic_check)
> + if (funcs && funcs->atomic_check)
>   ret = funcs->atomic_check(connector, 
> new_connector_state);
>   if (ret)
>   return ret;
> @@ -972,14 +972,16 @@ disable_outputs(struct drm_device *dev, struct 
> drm_atomic_state *old_state)
>  
>  
>   /* Right function depends upon target state. */
> - if (new_crtc_state->enable && funcs->prepare)
> - funcs->prepare(crtc);
> - else if (funcs->atomic_disable)
> - funcs->atomic_disable(crtc, old_crtc_state);
> - else if (funcs->disable)
> - funcs->disable(crtc);
> - else
> - funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
> + if (funcs) {
> + if (new_crtc_state->enable && funcs->prepare)
> + funcs->prepare(crtc);
> + else if (funcs->atomic_disable)
> + funcs->atomic_disable(crtc, old_crtc_state);
> + else if (funcs->disable)
> + funcs->disable(crtc);
> + else
> + funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
> + }
>  
>   if (!(dev->irq_enabled && dev->num_crtcs))
>   continue;
> @@ -1093,7 +1095,7 @@ crtc_set_mode(struct drm_device *dev, struct 
> drm_atomic_state *old_state)
>  
> 

[PATCH] drm: Add checks for NULL drm_*_helper_funcs

[Haneen Mohammed]

This patch add checks for NULL drm_[connector/crtc/plane]_helper_funcs
pointers before derefrencing the variable to avoid NULL pointer
dereference and make the helper functions as optional as possible.

Signed-off-by: Haneen Mohammed 
---
 drivers/gpu/drm/drm_atomic_helper.c | 42 +++--
 drivers/gpu/drm/drm_probe_helper.c  | 11 
 2 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index c35654591c12..52092deb741d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -112,9 +112,9 @@ static int handle_conflicting_encoders(struct 
drm_atomic_state *state,
if (!new_conn_state->crtc)
continue;
 
-   if (funcs->atomic_best_encoder)
+   if (funcs && funcs->atomic_best_encoder)
new_encoder = funcs->atomic_best_encoder(connector, 
new_conn_state);
-   else if (funcs->best_encoder)
+   else if (funcs && funcs->best_encoder)
new_encoder = funcs->best_encoder(connector);
else
new_encoder = drm_atomic_helper_best_encoder(connector);
@@ -308,10 +308,10 @@ update_connector_routing(struct drm_atomic_state *state,
 
funcs = connector->helper_private;
 
-   if (funcs->atomic_best_encoder)
+   if (funcs && funcs->atomic_best_encoder)
new_encoder = funcs->atomic_best_encoder(connector,
 new_connector_state);
-   else if (funcs->best_encoder)
+   else if (funcs && funcs->best_encoder)
new_encoder = funcs->best_encoder(connector);
else
new_encoder = drm_atomic_helper_best_encoder(connector);
@@ -438,7 +438,7 @@ mode_fixup(struct drm_atomic_state *state)
continue;
 
funcs = crtc->helper_private;
-   if (!funcs->mode_fixup)
+   if (!funcs || !funcs->mode_fixup)
continue;
 
ret = funcs->mode_fixup(crtc, _crtc_state->mode,
@@ -639,7 +639,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
new_crtc_state->connectors_changed = true;
}
 
-   if (funcs->atomic_check)
+   if (funcs && funcs->atomic_check)
ret = funcs->atomic_check(connector, 
new_connector_state);
if (ret)
return ret;
@@ -681,7 +681,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
if (connectors_mask & BIT(i))
continue;
 
-   if (funcs->atomic_check)
+   if (funcs && funcs->atomic_check)
ret = funcs->atomic_check(connector, 
new_connector_state);
if (ret)
return ret;
@@ -972,14 +972,16 @@ disable_outputs(struct drm_device *dev, struct 
drm_atomic_state *old_state)
 
 
/* Right function depends upon target state. */
-   if (new_crtc_state->enable && funcs->prepare)
-   funcs->prepare(crtc);
-   else if (funcs->atomic_disable)
-   funcs->atomic_disable(crtc, old_crtc_state);
-   else if (funcs->disable)
-   funcs->disable(crtc);
-   else
-   funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
+   if (funcs) {
+   if (new_crtc_state->enable && funcs->prepare)
+   funcs->prepare(crtc);
+   else if (funcs->atomic_disable)
+   funcs->atomic_disable(crtc, old_crtc_state);
+   else if (funcs->disable)
+   funcs->disable(crtc);
+   else
+   funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
+   }
 
if (!(dev->irq_enabled && dev->num_crtcs))
continue;
@@ -1093,7 +1095,7 @@ crtc_set_mode(struct drm_device *dev, struct 
drm_atomic_state *old_state)
 
funcs = crtc->helper_private;
 
-   if (new_crtc_state->enable && funcs->mode_set_nofb) {
+   if (new_crtc_state->enable && funcs && funcs->mode_set_nofb) {
DRM_DEBUG_ATOMIC("modeset on [CRTC:%d:%s]\n",
 crtc->base.id, crtc->name);
 
@@ -1197,7 +1199,7 @@ void drm_atomic_helper_commit_modeset_enables(struct 
drm_device *dev,
 
funcs = crtc->helper_private;
 
-   if (new_crtc_state->enable) {
+   if (funcs && new_crtc_state->enable) {
DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n",
 crtc->base.id,