Date:   Sat, 25 Mar 2017 15:03:33 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Chanwoo Choi <cw00.choi@...sung.com>,
        MyungJoo Ham <myungjoo.ham@...sung.com>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] extcon: Allow registering a single notifier for all
 cables on an extcon_dev

Hi,

On 20-03-17 10:48, Hans de Goede wrote:
> Hi,
>
> On 20-03-17 01:55, Chanwoo Choi wrote:
>> Hi,
>>
>> On 2017년 03월 20일 03:23, Hans de Goede wrote:
>>> In some cases a driver may want to monitor multiple cables on a single
>>> extcon. For example a charger driver will typically want to monitor all
>>> of EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP to configure
>>> the max. current it can sink while charging.
>>>
>>> Due to the signature of the notifier_call function + how extcon passes
>>> state and the extcon_dev as parameters this requires using one
>>> notifier_block + one notifier_call function per cable, otherwise the
>>> notifier_call function cannot get to its driver's data (using container_of
>>> requires it to know which notifier block its first parameter is).
>>>
>>> For a driver wanting to monitor the above 3 cables that would result
>>> in something like this:
>>>
>>> static const unsigned int vbus_cable_ids[] = {
>>>     EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
>>>
>>> struct driver_data {
>>>     struct notifier_block vbus_nb[ARRAY_SIZE(vbus_cable_ids)];
>>> }
>>>
>>> /*
>>>  * We need 3 copies of this, because there is no way to find out for which
>>>  * cable id we are being called from the passed in arguments; and we must
>>>  * have a separate nb for each extcon_register_notifier call.
>>>  */
>>> static int vbus_cable0_evt(struct notifier_block *nb, unsigned long e, void *p)
>>> {
>>>     struct driver_data *data =
>>>         container_of(nb, struct driver_data, vbus_nb[0]);
>>>     ...
>>> }
>>>
>>> static int vbus_cable1_evt(struct notifier_block *nb, unsigned long e, void *p)
>>> {
>>>     struct driver_data *data =
>>>         container_of(nb, struct driver_data, vbus_nb[1]);
>>>     ...
>>> }
>>>
>>> static int vbus_cable2_evt(struct notifier_block *nb, unsigned long e, void *p)
>>> {
>>>     struct driver_data *data =
>>>         container_of(nb, struct driver_data, vbus_nb[2]);
>>>     ...
>>> }
>>>
>>> int probe(...)
>>> {
>>>     /* Register for vbus notification */
>>>     data->vbus_nb[0].notifier_call = vbus_cable0_evt;
>>>     data->vbus_nb[1].notifier_call = vbus_cable1_evt;
>>>     data->vbus_nb[2].notifier_call = vbus_cable2_evt;
>>>     for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
>>>         ret = devm_extcon_register_notifier(dev, data->vbus_extcon,
>>>                     vbus_cable_ids[i], &data->vbus_nb[i]);
>>>         if (ret)
>>>             ...
>>>     }
>>> }
>>
>> You can get the notification for multiple external connector
>> by using the only one notifier_block as following:
>>
>> static const unsigned int vbus_cable_ids[] = {
>>     EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
>>
>> struct driver_data {
>>     struct notifier_block vbus_nb;
>> }
>>
>> static int vbus_cable_evt(struct notifier_block *nb, unsigned long e, void *p)
>> {
>>     struct driver_data *data =
>>         container_of(nb, struct driver_data, vbus_nb);
>>     int sdp_state, cdp_state, dcp_state;
>>
>>     sdp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_SDP);
>>     cdp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_CDP);
>>     dcp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_DCP);
>>
>>     ...
>> }
>>
>> int probe(...)
>> {
>>     /* Register for vbus notification */
>>     data->vbus_nb.notifier_call = vbus_cable_evt;
>>     for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
>>         ret = devm_extcon_register_notifier(dev, data->vbus_extcon,
>>                     vbus_cable_ids[i], &data->vbus_nb);
>>         if (ret)
>>             ...
>>     }
>> }
>
> No that will not work, you cannot add the same notifier_block to 3
> different notifier_heads, the list_head can only be part of one list!
>
> Also see:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply?id=577b1f06e22057e9cdc14b1ee5bd25435c71ff0f
>
> Which actually fixes an issue I encountered exactly because of doing
> the above.
>
> So we really need something like my patch to make this simpler
> and to remove the ugliness we currently have in axp288_charger.c
> because of this problem (and are about to have in more drivers
> I'm upstreaming). Also note that my patch is quite simple, the
> amount of lines it adds is way less then the amount of lines
> it will allow to remove in a single driver!

Ping ?

As explained this is a real problem and we really need a proper solution
for this. I'm about to upstream 2 more drivers which need to monitor
an antire extcon rather then a single cable on it, and I do not want
to have to add hacks like this:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply?id=577b1f06e22057e9cdc14b1ee5bd25435c71ff0f

To both drivers. My patch is IMHO a nice and clean solution for this,
can you please merge this or provide an alternative solution ?

Regards,

Hans




>
> Regards,
>
> Hans
>
>
>>>
>>> And then in the event handling the driver often checks the state of
>>> all cables explicitly using extcon_get_state, rather then using the
>>> event argument to the notifier_call.
>>>
>>> This commit makes extcon_[un]register_notifier accept -1 as cable-id,
>>> which will cause the notifier to get called for changes on any cable
>>> on the extcon_dev. Compared to the above example code this allows much
>>> simpler code in drivers which want to monitor multiple cable types.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
>>> ---
>>> Changes in v2:
>>> -Fix false-positive "warning: 'idx' may be used uninitialized" warning
>>>  seen with some compiler versions
>>> ---
>>>  drivers/extcon/extcon.c | 35 ++++++++++++++++++++++++++---------
>>>  drivers/extcon/extcon.h |  1 +
>>>  2 files changed, 27 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>> index 09ac5e7..2a042ee 100644
>>> --- a/drivers/extcon/extcon.c
>>> +++ b/drivers/extcon/extcon.c
>>> @@ -449,6 +449,7 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>>>
>>>      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);
>>> @@ -900,6 +901,8 @@ EXPORT_SYMBOL_GPL(extcon_get_extcon_dev);
>>>   *                any attach status changes from the extcon.
>>>   * @edev:    the extcon device that has the external connecotr.
>>>   * @id:        the unique id of each external connector in extcon enumeration.
>>> + *        or -1 to get notififications for all cables on edev, in this
>>> + *        case no state info will get passed to the notifier_call.
>>>   * @nb:        a notifier block to be registered.
>>>   *
>>>   * Note that the second parameter given to the callback of nb (val) is
>>> @@ -915,12 +918,18 @@ int extcon_register_notifier(struct extcon_dev *edev, unsigned int id,
>>>      if (!edev || !nb)
>>>          return -EINVAL;
>>>
>>> -    idx = find_cable_index_by_id(edev, id);
>>> -    if (idx < 0)
>>> -        return idx;
>>> +    if ((int)id != -1) {
>>> +        idx = find_cable_index_by_id(edev, id);
>>> +        if (idx < 0)
>>> +            return idx;
>>> +    }
>>>
>>>      spin_lock_irqsave(&edev->lock, flags);
>>> -    ret = raw_notifier_chain_register(&edev->nh[idx], nb);
>>> +    if ((int)id != -1)
>>> +        ret = raw_notifier_chain_register(&edev->nh[idx], nb);
>>> +    else
>>> +        ret = raw_notifier_chain_register(&edev->nh_all, nb);
>>> +
>>>      spin_unlock_irqrestore(&edev->lock, flags);
>>>
>>>      return ret;
>>> @@ -937,17 +946,23 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id,
>>>                  struct notifier_block *nb)
>>>  {
>>>      unsigned long flags;
>>> -    int ret, idx;
>>> +    int ret, idx = 0;
>>>
>>>      if (!edev || !nb)
>>>          return -EINVAL;
>>>
>>> -    idx = find_cable_index_by_id(edev, id);
>>> -    if (idx < 0)
>>> -        return idx;
>>> +    if ((int)id != -1) {
>>> +        idx = find_cable_index_by_id(edev, id);
>>> +        if (idx < 0)
>>> +            return idx;
>>> +    }
>>>
>>>      spin_lock_irqsave(&edev->lock, flags);
>>> -    ret = raw_notifier_chain_unregister(&edev->nh[idx], nb);
>>> +    if ((int)id != -1)
>>> +        ret = raw_notifier_chain_unregister(&edev->nh[idx], nb);
>>> +    else
>>> +        ret = raw_notifier_chain_unregister(&edev->nh_all, nb);
>>> +
>>>      spin_unlock_irqrestore(&edev->lock, flags);
>>>
>>>      return ret;
>>> @@ -1212,6 +1227,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 993ddcc..2e6c09d 100644
>>> --- a/drivers/extcon/extcon.h
>>> +++ b/drivers/extcon/extcon.h
>>> @@ -44,6 +44,7 @@ struct extcon_dev {
>>>      /* Internal data. Please do not set. */
>>>      struct device dev;
>>>      struct raw_notifier_head *nh;
>>> +    struct raw_notifier_head nh_all;
>>>      struct list_head entry;
>>>      int max_supported;
>>>      spinlock_t lock;    /* could be called by irq handler */
>>>
>>
>>

Powered by blists - more mailing lists