Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Alexander Shishkin <alexander.shishkin@linux.intel.com>
To:     Mao Jinlong <quic_jinlmao@quicinc.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        Alexandre Torgue <alexandre.torgue@foss.st.com>,
        Tingwei Zhang <quic_tingweiz@quicinc.com>
Cc:     Mao Jinlong <quic_jinlmao@quicinc.com>,
        linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org,
        linux-stm32@st-md-mailman.stormreply.com,
        linux-arm-kernel@lists.infradead.org,
        linux-arm-msm@vger.kernel.org,
        Yuanfang Zhang <quic_yuanfang@quicinc.com>,
        Tao Zhang <quic_taozha@quicinc.com>,
        Hao Zhang <quic_hazha@quicinc.com>,
        alexander.shishkin@linux.intel.com
Subject: Re: [PATCH v2] stm: class: Add MIPI OST protocol support
In-Reply-To: <20230419141328.37472-1-quic_jinlmao@quicinc.com>
References: <20230419141328.37472-1-quic_jinlmao@quicinc.com>
Date:   Thu, 20 Apr 2023 13:02:16 +0300
Message-ID: <87cz3yyiqf.fsf@ubik.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Mao Jinlong <quic_jinlmao@quicinc.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.

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