Re: [PATCH v5 3/8] hisi_ptt: Register PMU device for PTT trace

[Yicong Yang <yangyicong@huawei.com>]

On 2022/3/8 18:21, Jonathan Cameron wrote:
> On Tue, 8 Mar 2022 16:49:25 +0800
> Yicong Yang <yangyicong@hisilicon.com> wrote:
> 
>> Register PMU device of PTT trace, then users can use trace through perf
>> command. The driver makes use of perf AUX trace and support following
>> events to configure the trace:
>>
>> - filter: select Root port or Endpoint to trace
>> - type: select the type of traced TLP headers
>> - direction: select the direction of traced TLP headers
>> - format: select the data format of the traced TLP headers
>>
>> This patch adds the PMU driver part of PTT trace. The perf command support
>> of PTT trace is added in the following patch.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> 
> It seems to me that you ended up doing both suggestions for
> how to clean up the remove order when it was meant to be
> a question of picking one or the other.
> 
> Otherwise this looks good to me - so with that tidied up
> 

Hi Jonathan,

Thanks for the comments. I'd like to illustrate the reason why I decide to
manually unregister the PMU device.

The DMA buffers are devm allocated when necessary. They're only allocated
when user is going to use the PTT in the first time after the driver's probe,
so when driver removal the buffers are released prior to the PMU device's
unregistration. I think there's a race condition.

IIUC, The PMU device(as the user interface) should be unregistered first then
we're safe to free the DMA buffers. But unregister the PMU device by devm
cannot keep that order.

Thanks,
Yicong

> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
>> ---
> 
>> +
>> +static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt)
>> +{
>> +	u16 core_id, sicl_id;
>> +	char *pmu_name;
>> +	u32 reg;
>> +
>> +	hisi_ptt->hisi_ptt_pmu = (struct pmu) {
>> +		.module		= THIS_MODULE,
>> +		.capabilities	= PERF_PMU_CAP_EXCLUSIVE | PERF_PMU_CAP_ITRACE,
>> +		.task_ctx_nr	= perf_sw_context,
>> +		.attr_groups	= hisi_ptt_pmu_groups,
>> +		.event_init	= hisi_ptt_pmu_event_init,
>> +		.setup_aux	= hisi_ptt_pmu_setup_aux,
>> +		.free_aux	= hisi_ptt_pmu_free_aux,
>> +		.start		= hisi_ptt_pmu_start,
>> +		.stop		= hisi_ptt_pmu_stop,
>> +		.add		= hisi_ptt_pmu_add,
>> +		.del		= hisi_ptt_pmu_del,
>> +	};
>> +
>> +	reg = readl(hisi_ptt->iobase + HISI_PTT_LOCATION);
>> +	core_id = FIELD_GET(HISI_PTT_CORE_ID, reg);
>> +	sicl_id = FIELD_GET(HISI_PTT_SICL_ID, reg);
>> +
>> +	pmu_name = devm_kasprintf(&hisi_ptt->pdev->dev, GFP_KERNEL, "hisi_ptt%u_%u",
>> +				  sicl_id, core_id);
>> +	if (!pmu_name)
>> +		return -ENOMEM;
>> +
>> +	return perf_pmu_register(&hisi_ptt->hisi_ptt_pmu, pmu_name, -1);
> 
> As below, you can put back the devm cleanup that you had in v4 now you
> have modified how the filter cleanup is done to also be devm managed.
> 
>> +}
>> +
>>  /*
>>   * The DMA of PTT trace can only use direct mapping, due to some
>>   * hardware restriction. Check whether there is an IOMMU or the
>> @@ -303,15 +825,32 @@ static int hisi_ptt_probe(struct pci_dev *pdev,
>>  
>>  	pci_set_master(pdev);
>>  
>> +	ret = hisi_ptt_register_irq(hisi_ptt);
>> +	if (ret)
>> +		return ret;
>> +
>>  	ret = hisi_ptt_init_ctrls(hisi_ptt);
>>  	if (ret) {
>>  		pci_err(pdev, "failed to init controls, ret = %d.\n", ret);
>>  		return ret;
>>  	}
>>  
>> +	ret = hisi_ptt_register_pmu(hisi_ptt);
>> +	if (ret) {
>> +		pci_err(pdev, "failed to register pmu device, ret = %d", ret);
>> +		return ret;
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> +void hisi_ptt_remove(struct pci_dev *pdev)
>> +{
>> +	struct hisi_ptt *hisi_ptt = pci_get_drvdata(pdev);
>> +
>> +	perf_pmu_unregister(&hisi_ptt->hisi_ptt_pmu);
> 
> Now you have the filter cleanup occurring using a devm_add_action_or_reset()
> there is no need to have a manual cleanup of this - you can
> use the approach of a devm_add_action_or_reset like you had in v4.
> 
> As it is the last call in the probe() order it will be the first one
> called in the device managed cleanup.
> 
>> +}
>> +
> 
> 
> .
>