From: Stuart Yoder <stuart.yoder@nxp.com>
To: Greg KH <gregkh@linuxfoundation.org>
CC: "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "agraf@suse.de" <agraf@suse.de>, "arnd@arndb.de" <arnd@arndb.de>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Leo Li <leoyang.li@nxp.com>,
        Catalin Horghidan <catalin.horghidan@nxp.com>,
        "Ioana Ciornei" <ioana.ciornei@nxp.com>,
        Laurentiu Tudor <laurentiu.tudor@nxp.com>
Subject: RE: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging
Thread-Topic: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging
Thread-Index: AQHSTCUaF8l5kM8cdUiKwJF2//8LNKD8q6AAgABAMICAAVWqAIAAhxjw
Date: Fri, 9 Dec 2016 00:36:26 +0000
Message-ID: <VI1PR0401MB26386D1D87DD3136788DA0628D870@VI1PR0401MB2638.eurprd04.prod.outlook.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>
 <VI1PR0401MB2638E11E4F86F3622FA0FAA28D850@VI1PR0401MB2638.eurprd04.prod.outlook.com>
 <20161208160524.GA4977@kroah.com>
In-Reply-To: <20161208160524.GA4977@kroah.com>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Dec 2016 00:36:26.3219
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0401MB1726
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uB91A8fV011911
Content-Length: 4952
Lines: 113

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, December 08, 2016 10:05 AM
> To: Stuart Yoder <stuart.yoder@nxp.com>
> Cc: devel@driverdev.osuosl.org; agraf@suse.de; arnd@arndb.de; linux-kernel@vger.kernel.org; Leo Li
> <leoyang.li@nxp.com>; Catalin Horghidan <catalin.horghidan@nxp.com>; Ioana Ciornei
> <ioana.ciornei@nxp.com>; Laurentiu Tudor <laurentiu.tudor@nxp.com>
> 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 <stuart.yoder@nxp.com>
> > > Cc: devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org; agraf@suse.de; arnd@arndb.de; Leo Li
> > > <leoyang.li@nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>; Catalin Horghidan
> > > <catalin.horghidan@nxp.com>; Laurentiu Tudor <laurentiu.tudor@nxp.com>; Ruxandra Ioana Radulescu
> > > <ruxandra.radulescu@nxp.com>
> > > 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.

> > > 	- 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.

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.

Thanks,
Stuart