Hi, On 2017년 04월 03일 20:14, Hans de Goede wrote: > Hi, > > On 03-04-17 09:24, Chanwoo Choi wrote: >> Hi, >> >> On 2017년 03월 30일 23:58, Hans de Goede wrote: >>> Hi, >>> >>> On 30-03-17 11:20, Chanwoo Choi wrote: >>>> On 2017년 03월 30일 18:04, Hans de Goede wrote: > > <snip> > >>>>> 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. >> >> >> Actually, I knew that if the extcon consumer driver uses >> the extcon_get_state() in the callback function, there is a deadlock >> between extcon_sync() and extcon_get_state(). So, all extcon consumer >> uses the workqueue when receiving the notfication from extcon. >> >> But, extcon_sync() have to call the number of callback functions >> in the notifier chanin. If one specific extcon consumer spend many >> time in the own callback function, other extcon consumer might receive >> the notification late. So, I think that each extcon consumer >> better to use the work in the their callback function. >> >> As I already said, I think that extcon focus on sending the notification >> to all of extcon consumers. > > Ok, then lets keep your patches as they are, I've added the patches > from your extcon-test branch to my local repository, modified the drivers > which I've pending upstream which need this to use the new functionality > and tested things. > > Everything looks and works good with these patches, so please add my: > > Acked-and-Tested-by: Hans de Goede <hdego...@redhat.com> > > to them. > > It would be great if you can push these patches to extcon-next and > then create a stable branch with these patches which other subsys > maintainers can merge, so that I can start submitting patches which > need this upstream (and also do a cleanup patch for the current axp288 > charger code). >
Sure, After reviewing the patches, I'll make the immutable branch and send the pull request for other subsystem maintainer as you mentioned. -- Best Regards, Chanwoo Choi Samsung Electronics
