Messages in this thread Patch in this message | | | Subject | Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors | From | Hans de Goede <> | Date | Thu, 30 Mar 2017 16:58:13 +0200 |
| |
Hi,
On 30-03-17 11:20, Chanwoo Choi wrote: > On 2017년 03월 30일 18:04, Hans de Goede wrote: >> Hi Chanwoo, >> >> On 30-03-17 10:39, Chanwoo Choi wrote: >>> The extcon core already provides the extcon_register_notifier() function >>> in order to register the notifier block which is used to monitor >>> the status change for the specific external connector such as EXTCON_USB, >>> EXTCON_USB_HOST and so on. The extcon consumer uses the this function. >>> >>> The extcon consumer may need to monitor the all supported external >>> connectors from the extcon device. In this case, The extcon consumer >>> should have each notifier_block structure for each external connector. >>> >>> This patch adds the new extcon_register_notifier_all() function >>> that extcon consumer is able to monitor the status change of all >>> supported external connectors by using only one notifier_block structure. >>> >>> - List of new added functions: >>> 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); >>> >>> Suggested-by: Hans de Goede <hdegoede@redhat.com> >>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >>> --- >>> drivers/extcon/devres.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ >>> drivers/extcon/extcon.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++ >>> drivers/extcon/extcon.h | 3 +++ >>> include/linux/extcon.h | 21 ++++++++++++---- >>> 4 files changed, 145 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/extcon/devres.c b/drivers/extcon/devres.c >>> index b40eb1805927..3fc8eea52f1d 100644 >>> --- a/drivers/extcon/devres.c >>> +++ b/drivers/extcon/devres.c >>> @@ -50,6 +50,13 @@ static void devm_extcon_dev_notifier_unreg(struct device *dev, void *res) >>> extcon_unregister_notifier(this->edev, this->id, this->nb); >>> } >>> >>> +static void devm_extcon_dev_notifier_all_unreg(struct device *dev, void *res) >>> +{ >>> + struct extcon_dev_notifier_devres *this = res; >>> + >>> + extcon_unregister_notifier_all(this->edev, this->nb); >>> +} >>> + >>> /** >>> * devm_extcon_dev_allocate - Allocate managed extcon device >>> * @dev: device owning the extcon device being created >>> @@ -214,3 +221,60 @@ void devm_extcon_unregister_notifier(struct device *dev, >>> devm_extcon_dev_match, edev)); >>> } >>> EXPORT_SYMBOL(devm_extcon_unregister_notifier); >>> + >>> +/** >>> + * devm_extcon_register_notifier_all() >>> + * - Resource-managed extcon_register_notifier_all() >>> + * @dev: device to allocate extcon device >>> + * @edev: the extcon device that has the external connecotr. >>> + * @nb: a notifier block to be registered. >>> + * >>> + * This function manages automatically the notifier of extcon device using >>> + * device resource management and simplify the control of unregistering >>> + * the notifier of extcon device. >>> + * >>> + * Note that the second parameter given to the callback of nb (val) is >>> + * the current state and third parameter is the edev pointer. >>> + * >>> + * Returns 0 if success or negaive error number if failure. >>> + */ >>> +int devm_extcon_register_notifier_all(struct device *dev, struct extcon_dev *edev, >>> + struct notifier_block *nb) >>> +{ >>> + struct extcon_dev_notifier_devres *ptr; >>> + int ret; >>> + >>> + ptr = devres_alloc(devm_extcon_dev_notifier_all_unreg, sizeof(*ptr), >>> + GFP_KERNEL); >>> + if (!ptr) >>> + return -ENOMEM; >>> + >>> + ret = extcon_register_notifier_all(edev, nb); >>> + if (ret) { >>> + devres_free(ptr); >>> + return ret; >>> + } >>> + >>> + ptr->edev = edev; >>> + ptr->nb = nb; >>> + devres_add(dev, ptr); >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL(devm_extcon_register_notifier_all); >>> + >>> +/** >>> + * devm_extcon_unregister_notifier_all() >>> + - Resource-managed extcon_unregister_notifier_all() >>> + * @dev: device to allocate extcon device >>> + * @edev: the extcon device that has the external connecotr. >>> + * @nb: a notifier block to be registered. >>> + */ >>> +void devm_extcon_unregister_notifier_all(struct device *dev, >>> + struct extcon_dev *edev, >>> + struct notifier_block *nb) >>> +{ >>> + WARN_ON(devres_release(dev, devm_extcon_dev_notifier_all_unreg, >>> + devm_extcon_dev_match, edev)); >>> +} >>> +EXPORT_SYMBOL(devm_extcon_unregister_notifier_all); >>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c >>> index 193a3a673d10..4873fe8d5cd8 100644 >>> --- a/drivers/extcon/extcon.c >>> +++ b/drivers/extcon/extcon.c >>> @@ -445,8 +445,19 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id) >>> spin_lock_irqsave(&edev->lock, flags); >>> >>> state = !!(edev->state & BIT(index)); >>> + >>> + /* >>> + * Call functions in a raw notifier chain for the specific one >>> + * external connector. >>> + */ >>> raw_notifier_call_chain(&edev->nh[index], state, edev); >>> >>> + /* >>> + * Call functions in a raw notifier chain for the all supported >>> + * external connectors. >>> + */ >>> + raw_notifier_call_chain(&edev->nh_all, state, edev); >>> + >> >> Passing state here is not useful since the receiver of >> the notifier call will not know for which cable the state is. > > No, the receiver can use the 'state' value whether connector is attached > or detached. Also, the extcon have to keep the same value > for both extcon_register_notifier() and extcon_register_notfier_all().
Ok.
>> Also this should be moved outside of the area of the >> function holding the edev->lock spinlock, since we don't >> pass state we do not need the lock and the called >> notifier function may very well want to call extcon_get_state >> to find out the state of one or more of the cables, >> which takes the lock. > > The extcon uses the spinlock for the short delay. > Extcon should update the status of external connector > to the extcon consumer as soon as possible.
Right, what I'm suggestion actually also applies to the current cable notification, what I'm suggesting is to move the notification like this:
--- a/drivers/extcon/extcon.c +++ b/drivers/extcon/extcon.c @@ -448,8 +448,6 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id) spin_lock_irqsave(&edev->lock, flags);
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); @@ -482,6 +480,10 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
/* Unlock early before uevent */ spin_unlock_irqrestore(&edev->lock, flags); + + raw_notifier_call_chain(&edev->nh[index], state, edev); + raw_notifier_call_chain(&edev->nh_all, 0, edev); + kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp); free_page((unsigned long)prop_buf);
So that the nb.notifier_call function can call extcon_get_state to find out what cable is plugged in without deadlocking because extcon_get_state does spin_lock_irqsave(&edev->lock, flags) too.
This is esp. important for the edev->nh_all notifier chain since when used in charger drivers the callback will want to call extcon_get_state for all of: EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_DCP, EXTCON_CHG_USB_CDP, etc. to find out how much current it can draw from the port.
AFAICT tell there is no race in moving this outside of the locked section of extcon_sync() and it avoids potential deadlocks in the nb.notifier_call function.
Regards,
Hans
>>> /* This could be in interrupt handler */ >>> prop_buf = (char *)get_zeroed_page(GFP_ATOMIC); >>> if (!prop_buf) { >>> @@ -951,6 +962,55 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id, >>> } >>> EXPORT_SYMBOL_GPL(extcon_unregister_notifier); >>> >>> +/** >>> + * extcon_register_notifier_all() - Register a notifier block to get the noti >>> + * of the status change for all supported external >>> + * connectors from extcon. >>> + * @edev: the extcon device that has the external connecotr. >>> + * @nb: a notifier block to be registered. >>> + * >>> + * Note that the second parameter given to the callback of nb (val) is >>> + * the current state and third parameter is the edev pointer. >>> + */ >>> +int extcon_register_notifier_all(struct extcon_dev *edev, >>> + struct notifier_block *nb) >>> +{ >>> + unsigned long flags; >>> + int ret; >>> + >>> + if (!edev || !nb) >>> + return -EINVAL; >>> + >>> + spin_lock_irqsave(&edev->lock, flags); >>> + ret = raw_notifier_chain_register(&edev->nh_all, nb); >>> + spin_unlock_irqrestore(&edev->lock, flags); >>> + >>> + return ret; >>> +} >>> +EXPORT_SYMBOL_GPL(extcon_register_notifier_all); >>> + >>> +/** >>> + * extcon_unregister_notifier_all() - Unregister a notifiee from the extcon device. >>> + * @edev: the extcon device that has the external connecotr. >>> + * @nb: a notifier block to be registered. >>> + */ >>> +int extcon_unregister_notifier_all(struct extcon_dev *edev, >>> + struct notifier_block *nb) >>> +{ >>> + unsigned long flags; >>> + int ret; >>> + >>> + if (!edev || !nb) >>> + return -EINVAL; >>> + >>> + spin_lock_irqsave(&edev->lock, flags); >>> + ret = raw_notifier_chain_unregister(&edev->nh_all, nb); >>> + spin_unlock_irqrestore(&edev->lock, flags); >>> + >>> + return ret; >>> +} >>> +EXPORT_SYMBOL_GPL(extcon_unregister_notifier_all); >>> + >>> static struct attribute *extcon_attrs[] = { >>> &dev_attr_state.attr, >>> &dev_attr_name.attr, >>> @@ -1199,6 +1259,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 993ddccafe11..dddddcfa0587 100644 >>> --- a/drivers/extcon/extcon.h >>> +++ b/drivers/extcon/extcon.h >>> @@ -21,6 +21,8 @@ >>> * @dev: Device of this extcon. >>> * @state: Attach/detach state of this extcon. Do not provide at >>> * register-time. >>> + * @nh_all: Notifier for the state change events for all supported >>> + * external connectors from this extcon. >>> * @nh: Notifier for the state change events from this extcon >>> * @entry: To support list of extcon devices so that users can >>> * search for extcon devices based on the extcon name. >>> @@ -43,6 +45,7 @@ struct extcon_dev { >>> >>> /* Internal data. Please do not set. */ >>> struct device dev; >>> + struct raw_notifier_head nh_all; >>> struct raw_notifier_head *nh; >>> struct list_head entry; >>> int max_supported; >>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h >>> index 3929d2e8a3c7..61e8d8c591a4 100644 >>> --- a/include/linux/extcon.h >>> +++ b/include/linux/extcon.h >>> @@ -270,11 +270,11 @@ extern int extcon_set_property_capability(struct extcon_dev *edev, >>> unsigned int id, unsigned int prop); >>> >>> /* >>> - * Following APIs are to monitor every action of a notifier. >>> - * Registrar gets notified for every external port of a connection device. >>> - * Probably this could be used to debug an action of notifier; however, >>> - * we do not recommend to use this for normal 'notifiee' device drivers who >>> - * want to be notified by a specific external port of the notifier. >>> + * Following APIs are to monitor the status change of the external connectors. >>> + * extcon_register_notifier(*edev, id, *nb) : Register a notifier >>> + * for specific external connector from the extcon. >>> + * extcon_register_notifier_all(*edev, *nb) : Register a notifier >>> + * for all supported external connectors from the extcon. >>> */ >>> extern int extcon_register_notifier(struct extcon_dev *edev, unsigned int id, >>> struct notifier_block *nb); >>> @@ -287,6 +287,17 @@ extern void devm_extcon_unregister_notifier(struct device *dev, >>> struct extcon_dev *edev, unsigned int id, >>> struct notifier_block *nb); >>> >>> +extern int extcon_register_notifier_all(struct extcon_dev *edev, >>> + struct notifier_block *nb); >>> +extern int extcon_unregister_notifier_all(struct extcon_dev *edev, >>> + struct notifier_block *nb); >>> +extern int devm_extcon_register_notifier_all(struct device *dev, >>> + struct extcon_dev *edev, >>> + struct notifier_block *nb); >>> +extern void devm_extcon_unregister_notifier_all(struct device *dev, >>> + struct extcon_dev *edev, >>> + struct notifier_block *nb); >>> + >>> /* >>> * Following API get the extcon device from devicetree. >>> * This function use phandle of devicetree to get extcon device directly. >>> >> >> >> > >
| |