On 07/17/2018 09:49 AM, Andrew Lunn wrote:
>>>> 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.
It is not necessary to clear those registers for a number of reasons:
- they are only active if the corresponding bit to enable those is also
programmed in RXHCK_CONTROL, which is only done during
bcm_sysport_suspend_to_wol(), though I suppose for safety one could be
moving the RXCHK_BRCM_TAG_MATCH_MASK clearing outside of the WAKE_FILTER
check though again, not necessary because:
- HW starts with those bits cleared
- if you entered WoL once with any of those filters, we would be
clearing those bits again during WoL resume
- if WoL is disabled, we don't even enable network ports to forward
traffic and remotely allow a packet to enter the switch (see
drivers/net/dsa/bcm_sf2.c)
>
>>
>>>
>>>> +
>>>> + 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.
We are only programming the HW to be matching bits [23:16] and with a
corresponding mask of 0xff00_ffff so even if these bits contained
garbage, they would not be matched by the HW.
--
Florian
