From: hejunhao <hejunhao3@huawei.com>
To: Suzuki K Poulose <suzuki.poulose@arm.com>,
<mathieu.poirier@linaro.org>, <mike.leach@linaro.org>,
<leo.yan@linaro.org>, <jonathan.cameron@huawei.com>
Cc: <coresight@lists.linaro.org>, <linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-doc@vger.kernel.org>, <lpieralisi@kernel.org>,
<linuxarm@huawei.com>, <yangyicong@huawei.com>,
<liuqi6124@gmail.com>, <f.fangjian@huawei.com>,
<prime.zeng@hisilicon.com>
Subject: Re: [PATCH v14 1/2] drivers/coresight: Add UltraSoc System Memory Buffer driver
Date: Fri, 25 Nov 2022 22:26:29 +0800 [thread overview]
Message-ID: <8e3b1957-33a7-83ed-6d62-1294cdb679e4@huawei.com> (raw)
In-Reply-To: <80cf9c73-4b9a-f2f7-f72d-de985c045f9c@arm.com>
On 2022/11/24 21:45, Suzuki K Poulose wrote:
> On 24/11/2022 13:33, hejunhao wrote:
>>
>> On 2022/11/23 22:03, Suzuki K Poulose wrote:
>>> On 23/11/2022 12:38, Junhao He wrote:
>>>> From: Qi Liu <liuqi115@huawei.com>
>>>>
>>>> Add driver for UltraSoc SMB(System Memory Buffer) device.
>>>> SMB provides a way to buffer messages from ETM, and store
>>>> these "CPU instructions trace" in system memory.
>>>> The SMB device is identifier as ACPI HID "HISI03A1". Device
>>>> system memory address resources are allocated using the _CRS
>>>> method and buffer modes is the circular buffer mode.
>>>>
>>>> SMB is developed by UltraSoc technology, which is acquired by
>>>> Siemens, and we still use "UltraSoc" to name driver.
>>>>
>>>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>>>> Signed-off-by: Junhao He <hejunhao3@huawei.com>
>>>> Tested-by: JunHao He <hejunhao3@huawei.com>
>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>> ---
>>>> drivers/hwtracing/coresight/Kconfig | 12 +
>>>> drivers/hwtracing/coresight/Makefile | 1 +
>>>> drivers/hwtracing/coresight/ultrasoc-smb.c | 658
>>>> +++++++++++++++++++++
>>>> drivers/hwtracing/coresight/ultrasoc-smb.h | 129 ++++
>>>> 4 files changed, 800 insertions(+)
>>>> create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.c
>>>> create mode 100644 drivers/hwtracing/coresight/ultrasoc-smb.h
>>>>
>>>
>>>> +static void smb_sync_perf_buffer(struct smb_drv_data *drvdata,
>>>> + struct cs_buffers *buf,
>>>> + unsigned long head,
>>>> + unsigned long data_size)
>>>> +{
>>>> + struct smb_data_buffer *sdb = &drvdata->sdb;
>>>> + char **dst_pages = (char **)buf->data_pages;
>>>> + unsigned long to_copy;
>>>> + long pg_idx, pg_offset;
>>>> +
>>>> + pg_idx = head >> PAGE_SHIFT;
>>>> + pg_offset = head & (PAGE_SIZE - 1);
>>>> +
>>>> + while (data_size) {
>>>> + unsigned long pg_space = PAGE_SIZE - pg_offset;
>>>> +
>>>> + /* Copy parts of trace data when read pointer wrap around */
>>>> + if (sdb->rd_offset + pg_space > sdb->buf_size)
>>>> + to_copy = sdb->buf_size - sdb->rd_offset;
>>>> + else
>>>> + to_copy = min(data_size, pg_space);
>>>> +
>>>> + memcpy(dst_pages[pg_idx] + pg_offset,
>>>> + sdb->buf_base + sdb->rd_offset, to_copy);
>>>> +
>>>> + pg_offset += to_copy;
>>>> + if (pg_offset >= PAGE_SIZE) {
>>>> + pg_offset = 0;
>>>> + pg_idx++;
>>>> + pg_idx %= buf->nr_pages;
>>>> + }
>>>> + data_size -= to_copy;
>>>> + sdb->rd_offset += to_copy;
>>>> + sdb->rd_offset %= sdb->buf_size;
>>>> + }
>>>> +
>>>> + sdb->data_size = 0;
>>>
>>>
>>> --8>-- cut here --<8--
>>>
>>>> + writel(sdb->start_addr + sdb->rd_offset,
>>>> + drvdata->base + SMB_LB_RD_ADDR_REG);
>>>> +
>>>> + /*
>>>> + * Data remained in link cannot be purged when SMB is full, so
>>>> + * synchronize the read pointer to write pointer, to make sure
>>>> + * these remained data won't influence next trace.
>>>> + */
>>>> + if (sdb->full) {
>>>> + smb_purge_data(drvdata);
>>>> + writel(readl(drvdata->base + SMB_LB_WR_ADDR_REG),
>>>> + drvdata->base + SMB_LB_RD_ADDR_REG);
>>>> + }
>>>
>>> --<8-- end here --8>--
>>>
>>> As pointed out in the last review, we must do this step
>>> everytime for perf mode irrespective of whether the buffer
>>> was "FULL" or not.
>>>
>>> i.e, the above block should simply be:
>>>
>>> if (sdb->full)
>>> smb_purge_data(drvdata);
>>>
>>> /*
>>> * The uncollected Data must be discarded for perf,
>>> * as it cannot be clubbed with next schedule. We
>>> * any way TRUNCATE the buffer in this case.
>>> */
>>> writel(readl(drvdata->base + SMB_LB_WR_ADDR_REG),
>>> drvdata->base + SMB_LB_RD_ADDR_REG);
>>>
>>> Suzuki
>>
>> Hi Suzuki,
>>
>> We need to update SMB_LB_RD_ADDR_REG register first, then
>> check the "full" flag, whether the register needs to be
>> updated again.
>
> Why ? sdb->full is not updated after the write to RD_ADDR_REG.
>
Hi Suzuki,
Maybe using the code below is more appropriate.
i.e,
writel(sdb->start_addr + sdb->rd_offset,
drvdata->base + SMB_LB_RD_ADDR_REG);
/*
* The uncollected Data must be discarded for perf,
* as it cannot be clubbed with next schedule. We
* any way TRUNCATE the buffer in this case.
*/
smb_update_data_size(drvdata);
if (sdb->data_size)
writel(readl(drvdata->base + SMB_LB_WR_ADDR_REG),
drvdata->base + SMB_LB_RD_ADDR_REG);
Best regards,
Junhao.
>>
>> If we don`t update the value of SMB_LB_RD_ADDR_REG register
>> or reset buffer state, the buffer state will still be "full".
>> The buffer has not free area,so the data will still remain
>> in link.
>
> My issue here is with potentially "leaving the trace from a previous
> session for the next session". i.e., at the end of a run, we must always
> make sure that the buffer is left empty (unlike the sysfs mode).
>
> e.g.,
>
> perf_session_x: RUN0: RD=0x0, WR=0x5000, HANDLE_SIZE=0x3000, full=false.
> At the end of the above routine we will have :
> RD=0x3000, WR=0x5000,
>
> and if a different perf session comes in, say perf_session_y, it will
> consume trace written by "x" at 0x3000-0x50000, right ?
>
> This is fine in the sysfs mode as we expect the entire sysfs mode
> to be owned by a single session and is left to the user to split it.
> But for perf mode we cannot do that and thus must make sure we don't
> leak trace from one session to antoher.
>
> Suzuki
>
In this cace: RUN0: RD=0x0, WR=0x5000, HANDLE_SIZE=0x3000, full=false.
We will update the "rd_offset" in smb_update_buffer() function.
like this:
...
if (data_size > handle->size) {
sdb->rd_offset += data_size - handle->size;
sdb->rd_offset %= sdb->buf_size;
...
}
...
So the rd_offset will advance to 0x2000 first,
then we dump the latest trace data (0x2000 - 0x5000) from "buf_base +
rd_offset".
Best regards,
Junhao.
>
>>
>> Thanks.
>> HeJunhao.
>>
>>> _______________________________________________
>>> CoreSight mailing list -- coresight@lists.linaro.org
>>> To unsubscribe send an email to coresight-leave@lists.linaro.org
>>>
>>> .
>>>
>>
>
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org
next prev parent reply other threads:[~2022-11-25 14:26 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-23 12:38 [PATCH v14 0/2] Add support for UltraSoc System Memory Buffer Junhao He
2022-11-23 12:38 ` [PATCH v14 1/2] drivers/coresight: Add UltraSoc System Memory Buffer driver Junhao He
2022-11-23 14:03 ` Suzuki K Poulose
2022-11-24 13:33 ` hejunhao
2022-11-24 13:45 ` Suzuki K Poulose
2022-11-25 14:26 ` hejunhao [this message]
2022-11-28 11:06 ` Suzuki K Poulose
2022-12-16 9:30 ` hejunhao
2022-11-23 12:38 ` [PATCH v14 2/2] Documentation: Add document for UltraSoc SMB driver Junhao He
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=8e3b1957-33a7-83ed-6d62-1294cdb679e4@huawei.com \
--to=hejunhao3@huawei.com \
--cc=coresight@lists.linaro.org \
--cc=f.fangjian@huawei.com \
--cc=jonathan.cameron@huawei.com \
--cc=leo.yan@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxarm@huawei.com \
--cc=liuqi6124@gmail.com \
--cc=lpieralisi@kernel.org \
--cc=mathieu.poirier@linaro.org \
--cc=mike.leach@linaro.org \
--cc=prime.zeng@hisilicon.com \
--cc=suzuki.poulose@arm.com \
--cc=yangyicong@huawei.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).