| Message ID | 20180503164009.14395-6-boris.brezillon@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote: > Having a device with a status property != "okay" in the DT is a valid > use case, and we should not prevent the registration of the DRM device > when the DSI device connected to the DSI controller is disabled. > > Consider the ENODEV return code as a valid result and do not expose the > DSI encoder/connector when it happens. > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c > index 8aa897835118..db2f137f8b7b 100644 > --- a/drivers/gpu/drm/vc4/vc4_dsi.c > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c > @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) > > ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, > &panel, &dsi->bridge); > - if (ret) > + if (ret) { > + /* If the bridge or panel pointed by dev->of_node is not > + * enabled, just return 0 here so that we don't prevent the DRM > + * dev from being registered. Of course that means the DSI > + * encoder won't be exposed, but that's not a problem since > + * nothing is connected to it. > + */ > + if (ret == -ENODEV) > + return 0; > + > return ret; > + } > > if (panel) { > dsi->bridge = devm_drm_panel_bridge_add(dev, panel, > @@ -1652,7 +1662,8 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master, > struct vc4_dev *vc4 = to_vc4_dev(drm); > struct vc4_dsi *dsi = dev_get_drvdata(dev); > > - pm_runtime_disable(dev); > + if (dsi->bridge) > + pm_runtime_disable(dev); Is this safe? This uses component/master, so dsi->bridge is going to remain valid until the driver's ->remove() is called. So technically you could have a situation where drm_of_find_panel_or_bridge() returned some error code that remains stored in dsi->bridge and cause the above condition to be incorrectly true. Thierry
On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote: > Having a device with a status property != "okay" in the DT is a valid > use case, and we should not prevent the registration of the DRM device > when the DSI device connected to the DSI controller is disabled. > > Consider the ENODEV return code as a valid result and do not expose the > DSI encoder/connector when it happens. > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c > index 8aa897835118..db2f137f8b7b 100644 > --- a/drivers/gpu/drm/vc4/vc4_dsi.c > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c > @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) > > ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, > &panel, &dsi->bridge); > - if (ret) > + if (ret) { > + /* If the bridge or panel pointed by dev->of_node is not > + * enabled, just return 0 here so that we don't prevent the DRM > + * dev from being registered. Of course that means the DSI > + * encoder won't be exposed, but that's not a problem since > + * nothing is connected to it. > + */ Also, nit: this isn't the correct style for block comments. Thierry
On Fri, 4 May 2018 12:30:25 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote: > > Having a device with a status property != "okay" in the DT is a valid > > use case, and we should not prevent the registration of the DRM device > > when the DSI device connected to the DSI controller is disabled. > > > > Consider the ENODEV return code as a valid result and do not expose the > > DSI encoder/connector when it happens. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > --- > > drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c > > index 8aa897835118..db2f137f8b7b 100644 > > --- a/drivers/gpu/drm/vc4/vc4_dsi.c > > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c > > @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) > > > > ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, > > &panel, &dsi->bridge); > > - if (ret) > > + if (ret) { > > + /* If the bridge or panel pointed by dev->of_node is not > > + * enabled, just return 0 here so that we don't prevent the DRM > > + * dev from being registered. Of course that means the DSI > > + * encoder won't be exposed, but that's not a problem since > > + * nothing is connected to it. > > + */ > > Also, nit: this isn't the correct style for block comments. Just trying to keep it consistent with the rest of the file.
On Fri, 4 May 2018 12:28:33 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote: > > Having a device with a status property != "okay" in the DT is a valid > > use case, and we should not prevent the registration of the DRM device > > when the DSI device connected to the DSI controller is disabled. > > > > Consider the ENODEV return code as a valid result and do not expose the > > DSI encoder/connector when it happens. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > --- > > drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c > > index 8aa897835118..db2f137f8b7b 100644 > > --- a/drivers/gpu/drm/vc4/vc4_dsi.c > > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c > > @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) > > > > ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, > > &panel, &dsi->bridge); > > - if (ret) > > + if (ret) { > > + /* If the bridge or panel pointed by dev->of_node is not > > + * enabled, just return 0 here so that we don't prevent the DRM > > + * dev from being registered. Of course that means the DSI > > + * encoder won't be exposed, but that's not a problem since > > + * nothing is connected to it. > > + */ > > + if (ret == -ENODEV) > > + return 0; > > + > > return ret; > > + } > > > > if (panel) { > > dsi->bridge = devm_drm_panel_bridge_add(dev, panel, > > @@ -1652,7 +1662,8 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master, > > struct vc4_dev *vc4 = to_vc4_dev(drm); > > struct vc4_dsi *dsi = dev_get_drvdata(dev); > > > > - pm_runtime_disable(dev); > > + if (dsi->bridge) > > + pm_runtime_disable(dev); > > Is this safe? This uses component/master, so dsi->bridge is going to > remain valid until the driver's ->remove() is called. So technically you > could have a situation where drm_of_find_panel_or_bridge() returned some > error code that remains stored in dsi->bridge and cause the above > condition to be incorrectly true. No, because of_drm_find_bridge() (which is called from drm_of_find_panel_or_bridge() returns either NULL or a valid bridge pointer), so dsi->bridge either points to a valid bridge object or is NULL. Am I missing something?
On Fri, May 04, 2018 at 02:05:25PM +0200, Boris Brezillon wrote: > On Fri, 4 May 2018 12:28:33 +0200 > Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote: > > > Having a device with a status property != "okay" in the DT is a valid > > > use case, and we should not prevent the registration of the DRM device > > > when the DSI device connected to the DSI controller is disabled. > > > > > > Consider the ENODEV return code as a valid result and do not expose the > > > DSI encoder/connector when it happens. > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > --- > > > drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++-- > > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c > > > index 8aa897835118..db2f137f8b7b 100644 > > > --- a/drivers/gpu/drm/vc4/vc4_dsi.c > > > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c > > > @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) > > > > > > ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, > > > &panel, &dsi->bridge); > > > - if (ret) > > > + if (ret) { > > > + /* If the bridge or panel pointed by dev->of_node is not > > > + * enabled, just return 0 here so that we don't prevent the DRM > > > + * dev from being registered. Of course that means the DSI > > > + * encoder won't be exposed, but that's not a problem since > > > + * nothing is connected to it. > > > + */ > > > + if (ret == -ENODEV) > > > + return 0; > > > + > > > return ret; > > > + } > > > > > > if (panel) { > > > dsi->bridge = devm_drm_panel_bridge_add(dev, panel, > > > @@ -1652,7 +1662,8 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master, > > > struct vc4_dev *vc4 = to_vc4_dev(drm); > > > struct vc4_dsi *dsi = dev_get_drvdata(dev); > > > > > > - pm_runtime_disable(dev); > > > + if (dsi->bridge) > > > + pm_runtime_disable(dev); > > > > Is this safe? This uses component/master, so dsi->bridge is going to > > remain valid until the driver's ->remove() is called. So technically you > > could have a situation where drm_of_find_panel_or_bridge() returned some > > error code that remains stored in dsi->bridge and cause the above > > condition to be incorrectly true. > > No, because of_drm_find_bridge() (which is called from > drm_of_find_panel_or_bridge() returns either NULL or a valid bridge > pointer), so dsi->bridge either points to a valid bridge object or is > NULL. Am I missing something? The return value of devm_drm_panel_bridge_add() is also assigned to dsi->bridge later on (in the "if (panel)" conditional). Thierry
On Fri, 4 May 2018 15:29:15 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Fri, May 04, 2018 at 02:05:25PM +0200, Boris Brezillon wrote: > > On Fri, 4 May 2018 12:28:33 +0200 > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote: > > > > Having a device with a status property != "okay" in the DT is a valid > > > > use case, and we should not prevent the registration of the DRM device > > > > when the DSI device connected to the DSI controller is disabled. > > > > > > > > Consider the ENODEV return code as a valid result and do not expose the > > > > DSI encoder/connector when it happens. > > > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > > --- > > > > drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++-- > > > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c > > > > index 8aa897835118..db2f137f8b7b 100644 > > > > --- a/drivers/gpu/drm/vc4/vc4_dsi.c > > > > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c > > > > @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) > > > > > > > > ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, > > > > &panel, &dsi->bridge); > > > > - if (ret) > > > > + if (ret) { > > > > + /* If the bridge or panel pointed by dev->of_node is not > > > > + * enabled, just return 0 here so that we don't prevent the DRM > > > > + * dev from being registered. Of course that means the DSI > > > > + * encoder won't be exposed, but that's not a problem since > > > > + * nothing is connected to it. > > > > + */ > > > > + if (ret == -ENODEV) > > > > + return 0; > > > > + > > > > return ret; > > > > + } > > > > > > > > if (panel) { > > > > dsi->bridge = devm_drm_panel_bridge_add(dev, panel, > > > > @@ -1652,7 +1662,8 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master, > > > > struct vc4_dev *vc4 = to_vc4_dev(drm); > > > > struct vc4_dsi *dsi = dev_get_drvdata(dev); > > > > > > > > - pm_runtime_disable(dev); > > > > + if (dsi->bridge) > > > > + pm_runtime_disable(dev); > > > > > > Is this safe? This uses component/master, so dsi->bridge is going to > > > remain valid until the driver's ->remove() is called. So technically you > > > could have a situation where drm_of_find_panel_or_bridge() returned some > > > error code that remains stored in dsi->bridge and cause the above > > > condition to be incorrectly true. > > > > No, because of_drm_find_bridge() (which is called from > > drm_of_find_panel_or_bridge() returns either NULL or a valid bridge > > pointer), so dsi->bridge either points to a valid bridge object or is > > NULL. Am I missing something? > > The return value of devm_drm_panel_bridge_add() is also assigned to > dsi->bridge later on (in the "if (panel)" conditional). But then we return an error code if IS_ERR(dsi->bridge) [1], which should prevent the unbind function from being called, right? [1]https://elixir.bootlin.com/linux/v4.17-rc3/source/drivers/gpu/drm/vc4/vc4_dsi.c#L1610
On Fri, May 04, 2018 at 03:49:06PM +0200, Boris Brezillon wrote: > On Fri, 4 May 2018 15:29:15 +0200 > Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Fri, May 04, 2018 at 02:05:25PM +0200, Boris Brezillon wrote: > > > On Fri, 4 May 2018 12:28:33 +0200 > > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote: > > > > > Having a device with a status property != "okay" in the DT is a valid > > > > > use case, and we should not prevent the registration of the DRM device > > > > > when the DSI device connected to the DSI controller is disabled. > > > > > > > > > > Consider the ENODEV return code as a valid result and do not expose the > > > > > DSI encoder/connector when it happens. > > > > > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > > > --- > > > > > drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++-- > > > > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c > > > > > index 8aa897835118..db2f137f8b7b 100644 > > > > > --- a/drivers/gpu/drm/vc4/vc4_dsi.c > > > > > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c > > > > > @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) > > > > > > > > > > ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, > > > > > &panel, &dsi->bridge); > > > > > - if (ret) > > > > > + if (ret) { > > > > > + /* If the bridge or panel pointed by dev->of_node is not > > > > > + * enabled, just return 0 here so that we don't prevent the DRM > > > > > + * dev from being registered. Of course that means the DSI > > > > > + * encoder won't be exposed, but that's not a problem since > > > > > + * nothing is connected to it. > > > > > + */ > > > > > + if (ret == -ENODEV) > > > > > + return 0; > > > > > + > > > > > return ret; > > > > > + } > > > > > > > > > > if (panel) { > > > > > dsi->bridge = devm_drm_panel_bridge_add(dev, panel, > > > > > @@ -1652,7 +1662,8 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master, > > > > > struct vc4_dev *vc4 = to_vc4_dev(drm); > > > > > struct vc4_dsi *dsi = dev_get_drvdata(dev); > > > > > > > > > > - pm_runtime_disable(dev); > > > > > + if (dsi->bridge) > > > > > + pm_runtime_disable(dev); > > > > > > > > Is this safe? This uses component/master, so dsi->bridge is going to > > > > remain valid until the driver's ->remove() is called. So technically you > > > > could have a situation where drm_of_find_panel_or_bridge() returned some > > > > error code that remains stored in dsi->bridge and cause the above > > > > condition to be incorrectly true. > > > > > > No, because of_drm_find_bridge() (which is called from > > > drm_of_find_panel_or_bridge() returns either NULL or a valid bridge > > > pointer), so dsi->bridge either points to a valid bridge object or is > > > NULL. Am I missing something? > > > > The return value of devm_drm_panel_bridge_add() is also assigned to > > dsi->bridge later on (in the "if (panel)" conditional). > > But then we return an error code if IS_ERR(dsi->bridge) [1], which > should prevent the unbind function from being called, right? Right, that should work. Seems safe, then. Thierry
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c index 8aa897835118..db2f137f8b7b 100644 --- a/drivers/gpu/drm/vc4/vc4_dsi.c +++ b/drivers/gpu/drm/vc4/vc4_dsi.c @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, &dsi->bridge); - if (ret) + if (ret) { + /* If the bridge or panel pointed by dev->of_node is not + * enabled, just return 0 here so that we don't prevent the DRM + * dev from being registered. Of course that means the DSI + * encoder won't be exposed, but that's not a problem since + * nothing is connected to it. + */ + if (ret == -ENODEV) + return 0; + return ret; + } if (panel) { dsi->bridge = devm_drm_panel_bridge_add(dev, panel, @@ -1652,7 +1662,8 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master, struct vc4_dev *vc4 = to_vc4_dev(drm); struct vc4_dsi *dsi = dev_get_drvdata(dev); - pm_runtime_disable(dev); + if (dsi->bridge) + pm_runtime_disable(dev); vc4_dsi_encoder_destroy(dsi->encoder);
Having a device with a status property != "okay" in the DT is a valid use case, and we should not prevent the registration of the DRM device when the DSI device connected to the DSI controller is disabled. Consider the ENODEV return code as a valid result and do not expose the DSI encoder/connector when it happens. Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> --- drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)