[PATCH] ethdev: fix strict aliasing lead to link cannot be up

[PATCH] ethdev: fix strict aliasing lead to link cannot be up

[Chengwen Feng]

Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
which will lead the hns3 NIC can't link up. The root cause is strict
aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
[1] for more details.

This commit use union to avoid such aliasing violation.

[1] Strict aliasing problem with rte_eth_linkstatus_set()
    https://marc.info/?l=dpdk-dev&m=171274148514777&w=3

Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
---
 lib/ethdev/ethdev_driver.h | 23 +++++++----------------
 lib/ethdev/rte_ethdev.h    | 16 ++++++++++------
 2 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 0dbf2dd6a2..9d831d5c84 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1674,18 +1674,13 @@ static inline int
 rte_eth_linkstatus_set(struct rte_eth_dev *dev,
 		       const struct rte_eth_link *new_link)
 {
-	RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev->data->dev_link);
-	union {
-		uint64_t val64;
-		struct rte_eth_link link;
-	} orig;
-
-	RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
+	struct rte_eth_link old_link;
 
-	orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const uint64_t *)new_link,
-					rte_memory_order_seq_cst);
+	old_link.val64 = rte_atomic_exchange_explicit(&dev->data->dev_link.val64,
+						      new_link->val64,
+						      rte_memory_order_seq_cst);
 
-	return (orig.link.link_status == new_link->link_status) ? -1 : 0;
+	return (old_link.link_status == new_link->link_status) ? -1 : 0;
 }
 
 /**
@@ -1701,12 +1696,8 @@ static inline void
 rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
 		       struct rte_eth_link *link)
 {
-	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data->dev_link);
-	uint64_t *dst = (uint64_t *)link;
-
-	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
-
-	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
+	link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
+					       rte_memory_order_seq_cst);
 }
 
 /**
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 147257d6a2..0b5d3d2318 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -332,12 +332,16 @@ struct rte_eth_stats {
 /**
  * A structure used to retrieve link-level information of an Ethernet port.
  */
