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

  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(-)
  

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
  

Marvell bnx2x PMD maintainers are silent. Is the PMD not maintained anymore?

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Thursday, 9 February 2023 17.50
> To: dev@dpdk.org; rmody@marvell.com; [...]
> Cc: shshaikh@marvell.com; [...]
> Subject: RE: [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy
> lengths
> 
> 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>
  

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.
  

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?
  

++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?
>
  

-----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>
  

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.
  

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.