[PATCH] drm/mipi-dsi: Fix detach call without attach

[PATCH] drm/mipi-dsi: Fix detach call without attach

[Tomi Valkeinen]

It's been reported that DSI host driver's detach can be called without
the attach ever happening:

https://lore.kernel.org/all/20230412073954.20601-1-tony@atomide.com/

After reading the code, I think this is what happens:

We have a DSI host defined in the device tree and a DSI peripheral under
that host (i.e. an i2c device using the DSI as data bus doesn't exhibit
this behavior).

The host driver calls mipi_dsi_host_register(), which causes (via a few
functions) mipi_dsi_device_add() to be called for the DSI peripheral. So
now we have a DSI device under the host, but attach hasn't been called.

Normally the probing of the devices continues, and eventually the DSI
peripheral's driver will call mipi_dsi_attach(), attaching the
peripheral.

However, if the host driver's probe encounters an error after calling
mipi_dsi_host_register(), and before the peripheral has called
mipi_dsi_attach(), the host driver will do cleanups and return an error
from its probe function. The cleanups include calling
mipi_dsi_host_unregister().

mipi_dsi_host_unregister() will call two functions for all its DSI
peripheral devices: mipi_dsi_detach() and mipi_dsi_device_unregister().
The latter makes sense, as the device exists, but the former may be
wrong as attach has not necessarily been done.

To fix this, track the attached state of the peripheral, and only detach
from mipi_dsi_host_unregister() if the peripheral was attached.

Note that I have only tested this with a board with an i2c DSI
peripheral, not with a "pure" DSI peripheral.

However, slightly related, the unregister machinery still seems broken.
E.g. if the DSI host driver is unbound, it'll detach and unregister the
DSI peripherals. After that, when the DSI peripheral driver unbound
it'll call detach either directly or using the devm variant, leading to
a crash. And probably the driver will crash if it happens, for some
reason, to try to send a message via the DSI bus.

But that's another topic.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 17 +++++++++++++++--
 include/drm/drm_mipi_dsi.h     |  2 ++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 14201f73aab1..843a6dbda93a 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -347,7 +347,8 @@ static int mipi_dsi_remove_device_fn(struct device *dev, void *priv)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
 
-	mipi_dsi_detach(dsi);
+	if (dsi->attached)
+		mipi_dsi_detach(dsi);
 	mipi_dsi_device_unregister(dsi);
 
 	return 0;
@@ -370,11 +371,18 @@ EXPORT_SYMBOL(mipi_dsi_host_unregister);
 int mipi_dsi_attach(struct mipi_dsi_device *dsi)
 {
 	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
+	int ret;
 
 	if (!ops || !ops->attach)
 		return -ENOSYS;
 
-	return ops->attach(dsi->host, dsi);
+	ret = ops->attach(dsi->host, dsi);
+	if (ret)
+		return ret;
+
+	dsi->attached = true;
+
+	return 0;
 }
 EXPORT_SYMBOL(mipi_dsi_attach);
 
