diff mbox series

[04/13] net/ionic: fix missing volatile type for cqe pointers

Message ID 20240202193238.62669-5-andrew.boyer@amd.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series net/ionic: miscellaneous fixes and improvements |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Boyer, Andrew Feb. 2, 2024, 7:32 p.m. UTC 
  From: Neel Patel <neel.patel@amd.com>

This memory may be changed by the hardware, so the volatile
keyword is required for correctness.

Fixes: e86a6fcc7cf3 ("net/ionic: add optimized non-scattered Rx/Tx")
cc: stable@dpdk.org

Signed-off-by: Andrew Boyer <andrew.boyer@amd.com>
Signed-off-by: Neel Patel <neel.patel@amd.com>
---
 drivers/net/ionic/ionic_rxtx.c        | 4 ++--
 drivers/net/ionic/ionic_rxtx_sg.c     | 8 +++++---
 drivers/net/ionic/ionic_rxtx_simple.c | 8 +++++---
 3 files changed, 12 insertions(+), 8 deletions(-)

Comments

Stephen Hemminger Feb. 3, 2024, 4:26 a.m. UTC | #1 
On Fri, 2 Feb 2024 11:32:29 -0800
Andrew Boyer <andrew.boyer@amd.com> wrote:

> From: Neel Patel <neel.patel@amd.com>
> 
> This memory may be changed by the hardware, so the volatile
> keyword is required for correctness.
> 
> Fixes: e86a6fcc7cf3 ("net/ionic: add optimized non-scattered Rx/Tx")
> cc: stable@dpdk.org
> 
> Signed-off-by: Andrew Boyer <andrew.boyer@amd.com>
> Signed-off-by: Neel Patel <neel.patel@amd.com>

In general barriers are better than volatile.
Volatile is a compiler not hardware construct really.

https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
Boyer, Andrew Feb. 5, 2024, 1:33 p.m. UTC | #2 
> On Feb 2, 2024, at 11:26 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Fri, 2 Feb 2024 11:32:29 -0800
> Andrew Boyer <andrew.boyer@amd.com> wrote:
> 
>> From: Neel Patel <neel.patel@amd.com>
>> 
>> This memory may be changed by the hardware, so the volatile
>> keyword is required for correctness.
>> 
>> Fixes: e86a6fcc7cf3 ("net/ionic: add optimized non-scattered Rx/Tx")
>> cc: stable@dpdk.org
>> 
>> Signed-off-by: Andrew Boyer <andrew.boyer@amd.com>
>> Signed-off-by: Neel Patel <neel.patel@amd.com>
> 
> In general barriers are better than volatile.
> Volatile is a compiler not hardware construct really.
> 
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html

Thanks for looking. The goal here is to prevent inappropriate compiler optimization.

From the link you posted:

> Pointers to data structures in coherent memory which might be modified by I/O devices can, sometimes, legitimately be volatile. A ring buffer used by a network adapter, where that adapter changes pointers to indicate which descriptors have been processed, is an example of this type of situation.


Doesn't that sound like what we are doing?

Thanks,
Andrew
diff mbox series

Patch

