Date:   Mon, 03 Apr 2017 16:24:29 +0900
From:   Chanwoo Choi <cw00.choi@...sung.com>
To:     Hans de Goede <hdegoede@...hat.com>, linux-kernel@...r.kernel.org
Cc:     chanwoo@...nel.org, myungjoo.ham@...sung.com
Subject: Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to
 monitor all external connectors

Hi,

On 2017년 03월 30일 23:58, Hans de Goede wrote:
> 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@...hat.com>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.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.


Actually, I knew that if the extcon consumer driver uses
the extcon_get_state() in the callback function, there is a deadlock
between extcon_sync() and extcon_get_state(). So, all extcon consumer
uses the workqueue when receiving the notfication from extcon.

But, extcon_sync() have to call the number of callback functions
in the notifier chanin. If one specific extcon consumer spend many
time in the own callback function, other extcon consumer might receive
the notification late. So, I think that each extcon consumer
better to use the work in the their callback function.

As I already said, I think that extcon focus on sending the notification
to all of extcon consumers.

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


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Powered by blists - more mailing lists