| Message ID | 20190424142148.25927-1-matt.redfearn@thinci.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge/synopsys: dsi: Don't blindly call post_disable | expand |
Hi Matt, Thank you for the patch. On Wed, Apr 24, 2019 at 02:22:00PM +0000, Matt Redfearn wrote: > The DRM documentation states that post_disable is an optional callback. > As such an implementing device may not populate it. To avoid panicing > the kernel by calling a NULL function pointer, we should NULL check it > before blindy calling it. > > Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > index 38e88071363..0ee440216b8 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > @@ -805,7 +805,8 @@ static void dw_mipi_dsi_bridge_post_disable(struct drm_bridge *bridge) > * This needs to be fixed in the drm_bridge framework and the API > * needs to be updated to manage our own call chains... > */ > - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > + if (dsi->panel_bridge->funcs->post_disable) > + dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > > if (dsi->slave) { > dw_mipi_dsi_disable(dsi->slave);
On 24.04.2019 16:22, Matt Redfearn wrote: > The DRM documentation states that post_disable is an optional callback. > As such an implementing device may not populate it. To avoid panicing > the kernel by calling a NULL function pointer, we should NULL check it > before blindy calling it. > > Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com> > --- > > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > index 38e88071363..0ee440216b8 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > @@ -805,7 +805,8 @@ static void dw_mipi_dsi_bridge_post_disable(struct drm_bridge *bridge) > * This needs to be fixed in the drm_bridge framework and the API > * needs to be updated to manage our own call chains... > */ > - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > + if (dsi->panel_bridge->funcs->post_disable) > + dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > Why not drm_bridge_post_disable ? Regards Andrzej > if (dsi->slave) { > dw_mipi_dsi_disable(dsi->slave);
Hi Andrzej, On 25/04/2019 13:13, Andrzej Hajda wrote: > On 24.04.2019 16:22, Matt Redfearn wrote: >> The DRM documentation states that post_disable is an optional callback. >> As such an implementing device may not populate it. To avoid panicing >> the kernel by calling a NULL function pointer, we should NULL check it >> before blindy calling it. >> >> Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com> > >> --- >> >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> index 38e88071363..0ee440216b8 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >> @@ -805,7 +805,8 @@ static void dw_mipi_dsi_bridge_post_disable(struct drm_bridge *bridge) >> * This needs to be fixed in the drm_bridge framework and the API >> * needs to be updated to manage our own call chains... >> */ >> - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); >> + if (dsi->panel_bridge->funcs->post_disable) >> + dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); >> > > Why not drm_bridge_post_disable ? Ah - that seems like a nicer fix! Do you think the comment above describing why this function pointer is called directly can be removed as well if we go this route? If someone calls drm_bridge_post_disable() on the Synposys DSI drm_bridge it will go on to call post_disable on all other bridges in the chain, in addition to us calling them here. Is it an issue to call it multiple times? Thanks, Matt > > > Regards > > Andrzej > > >> if (dsi->slave) { >> dw_mipi_dsi_disable(dsi->slave); > >
Hi Matt, On Thu, Apr 25, 2019 at 12:39:27PM +0000, Matt Redfearn wrote: > On 25/04/2019 13:13, Andrzej Hajda wrote: > > On 24.04.2019 16:22, Matt Redfearn wrote: > >> The DRM documentation states that post_disable is an optional callback. > >> As such an implementing device may not populate it. To avoid panicing > >> the kernel by calling a NULL function pointer, we should NULL check it > >> before blindy calling it. > >> > >> Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com> > > > >> --- > >> > >> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >> index 38e88071363..0ee440216b8 100644 > >> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >> @@ -805,7 +805,8 @@ static void dw_mipi_dsi_bridge_post_disable(struct drm_bridge *bridge) > >> * This needs to be fixed in the drm_bridge framework and the API > >> * needs to be updated to manage our own call chains... > >> */ > >> - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > >> + if (dsi->panel_bridge->funcs->post_disable) > >> + dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > >> > > > > Why not drm_bridge_post_disable ? > > Ah - that seems like a nicer fix! Do you think the comment above > describing why this function pointer is called directly can be removed > as well if we go this route? It shouldn't be necessary to call ->post_disable manually here as the bridge core handles it internally. This is a hack to work around a problem, and should be fixed properly. > If someone calls drm_bridge_post_disable() on the Synposys DSI > drm_bridge it will go on to call post_disable on all other bridges in > the chain, in addition to us calling them here. Is it an issue to call > it multiple times? It depends on the panel implementation, but in general it's not a good idea. It may happen to work, but could break at any time in the future. > >> if (dsi->slave) { > >> dw_mipi_dsi_disable(dsi->slave);
On Thu, Apr 25, 2019 at 08:59:15PM +0300, Laurent Pinchart wrote: > On Thu, Apr 25, 2019 at 12:39:27PM +0000, Matt Redfearn wrote: > > On 25/04/2019 13:13, Andrzej Hajda wrote: > >> On 24.04.2019 16:22, Matt Redfearn wrote: > >>> The DRM documentation states that post_disable is an optional callback. > >>> As such an implementing device may not populate it. To avoid panicing > >>> the kernel by calling a NULL function pointer, we should NULL check it > >>> before blindy calling it. > >>> > >>> Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com> > >> > >>> --- > >>> > >>> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >>> index 38e88071363..0ee440216b8 100644 > >>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c > >>> @@ -805,7 +805,8 @@ static void dw_mipi_dsi_bridge_post_disable(struct drm_bridge *bridge) > >>> * This needs to be fixed in the drm_bridge framework and the API > >>> * needs to be updated to manage our own call chains... > >>> */ > >>> - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > >>> + if (dsi->panel_bridge->funcs->post_disable) > >>> + dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); > >>> > >> > >> Why not drm_bridge_post_disable ? > > > > Ah - that seems like a nicer fix! Do you think the comment above > > describing why this function pointer is called directly can be removed > > as well if we go this route? > > It shouldn't be necessary to call ->post_disable manually here as the > bridge core handles it internally. This is a hack to work around a > problem, and should be fixed properly. > > > If someone calls drm_bridge_post_disable() on the Synposys DSI > > drm_bridge it will go on to call post_disable on all other bridges in > > the chain, in addition to us calling them here. Is it an issue to call > > it multiple times? > > It depends on the panel implementation, but in general it's not a good > idea. It may happen to work, but could break at any time in the future. Double-checking the driver, the .attach() operation doesn't propagate to the next bridge, so the bridge core will not know about it, and will not propagate .post_disable() either. I think this should be fixed in a way that uses the drm bridge core infrastructure. > >>> if (dsi->slave) { > >>> dw_mipi_dsi_disable(dsi->slave);
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index 38e88071363..0ee440216b8 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -805,7 +805,8 @@ static void dw_mipi_dsi_bridge_post_disable(struct drm_bridge *bridge) * This needs to be fixed in the drm_bridge framework and the API * needs to be updated to manage our own call chains... */ - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); + if (dsi->panel_bridge->funcs->post_disable) + dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); if (dsi->slave) { dw_mipi_dsi_disable(dsi->slave);
The DRM documentation states that post_disable is an optional callback. As such an implementing device may not populate it. To avoid panicing the kernel by calling a NULL function pointer, we should NULL check it before blindy calling it. Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com> --- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)