From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753955AbdC2HGX (ORCPT ); Wed, 29 Mar 2017 03:06:23 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:57453 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753420AbdC2HGV (ORCPT ); Wed, 29 Mar 2017 03:06:21 -0400 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 X-AuditID: b6c32a2e-f79506d0000046c0-70-58db5ce5e3b1 Content-transfer-encoding: 8BIT Message-id: <58DB5CE5.7050605@samsung.com> Date: Wed, 29 Mar 2017 16:06:13 +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: <0ea05e7a-a86a-b770-f113-c85d0e3fc11e@redhat.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrCKsWRmVeSWpSXmKPExsWy7bCmuu7TmNsRBvObTCzeHJ/OZHF51xw2 i9uNK9gcmD3e77vK5tG3ZRWjx+dNcgHMUak2GamJKalFCql5yfkpmXnptkrewfHO8aZmBoa6 hpYW5koKeYm5qbZKLj4Bum6ZOUCLlBTKEnNKgUIBicXFSvp2NkX5pSWpChn5xSW2StGGhkZ6 hgbmekZGRnomxrFWRqZAJQmpGSuPnmUtOBlZMWdWeQPjbLcuRk4OCQETia37/zBC2GISF+6t Z+ti5OIQEljKKLFrxRpWCKedSaLz3DlmmI6VW++xQCSWM0o8al3JDpLgFRCU+DEZJMHBwSwg L3HkUjZImFlAU+LFl0lQ9fcYJVa96GeFqNeSuL/sGBNIPYuAqsTFO9UgYTag8P4XN9hAbH4B RYmrPx6DXScqECGxc/43sFUiAkESN76vYIWYryDx694mMFtYIFWid/EOsF5OATuJT8tamUH2 Sgi0s0scPHOQDWSXhICsxKYDUL+4SDQ9n8ICYQtLvDq+hR3Clpb4u/QWI1Qvo8TmORAPSwh0 MErcX9nIClFlLHH/wT1miCv4JHp/P2GCWMAr0dEmBFHiITH/4z02CNtRYnrfCiZIQExikpjV 94h9AqPCLKSwm4UIu1lIYbeAkXkVo1hqQXFuemqxaYGxXnFibnFpXrpecn7uJkZwitPS28H4 b4H3IUYBDkYlHt6MfbcihFgTy4orcw8xSnAwK4nw6ivejhDiTUmsrEotyo8vKs1JLT7EaAoM +onMUqLJ+cD0m1cSb2hiaWBiamZsYGxoYakkzqux8lqEkEB6YklqdmpqQWoRTB8TB6dUA6PZ RcMD9lZnr+9rV3R/esPi6pyX/0NKu/mnOK5JEKi7Zz+j9KLSonKRqvKLn9a5lHnotzk55HVJ hojxXvazj370c/ep5ZdfTTXzmDqZ8+xxdfGlTz7vnR7o+W7ZZstf95pO60+Ycmr3U39lDk6u yYFW7yqu1LBYTNhhLLrGvi/or5Yp842/TySUWIozEg21mIuKEwFSn221hwMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprDIsWRmVeSWpSXmKPExsVy+t9jAd2nMbcjDB58FrN4c3w6k8XlXXPY LG43rmBzYPZ4v+8qm0ffllWMHp83yQUwR7nZZKQmpqQWKaTmJeenZOal2yqFhrjpWigp5CXm ptoqRej6hgQpKZQl5pQCeUYGaMDBOcA9WEnfLsEtY+XRs6wFJyMr5swqb2Cc7dbFyMkhIWAi sXLrPRYIW0ziwr31bF2MXBxCAksZJc5PmMAEkuAVEJT4MRmkiIODWUBe4silbAhTXWLKlFyI 8geMErM7PkOVa0ncX3aMCaSGRUBV4uKdapAwG1B4/4sbbCA2v4CixNUfjxlBSkQFIiS6T1SC hEUEAiR+nupnB7GZBRQkft3bxApiCwukSvQu3gHWKiQwhUni4VN5EJtTwE7i07JW5gmMgrOQ 3DkL4c5ZCHcuYGRexSiRWpBcUJyUnmuUl1quV5yYW1yal66XnJ+7iREcM8+kdzAe3uV+iFGA g1GJh3dH3q0IIdbEsuLK3EOMEhzMSiK8+oq3I4R4UxIrq1KL8uOLSnNSiw8xmgI9OpFZSjQ5 HxjPeSXxhibmJubGBhbmlpYmRkrivI2zn4ULCaQnlqRmp6YWpBbB9DFxcEo1MFbmmvMtnW3w 0tHfKD/iwt45K/faH3yyb5X8tcaqO8t1lQ5NkPsZFRLpdPWlFbvdpcf5368aNM7u+fdfoV55 j4TAtKMurLmKpc8uZNeeznmTtdlEMvfuZ5YXV8301wUtkXiQOk0oISp1csOx1OzTXf8vqv14 uPTi4rULGtv/HLydtbmsUs2cZ5YSS3FGoqEWc1FxIgBK8cDRrwIAAA== X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170329070613epcas5p1879b6e95da9be5fea2274de2f2e94461 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-HopCount: 7 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> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>> --- >>>> 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