linux-kernel - Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 8 Dec 2016 17:05:24 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Stuart Yoder <stuart.yoder@....com>
Cc:     "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        "agraf@...e.de" <agraf@...e.de>, "arnd@...db.de" <arnd@...db.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Leo Li <leoyang.li@....com>,
        Catalin Horghidan <catalin.horghidan@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Laurentiu Tudor <laurentiu.tudor@....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@...uxfoundation.org]
> > Sent: Wednesday, December 07, 2016 9:53 AM
> > To: Stuart Yoder <stuart.yoder@....com>
> > Cc: devel@...verdev.osuosl.org; linux-kernel@...r.kernel.org; agraf@...e.de; arnd@...db.de; Leo Li
> > <leoyang.li@....com>; Ioana Ciornei <ioana.ciornei@....com>; Catalin Horghidan
> > <catalin.horghidan@....com>; Laurentiu Tudor <laurentiu.tudor@....com>; Ruxandra Ioana Radulescu
> > <ruxandra.radulescu@....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?

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