| Message ID | 20230921-dsi-detach-fix-v1-1-d0de2d1621d9@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mipi-dsi: Fix detach call without attach | expand |
* 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>
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> >
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
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,
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
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;
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(-) --- base-commit: 9fc75c40faa29df14ba16066be6bdfaea9f39ce4 change-id: 20230921-dsi-detach-fix-6736f7a48ba7 Best regards,