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

[Stuart Yoder]

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

As far as who accesses it... fsl-mc device drivers will reference
the 'struct device' when registering interrupts, when calling
functions like devm*, dev_err(), and for maintaining driver
private data in 'driver_data'.

Example of registering an irq where you can see the embedded
struct dev (dpio_dev->dev) referenced:

        error = devm_request_irq(&dpio_dev->dev,
                                 irq->msi_desc->irq,
                                 dpio_irq_handler,
                                 0,
                                 dev_name(&dpio_dev->dev),
                                 &dpio_dev->dev);

> 	- do you have any Documentation/ABI entries?

Not currently, but it looks like we need ones for bind/unbind.
I will submit a patch to document these.

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

> 	- don't call pr_info() in fsl_mc_bus_driver_init(), no need to
> 	  say anything if all goes well.  Same goes with random
> 	  dev_info() calls, please remove.

Ok, will do.

Thanks,
Stuart