On Tue, Oct 15, 2024 at 6:23 PM Bruce Richardson
<bruce.richard...@intel.com> wrote:
>
> On Tue, Oct 01, 2024 at 11:15:07AM +0200, David Marchand wrote:
> > Caught by code review.
> > The mailbox message maximum size is larger than the biggest message
> > to update mac addresses.
> > Remove the double loop and add some build / assert checks.
> >
> > This cleanup also fixes a (never hit) issue where multiple mac addresses
> > would be marked as VIRTCHNL_ETHER_ADDR_PRIMARY if multiple messages had
> > been effectively sent.
> >
> > Signed-off-by: David Marchand <david.march...@redhat.com>
> > ---
>
> Thanks David. The overall approach looks good. While not familiar (yet)
> with the iavf to pf protocol and messaging, some review comments inline
> below.
>
> /Bruce
>
> > drivers/net/iavf/iavf_vchnl.c | 86 ++++++++++++++---------------------
> > 1 file changed, 34 insertions(+), 52 deletions(-)
> >
> > diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
> > index 065ab3594c..e5113605ac 100644
> > --- a/drivers/net/iavf/iavf_vchnl.c
> > +++ b/drivers/net/iavf/iavf_vchnl.c
> > @@ -1397,62 +1397,44 @@ void
> > iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add)
> > {
> > struct virtchnl_ether_addr_list *list;
> > + char buf[sizeof(*list) + sizeof(struct virtchnl_ether_addr) *
> > IAVF_NUM_MACADDR_MAX];
>
> The struct virtchnl_ether_addr_list variable includes a virtchnl_ether_addr
> element in it, so I think we may have an off-by-one error in a number of
> places here. For example, should the multiplication not be
> sizeof(...) * (IAVF_NUM_MACADDR_MAX - 1).
I think the previous code had the same issue.
IIUC, it should not be a problem: the message would be larger than
actual content, but the header contains the exact number of
transported macs (virtchnl_ether_addr_list::num_elements).
>
> The overall logic I think would be more sane if the 'list' element of the
> list structure was made an undimensioned array rather than being size 1,
> but that's a more significant change.
Yes, that would be the cleaner approach.
Note that this struct is not the only one in virtchnl.h which has such issue.
And now, as I was diffing dpdk and kernel virtchnl.h, I can see they
went through the same approach to fix this, not that long ago.
5e7f59fa07f8 ("virtchnl: fix fake 1-elem arrays in structures
allocated as `nents + 1`")
It would be worth cleaning all of those, but I prefer to postpone this
for a separate patch.
I'll relook at your other comments but they look ok to me, I'll handle
those for v2 after rc1.
--
David Marchand