@@ -386,9 +394,14 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
 {
 	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
 
+	if (WARN_ON(!dsi->attached))
+		return -EINVAL;
+
 	if (!ops || !ops->detach)
 		return -ENOSYS;
 
+	dsi->attached = false;
+
 	return ops->detach(dsi->host, dsi);
 }
 EXPORT_SYMBOL(mipi_dsi_detach);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index c9df0407980c..c0aec0d4d664 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -168,6 +168,7 @@ struct mipi_dsi_device_info {
  * struct mipi_dsi_device - DSI peripheral device
  * @host: DSI host for this peripheral
  * @dev: driver model device node for this peripheral
+ * @attached: the DSI device has been successfully attached
  * @name: DSI peripheral chip type
  * @channel: virtual channel assigned to the peripheral
  * @format: pixel format for video mode
@@ -184,6 +185,7 @@ struct mipi_dsi_device_info {
 struct mipi_dsi_device {
 	struct mipi_dsi_host *host;
 	struct device dev;
+	bool attached;
 
 	char name[DSI_DEV_NAME_SIZE];
 	unsigned int channel;

---
base-commit: 9fc75c40faa29df14ba16066be6bdfaea9f39ce4
change-id: 20230921-dsi-detach-fix-6736f7a48ba7

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Re: [PATCH] drm/mipi-dsi: Fix detach call without attach

[Tony Lindgren]

* Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> [230921 10:51]:
> mipi_dsi_host_unregister() will call two functions for all its DSI
> peripheral devices: mipi_dsi_detach() and mipi_dsi_device_unregister().
> The latter makes sense, as the device exists, but the former may be
> wrong as attach has not necessarily been done.
> 
> To fix this, track the attached state of the peripheral, and only detach
> from mipi_dsi_host_unregister() if the peripheral was attached.
> 
> Note that I have only tested this with a board with an i2c DSI
> peripheral, not with a "pure" DSI peripheral.

Thanks this fixes the deferred probe warning I've been seeing:

Tested-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH] drm/mipi-dsi: Fix detach call without attach

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4882 bytes --]

Hi,

On Thu, Sep 21, 2023 at 01:50:32PM +0300, Tomi Valkeinen wrote:
> It's been reported that DSI host driver's detach can be called without
> the attach ever happening:
> 
> https://lore.kernel.org/all/20230412073954.20601-1-tony@atomide.com/
> 
> After reading the code, I think this is what happens:
> 
> We have a DSI host defined in the device tree and a DSI peripheral under
> that host (i.e. an i2c device using the DSI as data bus doesn't exhibit
> this behavior).
> 
> The host driver calls mipi_dsi_host_register(), which causes (via a few
> functions) mipi_dsi_device_add() to be called for the DSI peripheral. So
> now we have a DSI device under the host, but attach hasn't been called.
> 
> Normally the probing of the devices continues, and eventually the DSI
> peripheral's driver will call mipi_dsi_attach(), attaching the
> peripheral.
> 
> However, if the host driver's probe encounters an error after calling
> mipi_dsi_host_register(), and before the peripheral has called
> mipi_dsi_attach(), the host driver will do cleanups and return an error
> from its probe function. The cleanups include calling
> mipi_dsi_host_unregister().
> 
> mipi_dsi_host_unregister() will call two functions for all its DSI
> peripheral devices: mipi_dsi_detach() and mipi_dsi_device_unregister().
> The latter makes sense, as the device exists, but the former may be
> wrong as attach has not necessarily been done.
> 
> To fix this, track the attached state of the peripheral, and only detach
> from mipi_dsi_host_unregister() if the peripheral was attached.
> 
> Note that I have only tested this with a board with an i2c DSI
> peripheral, not with a "pure" DSI peripheral.
> 
> However, slightly related, the unregister machinery still seems broken.
> E.g. if the DSI host driver is unbound, it'll detach and unregister the
> DSI peripherals. After that, when the DSI peripheral driver unbound
> it'll call detach either directly or using the devm variant, leading to
> a crash. And probably the driver will crash if it happens, for some
> reason, to try to send a message via the DSI bus.
> 
> But that's another topic.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/gpu/drm/drm_mipi_dsi.c | 17 +++++++++++++++--
>  include/drm/drm_mipi_dsi.h     |  2 ++
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 14201f73aab1..843a6dbda93a 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -347,7 +347,8 @@ static int mipi_dsi_remove_device_fn(struct device *dev, void *priv)
>  {
>  	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
>  
> -	mipi_dsi_detach(dsi);
> +	if (dsi->attached)
> +		mipi_dsi_detach(dsi);
>  	mipi_dsi_device_unregister(dsi);
>  
>  	return 0;
> @@ -370,11 +371,18 @@ EXPORT_SYMBOL(mipi_dsi_host_unregister);
>  int mipi_dsi_attach(struct mipi_dsi_device *dsi)
>  {
>  	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
> +	int ret;
>  
>  	if (!ops || !ops->attach)
>  		return -ENOSYS;
>  
> -	return ops->attach(dsi->host, dsi);
> +	ret = ops->attach(dsi->host, dsi);
> +	if (ret)
> +		return ret;
> +
> +	dsi->attached = true;
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(mipi_dsi_attach);
>  
> @@ -386,9 +394,14 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
>  {
>  	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>  
> +	if (WARN_ON(!dsi->attached))
> +		return -EINVAL;
> +
>  	if (!ops || !ops->detach)
>  		return -ENOSYS;
>  
> +	dsi->attached = false;
> +
>  	return ops->detach(dsi->host, dsi);
>  }
>  EXPORT_SYMBOL(mipi_dsi_detach);
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index c9df0407980c..c0aec0d4d664 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -168,6 +168,7 @@ struct mipi_dsi_device_info {
>   * struct mipi_dsi_device - DSI peripheral device
>   * @host: DSI host for this peripheral
>   * @dev: driver model device node for this peripheral
> + * @attached: the DSI device has been successfully attached
>   * @name: DSI peripheral chip type
>   * @channel: virtual channel assigned to the peripheral
>   * @format: pixel format for video mode
> @@ -184,6 +185,7 @@ struct mipi_dsi_device_info {
>  struct mipi_dsi_device {
>  	struct mipi_dsi_host *host;
>  	struct device dev;
> +	bool attached;
>  
>  	char name[DSI_DEV_NAME_SIZE];
>  	unsigned int channel;
> 
> ---
> base-commit: 9fc75c40faa29df14ba16066be6bdfaea9f39ce4
> change-id: 20230921-dsi-detach-fix-6736f7a48ba7
> 
> Best regards,
> -- 
> Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm/mipi-dsi: Fix detach call without attach

[H. Nikolaus Schaller]

Hi,
I have tested this on an OMAP5 Pyra.

> Am 21.09.2023 um 12:50 schrieb Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>:
> 
> It's been reported that DSI host driver's detach can be called without
> the attach ever happening:
> 
> https://lore.kernel.org/all/20230412073954.20601-1-tony@atomide.com/
> 

This patch works equally well as my proposal and is indeed a better solution
(solving the issue and not just suppressing a warning).

Tested-by: H. Nikolaus Schaller <hns@goldelico.com>

BR and thanks,
Nikolaus

Re: [PATCH] drm/mipi-dsi: Fix detach call without attach

[Tomi Valkeinen]

Hi mipi dsi maintainers (I'm not sure who that is =),

On 21/09/2023 13:50, Tomi Valkeinen wrote:
> It's been reported that DSI host driver's detach can be called without
> the attach ever happening:
> 
> https://lore.kernel.org/all/20230412073954.20601-1-tony@atomide.com/
> 
> After reading the code, I think this is what happens:
> 
> We have a DSI host defined in the device tree and a DSI peripheral under
> that host (i.e. an i2c device using the DSI as data bus doesn't exhibit
> this behavior).
> 
> The host driver calls mipi_dsi_host_register(), which causes (via a few
> functions) mipi_dsi_device_add() to be called for the DSI peripheral. So
> now we have a DSI device under the host, but attach hasn't been called.
> 
> Normally the probing of the devices continues, and eventually the DSI
> peripheral's driver will call mipi_dsi_attach(), attaching the
> peripheral.
> 
> However, if the host driver's probe encounters an error after calling
> mipi_dsi_host_register(), and before the peripheral has called
> mipi_dsi_attach(), the host driver will do cleanups and return an error
> from its probe function. The cleanups include calling
> mipi_dsi_host_unregister().
> 
> mipi_dsi_host_unregister() will call two functions for all its DSI
> peripheral devices: mipi_dsi_detach() and mipi_dsi_device_unregister().
> The latter makes sense, as the device exists, but the former may be
> wrong as attach has not necessarily been done.
> 
> To fix this, track the attached state of the peripheral, and only detach
> from mipi_dsi_host_unregister() if the peripheral was attached.
> 
> Note that I have only tested this with a board with an i2c DSI
> peripheral, not with a "pure" DSI peripheral.
> 
> However, slightly related, the unregister machinery still seems broken.
> E.g. if the DSI host driver is unbound, it'll detach and unregister the
> DSI peripherals. After that, when the DSI peripheral driver unbound
> it'll call detach either directly or using the devm variant, leading to
> a crash. And probably the driver will crash if it happens, for some
> reason, to try to send a message via the DSI bus.
> 
> But that's another topic.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---

Any comments? I can push this via drm-misc, but I'd like an ack.

  Tomi


>   drivers/gpu/drm/drm_mipi_dsi.c | 17 +++++++++++++++--
>   include/drm/drm_mipi_dsi.h     |  2 ++
>   2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 14201f73aab1..843a6dbda93a 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -347,7 +347,8 @@ static int mipi_dsi_remove_device_fn(struct device *dev, void *priv)
>   {
>   	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
>   
> -	mipi_dsi_detach(dsi);
> +	if (dsi->attached)
> +		mipi_dsi_detach(dsi);
>   	mipi_dsi_device_unregister(dsi);
>   
>   	return 0;
> @@ -370,11 +371,18 @@ EXPORT_SYMBOL(mipi_dsi_host_unregister);
>   int mipi_dsi_attach(struct mipi_dsi_device *dsi)
>   {
>   	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
> +	int ret;
>   
>   	if (!ops || !ops->attach)
>   		return -ENOSYS;
>   
> -	return ops->attach(dsi->host, dsi);
> +	ret = ops->attach(dsi->host, dsi);
> +	if (ret)
> +		return ret;
> +
> +	dsi->attached = true;
> +
> +	return 0;
>   }
>   EXPORT_SYMBOL(mipi_dsi_attach);
>   
> @@ -386,9 +394,14 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
>   {
>   	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>   
> +	if (WARN_ON(!dsi->attached))
> +		return -EINVAL;
> +
>   	if (!ops || !ops->detach)
>   		return -ENOSYS;
>   
> +	dsi->attached = false;
> +
>   	return ops->detach(dsi->host, dsi);
>   }
>   EXPORT_SYMBOL(mipi_dsi_detach);
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index c9df0407980c..c0aec0d4d664 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -168,6 +168,7 @@ struct mipi_dsi_device_info {
>    * struct mipi_dsi_device - DSI peripheral device
>    * @host: DSI host for this peripheral
>    * @dev: driver model device node for this peripheral
> + * @attached: the DSI device has been successfully attached
>    * @name: DSI peripheral chip type
>    * @channel: virtual channel assigned to the peripheral
>    * @format: pixel format for video mode
> @@ -184,6 +185,7 @@ struct mipi_dsi_device_info {
>   struct mipi_dsi_device {
>   	struct mipi_dsi_host *host;
>   	struct device dev;
> +	bool attached;
>   
>   	char name[DSI_DEV_NAME_SIZE];
>   	unsigned int channel;
> 
> ---
> base-commit: 9fc75c40faa29df14ba16066be6bdfaea9f39ce4
> change-id: 20230921-dsi-detach-fix-6736f7a48ba7
> 
> Best regards,

Re: [PATCH] drm/mipi-dsi: Fix detach call without attach

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2630 bytes --]

Hi,

On Wed, Dec 06, 2023 at 02:48:58PM +0200, Tomi Valkeinen wrote:
> Hi mipi dsi maintainers (I'm not sure who that is =),
> 
> On 21/09/2023 13:50, Tomi Valkeinen wrote:
> > It's been reported that DSI host driver's detach can be called without
> > the attach ever happening:
> > 
> > https://lore.kernel.org/all/20230412073954.20601-1-tony@atomide.com/
> > 
> > After reading the code, I think this is what happens:
> > 
> > We have a DSI host defined in the device tree and a DSI peripheral under
> > that host (i.e. an i2c device using the DSI as data bus doesn't exhibit
> > this behavior).
> > 
> > The host driver calls mipi_dsi_host_register(), which causes (via a few
> > functions) mipi_dsi_device_add() to be called for the DSI peripheral. So
> > now we have a DSI device under the host, but attach hasn't been called.
> > 
> > Normally the probing of the devices continues, and eventually the DSI
> > peripheral's driver will call mipi_dsi_attach(), attaching the
> > peripheral.
> > 
> > However, if the host driver's probe encounters an error after calling
> > mipi_dsi_host_register(), and before the peripheral has called
> > mipi_dsi_attach(), the host driver will do cleanups and return an error
> > from its probe function. The cleanups include calling
> > mipi_dsi_host_unregister().
> > 
> > mipi_dsi_host_unregister() will call two functions for all its DSI
> > peripheral devices: mipi_dsi_detach() and mipi_dsi_device_unregister().
> > The latter makes sense, as the device exists, but the former may be
> > wrong as attach has not necessarily been done.
> > 
> > To fix this, track the attached state of the peripheral, and only detach
> > from mipi_dsi_host_unregister() if the peripheral was attached.
> > 
> > Note that I have only tested this with a board with an i2c DSI
> > peripheral, not with a "pure" DSI peripheral.
> > 
> > However, slightly related, the unregister machinery still seems broken.
> > E.g. if the DSI host driver is unbound, it'll detach and unregister the
> > DSI peripherals. After that, when the DSI peripheral driver unbound
> > it'll call detach either directly or using the devm variant, leading to
> > a crash. And probably the driver will crash if it happens, for some
> > reason, to try to send a message via the DSI bus.
> > 
> > But that's another topic.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > ---
> 
> Any comments? I can push this via drm-misc, but I'd like an ack.

An ack is a requirement, it's not optional.

Acked-by: Maxime Ripard <mripard@kernel.org>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]