Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Jacopo Mondi <jacopo@jmondi.org>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: linux-media@vger.kernel.org, hverkuil@xs4all.nl,
	laurent.pinchart@ideasonboard.com, niklas.soderlund@ragnatech.se
Subject: Re: [PATCH v2 3/9] v4l2-async: Get fwnode reference when putting it to the notifier's list
Date: Fri, 14 Jun 2019 18:01:41 +0200
Message-ID: <20190614160141.mlw5imwejevunhou@uno.localdomain> (raw)
In-Reply-To: <20190606130225.10751-4-sakari.ailus@linux.intel.com>

[-- Attachment #1: Type: text/plain, Size: 8641 bytes --]

Hi Sakari,

On Thu, Jun 06, 2019 at 04:02:19PM +0300, Sakari Ailus wrote:
> The v4l2_async_notifier_add_fwnode_subdev() did not take a reference of
> the added fwnode, relying on the caller to handle that instead, in essence
> putting the fwnode to be added if there was an error.
>
> As the reference is eventually released during the notifier cleanup, this
> is not intuitive nor logical. Improve this by always getting a reference
> when the function succeeds, and the caller releasing the reference when it
> does not *itself* need it anymore.
>
> Luckily, perhaps, there were just a handful of callers using the function.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/platform/am437x/am437x-vpfe.c   |  5 ++---
>  drivers/media/platform/davinci/vpif_capture.c | 16 ++++++++--------
>  drivers/media/platform/qcom/camss/camss.c     |  2 +-
>  drivers/media/platform/xilinx/xilinx-vipp.c   |  2 +-
>  drivers/media/v4l2-core/v4l2-async.c          |  3 ++-
>  drivers/media/v4l2-core/v4l2-fwnode.c         | 23 ++++++-----------------
>  include/media/v4l2-async.h                    |  5 +++--
>  7 files changed, 23 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index db263c0ce48e..ccdbd9227955 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -2505,10 +2505,9 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
>  		pdata->asd[i] = v4l2_async_notifier_add_fwnode_subdev(
>  			&vpfe->notifier, of_fwnode_handle(rem),
>  			sizeof(struct v4l2_async_subdev));
> -		if (IS_ERR(pdata->asd[i])) {
> -			of_node_put(rem);
> +		of_node_put(rem);
> +		if (IS_ERR(pdata->asd[i]))
>  			goto cleanup;
> -		}
>  	}
>
>  	of_node_put(endpoint);
> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> index 8fdea45ae090..2a0c3f3fb443 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -1512,6 +1512,7 @@ static struct vpif_capture_config *
>  vpif_capture_get_pdata(struct platform_device *pdev)
>  {
>  	struct device_node *endpoint = NULL;
> +	struct device_node *rem = NULL, *rem_ep = NULL;
>  	struct vpif_capture_config *pdata;
>  	struct vpif_subdev_info *sdinfo;
>  	struct vpif_capture_chan_config *chan;
> @@ -1542,7 +1543,6 @@ vpif_capture_get_pdata(struct platform_device *pdev)
>
>  	for (i = 0; i < VPIF_CAPTURE_NUM_CHANNELS; i++) {
>  		struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> -		struct device_node *rem, *rem_ep;
>  		unsigned int flags;
>  		int err;
>
> @@ -1565,7 +1565,6 @@ vpif_capture_get_pdata(struct platform_device *pdev)
>  			of_node_put(rem_ep);
>  			goto done;
>  		}
> -		of_node_put(rem_ep);
>
>  		sdinfo = &pdata->subdev_info[i];
>  		chan = &pdata->chan_config[i];
> @@ -1573,10 +1572,8 @@ vpif_capture_get_pdata(struct platform_device *pdev)
>  					    VPIF_CAPTURE_NUM_CHANNELS,
>  					    sizeof(*chan->inputs),
>  					    GFP_KERNEL);
> -		if (!chan->inputs) {
> -			of_node_put(rem);
> +		if (!chan->inputs)
>  			goto err_cleanup;
> -		}
>
>  		chan->input_count++;
>  		chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;
> @@ -1588,6 +1585,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
>  		if (err) {
>  			dev_err(&pdev->dev, "Could not parse the endpoint\n");
>  			of_node_put(rem);
> +			of_node_put(rem_ep);
>  			goto done;
>  		}
>
> @@ -1608,11 +1606,11 @@ vpif_capture_get_pdata(struct platform_device *pdev)
>  		pdata->asd[i] = v4l2_async_notifier_add_fwnode_subdev(
>  			&vpif_obj.notifier, of_fwnode_handle(rem_ep),
>  			sizeof(struct v4l2_async_subdev));
> -		if (IS_ERR(pdata->asd[i])) {
> -			of_node_put(rem);
> +		if (IS_ERR(pdata->asd[i]))
>  			goto err_cleanup;
> -		}
>
> +		of_node_put(rem);
> +		of_node_put(rem_ep);
>  	}
>
>  done:
> @@ -1624,6 +1622,8 @@ vpif_capture_get_pdata(struct platform_device *pdev)
>  	return pdata;
>
>  err_cleanup:
> +	of_node_put(rem);
> +	of_node_put(rem_ep);
>  	of_node_put(endpoint);
>  	v4l2_async_notifier_cleanup(&vpif_obj.notifier);
>
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index a979f210f441..4ab4a47d34f3 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -486,9 +486,9 @@ static int camss_of_parse_ports(struct camss *camss)
>
>  		asd = v4l2_async_notifier_add_fwnode_subdev(
>  			&camss->notifier, remote, sizeof(*csd));
> +		fwnode_handle_put(remote);
>  		if (IS_ERR(asd)) {
>  			ret = PTR_ERR(asd);
> -			fwnode_handle_put(remote);
>  			goto err_cleanup;
>  		}
>
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
> index 41df417153bd..d788f4870a23 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -392,9 +392,9 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
>  		asd = v4l2_async_notifier_add_fwnode_subdev(
>  			&xdev->notifier, remote,
>  			sizeof(struct xvip_graph_entity));
> +		fwnode_handle_put(remote);
>  		if (IS_ERR(asd)) {
>  			ret = PTR_ERR(asd);
> -			fwnode_handle_put(remote);
>  			goto err_notifier_cleanup;
>  		}
>  	}
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 304969ff3191..dc4e83b4f6ba 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -596,10 +596,11 @@ v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
>  		return ERR_PTR(-ENOMEM);
>
>  	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> -	asd->match.fwnode = fwnode;
> +	asd->match.fwnode = fwnode_handle_get(fwnode);
>
>  	ret = v4l2_async_notifier_add_subdev(notifier, asd);
>  	if (ret) {
> +		fwnode_handle_put(fwnode);
>  		kfree(asd);
>  		return ERR_PTR(ret);
>  	}
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 810b6584b522..b48725824580 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -780,23 +780,17 @@ static int v4l2_fwnode_reference_parse(struct device *dev,
>  		asd = v4l2_async_notifier_add_fwnode_subdev(notifier,
>  							    args.fwnode,
>  							    sizeof(*asd));
> +		fwnode_handle_put(args.fwnode);
>  		if (IS_ERR(asd)) {
> -			ret = PTR_ERR(asd);
>  			/* not an error if asd already exists */
> -			if (ret == -EEXIST) {
> -				fwnode_handle_put(args.fwnode);
> +			if (PTR_ERR(asd) == -EEXIST)
>  				continue;
> -			}
>
> -			goto error;
> +			return PTR_ERR(asd);
>  		}
>  	}
>
>  	return 0;
> -
> -error:
> -	fwnode_handle_put(args.fwnode);
> -	return ret;
>  }
>
>  /*
> @@ -1085,22 +1079,17 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
>
>  		asd = v4l2_async_notifier_add_fwnode_subdev(notifier, fwnode,
>  							    sizeof(*asd));
> +		fwnode_handle_put(fwnode);
>  		if (IS_ERR(asd)) {
>  			/* not an error if asd already exists */
> -			if (PTR_ERR(asd) == -EEXIST) {
> -				fwnode_handle_put(fwnode);
> +			if (PTR_ERR(asd) == -EEXIST)
>  				continue;
> -			}
>
> -			goto error;
> +			return PTR_ERR(asd);
>  		}
>  	}
>
>  	return PTR_ERR(fwnode) == -ENOENT ? 0 : PTR_ERR(fwnode);
> -
> -error:
> -	fwnode_handle_put(fwnode);
> -	return PTR_ERR(fwnode);
>  }
>
>  int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 1497bda66c3b..b9141ffa188a 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -175,8 +175,9 @@ int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
>   *		     the driver's async sub-device struct, i.e. both
>   *		     begin at the same memory address.
>   *
> - * Allocate a fwnode-matched asd of size asd_struct_size, and add it
> - * to the notifiers @asd_list.
> + * Allocate a fwnode-matched asd of size asd_struct_size, and add it to the
> + * notifiers @asd_list. The function also gets a reference of the fwnode which
> + * is released later at notifier cleanup time.

