diff mbox series

[PATCHv8,03/13] cec: add new notifier functions

Message ID 20190624160330.38048-4-hverkuil-cisco@xs4all.nl (mailing list archive)
State New, archived
Headers show
Series cec: improve notifier support, add connector info | expand

Commit Message

Hans Verkuil June 24, 2019, 4:03 p.m. UTC
In order to support multiple CEC devices for an HDMI connector,
and to support cec_connector_info, drivers should use either a
cec_notifier_conn_(un)register pair of functions (HDMI drivers)
or a cec_notifier_cec_adap_(un)register pair (CEC adapter drivers).

This replaces cec_notifier_get_conn/cec_notifier_put.

For CEC adapters it is also no longer needed to call cec_notifier_register,
cec_register_cec_notifier and cec_notifier_unregister. This is now
all handled internally by the new functions.

The called_cec_notifier_register bool is needed to handle the case where
a CEC adapter driver called the old cec_notifier_register() function
instead of cec_notifier_cec_adap_unregister().

Once those old functions are removed in the future, that bool can also
be removed.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/cec/cec-notifier.c | 89 ++++++++++++++++++++++++++++++++
 include/media/cec-notifier.h     | 78 ++++++++++++++++++++++++++++
 2 files changed, 167 insertions(+)

Comments

Dariusz Marcinkiewicz June 25, 2019, 1:48 p.m. UTC | #1
Hello.

Some small comments/questions.

