From: Yicong Yang <yangyicong@huawei.com>
To: John Garry <john.garry@huawei.com>,
Yicong Yang <yangyicong@hisilicon.com>,
<gregkh@linuxfoundation.org>, <helgaas@kernel.org>,
<alexander.shishkin@linux.intel.com>, <lorenzo.pieralisi@arm.com>,
<will@kernel.org>, <mark.rutland@arm.com>,
<mathieu.poirier@linaro.org>, <suzuki.poulose@arm.com>,
<mike.leach@linaro.org>, <leo.yan@linaro.org>,
<jonathan.cameron@huawei.com>, <daniel.thompson@linaro.org>,
<joro@8bytes.org>, <shameerali.kolothum.thodi@huawei.com>,
<robin.murphy@arm.com>, <peterz@infradead.org>,
<mingo@redhat.com>, <acme@kernel.org>,
<linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<coresight@lists.linaro.org>, <linux-pci@vger.kernel.org>,
<linux-perf-users@vger.kernel.org>,
<iommu@lists.linux-foundation.org>
Cc: <prime.zeng@huawei.com>, <liuqi115@huawei.com>,
<zhangshaokun@hisilicon.com>, <linuxarm@huawei.com>,
<song.bao.hua@hisilicon.com>
Subject: Re: [PATCH v5 2/8] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
Date: Wed, 16 Mar 2022 15:49:38 +0800 [thread overview]
Message-ID: <3f8bff53-e6fb-d4ae-b48f-d9d9ef22f929@huawei.com> (raw)
In-Reply-To: <2049e270-7cf4-84c5-eb84-5bb1a425e897@huawei.com>
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-03-16 7:51 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-08 8:49 [PATCH v5 0/8] Add support for HiSilicon PCIe Tune and Trace device Yicong Yang
2022-03-08 8:49 ` [PATCH v5 1/8] iommu/arm-smmu-v3: Make default domain type of HiSilicon PTT device to identity Yicong Yang
2022-03-11 17:27 ` John Garry
2022-03-16 6:15 ` Yicong Yang
2022-03-08 8:49 ` [PATCH v5 2/8] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device Yicong Yang
2022-03-11 17:55 ` John Garry
2022-03-16 7:49 ` Yicong Yang [this message]
2022-03-08 8:49 ` [PATCH v5 3/8] hisi_ptt: Register PMU device for PTT trace Yicong Yang
2022-03-08 10:21 ` Jonathan Cameron
2022-03-08 11:13 ` Yicong Yang
2022-03-08 12:06 ` Jonathan Cameron
2022-03-08 13:20 ` Yicong Yang
2022-03-08 8:49 ` [PATCH v5 4/8] hisi_ptt: Add support for dynamically updating the filter list Yicong Yang
2022-03-08 10:23 ` Jonathan Cameron
2022-03-08 8:49 ` [PATCH v5 5/8] hisi_ptt: Add tune function support for HiSilicon PCIe Tune and Trace device Yicong Yang
2022-03-08 8:49 ` [PATCH v5 6/8] perf tool: Add support for HiSilicon PCIe Tune and Trace device driver Yicong Yang
2022-03-08 8:49 ` [PATCH v5 7/8] docs: Add HiSilicon PTT device driver documentation Yicong Yang
2022-03-08 8:49 ` [PATCH v5 8/8] MAINTAINERS: Add maintainer for HiSilicon PTT driver Yicong Yang
2022-03-08 10:32 ` Jonathan Cameron
2022-03-08 11:49 ` Yicong Yang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3f8bff53-e6fb-d4ae-b48f-d9d9ef22f929@huawei.com \
--to=yangyicong@huawei.com \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=coresight@lists.linaro.org \
--cc=daniel.thompson@linaro.org \
--cc=gregkh@linuxfoundation.org \
--cc=helgaas@kernel.org \
--cc=iommu@lists.linux-foundation.org \
--cc=john.garry@huawei.com \
--cc=jonathan.cameron@huawei.com \
--cc=joro@8bytes.org \
--cc=leo.yan@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=linuxarm@huawei.com \
--cc=liuqi115@huawei.com \
--cc=lorenzo.pieralisi@arm.com \
--cc=mark.rutland@arm.com \
--cc=mathieu.poirier@linaro.org \
--cc=mike.leach@linaro.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=prime.zeng@huawei.com \
--cc=robin.murphy@arm.com \
--cc=shameerali.kolothum.thodi@huawei.com \
--cc=song.bao.hua@hisilicon.com \
--cc=suzuki.poulose@arm.com \
--cc=will@kernel.org \
--cc=yangyicong@hisilicon.com \
--cc=zhangshaokun@hisilicon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).