* [PATCH net v2 1/5] ionic: Allow the first devcmd to trigger deferred probe
2026-05-06 3:57 [PATCH net v2 0/5] ionic: Various bugfixes Eric Joyner
@ 2026-05-06 3:57 ` Eric Joyner
2026-05-07 15:57 ` Jakub Kicinski
2026-05-07 15:59 ` Jakub Kicinski
2026-05-06 3:57 ` [PATCH net v2 2/5] ionic: Handle failures from ionic_reset() when relevant Eric Joyner
` (3 subsequent siblings)
4 siblings, 2 replies; 10+ messages in thread
From: Eric Joyner @ 2026-05-06 3:57 UTC (permalink / raw)
To: netdev
Cc: Brett Creeley, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Eric Joyner
From: Brett Creeley <brett.creeley@amd.com>
There's a chance the register signature value is set before the
firmware is ready to respond to the driver. This doesn't mean the
device isn't there, but just means it's not yet ready. If the first
devcmd fails, then return -EPROBE_DEFER so the device can be probed
at a later time. As part of this make sure the reset devcmd, which
is the first devcmd, is not so alarming when it fails by printing
an information message instead of the standard devcmd failure
messages.
Note, that the ionic_reset() function could be reworked a bit to
keep retrying if err is -EAGAIN or -ETIMEDOUT, but for now I chose
not to change this as 5 seconds should be enough for the firmware
to come up after the devcmd registers are initialized.
Fixes: fbfb8031533c ("ionic: Add hardware init and device commands")
Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Eric Joyner <eric.joyner@amd.com>
---
drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c | 8 ++++++--
drivers/net/ethernet/pensando/ionic/ionic_main.c | 8 ++++++--
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
index 05f19489ec5c..59ce35404e53 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
@@ -260,7 +260,8 @@ static int ionic_setup_one(struct ionic *ionic)
/* Configure the device */
err = ionic_setup(ionic);
if (err) {
- dev_err(dev, "Cannot setup device: %d, aborting\n", err);
+ if (err != -EPROBE_DEFER)
+ dev_err(dev, "Cannot setup device: %d, aborting\n", err);
goto err_out_clear_pci;
}
pci_set_master(pdev);
@@ -335,8 +336,11 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
#endif
err = ionic_setup_one(ionic);
- if (err)
+ if (err) {
+ if (err == -EPROBE_DEFER)
+ dev_info(dev, "Device isn't ready, deferring probe\n");
goto err_out;
+ }
/* Allocate and init the LIF */
err = ionic_lif_size(ionic);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
index 3c5200e2fdb7..91f89b9ff807 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
@@ -603,7 +603,11 @@ int ionic_setup(struct ionic *ionic)
err = ionic_dev_setup(ionic);
if (err)
return err;
- ionic_reset(ionic);
+
+ err = ionic_reset(ionic);
+ /* firmware may not be ready to respond yet */
+ if (err == -EAGAIN || err == -ETIMEDOUT)
+ return -EPROBE_DEFER;
return 0;
}
@@ -687,7 +691,7 @@ int ionic_reset(struct ionic *ionic)
mutex_lock(&ionic->dev_cmd_lock);
ionic_dev_cmd_reset(idev);
- err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
+ err = ionic_dev_cmd_wait_nomsg(ionic, DEVCMD_TIMEOUT);
mutex_unlock(&ionic->dev_cmd_lock);
return err;
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH net v2 1/5] ionic: Allow the first devcmd to trigger deferred probe
2026-05-06 3:57 ` [PATCH net v2 1/5] ionic: Allow the first devcmd to trigger deferred probe Eric Joyner
@ 2026-05-07 15:57 ` Jakub Kicinski
2026-05-07 15:59 ` Jakub Kicinski
1 sibling, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2026-05-07 15:57 UTC (permalink / raw)
To: Eric Joyner
Cc: netdev, Brett Creeley, Andrew Lunn, David S. Miller, Eric Dumazet,
Paolo Abeni
On Tue, 5 May 2026 20:57:02 -0700 Eric Joyner wrote:
> There's a chance the register signature value is set before the
> firmware is ready to respond to the driver. This doesn't mean the
> device isn't there, but just means it's not yet ready. If the first
> devcmd fails, then return -EPROBE_DEFER so the device can be probed
> at a later time. As part of this make sure the reset devcmd, which
> is the first devcmd, is not so alarming when it fails by printing
> an information message instead of the standard devcmd failure
> messages.
I don't think that's how PROBE_DEFER works / is supposed to be used.
It re-tries probe after other devices have been probed. So if your
driver is the last one "one the list" and the system is completely
stable afterwards you will never get another probe attempt.
At least that's my recollection. IOW it's for use when you have
dependencies on other components in the system. If you just need
to wait for you own device to become ready you should simply wait..
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v2 1/5] ionic: Allow the first devcmd to trigger deferred probe
2026-05-06 3:57 ` [PATCH net v2 1/5] ionic: Allow the first devcmd to trigger deferred probe Eric Joyner
2026-05-07 15:57 ` Jakub Kicinski
@ 2026-05-07 15:59 ` Jakub Kicinski
1 sibling, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2026-05-07 15:59 UTC (permalink / raw)
To: eric.joyner
Cc: Jakub Kicinski, netdev, brett.creeley, andrew+netdev, davem,
edumazet, pabeni, prabu.ponrajthayalan
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ionic: Allow the first devcmd to trigger deferred probe
There's a chance the register signature value is set before the
firmware is ready to respond to the driver. This doesn't mean the
device isn't there, but just means it's not yet ready. If the first
devcmd fails, then return -EPROBE_DEFER so the device can be probed
at a later time. As part of this make sure the reset devcmd, which
is the first devcmd, is not so alarming when it fails by printing
an information message instead of the standard devcmd failure
messages.
[ ... ]
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> index 05f19489ec5c8..59ce35404e532 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> @@ -260,7 +260,8 @@ static int ionic_setup_one(struct ionic *ionic)
> /* Configure the device */
> err = ionic_setup(ionic);
> if (err) {
> - dev_err(dev, "Cannot setup device: %d, aborting\n", err);
> + if (err != -EPROBE_DEFER)
> + dev_err(dev, "Cannot setup device: %d, aborting\n", err);
> goto err_out_clear_pci;
Does this code leak the workqueue?
ionic_setup() first calls ionic_dev_setup(), which allocates the workqueue
via ionic_watchdog_init(). If ionic_reset() then fails and returns
-EPROBE_DEFER, the goto here jumps to err_out_clear_pci.
This bypasses err_out_teardown and skips ionic_dev_teardown(), which is
responsible for calling destroy_workqueue(ionic->wq). Because -EPROBE_DEFER
will cause the kernel to retry probing, could this leak a new workqueue on
every failed probe attempt?
> }
[ ... ]
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
> index 3c5200e2fdb7d..91f89b9ff8075 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
> @@ -603,7 +603,11 @@ int ionic_setup(struct ionic *ionic)
> err = ionic_dev_setup(ionic);
> if (err)
> return err;
> - ionic_reset(ionic);
> +
> + err = ionic_reset(ionic);
> + /* firmware may not be ready to respond yet */
> + if (err == -EAGAIN || err == -ETIMEDOUT)
> + return -EPROBE_DEFER;
Will this permanently fail the probe with -ENXIO if the firmware is so early
in its boot process that the IONIC_FW_STS_F_RUNNING bit isn't set yet?
If ionic_is_fw_running() is false, ionic_reset() skips execution and
returns 0:
drivers/net/ethernet/pensando/ionic/ionic_main.c:ionic_reset() {
if (!ionic_is_fw_running(idev))
return 0;
...
}
If this happens, ionic_setup() returns success, and the driver proceeds to
ionic_identify(). When it issues a command and calls __ionic_dev_cmd_wait(),
the wait loop is skipped entirely because the firmware isn't running,
returning -ENXIO. This bypasses the -EPROBE_DEFER logic here and causes a
permanent probe failure.
>
> return 0;
> }
> @@ -687,7 +691,7 @@ int ionic_reset(struct ionic *ionic)
>
> mutex_lock(&ionic->dev_cmd_lock);
> ionic_dev_cmd_reset(idev);
> - err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
> + err = ionic_dev_cmd_wait_nomsg(ionic, DEVCMD_TIMEOUT);
Does this actually suppress the timeout warning?
The commit message says this aims to make the first devcmd failure "not so
alarming" by suppressing standard devcmd failure messages. However,
ionic_dev_cmd_wait_nomsg() passes do_msg = false to __ionic_dev_cmd_wait(),
which only suppresses firmware-returned error codes parsed by
ionic_dev_cmd_dev_err_print().
If the command times out, __ionic_dev_cmd_wait() will still unconditionally
print the dev_warn() for the timeout, so the alarming message might still be
printed.
> mutex_unlock(&ionic->dev_cmd_lock);
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net v2 2/5] ionic: Handle failures from ionic_reset() when relevant
2026-05-06 3:57 [PATCH net v2 0/5] ionic: Various bugfixes Eric Joyner
2026-05-06 3:57 ` [PATCH net v2 1/5] ionic: Allow the first devcmd to trigger deferred probe Eric Joyner
@ 2026-05-06 3:57 ` Eric Joyner
2026-05-06 3:57 ` [PATCH net v2 3/5] ionic: Fix unexpected dev_cmd failures Eric Joyner
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Eric Joyner @ 2026-05-06 3:57 UTC (permalink / raw)
To: netdev
Cc: Brett Creeley, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Eric Joyner
From: Brett Creeley <brett.creeley@amd.com>
If ionic_reset() fails, then the device either wasn't ready to be
communicated with, the firmware is down, and/or the devcmd path is
already torn down. For teardown/remove cases we can ignore the
result of ionic_reset(). However, for any setup cases, we should
take the result seriously.
Note, older firmware always returns success for IONIC_CMD_RESET, so this
change will not break those. However, newer firmware may return failure
if the IONIC_CMD_RESET dev cmd fails.
Fixes: 8097a2f3d21a ("ionic: Reset LIF device while restarting LIF")
Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Eric Joyner <eric.joyner@amd.com>
---
drivers/net/ethernet/pensando/ionic/ionic_lif.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index 637e635bbf03..db4bbeda0b29 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -3473,7 +3473,9 @@ static void ionic_lif_handle_fw_up(struct ionic_lif *lif)
* just need to reanimate it.
*/
ionic_init_devinfo(ionic);
- ionic_reset(ionic);
+ err = ionic_reset(ionic);
+ if (err)
+ goto err_out;
err = ionic_identify(ionic);
if (err)
goto err_out;
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH net v2 3/5] ionic: Fix unexpected dev_cmd failures
2026-05-06 3:57 [PATCH net v2 0/5] ionic: Various bugfixes Eric Joyner
2026-05-06 3:57 ` [PATCH net v2 1/5] ionic: Allow the first devcmd to trigger deferred probe Eric Joyner
2026-05-06 3:57 ` [PATCH net v2 2/5] ionic: Handle failures from ionic_reset() when relevant Eric Joyner
@ 2026-05-06 3:57 ` Eric Joyner
2026-05-07 15:59 ` Jakub Kicinski
2026-05-06 3:57 ` [PATCH net v2 4/5] ionic: Fix check in ionic_get_link_ext_stats Eric Joyner
2026-05-06 3:57 ` [PATCH net v2 5/5] ionic: fix completion descriptor access with 2x desc size Eric Joyner
4 siblings, 1 reply; 10+ messages in thread
From: Eric Joyner @ 2026-05-06 3:57 UTC (permalink / raw)
To: netdev
Cc: Brett Creeley, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Eric Joyner
From: Brett Creeley <brett.creeley@amd.com>
When polling for a devcmd completion it's possible for the driver to
timeout the command even if the dev_cmd has completed. This can cause
unexpected failures and device probe to fail. Fix this by reading
the dev_cmd's done bit one last time after breaking out of the poll
loop.
Fixes: fbfb8031533c ("ionic: Add hardware init and device commands")
Suggested-by: Neel Patel <neel.patel@amd.com>
Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Eric Joyner <eric.joyner@amd.com>
---
drivers/net/ethernet/pensando/ionic/ionic_main.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
index 91f89b9ff807..810cef0fec93 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
@@ -541,6 +541,10 @@ static int __ionic_dev_cmd_wait(struct ionic *ionic, unsigned long max_seconds,
}
duration = jiffies - start_time;
+ /* one final check to prevent unexpected timeout */
+ if (!done)
+ done = ionic_dev_cmd_done(idev);
+
dev_dbg(ionic->dev, "DEVCMD %s (%d) done=%d took %ld secs (%ld jiffies)\n",
ionic_opcode_to_str(opcode), opcode,
done, duration / HZ, duration);
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH net v2 3/5] ionic: Fix unexpected dev_cmd failures
2026-05-06 3:57 ` [PATCH net v2 3/5] ionic: Fix unexpected dev_cmd failures Eric Joyner
@ 2026-05-07 15:59 ` Jakub Kicinski
0 siblings, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2026-05-07 15:59 UTC (permalink / raw)
To: eric.joyner
Cc: Jakub Kicinski, netdev, brett.creeley, andrew+netdev, davem,
edumazet, pabeni, prabu.ponrajthayalan
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ionic: Fix unexpected dev_cmd failures
This commit adds a final check of the done bit after exiting the dev_cmd
poll loop. It attempts to prevent timing out commands that have actually
completed.
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
> --- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
> @@ -541,6 +541,10 @@ static int __ionic_dev_cmd_wait(struct ionic *ionic, unsigned long max_seconds,
> }
> duration = jiffies - start_time;
>
> + /* one final check to prevent unexpected timeout */
> + if (!done)
> + done = ionic_dev_cmd_done(idev);
If the polling loop exits early because the firmware crashed or the device
was hot-removed, fw_up evaluates to false. Will this final check then read
from a dead device?
On a disconnected device, MMIO reads typically return all 1s (0xffffffff).
Looking at ionic_dev_cmd_done():
drivers/net/ethernet/pensando/ionic/ionic_dev.c:ionic_dev_cmd_done() {
...
return ioread32(&idev->dev_cmd_regs->done) & IONIC_DEV_CMD_DONE;
}
Since IONIC_DEV_CMD_DONE is 0x1, the bitwise AND with 0xffffffff evaluates
to true, making done = true.
If done becomes true on a dead device, does it bypass the firmware down
error path further down in this function?
__ionic_dev_cmd_wait() {
...
if (!done && !fw_up) {
ionic_dev_cmd_clean(ionic);
dev_warn(ionic->dev, "DEVCMD %s (%d) interrupted - FW is down\n",
ionic_opcode_to_str(opcode), opcode);
return -ENXIO;
}
...
}
It appears the driver will instead proceed to ionic_dev_cmd_status(), read
0xff from the status register, log an unknown error, and return -EIO rather
than correctly handling the device removal with -ENXIO.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net v2 4/5] ionic: Fix check in ionic_get_link_ext_stats
2026-05-06 3:57 [PATCH net v2 0/5] ionic: Various bugfixes Eric Joyner
` (2 preceding siblings ...)
2026-05-06 3:57 ` [PATCH net v2 3/5] ionic: Fix unexpected dev_cmd failures Eric Joyner
@ 2026-05-06 3:57 ` Eric Joyner
2026-05-06 3:57 ` [PATCH net v2 5/5] ionic: fix completion descriptor access with 2x desc size Eric Joyner
4 siblings, 0 replies; 10+ messages in thread
From: Eric Joyner @ 2026-05-06 3:57 UTC (permalink / raw)
To: netdev
Cc: Brett Creeley, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Eric Joyner
From: Brett Creeley <brett.creeley@amd.com>
The current check will fail if SR-IOV is not initialized for the
physical function; this is because is_physfn is 0 if sriov_init() isn't
run or fails. Change the check that prevents getting the link down count
to use is_virtfn instead so that VFs don't get this functionality, which
was the original intent.
Fixes: 132b4ebfa090 ("ionic: add support for ethtool extended stat link_down_count")
Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Eric Joyner <eric.joyner@amd.com>
---
drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
index 78a802eb159f..296f831a514d 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
@@ -116,8 +116,10 @@ static void ionic_get_link_ext_stats(struct net_device *netdev,
{
struct ionic_lif *lif = netdev_priv(netdev);
- if (lif->ionic->pdev->is_physfn)
- stats->link_down_events = lif->link_down_count;
+ if (lif->ionic->pdev->is_virtfn)
+ return;
+
+ stats->link_down_events = lif->link_down_count;
}
static int ionic_get_link_ksettings(struct net_device *netdev,
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH net v2 5/5] ionic: fix completion descriptor access with 2x desc size
2026-05-06 3:57 [PATCH net v2 0/5] ionic: Various bugfixes Eric Joyner
` (3 preceding siblings ...)
2026-05-06 3:57 ` [PATCH net v2 4/5] ionic: Fix check in ionic_get_link_ext_stats Eric Joyner
@ 2026-05-06 3:57 ` Eric Joyner
2026-05-07 15:59 ` Jakub Kicinski
4 siblings, 1 reply; 10+ messages in thread
From: Eric Joyner @ 2026-05-06 3:57 UTC (permalink / raw)
To: netdev
Cc: Brett Creeley, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Eric Joyner, Prabu Thayalan
From: Prabu Thayalan <prabu.ponrajthayalan@amd.com>
The old ionic_rx_service() and ionic_tx_service() used array
indexing to access completion descriptors:
comp = &((struct ionic_rxq_comp *)cq->base)[cq->tail_idx];
This assumes the stride is sizeof(struct ionic_rxq_comp) = 16 bytes.
However, when the IONIC_Q_F_2X_CQ_DESC flag is set, the actual
completion descriptor size is 32 bytes (2 * sizeof(comp)), and the
completion itself is located at the end of that 32-byte slot. Array
indexing with a 16-byte stride would access the wrong offset.
Use pointer arithmetic that accounts for the actual descriptor size
from cq->desc_size:
comp = cq->base +
cq->desc_size * cq->tail_idx +
cq->desc_size - sizeof(*comp);
This correctly calculates the completion location regardless of
descriptor size. For the common case where desc_size equals
sizeof(*comp), use array indexing in a likely() fast path to avoid
performance regression.
Fixes: 0ec9f6669a7d ("ionic: add handling of larger descriptors")
Signed-off-by: Prabu Thayalan <prabu.ponrajthayalan@amd.com>
Signed-off-by: Eric Joyner <eric.joyner@amd.com>
---
.../net/ethernet/pensando/ionic/ionic_txrx.c | 27 ++++++++++---------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
index 301ebee2fdc5..27a113d63d28 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
@@ -701,11 +701,7 @@ static void ionic_rx_clean(struct ionic_queue *q,
__le64 *cq_desc_hwstamp;
u64 hwstamp;
- cq_desc_hwstamp =
- (void *)comp +
- qcq->cq.desc_size -
- sizeof(struct ionic_rxq_comp) -
- IONIC_HWSTAMP_CQ_NEGOFFSET;
+ cq_desc_hwstamp = (void *)comp - IONIC_HWSTAMP_CQ_NEGOFFSET;
hwstamp = le64_to_cpu(*cq_desc_hwstamp);
@@ -729,7 +725,12 @@ static bool __ionic_rx_service(struct ionic_cq *cq, struct bpf_prog *xdp_prog)
struct ionic_queue *q = cq->bound_q;
struct ionic_rxq_comp *comp;
- comp = &((struct ionic_rxq_comp *)cq->base)[cq->tail_idx];
+ if (likely(cq->desc_size == sizeof(*comp)))
+ comp = &((struct ionic_rxq_comp *)cq->base)[cq->tail_idx];
+ else
+ comp = cq->base +
+ cq->desc_size * cq->tail_idx +
+ cq->desc_size - sizeof(*comp);
if (!color_match(comp->pkt_type_color, cq->done_color))
return false;
@@ -1180,7 +1181,6 @@ static void ionic_tx_clean(struct ionic_queue *q,
bool in_napi)
{
struct ionic_tx_stats *stats = q_to_tx_stats(q);
- struct ionic_qcq *qcq = q_to_qcq(q);
struct sk_buff *skb;
if (desc_info->xdpf) {
@@ -1205,11 +1205,7 @@ static void ionic_tx_clean(struct ionic_queue *q,
__le64 *cq_desc_hwstamp;
u64 hwstamp;
- cq_desc_hwstamp =
- (void *)comp +
- qcq->cq.desc_size -
- sizeof(struct ionic_txq_comp) -
- IONIC_HWSTAMP_CQ_NEGOFFSET;
+ cq_desc_hwstamp = (void *)comp - IONIC_HWSTAMP_CQ_NEGOFFSET;
hwstamp = le64_to_cpu(*cq_desc_hwstamp);
@@ -1244,7 +1240,12 @@ static bool ionic_tx_service(struct ionic_cq *cq,
unsigned int pkts = 0;
u16 index;
- comp = &((struct ionic_txq_comp *)cq->base)[cq->tail_idx];
+ if (likely(cq->desc_size == sizeof(*comp)))
+ comp = &((struct ionic_txq_comp *)cq->base)[cq->tail_idx];
+ else
+ comp = cq->base +
+ cq->desc_size * cq->tail_idx +
+ cq->desc_size - sizeof(*comp);
if (!color_match(comp->color, cq->done_color))
return false;
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH net v2 5/5] ionic: fix completion descriptor access with 2x desc size
2026-05-06 3:57 ` [PATCH net v2 5/5] ionic: fix completion descriptor access with 2x desc size Eric Joyner
@ 2026-05-07 15:59 ` Jakub Kicinski
0 siblings, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2026-05-07 15:59 UTC (permalink / raw)
To: eric.joyner
Cc: Jakub Kicinski, netdev, brett.creeley, andrew+netdev, davem,
edumazet, pabeni, prabu.ponrajthayalan
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ionic: fix completion descriptor access with 2x desc size
This commit fixes completion descriptor access when the IONIC_Q_F_2X_CQ_DESC
flag is set. It introduces pointer arithmetic to correctly calculate the
offset of the 32-byte completion descriptor slot.
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
[ ... ]
> @@ -729,7 +725,12 @@ static bool __ionic_rx_service(struct ionic_cq *cq, struct bpf_prog *xdp_prog)
> struct ionic_queue *q = cq->bound_q;
> struct ionic_rxq_comp *comp;
>
> - comp = &((struct ionic_rxq_comp *)cq->base)[cq->tail_idx];
> + if (likely(cq->desc_size == sizeof(*comp)))
> + comp = &((struct ionic_rxq_comp *)cq->base)[cq->tail_idx];
> + else
> + comp = cq->base +
> + cq->desc_size * cq->tail_idx +
> + cq->desc_size - sizeof(*comp);
>
> if (!color_match(comp->pkt_type_color, cq->done_color))
> return false;
This is a pre-existing issue, but since the descriptor access is being
modified here, should there be a dma_rmb() after the color_match() check?
Without a read memory barrier, weakly-ordered CPU architectures might
speculatively read other fields of the descriptor from memory before checking
the color bit. This could result in processing stale data from the previous
ring cycle.
[ ... ]
> @@ -1244,7 +1240,12 @@ static bool ionic_tx_service(struct ionic_cq *cq,
> unsigned int pkts = 0;
> u16 index;
>
> - comp = &((struct ionic_txq_comp *)cq->base)[cq->tail_idx];
> + if (likely(cq->desc_size == sizeof(*comp)))
> + comp = &((struct ionic_txq_comp *)cq->base)[cq->tail_idx];
> + else
> + comp = cq->base +
> + cq->desc_size * cq->tail_idx +
> + cq->desc_size - sizeof(*comp);
>
> if (!color_match(comp->color, cq->done_color))
> return false;
Does this color_match() check also need a dma_rmb() immediately following it
to prevent stale descriptor reads on the transmit side?
^ permalink raw reply [flat|nested] 10+ messages in thread