* Re: [PATCH 3/7] s5p_cec: use new cec_notifier_find_hdmi_dev helper
[not found] <201904091005306593106@zte.com.cn>
@ 2019-04-10 9:10 ` Hans Verkuil
0 siblings, 0 replies; 2+ messages in thread
From: Hans Verkuil @ 2019-04-10 9:10 UTC (permalink / raw)
To: wen.yang99
Cc: linux-media, narmstrong, snawrocki, benjamin.gaignard, treding,
ek5.chimenti
On 4/9/19 4:05 AM, wen.yang99@zte.com.cn wrote:
> Hi Hans,
>
>> Subject: [PATCH 3/7] s5p_cec: use new cec_notifier_find_hdmi_dev helper
>> 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);
>>
>
> The hdmi_dev returned by the cec_notifier_find_hdmi_dev function is just a key value
> and cannot be used as a struct device *, so the name cec_notifier_find_hdmi_dev
> is a bit inappropriate.
>
>> - 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);
>
> The hdmi_dev variable is only used as a key for the input of the cec_notifier_get function.
> So here is a little advice:
> The current code mode is like this:
> struct device *dev = &pdev->dev;
> ...
> + struct device *hdmi_dev;
> ...
> + hdmi_dev = cec_notifier_find_hdmi_dev(dev);
> ...
> + cec->notifier = cec_notifier_get(hdmi_dev);
>
> Can we replace it like this:
> struct device *dev = &pdev->dev;
> ...
> + cec->notifier = cec_notifier_get(dev);
>
> This way we can remove the hdmi_dev temporary variable,
> and we can also remove the cec_notifier_find_hdmi_dev function.
> It is enough to provide an API similar to cec_notifier_get.
Some drivers get the HDMI device through other means (e.g. cros-ec-cec)
so combining this in one function is not a good idea.
I did decide to rename cec_notifier_find_hdmi_dev to
cec_notifier_parse_hdmi_phandle. I think that is a better description of
what the helper does.
Regards,
Hans
>
> --
> Regards,
> Wen
>
>> if (cec->notifier == NULL)
>> return -ENOMEM;
^ permalink raw reply [flat|nested] 2+ messages in thread
* [PATCH 0/7] cec: introduce cec_notifier_find_hdmi_dev helper
@ 2019-04-08 11:03 Hans Verkuil
2019-04-08 11:03 ` [PATCH 3/7] s5p_cec: use new " Hans Verkuil
0 siblings, 1 reply; 2+ messages in thread
From: Hans Verkuil @ 2019-04-08 11:03 UTC (permalink / raw)
To: linux-media
Cc: Neil Armstrong, Sylwester Nawrocki, Benjamin Gaignard,
Thierry Reding, Ettore Chimenti, Wen Yang
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* [PATCH 3/7] s5p_cec: use new cec_notifier_find_hdmi_dev helper
2019-04-08 11:03 [PATCH 0/7] cec: introduce " Hans Verkuil
@ 2019-04-08 11:03 ` Hans Verkuil
0 siblings, 0 replies; 2+ messages in thread
From: Hans Verkuil @ 2019-04-08 11:03 UTC (permalink / raw)
To: linux-media
Cc: Neil Armstrong, Sylwester Nawrocki, Benjamin Gaignard,
Thierry Reding, Ettore Chimenti, Wen Yang, 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
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-04-10 9:10 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <201904091005306593106@zte.com.cn>
2019-04-10 9:10 ` [PATCH 3/7] s5p_cec: use new cec_notifier_find_hdmi_dev helper Hans Verkuil
2019-04-08 11:03 [PATCH 0/7] cec: introduce " Hans Verkuil
2019-04-08 11:03 ` [PATCH 3/7] s5p_cec: use new " Hans Verkuil
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.