From: Greg KH <gregkh@linuxfoundation.org>
To: Stuart Yoder <stuart.yoder@nxp.com>
Cc: "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
"arnd@arndb.de" <arnd@arndb.de>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"agraf@suse.de" <agraf@suse.de>, 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
Date: Fri, 9 Dec 2016 08:10:43 +0100 [thread overview]
Message-ID: <20161209071043.GA2940@kroah.com> (raw)
In-Reply-To: <VI1PR0401MB26386D1D87DD3136788DA0628D870@VI1PR0401MB2638.eurprd04.prod.outlook.com>
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 <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.
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
next prev parent reply other threads:[~2016-12-09 7:10 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-01 22:41 [PATCH v3 0/9] staging: fsl-mc: move bus driver out of staging, add dpio Stuart Yoder
2016-12-01 22:41 ` [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging Stuart Yoder
2016-12-07 15:52 ` Greg KH
2016-12-07 20:19 ` Stuart Yoder
2016-12-08 16:05 ` Greg KH
2016-12-09 0:36 ` Stuart Yoder
2016-12-09 7:10 ` Greg KH [this message]
2016-12-09 15:53 ` Stuart Yoder
2016-12-01 22:41 ` [PATCH v3 2/9] bus: fsl-mc: dpio: add DPIO driver overview document Stuart Yoder
2016-12-01 22:41 ` [PATCH v3 3/9] bus: fsl-mc: dpio: add APIs for DPIO objects Stuart Yoder
2016-12-02 11:26 ` Laurentiu Tudor
2016-12-15 16:36 ` Stuart Yoder
2016-12-01 22:41 ` [PATCH v3 4/9] bus: fsl-mc: dpio: add frame descriptor and scatter/gather APIs Stuart Yoder
2016-12-02 12:12 ` Laurentiu Tudor
2016-12-05 20:52 ` Dan Carpenter
2016-12-06 10:35 ` Laurentiu Tudor
2016-12-15 16:41 ` Stuart Yoder
2016-12-01 22:41 ` [PATCH v3 5/9] bus: fsl-mc: dpio: add global dpaa2 definitions Stuart Yoder
2016-12-02 12:19 ` Laurentiu Tudor
2016-12-15 17:12 ` Stuart Yoder
2016-12-01 22:41 ` [PATCH v3 6/9] bus: fsl-mc: dpio: add QBMan portal APIs for DPAA2 Stuart Yoder
2016-12-02 12:40 ` Laurentiu Tudor
2016-12-15 22:20 ` Stuart Yoder
2016-12-01 22:41 ` [PATCH v3 7/9] bus: fsl-mc: dpio: add the DPAA2 DPIO service interface Stuart Yoder
2016-12-02 13:02 ` Laurentiu Tudor
2016-12-15 22:43 ` Stuart Yoder
2016-12-01 22:41 ` [PATCH v3 8/9] bus: fsl-mc: dpio: add the DPAA2 DPIO object driver Stuart Yoder
2016-12-01 22:41 ` [PATCH v3 9/9] bus: fsl-mc: dpio: add maintainer for DPIO Stuart Yoder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161209071043.GA2940@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=agraf@suse.de \
--cc=arnd@arndb.de \
--cc=catalin.horghidan@nxp.com \
--cc=devel@driverdev.osuosl.org \
--cc=ioana.ciornei@nxp.com \
--cc=laurentiu.tudor@nxp.com \
--cc=leoyang.li@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=stuart.yoder@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).