Date:   Fri, 2 Dec 2016 10:04:39 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Oliver Neukum <oneukum@...e.com>, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org
Subject: Re: [PATCHv13 2/3] usb: USB Type-C connector class

On Wed, Nov 30, 2016 at 11:19:10AM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Tue, Nov 29, 2016 at 05:27:44PM +0100, Greg KH wrote:
> > > +struct typec_cable {
> > > +	struct device		dev;
> > > +	enum typec_plug_type	type;
> > > +	u32			vdo;
> > > +	unsigned int		usb_pd:1;
> > > +	unsigned int		active:1;
> > > +	unsigned int		sop_pp_controller:1;
> > > +
> > > +	struct typec_plug	plug[2];
> > 
> > WTF???
> > 
> > Think about what this structure now represents.  You have 3 different
> > reference counted objects, all embedded in the same structure.  Who
> > "owns" the lifecycle of it?  What happens if plug[1]'s reference count
> > is grabbed a bunch by someone reading a lot of files for it, and then
> > the "larger" typec_cable.dev reference count is decremented to 0 because
> > the core is done with it.  oops, boom, ick, splat, and if you are lucky
> > the device reboots itself, if not, someone just got root and read your
> > bank account information...
> 
> I have to ask. How could that happen since the cable is the parent?
> 
> > I'm being harsh here because this is really really really badly designed
> 
> Don't worry about it, I can handle it. In fact, one could argue that I
> like getting slapped by you based on the comments I keep getting, but
> I assure you that is not the case ;-)
> 
> > code.  Go back and think about your data structures, what they are
> > trying to represent, and _WHO_ owns and controls them.  The typec core
> > should be the one that allocates and manages the lifecycle of them, not
> > some random external entity where you just hope and pray that they got
> > it right (hint, they can not as they do not know what the core did with
> > the reference counts.)
> > 
> > Right now you are almost there, the typec core registers and tries to
> > manage the structures, but it doesn't allocate/free them, and that's the
> > big problem, because really, with a structure that has 3 different
> > reference counts, no one can.  That's an impossibility.
> > 
> > This needs a lot more work, sorry.
> 
> I was trying to cut corners, which clearly was wrong. I know what I
> need to do. All the parts simply need to be registered normally. No
> shortcuts.
> 
> > I'm now going to require that you get other internal Intel developers to
> > sign off on this code before I review it again.  You have resources at
> > your disposal that others do not with your internal mailing lists
> > containing senior kernel developers.  Use it and don't waste the
> > community's time to do basic code review that they should be doing
> > instead.
> 
> Fair enough.
> 
> > How did we get to a v13 of this patch series without anyone else even
> > seeing this?  That's worrysome as well...
> 
> I guess for somebody writing the port drivers my awesome shortcut felt
> kinda nice. All they would have to do is announce connection when it
> happens, and the class then tried figured out everything for them,
> what needs to be created and so on. But yes, that is wrong!
> 
At least for my part I very much concentrated on making sure that
the user space ABI as well as the port driver API are sane and usable.

The driver interface is not my area of expertise. As such, my testing
and understanding of that part was limited to "it appears to work,
it must be ok". I very much relied on you to get this part right.

That makes me feel really bad. It isn't fun to have my "Reviewed-by"
on a patch that gets (and apparently deserves) a WTF from a senior
kernel maintainer. This hurts both your and my reputation, and obviously
will make me quite hesitant to add a "Reviewed-by:" to the next version
of the series.

No more shortcuts, please.

Thanks,
Guenter

Powered by blists - more mailing lists