This patch series fixes a regression that was introduced by one of the changes I made to when Tegra registers its AUX adapters, along with fixing some reference leaks that I found along the way.
!!!NOTE!!!
There's one thing I'm not entirely sure about, which is the use of module references (e.g. try_module_get()) here. If I'm understanding how this code worked previously: since the get_device call in tegra_sor_probe() was previously the i2c adapter for the AUX channel - which itself is initialized in drm_dp_aux_register() - then I -think- that the module owner for the DDC adapter would likely have been drm_kms_helper. With these changes, if I'm understanding things correctly we're now just grabbing a module reference for ourselves - something which might not be the best idea?
If anyone could confirm if I need to fix this or not that'd be appreciated, along with reviews of course :P
Lyude Paul (2): drm/tegra: Get ref for DP AUX channel, not its ddc adapter drm/tegra: Fix DP AUX channel reference leaks
drivers/gpu/drm/tegra/sor.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
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); } }
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
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.
v2: * Avoid calling i2c_put_adapter() in tegra_output_remove() for eDP/DP cases
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/output.c | 5 ++++- drivers/gpu/drm/tegra/sor.c | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c index 47d26b5d9945..2dacce1ab6ee 100644 --- a/drivers/gpu/drm/tegra/output.c +++ b/drivers/gpu/drm/tegra/output.c @@ -180,10 +180,13 @@ int tegra_output_probe(struct tegra_output *output)
void tegra_output_remove(struct tegra_output *output) { + int connector_type = output->connector.connector_type; + if (output->hpd_gpio) free_irq(output->hpd_irq, output);
- if (output->ddc) + if (connector_type != DRM_MODE_CONNECTOR_eDP && + connector_type != DRM_MODE_CONNECTOR_DisplayPort && output->ddc) i2c_put_adapter(output->ddc); }
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); } }
Noticed while fixing the regression I introduced in Tegra that Tegra seems to actually never release the device or module references it's grabbing for the DP AUX channel. So, let's fix that by dropping them when appropriate.
Signed-off-by: Lyude Paul lyude@redhat.com --- drivers/gpu/drm/tegra/sor.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c index 4e0e3a63e586..474586e18d06 100644 --- a/drivers/gpu/drm/tegra/sor.c +++ b/drivers/gpu/drm/tegra/sor.c @@ -3772,12 +3772,13 @@ static int tegra_sor_probe(struct platform_device *pdev)
err = tegra_sor_parse_dt(sor); if (err < 0) - return err; + goto put_aux;
err = tegra_output_probe(&sor->output); - if (err < 0) - return dev_err_probe(&pdev->dev, err, - "failed to probe output\n"); + if (err < 0) { + err = dev_err_probe(&pdev->dev, err, "failed to probe output\n"); + goto put_aux; + }
if (sor->ops && sor->ops->probe) { err = sor->ops->probe(sor); @@ -3966,6 +3967,11 @@ static int tegra_sor_probe(struct platform_device *pdev) pm_runtime_disable(&pdev->dev); remove: tegra_output_remove(&sor->output); +put_aux: + if (sor->aux && sor->output.ddc) { + module_put(sor->aux->dev->driver->owner); + put_device(sor->aux->dev); + } return err; }
@@ -3985,6 +3991,11 @@ static int tegra_sor_remove(struct platform_device *pdev)
tegra_output_remove(&sor->output);
+ if (sor->aux && sor->output.ddc) { + module_put(sor->aux->dev->driver->owner); + put_device(sor->aux->dev); + } + return 0; }
dri-devel@lists.freedesktop.org
-
Lyude Paul -
Thierry Reding