-__extension__
-struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64 read/write */
-	uint32_t link_speed;        /**< RTE_ETH_SPEED_NUM_ */
-	uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
-	uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
-	uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
+struct rte_eth_link {
+	union {
+		uint64_t val64; /**< used for atomic64 read/write */
+		struct {
+			uint32_t link_speed;	    /**< RTE_ETH_SPEED_NUM_ */
+			uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
+			uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
+			uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
+		};
+	};
 };
 
 /**@{@name Link negotiation
-- 
2.17.1

RE: [PATCH] ethdev: fix strict aliasing lead to link cannot be up

[Morten Brørup]

> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> Sent: Thursday, 11 April 2024 05.08
> 
> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
> which will lead the hns3 NIC can't link up. The root cause is strict
> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
> [1] for more details.
> 
> This commit use union to avoid such aliasing violation.
> 
> [1] Strict aliasing problem with rte_eth_linkstatus_set()
>     https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> ---

The patch mixes atomic and non-atomic access.
This is not new for DPDK, which used to rely on compiler built-in atomics.

I'm not sure if it needs to be changed, but my suggestion is inline below.

>  lib/ethdev/ethdev_driver.h | 23 +++++++----------------
>  lib/ethdev/rte_ethdev.h    | 16 ++++++++++------
>  2 files changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 0dbf2dd6a2..9d831d5c84 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1674,18 +1674,13 @@ static inline int
>  rte_eth_linkstatus_set(struct rte_eth_dev *dev,
>  		       const struct rte_eth_link *new_link)
>  {
> -	RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev-
> >data->dev_link);
> -	union {
> -		uint64_t val64;
> -		struct rte_eth_link link;
> -	} orig;
> -
> -	RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
> +	struct rte_eth_link old_link;
> 
> -	orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const
> uint64_t *)new_link,
> -					rte_memory_order_seq_cst);
> +	old_link.val64 = rte_atomic_exchange_explicit(&dev->data-
> >dev_link.val64,

old_link.val64 should be written using:
rte_atomic_store_explicit(&old_link.val64, rte_atomic_exchange_explicit(dev_link, ...), rte_memory_order_seq_cst)

> +						      new_link->val64,

new_link->val64 should be read using:
rte_atomic_load_explicit(&new_link->val64, rte_memory_order_seq_cst)

> +						      rte_memory_order_seq_cst);

> 
> -	return (orig.link.link_status == new_link->link_status) ? -1 : 0;
> +	return (old_link.link_status == new_link->link_status) ? -1 : 0;
>  }
> 
>  /**
> @@ -1701,12 +1696,8 @@ static inline void
>  rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
>  		       struct rte_eth_link *link)
>  {
> -	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data-
> >dev_link);
> -	uint64_t *dst = (uint64_t *)link;
> -
> -	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> -
> -	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
> +	link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,

link->val64 should be written using:
rte_atomic_store_explicit(&link->val64, rte_atomic_load_explicit(&dev->data->dev_link.val64, ...), rte_memory_order_seq_cst)


> +					       rte_memory_order_seq_cst);
>  }
> 
>  /**
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 147257d6a2..0b5d3d2318 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -332,12 +332,16 @@ struct rte_eth_stats {
>  /**
>   * A structure used to retrieve link-level information of an Ethernet
> port.
>   */
> -__extension__
> -struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64
> read/write */
> -	uint32_t link_speed;        /**< RTE_ETH_SPEED_NUM_ */
> -	uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX
> */
> -	uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
> -	uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
> +struct rte_eth_link {
> +	union {
> +		uint64_t val64; /**< used for atomic64 read/write */
> +		struct {
> +			uint32_t link_speed;	    /**< RTE_ETH_SPEED_NUM_
> */
> +			uint16_t link_duplex  : 1;  /**<
> RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
> +			uint16_t link_autoneg : 1;  /**<
> RTE_ETH_LINK_[AUTONEG/FIXED] */
> +			uint16_t link_status  : 1;  /**<
> RTE_ETH_LINK_[DOWN/UP] */
> +		};
> +	};
>  };
> 
>  /**@{@name Link negotiation
> --
> 2.17.1

RE: [PATCH] ethdev: fix strict aliasing lead to link cannot be up

[Morten Brørup]

> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> Sent: Thursday, 11 April 2024 05.08
> 
> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
> which will lead the hns3 NIC can't link up. The root cause is strict
> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
> [1] for more details.
> 
> This commit use union to avoid such aliasing violation.
> 
> [1] Strict aliasing problem with rte_eth_linkstatus_set()
>     https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> ---

The patch mixes atomic and non-atomic access.
This is not new for DPDK, which used to rely on compiler built-in atomics.

I'm not sure it needs to be changed, but my suggestion is inline below.
I don't think it makes any practical different for 64 bit arch, but it might for 32 bit arch.

>  lib/ethdev/ethdev_driver.h | 23 +++++++----------------
>  lib/ethdev/rte_ethdev.h    | 16 ++++++++++------
>  2 files changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 0dbf2dd6a2..9d831d5c84 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1674,18 +1674,13 @@ static inline int
>  rte_eth_linkstatus_set(struct rte_eth_dev *dev,
>  		       const struct rte_eth_link *new_link)
>  {
> -	RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev-
> >data->dev_link);
> -	union {
> -		uint64_t val64;
> -		struct rte_eth_link link;
> -	} orig;
> -
> -	RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
> +	struct rte_eth_link old_link;
> 
> -	orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const
> uint64_t *)new_link,
> -					rte_memory_order_seq_cst);
> +	old_link.val64 = rte_atomic_exchange_explicit(&dev->data-
> >dev_link.val64,

old_link.val64 should be written using:
rte_atomic_store_explicit(&old_link.val64, ..., rte_memory_order_seq_cst)

> +						      new_link->val64,

new_link->val64 should be read using:
rte_atomic_load_explicit(&new_link->val64, rte_memory_order_seq_cst)

> +						      rte_memory_order_seq_cst);

> 
> -	return (orig.link.link_status == new_link->link_status) ? -1 : 0;
> +	return (old_link.link_status == new_link->link_status) ? -1 : 0;
>  }
> 
>  /**
> @@ -1701,12 +1696,8 @@ static inline void
>  rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
>  		       struct rte_eth_link *link)
>  {
> -	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data-
> >dev_link);
> -	uint64_t *dst = (uint64_t *)link;
> -
> -	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> -
> -	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
> +	link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,

link->val64 should be written using:
rte_atomic_store_explicit(&link->val64, ..., rte_memory_order_seq_cst)

> +					       rte_memory_order_seq_cst);
>  }
> 
>  /**
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 147257d6a2..0b5d3d2318 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -332,12 +332,16 @@ struct rte_eth_stats {
>  /**
>   * A structure used to retrieve link-level information of an Ethernet
> port.
>   */
> -__extension__
> -struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64
> read/write */
> -	uint32_t link_speed;        /**< RTE_ETH_SPEED_NUM_ */
> -	uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX
> */
> -	uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
> -	uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
> +struct rte_eth_link {
> +	union {
> +		uint64_t val64; /**< used for atomic64 read/write */

The type of val64 should be:
RTE_ATOMIC(uint64_t)

> +		struct {
> +			uint32_t link_speed;	    /**< RTE_ETH_SPEED_NUM_
> */
> +			uint16_t link_duplex  : 1;  /**<
> RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
> +			uint16_t link_autoneg : 1;  /**<
> RTE_ETH_LINK_[AUTONEG/FIXED] */
> +			uint16_t link_status  : 1;  /**<
> RTE_ETH_LINK_[DOWN/UP] */
> +		};
> +	};
>  };
> 
>  /**@{@name Link negotiation
> --
> 2.17.1

Re: [PATCH] ethdev: fix strict aliasing lead to link cannot be up

[fengchengwen]

Hi Morten,

On 2024/4/11 14:58, Morten Brørup wrote:
>> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
>> Sent: Thursday, 11 April 2024 05.08
>>
>> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
>> which will lead the hns3 NIC can't link up. The root cause is strict
>> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
>> [1] for more details.
>>
>> This commit use union to avoid such aliasing violation.
>>
>> [1] Strict aliasing problem with rte_eth_linkstatus_set()
>>     https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
>>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
>> ---
> 
> The patch mixes atomic and non-atomic access.
> This is not new for DPDK, which used to rely on compiler built-in atomics.
> 
> I'm not sure it needs to be changed, but my suggestion is inline below.
> I don't think it makes any practical different for 64 bit arch, but it might for 32 bit arch.
> 
>>  lib/ethdev/ethdev_driver.h | 23 +++++++----------------
>>  lib/ethdev/rte_ethdev.h    | 16 ++++++++++------
>>  2 files changed, 17 insertions(+), 22 deletions(-)
>>
>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>> index 0dbf2dd6a2..9d831d5c84 100644
>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -1674,18 +1674,13 @@ static inline int
>>  rte_eth_linkstatus_set(struct rte_eth_dev *dev,
>>  		       const struct rte_eth_link *new_link)
>>  {
>> -	RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev-
>>> data->dev_link);
>> -	union {
>> -		uint64_t val64;
>> -		struct rte_eth_link link;
>> -	} orig;
>> -
>> -	RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
>> +	struct rte_eth_link old_link;
>>
>> -	orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const
>> uint64_t *)new_link,
>> -					rte_memory_order_seq_cst);
>> +	old_link.val64 = rte_atomic_exchange_explicit(&dev->data-
>>> dev_link.val64,
> 
> old_link.val64 should be written using:
> rte_atomic_store_explicit(&old_link.val64, ..., rte_memory_order_seq_cst)

I'm afraid I don't agree this, the &dev->data->dev_link.val64 should use atomic not the stack variable old_link.

> 
>> +						      new_link->val64,
> 
> new_link->val64 should be read using:
> rte_atomic_load_explicit(&new_link->val64, rte_memory_order_seq_cst)

The same reason with above.

> 
>> +						      rte_memory_order_seq_cst);
> 
>>
>> -	return (orig.link.link_status == new_link->link_status) ? -1 : 0;
>> +	return (old_link.link_status == new_link->link_status) ? -1 : 0;
>>  }
>>
>>  /**
>> @@ -1701,12 +1696,8 @@ static inline void
>>  rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
>>  		       struct rte_eth_link *link)
>>  {
>> -	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data-
>>> dev_link);
>> -	uint64_t *dst = (uint64_t *)link;
>> -
>> -	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
>> -
>> -	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
>> +	link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
> 
> link->val64 should be written using:
> rte_atomic_store_explicit(&link->val64, ..., rte_memory_order_seq_cst)

The same reason with above, the &dev->data->dev_link.val64 should use atomic not the stack variable link.

> 
>> +					       rte_memory_order_seq_cst);
>>  }
>>
>>  /**
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index 147257d6a2..0b5d3d2318 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -332,12 +332,16 @@ struct rte_eth_stats {
>>  /**
>>   * A structure used to retrieve link-level information of an Ethernet
>> port.
>>   */
>> -__extension__
>> -struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64
>> read/write */
>> -	uint32_t link_speed;        /**< RTE_ETH_SPEED_NUM_ */
>> -	uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX
>> */
>> -	uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
>> -	uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
>> +struct rte_eth_link {
>> +	union {
>> +		uint64_t val64; /**< used for atomic64 read/write */
> 
> The type of val64 should be:
> RTE_ATOMIC(uint64_t)

ack

Plus: yes, this patch mixes atomic and non-atomic access, but the main reason
is that we want to simplify the implementation. If we want to separate it clearly,
maybe we should defined as this:
    struct rte_eth_link {
        union {
            RTE_ATOMIC(uint64_t) atomic64; /**< used for atomic64 read/write */
            struct {
                uint64_t val64;
            };
            struct {
                uint32_t link_speed;	    /**< RTE_ETH_SPEED_NUM_ */
                uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
                uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
                uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
            };
        };
    };

Thanks

> 
>> +		struct {
>> +			uint32_t link_speed;	    /**< RTE_ETH_SPEED_NUM_
>> */
>> +			uint16_t link_duplex  : 1;  /**<
>> RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
>> +			uint16_t link_autoneg : 1;  /**<
>> RTE_ETH_LINK_[AUTONEG/FIXED] */
>> +			uint16_t link_status  : 1;  /**<
>> RTE_ETH_LINK_[DOWN/UP] */
>> +		};
>> +	};
>>  };
>>
>>  /**@{@name Link negotiation
>> --
>> 2.17.1
> 
> .
> 

[PATCH v3] ethdev: fix strict aliasing lead to link cannot be up

[Chengwen Feng]

Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
which will lead the hns3 NIC can't link up. The root cause is strict
aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
[1] for more details.

This commit use union to avoid such aliasing violation.

[1] Strict aliasing problem with rte_eth_linkstatus_set()
    https://marc.info/?l=dpdk-dev&m=171274148514777&w=3

Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>

---
v3: fix checkpatch warning "missing --in-reply-to".
v2: add RTE_ATOMIC(uint64_t) wrap which address Morten's comment.

---
 lib/ethdev/ethdev_driver.h | 23 +++++++----------------
 lib/ethdev/rte_ethdev.h    | 16 ++++++++++------
 2 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 0dbf2dd6a2..9d831d5c84 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1674,18 +1674,13 @@ static inline int
 rte_eth_linkstatus_set(struct rte_eth_dev *dev,
 		       const struct rte_eth_link *new_link)
 {
-	RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev->data->dev_link);
-	union {
-		uint64_t val64;
-		struct rte_eth_link link;
-	} orig;
-
-	RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
+	struct rte_eth_link old_link;
 
-	orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const uint64_t *)new_link,
-					rte_memory_order_seq_cst);
+	old_link.val64 = rte_atomic_exchange_explicit(&dev->data->dev_link.val64,
+						      new_link->val64,
+						      rte_memory_order_seq_cst);
 
-	return (orig.link.link_status == new_link->link_status) ? -1 : 0;
+	return (old_link.link_status == new_link->link_status) ? -1 : 0;
 }
 
 /**
@@ -1701,12 +1696,8 @@ static inline void
 rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
 		       struct rte_eth_link *link)
 {
-	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data->dev_link);
-	uint64_t *dst = (uint64_t *)link;
-
-	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
-
-	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
+	link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
+					       rte_memory_order_seq_cst);
 }
 
 /**
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 147257d6a2..ccf43e468a 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -332,12 +332,16 @@ struct rte_eth_stats {
 /**
  * A structure used to retrieve link-level information of an Ethernet port.
  */
