Re: [PATCH 1/7] cec-notifier: add cec_notifier_find_hdmi_dev helper

[Thierry Reding <treding@nvidia.com>]

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

On Mon, Apr 08, 2019 at 01:03:55PM +0200, Hans Verkuil wrote:
> Add helper function to parse the DT for the hdmi-phandle property
> and find the corresponding struct device pointer.
> 
> It takes care to avoid increasing the device refcount since all
> we need is the device pointer. This pointer is used in the
> notifier list as a key, but it is never accessed by the CEC driver.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reported-by: Wen Yang <wen.yang99@zte.com.cn>
> ---
>  drivers/media/cec/cec-notifier.c | 24 ++++++++++++++++++++++++
>  include/media/cec-notifier.h     | 19 ++++++++++++++++++-
>  2 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
> index dd2078b27a41..870b3788e9f7 100644
> --- a/drivers/media/cec/cec-notifier.c
> +++ b/drivers/media/cec/cec-notifier.c
> @@ -11,6 +11,7 @@
>  #include <linux/slab.h>
>  #include <linux/list.h>
>  #include <linux/kref.h>
> +#include <linux/of_platform.h>
>  
>  #include <media/cec.h>
>  #include <media/cec-notifier.h>
> @@ -127,3 +128,26 @@ void cec_notifier_unregister(struct cec_notifier *n)
>  	cec_notifier_put(n);
>  }
>  EXPORT_SYMBOL_GPL(cec_notifier_unregister);
> +
> +struct device *cec_notifier_find_hdmi_dev(struct device *dev)

This doesn't really do anything with notifiers, so perhaps rename this
to something more independent?

> +{
> +	struct platform_device *hdmi_pdev;
> +	struct device *hdmi_dev = NULL;
> +	struct device_node *np;
> +
> +	np = of_parse_phandle(dev->of_node, "hdmi-phandle", 0);
> +
> +	if (!np) {
> +		dev_err(dev, "Failed to find hdmi node in device tree\n");

Perhaps "... to find HDMI node..."? HDMI here doesn't refer to a name,
so I think it's better to use the capitalized abbreviation to avoid any
potential confusion.

> +		return ERR_PTR(-ENODEV);
> +	}
> +	hdmi_pdev = of_find_device_by_node(np);
> +	of_node_put(np);
> +	if (hdmi_pdev) {
> +		hdmi_dev = &hdmi_pdev->dev;
> +		put_device(hdmi_dev);

Don't you need to hold onto that reference and pass it to the caller?
Otherwise somebody could be dropping the last reference to the struct
device backing the HDMI device after this call finishes.

In conjunction with the above comment, perhaps it'd be clearer if we had
a cec_hdmi_get()/cec_hdmi_put() pair of functions so that callers can
explicitly manage the reference count.

Thierry

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