diff mbox

[v4,1/5] gadget: Introduce the notifier functions

Message ID e58e4f26028c37877d02907831cf4ae411eaf984.1443057231.git.baolin.wang@linaro.org
State New
Headers show

Commit Message

(Exiting) Baolin Wang Sept. 24, 2015, 5:39 p.m. UTC
The usb charger framework is based on usb gadget. The usb charger
need to be notified the state changing of usb gadget to confirm the
usb charger state.

Thus this patch adds a notifier mechanism for usb gadget to report a
event to usb charger when the usb gadget state is changed.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/usb/gadget/udc/udc-core.c | 32 ++++++++++++++++++++++++++++++++
 include/linux/usb/gadget.h        | 18 ++++++++++++++++++
 2 files changed, 50 insertions(+)

Comments

Felipe Balbi Oct. 1, 2015, 5:29 p.m. UTC | #1
On Thu, Sep 24, 2015 at 10:39:23AM -0700, Baolin Wang wrote:
> The usb charger framework is based on usb gadget. The usb charger
> need to be notified the state changing of usb gadget to confirm the
> usb charger state.
> 
> Thus this patch adds a notifier mechanism for usb gadget to report a
> event to usb charger when the usb gadget state is changed.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/usb/gadget/udc/udc-core.c | 32 ++++++++++++++++++++++++++++++++
>  include/linux/usb/gadget.h        | 18 ++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> index f660afb..4238fc3 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
>  }
>  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
>  
> +int usb_gadget_register_notify(struct usb_gadget *gadget,
> +			       struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	mutex_lock(&gadget->lock);
> +	ret = raw_notifier_chain_register(&gadget->nh, nb);
> +	mutex_unlock(&gadget->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
> +
> +int usb_gadget_unregister_notify(struct usb_gadget *gadget,
> +				 struct notifier_block *nb)
> +{
> +	int ret;
> +
> +	mutex_lock(&gadget->lock);
> +	ret = raw_notifier_chain_unregister(&gadget->nh, nb);

Greg, this is the kind of thing I wanted to avoid adding more of.

I was wondering if you would accept subsystems using kdbus for
this sort of notification. I'm okay waiting for kdbus for another
couple merge windows (if we have to) before that's merged, but
if we take this raw notifier approach now, we will end up having
to support it forever.

Also, because soon enough we will have to support USB Power Delivery
with Type C connector, this is bound to change in the coming months.

Frankly, I wanted all of this to be decided in userland with the
kernel just providing notification and basic safety checks (we don't
want to allow a bogus userspace daemon frying anybody's devices).

How would you feel about that ?
Mark Brown Oct. 1, 2015, 5:43 p.m. UTC | #2
On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:

> Frankly, I wanted all of this to be decided in userland with the
> kernel just providing notification and basic safety checks (we don't
> want to allow a bogus userspace daemon frying anybody's devices).

What's the advantage of pushing this to userspace?  By the time we
provide enough discoverability to enable userspace to configure itself
it seems like we'd have enough information to do the job anyway.
Felipe Balbi Oct. 1, 2015, 5:58 p.m. UTC | #3
Hi,

On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote:
> On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:
> 
> > Frankly, I wanted all of this to be decided in userland with the
> > kernel just providing notification and basic safety checks (we don't
> > want to allow a bogus userspace daemon frying anybody's devices).
> 
> What's the advantage of pushing this to userspace?  By the time we
> provide enough discoverability to enable userspace to configure itself
> it seems like we'd have enough information to do the job anyway.

you're going to be dealing with a setup where each vendor does one thing
differently. Some will have it all in the SoC part of a single IP (dwc3 can be
configured that way), some will push it out to companion IC, some might even use
some mostly discrete setup and so on...

We're gonna be dealing with a decision that involves information from multiple
subsystems (USB, regulator, hwmon, power supply to name a few).

We tried doing it all in the kernel back in N800, N810 and N900/N9 days and it's
just plain difficult. To make matters worse, N900 had two USB PHYs, one for
actual runtime use and another just for detecting the charger type on the other
end.

On top of all that, we still need to figure out what to do when a particular
configuration gets chosen, and what to do when the bus goes into the different
suspend levels.

It's going to be a lot of policy getting added to the kernel. On top of all
that, when Type C and power delivery is finally a thing, there will an entire
new stack for USB C commands (using the Type C CC pins or modulated on VBUS for
legacy connectors) which we will have to add to the kernel. And different
devices will support different charging profiles (there are at least 6 of those,
IIRC).

With all these different things going on, it's best to do what e.g. NFC folks
did. Just a small set of hooks in the kernel, but actual implementation done by
a userspace daemon. This makes it even easier for middleware development since
we can provide a higher level API for middleware to talk to the charging daemon.

Another benefit is that this charging daemon can also give hints to power
management policy that e.g. we're charging at 10W or 100W and we can e.g. switch
to performance governor safely even though battery is rather low.

Anyway, there's really a lot that needs to happen and shuving it all in the
kernel is, IMHO, the wrong decision. I would be much happier with minimal safety
requirements in the kernel and let a userspace daemon (which we should certainly
provide a reference implementation) figure out what to do. This might be better
for things like Chromebooks and Android phones too which might make completely
different decisions given a certain charging profile.
Felipe Balbi Oct. 1, 2015, 6:01 p.m. UTC | #4
On Thu, Oct 01, 2015 at 12:58:49PM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote:
> > On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:
> > 
> > > Frankly, I wanted all of this to be decided in userland with the
> > > kernel just providing notification and basic safety checks (we don't
> > > want to allow a bogus userspace daemon frying anybody's devices).
> > 
> > What's the advantage of pushing this to userspace?  By the time we
> > provide enough discoverability to enable userspace to configure itself
> > it seems like we'd have enough information to do the job anyway.
> 
> you're going to be dealing with a setup where each vendor does one thing
> differently. Some will have it all in the SoC part of a single IP (dwc3 can be
> configured that way), some will push it out to companion IC, some might even use

oh, and as for dwc3 itself: it *can* be configured that way, but all those
charging blocks are optional :-) So you will even have setups where the very
same IP works differently because SoC vendor A configured it differently from
SoC vendor B.
Greg KH Oct. 2, 2015, 5:41 a.m. UTC | #5
On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:
> On Thu, Sep 24, 2015 at 10:39:23AM -0700, Baolin Wang wrote:
> > The usb charger framework is based on usb gadget. The usb charger
> > need to be notified the state changing of usb gadget to confirm the
> > usb charger state.
> > 
> > Thus this patch adds a notifier mechanism for usb gadget to report a
> > event to usb charger when the usb gadget state is changed.
> > 
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> >  drivers/usb/gadget/udc/udc-core.c | 32 ++++++++++++++++++++++++++++++++
> >  include/linux/usb/gadget.h        | 18 ++++++++++++++++++
> >  2 files changed, 50 insertions(+)
> > 
> > diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> > index f660afb..4238fc3 100644
> > --- a/drivers/usb/gadget/udc/udc-core.c
> > +++ b/drivers/usb/gadget/udc/udc-core.c
> > @@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
> >  }
> >  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
> >  
> > +int usb_gadget_register_notify(struct usb_gadget *gadget,
> > +			       struct notifier_block *nb)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&gadget->lock);
> > +	ret = raw_notifier_chain_register(&gadget->nh, nb);
> > +	mutex_unlock(&gadget->lock);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
> > +
> > +int usb_gadget_unregister_notify(struct usb_gadget *gadget,
> > +				 struct notifier_block *nb)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&gadget->lock);
> > +	ret = raw_notifier_chain_unregister(&gadget->nh, nb);
> 
> Greg, this is the kind of thing I wanted to avoid adding more of.
> 
> I was wondering if you would accept subsystems using kdbus for
> this sort of notification. I'm okay waiting for kdbus for another
> couple merge windows (if we have to) before that's merged, but
> if we take this raw notifier approach now, we will end up having
> to support it forever.

kdbus is userspace <-> userspace messages, it doesn't do kernel <->
userspace messages, sorry.

> Also, because soon enough we will have to support USB Power Delivery
> with Type C connector, this is bound to change in the coming months.
> 
> Frankly, I wanted all of this to be decided in userland with the
> kernel just providing notification and basic safety checks (we don't
> want to allow a bogus userspace daemon frying anybody's devices).
> 
> How would you feel about that ?

I agree I don't like new notifier chains being added, but as this is all
in-kernel, we can change the api in the future and there would not be a
problem, right?

And yeah, I'm worried about the USB Power delivery patches, I haven't
seen them yet, but I hear about different groups doing different things
here, and that worries me.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mark Brown Oct. 2, 2015, 4:47 p.m. UTC | #6
On Thu, Oct 01, 2015 at 12:58:49PM -0500, Felipe Balbi wrote:
> On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote:
> > On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:

> > > Frankly, I wanted all of this to be decided in userland with the
> > > kernel just providing notification and basic safety checks (we don't
> > > want to allow a bogus userspace daemon frying anybody's devices).

> > What's the advantage of pushing this to userspace?  By the time we
> > provide enough discoverability to enable userspace to configure itself
> > it seems like we'd have enough information to do the job anyway.


> you're going to be dealing with a setup where each vendor does one thing
> differently. Some will have it all in the SoC part of a single IP (dwc3 can be
> configured that way), some will push it out to companion IC, some might even use
> some mostly discrete setup and so on...

Right, and that was exactly what this was supposed to be all about when
I was discussing this originally with Baolin (who's on holiday until
sometime next week BTW, hence me answering).

> We're gonna be dealing with a decision that involves information from multiple
> subsystems (USB, regulator, hwmon, power supply to name a few).

Sure, that was part of the idea here - provide a central point
representing the logical port where we can aggregate all the information
we get in and distribute it to consumers.  

> We tried doing it all in the kernel back in N800, N810 and N900/N9 days and it's
> just plain difficult. To make matters worse, N900 had two USB PHYs, one for
> actual runtime use and another just for detecting the charger type on the other
> end.

Can you elaborate on what the difficulties are and how punting to
userspace helps?  If anything I'd expect punting to userspace to make
things more difficult, if nothing else it means we need to get whatever
userspace component deployed in all relevant userspace stacks with
appropriate configuration in order to do things.  

We do punt a lot of configuration to userspace for audio because there
are substantial device specific policy elements in the configuration and
it's a far from painless experience getting the code and configuration
deployed where people need it, definitely a not great for users.

> On top of all that, we still need to figure out what to do when a particular
> configuration gets chosen, and what to do when the bus goes into the different
> suspend levels.

> It's going to be a lot of policy getting added to the kernel. On top of all
> that, when Type C and power delivery is finally a thing, there will an entire
> new stack for USB C commands (using the Type C CC pins or modulated on VBUS for
> legacy connectors) which we will have to add to the kernel. And different
> devices will support different charging profiles (there are at least 6 of those,
> IIRC).

Yes, USB C was part of the thinking with starting this work - it's going
to make ensuring that the system is paying attention to limits much more
important.  I think there's two things here.  One is working out how the
system is glued together and the other thing is working out what to do
with that system.  

I think that where we can do it the bit where work out how the various
components are connected and aggregate their functionality together is
definitely a kernel job.  It means that userspace doesn't need to worry
about implementation details of the particular system and instead gets a
consistent, standard view of the device (in this case a USB port and how
much power we can draw from it).

For the policy stuff we do want to have userspace be able to control
things but we need to think about how to do that - for example does the
entire flow need to be in userspace, or can we just expose settings
which userspace can manage?

> With all these different things going on, it's best to do what e.g. NFC folks
> did. Just a small set of hooks in the kernel, but actual implementation done by
> a userspace daemon. This makes it even easier for middleware development since
> we can provide a higher level API for middleware to talk to the charging daemon.

I'm not sure that the NFC model is working well for everyone - it's OK
if you happen to be running Android but if you aren't you're often left
sitting there working out what to do with an Android HAL.  We can do the
middleware thing without going quite that far, we do already have power
management daemons in userspace even on desktop (GNOME has upowerd for
example).

> Another benefit is that this charging daemon can also give hints to power
> management policy that e.g. we're charging at 10W or 100W and we can e.g. switch
> to performance governor safely even though battery is rather low.

We definitely want userspace to be able to see the current state, even
if it just passes it straight on to the user it's a common part of UIs
on both desktop and mobile operating systems.

> Anyway, there's really a lot that needs to happen and shuving it all in the
> kernel is, IMHO, the wrong decision. I would be much happier with minimal safety
> requirements in the kernel and let a userspace daemon (which we should certainly
> provide a reference implementation) figure out what to do. This might be better
> for things like Chromebooks and Android phones too which might make completely
> different decisions given a certain charging profile.

Saying we don't want to have absolutely everything in kernel space
doesn't mean we should have nothing at all there, doing that seems like
going too far in the other direction.
Felipe Balbi Oct. 2, 2015, 5:23 p.m. UTC | #7
Hi,

On Fri, Oct 02, 2015 at 05:47:43PM +0100, Mark Brown wrote:
> On Thu, Oct 01, 2015 at 12:58:49PM -0500, Felipe Balbi wrote:
> > On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote:
> > > On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:
> 
> > > > Frankly, I wanted all of this to be decided in userland with the
> > > > kernel just providing notification and basic safety checks (we don't
> > > > want to allow a bogus userspace daemon frying anybody's devices).
> 
> > > What's the advantage of pushing this to userspace?  By the time we
> > > provide enough discoverability to enable userspace to configure itself
> > > it seems like we'd have enough information to do the job anyway.
> 
> 
> > you're going to be dealing with a setup where each vendor does one thing
> > differently. Some will have it all in the SoC part of a single IP (dwc3 can be
> > configured that way), some will push it out to companion IC, some might even use
> > some mostly discrete setup and so on...
> 
> Right, and that was exactly what this was supposed to be all about when
> I was discussing this originally with Baolin (who's on holiday until
> sometime next week BTW, hence me answering).
> 
> > We're gonna be dealing with a decision that involves information from multiple
> > subsystems (USB, regulator, hwmon, power supply to name a few).
> 
> Sure, that was part of the idea here - provide a central point
> representing the logical port where we can aggregate all the information
> we get in and distribute it to consumers.  
> 
> > We tried doing it all in the kernel back in N800, N810 and N900/N9 days and it's
> > just plain difficult. To make matters worse, N900 had two USB PHYs, one for
> > actual runtime use and another just for detecting the charger type on the other
> > end.
> 
> Can you elaborate on what the difficulties are and how punting to
> userspace helps?  If anything I'd expect punting to userspace to make

the difficulty was mainly making sure all involved parties talk the same
language. I mean, you plug cable and detect charger (this is usually done by the
PMIC or by a BQ-type device - probably done at drivers/power_supply).

First difficulty comes right here. Power_supply notifies that we're attached to
a charger (sometimes it can't differentiate a host/hub charger from a wall
charger), a few ms later you get a notification from the gadget that it got
enumerated with a 500mA configuration. Depending on the order of things, your
load will be configured either to 2A (maximum host/hub charger current) or
500mA. Yes, this can be mitigated :-)

Focussing on this alone, you can have as much as 4 different subsystems
involved, all throwing raw_notifiers at each other. Now each of these subsystems
need to maintain their own state machine about what notification we have
received and what are the valid next states.

I would rather have userspace be the single place getting notifications because
then we solve these details at a single location.

> Things more difficult, if nothing else it means we need to get whatever
> userspace component deployed in all relevant userspace stacks with
> appropriate configuration in order to do things.

but that will be a thing for the kernel too. We will have to deploy this new
kernel component in all relevant stacks with appropriate configuration in order
to do things :-) No changes there.

> We do punt a lot of configuration to userspace for audio because there
> are substantial device specific policy elements in the configuration and
> it's a far from painless experience getting the code and configuration
> deployed where people need it, definitely a not great for users.

it's the same for this situation. We will have a ton of device specific
configuration, specially with power delivery class, billboard class, the
alternate modes and so on. If we already do this part in userland, adding all
those extras will be, IMO, far simpler.

> > On top of all that, we still need to figure out what to do when a particular
> > configuration gets chosen, and what to do when the bus goes into the different
> > suspend levels.
> 
> > It's going to be a lot of policy getting added to the kernel. On top of all
> > that, when Type C and power delivery is finally a thing, there will an entire
> > new stack for USB C commands (using the Type C CC pins or modulated on VBUS for
> > legacy connectors) which we will have to add to the kernel. And different
> > devices will support different charging profiles (there are at least 6 of those,
> > IIRC).
> 
> Yes, USB C was part of the thinking with starting this work - it's going
> to make ensuring that the system is paying attention to limits much more
> important.  I think there's two things here.  One is working out how the
> system is glued together and the other thing is working out what to do
> with that system.

right, and IMO, what to do should be a decision made outside of the kernel as
long as kernel provides safety.

> I think that where we can do it the bit where work out how the various
> components are connected and aggregate their functionality together is
> definitely a kernel job.  It means that userspace doesn't need to worry
> about implementation details of the particular system and instead gets a
> consistent, standard view of the device (in this case a USB port and how
> much power we can draw from it).

Actually, I was thinking about it in a more granular fashion. Kernel exposes a
charger/power_supply, a few regulators, a UDC view and this single userspace
daemon figures out how to tie those together.

The view here isn't really a USB port, it's just a source of power. But how much
power we can draw from it depends on, possibly, a ton of different chips and
different notifications.

> For the policy stuff we do want to have userspace be able to control
> things but we need to think about how to do that - for example does the
> entire flow need to be in userspace, or can we just expose settings
> which userspace can manage?

considering the amount of different designs that are already out there and all
the extras that are going to show up due to type-c, I think we'd be better off
exposing as much to userspace as possible.

> > With all these different things going on, it's best to do what e.g. NFC folks
> > did. Just a small set of hooks in the kernel, but actual implementation done by
> > a userspace daemon. This makes it even easier for middleware development since
> > we can provide a higher level API for middleware to talk to the charging daemon.
> 
> I'm not sure that the NFC model is working well for everyone - it's OK
> if you happen to be running Android but if you aren't you're often left
> sitting there working out what to do with an Android HAL.  We can do the

NFC works pretty well for neard, afaict. And without android.

> middleware thing without going quite that far, we do already have power
> management daemons in userspace even on desktop (GNOME has upowerd for
> example).

right

> > Another benefit is that this charging daemon can also give hints to power
> > management policy that e.g. we're charging at 10W or 100W and we can e.g. switch
> > to performance governor safely even though battery is rather low.
> 
> We definitely want userspace to be able to see the current state, even
> if it just passes it straight on to the user it's a common part of UIs
> on both desktop and mobile operating systems.
> 
> > Anyway, there's really a lot that needs to happen and shuving it all in the
> > kernel is, IMHO, the wrong decision. I would be much happier with minimal safety
> > requirements in the kernel and let a userspace daemon (which we should certainly
> > provide a reference implementation) figure out what to do. This might be better
> > for things like Chromebooks and Android phones too which might make completely
> > different decisions given a certain charging profile.
> 
> Saying we don't want to have absolutely everything in kernel space
> doesn't mean we should have nothing at all there, doing that seems like
> going too far in the other direction.

we _have_ to have something in the kernel :-) I'm arguing that we might not want
as much as you think we do :-)

The real difficulty here is coming up with an interface which we're agreeing to
supporting for the next 30 years. Whatever that is, as soon as it gets exposed
to userland, we will have to support it.

And this another reason I think a more granular approach is better, as it allows
us to easily add more pieces as they come along.
Felipe Balbi Oct. 2, 2015, 5:27 p.m. UTC | #8
Hi,

On Fri, Oct 02, 2015 at 07:41:07AM +0200, Greg KH wrote:
> On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:
> > On Thu, Sep 24, 2015 at 10:39:23AM -0700, Baolin Wang wrote:
> > > The usb charger framework is based on usb gadget. The usb charger
> > > need to be notified the state changing of usb gadget to confirm the
> > > usb charger state.
> > > 
> > > Thus this patch adds a notifier mechanism for usb gadget to report a
> > > event to usb charger when the usb gadget state is changed.
> > > 
> > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > ---
> > >  drivers/usb/gadget/udc/udc-core.c | 32 ++++++++++++++++++++++++++++++++
> > >  include/linux/usb/gadget.h        | 18 ++++++++++++++++++
> > >  2 files changed, 50 insertions(+)
> > > 
> > > diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> > > index f660afb..4238fc3 100644
> > > --- a/drivers/usb/gadget/udc/udc-core.c
> > > +++ b/drivers/usb/gadget/udc/udc-core.c
> > > @@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
> > >  }
> > >  EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
> > >  
> > > +int usb_gadget_register_notify(struct usb_gadget *gadget,
> > > +			       struct notifier_block *nb)
> > > +{
> > > +	int ret;
> > > +
> > > +	mutex_lock(&gadget->lock);
> > > +	ret = raw_notifier_chain_register(&gadget->nh, nb);
> > > +	mutex_unlock(&gadget->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
> > > +
> > > +int usb_gadget_unregister_notify(struct usb_gadget *gadget,
> > > +				 struct notifier_block *nb)
> > > +{
> > > +	int ret;
> > > +
> > > +	mutex_lock(&gadget->lock);
> > > +	ret = raw_notifier_chain_unregister(&gadget->nh, nb);
> > 
> > Greg, this is the kind of thing I wanted to avoid adding more of.
> > 
> > I was wondering if you would accept subsystems using kdbus for
> > this sort of notification. I'm okay waiting for kdbus for another
> > couple merge windows (if we have to) before that's merged, but
> > if we take this raw notifier approach now, we will end up having
> > to support it forever.
> 
> kdbus is userspace <-> userspace messages, it doesn't do kernel <->
> userspace messages, sorry.

oh well, tough luck :-)

having a more well-constructed notification method to userspace would be a
little better, but I guess we will have to resort to sysfs_notify() or something
like that.

> > Also, because soon enough we will have to support USB Power Delivery
> > with Type C connector, this is bound to change in the coming months.
> > 
> > Frankly, I wanted all of this to be decided in userland with the
> > kernel just providing notification and basic safety checks (we don't
> > want to allow a bogus userspace daemon frying anybody's devices).
> > 
> > How would you feel about that ?
> 
> I agree I don't like new notifier chains being added, but as this is all
> in-kernel, we can change the api in the future and there would not be a
> problem, right?

not necessarily. This will, eventually, get exposed to userspace. At minimum for
adding eye candy to the UI. Chaging battery icon or whatever. The danger here is
that when that something gets exposed, we will have to support it for the next
30 years :-)

> And yeah, I'm worried about the USB Power delivery patches, I haven't
> seen them yet, but I hear about different groups doing different things
> here, and that worries me.

my worries exactly :-) This is why I'm looking for a flexible enough approach
which puts as little into the kernel as necessary.
Mark Brown Oct. 2, 2015, 6:49 p.m. UTC | #9
On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote:
> On Fri, Oct 02, 2015 at 05:47:43PM +0100, Mark Brown wrote:

> > Can you elaborate on what the difficulties are and how punting to
> > userspace helps?  If anything I'd expect punting to userspace to make

> the difficulty was mainly making sure all involved parties talk the same
> language. I mean, you plug cable and detect charger (this is usually done by the
> PMIC or by a BQ-type device - probably done at drivers/power_supply).

> First difficulty comes right here. Power_supply notifies that we're attached to
> a charger (sometimes it can't differentiate a host/hub charger from a wall
> charger), a few ms later you get a notification from the gadget that it got
> enumerated with a 500mA configuration. Depending on the order of things, your
> load will be configured either to 2A (maximum host/hub charger current) or
> 500mA. Yes, this can be mitigated :-)

> Focussing on this alone, you can have as much as 4 different subsystems
> involved, all throwing raw_notifiers at each other. Now each of these subsystems
> need to maintain their own state machine about what notification we have
> received and what are the valid next states.

OK, so part of the goal here was to outsource all the state machines to
some generic code - what you're describing here is definitely a problem
and the idea was to provide a central place that has the logic and makes
the decisions about what we should be doing.  If we don't have that it's
going to get messy.

> I would rather have userspace be the single place getting notifications because
> then we solve these details at a single location.

No argument on the place, making that place be userspace I'm less sold
on.

> > Things more difficult, if nothing else it means we need to get whatever
> > userspace component deployed in all relevant userspace stacks with
> > appropriate configuration in order to do things.

> but that will be a thing for the kernel too. We will have to deploy this new
> kernel component in all relevant stacks with appropriate configuration in order
> to do things :-) No changes there.