-__extension__
-struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64 read/write */
-	uint32_t link_speed;        /**< RTE_ETH_SPEED_NUM_ */
-	uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
-	uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
-	uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
+struct rte_eth_link {
+	union {
+		RTE_ATOMIC(uint64_t) val64; /**< used for atomic64 read/write */
+		struct {
+			uint32_t link_speed;	    /**< RTE_ETH_SPEED_NUM_ */
+			uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
+			uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
+			uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
+		};
+	};
 };
 
 /**@{@name Link negotiation
-- 
2.17.1

RE: [PATCH v3] ethdev: fix strict aliasing lead to link cannot be up

[Morten Brørup]

> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> Sent: Thursday, 11 April 2024 14.04
> 
> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
> which will lead the hns3 NIC can't link up. The root cause is strict
> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
> [1] for more details.
> 
> This commit use union to avoid such aliasing violation.
> 
> [1] Strict aliasing problem with rte_eth_linkstatus_set()
>     https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> 
> ---
> v3: fix checkpatch warning "missing --in-reply-to".
> v2: add RTE_ATOMIC(uint64_t) wrap which address Morten's comment.
> 
> ---
>  lib/ethdev/ethdev_driver.h | 23 +++++++----------------
>  lib/ethdev/rte_ethdev.h    | 16 ++++++++++------
>  2 files changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 0dbf2dd6a2..9d831d5c84 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1674,18 +1674,13 @@ static inline int
>  rte_eth_linkstatus_set(struct rte_eth_dev *dev,
>  		       const struct rte_eth_link *new_link)
>  {
> -	RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev-
> >data->dev_link);
> -	union {
> -		uint64_t val64;
> -		struct rte_eth_link link;
> -	} orig;
> -
> -	RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
> +	struct rte_eth_link old_link;
> 
> -	orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const
> uint64_t *)new_link,
> -					rte_memory_order_seq_cst);
> +	old_link.val64 = rte_atomic_exchange_explicit(&dev->data-
> >dev_link.val64,

You are right; old_link has local scope and is on the stack, so atomic store is not required.

And since rte_eth_linkstatus_set() is an internal function called from the driver only, it is probably safe to assume that *new_link is on the caller's stack and doesn't change while being accessed by this function.
I guess that new_link is passed by reference for performance and future-proofing reasons; it could have been passed by value instead. If it was passed by value, atomic access would certainly not be required.
In other words: You are right here too; new_link does not require atomic load.

> +						      new_link->val64,
> +						      rte_memory_order_seq_cst);
> 
> -	return (orig.link.link_status == new_link->link_status) ? -1 : 0;
> +	return (old_link.link_status == new_link->link_status) ? -1 : 0;
>  }
> 
>  /**
> @@ -1701,12 +1696,8 @@ static inline void
>  rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
>  		       struct rte_eth_link *link)
>  {
> -	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data-
> >dev_link);
> -	uint64_t *dst = (uint64_t *)link;
> -
> -	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> -
> -	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
> +	link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
> +					       rte_memory_order_seq_cst);

It is not safe to assume that the link pointer points to local memory on the caller's stack.
The link pointer might point to a shared memory area, used by multiple threads/processes, so it needs to be stored atomically using rte_atomic_store_explicit(&link->val64, ..., rte_memory_order_seq_cst).

>  }
> 
>  /**
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 147257d6a2..ccf43e468a 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -332,12 +332,16 @@ struct rte_eth_stats {
>  /**
>   * A structure used to retrieve link-level information of an Ethernet
> port.
>   */
> -__extension__
> -struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64
> read/write */
> -	uint32_t link_speed;        /**< RTE_ETH_SPEED_NUM_ */
> -	uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX
> */
> -	uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
> -	uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
> +struct rte_eth_link {
> +	union {
> +		RTE_ATOMIC(uint64_t) val64; /**< used for atomic64
> read/write */
> +		struct {
> +			uint32_t link_speed;	    /**< RTE_ETH_SPEED_NUM_
> */
> +			uint16_t link_duplex  : 1;  /**<
> RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
> +			uint16_t link_autoneg : 1;  /**<
> RTE_ETH_LINK_[AUTONEG/FIXED] */
> +			uint16_t link_status  : 1;  /**<
> RTE_ETH_LINK_[DOWN/UP] */
> +		};
> +	};
>  };
> 
>  /**@{@name Link negotiation
> --
> 2.17.1

RE: [PATCH] ethdev: fix strict aliasing lead to link cannot be up

[Morten Brørup]

> From: fengchengwen [mailto:fengchengwen@huawei.com]
> Sent: Thursday, 11 April 2024 13.58

[...]

> Plus: yes, this patch mixes atomic and non-atomic access, but the main
> reason is that we want to simplify the implementation.

Yes, your design in patch v3 follows the current standard design pattern for atomics in DPDK.
I agree with you on this.

Thank you for describing the alternative, though.

> If we want to separate it clearly,
> maybe we should defined as this:
>     struct rte_eth_link {
>         union {
>             RTE_ATOMIC(uint64_t) atomic64; /**< used for atomic64
> read/write */
>             struct {
>                 uint64_t val64;
>             };
>             struct {
>                 uint32_t link_speed;	    /**< RTE_ETH_SPEED_NUM_ */
>                 uint16_t link_duplex  : 1;  /**<
> RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
>                 uint16_t link_autoneg : 1;  /**<
> RTE_ETH_LINK_[AUTONEG/FIXED] */
>                 uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP]
> */
>             };
>         };
>     };

PS: More review comments provided in reply to the v3 patch.

Re: [PATCH] ethdev: fix strict aliasing lead to link cannot be up

[Stephen Hemminger]

On Thu, 11 Apr 2024 03:07:49 +0000
Chengwen Feng <fengchengwen@huawei.com> wrote:

> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
> which will lead the hns3 NIC can't link up. The root cause is strict
> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
> [1] for more details.
> 
> This commit use union to avoid such aliasing violation.
> 
> [1] Strict aliasing problem with rte_eth_linkstatus_set()
>     https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> ---

The patch to use union is correct.
Examining the link status fuller raises a couple of pre-existing issues.

1. Why is this an inline function, there is no way this is in the
   fast path of any driver or application?

2. Why is it marked sequential consistent and not relaxed? How could there be a visible
   relationship between link status and other variables. Drivers would
   not be using the link status state as internal variable.

Re: [PATCH v3] ethdev: fix strict aliasing lead to link cannot be up

[fengchengwen]

Hi Morten,

