[dpdk-dev] i40e: pci probe fails when using one bogus sfp

[Olivier MATZ]

Hi,

On Tue, 13 Jun 2017 14:14:43 +0000, "Wu, Jingjing" <jingjing.wu at intel.com> wrote:
> > -----Original Message-----
> > From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> > Sent: Tuesday, June 13, 2017 4:28 PM
> > To: Wu, Jingjing <jingjing.wu at intel.com>
> > Cc: Xing, Beilei <beilei.xing at intel.com>; Richardson, Bruce <bruce.richardson at intel.com>;
> > Zhang, Helin <helin.zhang at intel.com>; dev at dpdk.org
> > Subject: Re: [dpdk-dev] i40e: pci probe fails when using one bogus sfp
> > 
> > Hi Jingjing,
> > 
> > On Mon, 12 Jun 2017 16:23:47 +0000, "Wu, Jingjing" <jingjing.wu at intel.com> wrote:  
> > > HI, Olivier
> > >  
> > > > Thank you for your quick answer.
> > > >
> > > > Yes, the pci probing continues for the other ports even if one port
> > > > failed (since v17.05, commit 10f6c93cea).
> > > >
> > > > But I find a bit strange to have this check about the SFP in the
> > > > PCI probing function instead of having it the port initialization
> > > > function. My understanding is the SFP check is not related to PCI
> > > > probing. Is it consistent with other drivers?
> > > >  
> > > Could your customer help to check what is the exactly error code is by
> > > Checking the "hw->aq.asq_last_status" when eth_i40e_dev_init() fails.  
> > 
> > I'm afraid it won't be possible, since it's a random issue that is
> > not reproducible.
> > 
> > What do you think about adding a log in i40e_dev_sync_phy_type() to
> > display the status value in case of failure? It would help for next
> > times. I can submit a patch for that if you want.
> >  
> Yes, It makes sense. Thanks for doing that.
> 
> > > Yes, it seems better PHY init fails doesn't block PCI probe. Just compared with i40e
> > > Kernel version, PHY init fails doesn't block CPI probe. And there is watchdog task to
> > > Check the PHY status. But DPDK is polling mode, If PCI probe fails, PCI probe continues,
> > > then application need poll PHY status to support SFP change.  
> > 
> > From what I understand, i40e_dev_sync_phy_type() was added
> > to know the PHY capabilities, which useful for instance for devinfo().
> > Indeed, devinfo() can be call before the port is started, so
> > we need to have get the PHY capabilities before starting the port.
> > 
> > I've done a quick patch that:
> > - keeps the call to i40e_dev_sync_phy_type() in pci probing but
> >   continue in case of failure
> > - add another call to i40e_dev_sync_phy_type() in port start
> >   function
> >  
> I think this workaround would not introduce issue if SFP is ready on NIC before Probe.
> But I'm not sure if it can resolve the issue, because during probe, dozens of initialization work
> Is ongoing. I think we need to verify it PHY init during start phase works or not. Maybe we
> Need to do more than that? Not sure.

After discussion with Helin, I moved the call to i40e_dev_sync_phy_type()
in dev_configure(). I'm sending it as a reply to this mail.

I see if I can get my patch tested by the customer.


> 
> > I think it would solve the issue without impacting the result
> > of the devinfo() function. What would you think of a patch like
> > this?
> >   
> > > And I also checked ixgbe driver, it seems phy init is done at probe time.
> > > In my opinion, dev_start and dev_stop is meaning ready for receiving and transmitting
> > > packets, it may not be suitable to put it in the start/stop phase.  
> > 
> > I'm wondering: what would/should occur if the SFP is unplugged and
> > replugged while the port is running?
> > 
> > I suppose we don't have any PCI notification when unplugging/plugging
> > the SFP, so I'm not sure we should have this check at the PCI level,
> > because the application does not know if the bus has to be probed again.
> >  
> We can consider about unplugging/plugging as link event in DPDK. Now we
> have LSC or user can polling link status. Maybe that is a way to go.

Yes, I agree. Thanks for the confirmation.