| Message ID | 20240607-bridge-hdmi-connector-v5-2-ab384e6021af@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm: make use of the HDMI connector infrastructure | expand |
Hi, On Fri, Jun 07, 2024 at 04:22:59PM GMT, Dmitry Baryshkov wrote: > Turn drm_bridge_connector to using drmm_kzalloc() and > drmm_connector_init() and drop the custom destroy function. The > drm_connector_unregister() and fwnode_handle_put() are already handled > by the drm_connector_cleanup() and so are safe to be dropped. > > Acked-by: Maxime Ripard <mripard@kernel.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------ > 1 file changed, 5 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c > index 982552c9f92c..e093fc8928dc 100644 > --- a/drivers/gpu/drm/drm_bridge_connector.c > +++ b/drivers/gpu/drm/drm_bridge_connector.c > @@ -15,6 +15,7 @@ > #include <drm/drm_connector.h> > #include <drm/drm_device.h> > #include <drm/drm_edid.h> > +#include <drm/drm_managed.h> > #include <drm/drm_modeset_helper_vtables.h> > #include <drm/drm_probe_helper.h> > > @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force) > return status; > } > > -static void drm_bridge_connector_destroy(struct drm_connector *connector) > -{ > - struct drm_bridge_connector *bridge_connector = > - to_drm_bridge_connector(connector); > - > - drm_connector_unregister(connector); > - drm_connector_cleanup(connector); > - > - fwnode_handle_put(connector->fwnode); > - > - kfree(bridge_connector); > -} > - > static void drm_bridge_connector_debugfs_init(struct drm_connector *connector, > struct dentry *root) > { > @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = { > .reset = drm_atomic_helper_connector_reset, > .detect = drm_bridge_connector_detect, > .fill_modes = drm_helper_probe_single_connector_modes, > - .destroy = drm_bridge_connector_destroy, > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > .debugfs_init = drm_bridge_connector_debugfs_init, > @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > int connector_type; > int ret; > > - bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); > + bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL); So you make destroy's kfree call unnecessary here ... > if (!bridge_connector) > return ERR_PTR(-ENOMEM); > > @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > return ERR_PTR(-EINVAL); > } > > - ret = drm_connector_init_with_ddc(drm, connector, > - &drm_bridge_connector_funcs, > - connector_type, ddc); > + ret = drmm_connector_init(drm, connector, > + &drm_bridge_connector_funcs, > + connector_type, ddc); ... and here of drm_connector_cleanup. drm_connector_unregister wasn't needed, so can ignore it, but you leak a reference to connector->fwnode since you don't call fwnode_handle_put anymore. We should register a drmm action right below the call to fwnode_handle_get too. Maxime
On Mon, 10 Jun 2024 at 11:04, Maxime Ripard <mripard@kernel.org> wrote: > > Hi, > > On Fri, Jun 07, 2024 at 04:22:59PM GMT, Dmitry Baryshkov wrote: > > Turn drm_bridge_connector to using drmm_kzalloc() and > > drmm_connector_init() and drop the custom destroy function. The > > drm_connector_unregister() and fwnode_handle_put() are already handled > > by the drm_connector_cleanup() and so are safe to be dropped. > > > > Acked-by: Maxime Ripard <mripard@kernel.org> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------ > > 1 file changed, 5 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c > > index 982552c9f92c..e093fc8928dc 100644 > > --- a/drivers/gpu/drm/drm_bridge_connector.c > > +++ b/drivers/gpu/drm/drm_bridge_connector.c > > @@ -15,6 +15,7 @@ > > #include <drm/drm_connector.h> > > #include <drm/drm_device.h> > > #include <drm/drm_edid.h> > > +#include <drm/drm_managed.h> > > #include <drm/drm_modeset_helper_vtables.h> > > #include <drm/drm_probe_helper.h> > > > > @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force) > > return status; > > } > > > > -static void drm_bridge_connector_destroy(struct drm_connector *connector) > > -{ > > - struct drm_bridge_connector *bridge_connector = > > - to_drm_bridge_connector(connector); > > - > > - drm_connector_unregister(connector); > > - drm_connector_cleanup(connector); > > - > > - fwnode_handle_put(connector->fwnode); > > - > > - kfree(bridge_connector); > > -} > > - > > static void drm_bridge_connector_debugfs_init(struct drm_connector *connector, > > struct dentry *root) > > { > > @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = { > > .reset = drm_atomic_helper_connector_reset, > > .detect = drm_bridge_connector_detect, > > .fill_modes = drm_helper_probe_single_connector_modes, > > - .destroy = drm_bridge_connector_destroy, > > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > .debugfs_init = drm_bridge_connector_debugfs_init, > > @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > int connector_type; > > int ret; > > > > - bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); > > + bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL); > > So you make destroy's kfree call unnecessary here ... > > > if (!bridge_connector) > > return ERR_PTR(-ENOMEM); > > > > @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > return ERR_PTR(-EINVAL); > > } > > > > - ret = drm_connector_init_with_ddc(drm, connector, > > - &drm_bridge_connector_funcs, > > - connector_type, ddc); > > + ret = drmm_connector_init(drm, connector, > > + &drm_bridge_connector_funcs, > > + connector_type, ddc); > > ... and here of drm_connector_cleanup. > > drm_connector_unregister wasn't needed, so can ignore it, but you leak a reference to > connector->fwnode since you don't call fwnode_handle_put anymore. > > We should register a drmm action right below the call to fwnode_handle_get too. But drm_connector_cleanup() already contains fwnode_handle_put(connector->fwnode). Isn't that enough?
Hi, +Hans On Mon, Jun 10, 2024 at 02:46:03PM GMT, Dmitry Baryshkov wrote: > On Mon, 10 Jun 2024 at 11:04, Maxime Ripard <mripard@kernel.org> wrote: > > > > Hi, > > > > On Fri, Jun 07, 2024 at 04:22:59PM GMT, Dmitry Baryshkov wrote: > > > Turn drm_bridge_connector to using drmm_kzalloc() and > > > drmm_connector_init() and drop the custom destroy function. The > > > drm_connector_unregister() and fwnode_handle_put() are already handled > > > by the drm_connector_cleanup() and so are safe to be dropped. > > > > > > Acked-by: Maxime Ripard <mripard@kernel.org> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------ > > > 1 file changed, 5 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c > > > index 982552c9f92c..e093fc8928dc 100644 > > > --- a/drivers/gpu/drm/drm_bridge_connector.c > > > +++ b/drivers/gpu/drm/drm_bridge_connector.c > > > @@ -15,6 +15,7 @@ > > > #include <drm/drm_connector.h> > > > #include <drm/drm_device.h> > > > #include <drm/drm_edid.h> > > > +#include <drm/drm_managed.h> > > > #include <drm/drm_modeset_helper_vtables.h> > > > #include <drm/drm_probe_helper.h> > > > > > > @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force) > > > return status; > > > } > > > > > > -static void drm_bridge_connector_destroy(struct drm_connector *connector) > > > -{ > > > - struct drm_bridge_connector *bridge_connector = > > > - to_drm_bridge_connector(connector); > > > - > > > - drm_connector_unregister(connector); > > > - drm_connector_cleanup(connector); > > > - > > > - fwnode_handle_put(connector->fwnode); > > > - > > > - kfree(bridge_connector); > > > -} > > > - > > > static void drm_bridge_connector_debugfs_init(struct drm_connector *connector, > > > struct dentry *root) > > > { > > > @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = { > > > .reset = drm_atomic_helper_connector_reset, > > > .detect = drm_bridge_connector_detect, > > > .fill_modes = drm_helper_probe_single_connector_modes, > > > - .destroy = drm_bridge_connector_destroy, > > > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > > .debugfs_init = drm_bridge_connector_debugfs_init, > > > @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > > int connector_type; > > > int ret; > > > > > > - bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); > > > + bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL); > > > > So you make destroy's kfree call unnecessary here ... > > > > > if (!bridge_connector) > > > return ERR_PTR(-ENOMEM); > > > > > > @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > > return ERR_PTR(-EINVAL); > > > } > > > > > > - ret = drm_connector_init_with_ddc(drm, connector, > > > - &drm_bridge_connector_funcs, > > > - connector_type, ddc); > > > + ret = drmm_connector_init(drm, connector, > > > + &drm_bridge_connector_funcs, > > > + connector_type, ddc); > > > > ... and here of drm_connector_cleanup. > > > > drm_connector_unregister wasn't needed, so can ignore it, but you leak a reference to > > connector->fwnode since you don't call fwnode_handle_put anymore. > > > > We should register a drmm action right below the call to fwnode_handle_get too. > > But drm_connector_cleanup() already contains > fwnode_handle_put(connector->fwnode). Isn't that enough? It does, but now I'm confused. drm_bridge_connector_init takes a reference, drm_connector_init doesn't. It will call drm_bridge_connector_destroy() that gives back its reference (which makes sense to me), but then why do drm_connector_cleanup() does? None of the drm_connector code even took that reference, and we end up with a double-put. It looks like it was introduced by commit 48c429c6d18d ("drm/connector: Add a fwnode pointer to drm_connector and register with ACPI (v2)") from Hans, which does call put, but never gets that reference. It has nothing to do with this series anymore, but that's super fishy to me, and the source of bugs as we can see here. Maxime
On Mon, Jun 10, 2024 at 02:07:06PM +0200, Maxime Ripard wrote: > Hi, > > +Hans > > On Mon, Jun 10, 2024 at 02:46:03PM GMT, Dmitry Baryshkov wrote: > > On Mon, 10 Jun 2024 at 11:04, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > Hi, > > > > > > On Fri, Jun 07, 2024 at 04:22:59PM GMT, Dmitry Baryshkov wrote: > > > > Turn drm_bridge_connector to using drmm_kzalloc() and > > > > drmm_connector_init() and drop the custom destroy function. The > > > > drm_connector_unregister() and fwnode_handle_put() are already handled > > > > by the drm_connector_cleanup() and so are safe to be dropped. > > > > > > > > Acked-by: Maxime Ripard <mripard@kernel.org> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > --- > > > > drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------ > > > > 1 file changed, 5 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c > > > > index 982552c9f92c..e093fc8928dc 100644 > > > > --- a/drivers/gpu/drm/drm_bridge_connector.c > > > > +++ b/drivers/gpu/drm/drm_bridge_connector.c > > > > @@ -15,6 +15,7 @@ > > > > #include <drm/drm_connector.h> > > > > #include <drm/drm_device.h> > > > > #include <drm/drm_edid.h> > > > > +#include <drm/drm_managed.h> > > > > #include <drm/drm_modeset_helper_vtables.h> > > > > #include <drm/drm_probe_helper.h> > > > > > > > > @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force) > > > > return status; > > > > } > > > > > > > > -static void drm_bridge_connector_destroy(struct drm_connector *connector) > > > > -{ > > > > - struct drm_bridge_connector *bridge_connector = > > > > - to_drm_bridge_connector(connector); > > > > - > > > > - drm_connector_unregister(connector); > > > > - drm_connector_cleanup(connector); > > > > - > > > > - fwnode_handle_put(connector->fwnode); > > > > - > > > > - kfree(bridge_connector); > > > > -} > > > > - > > > > static void drm_bridge_connector_debugfs_init(struct drm_connector *connector, > > > > struct dentry *root) > > > > { > > > > @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = { > > > > .reset = drm_atomic_helper_connector_reset, > > > > .detect = drm_bridge_connector_detect, > > > > .fill_modes = drm_helper_probe_single_connector_modes, > > > > - .destroy = drm_bridge_connector_destroy, > > > > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > > > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > > > .debugfs_init = drm_bridge_connector_debugfs_init, > > > > @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > > > int connector_type; > > > > int ret; > > > > > > > > - bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); > > > > + bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL); > > > > > > So you make destroy's kfree call unnecessary here ... > > > > > > > if (!bridge_connector) > > > > return ERR_PTR(-ENOMEM); > > > > > > > > @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > > > return ERR_PTR(-EINVAL); > > > > } > > > > > > > > - ret = drm_connector_init_with_ddc(drm, connector, > > > > - &drm_bridge_connector_funcs, > > > > - connector_type, ddc); > > > > + ret = drmm_connector_init(drm, connector, > > > > + &drm_bridge_connector_funcs, > > > > + connector_type, ddc); > > > > > > ... and here of drm_connector_cleanup. > > > > > > drm_connector_unregister wasn't needed, so can ignore it, but you leak a reference to > > > connector->fwnode since you don't call fwnode_handle_put anymore. > > > > > > We should register a drmm action right below the call to fwnode_handle_get too. > > > > But drm_connector_cleanup() already contains > > fwnode_handle_put(connector->fwnode). Isn't that enough? > > It does, but now I'm confused. > > drm_bridge_connector_init takes a reference, drm_connector_init doesn't. > It will call drm_bridge_connector_destroy() that gives back its > reference (which makes sense to me), but then why do > drm_connector_cleanup() does? None of the drm_connector code even took > that reference, and we end up with a double-put. > > It looks like it was introduced by commit 48c429c6d18d ("drm/connector: > Add a fwnode pointer to drm_connector and register with ACPI (v2)") from > Hans, which does call put, but never gets that reference. The mentioned patch documents that pretty clearly: * Drivers can set this to associate a fwnode with a connector, drivers * are expected to get a reference on the fwnode when setting this. * drm_connector_cleanup() will call fwnode_handle_put() on this. This is logical. Whoever sets the drm_connector::fwnode pointer, should get reference. This way drm_connector_init() doesn't need to play with the reference counting. The cleanup code drops the reference (so the driver doesn't need to), because cleanup might be assynchronous.. The drm_bridge_connector follows this approach: it sets drm_connector->fwnode, so it gets the reference. It uses drm_connector_cleanup(), so it doesn't need to put it. > > It has nothing to do with this series anymore, but that's super fishy to > me, and the source of bugs as we can see here.
On Mon, Jun 10, 2024 at 08:54:09PM GMT, Dmitry Baryshkov wrote: > On Mon, Jun 10, 2024 at 02:07:06PM +0200, Maxime Ripard wrote: > > Hi, > > > > +Hans > > > > On Mon, Jun 10, 2024 at 02:46:03PM GMT, Dmitry Baryshkov wrote: > > > On Mon, 10 Jun 2024 at 11:04, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > > > Hi, > > > > > > > > On Fri, Jun 07, 2024 at 04:22:59PM GMT, Dmitry Baryshkov wrote: > > > > > Turn drm_bridge_connector to using drmm_kzalloc() and > > > > > drmm_connector_init() and drop the custom destroy function. The > > > > > drm_connector_unregister() and fwnode_handle_put() are already handled > > > > > by the drm_connector_cleanup() and so are safe to be dropped. > > > > > > > > > > Acked-by: Maxime Ripard <mripard@kernel.org> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > --- > > > > > drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------ > > > > > 1 file changed, 5 insertions(+), 18 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c > > > > > index 982552c9f92c..e093fc8928dc 100644 > > > > > --- a/drivers/gpu/drm/drm_bridge_connector.c > > > > > +++ b/drivers/gpu/drm/drm_bridge_connector.c > > > > > @@ -15,6 +15,7 @@ > > > > > #include <drm/drm_connector.h> > > > > > #include <drm/drm_device.h> > > > > > #include <drm/drm_edid.h> > > > > > +#include <drm/drm_managed.h> > > > > > #include <drm/drm_modeset_helper_vtables.h> > > > > > #include <drm/drm_probe_helper.h> > > > > > > > > > > @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force) > > > > > return status; > > > > > } > > > > > > > > > > -static void drm_bridge_connector_destroy(struct drm_connector *connector) > > > > > -{ > > > > > - struct drm_bridge_connector *bridge_connector = > > > > > - to_drm_bridge_connector(connector); > > > > > - > > > > > - drm_connector_unregister(connector); > > > > > - drm_connector_cleanup(connector); > > > > > - > > > > > - fwnode_handle_put(connector->fwnode); > > > > > - > > > > > - kfree(bridge_connector); > > > > > -} > > > > > - > > > > > static void drm_bridge_connector_debugfs_init(struct drm_connector *connector, > > > > > struct dentry *root) > > > > > { > > > > > @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = { > > > > > .reset = drm_atomic_helper_connector_reset, > > > > > .detect = drm_bridge_connector_detect, > > > > > .fill_modes = drm_helper_probe_single_connector_modes, > > > > > - .destroy = drm_bridge_connector_destroy, > > > > > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > > > > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > > > > .debugfs_init = drm_bridge_connector_debugfs_init, > > > > > @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > > > > int connector_type; > > > > > int ret; > > > > > > > > > > - bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); > > > > > + bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL); > > > > > > > > So you make destroy's kfree call unnecessary here ... > > > > > > > > > if (!bridge_connector) > > > > > return ERR_PTR(-ENOMEM); > > > > > > > > > > @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > > > > return ERR_PTR(-EINVAL); > > > > > } > > > > > > > > > > - ret = drm_connector_init_with_ddc(drm, connector, > > > > > - &drm_bridge_connector_funcs, > > > > > - connector_type, ddc); > > > > > + ret = drmm_connector_init(drm, connector, > > > > > + &drm_bridge_connector_funcs, > > > > > + connector_type, ddc); > > > > > > > > ... and here of drm_connector_cleanup. > > > > > > > > drm_connector_unregister wasn't needed, so can ignore it, but you leak a reference to > > > > connector->fwnode since you don't call fwnode_handle_put anymore. > > > > > > > > We should register a drmm action right below the call to fwnode_handle_get too. > > > > > > But drm_connector_cleanup() already contains > > > fwnode_handle_put(connector->fwnode). Isn't that enough? > > > > It does, but now I'm confused. > > > > drm_bridge_connector_init takes a reference, drm_connector_init doesn't. > > It will call drm_bridge_connector_destroy() that gives back its > > reference (which makes sense to me), but then why do > > drm_connector_cleanup() does? None of the drm_connector code even took > > that reference, and we end up with a double-put. > > > > It looks like it was introduced by commit 48c429c6d18d ("drm/connector: > > Add a fwnode pointer to drm_connector and register with ACPI (v2)") from > > Hans, which does call put, but never gets that reference. > > The mentioned patch documents that pretty clearly: > > * Drivers can set this to associate a fwnode with a connector, drivers > * are expected to get a reference on the fwnode when setting this. > * drm_connector_cleanup() will call fwnode_handle_put() on this. > > This is logical. Whoever sets the drm_connector::fwnode pointer, should > get reference. This way drm_connector_init() doesn't need to play with > the reference counting. The cleanup code drops the reference (so the > driver doesn't need to), because cleanup might be assynchronous.. Right, but it's the cleanup part that isn't logical. It makes total sense to have the connector that sets connector->fwnode get the reference itself. It doesn't make sense to have the core give that reference instead of the driver. It's confusing, because if the driver is supposed to handle its reference itself, then it should handle all of it itself. This bug is the testament for that: the natural approach is buggy. > The drm_bridge_connector follows this approach: it sets > drm_connector->fwnode, so it gets the reference. It uses > drm_connector_cleanup(), so it doesn't need to put it. Yet, it calls fwnode_handle_put in its destroy path, because it grabbed a reference before. Maxime
On Tue, 11 Jun 2024 at 11:54, Maxime Ripard <mripard@kernel.org> wrote: > > On Mon, Jun 10, 2024 at 08:54:09PM GMT, Dmitry Baryshkov wrote: > > On Mon, Jun 10, 2024 at 02:07:06PM +0200, Maxime Ripard wrote: > > > Hi, > > > > > > +Hans > > > > > > On Mon, Jun 10, 2024 at 02:46:03PM GMT, Dmitry Baryshkov wrote: > > > > On Mon, 10 Jun 2024 at 11:04, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > > > > > Hi, > > > > > > > > > > On Fri, Jun 07, 2024 at 04:22:59PM GMT, Dmitry Baryshkov wrote: > > > > > > Turn drm_bridge_connector to using drmm_kzalloc() and > > > > > > drmm_connector_init() and drop the custom destroy function. The > > > > > > drm_connector_unregister() and fwnode_handle_put() are already handled > > > > > > by the drm_connector_cleanup() and so are safe to be dropped. > > > > > > > > > > > > Acked-by: Maxime Ripard <mripard@kernel.org> > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > --- > > > > > > drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------ > > > > > > 1 file changed, 5 insertions(+), 18 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c > > > > > > index 982552c9f92c..e093fc8928dc 100644 > > > > > > --- a/drivers/gpu/drm/drm_bridge_connector.c > > > > > > +++ b/drivers/gpu/drm/drm_bridge_connector.c > > > > > > @@ -15,6 +15,7 @@ > > > > > > #include <drm/drm_connector.h> > > > > > > #include <drm/drm_device.h> > > > > > > #include <drm/drm_edid.h> > > > > > > +#include <drm/drm_managed.h> > > > > > > #include <drm/drm_modeset_helper_vtables.h> > > > > > > #include <drm/drm_probe_helper.h> > > > > > > > > > > > > @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force) > > > > > > return status; > > > > > > } > > > > > > > > > > > > -static void drm_bridge_connector_destroy(struct drm_connector *connector) > > > > > > -{ > > > > > > - struct drm_bridge_connector *bridge_connector = > > > > > > - to_drm_bridge_connector(connector); > > > > > > - > > > > > > - drm_connector_unregister(connector); > > > > > > - drm_connector_cleanup(connector); > > > > > > - > > > > > > - fwnode_handle_put(connector->fwnode); > > > > > > - > > > > > > - kfree(bridge_connector); > > > > > > -} > > > > > > - > > > > > > static void drm_bridge_connector_debugfs_init(struct drm_connector *connector, > > > > > > struct dentry *root) > > > > > > { > > > > > > @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = { > > > > > > .reset = drm_atomic_helper_connector_reset, > > > > > > .detect = drm_bridge_connector_detect, > > > > > > .fill_modes = drm_helper_probe_single_connector_modes, > > > > > > - .destroy = drm_bridge_connector_destroy, > > > > > > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > > > > > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > > > > > .debugfs_init = drm_bridge_connector_debugfs_init, > > > > > > @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > > > > > int connector_type; > > > > > > int ret; > > > > > > > > > > > > - bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); > > > > > > + bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL); > > > > > > > > > > So you make destroy's kfree call unnecessary here ... > > > > > > > > > > > if (!bridge_connector) > > > > > > return ERR_PTR(-ENOMEM); > > > > > > > > > > > > @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > > > > > return ERR_PTR(-EINVAL); > > > > > > } > > > > > > > > > > > > - ret = drm_connector_init_with_ddc(drm, connector, > > > > > > - &drm_bridge_connector_funcs, > > > > > > - connector_type, ddc); > > > > > > + ret = drmm_connector_init(drm, connector, > > > > > > + &drm_bridge_connector_funcs, > > > > > > + connector_type, ddc); > > > > > > > > > > ... and here of drm_connector_cleanup. > > > > > > > > > > drm_connector_unregister wasn't needed, so can ignore it, but you leak a reference to > > > > > connector->fwnode since you don't call fwnode_handle_put anymore. > > > > > > > > > > We should register a drmm action right below the call to fwnode_handle_get too. > > > > > > > > But drm_connector_cleanup() already contains > > > > fwnode_handle_put(connector->fwnode). Isn't that enough? > > > > > > It does, but now I'm confused. > > > > > > drm_bridge_connector_init takes a reference, drm_connector_init doesn't. > > > It will call drm_bridge_connector_destroy() that gives back its > > > reference (which makes sense to me), but then why do > > > drm_connector_cleanup() does? None of the drm_connector code even took > > > that reference, and we end up with a double-put. > > > > > > It looks like it was introduced by commit 48c429c6d18d ("drm/connector: > > > Add a fwnode pointer to drm_connector and register with ACPI (v2)") from > > > Hans, which does call put, but never gets that reference. > > > > The mentioned patch documents that pretty clearly: > > > > * Drivers can set this to associate a fwnode with a connector, drivers > > * are expected to get a reference on the fwnode when setting this. > > * drm_connector_cleanup() will call fwnode_handle_put() on this. > > > > This is logical. Whoever sets the drm_connector::fwnode pointer, should > > get reference. This way drm_connector_init() doesn't need to play with > > the reference counting. The cleanup code drops the reference (so the > > driver doesn't need to), because cleanup might be assynchronous.. > > Right, but it's the cleanup part that isn't logical. It makes total > sense to have the connector that sets connector->fwnode get the > reference itself. It doesn't make sense to have the core give that > reference instead of the driver. > > It's confusing, because if the driver is supposed to handle its > reference itself, then it should handle all of it itself. This bug is > the testament for that: the natural approach is buggy. I'd say this is the 'transfer of the ownership'. The base driver gets the reference, and then gives it away to the drm_connecter. But indeed this is not very intuitive. I have looked at the original series by Hans/Heikky, but I don't seem to be able to find a good way to solve that. The fwnode can be set after initialising the drm_connector. And at the same time it doesn't make so much sense to put that burden onto the driver. One option might be to add drm_connector_set_fwnode() that will get the reference internally, but that looks a bit like an overkill. > > > The drm_bridge_connector follows this approach: it sets > > drm_connector->fwnode, so it gets the reference. It uses > > drm_connector_cleanup(), so it doesn't need to put it. > > Yet, it calls fwnode_handle_put in its destroy path, because it grabbed > a reference before. > > Maxime
On Tue, Jun 11, 2024 at 02:26:12PM GMT, Dmitry Baryshkov wrote: > On Tue, 11 Jun 2024 at 11:54, Maxime Ripard <mripard@kernel.org> wrote: > > > > On Mon, Jun 10, 2024 at 08:54:09PM GMT, Dmitry Baryshkov wrote: > > > On Mon, Jun 10, 2024 at 02:07:06PM +0200, Maxime Ripard wrote: > > > > Hi, > > > > > > > > +Hans > > > > > > > > On Mon, Jun 10, 2024 at 02:46:03PM GMT, Dmitry Baryshkov wrote: > > > > > On Mon, 10 Jun 2024 at 11:04, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > On Fri, Jun 07, 2024 at 04:22:59PM GMT, Dmitry Baryshkov wrote: > > > > > > > Turn drm_bridge_connector to using drmm_kzalloc() and > > > > > > > drmm_connector_init() and drop the custom destroy function. The > > > > > > > drm_connector_unregister() and fwnode_handle_put() are already handled > > > > > > > by the drm_connector_cleanup() and so are safe to be dropped. > > > > > > > > > > > > > > Acked-by: Maxime Ripard <mripard@kernel.org> > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > > --- > > > > > > > drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------ > > > > > > > 1 file changed, 5 insertions(+), 18 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c > > > > > > > index 982552c9f92c..e093fc8928dc 100644 > > > > > > > --- a/drivers/gpu/drm/drm_bridge_connector.c > > > > > > > +++ b/drivers/gpu/drm/drm_bridge_connector.c > > > > > > > @@ -15,6 +15,7 @@ > > > > > > > #include <drm/drm_connector.h> > > > > > > > #include <drm/drm_device.h> > > > > > > > #include <drm/drm_edid.h> > > > > > > > +#include <drm/drm_managed.h> > > > > > > > #include <drm/drm_modeset_helper_vtables.h> > > > > > > > #include <drm/drm_probe_helper.h> > > > > > > > > > > > > > > @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force) > > > > > > > return status; > > > > > > > } > > > > > > > > > > > > > > -static void drm_bridge_connector_destroy(struct drm_connector *connector) > > > > > > > -{ > > > > > > > - struct drm_bridge_connector *bridge_connector = > > > > > > > - to_drm_bridge_connector(connector); > > > > > > > - > > > > > > > - drm_connector_unregister(connector); > > > > > > > - drm_connector_cleanup(connector); > > > > > > > - > > > > > > > - fwnode_handle_put(connector->fwnode); > > > > > > > - > > > > > > > - kfree(bridge_connector); > > > > > > > -} > > > > > > > - > > > > > > > static void drm_bridge_connector_debugfs_init(struct drm_connector *connector, > > > > > > > struct dentry *root) > > > > > > > { > > > > > > > @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = { > > > > > > > .reset = drm_atomic_helper_connector_reset, > > > > > > > .detect = drm_bridge_connector_detect, > > > > > > > .fill_modes = drm_helper_probe_single_connector_modes, > > > > > > > - .destroy = drm_bridge_connector_destroy, > > > > > > > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > > > > > > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > > > > > > .debugfs_init = drm_bridge_connector_debugfs_init, > > > > > > > @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > > > > > > int connector_type; > > > > > > > int ret; > > > > > > > > > > > > > > - bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); > > > > > > > + bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL); > > > > > > > > > > > > So you make destroy's kfree call unnecessary here ... > > > > > > > > > > > > > if (!bridge_connector) > > > > > > > return ERR_PTR(-ENOMEM); > > > > > > > > > > > > > > @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > > > > > > return ERR_PTR(-EINVAL); > > > > > > > } > > > > > > > > > > > > > > - ret = drm_connector_init_with_ddc(drm, connector, > > > > > > > - &drm_bridge_connector_funcs, > > > > > > > - connector_type, ddc); > > > > > > > + ret = drmm_connector_init(drm, connector, > > > > > > > + &drm_bridge_connector_funcs, > > > > > > > + connector_type, ddc); > > > > > > > > > > > > ... and here of drm_connector_cleanup. > > > > > > > > > > > > drm_connector_unregister wasn't needed, so can ignore it, but you leak a reference to > > > > > > connector->fwnode since you don't call fwnode_handle_put anymore. > > > > > > > > > > > > We should register a drmm action right below the call to fwnode_handle_get too. > > > > > > > > > > But drm_connector_cleanup() already contains > > > > > fwnode_handle_put(connector->fwnode). Isn't that enough? > > > > > > > > It does, but now I'm confused. > > > > > > > > drm_bridge_connector_init takes a reference, drm_connector_init doesn't. > > > > It will call drm_bridge_connector_destroy() that gives back its > > > > reference (which makes sense to me), but then why do > > > > drm_connector_cleanup() does? None of the drm_connector code even took > > > > that reference, and we end up with a double-put. > > > > > > > > It looks like it was introduced by commit 48c429c6d18d ("drm/connector: > > > > Add a fwnode pointer to drm_connector and register with ACPI (v2)") from > > > > Hans, which does call put, but never gets that reference. > > > > > > The mentioned patch documents that pretty clearly: > > > > > > * Drivers can set this to associate a fwnode with a connector, drivers > > > * are expected to get a reference on the fwnode when setting this. > > > * drm_connector_cleanup() will call fwnode_handle_put() on this. > > > > > > This is logical. Whoever sets the drm_connector::fwnode pointer, should > > > get reference. This way drm_connector_init() doesn't need to play with > > > the reference counting. The cleanup code drops the reference (so the > > > driver doesn't need to), because cleanup might be assynchronous.. > > > > Right, but it's the cleanup part that isn't logical. It makes total > > sense to have the connector that sets connector->fwnode get the > > reference itself. It doesn't make sense to have the core give that > > reference instead of the driver. > > > > It's confusing, because if the driver is supposed to handle its > > reference itself, then it should handle all of it itself. This bug is > > the testament for that: the natural approach is buggy. > > I'd say this is the 'transfer of the ownership'. The base driver gets > the reference, and then gives it away to the drm_connecter. But indeed > this is not very intuitive. > > I have looked at the original series by Hans/Heikky, but I don't seem > to be able to find a good way to solve that. The fwnode can be set > after initialising the drm_connector. And at the same time it doesn't > make so much sense to put that burden onto the driver. One option > might be to add drm_connector_set_fwnode() that will get the reference > internally, but that looks a bit like an overkill. It looks like there's only one driver that actually requires that kind of hack: i915. So, imo, the hacks should be in i915 and not cripple the core API. Maxime
diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c index 982552c9f92c..e093fc8928dc 100644 --- a/drivers/gpu/drm/drm_bridge_connector.c +++ b/drivers/gpu/drm/drm_bridge_connector.c @@ -15,6 +15,7 @@ #include <drm/drm_connector.h> #include <drm/drm_device.h> #include <drm/drm_edid.h> +#include <drm/drm_managed.h> #include <drm/drm_modeset_helper_vtables.h> #include <drm/drm_probe_helper.h> @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force) return status; } -static void drm_bridge_connector_destroy(struct drm_connector *connector) -{ - struct drm_bridge_connector *bridge_connector = - to_drm_bridge_connector(connector); - - drm_connector_unregister(connector); - drm_connector_cleanup(connector); - - fwnode_handle_put(connector->fwnode); - - kfree(bridge_connector); -} - static void drm_bridge_connector_debugfs_init(struct drm_connector *connector, struct dentry *root) { @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = { .reset = drm_atomic_helper_connector_reset, .detect = drm_bridge_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = drm_bridge_connector_destroy, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, .debugfs_init = drm_bridge_connector_debugfs_init, @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, int connector_type; int ret; - bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); + bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL); if (!bridge_connector) return ERR_PTR(-ENOMEM); @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, return ERR_PTR(-EINVAL); } - ret = drm_connector_init_with_ddc(drm, connector, - &drm_bridge_connector_funcs, - connector_type, ddc); + ret = drmm_connector_init(drm, connector, + &drm_bridge_connector_funcs, + connector_type, ddc); if (ret) { kfree(bridge_connector); return ERR_PTR(ret);