Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Chanwoo Choi]

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 <hdegoede@xxxxxxxxxx>
>
> 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