Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1537864 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging Greg KH <gregkh@linuxfoundation.org> - 2016-12-07 17:00 +0100
    RE: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging Stuart Yoder <stuart.yoder@nxp.com> - 2016-12-07 21:20 +0100
      Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging Greg KH <gregkh@linuxfoundation.org> - 2016-12-08 17:10 +0100
        RE: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging Stuart Yoder <stuart.yoder@nxp.com> - 2016-12-09 02:20 +0100
          Re: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging Greg KH <gregkh@linuxfoundation.org> - 2016-12-09 08:20 +0100
            RE: [PATCH v3 1/9] staging: fsl-mc: move bus driver out of staging Stuart Yoder <stuart.yoder@nxp.com> - 2016-12-09 18:30 +0100

#1537864 — 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?
	- do you have any Documentation/ABI entries?
	- root_dprc_count, why are you using an atomic variable for
	  this?  What is it for other than "look, I'm running!"?
	- 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.

That's good enough for now, I think the answer to the first question is
going to cause more to come out :)

thanks,

greg k-h

[toc] | [next] | [standalone]

#1538054

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

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

[toc] | [prev] | [next] | [standalone]

#1538660

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?

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

[toc] | [prev] | [next] | [standalone]

#1538997

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

[toc] | [prev] | [next] | [standalone]

#1539147

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

[toc] | [prev] | [next] | [standalone]

#1539571

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

Yes, device removal has been working from day 1.  A brief look
seems to indicate that the ref count never goes to zero, which
needs to be debugged.

Thanks,
Stuart

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web