From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from lb1-smtp-cloud6.xs4all.net ([194.109.24.24]:37943 "EHLO lb1-smtp-cloud6.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757068AbdACHy6 (ORCPT ); Tue, 3 Jan 2017 02:54:58 -0500 Subject: Re: [PATCHv2 1/4] video: add hotplug detect notifier support To: Andrzej Hajda , linux-media@vger.kernel.org References: <1483366747-34288-1-git-send-email-hverkuil@xs4all.nl> <1483366747-34288-2-git-send-email-hverkuil@xs4all.nl> Cc: linux-samsung-soc@vger.kernel.org, Russell King , dri-devel@lists.freedesktop.org, Javier Martinez Canillas , Hans Verkuil , Krzysztof Kozlowski , Marek Szyprowski From: Hans Verkuil Message-ID: Date: Tue, 3 Jan 2017 08:54:52 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: On 01/03/2017 08:50 AM, Andrzej Hajda wrote: > Hi Hans, > > On 02.01.2017 15:19, Hans Verkuil wrote: >> From: Hans Verkuil >> >> 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 >> Tested-by: Marek Szyprowski >> --- >> 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 >> +#include >> +#include >> +#include >> +#include >> + >> +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 >