[PATCH 0/7] cec: introduce cec_notifier_find_hdmi_dev helper

[PATCH 0/7] cec: introduce cec_notifier_find_hdmi_dev helper

[Hans Verkuil]

Wen Yang reported that some CEC drivers incremented the refcount
of the HDMI device, but never decremented it, potentially leading
to memory leaks.

Rather than fixing each driver I decided to create a helper function
that finds the HDMI device and handles the refcounting correctly.

Two drivers (seco-cec and cros-ec-cec) still required a manual patch.

Regards,

	Hans

Hans Verkuil (7):
  cec-notifier: add cec_notifier_find_hdmi_dev helper
  meson: ao-cec: use new cec_notifier_find_hdmi_dev helper
  s5p_cec: use new cec_notifier_find_hdmi_dev helper
  stih_cec: use new cec_notifier_find_hdmi_dev helper
  tegra_cec: use new cec_notifier_find_hdmi_dev helper
  seco-cec: decrement HDMI device refcount
  cros-ec-cec: decrement HDMI device refcount

 drivers/media/cec/cec-notifier.c              | 24 +++++++++++++++++++
 .../media/platform/cros-ec-cec/cros-ec-cec.c  |  1 +
 drivers/media/platform/meson/ao-cec.c         | 16 ++++---------
 drivers/media/platform/s5p-cec/s5p_cec.c      | 16 ++++---------
 drivers/media/platform/seco-cec/seco-cec.c    |  1 +
 drivers/media/platform/sti/cec/stih-cec.c     | 21 ++++++----------
 drivers/media/platform/tegra-cec/tegra_cec.c  | 14 ++++-------
 include/media/cec-notifier.h                  | 19 ++++++++++++++-
 8 files changed, 65 insertions(+), 47 deletions(-)

-- 
2.20.1

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

[Hans Verkuil]

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)
+{
+	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");
+		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);
+		return hdmi_dev;
+	}
+	return ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL_GPL(cec_notifier_find_hdmi_dev);
diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
index 814eeef35a5c..ff4d6e0ea3d4 100644
--- a/include/media/cec-notifier.h
+++ b/include/media/cec-notifier.h
@@ -9,7 +9,7 @@
 #ifndef LINUX_CEC_NOTIFIER_H
 #define LINUX_CEC_NOTIFIER_H
 
-#include <linux/types.h>
+#include <linux/err.h>
 #include <media/cec.h>
 
 struct device;
@@ -87,6 +87,17 @@ void cec_notifier_unregister(struct cec_notifier *n);
 void cec_register_cec_notifier(struct cec_adapter *adap,
 			       struct cec_notifier *notifier);
 
+/**
+ * cec_notifier_find_hdmi_dev - find the hdmi device from "hdmi-phandle"
+ * @dev: the device with the "hdmi-phandle" device tree property
+ *
+ * Returns the device pointer referenced by the "hdmi-phandle" property.
+ * Note that the refcount of the returned device is not incremented.
+ * This device pointer is only used as a key value in the notifier
+ * list, but it is never accessed by the CEC driver.
+ */
+struct device *cec_notifier_find_hdmi_dev(struct device *dev);
+
 #else
 static inline struct cec_notifier *cec_notifier_get_conn(struct device *dev,
 							 const char *conn)
@@ -122,6 +133,12 @@ static inline void cec_register_cec_notifier(struct cec_adapter *adap,
 					     struct cec_notifier *notifier)
 {
 }
