[Intel-wired-lan] [PATCH Linux-6.8-rc5 1/1] ixgbevf: start negotiate with api version 1.4

[yifei.l.liu at oracle.com]

> Hi Paul,
> I apologize for possible html text in the previous email. I cleared 
> the format and resend them as plain text.
> Thank you.
> Yifei
>
> From: Yifei Liu <yifei.l.liu at oracle.com>
> Sent: Monday, March 4, 2024 1:48 PM
> To: Paul Menzel <pmenzel at molgen.mpg.de>
> Cc: jesse.brandeburg at intel.com <jesse.brandeburg at intel.com>; 
> anthony.l.nguyen at intel.com <anthony.l.nguyen at intel.com>; 
> davem at davemloft.net <davem at davemloft.net>; edumazet at google.com 
> <edumazet at google.com>; kuba at kernel.org <kuba at kernel.org>; 
> pabeni at redhat.com <pabeni at redhat.com>; lihong.yang at intel.com 
> <lihong.yang at intel.com>; Harshit Mogalapalli 
> <harshit.m.mogalapalli at oracle.com>; linux-kernel at vger.kernel.org 
> <linux-kernel at vger.kernel.org>; intel-wired-lan at lists.osuosl.org 
> <intel-wired-lan at lists.osuosl.org>; Jack Vogel 
> <jack.vogel at oracle.com>; netdev at vger.kernel.org 
> <netdev at vger.kernel.org>; Ramanan Govindarajan 
> <ramanan.govindarajan at oracle.com>
> Subject: Re: [Intel-wired-lan] [PATCH Linux-6.8-rc5 1/1] ixgbevf: 
> start negotiate with api version 1.4
> Hi Paul
> Thank you for your replay. Please see inline.
> From: Paul Menzel <pmenzel at molgen.mpg.de>
> Date: Saturday, March 2, 2024 at 12:20 AM
> To: Yifei Liu <yifei.l.liu at oracle.com>
> Cc: jesse.brandeburg at intel.com <jesse.brandeburg at intel.com>, 
> anthony.l.nguyen at intel.com <anthony.l.nguyen at intel.com>, 
> davem at davemloft.net <davem at davemloft.net>, edumazet at google.com 
> <edumazet at google.com>, kuba at kernel.org <kuba at kernel.org>, 
> pabeni at redhat.com <pabeni at redhat.com>, lihong.yang at intel.com 
> <lihong.yang at intel.com>, Harshit Mogalapalli 
> <harshit.m.mogalapalli at oracle.com>, linux-kernel at vger.kernel.org 
> <linux-kernel at vger.kernel.org>, intel-wired-lan at lists.osuosl.org 
> <intel-wired-lan at lists.osuosl.org>, Jack Vogel 
> <jack.vogel at oracle.com>, netdev at vger.kernel.org 
> <netdev at vger.kernel.org>, Ramanan Govindarajan 
> <ramanan.govindarajan at oracle.com>
> Subject: Re: [Intel-wired-lan] [PATCH Linux-6.8-rc5 1/1] ixgbevf: 
> start negotiate with api version 1.4
> Dear Yifei,
>
>
> Thank you very much for your patch.
>
> Am 02.03.24 um 00:58 schrieb Yifei Liu:
> > ixgbevf updates to api version to 1.5 via
> >        commit 339f28964147d ("ixgbevf: Add support for new mailbox
> >        communication between PF and VF")
> > while the pf side is not updated to 1.5 properly. It will lead to a
> > failure of negotiation of api version 1.5 This commit will enforce
> > the negotiation to start with 1.4 which is working fine.
> >
> > Normally the pf and vf side should be updated together. Example:
> >        commit adef9a26d6c39 ("ixgbevf: add defines for IPsec offload 
> request")
> >        commit 7269824046376 ("ixgbe: add VF IPsec offload request 
> message handling")
>
> Why can’t the PF side not be updated to version 1.5 too?
>
> I tried to add the new api version to the switch in pf side. However, 
> that would lead to another issue. Function ixgbe_read_mbx_pf() returns 
> an error code -100, which should be IXGBE_ERR_MBX. The root cause of 
> this is function ixgbe_obtain_mbx_lock_pf returns that error code. It 
> is likely to be a hardware issue communicating with the Ethernet card 
> (IXGBE_READ_REG returns a failure)
>
> If you don’t mind, I’d format the commit message like below.
>
> Sure thanks
>
> Commit 339f28964147d ("ixgbevf: Add support for new mailbox communication
> between PF and VF") updates the driver ixgbevf to API version 1.5 
> while the
> pf side is not updated to 1.5 properly. This leads to a negotiation 
> failure
> of api version 1.5. So, enforce the negotiation to start with 1.4 which is
> working fine.
>
> Normally the pf and vf side should be updated together. Example:
>
> 1.  commit adef9a26d6c39 ("ixgbevf: add defines for IPsec offload 
> request")
> 2.  commit 7269824046376 ("ixgbe: add VF IPsec offload request message
> handling")
>
> > Reported-by: Manjunatha Gowda <manjunatha.gowda at oracle.com>
> > Signed-off-by: Yifei Liu <yifei.l.liu at oracle.com>
> > Reviewed-by: Jack Vogel <jack.vogel at oracle.com>
>
> Please add a Fixes: tag.
>
> Fixes: 39f28964147d ("ixgbevf: Add support for new mailbox communication
> between PF and VF")
>
> Sure. Do I need to resend the patch with the fixes tag and new commit 
> message?
>
> Unfortunately, I am unable to find this commit hash. What archive/tree
> is it from?
> The commit message is 339f28964147d. It seems you missed the one of 
> the double 3 at the very beginning. It is in linux-stable from Linux 
> 5.17.y
>
> > ---
> >   drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c 
> b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > index a44e4bd56142..a1b9b789d1d4 100644
> > --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > @@ -2286,6 +2286,12 @@ static void ixgbevf_negotiate_api(struct 
> ixgbevf_adapter *adapter)
> >
> >        spin_lock_bh(&adapter->mbx_lock);
> >
> > +     /* There is no corresponding drivers in pf for
> > +      * api version 1.5. Try to negociate with version
>
> negotiate
>
> > +      * 1.5 will always fail. Start to negociate with
> > +      * version 1.4.
>
> Could you please use the fully allowed line length, so less lines are 
> used?
>
> > +      */
> > +     idx = 1; >       while (api[idx] != ixgbe_mbox_api_unknown) {
> >                err = hw->mac.ops.negotiate_api_version(hw, api[idx]);
> >                if (!err)
>
> Where is `idx` set before?
>
> idx was 0 before in line 2285.
> int err, idx = 0;
>
> Unrelated to the problem at hand, but enums or macros should be used for
> the API version.
>
> I agree. But for this case, there is an integer array defined before 
> with a reversed sequence of the api version enum. (the desired attempt 
> sequence is from newest to oldest) It may be more readable to use 
> index of the api array. (e.g. api[0] means ixgbe_mbox_api_15, which is 
> enum 6)
> FYI, the api array starting from line 2276 in 
> /drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> static const int api[] = {
> ixgbe_mbox_api_15,
> ixgbe_mbox_api_14,
> ixgbe_mbox_api_13,
> ixgbe_mbox_api_12,
> ixgbe_mbox_api_11,
> ixgbe_mbox_api_10,
> ixgbe_mbox_api_unknown
> };
>
>
> Kind regards,
>
> Paul
> Thank you again.
> Yifei