Re: [PATCH net-next 6/7] net: systemport: Add support for WAKE_FILTER

[Andrew Lunn <andrew@lunn.ch>]

> >>  			       struct ethtool_wolinfo *wol)
> >>  {
> >>  	struct bcm_sysport_priv *priv = netdev_priv(dev);
> >>  	struct device *kdev = &priv->pdev->dev;
> >> -	u32 supported = WAKE_MAGIC | WAKE_MAGICSECURE;
> >> +	u32 supported = WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER;
> >> +	unsigned int index, i = 0;
> >> +	u32 reg;
> >>  
> >>  	if (!device_can_wakeup(kdev))
> >>  		return -ENOTSUPP;
> >> @@ -555,6 +561,32 @@ static int bcm_sysport_set_wol(struct net_device *dev,
> >>  			    UMAC_PSW_LS);
> >>  	}
> >>  
> >> +	/* We support matching up to 8 filters only */
> >> +	if (wol->wolopts & WAKE_FILTER) {
> >> +		bitmap_copy(priv->filters, (unsigned long *)wol->sopass,
> >> +			    WAKE_FILTER_BITS);
> > 
> > Shouldn't this be done after to the two checks for errors? Otherwise
> > you have unexpected side effects.
> 
> How would you use the bitmap_* routines if you don't copy the bitmap
> first? Besides, if the bitmap is too wide (next check), we zero it out,
> so nothing will get programmed if we attempt a Wake-on-LAN suspend (and
> priv->wolopts is not copied anyway) and the second check would reject a
> zero bitmap as well.

Zero'ing it is a side effect. get_wol() will now return that no
filtered are programmed. However, it appears the hardware is still
programmed with the old filters. Maybe there is a 

rxchk_writel(priv, 0, RXCHK_BRCM_TAG(i)

hiding in this code somewhere, clearing out the old bits, but i don't
see it.

> 
> > 
> >> +
> >> +		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) >
> >> +				  RXCHK_BRCM_TAG_MAX) {
> >> +			bitmap_zero(priv->filters, WAKE_FILTER_BITS);
> >> +			return -ENOSPC;
> >> +		}
> >> +
> >> +		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) == 0)
> >> +			return -EINVAL;
> >> +
> >> +		for_each_set_bit(index, priv->filters, WAKE_FILTER_BITS) {
> >> +			/* Write the index we want to match within the CID field */
> >> +			reg = rxchk_readl(priv, RXCHK_BRCM_TAG(i));
> >> +			reg &= ~(RXCHK_BRCM_TAG_CID_MASK <<
> >> +				 RXCHK_BRCM_TAG_CID_SHIFT);
> >> +			reg |= index << RXCHK_BRCM_TAG_CID_SHIFT;
> >> +			rxchk_writel(priv, reg, RXCHK_BRCM_TAG(i));
> >> +			rxchk_writel(priv, 0xff00ffff, RXCHK_BRCM_TAG_MASK(i));
> >> +			i++;
> >> +		}
> >> +	}
> > 
> > How do you disable filters? It looks like you cannot pass all bits set
> > to 0. Also, how do you disable a specific filter? The code above seems
> > to be additive only. There does not appear to be a first write which
> > disables all existing filters before writing the new set of filters.
> 
> Either you disable WoL entirely (ethtool -s gphy wol d) and then we
> don't put the hardware in a state that allows it to wake-up the system,
> or you re-program a different set of filters by re-sending a new bitmask
> of desired filters.

This appears to be read-modify-write:

> >> +			reg = rxchk_readl(priv, RXCHK_BRCM_TAG(i));
> >> +			reg &= ~(RXCHK_BRCM_TAG_CID_MASK <<
> >> +				 RXCHK_BRCM_TAG_CID_SHIFT);
> >> +			reg |= index << RXCHK_BRCM_TAG_CID_SHIFT;
> >> +			rxchk_writel(priv, reg, RXCHK_BRCM_TAG(i));

It looks like you can add more bits, but i don't see any way to clear
bits. As i said above, there might be an initial write of 0, but i
cannot see it. The obvious place for it would be just before the
for_each_set_bit(), or at the beginning of the function.

    Andrew