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

[Felipe Balbi]

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.

-- 
balbi

Attachment: signature.asc
Description: PGP signature