Sure, but it's one project which is developed entirely in the open -
that's a lot more manageable.

> > We do punt a lot of configuration to userspace for audio because there
> > are substantial device specific policy elements in the configuration and
> > it's a far from painless experience getting the code and configuration
> > deployed where people need it, definitely a not great for users.

> it's the same for this situation. We will have a ton of device specific
> configuration, specially with power delivery class, billboard class, the
> alternate modes and so on. If we already do this part in userland, adding all
> those extras will be, IMO, far simpler.

Can you elaborate a bit?  As far as I can tell all those are still in
the generic USB space rather than the sort of device specifics I'm
thinking of.

The things we've got with audio are a combination of tuning to taste and
(even more importantly) very different ideas about what you want to do
with an audio subsystem that influence how you set things up in a given
use case.

> > I think that where we can do it the bit where work out how the various
> > components are connected and aggregate their functionality together is
> > definitely a kernel job.  It means that userspace doesn't need to worry
> > about implementation details of the particular system and instead gets a
> > consistent, standard view of the device (in this case a USB port and how
> > much power we can draw from it).

> Actually, I was thinking about it in a more granular fashion. Kernel exposes a
> charger/power_supply, a few regulators, a UDC view and this single userspace
> daemon figures out how to tie those together.

That sounds worrying - it means that new hardware support needs
supporting in every userspace (it seems inevitable that there will be at
least some reimplementations because that's fun) which gets old really
fast, and every userspace needs to understand every topology.

> The view here isn't really a USB port, it's just a source of power. But how much
> power we can draw from it depends on, possibly, a ton of different chips and
> different notifications.

Right, yes - it's the power input/output associated with the USB port.
The USB port is just the physical thing users can see so it's a good way
to label it.  That could mean that we ought to move the aggregation bit
into the power supply code of course, but then a lot of the decisions
are going to be specific to USB.

> > For the policy stuff we do want to have userspace be able to control
> > things but we need to think about how to do that - for example does the
> > entire flow need to be in userspace, or can we just expose settings
> > which userspace can manage?

> considering the amount of different designs that are already out there and all
> the extras that are going to show up due to type-c, I think we'd be better off
> exposing as much to userspace as possible.

How different are the end goals of these designs really going to be
though - that's more of the question for me?  Part of what the kernel is
doing is hiding implementation details from userspace.  I'd expect
userspace to be interested in things like the maximum current allowed in
or out in a given mode rather than the details of how that is accomplished.

> > I'm not sure that the NFC model is working well for everyone - it's OK
> > if you happen to be running Android but if you aren't you're often left
> > sitting there working out what to do with an Android HAL.  We can do the

> NFC works pretty well for neard, afaict. And without android.

Ah, that looks better than it was now - it looks like they're providing
a reasonable hardware abstraction for messaging.  Still, there's other
examples of this model that are less successful where the kernel
interface isn't generic.

> The real difficulty here is coming up with an interface which we're agreeing to
> supporting for the next 30 years. Whatever that is, as soon as it gets exposed
> to userland, we will have to support it.

To me that sounds like an argument for hiding things :)

