Skip to content

Commit

Permalink
drm/mipi-dsi: Fix detach call without attach
Browse files Browse the repository at this point in the history
It's been reported that DSI host driver's detach can be called without
the attach ever happening:

https://lore.kernel.org/all/[email protected]/

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.

Tested-by: H. Nikolaus Schaller <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Reviewed-by: Sebastian Reichel <[email protected]>
Tested-by: Tony Lindgren <[email protected]>
Signed-off-by: Tomi Valkeinen <[email protected]>
Link: https://patchwork.freedesktop.org/patch/msgid/[email protected]
  • Loading branch information
tomba committed Dec 7, 2023
1 parent 32bd29b commit 90d50b8
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 2 deletions.
17 changes: 15 additions & 2 deletions drivers/gpu/drm/drm_mipi_dsi.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);

Expand All @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions include/drm/drm_mipi_dsi.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down

0 comments on commit 90d50b8

Please sign in to comment.