[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <fbb693a7-224a-4155-88a2-67e05faaf21e@quicinc.com>
Date: Thu, 7 Aug 2025 14:34:29 +0800
From: Jinlong Mao <quic_jinlmao@...cinc.com>
To: Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Steven Rostedt
<rostedt@...dmis.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
"Jonathan
Corbet" <corbet@....net>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Tingwei Zhang
<quic_tingweiz@...cinc.com>
CC: <linux-kernel@...r.kernel.org>, <linux-trace-kernel@...r.kernel.org>,
<linux-doc@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-arm-msm@...r.kernel.org>,
Yuanfang Zhang <quic_yuanfang@...cinc.com>,
Tao Zhang
<quic_taozha@...cinc.com>,
Hao Zhang <quic_hazha@...cinc.com>
Subject: Re: [PATCH v2] stm: class: Add MIPI OST protocol support
On 4/20/2023 6:02 PM, Alexander Shishkin wrote:
> Mao Jinlong <quic_jinlmao@...cinc.com> writes:
>
>> Add MIPI OST(Open System Trace) protocol support for stm to format
>> the traces. OST over STP packet consists of Header/Payload/End. In
>> header, there will be STARTSIMPLE/VERSION/ENTITY/PROTOCOL. STARTSIMPLE
>> is used to signal the beginning of a simplified OST base protocol
>> packet.The Entity ID field is a one byte unsigned number that identifies
>> the source. FLAG packet is used for END token.
>
> We'd need a better explanation of what OST is, maybe a link to the spec
> if one exists.
>
Hi Alexander,
Checked with different internal teams. Spec is not public. We need to
upstream these codes. We can add more explanation into the commit
text. Is that ok for your ?
Thanks
Jinlong Mao
> Another thing that this patch does is adding source identification,
> which needs to be described better.
>
> [...]
>
>> +CONFIG_STM_PROTO_OST is for p_ost driver enablement. Once this config
>> +is enabled, you can select the p_ost protocol by command below:
>> +
>> +# mkdir /sys/kernel/config/stp-policy/stm0:p_ost.policy
>> +
>> +The policy name format is extended like this:
>> + <device_name>:<protocol_name>.<policy_name>
>> +
>> +With coresight-stm device, it will be look like "stm0:p_ost.policy".
>
> The part about protocol selection should probably be in stm.rst
> instead.
>
>> +You can check if the protocol is set successfully by:
>> +# cat /sys/kernel/config/stp-policy/stm0:p_ost.policy/protocol
>> +p_ost
>
> A successful mkdir is technically enough.
>
>> +With MIPI OST protocol driver, the attributes for each protocol node is:
>> +# mkdir /sys/kernel/config/stp-policy/stm0:p_ost.policy/default
>> +# ls /sys/kernel/config/stp-policy/stm0:p_ost.policy/default
>> +channels entity masters
>
> Where's "entity_available"?
>
>> +The entity here is the set the entity that p_ost supports. Currently
>> +p_ost supports ftrace and console entity.
>> +
>> +Get current available entity that p_ost supports:
>> +# cat /sys/kernel/config/stp-policy/stm0:p_ost.policy/default/entity_available
>> +ftrace console
>> +
>> +Set entity:
>> +# echo 'ftrace' > /sys/kernel/config/stp-policy/stm0:p_ost.policy/default/entity
>
> This is not a very good example, as it will flag everything that goes
> through STM as "ftrace", which is probably not what anybody wants.
>
> The bigger question is, why do we need to set the source type (for
> which "entity" is not a very good name, btw) in the configfs when
> corresponding stm source drivers already carry this information.
> There should be a way to propagate the source type from stm source
> driver to the protocol driver without relying on the user to set it
> correctly.
>
>> +See Documentation/ABI/testing/configfs-stp-policy-p_ost for more details.
>> diff --git a/drivers/hwtracing/stm/Kconfig b/drivers/hwtracing/stm/Kconfig
>> index eda6b11d40a1..daa4aa09f64d 100644
>> --- a/drivers/hwtracing/stm/Kconfig
>> +++ b/drivers/hwtracing/stm/Kconfig
>> @@ -40,6 +40,20 @@ config STM_PROTO_SYS_T
>>
>> If you don't know what this is, say N.
>>
>> +config STM_PROTO_OST
>> + tristate "MIPI OST STM framing protocol driver"
>> + default CONFIG_STM
>> + help
>> + This is an implementation of MIPI OST protocol to be used
>> + over the STP transport. In addition to the data payload, it
>> + also carries additional metadata for entity, better
>> + means of trace source identification, etc.
>
> What does "entity" mean here?
>
> [...]
>
>> +#define OST_TOKEN_STARTSIMPLE (0x10)
>> +#define OST_VERSION_MIPI1 (0x10 << 8)
>
> Either write them as bits (BIT(12)) or as a hex value (0x1000).
>
>> +/* entity id to identify the source*/
>> +#define OST_ENTITY_FTRACE (0x01 << 16)
>> +#define OST_ENTITY_CONSOLE (0x02 << 16)
>> +
>> +#define OST_CONTROL_PROTOCOL (0x0 << 24)
>
> Zero, really? At this point I'm wondering if this code has even been
> tested.
>
> [...]
>
>> +static ssize_t
>> +ost_t_policy_entity_store(struct config_item *item, const char *page,
>> + size_t count)
>> +{
>> + struct mutex *mutexp = &item->ci_group->cg_subsys->su_mutex;
>> + struct ost_policy_node *pn = to_pdrv_policy_node(item);
>> + char str[10] = "";
>> +
>> + mutex_lock(mutexp);
>> + if (sscanf(page, "%s", str) != 1)
>> + return -EINVAL;
>> + mutex_unlock(mutexp);
>
> You forgot to release the mutex in the error path.
> Also, why do you need a mutex around sscanf() in the first place?
> Also, the sscanf() can overrun str.
>
>> + if (!strcmp(str, str_ost_entity_type[OST_ENTITY_TYPE_FTRACE]))
>> + pn->entity_type = OST_ENTITY_TYPE_FTRACE;
>> + else if (!strcmp(str, str_ost_entity_type[OST_ENTITY_TYPE_CONSOLE]))
>> + pn->entity_type = OST_ENTITY_TYPE_CONSOLE;
>
> Why can't you strcmp() on the page directly?
> Also, this is where you do want to hold the mutex.
> Also, what if there are more source types?
>
>> + else
>> + return -EINVAL;
>> + return count;
>> +}
>> +CONFIGFS_ATTR(ost_t_policy_, entity);
>> +
>> +static ssize_t ost_t_policy_entity_available_show(struct config_item *item,
>> + char *page)
>> +{
>> + return scnprintf(page, PAGE_SIZE, "%s\n", "ftrace console");
>
> Don't hardcode these.
>
>> +}
>> +CONFIGFS_ATTR_RO(ost_t_policy_, entity_available);
>> +
>> +static struct configfs_attribute *ost_t_policy_attrs[] = {
>> + &ost_t_policy_attr_entity,
>> + &ost_t_policy_attr_entity_available,
>> + NULL,
>> +};
>> +
>> +static ssize_t notrace ost_write(struct stm_data *data,
>> + struct stm_output *output, unsigned int chan,
>> + const char *buf, size_t count)
>> +{
>> + unsigned int c = output->channel + chan;
>> + unsigned int m = output->master;
>> + const unsigned char nil = 0;
>> + u32 header = DATA_HEADER;
>> + u8 trc_hdr[16];
>> + ssize_t sz;
>> +
>> + struct ost_output *op = output->pdrv_private;
>
> As said above, the stm source driver that calls here already knows its
> own source type, there's no need to store it separately.
>
>> +
>> + /*
>> + * Identify the source by entity type.
>> + * If entity type is not set, return error value.
>> + */
>> + if (op->node.entity_type == OST_ENTITY_TYPE_FTRACE) {
>> + header |= OST_ENTITY_FTRACE;
>> + } else if (op->node.entity_type == OST_ENTITY_TYPE_CONSOLE) {
>> + header |= OST_ENTITY_CONSOLE;
>> + } else {
>> + pr_debug("p_ost: Entity must be set for trace data.");
>
> You forgot a newline.
> Also, this message seems to be quite useless: it's either a nop or a
> dmesg storm. In general, it's a bad idea to printk() in the write
> callback.
>
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * STP framing rules for OST frames:
>> + * * the first packet of the OST frame is marked;
>> + * * the last packet is a FLAG with timestamped tag.
>> + */
>> + /* Message layout: HEADER / DATA / TAIL */
>> + /* HEADER */
>> + sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED,
>> + 4, (u8 *)&header);
>> + if (sz <= 0)
>> + return sz;
>> +
>> + /* DATA */
>> + *(u16 *)(trc_hdr) = STM_MAKE_VERSION(0, 4);
>> + *(u16 *)(trc_hdr + 2) = STM_HEADER_MAGIC;
>> + *(u32 *)(trc_hdr + 4) = raw_smp_processor_id();
>> + *(u64 *)(trc_hdr + 8) = task_tgid_nr(get_current());
>
> What's the value in exporting PIDs when there are PID namespaces? How is
> this useful? Also, neither console nor ftrace are required to come in a
> task context.
>
> I already asked in the previous version, why is trc_hdr not a struct?
>
> There also used to be a timestamp field in trc_hdr, what happened to it?
>
> Regards,
> --
> Alex
Powered by blists - more mailing lists