RE: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

RE: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Morten Brørup]

[-- Attachment #1: Type: text/plain, Size: 842 bytes --]

Please wait. Jerin is chasing this internally.-MortenSent from smartphone. Please excuse brevity and spelling.
-------- Oprindelig besked --------Fra: David Marchand <david.marchand@redhat.com> Dato: 09.03.2023  11.26  (GMT+01:00) Til: Morten Brørup <mb@smartsharesystems.com>, rmody@marvell.com, shshaikh@marvell.com Cc: dev@dpdk.org, Thomas Monjalon <thomas@monjalon.net> Emne: Re: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths On Thu, Feb 9, 2023 at 5:49 PM Morten Brørup <mb@smartsharesystems.com> wrote:>> PING bnx2x maintainers. Care to review this bugfix, so it can be included in 23.03?>Still no luck in getting attention from bnx2x maintainers.One option is to declare this driver as UNMAINTAINED and disable itscompilation when the memcpy annotations are finalised and merged in anext release.-- David Marchand

[-- Attachment #2: Type: text/html, Size: 1512 bytes --]

Re: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Jerin Jacob]

On Thu, Mar 9, 2023 at 9:53 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Thu, 9 Feb 2023 17:49:31 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > >      rte_memcpy(old, new, sizeof(struct nig_stats));
> > >
> > > -    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats-
> > > >mac_stx[1]),
> > > -      sizeof(struct mac_stx));
> > > +   rte_memcpy(RTE_PTR_ADD(estats,
> > > +                   offsetof(struct bnx2x_eth_stats,
> > > rx_stat_ifhcinbadoctets_hi)),
> > > +                   &pstats->mac_stx[1], sizeof(struct mac_stx));
>
> Stop using rte_memcpy() in slow path like this.
> memcpy() is just as fast, compiler can optimize, and the checking tools
> are better with it.

+1

@Morten Brørup Could you send the next version? I am marking as Change
requested.

Re: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Stephen Hemminger]

On Thu, 9 Feb 2023 17:49:31 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:

> >      rte_memcpy(old, new, sizeof(struct nig_stats));
> > 
> > -    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats-  
> > >mac_stx[1]),  
> > -	   sizeof(struct mac_stx));
> > +	rte_memcpy(RTE_PTR_ADD(estats,
> > +			offsetof(struct bnx2x_eth_stats,
> > rx_stat_ifhcinbadoctets_hi)),
> > +			&pstats->mac_stx[1], sizeof(struct mac_stx));

Stop using rte_memcpy() in slow path like this.
memcpy() is just as fast, compiler can optimize, and the checking tools
are better with it.

Re: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Devendra Singh Rawat]

-----Original Message-----
From: Akhil Goyal <gakhil@marvell.com>
Sent: 09 March 2023 21:41
To: Thomas Monjalon <thomas@monjalon.net>; David Marchand <david.marchand@redhat.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>
Cc: Morten Brørup <mb@smartsharesystems.com>; Rasesh Mody <rmody@marvell.com>; Shahed Shaikh <shshaikh@marvell.com>; dev@dpdk.org; Alok Prasad <palok@marvell.com>
Subject: RE: [EXT] Re: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

++Alok Prasad

> 09/03/2023 11:25, David Marchand:
> > On Thu, Feb 9, 2023 at 5:49 PM Morten Brørup
> <mb@smartsharesystems.com> wrote:
> > >
> > > PING bnx2x maintainers. Care to review this bugfix, so it can be 
> > > included in
> 23.03?
> > >
> >
> > Still no luck in getting attention from bnx2x maintainers.
> > One option is to declare this driver as UNMAINTAINED and disable its 
> > compilation when the memcpy annotations are finalised and merged in 
> > a next release.
> 
> Yes that's an option.
> Jerin, what is your opinion?
> 

Both the maintainers for bnx2x PMD have left Marvell, We are in process of finalizing new maintainers for this PMD.
Meanwhile I have reviewed the patch and I am Acking it.

Acked-by: Devendra Singh Rawat <dsinghrawat@marvell.com>

Re: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Thomas Monjalon]

09/03/2023 11:25, David Marchand:
> On Thu, Feb 9, 2023 at 5:49 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > PING bnx2x maintainers. Care to review this bugfix, so it can be included in 23.03?
> >
> 
> Still no luck in getting attention from bnx2x maintainers.
> One option is to declare this driver as UNMAINTAINED and disable its
> compilation when the memcpy annotations are finalised and merged in a
> next release.

Yes that's an option.
Jerin, what is your opinion?

Re: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[David Marchand]

On Thu, Feb 9, 2023 at 5:49 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> PING bnx2x maintainers. Care to review this bugfix, so it can be included in 23.03?
>

Still no luck in getting attention from bnx2x maintainers.
One option is to declare this driver as UNMAINTAINED and disable its
compilation when the memcpy annotations are finalised and merged in a
next release.


-- 
David Marchand

RE: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Morten Brørup]

