* [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable @ 2019-04-24 14:22 ` Matt Redfearn 2019-04-25 9:20 ` Laurent Pinchart 2019-04-25 12:13 ` Andrzej Hajda 0 siblings, 2 replies; 6+ messages in thread From: Matt Redfearn @ 2019-04-24 14:22 UTC (permalink / raw) To: Andrzej Hajda, Laurent Pinchart, Philippe Cornu Cc: dri-devel, Matthew Redfearn, Nickey Yang, Heiko Stuebner, Archit Taneja, linux-kernel, David Airlie, Daniel Vetter 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); if (dsi->slave) { dw_mipi_dsi_disable(dsi->slave); -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable 2019-04-24 14:22 ` [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable Matt Redfearn @ 2019-04-25 9:20 ` Laurent Pinchart 2019-04-25 12:13 ` Andrzej Hajda 1 sibling, 0 replies; 6+ messages in thread From: Laurent Pinchart @ 2019-04-25 9:20 UTC (permalink / raw) To: Matt Redfearn Cc: Andrzej Hajda, Philippe Cornu, dri-devel, Matthew Redfearn, Nickey Yang, Heiko Stuebner, Archit Taneja, linux-kernel, David Airlie, Daniel Vetter 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); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable 2019-04-24 14:22 ` [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable Matt Redfearn 2019-04-25 9:20 ` Laurent Pinchart @ 2019-04-25 12:13 ` Andrzej Hajda 2019-04-25 12:39 ` Matt Redfearn 1 sibling, 1 reply; 6+ messages in thread From: Andrzej Hajda @ 2019-04-25 12:13 UTC (permalink / raw) To: Matt Redfearn, Laurent Pinchart, Philippe Cornu Cc: dri-devel, Matthew Redfearn, Nickey Yang, Heiko Stuebner, Archit Taneja, linux-kernel, David Airlie, Daniel Vetter 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); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable 2019-04-25 12:13 ` Andrzej Hajda @ 2019-04-25 12:39 ` Matt Redfearn 2019-04-25 17:59 ` Laurent Pinchart 0 siblings, 1 reply; 6+ messages in thread From: Matt Redfearn @ 2019-04-25 12:39 UTC (permalink / raw) To: Andrzej Hajda, Laurent Pinchart, Philippe Cornu Cc: dri-devel, Matthew Redfearn, Nickey Yang, Heiko Stuebner, Archit Taneja, linux-kernel, David Airlie, Daniel Vetter 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); > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable 2019-04-25 12:39 ` Matt Redfearn @ 2019-04-25 17:59 ` Laurent Pinchart 2019-04-25 18:02 ` Laurent Pinchart 0 siblings, 1 reply; 6+ messages in thread From: Laurent Pinchart @ 2019-04-25 17:59 UTC (permalink / raw) To: Matt Redfearn Cc: Andrzej Hajda, Philippe Cornu, dri-devel, Matthew Redfearn, Nickey Yang, Heiko Stuebner, Archit Taneja, linux-kernel, David Airlie, Daniel Vetter 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); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable 2019-04-25 17:59 ` Laurent Pinchart @ 2019-04-25 18:02 ` Laurent Pinchart 0 siblings, 0 replies; 6+ messages in thread From: Laurent Pinchart @ 2019-04-25 18:02 UTC (permalink / raw) To: Matt Redfearn Cc: Andrzej Hajda, Philippe Cornu, dri-devel, Matthew Redfearn, Nickey Yang, Heiko Stuebner, Archit Taneja, linux-kernel, David Airlie, Daniel Vetter 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); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-25 18:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20190424142210epcas3p403bd5159852ab4e90eca1be67800ecde@epcas3p4.samsung.com>
2019-04-24 14:22 ` [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable Matt Redfearn
2019-04-25 9:20 ` Laurent Pinchart
2019-04-25 12:13 ` Andrzej Hajda
2019-04-25 12:39 ` Matt Redfearn
2019-04-25 17:59 ` Laurent Pinchart
2019-04-25 18:02 ` Laurent Pinchart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).