Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Tue, Aug 08, 2017 at 02:25:10PM +0100, Will Deacon wrote:
> On Fri, Jul 28, 2017 at 04:12:33PM -0700, Greg KH wrote:
> > On Thu, Jul 27, 2017 at 03:15:15PM +0200, Borislav Petkov wrote:
> > > On Thu, Jul 27, 2017 at 11:08:56AM +0200, Jan Glauber wrote:
> > > > OK. As fixing the firmware will take quite some time I'll go for the 
> > > > memory
> > > > controller driver that starts EDAC / PMU depending on their CONFIG_.
> > > > 
> > > > What would be the proper location for the multiplexer?
> > > > drivers/soc/cavium or drivers/misc?
> > > 
> > > Uff, no idea. Greg? You're the special drivers guy. :-)
> > 
> > Start out in misc/ and let's see the code to determine if it should be
> > moved elsewhere.
> 
> Jan, are you ok making this change, or are you waiting for anything else
> from us? (Mark and I are just going through the pending PMU patches and
> we're not clear on the status of this one).

Hi Will,
it's my turn but I was on vacation. It was not completely trivial (well,
to me) to get the multiplexing working with modules but I'll look at it
again this week.

Cheers,
Jan

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Tue, Aug 08, 2017 at 02:25:10PM +0100, Will Deacon wrote:
> On Fri, Jul 28, 2017 at 04:12:33PM -0700, Greg KH wrote:
> > On Thu, Jul 27, 2017 at 03:15:15PM +0200, Borislav Petkov wrote:
> > > On Thu, Jul 27, 2017 at 11:08:56AM +0200, Jan Glauber wrote:
> > > > OK. As fixing the firmware will take quite some time I'll go for the 
> > > > memory
> > > > controller driver that starts EDAC / PMU depending on their CONFIG_.
> > > > 
> > > > What would be the proper location for the multiplexer?
> > > > drivers/soc/cavium or drivers/misc?
> > > 
> > > Uff, no idea. Greg? You're the special drivers guy. :-)
> > 
> > Start out in misc/ and let's see the code to determine if it should be
> > moved elsewhere.
> 
> Jan, are you ok making this change, or are you waiting for anything else
> from us? (Mark and I are just going through the pending PMU patches and
> we're not clear on the status of this one).

Hi Will,
it's my turn but I was on vacation. It was not completely trivial (well,
to me) to get the multiplexing working with modules but I'll look at it
again this week.

Cheers,
Jan

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Will Deacon]

On Fri, Jul 28, 2017 at 04:12:33PM -0700, Greg KH wrote:
> On Thu, Jul 27, 2017 at 03:15:15PM +0200, Borislav Petkov wrote:
> > On Thu, Jul 27, 2017 at 11:08:56AM +0200, Jan Glauber wrote:
> > > OK. As fixing the firmware will take quite some time I'll go for the 
> > > memory
> > > controller driver that starts EDAC / PMU depending on their CONFIG_.
> > > 
> > > What would be the proper location for the multiplexer?
> > > drivers/soc/cavium or drivers/misc?
> > 
> > Uff, no idea. Greg? You're the special drivers guy. :-)
> 
> Start out in misc/ and let's see the code to determine if it should be
> moved elsewhere.

Jan, are you ok making this change, or are you waiting for anything else
from us? (Mark and I are just going through the pending PMU patches and
we're not clear on the status of this one).

Cheers,

Will

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Will Deacon]

On Fri, Jul 28, 2017 at 04:12:33PM -0700, Greg KH wrote:
> On Thu, Jul 27, 2017 at 03:15:15PM +0200, Borislav Petkov wrote:
> > On Thu, Jul 27, 2017 at 11:08:56AM +0200, Jan Glauber wrote:
> > > OK. As fixing the firmware will take quite some time I'll go for the 
> > > memory
> > > controller driver that starts EDAC / PMU depending on their CONFIG_.
> > > 
> > > What would be the proper location for the multiplexer?
> > > drivers/soc/cavium or drivers/misc?
> > 
> > Uff, no idea. Greg? You're the special drivers guy. :-)
> 
> Start out in misc/ and let's see the code to determine if it should be
> moved elsewhere.

Jan, are you ok making this change, or are you waiting for anything else
from us? (Mark and I are just going through the pending PMU patches and
we're not clear on the status of this one).

Cheers,

Will

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Suzuki K Poulose]

On 27/07/17 10:08, Jan Glauber wrote:

On Thu, Jul 27, 2017 at 07:11:57AM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 02:02:42PM -0700, David Daney wrote:

Also, if a given configuration disables CONFIG_EDAC there is some hackery
needed to get the perf portion of the driver included.


Yes, and we don't do performance counters in EDAC.

So you could add a small memory controller driver which does the
arbitration or fix the firmware.


OK. As fixing the firmware will take quite some time I'll go for the memory
controller driver that starts EDAC / PMU depending on their CONFIG_.


Please could you also rename CAVIUM_PMU to something like CAVIUM_MMC_PMU or even
CAVIUM_UNCOR_PMU ? The symbol could easily be mistaken for a "cavium" CPU PMU.

Suzuki




What would be the proper location for the multiplexer?
drivers/soc/cavium or drivers/misc?

--Jan

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Suzuki K Poulose]

On 27/07/17 10:08, Jan Glauber wrote:

On Thu, Jul 27, 2017 at 07:11:57AM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 02:02:42PM -0700, David Daney wrote:

Also, if a given configuration disables CONFIG_EDAC there is some hackery
needed to get the perf portion of the driver included.


Yes, and we don't do performance counters in EDAC.

So you could add a small memory controller driver which does the
arbitration or fix the firmware.


OK. As fixing the firmware will take quite some time I'll go for the memory
controller driver that starts EDAC / PMU depending on their CONFIG_.


Please could you also rename CAVIUM_PMU to something like CAVIUM_MMC_PMU or even
CAVIUM_UNCOR_PMU ? The symbol could easily be mistaken for a "cavium" CPU PMU.

Suzuki




What would be the proper location for the multiplexer?
drivers/soc/cavium or drivers/misc?

--Jan

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Greg KH]

On Thu, Jul 27, 2017 at 03:15:15PM +0200, Borislav Petkov wrote:
> On Thu, Jul 27, 2017 at 11:08:56AM +0200, Jan Glauber wrote:
> > OK. As fixing the firmware will take quite some time I'll go for the memory
> > controller driver that starts EDAC / PMU depending on their CONFIG_.
> > 
> > What would be the proper location for the multiplexer?
> > drivers/soc/cavium or drivers/misc?
> 
> Uff, no idea. Greg? You're the special drivers guy. :-)

Start out in misc/ and let's see the code to determine if it should be
moved elsewhere.

thanks,

greg k-h

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Greg KH]

On Thu, Jul 27, 2017 at 03:15:15PM +0200, Borislav Petkov wrote:
> On Thu, Jul 27, 2017 at 11:08:56AM +0200, Jan Glauber wrote:
> > OK. As fixing the firmware will take quite some time I'll go for the memory
> > controller driver that starts EDAC / PMU depending on their CONFIG_.
> > 
> > What would be the proper location for the multiplexer?
> > drivers/soc/cavium or drivers/misc?
> 
> Uff, no idea. Greg? You're the special drivers guy. :-)

Start out in misc/ and let's see the code to determine if it should be
moved elsewhere.

thanks,

greg k-h

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Thu, Jul 27, 2017 at 06:11:30PM -0700, Greg KH wrote:
> Oh well, fix it in the kernel, that's what it's there for :)

Duude, don't put crazy ideas in people's heads!

:-)))

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Thu, Jul 27, 2017 at 06:11:30PM -0700, Greg KH wrote:
> Oh well, fix it in the kernel, that's what it's there for :)

Duude, don't put crazy ideas in people's heads!

:-)))

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Greg KH]

On Thu, Jul 27, 2017 at 10:29:53AM -0700, David Daney wrote:
> On 07/26/2017 07:29 PM, Greg KH wrote:
> > On Wed, Jul 26, 2017 at 02:02:42PM -0700, David Daney wrote:
> > > On 07/26/2017 01:08 PM, Greg KH wrote:
> > > > On Wed, Jul 26, 2017 at 01:02:38PM -0700, David Daney wrote:
> > > > > On 07/26/2017 10:33 AM, Greg KH wrote:
> > > > > > On Wed, Jul 26, 2017 at 06:30:49PM +0200, Borislav Petkov wrote:
> > > > > > > On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:
> > > > > > > > On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:
> > > > > > > > > On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:
> > > > > > > > > > The PMU/EDAC devices are all PCI devices do I need the 
> > > > > > > > > > 'struct pci_dev *'.
> > > > > > > > > > I'm not aware of other ways to access these devices. Please 
> > > > > > > > > > enlighten
> > > > > > > > > > me if I'm missing something.
> > > > > > > > > 
> > > > > > > > > Me enlighten you on Cavium hardware?! You're funny.
> > > > > > > > > 
> > > > > > > > > So I don't know whether the PCI hotplug code can run more 
> > > > > > > > > than one
> > > > > > > > > function upon PCI ID detection. Probably Greg will say, write 
> > > > > > > > > a
> > > > > > > > > multiplexer wrapper. :-)
> > > > > > > > 
> > > > > > > > -ENOCONTEXT
> > > > > > > > 
> > > > > > > > Anyway, pci questions are best asked on the linux-pci@vger 
> > > > > > > > list.  And
> > > > > > > > yes, all PCI devices end up with a 'struct pci_dev *' 
> > > > > > > > automatically.
> > > > > > > 
> > > > > > > Simple: so they have a PCI ID of a memory contoller and want to 
> > > > > > > hotplug
> > > > > > > two drivers for it. And those two drivers should remain 
> > > > > > > independent from
> > > > > > > each other.
> > > > > > 
> > > > > > Hahahahaha, no.  That's crazy, you were right in guessing what my 
> > > > > > answer
> > > > > > was going to be :)
> > > > > > 
> > > > > 
> > > > > 
> > > > > Just to be clear about the situation, the device is a memory 
> > > > > controller.  It
> > > > > has two main behaviors we are interested in:
> > > > > 
> > > > > A) Error Detection And Correction (EDAC).  This should be connected 
> > > > > to the
> > > > > kernel's EDAC subsystem.  An existing driver 
> > > > > (drivers/edac/thunderx_edac.c)
> > > > > does exactly this.
> > > > > 
> > > > > B) Performance Counters for actions taken in the corresponding 
> > > > > memory. This
> > > > > should be connected to the kernel's perf framework as an uncore-PMU 
> > > > > (the
> > > > > subject of this patch set).
> > > > > 
> > > > > It is a single PCI device.  What should the driver architecture look 
> > > > > like to
> > > > > connect it to two different kernel subsystems?
> > > > 
> > > > Modify the drivers/edac/thunderx_edac.c code to add support for
> > > > performance counters.
> > > > 
> > > 
> > > Thanks Greg.  This adds some clarity to the situation.
> > > 
> > > This technique does slightly complicate the mapping of files and 
> > > directories
> > > in the kernel source tree to maintainers.
> > > 
> > > Also, if a given configuration disables CONFIG_EDAC there is some hackery
> > > needed to get the perf portion of the driver included.
> > 
> > Well, you all deserve it for trying to have a single PCI device do
> > multiple things at the same time.  There's no real good reason for
> > creating hardware that way, PCI devices are "free", you should go throw
> > a printed copy of the PCI spec at the firmware developers who did this
> > to you.
> 
> The problem lies in something even more congealed than the "firmware". The
> PCI topology is etched in to the very fabric of the silicon of the SoC.

That's not very wise, as you can see here, most "modern" chips allow
stuff like this to be changeable at the firmware level.  I strongly
suggest telling the hardware developers to fix this for your next
revision.

Oh well, fix it in the kernel, that's what it's there for :)

greg k-h

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Greg KH]

On Thu, Jul 27, 2017 at 10:29:53AM -0700, David Daney wrote:
> On 07/26/2017 07:29 PM, Greg KH wrote:
> > On Wed, Jul 26, 2017 at 02:02:42PM -0700, David Daney wrote:
> > > On 07/26/2017 01:08 PM, Greg KH wrote:
> > > > On Wed, Jul 26, 2017 at 01:02:38PM -0700, David Daney wrote:
> > > > > On 07/26/2017 10:33 AM, Greg KH wrote:
> > > > > > On Wed, Jul 26, 2017 at 06:30:49PM +0200, Borislav Petkov wrote:
> > > > > > > On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:
> > > > > > > > On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:
> > > > > > > > > On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:
> > > > > > > > > > The PMU/EDAC devices are all PCI devices do I need the 
> > > > > > > > > > 'struct pci_dev *'.
> > > > > > > > > > I'm not aware of other ways to access these devices. Please 
> > > > > > > > > > enlighten
> > > > > > > > > > me if I'm missing something.
> > > > > > > > > 
> > > > > > > > > Me enlighten you on Cavium hardware?! You're funny.
> > > > > > > > > 
> > > > > > > > > So I don't know whether the PCI hotplug code can run more 
> > > > > > > > > than one
> > > > > > > > > function upon PCI ID detection. Probably Greg will say, write 
> > > > > > > > > a
> > > > > > > > > multiplexer wrapper. :-)
> > > > > > > > 
> > > > > > > > -ENOCONTEXT
> > > > > > > > 
> > > > > > > > Anyway, pci questions are best asked on the linux-pci@vger 
> > > > > > > > list.  And
> > > > > > > > yes, all PCI devices end up with a 'struct pci_dev *' 
> > > > > > > > automatically.
> > > > > > > 
> > > > > > > Simple: so they have a PCI ID of a memory contoller and want to 
> > > > > > > hotplug
> > > > > > > two drivers for it. And those two drivers should remain 
> > > > > > > independent from
> > > > > > > each other.
> > > > > > 
> > > > > > Hahahahaha, no.  That's crazy, you were right in guessing what my 
> > > > > > answer
> > > > > > was going to be :)
> > > > > > 
> > > > > 
> > > > > 
> > > > > Just to be clear about the situation, the device is a memory 
> > > > > controller.  It
> > > > > has two main behaviors we are interested in:
> > > > > 
> > > > > A) Error Detection And Correction (EDAC).  This should be connected 
> > > > > to the
> > > > > kernel's EDAC subsystem.  An existing driver 
> > > > > (drivers/edac/thunderx_edac.c)
> > > > > does exactly this.
> > > > > 
> > > > > B) Performance Counters for actions taken in the corresponding 
> > > > > memory. This
> > > > > should be connected to the kernel's perf framework as an uncore-PMU 
> > > > > (the
> > > > > subject of this patch set).
> > > > > 
> > > > > It is a single PCI device.  What should the driver architecture look 
> > > > > like to
> > > > > connect it to two different kernel subsystems?
> > > > 
> > > > Modify the drivers/edac/thunderx_edac.c code to add support for
> > > > performance counters.
> > > > 
> > > 
> > > Thanks Greg.  This adds some clarity to the situation.
> > > 
> > > This technique does slightly complicate the mapping of files and 
> > > directories
> > > in the kernel source tree to maintainers.
> > > 
> > > Also, if a given configuration disables CONFIG_EDAC there is some hackery
> > > needed to get the perf portion of the driver included.
> > 
> > Well, you all deserve it for trying to have a single PCI device do
> > multiple things at the same time.  There's no real good reason for
> > creating hardware that way, PCI devices are "free", you should go throw
> > a printed copy of the PCI spec at the firmware developers who did this
> > to you.
> 
> The problem lies in something even more congealed than the "firmware". The
> PCI topology is etched in to the very fabric of the silicon of the SoC.

