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:57:01 -0700 Message-ID: References: <20180717153645.7500-1-f.fainelli@gmail.com> <20180717153645.7500-8-f.fainelli@gmail.com> <20180717161459.GG968@lunn.ch> <87910e9c-0783-98e9-eb44-ce85656912d8@gmail.com> <20180717164908.GI968@lunn.ch> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: netdev@vger.kernel.org, linville@tuxdriver.com, davem@davemloft.net, vivien.didelot@savoirfairelinux.com To: Andrew Lunn Return-path: Received: from mail-qk0-f196.google.com ([209.85.220.196]:40290 "EHLO mail-qk0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729689AbeGQRaj (ORCPT ); Tue, 17 Jul 2018 13:30:39 -0400 Received: by mail-qk0-f196.google.com with SMTP id c126-v6so876754qkd.7 for ; Tue, 17 Jul 2018 09:57:05 -0700 (PDT) In-Reply-To: <20180717164908.GI968@lunn.ch> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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