lkml.org 
[lkml]   [2017]   [Mar]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors
From
Date
Hi,

On 30-03-17 11:20, Chanwoo Choi wrote:
> On 2017년 03월 30일 18:04, Hans de Goede wrote:
>> Hi Chanwoo,
>>
>> On 30-03-17 10:39, Chanwoo Choi wrote:
>>> The extcon core already provides the extcon_register_notifier() function
>>> in order to register the notifier block which is used to monitor
>>> the status change for the specific external connector such as EXTCON_USB,
>>> EXTCON_USB_HOST and so on. The extcon consumer uses the this function.
>>>
>>> The extcon consumer may need to monitor the all supported external
>>> connectors from the extcon device. In this case, The extcon consumer
>>> should have each notifier_block structure for each external connector.
>>>
>>> This patch adds the new extcon_register_notifier_all() function
>>> that extcon consumer is able to monitor the status change of all
>>> supported external connectors by using only one notifier_block structure.
>>>
>>> - List of new added functions:
>>> int extcon_register_notifier_all(struct extcon_dev *edev,
>>> struct notifier_block *nb);
>>> int extcon_unregister_notifier_all(struct extcon_dev *edev,
>>> struct notifier_block *nb);
>>> int devm_extcon_register_notifier_all(struct device *dev,
>>> struct extcon_dev *edev, struct notifier_block *nb);
>>> void devm_extcon_unregister_notifier_all(struct device *dev,
>>> struct extcon_dev *edev, struct notifier_block *nb);
>>>
>>> Suggested-by: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>> drivers/extcon/devres.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>>> drivers/extcon/extcon.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
>>> drivers/extcon/extcon.h | 3 +++
>>> include/linux/extcon.h | 21 ++++++++++++----
>>> 4 files changed, 145 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/extcon/devres.c b/drivers/extcon/devres.c
>>> index b40eb1805927..3fc8eea52f1d 100644
>>> --- a/drivers/extcon/devres.c
>>> +++ b/drivers/extcon/devres.c
>>> @@ -50,6 +50,13 @@ static void devm_extcon_dev_notifier_unreg(struct device *dev, void *res)
>>> extcon_unregister_notifier(this->edev, this->id, this->nb);
>>> }
>>>
>>> +static void devm_extcon_dev_notifier_all_unreg(struct device *dev, void *res)
>>> +{
>>> + struct extcon_dev_notifier_devres *this = res;
>>> +
>>> + extcon_unregister_notifier_all(this->edev, this->nb);
>>> +}
>>> +
>>> /**
>>> * devm_extcon_dev_allocate - Allocate managed extcon device
>>> * @dev: device owning the extcon device being created
>>> @@ -214,3 +221,60 @@ void devm_extcon_unregister_notifier(struct device *dev,
>>> devm_extcon_dev_match, edev));
>>> }
>>> EXPORT_SYMBOL(devm_extcon_unregister_notifier);
>>> +
>>> +/**
>>> + * devm_extcon_register_notifier_all()
>>> + * - Resource-managed extcon_register_notifier_all()
>>> + * @dev: device to allocate extcon device
>>> + * @edev: the extcon device that has the external connecotr.
>>> + * @nb: a notifier block to be registered.
>>> + *
>>> + * This function manages automatically the notifier of extcon device using
>>> + * device resource management and simplify the control of unregistering
>>> + * the notifier of extcon device.
>>> + *
>>> + * Note that the second parameter given to the callback of nb (val) is
>>> + * the current state and third parameter is the edev pointer.
>>> + *
>>> + * Returns 0 if success or negaive error number if failure.
>>> + */
>>> +int devm_extcon_register_notifier_all(struct device *dev, struct extcon_dev *edev,
>>> + struct notifier_block *nb)
>>> +{
>>> + struct extcon_dev_notifier_devres *ptr;
>>> + int ret;
>>> +
>>> + ptr = devres_alloc(devm_extcon_dev_notifier_all_unreg, sizeof(*ptr),
>>> + GFP_KERNEL);
>>> + if (!ptr)
>>> + return -ENOMEM;
>>> +
>>> + ret = extcon_register_notifier_all(edev, nb);
>>> + if (ret) {
>>> + devres_free(ptr);
>>> + return ret;
>>> + }
>>> +
>>> + ptr->edev = edev;
>>> + ptr->nb = nb;
>>> + devres_add(dev, ptr);
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(devm_extcon_register_notifier_all);
>>> +
>>> +/**
>>> + * devm_extcon_unregister_notifier_all()
>>> + - Resource-managed extcon_unregister_notifier_all()
>>> + * @dev: device to allocate extcon device
>>> + * @edev: the extcon device that has the external connecotr.
>>> + * @nb: a notifier block to be registered.
>>> + */
>>> +void devm_extcon_unregister_notifier_all(struct device *dev,
>>> + struct extcon_dev *edev,
>>> + struct notifier_block *nb)
>>> +{
>>> + WARN_ON(devres_release(dev, devm_extcon_dev_notifier_all_unreg,
>>> + devm_extcon_dev_match, edev));
>>> +}
>>> +EXPORT_SYMBOL(devm_extcon_unregister_notifier_all);
>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>> index 193a3a673d10..4873fe8d5cd8 100644
>>> --- a/drivers/extcon/extcon.c
>>> +++ b/drivers/extcon/extcon.c
>>> @@ -445,8 +445,19 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>>> spin_lock_irqsave(&edev->lock, flags);
>>>
>>> state = !!(edev->state & BIT(index));
>>> +
>>> + /*
>>> + * Call functions in a raw notifier chain for the specific one
>>> + * external connector.
>>> + */
>>> raw_notifier_call_chain(&edev->nh[index], state, edev);
>>>
>>> + /*
>>> + * Call functions in a raw notifier chain for the all supported
>>> + * external connectors.
>>> + */
>>> + raw_notifier_call_chain(&edev->nh_all, state, edev);
>>> +
>>
>> Passing state here is not useful since the receiver of
>> the notifier call will not know for which cable the state is.
>
> No, the receiver can use the 'state' value whether connector is attached
> or detached. Also, the extcon have to keep the same value
> for both extcon_register_notifier() and extcon_register_notfier_all().

Ok.

>> Also this should be moved outside of the area of the
>> function holding the edev->lock spinlock, since we don't
>> pass state we do not need the lock and the called
>> notifier function may very well want to call extcon_get_state
>> to find out the state of one or more of the cables,
>> which takes the lock.
>
> The extcon uses the spinlock for the short delay.
> Extcon should update the status of external connector
> to the extcon consumer as soon as possible.

Right, what I'm suggestion actually also applies to the
current cable notification, what I'm suggesting is to
move the notification like this:

--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -448,8 +448,6 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
spin_lock_irqsave(&edev->lock, flags);

state = !!(edev->state & BIT(index));
- raw_notifier_call_chain(&edev->nh[index], state, edev);
- raw_notifier_call_chain(&edev->nh_all, 0, edev);

/* This could be in interrupt handler */
prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
@@ -482,6 +480,10 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)

