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

[Florian Fainelli <f.fainelli@gmail.com>]

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