On Fri, May 26, 2017 at 06:26:01AM -0700, Guenter Roeck wrote:
> > > What happens if trace is not enabled ? If I recall discussions around
> > > CLANG
> > > correctly, it complains about unused static inline functions.
> >
> > Nothing happens if trace is not enable. Everything continues to
> > compile just like before.
> >
> > Maybe you were trying to define the tracepoints in C code? That won't
> > work.
> >
> > > Anyway, is it really necessary to have an include file for this instead of
> > > keeping the code in trace.c ? I am somewhat wary of variable declarations
> > > in include files - include twice and there will be two instances.
> >
> > The tracepoint documentation actually states that you need to place the
> > definitions of the tracepoints in a header. I have the trace.c file only
> > because it was a convenient place to put the tracepoint statement in.
> >
> > I don't know the inner workings of tracepoints, but they do work fine
> > for me. In case you want more info on the topic, I never read these
> > articles, but they were mentioned in Documentation/trace/tracepoints.txt:
> >
> > http://lwn.net/Articles/379903
> > http://lwn.net/Articles/381064
> > http://lwn.net/Articles/383362
> >
> > I believe they provide at least some kind of description on how
> > tracepoints actually work.
> >
> Sorry for not being clear; that was a question about your code, not mine.
> If it builds without warnings with trace disabled, no problem.
Ah, of course. Silly me. Sorry about that.
> > > > +#include <linux/completion.h>
> > > > +#include <linux/property.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/slab.h>
> > > > +#include <linux/usb/typec.h>
> > >
> > > Any preference in usb about alphabetic order of include files ?
> >
> > I don't see any mention of it in the documentation, so this is
> > probable just matter of taste in the end. One likes the mother, one
> > likes the daughter.
> >
>
> I like alphabetic because it makes life easier later on, if/when additional
> include files are added, and it is easier to find a specific include file in
> an alphabetic list. Some upstream maintainers ask for it nowadays (including
> me :-).
It's good to keep the headers in some order just for the sake of being
organized, but I don't see how alphabetical order is any better then
for example descending.
I normally would not care about this topic and I would have just
ordered the headers alphabetically if anybody asked for it, but I've
actually proposed ordering the headers alphabetically myself ones, and
this is what the upstream maintainer said: "Can we stick to serious
critiques"
I think some maintainers do not see the need to dictate styling rules
on everything, and leave some things to be decided by the developer. I
kind of like that.
But if you still feel strongly about this, I'll change the style, but
for now, I'll keep it the way it is. Actually, I'll move the last one
on top.
> > > > +/**
> > > > + * ucsi_notify - PPM notification handler
> > > > + * @ucsi: Source UCSI Interface for the notifications
> > > > + *
> > > > + * Handle notifications from PPM of @ucsi.
> > > > + */
> > > > +void ucsi_notify(struct ucsi *ucsi)
> > > > +{
> > > > + struct ucsi_cci *cci;
> > > > +
> > > > + /* There is no requirement to sync here, so only warning if it
> > > > fails */
> > > > + if (ucsi_sync(ucsi))
> > > > + dev_warn(ucsi->dev, "%s: sync failed\n", __func__);
> > > > +
> > > Why even a warning if it is not a requirement ?
> >
> > It still should not "ever" fail. Why not create the warning? It is
> > after all indication that something is really wrong, and we probable
> > want to know about it.
> >
> Your call, of course. I just dislike noisy drivers, and I am always concerned
> about flooding the kernel log.
Fair enough, I'll remove the warning.
> > > > +static int
> > > > +ucsi_dr_swap(const struct typec_capability *cap, enum typec_data_role
> > > > role)
> > > > +{
> > > > + struct ucsi_connector *con = to_ucsi_connector(cap);
> > > > + struct ucsi_control ctrl;
> > > > + int ret = 0;
> > > > +
> > > > + if (!con->partner)
> > > > + return -EAGAIN;
> > >
> > > Doesn't this result in an immediate retry by user space ?
> >
> > Isn't that what tcpm.c reports? Maybe I misunderstood the code.
> > I'll change that to something else.
> >
> Good point; tcpm returns -EAGAIN if the port is not in a READY state.
> Maybe I should change the tcpm code ? No idea what error to use instead,
> though.
I'll use -ENOTCONN for now. Let me know if it's not good.
> > > > +/**
> > > > + * ucsi_register_ppm - Register UCSI PPM Interface
> > > > + * @dev: Device interface to the PPM
> > > > + * @ppm: The PPM interface
> > > > + *
> > > > + * Allocates UCSI instance, associates it with @ppm and returns it to
> > > > the
> > > > + * caller, and schedules initialization of the interface.
> > > > + */
> > > > +struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm
> > > > *ppm)
> > > > +{
> > > > + struct ucsi *ucsi;
> > > > +
> > > > + ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
> > > > + if (!ucsi)
> > > > + return ERR_PTR(-ENOMEM);
> > > > +
> > > > + INIT_WORK(&ucsi->work, ucsi_init);
> > > > + init_completion(&ucsi->complete);
> > > > + mutex_init(&ucsi->ppm_lock);
> > > > +
> > > > + ucsi->dev = dev;
> > > > + ucsi->ppm = ppm;
> > > > +
> > > > + queue_work(system_long_wq, &ucsi->work);
> > >
> > > This is kind of weird/odd. A failure won't be reported to the caller.
> > > Does this have to be asynchronous ? unregister is synchronous, after all.
> >
> > The communication with the PPM is painfully slow, especially during
> > boot time probable because there are many other things trying to
> > communicate with EC at the same time. It quite often takes something
> > like one second per port to finish the probe on average. With four
> > ports it has taken as much as six seconds. Even one second is totally
> > unacceptable. We are being pushed to fix our drivers that exceed 50ms
> > probe time, and as a way to achieve it with slow drivers, handling
> > things in a work was suggested.
> >
> > So that's the reason for this approach. I have not come up with
> > anything better for it, and nobody has proposed anything else. Do
> > you have any suggestions?
> >
> Makes sense. Not better idea, sorry.
>
> It might be worthwhile adding a note describing the reason.
> Also, it might be useful to explain why the kcalloc() in usci_init()
> won't result in a memory leak, not even on error (that took me
> a while to understand).
You are correct. I'll add an explanation.
Thanks Guenter,
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
