* [PATCH] drm/kmb: Fix possible oops in probe error handling
@ 2020-11-17 7:21 Dan Carpenter
2020-11-20 1:29 ` Chrisanthus, Anitha
2020-11-20 8:11 ` [PATCH v2] " Dan Carpenter
0 siblings, 2 replies; 7+ messages in thread
From: Dan Carpenter @ 2020-11-17 7:21 UTC (permalink / raw)
To: Anitha Chrisanthus
Cc: David Airlie, Edmund Dea, kernel-janitors, dri-devel, Sam Ravnborg
If kmb_dsi_init() fails the error handling will dereference an error
pointer which will cause an Oops.
Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/gpu/drm/kmb/kmb_drv.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
index a31a840ce634..8c43b136765c 100644
--- a/drivers/gpu/drm/kmb/kmb_drv.c
+++ b/drivers/gpu/drm/kmb/kmb_drv.c
@@ -504,7 +504,7 @@ static int kmb_probe(struct platform_device *pdev)
if (IS_ERR(kmb->kmb_dsi)) {
drm_err(&kmb->drm, "failed to initialize DSI\n");
ret = PTR_ERR(kmb->kmb_dsi);
- goto err_free1;
+ goto err_clear_drvdata;
}
kmb->kmb_dsi->dev = &dsi_pdev->dev;
@@ -540,8 +540,9 @@ static int kmb_probe(struct platform_device *pdev)
drm_crtc_cleanup(&kmb->crtc);
drm_mode_config_cleanup(&kmb->drm);
err_free1:
- dev_set_drvdata(dev, NULL);
kmb_dsi_host_unregister(kmb->kmb_dsi);
+ err_clear_drvdata:
+ dev_set_drvdata(dev, NULL);
return ret;
}
--
2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH] drm/kmb: Fix possible oops in probe error handling
2020-11-17 7:21 [PATCH] drm/kmb: Fix possible oops in probe error handling Dan Carpenter
@ 2020-11-20 1:29 ` Chrisanthus, Anitha
2020-11-20 8:09 ` Dan Carpenter
2020-11-20 8:11 ` [PATCH v2] " Dan Carpenter
1 sibling, 1 reply; 7+ messages in thread
From: Chrisanthus, Anitha @ 2020-11-20 1:29 UTC (permalink / raw)
To: Dan Carpenter
Cc: David Airlie, Dea, Edmund J, kernel-janitors, dri-devel, Sam Ravnborg
Hi Dan,
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Monday, November 16, 2020 11:21 PM
> To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>
> Cc: Dea, Edmund J <edmund.j.dea@intel.com>; David Airlie <airlied@linux.ie>;
> Daniel Vetter <daniel@ffwll.ch>; Sam Ravnborg <sam@ravnborg.org>; dri-
> devel@lists.freedesktop.org; kernel-janitors@vger.kernel.org
> Subject: [PATCH] drm/kmb: Fix possible oops in probe error handling
>
> If kmb_dsi_init() fails the error handling will dereference an error
> pointer which will cause an Oops.
>
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> drivers/gpu/drm/kmb/kmb_drv.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> index a31a840ce634..8c43b136765c 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -504,7 +504,7 @@ static int kmb_probe(struct platform_device *pdev)
> if (IS_ERR(kmb->kmb_dsi)) {
> drm_err(&kmb->drm, "failed to initialize DSI\n");
> ret = PTR_ERR(kmb->kmb_dsi);
> - goto err_free1;
> + goto err_clear_drvdata;
> }
dsi host is registered earlier, it needs to be unregistered, original code is correct.
Anitha
>
> kmb->kmb_dsi->dev = &dsi_pdev->dev;
> @@ -540,8 +540,9 @@ static int kmb_probe(struct platform_device *pdev)
> drm_crtc_cleanup(&kmb->crtc);
> drm_mode_config_cleanup(&kmb->drm);
> err_free1:
> - dev_set_drvdata(dev, NULL);
> kmb_dsi_host_unregister(kmb->kmb_dsi);
> + err_clear_drvdata:
> + dev_set_drvdata(dev, NULL);
>
> return ret;
> }
> --
> 2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/kmb: Fix possible oops in probe error handling
2020-11-20 1:29 ` Chrisanthus, Anitha
@ 2020-11-20 8:09 ` Dan Carpenter
0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2020-11-20 8:09 UTC (permalink / raw)
To: Chrisanthus, Anitha
Cc: David Airlie, Dea, Edmund J, kernel-janitors, dri-devel, Sam Ravnborg
On Fri, Nov 20, 2020 at 01:29:45AM +0000, Chrisanthus, Anitha wrote:
> Hi Dan,
>
> > -----Original Message-----
> > From: Dan Carpenter <dan.carpenter@oracle.com>
> > Sent: Monday, November 16, 2020 11:21 PM
> > To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>
> > Cc: Dea, Edmund J <edmund.j.dea@intel.com>; David Airlie <airlied@linux.ie>;
> > Daniel Vetter <daniel@ffwll.ch>; Sam Ravnborg <sam@ravnborg.org>; dri-
> > devel@lists.freedesktop.org; kernel-janitors@vger.kernel.org
> > Subject: [PATCH] drm/kmb: Fix possible oops in probe error handling
> >
> > If kmb_dsi_init() fails the error handling will dereference an error
> > pointer which will cause an Oops.
> >
> > Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > drivers/gpu/drm/kmb/kmb_drv.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> > b/drivers/gpu/drm/kmb/kmb_drv.c
> > index a31a840ce634..8c43b136765c 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > @@ -504,7 +504,7 @@ static int kmb_probe(struct platform_device *pdev)
> > if (IS_ERR(kmb->kmb_dsi)) {
^^^^^^^^^^^^
We check that this is an error pointer.
> > drm_err(&kmb->drm, "failed to initialize DSI\n");
> > ret = PTR_ERR(kmb->kmb_dsi);
> > - goto err_free1;
^^^^^^^^^^^^^^^
We go to err_free1.
> > + goto err_clear_drvdata;
> > }
> dsi host is registered earlier, it needs to be unregistered, original code is correct.
>
> Anitha
> >
> > kmb->kmb_dsi->dev = &dsi_pdev->dev;
> > @@ -540,8 +540,9 @@ static int kmb_probe(struct platform_device *pdev)
> > drm_crtc_cleanup(&kmb->crtc);
> > drm_mode_config_cleanup(&kmb->drm);
> > err_free1:
^^^^^^^^^
Which is here.
> > - dev_set_drvdata(dev, NULL);
> > kmb_dsi_host_unregister(kmb->kmb_dsi);
^^^^^^^^^^^^
And then we crash.
Probably my commit message was not clear enough. I will change the
commit message and resend.
regards,
dan carpenter
> > + err_clear_drvdata:
> > + dev_set_drvdata(dev, NULL);
> >
> > return ret;
> > }
> > --
> > 2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] drm/kmb: Fix possible oops in probe error handling
2020-11-17 7:21 [PATCH] drm/kmb: Fix possible oops in probe error handling Dan Carpenter
2020-11-20 1:29 ` Chrisanthus, Anitha
@ 2020-11-20 8:11 ` Dan Carpenter
2020-11-20 22:15 ` Chrisanthus, Anitha
1 sibling, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2020-11-20 8:11 UTC (permalink / raw)
To: Anitha Chrisanthus
Cc: David Airlie, Edmund Dea, kernel-janitors, dri-devel, Sam Ravnborg
If kmb_dsi_init() fails the "kmb->kmb_dsi" variable is an error pointer.
The kernel will Oops when we pass it to kmb_dsi_host_unregister().
Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: write a better commit message
drivers/gpu/drm/kmb/kmb_drv.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
index a31a840ce634..8c43b136765c 100644
--- a/drivers/gpu/drm/kmb/kmb_drv.c
+++ b/drivers/gpu/drm/kmb/kmb_drv.c
@@ -504,7 +504,7 @@ static int kmb_probe(struct platform_device *pdev)
if (IS_ERR(kmb->kmb_dsi)) {
drm_err(&kmb->drm, "failed to initialize DSI\n");
ret = PTR_ERR(kmb->kmb_dsi);
- goto err_free1;
+ goto err_clear_drvdata;
}
kmb->kmb_dsi->dev = &dsi_pdev->dev;
@@ -540,8 +540,9 @@ static int kmb_probe(struct platform_device *pdev)
drm_crtc_cleanup(&kmb->crtc);
drm_mode_config_cleanup(&kmb->drm);
err_free1:
- dev_set_drvdata(dev, NULL);
kmb_dsi_host_unregister(kmb->kmb_dsi);
+ err_clear_drvdata:
+ dev_set_drvdata(dev, NULL);
return ret;
}
--
2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH v2] drm/kmb: Fix possible oops in probe error handling
2020-11-20 8:11 ` [PATCH v2] " Dan Carpenter
@ 2020-11-20 22:15 ` Chrisanthus, Anitha
2020-11-30 7:48 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Chrisanthus, Anitha @ 2020-11-20 22:15 UTC (permalink / raw)
To: Dan Carpenter
Cc: David Airlie, Dea, Edmund J, kernel-janitors, dri-devel, Sam Ravnborg
Hi Dan,
I see the problem now, thanks for the patch.
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Friday, November 20, 2020 12:11 AM
> To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>
> Cc: Dea, Edmund J <edmund.j.dea@intel.com>; David Airlie <airlied@linux.ie>;
> Daniel Vetter <daniel@ffwll.ch>; Sam Ravnborg <sam@ravnborg.org>; dri-
> devel@lists.freedesktop.org; kernel-janitors@vger.kernel.org
> Subject: [PATCH v2] drm/kmb: Fix possible oops in probe error handling
>
> If kmb_dsi_init() fails the "kmb->kmb_dsi" variable is an error pointer.
> The kernel will Oops when we pass it to kmb_dsi_host_unregister().
>
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: write a better commit message
>
> drivers/gpu/drm/kmb/kmb_drv.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> b/drivers/gpu/drm/kmb/kmb_drv.c
> index a31a840ce634..8c43b136765c 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -504,7 +504,7 @@ static int kmb_probe(struct platform_device *pdev)
> if (IS_ERR(kmb->kmb_dsi)) {
> drm_err(&kmb->drm, "failed to initialize DSI\n");
> ret = PTR_ERR(kmb->kmb_dsi);
> - goto err_free1;
> + goto err_clear_drvdata;
> }
>
> kmb->kmb_dsi->dev = &dsi_pdev->dev;
> @@ -540,8 +540,9 @@ static int kmb_probe(struct platform_device *pdev)
> drm_crtc_cleanup(&kmb->crtc);
> drm_mode_config_cleanup(&kmb->drm);
> err_free1:
> - dev_set_drvdata(dev, NULL);
> kmb_dsi_host_unregister(kmb->kmb_dsi);
> + err_clear_drvdata:
We still need to unregister the dsi_host that was registered in this call kmb_dsi_host_bridge_init.
This will require more changes in kmb_dsi_host_unregister and/or separate out mipi_dsi_host_unregister.
FYI - I will be out all of next week, will be back the next Monday.
> + dev_set_drvdata(dev, NULL);
>
> return ret;
> }
> --
> 2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm/kmb: Fix possible oops in probe error handling
2020-11-20 22:15 ` Chrisanthus, Anitha
@ 2020-11-30 7:48 ` Dan Carpenter
2020-12-03 1:11 ` Chrisanthus, Anitha
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2020-11-30 7:48 UTC (permalink / raw)
To: Chrisanthus, Anitha
Cc: David Airlie, Dea, Edmund J, kernel-janitors, dri-devel, Sam Ravnborg
On Fri, Nov 20, 2020 at 10:15:57PM +0000, Chrisanthus, Anitha wrote:
> Hi Dan,
> I see the problem now, thanks for the patch.
>
> > -----Original Message-----
> > From: Dan Carpenter <dan.carpenter@oracle.com>
> > Sent: Friday, November 20, 2020 12:11 AM
> > To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>
> > Cc: Dea, Edmund J <edmund.j.dea@intel.com>; David Airlie <airlied@linux.ie>;
> > Daniel Vetter <daniel@ffwll.ch>; Sam Ravnborg <sam@ravnborg.org>; dri-
> > devel@lists.freedesktop.org; kernel-janitors@vger.kernel.org
> > Subject: [PATCH v2] drm/kmb: Fix possible oops in probe error handling
> >
> > If kmb_dsi_init() fails the "kmb->kmb_dsi" variable is an error pointer.
> > The kernel will Oops when we pass it to kmb_dsi_host_unregister().
> >
> > Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > v2: write a better commit message
> >
> > drivers/gpu/drm/kmb/kmb_drv.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> > b/drivers/gpu/drm/kmb/kmb_drv.c
> > index a31a840ce634..8c43b136765c 100644
> > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > @@ -504,7 +504,7 @@ static int kmb_probe(struct platform_device *pdev)
> > if (IS_ERR(kmb->kmb_dsi)) {
> > drm_err(&kmb->drm, "failed to initialize DSI\n");
> > ret = PTR_ERR(kmb->kmb_dsi);
> > - goto err_free1;
> > + goto err_clear_drvdata;
> > }
> >
> > kmb->kmb_dsi->dev = &dsi_pdev->dev;
> > @@ -540,8 +540,9 @@ static int kmb_probe(struct platform_device *pdev)
> > drm_crtc_cleanup(&kmb->crtc);
> > drm_mode_config_cleanup(&kmb->drm);
> > err_free1:
> > - dev_set_drvdata(dev, NULL);
> > kmb_dsi_host_unregister(kmb->kmb_dsi);
> > + err_clear_drvdata:
> We still need to unregister the dsi_host that was registered in this call kmb_dsi_host_bridge_init.
> This will require more changes in kmb_dsi_host_unregister and/or separate out mipi_dsi_host_unregister.
> FYI - I will be out all of next week, will be back the next Monday.
Hm... Yes. Now that you point it out, there are several bugs related
to kmb_dsi_host_bridge_init()...
182 void kmb_dsi_host_unregister(struct kmb_dsi *kmb_dsi)
183 {
184 kmb_dsi_clk_disable(kmb_dsi);
185 mipi_dsi_host_unregister(kmb_dsi->host);
^^^^^^^^^^^^^
kmb_dsi->host is dsi_host.
Every user unregisters it, but only the first user registers it. So
if there are multiple users it will be unregistered prematurely. Should
there be a kfree to prevent a leak?
kfree(kmb_dsi->host);
dsi_host = NULL;
186 }
[ snip ]
216 int kmb_dsi_host_bridge_init(struct device *dev)
217 {
218 struct device_node *encoder_node, *dsi_out;
219
220 /* Create and register MIPI DSI host */
221 if (!dsi_host) {
^^^^^^^^
This is only allocated for the first user.
222 dsi_host = kzalloc(sizeof(*dsi_host), GFP_KERNEL);
223 if (!dsi_host)
224 return -ENOMEM;
225
226 dsi_host->ops = &kmb_dsi_host_ops;
227
228 if (!dsi_device) {
229 dsi_device = kzalloc(sizeof(*dsi_device), GFP_KERNEL);
230 if (!dsi_device) {
231 kfree(dsi_host);
^^^^^^^^^^^^^^^
But now it is non-NULL but it is a freed pointer. dsi_host = NULL;
232 return -ENOMEM;
233 }
234 }
235
236 dsi_host->dev = dev;
237 mipi_dsi_host_register(dsi_host);
238 }
239
[ snip ]
482
483 of_node_put(dsi_in);
484 of_node_put(dsi_node);
485 ret = kmb_dsi_host_bridge_init(get_device(&dsi_pdev->dev));
^^^^^^^^^^^^^^^^^^^^^^^^^^
This get_device() needs a matching put_device(). I kind of like to put
the kref_get() calls on their own line so that they're more obvious to
the reader.
get_device(&dsi_pdev->dev);
kmb_dsi_host_bridge_init(&dsi_pdev->dev);
486
487 if (ret == -EPROBE_DEFER) {
488 return -EPROBE_DEFER;
489 } else if (ret) {
490 DRM_ERROR("probe failed to initialize DSI host bridge\n");
491 return ret;
492 }
493
494 /* Create DRM device */
495 kmb = devm_drm_dev_alloc(dev, &kmb_driver,
496 struct kmb_drm_private, drm);
497 if (IS_ERR(kmb))
498 return PTR_ERR(kmb);
On these error paths we would want to unwind using a call to
kmb_dsi_host_unregister().
499
500 dev_set_drvdata(dev, &kmb->drm);
501
502 /* Initialize MIPI DSI */
503 kmb->kmb_dsi = kmb_dsi_init(dsi_pdev);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is the call where the "kmb_dsi->host = dsi_host;" assignment
actually happens.
504 if (IS_ERR(kmb->kmb_dsi)) {
505 drm_err(&kmb->drm, "failed to initialize DSI\n");
506 ret = PTR_ERR(kmb->kmb_dsi);
507 goto err_free1;
508 }
509
510 kmb->kmb_dsi->dev = &dsi_pdev->dev;
511 kmb->kmb_dsi->pdev = dsi_pdev;
512 ret = kmb_hw_init(&kmb->drm, 0);
It feels like it would be a lot easier if the kmb_dsi_init() and
kmb_dsi_host_bridge_init() functions were combined. Probably the
dsi_host and dsi_device stuff needs to be refcounted?
Anyway, I can't test this stuff and I'm not really familiar with the
driver. Could you fix it and CC me on the fix?
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v2] drm/kmb: Fix possible oops in probe error handling
2020-11-30 7:48 ` Dan Carpenter
@ 2020-12-03 1:11 ` Chrisanthus, Anitha
0 siblings, 0 replies; 7+ messages in thread
From: Chrisanthus, Anitha @ 2020-12-03 1:11 UTC (permalink / raw)
To: Dan Carpenter
Cc: David Airlie, Dea, Edmund J, kernel-janitors, dri-devel, Sam Ravnborg
Thanks Dan.
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Sunday, November 29, 2020 11:48 PM
> To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>
> Cc: Dea, Edmund J <edmund.j.dea@intel.com>; David Airlie <airlied@linux.ie>;
> Daniel Vetter <daniel@ffwll.ch>; Sam Ravnborg <sam@ravnborg.org>; dri-
> devel@lists.freedesktop.org; kernel-janitors@vger.kernel.org
> Subject: Re: [PATCH v2] drm/kmb: Fix possible oops in probe error handling
>
> On Fri, Nov 20, 2020 at 10:15:57PM +0000, Chrisanthus, Anitha wrote:
> > Hi Dan,
> > I see the problem now, thanks for the patch.
> >
> > > -----Original Message-----
> > > From: Dan Carpenter <dan.carpenter@oracle.com>
> > > Sent: Friday, November 20, 2020 12:11 AM
> > > To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>
> > > Cc: Dea, Edmund J <edmund.j.dea@intel.com>; David Airlie
> <airlied@linux.ie>;
> > > Daniel Vetter <daniel@ffwll.ch>; Sam Ravnborg <sam@ravnborg.org>; dri-
> > > devel@lists.freedesktop.org; kernel-janitors@vger.kernel.org
> > > Subject: [PATCH v2] drm/kmb: Fix possible oops in probe error handling
> > >
> > > If kmb_dsi_init() fails the "kmb->kmb_dsi" variable is an error pointer.
> > > The kernel will Oops when we pass it to kmb_dsi_host_unregister().
> > >
> > > Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > > v2: write a better commit message
> > >
> > > drivers/gpu/drm/kmb/kmb_drv.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c
> > > b/drivers/gpu/drm/kmb/kmb_drv.c
> > > index a31a840ce634..8c43b136765c 100644
> > > --- a/drivers/gpu/drm/kmb/kmb_drv.c
> > > +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> > > @@ -504,7 +504,7 @@ static int kmb_probe(struct platform_device
> *pdev)
> > > if (IS_ERR(kmb->kmb_dsi)) {
> > > drm_err(&kmb->drm, "failed to initialize DSI\n");
> > > ret = PTR_ERR(kmb->kmb_dsi);
> > > - goto err_free1;
> > > + goto err_clear_drvdata;
> > > }
> > >
> > > kmb->kmb_dsi->dev = &dsi_pdev->dev;
> > > @@ -540,8 +540,9 @@ static int kmb_probe(struct platform_device
> *pdev)
> > > drm_crtc_cleanup(&kmb->crtc);
> > > drm_mode_config_cleanup(&kmb->drm);
> > > err_free1:
> > > - dev_set_drvdata(dev, NULL);
> > > kmb_dsi_host_unregister(kmb->kmb_dsi);
> > > + err_clear_drvdata:
> > We still need to unregister the dsi_host that was registered in this call
> kmb_dsi_host_bridge_init.
> > This will require more changes in kmb_dsi_host_unregister and/or separate
> out mipi_dsi_host_unregister.
> > FYI - I will be out all of next week, will be back the next Monday.
>
> Hm... Yes. Now that you point it out, there are several bugs related
> to kmb_dsi_host_bridge_init()...
>
> 182 void kmb_dsi_host_unregister(struct kmb_dsi *kmb_dsi)
> 183 {
> 184 kmb_dsi_clk_disable(kmb_dsi);
> 185 mipi_dsi_host_unregister(kmb_dsi->host);
> ^^^^^^^^^^^^^
> kmb_dsi->host is dsi_host.
>
> Every user unregisters it, but only the first user registers it. So
> if there are multiple users it will be unregistered prematurely. Should
> there be a kfree to prevent a leak?
>
> kfree(kmb_dsi->host);
> dsi_host = NULL;
>
> 186 }
>
> [ snip ]
>
> 216 int kmb_dsi_host_bridge_init(struct device *dev)
> 217 {
> 218 struct device_node *encoder_node, *dsi_out;
> 219
> 220 /* Create and register MIPI DSI host */
> 221 if (!dsi_host) {
> ^^^^^^^^
> This is only allocated for the first user.
>
> 222 dsi_host = kzalloc(sizeof(*dsi_host), GFP_KERNEL);
> 223 if (!dsi_host)
> 224 return -ENOMEM;
> 225
> 226 dsi_host->ops = &kmb_dsi_host_ops;
> 227
> 228 if (!dsi_device) {
> 229 dsi_device = kzalloc(sizeof(*dsi_device), GFP_KERNEL);
> 230 if (!dsi_device) {
> 231 kfree(dsi_host);
> ^^^^^^^^^^^^^^^
> But now it is non-NULL but it is a freed pointer. dsi_host = NULL;
>
> 232 return -ENOMEM;
> 233 }
> 234 }
> 235
> 236 dsi_host->dev = dev;
> 237 mipi_dsi_host_register(dsi_host);
> 238 }
> 239
>
> [ snip ]
>
> 482
> 483 of_node_put(dsi_in);
> 484 of_node_put(dsi_node);
> 485 ret = kmb_dsi_host_bridge_init(get_device(&dsi_pdev->dev));
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
> This get_device() needs a matching put_device(). I kind of like to put
> the kref_get() calls on their own line so that they're more obvious to
> the reader.
>
> get_device(&dsi_pdev->dev);
> kmb_dsi_host_bridge_init(&dsi_pdev->dev);
>
> 486
> 487 if (ret == -EPROBE_DEFER) {
> 488 return -EPROBE_DEFER;
> 489 } else if (ret) {
> 490 DRM_ERROR("probe failed to initialize DSI host bridge\n");
> 491 return ret;
> 492 }
> 493
> 494 /* Create DRM device */
> 495 kmb = devm_drm_dev_alloc(dev, &kmb_driver,
> 496 struct kmb_drm_private, drm);
> 497 if (IS_ERR(kmb))
> 498 return PTR_ERR(kmb);
>
> On these error paths we would want to unwind using a call to
> kmb_dsi_host_unregister().
>
> 499
> 500 dev_set_drvdata(dev, &kmb->drm);
> 501
> 502 /* Initialize MIPI DSI */
> 503 kmb->kmb_dsi = kmb_dsi_init(dsi_pdev);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This is the call where the "kmb_dsi->host = dsi_host;" assignment
> actually happens.
>
> 504 if (IS_ERR(kmb->kmb_dsi)) {
> 505 drm_err(&kmb->drm, "failed to initialize DSI\n");
> 506 ret = PTR_ERR(kmb->kmb_dsi);
> 507 goto err_free1;
> 508 }
> 509
> 510 kmb->kmb_dsi->dev = &dsi_pdev->dev;
> 511 kmb->kmb_dsi->pdev = dsi_pdev;
> 512 ret = kmb_hw_init(&kmb->drm, 0);
>
> It feels like it would be a lot easier if the kmb_dsi_init() and
> kmb_dsi_host_bridge_init() functions were combined. Probably the
> dsi_host and dsi_device stuff needs to be refcounted?
>
> Anyway, I can't test this stuff and I'm not really familiar with the
> driver. Could you fix it and CC me on the fix?
I will work on the fix later and will CC you, very busy with other stuff right now.
>
> regards,
> dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-12-03 1:11 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-17 7:21 [PATCH] drm/kmb: Fix possible oops in probe error handling Dan Carpenter
2020-11-20 1:29 ` Chrisanthus, Anitha
2020-11-20 8:09 ` Dan Carpenter
2020-11-20 8:11 ` [PATCH v2] " Dan Carpenter
2020-11-20 22:15 ` Chrisanthus, Anitha
2020-11-30 7:48 ` Dan Carpenter
2020-12-03 1:11 ` Chrisanthus, Anitha
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).