> And this another reason I think a more granular approach is better, as it allows
> us to easily add more pieces as they come along.

OTOH it's more work for the system as a whole.
Felipe Balbi Oct. 2, 2015, 7:11 p.m. UTC | #10
On Fri, Oct 02, 2015 at 07:49:09PM +0100, Mark Brown wrote:
> On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote:
> > On Fri, Oct 02, 2015 at 05:47:43PM +0100, Mark Brown wrote:
> 
> > > Can you elaborate on what the difficulties are and how punting to
> > > userspace helps?  If anything I'd expect punting to userspace to make
> 
> > the difficulty was mainly making sure all involved parties talk the same
> > language. I mean, you plug cable and detect charger (this is usually done by the
> > PMIC or by a BQ-type device - probably done at drivers/power_supply).
> 
> > First difficulty comes right here. Power_supply notifies that we're attached to
> > a charger (sometimes it can't differentiate a host/hub charger from a wall
> > charger), a few ms later you get a notification from the gadget that it got
> > enumerated with a 500mA configuration. Depending on the order of things, your
> > load will be configured either to 2A (maximum host/hub charger current) or
> > 500mA. Yes, this can be mitigated :-)
> 
> > Focussing on this alone, you can have as much as 4 different subsystems
> > involved, all throwing raw_notifiers at each other. Now each of these subsystems
> > need to maintain their own state machine about what notification we have
> > received and what are the valid next states.
> 
> OK, so part of the goal here was to outsource all the state machines to
> some generic code - what you're describing here is definitely a problem
> and the idea was to provide a central place that has the logic and makes
> the decisions about what we should be doing.  If we don't have that it's
> going to get messy.
> 
> > I would rather have userspace be the single place getting notifications because
> > then we solve these details at a single location.
> 
> No argument on the place, making that place be userspace I'm less sold
> on.

