Re: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev

From: Chanwoo Choi
Date: Wed Mar 29 2017 - 03:06:25 EST

Next message: Greg KH: "Re: [PATCH] Remove atomisp/i2c style errors."
Previous message: Dmitry Torokhov: "Re: [PATCH v1 4/8] gpio: acpi: Even more tighten up ACPI GPIO lookups"
In reply to: Hans de Goede: "Re: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev"
Next in thread: Hans de Goede: "Re: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 2017ë 03ì 25ì 23:03, Hans de Goede wrote:
> 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 ?

As you said, when using one notifier_block for multiple external connector,
it has a problem. But, it is not only for extcon framework. It depends
on the notifier_block code. If each external connector uses the
each notifier_block (it is a not hack), there is no any problem.

As I already said on my reply[1] as following,
---------------------------------------------
The extcon_register_notifier() function must support the notification
for only one external connector.
[1] https://www.spinics.net/lists/kernel/msg2468906.html
---------------------------------------------

I agree that the EXTCON needs to support the notification
for all supported external connector of extcon device
by using only one notifier_chain.

I don't prefer to use the '-1' as the unique id for getting the
notification for all external connectors. Each argument of function
must have the correct meaning.

I think that thanks for your report and suggestion.

I'm considering to add the new function as following:
Following function will support the all external connector for notification
and the function argument don't include the unique id for external connector.
- extcon_regiter_all_notifier(struct extcon_dev *edev, struct notifier_block *nb);
(The function name is not fixed)

>
> 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@xxxxxxxxxx>
>>>> ---
>>>> 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 */
>>>>
>>>
>>>
>
>

--
Best Regards,
Chanwoo Choi
Samsung Electronics

Next message: Greg KH: "Re: [PATCH] Remove atomisp/i2c style errors."
Previous message: Dmitry Torokhov: "Re: [PATCH v1 4/8] gpio: acpi: Even more tighten up ACPI GPIO lookups"
In reply to: Hans de Goede: "Re: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev"
Next in thread: Hans de Goede: "Re: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]