On 2022/3/12 1:55, John Garry wrote:
>> +
>> +static int hisi_ptt_alloc_trace_buf(struct hisi_ptt *hisi_ptt)
>
> no caller
>
>> +{
>> + struct hisi_ptt_trace_ctrl *ctrl = &hisi_ptt->trace_ctrl;
>> + struct device *dev = &hisi_ptt->pdev->dev;
>> + int i;
>> +
>> + hisi_ptt->trace_ctrl.buf_index = 0;
>> +
>> + /* If the trace buffer has already been allocated, zero it. */
>> + if (ctrl->trace_buf) {
>> + for (i = 0; i < HISI_PTT_TRACE_BUF_CNT; i++)
>> + memset(ctrl->trace_buf[i].addr, 0, HISI_PTT_TRACE_BUF_SIZE);
>> + return 0;
>> + }
>> +
>> + ctrl->trace_buf = devm_kcalloc(dev, HISI_PTT_TRACE_BUF_CNT,
>> + sizeof(struct hisi_ptt_dma_buffer), GFP_KERNEL);
>> + if (!ctrl->trace_buf)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < HISI_PTT_TRACE_BUF_CNT; ++i) {
>> + ctrl->trace_buf[i].addr = dmam_alloc_coherent(dev, HISI_PTT_TRACE_BUF_SIZE,
>> + &ctrl->trace_buf[i].dma,
>> + GFP_KERNEL);
>> + if (!ctrl->trace_buf[i].addr) {
>> + hisi_ptt_free_trace_buf(hisi_ptt);
>> + return -ENOMEM;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void hisi_ptt_trace_end(struct hisi_ptt *hisi_ptt)
>> +{
>> + writel(0, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);
>> + hisi_ptt->trace_ctrl.started = false;
>> +}
>> +
>> +static int hisi_ptt_trace_start(struct hisi_ptt *hisi_ptt)
>
> again this function has no caller, so I assume a warn is generated if we only apply up to this patch (when compiling)
>
> please only add code per-patch which is actually referenced
>
it's because I split the trace part into 2 patches as 2/8 provides probe and some basic
functions (mentioned in the commit message) and 3/8 adds the PMU device which use the function
in 2/8, assuming that it'll be easier to review..
I think it's suggested to squash patch 2/8 and 3/8, then the comments here is addressed.
>> +{
>> + struct hisi_ptt_trace_ctrl *ctrl = &hisi_ptt->trace_ctrl;
>> + u32 val;
>> + int i;
>> +
>> + /* Check device idle before start trace */
>> + if (!hisi_ptt_wait_trace_hw_idle(hisi_ptt)) {
>> + pci_err(hisi_ptt->pdev, "Failed to start trace, the device is still busy.\n");
>
> Are we already going to have a "device busy" message? I just wonder if we need this at all
>
The message is necessary as the caller pmu::start() is void and we cannot pass the failure
to the user by the return value. So a message is printed here to notify user the error reason.
>> + return -EBUSY;
>> + }
>> +
>> + ctrl->started = true;
>> +
[...]
>> +static int hisi_ptt_init_ctrls(struct hisi_ptt *hisi_ptt)
>> +{
>> + struct pci_dev *pdev = hisi_ptt->pdev;
>> + struct pci_bus *bus;
>> + int ret;
>> + u32 reg;
>> +
>> + INIT_LIST_HEAD(&hisi_ptt->port_filters);
>> + INIT_LIST_HEAD(&hisi_ptt->req_filters);
>> +
>> + /*
>> + * The device range register provides the information about the
>> + * root ports which the RCiEP can control and trace. The RCiEP
>> + * and the root ports it support are on the same PCIe core, with
>> + * same domain number but maybe different bus number. The device
>> + * range register will tell us which root ports we can support,
>> + * Bit[31:16] indicates the upper BDF numbers of the root port,
>> + * while Bit[15:0] indicates the lower.
>> + */
>> + reg = readl(hisi_ptt->iobase + HISI_PTT_DEVICE_RANGE);
>> + hisi_ptt->upper = FIELD_GET(HISI_PTT_DEVICE_RANGE_UPPER, reg);
>> + hisi_ptt->lower = FIELD_GET(HISI_PTT_DEVICE_RANGE_LOWER, reg);
>> +
>> + /*
>> + * hisi_ptt_init_filters() only fails when the memory allocation failed.
>> + * We don't check the failure here as it won't fail after adding the
>> + * support of dynamically updating the filters in the following patch.
>
> please structure the series such that we don't need to talk about how we will fix it later
>
will drop the comment here and handle the error as usual.
>> + */
>> + bus = pci_find_bus(pci_domain_nr(pdev->bus), PCI_BUS_NUM(hisi_ptt->upper));
>> + if (bus)
>> + pci_walk_bus(bus, hisi_ptt_init_filters, hisi_ptt);
>> +
>> + ret = devm_add_action_or_reset(&pdev->dev, hisi_ptt_release_filters, hisi_ptt);
>> + if (ret)
>> + return ret;
>> +
>> + hisi_ptt->trace_ctrl.default_cpu = cpumask_first(cpumask_of_node(dev_to_node(&pdev->dev)));
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * The DMA of PTT trace can only use direct mapping, due to some
>> + * hardware restriction. Check whether there is an IOMMU or the
>> + * policy of the IOMMU domain is passthrough, otherwise the trace
>> + * cannot work.
>> + *
>> + * The PTT device is supposed to behind the ARM SMMUv3, which
>> + * should have passthrough the device by a quirk.
>> + */
>> +static int hisi_ptt_check_iommu_mapping(struct pci_dev *pdev)
>> +{
>> + struct iommu_domain *iommu_domain;
>> +
>> + iommu_domain = iommu_get_domain_for_dev(&pdev->dev);
>> + if (!iommu_domain || iommu_domain->type == IOMMU_DOMAIN_IDENTITY)
>> + return 0;
>> +
>> + return -EOPNOTSUPP;
>> +}
>> +
>> +static int hisi_ptt_probe(struct pci_dev *pdev,
>> + const struct pci_device_id *id)
>> +{
>> + struct hisi_ptt *hisi_ptt;
>> + int ret;
>> +
>> + ret = hisi_ptt_check_iommu_mapping(pdev);
>> + if (ret) {
>> + pci_err(pdev, "cannot work with non-direct DMA mapping.\n");
>
> please no double-negatives like this, so maybe "requires direct DMA mappings"
>
will refine the message.
>> + return ret;
>> + }
>> +
>> + hisi_ptt = devm_kzalloc(&pdev->dev, sizeof(*hisi_ptt), GFP_KERNEL);
>> + if (!hisi_ptt)
>> + return -ENOMEM;
>> +
>> + mutex_init(&hisi_ptt->mutex);
>> + hisi_ptt->pdev = pdev;
>> + pci_set_drvdata(pdev, hisi_ptt);
>> +
>> + ret = pcim_enable_device(pdev);
>> + if (ret) {
>> + pci_err(pdev, "failed to enable device, ret = %d.\n", ret);
>
> nit: no '.' at end of any messages
>
will drop '.' for all the messages.
>> + return ret;
>> + }
>> +
>> + ret = pcim_iomap_regions(pdev, BIT(2), DRV_NAME);
>> + if (ret) {
>> + pci_err(pdev, "failed to remap io memory, ret = %d.\n", ret);
>> + return ret;
>> + }
>> +
>> + hisi_ptt->iobase = pcim_iomap_table(pdev)[2];
>> +
>> + ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
>> + if (ret) {
>> + pci_err(pdev, "failed to set 64 bit dma mask, ret = %d.\n", ret);
>
> I do doubt that this message is any use
>
I think it's useful as with the message it will be easier to find on which stage the probe fails.
Thanks,
Yicong
