[PATCH v3 net 0/4] pds_core: updates and fixes

[PATCH v3 net 0/4] pds_core: updates and fixes

[Shannon Nelson]

This patchset has fixes for issues seen in recent internal testing
of error conditions and stress handling.

Note that the first patch in this series is a leftover from an
earlier patchset that was abandoned:
Link: https://lore.kernel.org/netdev/20250129004337.36898-2-shannon.nelson@amd.com/

v3:
 - dropped the kdoc complaint fix, save for later net-next update

v2:
https://lore.kernel.org/netdev/20250411003209.44053-1-shannon.nelson@amd.com/
 - dropped patch 5/6 "adminq poll interval", will send for net-next later
 - updated commit comments and tags

v1:
https://lore.kernel.org/netdev/20250407225113.51850-1-shannon.nelson@amd.com/

Brett Creeley (3):
  pds_core: Prevent possible adminq overflow/stuck condition
  pds_core: handle unsupported PDS_CORE_CMD_FW_CONTROL result
  pds_core: Remove unnecessary check in pds_client_adminq_cmd()

Shannon Nelson (1):
  pds_core: make wait_context part of q_info

 drivers/net/ethernet/amd/pds_core/adminq.c  | 23 +++++++--------------
 drivers/net/ethernet/amd/pds_core/auxbus.c  |  3 ---
 drivers/net/ethernet/amd/pds_core/core.c    |  5 +----
 drivers/net/ethernet/amd/pds_core/core.h    |  9 ++++++--
 drivers/net/ethernet/amd/pds_core/devlink.c |  4 +---
 5 files changed, 16 insertions(+), 28 deletions(-)

-- 
2.17.1

[PATCH v3 net 1/4] pds_core: Prevent possible adminq overflow/stuck condition

[Shannon Nelson]

From: Brett Creeley <brett.creeley@amd.com>

The pds_core's adminq is protected by the adminq_lock, which prevents
more than 1 command to be posted onto it at any one time. This makes it
so the client drivers cannot simultaneously post adminq commands.
However, the completions happen in a different context, which means
multiple adminq commands can be posted sequentially and all waiting
on completion.

On the FW side, the backing adminq request queue is only 16 entries
long and the retry mechanism and/or overflow/stuck prevention is
lacking. This can cause the adminq to get stuck, so commands are no
longer processed and completions are no longer sent by the FW.

As an initial fix, prevent more than 16 outstanding adminq commands so
there's no way to cause the adminq from getting stuck. This works
because the backing adminq request queue will never have more than 16
pending adminq commands, so it will never overflow. This is done by
reducing the adminq depth to 16.

Fixes: 45d76f492938 ("pds_core: set up device and adminq")
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/amd/pds_core/core.c | 5 +----
 drivers/net/ethernet/amd/pds_core/core.h | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
index 1eb0d92786f7..55163457f12b 100644
--- a/drivers/net/ethernet/amd/pds_core/core.c
+++ b/drivers/net/ethernet/amd/pds_core/core.c
@@ -325,10 +325,7 @@ static int pdsc_core_init(struct pdsc *pdsc)
 	size_t sz;
 	int err;
 