/* Unlock early before uevent */
spin_unlock_irqrestore(&edev->lock, flags);
+
+ raw_notifier_call_chain(&edev->nh[index], state, edev);
+ raw_notifier_call_chain(&edev->nh_all, 0, edev);
+
kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp);
free_page((unsigned long)prop_buf);

So that the nb.notifier_call function can call extcon_get_state
to find out what cable is plugged in without deadlocking because
extcon_get_state does spin_lock_irqsave(&edev->lock, flags) too.

This is esp. important for the edev->nh_all notifier chain
since when used in charger drivers the callback will want to call
extcon_get_state for all of: EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_DCP,
EXTCON_CHG_USB_CDP, etc. to find out how much current it can draw
from the port.

AFAICT tell there is no race in moving this outside of the locked
section of extcon_sync() and it avoids potential deadlocks in the
nb.notifier_call function.

Regards,

Hans


>>> /* This could be in interrupt handler */
>>> prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
>>> if (!prop_buf) {
>>> @@ -951,6 +962,55 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id,
>>> }
>>> EXPORT_SYMBOL_GPL(extcon_unregister_notifier);
>>>
>>> +/**
>>> + * extcon_register_notifier_all() - Register a notifier block to get the noti
>>> + * of the status change for all supported external
>>> + * connectors from extcon.
>>> + * @edev: the extcon device that has the external connecotr.
>>> + * @nb: a notifier block to be registered.
>>> + *
>>> + * Note that the second parameter given to the callback of nb (val) is
>>> + * the current state and third parameter is the edev pointer.
>>> + */
>>> +int extcon_register_notifier_all(struct extcon_dev *edev,
>>> + struct notifier_block *nb)
>>> +{
>>> + unsigned long flags;
>>> + int ret;
>>> +
>>> + if (!edev || !nb)
>>> + return -EINVAL;
>>> +
>>> + spin_lock_irqsave(&edev->lock, flags);
>>> + ret = raw_notifier_chain_register(&edev->nh_all, nb);
>>> + spin_unlock_irqrestore(&edev->lock, flags);
>>> +
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(extcon_register_notifier_all);
>>> +
>>> +/**
>>> + * extcon_unregister_notifier_all() - Unregister a notifiee from the extcon device.
>>> + * @edev: the extcon device that has the external connecotr.
>>> + * @nb: a notifier block to be registered.
>>> + */
>>> +int extcon_unregister_notifier_all(struct extcon_dev *edev,
>>> + struct notifier_block *nb)
>>> +{
>>> + unsigned long flags;
>>> + int ret;
>>> +
>>> + if (!edev || !nb)
>>> + return -EINVAL;
>>> +
>>> + spin_lock_irqsave(&edev->lock, flags);
>>> + ret = raw_notifier_chain_unregister(&edev->nh_all, nb);
>>> + spin_unlock_irqrestore(&edev->lock, flags);
>>> +
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(extcon_unregister_notifier_all);
>>> +
>>> static struct attribute *extcon_attrs[] = {
>>> &dev_attr_state.attr,
>>> &dev_attr_name.attr,
>>> @@ -1199,6 +1259,8 @@ int extcon_dev_register(struct extcon_dev *edev)
>>> for (index = 0; index < edev->max_supported; index++)
>>> RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]);
>>>
>>> + RAW_INIT_NOTIFIER_HEAD(&edev->nh_all);
>>> +
>>> dev_set_drvdata(&edev->dev, edev);
>>> edev->state = 0;
>>>
>>> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
>>> index 993ddccafe11..dddddcfa0587 100644
>>> --- a/drivers/extcon/extcon.h
>>> +++ b/drivers/extcon/extcon.h
>>> @@ -21,6 +21,8 @@
>>> * @dev: Device of this extcon.
>>> * @state: Attach/detach state of this extcon. Do not provide at
>>> * register-time.
>>> + * @nh_all: Notifier for the state change events for all supported
>>> + * external connectors from this extcon.
>>> * @nh: Notifier for the state change events from this extcon
>>> * @entry: To support list of extcon devices so that users can
>>> * search for extcon devices based on the extcon name.
>>> @@ -43,6 +45,7 @@ struct extcon_dev {
>>>
>>> /* Internal data. Please do not set. */
>>> struct device dev;
>>> + struct raw_notifier_head nh_all;
>>> struct raw_notifier_head *nh;
>>> struct list_head entry;
>>> int max_supported;
>>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>>> index 3929d2e8a3c7..61e8d8c591a4 100644
>>> --- a/include/linux/extcon.h
>>> +++ b/include/linux/extcon.h
>>> @@ -270,11 +270,11 @@ extern int extcon_set_property_capability(struct extcon_dev *edev,
>>> unsigned int id, unsigned int prop);
>>>
>>> /*
>>> - * Following APIs are to monitor every action of a notifier.
>>> - * Registrar gets notified for every external port of a connection device.
>>> - * Probably this could be used to debug an action of notifier; however,
>>> - * we do not recommend to use this for normal 'notifiee' device drivers who
>>> - * want to be notified by a specific external port of the notifier.
>>> + * Following APIs are to monitor the status change of the external connectors.
>>> + * extcon_register_notifier(*edev, id, *nb) : Register a notifier
>>> + * for specific external connector from the extcon.
>>> + * extcon_register_notifier_all(*edev, *nb) : Register a notifier
>>> + * for all supported external connectors from the extcon.
>>> */
>>> extern int extcon_register_notifier(struct extcon_dev *edev, unsigned int id,
>>> struct notifier_block *nb);
>>> @@ -287,6 +287,17 @@ extern void devm_extcon_unregister_notifier(struct device *dev,
>>> struct extcon_dev *edev, unsigned int id,
>>> struct notifier_block *nb);
>>>
>>> +extern int extcon_register_notifier_all(struct extcon_dev *edev,
>>> + struct notifier_block *nb);
>>> +extern int extcon_unregister_notifier_all(struct extcon_dev *edev,
>>> + struct notifier_block *nb);
>>> +extern int devm_extcon_register_notifier_all(struct device *dev,
>>> + struct extcon_dev *edev,
>>> + struct notifier_block *nb);
>>> +extern void devm_extcon_unregister_notifier_all(struct device *dev,
>>> + struct extcon_dev *edev,
>>> + struct notifier_block *nb);
>>> +
>>> /*
>>> * Following API get the extcon device from devicetree.
>>> * This function use phandle of devicetree to get extcon device directly.
>>>
>>
>>
>>
>
>

\
 
 \ /
  Last update: 2017-03-30 16:58    [W:0.104 / U:0.680 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site