Re: [PATCH 3/3] perf/x86/intel/uncore: Add Skylake server uncore support

[Nilay Vaish]

On 12 August 2016 at 16:30, Kan Liang <kan.liang@xxxxxxxxx> wrote:
> From: Kan Liang <kan.liang@xxxxxxxxx>
>
> This patch implements the uncore monitoring driver for Skylake server.
> The uncore subsystem in Skylake server is similar to previous
> server. There are some differences in config register encoding and pci
> device IDs. Besides, Skylake introduces many new boxes to reflect the
> MESH architecture changes.
>
> The control registers for IIO and UPI have been extended to 64 bit. This
> patch also introduces event_mask_ext to handle the high 32 bit mask.
>
> The CHA box number could vary for different machines. This patch gets
> the CHA box number by counting the CHA register space during
> initialization at runtime.
>

The code in this patch contains a lot of constants related to the
Skylake server.  Is it possible to mention in the description the
source from where these values have been picked?  May be Intel
provides some manual that has these values.  It would be good if we
can mention the name of the manual (or a URL) and its version.

> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxx>
> ---
>  arch/x86/events/intel/uncore.c       |   9 +-
>  arch/x86/events/intel/uncore.h       |   3 +
>  arch/x86/events/intel/uncore_snbep.c | 589 +++++++++++++++++++++++++++++++++++
>  3 files changed, 600 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index 3719af5..55a081e 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> +
> +static int skx_count_chabox(void)
> +{
> +       struct pci_dev *chabox_dev = NULL;
> +       int bus, count = 0;
> +
> +       while (1) {
> +               chabox_dev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x208d, chabox_dev);
> +               if (!chabox_dev)
> +                       break;
> +               if (count == 0)
> +                       bus = chabox_dev->bus->number;
> +               if (bus != chabox_dev->bus->number)
> +                       break;
> +               count++;
> +       }
> +
> +       pci_dev_put(chabox_dev);
> +       return count;
> +}

Kan,  do we not need to call pci_dev_put() each time we call pci_get_device()?


--
Nilay