[PATCH 1/2] drm/tegra: Get ref for DP AUX channel, not its ddc adapter

[PATCH 1/2] drm/tegra: Get ref for DP AUX channel, not its ddc adapter

[Lyude Paul]

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

Re: [PATCH 1/2] drm/tegra: Get ref for DP AUX channel, not its ddc adapter

[Thierry Reding]

[-- Attachment #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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] drm/tegra: Get ref for DP AUX channel, not its ddc adapter

[Lyude Paul]

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

[PATCH v2] drm/tegra: Get ref for DP AUX channel, not its ddc adapter

[Lyude Paul]

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

[PATCH 2/2] drm/tegra: Fix DP AUX channel reference leaks

[Lyude Paul]

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