| Message ID | 20241231-hotplug-drm-bridge-v5-8-173065a1ece1@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for hot-pluggable DRM bridges | expand |
On Tue, Dec 31, 2024 at 11:40:02AM +0100, Luca Ceresoli wrote: > Instead of using dsi->out_bridge during the bridge search process, use a > temporary variable and assign dsi->out_bridge only on successful > completion. > > The main goal is to be able to drm_bridge_get() the out_bridge before > setting it in dsi->out_bridge, which is done in a later commit. Setting > dsi->out_bridge as in current code would leave a use-after-free window in > case the bridge is deallocated by some other thread between > 'dsi->out_bridge = devm_drm_panel_bridge_add()' and drm_bridge_get(). I don't think that's how refcounting should work. Any of the functions that give you the bridge should also increase refcount, requiring manual _put() call afterwards. We might need a separate API for that. > > This change additionally avoids leaving an ERR_PTR value in dsi->out_bridge > on failure. This is not necessarily a problem but it is not clean. > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > --- > > This patch was added in v5. > --- > drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c > index f8b4fb8357659018ec0db65374ee5d05330639ae..c4d1563fd32019efde523dfc0863be044c05a826 100644 > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > @@ -1705,6 +1705,7 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host, > struct device *dev = dsi->dev; > struct device_node *np = dev->of_node; > struct device_node *remote; > + struct drm_bridge *out_bridge; > struct drm_panel *panel; > int ret; > > @@ -1740,21 +1741,23 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host, > > panel = of_drm_find_panel(remote); > if (!IS_ERR(panel)) { > - dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel); > + out_bridge = devm_drm_panel_bridge_add(dev, panel); > } else { > - dsi->out_bridge = of_drm_find_bridge(remote); > - if (!dsi->out_bridge) > - dsi->out_bridge = ERR_PTR(-EINVAL); > + out_bridge = of_drm_find_bridge(remote); > + if (!out_bridge) > + out_bridge = ERR_PTR(-EINVAL); > } While looking at this patch, I think we should migrate the driver to drm_of_find_panel_or_bridge(). Then your patch might add a function close to devm_drm_of_get_bridge() or drmm_of_get_bridge(). Otherwise we end up having too much duplicate boilerplate code. > > of_node_put(remote); > > - if (IS_ERR(dsi->out_bridge)) { > - ret = PTR_ERR(dsi->out_bridge); > + if (IS_ERR(out_bridge)) { > + ret = PTR_ERR(out_bridge); > DRM_DEV_ERROR(dev, "failed to find the bridge: %d\n", ret); > return ret; > } > > + dsi->out_bridge = out_bridge; > + > DRM_DEV_INFO(dev, "Attached %s device (lanes:%d bpp:%d mode-flags:0x%lx)\n", > device->name, device->lanes, > mipi_dsi_pixel_format_to_bpp(device->format), > > -- > 2.34.1 >
Hi Dmitry, On Tue, 31 Dec 2024 16:57:38 +0200 Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > On Tue, Dec 31, 2024 at 11:40:02AM +0100, Luca Ceresoli wrote: > > Instead of using dsi->out_bridge during the bridge search process, use a > > temporary variable and assign dsi->out_bridge only on successful > > completion. > > > > The main goal is to be able to drm_bridge_get() the out_bridge before > > setting it in dsi->out_bridge, which is done in a later commit. Setting > > dsi->out_bridge as in current code would leave a use-after-free window in > > case the bridge is deallocated by some other thread between > > 'dsi->out_bridge = devm_drm_panel_bridge_add()' and drm_bridge_get(). > > I don't think that's how refcounting should work. Any of the functions > that give you the bridge should also increase refcount, requiring manual > _put() call afterwards. We might need a separate API for that. You're perfectly right. > > This change additionally avoids leaving an ERR_PTR value in dsi->out_bridge > > on failure. This is not necessarily a problem but it is not clean. > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > > > --- > > > > This patch was added in v5. > > --- > > drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c > > index f8b4fb8357659018ec0db65374ee5d05330639ae..c4d1563fd32019efde523dfc0863be044c05a826 100644 > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > > @@ -1705,6 +1705,7 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host, > > struct device *dev = dsi->dev; > > struct device_node *np = dev->of_node; > > struct device_node *remote; > > + struct drm_bridge *out_bridge; > > struct drm_panel *panel; > > int ret; > > > > @@ -1740,21 +1741,23 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host, > > > > panel = of_drm_find_panel(remote); > > if (!IS_ERR(panel)) { > > - dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel); > > + out_bridge = devm_drm_panel_bridge_add(dev, panel); > > } else { > > - dsi->out_bridge = of_drm_find_bridge(remote); > > - if (!dsi->out_bridge) > > - dsi->out_bridge = ERR_PTR(-EINVAL); > > + out_bridge = of_drm_find_bridge(remote); > > + if (!out_bridge) > > + out_bridge = ERR_PTR(-EINVAL); > > } > > While looking at this patch, I think we should migrate the driver to > drm_of_find_panel_or_bridge(). Indeed, the code here is duplicating drm_of_find_panel_or_bridge(). I'm going to convert it. > Then your patch might add a function > close to devm_drm_of_get_bridge() or drmm_of_get_bridge(). ...which would return a bridge pointer, with refcount already incremented. Sure, except I think it should _not_ be a drmm, as the bridge might itself disappear while the card keeps existing. Luca
On Thu, Jan 02, 2025 at 01:01:49PM +0100, Luca Ceresoli wrote: > Hi Dmitry, > > On Tue, 31 Dec 2024 16:57:38 +0200 > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > On Tue, Dec 31, 2024 at 11:40:02AM +0100, Luca Ceresoli wrote: > > > Instead of using dsi->out_bridge during the bridge search process, use a > > > temporary variable and assign dsi->out_bridge only on successful > > > completion. > > > > > > The main goal is to be able to drm_bridge_get() the out_bridge before > > > setting it in dsi->out_bridge, which is done in a later commit. Setting > > > dsi->out_bridge as in current code would leave a use-after-free window in > > > case the bridge is deallocated by some other thread between > > > 'dsi->out_bridge = devm_drm_panel_bridge_add()' and drm_bridge_get(). > > > > I don't think that's how refcounting should work. Any of the functions > > that give you the bridge should also increase refcount, requiring manual > > _put() call afterwards. We might need a separate API for that. > > You're perfectly right. > > > > This change additionally avoids leaving an ERR_PTR value in dsi->out_bridge > > > on failure. This is not necessarily a problem but it is not clean. > > > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > > > > > --- > > > > > > This patch was added in v5. > > > --- > > > drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++------ > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > [...] > > > @@ -1740,21 +1741,23 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host, > > > > > > panel = of_drm_find_panel(remote); > > > if (!IS_ERR(panel)) { > > > - dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel); > > > + out_bridge = devm_drm_panel_bridge_add(dev, panel); > > > } else { > > > - dsi->out_bridge = of_drm_find_bridge(remote); > > > - if (!dsi->out_bridge) > > > - dsi->out_bridge = ERR_PTR(-EINVAL); > > > + out_bridge = of_drm_find_bridge(remote); > > > + if (!out_bridge) > > > + out_bridge = ERR_PTR(-EINVAL); > > > } > > > > While looking at this patch, I think we should migrate the driver to > > drm_of_find_panel_or_bridge(). > > Indeed, the code here is duplicating drm_of_find_panel_or_bridge(). I'm > going to convert it. > > > Then your patch might add a function > > close to devm_drm_of_get_bridge() or drmm_of_get_bridge(). > > ...which would return a bridge pointer, with refcount already > incremented. Sure, except I think it should _not_ be a drmm, as > the bridge might itself disappear while the card keeps existing. Feel free to add new one. > > Luca > > -- > Luca Ceresoli, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c index f8b4fb8357659018ec0db65374ee5d05330639ae..c4d1563fd32019efde523dfc0863be044c05a826 100644 --- a/drivers/gpu/drm/bridge/samsung-dsim.c +++ b/drivers/gpu/drm/bridge/samsung-dsim.c @@ -1705,6 +1705,7 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host, struct device *dev = dsi->dev; struct device_node *np = dev->of_node; struct device_node *remote; + struct drm_bridge *out_bridge; struct drm_panel *panel; int ret; @@ -1740,21 +1741,23 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host, panel = of_drm_find_panel(remote); if (!IS_ERR(panel)) { - dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel); + out_bridge = devm_drm_panel_bridge_add(dev, panel); } else { - dsi->out_bridge = of_drm_find_bridge(remote); - if (!dsi->out_bridge) - dsi->out_bridge = ERR_PTR(-EINVAL); + out_bridge = of_drm_find_bridge(remote); + if (!out_bridge) + out_bridge = ERR_PTR(-EINVAL); } of_node_put(remote); - if (IS_ERR(dsi->out_bridge)) { - ret = PTR_ERR(dsi->out_bridge); + if (IS_ERR(out_bridge)) { + ret = PTR_ERR(out_bridge); DRM_DEV_ERROR(dev, "failed to find the bridge: %d\n", ret); return ret; } + dsi->out_bridge = out_bridge; + DRM_DEV_INFO(dev, "Attached %s device (lanes:%d bpp:%d mode-flags:0x%lx)\n", device->name, device->lanes, mipi_dsi_pixel_format_to_bpp(device->format),
Instead of using dsi->out_bridge during the bridge search process, use a temporary variable and assign dsi->out_bridge only on successful completion. The main goal is to be able to drm_bridge_get() the out_bridge before setting it in dsi->out_bridge, which is done in a later commit. Setting dsi->out_bridge as in current code would leave a use-after-free window in case the bridge is deallocated by some other thread between 'dsi->out_bridge = devm_drm_panel_bridge_add()' and drm_bridge_get(). This change additionally avoids leaving an ERR_PTR value in dsi->out_bridge on failure. This is not necessarily a problem but it is not clean. Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- This patch was added in v5. --- drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)