xiaoxiang781216 commented on code in PR #20130:
URL: https://github.com/apache/nuttx/pull/20130#discussion_r4032799268
##########
include/nuttx/fdpic.h:
##########
@@ -100,29 +100,32 @@ static inline FAR void *fdpic_callback(FAR void *fn)
* Name: fdpic_invoke
*
* Description:
- * Call a resolved module entry point with the module data base in the PIC
- * base register. For a callback that runs on a shared thread, which
- * carries no module base. Elsewhere fdpic_callback() is enough.
+ * Call a function pointer as it was received, with the data base it names
+ * in the PIC base register. For a callback that runs on a shared thread,
+ * which carries no module base. Elsewhere fdpic_callback() is enough.
+ *
+ * A function pointer is the address of a descriptor, so the entry point
+ * and the data base both come from it.
*
* Input Parameters:
- * arg - The one word argument.
- * entry - The code address to enter, already resolved from the descriptor.
- * got - The module data base to install.
+ * arg - The one word argument.
+ * fn - The callback, as the caller received it.
*
****************************************************************************/
-static inline void fdpic_invoke(uintptr_t arg, uintptr_t entry,
- uintptr_t got)
+static inline void fdpic_invoke(uintptr_t arg, FAR void *fn)
Review Comment:
could we use `struct fdpic_desc_s *`
##########
sched/signal/sig_notification.c:
##########
@@ -70,7 +74,23 @@ static void nxsig_notification_worker(FAR void *arg)
/* Perform the callback */
+#ifdef CONFIG_FDPIC
+ /* The worker does not carry the module's data base. Install the base
+ * captured at registration around the call. A zero base means the
+ * callback is not a module's.
+ */
+
+ if (work->desc.got != 0)
Review Comment:
why not move the check into fdpic_invoke
##########
sched/signal/sig_notification.c:
##########
@@ -155,7 +175,11 @@ int nxsig_notification(pid_t pid, FAR struct sigevent
*event,
/* Initialize the work information */
work->value = event->sigev_value;
+#ifdef CONFIG_FDPIC
+ work->desc.entry = (uintptr_t)event->sigev_notify_function;
Review Comment:
let's call function suggestion by
https://github.com/apache/nuttx/pull/20130/changes/23345fb7815b563cccdea4ea58799396dbc3fda2#r4029482478
##########
sched/timer/timer_create.c:
##########
@@ -196,6 +200,30 @@ int timer_create(clockid_t clockid, FAR struct sigevent
*evp,
/* Yes, copy the entire struct sigevent content */
memcpy(&ret->pt_event, evp, sizeof(struct sigevent));
+
+#if defined(CONFIG_FDPIC) && defined(CONFIG_SIG_EVTHREAD)
+ /* Resolve the callback here, where this still runs in the
+ * module's context. It fires later on a worker that carries
+ * no data base, so the base travels with it. The descriptor
+ * holds the base of the module the callback belongs to,
+ * which is not always the caller's. The function shares a
+ * union with the thread ID, so only a SIGEV_THREAD event has
+ * one to resolve.
+ */
+
+ ret->pt_work.desc.got = 0;
+
+ if ((evp->sigev_notify & SIGEV_THREAD) != 0 &&
+ fdpic_base() != 0)
+ {
+ FAR struct fdpic_desc_s *desc =
Review Comment:
can we add a new function which initialize desc with the function pointer
and got base with fdpic_base()
##########
include/nuttx/signal.h:
##########
@@ -67,7 +70,14 @@ struct sigwork_s
{
struct work_s work; /* Work queue structure */
union sigval value; /* Data passed with notification */
+#ifdef CONFIG_FDPIC
+ struct fdpic_desc_s desc; /* Notification function, and the data base
Review Comment:
change to func
##########
sched/mqueue/mq_notify.c:
##########
@@ -156,6 +160,29 @@ int mq_notify(mqd_t mqdes, FAR const struct sigevent
*notification)
sizeof(struct sigevent));
msgq->ntpid = rtcb->pid;
+
+#if defined(CONFIG_FDPIC) && defined(CONFIG_SIG_EVTHREAD)
+ /* Resolve the callback here, where this still runs in the
+ * module's context. It fires later on a worker that carries no
+ * data base, so the base travels with it. The descriptor holds
+ * the base of the module the callback belongs to, which is not
+ * always the caller's. The function shares a union with the
+ * thread ID, so only a SIGEV_THREAD event has one to resolve.
+ */
+
+ msgq->ntwork.desc.got = 0;
+
+ if ((notification->sigev_notify & SIGEV_THREAD) != 0 &&
Review Comment:
let's call function suggested at
https://github.com/apache/nuttx/pull/20130/changes/23345fb7815b563cccdea4ea58799396dbc3fda2#r4029482478
--
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]