fair enough. This would be an entire new entity, though, and users (chargers,
battery chips, USB controllers, USB PHYs, etc) should blindly notify this entity
and no care about what happened before or what are possible next states.

As long as we can guarantee that, then I'd be okay adding this to the kernel.

> > > Things more difficult, if nothing else it means we need to get whatever
> > > userspace component deployed in all relevant userspace stacks with
> > > appropriate configuration in order to do things.
> 
> > but that will be a thing for the kernel too. We will have to deploy this new
> > kernel component in all relevant stacks with appropriate configuration in order
> > to do things :-) No changes there.
> 
> Sure, but it's one project which is developed entirely in the open -
> that's a lot more manageable.

We can have a fully open source userland daemon too. Much like systemd, bluez,
neard, and many, many others. Why wouldn't that be a thing ?

> > > We do punt a lot of configuration to userspace for audio because there
> > > are substantial device specific policy elements in the configuration and
> > > it's a far from painless experience getting the code and configuration
> > > deployed where people need it, definitely a not great for users.
> 
> > it's the same for this situation. We will have a ton of device specific
> > configuration, specially with power delivery class, billboard class, the
> > alternate modes and so on. If we already do this part in userland, adding all
> > those extras will be, IMO, far simpler.
> 
> Can you elaborate a bit?  As far as I can tell all those are still in
> the generic USB space rather than the sort of device specifics I'm
> thinking of.

let's consider the billboard class, for example. With the new Type-C connector,
the extra CC pins get added and power delivery messaging goes on top of
that. Billboard class gives us the opportunity to send a power delivery request
to mux the data pins on the TypeC connector to something completely non-USB.

So we could run NATIVE PCIe (single lane though) on top of this. Native
DisplayPort, Native Thunderbolt (which the Fruit company announced will switch
to type-C, so that will be a thing), native VGA... the list goes on and on.

At that point, this is not entirely USB realm anymore :-)

Similar applies to power delivery charging profile themselves. Not all profiles
are mandatory. Some are optional and that will be completely device/board
specific. How an OEM implements that in the PCB is also completely
board-specific :-) Some might have a dumb IC hardcoding some messages, some
might have something largely more flexible.

> The things we've got with audio are a combination of tuning to taste and
> (even more importantly) very different ideas about what you want to do
> with an audio subsystem that influence how you set things up in a given
> use case.

the same thing will happen with Type-C and power delivery. There won't be tuning
to taste, of course :-) But there will be "very different ideas what what you
want to do with" your charging methodology.

> > > I think that where we can do it the bit where work out how the various
> > > components are connected and aggregate their functionality together is
> > > definitely a kernel job.  It means that userspace doesn't need to worry
> > > about implementation details of the particular system and instead gets a
> > > consistent, standard view of the device (in this case a USB port and how
> > > much power we can draw from it).
> 
> > Actually, I was thinking about it in a more granular fashion. Kernel exposes a
> > charger/power_supply, a few regulators, a UDC view and this single userspace
> > daemon figures out how to tie those together.
> 
> That sounds worrying - it means that new hardware support needs
> supporting in every userspace (it seems inevitable that there will be at
> least some reimplementations because that's fun) which gets old really
> fast, and every userspace needs to understand every topology.

very true, but it's also true for a kernel solution.

It seems like you want it in the kernel because of it being convenient :-)

