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@xxxxxxxxxxx/ > > 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@xxxxxxxxxxxxxxxx> > --- Reviewed-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx> -- 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@xxxxxxxxxxxxxxxx> >
Attachment:
signature.asc
Description: PGP signature
