From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755495AbdC2Juz (ORCPT ); Wed, 29 Mar 2017 05:50:55 -0400 Received: from mailout2.samsung.com ([203.254.224.25]:31392 "EHLO epoutp02.samsung.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755116AbdC2Juv (ORCPT ); Wed, 29 Mar 2017 05:50:51 -0400 X-AuditID: b6c32a59-f79166d0000017ce-7e-58db83785a12 MIME-version: 1.0 Content-transfer-encoding: 8BIT Content-type: text/plain; charset="utf-8" Message-id: <58DB8377.3030507@samsung.com> Date: Wed, 29 Mar 2017 18:50:47 +0900 From: Chanwoo Choi Organization: Samsung Electronics User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: Hans de Goede , MyungJoo Ham Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev In-reply-to: <41014ea2-a76f-1745-0723-e269cbb8adef@redhat.com> X-Brightmail-Tracker: H4sIAAAAAAAAA01SbUhTYRTm3d3mVVzd5kcH+3Bd6EPDuU1nM5xlRS2KWkm0itCbvmwjt9nu FhURSlTD0jJBaQhGpqkU1pQyQ6tZWFQqmuIHzT60iRQhSVoZte0q+e855zzPed734ZCEeFIQ RRrNNmw1Mzm0MIR/vy1mddyJs0M6We9omOpLexlP1dNcLlQN5dcINxKab629Qk1RYx3SfHct 1xIHcYoBM9nYKsHmLEu20axX0zvSMzZnKJNk8jh5smodLTEzJqymt+zUxm015viMaMlxJsfu a2kZlqXjU1OsFrsNSwwW1qamD8nlCqlctk6qUCikiQmH1yuUPkomNnjf1wpyv+MTE8Nn8tDn 3QUomAQqEbzvXDwOR0KXp15YgEJIMVWFoPlFCeKKCzxwn2tAc4rpnhIBN7iF4P7lisBARC2C 6RIPvwCRJEFFw7Puo/42QcXA2ORVPsf3ILh3zc3zc0RULLSUGf0cPrUSSvs/B/mx0Nd+PNYv 9OOF1Aronf4UWB9B6eBhxY8AJ5zaC/1TNQJuvwR+eVwBHEZhKKxsCmiDqVTofJFH+H2BuhAE N0rdQX5foJaB6wnB/WULjM44hRwOg/H2xiAOL4E/VYNoVougodzD5woHguHafAHHSoDh9x6C e8UCKPw9wuMMROA4L+YoGqiY8MwapEFZUQ2PC8LLg5nxj8QVJHHOy875PzvnvOyuI6IOReJc 1qTHrDJXKWUZE2s366VZFpMLBS4u9kATenttrxtRJKJDRTU/BnViAXOcPWlyIyAJOlwUv2JI JxZlMydPYaslw2rPwawbKX3ZFxNREVkW3/2abRnyxGRZojIpQZYgVyXTi0Vravt0YkrP2PBR jHOxdU7HI4Oj8lBe5K6WR5dG90zsbzoUrT5TfZMcC/1AIufr7H2nx+5UDvyODh2pq75YeuxV xKYmS4x2vOpwm0Otqw9JO7b9gfYubTGpby162eG2wc+LXzMNi7cRjfVmx8ht576lSyXixoG1 Ku/4c0dre2Xt3+Kkp+md3W+6jB0bZo60lU9M/VxV3UfzWQMjjyWsLPMP/SXDuocDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprDIsWRmVeSWpSXmKPExsVy+t9jAd2K5tsRBuv2sVq8OT6dyeLyrjls FrcbV7A5MHu833eVzaNvyypGj8+b5AKYo9xsMlITU1KLFFLzkvNTMvPSbZVCQ9x0LZQU8hJz U22VInR9Q4KUFMoSc0qBPCMDNODgHOAerKRvl+CW8fzBStaCz6kVH+/XNTA+8+9i5OSQEDCR +HF5MiuELSZx4d56NhBbSGApo0TbLT0Qm1dAUOLH5HssXYwcHMwC8hJHLmVDmOoSU6bkdjFy AVU/YJT4OWcmG0icV0BLYu/0TJBOFgFViWk3nrGD2GxA4f0vboBN5xdQlLj64zEjSLmoQIRE 94lKkLCIQIDEz1P9YOXMAgoSv+5tAjtMWCBVonfxDjaIVS+ZJL5P/sgCkuAUsJM4f6KBeQKj 4Cwkh85COHQWwqELGJlXMUqkFiQXFCel5xrlpZbrFSfmFpfmpesl5+duYgTHzDPpHYyHd7kf YhTgYFTi4d2RdytCiDWxrLgy9xCjBAezkgivvuLtCCHelMTKqtSi/Pii0pzU4kOMpkCvTmSW Ek3OB8ZzXkm8oYm5ibmxgYW5paWJkZI4b+PsZ+FCAumJJanZqakFqUUwfUwcnFINjNK/9KxP WrN7z9/2+9hNBp5jKYn7mX9eTAjL+pmb9eqKB0vy03C3+tls/u4V/XW6tkEB91L9PDg9Mm03 ynqvkfjx2HPTTDX350unC+92FdrAEHLijNbL1ETD2ycO7JetPBJldsfs1LFps40u9Mfxz/SS yOh4+0Bv3xppySMJqv5GEhxGfz4yKLEUZyQaajEXFScCAC43J7uvAgAA X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170329095048epcas5p35a2edd28ed61e9016b7d3ee455291428 X-Msg-Generator: CA X-Sender-IP: 203.254.230.27 X-Local-Sender: =?UTF-8?B?7LWc7LCs7JqwG1RpemVuIFBsYXRmb3JtIExhYihTL1fshLw=?= =?UTF-8?B?7YSwKRvsgrzshLHsoITsnpAbU2VuaW9yIEVuZ2luZWVy?= X-Global-Sender: =?UTF-8?B?Q2hhbndvbyBDaG9pG1RpemVuIFBsYXRmb3JtIExhYi4bU2Ft?= =?UTF-8?B?c3VuZyBFbGVjdHJvbmljcxtTZW5pb3IgRW5naW5lZXI=?= X-Sender-Code: =?UTF-8?B?QzEwG1NUQUYbQzEwVjgxMTE=?= CMS-TYPE: 105P DLP-Filter: Pass X-CFilter-Loop: Reflected X-CMS-RootMailID: 20170319182339epcas4p16545551b1d3a114b9426edc54c610ac1 X-RootMTR: 20170319182339epcas4p16545551b1d3a114b9426edc54c610ac1 References: <20170319182334.6456-1-hdegoede@redhat.com> <58CF288A.9090507@samsung.com> <0ea05e7a-a86a-b770-f113-c85d0e3fc11e@redhat.com> <58DB5CE5.7050605@samsung.com> <41014ea2-a76f-1745-0723-e269cbb8adef@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 2017년 03월 29일 16:38, Hans de Goede wrote: > Hi, > > On 29-03-17 09:06, Chanwoo Choi wrote: >> 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) > > Adding a new extcon_register_all_notifier function for this is fine with > me, shall I send a new patch for this ? I'll develop the new following functions with your reported-by. 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); > > Regards, > > Hans > > > > >> >> >>> >>> 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 >>>>>> --- >>>>>> 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