That's not very wise, as you can see here, most "modern" chips allow
stuff like this to be changeable at the firmware level.  I strongly
suggest telling the hardware developers to fix this for your next
revision.

Oh well, fix it in the kernel, that's what it's there for :)

greg k-h

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[David Daney]

On 07/26/2017 07:29 PM, Greg KH wrote:

On Wed, Jul 26, 2017 at 02:02:42PM -0700, David Daney wrote:

On 07/26/2017 01:08 PM, Greg KH wrote:

On Wed, Jul 26, 2017 at 01:02:38PM -0700, David Daney wrote:

On 07/26/2017 10:33 AM, Greg KH wrote:

On Wed, Jul 26, 2017 at 06:30:49PM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:

On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:

The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev *'.
I'm not aware of other ways to access these devices. Please enlighten
me if I'm missing something.


Me enlighten you on Cavium hardware?! You're funny.

So I don't know whether the PCI hotplug code can run more than one
function upon PCI ID detection. Probably Greg will say, write a
multiplexer wrapper. :-)


-ENOCONTEXT

Anyway, pci questions are best asked on the linux-pci@vger list.  And
yes, all PCI devices end up with a 'struct pci_dev *' automatically.


Simple: so they have a PCI ID of a memory contoller and want to hotplug
two drivers for it. And those two drivers should remain independent from
each other.


Hahahahaha, no.  That's crazy, you were right in guessing what my answer
was going to be :)




Just to be clear about the situation, the device is a memory controller.  It
has two main behaviors we are interested in:

A) Error Detection And Correction (EDAC).  This should be connected to the
kernel's EDAC subsystem.  An existing driver (drivers/edac/thunderx_edac.c)
does exactly this.

B) Performance Counters for actions taken in the corresponding memory. This
should be connected to the kernel's perf framework as an uncore-PMU (the
subject of this patch set).

It is a single PCI device.  What should the driver architecture look like to
connect it to two different kernel subsystems?


Modify the drivers/edac/thunderx_edac.c code to add support for
performance counters.



Thanks Greg.  This adds some clarity to the situation.

This technique does slightly complicate the mapping of files and directories
in the kernel source tree to maintainers.

Also, if a given configuration disables CONFIG_EDAC there is some hackery
needed to get the perf portion of the driver included.


Well, you all deserve it for trying to have a single PCI device do
multiple things at the same time.  There's no real good reason for
creating hardware that way, PCI devices are "free", you should go throw
a printed copy of the PCI spec at the firmware developers who did this
to you.


The problem lies in something even more congealed than the "firmware". 
The PCI topology is etched in to the very fabric of the silicon of the SoC.





Then get them to fix the firmware so you have multiple PCI devices...

good luck!


We'll use all of that we can get!  Thanks.




greg k-h

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[David Daney]

On 07/26/2017 07:29 PM, Greg KH wrote:

On Wed, Jul 26, 2017 at 02:02:42PM -0700, David Daney wrote:

On 07/26/2017 01:08 PM, Greg KH wrote:

On Wed, Jul 26, 2017 at 01:02:38PM -0700, David Daney wrote:

On 07/26/2017 10:33 AM, Greg KH wrote:

On Wed, Jul 26, 2017 at 06:30:49PM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:

On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:

The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev *'.
I'm not aware of other ways to access these devices. Please enlighten
me if I'm missing something.


Me enlighten you on Cavium hardware?! You're funny.

So I don't know whether the PCI hotplug code can run more than one
function upon PCI ID detection. Probably Greg will say, write a
multiplexer wrapper. :-)


-ENOCONTEXT

Anyway, pci questions are best asked on the linux-pci@vger list.  And
yes, all PCI devices end up with a 'struct pci_dev *' automatically.


Simple: so they have a PCI ID of a memory contoller and want to hotplug
two drivers for it. And those two drivers should remain independent from
each other.


Hahahahaha, no.  That's crazy, you were right in guessing what my answer
was going to be :)




Just to be clear about the situation, the device is a memory controller.  It
has two main behaviors we are interested in:

A) Error Detection And Correction (EDAC).  This should be connected to the
kernel's EDAC subsystem.  An existing driver (drivers/edac/thunderx_edac.c)
does exactly this.

B) Performance Counters for actions taken in the corresponding memory. This
should be connected to the kernel's perf framework as an uncore-PMU (the
subject of this patch set).

It is a single PCI device.  What should the driver architecture look like to
connect it to two different kernel subsystems?


Modify the drivers/edac/thunderx_edac.c code to add support for
performance counters.



Thanks Greg.  This adds some clarity to the situation.

This technique does slightly complicate the mapping of files and directories
in the kernel source tree to maintainers.

Also, if a given configuration disables CONFIG_EDAC there is some hackery
needed to get the perf portion of the driver included.


Well, you all deserve it for trying to have a single PCI device do
multiple things at the same time.  There's no real good reason for
creating hardware that way, PCI devices are "free", you should go throw
a printed copy of the PCI spec at the firmware developers who did this
to you.


The problem lies in something even more congealed than the "firmware". 
The PCI topology is etched in to the very fabric of the silicon of the SoC.





Then get them to fix the firmware so you have multiple PCI devices...

good luck!


We'll use all of that we can get!  Thanks.




greg k-h

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Thu, Jul 27, 2017 at 11:08:56AM +0200, Jan Glauber wrote:
> OK. As fixing the firmware will take quite some time I'll go for the memory
> controller driver that starts EDAC / PMU depending on their CONFIG_.
> 
> What would be the proper location for the multiplexer?
> drivers/soc/cavium or drivers/misc?

Uff, no idea. Greg? You're the special drivers guy. :-)

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Thu, Jul 27, 2017 at 11:08:56AM +0200, Jan Glauber wrote:
> OK. As fixing the firmware will take quite some time I'll go for the memory
> controller driver that starts EDAC / PMU depending on their CONFIG_.
> 
> What would be the proper location for the multiplexer?
> drivers/soc/cavium or drivers/misc?

Uff, no idea. Greg? You're the special drivers guy. :-)

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Thu, Jul 27, 2017 at 07:11:57AM +0200, Borislav Petkov wrote:
> On Wed, Jul 26, 2017 at 02:02:42PM -0700, David Daney wrote:
> > Also, if a given configuration disables CONFIG_EDAC there is some hackery
> > needed to get the perf portion of the driver included.
> 
> Yes, and we don't do performance counters in EDAC.
> 
> So you could add a small memory controller driver which does the
> arbitration or fix the firmware.

OK. As fixing the firmware will take quite some time I'll go for the memory
controller driver that starts EDAC / PMU depending on their CONFIG_.

What would be the proper location for the multiplexer?
drivers/soc/cavium or drivers/misc?

--Jan

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Thu, Jul 27, 2017 at 07:11:57AM +0200, Borislav Petkov wrote:
> On Wed, Jul 26, 2017 at 02:02:42PM -0700, David Daney wrote:
> > Also, if a given configuration disables CONFIG_EDAC there is some hackery
> > needed to get the perf portion of the driver included.
> 
> Yes, and we don't do performance counters in EDAC.
> 
> So you could add a small memory controller driver which does the
> arbitration or fix the firmware.

OK. As fixing the firmware will take quite some time I'll go for the memory
controller driver that starts EDAC / PMU depending on their CONFIG_.

What would be the proper location for the multiplexer?
drivers/soc/cavium or drivers/misc?

--Jan

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Wed, Jul 26, 2017 at 02:02:42PM -0700, David Daney wrote:
> Also, if a given configuration disables CONFIG_EDAC there is some hackery
> needed to get the perf portion of the driver included.

Yes, and we don't do performance counters in EDAC.

So you could add a small memory controller driver which does the
arbitration or fix the firmware.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Wed, Jul 26, 2017 at 02:02:42PM -0700, David Daney wrote:
> Also, if a given configuration disables CONFIG_EDAC there is some hackery
> needed to get the perf portion of the driver included.

Yes, and we don't do performance counters in EDAC.

So you could add a small memory controller driver which does the
arbitration or fix the firmware.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Greg KH]

On Wed, Jul 26, 2017 at 02:02:42PM -0700, David Daney wrote:
> On 07/26/2017 01:08 PM, Greg KH wrote:
> > On Wed, Jul 26, 2017 at 01:02:38PM -0700, David Daney wrote:
> > > On 07/26/2017 10:33 AM, Greg KH wrote:
> > > > On Wed, Jul 26, 2017 at 06:30:49PM +0200, Borislav Petkov wrote:
> > > > > On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:
> > > > > > On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:
> > > > > > > On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:
> > > > > > > > The PMU/EDAC devices are all PCI devices do I need the 'struct 
> > > > > > > > pci_dev *'.
> > > > > > > > I'm not aware of other ways to access these devices. Please 
> > > > > > > > enlighten
> > > > > > > > me if I'm missing something.
> > > > > > > 
> > > > > > > Me enlighten you on Cavium hardware?! You're funny.
> > > > > > > 
> > > > > > > So I don't know whether the PCI hotplug code can run more than one
> > > > > > > function upon PCI ID detection. Probably Greg will say, write a
> > > > > > > multiplexer wrapper. :-)
> > > > > > 
> > > > > > -ENOCONTEXT
> > > > > > 
> > > > > > Anyway, pci questions are best asked on the linux-pci@vger list.  
> > > > > > And
> > > > > > yes, all PCI devices end up with a 'struct pci_dev *' automatically.
> > > > > 
> > > > > Simple: so they have a PCI ID of a memory contoller and want to 
> > > > > hotplug
> > > > > two drivers for it. And those two drivers should remain independent 
> > > > > from
> > > > > each other.
> > > > 
> > > > Hahahahaha, no.  That's crazy, you were right in guessing what my answer
> > > > was going to be :)
> > > > 
> > > 
> > > 
> > > Just to be clear about the situation, the device is a memory controller.  
> > > It
> > > has two main behaviors we are interested in:
> > > 
> > > A) Error Detection And Correction (EDAC).  This should be connected to the
> > > kernel's EDAC subsystem.  An existing driver 
> > > (drivers/edac/thunderx_edac.c)
> > > does exactly this.
> > > 
> > > B) Performance Counters for actions taken in the corresponding memory. 
> > > This
> > > should be connected to the kernel's perf framework as an uncore-PMU (the
> > > subject of this patch set).
> > > 
> > > It is a single PCI device.  What should the driver architecture look like 
> > > to
> > > connect it to two different kernel subsystems?
> > 
> > Modify the drivers/edac/thunderx_edac.c code to add support for
> > performance counters.
> > 
> 
> Thanks Greg.  This adds some clarity to the situation.
> 
> This technique does slightly complicate the mapping of files and directories
> in the kernel source tree to maintainers.
> 
> Also, if a given configuration disables CONFIG_EDAC there is some hackery
> needed to get the perf portion of the driver included.

Well, you all deserve it for trying to have a single PCI device do
multiple things at the same time.  There's no real good reason for
creating hardware that way, PCI devices are "free", you should go throw
a printed copy of the PCI spec at the firmware developers who did this
to you.

Then get them to fix the firmware so you have multiple PCI devices...

good luck!

greg k-h

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Greg KH]

