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 <[email protected]>
Fixes: 39c17ae60ea9 ("drm/tegra: Don't register DP AUX channels before connectors")
Cc: Lyude Paul <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Jonathan Hunter <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
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 <[email protected]>
> Fixes: 39c17ae60ea9 ("drm/tegra: Don't register DP AUX channels before connectors")
> Cc: Lyude Paul <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Cc: Jonathan Hunter <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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 <[email protected]>
> > Fixes: 39c17ae60ea9 ("drm/tegra: Don't register DP AUX channels before
> > connectors")
> > Cc: Lyude Paul <[email protected]>
> > Cc: Thierry Reding <[email protected]>
> > Cc: Jonathan Hunter <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > 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
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 <[email protected]>
Fixes: 39c17ae60ea9 ("drm/tegra: Don't register DP AUX channels before connectors")
Cc: Lyude Paul <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Jonathan Hunter <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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