From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754559AbcKUNLJ (ORCPT ); Mon, 21 Nov 2016 08:11:09 -0500 Received: from mga14.intel.com ([192.55.52.115]:7147 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754499AbcKUNLI (ORCPT ); Mon, 21 Nov 2016 08:11:08 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,675,1473145200"; d="scan'208";a="1071544279" Date: Mon, 21 Nov 2016 15:11:03 +0200 From: Heikki Krogerus To: Greg KH 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: <20161121131103.GA18501@kuha.fi.intel.com> References: <20161117105036.133406-1-heikki.krogerus@linux.intel.com> <20161117105036.133406-3-heikki.krogerus@linux.intel.com> <20161121103528.GB2233@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161121103528.GB2233@kroah.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 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. 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. 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. Thanks, -- heikki