diff --git a/drivers/net/ionic/ionic_rxtx.c b/drivers/net/ionic/ionic_rxtx.c
index d92b231f8f..d92fa1cca7 100644
--- a/drivers/net/ionic/ionic_rxtx.c
+++ b/drivers/net/ionic/ionic_rxtx.c
@@ -755,7 +755,7 @@  ionic_dev_rx_descriptor_status(void *rx_queue, uint16_t offset)
 {
 	struct ionic_rx_qcq *rxq = rx_queue;
 	struct ionic_qcq *qcq = &rxq->qcq;
-	struct ionic_rxq_comp *cq_desc;
+	volatile struct ionic_rxq_comp *cq_desc;
 	uint16_t mask, head, tail, pos;
 	bool done_color;
 
@@ -794,7 +794,7 @@  ionic_dev_tx_descriptor_status(void *tx_queue, uint16_t offset)
 {
 	struct ionic_tx_qcq *txq = tx_queue;
 	struct ionic_qcq *qcq = &txq->qcq;
-	struct ionic_txq_comp *cq_desc;
+	volatile struct ionic_txq_comp *cq_desc;
 	uint16_t mask, head, tail, pos, cq_pos;
 	bool done_color;
 
diff --git a/drivers/net/ionic/ionic_rxtx_sg.c b/drivers/net/ionic/ionic_rxtx_sg.c
index 6c028a698c..1392342463 100644
--- a/drivers/net/ionic/ionic_rxtx_sg.c
+++ b/drivers/net/ionic/ionic_rxtx_sg.c
@@ -28,7 +28,8 @@  ionic_tx_flush_sg(struct ionic_tx_qcq *txq)
 	struct ionic_queue *q = &txq->qcq.q;
 	struct ionic_tx_stats *stats = &txq->stats;
 	struct rte_mbuf *txm;
-	struct ionic_txq_comp *cq_desc, *cq_desc_base = cq->base;
+	struct ionic_txq_comp *cq_desc_base = cq->base;
+	volatile struct ionic_txq_comp *cq_desc;
 	void **info;
 	uint32_t i;
 
@@ -254,7 +255,7 @@  ionic_xmit_pkts_sg(void *tx_queue, struct rte_mbuf **tx_pkts,
  */
 static __rte_always_inline void
 ionic_rx_clean_one_sg(struct ionic_rx_qcq *rxq,
-		struct ionic_rxq_comp *cq_desc,
+		volatile struct ionic_rxq_comp *cq_desc,
 		struct ionic_rx_service *rx_svc)
 {
 	struct ionic_queue *q = &rxq->qcq.q;
@@ -440,7 +441,8 @@  ionic_rxq_service_sg(struct ionic_rx_qcq *rxq, uint32_t work_to_do,
 	struct ionic_cq *cq = &rxq->qcq.cq;
 	struct ionic_queue *q = &rxq->qcq.q;
 	struct ionic_rxq_desc *q_desc_base = q->base;
-	struct ionic_rxq_comp *cq_desc, *cq_desc_base = cq->base;
+	struct ionic_rxq_comp *cq_desc_base = cq->base;
+	volatile struct ionic_rxq_comp *cq_desc;
 	uint32_t work_done = 0;
 	uint64_t then, now, hz, delta;
 
diff --git a/drivers/net/ionic/ionic_rxtx_simple.c b/drivers/net/ionic/ionic_rxtx_simple.c
index 5969287b66..00152c885a 100644
--- a/drivers/net/ionic/ionic_rxtx_simple.c
+++ b/drivers/net/ionic/ionic_rxtx_simple.c
@@ -28,7 +28,8 @@  ionic_tx_flush(struct ionic_tx_qcq *txq)
 	struct ionic_queue *q = &txq->qcq.q;
 	struct ionic_tx_stats *stats = &txq->stats;
 	struct rte_mbuf *txm;
-	struct ionic_txq_comp *cq_desc, *cq_desc_base = cq->base;
+	struct ionic_txq_comp *cq_desc_base = cq->base;
+	volatile struct ionic_txq_comp *cq_desc;
 	void **info;
 
 	cq_desc = &cq_desc_base[cq->tail_idx];
@@ -227,7 +228,7 @@  ionic_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
  */
 static __rte_always_inline void
 ionic_rx_clean_one(struct ionic_rx_qcq *rxq,
-		struct ionic_rxq_comp *cq_desc,
+		volatile struct ionic_rxq_comp *cq_desc,
 		struct ionic_rx_service *rx_svc)
 {
 	struct ionic_queue *q = &rxq->qcq.q;
@@ -361,7 +362,8 @@  ionic_rxq_service(struct ionic_rx_qcq *rxq, uint32_t work_to_do,
 	struct ionic_cq *cq = &rxq->qcq.cq;
 	struct ionic_queue *q = &rxq->qcq.q;
 	struct ionic_rxq_desc *q_desc_base = q->base;
-	struct ionic_rxq_comp *cq_desc, *cq_desc_base = cq->base;
+	struct ionic_rxq_comp *cq_desc_base = cq->base;
+	volatile struct ionic_rxq_comp *cq_desc;
 	uint32_t work_done = 0;
 	uint64_t then, now, hz, delta;