On Wed, Jul 26, 2017 at 02:02:42PM -0700, David Daney wrote:
> On 07/26/2017 01:08 PM, Greg KH wrote:
> > On Wed, Jul 26, 2017 at 01:02:38PM -0700, David Daney wrote:
> > > On 07/26/2017 10:33 AM, Greg KH wrote:
> > > > On Wed, Jul 26, 2017 at 06:30:49PM +0200, Borislav Petkov wrote:
> > > > > On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:
> > > > > > On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:
> > > > > > > On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:
> > > > > > > > The PMU/EDAC devices are all PCI devices do I need the 'struct 
> > > > > > > > pci_dev *'.
> > > > > > > > I'm not aware of other ways to access these devices. Please 
> > > > > > > > enlighten
> > > > > > > > me if I'm missing something.
> > > > > > > 
> > > > > > > Me enlighten you on Cavium hardware?! You're funny.
> > > > > > > 
> > > > > > > So I don't know whether the PCI hotplug code can run more than one
> > > > > > > function upon PCI ID detection. Probably Greg will say, write a
> > > > > > > multiplexer wrapper. :-)
> > > > > > 
> > > > > > -ENOCONTEXT
> > > > > > 
> > > > > > Anyway, pci questions are best asked on the linux-pci@vger list.  
> > > > > > And
> > > > > > yes, all PCI devices end up with a 'struct pci_dev *' automatically.
> > > > > 
> > > > > Simple: so they have a PCI ID of a memory contoller and want to 
> > > > > hotplug
> > > > > two drivers for it. And those two drivers should remain independent 
> > > > > from
> > > > > each other.
> > > > 
> > > > Hahahahaha, no.  That's crazy, you were right in guessing what my answer
> > > > was going to be :)
> > > > 
> > > 
> > > 
> > > Just to be clear about the situation, the device is a memory controller.  
> > > It
> > > has two main behaviors we are interested in:
> > > 
> > > A) Error Detection And Correction (EDAC).  This should be connected to the
> > > kernel's EDAC subsystem.  An existing driver 
> > > (drivers/edac/thunderx_edac.c)
> > > does exactly this.
> > > 
> > > B) Performance Counters for actions taken in the corresponding memory. 
> > > This
> > > should be connected to the kernel's perf framework as an uncore-PMU (the
> > > subject of this patch set).
> > > 
> > > It is a single PCI device.  What should the driver architecture look like 
> > > to
> > > connect it to two different kernel subsystems?
> > 
> > Modify the drivers/edac/thunderx_edac.c code to add support for
> > performance counters.
> > 
> 
> Thanks Greg.  This adds some clarity to the situation.
> 
> This technique does slightly complicate the mapping of files and directories
> in the kernel source tree to maintainers.
> 
> Also, if a given configuration disables CONFIG_EDAC there is some hackery
> needed to get the perf portion of the driver included.

Well, you all deserve it for trying to have a single PCI device do
multiple things at the same time.  There's no real good reason for
creating hardware that way, PCI devices are "free", you should go throw
a printed copy of the PCI spec at the firmware developers who did this
to you.

Then get them to fix the firmware so you have multiple PCI devices...

good luck!

greg k-h

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[David Daney]

On 07/26/2017 01:08 PM, Greg KH wrote:

On Wed, Jul 26, 2017 at 01:02:38PM -0700, David Daney wrote:

On 07/26/2017 10:33 AM, Greg KH wrote:

On Wed, Jul 26, 2017 at 06:30:49PM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:

On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:

The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev *'.
I'm not aware of other ways to access these devices. Please enlighten
me if I'm missing something.


Me enlighten you on Cavium hardware?! You're funny.

So I don't know whether the PCI hotplug code can run more than one
function upon PCI ID detection. Probably Greg will say, write a
multiplexer wrapper. :-)


-ENOCONTEXT

Anyway, pci questions are best asked on the linux-pci@vger list.  And
yes, all PCI devices end up with a 'struct pci_dev *' automatically.


Simple: so they have a PCI ID of a memory contoller and want to hotplug
two drivers for it. And those two drivers should remain independent from
each other.


Hahahahaha, no.  That's crazy, you were right in guessing what my answer
was going to be :)




Just to be clear about the situation, the device is a memory controller.  It
has two main behaviors we are interested in:

A) Error Detection And Correction (EDAC).  This should be connected to the
kernel's EDAC subsystem.  An existing driver (drivers/edac/thunderx_edac.c)
does exactly this.

B) Performance Counters for actions taken in the corresponding memory. This
should be connected to the kernel's perf framework as an uncore-PMU (the
subject of this patch set).

It is a single PCI device.  What should the driver architecture look like to
connect it to two different kernel subsystems?


Modify the drivers/edac/thunderx_edac.c code to add support for
performance counters.



Thanks Greg.  This adds some clarity to the situation.

This technique does slightly complicate the mapping of files and 
directories in the kernel source tree to maintainers.


Also, if a given configuration disables CONFIG_EDAC there is some 
hackery needed to get the perf portion of the driver included.


David Daney

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[David Daney]

On 07/26/2017 01:08 PM, Greg KH wrote:

On Wed, Jul 26, 2017 at 01:02:38PM -0700, David Daney wrote:

On 07/26/2017 10:33 AM, Greg KH wrote:

On Wed, Jul 26, 2017 at 06:30:49PM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:

On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:

The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev *'.
I'm not aware of other ways to access these devices. Please enlighten
me if I'm missing something.


Me enlighten you on Cavium hardware?! You're funny.

So I don't know whether the PCI hotplug code can run more than one
function upon PCI ID detection. Probably Greg will say, write a
multiplexer wrapper. :-)


-ENOCONTEXT

Anyway, pci questions are best asked on the linux-pci@vger list.  And
yes, all PCI devices end up with a 'struct pci_dev *' automatically.


Simple: so they have a PCI ID of a memory contoller and want to hotplug
two drivers for it. And those two drivers should remain independent from
each other.


Hahahahaha, no.  That's crazy, you were right in guessing what my answer
was going to be :)




Just to be clear about the situation, the device is a memory controller.  It
has two main behaviors we are interested in:

A) Error Detection And Correction (EDAC).  This should be connected to the
kernel's EDAC subsystem.  An existing driver (drivers/edac/thunderx_edac.c)
does exactly this.

B) Performance Counters for actions taken in the corresponding memory. This
should be connected to the kernel's perf framework as an uncore-PMU (the
subject of this patch set).

It is a single PCI device.  What should the driver architecture look like to
connect it to two different kernel subsystems?


Modify the drivers/edac/thunderx_edac.c code to add support for
performance counters.



Thanks Greg.  This adds some clarity to the situation.

This technique does slightly complicate the mapping of files and 
directories in the kernel source tree to maintainers.


Also, if a given configuration disables CONFIG_EDAC there is some 
hackery needed to get the perf portion of the driver included.


David Daney

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Greg KH]

On Wed, Jul 26, 2017 at 01:02:38PM -0700, David Daney wrote:
> On 07/26/2017 10:33 AM, Greg KH wrote:
> > On Wed, Jul 26, 2017 at 06:30:49PM +0200, Borislav Petkov wrote:
> > > On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:
> > > > On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:
> > > > > On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:
> > > > > > The PMU/EDAC devices are all PCI devices do I need the 'struct 
> > > > > > pci_dev *'.
> > > > > > I'm not aware of other ways to access these devices. Please 
> > > > > > enlighten
> > > > > > me if I'm missing something.
> > > > > 
> > > > > Me enlighten you on Cavium hardware?! You're funny.
> > > > > 
> > > > > So I don't know whether the PCI hotplug code can run more than one
> > > > > function upon PCI ID detection. Probably Greg will say, write a
> > > > > multiplexer wrapper. :-)
> > > > 
> > > > -ENOCONTEXT
> > > > 
> > > > Anyway, pci questions are best asked on the linux-pci@vger list.  And
> > > > yes, all PCI devices end up with a 'struct pci_dev *' automatically.
> > > 
> > > Simple: so they have a PCI ID of a memory contoller and want to hotplug
> > > two drivers for it. And those two drivers should remain independent from
> > > each other.
> > 
> > Hahahahaha, no.  That's crazy, you were right in guessing what my answer
> > was going to be :)
> > 
> 
> 
> Just to be clear about the situation, the device is a memory controller.  It
> has two main behaviors we are interested in:
> 
> A) Error Detection And Correction (EDAC).  This should be connected to the
> kernel's EDAC subsystem.  An existing driver (drivers/edac/thunderx_edac.c)
> does exactly this.
> 
> B) Performance Counters for actions taken in the corresponding memory. This
> should be connected to the kernel's perf framework as an uncore-PMU (the
> subject of this patch set).
> 
> It is a single PCI device.  What should the driver architecture look like to
> connect it to two different kernel subsystems?

Modify the drivers/edac/thunderx_edac.c code to add support for
performance counters.

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Greg KH]

On Wed, Jul 26, 2017 at 01:02:38PM -0700, David Daney wrote:
> On 07/26/2017 10:33 AM, Greg KH wrote:
> > On Wed, Jul 26, 2017 at 06:30:49PM +0200, Borislav Petkov wrote:
> > > On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:
> > > > On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:
> > > > > On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:
> > > > > > The PMU/EDAC devices are all PCI devices do I need the 'struct 
> > > > > > pci_dev *'.
> > > > > > I'm not aware of other ways to access these devices. Please 
> > > > > > enlighten
> > > > > > me if I'm missing something.
> > > > > 
> > > > > Me enlighten you on Cavium hardware?! You're funny.
> > > > > 
> > > > > So I don't know whether the PCI hotplug code can run more than one
> > > > > function upon PCI ID detection. Probably Greg will say, write a
> > > > > multiplexer wrapper. :-)
> > > > 
> > > > -ENOCONTEXT
> > > > 
> > > > Anyway, pci questions are best asked on the linux-pci@vger list.  And
> > > > yes, all PCI devices end up with a 'struct pci_dev *' automatically.
> > > 
> > > Simple: so they have a PCI ID of a memory contoller and want to hotplug
> > > two drivers for it. And those two drivers should remain independent from
> > > each other.
> > 
> > Hahahahaha, no.  That's crazy, you were right in guessing what my answer
> > was going to be :)
> > 
> 
> 
> Just to be clear about the situation, the device is a memory controller.  It
> has two main behaviors we are interested in:
> 
> A) Error Detection And Correction (EDAC).  This should be connected to the
> kernel's EDAC subsystem.  An existing driver (drivers/edac/thunderx_edac.c)
> does exactly this.
> 
> B) Performance Counters for actions taken in the corresponding memory. This
> should be connected to the kernel's perf framework as an uncore-PMU (the
> subject of this patch set).
> 
> It is a single PCI device.  What should the driver architecture look like to
> connect it to two different kernel subsystems?

Modify the drivers/edac/thunderx_edac.c code to add support for
performance counters.

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[David Daney]

On 07/26/2017 10:33 AM, Greg KH wrote:

On Wed, Jul 26, 2017 at 06:30:49PM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:

On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:

The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev *'.
I'm not aware of other ways to access these devices. Please enlighten
me if I'm missing something.


Me enlighten you on Cavium hardware?! You're funny.

So I don't know whether the PCI hotplug code can run more than one
function upon PCI ID detection. Probably Greg will say, write a
multiplexer wrapper. :-)


-ENOCONTEXT

Anyway, pci questions are best asked on the linux-pci@vger list.  And
yes, all PCI devices end up with a 'struct pci_dev *' automatically.


Simple: so they have a PCI ID of a memory contoller and want to hotplug
two drivers for it. And those two drivers should remain independent from
each other.


Hahahahaha, no.  That's crazy, you were right in guessing what my answer
was going to be :)




Just to be clear about the situation, the device is a memory controller. 
 It has two main behaviors we are interested in:


A) Error Detection And Correction (EDAC).  This should be connected to 
the kernel's EDAC subsystem.  An existing driver 
(drivers/edac/thunderx_edac.c) does exactly this.


B) Performance Counters for actions taken in the corresponding memory. 
This should be connected to the kernel's perf framework as an uncore-PMU 
(the subject of this patch set).


It is a single PCI device.  What should the driver architecture look 
like to connect it to two different kernel subsystems?


Thanks,
David Daney

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[David Daney]

On 07/26/2017 10:33 AM, Greg KH wrote:

On Wed, Jul 26, 2017 at 06:30:49PM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:

On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:

The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev *'.
I'm not aware of other ways to access these devices. Please enlighten
me if I'm missing something.


Me enlighten you on Cavium hardware?! You're funny.

So I don't know whether the PCI hotplug code can run more than one
function upon PCI ID detection. Probably Greg will say, write a
multiplexer wrapper. :-)


-ENOCONTEXT

Anyway, pci questions are best asked on the linux-pci@vger list.  And
yes, all PCI devices end up with a 'struct pci_dev *' automatically.


Simple: so they have a PCI ID of a memory contoller and want to hotplug
two drivers for it. And those two drivers should remain independent from
each other.


Hahahahaha, no.  That's crazy, you were right in guessing what my answer
was going to be :)




Just to be clear about the situation, the device is a memory controller. 
 It has two main behaviors we are interested in:


A) Error Detection And Correction (EDAC).  This should be connected to 
the kernel's EDAC subsystem.  An existing driver 
(drivers/edac/thunderx_edac.c) does exactly this.


B) Performance Counters for actions taken in the corresponding memory. 
This should be connected to the kernel's perf framework as an uncore-PMU 
(the subject of this patch set).


It is a single PCI device.  What should the driver architecture look 
like to connect it to two different kernel subsystems?


Thanks,
David Daney

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Greg KH]

On Wed, Jul 26, 2017 at 06:30:49PM +0200, Borislav Petkov wrote:
> On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:
> > On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:
> > > On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:
> > > > The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev 
> > > > *'.
> > > > I'm not aware of other ways to access these devices. Please enlighten
> > > > me if I'm missing something.
> > > 
> > > Me enlighten you on Cavium hardware?! You're funny.
> > > 
> > > So I don't know whether the PCI hotplug code can run more than one
> > > function upon PCI ID detection. Probably Greg will say, write a
> > > multiplexer wrapper. :-)
> > 
> > -ENOCONTEXT
> > 
> > Anyway, pci questions are best asked on the linux-pci@vger list.  And
> > yes, all PCI devices end up with a 'struct pci_dev *' automatically.
> 
> Simple: so they have a PCI ID of a memory contoller and want to hotplug
> two drivers for it. And those two drivers should remain independent from
> each other.

Hahahahaha, no.  That's crazy, you were right in guessing what my answer
was going to be :)

greg k-h

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Greg KH]

