| Message ID | 20190408110401.3345-2-hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cec: introduce cec_notifier_find_hdmi_dev helper | expand |
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
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 >
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 /**
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(-)