lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 29 Mar 2017 11:51:38 +0200 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 29-03-17 11:50, Chanwoo Choi wrote: > 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); Looks good to me, thank you for looking into this. When you've a patch ready please Cc me and I'll test it. Regards, Hans > > >> >> 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 <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