+
+static inline struct device *cec_notifier_find_hdmi_dev(struct device *dev)
+{
+	return ERR_PTR(-ENODEV);
+}
+
 #endif
 
 /**
-- 
2.20.1

[PATCH 2/7] meson: ao-cec: use new cec_notifier_find_hdmi_dev helper

[Hans Verkuil]

The meson CEC driver increased the HDMI device refcount when
it shouldn't. Use the new helper function to ensure that that
doesn't happen and to simplify the driver code.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/meson/ao-cec.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/meson/ao-cec.c b/drivers/media/platform/meson/ao-cec.c
index cd4be38ab5ac..c8b374741a7c 100644
--- a/drivers/media/platform/meson/ao-cec.c
+++ b/drivers/media/platform/meson/ao-cec.c
@@ -601,20 +601,14 @@ static const struct cec_adap_ops meson_ao_cec_ops = {
 static int meson_ao_cec_probe(struct platform_device *pdev)
 {
 	struct meson_ao_cec_device *ao_cec;
-	struct platform_device *hdmi_dev;
-	struct device_node *np;
+	struct device *hdmi_dev;
 	struct resource *res;
 	int ret, irq;
 
-	np = of_parse_phandle(pdev->dev.of_node, "hdmi-phandle", 0);
-	if (!np) {
-		dev_err(&pdev->dev, "Failed to find hdmi node\n");
-		return -ENODEV;
-	}
+	hdmi_dev = cec_notifier_find_hdmi_dev(&pdev->dev);
 
-	hdmi_dev = of_find_device_by_node(np);
-	if (hdmi_dev == NULL)
-		return -EPROBE_DEFER;
+	if (IS_ERR(hdmi_dev))
+		return PTR_ERR(hdmi_dev);
 
 	ao_cec = devm_kzalloc(&pdev->dev, sizeof(*ao_cec), GFP_KERNEL);
 	if (!ao_cec)
@@ -622,7 +616,7 @@ static int meson_ao_cec_probe(struct platform_device *pdev)
 
 	spin_lock_init(&ao_cec->cec_reg_lock);
 
-	ao_cec->notify = cec_notifier_get(&hdmi_dev->dev);
+	ao_cec->notify = cec_notifier_get(hdmi_dev);
 	if (!ao_cec->notify)
 		return -ENOMEM;
 
-- 
2.20.1

[PATCH 3/7] s5p_cec: use new cec_notifier_find_hdmi_dev helper

[Hans Verkuil]

The S5P CEC driver increased the HDMI device refcount when
it shouldn't. Use the new helper function to ensure that that
doesn't happen and to simplify the driver code.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/s5p-cec/s5p_cec.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/s5p-cec/s5p_cec.c b/drivers/media/platform/s5p-cec/s5p_cec.c
index 8837e2678bde..7e2c94816c55 100644
--- a/drivers/media/platform/s5p-cec/s5p_cec.c
+++ b/drivers/media/platform/s5p-cec/s5p_cec.c
@@ -178,22 +178,16 @@ static const struct cec_adap_ops s5p_cec_adap_ops = {
 static int s5p_cec_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *np;
-	struct platform_device *hdmi_dev;
+	struct device *hdmi_dev;
 	struct resource *res;
 	struct s5p_cec_dev *cec;
 	bool needs_hpd = of_property_read_bool(pdev->dev.of_node, "needs-hpd");
 	int ret;
 
-	np = of_parse_phandle(pdev->dev.of_node, "hdmi-phandle", 0);
+	hdmi_dev = cec_notifier_find_hdmi_dev(dev);
 
-	if (!np) {
-		dev_err(&pdev->dev, "Failed to find hdmi node in device tree\n");
-		return -ENODEV;
-	}
-	hdmi_dev = of_find_device_by_node(np);
-	if (hdmi_dev == NULL)
-		return -EPROBE_DEFER;
+	if (IS_ERR(hdmi_dev))
+		return PTR_ERR(hdmi_dev);
 
 	cec = devm_kzalloc(&pdev->dev, sizeof(*cec), GFP_KERNEL);
 	if (!cec)
@@ -224,7 +218,7 @@ static int s5p_cec_probe(struct platform_device *pdev)
 	if (IS_ERR(cec->reg))
 		return PTR_ERR(cec->reg);
 
-	cec->notifier = cec_notifier_get(&hdmi_dev->dev);
+	cec->notifier = cec_notifier_get(hdmi_dev);
 	if (cec->notifier == NULL)
 		return -ENOMEM;
 
-- 
2.20.1

[PATCH 4/7] stih_cec: use new cec_notifier_find_hdmi_dev helper

[Hans Verkuil]

The STI CEC driver increased the HDMI device refcount when
it shouldn't. Use the new helper function to ensure that that
doesn't happen and to simplify the driver code.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/sti/cec/stih-cec.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/sti/cec/stih-cec.c b/drivers/media/platform/sti/cec/stih-cec.c
index d34099f75990..678d7cb38637 100644
--- a/drivers/media/platform/sti/cec/stih-cec.c
+++ b/drivers/media/platform/sti/cec/stih-cec.c
@@ -301,26 +301,19 @@ static int stih_cec_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct resource *res;
 	struct stih_cec *cec;
-	struct device_node *np;
-	struct platform_device *hdmi_dev;
+	struct device *hdmi_dev;
 	int ret;
 
+	hdmi_dev = cec_notifier_find_hdmi_dev(dev);
+
+	if (IS_ERR(hdmi_dev))
+		return PTR_ERR(hdmi_dev);
+
 	cec = devm_kzalloc(dev, sizeof(*cec), GFP_KERNEL);
 	if (!cec)
 		return -ENOMEM;
 
-	np = of_parse_phandle(pdev->dev.of_node, "hdmi-phandle", 0);
-
-	if (!np) {
-		dev_err(&pdev->dev, "Failed to find hdmi node in device tree\n");
-		return -ENODEV;
-	}
-
-	hdmi_dev = of_find_device_by_node(np);
-	if (!hdmi_dev)
-		return -EPROBE_DEFER;
-
-	cec->notifier = cec_notifier_get(&hdmi_dev->dev);
+	cec->notifier = cec_notifier_get(hdmi_dev);
 	if (!cec->notifier)
 		return -ENOMEM;
 
-- 
2.20.1

[PATCH 5/7] tegra_cec: use new cec_notifier_find_hdmi_dev helper

[Hans Verkuil]

The Tegra CEC driver increased the HDMI device refcount when
it shouldn't. Use the new helper function to ensure that that
doesn't happen and to simplify the driver code.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/tegra-cec/tegra_cec.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/tegra-cec/tegra_cec.c b/drivers/media/platform/tegra-cec/tegra_cec.c
index aba488cd0e64..074d647f2908 100644
--- a/drivers/media/platform/tegra-cec/tegra_cec.c
+++ b/drivers/media/platform/tegra-cec/tegra_cec.c
@@ -327,21 +327,15 @@ static const struct cec_adap_ops tegra_cec_ops = {
 
 static int tegra_cec_probe(struct platform_device *pdev)
 {
-	struct platform_device *hdmi_dev;
-	struct device_node *np;
+	struct device *hdmi_dev;
 	struct tegra_cec *cec;
 	struct resource *res;
 	int ret = 0;
 
-	np = of_parse_phandle(pdev->dev.of_node, "hdmi-phandle", 0);
+	hdmi_dev = cec_notifier_find_hdmi_dev(&pdev->dev);
 
-	if (!np) {
-		dev_err(&pdev->dev, "Failed to find hdmi node in device tree\n");
+	if (!hdmi_dev)
 		return -ENODEV;
-	}
-	hdmi_dev = of_find_device_by_node(np);
-	if (hdmi_dev == NULL)
-		return -EPROBE_DEFER;
 
 	cec = devm_kzalloc(&pdev->dev, sizeof(struct tegra_cec), GFP_KERNEL);
 
@@ -400,7 +394,7 @@ static int tegra_cec_probe(struct platform_device *pdev)
 		goto clk_error;
 	}
 
-	cec->notifier = cec_notifier_get(&hdmi_dev->dev);
+	cec->notifier = cec_notifier_get(hdmi_dev);
 	if (!cec->notifier) {
 		ret = -ENOMEM;
 		goto clk_error;
-- 
2.20.1

[PATCH 6/7] seco-cec: decrement HDMI device refcount

[Hans Verkuil]

The SECO CEC driver never decremented the HDMI device refcount.
CEC drivers only need the HDMI device pointer as a key in the
notifier list, it never accesses the device, so there is no
need to keep a reference.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/seco-cec/seco-cec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/seco-cec/seco-cec.c b/drivers/media/platform/seco-cec/seco-cec.c
index a425a10540c1..e5080d6f5b2d 100644
--- a/drivers/media/platform/seco-cec/seco-cec.c
+++ b/drivers/media/platform/seco-cec/seco-cec.c
@@ -536,6 +536,7 @@ static int secocec_cec_get_notifier(struct cec_notifier **notify)
 				return -EPROBE_DEFER;
 
 			*notify = cec_notifier_get_conn(d, m->conn);
+			put_device(d);
 
 			return 0;
 		}
-- 
2.20.1

[PATCH 7/7] cros-ec-cec: decrement HDMI device refcount

[Hans Verkuil]

The CrosEC CEC driver never decremented the HDMI device refcount.
CEC drivers only need the HDMI device pointer as a key in the
notifier list, it never accesses the device, so there is no
need to keep a reference.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/cros-ec-cec/cros-ec-cec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/cros-ec-cec/cros-ec-cec.c b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
index 7bc4d8a9af28..068df9888dbf 100644
--- a/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
+++ b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
@@ -236,6 +236,7 @@ static int cros_ec_cec_get_notifier(struct device *dev,
 				return -EPROBE_DEFER;
 
 			*notify = cec_notifier_get_conn(d, m->conn);
+			put_device(d);
 			return 0;
 		}
 	}
-- 
2.20.1

Re: [PATCH 2/7] meson: ao-cec: use new cec_notifier_find_hdmi_dev helper

[Neil Armstrong]

On 08/04/2019 13:03, Hans Verkuil wrote:
> The meson CEC driver increased the HDMI device refcount when
> it shouldn't. Use the new helper function to ensure that that
> doesn't happen and to simplify the driver code.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/platform/meson/ao-cec.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/meson/ao-cec.c b/drivers/media/platform/meson/ao-cec.c
> index cd4be38ab5ac..c8b374741a7c 100644
> --- a/drivers/media/platform/meson/ao-cec.c
> +++ b/drivers/media/platform/meson/ao-cec.c
> @@ -601,20 +601,14 @@ static const struct cec_adap_ops meson_ao_cec_ops = {
>  static int meson_ao_cec_probe(struct platform_device *pdev)
>  {
>  	struct meson_ao_cec_device *ao_cec;
> -	struct platform_device *hdmi_dev;
> -	struct device_node *np;
> +	struct device *hdmi_dev;
>  	struct resource *res;
>  	int ret, irq;
>  
> -	np = of_parse_phandle(pdev->dev.of_node, "hdmi-phandle", 0);
> -	if (!np) {
> -		dev_err(&pdev->dev, "Failed to find hdmi node\n");
> -		return -ENODEV;
> -	}
> +	hdmi_dev = cec_notifier_find_hdmi_dev(&pdev->dev);
>  
> -	hdmi_dev = of_find_device_by_node(np);
> -	if (hdmi_dev == NULL)
> -		return -EPROBE_DEFER;
> +	if (IS_ERR(hdmi_dev))
> +		return PTR_ERR(hdmi_dev);
>  
>  	ao_cec = devm_kzalloc(&pdev->dev, sizeof(*ao_cec), GFP_KERNEL);
>  	if (!ao_cec)
> @@ -622,7 +616,7 @@ static int meson_ao_cec_probe(struct platform_device *pdev)
>  
>  	spin_lock_init(&ao_cec->cec_reg_lock);
>  
> -	ao_cec->notify = cec_notifier_get(&hdmi_dev->dev);
> +	ao_cec->notify = cec_notifier_get(hdmi_dev);
>  	if (!ao_cec->notify)
>  		return -ENOMEM;
>  
> 

Acked-by: Neil Armstrong <narmstrong@baylibre.com>

Thanks,
Neil

Re: [PATCH 4/7] stih_cec: use new cec_notifier_find_hdmi_dev helper

[Benjamin Gaignard]

Le lun. 8 avr. 2019 à 13:04, Hans Verkuil <hverkuil-cisco@xs4all.nl> a écrit :
>
> The STI CEC driver increased the HDMI device refcount when
> it shouldn't. Use the new helper function to ensure that that
> doesn't happen and to simplify the driver code.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Acked-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>

> ---
>  drivers/media/platform/sti/cec/stih-cec.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/platform/sti/cec/stih-cec.c b/drivers/media/platform/sti/cec/stih-cec.c
> index d34099f75990..678d7cb38637 100644
> --- a/drivers/media/platform/sti/cec/stih-cec.c
> +++ b/drivers/media/platform/sti/cec/stih-cec.c
> @@ -301,26 +301,19 @@ static int stih_cec_probe(struct platform_device *pdev)
>         struct device *dev = &pdev->dev;
>         struct resource *res;
>         struct stih_cec *cec;
> -       struct device_node *np;
> -       struct platform_device *hdmi_dev;
> +       struct device *hdmi_dev;
>         int ret;
>
> +       hdmi_dev = cec_notifier_find_hdmi_dev(dev);
> +
> +       if (IS_ERR(hdmi_dev))
> +               return PTR_ERR(hdmi_dev);
> +
>         cec = devm_kzalloc(dev, sizeof(*cec), GFP_KERNEL);
>         if (!cec)
>                 return -ENOMEM;
>
> -       np = of_parse_phandle(pdev->dev.of_node, "hdmi-phandle", 0);
> -
> -       if (!np) {
> -               dev_err(&pdev->dev, "Failed to find hdmi node in device tree\n");
> -               return -ENODEV;
> -       }
> -
> -       hdmi_dev = of_find_device_by_node(np);
> -       if (!hdmi_dev)
> -               return -EPROBE_DEFER;
> -
> -       cec->notifier = cec_notifier_get(&hdmi_dev->dev);
> +       cec->notifier = cec_notifier_get(hdmi_dev);
>         if (!cec->notifier)
>                 return -ENOMEM;
>
> --
> 2.20.1
>

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

[Thierry Reding]

[-- 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 --]

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

[Hans Verkuil]

On 4/8/19 3:09 PM, Thierry Reding wrote:
> 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?

It's only used if the driver uses notifiers, and is also in cec-notifier.c.
So I prefer to keep it like this.

> 
>> +{
>> +	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.

OK.

> 
>> +		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.

No, hdmi_dev is just a key to search in the list of notifiers. It is not
accessed or used in any other way.

I'll add a comment here to clarify this.

Regards,

	Hans

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

Re: [PATCH 6/7] seco-cec: decrement HDMI device refcount

[Ettore Chimenti]

On Mon, Apr 08, 2019 at 01:04:00PM +0200, Hans Verkuil wrote:
> The SECO CEC driver never decremented the HDMI device refcount.
> CEC drivers only need the HDMI device pointer as a key in the
> notifier list, it never accesses the device, so there is no
> need to keep a reference.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Acked-by: Ettore Chimenti <ek5.chimenti@gmail.com>

Thanks,
Ettore

> ---
>  drivers/media/platform/seco-cec/seco-cec.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/seco-cec/seco-cec.c b/drivers/media/platform/seco-cec/seco-cec.c
> index a425a10540c1..e5080d6f5b2d 100644
> --- a/drivers/media/platform/seco-cec/seco-cec.c
> +++ b/drivers/media/platform/seco-cec/seco-cec.c
> @@ -536,6 +536,7 @@ static int secocec_cec_get_notifier(struct cec_notifier **notify)
>  				return -EPROBE_DEFER;
>  
>  			*notify = cec_notifier_get_conn(d, m->conn);
> +			put_device(d);
>  
>  			return 0;
>  		}
> -- 
> 2.20.1
>