From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753423AbcLIHKh (ORCPT ); Fri, 9 Dec 2016 02:10:37 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:44388 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750744AbcLIHKg (ORCPT ); Fri, 9 Dec 2016 02:10:36 -0500 Date: Fri, 9 Dec 2016 08:10:43 +0100 From: Greg KH To: Stuart Yoder Cc: "devel@driverdev.osuosl.org" , "arnd@arndb.de" , "linux-kernel@vger.kernel.org" , "agraf@suse.de" , Leo Li , Catalin Horghidan , Ioana Ciornei , Laurentiu Tudor Subject: Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging Message-ID: <20161209071043.GA2940@kroah.com> References: <1480632094-3621-1-git-send-email-stuart.yoder@nxp.com> <1480632094-3621-2-git-send-email-stuart.yoder@nxp.com> <20161207155248.GA11794@kroah.com> <20161208160524.GA4977@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 09, 2016 at 12:36:26AM +0000, Stuart Yoder wrote: > > -----Original Message----- > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > Sent: Thursday, December 08, 2016 10:05 AM > > To: Stuart Yoder > > Cc: devel@driverdev.osuosl.org; agraf@suse.de; arnd@arndb.de; linux-kernel@vger.kernel.org; Leo Li > > ; Catalin Horghidan ; Ioana Ciornei > > ; Laurentiu Tudor > > Subject: Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging > > > > On Wed, Dec 07, 2016 at 08:19:20PM +0000, Stuart Yoder wrote: > > > > -----Original Message----- > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > > Sent: Wednesday, December 07, 2016 9:53 AM > > > > To: Stuart Yoder > > > > Cc: devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org; agraf@suse.de; arnd@arndb.de; Leo Li > > > > ; Ioana Ciornei ; Catalin Horghidan > > > > ; Laurentiu Tudor ; Ruxandra Ioana Radulescu > > > > > > > > Subject: Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging > > > > > > > > On Thu, Dec 01, 2016 at 04:41:26PM -0600, Stuart Yoder wrote: > > > > > Move the source files out of staging into their final locations: > > > > > -include files in drivers/staging/fsl-mc/include go to include/linux/fsl > > > > > -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > > > > > -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > > > > > -README.txt, providing and overview of DPAA goes to > > > > > Documentation/dpaa2/overview.txt > > > > > -update MAINTAINERS with new location > > > > > > > > > > Delete other remaining staging files-- Makefile, Kconfig, TODO > > > > > > > > Ok, given that I haven't ever reviewed this code, I had a few questions > > > > that I couldn't easily figure out by looking at your code: > > > > - what is the lifecycle of your 'struct device' usage? Who > > > > creates it, who frees it, and who accesses it? > > > > > > We embed a 'struct device' inside our bus specific device struct > > > 'struct fsl_mc_device'. So, when a new fsl-mc object is discovered > > > on the bus during initial enumeration or hotplug we create a new > > > 'struct fsl_mc_device' and do a device_initialize()/device_add(). > > > (see fsl_mc_device_add() for where this is done) > > > > > > 'struct device' is freed when a device is removed-- the reverse > > > of the above. > > > > Where is the device freed? I see you trying to do some "odd" stuff in > > fsl_mc_device_remove() by deleting and then putting a device structure. > > I can't find a "release()" callback anywhere for your bus, where is it? > > > > What happens when the reference count falls to 0 for your struct device? > > Hrm...something seems wrong in free path, and I think this needs to > be refactored. > > IIRC, when German (former maintainer) wrote that code he loosely based > it on the register/unregister platform bus code: > > int platform_device_register(struct platform_device *pdev) > { > device_initialize(&pdev->dev); > arch_setup_pdev_archdata(pdev); > return platform_device_add(pdev); > } > void platform_device_unregister(struct platform_device *pdev) > { > platform_device_del(pdev); > platform_device_put(pdev); > } > > ...I'm puzzling over how that code handles a refcount of zero > as I see no 'release' callback anywhere, but I must be missing > something. > > In any case, we'll get this refactored. Have you tried removing a device? The kernel should complain loudly about there not being a release function for your device. > > > > - root_dprc_count, why are you using an atomic variable for > > > > this? What is it for other than "look, I'm running!"? > > > > > > There can be multiple root buses, and this variable simply tracks the count > > > of them. > > > > Why does it matter? > > > > > It's is atomic there might be a theoretical race condition where 2 > > > buses might be added at the same time. The root buses are found in > > > the device tree and so if there is no chance that device tree > > > processing happens in parallel on multiple cores then we could remove > > > the atomic. > > > > Why not just use a lock, or better yet, not care about a "count" at all? > > I don't see you doing anything with the count, other than emitting a > > WARN() if it drops down below 0 for some reason, or when you call > > fsl_mc_bus_exists() which for some reason is exported yet no one uses > > it... > > We can drop this count. At one time I think there was envisioned an > external user who needed it, but it's no longer the case. Please do, we are trying to get rid of atomic_t abuse on other mailing lists, and this one fits the pattern of "no real need for it" :) > Given the additional refactoring, I think the fsl-mc bus driver needs > to stay in staging for a bit. In order to facilitate further review > I'm going to refactor the patch series: > staging: fsl-mc: move bus driver out of staging, add dpio > > ...to just add dpio (into staging). This will allow the Eth driver > series sent earlier this week to go into staging: > staging: Introduce Freescale DPAA2 Ethernet driver > > With all that in staging we'll have a fully functional Ethernet > driver. Ok, that sounds reasonable. thanks, greg k-h