On Fri, Feb 3, 2023 at 9:39 AM Yang, Qiming <qiming.y...@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: David Marchand <david.march...@redhat.com>
> > Sent: Thursday, February 2, 2023 9:49 PM
> > To: Su, Simei <simei...@intel.com>; Zhang, Yuying
> > <yuying.zh...@intel.com>; Xing, Beilei <beilei.x...@intel.com>; Zhang, Qi Z
> > <qi.z.zh...@intel.com>
> > Cc: dev@dpdk.org; Yang, Qiming <qiming.y...@intel.com>;
> > sta...@dpdk.org
> > Subject: Re: [PATCH v5] net/i40e: rework maximum frame size configuration
> >
> > On Thu, Feb 2, 2023 at 2:24 PM David Marchand
> > <david.march...@redhat.com> wrote:
> > >
> > > On Thu, Feb 2, 2023 at 1:37 PM Simei Su <simei...@intel.com> wrote:
> > > > @@ -2467,8 +2466,16 @@ i40e_dev_start(struct rte_eth_dev *dev)
> > > > "please call hierarchy_commit() "
> > > > "before starting the port");
> > > >
> > > > - max_frame_size = dev->data->mtu + I40E_ETH_OVERHEAD;
> > > > - i40e_set_mac_max_frame(dev, max_frame_size);
> > > > + max_frame_size = dev->data->mtu ?
> > > > + dev->data->mtu + I40E_ETH_OVERHEAD :
> > > > + I40E_FRAME_SIZE_MAX;
> > > > +
> > > > + /* Set the max frame size to HW*/
> > > > + ret = i40e_aq_set_mac_config(hw, max_frame_size, TRUE, false, 0,
> > NULL);
> > > > + if (ret) {
> > > > + PMD_DRV_LOG(ERR, "Fail to set mac config");
> > > > + return ret;
> > > > + }
> > >
> > > Reading this patch again.
> > >
> > > Returning here seems incorrect as we leave rx/tx queue in started state.
> > > Don't we need to jump to tx_err label on error?
> >
> > There is probably an issue with interrupt handler still being registered
> > too.
> > Qi, i40e maintainers, please review this patch carefully, and ping me when
> > it
> > is ready so that we can test it at RH.
> >
>
> This change will not break interrupt handler, the second parameter is waiting
> to complete. Just waiting more time to make sure adminq command execute
> completed. So that subsequent commands(MTU set) can be executed.
> And if you have other issues report at RH system, please report it.
If i40e_aq_set_mac_config() fails and we return directly, an interrupt
handler is left registered, don't you think it is an issue?
In any case, I will wait for the next revision before asking for tests
on my side.
Thanks.
--
David Marchand
