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@xxxxxxxxx>
>> Reported-by: Wen Yang <wen.yang99@xxxxxxxxxx>
>> ---
>> 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
>