On 2024/4/11 20:44, Morten Brørup wrote:
>> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
>> Sent: Thursday, 11 April 2024 14.04
>>
>> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
>> which will lead the hns3 NIC can't link up. The root cause is strict
>> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
>> [1] for more details.
>>
>> This commit use union to avoid such aliasing violation.
>>
>> [1] Strict aliasing problem with rte_eth_linkstatus_set()
>>     https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
>>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
>>
>> ---
>> v3: fix checkpatch warning "missing --in-reply-to".
>> v2: add RTE_ATOMIC(uint64_t) wrap which address Morten's comment.
>>
>> ---
>>  lib/ethdev/ethdev_driver.h | 23 +++++++----------------
>>  lib/ethdev/rte_ethdev.h    | 16 ++++++++++------
>>  2 files changed, 17 insertions(+), 22 deletions(-)
>>
>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>> index 0dbf2dd6a2..9d831d5c84 100644
>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -1674,18 +1674,13 @@ static inline int
>>  rte_eth_linkstatus_set(struct rte_eth_dev *dev,
>>  		       const struct rte_eth_link *new_link)
>>  {
>> -	RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev-
>>> data->dev_link);
>> -	union {
>> -		uint64_t val64;
>> -		struct rte_eth_link link;
>> -	} orig;
>> -
>> -	RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
>> +	struct rte_eth_link old_link;
>>
>> -	orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const
>> uint64_t *)new_link,
>> -					rte_memory_order_seq_cst);
>> +	old_link.val64 = rte_atomic_exchange_explicit(&dev->data-
>>> dev_link.val64,
> 
> You are right; old_link has local scope and is on the stack, so atomic store is not required.
> 
> And since rte_eth_linkstatus_set() is an internal function called from the driver only, it is probably safe to assume that *new_link is on the caller's stack and doesn't change while being accessed by this function.
> I guess that new_link is passed by reference for performance and future-proofing reasons; it could have been passed by value instead. If it was passed by value, atomic access would certainly not be required.
> In other words: You are right here too; new_link does not require atomic load.
> 
>> +						      new_link->val64,
>> +						      rte_memory_order_seq_cst);
>>
>> -	return (orig.link.link_status == new_link->link_status) ? -1 : 0;
>> +	return (old_link.link_status == new_link->link_status) ? -1 : 0;
>>  }
>>
>>  /**
>> @@ -1701,12 +1696,8 @@ static inline void
>>  rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
>>  		       struct rte_eth_link *link)
>>  {
>> -	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data-
>>> dev_link);
>> -	uint64_t *dst = (uint64_t *)link;
>> -
>> -	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
>> -
>> -	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
>> +	link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
>> +					       rte_memory_order_seq_cst);
> 
> It is not safe to assume that the link pointer points to local memory on the caller's stack.
> The link pointer might point to a shared memory area, used by multiple threads/processes, so it needs to be stored atomically using rte_atomic_store_explicit(&link->val64, ..., rte_memory_order_seq_cst).

I checked every call of rte_eth_linkstatus_get in DPDK, and all the link parameters are local variables.
The dev->data->dev_link is placed in shared memory which could access from different threads/processes, it seems no need maintain another link struct which act the same role.

So I think we should keep current impl, and not using rte_atomic_store_explicit(&link->val64,...

Thanks

> 
>>  }
>>
>>  /**
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index 147257d6a2..ccf43e468a 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -332,12 +332,16 @@ struct rte_eth_stats {
>>  /**
>>   * A structure used to retrieve link-level information of an Ethernet
>> port.
>>   */
>> -__extension__
>> -struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64
>> read/write */
>> -	uint32_t link_speed;        /**< RTE_ETH_SPEED_NUM_ */
>> -	uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX
>> */
>> -	uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
>> -	uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
>> +struct rte_eth_link {
>> +	union {
>> +		RTE_ATOMIC(uint64_t) val64; /**< used for atomic64
>> read/write */
>> +		struct {
>> +			uint32_t link_speed;	    /**< RTE_ETH_SPEED_NUM_
>> */
>> +			uint16_t link_duplex  : 1;  /**<
>> RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
>> +			uint16_t link_autoneg : 1;  /**<
>> RTE_ETH_LINK_[AUTONEG/FIXED] */
>> +			uint16_t link_status  : 1;  /**<
>> RTE_ETH_LINK_[DOWN/UP] */
>> +		};
>> +	};
>>  };
>>
>>  /**@{@name Link negotiation
>> --
>> 2.17.1
> 
> .
> 

RE: [PATCH v3] ethdev: fix strict aliasing lead to link cannot be up

[Morten Brørup]

> From: fengchengwen [mailto:fengchengwen@huawei.com]
> Sent: Friday, 12 April 2024 05.28

[...]

> >> @@ -1701,12 +1696,8 @@ static inline void
> >>  rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
> >>  		       struct rte_eth_link *link)
> >>  {
> >> -	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data-
> >>> dev_link);
> >> -	uint64_t *dst = (uint64_t *)link;
> >> -
> >> -	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> >> -
> >> -	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
> >> +	link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
> >> +					       rte_memory_order_seq_cst);
> >
> > It is not safe to assume that the link pointer points to local memory
> on the caller's stack.
> > The link pointer might point to a shared memory area, used by multiple
> threads/processes, so it needs to be stored atomically using
> rte_atomic_store_explicit(&link->val64, ..., rte_memory_order_seq_cst).
> 
> I checked every call of rte_eth_linkstatus_get in DPDK, and all the link
> parameters are local variables.
> The dev->data->dev_link is placed in shared memory which could access
> from different threads/processes, it seems no need maintain another link
> struct which act the same role.
> 
> So I think we should keep current impl, and not using
> rte_atomic_store_explicit(&link->val64,...

The application may pass a pointer to shared memory to the public rte_eth_link_get() function, which passes the pointer on to rte_eth_linkstatus_get():
https://elixir.bootlin.com/dpdk/v24.03/source/lib/ethdev/rte_ethdev.c#L2986

Re: [PATCH] ethdev: fix strict aliasing lead to link cannot be up

[fengchengwen]

Hi Stephen,

On 2024/4/11 23:05, Stephen Hemminger wrote:
> On Thu, 11 Apr 2024 03:07:49 +0000
> Chengwen Feng <fengchengwen@huawei.com> wrote:
> 
>> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
>> which will lead the hns3 NIC can't link up. The root cause is strict
>> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
>> [1] for more details.
>>
>> This commit use union to avoid such aliasing violation.
>>
>> [1] Strict aliasing problem with rte_eth_linkstatus_set()
>>     https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
>>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
>> ---
> 
> The patch to use union is correct.
> Examining the link status fuller raises a couple of pre-existing issues.
> 
> 1. Why is this an inline function, there is no way this is in the
>    fast path of any driver or application?

Nice catch.

The rte_eth_linkstatus_set/get was first introduced by commit "b77d21cc23 ethdev: add link status get/set helper functions".
which were inline function.

But I check this patchset [1], and found in v1~v3 is was impl without static inline, and the v4 just with static inline. and
I can't get the change reason. @Stephen could you recall something about this?

Below is what I try to find the reason from source code:
1, Why rte_eth_linkstatus_get is inline, I think maybe due:
   a, memif driver invoke rte_eth_link_get() in Rx/Tx path (eth_memif_rx/eth_memif_rx_zc/eth_memif_tx/eth_memif_tx_zc)
   b, before the above commit, rte_eth_link_get() invoke rte_eth_dev_atomic_read_link_status() which was static inline.
   from above, I think it mainly due to performance reason, so keep the original impl.
2, Why rte_eth_linkstatus_set is inline, I check the following patch:
    5042dde07d net/enic: use link status helper functions
    2b4ab4223d net/octeontx: use link status helper functions
    cc92eb9a97 net/szedata2: use link status helper functions
    8e14dc285a net/thunderx: use link status helper functions
    e324523c6c net/liquidio: use link status helper functions
    e66b0fd123 net/i40e: use link status helper functions
    4abe903e50 net/sfc: use link status helper functions
    faadebad81 net/ixgbe: use link status helper functions
    80ba61115e net/e1000: use link status helper functions
    aab28ea2bc net/nfp: use link status helper functions
    7e2eb5f0d2 net/dpaa2: use link status helper functions
    13086a8f50 net/vmxnet3: use link status helper functions
    717b2e8eae net/virtio: use link status helper functions
  almost all pmd's set link function is static inline, I also check, and found only sfc driver will invoke it in
  Tx path: sfc_efx_recv_pkts->sfc_ev_qpoll->efx_ev_qpoll->eevo_qpoll->siena_ef10_ev_qpoll->rhead_ev_mcdi->ef10_ev_mcdi->sfc_ev_link_change->rte_eth_linkstatus_set

[1] https://inbox.dpdk.org/dev/20180108174514.14688-1-stephen@networkplumber.org/
    https://inbox.dpdk.org/dev/20180116182558.6254-1-stephen@networkplumber.org/

> 
> 2. Why is it marked sequential consistent and not relaxed? How could there be a visible
>    relationship between link status and other variables. Drivers would
>    not be using the link status state as internal variable.

The original was impl by rte_atomic64_cmpset():
1, In powerpc platform, it was impl as:
    static inline int
    rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)
    {
        return rte_atomic_compare_exchange_strong_explicit(dst, &exp, src, rte_memory_order_acquire,
            rte_memory_order_acquire) ? 1 : 0;
    }
2, In ARM64 platform, it was impl as:
    static inline int
    rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)
    {
        return __sync_bool_compare_and_swap(dst, exp, src);
    }
    The __sync_bool_compare_and_swap will compiled with a dmb.

So there's no problem from the point of view of replacement (just with the memory barrier).

I also think there are no need with sequential consistent, because this atomic mainly to solve
the concurrent problem of rte_eth_link_get and driver lsc interrupt setting.

BTW: the above two problem is OK, but I think we could tackle with another commit (NOT this).

Thanks

> 
> .
> 

[PATCH v4] ethdev: fix strict aliasing lead to link cannot be up

[Chengwen Feng]

Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
which will lead the hns3 NIC can't link up. The root cause is strict
aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
[1] for more details.

This commit use union to avoid such aliasing violation.

Note: DPDK CI report compiler error (see [2] for more details):
../drivers/net/cxgbe/cxgbe_ethdev.c:214:9: error: missing braces around
initializer [-Werror=missing-braces]
  struct rte_eth_link new_link = { 0 };
So this commit replace { 0 } with memset in cxgbe.

[1] Strict aliasing problem with rte_eth_linkstatus_set()
    https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
[2] https://mails.dpdk.org/archives/test-report/2024-April/637966.html

Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>

---
v4: fix DPDK CI compiler error with cxgbe.
v3: fix checkpatch warning "missing --in-reply-to".
v2: add RTE_ATOMIC(uint64_t) wrap which address Morten's comment.

---
 drivers/net/cxgbe/cxgbe_ethdev.c |  3 ++-
 lib/ethdev/ethdev_driver.h       | 23 +++++++----------------
 lib/ethdev/rte_ethdev.h          | 16 ++++++++++------
 3 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
index a27b9b266e..9b6a3651f9 100644
--- a/drivers/net/cxgbe/cxgbe_ethdev.c
+++ b/drivers/net/cxgbe/cxgbe_ethdev.c
@@ -211,9 +211,9 @@ int cxgbe_dev_link_update(struct rte_eth_dev *eth_dev,
 	unsigned int i, work_done, budget = 32;
 	struct link_config *lc = &pi->link_cfg;
 	struct adapter *adapter = pi->adapter;
-	struct rte_eth_link new_link = { 0 };
 	u8 old_link = pi->link_cfg.link_ok;
 	struct sge *s = &adapter->sge;
+	struct rte_eth_link new_link;
 
 	for (i = 0; i < CXGBE_LINK_STATUS_POLL_CNT; i++) {
 		if (!s->fw_evtq.desc)
@@ -232,6 +232,7 @@ int cxgbe_dev_link_update(struct rte_eth_dev *eth_dev,
 		rte_delay_ms(CXGBE_LINK_STATUS_POLL_MS);
 	}
 
+	memset(&new_link, 0, sizeof(new_link));
 	new_link.link_status = cxgbe_force_linkup(adapter) ?
 			       RTE_ETH_LINK_UP : pi->link_cfg.link_ok;
 	new_link.link_autoneg = (lc->link_caps & FW_PORT_CAP32_ANEG) ? 1 : 0;
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 0dbf2dd6a2..9d831d5c84 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1674,18 +1674,13 @@ static inline int
 rte_eth_linkstatus_set(struct rte_eth_dev *dev,
 		       const struct rte_eth_link *new_link)
 {
-	RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev->data->dev_link);
-	union {
-		uint64_t val64;
-		struct rte_eth_link link;
-	} orig;
-
-	RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
+	struct rte_eth_link old_link;
 
-	orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const uint64_t *)new_link,
-					rte_memory_order_seq_cst);
+	old_link.val64 = rte_atomic_exchange_explicit(&dev->data->dev_link.val64,
+						      new_link->val64,
+						      rte_memory_order_seq_cst);
 
-	return (orig.link.link_status == new_link->link_status) ? -1 : 0;
+	return (old_link.link_status == new_link->link_status) ? -1 : 0;
 }
 
 /**
@@ -1701,12 +1696,8 @@ static inline void
 rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
 		       struct rte_eth_link *link)
 {
-	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data->dev_link);
-	uint64_t *dst = (uint64_t *)link;
-
-	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
-
-	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
+	link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
+					       rte_memory_order_seq_cst);
 }
 
 /**
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 147257d6a2..ccf43e468a 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -332,12 +332,16 @@ struct rte_eth_stats {
 /**
  * A structure used to retrieve link-level information of an Ethernet port.
  */