On Wed, Jul 26, 2017 at 06:30:49PM +0200, Borislav Petkov wrote:
> On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:
> > On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:
> > > On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:
> > > > The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev 
> > > > *'.
> > > > I'm not aware of other ways to access these devices. Please enlighten
> > > > me if I'm missing something.
> > > 
> > > Me enlighten you on Cavium hardware?! You're funny.
> > > 
> > > So I don't know whether the PCI hotplug code can run more than one
> > > function upon PCI ID detection. Probably Greg will say, write a
> > > multiplexer wrapper. :-)
> > 
> > -ENOCONTEXT
> > 
> > Anyway, pci questions are best asked on the linux-pci@vger list.  And
> > yes, all PCI devices end up with a 'struct pci_dev *' automatically.
> 
> Simple: so they have a PCI ID of a memory contoller and want to hotplug
> two drivers for it. And those two drivers should remain independent from
> each other.

Hahahahaha, no.  That's crazy, you were right in guessing what my answer
was going to be :)

greg k-h

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Wed, Jul 26, 2017 at 05:25:15PM +0100, Jonathan Cameron wrote:
> On Wed, 26 Jul 2017 17:46:23 +0200
> Jan Glauber  wrote:
> 
> > On Wed, Jul 26, 2017 at 04:17:11PM +0100, Suzuki K Poulose wrote:
> > > How about adding a soc specific (wrapper) driver for the memory 
> > > controller, which
> > > could use the PCI id and trigger EDAC and PMU drivers (based on what is
> > > selected by configs)  ?  
> > 
> > Sounds good to me. Is there a driver that already does this?
> Sounds like a classic MFD (multifunction device). There are quite a few pci
> devices to be found under drivers/mfd/ than may provide some inspiration.

I've looked into that before, from what I recall it did not fit my use
case. After all these are multi-fn devices.

> Jonathan
> > 
> > --Jan
> > 
> > ___
> > linux-arm-kernel mailing list
> > linux-arm-ker...@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Wed, Jul 26, 2017 at 05:25:15PM +0100, Jonathan Cameron wrote:
> On Wed, 26 Jul 2017 17:46:23 +0200
> Jan Glauber  wrote:
> 
> > On Wed, Jul 26, 2017 at 04:17:11PM +0100, Suzuki K Poulose wrote:
> > > How about adding a soc specific (wrapper) driver for the memory 
> > > controller, which
> > > could use the PCI id and trigger EDAC and PMU drivers (based on what is
> > > selected by configs)  ?  
> > 
> > Sounds good to me. Is there a driver that already does this?
> Sounds like a classic MFD (multifunction device). There are quite a few pci
> devices to be found under drivers/mfd/ than may provide some inspiration.

I've looked into that before, from what I recall it did not fit my use
case. After all these are multi-fn devices.

> Jonathan
> > 
> > --Jan
> > 
> > ___
> > linux-arm-kernel mailing list
> > linux-arm-ker...@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:
> On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:
> > On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:
> > > The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev *'.
> > > I'm not aware of other ways to access these devices. Please enlighten
> > > me if I'm missing something.
> > 
> > Me enlighten you on Cavium hardware?! You're funny.
> > 
> > So I don't know whether the PCI hotplug code can run more than one
> > function upon PCI ID detection. Probably Greg will say, write a
> > multiplexer wrapper. :-)
> 
> -ENOCONTEXT
> 
> Anyway, pci questions are best asked on the linux-pci@vger list.  And
> yes, all PCI devices end up with a 'struct pci_dev *' automatically.

Simple: so they have a PCI ID of a memory contoller and want to hotplug
two drivers for it. And those two drivers should remain independent from
each other.

Thanks.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Wed, Jul 26, 2017 at 09:19:49AM -0700, Greg KH wrote:
> On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:
> > On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:
> > > The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev *'.
> > > I'm not aware of other ways to access these devices. Please enlighten
> > > me if I'm missing something.
> > 
> > Me enlighten you on Cavium hardware?! You're funny.
> > 
> > So I don't know whether the PCI hotplug code can run more than one
> > function upon PCI ID detection. Probably Greg will say, write a
> > multiplexer wrapper. :-)
> 
> -ENOCONTEXT
> 
> Anyway, pci questions are best asked on the linux-pci@vger list.  And
> yes, all PCI devices end up with a 'struct pci_dev *' automatically.

Simple: so they have a PCI ID of a memory contoller and want to hotplug
two drivers for it. And those two drivers should remain independent from
each other.

Thanks.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jonathan Cameron]

On Wed, 26 Jul 2017 17:46:23 +0200
Jan Glauber  wrote:

> On Wed, Jul 26, 2017 at 04:17:11PM +0100, Suzuki K Poulose wrote:
> > How about adding a soc specific (wrapper) driver for the memory controller, 
> > which
> > could use the PCI id and trigger EDAC and PMU drivers (based on what is
> > selected by configs)  ?  
> 
> Sounds good to me. Is there a driver that already does this?
Sounds like a classic MFD (multifunction device). There are quite a few pci
devices to be found under drivers/mfd/ than may provide some inspiration.

Jonathan
> 
> --Jan
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jonathan Cameron]

On Wed, 26 Jul 2017 17:46:23 +0200
Jan Glauber  wrote:

> On Wed, Jul 26, 2017 at 04:17:11PM +0100, Suzuki K Poulose wrote:
> > How about adding a soc specific (wrapper) driver for the memory controller, 
> > which
> > could use the PCI id and trigger EDAC and PMU drivers (based on what is
> > selected by configs)  ?  
> 
> Sounds good to me. Is there a driver that already does this?
Sounds like a classic MFD (multifunction device). There are quite a few pci
devices to be found under drivers/mfd/ than may provide some inspiration.

Jonathan
> 
> --Jan
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Greg KH]

On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:
> On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:
> > The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev *'.
> > I'm not aware of other ways to access these devices. Please enlighten
> > me if I'm missing something.
> 
> Me enlighten you on Cavium hardware?! You're funny.
> 
> So I don't know whether the PCI hotplug code can run more than one
> function upon PCI ID detection. Probably Greg will say, write a
> multiplexer wrapper. :-)

-ENOCONTEXT

Anyway, pci questions are best asked on the linux-pci@vger list.  And
yes, all PCI devices end up with a 'struct pci_dev *' automatically.

thanks,

greg k-h

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Greg KH]

On Wed, Jul 26, 2017 at 05:55:48PM +0200, Borislav Petkov wrote:
> On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:
> > The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev *'.
> > I'm not aware of other ways to access these devices. Please enlighten
> > me if I'm missing something.
> 
> Me enlighten you on Cavium hardware?! You're funny.
> 
> So I don't know whether the PCI hotplug code can run more than one
> function upon PCI ID detection. Probably Greg will say, write a
> multiplexer wrapper. :-)

-ENOCONTEXT

Anyway, pci questions are best asked on the linux-pci@vger list.  And
yes, all PCI devices end up with a 'struct pci_dev *' automatically.

thanks,

greg k-h

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:
> The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev *'.
> I'm not aware of other ways to access these devices. Please enlighten
> me if I'm missing something.

Me enlighten you on Cavium hardware?! You're funny.

So I don't know whether the PCI hotplug code can run more than one
function upon PCI ID detection. Probably Greg will say, write a
multiplexer wrapper. :-)

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Wed, Jul 26, 2017 at 05:45:15PM +0200, Jan Glauber wrote:
> The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev *'.
> I'm not aware of other ways to access these devices. Please enlighten
> me if I'm missing something.

Me enlighten you on Cavium hardware?! You're funny.

So I don't know whether the PCI hotplug code can run more than one
function upon PCI ID detection. Probably Greg will say, write a
multiplexer wrapper. :-)

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Wed, Jul 26, 2017 at 04:17:11PM +0100, Suzuki K Poulose wrote:
> How about adding a soc specific (wrapper) driver for the memory controller, 
> which
> could use the PCI id and trigger EDAC and PMU drivers (based on what is
> selected by configs)  ?

Sounds good to me. Is there a driver that already does this?

--Jan

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Wed, Jul 26, 2017 at 04:17:11PM +0100, Suzuki K Poulose wrote:
> How about adding a soc specific (wrapper) driver for the memory controller, 
> which
> could use the PCI id and trigger EDAC and PMU drivers (based on what is
> selected by configs)  ?

Sounds good to me. Is there a driver that already does this?

--Jan

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Wed, Jul 26, 2017 at 05:35:02PM +0200, Borislav Petkov wrote:
> On Wed, Jul 26, 2017 at 05:13:14PM +0200, Jan Glauber wrote:
> > I'm also looking for CPU implementor (MIDR), I could check for the model
> > too but I still need to detect devices based on PCI IDs as the model
> > check is not sufficient here (only multi-socket ThunderX has OCX TLK
> > devices).
> 
> So what does that mean? The only way to load a PMU driver and an EDAC
> driver is the PCI ID of the memory controller? No other way?

I already tried multiple ways to load the drivers, so far with limited
success :)

The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev *'.
I'm not aware of other ways to access these devices. Please enlighten
me if I'm missing something.

--Jan

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Wed, Jul 26, 2017 at 05:35:02PM +0200, Borislav Petkov wrote:
> On Wed, Jul 26, 2017 at 05:13:14PM +0200, Jan Glauber wrote:
> > I'm also looking for CPU implementor (MIDR), I could check for the model
> > too but I still need to detect devices based on PCI IDs as the model
> > check is not sufficient here (only multi-socket ThunderX has OCX TLK
> > devices).
> 
> So what does that mean? The only way to load a PMU driver and an EDAC
> driver is the PCI ID of the memory controller? No other way?

I already tried multiple ways to load the drivers, so far with limited
success :)

The PMU/EDAC devices are all PCI devices do I need the 'struct pci_dev *'.
I'm not aware of other ways to access these devices. Please enlighten
me if I'm missing something.

--Jan

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Wed, Jul 26, 2017 at 05:13:14PM +0200, Jan Glauber wrote:
> I'm also looking for CPU implementor (MIDR), I could check for the model
> too but I still need to detect devices based on PCI IDs as the model
> check is not sufficient here (only multi-socket ThunderX has OCX TLK
> devices).

So what does that mean? The only way to load a PMU driver and an EDAC
driver is the PCI ID of the memory controller? No other way?

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Wed, Jul 26, 2017 at 05:13:14PM +0200, Jan Glauber wrote:
> I'm also looking for CPU implementor (MIDR), I could check for the model
> too but I still need to detect devices based on PCI IDs as the model
> check is not sufficient here (only multi-socket ThunderX has OCX TLK
> devices).

So what does that mean? The only way to load a PMU driver and an EDAC
driver is the PCI ID of the memory controller? No other way?

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Wed, Jul 26, 2017 at 04:17:11PM +0100, Suzuki K Poulose wrote:
> How about adding a soc specific (wrapper) driver for the memory controller, 
> which
> could use the PCI id and trigger EDAC and PMU drivers (based on what is
> selected by configs)  ?

That was the last idea on my list if nothing slicker comes up. Having a
"multiplexer" driver just for the loading is meh.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Wed, Jul 26, 2017 at 04:17:11PM +0100, Suzuki K Poulose wrote:
> How about adding a soc specific (wrapper) driver for the memory controller, 
> which
> could use the PCI id and trigger EDAC and PMU drivers (based on what is
> selected by configs)  ?

That was the last idea on my list if nothing slicker comes up. Having a
"multiplexer" driver just for the loading is meh.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Suzuki K Poulose]

On 26/07/17 16:13, Jan Glauber wrote:

On Wed, Jul 26, 2017 at 04:55:22PM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 03:35:25PM +0100, Suzuki K Poulose wrote:

So the Cavium EDACs, which appear as PCI devices have a PMU attached to it.


Cavium EDACs?

So let me set something straight first: An EDAC driver simply talks to
some RAS IP block and reports errors. It shouldn't have anything to do
with a PMU.


In order to build this PMU driver as a module, we need a way to load the module
automatically based on the PCI id. However, since the EDAC driver already
registers with that PCI id, we cannot use the same for the PMU. Ideally,


So this is strange. There's a single PCI ID but multiple functionalities
behind it?


Yes, but I would still not call a memory controller a RAS IP block.



There are a number of registers on the memory controller (or on the OCX
TLK interconnect), and while some of them are RAS related there are also
other registers in the same device like the counters we want to access
via PMU code.


How about adding a soc specific (wrapper) driver for the memory controller, 
which
could use the PCI id and trigger EDAC and PMU drivers (based on what is
selected by configs)  ?

Suzuki

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Suzuki K Poulose]

On 26/07/17 16:13, Jan Glauber wrote:

On Wed, Jul 26, 2017 at 04:55:22PM +0200, Borislav Petkov wrote:

On Wed, Jul 26, 2017 at 03:35:25PM +0100, Suzuki K Poulose wrote:

So the Cavium EDACs, which appear as PCI devices have a PMU attached to it.


Cavium EDACs?

So let me set something straight first: An EDAC driver simply talks to
some RAS IP block and reports errors. It shouldn't have anything to do
with a PMU.


In order to build this PMU driver as a module, we need a way to load the module
automatically based on the PCI id. However, since the EDAC driver already
registers with that PCI id, we cannot use the same for the PMU. Ideally,


So this is strange. There's a single PCI ID but multiple functionalities
behind it?


Yes, but I would still not call a memory controller a RAS IP block.



There are a number of registers on the memory controller (or on the OCX
TLK interconnect), and while some of them are RAS related there are also
other registers in the same device like the counters we want to access
via PMU code.


How about adding a soc specific (wrapper) driver for the memory controller, 
which
could use the PCI id and trigger EDAC and PMU drivers (based on what is
selected by configs)  ?

