>> From: Mariusz Stachura <mariusz.stach...@intel.com> >> >> This patch introduces new ethtool private flag used for forcing true >> link state. Function i40e_force_link_state that implements this >> functionality was added, it sets phy_type = 0 in order to work-around >> firmware's LESM. False positive error messages were suppressed. >> >> The ndo_open() should not succeed if there were issues with forcing >> link state to be UP. >> >> Added I40E_PHY_TYPES_BITMASK define with all phy types OR-ed together >> in one bitmask. Added after phy type definition, so it will be hard >> to forget to include new phy types to the bitmask. >> >> CC: Jakub Kicinski <kubak...@wp.pl> >> Signed-off-by: Mariusz Stachura <mariusz.stach...@intel.com> >> Signed-off-by: Mitch Williams <mitch.a.willi...@intel.com> >> Tested-by: Andrew Bowers <andrewx.bow...@intel.com> >> Signed-off-by: Jeff Kirsher <jeffrey.t.kirs...@intel.com> > > Thanks, from functional perspective this looks reasonable AFAICT. > > We may want to have a conversation about priv flags like this at later time, > since I guess more NICs won't bring the link down when netdev is closed > today.. Although it maybe a larger endeavour, perhaps it would be cool to > communicate to the user the reason why MAC/PHY is on in some standard way.. > NC-SI and multi-host comes to mind. >
Yep, the main reason for not bringing the link down is NPAR and SRIOV functionality. Link is up by default, so I think it was assumed that there is no reason to inform the user about it. The operator can use private flag to force it down, assuming she/he knows it will disrupt any undergoing traffic. I get your idea, nonetheless it was designed to use slightly different approach. >> @@ -7524,6 +7596,9 @@ int i40e_open(struct net_device *netdev) >> >> netif_carrier_off(netdev); >> >> + if (i40e_force_link_state(pf, true)) >> + return -EAGAIN; > > There are some minor style issues with the rest of the patch, but here you > may want to (a) propagate the actual error value; > The actual error value is printed inside the i40e_force_link_state() function [with KERN_ERR severity], here I wanted to let the user know the generic information that the resource is temporarily unavailable. >> err = i40e_vsi_open(vsi); >> if (err) >> return err; > > (b) undo the link state if vsi_open() fails. > > Maybe that's a bit of a nitpick... > I think it is, in my humble opinion there is no need to force link back down if something went wrong during further setup. Of course I'm always open for discussion :) -------------------------------------------------------------------- Intel Technology Poland sp. z o.o. ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN. Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