I would add that, as a consequence, the caller need to put their local
references as soon as they don't need them anymore (possibly just
after calling this function).

That apart
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j

>   */
>  struct v4l2_async_subdev *
>  v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
> --
> 2.11.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-06 13:02 [PATCH v2 0/9] Rework V4L2 fwnode parsing; add defaults and avoid iteration Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 1/9] davinci-vpif: Don't dereference endpoint after putting it, fix refcounting Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 2/9] v4l2-async: Use endpoint node, not device node, for fwnode match Sakari Ailus
2019-06-14 15:45   ` Jacopo Mondi
2019-06-14 21:21   ` Niklas Söderlund
2019-06-21  9:19     ` Jacopo Mondi
2019-06-21 11:43       ` Niklas Söderlund
2019-06-06 13:02 ` [PATCH v2 3/9] v4l2-async: Get fwnode reference when putting it to the notifier's list Sakari Ailus
2019-06-14 16:01   ` Jacopo Mondi [this message]
2019-06-06 13:02 ` [PATCH v2 4/9] v4l2-async: Add v4l2_async_notifier_add_fwnode_remote_subdev Sakari Ailus
2019-06-14 16:14   ` Jacopo Mondi
2019-06-06 13:02 ` [PATCH v2 5/9] omap3isp: Rework OF endpoint parsing Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 6/9] v4l2-async: Safely clean up an uninitialised notifier Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 7/9] ipu3-cio2: Clean up notifier's subdev list if parsing endpoints fails Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 8/9] ipu3-cio2: Proceed with notifier init even if there are no subdevs Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 9/9] ipu3-cio2: Parse information from firmware without using callbacks Sakari Ailus
2019-06-14 16:31   ` Jacopo Mondi

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190614160141.mlw5imwejevunhou@uno.localdomain \
    --to=jacopo@jmondi.org \
    --cc=hverkuil@xs4all.nl \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=niklas.soderlund@ragnatech.se \
    --cc=sakari.ailus@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org linux-media@archiver.kernel.org
	public-inbox-index linux-media


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/ public-inbox