> > The view here isn't really a USB port, it's just a source of power. But how much
> > power we can draw from it depends on, possibly, a ton of different chips and
> > different notifications.
> 
> Right, yes - it's the power input/output associated with the USB port.
> The USB port is just the physical thing users can see so it's a good way
> to label it.  That could mean that we ought to move the aggregation bit
> into the power supply code of course, but then a lot of the decisions
> are going to be specific to USB.

in a sense, yes. But they can happen at a layer which knows nothing (or very
little) about USB. Here's an example:

Not everybody follows USB Battery Charging class. Some chargers don't short
D+/D- to signify they are a wall charger. Some will use different resistance
values on the ID pin to tell you how much current you can draw from them.

From a PMIC (or something else) point of view, all it needs is a tiny current
source and a an ADC, then it reports the ADC value to the upper layer. This
really doesn't even need to know that it's using an ID pin, or whatever.

> > > For the policy stuff we do want to have userspace be able to control
> > > things but we need to think about how to do that - for example does the
> > > entire flow need to be in userspace, or can we just expose settings
> > > which userspace can manage?
> 
> > considering the amount of different designs that are already out there and all
> > the extras that are going to show up due to type-c, I think we'd be better off
> > exposing as much to userspace as possible.
> 
> How different are the end goals of these designs really going to be
> though - that's more of the question for me?  Part of what the kernel is
> doing is hiding implementation details from userspace.  I'd expect
> userspace to be interested in things like the maximum current allowed in
> or out in a given mode rather than the details of how that is accomplished.

okay, this is a fair point :-) I just don't see, as of now at least, how we can
get to that in a way that's somewhat future-proof. It seems like we will
add more and more notifications until we can't maintain this anymore.

Say, we get a "certifiable" USB BC1.2 solution in the kernel. Current systems
are happy, we drop yet another out-of-tree set of patches from AOSP, every holds
hands and sings Kumbaya.

In another year or so, power delivery will be *the* thing. Now all the work done
assuming BC1.2 is gone, outdated. Adding power delivery brings an entire new set
of requirements. A new protocol stack (yes! it runs on CC pins, remember ?), new
messages, etc etc...

With Type-C there's no port type anymore. What we used to refer as Host/Hub
Charger, Standard Port, Charging Port, Wall Charger, etc, now becomes a single
type (let's call it type-c for short) which can provide profiles. These profiles
are negotiated using that new stack I pointed out above, not by checking
resistance on data lines or ID pin.

If we're not careful, it will be really difficult to make power delivery fit
into this.

Add to that the fact that the same power delivery pipe (the CC pins) can be used
to tell the other end of remux the pins to e.g. PCIe, and s**t just hit the fan
:-)

In short, whatever we do, needs to consider coping with incoming requests from
userspace at a minimum because userspace will need control over the port
alternate modes. I mean, don't get me wrong, these is all uber-cool to me, but
it'll be a pain to make a flexible/generic solution :-)

> > > I'm not sure that the NFC model is working well for everyone - it's OK
> > > if you happen to be running Android but if you aren't you're often left
> > > sitting there working out what to do with an Android HAL.  We can do the
> 
> > NFC works pretty well for neard, afaict. And without android.
> 
> Ah, that looks better than it was now - it looks like they're providing
> a reasonable hardware abstraction for messaging.  Still, there's other
> examples of this model that are less successful where the kernel
> interface isn't generic.
> 
> > The real difficulty here is coming up with an interface which we're agreeing to
> > supporting for the next 30 years. Whatever that is, as soon as it gets exposed
> > to userland, we will have to support it.
> 
> To me that sounds like an argument for hiding things :)

the problem with hiding, though, is that once something new comes about, it
might not fit our current abstraction and it might be big PITA to support "old"
and new systems.

> > And this another reason I think a more granular approach is better, as it allows
> > us to easily add more pieces as they come along.
> 
> OTOH it's more work for the system as a whole.

it's more work outside the kernel, yes. The total amount of work (kernel +
userspace) will be the same, regardless if you hide it in the kernel or in userspace.
Mark Brown Oct. 4, 2015, 10:55 p.m. UTC | #11
On Fri, Oct 02, 2015 at 02:11:25PM -0500, Felipe Balbi wrote:
> On Fri, Oct 02, 2015 at 07:49:09PM +0100, Mark Brown wrote:
> > On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote:

> > > > Things more difficult, if nothing else it means we need to get whatever
> > > > userspace component deployed in all relevant userspace stacks with
> > > > appropriate configuration in order to do things.

> > > but that will be a thing for the kernel too. We will have to deploy this new
> > > kernel component in all relevant stacks with appropriate configuration in order
> > > to do things :-) No changes there.

> > Sure, but it's one project which is developed entirely in the open -
> > that's a lot more manageable.

> We can have a fully open source userland daemon too. Much like systemd, bluez,
> neard, and many, many others. Why wouldn't that be a thing ?

The trouble is getting traction on adoption.  Vendors have a habit of
doing things like finding problems and rather than reporting them
deciding that the open source solution isn't great and going and writing
their own thing (especially when they're in stealth mode) rather than
talking to anyone.  Sometimes we manage to avoid that but it can be
challenging, and often we only even hear about the new thing after it's
shipping and there's a lot of inertia behind changing it.  The kernel
ABIs tend to be a lot sticker than userspace things here.

> Similar applies to power delivery charging profile themselves. Not all profiles
> are mandatory. Some are optional and that will be completely device/board
> specific. How an OEM implements that in the PCB is also completely
> board-specific :-) Some might have a dumb IC hardcoding some messages, some
> might have something largely more flexible.

Right, and what I was trying to say was that it seems like the kernel
ought to be worrying about the board specific bits and userspace
worrying about what to do with those bits.

> > The things we've got with audio are a combination of tuning to taste and
> > (even more importantly) very different ideas about what you want to do
> > with an audio subsystem that influence how you set things up in a given
> > use case.

> the same thing will happen with Type-C and power delivery. There won't be tuning
> to taste, of course :-) But there will be "very different ideas what what you
> want to do with" your charging methodology.

Are those very different things about the use cases ("don't bother with
high speed data, get all the power you can" or whatever) or are they
about the details of the board?

> > > Actually, I was thinking about it in a more granular fashion. Kernel exposes a
> > > charger/power_supply, a few regulators, a UDC view and this single userspace
> > > daemon figures out how to tie those together.

> > That sounds worrying - it means that new hardware support needs
> > supporting in every userspace (it seems inevitable that there will be at
> > least some reimplementations because that's fun) which gets old really
> > fast, and every userspace needs to understand every topology.

> very true, but it's also true for a kernel solution.

> It seems like you want it in the kernel because of it being convenient :-)

Yes, definitely - my experience both in terms of watching how people
like handset vendors often work internally and in terms of getting
things adopted is that it's a lot easier if you can have one component
you need to update to handle new hardware rather than multiple ones that
need to be upgraded for things that are purely board differences.

> > > The view here isn't really a USB port, it's just a source of power. But how much
> > > power we can draw from it depends on, possibly, a ton of different chips and
> > > different notifications.

> > Right, yes - it's the power input/output associated with the USB port.
> > The USB port is just the physical thing users can see so it's a good way
> > to label it.  That could mean that we ought to move the aggregation bit
> > into the power supply code of course, but then a lot of the decisions
> > are going to be specific to USB.

> in a sense, yes. But they can happen at a layer which knows nothing (or very
> little) about USB. Here's an example:

> Not everybody follows USB Battery Charging class. Some chargers don't short
> D+/D- to signify they are a wall charger. Some will use different resistance
> values on the ID pin to tell you how much current you can draw from them.

> From a PMIC (or something else) point of view, all it needs is a tiny current
> source and a an ADC, then it reports the ADC value to the upper layer. This
> really doesn't even need to know that it's using an ID pin, or whatever.

This doesn't seem much different to things like the various GPIO to
subsystem X drivers we've got - we already have some for IIO type
things IIRC.

> > How different are the end goals of these designs really going to be
> > though - that's more of the question for me?  Part of what the kernel is
> > doing is hiding implementation details from userspace.  I'd expect
> > userspace to be interested in things like the maximum current allowed in
> > or out in a given mode rather than the details of how that is accomplished.

> okay, this is a fair point :-) I just don't see, as of now at least, how we can
> get to that in a way that's somewhat future-proof. It seems like we will
> add more and more notifications until we can't maintain this anymore.

