From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752826AbdDCLPJ (ORCPT ); Mon, 3 Apr 2017 07:15:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59458 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751435AbdDCLPG (ORCPT ); Mon, 3 Apr 2017 07:15:06 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D12893D953 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=hdegoede@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com D12893D953 Subject: Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors To: Chanwoo Choi , linux-kernel@vger.kernel.org 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> Cc: chanwoo@kernel.org, myungjoo.ham@samsung.com From: Hans de Goede Message-ID: <655587e2-5463-a5aa-9ddb-b26d6acbe545@redhat.com> Date: Mon, 3 Apr 2017 13:14:48 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <58E1F8AD.9090409@samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Mon, 03 Apr 2017 11:15:06 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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). Regards, Hans