Re: [PATCHv2 1/4] video: add hotplug detect notifier support

[Hans Verkuil <hverkuil@xs4all.nl>]

On 01/03/2017 08:50 AM, Andrzej Hajda wrote:
> Hi Hans,
> 
> On 02.01.2017 15:19, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Add support for video hotplug detect and EDID/ELD notifiers, which is used
>> to convey information from video drivers to their CEC and audio counterparts.
>>
>> Based on an earlier version from Russell King:
>>
>> https://patchwork.kernel.org/patch/9277043/
>>
>> The hpd_notifier is a reference counted object containing the HPD/EDID/ELD state
>> of a video device.
>>
>> When a new notifier is registered the current state will be reported to
>> that notifier at registration time.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>  drivers/video/Kconfig        |   3 +
>>  drivers/video/Makefile       |   1 +
>>  drivers/video/hpd-notifier.c | 134 +++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/hpd-notifier.h | 109 +++++++++++++++++++++++++++++++++++
>>  4 files changed, 247 insertions(+)
>>  create mode 100644 drivers/video/hpd-notifier.c
>>  create mode 100644 include/linux/hpd-notifier.h
>>
>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
>> index 3c20af9..cddc860 100644
>> --- a/drivers/video/Kconfig
>> +++ b/drivers/video/Kconfig
>> @@ -36,6 +36,9 @@ config VIDEOMODE_HELPERS
>>  config HDMI
>>  	bool
>>  
>> +config HPD_NOTIFIERS
>> +	bool
>> +
>>  if VT
>>  	source "drivers/video/console/Kconfig"
>>  endif
>> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
>> index 9ad3c17..424698b 100644
>> --- a/drivers/video/Makefile
>> +++ b/drivers/video/Makefile
>> @@ -1,5 +1,6 @@
>>  obj-$(CONFIG_VGASTATE)            += vgastate.o
>>  obj-$(CONFIG_HDMI)                += hdmi.o
>> +obj-$(CONFIG_HPD_NOTIFIERS)       += hpd-notifier.o
>>  
>>  obj-$(CONFIG_VT)		  += console/
>>  obj-$(CONFIG_LOGO)		  += logo/
>> diff --git a/drivers/video/hpd-notifier.c b/drivers/video/hpd-notifier.c
>> new file mode 100644
>> index 0000000..54f7a6b
>> --- /dev/null
>> +++ b/drivers/video/hpd-notifier.c
>> @@ -0,0 +1,134 @@
>> +#include <linux/export.h>
>> +#include <linux/hpd-notifier.h>
>> +#include <linux/string.h>
>> +#include <linux/slab.h>
>> +#include <linux/list.h>
>> +
>> +static LIST_HEAD(hpd_notifiers);
>> +static DEFINE_MUTEX(hpd_notifiers_lock);
>> +
>> +struct hpd_notifier *hpd_notifier_get(struct device *dev)
>> +{
>> +	struct hpd_notifier *n;
>> +
>> +	mutex_lock(&hpd_notifiers_lock);
>> +	list_for_each_entry(n, &hpd_notifiers, head) {
>> +		if (n->dev == dev) {
>> +			mutex_unlock(&hpd_notifiers_lock);
> 
> I think this place is racy, we have pointer to unprotected area
> (n->kref), so if concurrent thread calls hpd_notifier_put in this moment
> &n->kref could be freed and kref_get in the next line will operate on
> dangling pointer. Am I right?

You're right. I took the hpd_notifiers_lock in hpd_notifier_release,
but I should take it in hpd_notifier_put.

Thanks! I'll fix that.

Regards,

	Hans

> 
> Regards
> Andrzej
> 
>> +			kref_get(&n->kref);
>> +			return n;
>> +		}
>> +	}
>> +	n = kzalloc(sizeof(*n), GFP_KERNEL);
>> +	if (!n)
>> +		goto unlock;
>> +	n->dev = dev;
>> +	mutex_init(&n->lock);
>> +	BLOCKING_INIT_NOTIFIER_HEAD(&n->notifiers);
>> +	kref_init(&n->kref);
>> +	list_add_tail(&n->head, &hpd_notifiers);
>> +unlock:
>> +	mutex_unlock(&hpd_notifiers_lock);
>> +	return n;
>> +}
>> +EXPORT_SYMBOL_GPL(hpd_notifier_get);
>> +
>> +static void hpd_notifier_release(struct kref *kref)
>> +{
>> +	struct hpd_notifier *n =
>> +		container_of(kref, struct hpd_notifier, kref);
>> +
>> +	mutex_lock(&hpd_notifiers_lock);
>> +	list_del(&n->head);
>> +	mutex_unlock(&hpd_notifiers_lock);
>> +	kfree(n->edid);
>> +	kfree(n);
>> +}
>> +
>> +void hpd_notifier_put(struct hpd_notifier *n)
>> +{
>> +	kref_put(&n->kref, hpd_notifier_release);
>> +}
>> +EXPORT_SYMBOL_GPL(hpd_notifier_put);
>> +
>> +int hpd_notifier_register(struct hpd_notifier *n, struct notifier_block *nb)
>> +{
>> +	int ret = blocking_notifier_chain_register(&n->notifiers, nb);
>> +
>> +	if (ret)
>> +		return ret;
>> +	kref_get(&n->kref);
>> +	mutex_lock(&n->lock);
>> +	if (n->connected) {
>> +		blocking_notifier_call_chain(&n->notifiers, HPD_CONNECTED, n);
>> +		if (n->edid_size)
>> +			blocking_notifier_call_chain(&n->notifiers, HPD_NEW_EDID, n);
>> +		if (n->has_eld)
>> +			blocking_notifier_call_chain(&n->notifiers, HPD_NEW_ELD, n);
>> +	}
>> +	mutex_unlock(&n->lock);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(hpd_notifier_register);
>> +
>> +int hpd_notifier_unregister(struct hpd_notifier *n, struct notifier_block *nb)
>> +{
>> +	int ret = blocking_notifier_chain_unregister(&n->notifiers, nb);
>> +
>> +	if (ret == 0)
>> +		hpd_notifier_put(n);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(hpd_notifier_unregister);
> (...)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>