Re: [PATCHv13 2/3] usb: USB Type-C connector class

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

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!

But man, v14! I have got to be breaking the record with this one.


Thanks,

-- 
heikki