Re: [PATCH 3/4] hwtracing: hisi_ptt: Export available filters through sysfs

[Jonathan Cameron]

On Wed, 15 Mar 2023 17:43:15 +0800
Yicong Yang <yangyicong@xxxxxxxxxx> wrote:

> From: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> 
> The PTT can only filter the traced TLP headers by the Root Ports or the
> Requester ID of the Endpoint, which are located on the same core of the
> PTT device. The filter value used is derived from the BDF number of the
> supported Root Port or the Endpoint. It's not friendly enough for the
> users since it requires the user to be familiar enough with the platform
> and calculate the filter value manually.
> 
> This patch export the available filters through sysfs. Each available
> filters is presented as an individual file with the name of the BDF
> number of the related PCIe device. The files are created under
> $(PTT PMU dir)/available_root_port_filters and
> $(PTT PMU dir)/available_requester_filters respectively. The filter
> value can be known by reading the related file.
> 
> Then the users can easily know the available filters for trace and get
> the filter values without calculating.
> 
> Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>

Trivial comments only inline.

With those answered / tidied up.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>


> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
> index 010cdbc3c172..a5cd87edb813 100644
> --- a/drivers/hwtracing/ptt/hisi_ptt.c
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c


>
> +
> +static int hisi_ptt_init_filter_attributes(struct hisi_ptt *hisi_ptt)
> +{
> +	struct hisi_ptt_filter_desc *filter;
> +	int ret;
> +
> +	mutex_lock(&hisi_ptt->filter_lock);
> +
> +	list_for_each_entry(filter, &hisi_ptt->port_filters, list) {
> +		ret = hisi_ptt_create_filter_attr(hisi_ptt, filter);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	list_for_each_entry(filter, &hisi_ptt->req_filters, list) {
> +		ret = hisi_ptt_create_filter_attr(hisi_ptt, filter);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	ret = devm_add_action_or_reset(&hisi_ptt->pdev->dev,
> +				       hisi_ptt_remove_all_filter_attributes,
> +				       hisi_ptt);
> +	if (ret)
> +		goto err;
> +
> +	hisi_ptt->sysfs_inited = true;

err:

> +	mutex_unlock(&hisi_ptt->filter_lock);

	return ret;

No need for separate exit block when nothing to do but unlock.

> +	return 0;
> +err:
> +	mutex_unlock(&hisi_ptt->filter_lock);
> +	return ret;
> +}
> +
>  static void hisi_ptt_update_filters(struct work_struct *work)
>  {
>  	struct delayed_work *delayed_work = to_delayed_work(work);
> @@ -384,8 +517,28 @@ static void hisi_ptt_update_filters(struct work_struct *work)
>  				continue;
>  			}
>  
> +			filter->name = kstrdup(pci_name(info.pdev), GFP_KERNEL);
> +			if (!filter->name) {
> +				pci_err(hisi_ptt->pdev, "failed to add filter %s\n",
> +					pci_name(info.pdev));
> +				kfree(filter);
> +				continue;
> +			}
> +
>  			filter->devid = devid;
>  			filter->is_port = is_port;
> +
> +			/*
> +			 * If filters' sysfs entries hasn't been initialized, then
> +			 * we're still at probe stage and leave it to handled by
> +			 * others.
> +			 */
> +			if (hisi_ptt->sysfs_inited &&

Can we move this sysfs_inited check earlier? Seems silly to leave a simple check
like that so late.

> +			    hisi_ptt_create_filter_attr(hisi_ptt, filter)) {
> +				kfree(filter);
> +				continue;
> +			}
> +
>  			list_add_tail(&filter->list, target_list);
>  
>  			if (is_port)
> @@ -394,6 +547,11 @@ static void hisi_ptt_update_filters(struct work_struct *work)
>  			list_for_each_entry(filter, target_list, list)
>  				if (filter->devid == devid) {
>  					list_del(&filter->list);
> +
> +					if (hisi_ptt->sysfs_inited)
> +						hisi_ptt_remove_filter_attr(hisi_ptt, filter);
> +
> +					kfree(filter->name);
>  					kfree(filter);
>  					break;
>  				}
> @@ -486,10 +644,12 @@ static int hisi_ptt_init_filters(struct pci_dev *pdev, void *data)
>  	 * through the log. Other functions of PTT device are still available.
>  	 */
>  	filter = kzalloc(sizeof(*filter), GFP_KERNEL);
> -	if (!filter) {
> -		pci_err(hisi_ptt->pdev, "failed to add filter %s\n", pci_name(pdev));
> -		return -ENOMEM;
> -	}
> +	if (!filter)
> +		goto err_mem;
> +
> +	filter->name = kstrdup(pci_name(pdev), GFP_KERNEL);
> +	if (!filter->name)
> +		goto err_name;
>  
>  	filter->devid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>  
> @@ -504,6 +664,11 @@ static int hisi_ptt_init_filters(struct pci_dev *pdev, void *data)
>  	}
>  
>  	return 0;
> +err_name:
> +	kfree(filter);
> +err_mem:
> +	pci_err(hisi_ptt->pdev, "failed to add filter %s\n", pci_name(pdev));

I'd rather see a message for each of the error paths so we have some information on why.
Original message wasn't great for this obviously and perhaps given they are both allocation
errors it's not worth splitting them up.

> +	return -ENOMEM;
>  }