* [PATCH 0/2] drm/tegra: Fix device/module refs for DP @ 2021-04-23 18:21 Lyude Paul 2021-04-23 18:21 ` [PATCH 1/2] drm/tegra: Get ref for DP AUX channel, not its ddc adapter Lyude Paul 2021-04-23 18:21 ` [PATCH 2/2] drm/tegra: Fix DP AUX channel reference leaks Lyude Paul 0 siblings, 2 replies; 6+ messages in thread From: Lyude Paul @ 2021-04-23 18:21 UTC (permalink / raw) To: linux-tegra, dri-devel; +Cc: Thierry Reding 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(-) -- 2.30.2 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] drm/tegra: Get ref for DP AUX channel, not its ddc adapter 2021-04-23 18:21 [PATCH 0/2] drm/tegra: Fix device/module refs for DP Lyude Paul @ 2021-04-23 18:21 ` Lyude Paul 2021-04-26 7:42 ` Thierry Reding 2021-05-14 22:13 ` [PATCH v2] " Lyude Paul 2021-04-23 18:21 ` [PATCH 2/2] drm/tegra: Fix DP AUX channel reference leaks Lyude Paul 1 sibling, 2 replies; 6+ messages in thread From: Lyude Paul @ 2021-04-23 18:21 UTC (permalink / raw) To: linux-tegra, dri-devel Cc: David Airlie, open list, Jonathan Hunter, Thierry Reding, Thierry Reding 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); } } -- 2.30.2 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/tegra: Get ref for DP AUX channel, not its ddc adapter 2021-04-23 18:21 ` [PATCH 1/2] drm/tegra: Get ref for DP AUX channel, not its ddc adapter Lyude Paul @ 2021-04-26 7:42 ` Thierry Reding 2021-04-27 22:44 ` Lyude Paul 2021-05-14 22:13 ` [PATCH v2] " Lyude Paul 1 sibling, 1 reply; 6+ messages in thread From: Thierry Reding @ 2021-04-26 7:42 UTC (permalink / raw) To: Lyude Paul Cc: David Airlie, open list, dri-devel, Jonathan Hunter, linux-tegra, Thierry Reding [-- Attachment #1.1: Type: text/plain, Size: 2992 bytes --] 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 [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/tegra: Get ref for DP AUX channel, not its ddc adapter 2021-04-26 7:42 ` Thierry Reding @ 2021-04-27 22:44 ` Lyude Paul 0 siblings, 0 replies; 6+ messages in thread From: Lyude Paul @ 2021-04-27 22:44 UTC (permalink / raw) To: Thierry Reding Cc: David Airlie, open list, dri-devel, Jonathan Hunter, linux-tegra, Thierry Reding 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 -- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] drm/tegra: Get ref for DP AUX channel, not its ddc adapter 2021-04-23 18:21 ` [PATCH 1/2] drm/tegra: Get ref for DP AUX channel, not its ddc adapter Lyude Paul 2021-04-26 7:42 ` Thierry Reding @ 2021-05-14 22:13 ` Lyude Paul 1 sibling, 0 replies; 6+ messages in thread From: Lyude Paul @ 2021-05-14 22:13 UTC (permalink / raw) To: dri-devel, Thierry Reding Cc: linux-tegra, David Airlie, open list, Jonathan Hunter 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); } } -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/tegra: Fix DP AUX channel reference leaks 2021-04-23 18:21 [PATCH 0/2] drm/tegra: Fix device/module refs for DP Lyude Paul 2021-04-23 18:21 ` [PATCH 1/2] drm/tegra: Get ref for DP AUX channel, not its ddc adapter Lyude Paul @ 2021-04-23 18:21 ` Lyude Paul 1 sibling, 0 replies; 6+ messages in thread From: Lyude Paul @ 2021-04-23 18:21 UTC (permalink / raw) To: linux-tegra, dri-devel Cc: David Airlie, open list, Jonathan Hunter, Thierry Reding, Thierry Reding 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; } -- 2.30.2 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-14 22:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-23 18:21 [PATCH 0/2] drm/tegra: Fix device/module refs for DP Lyude Paul 2021-04-23 18:21 ` [PATCH 1/2] drm/tegra: Get ref for DP AUX channel, not its ddc adapter Lyude Paul 2021-04-26 7:42 ` Thierry Reding 2021-04-27 22:44 ` Lyude Paul 2021-05-14 22:13 ` [PATCH v2] " Lyude Paul 2021-04-23 18:21 ` [PATCH 2/2] drm/tegra: Fix DP AUX channel reference leaks Lyude Paul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).