Re: [PATCH v5 2/4] net/bnx2x: fix warnings about rte_memcpy lengths

[David Marchand <david.marchand@redhat.com>]

Hello bnx2x maintainers,

On Wed, Dec 28, 2022 at 6:37 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Wednesday, 28 December 2022 18.03
> >
> > On Wed, 28 Dec 2022 17:38:56 +0100
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > > From: Stanisław Kardach [mailto:kda@semihalf.com]
> > > Sent: Wednesday, 28 December 2022 17.14
> > > > On Wed, Dec 28, 2022, 16:10 Morten Brørup
> > <mb@smartsharesystems.com> wrote:
> > > > > 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.
> > > > It is a small nitpick but why use rte_memcpy for a 2 byte / half-
> > word copy? Shouldn't assignment with casts be enough?
> > >
> > > Absolutely. It would also have prevented the bug to begin with.
> > > But in order to keep the changes minimal, I kept the rte_memcpy().
> >
> > For small fixed values compiler can optimize memcpy into one
> > instruction.
> > Not so with current rte_memcpy
>
> Good point, Stephen.
>
> I took another look at it, and the two byte rte_memcpy() is only used in a slow path function, so no need to optimize further.
>
> If the maintainers disagree, and want to optimize anyway, here's a proposal:
>
> /* check the mac address and VLAN and allocate memory if valid */
> if (valid_bitmap & (1 << MAC_ADDR_VALID) && !rte_is_same_ether_addr(
>                 (const struct rte_ether_addr *)&bull->mac,
>                 (const struct rte_ether_addr *)&sc->old_bulletin.mac))
>         rte_ether_addr_copy((const struct rte_ether_addr *)&bull->mac,
>                         (struct rte_ether_addr *)&sc->link_params.mac_addr);
> if (valid_bitmap & (1 << VLAN_VALID))
>         bull->vlan = sc->old_bulletin.vlan;
>

Please review/advise.
Thanks.


-- 
David Marchand