From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Subject: Re: [PATCH net-next 6/7] net: systemport: Add support for WAKE_FILTER Date: Tue, 17 Jul 2018 09:26:04 -0700 Message-ID: <87910e9c-0783-98e9-eb44-ce85656912d8@gmail.com> References: <20180717153645.7500-1-f.fainelli@gmail.com> <20180717153645.7500-8-f.fainelli@gmail.com> <20180717161459.GG968@lunn.ch> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, linville@tuxdriver.com, davem@davemloft.net, vivien.didelot@savoirfairelinux.com To: Andrew Lunn Return-path: Received: from mail-qt0-f196.google.com ([209.85.216.196]:33071 "EHLO mail-qt0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729648AbeGQQ7f (ORCPT ); Tue, 17 Jul 2018 12:59:35 -0400 Received: by mail-qt0-f196.google.com with SMTP id c15-v6so1406489qtp.0 for ; Tue, 17 Jul 2018 09:26:08 -0700 (PDT) In-Reply-To: <20180717161459.GG968@lunn.ch> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 >> --- >> 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