On Thu, 2018-11-08 at 07:42 +0100, Michal Kubecek wrote: > On Thu, Nov 08, 2018 at 06:05:26AM +0000, Kevin Easton wrote: > > On Wed, Nov 07, 2018 at 02:48:24PM -0800, Jeff Kirsher wrote: > > > From: Todd Fujinaka <todd.fujin...@intel.com> > > > > > > There's a new flag for setting WoL filters that is only > > > enabled on one manufacturer's NICs, and it's not ours. Fail > > > with EOPNOTSUPP. > > > > > > Signed-off-by: Todd Fujinaka <todd.fujin...@intel.com> > > > Tested-by: Andrew Bowers <andrewx.bow...@intel.com> > > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirs...@intel.com> > > > --- > > > drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 3 ++- > > > drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 +- > > > drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 3 ++- > > > 3 files changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > > > b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > > > index 9f8464f80783..9c1211ad2c6b 100644 > > > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > > > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > > > @@ -2377,7 +2377,8 @@ static int i40e_set_wol(struct net_device > > > *netdev, struct ethtool_wolinfo *wol) > > > return -EOPNOTSUPP; > > > > > > /* only magic packet is supported */ > > > - if (wol->wolopts && (wol->wolopts != WAKE_MAGIC)) > > > + if (wol->wolopts && (wol->wolopts != WAKE_MAGIC) > > > + | (wol->wolopts != WAKE_FILTER)) > > > return -EOPNOTSUPP; > > > > This doesn't look right. WAKE_MAGIC and WAKE_FILTER are distinct, so > > > > (wol->wolopts != WAKE_MAGIC) | (wol->wolopts != WAKE_FILTER) > > > > will always be 1. > > Right. Also, using "|" with logical values is rather confusing. While > the result works as expected, its priority is higher than priority of > && (which would not be true for ||), making the code counterintuitive. > > BtW, the patch subject is also wrong, the newly added flag it is dealing > with is WAKE_FILTER, not WAKE_MAGICSECURE.
After looking into this this more, this does appear to be incorrect. The
author of this change is currently out on vacation, so once he gets back, I
will address both Kevin's and your concerns.
>
> > It looks like the existing test in this driver was fine - it *only*
> > accepted wol->wolopts of either 0 or WAKE_MAGIC, it was already
> > rejecting everything else including WAKE_FILTER.
>
> Another way to write the check would be
>
> if (wol->wolopts & ~WAKE_MAGIC)
>
> > > diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > index 5acf3b743876..c57671068245 100644
> > > --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > @@ -2113,7 +2113,7 @@ static int igb_set_wol(struct net_device
> > > *netdev, struct ethtool_wolinfo *wol)
> > > {
> > > struct igb_adapter *adapter = netdev_priv(netdev);
> > >
> > > - if (wol->wolopts & (WAKE_ARP | WAKE_MAGICSECURE))
> > > + if (wol->wolopts & (WAKE_ARP | WAKE_MAGICSECURE | WAKE_FILTER))
> > > return -EOPNOTSUPP;
> > >
> > > if (!(adapter->flags & IGB_FLAG_WOL_SUPPORTED))
>
> I would also suggest taking the opposite approach here, i.e. listing the
> flags which _are_ supported so that we don't have to update the code if
> another wol flag is added (or userspace sends an invalid one):
>
> #define SUPPORTED_WOL_MODES \
> (WAKE_PHY | WAKE_UCAST | WAKE_MCAST | WAKE_BCAST | WAKE_MAGIC)
> ...
> if (wol->wolopts & ~SUPPORTED_WOL_MODES)
> return -EOPNOTSUPP;
>
>
> > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> > > index 732b1e6ecc43..acba067cc15a 100644
> > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> > > @@ -2206,7 +2206,8 @@ static int ixgbe_set_wol(struct net_device
> > > *netdev, struct ethtool_wolinfo *wol)
> > > {
> > > struct ixgbe_adapter *adapter = netdev_priv(netdev);
> > >
> > > - if (wol->wolopts & (WAKE_PHY | WAKE_ARP | WAKE_MAGICSECURE))
> > > + if (wol->wolopts & (WAKE_PHY | WAKE_ARP | WAKE_MAGICSECURE |
> > > + WAKE_FILTER))
> > > return -EOPNOTSUPP;
> > >
> > > if (ixgbe_wol_exclusion(adapter, wol))
>
> ...and here.
>
> Michal Kubecek
signature.asc
Description: This is a digitally signed message part