Suzuki

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Wed, Jul 26, 2017 at 04:55:22PM +0200, Borislav Petkov wrote:
> On Wed, Jul 26, 2017 at 03:35:25PM +0100, Suzuki K Poulose wrote:
> > So the Cavium EDACs, which appear as PCI devices have a PMU attached to it.
> 
> Cavium EDACs?
> 
> So let me set something straight first: An EDAC driver simply talks to
> some RAS IP block and reports errors. It shouldn't have anything to do
> with a PMU.
> 
> > In order to build this PMU driver as a module, we need a way to load the 
> > module
> > automatically based on the PCI id. However, since the EDAC driver already
> > registers with that PCI id, we cannot use the same for the PMU. Ideally,
> 
> So this is strange. There's a single PCI ID but multiple functionalities
> behind it?

Yes, but I would still not call a memory controller a RAS IP block.
There are a number of registers on the memory controller (or on the OCX
TLK interconnect), and while some of them are RAS related there are also
other registers in the same device like the counters we want to access
via PMU code.

> > the PMU driver should be loaded when the EDAC driver is loaded. But looking
> > at the links above, it looks like you don't like the idea of triggering a
> > probe of the PMU component from the EDAC driver. We may be able to get rid
> > of the PMU specific information from the EDAC driver by maintaining the PCI
> > id of the device in the PMU driver. But we may still need to make sure that
> > the PMU driver gets a chance to probe the PMU when the device is available.
> > 
> > What do you think is the best option here ?
> 
> Can either of the two - EDAC or PMU driver - use an alternate detection
> method?

I'm currently using pci_get_device(vendor-ID, device-ID, ...) which
works fine.

> For example, we moved the main x86 EDAC drivers we moved to x86 CPU
> family, model, stepping detection from PCI IDs because the PCI IDs were
> clumsy to use.

I'm also looking for CPU implementor (MIDR), I could check for the model
too but I still need to detect devices based on PCI IDs as the model
check is not sufficient here (only multi-socket ThunderX has OCX TLK
devices).

--Jan

> -- 
> Regards/Gruss,
> Boris.
> 
> ECO tip #101: Trim your mails when you reply.
> --

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Wed, Jul 26, 2017 at 04:55:22PM +0200, Borislav Petkov wrote:
> On Wed, Jul 26, 2017 at 03:35:25PM +0100, Suzuki K Poulose wrote:
> > So the Cavium EDACs, which appear as PCI devices have a PMU attached to it.
> 
> Cavium EDACs?
> 
> So let me set something straight first: An EDAC driver simply talks to
> some RAS IP block and reports errors. It shouldn't have anything to do
> with a PMU.
> 
> > In order to build this PMU driver as a module, we need a way to load the 
> > module
> > automatically based on the PCI id. However, since the EDAC driver already
> > registers with that PCI id, we cannot use the same for the PMU. Ideally,
> 
> So this is strange. There's a single PCI ID but multiple functionalities
> behind it?

Yes, but I would still not call a memory controller a RAS IP block.
There are a number of registers on the memory controller (or on the OCX
TLK interconnect), and while some of them are RAS related there are also
other registers in the same device like the counters we want to access
via PMU code.

> > the PMU driver should be loaded when the EDAC driver is loaded. But looking
> > at the links above, it looks like you don't like the idea of triggering a
> > probe of the PMU component from the EDAC driver. We may be able to get rid
> > of the PMU specific information from the EDAC driver by maintaining the PCI
> > id of the device in the PMU driver. But we may still need to make sure that
> > the PMU driver gets a chance to probe the PMU when the device is available.
> > 
> > What do you think is the best option here ?
> 
> Can either of the two - EDAC or PMU driver - use an alternate detection
> method?

I'm currently using pci_get_device(vendor-ID, device-ID, ...) which
works fine.

> For example, we moved the main x86 EDAC drivers we moved to x86 CPU
> family, model, stepping detection from PCI IDs because the PCI IDs were
> clumsy to use.

I'm also looking for CPU implementor (MIDR), I could check for the model
too but I still need to detect devices based on PCI IDs as the model
check is not sufficient here (only multi-socket ThunderX has OCX TLK
devices).

--Jan

> -- 
> Regards/Gruss,
> Boris.
> 
> ECO tip #101: Trim your mails when you reply.
> --

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Wed, Jul 26, 2017 at 03:35:25PM +0100, Suzuki K Poulose wrote:
> So the Cavium EDACs, which appear as PCI devices have a PMU attached to it.

Cavium EDACs?

So let me set something straight first: An EDAC driver simply talks to
some RAS IP block and reports errors. It shouldn't have anything to do
with a PMU.

> In order to build this PMU driver as a module, we need a way to load the 
> module
> automatically based on the PCI id. However, since the EDAC driver already
> registers with that PCI id, we cannot use the same for the PMU. Ideally,

So this is strange. There's a single PCI ID but multiple functionalities
behind it?

> the PMU driver should be loaded when the EDAC driver is loaded. But looking
> at the links above, it looks like you don't like the idea of triggering a
> probe of the PMU component from the EDAC driver. We may be able to get rid
> of the PMU specific information from the EDAC driver by maintaining the PCI
> id of the device in the PMU driver. But we may still need to make sure that
> the PMU driver gets a chance to probe the PMU when the device is available.
> 
> What do you think is the best option here ?

Can either of the two - EDAC or PMU driver - use an alternate detection
method?

For example, we moved the main x86 EDAC drivers we moved to x86 CPU
family, model, stepping detection from PCI IDs because the PCI IDs were
clumsy to use.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Borislav Petkov]

On Wed, Jul 26, 2017 at 03:35:25PM +0100, Suzuki K Poulose wrote:
> So the Cavium EDACs, which appear as PCI devices have a PMU attached to it.

Cavium EDACs?

So let me set something straight first: An EDAC driver simply talks to
some RAS IP block and reports errors. It shouldn't have anything to do
with a PMU.

> In order to build this PMU driver as a module, we need a way to load the 
> module
> automatically based on the PCI id. However, since the EDAC driver already
> registers with that PCI id, we cannot use the same for the PMU. Ideally,

So this is strange. There's a single PCI ID but multiple functionalities
behind it?

> the PMU driver should be loaded when the EDAC driver is loaded. But looking
> at the links above, it looks like you don't like the idea of triggering a
> probe of the PMU component from the EDAC driver. We may be able to get rid
> of the PMU specific information from the EDAC driver by maintaining the PCI
> id of the device in the PMU driver. But we may still need to make sure that
> the PMU driver gets a chance to probe the PMU when the device is available.
> 
> What do you think is the best option here ?

Can either of the two - EDAC or PMU driver - use an alternate detection
method?

For example, we moved the main x86 EDAC drivers we moved to x86 CPU
family, model, stepping detection from PCI IDs because the PCI IDs were
clumsy to use.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Suzuki K Poulose]

+To: Boris

Hi Boris,

On 26/07/17 14:10, Jan Glauber wrote:

On Wed, Jul 26, 2017 at 01:47:35PM +0100, Suzuki K Poulose wrote:

On 26/07/17 12:19, Jan Glauber wrote:

On Tue, Jul 25, 2017 at 04:39:18PM +0100, Suzuki K Poulose wrote:

On 25/07/17 16:04, Jan Glauber wrote:

Add support for the PMU counters on Cavium SOC memory controllers.

This patch also adds generic functions to allow supporting more
devices with PMU counters.

Properties of the LMC PMU counters:
- not stoppable
- fixed purpose
- read-only
- one PCI device per memory controller

Signed-off-by: Jan Glauber 
---
drivers/perf/Kconfig   |   8 +
drivers/perf/Makefile  |   1 +
drivers/perf/cavium_pmu.c  | 424 +
include/linux/cpuhotplug.h |   1 +
4 files changed, 434 insertions(+)
create mode 100644 drivers/perf/cavium_pmu.c

diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index e5197ff..a46c3f0 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -43,4 +43,12 @@ config XGENE_PMU
   help
 Say y if you want to use APM X-Gene SoC performance monitors.

+config CAVIUM_PMU
+   bool "Cavium SOC PMU"


Is there any specific reason why this can't be built as a module ?


Yes. I don't know how to load the module automatically. I can't make it
a pci driver as the EDAC driver "owns" the device (and having two
drivers for one device wont work as far as I know). I tried to hook
into the EDAC driver but the EDAC maintainer was not overly welcoming
that approach.




And while it would be possible to have it a s a module I think it is of
no use if it requires manualy loading. But maybe there is a simple
solution I'm missing here?



If you are talking about a Cavium specific EDAC driver, may be we could
make that depend on this driver "at runtime" via symbols (may be even,
trigger the probe of PMU), which will be referenced only when CONFIG_CAVIUM_PMU
is defined. It is not the perfect solution, but that should do the trick.


I think that is roughly what I proposed in v6. Can you have a look at:

https://lkml.org/lkml/2017/6/23/333
https://patchwork.kernel.org/patch/9806427/


So the Cavium EDACs, which appear as PCI devices have a PMU attached to it.
In order to build this PMU driver as a module, we need a way to load the module
automatically based on the PCI id. However, since the EDAC driver already
registers with that PCI id, we cannot use the same for the PMU. Ideally,
the PMU driver should be loaded when the EDAC driver is loaded. But looking
at the links above, it looks like you don't like the idea of triggering a
probe of the PMU component from the EDAC driver. We may be able to get rid
of the PMU specific information from the EDAC driver by maintaining the PCI
id of the device in the PMU driver. But we may still need to make sure that
the PMU driver gets a chance to probe the PMU when the device is available.

What do you think is the best option here ?

Suzuki

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Suzuki K Poulose]

+To: Boris

Hi Boris,

On 26/07/17 14:10, Jan Glauber wrote:

On Wed, Jul 26, 2017 at 01:47:35PM +0100, Suzuki K Poulose wrote:

On 26/07/17 12:19, Jan Glauber wrote:

On Tue, Jul 25, 2017 at 04:39:18PM +0100, Suzuki K Poulose wrote:

On 25/07/17 16:04, Jan Glauber wrote:

Add support for the PMU counters on Cavium SOC memory controllers.

This patch also adds generic functions to allow supporting more
devices with PMU counters.

Properties of the LMC PMU counters:
- not stoppable
- fixed purpose
- read-only
- one PCI device per memory controller

Signed-off-by: Jan Glauber 
---
drivers/perf/Kconfig   |   8 +
drivers/perf/Makefile  |   1 +
drivers/perf/cavium_pmu.c  | 424 +
include/linux/cpuhotplug.h |   1 +
4 files changed, 434 insertions(+)
create mode 100644 drivers/perf/cavium_pmu.c

diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index e5197ff..a46c3f0 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -43,4 +43,12 @@ config XGENE_PMU
   help
 Say y if you want to use APM X-Gene SoC performance monitors.

+config CAVIUM_PMU
+   bool "Cavium SOC PMU"


Is there any specific reason why this can't be built as a module ?


Yes. I don't know how to load the module automatically. I can't make it
a pci driver as the EDAC driver "owns" the device (and having two
drivers for one device wont work as far as I know). I tried to hook
into the EDAC driver but the EDAC maintainer was not overly welcoming
that approach.




And while it would be possible to have it a s a module I think it is of
no use if it requires manualy loading. But maybe there is a simple
solution I'm missing here?



If you are talking about a Cavium specific EDAC driver, may be we could
make that depend on this driver "at runtime" via symbols (may be even,
trigger the probe of PMU), which will be referenced only when CONFIG_CAVIUM_PMU
is defined. It is not the perfect solution, but that should do the trick.


I think that is roughly what I proposed in v6. Can you have a look at:

https://lkml.org/lkml/2017/6/23/333
https://patchwork.kernel.org/patch/9806427/


So the Cavium EDACs, which appear as PCI devices have a PMU attached to it.
In order to build this PMU driver as a module, we need a way to load the module
automatically based on the PCI id. However, since the EDAC driver already
registers with that PCI id, we cannot use the same for the PMU. Ideally,
the PMU driver should be loaded when the EDAC driver is loaded. But looking
at the links above, it looks like you don't like the idea of triggering a
probe of the PMU component from the EDAC driver. We may be able to get rid
of the PMU specific information from the EDAC driver by maintaining the PCI
id of the device in the PMU driver. But we may still need to make sure that
the PMU driver gets a chance to probe the PMU when the device is available.

What do you think is the best option here ?

Suzuki

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Wed, Jul 26, 2017 at 01:47:35PM +0100, Suzuki K Poulose wrote:
> On 26/07/17 12:19, Jan Glauber wrote:
> >On Tue, Jul 25, 2017 at 04:39:18PM +0100, Suzuki K Poulose wrote:
> >>On 25/07/17 16:04, Jan Glauber wrote:
> >>>Add support for the PMU counters on Cavium SOC memory controllers.
> >>>
> >>>This patch also adds generic functions to allow supporting more
> >>>devices with PMU counters.
> >>>
> >>>Properties of the LMC PMU counters:
> >>>- not stoppable
> >>>- fixed purpose
> >>>- read-only
> >>>- one PCI device per memory controller
> >>>
> >>>Signed-off-by: Jan Glauber 
> >>>---
> >>>drivers/perf/Kconfig   |   8 +
> >>>drivers/perf/Makefile  |   1 +
> >>>drivers/perf/cavium_pmu.c  | 424 
> >>>+
> >>>include/linux/cpuhotplug.h |   1 +
> >>>4 files changed, 434 insertions(+)
> >>>create mode 100644 drivers/perf/cavium_pmu.c
> >>>
> >>>diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> >>>index e5197ff..a46c3f0 100644
> >>>--- a/drivers/perf/Kconfig
> >>>+++ b/drivers/perf/Kconfig
> >>>@@ -43,4 +43,12 @@ config XGENE_PMU
> >>>help
> >>>  Say y if you want to use APM X-Gene SoC performance monitors.
> >>>
> >>>+config CAVIUM_PMU
> >>>+  bool "Cavium SOC PMU"
> >>
> >>Is there any specific reason why this can't be built as a module ?
> >
> >Yes. I don't know how to load the module automatically. I can't make it
> >a pci driver as the EDAC driver "owns" the device (and having two
> >drivers for one device wont work as far as I know). I tried to hook
> >into the EDAC driver but the EDAC maintainer was not overly welcoming
> >that approach.
> 
> >
> >And while it would be possible to have it a s a module I think it is of
> >no use if it requires manualy loading. But maybe there is a simple
> >solution I'm missing here?
> 
> 
> If you are talking about a Cavium specific EDAC driver, may be we could
> make that depend on this driver "at runtime" via symbols (may be even,
> trigger the probe of PMU), which will be referenced only when 
> CONFIG_CAVIUM_PMU
> is defined. It is not the perfect solution, but that should do the trick.