So we need to look at the new/upcoming USB specs and understand the
problem space, and how much of it we need to worry about right now in
this scope.

> With Type-C there's no port type anymore. What we used to refer as Host/Hub
> Charger, Standard Port, Charging Port, Wall Charger, etc, now becomes a single
> type (let's call it type-c for short) which can provide profiles. These profiles
> are negotiated using that new stack I pointed out above, not by checking
> resistance on data lines or ID pin.

Yup, which is a really cool feature and keeps us all employed too! :)
This is one reason for attaching things to the USB stack, right now it
does a limited negotiation but in future like you say there's more and
more features coming.

> Add to that the fact that the same power delivery pipe (the CC pins) can be used
> to tell the other end of remux the pins to e.g. PCIe, and s**t just hit the fan
> :-)

> In short, whatever we do, needs to consider coping with incoming requests from
> userspace at a minimum because userspace will need control over the port
> alternate modes. I mean, don't get me wrong, these is all uber-cool to me, but
> it'll be a pain to make a flexible/generic solution :-)

That seems to make sense to me - anything the kernel is able to do
autonomously for USB C should probably be pretty dumb.  Older USB
standards are already pretty dumb themselves (although inconsistently
standardised).

> > > The real difficulty here is coming up with an interface which we're agreeing to
> > > supporting for the next 30 years. Whatever that is, as soon as it gets exposed
> > > to userland, we will have to support it.

> > To me that sounds like an argument for hiding things :)

> the problem with hiding, though, is that once something new comes about, it
> might not fit our current abstraction and it might be big PITA to support "old"
> and new systems.

Does the problem not get easier if we break it down to just the charger
elements and realise that the USB C modes are going to have a lot more
policy in them?

> > > And this another reason I think a more granular approach is better, as it allows
> > > us to easily add more pieces as they come along.

> > OTOH it's more work for the system as a whole.

> it's more work outside the kernel, yes. The total amount of work (kernel +
> userspace) will be the same, regardless if you hide it in the kernel or in userspace.

Like I say I'm worried about deployment issues for the split solutions
making things harder than they should be.
Felipe Balbi Oct. 5, 2015, 3:15 p.m. UTC | #12
On Sun, Oct 04, 2015 at 11:55:06PM +0100, Mark Brown wrote:
> On Fri, Oct 02, 2015 at 02:11:25PM -0500, Felipe Balbi wrote:
> > On Fri, Oct 02, 2015 at 07:49:09PM +0100, Mark Brown wrote:
> > > On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote:
> 
> > > > > Things more difficult, if nothing else it means we need to get whatever
> > > > > userspace component deployed in all relevant userspace stacks with
> > > > > appropriate configuration in order to do things.
> 
> > > > but that will be a thing for the kernel too. We will have to deploy this new
> > > > kernel component in all relevant stacks with appropriate configuration in order
> > > > to do things :-) No changes there.
> 
> > > Sure, but it's one project which is developed entirely in the open -
> > > that's a lot more manageable.
> 
> > We can have a fully open source userland daemon too. Much like systemd, bluez,
> > neard, and many, many others. Why wouldn't that be a thing ?
> 
> The trouble is getting traction on adoption.  Vendors have a habit of doing
> things like finding problems and rather than reporting them deciding that the
> open source solution isn't great and going and writing their own thing
> (especially when they're in stealth mode) rather than talking to anyone.
> Sometimes we manage to avoid that but it can be challenging, and often we only
> even hear about the new thing after it's shipping and there's a lot of inertia
> behind changing it.  The kernel ABIs tend to be a lot sticker than userspace
> things here.

but this is not a solid enough argument to push anything into the kernel.

> > Similar applies to power delivery charging profile themselves. Not all
> > profiles are mandatory. Some are optional and that will be completely
> > device/board specific. How an OEM implements that in the PCB is also
> > completely board-specific :-) Some might have a dumb IC hardcoding some
> > messages, some might have something largely more flexible.
> 
> Right, and what I was trying to say was that it seems like the kernel ought to
> be worrying about the board specific bits and userspace worrying about what to
> do with those bits.

we're not saying anything different in this front. I, too, want kernel to deal
with certain details and expose notifications. Where we disagree is how granular
should these notifications be and where they should be sent to.

 > > The things we've got with audio are a combination of tuning to taste and
> > > (even more importantly) very different ideas about what you want to do
> > > with an audio subsystem that influence how you set things up in a given
> > > use case.
> 
> > the same thing will happen with Type-C and power delivery. There won't be tuning
> > to taste, of course :-) But there will be "very different ideas what what you
> > want to do with" your charging methodology.
> 
> Are those very different things about the use cases ("don't bother with
> high speed data, get all the power you can" or whatever) or are they
> about the details of the board?

I'm pretty sure there will be interesting designs in the market. I'm pretty sure
there will be type-C systems which won't support USB 3.1 for example, even
though they'll support USB Power Delivery in its entirety.

> > > > Actually, I was thinking about it in a more granular fashion. Kernel
> > > > exposes a charger/power_supply, a few regulators, a UDC view and this
> > > > single userspace daemon figures out how to tie those together.
> 
> > > That sounds worrying - it means that new hardware support needs
> > > supporting in every userspace (it seems inevitable that there will be at
> > > least some reimplementations because that's fun) which gets old really
> > > fast, and every userspace needs to understand every topology.
> 
> > very true, but it's also true for a kernel solution.
> 
> > It seems like you want it in the kernel because of it being convenient :-)
> 
> Yes, definitely - my experience both in terms of watching how people
> like handset vendors often work internally and in terms of getting
> things adopted is that it's a lot easier if you can have one component
> you need to update to handle new hardware rather than multiple ones that
> need to be upgraded for things that are purely board differences.

IMO, just because you have it in the kernel, it doesn't mean it'll be
adopted. Look at pm_runtime, for example; it's a very nice API and, yet, not
used by Android (or has that changed ?)

> > > > The view here isn't really a USB port, it's just a source of power. But how much
> > > > power we can draw from it depends on, possibly, a ton of different chips and
> > > > different notifications.
> 
> > > Right, yes - it's the power input/output associated with the USB port.
> > > The USB port is just the physical thing users can see so it's a good way
> > > to label it.  That could mean that we ought to move the aggregation bit
> > > into the power supply code of course, but then a lot of the decisions
> > > are going to be specific to USB.
> 
> > in a sense, yes. But they can happen at a layer which knows nothing (or very
> > little) about USB. Here's an example:
> 
> > Not everybody follows USB Battery Charging class. Some chargers don't short
> > D+/D- to signify they are a wall charger. Some will use different resistance
> > values on the ID pin to tell you how much current you can draw from them.
> 
> > From a PMIC (or something else) point of view, all it needs is a tiny current
> > source and a an ADC, then it reports the ADC value to the upper layer. This
> > really doesn't even need to know that it's using an ID pin, or whatever.
> 
> This doesn't seem much different to things like the various GPIO to
> subsystem X drivers we've got - we already have some for IIO type
> things IIRC.

that's really not what I was trying to point out :-)

> > > How different are the end goals of these designs really going to be
> > > though - that's more of the question for me?  Part of what the kernel is
> > > doing is hiding implementation details from userspace.  I'd expect
> > > userspace to be interested in things like the maximum current allowed in
> > > or out in a given mode rather than the details of how that is accomplished.
> 
> > okay, this is a fair point :-) I just don't see, as of now at least, how we can
> > get to that in a way that's somewhat future-proof. It seems like we will
> > add more and more notifications until we can't maintain this anymore.
> 
> So we need to look at the new/upcoming USB specs and understand the

not upcoming, it's already out there.

> problem space, and how much of it we need to worry about right now in
> this scope.
> 
> > With Type-C there's no port type anymore. What we used to refer as Host/Hub
> > Charger, Standard Port, Charging Port, Wall Charger, etc, now becomes a single
> > type (let's call it type-c for short) which can provide profiles. These profiles
> > are negotiated using that new stack I pointed out above, not by checking
> > resistance on data lines or ID pin.
> 
> Yup, which is a really cool feature and keeps us all employed too! :)
> This is one reason for attaching things to the USB stack, right now it
> does a limited negotiation but in future like you say there's more and
> more features coming.

