From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932230AbcKUOo6 (ORCPT ); Mon, 21 Nov 2016 09:44:58 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:37806 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751437AbcKUOo5 (ORCPT ); Mon, 21 Nov 2016 09:44:57 -0500 Date: Mon, 21 Nov 2016 15:45:06 +0100 From: Greg KH To: Heikki Krogerus Cc: Guenter Roeck , Badhri Jagan Sridharan , Oliver Neukum , linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Subject: Re: [PATCHv11 2/3] usb: USB Type-C connector class Message-ID: <20161121144506.GA28410@kroah.com> References: <20161117105036.133406-1-heikki.krogerus@linux.intel.com> <20161117105036.133406-3-heikki.krogerus@linux.intel.com> <20161121103528.GB2233@kroah.com> <20161121131103.GA18501@kuha.fi.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161121131103.GA18501@kuha.fi.intel.com> User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Nov 21, 2016 at 03:11:03PM +0200, Heikki Krogerus wrote: > Hi Greg, > > On Mon, Nov 21, 2016 at 11:35:28AM +0100, Greg KH wrote: > > > +static void typec_partner_release(struct device *dev) > > > +{ > > > + struct typec_port *port = to_typec_port(dev->parent); > > > + > > > + typec_unregister_altmodes(dev); > > > + port->partner = NULL; > > > +} > > > > This doesn't feel right, why are you both exporting > > typec_unregister_altmodes() and also calling it from release callbacks? > > That implies there is two way to clean stuff up, so what is a driver > > writer to use? Please simplify and force it to be one way or the other. > > OK. > > > Also typec_unregister_altmodes() doesn't free memory, which release is > > supposed to be doing, which also implies that the reference counting > > logic is all wrong here. Please fix, like I asked you to previously. > > There is nothing wrong with the reference counting, and nothing has > been allocated so there is nothing to free. The device structure itself that this release call is for needs to be freed, right? If not, something is really wrong... > Please note that the partner device is meant to just represent the > partner in user space and not to be actually used for anything. And > please also note that there can only be one partner for a port at a > time. Ok, but these are still reference counted devices, you need to handle that properly. > We could allocate an extra structure for the partner when > typec_connect() is called, but we would do that just for the sake of > having something to free in the release hook. It would not be useful > for anything. It would not help us increase/decrease the reference > count of the device, and the port driver would still have to provide > details about the partner capabilities the moment it tells us the > partner was connected. Again, free the device for which this release function is being called for, that is why it is there. thanks, greg k-h