casaroli commented on code in PR #21374: URL: https://github.com/apache/nuttx/pull/21374#discussion_r4448850689
########## drivers/rpmsg/rpmsg_wakelock.c: ########## @@ -0,0 +1,379 @@ +/**************************************************************************** + * drivers/rpmsg/rpmsg_wakelock.c + * + * 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 <debug.h> + +#include <sys/param.h> + +#include <nuttx/list.h> +#include <nuttx/kmalloc.h> +#include <nuttx/mutex.h> +#include <nuttx/power/pm.h> +#include <nuttx/rpmsg/rpmsg.h> + +#include "rpmsg_wakelock.h" + +/**************************************************************************** + * Pre-processor definitions + ****************************************************************************/ + +#define RPMSG_WAKELOCK_NAME "rpmsg-wakelock" + +#define RPMSG_WAKELOCK_ACQUIRE_REQ 1 +#define RPMSG_WAKELOCK_RELEASE_REQ 2 + +/**************************************************************************** + * Private Types + ****************************************************************************/ + +begin_packed_struct struct rpmsg_wakelock_msg_s +{ + uint32_t cmd; + uint32_t len; + int32_t err; + uint8_t data[1]; +} end_packed_struct; + +struct rpmsg_wakelock_node_s +{ + struct pm_wakelock_s wakelock; /* wakelock */ + struct list_node node; /* list node */ +}; + +struct rpmsg_wakelock_s +{ + FAR struct rpmsg_endpoint *ept; /* Rpmsg endpoint */ + mutex_t lock; /* List lock */ + struct list_node list; /* List node */ +}; + +/**************************************************************************** + * Private Function Prototypes + ****************************************************************************/ + +/* Functions handle the responses from the remote cpu */ + +static int rpmsg_wakelock_acquire_req_handler(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, + uint32_t src, FAR void *priv); + +static int rpmsg_wakelock_release_req_handler(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, + uint32_t src, FAR void *priv); + +/* Functions for creating communication with remote cpu */ + +static int rpmsg_wakelock_ept_cb(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, uint32_t src, + FAR void *priv); + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +static const rpmsg_ept_cb g_rpmsg_wakelock_handler[] = +{ + [RPMSG_WAKELOCK_ACQUIRE_REQ] = rpmsg_wakelock_acquire_req_handler, + [RPMSG_WAKELOCK_RELEASE_REQ] = rpmsg_wakelock_release_req_handler, +}; + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +static int rpmsg_wakelock_control(FAR struct rpmsg_wakelock_s *priv, + FAR const char *name, bool acquire) +{ + FAR struct rpmsg_wakelock_node_s *tmp; + FAR struct rpmsg_wakelock_node_s *wl = NULL; + + nxmutex_lock(&priv->lock); + + list_for_every_entry(&priv->list, tmp, + struct rpmsg_wakelock_node_s, node) + { + if (strcmp(tmp->wakelock.name, name) == 0) Review Comment: `pm_wakelock_init()` keeps only 31 characters of the name (`char name[32]`), but this compares the full received name. With a name of 32 bytes or more every acquire creates a new wakelock and a release finds none. Fix: reject a name of `sizeof(wl->wakelock.name)` bytes or more in `rpmsg_wakelock()` with -ENAMETOOLONG. ########## drivers/rpmsg/rpmsg_wakelock.c: ########## @@ -0,0 +1,379 @@ +/**************************************************************************** + * drivers/rpmsg/rpmsg_wakelock.c + * + * 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 <debug.h> + +#include <sys/param.h> + +#include <nuttx/list.h> +#include <nuttx/kmalloc.h> +#include <nuttx/mutex.h> +#include <nuttx/power/pm.h> +#include <nuttx/rpmsg/rpmsg.h> + +#include "rpmsg_wakelock.h" + +/**************************************************************************** + * Pre-processor definitions + ****************************************************************************/ + +#define RPMSG_WAKELOCK_NAME "rpmsg-wakelock" + +#define RPMSG_WAKELOCK_ACQUIRE_REQ 1 +#define RPMSG_WAKELOCK_RELEASE_REQ 2 + +/**************************************************************************** + * Private Types + ****************************************************************************/ + +begin_packed_struct struct rpmsg_wakelock_msg_s +{ + uint32_t cmd; + uint32_t len; + int32_t err; + uint8_t data[1]; +} end_packed_struct; + +struct rpmsg_wakelock_node_s +{ + struct pm_wakelock_s wakelock; /* wakelock */ + struct list_node node; /* list node */ +}; + +struct rpmsg_wakelock_s +{ + FAR struct rpmsg_endpoint *ept; /* Rpmsg endpoint */ + mutex_t lock; /* List lock */ + struct list_node list; /* List node */ +}; + +/**************************************************************************** + * Private Function Prototypes + ****************************************************************************/ + +/* Functions handle the responses from the remote cpu */ + +static int rpmsg_wakelock_acquire_req_handler(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, + uint32_t src, FAR void *priv); + +static int rpmsg_wakelock_release_req_handler(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, + uint32_t src, FAR void *priv); + +/* Functions for creating communication with remote cpu */ + +static int rpmsg_wakelock_ept_cb(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, uint32_t src, + FAR void *priv); + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +static const rpmsg_ept_cb g_rpmsg_wakelock_handler[] = +{ + [RPMSG_WAKELOCK_ACQUIRE_REQ] = rpmsg_wakelock_acquire_req_handler, + [RPMSG_WAKELOCK_RELEASE_REQ] = rpmsg_wakelock_release_req_handler, +}; + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +static int rpmsg_wakelock_control(FAR struct rpmsg_wakelock_s *priv, + FAR const char *name, bool acquire) +{ + FAR struct rpmsg_wakelock_node_s *tmp; + FAR struct rpmsg_wakelock_node_s *wl = NULL; + + nxmutex_lock(&priv->lock); + + list_for_every_entry(&priv->list, tmp, + struct rpmsg_wakelock_node_s, node) + { + if (strcmp(tmp->wakelock.name, name) == 0) + { + wl = tmp; + break; + } + } + + if (acquire) + { + if (wl == NULL) + { + wl = kmm_zalloc(sizeof(*wl)); + if (wl == NULL) + { + rpmsgerr("rpmsg_wakelock_ctl_wakelock: kmm_zalloc failed\n"); + nxmutex_unlock(&priv->lock); + return -ENOMEM; + } + + rpmsginfo("wakelock created: %s\n", name); + pm_wakelock_init(&wl->wakelock, name, PM_IDLE_DOMAIN, PM_NORMAL); + list_add_tail(&priv->list, &wl->node); + } + + rpmsginfo("acquire wake lock: %s\n", name); + pm_wakelock_stay(&wl->wakelock); + } + else + { + if (wl != NULL) + { + rpmsginfo("release wake lock: %s\n", name); + pm_wakelock_relax(&wl->wakelock); + if (pm_wakelock_staycount(&wl->wakelock) == 0) + { + rpmsginfo("wakelock destroyed: %s\n", name); + list_delete(&wl->node); + pm_wakelock_uninit(&wl->wakelock); + kmm_free(wl); + } + } + } + + nxmutex_unlock(&priv->lock); + return 0; +} + +/**************************************************************************** + * Name: rpmsg_wakelock_acquire_req_handler + * + * Description: + * rpmsg-wakelock acquire req handler. + * + * Parameters: + * ept - The rpmsg endpoint + * data - The return message + * len - The return message length + * + * Returned Values: + * Always OK + * + ****************************************************************************/ + +static int rpmsg_wakelock_acquire_req_handler(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, + uint32_t src, FAR void *priv_) +{ + FAR struct rpmsg_wakelock_s *priv = priv_; + FAR struct rpmsg_wakelock_msg_s *msg = data; + FAR const char *name = (FAR const char *)msg->data; + + msg->err = rpmsg_wakelock_control(priv, name, true); + return 0; +} + +/**************************************************************************** + * Name: rpmsg_wakelock_release_req_handler + * + * Description: + * rpmsg-wakelock release req handler. + * + * Parameters: + * ept - The rpmsg endpoint + * data - The return message + * len - The return message length + * + * Returned Values: + * Always OK + * + ****************************************************************************/ + +static int rpmsg_wakelock_release_req_handler(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, + uint32_t src, FAR void *priv_) +{ + FAR struct rpmsg_wakelock_s *priv = ept->priv; + FAR struct rpmsg_wakelock_msg_s *msg = data; + FAR const char *name = (FAR const char *)msg->data; + + msg->err = rpmsg_wakelock_control(priv, name, false); + return 0; +} + +/**************************************************************************** + * Name: rpmsg_wakelock_ept_cb + * + * Description: + * Rpmsg device end point callback function, this function will be called + * when receive the remote cpu message. + * + * Parameters: + * ept - The rpmsg-device end point + * data - The received data + * len - The received data length + * src - The source address of the message + * priv - Private data of the endpoint + * + * Returned Values: + * OK on success; A negated errno value is returned on any failure. + * + ****************************************************************************/ + +static int rpmsg_wakelock_ept_cb(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, uint32_t src, + FAR void *priv) +{ + FAR struct rpmsg_wakelock_msg_s *msg = data; + uint32_t command = msg->cmd; + + if (command < nitems(g_rpmsg_wakelock_handler)) + { + return g_rpmsg_wakelock_handler[command](ept, data, len, src, priv); + } + + rpmsgerr("rpmsg_wakelock_ept_cb: invalid command: %d\n", command); + return -EINVAL; +} + +/**************************************************************************** + * Name: rpmsg_wakelock + * + * Description: + * rpmsg-wakelock ioctl operation + * + * Parameters: + * ept - The rpmsg endpoint + * cmd - the ioctl command + * arg - the ioctl argument + * + * Returned Values: + * OK on success; A negated errno value is returned on any failure. + * + ****************************************************************************/ + +int rpmsg_wakelock(FAR struct rpmsg_endpoint *ept, + int cmd, unsigned long arg) +{ + FAR struct rpmsg_wakelock_s *priv = ept->priv; + FAR const char *str = (FAR const char *)arg; + FAR struct rpmsg_wakelock_msg_s *msg; + uint32_t msglen; + int ret; + + DEBUGASSERT(priv != NULL); + + msg = rpmsg_get_tx_payload_buffer(ept, &msglen, true); + if (msg == NULL) + { + return -ENOMEM; + } + + msglen = MIN(msglen, sizeof(*msg) + strlen(str)); + + if (cmd == RPMSGIOC_ACQUIRE_WAKELOCK) + { + msg->cmd = RPMSG_WAKELOCK_ACQUIRE_REQ; + } + else if (cmd == RPMSGIOC_RELEASE_WAKELOCK) + { + msg->cmd = RPMSG_WAKELOCK_RELEASE_REQ; + } + else + { + rpmsgerr("rpmsg_wakelock_ioctl: invalid command: %d\n", cmd); + rpmsg_release_tx_buffer(ept, msg); + return -EINVAL; + } + + memcpy(msg->data, str, msglen - sizeof(*msg) + 1); + + msg->len = msglen; + msg->err = 0; + + ret = rpmsg_send_nocopy(priv->ept, msg, msglen); + if (ret < 0) + { + rpmsgerr("rpmsg_wakelock_ioctl: rpmsg_send_nocopy failed: %d\n", ret); + rpmsg_release_tx_buffer(priv->ept, msg); + return ret; + } + + return OK; +} + +/**************************************************************************** + * Name: rpmsg_wakelock_init + * + * Description: + * Using remote wakelock to wake/sleep CPU + * + * Parameters: + * rdev - rpmsg device + * ept - rpmsg endpoint + * + * Returned Values: + * OK on success; A negated errno value is returned on any failure. + ****************************************************************************/ + +int rpmsg_wakelock_init(FAR struct rpmsg_device *rdev, + FAR struct rpmsg_endpoint *ept) +{ + FAR struct rpmsg_wakelock_s *priv; + + priv = kmm_zalloc(sizeof(*priv)); + if (priv == NULL) + { + rpmsgerr("rpmsg_wakelock_initialize: kmm_zalloc failed\n"); + return -ENOMEM; + } + + /* Initialize rpmsg device */ + + nxmutex_init(&priv->lock); + list_initialize(&priv->list); + + priv->ept = ept; + ept->priv = priv; + + return rpmsg_create_ept(ept, rdev, RPMSG_WAKELOCK_NAME, + RPMSG_ADDR_ANY, RPMSG_ADDR_ANY, + rpmsg_wakelock_ept_cb, NULL); +} + +/**************************************************************************** + * Name: rpmsg_wakelock_deinit + * + * Description: + * rpmsg wakelock deinit + * + * Parameters: + * ept - rpmsg endpoint + * + * Returned Values: + * None + ****************************************************************************/ + +void rpmsg_wakelock_deinit(FAR struct rpmsg_endpoint *ept) +{ + FAR struct rpmsg_wakelock_s *priv = ept->priv; + + rpmsg_destroy_ept(ept); + nxmutex_destroy(&priv->lock); + kmm_free(priv); Review Comment: `rpmsg_wakelock_deinit()` frees `priv` but not the wakelocks in `priv->list`. They stay registered with PM in the stay state, so a peer that acquired a wakelock and then went down (rpmsg_port_uart unregisters the device on a CONNREQ from a restarted peer) keeps this CPU awake forever. Fix: before `kmm_free(priv)`, walk `priv->list` and call `pm_wakelock_uninit()` and `kmm_free()` on each node. ########## drivers/rpmsg/rpmsg_wakelock.c: ########## @@ -0,0 +1,379 @@ +/**************************************************************************** + * drivers/rpmsg/rpmsg_wakelock.c + * + * 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 <debug.h> + +#include <sys/param.h> + +#include <nuttx/list.h> +#include <nuttx/kmalloc.h> +#include <nuttx/mutex.h> +#include <nuttx/power/pm.h> +#include <nuttx/rpmsg/rpmsg.h> + +#include "rpmsg_wakelock.h" + +/**************************************************************************** + * Pre-processor definitions + ****************************************************************************/ + +#define RPMSG_WAKELOCK_NAME "rpmsg-wakelock" + +#define RPMSG_WAKELOCK_ACQUIRE_REQ 1 +#define RPMSG_WAKELOCK_RELEASE_REQ 2 + +/**************************************************************************** + * Private Types + ****************************************************************************/ + +begin_packed_struct struct rpmsg_wakelock_msg_s +{ + uint32_t cmd; + uint32_t len; + int32_t err; + uint8_t data[1]; +} end_packed_struct; + +struct rpmsg_wakelock_node_s +{ + struct pm_wakelock_s wakelock; /* wakelock */ + struct list_node node; /* list node */ +}; + +struct rpmsg_wakelock_s +{ + FAR struct rpmsg_endpoint *ept; /* Rpmsg endpoint */ + mutex_t lock; /* List lock */ + struct list_node list; /* List node */ +}; + +/**************************************************************************** + * Private Function Prototypes + ****************************************************************************/ + +/* Functions handle the responses from the remote cpu */ + +static int rpmsg_wakelock_acquire_req_handler(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, + uint32_t src, FAR void *priv); + +static int rpmsg_wakelock_release_req_handler(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, + uint32_t src, FAR void *priv); + +/* Functions for creating communication with remote cpu */ + +static int rpmsg_wakelock_ept_cb(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, uint32_t src, + FAR void *priv); + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +static const rpmsg_ept_cb g_rpmsg_wakelock_handler[] = +{ + [RPMSG_WAKELOCK_ACQUIRE_REQ] = rpmsg_wakelock_acquire_req_handler, + [RPMSG_WAKELOCK_RELEASE_REQ] = rpmsg_wakelock_release_req_handler, +}; + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +static int rpmsg_wakelock_control(FAR struct rpmsg_wakelock_s *priv, + FAR const char *name, bool acquire) +{ + FAR struct rpmsg_wakelock_node_s *tmp; + FAR struct rpmsg_wakelock_node_s *wl = NULL; + + nxmutex_lock(&priv->lock); + + list_for_every_entry(&priv->list, tmp, + struct rpmsg_wakelock_node_s, node) + { + if (strcmp(tmp->wakelock.name, name) == 0) + { + wl = tmp; + break; + } + } + + if (acquire) + { + if (wl == NULL) + { + wl = kmm_zalloc(sizeof(*wl)); + if (wl == NULL) + { + rpmsgerr("rpmsg_wakelock_ctl_wakelock: kmm_zalloc failed\n"); + nxmutex_unlock(&priv->lock); + return -ENOMEM; + } + + rpmsginfo("wakelock created: %s\n", name); + pm_wakelock_init(&wl->wakelock, name, PM_IDLE_DOMAIN, PM_NORMAL); + list_add_tail(&priv->list, &wl->node); + } + + rpmsginfo("acquire wake lock: %s\n", name); + pm_wakelock_stay(&wl->wakelock); + } + else + { + if (wl != NULL) + { + rpmsginfo("release wake lock: %s\n", name); + pm_wakelock_relax(&wl->wakelock); + if (pm_wakelock_staycount(&wl->wakelock) == 0) + { + rpmsginfo("wakelock destroyed: %s\n", name); + list_delete(&wl->node); + pm_wakelock_uninit(&wl->wakelock); + kmm_free(wl); + } + } + } + + nxmutex_unlock(&priv->lock); + return 0; +} + +/**************************************************************************** + * Name: rpmsg_wakelock_acquire_req_handler + * + * Description: + * rpmsg-wakelock acquire req handler. + * + * Parameters: + * ept - The rpmsg endpoint + * data - The return message + * len - The return message length + * + * Returned Values: + * Always OK + * + ****************************************************************************/ + +static int rpmsg_wakelock_acquire_req_handler(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, + uint32_t src, FAR void *priv_) +{ + FAR struct rpmsg_wakelock_s *priv = priv_; + FAR struct rpmsg_wakelock_msg_s *msg = data; + FAR const char *name = (FAR const char *)msg->data; + + msg->err = rpmsg_wakelock_control(priv, name, true); + return 0; +} + +/**************************************************************************** + * Name: rpmsg_wakelock_release_req_handler + * + * Description: + * rpmsg-wakelock release req handler. + * + * Parameters: + * ept - The rpmsg endpoint + * data - The return message + * len - The return message length + * + * Returned Values: + * Always OK + * + ****************************************************************************/ + +static int rpmsg_wakelock_release_req_handler(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, + uint32_t src, FAR void *priv_) +{ + FAR struct rpmsg_wakelock_s *priv = ept->priv; + FAR struct rpmsg_wakelock_msg_s *msg = data; + FAR const char *name = (FAR const char *)msg->data; + + msg->err = rpmsg_wakelock_control(priv, name, false); + return 0; +} + +/**************************************************************************** + * Name: rpmsg_wakelock_ept_cb + * + * Description: + * Rpmsg device end point callback function, this function will be called + * when receive the remote cpu message. + * + * Parameters: + * ept - The rpmsg-device end point + * data - The received data + * len - The received data length + * src - The source address of the message + * priv - Private data of the endpoint + * + * Returned Values: + * OK on success; A negated errno value is returned on any failure. + * + ****************************************************************************/ + +static int rpmsg_wakelock_ept_cb(FAR struct rpmsg_endpoint *ept, + FAR void *data, size_t len, uint32_t src, + FAR void *priv) +{ + FAR struct rpmsg_wakelock_msg_s *msg = data; + uint32_t command = msg->cmd; + + if (command < nitems(g_rpmsg_wakelock_handler)) Review Comment: This accepts `command` 0, but `g_rpmsg_wakelock_handler[0]` is NULL, and `len` is not checked before `msg->cmd` and the name are read. Fix: return -EINVAL when `len < sizeof(*msg)` or the handler is NULL, and check that the name ends with a NUL within `len`. -- 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]