On Mon, Jun 24, 2019 at 6:03 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
...
> @@ -22,9 +22,11 @@ struct cec_notifier {
>         struct list_head head;
>         struct kref kref;
>         struct device *hdmi_dev;
> +       struct cec_connector_info conn_info;
>         const char *conn_name;
>         struct cec_adapter *cec_adap;
>         void (*callback)(struct cec_adapter *adap, u16 pa);
> +       bool called_cec_notifier_register;
If I read his correctly callback is set only by cec_notifier_register
(and not by the cec_notifier_cec_adap_register) so maybe that boolean
is not needed and just checking if the callback is set is enough to
tell those 2 cases apart?

...
> +struct cec_notifier *
> +cec_notifier_cec_adap_register(struct device *hdmi_dev, const char *conn_name,
> +                              struct cec_adapter *adap)
> +{
> +       struct cec_notifier *n;
> +
> +       if (WARN_ON(!adap))
> +               return NULL;
> +
> +       n = cec_notifier_get_conn(hdmi_dev, conn_name);
> +       if (!n)
> +               return n;
> +
> +       mutex_lock(&n->lock);
> +       n->cec_adap = adap;
> +       adap->conn_info = n->conn_info;
Would it make sense to use cec_s_conn_info? There is a certain
asymmetry here  - cec_s_phys_addr is used to configure physical
address, which also takes the adapter's lock while setting the
physical address. That lock is not taken while the connector info is
being set (not sure if that really matters for the code paths that
would call into this function).

> +       adap->notifier = n;
> +       cec_s_phys_addr(adap, n->phys_addr, false);
> +       mutex_unlock(&n->lock);
> +       return n;
> +}
> +EXPORT_SYMBOL_GPL(cec_notifier_cec_adap_register);
> +
> +void cec_notifier_cec_adap_unregister(struct cec_notifier *n)
> +{
> +       if (!n)
> +               return;
> +
> +       mutex_lock(&n->lock);
> +       memset(&n->cec_adap->conn_info, 0, sizeof(n->cec_adap->conn_info));
Could cec_s_conn_info be used to reset the connector info?
Also, we explicitly clear connector info here. Since the notifier
provides both connector info and physical address, maybe it would make
sense to clear physical address as well?


...
>  void cec_notifier_unregister(struct cec_notifier *n)
>  {
> +       /* Do nothing unless cec_notifier_register was called first */
> +       if (!n->called_cec_notifier_register)
Could this check be made with n->lock held? cec_notifier_register sets
this value while holding that lock.
...


Thank you.
Hans Verkuil June 25, 2019, 2:26 p.m. UTC | #2
On 6/25/19 3:48 PM, Dariusz Marcinkiewicz wrote:
> Hello.
> 
> Some small comments/questions.
> 
> On Mon, Jun 24, 2019 at 6:03 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
> ...
>> @@ -22,9 +22,11 @@ struct cec_notifier {
>>         struct list_head head;
>>         struct kref kref;
>>         struct device *hdmi_dev;
>> +       struct cec_connector_info conn_info;
>>         const char *conn_name;
>>         struct cec_adapter *cec_adap;
>>         void (*callback)(struct cec_adapter *adap, u16 pa);
>> +       bool called_cec_notifier_register;
> If I read his correctly callback is set only by cec_notifier_register
> (and not by the cec_notifier_cec_adap_register) so maybe that boolean
> is not needed and just checking if the callback is set is enough to
> tell those 2 cases apart?

True. I'll change this.

> 
> ...
>> +struct cec_notifier *
>> +cec_notifier_cec_adap_register(struct device *hdmi_dev, const char *conn_name,
>> +                              struct cec_adapter *adap)
>> +{
>> +       struct cec_notifier *n;
>> +
>> +       if (WARN_ON(!adap))
>> +               return NULL;
>> +
>> +       n = cec_notifier_get_conn(hdmi_dev, conn_name);
>> +       if (!n)
>> +               return n;
>> +
>> +       mutex_lock(&n->lock);
>> +       n->cec_adap = adap;
>> +       adap->conn_info = n->conn_info;
> Would it make sense to use cec_s_conn_info? There is a certain
> asymmetry here  - cec_s_phys_addr is used to configure physical
> address, which also takes the adapter's lock while setting the
> physical address. That lock is not taken while the connector info is
> being set (not sure if that really matters for the code paths that
> would call into this function).

I thought about that, but there is a side-effect if I do that:
both cec_s_conn_info and cec_s_phys_addr send a state_changed
event, and I want to avoid that. Doing it this way ensures that
there will be only one event.

But there is definitely room for improvement here and I want to
work on that for the next kernel.

> 
>> +       adap->notifier = n;
>> +       cec_s_phys_addr(adap, n->phys_addr, false);
>> +       mutex_unlock(&n->lock);
>> +       return n;
>> +}
>> +EXPORT_SYMBOL_GPL(cec_notifier_cec_adap_register);
>> +
>> +void cec_notifier_cec_adap_unregister(struct cec_notifier *n)
>> +{
>> +       if (!n)
>> +               return;
>> +
>> +       mutex_lock(&n->lock);
>> +       memset(&n->cec_adap->conn_info, 0, sizeof(n->cec_adap->conn_info));
> Could cec_s_conn_info be used to reset the connector info?
> Also, we explicitly clear connector info here. Since the notifier
> provides both connector info and physical address, maybe it would make
> sense to clear physical address as well?

Actually, this memset should be removed altogether. The connector info
doesn't change at all, it's just that right after this function is called
the whole CEC adapter will be unregistered. I thought that it might make
sense to zero the connector info, but really I should just leave it alone.

> 
> 
> ...
>>  void cec_notifier_unregister(struct cec_notifier *n)
>>  {
>> +       /* Do nothing unless cec_notifier_register was called first */
>> +       if (!n->called_cec_notifier_register)
> Could this check be made with n->lock held? cec_notifier_register sets
> this value while holding that lock.

It's not needed: you can never have a race here.

BTW, I'll be glad when I can remove these cec_notifier_(un)register
functions, they are pretty ugly and confusing.

Regards,

	Hans

> ...
> 
> 
> Thank you.
>
diff mbox series

Patch

diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
index ce10dfd400bb..49d6ec198d38 100644
--- a/drivers/media/cec/cec-notifier.c
+++ b/drivers/media/cec/cec-notifier.c
@@ -22,9 +22,11 @@  struct cec_notifier {
 	struct list_head head;
 	struct kref kref;
 	struct device *hdmi_dev;
+	struct cec_connector_info conn_info;
 	const char *conn_name;
 	struct cec_adapter *cec_adap;
 	void (*callback)(struct cec_adapter *adap, u16 pa);
+	bool called_cec_notifier_register;
 
 	u16 phys_addr;
 };
@@ -87,6 +89,85 @@  void cec_notifier_put(struct cec_notifier *n)
 }
 EXPORT_SYMBOL_GPL(cec_notifier_put);
 
+struct cec_notifier *
+cec_notifier_conn_register(struct device *hdmi_dev, const char *conn_name,
+			   const struct cec_connector_info *conn_info)
+{
+	struct cec_notifier *n = cec_notifier_get_conn(hdmi_dev, conn_name);
+
+	if (!n)
+		return n;
+
+	mutex_lock(&n->lock);
+	n->phys_addr = CEC_PHYS_ADDR_INVALID;
+	if (conn_info)
+		n->conn_info = *conn_info;
+	else
+		memset(&n->conn_info, 0, sizeof(n->conn_info));
+	if (n->cec_adap) {
+		cec_phys_addr_invalidate(n->cec_adap);
+		cec_s_conn_info(n->cec_adap, conn_info);
+	}
+	mutex_unlock(&n->lock);
+	return n;
+}
+EXPORT_SYMBOL_GPL(cec_notifier_conn_register);
+
+void cec_notifier_conn_unregister(struct cec_notifier *n)
+{
+	if (!n)
+		return;
+
+	mutex_lock(&n->lock);
+	memset(&n->conn_info, 0, sizeof(n->conn_info));
+	n->phys_addr = CEC_PHYS_ADDR_INVALID;
+	if (n->cec_adap) {
+		cec_phys_addr_invalidate(n->cec_adap);
+		cec_s_conn_info(n->cec_adap, NULL);
+	}
+	mutex_unlock(&n->lock);
+	cec_notifier_put(n);
+}
+EXPORT_SYMBOL_GPL(cec_notifier_conn_unregister);
+
+struct cec_notifier *
+cec_notifier_cec_adap_register(struct device *hdmi_dev, const char *conn_name,
+			       struct cec_adapter *adap)
+{
+	struct cec_notifier *n;
+
+	if (WARN_ON(!adap))
+		return NULL;
+
+	n = cec_notifier_get_conn(hdmi_dev, conn_name);
+	if (!n)
+		return n;
+
+	mutex_lock(&n->lock);
+	n->cec_adap = adap;
+	adap->conn_info = n->conn_info;
+	adap->notifier = n;
+	cec_s_phys_addr(adap, n->phys_addr, false);
+	mutex_unlock(&n->lock);
+	return n;
+}
+EXPORT_SYMBOL_GPL(cec_notifier_cec_adap_register);
+
+void cec_notifier_cec_adap_unregister(struct cec_notifier *n)
+{
+	if (!n)
+		return;
+
+	mutex_lock(&n->lock);
+	memset(&n->cec_adap->conn_info, 0, sizeof(n->cec_adap->conn_info));
+	n->cec_adap->notifier = NULL;
+	n->cec_adap = NULL;
+	n->callback = NULL;
+	mutex_unlock(&n->lock);
+	cec_notifier_put(n);
+}
+EXPORT_SYMBOL_GPL(cec_notifier_cec_adap_unregister);
+
 void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa)
 {
 	if (n == NULL)
@@ -96,6 +177,8 @@  void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa)
 	n->phys_addr = pa;
 	if (n->callback)
 		n->callback(n->cec_adap, n->phys_addr);
+	else if (n->cec_adap)
+		cec_s_phys_addr(n->cec_adap, n->phys_addr, false);
 	mutex_unlock(&n->lock);
 }
 EXPORT_SYMBOL_GPL(cec_notifier_set_phys_addr);
@@ -121,6 +204,7 @@  void cec_notifier_register(struct cec_notifier *n,
 {
 	kref_get(&n->kref);
 	mutex_lock(&n->lock);
+	n->called_cec_notifier_register = true;
 	n->cec_adap = adap;
 	n->callback = callback;
 	n->callback(adap, n->phys_addr);
@@ -130,8 +214,13 @@  EXPORT_SYMBOL_GPL(cec_notifier_register);
 
 void cec_notifier_unregister(struct cec_notifier *n)
 {
+	/* Do nothing unless cec_notifier_register was called first */
+	if (!n->called_cec_notifier_register)
+		return;
+
 	mutex_lock(&n->lock);
 	n->callback = NULL;
+	n->called_cec_notifier_register = false;
 	mutex_unlock(&n->lock);
 	cec_notifier_put(n);
 }
diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
index 57b3a9f6ea1d..313b6905c203 100644
--- a/include/media/cec-notifier.h
+++ b/include/media/cec-notifier.h
@@ -42,6 +42,60 @@  struct cec_notifier *cec_notifier_get_conn(struct device *dev,
  */
 void cec_notifier_put(struct cec_notifier *n);
 
+/**
+ * cec_notifier_conn_register - find or create a new cec_notifier for the given
+ * HDMI device and connector tuple.
+ * @hdmi_dev: HDMI device that sends the events.
+ * @conn_name: the connector name from which the event occurs. May be NULL
+ * if there is always only one HDMI connector created by the HDMI device.
+ * @conn_info: the connector info from which the event occurs (may be NULL)
+ *
+ * If a notifier for device @dev and connector @conn_name 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_conn_register(struct device *hdmi_dev, const char *conn_name,
+			   const struct cec_connector_info *conn_info);
+
+/**
+ * cec_notifier_conn_unregister - decrease refcount and delete when the
+ * refcount reaches 0.
+ * @n: notifier
+ */
+void cec_notifier_conn_unregister(struct cec_notifier *n);
+
+/**
+ * cec_notifier_cec_adap_register - find or create a new cec_notifier for the
+ * given device.
+ * @hdmi_dev: HDMI device that sends the events.
+ * @conn_name: the connector name from which the event occurs. May be NULL
+ * if there is always only one HDMI connector created by the HDMI device.
+ * @adap: the cec adapter that registered this notifier.
+ *
+ * If a notifier for device @dev and connector @conn_name 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_cec_adap_register(struct device *hdmi_dev, const char *conn_name,
+			       struct cec_adapter *adap);
+
+/**
+ * cec_notifier_cec_adap_unregister - decrease refcount and delete when the
+ * refcount reaches 0.
+ * @n: notifier
+ */
+void cec_notifier_cec_adap_unregister(struct cec_notifier *n);
+
 /**
  * cec_notifier_set_phys_addr - set a new physical address.
  * @n: the CEC notifier
@@ -110,6 +164,30 @@  static inline void cec_notifier_put(struct cec_notifier *n)
 {
 }
 
+static inline struct cec_notifier *
+cec_notifier_conn_register(struct device *hdmi_dev, const char *conn_name,
+			   const struct cec_connector_info *conn_info)
+{
+	/* A non-NULL pointer is expected on success */
+	return (struct cec_notifier *)0xdeadfeed;
+}
+
+static inline void cec_notifier_conn_unregister(struct cec_notifier *n)
+{
+}
+
+static inline struct cec_notifier *
+cec_notifier_cec_adap_register(struct device *hdmi_dev, const char *conn_name,
+			       struct cec_adapter *adap)
+{
+	/* A non-NULL pointer is expected on success */
+	return (struct cec_notifier *)0xdeadfeed;
+}
+
+static inline void cec_notifier_cec_adap_unregister(struct cec_notifier *n)
+{
+}
+
 static inline void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa)
 {
 }