On Wed, Dec 07, 2016 at 08:19:20PM +0000, Stuart Yoder wrote: > > -----Original Message----- > > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] > > Sent: Wednesday, December 07, 2016 9:53 AM > > To: Stuart Yoder <stuart.yoder@xxxxxxx> > > Cc: devel@xxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; agraf@xxxxxxx; arnd@xxxxxxxx; Leo Li > > <leoyang.li@xxxxxxx>; Ioana Ciornei <ioana.ciornei@xxxxxxx>; Catalin Horghidan > > <catalin.horghidan@xxxxxxx>; Laurentiu Tudor <laurentiu.tudor@xxxxxxx>; Ruxandra Ioana Radulescu > > <ruxandra.radulescu@xxxxxxx> > > 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? > > - 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... thanks, greg k-h _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
