| Message ID | 20230419141328.37472-1-quic_jinlmao@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] stm: class: Add MIPI OST protocol support | expand |
Hi Mao,
kernel test robot noticed the following build errors:
[auto build test ERROR on atorgue-stm32/stm32-next]
[also build test ERROR on linus/master v6.3-rc7 next-20230418]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mao-Jinlong/stm-class-Add-MIPI-OST-protocol-support/20230419-221653
base: https://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next
patch link: https://lore.kernel.org/r/20230419141328.37472-1-quic_jinlmao%40quicinc.com
patch subject: [PATCH v2] stm: class: Add MIPI OST protocol support
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230420/202304200216.kvZgZcao-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/54db7d137859caf5a14de2b166d80913b0c80218
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Mao-Jinlong/stm-class-Add-MIPI-OST-protocol-support/20230419-221653
git checkout 54db7d137859caf5a14de2b166d80913b0c80218
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304200216.kvZgZcao-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
drivers/hwtracing/stm/p_ost.c: In function 'ost_write':
>> drivers/hwtracing/stm/p_ost.c:174:46: error: implicit declaration of function 'get_current'; did you mean 'get_cred'? [-Werror=implicit-function-declaration]
174 | *(u64 *)(trc_hdr + 8) = task_tgid_nr(get_current());
| ^~~~~~~~~~~
| get_cred
>> drivers/hwtracing/stm/p_ost.c:174:46: warning: passing argument 1 of 'task_tgid_nr' makes pointer from integer without a cast [-Wint-conversion]
174 | *(u64 *)(trc_hdr + 8) = task_tgid_nr(get_current());
| ^~~~~~~~~~~~~
| |
| int
In file included from include/linux/sched/mm.h:7,
from include/linux/xarray.h:19,
from include/linux/radix-tree.h:21,
from include/linux/idr.h:15,
from include/linux/kernfs.h:12,
from include/linux/sysfs.h:16,
from include/linux/kobject.h:20,
from include/linux/module.h:21,
from drivers/hwtracing/stm/p_ost.c:9:
include/linux/sched.h:1582:54: note: expected 'struct task_struct *' but argument is of type 'int'
1582 | static inline pid_t task_tgid_nr(struct task_struct *tsk)
| ~~~~~~~~~~~~~~~~~~~~^~~
cc1: some warnings being treated as errors
vim +174 drivers/hwtracing/stm/p_ost.c
131
132 static ssize_t notrace ost_write(struct stm_data *data,
133 struct stm_output *output, unsigned int chan,
134 const char *buf, size_t count)
135 {
136 unsigned int c = output->channel + chan;
137 unsigned int m = output->master;
138 const unsigned char nil = 0;
139 u32 header = DATA_HEADER;
140 u8 trc_hdr[16];
141 ssize_t sz;
142
143 struct ost_output *op = output->pdrv_private;
144
145 /*
146 * Identify the source by entity type.
147 * If entity type is not set, return error value.
148 */
149 if (op->node.entity_type == OST_ENTITY_TYPE_FTRACE) {
150 header |= OST_ENTITY_FTRACE;
151 } else if (op->node.entity_type == OST_ENTITY_TYPE_CONSOLE) {
152 header |= OST_ENTITY_CONSOLE;
153 } else {
154 pr_debug("p_ost: Entity must be set for trace data.");
155 return -EINVAL;
156 }
157
158 /*
159 * STP framing rules for OST frames:
160 * * the first packet of the OST frame is marked;
161 * * the last packet is a FLAG with timestamped tag.
162 */
163 /* Message layout: HEADER / DATA / TAIL */
164 /* HEADER */
165 sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED,
166 4, (u8 *)&header);
167 if (sz <= 0)
168 return sz;
169
170 /* DATA */
171 *(u16 *)(trc_hdr) = STM_MAKE_VERSION(0, 4);
172 *(u16 *)(trc_hdr + 2) = STM_HEADER_MAGIC;
173 *(u32 *)(trc_hdr + 4) = raw_smp_processor_id();
> 174 *(u64 *)(trc_hdr + 8) = task_tgid_nr(get_current());
175 sz = stm_data_write(data, m, c, false, trc_hdr, sizeof(trc_hdr));
176 if (sz <= 0)
177 return sz;
178
179 sz = stm_data_write(data, m, c, false, buf, count);
180
181 /* TAIL */
182 if (sz > 0)
183 data->packet(data, m, c, STP_PACKET_FLAG,
184 STP_PACKET_TIMESTAMPED, 0, &nil);
185
186 return sz;
187 }
188
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
diff --git a/Documentation/ABI/testing/configfs-stp-policy-p_ost b/Documentation/ABI/testing/configfs-stp-policy-p_ost new file mode 100644 index 000000000000..7ad11ae21760 --- /dev/null +++ b/Documentation/ABI/testing/configfs-stp-policy-p_ost @@ -0,0 +1,13 @@ +What: /config/stp-policy/<device>:p_ost.<policy>/<node>/entity_available +Date: April 2023 +KernelVersion: 6.3 +Description: + Show entity that p_ost supports, RO. + +What: /config/stp-policy/<device>:p_ost.<policy>/<node>/entity +Date: April 2023 +KernelVersion: 6.3 +Description: + Set the entity which is to identify the source, RW. + The available entity can be gotten by reading entity_available. + diff --git a/Documentation/trace/p_ost.rst b/Documentation/trace/p_ost.rst new file mode 100644 index 000000000000..8d7afa341b9e --- /dev/null +++ b/Documentation/trace/p_ost.rst @@ -0,0 +1,40 @@ +.. SPDX-License-Identifier: GPL-2.0 + +=================== +MIPI OST over STP +=================== + +The OST(Open System Trace) driver is used with STM class devices to +generate standardized trace stream. Trace sources can be identified +by different entity ids. + +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". + +You can check if the protocol is set successfully by: +# cat /sys/kernel/config/stp-policy/stm0:p_ost.policy/protocol +p_ost + +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 + +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 + +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. + + The receiving side must be able to decode this protocol in + addition to the MIPI STP, in order to extract the data. + + If you don't know what this is, say N. + config STM_DUMMY tristate "Dummy STM driver" help diff --git a/drivers/hwtracing/stm/Makefile b/drivers/hwtracing/stm/Makefile index 1692fcd29277..715fc721891e 100644 --- a/drivers/hwtracing/stm/Makefile +++ b/drivers/hwtracing/stm/Makefile @@ -5,9 +5,11 @@ stm_core-y := core.o policy.o obj-$(CONFIG_STM_PROTO_BASIC) += stm_p_basic.o obj-$(CONFIG_STM_PROTO_SYS_T) += stm_p_sys-t.o +obj-$(CONFIG_STM_PROTO_OST) += stm_p_ost.o stm_p_basic-y := p_basic.o stm_p_sys-t-y := p_sys-t.o +stm_p_ost-y := p_ost.o obj-$(CONFIG_STM_DUMMY) += dummy_stm.o diff --git a/drivers/hwtracing/stm/p_ost.c b/drivers/hwtracing/stm/p_ost.c new file mode 100644 index 000000000000..fe99b68569d3 --- /dev/null +++ b/drivers/hwtracing/stm/p_ost.c @@ -0,0 +1,213 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved. + * + * MIPI OST framing protocol for STM devices. + */ + +#include <linux/configfs.h> +#include <linux/module.h> +#include <linux/device.h> +#include <linux/slab.h> +#include <linux/stm.h> +#include "stm.h" + + +#define OST_TOKEN_STARTSIMPLE (0x10) +#define OST_VERSION_MIPI1 (0x10 << 8) + +/* entity id to identify the source*/ +#define OST_ENTITY_FTRACE (0x01 << 16) +#define OST_ENTITY_CONSOLE (0x02 << 16) + +#define OST_CONTROL_PROTOCOL (0x0 << 24) + +/* + * OST Base Protocol Header + * + * Position Bits Field Name + * 0 8 STARTSIMPLE + * 1 8 Version + * 2 8 Entity ID + * 3 8 protocol ID + */ +#define DATA_HEADER (OST_TOKEN_STARTSIMPLE | OST_VERSION_MIPI1 | \ + OST_CONTROL_PROTOCOL) + +#define STM_MAKE_VERSION(ma, mi) ((ma << 8) | mi) +#define STM_HEADER_MAGIC (0x5953) + +enum ost_entity_type { + OST_ENTITY_TYPE_NONE, + OST_ENTITY_TYPE_FTRACE, + OST_ENTITY_TYPE_CONSOLE, +}; + +static const char * const str_ost_entity_type[] = { + [OST_ENTITY_TYPE_NONE] = "none", + [OST_ENTITY_TYPE_FTRACE] = "ftrace", + [OST_ENTITY_TYPE_CONSOLE] = "console", +}; + +struct ost_policy_node { + enum ost_entity_type entity_type; +}; + +struct ost_output { + struct ost_policy_node node; +}; + +/* Set default entity type as none */ +static void ost_policy_node_init(void *priv) +{ + struct ost_policy_node *pn = priv; + + pn->entity_type = OST_ENTITY_TYPE_NONE; +} + +static int ost_output_open(void *priv, struct stm_output *output) +{ + struct ost_policy_node *pn = priv; + struct ost_output *opriv; + + opriv = kzalloc(sizeof(*opriv), GFP_ATOMIC); + if (!opriv) + return -ENOMEM; + + memcpy(&opriv->node, pn, sizeof(opriv->node)); + output->pdrv_private = opriv; + return 0; +} + +static void ost_output_close(struct stm_output *output) +{ + kfree(output->pdrv_private); +} + +static ssize_t ost_t_policy_entity_show(struct config_item *item, + char *page) +{ + struct ost_policy_node *pn = to_pdrv_policy_node(item); + + return scnprintf(page, PAGE_SIZE, "%s\n", + str_ost_entity_type[pn->entity_type]); +} + +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); + + 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; + 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"); +} +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; + + /* + * 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."); + 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()); + sz = stm_data_write(data, m, c, false, trc_hdr, sizeof(trc_hdr)); + if (sz <= 0) + return sz; + + sz = stm_data_write(data, m, c, false, buf, count); + + /* TAIL */ + if (sz > 0) + data->packet(data, m, c, STP_PACKET_FLAG, + STP_PACKET_TIMESTAMPED, 0, &nil); + + return sz; +} + +static const struct stm_protocol_driver ost_pdrv = { + .owner = THIS_MODULE, + .name = "p_ost", + .write = ost_write, + .policy_attr = ost_t_policy_attrs, + .output_open = ost_output_open, + .output_close = ost_output_close, + .policy_node_init = ost_policy_node_init, +}; + +static int ost_stm_init(void) +{ + return stm_register_protocol(&ost_pdrv); +} + +static void ost_stm_exit(void) +{ + stm_unregister_protocol(&ost_pdrv); +} + +module_init(ost_stm_init); +module_exit(ost_stm_exit); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("MIPI Open System Trace STM framing protocol driver");