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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180717161459.GG968@lunn.ch>
Date:   Tue, 17 Jul 2018 18:14:59 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     netdev@...r.kernel.org, linville@...driver.com,
        davem@...emloft.net, vivien.didelot@...oirfairelinux.com
Subject: Re: [PATCH net-next 6/7] net: systemport: Add support for WAKE_FILTER

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@...il.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.

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

	 Andrew