PING bnx2x maintainers. Care to review this bugfix, so it can be included in 23.03?

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Monday, 16 January 2023 14.07
> 
> Bugfix: The vlan in the bulletin does not contain a VLAN header, only
> the
> VLAN ID, so only copy 2 byte, not 4. The target structure has padding
> after the field, so copying 2 byte too many is effectively harmless.
> There is no need to backport this patch.
> 
> Use RTE_PTR_ADD where copying arrays to the offset of a first field in
> a
> structure holding multiple fields, to avoid compiler warnings with
> decorated rte_memcpy.
> 
> Bugzilla ID: 1146
> 
> Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core")
> Cc: stephen@networkplumber.org
> Cc: rmody@marvell.com
> Cc: shshaikh@marvell.com
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> v7:
> * No changes.
> v6:
> * Add Fixes to patch description.
> * Fix checkpatch warnings.
> v5:
> * No changes.
> v4:
> * Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
> v3:
> * First patch in series.
> ---
>  drivers/net/bnx2x/bnx2x_stats.c | 11 +++++++----
>  drivers/net/bnx2x/bnx2x_vfpf.c  |  2 +-
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/bnx2x/bnx2x_stats.c
> b/drivers/net/bnx2x/bnx2x_stats.c
> index c07b01510a..bc4a8b8e71 100644
> --- a/drivers/net/bnx2x/bnx2x_stats.c
> +++ b/drivers/net/bnx2x/bnx2x_stats.c
> @@ -819,8 +819,9 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
> 
>      rte_memcpy(old, new, sizeof(struct nig_stats));
> 
> -    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats-
> >mac_stx[1]),
> -	   sizeof(struct mac_stx));
> +	rte_memcpy(RTE_PTR_ADD(estats,
> +			offsetof(struct bnx2x_eth_stats,
> rx_stat_ifhcinbadoctets_hi)),
> +			&pstats->mac_stx[1], sizeof(struct mac_stx));
>      estats->brb_drop_hi = pstats->brb_drop_hi;
>      estats->brb_drop_lo = pstats->brb_drop_lo;
> 
> @@ -1492,9 +1493,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
>  		REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
>  	if (!CHIP_IS_E3(sc)) {
>  		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
> -				&(sc->port.old_nig_stats.egress_mac_pkt0_lo),
> 2);
> +				RTE_PTR_ADD(&sc->port.old_nig_stats,
> +				offsetof(struct nig_stats,
> egress_mac_pkt0_lo)), 2);
>  		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
> -				&(sc->port.old_nig_stats.egress_mac_pkt1_lo),
> 2);
> +				RTE_PTR_ADD(&sc->port.old_nig_stats,
> +				offsetof(struct nig_stats,
> egress_mac_pkt1_lo)), 2);
>  	}
> 
>  	/* function stats */
> diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c
> b/drivers/net/bnx2x/bnx2x_vfpf.c
> index 63953c2979..87631c76ca 100644
> --- a/drivers/net/bnx2x/bnx2x_vfpf.c
> +++ b/drivers/net/bnx2x/bnx2x_vfpf.c
> @@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
>  	if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac, sc-
> >old_bulletin.mac, ETH_ALEN))
>  		rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
>  	if (valid_bitmap & (1 << VLAN_VALID))
> -		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan,
> RTE_VLAN_HLEN);
> +		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan,
> sizeof(bull->vlan));
> 
>  	sc->old_bulletin = *bull;
> 
> --
> 2.17.1

[PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths

[Morten Brørup]

Bugfix: The vlan in the bulletin does not contain a VLAN header, only the
VLAN ID, so only copy 2 byte, not 4. The target structure has padding
after the field, so copying 2 byte too many is effectively harmless.
There is no need to backport this patch.

Use RTE_PTR_ADD where copying arrays to the offset of a first field in a
structure holding multiple fields, to avoid compiler warnings with
decorated rte_memcpy.

Bugzilla ID: 1146

Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core")
Cc: stephen@networkplumber.org
Cc: rmody@marvell.com
Cc: shshaikh@marvell.com

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v7:
* No changes.
v6:
* Add Fixes to patch description.
* Fix checkpatch warnings.
v5:
* No changes.
v4:
* Type casting did not fix the warnings, so use RTE_PTR_ADD instead.
v3:
* First patch in series.
---
 drivers/net/bnx2x/bnx2x_stats.c | 11 +++++++----
 drivers/net/bnx2x/bnx2x_vfpf.c  |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bnx2x/bnx2x_stats.c b/drivers/net/bnx2x/bnx2x_stats.c
index c07b01510a..bc4a8b8e71 100644
--- a/drivers/net/bnx2x/bnx2x_stats.c
+++ b/drivers/net/bnx2x/bnx2x_stats.c
@@ -819,8 +819,9 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc)
 
     rte_memcpy(old, new, sizeof(struct nig_stats));
 
-    rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[1]),
-	   sizeof(struct mac_stx));
+	rte_memcpy(RTE_PTR_ADD(estats,
+			offsetof(struct bnx2x_eth_stats, rx_stat_ifhcinbadoctets_hi)),
+			&pstats->mac_stx[1], sizeof(struct mac_stx));
     estats->brb_drop_hi = pstats->brb_drop_hi;
     estats->brb_drop_lo = pstats->brb_drop_lo;
 
@@ -1492,9 +1493,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc)
 		REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38);
 	if (!CHIP_IS_E3(sc)) {
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2);
+				RTE_PTR_ADD(&sc->port.old_nig_stats,
+				offsetof(struct nig_stats, egress_mac_pkt0_lo)), 2);
 		REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50,
-				&(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2);
+				RTE_PTR_ADD(&sc->port.old_nig_stats,
+				offsetof(struct nig_stats, egress_mac_pkt1_lo)), 2);
 	}
 
 	/* function stats */
diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfpf.c
index 63953c2979..87631c76ca 100644
--- a/drivers/net/bnx2x/bnx2x_vfpf.c
+++ b/drivers/net/bnx2x/bnx2x_vfpf.c
@@ -54,7 +54,7 @@ bnx2x_check_bull(struct bnx2x_softc *sc)
 	if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac, sc->old_bulletin.mac, ETH_ALEN))
 		rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN);
 	if (valid_bitmap & (1 << VLAN_VALID))
-		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN);
+		rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, sizeof(bull->vlan));
 
 	sc->old_bulletin = *bull;
 
-- 
2.17.1