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@xxxxxxxxxxxxxxxx>
>>> ---
>>> 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
>>>
>