-__extension__
-struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64 read/write */
-	uint32_t link_speed;        /**< RTE_ETH_SPEED_NUM_ */
-	uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
-	uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
-	uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
+struct rte_eth_link {
+	union {
+		RTE_ATOMIC(uint64_t) val64; /**< used for atomic64 read/write */
+		struct {
+			uint32_t link_speed;	    /**< RTE_ETH_SPEED_NUM_ */
+			uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
+			uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
+			uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
+		};
+	};
 };
 
 /**@{@name Link negotiation
-- 
2.17.1

[PATCH v5] ethdev: fix strict aliasing lead to link cannot be up

[Chengwen Feng]

Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
which will lead the hns3 NIC can't link up. The root cause is strict
aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
[1] for more details.

This commit use union to avoid such aliasing violation.

Note: DPDK CI report compiler error (see [2] for more details):
../drivers/net/cxgbe/cxgbe_ethdev.c:214:9: error: missing braces around
initializer [-Werror=missing-braces]
  struct rte_eth_link new_link = { 0 };
So this commit replace { 0 } with memset in cxgbe.

[1] Strict aliasing problem with rte_eth_linkstatus_set()
    https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
[2] https://mails.dpdk.org/archives/test-report/2024-April/637966.html

Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>

---
v5: fix DPDK CI compiler error due to GCC extension [-Werror=pedantic]
v4: fix DPDK CI compiler error with cxgbe.
v3: fix checkpatch warning "missing --in-reply-to".
v2: add RTE_ATOMIC(uint64_t) wrap which address Morten's comment.

---
 drivers/net/cxgbe/cxgbe_ethdev.c |  3 ++-
 lib/ethdev/ethdev_driver.h       | 23 +++++++----------------
 lib/ethdev/rte_ethdev.h          | 17 +++++++++++------
 3 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
index a27b9b266e..9b6a3651f9 100644
--- a/drivers/net/cxgbe/cxgbe_ethdev.c
+++ b/drivers/net/cxgbe/cxgbe_ethdev.c
@@ -211,9 +211,9 @@ int cxgbe_dev_link_update(struct rte_eth_dev *eth_dev,
 	unsigned int i, work_done, budget = 32;
 	struct link_config *lc = &pi->link_cfg;
 	struct adapter *adapter = pi->adapter;
-	struct rte_eth_link new_link = { 0 };
 	u8 old_link = pi->link_cfg.link_ok;
 	struct sge *s = &adapter->sge;
+	struct rte_eth_link new_link;
 
 	for (i = 0; i < CXGBE_LINK_STATUS_POLL_CNT; i++) {
 		if (!s->fw_evtq.desc)
@@ -232,6 +232,7 @@ int cxgbe_dev_link_update(struct rte_eth_dev *eth_dev,
 		rte_delay_ms(CXGBE_LINK_STATUS_POLL_MS);
 	}
 
+	memset(&new_link, 0, sizeof(new_link));
 	new_link.link_status = cxgbe_force_linkup(adapter) ?
 			       RTE_ETH_LINK_UP : pi->link_cfg.link_ok;
 	new_link.link_autoneg = (lc->link_caps & FW_PORT_CAP32_ANEG) ? 1 : 0;
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 0dbf2dd6a2..9d831d5c84 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1674,18 +1674,13 @@ static inline int
 rte_eth_linkstatus_set(struct rte_eth_dev *dev,
 		       const struct rte_eth_link *new_link)
 {
-	RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev->data->dev_link);
-	union {
-		uint64_t val64;
-		struct rte_eth_link link;
-	} orig;
-
-	RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
+	struct rte_eth_link old_link;
 
-	orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const uint64_t *)new_link,
-					rte_memory_order_seq_cst);
+	old_link.val64 = rte_atomic_exchange_explicit(&dev->data->dev_link.val64,
+						      new_link->val64,
+						      rte_memory_order_seq_cst);
 
-	return (orig.link.link_status == new_link->link_status) ? -1 : 0;
+	return (old_link.link_status == new_link->link_status) ? -1 : 0;
 }
 
 /**
@@ -1701,12 +1696,8 @@ static inline void
 rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
 		       struct rte_eth_link *link)
 {
-	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data->dev_link);
-	uint64_t *dst = (uint64_t *)link;
-
-	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
-
-	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
+	link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
+					       rte_memory_order_seq_cst);
 }
 
 /**
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 147257d6a2..548fada1c7 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -332,12 +332,17 @@ struct rte_eth_stats {
 /**
  * A structure used to retrieve link-level information of an Ethernet port.
  */
