| Message ID | 20200221210319.2245170-38-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm managed resources, v2 | expand |
On Fri, Feb 21, 2020 at 10:04 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > It's (almost, there's some iommu stuff without significance) right > above the drm_dev_put(). > > This is made possible by a preceeding patch which added a drmm_ > cleanup action to drm_mode_config_init(), hence all we need to do to > ensure that drm_mode_config_cleanup() is run on final drm_device > cleanup is check the new error code for _init(). > > Aside: Another driver with a bit much devm_kzalloc, which should > probably use drmm_kzalloc instead ... > > v2: Explain why this cleanup is possible (Laurent). > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Sandy Huang <hjc@rock-chips.com> > Cc: "Heiko Stübner" <heiko@sntech.de> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-rockchip@lists.infradead.org > --- > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > index 20ecb1508a22..d0eba21eebc9 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > @@ -135,7 +135,9 @@ static int rockchip_drm_bind(struct device *dev) > if (ret) > goto err_free; > > - drm_mode_config_init(drm_dev); > + ret = drm_mode_config_init(drm_dev); > + if (ret) > + goto err_free; Shouldn't the goto label be err_mode_config_cleanup here? Otherwise this error path misses the call to rockchip_iommu_cleanup(). > > rockchip_drm_mode_config_init(drm_dev); > > @@ -174,11 +176,8 @@ static int rockchip_drm_bind(struct device *dev) > err_unbind_all: > component_unbind_all(dev, drm_dev); > err_mode_config_cleanup: > - drm_mode_config_cleanup(drm_dev); > rockchip_iommu_cleanup(drm_dev); > err_free: > - drm_dev->dev_private = NULL; > - dev_set_drvdata(dev, NULL); > drm_dev_put(drm_dev); > return ret; > } On Fri, Feb 21, 2020 at 10:04 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > It's (almost, there's some iommu stuff without significance) right > above the drm_dev_put(). > > This is made possible by a preceeding patch which added a drmm_ > cleanup action to drm_mode_config_init(), hence all we need to do to > ensure that drm_mode_config_cleanup() is run on final drm_device > cleanup is check the new error code for _init(). > > Aside: Another driver with a bit much devm_kzalloc, which should > probably use drmm_kzalloc instead ... > > v2: Explain why this cleanup is possible (Laurent). > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Sandy Huang <hjc@rock-chips.com> > Cc: "Heiko Stübner" <heiko@sntech.de> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-rockchip@lists.infradead.org > --- > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > index 20ecb1508a22..d0eba21eebc9 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > @@ -135,7 +135,9 @@ static int rockchip_drm_bind(struct device *dev) > if (ret) > goto err_free; > > - drm_mode_config_init(drm_dev); > + ret = drm_mode_config_init(drm_dev); > + if (ret) > + goto err_free; > > rockchip_drm_mode_config_init(drm_dev); > > @@ -174,11 +176,8 @@ static int rockchip_drm_bind(struct device *dev) > err_unbind_all: > component_unbind_all(dev, drm_dev); > err_mode_config_cleanup: > - drm_mode_config_cleanup(drm_dev); > rockchip_iommu_cleanup(drm_dev); > err_free: > - drm_dev->dev_private = NULL; > - dev_set_drvdata(dev, NULL); > drm_dev_put(drm_dev); > return ret; > } > @@ -194,11 +193,8 @@ static void rockchip_drm_unbind(struct device *dev) > > drm_atomic_helper_shutdown(drm_dev); > component_unbind_all(dev, drm_dev); > - drm_mode_config_cleanup(drm_dev); > rockchip_iommu_cleanup(drm_dev); > > - drm_dev->dev_private = NULL; > - dev_set_drvdata(dev, NULL); > drm_dev_put(drm_dev); > } > > -- > 2.24.1 > > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
On Mon, Feb 24, 2020 at 8:13 PM Francesco Lavra <francescolavra.fl@gmail.com> wrote: > > On Fri, Feb 21, 2020 at 10:04 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > It's (almost, there's some iommu stuff without significance) right > > above the drm_dev_put(). > > > > This is made possible by a preceeding patch which added a drmm_ > > cleanup action to drm_mode_config_init(), hence all we need to do to > > ensure that drm_mode_config_cleanup() is run on final drm_device > > cleanup is check the new error code for _init(). > > > > Aside: Another driver with a bit much devm_kzalloc, which should > > probably use drmm_kzalloc instead ... > > > > v2: Explain why this cleanup is possible (Laurent). > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Sandy Huang <hjc@rock-chips.com> > > Cc: "Heiko Stübner" <heiko@sntech.de> > > Cc: linux-arm-kernel@lists.infradead.org > > Cc: linux-rockchip@lists.infradead.org > > --- > > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 10 +++------- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > > index 20ecb1508a22..d0eba21eebc9 100644 > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > > @@ -135,7 +135,9 @@ static int rockchip_drm_bind(struct device *dev) > > if (ret) > > goto err_free; > > > > - drm_mode_config_init(drm_dev); > > + ret = drm_mode_config_init(drm_dev); > > + if (ret) > > + goto err_free; > > Shouldn't the goto label be err_mode_config_cleanup here? Otherwise > this error path misses the call to rockchip_iommu_cleanup(). Indeed. I'll also rename the label to have a more meaningful name while at it. -Daniel > > > > > rockchip_drm_mode_config_init(drm_dev); > > > > @@ -174,11 +176,8 @@ static int rockchip_drm_bind(struct device *dev) > > err_unbind_all: > > component_unbind_all(dev, drm_dev); > > err_mode_config_cleanup: > > - drm_mode_config_cleanup(drm_dev); > > rockchip_iommu_cleanup(drm_dev); > > err_free: > > - drm_dev->dev_private = NULL; > > - dev_set_drvdata(dev, NULL); > > drm_dev_put(drm_dev); > > return ret; > > } > > On Fri, Feb 21, 2020 at 10:04 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > It's (almost, there's some iommu stuff without significance) right > > above the drm_dev_put(). > > > > This is made possible by a preceeding patch which added a drmm_ > > cleanup action to drm_mode_config_init(), hence all we need to do to > > ensure that drm_mode_config_cleanup() is run on final drm_device > > cleanup is check the new error code for _init(). > > > > Aside: Another driver with a bit much devm_kzalloc, which should > > probably use drmm_kzalloc instead ... > > > > v2: Explain why this cleanup is possible (Laurent). > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Sandy Huang <hjc@rock-chips.com> > > Cc: "Heiko Stübner" <heiko@sntech.de> > > Cc: linux-arm-kernel@lists.infradead.org > > Cc: linux-rockchip@lists.infradead.org > > --- > > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 10 +++------- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > > index 20ecb1508a22..d0eba21eebc9 100644 > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > > @@ -135,7 +135,9 @@ static int rockchip_drm_bind(struct device *dev) > > if (ret) > > goto err_free; > > > > - drm_mode_config_init(drm_dev); > > + ret = drm_mode_config_init(drm_dev); > > + if (ret) > > + goto err_free; > > > > rockchip_drm_mode_config_init(drm_dev); > > > > @@ -174,11 +176,8 @@ static int rockchip_drm_bind(struct device *dev) > > err_unbind_all: > > component_unbind_all(dev, drm_dev); > > err_mode_config_cleanup: > > - drm_mode_config_cleanup(drm_dev); > > rockchip_iommu_cleanup(drm_dev); > > err_free: > > - drm_dev->dev_private = NULL; > > - dev_set_drvdata(dev, NULL); > > drm_dev_put(drm_dev); > > return ret; > > } > > @@ -194,11 +193,8 @@ static void rockchip_drm_unbind(struct device *dev) > > > > drm_atomic_helper_shutdown(drm_dev); > > component_unbind_all(dev, drm_dev); > > - drm_mode_config_cleanup(drm_dev); > > rockchip_iommu_cleanup(drm_dev); > > > > - drm_dev->dev_private = NULL; > > - dev_set_drvdata(dev, NULL); > > drm_dev_put(drm_dev); > > } > > > > -- > > 2.24.1 > > > > > > _______________________________________________ > > Linux-rockchip mailing list > > Linux-rockchip@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-rockchip
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index 20ecb1508a22..d0eba21eebc9 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -135,7 +135,9 @@ static int rockchip_drm_bind(struct device *dev) if (ret) goto err_free; - drm_mode_config_init(drm_dev); + ret = drm_mode_config_init(drm_dev); + if (ret) + goto err_free; rockchip_drm_mode_config_init(drm_dev); @@ -174,11 +176,8 @@ static int rockchip_drm_bind(struct device *dev) err_unbind_all: component_unbind_all(dev, drm_dev); err_mode_config_cleanup: - drm_mode_config_cleanup(drm_dev); rockchip_iommu_cleanup(drm_dev); err_free: - drm_dev->dev_private = NULL; - dev_set_drvdata(dev, NULL); drm_dev_put(drm_dev); return ret; } @@ -194,11 +193,8 @@ static void rockchip_drm_unbind(struct device *dev) drm_atomic_helper_shutdown(drm_dev); component_unbind_all(dev, drm_dev); - drm_mode_config_cleanup(drm_dev); rockchip_iommu_cleanup(drm_dev); - drm_dev->dev_private = NULL; - dev_set_drvdata(dev, NULL); drm_dev_put(drm_dev); }
It's (almost, there's some iommu stuff without significance) right above the drm_dev_put(). This is made possible by a preceeding patch which added a drmm_ cleanup action to drm_mode_config_init(), hence all we need to do to ensure that drm_mode_config_cleanup() is run on final drm_device cleanup is check the new error code for _init(). Aside: Another driver with a bit much devm_kzalloc, which should probably use drmm_kzalloc instead ... v2: Explain why this cleanup is possible (Laurent). Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Sandy Huang <hjc@rock-chips.com> Cc: "Heiko Stübner" <heiko@sntech.de> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-rockchip@lists.infradead.org --- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)