I think that is roughly what I proposed in v6. Can you have a look at:

https://lkml.org/lkml/2017/6/23/333
https://patchwork.kernel.org/patch/9806427/

Probably there is a better way to do it. Or maybe we just keep it as
built-in for the time being.

--Jan

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Wed, Jul 26, 2017 at 01:47:35PM +0100, Suzuki K Poulose wrote:
> On 26/07/17 12:19, Jan Glauber wrote:
> >On Tue, Jul 25, 2017 at 04:39:18PM +0100, Suzuki K Poulose wrote:
> >>On 25/07/17 16:04, Jan Glauber wrote:
> >>>Add support for the PMU counters on Cavium SOC memory controllers.
> >>>
> >>>This patch also adds generic functions to allow supporting more
> >>>devices with PMU counters.
> >>>
> >>>Properties of the LMC PMU counters:
> >>>- not stoppable
> >>>- fixed purpose
> >>>- read-only
> >>>- one PCI device per memory controller
> >>>
> >>>Signed-off-by: Jan Glauber 
> >>>---
> >>>drivers/perf/Kconfig   |   8 +
> >>>drivers/perf/Makefile  |   1 +
> >>>drivers/perf/cavium_pmu.c  | 424 
> >>>+
> >>>include/linux/cpuhotplug.h |   1 +
> >>>4 files changed, 434 insertions(+)
> >>>create mode 100644 drivers/perf/cavium_pmu.c
> >>>
> >>>diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> >>>index e5197ff..a46c3f0 100644
> >>>--- a/drivers/perf/Kconfig
> >>>+++ b/drivers/perf/Kconfig
> >>>@@ -43,4 +43,12 @@ config XGENE_PMU
> >>>help
> >>>  Say y if you want to use APM X-Gene SoC performance monitors.
> >>>
> >>>+config CAVIUM_PMU
> >>>+  bool "Cavium SOC PMU"
> >>
> >>Is there any specific reason why this can't be built as a module ?
> >
> >Yes. I don't know how to load the module automatically. I can't make it
> >a pci driver as the EDAC driver "owns" the device (and having two
> >drivers for one device wont work as far as I know). I tried to hook
> >into the EDAC driver but the EDAC maintainer was not overly welcoming
> >that approach.
> 
> >
> >And while it would be possible to have it a s a module I think it is of
> >no use if it requires manualy loading. But maybe there is a simple
> >solution I'm missing here?
> 
> 
> If you are talking about a Cavium specific EDAC driver, may be we could
> make that depend on this driver "at runtime" via symbols (may be even,
> trigger the probe of PMU), which will be referenced only when 
> CONFIG_CAVIUM_PMU
> is defined. It is not the perfect solution, but that should do the trick.

I think that is roughly what I proposed in v6. Can you have a look at:

https://lkml.org/lkml/2017/6/23/333
https://patchwork.kernel.org/patch/9806427/

Probably there is a better way to do it. Or maybe we just keep it as
built-in for the time being.

--Jan

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Suzuki K Poulose]

On 26/07/17 12:19, Jan Glauber wrote:

On Tue, Jul 25, 2017 at 04:39:18PM +0100, Suzuki K Poulose wrote:

On 25/07/17 16:04, Jan Glauber wrote:

Add support for the PMU counters on Cavium SOC memory controllers.

This patch also adds generic functions to allow supporting more
devices with PMU counters.

Properties of the LMC PMU counters:
- not stoppable
- fixed purpose
- read-only
- one PCI device per memory controller

Signed-off-by: Jan Glauber 
---
drivers/perf/Kconfig   |   8 +
drivers/perf/Makefile  |   1 +
drivers/perf/cavium_pmu.c  | 424 +
include/linux/cpuhotplug.h |   1 +
4 files changed, 434 insertions(+)
create mode 100644 drivers/perf/cavium_pmu.c

diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index e5197ff..a46c3f0 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -43,4 +43,12 @@ config XGENE_PMU
help
  Say y if you want to use APM X-Gene SoC performance monitors.

+config CAVIUM_PMU
+   bool "Cavium SOC PMU"


Is there any specific reason why this can't be built as a module ?


Yes. I don't know how to load the module automatically. I can't make it
a pci driver as the EDAC driver "owns" the device (and having two
drivers for one device wont work as far as I know). I tried to hook
into the EDAC driver but the EDAC maintainer was not overly welcoming
that approach.




And while it would be possible to have it a s a module I think it is of
no use if it requires manualy loading. But maybe there is a simple
solution I'm missing here?



If you are talking about a Cavium specific EDAC driver, may be we could
make that depend on this driver "at runtime" via symbols (may be even,
trigger the probe of PMU), which will be referenced only when CONFIG_CAVIUM_PMU
is defined. It is not the perfect solution, but that should do the trick.



+   /*
+* Forbid groups containing mixed PMUs, software events are acceptable.
+*/
+   if (event->group_leader->pmu != event->pmu &&
+   !is_software_event(event->group_leader))
+   return -EINVAL;
+
+   list_for_each_entry(sibling, >group_leader->sibling_list,
+   group_entry)
+   if (sibling->pmu != event->pmu &&
+   !is_software_event(sibling))
+   return -EINVAL;


Do we also need to check if the events in the same group can be scheduled
at once ? i.e, there is enough resources to schedule the requested events from
the group.



Not sure what you mean, do I need to check for programmable counters
that no more counters are programmed than available?



Yes. What if there are two events, both trying to use the same counter (either
due to lack of programmable counters or duplicate events).


+
+   hwc->config = event->attr.config;
+   hwc->idx = -1;
+   return 0;
+}
+

...


+static int cvm_pmu_add(struct perf_event *event, int flags, u64 config_base,
+  u64 event_base)
+{
+   struct cvm_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
+   struct hw_perf_event *hwc = >hw;
+
+   if (!cmpxchg(_dev->events[hwc->config], NULL, event))
+   hwc->idx = hwc->config;
+
+   if (hwc->idx == -1)
+   return -EBUSY;
+
+   hwc->config_base = config_base;
+   hwc->event_base = event_base;
+   hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
+
+   if (flags & PERF_EF_START)
+   pmu_dev->pmu.start(event, PERF_EF_RELOAD);
+
+   return 0;
+}
+
+static void cvm_pmu_del(struct perf_event *event, int flags)
+{
+   struct cvm_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
+   struct hw_perf_event *hwc = >hw;
+   int i;
+
+   event->pmu->stop(event, PERF_EF_UPDATE);
+
+   /*
+* For programmable counters we need to check where we installed it.
+* To keep this function generic always test the more complicated
+* case (free running counters won't need the loop).
+*/
+   for (i = 0; i < pmu_dev->num_counters; i++)
+   if (cmpxchg(_dev->events[i], event, NULL) == event)
+   break;


I couldn't see why hwc->config wouldn't give us the index where we installed
the event in pmu_dev->events. What am I missing ?


Did you see the comment above? It is not yet needed but will be when I
add support for programmable counters.


Is it supported in this series ?


If it is still confusing I can
also remove that for now and add it back later when it is needed.


What is the hwc->idx for programmable counters ? is it going to be different
than hwc->config ? If so, can we use hwc->idx to keep the idx where we installed
the event ?

Suzuki

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Suzuki K Poulose]

On 26/07/17 12:19, Jan Glauber wrote:

On Tue, Jul 25, 2017 at 04:39:18PM +0100, Suzuki K Poulose wrote:

On 25/07/17 16:04, Jan Glauber wrote:

Add support for the PMU counters on Cavium SOC memory controllers.

This patch also adds generic functions to allow supporting more
devices with PMU counters.

Properties of the LMC PMU counters:
- not stoppable
- fixed purpose
- read-only
- one PCI device per memory controller

Signed-off-by: Jan Glauber 
---
drivers/perf/Kconfig   |   8 +
drivers/perf/Makefile  |   1 +
drivers/perf/cavium_pmu.c  | 424 +
include/linux/cpuhotplug.h |   1 +
4 files changed, 434 insertions(+)
create mode 100644 drivers/perf/cavium_pmu.c

diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index e5197ff..a46c3f0 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -43,4 +43,12 @@ config XGENE_PMU
help
  Say y if you want to use APM X-Gene SoC performance monitors.

+config CAVIUM_PMU
+   bool "Cavium SOC PMU"


Is there any specific reason why this can't be built as a module ?


Yes. I don't know how to load the module automatically. I can't make it
a pci driver as the EDAC driver "owns" the device (and having two
drivers for one device wont work as far as I know). I tried to hook
into the EDAC driver but the EDAC maintainer was not overly welcoming
that approach.




And while it would be possible to have it a s a module I think it is of
no use if it requires manualy loading. But maybe there is a simple
solution I'm missing here?



If you are talking about a Cavium specific EDAC driver, may be we could
make that depend on this driver "at runtime" via symbols (may be even,
trigger the probe of PMU), which will be referenced only when CONFIG_CAVIUM_PMU
is defined. It is not the perfect solution, but that should do the trick.



+   /*
+* Forbid groups containing mixed PMUs, software events are acceptable.
+*/
+   if (event->group_leader->pmu != event->pmu &&
+   !is_software_event(event->group_leader))
+   return -EINVAL;
+
+   list_for_each_entry(sibling, >group_leader->sibling_list,
+   group_entry)
+   if (sibling->pmu != event->pmu &&
+   !is_software_event(sibling))
+   return -EINVAL;


Do we also need to check if the events in the same group can be scheduled
at once ? i.e, there is enough resources to schedule the requested events from
the group.



Not sure what you mean, do I need to check for programmable counters
that no more counters are programmed than available?



Yes. What if there are two events, both trying to use the same counter (either
due to lack of programmable counters or duplicate events).


+
+   hwc->config = event->attr.config;
+   hwc->idx = -1;
+   return 0;
+}
+

...