-__extension__
-struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64 read/write */
-	uint32_t link_speed;        /**< RTE_ETH_SPEED_NUM_ */
-	uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
-	uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
-	uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
+struct rte_eth_link {
+	union {
+		RTE_ATOMIC(uint64_t) val64; /**< used for atomic64 read/write */
+		__extension__
+		struct {
+			uint32_t link_speed;	    /**< RTE_ETH_SPEED_NUM_ */
+			uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
+			uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
+			uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
+		};
+	};
 };
 
 /**@{@name Link negotiation
-- 
2.17.1

[PATCH v6] ethdev: fix strict aliasing lead to link cannot be up

[Chengwen Feng]

Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
which will lead the hns3 NIC can't link up. The root cause is strict
aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
[1] for more details.

This commit use union to avoid such aliasing violation.

Note: DPDK CI report compiler error (see [2] for more details):
../drivers/net/cxgbe/cxgbe_ethdev.c:214:9: error: missing braces around
initializer [-Werror=missing-braces]
  struct rte_eth_link new_link = { 0 };
The same error with qos_sched example:
../examples/qos_sched/init.c:338:10: error: missing braces around
initializer [-Werror=missing-braces]
   struct rte_eth_link link = {0};
So this commit replace { 0 } with memset in cxgbe and qos_sched.

[1] Strict aliasing problem with rte_eth_linkstatus_set()
    https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
[2] https://mails.dpdk.org/archives/test-report/2024-April/637966.html

Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
---
v6: fix DPDK CI compiler error with qos_sched.
v5: fix DPDK CI compiler error due to GCC extension [-Werror=pedantic]
v4: fix DPDK CI compiler error with cxgbe.
v3: fix checkpatch warning "missing --in-reply-to".
v2: add RTE_ATOMIC(uint64_t) wrap which address Morten's comment.
---
 drivers/net/cxgbe/cxgbe_ethdev.c |  3 ++-
 examples/qos_sched/init.c        |  3 ++-
 lib/ethdev/ethdev_driver.h       | 23 +++++++----------------
 lib/ethdev/rte_ethdev.h          | 17 +++++++++++------
 4 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
index a27b9b266e..9b6a3651f9 100644
--- a/drivers/net/cxgbe/cxgbe_ethdev.c
+++ b/drivers/net/cxgbe/cxgbe_ethdev.c
@@ -211,9 +211,9 @@ int cxgbe_dev_link_update(struct rte_eth_dev *eth_dev,
 	unsigned int i, work_done, budget = 32;
 	struct link_config *lc = &pi->link_cfg;
 	struct adapter *adapter = pi->adapter;
-	struct rte_eth_link new_link = { 0 };
 	u8 old_link = pi->link_cfg.link_ok;
 	struct sge *s = &adapter->sge;
+	struct rte_eth_link new_link;
 
 	for (i = 0; i < CXGBE_LINK_STATUS_POLL_CNT; i++) {
 		if (!s->fw_evtq.desc)
@@ -232,6 +232,7 @@ int cxgbe_dev_link_update(struct rte_eth_dev *eth_dev,
 		rte_delay_ms(CXGBE_LINK_STATUS_POLL_MS);
 	}
 
+	memset(&new_link, 0, sizeof(new_link));
 	new_link.link_status = cxgbe_force_linkup(adapter) ?
 			       RTE_ETH_LINK_UP : pi->link_cfg.link_ok;
 	new_link.link_autoneg = (lc->link_caps & FW_PORT_CAP32_ANEG) ? 1 : 0;
diff --git a/examples/qos_sched/init.c b/examples/qos_sched/init.c
index d8abae635a..32964fd57e 100644
--- a/examples/qos_sched/init.c
+++ b/examples/qos_sched/init.c
@@ -335,7 +335,7 @@ int app_init(void)
 	for(i = 0; i < nb_pfc; i++) {
 		uint32_t socket = rte_lcore_to_socket_id(qos_conf[i].rx_core);
 		struct rte_ring *ring;
-		struct rte_eth_link link = {0};
+		struct rte_eth_link link;
 		int retry_count = 100, retry_delay = 100; /* try every 100ms for 10 sec */
 
 		snprintf(ring_name, MAX_NAME_LEN, "ring-%u-%u", i, qos_conf[i].rx_core);
@@ -367,6 +367,7 @@ int app_init(void)
 		app_init_port(qos_conf[i].rx_port, qos_conf[i].mbuf_pool);
 		app_init_port(qos_conf[i].tx_port, qos_conf[i].mbuf_pool);
 
+		memset(&link, 0, sizeof(link));
 		rte_eth_link_get(qos_conf[i].tx_port, &link);
 		if (link.link_status == 0)
 			printf("Waiting for link on port %u\n", qos_conf[i].tx_port);
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 0dbf2dd6a2..9d831d5c84 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1674,18 +1674,13 @@ static inline int
 rte_eth_linkstatus_set(struct rte_eth_dev *dev,
 		       const struct rte_eth_link *new_link)
 {
-	RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev->data->dev_link);
-	union {
-		uint64_t val64;
-		struct rte_eth_link link;
-	} orig;
-
-	RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
+	struct rte_eth_link old_link;
 
-	orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const uint64_t *)new_link,
-					rte_memory_order_seq_cst);
+	old_link.val64 = rte_atomic_exchange_explicit(&dev->data->dev_link.val64,
+						      new_link->val64,
+						      rte_memory_order_seq_cst);
 
-	return (orig.link.link_status == new_link->link_status) ? -1 : 0;
+	return (old_link.link_status == new_link->link_status) ? -1 : 0;
 }
 
 /**
@@ -1701,12 +1696,8 @@ static inline void
 rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
 		       struct rte_eth_link *link)
 {
-	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data->dev_link);
-	uint64_t *dst = (uint64_t *)link;
-
-	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
-
-	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
+	link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
+					       rte_memory_order_seq_cst);
 }
 
 /**
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 147257d6a2..548fada1c7 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -332,12 +332,17 @@ struct rte_eth_stats {
 /**
  * A structure used to retrieve link-level information of an Ethernet port.
  */
