| Message ID | 20200212173727.19476-2-kieran.bingham+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | max9286: Refactor V4L2 support to prevent EPROBE_DEFER failures | expand |
Hi Me, On 12/02/2020 17:37, Kieran Bingham wrote: > Move all the V4L2 Subdev Async registration so that it can only happen once > we know we will not need to -EPROBE_DEFER... > > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++-------------- > 1 file changed, 55 insertions(+), 33 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 1b4ff3533795..03c5fa232b6d 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = { > .unbind = max9286_notify_unbind, > }; > > +static int max9286_v4l2_async_register(struct max9286_priv *priv) > +{ > + struct device *dev = &priv->client->dev; > + struct max9286_source *source = NULL; > + int ret; > + > + v4l2_async_notifier_init(&priv->notifier); > + > + for_each_source(priv, source) { > + unsigned int i = to_index(priv, source); > + > + dev_err(dev, "Registering v4l2-async for source %d\n", i); Debug line, can be removed. > + > + source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; > + source->asd.match.fwnode = source->fwnode; > + > + ret = v4l2_async_notifier_add_subdev(&priv->notifier, > + &source->asd); > + if (ret) { > + dev_err(dev, "Failed to add subdev for source %d", i); > + v4l2_async_notifier_cleanup(&priv->notifier); > + return ret; > + } > + } > + > + priv->notifier.ops = &max9286_notify_ops; > + > + ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier); > + if (ret) { > + dev_err(dev, "Failed to register subdev_notifier"); > + v4l2_async_notifier_cleanup(&priv->notifier); > + return ret; > + } > + > + return 0; > +} > + > +static void max9286_v4l2_async_unregister(struct max9286_priv *priv) > +{ > + v4l2_async_notifier_unregister(&priv->notifier); > + v4l2_async_notifier_cleanup(&priv->notifier); > +} > + > static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > { > struct max9286_priv *priv = sd_to_max9286(sd); > @@ -870,6 +913,13 @@ static int max9286_init(struct device *dev) > goto err_regulator; > } > > + /* Register v4l2 async notifiers */ > + ret = max9286_v4l2_async_register(priv); > + if (ret) { > + dev_err(dev, "Unable to register V4L2 async notifiers\n"); > + goto err_regulator; > + } > + > v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops); > priv->sd.internal_ops = &max9286_subdev_internal_ops; > priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > @@ -884,7 +934,7 @@ static int max9286_init(struct device *dev) > priv->sd.ctrl_handler = &priv->ctrls; > ret = priv->ctrls.error; > if (ret) > - goto err_regulator; > + goto err_async; > > priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > > @@ -927,6 +977,8 @@ static int max9286_init(struct device *dev) > max9286_i2c_mux_close(priv); > err_put_node: > fwnode_handle_put(ep); > +err_async: > + max9286_v4l2_async_unregister(priv); > err_regulator: > regulator_disable(priv->regulator); > priv->poc_enabled = false; > @@ -938,14 +990,6 @@ static void max9286_cleanup_dt(struct max9286_priv *priv) > { > struct max9286_source *source; > > - /* > - * Not strictly part of the DT, but the notifier is registered during > - * max9286_parse_dt(), and the notifier holds references to the fwnodes > - * thus the cleanup is here to mirror the registration. > - */ > - v4l2_async_notifier_unregister(&priv->notifier); > - v4l2_async_notifier_cleanup(&priv->notifier); > - > for_each_source(priv, source) { > fwnode_handle_put(source->fwnode); > source->fwnode = NULL; > @@ -958,7 +1002,6 @@ static int max9286_parse_dt(struct max9286_priv *priv) > struct device_node *i2c_mux; > struct device_node *node = NULL; > unsigned int i2c_mux_mask = 0; > - int ret; > > of_node_get(dev->of_node); > i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux"); > @@ -986,8 +1029,6 @@ static int max9286_parse_dt(struct max9286_priv *priv) > of_node_put(node); > of_node_put(i2c_mux); > > - v4l2_async_notifier_init(&priv->notifier); > - > /* Parse the endpoints */ > for_each_endpoint_of_node(dev->of_node, node) { > struct max9286_source *source; > @@ -1056,34 +1097,14 @@ static int max9286_parse_dt(struct max9286_priv *priv) > continue; > } > > - source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; > - source->asd.match.fwnode = source->fwnode; > - > - ret = v4l2_async_notifier_add_subdev(&priv->notifier, > - &source->asd); > - if (ret) { > - v4l2_async_notifier_cleanup(&priv->notifier); > - of_node_put(node); > - return ret; > - } > - > priv->source_mask |= BIT(ep.port); > priv->nsources++; > } > of_node_put(node); > > - /* Do not register the subdev notifier if there are no devices. */ > - if (!priv->nsources) > - return 0; > - > priv->route_mask = priv->source_mask; > - priv->notifier.ops = &max9286_notify_ops; > - > - ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier); > - if (ret) > - v4l2_async_notifier_cleanup(&priv->notifier); > > - return ret; > + return 0; > } > > static int max9286_probe(struct i2c_client *client) > @@ -1182,6 +1203,7 @@ static int max9286_remove(struct i2c_client *client) > > fwnode_handle_put(priv->sd.fwnode); > v4l2_async_unregister_subdev(&priv->sd); > + max9286_v4l2_async_unregister(priv); > > if (priv->poc_enabled) > regulator_disable(priv->regulator); >
Hi Kieran, very nice thanks for handling this Just a few minors On Wed, Feb 12, 2020 at 05:37:26PM +0000, Kieran Bingham wrote: > Move all the V4L2 Subdev Async registration so that it can only happen once > we know we will not need to -EPROBE_DEFER... > > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++-------------- > 1 file changed, 55 insertions(+), 33 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 1b4ff3533795..03c5fa232b6d 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = { > .unbind = max9286_notify_unbind, > }; > > +static int max9286_v4l2_async_register(struct max9286_priv *priv) Could you capture in the function name this actually deals with notifiers ? Like max9286_notifier_register() or similar... > +{ > + struct device *dev = &priv->client->dev; > + struct max9286_source *source = NULL; > + int ret; > + Do we want to init and register the notifier if there are no sources connected ? I would keep if (!priv->nsources) return 0; here or in the caller. > + v4l2_async_notifier_init(&priv->notifier); > + > + for_each_source(priv, source) { > + unsigned int i = to_index(priv, source); > + > + dev_err(dev, "Registering v4l2-async for source %d\n", i); dev_err ? > + > + source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; > + source->asd.match.fwnode = source->fwnode; > + > + ret = v4l2_async_notifier_add_subdev(&priv->notifier, > + &source->asd); > + if (ret) { > + dev_err(dev, "Failed to add subdev for source %d", i); > + v4l2_async_notifier_cleanup(&priv->notifier); v4l2_async_notifier_cleanup() does fwnode_handle_put() on the async subdevs registered to the notifier but not yet completed. All the other sources have to be put as well I think. How to do so might be not nice, as you would need to keep track of which sources have been registered to the notifier already and put the other ones in the error path. > + return ret; > + } > + } > + > + priv->notifier.ops = &max9286_notify_ops; > + > + ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier); > + if (ret) { > + dev_err(dev, "Failed to register subdev_notifier"); > + v4l2_async_notifier_cleanup(&priv->notifier); Here it's fine to call notifier_cleanup() > + return ret; > + } > + > + return 0; > +} > + > +static void max9286_v4l2_async_unregister(struct max9286_priv *priv) > +{ > + v4l2_async_notifier_unregister(&priv->notifier); > + v4l2_async_notifier_cleanup(&priv->notifier); I would first cleanup() then unregister() even if they deal with two different sets of asds (done and registred-but-not-yet-done ones). > +} > + > static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > { > struct max9286_priv *priv = sd_to_max9286(sd); > @@ -870,6 +913,13 @@ static int max9286_init(struct device *dev) > goto err_regulator; > } > > + /* Register v4l2 async notifiers */ > + ret = max9286_v4l2_async_register(priv); > + if (ret) { > + dev_err(dev, "Unable to register V4L2 async notifiers\n"); > + goto err_regulator; > + } > + > v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops); > priv->sd.internal_ops = &max9286_subdev_internal_ops; > priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > @@ -884,7 +934,7 @@ static int max9286_init(struct device *dev) > priv->sd.ctrl_handler = &priv->ctrls; > ret = priv->ctrls.error; > if (ret) > - goto err_regulator; > + goto err_async; > > priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > > @@ -927,6 +977,8 @@ static int max9286_init(struct device *dev) > max9286_i2c_mux_close(priv); > err_put_node: > fwnode_handle_put(ep); > +err_async: > + max9286_v4l2_async_unregister(priv); > err_regulator: > regulator_disable(priv->regulator); > priv->poc_enabled = false; > @@ -938,14 +990,6 @@ static void max9286_cleanup_dt(struct max9286_priv *priv) > { > struct max9286_source *source; > > - /* > - * Not strictly part of the DT, but the notifier is registered during > - * max9286_parse_dt(), and the notifier holds references to the fwnodes > - * thus the cleanup is here to mirror the registration. > - */ > - v4l2_async_notifier_unregister(&priv->notifier); > - v4l2_async_notifier_cleanup(&priv->notifier); > - > for_each_source(priv, source) { > fwnode_handle_put(source->fwnode); Wasn't this a double fwnode_handle_put() ? We called notifier_cleanup() and then put all sources again manually. I don't see one more get() when the asd gets registered to the notifier with v4l2_async_notifier_add_subdev() so I'm afraid this would result in a duplicated put(). Am I wrong ? It was there already, but I think it happens in this patch as well: if max9286_init() fails calls max9286_v4l2_unregister() which then calls max9286_v4l2_async_unregister() which put all the not-yet-completed subdevs by calling v4l2_async_notifier_cleanup(). Then in the probe() function error path we then call max9286_cleanup_dt() which puts again all the registered sources regardless of their completed status. I would call max9286_cleanup_dt() only if max9286_init() has not been called yet. If we get to register subdevs to the notifier, I would then provide a function that calls v4l2_async_notifier_cleanup() and then manually puts all sources not yet registered. I'm afraid this would need to keep a status flag in the source, unless you have a more elegant solution. Thanks j > source->fwnode = NULL; > @@ -958,7 +1002,6 @@ static int max9286_parse_dt(struct max9286_priv *priv) > struct device_node *i2c_mux; > struct device_node *node = NULL; > unsigned int i2c_mux_mask = 0; > - int ret; > > of_node_get(dev->of_node); > i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux"); > @@ -986,8 +1029,6 @@ static int max9286_parse_dt(struct max9286_priv *priv) > of_node_put(node); > of_node_put(i2c_mux); > > - v4l2_async_notifier_init(&priv->notifier); > - > /* Parse the endpoints */ > for_each_endpoint_of_node(dev->of_node, node) { > struct max9286_source *source; > @@ -1056,34 +1097,14 @@ static int max9286_parse_dt(struct max9286_priv *priv) > continue; > } > > - source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; > - source->asd.match.fwnode = source->fwnode; > - > - ret = v4l2_async_notifier_add_subdev(&priv->notifier, > - &source->asd); > - if (ret) { > - v4l2_async_notifier_cleanup(&priv->notifier); > - of_node_put(node); > - return ret; > - } > - > priv->source_mask |= BIT(ep.port); > priv->nsources++; > } > of_node_put(node); > > - /* Do not register the subdev notifier if there are no devices. */ > - if (!priv->nsources) > - return 0; > - > priv->route_mask = priv->source_mask; > - priv->notifier.ops = &max9286_notify_ops; > - > - ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier); > - if (ret) > - v4l2_async_notifier_cleanup(&priv->notifier); > > - return ret; > + return 0; > } > > static int max9286_probe(struct i2c_client *client) > @@ -1182,6 +1203,7 @@ static int max9286_remove(struct i2c_client *client) > > fwnode_handle_put(priv->sd.fwnode); > v4l2_async_unregister_subdev(&priv->sd); > + max9286_v4l2_async_unregister(priv); > > if (priv->poc_enabled) > regulator_disable(priv->regulator); > -- > 2.20.1 >
Hi Jacopo, On 13/02/2020 09:46, Jacopo Mondi wrote: > Hi Kieran, > very nice thanks for handling this :-) > Just a few minors :-s hehe > > On Wed, Feb 12, 2020 at 05:37:26PM +0000, Kieran Bingham wrote: >> Move all the V4L2 Subdev Async registration so that it can only happen once >> we know we will not need to -EPROBE_DEFER... >> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >> --- >> drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++-------------- >> 1 file changed, 55 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c >> index 1b4ff3533795..03c5fa232b6d 100644 >> --- a/drivers/media/i2c/max9286.c >> +++ b/drivers/media/i2c/max9286.c >> @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = { >> .unbind = max9286_notify_unbind, >> }; >> >> +static int max9286_v4l2_async_register(struct max9286_priv *priv) > > Could you capture in the function name this actually deals with > notifiers ? Like max9286_notifier_register() or similar... I'd like to keep the 'v4l2' in there somewhere... max9286_v4l2_notifier_register() ? But then maybe even that could be confused with the notifiers/async handling for the max9286 itself. My aim was that max9286_v4l2_async_{un,}register() dealt with subdevices connected to the max9286 only ... For ~20 lines of code, it could be inlined up a level into max9286_v4l2_register() but I do perhaps like trying to keep the symmetry clean somehow. <scratch that - below tells me not to inline> >> +{ >> + struct device *dev = &priv->client->dev; >> + struct max9286_source *source = NULL; >> + int ret; >> + > > Do we want to init and register the notifier if there are no sources > connected ? I would keep > > if (!priv->nsources) > return 0; > > here or in the caller. Ah yes, I had thought about that but clearly not acted upon it much. If we know there is nothing to notify us, I guess we won't expect any need to register the notifications! Although this would certainly mean keeping the sources registration in their own functions. > >> + v4l2_async_notifier_init(&priv->notifier); >> + >> + for_each_source(priv, source) { >> + unsigned int i = to_index(priv, source); >> + >> + dev_err(dev, "Registering v4l2-async for source %d\n", i); > > dev_err ? > Already removed, left over debug print. >> + >> + source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; >> + source->asd.match.fwnode = source->fwnode; >> + >> + ret = v4l2_async_notifier_add_subdev(&priv->notifier, >> + &source->asd); >> + if (ret) { >> + dev_err(dev, "Failed to add subdev for source %d", i); >> + v4l2_async_notifier_cleanup(&priv->notifier); > > v4l2_async_notifier_cleanup() does fwnode_handle_put() on the async > subdevs registered to the notifier but not yet completed. All the other > sources have to be put as well I think. > > How to do so might be not nice, as you would need to keep track of which > sources have been registered to the notifier already and put the other > ones in the error path. Or can we move all fwnode refcounting back to cleanup_dt perhaps? I'll have a look. > >> + return ret; >> + } >> + } >> + >> + priv->notifier.ops = &max9286_notify_ops; >> + >> + ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier); >> + if (ret) { >> + dev_err(dev, "Failed to register subdev_notifier"); >> + v4l2_async_notifier_cleanup(&priv->notifier); > > Here it's fine to call notifier_cleanup() > >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static void max9286_v4l2_async_unregister(struct max9286_priv *priv) >> +{ >> + v4l2_async_notifier_unregister(&priv->notifier); >> + v4l2_async_notifier_cleanup(&priv->notifier); > > I would first cleanup() then unregister() even if they deal with two > different sets of asds (done and registred-but-not-yet-done ones). Looking at max9286_v4l2_async_register(), the v4l2_async_subdev_notifier_register() call is last. Therefore that would make it the first thing to cleanup in the reverse procedure? > >> +} >> + >> static int max9286_s_stream(struct v4l2_subdev *sd, int enable) >> { >> struct max9286_priv *priv = sd_to_max9286(sd); >> @@ -870,6 +913,13 @@ static int max9286_init(struct device *dev) >> goto err_regulator; >> } >> >> + /* Register v4l2 async notifiers */ >> + ret = max9286_v4l2_async_register(priv); >> + if (ret) { >> + dev_err(dev, "Unable to register V4L2 async notifiers\n"); >> + goto err_regulator; >> + } >> + >> v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops); >> priv->sd.internal_ops = &max9286_subdev_internal_ops; >> priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >> @@ -884,7 +934,7 @@ static int max9286_init(struct device *dev) >> priv->sd.ctrl_handler = &priv->ctrls; >> ret = priv->ctrls.error; >> if (ret) >> - goto err_regulator; >> + goto err_async; >> >> priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; >> >> @@ -927,6 +977,8 @@ static int max9286_init(struct device *dev) >> max9286_i2c_mux_close(priv); >> err_put_node: >> fwnode_handle_put(ep); >> +err_async: >> + max9286_v4l2_async_unregister(priv); >> err_regulator: >> regulator_disable(priv->regulator); >> priv->poc_enabled = false; >> @@ -938,14 +990,6 @@ static void max9286_cleanup_dt(struct max9286_priv *priv) >> { >> struct max9286_source *source; >> >> - /* >> - * Not strictly part of the DT, but the notifier is registered during >> - * max9286_parse_dt(), and the notifier holds references to the fwnodes >> - * thus the cleanup is here to mirror the registration. >> - */ >> - v4l2_async_notifier_unregister(&priv->notifier); >> - v4l2_async_notifier_cleanup(&priv->notifier); >> - >> for_each_source(priv, source) { >> fwnode_handle_put(source->fwnode); > > Wasn't this a double fwnode_handle_put() ? We called > notifier_cleanup() and then put all sources again manually. > > I don't see one more get() when the asd gets registered to the > notifier with v4l2_async_notifier_add_subdev() so I'm afraid this > would result in a duplicated put(). Am I wrong ? Agh, all the implicit transfers of ownerships for refcnting with fwnodes is horrible. I hadn't actually noticed that _notifier_cleanup() was doing fwnode_handle_puts() ... But indeed, all of that was pre-existing before this patch series. Not something I was looking to modify as part of this patch. Lets fix that issue on top. (which is going to get squashed in all the same, but I'm not going to change /this/ patch for it) > It was there already, but I think it happens in this patch as > well: if max9286_init() fails calls max9286_v4l2_unregister() which > then calls max9286_v4l2_async_unregister() which put all the > not-yet-completed subdevs by calling v4l2_async_notifier_cleanup(). > Then in the probe() function error path we then call > max9286_cleanup_dt() which puts again all the registered sources > regardless of their completed status. > I would call max9286_cleanup_dt() only if max9286_init() has not been > called yet. If we get to register subdevs to the notifier, I would > then provide a function that calls v4l2_async_notifier_cleanup() and > then manually puts all sources not yet registered. I'm afraid this > would need to keep a status flag in the source, unless you have a more > elegant solution. It seems like we also have the issue where we need to cleanup partially registered sources, (i.e. if some have registered, and some haven't) ... so how about a per-source flag to note that the device /got/ registered, and thus 'ownership' moved to V4L2 v4l2_async_notifier_cleanup() to _put(). Then we can maintain a single cleanup function still, and it will be handled on a per-node basis. -- KB > > Thanks > j > >> source->fwnode = NULL; >> @@ -958,7 +1002,6 @@ static int max9286_parse_dt(struct max9286_priv *priv) >> struct device_node *i2c_mux; >> struct device_node *node = NULL; >> unsigned int i2c_mux_mask = 0; >> - int ret; >> >> of_node_get(dev->of_node); >> i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux"); >> @@ -986,8 +1029,6 @@ static int max9286_parse_dt(struct max9286_priv *priv) >> of_node_put(node); >> of_node_put(i2c_mux); >> >> - v4l2_async_notifier_init(&priv->notifier); >> - >> /* Parse the endpoints */ >> for_each_endpoint_of_node(dev->of_node, node) { >> struct max9286_source *source; >> @@ -1056,34 +1097,14 @@ static int max9286_parse_dt(struct max9286_priv *priv) >> continue; >> } >> >> - source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; >> - source->asd.match.fwnode = source->fwnode; >> - >> - ret = v4l2_async_notifier_add_subdev(&priv->notifier, >> - &source->asd); >> - if (ret) { >> - v4l2_async_notifier_cleanup(&priv->notifier); >> - of_node_put(node); >> - return ret; >> - } >> - >> priv->source_mask |= BIT(ep.port); >> priv->nsources++; >> } >> of_node_put(node); >> >> - /* Do not register the subdev notifier if there are no devices. */ >> - if (!priv->nsources) >> - return 0; >> - >> priv->route_mask = priv->source_mask; >> - priv->notifier.ops = &max9286_notify_ops; >> - >> - ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier); >> - if (ret) >> - v4l2_async_notifier_cleanup(&priv->notifier); >> >> - return ret; >> + return 0; >> } >> >> static int max9286_probe(struct i2c_client *client) >> @@ -1182,6 +1203,7 @@ static int max9286_remove(struct i2c_client *client) >> >> fwnode_handle_put(priv->sd.fwnode); >> v4l2_async_unregister_subdev(&priv->sd); >> + max9286_v4l2_async_unregister(priv); >> >> if (priv->poc_enabled) >> regulator_disable(priv->regulator); >> -- >> 2.20.1 >>
Hi Jacopo, On 13/02/2020 10:07, Kieran Bingham wrote: > Hi Jacopo, > > On 13/02/2020 09:46, Jacopo Mondi wrote: >> Hi Kieran, >> very nice thanks for handling this > > :-) > >> Just a few minors > > :-s hehe > >> >> On Wed, Feb 12, 2020 at 05:37:26PM +0000, Kieran Bingham wrote: >>> Move all the V4L2 Subdev Async registration so that it can only happen once >>> we know we will not need to -EPROBE_DEFER... >>> >>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >>> --- >>> drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++-------------- >>> 1 file changed, 55 insertions(+), 33 deletions(-) >>> >>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c >>> index 1b4ff3533795..03c5fa232b6d 100644 >>> --- a/drivers/media/i2c/max9286.c >>> +++ b/drivers/media/i2c/max9286.c >>> @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = { >>> .unbind = max9286_notify_unbind, >>> }; >>> >>> +static int max9286_v4l2_async_register(struct max9286_priv *priv) >> >> Could you capture in the function name this actually deals with >> notifiers ? Like max9286_notifier_register() or similar... > > I'd like to keep the 'v4l2' in there somewhere... > > max9286_v4l2_notifier_register() ? > > But then maybe even that could be confused with the notifiers/async > handling for the max9286 itself. > > My aim was that max9286_v4l2_async_{un,}register() dealt with subdevices > connected to the max9286 only ... > > For ~20 lines of code, it could be inlined up a level into > max9286_v4l2_register() but I do perhaps like trying to keep the > symmetry clean somehow. > > <scratch that - below tells me not to inline> > > >>> +{ >>> + struct device *dev = &priv->client->dev; >>> + struct max9286_source *source = NULL; >>> + int ret; >>> + >> >> Do we want to init and register the notifier if there are no sources >> connected ? I would keep >> >> if (!priv->nsources) >> return 0; >> >> here or in the caller. > > Ah yes, I had thought about that but clearly not acted upon it much. > > If we know there is nothing to notify us, I guess we won't expect any > need to register the notifications! > > Although this would certainly mean keeping the sources registration in > their own functions. > > >> >>> + v4l2_async_notifier_init(&priv->notifier); >>> + >>> + for_each_source(priv, source) { >>> + unsigned int i = to_index(priv, source); >>> + >>> + dev_err(dev, "Registering v4l2-async for source %d\n", i); >> >> dev_err ? >> > > Already removed, left over debug print. > > >>> + >>> + source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; >>> + source->asd.match.fwnode = source->fwnode; >>> + >>> + ret = v4l2_async_notifier_add_subdev(&priv->notifier, >>> + &source->asd); >>> + if (ret) { >>> + dev_err(dev, "Failed to add subdev for source %d", i); >>> + v4l2_async_notifier_cleanup(&priv->notifier); >> >> v4l2_async_notifier_cleanup() does fwnode_handle_put() on the async >> subdevs registered to the notifier but not yet completed. All the other >> sources have to be put as well I think. >> >> How to do so might be not nice, as you would need to keep track of which >> sources have been registered to the notifier already and put the other >> ones in the error path. > > Or can we move all fwnode refcounting back to cleanup_dt perhaps? > > I'll have a look. > >> >>> + return ret; >>> + } >>> + } >>> + >>> + priv->notifier.ops = &max9286_notify_ops; >>> + >>> + ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier); >>> + if (ret) { >>> + dev_err(dev, "Failed to register subdev_notifier"); >>> + v4l2_async_notifier_cleanup(&priv->notifier); >> >> Here it's fine to call notifier_cleanup() >> >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static void max9286_v4l2_async_unregister(struct max9286_priv *priv) >>> +{ >>> + v4l2_async_notifier_unregister(&priv->notifier); >>> + v4l2_async_notifier_cleanup(&priv->notifier); >> >> I would first cleanup() then unregister() even if they deal with two >> different sets of asds (done and registred-but-not-yet-done ones). > > > Looking at max9286_v4l2_async_register(), the > v4l2_async_subdev_notifier_register() call is last. > > Therefore that would make it the first thing to cleanup in the reverse > procedure? > > >> >>> +} >>> + >>> static int max9286_s_stream(struct v4l2_subdev *sd, int enable) >>> { >>> struct max9286_priv *priv = sd_to_max9286(sd); >>> @@ -870,6 +913,13 @@ static int max9286_init(struct device *dev) >>> goto err_regulator; >>> } >>> >>> + /* Register v4l2 async notifiers */ >>> + ret = max9286_v4l2_async_register(priv); >>> + if (ret) { >>> + dev_err(dev, "Unable to register V4L2 async notifiers\n"); >>> + goto err_regulator; >>> + } >>> + >>> v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops); >>> priv->sd.internal_ops = &max9286_subdev_internal_ops; >>> priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >>> @@ -884,7 +934,7 @@ static int max9286_init(struct device *dev) >>> priv->sd.ctrl_handler = &priv->ctrls; >>> ret = priv->ctrls.error; >>> if (ret) >>> - goto err_regulator; >>> + goto err_async; >>> >>> priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; >>> >>> @@ -927,6 +977,8 @@ static int max9286_init(struct device *dev) >>> max9286_i2c_mux_close(priv); >>> err_put_node: >>> fwnode_handle_put(ep); >>> +err_async: >>> + max9286_v4l2_async_unregister(priv); >>> err_regulator: >>> regulator_disable(priv->regulator); >>> priv->poc_enabled = false; >>> @@ -938,14 +990,6 @@ static void max9286_cleanup_dt(struct max9286_priv *priv) >>> { >>> struct max9286_source *source; >>> >>> - /* >>> - * Not strictly part of the DT, but the notifier is registered during >>> - * max9286_parse_dt(), and the notifier holds references to the fwnodes >>> - * thus the cleanup is here to mirror the registration. >>> - */ >>> - v4l2_async_notifier_unregister(&priv->notifier); >>> - v4l2_async_notifier_cleanup(&priv->notifier); >>> - >>> for_each_source(priv, source) { >>> fwnode_handle_put(source->fwnode); >> >> Wasn't this a double fwnode_handle_put() ? We called >> notifier_cleanup() and then put all sources again manually. >> >> I don't see one more get() when the asd gets registered to the >> notifier with v4l2_async_notifier_add_subdev() so I'm afraid this >> would result in a duplicated put(). Am I wrong ? > > Agh, all the implicit transfers of ownerships for refcnting with fwnodes > is horrible. > > I hadn't actually noticed that _notifier_cleanup() was doing > fwnode_handle_puts() ... > > But indeed, all of that was pre-existing before this patch series. Not > something I was looking to modify as part of this patch. > > Lets fix that issue on top. > > (which is going to get squashed in all the same, but I'm not going to > change /this/ patch for it) Perhaps the simplest option in this case actually is to do a fwnode_handle_get() on a successful async_notifier_add_subdev(). That will keep the refcnting balanced. >> It was there already, but I think it happens in this patch as >> well: if max9286_init() fails calls max9286_v4l2_unregister() which >> then calls max9286_v4l2_async_unregister() which put all the >> not-yet-completed subdevs by calling v4l2_async_notifier_cleanup(). >> Then in the probe() function error path we then call >> max9286_cleanup_dt() which puts again all the registered sources >> regardless of their completed status. > >> I would call max9286_cleanup_dt() only if max9286_init() has not been >> called yet. If we get to register subdevs to the notifier, I would >> then provide a function that calls v4l2_async_notifier_cleanup() and >> then manually puts all sources not yet registered. I'm afraid this >> would need to keep a status flag in the source, unless you have a more >> elegant solution. > > It seems like we also have the issue where we need to cleanup partially > registered sources, (i.e. if some have registered, and some haven't) ... > so how about a per-source flag to note that the device /got/ registered, > and thus 'ownership' moved to V4L2 v4l2_async_notifier_cleanup() to _put(). > > Then we can maintain a single cleanup function still, and it will be > handled on a per-node basis. And I think the extra 'fwnode_handle_get()' would resolve this too? > > -- > KB > > > >> >> Thanks >> j >> >>> source->fwnode = NULL; >>> @@ -958,7 +1002,6 @@ static int max9286_parse_dt(struct max9286_priv *priv) >>> struct device_node *i2c_mux; >>> struct device_node *node = NULL; >>> unsigned int i2c_mux_mask = 0; >>> - int ret; >>> >>> of_node_get(dev->of_node); >>> i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux"); >>> @@ -986,8 +1029,6 @@ static int max9286_parse_dt(struct max9286_priv *priv) >>> of_node_put(node); >>> of_node_put(i2c_mux); >>> >>> - v4l2_async_notifier_init(&priv->notifier); >>> - >>> /* Parse the endpoints */ >>> for_each_endpoint_of_node(dev->of_node, node) { >>> struct max9286_source *source; >>> @@ -1056,34 +1097,14 @@ static int max9286_parse_dt(struct max9286_priv *priv) >>> continue; >>> } >>> >>> - source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; >>> - source->asd.match.fwnode = source->fwnode; >>> - >>> - ret = v4l2_async_notifier_add_subdev(&priv->notifier, >>> - &source->asd); >>> - if (ret) { >>> - v4l2_async_notifier_cleanup(&priv->notifier); >>> - of_node_put(node); >>> - return ret; >>> - } >>> - >>> priv->source_mask |= BIT(ep.port); >>> priv->nsources++; >>> } >>> of_node_put(node); >>> >>> - /* Do not register the subdev notifier if there are no devices. */ >>> - if (!priv->nsources) >>> - return 0; >>> - >>> priv->route_mask = priv->source_mask; >>> - priv->notifier.ops = &max9286_notify_ops; >>> - >>> - ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier); >>> - if (ret) >>> - v4l2_async_notifier_cleanup(&priv->notifier); >>> >>> - return ret; >>> + return 0; >>> } >>> >>> static int max9286_probe(struct i2c_client *client) >>> @@ -1182,6 +1203,7 @@ static int max9286_remove(struct i2c_client *client) >>> >>> fwnode_handle_put(priv->sd.fwnode); >>> v4l2_async_unregister_subdev(&priv->sd); >>> + max9286_v4l2_async_unregister(priv); >>> >>> if (priv->poc_enabled) >>> regulator_disable(priv->regulator); >>> -- >>> 2.20.1 >>> >
Hi Kieran, On Thu, Feb 13, 2020 at 10:07:18AM +0000, Kieran Bingham wrote: > Hi Jacopo, > > On 13/02/2020 09:46, Jacopo Mondi wrote: > > Hi Kieran, > > very nice thanks for handling this > > :-) > > > Just a few minors > > :-s hehe > Turned out to be lengthier than expected :) > > > > On Wed, Feb 12, 2020 at 05:37:26PM +0000, Kieran Bingham wrote: > >> Move all the V4L2 Subdev Async registration so that it can only happen once > >> we know we will not need to -EPROBE_DEFER... > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >> --- > >> drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++-------------- > >> 1 file changed, 55 insertions(+), 33 deletions(-) > >> > >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > >> index 1b4ff3533795..03c5fa232b6d 100644 > >> --- a/drivers/media/i2c/max9286.c > >> +++ b/drivers/media/i2c/max9286.c > >> @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = { > >> .unbind = max9286_notify_unbind, > >> }; > >> > >> +static int max9286_v4l2_async_register(struct max9286_priv *priv) > > > > Could you capture in the function name this actually deals with > > notifiers ? Like max9286_notifier_register() or similar... > > I'd like to keep the 'v4l2' in there somewhere... > > max9286_v4l2_notifier_register() ? > > But then maybe even that could be confused with the notifiers/async > handling for the max9286 itself. > > My aim was that max9286_v4l2_async_{un,}register() dealt with subdevices > connected to the max9286 only ... To me async_register() calls for dealing with registering our own subdev to the async framework, not collecting remote asds and adding it our subnotifier. As you wish, it's really just a suggestion. > > For ~20 lines of code, it could be inlined up a level into > max9286_v4l2_register() but I do perhaps like trying to keep the > symmetry clean somehow. > > <scratch that - below tells me not to inline> > > > >> +{ > >> + struct device *dev = &priv->client->dev; > >> + struct max9286_source *source = NULL; > >> + int ret; > >> + > > > > Do we want to init and register the notifier if there are no sources > > connected ? I would keep > > > > if (!priv->nsources) > > return 0; > > > > here or in the caller. > > Ah yes, I had thought about that but clearly not acted upon it much. > > If we know there is nothing to notify us, I guess we won't expect any > need to register the notifications! > > Although this would certainly mean keeping the sources registration in > their own functions. > > > > > >> + v4l2_async_notifier_init(&priv->notifier); > >> + > >> + for_each_source(priv, source) { > >> + unsigned int i = to_index(priv, source); > >> + > >> + dev_err(dev, "Registering v4l2-async for source %d\n", i); > > > > dev_err ? > > > > Already removed, left over debug print. > > > >> + > >> + source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; > >> + source->asd.match.fwnode = source->fwnode; > >> + > >> + ret = v4l2_async_notifier_add_subdev(&priv->notifier, > >> + &source->asd); > >> + if (ret) { > >> + dev_err(dev, "Failed to add subdev for source %d", i); > >> + v4l2_async_notifier_cleanup(&priv->notifier); > > > > v4l2_async_notifier_cleanup() does fwnode_handle_put() on the async > > subdevs registered to the notifier but not yet completed. All the other > > sources have to be put as well I think. > > > > How to do so might be not nice, as you would need to keep track of which > > sources have been registered to the notifier already and put the other > > ones in the error path. > > Or can we move all fwnode refcounting back to cleanup_dt perhaps? > > I'll have a look. > > > > >> + return ret; > >> + } > >> + } > >> + > >> + priv->notifier.ops = &max9286_notify_ops; > >> + > >> + ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier); > >> + if (ret) { > >> + dev_err(dev, "Failed to register subdev_notifier"); > >> + v4l2_async_notifier_cleanup(&priv->notifier); > > > > Here it's fine to call notifier_cleanup() > > > >> + return ret; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static void max9286_v4l2_async_unregister(struct max9286_priv *priv) > >> +{ > >> + v4l2_async_notifier_unregister(&priv->notifier); > >> + v4l2_async_notifier_cleanup(&priv->notifier); > > > > I would first cleanup() then unregister() even if they deal with two > > different sets of asds (done and registred-but-not-yet-done ones). > > > Looking at max9286_v4l2_async_register(), the > v4l2_async_subdev_notifier_register() call is last. > > Therefore that would make it the first thing to cleanup in the reverse > procedure? > Ok then, reverse order for symmetry is fine. > > > > >> +} > >> + > >> static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > >> { > >> struct max9286_priv *priv = sd_to_max9286(sd); > >> @@ -870,6 +913,13 @@ static int max9286_init(struct device *dev) > >> goto err_regulator; > >> } > >> > >> + /* Register v4l2 async notifiers */ > >> + ret = max9286_v4l2_async_register(priv); > >> + if (ret) { > >> + dev_err(dev, "Unable to register V4L2 async notifiers\n"); > >> + goto err_regulator; > >> + } > >> + > >> v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops); > >> priv->sd.internal_ops = &max9286_subdev_internal_ops; > >> priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > >> @@ -884,7 +934,7 @@ static int max9286_init(struct device *dev) > >> priv->sd.ctrl_handler = &priv->ctrls; > >> ret = priv->ctrls.error; > >> if (ret) > >> - goto err_regulator; > >> + goto err_async; > >> > >> priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > >> > >> @@ -927,6 +977,8 @@ static int max9286_init(struct device *dev) > >> max9286_i2c_mux_close(priv); > >> err_put_node: > >> fwnode_handle_put(ep); > >> +err_async: > >> + max9286_v4l2_async_unregister(priv); > >> err_regulator: > >> regulator_disable(priv->regulator); > >> priv->poc_enabled = false; > >> @@ -938,14 +990,6 @@ static void max9286_cleanup_dt(struct max9286_priv *priv) > >> { > >> struct max9286_source *source; > >> > >> - /* > >> - * Not strictly part of the DT, but the notifier is registered during > >> - * max9286_parse_dt(), and the notifier holds references to the fwnodes > >> - * thus the cleanup is here to mirror the registration. > >> - */ > >> - v4l2_async_notifier_unregister(&priv->notifier); > >> - v4l2_async_notifier_cleanup(&priv->notifier); > >> - > >> for_each_source(priv, source) { > >> fwnode_handle_put(source->fwnode); > > > > Wasn't this a double fwnode_handle_put() ? We called > > notifier_cleanup() and then put all sources again manually. > > > > I don't see one more get() when the asd gets registered to the > > notifier with v4l2_async_notifier_add_subdev() so I'm afraid this > > would result in a duplicated put(). Am I wrong ? > > Agh, all the implicit transfers of ownerships for refcnting with fwnodes > is horrible. > Yup :( > I hadn't actually noticed that _notifier_cleanup() was doing > fwnode_handle_puts() ... > > But indeed, all of that was pre-existing before this patch series. Not > something I was looking to modify as part of this patch. > > Lets fix that issue on top. > Fine with me > (which is going to get squashed in all the same, but I'm not going to > change /this/ patch for it) > > > > It was there already, but I think it happens in this patch as > > well: if max9286_init() fails calls max9286_v4l2_unregister() which > > then calls max9286_v4l2_async_unregister() which put all the > > not-yet-completed subdevs by calling v4l2_async_notifier_cleanup(). > > Then in the probe() function error path we then call > > max9286_cleanup_dt() which puts again all the registered sources > > regardless of their completed status. > > > I would call max9286_cleanup_dt() only if max9286_init() has not been > > called yet. If we get to register subdevs to the notifier, I would > > then provide a function that calls v4l2_async_notifier_cleanup() and > > then manually puts all sources not yet registered. I'm afraid this > > would need to keep a status flag in the source, unless you have a more > > elegant solution. > > It seems like we also have the issue where we need to cleanup partially > registered sources, (i.e. if some have registered, and some haven't) ... > so how about a per-source flag to note that the device /got/ registered, > and thus 'ownership' moved to V4L2 v4l2_async_notifier_cleanup() to _put(). > > Then we can maintain a single cleanup function still, and it will be > handled on a per-node basis. That was my suggestion but indeed requires some state keeping in place, so I hoped we could come up with something more elegant :) But indeed, keeping a flag in the source to tell ownership has been passed to the async framework would probably be enough, and would allow us to possibly have a single cleanup function, yes. > > -- > KB > > > > > > > Thanks > > j > > > >> source->fwnode = NULL; > >> @@ -958,7 +1002,6 @@ static int max9286_parse_dt(struct max9286_priv *priv) > >> struct device_node *i2c_mux; > >> struct device_node *node = NULL; > >> unsigned int i2c_mux_mask = 0; > >> - int ret; > >> > >> of_node_get(dev->of_node); > >> i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux"); > >> @@ -986,8 +1029,6 @@ static int max9286_parse_dt(struct max9286_priv *priv) > >> of_node_put(node); > >> of_node_put(i2c_mux); > >> > >> - v4l2_async_notifier_init(&priv->notifier); > >> - > >> /* Parse the endpoints */ > >> for_each_endpoint_of_node(dev->of_node, node) { > >> struct max9286_source *source; > >> @@ -1056,34 +1097,14 @@ static int max9286_parse_dt(struct max9286_priv *priv) > >> continue; > >> } > >> > >> - source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; > >> - source->asd.match.fwnode = source->fwnode; > >> - > >> - ret = v4l2_async_notifier_add_subdev(&priv->notifier, > >> - &source->asd); > >> - if (ret) { > >> - v4l2_async_notifier_cleanup(&priv->notifier); > >> - of_node_put(node); > >> - return ret; > >> - } > >> - > >> priv->source_mask |= BIT(ep.port); > >> priv->nsources++; > >> } > >> of_node_put(node); > >> > >> - /* Do not register the subdev notifier if there are no devices. */ > >> - if (!priv->nsources) > >> - return 0; > >> - > >> priv->route_mask = priv->source_mask; > >> - priv->notifier.ops = &max9286_notify_ops; > >> - > >> - ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier); > >> - if (ret) > >> - v4l2_async_notifier_cleanup(&priv->notifier); > >> > >> - return ret; > >> + return 0; > >> } > >> > >> static int max9286_probe(struct i2c_client *client) > >> @@ -1182,6 +1203,7 @@ static int max9286_remove(struct i2c_client *client) > >> > >> fwnode_handle_put(priv->sd.fwnode); > >> v4l2_async_unregister_subdev(&priv->sd); > >> + max9286_v4l2_async_unregister(priv); > >> > >> if (priv->poc_enabled) > >> regulator_disable(priv->regulator); > >> -- > >> 2.20.1 > >> >
On 13/02/2020 10:20, Jacopo Mondi wrote: > Hi Kieran, > > On Thu, Feb 13, 2020 at 10:07:18AM +0000, Kieran Bingham wrote: >> Hi Jacopo, >> >> On 13/02/2020 09:46, Jacopo Mondi wrote: >>> Hi Kieran, >>> very nice thanks for handling this >> >> :-) >> >>> Just a few minors >> >> :-s hehe >> > > Turned out to be lengthier than expected :) > >>> >>> On Wed, Feb 12, 2020 at 05:37:26PM +0000, Kieran Bingham wrote: >>>> Move all the V4L2 Subdev Async registration so that it can only happen once >>>> we know we will not need to -EPROBE_DEFER... >>>> >>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >>>> --- >>>> drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++-------------- >>>> 1 file changed, 55 insertions(+), 33 deletions(-) >>>> >>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c >>>> index 1b4ff3533795..03c5fa232b6d 100644 >>>> --- a/drivers/media/i2c/max9286.c >>>> +++ b/drivers/media/i2c/max9286.c >>>> @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = { >>>> .unbind = max9286_notify_unbind, >>>> }; >>>> >>>> +static int max9286_v4l2_async_register(struct max9286_priv *priv) >>> >>> Could you capture in the function name this actually deals with >>> notifiers ? Like max9286_notifier_register() or similar... >> >> I'd like to keep the 'v4l2' in there somewhere... >> >> max9286_v4l2_notifier_register() ? >> >> But then maybe even that could be confused with the notifiers/async >> handling for the max9286 itself. >> >> My aim was that max9286_v4l2_async_{un,}register() dealt with subdevices >> connected to the max9286 only ... > > To me async_register() calls for dealing with registering our own > subdev to the async framework, not collecting remote asds and adding > it our subnotifier. As you wish, it's really just a suggestion. So would you like to see the async registration between the max9286 and the connected CSI2 receiver handled in this pair of functions too? That would mean moveing the subdevice registrations later too - to after all of the max9286 device creations but I think that's fine. As long as the case between "if (priv->nsources)" is handled correctly between the max9286 subdevices and the parent device it should be fine. >> For ~20 lines of code, it could be inlined up a level into >> max9286_v4l2_register() but I do perhaps like trying to keep the >> symmetry clean somehow. >> >> <scratch that - below tells me not to inline> >> >> >>>> +{ >>>> + struct device *dev = &priv->client->dev; >>>> + struct max9286_source *source = NULL; >>>> + int ret; >>>> + >>> >>> Do we want to init and register the notifier if there are no sources >>> connected ? I would keep >>> >>> if (!priv->nsources) >>> return 0; >>> >>> here or in the caller. >> >> Ah yes, I had thought about that but clearly not acted upon it much. >> >> If we know there is nothing to notify us, I guess we won't expect any >> need to register the notifications! >> >> Although this would certainly mean keeping the sources registration in >> their own functions. >> >> >>> >>>> + v4l2_async_notifier_init(&priv->notifier); >>>> + >>>> + for_each_source(priv, source) { >>>> + unsigned int i = to_index(priv, source); >>>> + >>>> + dev_err(dev, "Registering v4l2-async for source %d\n", i); >>> >>> dev_err ? >>> >> >> Already removed, left over debug print. >> >> >>>> + >>>> + source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; >>>> + source->asd.match.fwnode = source->fwnode; >>>> + >>>> + ret = v4l2_async_notifier_add_subdev(&priv->notifier, >>>> + &source->asd); >>>> + if (ret) { >>>> + dev_err(dev, "Failed to add subdev for source %d", i); >>>> + v4l2_async_notifier_cleanup(&priv->notifier); >>> >>> v4l2_async_notifier_cleanup() does fwnode_handle_put() on the async >>> subdevs registered to the notifier but not yet completed. All the other >>> sources have to be put as well I think. >>> >>> How to do so might be not nice, as you would need to keep track of which >>> sources have been registered to the notifier already and put the other >>> ones in the error path. >> >> Or can we move all fwnode refcounting back to cleanup_dt perhaps? >> >> I'll have a look. >> >>> >>>> + return ret; >>>> + } >>>> + } >>>> + >>>> + priv->notifier.ops = &max9286_notify_ops; >>>> + >>>> + ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier); >>>> + if (ret) { >>>> + dev_err(dev, "Failed to register subdev_notifier"); >>>> + v4l2_async_notifier_cleanup(&priv->notifier); >>> >>> Here it's fine to call notifier_cleanup() >>> >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void max9286_v4l2_async_unregister(struct max9286_priv *priv) >>>> +{ >>>> + v4l2_async_notifier_unregister(&priv->notifier); >>>> + v4l2_async_notifier_cleanup(&priv->notifier); >>> >>> I would first cleanup() then unregister() even if they deal with two >>> different sets of asds (done and registred-but-not-yet-done ones). >> >> >> Looking at max9286_v4l2_async_register(), the >> v4l2_async_subdev_notifier_register() call is last. >> >> Therefore that would make it the first thing to cleanup in the reverse >> procedure? >> > > Ok then, reverse order for symmetry is fine. > >> >>> >>>> +} >>>> + >>>> static int max9286_s_stream(struct v4l2_subdev *sd, int enable) >>>> { >>>> struct max9286_priv *priv = sd_to_max9286(sd); >>>> @@ -870,6 +913,13 @@ static int max9286_init(struct device *dev) >>>> goto err_regulator; >>>> } >>>> >>>> + /* Register v4l2 async notifiers */ >>>> + ret = max9286_v4l2_async_register(priv); >>>> + if (ret) { >>>> + dev_err(dev, "Unable to register V4L2 async notifiers\n"); >>>> + goto err_regulator; >>>> + } >>>> + >>>> v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops); >>>> priv->sd.internal_ops = &max9286_subdev_internal_ops; >>>> priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >>>> @@ -884,7 +934,7 @@ static int max9286_init(struct device *dev) >>>> priv->sd.ctrl_handler = &priv->ctrls; >>>> ret = priv->ctrls.error; >>>> if (ret) >>>> - goto err_regulator; >>>> + goto err_async; >>>> >>>> priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; >>>> >>>> @@ -927,6 +977,8 @@ static int max9286_init(struct device *dev) >>>> max9286_i2c_mux_close(priv); >>>> err_put_node: >>>> fwnode_handle_put(ep); >>>> +err_async: >>>> + max9286_v4l2_async_unregister(priv); >>>> err_regulator: >>>> regulator_disable(priv->regulator); >>>> priv->poc_enabled = false; >>>> @@ -938,14 +990,6 @@ static void max9286_cleanup_dt(struct max9286_priv *priv) >>>> { >>>> struct max9286_source *source; >>>> >>>> - /* >>>> - * Not strictly part of the DT, but the notifier is registered during >>>> - * max9286_parse_dt(), and the notifier holds references to the fwnodes >>>> - * thus the cleanup is here to mirror the registration. >>>> - */ >>>> - v4l2_async_notifier_unregister(&priv->notifier); >>>> - v4l2_async_notifier_cleanup(&priv->notifier); >>>> - >>>> for_each_source(priv, source) { >>>> fwnode_handle_put(source->fwnode); >>> >>> Wasn't this a double fwnode_handle_put() ? We called >>> notifier_cleanup() and then put all sources again manually. >>> >>> I don't see one more get() when the asd gets registered to the >>> notifier with v4l2_async_notifier_add_subdev() so I'm afraid this >>> would result in a duplicated put(). Am I wrong ? >> >> Agh, all the implicit transfers of ownerships for refcnting with fwnodes >> is horrible. >> > > Yup :( > >> I hadn't actually noticed that _notifier_cleanup() was doing >> fwnode_handle_puts() ... >> >> But indeed, all of that was pre-existing before this patch series. Not >> something I was looking to modify as part of this patch. >> >> Lets fix that issue on top. >> > > Fine with me > >> (which is going to get squashed in all the same, but I'm not going to >> change /this/ patch for it) >> >> >>> It was there already, but I think it happens in this patch as >>> well: if max9286_init() fails calls max9286_v4l2_unregister() which >>> then calls max9286_v4l2_async_unregister() which put all the >>> not-yet-completed subdevs by calling v4l2_async_notifier_cleanup(). >>> Then in the probe() function error path we then call >>> max9286_cleanup_dt() which puts again all the registered sources >>> regardless of their completed status. >> >>> I would call max9286_cleanup_dt() only if max9286_init() has not been >>> called yet. If we get to register subdevs to the notifier, I would >>> then provide a function that calls v4l2_async_notifier_cleanup() and >>> then manually puts all sources not yet registered. I'm afraid this >>> would need to keep a status flag in the source, unless you have a more >>> elegant solution. >> >> It seems like we also have the issue where we need to cleanup partially >> registered sources, (i.e. if some have registered, and some haven't) ... >> so how about a per-source flag to note that the device /got/ registered, >> and thus 'ownership' moved to V4L2 v4l2_async_notifier_cleanup() to _put(). >> >> Then we can maintain a single cleanup function still, and it will be >> handled on a per-node basis. > > That was my suggestion but indeed requires some state keeping in > place, so I hoped we could come up with something more elegant :) > > But indeed, keeping a flag in the source to tell ownership has been > passed to the async framework would probably be enough, and would > allow us to possibly have a single cleanup function, yes. Hopefully my proposed patch to simply increase the refcnt on successfully added subdevs will do the trick. > >> >> -- >> KB >> >> >> >>> >>> Thanks >>> j >>> >>>> source->fwnode = NULL; >>>> @@ -958,7 +1002,6 @@ static int max9286_parse_dt(struct max9286_priv *priv) >>>> struct device_node *i2c_mux; >>>> struct device_node *node = NULL; >>>> unsigned int i2c_mux_mask = 0; >>>> - int ret; >>>> >>>> of_node_get(dev->of_node); >>>> i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux"); >>>> @@ -986,8 +1029,6 @@ static int max9286_parse_dt(struct max9286_priv *priv) >>>> of_node_put(node); >>>> of_node_put(i2c_mux); >>>> >>>> - v4l2_async_notifier_init(&priv->notifier); >>>> - >>>> /* Parse the endpoints */ >>>> for_each_endpoint_of_node(dev->of_node, node) { >>>> struct max9286_source *source; >>>> @@ -1056,34 +1097,14 @@ static int max9286_parse_dt(struct max9286_priv *priv) >>>> continue; >>>> } >>>> >>>> - source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; >>>> - source->asd.match.fwnode = source->fwnode; >>>> - >>>> - ret = v4l2_async_notifier_add_subdev(&priv->notifier, >>>> - &source->asd); >>>> - if (ret) { >>>> - v4l2_async_notifier_cleanup(&priv->notifier); >>>> - of_node_put(node); >>>> - return ret; >>>> - } >>>> - >>>> priv->source_mask |= BIT(ep.port); >>>> priv->nsources++; >>>> } >>>> of_node_put(node); >>>> >>>> - /* Do not register the subdev notifier if there are no devices. */ >>>> - if (!priv->nsources) >>>> - return 0; >>>> - >>>> priv->route_mask = priv->source_mask; >>>> - priv->notifier.ops = &max9286_notify_ops; >>>> - >>>> - ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier); >>>> - if (ret) >>>> - v4l2_async_notifier_cleanup(&priv->notifier); >>>> >>>> - return ret; >>>> + return 0; >>>> } >>>> >>>> static int max9286_probe(struct i2c_client *client) >>>> @@ -1182,6 +1203,7 @@ static int max9286_remove(struct i2c_client *client) >>>> >>>> fwnode_handle_put(priv->sd.fwnode); >>>> v4l2_async_unregister_subdev(&priv->sd); >>>> + max9286_v4l2_async_unregister(priv); >>>> >>>> if (priv->poc_enabled) >>>> regulator_disable(priv->regulator); >>>> -- >>>> 2.20.1 >>>> >>
Hi Kieran, On Thu, Feb 13, 2020 at 10:27:11AM +0000, Kieran Bingham wrote: > On 13/02/2020 10:20, Jacopo Mondi wrote: > > Hi Kieran, > > > > On Thu, Feb 13, 2020 at 10:07:18AM +0000, Kieran Bingham wrote: > >> Hi Jacopo, > >> > >> On 13/02/2020 09:46, Jacopo Mondi wrote: > >>> Hi Kieran, > >>> very nice thanks for handling this > >> > >> :-) > >> > >>> Just a few minors > >> > >> :-s hehe > >> > > > > Turned out to be lengthier than expected :) > > > >>> > >>> On Wed, Feb 12, 2020 at 05:37:26PM +0000, Kieran Bingham wrote: > >>>> Move all the V4L2 Subdev Async registration so that it can only happen once > >>>> we know we will not need to -EPROBE_DEFER... > >>>> > >>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >>>> --- > >>>> drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++-------------- > >>>> 1 file changed, 55 insertions(+), 33 deletions(-) > >>>> > >>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > >>>> index 1b4ff3533795..03c5fa232b6d 100644 > >>>> --- a/drivers/media/i2c/max9286.c > >>>> +++ b/drivers/media/i2c/max9286.c > >>>> @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = { > >>>> .unbind = max9286_notify_unbind, > >>>> }; > >>>> > >>>> +static int max9286_v4l2_async_register(struct max9286_priv *priv) > >>> > >>> Could you capture in the function name this actually deals with > >>> notifiers ? Like max9286_notifier_register() or similar... > >> > >> I'd like to keep the 'v4l2' in there somewhere... > >> > >> max9286_v4l2_notifier_register() ? > >> > >> But then maybe even that could be confused with the notifiers/async > >> handling for the max9286 itself. > >> > >> My aim was that max9286_v4l2_async_{un,}register() dealt with subdevices > >> connected to the max9286 only ... > > > > To me async_register() calls for dealing with registering our own > > subdev to the async framework, not collecting remote asds and adding > > it our subnotifier. As you wish, it's really just a suggestion. > > > So would you like to see the async registration between the max9286 and > the connected CSI2 receiver handled in this pair of functions too? > No no, I was just bikeshedding on function names, no worries :)
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index 1b4ff3533795..03c5fa232b6d 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -503,6 +503,49 @@ static const struct v4l2_async_notifier_operations max9286_notify_ops = { .unbind = max9286_notify_unbind, }; +static int max9286_v4l2_async_register(struct max9286_priv *priv) +{ + struct device *dev = &priv->client->dev; + struct max9286_source *source = NULL; + int ret; + + v4l2_async_notifier_init(&priv->notifier); + + for_each_source(priv, source) { + unsigned int i = to_index(priv, source); + + dev_err(dev, "Registering v4l2-async for source %d\n", i); + + source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; + source->asd.match.fwnode = source->fwnode; + + ret = v4l2_async_notifier_add_subdev(&priv->notifier, + &source->asd); + if (ret) { + dev_err(dev, "Failed to add subdev for source %d", i); + v4l2_async_notifier_cleanup(&priv->notifier); + return ret; + } + } + + priv->notifier.ops = &max9286_notify_ops; + + ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier); + if (ret) { + dev_err(dev, "Failed to register subdev_notifier"); + v4l2_async_notifier_cleanup(&priv->notifier); + return ret; + } + + return 0; +} + +static void max9286_v4l2_async_unregister(struct max9286_priv *priv) +{ + v4l2_async_notifier_unregister(&priv->notifier); + v4l2_async_notifier_cleanup(&priv->notifier); +} + static int max9286_s_stream(struct v4l2_subdev *sd, int enable) { struct max9286_priv *priv = sd_to_max9286(sd); @@ -870,6 +913,13 @@ static int max9286_init(struct device *dev) goto err_regulator; } + /* Register v4l2 async notifiers */ + ret = max9286_v4l2_async_register(priv); + if (ret) { + dev_err(dev, "Unable to register V4L2 async notifiers\n"); + goto err_regulator; + } + v4l2_i2c_subdev_init(&priv->sd, client, &max9286_subdev_ops); priv->sd.internal_ops = &max9286_subdev_internal_ops; priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; @@ -884,7 +934,7 @@ static int max9286_init(struct device *dev) priv->sd.ctrl_handler = &priv->ctrls; ret = priv->ctrls.error; if (ret) - goto err_regulator; + goto err_async; priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; @@ -927,6 +977,8 @@ static int max9286_init(struct device *dev) max9286_i2c_mux_close(priv); err_put_node: fwnode_handle_put(ep); +err_async: + max9286_v4l2_async_unregister(priv); err_regulator: regulator_disable(priv->regulator); priv->poc_enabled = false; @@ -938,14 +990,6 @@ static void max9286_cleanup_dt(struct max9286_priv *priv) { struct max9286_source *source; - /* - * Not strictly part of the DT, but the notifier is registered during - * max9286_parse_dt(), and the notifier holds references to the fwnodes - * thus the cleanup is here to mirror the registration. - */ - v4l2_async_notifier_unregister(&priv->notifier); - v4l2_async_notifier_cleanup(&priv->notifier); - for_each_source(priv, source) { fwnode_handle_put(source->fwnode); source->fwnode = NULL; @@ -958,7 +1002,6 @@ static int max9286_parse_dt(struct max9286_priv *priv) struct device_node *i2c_mux; struct device_node *node = NULL; unsigned int i2c_mux_mask = 0; - int ret; of_node_get(dev->of_node); i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux"); @@ -986,8 +1029,6 @@ static int max9286_parse_dt(struct max9286_priv *priv) of_node_put(node); of_node_put(i2c_mux); - v4l2_async_notifier_init(&priv->notifier); - /* Parse the endpoints */ for_each_endpoint_of_node(dev->of_node, node) { struct max9286_source *source; @@ -1056,34 +1097,14 @@ static int max9286_parse_dt(struct max9286_priv *priv) continue; } - source->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; - source->asd.match.fwnode = source->fwnode; - - ret = v4l2_async_notifier_add_subdev(&priv->notifier, - &source->asd); - if (ret) { - v4l2_async_notifier_cleanup(&priv->notifier); - of_node_put(node); - return ret; - } - priv->source_mask |= BIT(ep.port); priv->nsources++; } of_node_put(node); - /* Do not register the subdev notifier if there are no devices. */ - if (!priv->nsources) - return 0; - priv->route_mask = priv->source_mask; - priv->notifier.ops = &max9286_notify_ops; - - ret = v4l2_async_subdev_notifier_register(&priv->sd, &priv->notifier); - if (ret) - v4l2_async_notifier_cleanup(&priv->notifier); - return ret; + return 0; } static int max9286_probe(struct i2c_client *client) @@ -1182,6 +1203,7 @@ static int max9286_remove(struct i2c_client *client) fwnode_handle_put(priv->sd.fwnode); v4l2_async_unregister_subdev(&priv->sd); + max9286_v4l2_async_unregister(priv); if (priv->poc_enabled) regulator_disable(priv->regulator);
Move all the V4L2 Subdev Async registration so that it can only happen once we know we will not need to -EPROBE_DEFER... Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> --- drivers/media/i2c/max9286.c | 88 +++++++++++++++++++++++-------------- 1 file changed, 55 insertions(+), 33 deletions(-)