| Message ID | 20210423182146.185633-2-lyude@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tegra: Fix device/module refs for DP | expand |
On Fri, Apr 23, 2021 at 02:21:45PM -0400, Lyude Paul wrote: > While we're taking a reference of the DDC adapter for a DP AUX channel in > tegra_sor_probe() because we're going to be using that adapter with the > SOR, now that we've moved where AUX registration happens the actual device > structure for the DDC adapter isn't initialized yet. Which means that we > can't really take a reference from it to try to keep it around anymore. > > This should be fine though, because we can just take a reference of its > parent instead. > > Signed-off-by: Lyude Paul <lyude@redhat.com> > Fixes: 39c17ae60ea9 ("drm/tegra: Don't register DP AUX channels before connectors") > Cc: Lyude Paul <lyude@redhat.com> > Cc: Thierry Reding <thierry.reding@gmail.com> > Cc: Jonathan Hunter <jonathanh@nvidia.com> > Cc: dri-devel@lists.freedesktop.org > Cc: linux-tegra@vger.kernel.org > --- > drivers/gpu/drm/tegra/sor.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c > index 7b88261f57bb..4e0e3a63e586 100644 > --- a/drivers/gpu/drm/tegra/sor.c > +++ b/drivers/gpu/drm/tegra/sor.c > @@ -3739,11 +3739,11 @@ static int tegra_sor_probe(struct platform_device *pdev) > if (!sor->aux) > return -EPROBE_DEFER; > > - if (get_device(&sor->aux->ddc.dev)) { > - if (try_module_get(sor->aux->ddc.owner)) > + if (get_device(sor->aux->dev)) { > + if (try_module_get(sor->aux->dev->driver->owner)) > sor->output.ddc = &sor->aux->ddc; > else > - put_device(&sor->aux->ddc.dev); > + put_device(sor->aux->dev); > } > } Unfortunately, I think it's a bit more subtle than that. The reason for this get_device()/try_module_get() dance was to mirror the behaviour of of_get_i2c_adapter_by_node() so that when we call i2c_put_adapter() in tegra_output_remove() we correctly decrease the reference count. The above will increase the reference on the I2C adapter's parent while i2c_put_adapter() will then only decrease the reference on the I2C adapter, so I think effectively we'd be leaking a reference to the I2C adapter's parent. Also, since we didn't take a reference on the I2C adapter explicitly, releasing that reference in tegra_output_remove() might free the I2C adapter too early. I wonder if perhaps it'd be easier to get rid of the struct tegra_output abstraction altogether and push this down into the individual drivers, even if that means a bit more code duplication. That's not the kind of quick fix to resolve this current situation, so perhaps as a stop-gap we just need to sprinkle a few more conditionals throughout tegra_output code. We could, for example, avoid calling i2c_put_adapter() in tegra_output_remove() for the DisplayPort cases and instead manually release the reference to the I2C adapter's parent in tegra_sor_remove(). On top of your patch above that /should/ fix things properly for now. Thierry
On Mon, 2021-04-26 at 09:42 +0200, Thierry Reding wrote: > On Fri, Apr 23, 2021 at 02:21:45PM -0400, Lyude Paul wrote: > > While we're taking a reference of the DDC adapter for a DP AUX channel in > > tegra_sor_probe() because we're going to be using that adapter with the > > SOR, now that we've moved where AUX registration happens the actual device > > structure for the DDC adapter isn't initialized yet. Which means that we > > can't really take a reference from it to try to keep it around anymore. > > > > This should be fine though, because we can just take a reference of its > > parent instead. > > > > Signed-off-by: Lyude Paul <lyude@redhat.com> > > Fixes: 39c17ae60ea9 ("drm/tegra: Don't register DP AUX channels before > > connectors") > > Cc: Lyude Paul <lyude@redhat.com> > > Cc: Thierry Reding <thierry.reding@gmail.com> > > Cc: Jonathan Hunter <jonathanh@nvidia.com> > > Cc: dri-devel@lists.freedesktop.org > > Cc: linux-tegra@vger.kernel.org > > --- > > drivers/gpu/drm/tegra/sor.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c > > index 7b88261f57bb..4e0e3a63e586 100644 > > --- a/drivers/gpu/drm/tegra/sor.c > > +++ b/drivers/gpu/drm/tegra/sor.c > > @@ -3739,11 +3739,11 @@ static int tegra_sor_probe(struct platform_device > > *pdev) > > if (!sor->aux) > > return -EPROBE_DEFER; > > > > - if (get_device(&sor->aux->ddc.dev)) { > > - if (try_module_get(sor->aux->ddc.owner)) > > + if (get_device(sor->aux->dev)) { > > + if (try_module_get(sor->aux->dev->driver->owner)) > > sor->output.ddc = &sor->aux->ddc; > > else > > - put_device(&sor->aux->ddc.dev); > > + put_device(sor->aux->dev); > > } > > } > > Unfortunately, I think it's a bit more subtle than that. The reason for > this get_device()/try_module_get() dance was to mirror the behaviour of > of_get_i2c_adapter_by_node() so that when we call i2c_put_adapter() in > tegra_output_remove() we correctly decrease the reference count. > > The above will increase the reference on the I2C adapter's parent while > i2c_put_adapter() will then only decrease the reference on the I2C > adapter, so I think effectively we'd be leaking a reference to the I2C > adapter's parent. > > Also, since we didn't take a reference on the I2C adapter explicitly, > releasing that reference in tegra_output_remove() might free the I2C > adapter too early. > > I wonder if perhaps it'd be easier to get rid of the struct tegra_output > abstraction altogether and push this down into the individual drivers, > even if that means a bit more code duplication. That's not the kind of > quick fix to resolve this current situation, so perhaps as a stop-gap we > just need to sprinkle a few more conditionals throughout tegra_output > code. We could, for example, avoid calling i2c_put_adapter() in > tegra_output_remove() for the DisplayPort cases and instead manually > release the reference to the I2C adapter's parent in tegra_sor_remove(). > On top of your patch above that /should/ fix things properly for now. Alright - I will try to get to this tomorrow > > Thierry
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c index 7b88261f57bb..4e0e3a63e586 100644 --- a/drivers/gpu/drm/tegra/sor.c +++ b/drivers/gpu/drm/tegra/sor.c @@ -3739,11 +3739,11 @@ static int tegra_sor_probe(struct platform_device *pdev) if (!sor->aux) return -EPROBE_DEFER; - if (get_device(&sor->aux->ddc.dev)) { - if (try_module_get(sor->aux->ddc.owner)) + if (get_device(sor->aux->dev)) { + if (try_module_get(sor->aux->dev->driver->owner)) sor->output.ddc = &sor->aux->ddc; else - put_device(&sor->aux->ddc.dev); + put_device(sor->aux->dev); } }
While we're taking a reference of the DDC adapter for a DP AUX channel in tegra_sor_probe() because we're going to be using that adapter with the SOR, now that we've moved where AUX registration happens the actual device structure for the DDC adapter isn't initialized yet. Which means that we can't really take a reference from it to try to keep it around anymore. This should be fine though, because we can just take a reference of its parent instead. Signed-off-by: Lyude Paul <lyude@redhat.com> Fixes: 39c17ae60ea9 ("drm/tegra: Don't register DP AUX channels before connectors") Cc: Lyude Paul <lyude@redhat.com> Cc: Thierry Reding <thierry.reding@gmail.com> Cc: Jonathan Hunter <jonathanh@nvidia.com> Cc: dri-devel@lists.freedesktop.org Cc: linux-tegra@vger.kernel.org --- drivers/gpu/drm/tegra/sor.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)