keep in mind that the same stack used to change a charging current, will also be
used to tell the other end to mux those lines differently.

> > Add to that the fact that the same power delivery pipe (the CC pins) can be used
> > to tell the other end of remux the pins to e.g. PCIe, and s**t just hit the fan
> > :-)
> 
> > In short, whatever we do, needs to consider coping with incoming requests from
> > userspace at a minimum because userspace will need control over the port
> > alternate modes. I mean, don't get me wrong, these is all uber-cool to me, but
> > it'll be a pain to make a flexible/generic solution :-)
> 
> That seems to make sense to me - anything the kernel is able to do
> autonomously for USB C should probably be pretty dumb.  Older USB
> standards are already pretty dumb themselves (although inconsistently
> standardised).

right.

> > > > The real difficulty here is coming up with an interface which we're agreeing to
> > > > supporting for the next 30 years. Whatever that is, as soon as it gets exposed
> > > > to userland, we will have to support it.
> 
> > > To me that sounds like an argument for hiding things :)
> 
> > the problem with hiding, though, is that once something new comes about, it
> > might not fit our current abstraction and it might be big PITA to support "old"
> > and new systems.
> 
> Does the problem not get easier if we break it down to just the charger
> elements and realise that the USB C modes are going to have a lot more
> policy in them?

the thing with USB C is that it's not only for negotiating a charging power
(with power delivery you're not necessarily tied to 5V, btw), that very stack
will also be used to change to other alternate modes, and those can be anything:
Thunderbolt, PCIe, DisplayPort, etc.

> > > > And this another reason I think a more granular approach is better, as it allows
> > > > us to easily add more pieces as they come along.
> 
> > > OTOH it's more work for the system as a whole.
> 
> > it's more work outside the kernel, yes. The total amount of work (kernel +
> > userspace) will be the same, regardless if you hide it in the kernel or in userspace.
> 
> Like I say I'm worried about deployment issues for the split solutions
> making things harder than they should be.

and I'm worried about us having too much inside the kernel and that making
things harder than they should be :-)
Mark Brown Oct. 5, 2015, 4:18 p.m. UTC | #13
On Mon, Oct 05, 2015 at 10:15:11AM -0500, Felipe Balbi wrote:
> On Sun, Oct 04, 2015 at 11:55:06PM +0100, Mark Brown wrote:

> > The trouble is getting traction on adoption.  Vendors have a habit of doing
> > things like finding problems and rather than reporting them deciding that the
> > open source solution isn't great and going and writing their own thing
> > (especially when they're in stealth mode) rather than talking to anyone.
> > Sometimes we manage to avoid that but it can be challenging, and often we only
> > even hear about the new thing after it's shipping and there's a lot of inertia
> > behind changing it.  The kernel ABIs tend to be a lot sticker than userspace
> > things here.

> but this is not a solid enough argument to push anything into the kernel.

To my mind it is a concern when considering design - it's not the only
consideration certainly but it should influence if or how we split
things.

> > > the same thing will happen with Type-C and power delivery. There won't be tuning
> > > to taste, of course :-) But there will be "very different ideas what what you
> > > want to do with" your charging methodology.

> > Are those very different things about the use cases ("don't bother with
> > high speed data, get all the power you can" or whatever) or are they
> > about the details of the board?

> I'm pretty sure there will be interesting designs in the market. I'm pretty sure
> there will be type-C systems which won't support USB 3.1 for example, even
> though they'll support USB Power Delivery in its entirety.

That's sounding like generic USB stuff to me.

> > Yes, definitely - my experience both in terms of watching how people
> > like handset vendors often work internally and in terms of getting
> > things adopted is that it's a lot easier if you can have one component
> > you need to update to handle new hardware rather than multiple ones that
> > need to be upgraded for things that are purely board differences.

> IMO, just because you have it in the kernel, it doesn't mean it'll be
> adopted. Look at pm_runtime, for example; it's a very nice API and, yet, not
> used by Android (or has that changed ?)

That's always been a bit of a myth - most Android systems use runtime PM
extensively when they're running, the discussion is really about the
edge case where you're idling the system.  The Android suspend behaviour
is about the system idle case and is as much about providing a simple
way to go into an idle state, especially bearing in mind that they have
apps installed from the app store there, as anything else.  It's the
making sure things are idle part of things that's different.

> > > okay, this is a fair point :-) I just don't see, as of now at least, how we can
> > > get to that in a way that's somewhat future-proof. It seems like we will
> > > add more and more notifications until we can't maintain this anymore.

> > So we need to look at the new/upcoming USB specs and understand the

> not upcoming, it's already out there.

I assume there's ongoing work on further revisions to the spec though?

> > Yup, which is a really cool feature and keeps us all employed too! :)
> > This is one reason for attaching things to the USB stack, right now it
> > does a limited negotiation but in future like you say there's more and
> > more features coming.

> keep in mind that the same stack used to change a charging current, will also be
> used to tell the other end to mux those lines differently.

Sure.

> > Does the problem not get easier if we break it down to just the charger
> > elements and realise that the USB C modes are going to have a lot more
> > policy in them?