-__extension__
-struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64 read/write */
-	uint32_t link_speed;        /**< RTE_ETH_SPEED_NUM_ */
-	uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
-	uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
-	uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
+struct rte_eth_link {
+	union {
+		RTE_ATOMIC(uint64_t) val64; /**< used for atomic64 read/write */
+		__extension__
+		struct {
+			uint32_t link_speed;	    /**< RTE_ETH_SPEED_NUM_ */
+			uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
+			uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
+			uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
+		};
+	};
 };
 
 /**@{@name Link negotiation
-- 
2.17.1

RE: [PATCH v6] ethdev: fix strict aliasing lead to link cannot be up

[Morten Brørup]

> @@ -1701,12 +1696,8 @@ static inline void
>  rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
>  		       struct rte_eth_link *link)
>  {
> -	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data-
> >dev_link);
> -	uint64_t *dst = (uint64_t *)link;
> -
> -	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> -
> -	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
> +	link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
> +					       rte_memory_order_seq_cst);
>  }

rte_eth_linkstatus_get() may be called by rte_eth_link_get():
https://elixir.bootlin.com/dpdk/v24.03/source/lib/ethdev/rte_ethdev.c#L2986

The application may call rte_eth_link_get() with the "link" parameter pointing to memory shared by other threads.

So link->val64 must be stored atomically:
rte_atomic_store_explicit(&link->val64,
		rte_atomic_load_explicit(&dev->data->dev_link.val64,
		rte_memory_order_seq_cst),
		rte_memory_order_seq_cst);

With the above modification,
Acked-by: Morten Brørup <mb@smartsharesystems.com>

[PATCH v7] ethdev: fix strict aliasing lead to link cannot be up

[Chengwen Feng]

Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
which will lead the hns3 NIC can't link up. The root cause is strict
aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
[1] for more details.

This commit use union to avoid such aliasing violation.

Note: DPDK CI report compiler error (see [2] for more details):
../drivers/net/cxgbe/cxgbe_ethdev.c:214:9: error: missing braces around
initializer [-Werror=missing-braces]
  struct rte_eth_link new_link = { 0 };
The same error with qos_sched example:
../examples/qos_sched/init.c:338:10: error: missing braces around
initializer [-Werror=missing-braces]
   struct rte_eth_link link = {0};
So this commit replace { 0 } with memset in cxgbe and qos_sched.

[1] Strict aliasing problem with rte_eth_linkstatus_set()
    https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
[2] https://mails.dpdk.org/archives/test-report/2024-April/637966.html

Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
v7: add rte_atomic_store_explicit to get link status which address
    Morten's comment, and add Morten's acked-by.
v6: fix DPDK CI compiler error with qos_sched.
v5: fix DPDK CI compiler error due to GCC extension [-Werror=pedantic]
v4: fix DPDK CI compiler error with cxgbe.
v3: fix checkpatch warning "missing --in-reply-to".
v2: add RTE_ATOMIC(uint64_t) wrap which address Morten's comment.
---
 drivers/net/cxgbe/cxgbe_ethdev.c |  3 ++-
 examples/qos_sched/init.c        |  3 ++-
 lib/ethdev/ethdev_driver.h       | 25 +++++++++----------------
 lib/ethdev/rte_ethdev.h          | 17 +++++++++++------
 4 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
index a27b9b266e..9b6a3651f9 100644
--- a/drivers/net/cxgbe/cxgbe_ethdev.c
+++ b/drivers/net/cxgbe/cxgbe_ethdev.c
@@ -211,9 +211,9 @@ int cxgbe_dev_link_update(struct rte_eth_dev *eth_dev,
 	unsigned int i, work_done, budget = 32;
 	struct link_config *lc = &pi->link_cfg;
 	struct adapter *adapter = pi->adapter;
-	struct rte_eth_link new_link = { 0 };
 	u8 old_link = pi->link_cfg.link_ok;
 	struct sge *s = &adapter->sge;
+	struct rte_eth_link new_link;
 
 	for (i = 0; i < CXGBE_LINK_STATUS_POLL_CNT; i++) {
 		if (!s->fw_evtq.desc)
@@ -232,6 +232,7 @@ int cxgbe_dev_link_update(struct rte_eth_dev *eth_dev,
 		rte_delay_ms(CXGBE_LINK_STATUS_POLL_MS);
 	}
 
+	memset(&new_link, 0, sizeof(new_link));
 	new_link.link_status = cxgbe_force_linkup(adapter) ?
 			       RTE_ETH_LINK_UP : pi->link_cfg.link_ok;
 	new_link.link_autoneg = (lc->link_caps & FW_PORT_CAP32_ANEG) ? 1 : 0;
diff --git a/examples/qos_sched/init.c b/examples/qos_sched/init.c
index d8abae635a..32964fd57e 100644
--- a/examples/qos_sched/init.c
+++ b/examples/qos_sched/init.c
@@ -335,7 +335,7 @@ int app_init(void)
 	for(i = 0; i < nb_pfc; i++) {
 		uint32_t socket = rte_lcore_to_socket_id(qos_conf[i].rx_core);
 		struct rte_ring *ring;
-		struct rte_eth_link link = {0};
+		struct rte_eth_link link;
 		int retry_count = 100, retry_delay = 100; /* try every 100ms for 10 sec */
 
 		snprintf(ring_name, MAX_NAME_LEN, "ring-%u-%u", i, qos_conf[i].rx_core);
@@ -367,6 +367,7 @@ int app_init(void)
 		app_init_port(qos_conf[i].rx_port, qos_conf[i].mbuf_pool);
 		app_init_port(qos_conf[i].tx_port, qos_conf[i].mbuf_pool);
 
+		memset(&link, 0, sizeof(link));
 		rte_eth_link_get(qos_conf[i].tx_port, &link);
 		if (link.link_status == 0)
 			printf("Waiting for link on port %u\n", qos_conf[i].tx_port);
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 0dbf2dd6a2..a7159cd1b1 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1674,18 +1674,13 @@ static inline int
 rte_eth_linkstatus_set(struct rte_eth_dev *dev,
 		       const struct rte_eth_link *new_link)
 {
-	RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev->data->dev_link);
-	union {
-		uint64_t val64;
-		struct rte_eth_link link;
-	} orig;
-
-	RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
+	struct rte_eth_link old_link;
 
-	orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const uint64_t *)new_link,
-					rte_memory_order_seq_cst);
+	old_link.val64 = rte_atomic_exchange_explicit(&dev->data->dev_link.val64,
+						      new_link->val64,
+						      rte_memory_order_seq_cst);
 
-	return (orig.link.link_status == new_link->link_status) ? -1 : 0;
+	return (old_link.link_status == new_link->link_status) ? -1 : 0;
 }
 
 /**
@@ -1701,12 +1696,10 @@ static inline void
 rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
 		       struct rte_eth_link *link)
 {
-	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data->dev_link);
-	uint64_t *dst = (uint64_t *)link;
-
-	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
-
-	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
+	rte_atomic_store_explicit(&link->val64,
+				  rte_atomic_load_explicit(&dev->data->dev_link.val64,
+							   rte_memory_order_seq_cst),
+				  rte_memory_order_seq_cst);
 }
 
 /**
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 147257d6a2..548fada1c7 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -332,12 +332,17 @@ struct rte_eth_stats {
 /**
  * A structure used to retrieve link-level information of an Ethernet port.
  */
