Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Hans de Goede <hdegoede@redhat.com>]

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.
>>>
>>
>>
>>
>
>