Thanks Dan.
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Sent: Sunday, November 29, 2020 11:48 PM
> To: Chrisanthus, Anitha <anitha.chrisanthus@xxxxxxxxx>
> Cc: Dea, Edmund J <edmund.j.dea@xxxxxxxxx>; David Airlie <airlied@xxxxxxxx>;
> Daniel Vetter <daniel@xxxxxxxx>; Sam Ravnborg <sam@xxxxxxxxxxxx>; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx; kernel-janitors@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> > > Sent: Friday, November 20, 2020 12:11 AM
> > > To: Chrisanthus, Anitha <anitha.chrisanthus@xxxxxxxxx>
> > > Cc: Dea, Edmund J <edmund.j.dea@xxxxxxxxx>; David Airlie
> <airlied@xxxxxxxx>;
> > > Daniel Vetter <daniel@xxxxxxxx>; Sam Ravnborg <sam@xxxxxxxxxxxx>; dri-
> > > devel@xxxxxxxxxxxxxxxxxxxxx; kernel-janitors@xxxxxxxxxxxxxxx
> > > 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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