-__extension__
-struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64 read/write */
-	uint32_t link_speed;        /**< RTE_ETH_SPEED_NUM_ */
-	uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
-	uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
-	uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
+struct rte_eth_link {
+	union {
+		RTE_ATOMIC(uint64_t) val64; /**< used for atomic64 read/write */
+		__extension__
+		struct {
+			uint32_t link_speed;	    /**< RTE_ETH_SPEED_NUM_ */
+			uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
+			uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
+			uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
+		};
+	};
 };
 
 /**@{@name Link negotiation
-- 
2.17.1

Re: [PATCH v7] ethdev: fix strict aliasing lead to link cannot be up

[Ferruh Yigit]

On 4/18/2024 8:28 AM, Chengwen Feng wrote:
> @@ -1701,12 +1696,10 @@ static inline void
>  rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
>  		       struct rte_eth_link *link)
>  {
> -	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data->dev_link);
> -	uint64_t *dst = (uint64_t *)link;
> -
> -	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> -
> -	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
> +	rte_atomic_store_explicit(&link->val64,
> +				  rte_atomic_load_explicit(&dev->data->dev_link.val64,
> +							   rte_memory_order_seq_cst),
> +				  rte_memory_order_seq_cst);
>

Hi Chengwen,

Overall looks good to me, but to increase readability what do you think
to extract function call in the function param, like
```
struct rte_eth_link new_link;
new_link->val64 = rte_atomic_load_explicit(..dev_link.val64, ..);
rte_atomic_store_explicit(&link->val64, new_link->val64, ..);
```

Re: [PATCH v7] ethdev: fix strict aliasing lead to link cannot be up

[Ferruh Yigit]

On 4/18/2024 8:28 AM, Chengwen Feng wrote:
> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
> which will lead the hns3 NIC can't link up. The root cause is strict
> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
> [1] for more details.
> 
> This commit use union to avoid such aliasing violation.
> 
> Note: DPDK CI report compiler error (see [2] for more details):
> ../drivers/net/cxgbe/cxgbe_ethdev.c:214:9: error: missing braces around
> initializer [-Werror=missing-braces]
>   struct rte_eth_link new_link = { 0 };
> The same error with qos_sched example:
> ../examples/qos_sched/init.c:338:10: error: missing braces around
> initializer [-Werror=missing-braces]
>    struct rte_eth_link link = {0};
> So this commit replace { 0 } with memset in cxgbe and qos_sched.
> 

As this commit is already fixing the build errors, not sure if there is
a value to provide reference to errors, you can briefly describe change
something like:
"The impacted components have been adapted to the struct change."


> [1] Strict aliasing problem with rte_eth_linkstatus_set()
>     https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
>

I wasn't aware marc.info, but for consistency you can use DPDK mail list
archive, inbox.dpdk.org, like:
https://inbox.dpdk.org/dev/8175c905-e661-b910-7f20-59b6ab605c38@huawei.com/

> [2] https://mails.dpdk.org/archives/test-report/2024-April/637966.html
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>

[PATCH v8] ethdev: fix strict aliasing lead to link cannot be up

[Chengwen Feng]

Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
which will lead the hns3 NIC can't link up. The root cause is strict
aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
[1] for more details.

This commit use union to avoid such aliasing violation. Also the
impacted components (cxgbe and qos_sched) have been adapted to the
struct change.

[1] https://inbox.dpdk.org/dev/8175c905-e661-b910-7f20-59b6ab605c38@huawei.com/

Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
v8: address Ferruh's comments.
v7: add rte_atomic_store_explicit to get link status which address
    Morten's comment, and add Morten's acked-by.
v6: fix DPDK CI compiler error with qos_sched.
v5: fix DPDK CI compiler error due to GCC extension [-Werror=pedantic]
v4: fix DPDK CI compiler error with cxgbe.
v3: fix checkpatch warning "missing --in-reply-to".
v2: add RTE_ATOMIC(uint64_t) wrap which address Morten's comment.
---
 drivers/net/cxgbe/cxgbe_ethdev.c |  3 ++-
 examples/qos_sched/init.c        |  3 ++-
 lib/ethdev/ethdev_driver.h       | 24 +++++++++---------------
 lib/ethdev/rte_ethdev.h          | 17 +++++++++++------
 4 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
index a27b9b266e..9b6a3651f9 100644
--- a/drivers/net/cxgbe/cxgbe_ethdev.c
+++ b/drivers/net/cxgbe/cxgbe_ethdev.c
@@ -211,9 +211,9 @@ int cxgbe_dev_link_update(struct rte_eth_dev *eth_dev,
 	unsigned int i, work_done, budget = 32;
 	struct link_config *lc = &pi->link_cfg;
 	struct adapter *adapter = pi->adapter;
-	struct rte_eth_link new_link = { 0 };
 	u8 old_link = pi->link_cfg.link_ok;
 	struct sge *s = &adapter->sge;
+	struct rte_eth_link new_link;
 
 	for (i = 0; i < CXGBE_LINK_STATUS_POLL_CNT; i++) {
 		if (!s->fw_evtq.desc)
@@ -232,6 +232,7 @@ int cxgbe_dev_link_update(struct rte_eth_dev *eth_dev,
 		rte_delay_ms(CXGBE_LINK_STATUS_POLL_MS);
 	}
 
+	memset(&new_link, 0, sizeof(new_link));
 	new_link.link_status = cxgbe_force_linkup(adapter) ?
 			       RTE_ETH_LINK_UP : pi->link_cfg.link_ok;
 	new_link.link_autoneg = (lc->link_caps & FW_PORT_CAP32_ANEG) ? 1 : 0;
diff --git a/examples/qos_sched/init.c b/examples/qos_sched/init.c
index d8abae635a..32964fd57e 100644
--- a/examples/qos_sched/init.c
+++ b/examples/qos_sched/init.c
@@ -335,7 +335,7 @@ int app_init(void)
 	for(i = 0; i < nb_pfc; i++) {
 		uint32_t socket = rte_lcore_to_socket_id(qos_conf[i].rx_core);
 		struct rte_ring *ring;
-		struct rte_eth_link link = {0};
+		struct rte_eth_link link;
 		int retry_count = 100, retry_delay = 100; /* try every 100ms for 10 sec */
 
 		snprintf(ring_name, MAX_NAME_LEN, "ring-%u-%u", i, qos_conf[i].rx_core);
@@ -367,6 +367,7 @@ int app_init(void)
 		app_init_port(qos_conf[i].rx_port, qos_conf[i].mbuf_pool);
 		app_init_port(qos_conf[i].tx_port, qos_conf[i].mbuf_pool);
 
+		memset(&link, 0, sizeof(link));
 		rte_eth_link_get(qos_conf[i].tx_port, &link);
 		if (link.link_status == 0)
 			printf("Waiting for link on port %u\n", qos_conf[i].tx_port);
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 0dbf2dd6a2..883e59a927 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1674,18 +1674,13 @@ static inline int
 rte_eth_linkstatus_set(struct rte_eth_dev *dev,
 		       const struct rte_eth_link *new_link)
 {
-	RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev->data->dev_link);
-	union {
-		uint64_t val64;
-		struct rte_eth_link link;
-	} orig;
-
-	RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
+	struct rte_eth_link old_link;
 
-	orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const uint64_t *)new_link,
-					rte_memory_order_seq_cst);
+	old_link.val64 = rte_atomic_exchange_explicit(&dev->data->dev_link.val64,
+						      new_link->val64,
+						      rte_memory_order_seq_cst);
 
-	return (orig.link.link_status == new_link->link_status) ? -1 : 0;
+	return (old_link.link_status == new_link->link_status) ? -1 : 0;
 }
 
 /**
@@ -1701,12 +1696,11 @@ static inline void
 rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
 		       struct rte_eth_link *link)
 {
-	RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data->dev_link);
-	uint64_t *dst = (uint64_t *)link;
-
-	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
+	struct rte_eth_link curr_link;
 
-	*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
+	curr_link.val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
+						   rte_memory_order_seq_cst);
+	rte_atomic_store_explicit(&link->val64, curr_link.val64, rte_memory_order_seq_cst);
 }
 
 /**
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 147257d6a2..548fada1c7 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -332,12 +332,17 @@ struct rte_eth_stats {
 /**
  * A structure used to retrieve link-level information of an Ethernet port.
  */
-__extension__
-struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64 read/write */
-	uint32_t link_speed;        /**< RTE_ETH_SPEED_NUM_ */
-	uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
-	uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
-	uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
+struct rte_eth_link {
+	union {
+		RTE_ATOMIC(uint64_t) val64; /**< used for atomic64 read/write */
+		__extension__
+		struct {
+			uint32_t link_speed;	    /**< RTE_ETH_SPEED_NUM_ */
+			uint16_t link_duplex  : 1;  /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
+			uint16_t link_autoneg : 1;  /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
+			uint16_t link_status  : 1;  /**< RTE_ETH_LINK_[DOWN/UP] */
+		};
+	};
 };
 
 /**@{@name Link negotiation
-- 
2.17.1

Re: [PATCH v7] ethdev: fix strict aliasing lead to link cannot be up

[fengchengwen]

Hi Ferruh,

On 2024/4/19 23:25, Ferruh Yigit wrote:
> On 4/18/2024 8:28 AM, Chengwen Feng wrote:
>> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
>> which will lead the hns3 NIC can't link up. The root cause is strict
>> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
>> [1] for more details.
>>
>> This commit use union to avoid such aliasing violation.
>>
>> Note: DPDK CI report compiler error (see [2] for more details):
>> ../drivers/net/cxgbe/cxgbe_ethdev.c:214:9: error: missing braces around
>> initializer [-Werror=missing-braces]
>>   struct rte_eth_link new_link = { 0 };
>> The same error with qos_sched example:
>> ../examples/qos_sched/init.c:338:10: error: missing braces around
>> initializer [-Werror=missing-braces]
>>    struct rte_eth_link link = {0};
>> So this commit replace { 0 } with memset in cxgbe and qos_sched.
>>
> 
> As this commit is already fixing the build errors, not sure if there is
> a value to provide reference to errors, you can briefly describe change
> something like:
> "The impacted components have been adapted to the struct change."

ok

> 
> 
>> [1] Strict aliasing problem with rte_eth_linkstatus_set()
>>     https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
>>
> 
> I wasn't aware marc.info, but for consistency you can use DPDK mail list
> archive, inbox.dpdk.org, like:
> https://inbox.dpdk.org/dev/8175c905-e661-b910-7f20-59b6ab605c38@huawei.com/

ok

both done in v8.

Thanks

> 
>> [2] https://mails.dpdk.org/archives/test-report/2024-April/637966.html
>>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> 
> .
> 

Re: [PATCH v8] ethdev: fix strict aliasing lead to link cannot be up

[Ferruh Yigit]

On 4/22/2024 7:38 AM, Chengwen Feng wrote:
> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
> which will lead the hns3 NIC can't link up. The root cause is strict
> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
> [1] for more details.
> 
> This commit use union to avoid such aliasing violation. Also the
> impacted components (cxgbe and qos_sched) have been adapted to the
> struct change.
> 
> [1] https://inbox.dpdk.org/dev/8175c905-e661-b910-7f20-59b6ab605c38@huawei.com/
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Applied to dpdk-next-net/main, thanks.