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

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

On 07/17/2018 09:14 AM, Andrew Lunn wrote:
> On Tue, Jul 17, 2018 at 08:36:44AM -0700, Florian Fainelli wrote:
>> The SYSTEMPORT MAC allows up to 8 filters to be programmed to wake-up
>> from LAN. Verify that we have up to 8 filters and program them to the
>> appropriate RXCHK entries to be matched (along with their masks).
>>
>> We need to update the entry and exit to Wake-on-LAN mode to keep the
>> RXCHK engine running to match during suspend, but this is otherwise
>> fairly similar to Magic Packet detection.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  drivers/net/ethernet/broadcom/bcmsysport.c | 111 +++++++++++++++++++++++++----
>>  drivers/net/ethernet/broadcom/bcmsysport.h |  14 +++-
>>  2 files changed, 109 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
>> index 511caec7030a..8d7ce3df1080 100644
>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>> @@ -521,25 +521,31 @@ static void bcm_sysport_get_wol(struct net_device *dev,
>>  	struct bcm_sysport_priv *priv = netdev_priv(dev);
>>  	u32 reg;
>>  
>> -	wol->supported = WAKE_MAGIC | WAKE_MAGICSECURE;
>> +	wol->supported = WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER;
>>  	wol->wolopts = priv->wolopts;
>>  
>> -	if (!(priv->wolopts & WAKE_MAGICSECURE))
>> -		return;
>> +	if (priv->wolopts & WAKE_MAGICSECURE) {
>> +		/* Return the programmed SecureOn password */
>> +		reg = umac_readl(priv, UMAC_PSW_MS);
>> +		put_unaligned_be16(reg, &wol->sopass[0]);
>> +		reg = umac_readl(priv, UMAC_PSW_LS);
>> +		put_unaligned_be32(reg, &wol->sopass[2]);
>> +	}
>>  
>> -	/* Return the programmed SecureOn password */
>> -	reg = umac_readl(priv, UMAC_PSW_MS);
>> -	put_unaligned_be16(reg, &wol->sopass[0]);
>> -	reg = umac_readl(priv, UMAC_PSW_LS);
>> -	put_unaligned_be32(reg, &wol->sopass[2]);
>> +	if (priv->wolopts & WAKE_FILTER)
>> +		bitmap_copy((unsigned long *)wol->sopass, priv->filters,
>> +			    WAKE_FILTER_BITS);
>>  }
>>  
>> +
>>  static int bcm_sysport_set_wol(struct net_device *dev,
> 
> Two blank lines...
> 
> 
> 
>>  			       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.

> 
>> +
>> +		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 is not different from how you program/unprogram
MagicPacket with SecureOn password.
-- 
Florian