From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752199AbdDDExw (ORCPT ); Tue, 4 Apr 2017 00:53:52 -0400 Received: from mailout4.samsung.com ([203.254.224.34]:56267 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751082AbdDDExu (ORCPT ); Tue, 4 Apr 2017 00:53:50 -0400 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 X-AuditID: b6c32a36-f79446d000002bcd-a7-58e326dccc3b Content-transfer-encoding: 8BIT Message-id: <58E326DB.8050902@samsung.com> Date: Tue, 04 Apr 2017 13:53: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 , linux-kernel@vger.kernel.org Cc: chanwoo@kernel.org, myungjoo.ham@samsung.com Subject: Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors In-reply-to: <655587e2-5463-a5aa-9ddb-b26d6acbe545@redhat.com> X-Brightmail-Tracker: H4sIAAAAAAAAA02Se0hTcRTH+e3eXae0+rWyTo5qXshQcu7OaTNS7Mkqi0UFI4l1mb9U2u7G 7pSKCIWyJall0WMMe0HpChJ7W1nYw6InqREVq7RSe1lUalTatlvkf5/fOefL9/y+HAWlGpTH KQoFD3ELvJ1lYuiz1xJTkp8ndFp0VZXxxp1P2mjjh5a9MmNro58xPiutZbJpU0NgG2PqbWpn TJWnA8j0tWGSmV5JZhYQPo+4NUSwOfMKhfxMdtEy6xxrWrqOS+YyjNNZjcA7SCY7N8ecPL/Q HrJkNcW8vShUMvOiyKZkzXQ7izxEU+AUPZlsLsfptZxuulav12sNqatm6NNCI6tJQXPjbrnr Wvy6tv5uWQkqUZejaAVgA1xpfSaXeBw8DJ5kwqzC5xEMXUotRzEh3ioD780v6J/gztGrSGr4 EZx55aPCDSUeDQO7gnQ5UigoPBmuP1obLlM4Ebq/VdPSfBDBr7dltDSfBB2H6yPONJ4Ch7rK IsyE6le6n0S2GIXjoX2gM2Iciy1w4UBfVJjH4nlw4OzFKMlABx3tTRHtGGyHE039EW00zoLA rUpZ2Bjw3ij4sa8ShZcDPBEarlLSZ+ZCy/V7jMRj4F3L6SiJ1RAYfPpXuxXBKX+Qlh5eBC/q Sv/mlQovXgYpaYuR8On7drlkoARvmUpCE1TfXi1Nz4Ku+98YKYhTFPSU+ukdSOMblp3vf3a+ YdkdRFQAjSMu0ZFPRM6l14q8QywS8rU2p6MBRW4vKf08Onw/pxlhBWJHKA/O7rCo5HyxuN7R jEBBsWOVfZpOi0qZx6/fQNxOq7vITsRmlBaKficVF2tzhi5Z8Fg5Q4bOkK4PXV0Gx7HjlbEn H1tUOJ/3kLWEuIj7n06miI4rQdM+VSzZbN4CHT6hqs534bdr0uvMe+bvqecuH/vYNcBkE+uX FWpO6ElZ4Bxa+nqkqHt1V3Xj836cWFth2PP+Za7t1vGWqYs8seya2qMLc3rUR3KVm94srl1Y o67xJtnqG3v9PfWt5cvr7X24rS35waWEz+8nZHf/jFH3V+d6N57IY2mxgOeSKLfI/wEZuvv3 kQMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrBIsWRmVeSWpSXmKPExsVy+t9jQd07ao8jDB7sELWYeOMKi8Wb49OZ LC7vmsNmcbtxBZsDi8emVZ1sHu/3XWXz6NuyitHj8ya5AJYoN5uM1MSU1CKF1Lzk/JTMvHRb pdAQN10LJYW8xNxUW6UIXd+QICWFssScUiDPyAANODgHuAcr6dsluGUc2jWFteCwYsWV7y+Y GhgbpLsYOTkkBEwkTi87wAhhi0lcuLeerYuRi0NIYBajxOQzP9hBErwCghI/Jt9j6WLk4GAW kJc4cikbwlSXmDIlF6RCSOABo8T27hSIai2JR4s2sILYLAKqEguft4HZbEDx/S9usIHY/AKK Eld/PGYEGSMqECHRfaISJCwi4Coxf9tusKXMAgYSj67uA2sVFsiRWLPvO9Rlm5klDp95yAKS 4BSwk1h1oo9pAqPgLCSHzkI4dBbCoQsYmVcxSqQWJBcUJ6XnGuallusVJ+YWl+al6yXn525i BMfSM6kdjAd3uR9iFOBgVOLhXeD0KEKINbGsuDL3EKMEB7OSCO83hccRQrwpiZVVqUX58UWl OanFhxhNgV6dyCwlmpwPjPO8knhDE3MTc2MDC3NLSxMjJXHextnPwoUE0hNLUrNTUwtSi2D6 mDg4pRoY1bODptycdN00oEzQs+jBw1nPLf1YDvzLeyZSwczJWyOjx5D6N6tV/yFf/Nr8X5q6 SVOEDJ5Pkjw5vVH3lMelWfzii6flGPduufxhy26FN3YKq/YcDHq1e+f8j2E3oqNObX4azVDz 6aKL6Yoo87xVQhtWMk/I26DyrDPPJUyD9YyeuSADT2GAEktxRqKhFnNRcSIAOe+4Q7sCAAA= X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170404045348epcas1p402b6898a2b7a06f8e70b0be5f5229ebd X-Msg-Generator: CA X-Sender-IP: 203.254.230.26 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: 101P DLP-Filter: Pass X-CFilter-Loop: Reflected X-HopCount: 7 X-CMS-RootMailID: 20170330083943epcas1p4c5559cab13ef732b6bf149f810aa2f46 X-RootMTR: 20170330083943epcas1p4c5559cab13ef732b6bf149f810aa2f46 References: <1490863178-12782-1-git-send-email-cw00.choi@samsung.com> <1490863178-12782-2-git-send-email-cw00.choi@samsung.com> <5ec5c655-2254-696d-cfca-029d5eb5f17c@redhat.com> <58DCCDED.6040605@samsung.com> <6c9c4abb-2d54-6ec3-93f4-24da903da650@redhat.com> <58E1F8AD.9090409@samsung.com> <655587e2-5463-a5aa-9ddb-b26d6acbe545@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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: > > > >>>>> 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 > > 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