-	/* Scale the descriptor ring length based on number of CPUs and VFs */
-	numdescs = max_t(int, PDSC_ADMINQ_MIN_LENGTH, num_online_cpus());
-	numdescs += 2 * pci_sriov_get_totalvfs(pdsc->pdev);
-	numdescs = roundup_pow_of_two(numdescs);
+	numdescs = PDSC_ADMINQ_MAX_LENGTH;
 	err = pdsc_qcq_alloc(pdsc, PDS_CORE_QTYPE_ADMINQ, 0, "adminq",
 			     PDS_CORE_QCQ_F_CORE | PDS_CORE_QCQ_F_INTR,
 			     numdescs,
diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h
index 0bf320c43083..199473112c29 100644
--- a/drivers/net/ethernet/amd/pds_core/core.h
+++ b/drivers/net/ethernet/amd/pds_core/core.h
@@ -16,7 +16,7 @@
 
 #define PDSC_WATCHDOG_SECS	5
 #define PDSC_QUEUE_NAME_MAX_SZ  16
-#define PDSC_ADMINQ_MIN_LENGTH	16	/* must be a power of two */
+#define PDSC_ADMINQ_MAX_LENGTH	16	/* must be a power of two */
 #define PDSC_NOTIFYQ_LENGTH	64	/* must be a power of two */
 #define PDSC_TEARDOWN_RECOVERY	false
 #define PDSC_TEARDOWN_REMOVING	true
-- 
2.17.1

Re: [PATCH v3 net 1/4] pds_core: Prevent possible adminq overflow/stuck condition

[Jacob Keller]

On 4/15/2025 4:29 PM, Shannon Nelson wrote:
> From: Brett Creeley <brett.creeley@amd.com>
> 
> The pds_core's adminq is protected by the adminq_lock, which prevents
> more than 1 command to be posted onto it at any one time. This makes it
> so the client drivers cannot simultaneously post adminq commands.
> However, the completions happen in a different context, which means
> multiple adminq commands can be posted sequentially and all waiting
> on completion.
> 
> On the FW side, the backing adminq request queue is only 16 entries
> long and the retry mechanism and/or overflow/stuck prevention is
> lacking. This can cause the adminq to get stuck, so commands are no
> longer processed and completions are no longer sent by the FW.
> 
> As an initial fix, prevent more than 16 outstanding adminq commands so
> there's no way to cause the adminq from getting stuck. This works
> because the backing adminq request queue will never have more than 16
> pending adminq commands, so it will never overflow. This is done by
> reducing the adminq depth to 16.
> 

What happens if a client driver tries to enqueue a request when the
adminq is full? Does it just block until there is space, presumably
holding the adminq_lock the entire time to prevent someone else from
inserting?

Re: [PATCH v3 net 1/4] pds_core: Prevent possible adminq overflow/stuck condition

[Nelson, Shannon]

On 4/16/2025 1:13 PM, Jacob Keller wrote:
> 
> On 4/15/2025 4:29 PM, Shannon Nelson wrote:
>> From: Brett Creeley <brett.creeley@amd.com>
>>
>> The pds_core's adminq is protected by the adminq_lock, which prevents
>> more than 1 command to be posted onto it at any one time. This makes it
>> so the client drivers cannot simultaneously post adminq commands.
>> However, the completions happen in a different context, which means
>> multiple adminq commands can be posted sequentially and all waiting
>> on completion.
>>
>> On the FW side, the backing adminq request queue is only 16 entries
>> long and the retry mechanism and/or overflow/stuck prevention is
>> lacking. This can cause the adminq to get stuck, so commands are no
>> longer processed and completions are no longer sent by the FW.
>>
>> As an initial fix, prevent more than 16 outstanding adminq commands so
>> there's no way to cause the adminq from getting stuck. This works
>> because the backing adminq request queue will never have more than 16
>> pending adminq commands, so it will never overflow. This is done by
>> reducing the adminq depth to 16.
>>
> 
> What happens if a client driver tries to enqueue a request when the
> adminq is full? Does it just block until there is space, presumably
> holding the adminq_lock the entire time to prevent someone else from
> inserting?

Right now we will return -ENOSPC and it is up to the client to decide 
whether or not it wants to do a retry.

We have another patch that has pdsc_adminq_post() doing a limited retry 
loop which was part of the original posting [1], but Kuba suggested 
using a semaphore instead.  That sent us down a redesign branch that we 
haven't been able to spend time on.  We'd like to have kept the retry 
loop patch until then to at least mitigate the situation, but the 
discussion got dropped.

sln

[1] 
https://lore.kernel.org/netdev/20250129004337.36898-3-shannon.nelson@amd.com/

Re: [PATCH v3 net 1/4] pds_core: Prevent possible adminq overflow/stuck condition

[Jacob Keller]

On 4/16/2025 1:49 PM, Nelson, Shannon wrote:
> On 4/16/2025 1:13 PM, Jacob Keller wrote:
>>
>> On 4/15/2025 4:29 PM, Shannon Nelson wrote:
>>> From: Brett Creeley <brett.creeley@amd.com>
>>>
>>> The pds_core's adminq is protected by the adminq_lock, which prevents
>>> more than 1 command to be posted onto it at any one time. This makes it
>>> so the client drivers cannot simultaneously post adminq commands.
>>> However, the completions happen in a different context, which means
>>> multiple adminq commands can be posted sequentially and all waiting
>>> on completion.
>>>
>>> On the FW side, the backing adminq request queue is only 16 entries
>>> long and the retry mechanism and/or overflow/stuck prevention is
>>> lacking. This can cause the adminq to get stuck, so commands are no
>>> longer processed and completions are no longer sent by the FW.
>>>
>>> As an initial fix, prevent more than 16 outstanding adminq commands so
>>> there's no way to cause the adminq from getting stuck. This works
>>> because the backing adminq request queue will never have more than 16
>>> pending adminq commands, so it will never overflow. This is done by
>>> reducing the adminq depth to 16.
>>>
>>
>> What happens if a client driver tries to enqueue a request when the
>> adminq is full? Does it just block until there is space, presumably
>> holding the adminq_lock the entire time to prevent someone else from
>> inserting?
> 
> Right now we will return -ENOSPC and it is up to the client to decide 
> whether or not it wants to do a retry.
> 
> We have another patch that has pdsc_adminq_post() doing a limited retry 
> loop which was part of the original posting [1], but Kuba suggested 
> using a semaphore instead.  That sent us down a redesign branch that we 
> haven't been able to spend time on.  We'd like to have kept the retry 
> loop patch until then to at least mitigate the situation, but the 
> discussion got dropped.

Sure. This fix makes sense in that context.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> 
> sln
> 
> [1] 
> https://lore.kernel.org/netdev/20250129004337.36898-3-shannon.nelson@amd.com/

[PATCH v3 net 2/4] pds_core: handle unsupported PDS_CORE_CMD_FW_CONTROL result

[Shannon Nelson]

From: Brett Creeley <brett.creeley@amd.com>

If the FW doesn't support the PDS_CORE_CMD_FW_CONTROL command
the driver might at the least print garbage and at the worst
crash when the user runs the "devlink dev info" devlink command.

This happens because the stack variable fw_list is not 0
initialized which results in fw_list.num_fw_slots being a
garbage value from the stack.  Then the driver tries to access
fw_list.fw_names[i] with i >= ARRAY_SIZE and runs off the end
of the array.

Fix this by initializing the fw_list and by not failing
completely if the devcmd fails because other useful information
is printed via devlink dev info even if the devcmd fails.

Fixes: 45d76f492938 ("pds_core: set up device and adminq")
Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/amd/pds_core/devlink.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/devlink.c b/drivers/net/ethernet/amd/pds_core/devlink.c
index c5c787df61a4..d8dc39da4161 100644
--- a/drivers/net/ethernet/amd/pds_core/devlink.c
+++ b/drivers/net/ethernet/amd/pds_core/devlink.c
@@ -105,7 +105,7 @@ int pdsc_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
 		.fw_control.opcode = PDS_CORE_CMD_FW_CONTROL,
 		.fw_control.oper = PDS_CORE_FW_GET_LIST,
 	};
