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

[Hans de Goede]

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

Regards,

Hans