+static int cvm_pmu_add(struct perf_event *event, int flags, u64 config_base,
+  u64 event_base)
+{
+   struct cvm_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
+   struct hw_perf_event *hwc = >hw;
+
+   if (!cmpxchg(_dev->events[hwc->config], NULL, event))
+   hwc->idx = hwc->config;
+
+   if (hwc->idx == -1)
+   return -EBUSY;
+
+   hwc->config_base = config_base;
+   hwc->event_base = event_base;
+   hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
+
+   if (flags & PERF_EF_START)
+   pmu_dev->pmu.start(event, PERF_EF_RELOAD);
+
+   return 0;
+}
+
+static void cvm_pmu_del(struct perf_event *event, int flags)
+{
+   struct cvm_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
+   struct hw_perf_event *hwc = >hw;
+   int i;
+
+   event->pmu->stop(event, PERF_EF_UPDATE);
+
+   /*
+* For programmable counters we need to check where we installed it.
+* To keep this function generic always test the more complicated
+* case (free running counters won't need the loop).
+*/
+   for (i = 0; i < pmu_dev->num_counters; i++)
+   if (cmpxchg(_dev->events[i], event, NULL) == event)
+   break;


I couldn't see why hwc->config wouldn't give us the index where we installed
the event in pmu_dev->events. What am I missing ?


Did you see the comment above? It is not yet needed but will be when I
add support for programmable counters.


Is it supported in this series ?


If it is still confusing I can
also remove that for now and add it back later when it is needed.


What is the hwc->idx for programmable counters ? is it going to be different
than hwc->config ? If so, can we use hwc->idx to keep the idx where we installed
the event ?

Suzuki

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Tue, Jul 25, 2017 at 04:39:18PM +0100, Suzuki K Poulose wrote:
> On 25/07/17 16:04, Jan Glauber wrote:
> >Add support for the PMU counters on Cavium SOC memory controllers.
> >
> >This patch also adds generic functions to allow supporting more
> >devices with PMU counters.
> >
> >Properties of the LMC PMU counters:
> >- not stoppable
> >- fixed purpose
> >- read-only
> >- one PCI device per memory controller
> >
> >Signed-off-by: Jan Glauber 
> >---
> > drivers/perf/Kconfig   |   8 +
> > drivers/perf/Makefile  |   1 +
> > drivers/perf/cavium_pmu.c  | 424 
> > +
> > include/linux/cpuhotplug.h |   1 +
> > 4 files changed, 434 insertions(+)
> > create mode 100644 drivers/perf/cavium_pmu.c
> >
> >diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> >index e5197ff..a46c3f0 100644
> >--- a/drivers/perf/Kconfig
> >+++ b/drivers/perf/Kconfig
> >@@ -43,4 +43,12 @@ config XGENE_PMU
> > help
> >   Say y if you want to use APM X-Gene SoC performance monitors.
> >
> >+config CAVIUM_PMU
> >+bool "Cavium SOC PMU"
> 
> Is there any specific reason why this can't be built as a module ?

Yes. I don't know how to load the module automatically. I can't make it
a pci driver as the EDAC driver "owns" the device (and having two
drivers for one device wont work as far as I know). I tried to hook
into the EDAC driver but the EDAC maintainer was not overly welcoming
that approach.

And while it would be possible to have it a s a module I think it is of
no use if it requires manualy loading. But maybe there is a simple
solution I'm missing here?

> 
> >+#define to_pmu_dev(x) container_of((x), struct cvm_pmu_dev, pmu)
> >+
> >+static int cvm_pmu_event_init(struct perf_event *event)
> >+{
> >+struct hw_perf_event *hwc = >hw;
> >+struct cvm_pmu_dev *pmu_dev;
> >+struct perf_event *sibling;
> >+
> >+if (event->attr.type != event->pmu->type)
> >+return -ENOENT;
> >+
> >+/* we do not support sampling */
> >+if (is_sampling_event(event))
> >+return -EINVAL;
> >+
> >+/* PMU counters do not support any these bits */
> >+if (event->attr.exclude_user||
> >+event->attr.exclude_kernel  ||
> >+event->attr.exclude_host||
> >+event->attr.exclude_guest   ||
> >+event->attr.exclude_hv  ||
> >+event->attr.exclude_idle)
> >+return -EINVAL;
> >+
> >+pmu_dev = to_pmu_dev(event->pmu);
> >+if (!pmu_dev->event_valid(event->attr.config))
> >+return -EINVAL;
> >+
> >+/*
> >+ * Forbid groups containing mixed PMUs, software events are acceptable.
> >+ */
> >+if (event->group_leader->pmu != event->pmu &&
> >+!is_software_event(event->group_leader))
> >+return -EINVAL;
> >+
> >+list_for_each_entry(sibling, >group_leader->sibling_list,
> >+group_entry)
> >+if (sibling->pmu != event->pmu &&
> >+!is_software_event(sibling))
> >+return -EINVAL;
> 
> Do we also need to check if the events in the same group can be scheduled
> at once ? i.e, there is enough resources to schedule the requested events from
> the group.
>

Not sure what you mean, do I need to check for programmable counters
that no more counters are programmed than available?

> >+
> >+hwc->config = event->attr.config;
> >+hwc->idx = -1;
> >+return 0;
> >+}
> >+
> ...
> 
> >+static int cvm_pmu_add(struct perf_event *event, int flags, u64 config_base,
> >+   u64 event_base)
> >+{
> >+struct cvm_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
> >+struct hw_perf_event *hwc = >hw;
> >+
> >+if (!cmpxchg(_dev->events[hwc->config], NULL, event))
> >+hwc->idx = hwc->config;
> >+
> >+if (hwc->idx == -1)
> >+return -EBUSY;
> >+
> >+hwc->config_base = config_base;
> >+hwc->event_base = event_base;
> >+hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
> >+
> >+if (flags & PERF_EF_START)
> >+pmu_dev->pmu.start(event, PERF_EF_RELOAD);
> >+
> >+return 0;
> >+}
> >+
> >+static void cvm_pmu_del(struct perf_event *event, int flags)
> >+{
> >+struct cvm_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
> >+struct hw_perf_event *hwc = >hw;
> >+int i;
> >+
> >+event->pmu->stop(event, PERF_EF_UPDATE);
> >+
> >+/*
> >+ * For programmable counters we need to check where we installed it.
> >+ * To keep this function generic always test the more complicated
> >+ * case (free running counters won't need the loop).
> >+ */
> >+for (i = 0; i < pmu_dev->num_counters; i++)
> >+if (cmpxchg(_dev->events[i], event, NULL) == event)
> >+break;
> 
> I couldn't see why hwc->config wouldn't give us the index where we installed
> the event in pmu_dev->events. What am I missing ?

Did you see the comment above? It is not yet needed but will 

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jan Glauber]

On Tue, Jul 25, 2017 at 04:39:18PM +0100, Suzuki K Poulose wrote:
> On 25/07/17 16:04, Jan Glauber wrote:
> >Add support for the PMU counters on Cavium SOC memory controllers.
> >
> >This patch also adds generic functions to allow supporting more
> >devices with PMU counters.
> >
> >Properties of the LMC PMU counters:
> >- not stoppable
> >- fixed purpose
> >- read-only
> >- one PCI device per memory controller
> >
> >Signed-off-by: Jan Glauber 
> >---
> > drivers/perf/Kconfig   |   8 +
> > drivers/perf/Makefile  |   1 +
> > drivers/perf/cavium_pmu.c  | 424 
> > +
> > include/linux/cpuhotplug.h |   1 +
> > 4 files changed, 434 insertions(+)
> > create mode 100644 drivers/perf/cavium_pmu.c
> >
> >diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> >index e5197ff..a46c3f0 100644
> >--- a/drivers/perf/Kconfig
> >+++ b/drivers/perf/Kconfig
> >@@ -43,4 +43,12 @@ config XGENE_PMU
> > help
> >   Say y if you want to use APM X-Gene SoC performance monitors.
> >
> >+config CAVIUM_PMU
> >+bool "Cavium SOC PMU"
> 
> Is there any specific reason why this can't be built as a module ?

Yes. I don't know how to load the module automatically. I can't make it
a pci driver as the EDAC driver "owns" the device (and having two
drivers for one device wont work as far as I know). I tried to hook
into the EDAC driver but the EDAC maintainer was not overly welcoming
that approach.

And while it would be possible to have it a s a module I think it is of
no use if it requires manualy loading. But maybe there is a simple
solution I'm missing here?

> 
> >+#define to_pmu_dev(x) container_of((x), struct cvm_pmu_dev, pmu)
> >+
> >+static int cvm_pmu_event_init(struct perf_event *event)
> >+{
> >+struct hw_perf_event *hwc = >hw;
> >+struct cvm_pmu_dev *pmu_dev;
> >+struct perf_event *sibling;
> >+
> >+if (event->attr.type != event->pmu->type)
> >+return -ENOENT;
> >+
> >+/* we do not support sampling */
> >+if (is_sampling_event(event))
> >+return -EINVAL;
> >+
> >+/* PMU counters do not support any these bits */
> >+if (event->attr.exclude_user||
> >+event->attr.exclude_kernel  ||
> >+event->attr.exclude_host||
> >+event->attr.exclude_guest   ||
> >+event->attr.exclude_hv  ||
> >+event->attr.exclude_idle)
> >+return -EINVAL;
> >+
> >+pmu_dev = to_pmu_dev(event->pmu);
> >+if (!pmu_dev->event_valid(event->attr.config))
> >+return -EINVAL;
> >+
> >+/*
> >+ * Forbid groups containing mixed PMUs, software events are acceptable.
> >+ */
> >+if (event->group_leader->pmu != event->pmu &&
> >+!is_software_event(event->group_leader))
> >+return -EINVAL;
> >+
> >+list_for_each_entry(sibling, >group_leader->sibling_list,
> >+group_entry)
> >+if (sibling->pmu != event->pmu &&
> >+!is_software_event(sibling))
> >+return -EINVAL;
> 
> Do we also need to check if the events in the same group can be scheduled
> at once ? i.e, there is enough resources to schedule the requested events from
> the group.
>

Not sure what you mean, do I need to check for programmable counters
that no more counters are programmed than available?

> >+
> >+hwc->config = event->attr.config;
> >+hwc->idx = -1;
> >+return 0;
> >+}
> >+
> ...
> 
> >+static int cvm_pmu_add(struct perf_event *event, int flags, u64 config_base,
> >+   u64 event_base)
> >+{
> >+struct cvm_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
> >+struct hw_perf_event *hwc = >hw;
> >+
> >+if (!cmpxchg(_dev->events[hwc->config], NULL, event))
> >+hwc->idx = hwc->config;
> >+
> >+if (hwc->idx == -1)
> >+return -EBUSY;
> >+
> >+hwc->config_base = config_base;
> >+hwc->event_base = event_base;
> >+hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
> >+
> >+if (flags & PERF_EF_START)
> >+pmu_dev->pmu.start(event, PERF_EF_RELOAD);
> >+
> >+return 0;
> >+}
> >+
> >+static void cvm_pmu_del(struct perf_event *event, int flags)
> >+{
> >+struct cvm_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
> >+struct hw_perf_event *hwc = >hw;
> >+int i;
> >+
> >+event->pmu->stop(event, PERF_EF_UPDATE);
> >+
> >+/*
> >+ * For programmable counters we need to check where we installed it.
> >+ * To keep this function generic always test the more complicated
> >+ * case (free running counters won't need the loop).
> >+ */
> >+for (i = 0; i < pmu_dev->num_counters; i++)
> >+if (cmpxchg(_dev->events[i], event, NULL) == event)
> >+break;
> 
> I couldn't see why hwc->config wouldn't give us the index where we installed
> the event in pmu_dev->events. What am I missing ?

Did you see the comment above? It is not yet needed but will be when I
add 

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jonathan Cameron]

On Tue, 25 Jul 2017 17:04:20 +0200
Jan Glauber  wrote:

> Add support for the PMU counters on Cavium SOC memory controllers.
> 
> This patch also adds generic functions to allow supporting more
> devices with PMU counters.
> 
> Properties of the LMC PMU counters:
> - not stoppable
> - fixed purpose
> - read-only
> - one PCI device per memory controller
> 
> Signed-off-by: Jan Glauber 
One trivial point inline which, whilst it obviously makes to actual
difference, makes review a tiny bit easier.

Otherwise looks good to me, but I'm somewhat new to this area
so who knows what I've missed ;)

> ---
>  drivers/perf/Kconfig   |   8 +
>  drivers/perf/Makefile  |   1 +
>  drivers/perf/cavium_pmu.c  | 424 
> +
>  include/linux/cpuhotplug.h |   1 +
>  4 files changed, 434 insertions(+)
>  create mode 100644 drivers/perf/cavium_pmu.c

> +static int cvm_pmu_lmc_probe(struct pci_dev *pdev)
> +{
> + struct cvm_pmu_dev *next, *lmc;
> + int nr = 0, ret = -ENOMEM;
> +
> + lmc = kzalloc(sizeof(*lmc), GFP_KERNEL);
> + if (!lmc)
> + return -ENOMEM;
> +
> + lmc->map = ioremap(pci_resource_start(pdev, 0),
> +pci_resource_len(pdev, 0));
> + if (!lmc->map)
> + goto fail_ioremap;
> +
> + list_for_each_entry(next, _pmu_lmcs, entry)
> + nr++;
> + lmc->pmu_name = kasprintf(GFP_KERNEL, "lmc%d", nr);
> + if (!lmc->pmu_name)
> + goto fail_kasprintf;
> +
> + lmc->pdev = pdev;
> + lmc->num_counters = ARRAY_SIZE(lmc_events);
> + lmc->pmu = (struct pmu) {
> + .task_ctx_nr= perf_invalid_context,
> + .event_init = cvm_pmu_event_init,
> + .add= cvm_pmu_lmc_add,
> + .del= cvm_pmu_del,
> + .start  = cvm_pmu_start,
> + .stop   = cvm_pmu_stop,
> + .read   = cvm_pmu_read,
> + .attr_groups= cvm_pmu_lmc_attr_groups,
> + };
> +
> + cpuhp_state_add_instance_nocalls(CPUHP_AP_PERF_ARM_CVM_ONLINE,
> +  >cpuhp_node);
> +
> + /*
> +  * perf PMU is CPU dependent so pick a random CPU and migrate away
> +  * if it goes offline.
> +  */
> + cpumask_set_cpu(smp_processor_id(), >active_mask);
> +
> + list_add(>entry, _pmu_lmcs);
> + lmc->event_valid = cvm_pmu_lmc_event_valid;
> +
> + ret = perf_pmu_register(>pmu, lmc->pmu_name, -1);
> + if (ret)
> + goto fail_pmu;
> +
> + dev_info(>dev, "Enabled %s PMU with %d counters\n",
> +  lmc->pmu_name, lmc->num_counters);
> + return 0;
> +
> +fail_pmu:
> + kfree(lmc->pmu_name);
> + cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_CVM_ONLINE,
> + >cpuhp_node);
Expected order to unwind the above would be the reverse of this.

> +fail_kasprintf:
> + iounmap(lmc->map);
> +fail_ioremap:
> + kfree(lmc);
> + return ret;
> +}
> +
> +static int __init cvm_pmu_init(void)
> +{
> + unsigned long implementor = read_cpuid_implementor();
> + unsigned int vendor_id = PCI_VENDOR_ID_CAVIUM;
> + struct pci_dev *pdev = NULL;
> + int rc;
> +
> + if (implementor != ARM_CPU_IMP_CAVIUM)
> + return -ENODEV;
> +
> + INIT_LIST_HEAD(_pmu_lmcs);
> +
> + rc = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_CVM_ONLINE,
> +  "perf/arm/cvm:online", NULL,
> +  cvm_pmu_offline_cpu);
> +
> + /* detect LMC devices */
> + while ((pdev = pci_get_device(vendor_id, 0xa022, pdev))) {
> + if (!pdev)
> + break;
> + rc = cvm_pmu_lmc_probe(pdev);
> + if (rc)
> + return rc;
> + }
> + return 0;
> +}
> +late_initcall(cvm_pmu_init); /* should come after PCI init */
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index b56573b..78ac3d2 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -141,6 +141,7 @@ enum cpuhp_state {
>   CPUHP_AP_PERF_ARM_QCOM_L3_ONLINE,
>   CPUHP_AP_WORKQUEUE_ONLINE,
>   CPUHP_AP_RCUTREE_ONLINE,
> + CPUHP_AP_PERF_ARM_CVM_ONLINE,
>   CPUHP_AP_ONLINE_DYN,
>   CPUHP_AP_ONLINE_DYN_END = CPUHP_AP_ONLINE_DYN + 30,
>   CPUHP_AP_X86_HPET_ONLINE,

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Jonathan Cameron]

On Tue, 25 Jul 2017 17:04:20 +0200
Jan Glauber  wrote:

