Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1615111

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

Show all headers | View raw

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@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).

Regards,

Hans

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 1/2] extcon: Use BIT() macro for the left-shift operation Chanwoo Choi <cw00.choi@samsung.com> - 2017-03-30 10:40 +0200
  [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor  all external connectors Chanwoo Choi <cw00.choi@samsung.com> - 2017-03-30 10:50 +0200
    Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to  monitor all external connectors Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-03-30 11:10 +0200
      Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to  monitor all external connectors Chanwoo Choi <cw00.choi@samsung.com> - 2017-03-30 11:30 +0200
        Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to  monitor all external connectors Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-03-30 12:50 +0200
          Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to  monitor all external connectors Chanwoo Choi <cw00.choi@samsung.com> - 2017-03-30 13:00 +0200
            Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to  monitor all external connectors Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-03-30 13:10 +0200
    Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to  monitor all external connectors Hans de Goede <hdegoede@redhat.com> - 2017-03-30 11:20 +0200
      Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to  monitor all external connectors Chanwoo Choi <cw00.choi@samsung.com> - 2017-03-30 11:30 +0200
        Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to  monitor all external connectors Hans de Goede <hdegoede@redhat.com> - 2017-03-30 17:00 +0200
          Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to  monitor all external connectors Chanwoo Choi <cw00.choi@samsung.com> - 2017-04-03 09:30 +0200
            Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to  monitor all external connectors Hans de Goede <hdegoede@redhat.com> - 2017-04-03 13:20 +0200
              Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to  monitor all external connectors Chanwoo Choi <cw00.choi@samsung.com> - 2017-04-04 07:00 +0200
              Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to  monitor all external connectors Hans de Goede <hdegoede@redhat.com> - 2017-04-04 12:50 +0200
                Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to  monitor all external connectors Chanwoo Choi <cw00.choi@samsung.com> - 2017-04-04 13:00 +0200
  Re: [PATCH 1/2] extcon: Use BIT() macro for the left-shift operation Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-03-30 11:10 +0200
    Re: [PATCH 1/2] extcon: Use BIT() macro for the left-shift operation Chanwoo Choi <cw00.choi@samsung.com> - 2017-03-30 11:20 +0200
      Re: [PATCH 1/2] extcon: Use BIT() macro for the left-shift operation Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-03-30 12:40 +0200
        Re: [PATCH 1/2] extcon: Use BIT() macro for the left-shift  operation Chanwoo Choi <cw00.choi@samsung.com> - 2017-03-30 13:00 +0200
          Re: [PATCH 1/2] extcon: Use BIT() macro for the left-shift operation Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-03-30 13:20 +0200

csiph-web