> >> 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
