Date: Thu, 15 Jun 2017 12:36:02 +0200
From: Jan Glauber <jan.glauber@caviumnetworks.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <will.deacon@arm.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Borislav Petkov <bp@alien8.de>
Subject: Re: [PATCH v5 3/3] perf: cavium: Support transmit-link PMU counters
Message-ID: <20170615103602.GB13427@hc>
References: <20170517083122.5050-1-jglauber@cavium.com>
 <20170517083122.5050-4-jglauber@cavium.com>
 <20170602163937.GN28299@leverpostej>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170602163937.GN28299@leverpostej>
User-Agent: Mutt/1.5.21 (2010-09-15)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Jun 2017 10:36:12.7554 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR07MB2586
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3739
Lines: 122

On Fri, Jun 02, 2017 at 05:39:38PM +0100, Mark Rutland wrote:
> On Wed, May 17, 2017 at 10:31:22AM +0200, Jan Glauber wrote:
> > Add support for the transmit-link (TLK) PMU counters found
> > on Caviums SOCs with an interconnect.
> > 
> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> > ---
> >  drivers/edac/thunderx_edac.c    |   7 ++
> >  drivers/perf/cavium_pmu.c       | 223 +++++++++++++++++++++++++++++++++++++++-
> >  include/linux/perf/cavium_pmu.h |   1 +
> >  3 files changed, 230 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/edac/thunderx_edac.c b/drivers/edac/thunderx_edac.c
> > index 8de4faf..d3da6d3 100644
> > --- a/drivers/edac/thunderx_edac.c
> > +++ b/drivers/edac/thunderx_edac.c
> > @@ -1496,6 +1496,10 @@ static int thunderx_ocx_probe(struct pci_dev *pdev,
> >  
> >  	writeq(OCX_COM_INT_ENA_ALL, ocx->regs + OCX_COM_INT_ENA_W1S);
> >  
> > +	if (IS_ENABLED(CONFIG_CAVIUM_PMU))
> > +		ocx->pmu_data = cvm_pmu_probe(pdev, ocx->regs,
> > +					      CVM_PMU_TLK);
> > +
> >  	return 0;
> >  err_free:
> >  	edac_device_free_ctl_info(edac_dev);
> > @@ -1509,6 +1513,9 @@ static void thunderx_ocx_remove(struct pci_dev *pdev)
> >  	struct thunderx_ocx *ocx = edac_dev->pvt_info;
> >  	int i;
> >  
> > +	if (IS_ENABLED(CONFIG_CAVIUM_PMU))
> > +		cvm_pmu_remove(pdev, ocx->pmu_data, CVM_PMU_TLK);
> > +
> 
> As with the prior patch, I don't think we can handle the PMU like this.
> 
> 
> [...]
> 
> > +static int cvm_pmu_tlk_add(struct perf_event *event, int flags)
> > +{
> > +	struct hw_perf_event *hwc = &event->hw;
> > +
> > +	return cvm_pmu_add(event, flags, TLK_STAT_CTL_OFFSET,
> > +			   TLK_STAT_OFFSET + hwc->config * 8);
> > +}
> 
> > +static void *cvm_pmu_tlk_probe_unit(struct pci_dev *pdev, void __iomem *regs,
> > +				    int nr)
> > +{
> > +	struct cvm_pmu_dev *tlk;
> > +	int ret = -ENOMEM;
> > +	char name[12];
> > +
> > +	tlk = kzalloc(sizeof(*tlk), GFP_KERNEL);
> > +	if (!tlk)
> > +		goto fail_nomem;
> > +
> > +	memset(name, 0, 12);
> > +	snprintf(name, 12, "ocx_tlk%d", nr);
> 
> Please kasprintf() the name and pass it to perf_pmu_register().
> 
> AFAICT, pmu::name is a char *, and perf_pmu_register() doesn't make a copy, so
> this isn't safe.

OK, will use kasprintf and embedd the char name * into cvm_pmu_dev so I
can free it on remove.

> > +
> > +	list_add(&tlk->entry, &cvm_pmu_tlks);
> 
> As withthe prio patch, please add the element to the list after
> handling the failure cases.

OK.

> > +
> > +	tlk->pdev = pdev;
> > +	tlk->map = regs + TLK_START_ADDR + nr * TLK_UNIT_OFFSET;
> > +	tlk->num_counters = ARRAY_SIZE(cvm_pmu_tlk_events_attr) - 1;
> > +	tlk->pmu = (struct pmu) {
> > +		.name		= name,
> > +		.task_ctx_nr    = perf_invalid_context,
> > +		.pmu_enable	= cvm_pmu_tlk_enable_pmu,
> > +		.pmu_disable	= cvm_pmu_tlk_disable_pmu,
> > +		.event_init	= cvm_pmu_event_init,
> > +		.add		= cvm_pmu_tlk_add,
> > +		.del		= cvm_pmu_del,
> > +		.start		= cvm_pmu_start,
> > +		.stop		= cvm_pmu_stop,
> > +		.read		= cvm_pmu_read,
> > +		.attr_groups	= cvm_pmu_tlk_attr_groups,
> > +	};
> > +
> > +	cpuhp_state_add_instance_nocalls(CPUHP_AP_PERF_ARM_CVM_ONLINE,
> > +					 &tlk->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(), &tlk->active_mask);
> > +
> > +	ret = perf_pmu_register(&tlk->pmu, tlk->pmu.name, -1);
> > +	if (ret)
> > +		goto fail_hp;
> > +
> > +	tlk->event_valid = cvm_pmu_tlk_event_valid;
> > +	return tlk;
> > +
> > +fail_hp:
> > +	cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_CVM_ONLINE,
> > +				    &tlk->cpuhp_node);
> > +	kfree(tlk);
> > +fail_nomem:
> > +	return ERR_PTR(ret);
> > +}
> 
> Thanks,
> Mark.