> Add support for the PMU counters on Cavium SOC memory controllers.
> 
> This patch also adds generic functions to allow supporting more
> devices with PMU counters.
> 
> Properties of the LMC PMU counters:
> - not stoppable
> - fixed purpose
> - read-only
> - one PCI device per memory controller
> 
> Signed-off-by: Jan Glauber 
One trivial point inline which, whilst it obviously makes to actual
difference, makes review a tiny bit easier.

Otherwise looks good to me, but I'm somewhat new to this area
so who knows what I've missed ;)

> ---
>  drivers/perf/Kconfig   |   8 +
>  drivers/perf/Makefile  |   1 +
>  drivers/perf/cavium_pmu.c  | 424 
> +
>  include/linux/cpuhotplug.h |   1 +
>  4 files changed, 434 insertions(+)
>  create mode 100644 drivers/perf/cavium_pmu.c

> +static int cvm_pmu_lmc_probe(struct pci_dev *pdev)
> +{
> + struct cvm_pmu_dev *next, *lmc;
> + int nr = 0, ret = -ENOMEM;
> +
> + lmc = kzalloc(sizeof(*lmc), GFP_KERNEL);
> + if (!lmc)
> + return -ENOMEM;
> +
> + lmc->map = ioremap(pci_resource_start(pdev, 0),
> +pci_resource_len(pdev, 0));
> + if (!lmc->map)
> + goto fail_ioremap;
> +
> + list_for_each_entry(next, _pmu_lmcs, entry)
> + nr++;
> + lmc->pmu_name = kasprintf(GFP_KERNEL, "lmc%d", nr);
> + if (!lmc->pmu_name)
> + goto fail_kasprintf;
> +
> + lmc->pdev = pdev;
> + lmc->num_counters = ARRAY_SIZE(lmc_events);
> + lmc->pmu = (struct pmu) {
> + .task_ctx_nr= perf_invalid_context,
> + .event_init = cvm_pmu_event_init,
> + .add= cvm_pmu_lmc_add,
> + .del= cvm_pmu_del,
> + .start  = cvm_pmu_start,
> + .stop   = cvm_pmu_stop,
> + .read   = cvm_pmu_read,
> + .attr_groups= cvm_pmu_lmc_attr_groups,
> + };
> +
> + cpuhp_state_add_instance_nocalls(CPUHP_AP_PERF_ARM_CVM_ONLINE,
> +  >cpuhp_node);
> +
> + /*
> +  * perf PMU is CPU dependent so pick a random CPU and migrate away
> +  * if it goes offline.
> +  */
> + cpumask_set_cpu(smp_processor_id(), >active_mask);
> +
> + list_add(>entry, _pmu_lmcs);
> + lmc->event_valid = cvm_pmu_lmc_event_valid;
> +
> + ret = perf_pmu_register(>pmu, lmc->pmu_name, -1);
> + if (ret)
> + goto fail_pmu;
> +
> + dev_info(>dev, "Enabled %s PMU with %d counters\n",
> +  lmc->pmu_name, lmc->num_counters);
> + return 0;
> +
> +fail_pmu:
> + kfree(lmc->pmu_name);
> + cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_CVM_ONLINE,
> + >cpuhp_node);
Expected order to unwind the above would be the reverse of this.

> +fail_kasprintf:
> + iounmap(lmc->map);
> +fail_ioremap:
> + kfree(lmc);
> + return ret;
> +}
> +
> +static int __init cvm_pmu_init(void)
> +{
> + unsigned long implementor = read_cpuid_implementor();
> + unsigned int vendor_id = PCI_VENDOR_ID_CAVIUM;
> + struct pci_dev *pdev = NULL;
> + int rc;
> +
> + if (implementor != ARM_CPU_IMP_CAVIUM)
> + return -ENODEV;
> +
> + INIT_LIST_HEAD(_pmu_lmcs);
> +
> + rc = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_CVM_ONLINE,
> +  "perf/arm/cvm:online", NULL,
> +  cvm_pmu_offline_cpu);
> +
> + /* detect LMC devices */
> + while ((pdev = pci_get_device(vendor_id, 0xa022, pdev))) {
> + if (!pdev)
> + break;
> + rc = cvm_pmu_lmc_probe(pdev);
> + if (rc)
> + return rc;
> + }
> + return 0;
> +}
> +late_initcall(cvm_pmu_init); /* should come after PCI init */
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index b56573b..78ac3d2 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -141,6 +141,7 @@ enum cpuhp_state {
>   CPUHP_AP_PERF_ARM_QCOM_L3_ONLINE,
>   CPUHP_AP_WORKQUEUE_ONLINE,
>   CPUHP_AP_RCUTREE_ONLINE,
> + CPUHP_AP_PERF_ARM_CVM_ONLINE,
>   CPUHP_AP_ONLINE_DYN,
>   CPUHP_AP_ONLINE_DYN_END = CPUHP_AP_ONLINE_DYN + 30,
>   CPUHP_AP_X86_HPET_ONLINE,

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Suzuki K Poulose]

On 25/07/17 16:04, Jan Glauber wrote:

Add support for the PMU counters on Cavium SOC memory controllers.

This patch also adds generic functions to allow supporting more
devices with PMU counters.

Properties of the LMC PMU counters:
- not stoppable
- fixed purpose
- read-only
- one PCI device per memory controller

Signed-off-by: Jan Glauber 
---
 drivers/perf/Kconfig   |   8 +
 drivers/perf/Makefile  |   1 +
 drivers/perf/cavium_pmu.c  | 424 +
 include/linux/cpuhotplug.h |   1 +
 4 files changed, 434 insertions(+)
 create mode 100644 drivers/perf/cavium_pmu.c

diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index e5197ff..a46c3f0 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -43,4 +43,12 @@ config XGENE_PMU
 help
   Say y if you want to use APM X-Gene SoC performance monitors.

+config CAVIUM_PMU
+   bool "Cavium SOC PMU"


Is there any specific reason why this can't be built as a module ?



+#define to_pmu_dev(x) container_of((x), struct cvm_pmu_dev, pmu)
+
+static int cvm_pmu_event_init(struct perf_event *event)
+{
+   struct hw_perf_event *hwc = >hw;
+   struct cvm_pmu_dev *pmu_dev;
+   struct perf_event *sibling;
+
+   if (event->attr.type != event->pmu->type)
+   return -ENOENT;
+
+   /* we do not support sampling */
+   if (is_sampling_event(event))
+   return -EINVAL;
+
+   /* PMU counters do not support any these bits */
+   if (event->attr.exclude_user ||
+   event->attr.exclude_kernel   ||
+   event->attr.exclude_host ||
+   event->attr.exclude_guest||
+   event->attr.exclude_hv   ||
+   event->attr.exclude_idle)
+   return -EINVAL;
+
+   pmu_dev = to_pmu_dev(event->pmu);
+   if (!pmu_dev->event_valid(event->attr.config))
+   return -EINVAL;
+
+   /*
+* Forbid groups containing mixed PMUs, software events are acceptable.
+*/
+   if (event->group_leader->pmu != event->pmu &&
+   !is_software_event(event->group_leader))
+   return -EINVAL;
+
+   list_for_each_entry(sibling, >group_leader->sibling_list,
+   group_entry)
+   if (sibling->pmu != event->pmu &&
+   !is_software_event(sibling))
+   return -EINVAL;


Do we also need to check if the events in the same group can be scheduled
at once ? i.e, there is enough resources to schedule the requested events from
the group.


+
+   hwc->config = event->attr.config;
+   hwc->idx = -1;
+   return 0;
+}
+

...


+static int cvm_pmu_add(struct perf_event *event, int flags, u64 config_base,
+  u64 event_base)
+{
+   struct cvm_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
+   struct hw_perf_event *hwc = >hw;
+
+   if (!cmpxchg(_dev->events[hwc->config], NULL, event))
+   hwc->idx = hwc->config;
+
+   if (hwc->idx == -1)
+   return -EBUSY;
+
+   hwc->config_base = config_base;
+   hwc->event_base = event_base;
+   hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
+
+   if (flags & PERF_EF_START)
+   pmu_dev->pmu.start(event, PERF_EF_RELOAD);
+
+   return 0;
+}
+
+static void cvm_pmu_del(struct perf_event *event, int flags)
+{
+   struct cvm_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
+   struct hw_perf_event *hwc = >hw;
+   int i;
+
+   event->pmu->stop(event, PERF_EF_UPDATE);
+
+   /*
+* For programmable counters we need to check where we installed it.
+* To keep this function generic always test the more complicated
+* case (free running counters won't need the loop).
+*/
+   for (i = 0; i < pmu_dev->num_counters; i++)
+   if (cmpxchg(_dev->events[i], event, NULL) == event)
+   break;


I couldn't see why hwc->config wouldn't give us the index where we installed
the event in pmu_dev->events. What am I missing ?


+static int __init cvm_pmu_init(void)
+{
+   unsigned long implementor = read_cpuid_implementor();
+   unsigned int vendor_id = PCI_VENDOR_ID_CAVIUM;
+   struct pci_dev *pdev = NULL;
+   int rc;
+
+   if (implementor != ARM_CPU_IMP_CAVIUM)
+   return -ENODEV;


As I mentioned in the beginning, it would be better to modularize it right
from the start, when we can, than coming back to this at a later point in time.

Btw, perf_event_update_userpage() is being exported for use from module.
See [0].

[0] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-July/520682.html

Cheers

Suzuki

Re: [PATCH v8 1/3] perf: cavium: Support memory controller PMU counters

[Suzuki K Poulose]

On 25/07/17 16:04, Jan Glauber wrote:

Add support for the PMU counters on Cavium SOC memory controllers.

This patch also adds generic functions to allow supporting more
devices with PMU counters.

Properties of the LMC PMU counters:
- not stoppable
- fixed purpose
- read-only
- one PCI device per memory controller

Signed-off-by: Jan Glauber 
---
 drivers/perf/Kconfig   |   8 +
 drivers/perf/Makefile  |   1 +
 drivers/perf/cavium_pmu.c  | 424 +
 include/linux/cpuhotplug.h |   1 +
 4 files changed, 434 insertions(+)
 create mode 100644 drivers/perf/cavium_pmu.c

diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index e5197ff..a46c3f0 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -43,4 +43,12 @@ config XGENE_PMU
 help
   Say y if you want to use APM X-Gene SoC performance monitors.

+config CAVIUM_PMU
+   bool "Cavium SOC PMU"


Is there any specific reason why this can't be built as a module ?



+#define to_pmu_dev(x) container_of((x), struct cvm_pmu_dev, pmu)
+
+static int cvm_pmu_event_init(struct perf_event *event)
+{
+   struct hw_perf_event *hwc = >hw;
+   struct cvm_pmu_dev *pmu_dev;
+   struct perf_event *sibling;
+
+   if (event->attr.type != event->pmu->type)
+   return -ENOENT;
+
+   /* we do not support sampling */
+   if (is_sampling_event(event))
+   return -EINVAL;
+
+   /* PMU counters do not support any these bits */
+   if (event->attr.exclude_user ||
+   event->attr.exclude_kernel   ||
+   event->attr.exclude_host ||
+   event->attr.exclude_guest||
+   event->attr.exclude_hv   ||
+   event->attr.exclude_idle)
+   return -EINVAL;
+
+   pmu_dev = to_pmu_dev(event->pmu);
+   if (!pmu_dev->event_valid(event->attr.config))
+   return -EINVAL;
+
+   /*
+* Forbid groups containing mixed PMUs, software events are acceptable.
+*/
+   if (event->group_leader->pmu != event->pmu &&
+   !is_software_event(event->group_leader))
+   return -EINVAL;
+
+   list_for_each_entry(sibling, >group_leader->sibling_list,
+   group_entry)
+   if (sibling->pmu != event->pmu &&
+   !is_software_event(sibling))
+   return -EINVAL;


Do we also need to check if the events in the same group can be scheduled
at once ? i.e, there is enough resources to schedule the requested events from
the group.


+
+   hwc->config = event->attr.config;
+   hwc->idx = -1;
+   return 0;
+}
+

...


+static int cvm_pmu_add(struct perf_event *event, int flags, u64 config_base,
+  u64 event_base)
+{
+   struct cvm_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
+   struct hw_perf_event *hwc = >hw;
+
+   if (!cmpxchg(_dev->events[hwc->config], NULL, event))
+   hwc->idx = hwc->config;
+
+   if (hwc->idx == -1)
+   return -EBUSY;
+
+   hwc->config_base = config_base;
+   hwc->event_base = event_base;
+   hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
+
+   if (flags & PERF_EF_START)
+   pmu_dev->pmu.start(event, PERF_EF_RELOAD);
+
+   return 0;
+}
+
+static void cvm_pmu_del(struct perf_event *event, int flags)
+{
+   struct cvm_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
+   struct hw_perf_event *hwc = >hw;
+   int i;
+
+   event->pmu->stop(event, PERF_EF_UPDATE);
+
+   /*
+* For programmable counters we need to check where we installed it.
+* To keep this function generic always test the more complicated
+* case (free running counters won't need the loop).
+*/
+   for (i = 0; i < pmu_dev->num_counters; i++)
+   if (cmpxchg(_dev->events[i], event, NULL) == event)
+   break;


I couldn't see why hwc->config wouldn't give us the index where we installed
the event in pmu_dev->events. What am I missing ?


+static int __init cvm_pmu_init(void)
+{
+   unsigned long implementor = read_cpuid_implementor();
+   unsigned int vendor_id = PCI_VENDOR_ID_CAVIUM;
+   struct pci_dev *pdev = NULL;
+   int rc;
+
+   if (implementor != ARM_CPU_IMP_CAVIUM)
+   return -ENODEV;


As I mentioned in the beginning, it would be better to modularize it right
from the start, when we can, than coming back to this at a later point in time.

Btw, perf_event_update_userpage() is being exported for use from module.
See [0].

[0] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-July/520682.html

Cheers

Suzuki