> the thing with USB C is that it's not only for negotiating a charging power
> (with power delivery you're not necessarily tied to 5V, btw), that very stack
> will also be used to change to other alternate modes, and those can be anything:
> Thunderbolt, PCIe, DisplayPort, etc.

Surely these features have sufficient orthogonality to allow us to split
things up and deal with some of the problems while providing spaces for
future work?
Felipe Balbi Oct. 5, 2015, 4:29 p.m. UTC | #14
Hi,

On Mon, Oct 05, 2015 at 05:18:33PM +0100, Mark Brown wrote:
> On Mon, Oct 05, 2015 at 10:15:11AM -0500, Felipe Balbi wrote:
> > On Sun, Oct 04, 2015 at 11:55:06PM +0100, Mark Brown wrote:
> 
> > > The trouble is getting traction on adoption.  Vendors have a habit of doing
> > > things like finding problems and rather than reporting them deciding that the
> > > open source solution isn't great and going and writing their own thing
> > > (especially when they're in stealth mode) rather than talking to anyone.
> > > Sometimes we manage to avoid that but it can be challenging, and often we only
> > > even hear about the new thing after it's shipping and there's a lot of inertia
> > > behind changing it.  The kernel ABIs tend to be a lot sticker than userspace
> > > things here.
> 
> > but this is not a solid enough argument to push anything into the kernel.
> 
> To my mind it is a concern when considering design - it's not the only
> consideration certainly but it should influence if or how we split
> things.

sure

> > > > the same thing will happen with Type-C and power delivery. There won't be tuning
> > > > to taste, of course :-) But there will be "very different ideas what what you
> > > > want to do with" your charging methodology.
> 
> > > Are those very different things about the use cases ("don't bother with
> > > high speed data, get all the power you can" or whatever) or are they
> > > about the details of the board?
> 
> > I'm pretty sure there will be interesting designs in the market. I'm pretty sure
> > there will be type-C systems which won't support USB 3.1 for example, even
> > though they'll support USB Power Delivery in its entirety.
> 
> That's sounding like generic USB stuff to me.
> 
> > > Yes, definitely - my experience both in terms of watching how people
> > > like handset vendors often work internally and in terms of getting
> > > things adopted is that it's a lot easier if you can have one component
> > > you need to update to handle new hardware rather than multiple ones that
> > > need to be upgraded for things that are purely board differences.
> 
> > IMO, just because you have it in the kernel, it doesn't mean it'll be
> > adopted. Look at pm_runtime, for example; it's a very nice API and, yet, not
> > used by Android (or has that changed ?)
> 
> That's always been a bit of a myth - most Android systems use runtime PM
> extensively when they're running, the discussion is really about the
> edge case where you're idling the system.  The Android suspend behaviour
> is about the system idle case and is as much about providing a simple
> way to go into an idle state, especially bearing in mind that they have
> apps installed from the app store there, as anything else.  It's the
> making sure things are idle part of things that's different.

okay

> > > > okay, this is a fair point :-) I just don't see, as of now at least, how we can
> > > > get to that in a way that's somewhat future-proof. It seems like we will
> > > > add more and more notifications until we can't maintain this anymore.
> 
> > > So we need to look at the new/upcoming USB specs and understand the
> 
> > not upcoming, it's already out there.
> 
> I assume there's ongoing work on further revisions to the spec though?

that'd be a correct assumption

> > > Does the problem not get easier if we break it down to just the charger
> > > elements and realise that the USB C modes are going to have a lot more
> > > policy in them?
> 
> > the thing with USB C is that it's not only for negotiating a charging power
> > (with power delivery you're not necessarily tied to 5V, btw), that very stack
> > will also be used to change to other alternate modes, and those can be anything:
> > Thunderbolt, PCIe, DisplayPort, etc.
> 
> Surely these features have sufficient orthogonality to allow us to split
> things up and deal with some of the problems while providing spaces for
> future work?

yeah, might. As long as we keep that voice in our heads that things are likely
to change.
Bjorn Andersson Oct. 7, 2015, 4:44 p.m. UTC | #15
On Fri, Oct 2, 2015 at 10:23 AM, Felipe Balbi via device-mainlining
<device-mainlining@lists.linuxfoundation.org> wrote:
[..]
> The real difficulty here is coming up with an interface which we're agreeing to
> supporting for the next 30 years. Whatever that is, as soon as it gets exposed
> to userland, we will have to support it.
>

Isn't this the main reason for not creating a user space ABI now?

Running with the in-kernel hooks would allow us to get things working
now, we can change it as we wish, we can explore the difficulties and
we can create an ABI from that - if we need to - that might have a
chance to work for the next 30 years.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pavel Machek Oct. 8, 2015, 3:50 p.m. UTC | #16
HI!

> > +	int ret;
> > +
> > +	mutex_lock(&gadget->lock);
> > +	ret = raw_notifier_chain_unregister(&gadget->nh, nb);
> 
> Greg, this is the kind of thing I wanted to avoid adding more of.
> 
> I was wondering if you would accept subsystems using kdbus for
> this sort of notification. I'm okay waiting for kdbus for another
> couple merge windows (if we have to) before that's merged, but
> if we take this raw notifier approach now, we will end up having
> to support it forever.
> 
> Also, because soon enough we will have to support USB Power Delivery
> with Type C connector, this is bound to change in the coming months.
> 
> Frankly, I wanted all of this to be decided in userland with the
> kernel just providing notification and basic safety checks (we don't
> want to allow a bogus userspace daemon frying anybody's devices).
> 
> How would you feel about that ?

So init=/bin/bash boot no longer provides machine that charges itself?

That would be bad. Traditionally, hardware controls battery charging,
and if hardware needs some help, we should do it in kernel, to mask
the difference from userspace.
								Pavel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pavel Machek Oct. 8, 2015, 3:51 p.m. UTC | #17
Hi!

> > What's the advantage of pushing this to userspace?  By the time we
> > provide enough discoverability to enable userspace to configure itself
> > it seems like we'd have enough information to do the job anyway.
> 
> you're going to be dealing with a setup where each vendor does one thing
> differently. Some will have it all in the SoC part of a single IP (dwc3 can be
> configured that way), some will push it out to companion IC, some might even use
> some mostly discrete setup and so on...
> 
> We're gonna be dealing with a decision that involves information from multiple
> subsystems (USB, regulator, hwmon, power supply to name a few).
> 
> We tried doing it all in the kernel back in N800, N810 and N900/N9 days and it's
> just plain difficult. To make matters worse, N900 had two USB PHYs, one for
> actual runtime use and another just for detecting the charger type on the other
> end.

N900 does charging from kernel these days.

Not being able to boot generic distribution would be regression there...

										Pavel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Felipe Balbi Oct. 9, 2015, 9:17 p.m. UTC | #18
Hi,

Pavel Machek <pavel@ucw.cz> writes:
> HI!
>
>> > +	int ret;
>> > +
>> > +	mutex_lock(&gadget->lock);
>> > +	ret = raw_notifier_chain_unregister(&gadget->nh, nb);
>> 
>> Greg, this is the kind of thing I wanted to avoid adding more of.
>> 
>> I was wondering if you would accept subsystems using kdbus for
>> this sort of notification. I'm okay waiting for kdbus for another
>> couple merge windows (if we have to) before that's merged, but
>> if we take this raw notifier approach now, we will end up having
>> to support it forever.
>> 
>> Also, because soon enough we will have to support USB Power Delivery
>> with Type C connector, this is bound to change in the coming months.
>> 
>> Frankly, I wanted all of this to be decided in userland with the
>> kernel just providing notification and basic safety checks (we don't
>> want to allow a bogus userspace daemon frying anybody's devices).
>> 
>> How would you feel about that ?
>
> So init=/bin/bash boot no longer provides machine that charges itself?
>
> That would be bad. Traditionally, hardware controls battery charging,
> and if hardware needs some help, we should do it in kernel, to mask
> the difference from userspace.

this is a very valid point which I hadn't considered :-)

Seems like kernel it is, no matter how easy or how difficult it gets.

Mark, when can we try to have a discussion about how to get this
upstream ? It seems like designing everything in the mailing list will
just take forever. Any ideas ?
Mark Brown Oct. 12, 2015, 4:56 p.m. UTC | #19
On Fri, Oct 09, 2015 at 04:17:27PM -0500, Felipe Balbi wrote:

> Mark, when can we try to have a discussion about how to get this
> upstream ? It seems like designing everything in the mailing list will
> just take forever. Any ideas ?

Can we take this off-list?  I'm in the UK, Baolin is in China and I
believe you're in the US so it might be a fun one to schedule...
diff mbox

Patch

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index f660afb..4238fc3 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -129,6 +129,32 @@  void usb_gadget_giveback_request(struct usb_ep *ep,
 }
 EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
 
+int usb_gadget_register_notify(struct usb_gadget *gadget,
+			       struct notifier_block *nb)
+{
+	int ret;
+
+	mutex_lock(&gadget->lock);
+	ret = raw_notifier_chain_register(&gadget->nh, nb);
+	mutex_unlock(&gadget->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
+
+int usb_gadget_unregister_notify(struct usb_gadget *gadget,
+				 struct notifier_block *nb)
+{
+	int ret;
+
+	mutex_lock(&gadget->lock);
+	ret = raw_notifier_chain_unregister(&gadget->nh, nb);
+	mutex_unlock(&gadget->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_unregister_notify);
+
 /* ------------------------------------------------------------------------- */
 
 /**
@@ -226,6 +252,10 @@  static void usb_gadget_state_work(struct work_struct *work)
 	struct usb_gadget *gadget = work_to_gadget(work);
 	struct usb_udc *udc = gadget->udc;
 
+	mutex_lock(&gadget->lock);
+	raw_notifier_call_chain(&gadget->nh, gadget->state, gadget);
+	mutex_unlock(&gadget->lock);
+
 	if (udc)
 		sysfs_notify(&udc->dev.kobj, NULL, "state");
 }
@@ -364,6 +394,8 @@  int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 
 	dev_set_name(&gadget->dev, "gadget");
 	INIT_WORK(&gadget->work, usb_gadget_state_work);
+	RAW_INIT_NOTIFIER_HEAD(&gadget->nh);
+	mutex_init(&gadget->lock);
 	gadget->dev.parent = parent;
 
 #ifdef	CONFIG_HAS_DMA
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index c14a69b..755e8bc 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -609,6 +609,8 @@  struct usb_gadget {
 	unsigned			out_epnum;
 	unsigned			in_epnum;
 	struct usb_otg_caps		*otg_caps;
+	struct raw_notifier_head	nh;
+	struct mutex			lock;
 
 	unsigned			sg_supported:1;
 	unsigned			is_otg:1;
@@ -1183,6 +1185,22 @@  extern void usb_gadget_unmap_request(struct usb_gadget *gadget,
 
 /*-------------------------------------------------------------------------*/
 
+/**
+ * Register a notifiee to get notified by any attach status changes from
+ * the usb gadget
+ */
+int usb_gadget_register_notify(struct usb_gadget *gadget,
+			       struct notifier_block *nb);
+
+/*-------------------------------------------------------------------------*/
+
+
+/* Unregister a notifiee from the usb gadget */
+int usb_gadget_unregister_notify(struct usb_gadget *gadget,
+				 struct notifier_block *nb);
+
+/*-------------------------------------------------------------------------*/
+
 /* utility to set gadget state properly */
 
 extern void usb_gadget_set_state(struct usb_gadget *gadget,