casaroli commented on code in PR #21373: URL: https://github.com/apache/nuttx/pull/21373#discussion_r4448850286
########## drivers/rpmsg/rpmsg_procfs.c: ########## @@ -0,0 +1,271 @@ +/**************************************************************************** + * drivers/rpmsg/rpmsg_procfs.c + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/config.h> + +#include <sys/types.h> +#include <sys/stat.h> + +#include <assert.h> +#include <errno.h> +#include <debug.h> +#include <fnmatch.h> + +#include <nuttx/fs/fs.h> +#include <nuttx/fs/procfs.h> +#include <nuttx/kmalloc.h> +#include <nuttx/nuttx.h> +#include <nuttx/rpmsg/rpmsg.h> + +#include "rpmsg.h" +#include "rpmsg_procfs.h" + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +/* Determines the size of an intermediate buffer that must be large enough + * to handle the longest line generated by this logic. + */ + +#define RPMSG_PROCFS_LINELEN 80 + +/**************************************************************************** + * Private Types + ****************************************************************************/ + +/* This structure describes one open "file" */ + +struct rpmsg_procfs_file_s +{ + struct procfs_file_s base; /* Base open file structure */ + char line[RPMSG_PROCFS_LINELEN]; /* Pre-allocated buffer for formatted lines */ +}; + +struct rpmsg_procfs_info_s +{ + FAR struct rpmsg_procfs_file_s *procfile; + size_t totalsize; + FAR char *buffer; + size_t buflen; + off_t offset; +}; + +/**************************************************************************** + * Private Function Prototypes + ****************************************************************************/ + +/* File system methods */ + +static int rpmsg_procfs_open(FAR struct file *filep, + FAR const char *relpath, + int oflags, mode_t mode); +static int rpmsg_procfs_close(FAR struct file *filep); +static ssize_t rpmsg_procfs_read(FAR struct file *filep, FAR char *buffer, + size_t buflen); +static ssize_t rpmsg_procfs_write(FAR struct file *filep, + FAR const char *buffer, size_t buflen); + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +static const struct procfs_operations g_rpmsg_procfs_ops = Review Comment: `g_rpmsg_procfs_ops` has no `stat` and no `dup` method, but `procfs_stat()` calls `ops->stat` of a registered entry (fs/procfs/fs_procfs.c:1169) and `procfs_dup()` calls `ops->dup` (fs_procfs.c:592). On qemu-armv8a:rpserver (RPMSG_PROCFS is on by default there) `ls -l /proc` stops at `Assertion failed g_procfs_entries[x].ops && g_procfs_entries[x].ops->stat`. Fix: add `rpmsg_procfs_dup()` and `rpmsg_procfs_stat()` as drivers/reset/core.c:726 does. ########## drivers/rpmsg/rpmsg_trace.c: ########## @@ -0,0 +1,127 @@ +/**************************************************************************** + * drivers/rpmsg/rpmsg_trace.c + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/config.h> + +#include <stdarg.h> +#include <stdint.h> +#include <sys/param.h> +#include <stdbool.h> +#include <execinfo.h> + +#include <nuttx/sched_note.h> +#include <nuttx/spinlock.h> +#include <nuttx/streams.h> +#include <nuttx/rpmsg/rpmsg.h> + +#include "rpmsg_procfs.h" + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +/* nc_length is a uint8_t, so a payload has to be split into notes that + * each fit in it. + */ + +#define RPMSG_TRACE_CHUNK (UINT8_MAX - SIZEOF_NOTE_EVENT(0)) + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +/* Keeps a text record and its binary payload next to each other in the + * note stream when several CPUs trace at the same time. + */ + +static spinlock_t g_rpmsg_trace_lock = SP_UNLOCKED; + +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +/**************************************************************************** + * Name: rpmsg_trace + ****************************************************************************/ + +void rpmsg_trace(FAR const char *name, bool bt, FAR const void *buf, + size_t len, FAR const char *format, ...) +{ + va_list ap; + + va_start(ap, format); + rpmsg_vtrace(name, bt, buf, len, format, ap); + va_end(ap); +} + +/**************************************************************************** + * Name: rpmsg_vtrace + ****************************************************************************/ + +void rpmsg_vtrace(FAR const char *name, bool bt, FAR const void *buf, + size_t len, FAR const char *format, va_list ap) +{ + irqstate_t flags; + + if (!rpmsg_procfs_trace_allow(name)) + { + return; + } + +#if CONFIG_RPMSG_TRACE_BACKTRACE > 0 + if (bt) + { + FAR void *frames[CONFIG_RPMSG_TRACE_BACKTRACE]; + char btbuf[BACKTRACE_BUFFER_SIZE(CONFIG_RPMSG_TRACE_BACKTRACE)]; + struct va_format vaf; + + vaf.fmt = format; + vaf.va = ≈ Review Comment: `&ap` of a `va_list` parameter is not a `va_list *` where `va_list` is an array (x86_64). x86_64-elf-gcc on this file: `error: assignment to '__va_list_tag (*)[1]' from incompatible pointer type '__va_list_tag **'` here and at line 114 (`sched_note_vprintf()` passes `&(ap)`), so sim on x86_64 does not build with RPMSG_TRACE. Fix: `va_copy()` `ap` to a local `va_list` and use the copy at both places, as libs/libc/misc/lib_err.c:63 does. ########## drivers/rpmsg/rpmsg_trace.c: ########## @@ -0,0 +1,127 @@ +/**************************************************************************** + * drivers/rpmsg/rpmsg_trace.c + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/config.h> + +#include <stdarg.h> +#include <stdint.h> +#include <sys/param.h> +#include <stdbool.h> +#include <execinfo.h> + +#include <nuttx/sched_note.h> +#include <nuttx/spinlock.h> +#include <nuttx/streams.h> +#include <nuttx/rpmsg/rpmsg.h> + +#include "rpmsg_procfs.h" + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +/* nc_length is a uint8_t, so a payload has to be split into notes that + * each fit in it. + */ + +#define RPMSG_TRACE_CHUNK (UINT8_MAX - SIZEOF_NOTE_EVENT(0)) + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +/* Keeps a text record and its binary payload next to each other in the + * note stream when several CPUs trace at the same time. + */ + +static spinlock_t g_rpmsg_trace_lock = SP_UNLOCKED; + +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +/**************************************************************************** + * Name: rpmsg_trace + ****************************************************************************/ + +void rpmsg_trace(FAR const char *name, bool bt, FAR const void *buf, + size_t len, FAR const char *format, ...) +{ + va_list ap; + + va_start(ap, format); + rpmsg_vtrace(name, bt, buf, len, format, ap); + va_end(ap); +} + +/**************************************************************************** + * Name: rpmsg_vtrace + ****************************************************************************/ + +void rpmsg_vtrace(FAR const char *name, bool bt, FAR const void *buf, + size_t len, FAR const char *format, va_list ap) +{ + irqstate_t flags; + + if (!rpmsg_procfs_trace_allow(name)) + { + return; + } + +#if CONFIG_RPMSG_TRACE_BACKTRACE > 0 + if (bt) + { + FAR void *frames[CONFIG_RPMSG_TRACE_BACKTRACE]; + char btbuf[BACKTRACE_BUFFER_SIZE(CONFIG_RPMSG_TRACE_BACKTRACE)]; + struct va_format vaf; + + vaf.fmt = format; + vaf.va = ≈ + + backtrace(frames, CONFIG_RPMSG_TRACE_BACKTRACE); + backtrace_format(btbuf, sizeof(btbuf), frames, + CONFIG_RPMSG_TRACE_BACKTRACE); + flags = spin_lock_irqsave(&g_rpmsg_trace_lock); + sched_note_printf(NOTE_TAG_RPMSG, "%pV, %s", &vaf, btbuf); Review Comment: The note driver keeps the format and the raw arguments, and noteram formats them later with lib_bsprintf(), which prints `%p` as a number and `V` as text. So this record shows the address of the stack variable `vaf` and a "V", not the message. Fix: format the message into a local buffer with vsnprintf() and record `"%s, %s"` with that buffer and `btbuf`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