-	struct pds_core_fw_list_info fw_list;
+	struct pds_core_fw_list_info fw_list = {};
 	struct pdsc *pdsc = devlink_priv(dl);
 	union pds_core_dev_comp comp;
 	char buf[32];
@@ -118,8 +118,6 @@ int pdsc_dl_info_get(struct devlink *dl, struct devlink_info_req *req,
 	if (!err)
 		memcpy_fromio(&fw_list, pdsc->cmd_regs->data, sizeof(fw_list));
 	mutex_unlock(&pdsc->devcmd_lock);
-	if (err && err != -EIO)
-		return err;
 
 	listlen = min(fw_list.num_fw_slots, ARRAY_SIZE(fw_list.fw_names));
 	for (i = 0; i < listlen; i++) {
-- 
2.17.1

Re: [PATCH v3 net 2/4] pds_core: handle unsupported PDS_CORE_CMD_FW_CONTROL result

[Jacob Keller]

On 4/15/2025 4:29 PM, Shannon Nelson wrote:
> From: Brett Creeley <brett.creeley@amd.com>
> 
> If the FW doesn't support the PDS_CORE_CMD_FW_CONTROL command
> the driver might at the least print garbage and at the worst
> crash when the user runs the "devlink dev info" devlink command.
> 
> This happens because the stack variable fw_list is not 0
> initialized which results in fw_list.num_fw_slots being a
> garbage value from the stack.  Then the driver tries to access
> fw_list.fw_names[i] with i >= ARRAY_SIZE and runs off the end
> of the array.
> 
> Fix this by initializing the fw_list and by not failing
> completely if the devcmd fails because other useful information
> is printed via devlink dev info even if the devcmd fails.
> 
> Fixes: 45d76f492938 ("pds_core: set up device and adminq")
> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

[PATCH v3 net 3/4] pds_core: Remove unnecessary check in pds_client_adminq_cmd()

[Shannon Nelson]

From: Brett Creeley <brett.creeley@amd.com>

When the pds_core driver was first created there were some race
conditions around using the adminq, especially for client drivers.
To reduce the possibility of a race condition there's a check
against pf->state in pds_client_adminq_cmd(). This is problematic
for a couple of reasons:

1. The PDSC_S_INITING_DRIVER bit is set during probe, but not
   cleared until after everything in probe is complete, which
   includes creating the auxiliary devices. For pds_fwctl this
   means it can't make any adminq commands until after pds_core's
   probe is complete even though the adminq is fully up by the
   time pds_fwctl's auxiliary device is created.

2. The race conditions around using the adminq have been fixed
   and this path is already protected against client drivers
   calling pds_client_adminq_cmd() if the adminq isn't ready,
   i.e. see pdsc_adminq_post() -> pdsc_adminq_inc_if_up().

Fix this by removing the pf->state check in pds_client_adminq_cmd()
because invalid accesses to pds_core's adminq is already handled by
pdsc_adminq_post()->pdsc_adminq_inc_if_up().

Fixes: 10659034c622 ("pds_core: add the aux client API")
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/amd/pds_core/auxbus.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
index eeb72b1809ea..c9aac27883a3 100644
--- a/drivers/net/ethernet/amd/pds_core/auxbus.c
+++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
@@ -107,9 +107,6 @@ int pds_client_adminq_cmd(struct pds_auxiliary_dev *padev,
 	dev_dbg(pf->dev, "%s: %s opcode %d\n",
 		__func__, dev_name(&padev->aux_dev.dev), req->opcode);
 
-	if (pf->state)
-		return -ENXIO;
-
 	/* Wrap the client's request */
 	cmd.client_request.opcode = PDS_AQ_CMD_CLIENT_CMD;
 	cmd.client_request.client_id = cpu_to_le16(padev->client_id);
-- 
2.17.1

Re: [PATCH v3 net 3/4] pds_core: Remove unnecessary check in pds_client_adminq_cmd()

[Jacob Keller]

On 4/15/2025 4:29 PM, Shannon Nelson wrote:
> From: Brett Creeley <brett.creeley@amd.com>
> 
> When the pds_core driver was first created there were some race
> conditions around using the adminq, especially for client drivers.
> To reduce the possibility of a race condition there's a check
> against pf->state in pds_client_adminq_cmd(). This is problematic
> for a couple of reasons:
> 
> 1. The PDSC_S_INITING_DRIVER bit is set during probe, but not
>    cleared until after everything in probe is complete, which
>    includes creating the auxiliary devices. For pds_fwctl this
>    means it can't make any adminq commands until after pds_core's
>    probe is complete even though the adminq is fully up by the
>    time pds_fwctl's auxiliary device is created.
> 
> 2. The race conditions around using the adminq have been fixed
>    and this path is already protected against client drivers
>    calling pds_client_adminq_cmd() if the adminq isn't ready,
>    i.e. see pdsc_adminq_post() -> pdsc_adminq_inc_if_up().
> 
> Fix this by removing the pf->state check in pds_client_adminq_cmd()
> because invalid accesses to pds_core's adminq is already handled by
> pdsc_adminq_post()->pdsc_adminq_inc_if_up().
> 

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

[PATCH v3 net 4/4] pds_core: make wait_context part of q_info

[Shannon Nelson]

Make the wait_context a full part of the q_info struct rather
than a stack variable that goes away after pdsc_adminq_post()
is done so that the context is still available after the wait
loop has given up.

There was a case where a slow development firmware caused
the adminq request to time out, but then later the FW finally
finished the request and sent the interrupt.  The handler tried
to complete_all() the completion context that had been created
on the stack in pdsc_adminq_post() but no longer existed.
This caused bad pointer usage, kernel crashes, and much wailing
and gnashing of teeth.

Fixes: 01ba61b55b20 ("pds_core: Add adminq processing and commands")
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/amd/pds_core/adminq.c | 23 +++++++---------------
 drivers/net/ethernet/amd/pds_core/core.h   |  7 ++++++-
 2 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/adminq.c b/drivers/net/ethernet/amd/pds_core/adminq.c
index c83a0a80d533..9bc246a4a9d8 100644
--- a/drivers/net/ethernet/amd/pds_core/adminq.c
+++ b/drivers/net/ethernet/amd/pds_core/adminq.c
@@ -5,11 +5,6 @@
 
 #include "core.h"
 
-struct pdsc_wait_context {
-	struct pdsc_qcq *qcq;
-	struct completion wait_completion;
-};
-
 static int pdsc_process_notifyq(struct pdsc_qcq *qcq)
 {
 	union pds_core_notifyq_comp *comp;
@@ -112,7 +107,7 @@ void pdsc_process_adminq(struct pdsc_qcq *qcq)
 		/* Copy out the completion data */
 		memcpy(q_info->dest, comp, sizeof(*comp));
 
-		complete_all(&q_info->wc->wait_completion);
+		complete_all(&q_info->wc.wait_completion);
 
 		if (cq->tail_idx == cq->num_descs - 1)
 			cq->done_color = !cq->done_color;
@@ -162,8 +157,7 @@ irqreturn_t pdsc_adminq_isr(int irq, void *data)
 static int __pdsc_adminq_post(struct pdsc *pdsc,
 			      struct pdsc_qcq *qcq,
 			      union pds_core_adminq_cmd *cmd,
-			      union pds_core_adminq_comp *comp,
-			      struct pdsc_wait_context *wc)
+			      union pds_core_adminq_comp *comp)
 {
 	struct pdsc_queue *q = &qcq->q;
 	struct pdsc_q_info *q_info;
@@ -205,7 +199,6 @@ static int __pdsc_adminq_post(struct pdsc *pdsc,
 	/* Post the request */
 	index = q->head_idx;
 	q_info = &q->info[index];
-	q_info->wc = wc;
 	q_info->dest = comp;
 	memcpy(q_info->desc, cmd, sizeof(*cmd));
 
@@ -231,11 +224,8 @@ int pdsc_adminq_post(struct pdsc *pdsc,
 		     union pds_core_adminq_comp *comp,
 		     bool fast_poll)
 {
-	struct pdsc_wait_context wc = {
-		.wait_completion =
-			COMPLETION_INITIALIZER_ONSTACK(wc.wait_completion),
-	};
 	unsigned long poll_interval = 1;
+	struct pdsc_wait_context *wc;
 	unsigned long poll_jiffies;
 	unsigned long time_limit;
 	unsigned long time_start;
@@ -250,19 +240,20 @@ int pdsc_adminq_post(struct pdsc *pdsc,
 		return -ENXIO;
 	}
 
-	wc.qcq = &pdsc->adminqcq;
-	index = __pdsc_adminq_post(pdsc, &pdsc->adminqcq, cmd, comp, &wc);
+	index = __pdsc_adminq_post(pdsc, &pdsc->adminqcq, cmd, comp);
 	if (index < 0) {
 		err = index;
 		goto err_out;
 	}
 
+	wc = &pdsc->adminqcq.q.info[index].wc;
+	wc->wait_completion = COMPLETION_INITIALIZER_ONSTACK(wc->wait_completion);
 	time_start = jiffies;
 	time_limit = time_start + HZ * pdsc->devcmd_timeout;
 	do {
 		/* Timeslice the actual wait to catch IO errors etc early */
 		poll_jiffies = msecs_to_jiffies(poll_interval);
-		remaining = wait_for_completion_timeout(&wc.wait_completion,
+		remaining = wait_for_completion_timeout(&wc->wait_completion,
 							poll_jiffies);
 		if (remaining)
 			break;
diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h
index 199473112c29..84fd814d7904 100644
--- a/drivers/net/ethernet/amd/pds_core/core.h
+++ b/drivers/net/ethernet/amd/pds_core/core.h
@@ -88,6 +88,11 @@ struct pdsc_buf_info {
 	u32 len;
 };
 
+struct pdsc_wait_context {
+	struct pdsc_qcq *qcq;
+	struct completion wait_completion;
+};
+
 struct pdsc_q_info {
 	union {
 		void *desc;
@@ -96,7 +101,7 @@ struct pdsc_q_info {
 	unsigned int bytes;
 	unsigned int nbufs;
 	struct pdsc_buf_info bufs[PDS_CORE_MAX_FRAGS];
-	struct pdsc_wait_context *wc;
+	struct pdsc_wait_context wc;
 	void *dest;
 };
 
-- 
2.17.1

Re: [PATCH v3 net 4/4] pds_core: make wait_context part of q_info

[Jacob Keller]

On 4/15/2025 4:29 PM, Shannon Nelson wrote:
> Make the wait_context a full part of the q_info struct rather
> than a stack variable that goes away after pdsc_adminq_post()
> is done so that the context is still available after the wait
> loop has given up.
> 
> There was a case where a slow development firmware caused
> the adminq request to time out, but then later the FW finally
> finished the request and sent the interrupt.  The handler tried
> to complete_all() the completion context that had been created
> on the stack in pdsc_adminq_post() but no longer existed.
> This caused bad pointer usage, kernel crashes, and much wailing
> and gnashing of teeth.

Ugh, I can imagine that was hard to track down..

> 
> Fixes: 01ba61b55b20 ("pds_core: Add adminq processing and commands")
> Reviewed-by: Simon Horman <horms@kernel.org>
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
Yea, this approach seems a bit cleaner overall too!

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH v3 net 4/4] pds_core: make wait_context part of q_info

[Jakub Kicinski]

On Tue, 15 Apr 2025 16:29:31 -0700 Shannon Nelson wrote:
> Make the wait_context a full part of the q_info struct rather
> than a stack variable that goes away after pdsc_adminq_post()
> is done so that the context is still available after the wait
> loop has given up.
> 
> There was a case where a slow development firmware caused
> the adminq request to time out, but then later the FW finally
> finished the request and sent the interrupt.  The handler tried
> to complete_all() the completion context that had been created
> on the stack in pdsc_adminq_post() but no longer existed.
> This caused bad pointer usage, kernel crashes, and much wailing
> and gnashing of teeth.

The patch will certainly redirect the access from the stack.
But since you're already processing the completions under a spin
lock, is it not possible to safely invalidate the completion
under the same lock on timeout?

Perhaps not I haven't looked very closely.

> +	wc = &pdsc->adminqcq.q.info[index].wc;
> +	wc->wait_completion = COMPLETION_INITIALIZER_ONSTACK(wc->wait_completion);

_ONSTACK you say? I don't think it's on the stack any more.

Re: [PATCH v3 net 4/4] pds_core: make wait_context part of q_info

[Nelson, Shannon]

On 4/17/2025 8:21 AM, Jakub Kicinski wrote:
> 
> On Tue, 15 Apr 2025 16:29:31 -0700 Shannon Nelson wrote:
>> Make the wait_context a full part of the q_info struct rather
>> than a stack variable that goes away after pdsc_adminq_post()
>> is done so that the context is still available after the wait
>> loop has given up.
>>
>> There was a case where a slow development firmware caused
>> the adminq request to time out, but then later the FW finally
>> finished the request and sent the interrupt.  The handler tried
>> to complete_all() the completion context that had been created
>> on the stack in pdsc_adminq_post() but no longer existed.
>> This caused bad pointer usage, kernel crashes, and much wailing
>> and gnashing of teeth.
> 
> The patch will certainly redirect the access from the stack.
> But since you're already processing the completions under a spin
> lock, is it not possible to safely invalidate the completion
> under the same lock on timeout?
> 
> Perhaps not I haven't looked very closely.

We have another patch under consideration that does something like this, 
but we're not sure we're happy with that patch yet, so haven't pushed it 
out yet.

> 
>> +     wc = &pdsc->adminqcq.q.info[index].wc;
>> +     wc->wait_completion = COMPLETION_INITIALIZER_ONSTACK(wc->wait_completion);
> 
> _ONSTACK you say? I don't think it's on the stack any more.

This worked, but digging a little further through other examples I see 
how we can more correctly use init_completion() and reinit_completion().

sln