Re: [PATCH 8/9] drm/tegra: dpaux: Add missing runtime PM references

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Nov 29, 2019 at 11:44:12AM +0100, Thierry Reding wrote:
> On Fri, Nov 29, 2019 at 10:23:19AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 28, 2019 at 04:37:40PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > Ensure that a runtime PM reference is acquired each time the DPAUX
> > > registers are accessed. Otherwise the code may end up running without
> > > the controller being powered, out-of-reset or clocked in some corner
> > > cases, resulting in a crash.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > 
> > On patches 4,5,7 in this series Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > On this one here I'm very confused.
> > 
> > - Why do you drop the runtime pm between enable and disable? Is that just
> >   how the hw works, i.e. the pad config stays, just the registers go away?
> 
> Now you've made me doubt this. I don't think the pad configuration stays
> across runtime suspend/resume, so you're right, this shouldn't work.
> I'll need to go retest this one specifically.
> 
> I had added these runtime PM references to ensure the device was
> properly configured at resume from suspend, but there ended up being an
> additional issue with the I2C driver that relies on this, so perhaps
> this may not be necessary in the end.
> 
> > - I'm not seeing any locking between the different users (dp aux and
> >   pinctrl). We might want to change drm_dp_aux->hw_mutex to a pointer to
> >   make this easier (but I'm not super fond of that pattern from i2c).
> 
> There should be no need to lock here. DP AUX transfers will only be used
> between drm_dp_aux_enable() and drm_dp_aux_disable().

So dp aux vs. dp aux aside (that's the next point below), there's
guaranteed no one else can get at that pinctrl mux? Since the other
setting is labelled I2C I assumed there's an i2c controller hanging of it,
exposed to userspace, and userspace might probe that whenever it feels
like (similar to the issue below). But I don't know your hw, nor do I
really know pinctrl. Just looked a bit strange.

Cheers, Daniel


> > - Your drm_dp_aux_enable/disable needs to be moved into the ->transfer
> >   callback, otherwise the various userspace interface (dp aux, but also
> >   i2c on top of that) won't work. Some pre/post_transfer functions like
> >   i2c has might be useful for stuff like this.
> 
> I suppose it would be possible for someone to attempt to use those
> userspace interfaces outside of drm_dp_aux_enable()/drm_dp_aux_disable()
> and then the locking would be required.
> 
> I'll look into that.
> 
> Thierry
> 
> > 
> > Cheers, Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/tegra/dpaux.c | 16 ++++++++++++++--
> > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
> > > index 622cdf1ad246..4b2b86aed1a5 100644
> > > --- a/drivers/gpu/drm/tegra/dpaux.c
> > > +++ b/drivers/gpu/drm/tegra/dpaux.c
> > > @@ -434,8 +434,13 @@ static int tegra_dpaux_set_mux(struct pinctrl_dev *pinctrl,
> > >  			       unsigned int function, unsigned int group)
> > >  {
> > >  	struct tegra_dpaux *dpaux = pinctrl_dev_get_drvdata(pinctrl);
> > > +	int err;
> > > +
> > > +	pm_runtime_get_sync(dpaux->dev);
> > > +	err = tegra_dpaux_pad_config(dpaux, function);
> > > +	pm_runtime_put(dpaux->dev);
> > >  
> > > -	return tegra_dpaux_pad_config(dpaux, function);
> > > +	return err;
> > >  }
> > >  
> > >  static const struct pinmux_ops tegra_dpaux_pinmux_ops = {
> > > @@ -809,15 +814,22 @@ enum drm_connector_status drm_dp_aux_detect(struct drm_dp_aux *aux)
> > >  int drm_dp_aux_enable(struct drm_dp_aux *aux)
> > >  {
> > >  	struct tegra_dpaux *dpaux = to_dpaux(aux);
> > > +	int err;
> > > +
> > > +	pm_runtime_get_sync(dpaux->dev);
> > > +	err = tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > > +	pm_runtime_put(dpaux->dev);
> > >  
> > > -	return tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > > +	return err;
> > >  }
> > >  
> > >  int drm_dp_aux_disable(struct drm_dp_aux *aux)
> > >  {
> > >  	struct tegra_dpaux *dpaux = to_dpaux(aux);
> > >  
> > > +	pm_runtime_get_sync(dpaux->dev);
> > >  	tegra_dpaux_pad_power_down(dpaux);
> > > +	pm_runtime_put(dpaux->dev);
> > >  
> > >  	return 0;
> > >  }
> > > -- 
> > > 2.23.0
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel