Hi Neil,
Thanks for this patch series!
Some comments below:
On 05/15/2018 12:40 AM, Neil Armstrong wrote:
> In non device-tree world, we can need to get the notifier by the driver
> name directly and eventually defer probe if not yet created.
>
> This patch adds a variant of the get function by using the device name
> instead and will not create a notifier if not yet created.
>
> But the i915 driver exposes at least 2 HDMI connectors, this patch also
> adds the possibility to add a connector name tied to the notifier device
> to form a tuple and associate different CEC controllers for each HDMI
> connectors.
>
> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> ---
> drivers/media/cec/cec-notifier.c | 30 ++++++++++++++++++++++++---
> include/media/cec-notifier.h | 44 ++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 69 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
> index 16dffa0..716070a 100644
> --- a/drivers/media/cec/cec-notifier.c
> +++ b/drivers/media/cec/cec-notifier.c
> @@ -21,6 +21,7 @@ struct cec_notifier {
> struct list_head head;
> struct kref kref;
> struct device *dev;
> + const char *conn;
> struct cec_adapter *cec_adap;
> void (*callback)(struct cec_adapter *adap, u16 pa);
>
> @@ -30,13 +31,34 @@ struct cec_notifier {
> static LIST_HEAD(cec_notifiers);
> static DEFINE_MUTEX(cec_notifiers_lock);
>
> -struct cec_notifier *cec_notifier_get(struct device *dev)
> +struct cec_notifier *cec_notifier_get_byname(const char *name,
> + const char *conn)
> {
> struct cec_notifier *n;
>
> mutex_lock(&cec_notifiers_lock);
> list_for_each_entry(n, &cec_notifiers, head) {
> - if (n->dev == dev) {
> + if (!strcmp(dev_name(n->dev), name) &&
> + (!conn || !strcmp(n->conn, conn))) {
> + kref_get(&n->kref);
> + mutex_unlock(&cec_notifiers_lock);
> + return n;
> + }
> + }
> + mutex_unlock(&cec_notifiers_lock);
> +
> + return NULL;
This doesn't seem right. For one it doesn't act like the other cec_notifier_get*
functions in that it doesn't make a new notifier if it wasn't found yet in the
list.
For another, I think this function shouldn't be here at all. How about calling
bus_find_device_by_name(), then use cec_notifier_get_conn()?
> +}
> +EXPORT_SYMBOL_GPL(cec_notifier_get_byname);
> +
> +struct cec_notifier *cec_notifier_get_conn(struct device *dev, const char *conn)
> +{
> + struct cec_notifier *n;
> +
> + mutex_lock(&cec_notifiers_lock);
> + list_for_each_entry(n, &cec_notifiers, head) {
> + if (n->dev == dev &&
> + (!conn || !strcmp(n->conn, conn))) {
> kref_get(&n->kref);
> mutex_unlock(&cec_notifiers_lock);
> return n;
> @@ -46,6 +68,8 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
> if (!n)
> goto unlock;
> n->dev = dev;
> + if (conn)
> + n->conn = devm_kstrdup(dev, conn, GFP_KERNEL);
The use of devm_kstrdup worries me. The problem is that when the 'dev' device
is removed, this memory is also automatically freed. But the notifier might
still have a reference through the CEC driver, so you end up with a n->conn
pointer that points to freed memory.
I think it is better to just use kstrdup and kfree it when the last notifier
reference is released.
> n->phys_addr = CEC_PHYS_ADDR_INVALID;
> mutex_init(&n->lock);
> kref_init(&n->kref);
> @@ -54,7 +78,7 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
> mutex_unlock(&cec_notifiers_lock);
> return n;
> }
> -EXPORT_SYMBOL_GPL(cec_notifier_get);
> +EXPORT_SYMBOL_GPL(cec_notifier_get_conn);
>
> static void cec_notifier_release(struct kref *kref)
> {
> diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
> index cf0add7..70f2974 100644
> --- a/include/media/cec-notifier.h
> +++ b/include/media/cec-notifier.h
> @@ -20,6 +20,37 @@ struct cec_notifier;
> #if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)
>
> /**
> + * cec_notifier_get_byname - find a cec_notifier for the given device name
> + * and connector tuple.
> + * @name: device name that sends the events.
> + * @conn: the connector name from which the event occurs
> + *
> + * If a notifier for device @name exists, then increase the refcount and
> + * return that notifier.
> + *
> + * If it doesn't exist, return NULL
> + */
> +struct cec_notifier *cec_notifier_get_byname(const char *name,
> + const char *conn);
> +
> +/**
> + * cec_notifier_get_conn - find or create a new cec_notifier for the given
> + * device and connector tuple.
> + * @dev: device that sends the events.
> + * @conn: the connector name from which the event occurs
> + *
> + * If a notifier for device @dev already exists, then increase the refcount
> + * and return that notifier.
> + *
> + * If it doesn't exist, then allocate a new notifier struct and return a
> + * pointer to that new struct.
> + *
> + * Return NULL if the memory could not be allocated.
> + */
> +struct cec_notifier *cec_notifier_get_conn(struct device *dev,
> + const char *conn);
> +
> +/**
> * cec_notifier_get - find or create a new cec_notifier for the given device.
> * @dev: device that sends the events.
> *
> @@ -31,7 +62,10 @@ struct cec_notifier;
> *
> * Return NULL if the memory could not be allocated.
> */
> -struct cec_notifier *cec_notifier_get(struct device *dev);
> +static inline struct cec_notifier *cec_notifier_get(struct device *dev)
> +{
> + return cec_notifier_get_conn(dev, NULL);
> +}
>
> /**
> * cec_notifier_put - decrease refcount and delete when the refcount reaches 0.
> @@ -85,12 +119,18 @@ void cec_register_cec_notifier(struct cec_adapter *adap,
> struct cec_notifier *notifier);
>
> #else
> -static inline struct cec_notifier *cec_notifier_get(struct device *dev)
> +static inline struct cec_notifier *cec_notifier_get_conn(struct device *dev,
> + const char *conn)
> {
> /* A non-NULL pointer is expected on success */
> return (struct cec_notifier *)0xdeadfeed;
> }
>
> +static inline struct cec_notifier *cec_notifier_get(struct device *dev)
> +{
> + return cec_notifier_get_conn(dev, NULL);
> +}
> +
> static inline void cec_notifier_put(struct cec_notifier *n)
> {
> }
>
Regards,
Hans
