* [PATCH] ath10: mark PM functions as __maybe_unused
From: Arnd Bergmann @ 2017-09-06 12:58 UTC (permalink / raw)
To: Kalle Valo
Cc: Arnd Bergmann, Colin Ian King, Bartosz Markowski, Govind Singh,
Ryan Hsu, Srinivas Kandagatla, Rajkumar Manoharan,
Ashok Raj Nagarajan, Ben Greear, ath10k, linux-wireless, netdev,
linux-kernel
When CONFIG_PM_SLEEP is disabled, we get a compile-time
warning:
drivers/net/wireless/ath/ath10k/pci.c:3417:12: error: 'ath10k_pci_pm_resume' defined but not used [-Werror=unused-function]
static int ath10k_pci_pm_resume(struct device *dev)
^~~~~~~~~~~~~~~~~~~~
drivers/net/wireless/ath/ath10k/pci.c:3401:12: error: 'ath10k_pci_pm_suspend' defined but not used [-Werror=unused-function]
static int ath10k_pci_pm_suspend(struct device *dev)
Rather than fixing the #ifdef, this just marks both functions
as __maybe_unused, which is a more robust way to do this.
Fixes: 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/wireless/ath/ath10k/pci.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index bc1633945a56..195dafb98131 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -3396,9 +3396,7 @@ static void ath10k_pci_remove(struct pci_dev *pdev)
MODULE_DEVICE_TABLE(pci, ath10k_pci_id_table);
-#ifdef CONFIG_PM
-
-static int ath10k_pci_pm_suspend(struct device *dev)
+static __maybe_unused int ath10k_pci_pm_suspend(struct device *dev)
{
struct ath10k *ar = dev_get_drvdata(dev);
int ret;
@@ -3414,7 +3412,7 @@ static int ath10k_pci_pm_suspend(struct device *dev)
return ret;
}
-static int ath10k_pci_pm_resume(struct device *dev)
+static __maybe_unused int ath10k_pci_pm_resume(struct device *dev)
{
struct ath10k *ar = dev_get_drvdata(dev);
int ret;
@@ -3433,7 +3431,6 @@ static int ath10k_pci_pm_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(ath10k_pci_pm_ops,
ath10k_pci_pm_suspend,
ath10k_pci_pm_resume);
-#endif
static struct pci_driver ath10k_pci_driver = {
.name = "ath10k_pci",
--
2.9.0
^ permalink raw reply related
* Re: hung task in mac80211
From: Johannes Berg @ 2017-09-06 13:03 UTC (permalink / raw)
To: Stefano Brivio, Matteo Croce; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <1504702115.13457.16.camel@sipsolutions.net>
On Wed, 2017-09-06 at 14:48 +0200, Johannes Berg wrote:
>
> I'm surprised nobody saw this before - though perhaps Sebastian's
> useless report is the same.
Oh, that's because this is only for the offloaded manager thing, and
that's only ath10k.
johannes
^ permalink raw reply
* Re: hung task in mac80211
From: Matteo Croce @ 2017-09-06 13:07 UTC (permalink / raw)
To: Stefano Brivio
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Johannes Berg
In-Reply-To: <20170906144019.1c98a636@elisabeth>
On Wed, Sep 6, 2017 at 2:40 PM, Stefano Brivio <sbrivio-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, 6 Sep 2017 13:57:47 +0200
> Matteo Croce <mcroce-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
>> Hi,
>>
>> I have an hung task on vanilla 4.13 kernel which I haven't on 4.12.
>> The problem is present both on my AP and on my notebook,
>> so it seems it affects AP and STA mode as well.
>> The generated messages are:
>>
>> INFO: task kworker/u16:6:120 blocked for more than 120 seconds.
>> Not tainted 4.13.0 #57
>> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>> kworker/u16:6 D 0 120 2 0x00000000
>> Workqueue: phy0 ieee80211_ba_session_work [mac80211]
>> Call Trace:
>> ? __schedule+0x174/0x5b0
>> ? schedule+0x31/0x80
>> ? schedule_preempt_disabled+0x9/0x10
>> ? __mutex_lock.isra.2+0x163/0x480
>> ? select_task_rq_fair+0xb9f/0xc60
>> ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
>> ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
>
> This is ugly and maybe wrong, but you could check perhaps...:
>
> diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
> index c92df492e898..bd7512a656f2 100644
> --- a/net/mac80211/ht.c
> +++ b/net/mac80211/ht.c
> @@ -320,28 +320,40 @@ void ieee80211_ba_session_work(struct work_struct *work)
>
> mutex_lock(&sta->ampdu_mlme.mtx);
> for (tid = 0; tid < IEEE80211_NUM_TIDS; tid++) {
> - if (test_and_clear_bit(tid, sta->ampdu_mlme.tid_rx_timer_expired))
> + if (test_and_clear_bit(tid, sta->ampdu_mlme.tid_rx_timer_expired)) {
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> ___ieee80211_stop_rx_ba_session(
> sta, tid, WLAN_BACK_RECIPIENT,
> WLAN_REASON_QSTA_TIMEOUT, true);
> + mutex_lock(&sta->ampdu_mlme.mtx);
> + }
>
> if (test_and_clear_bit(tid,
> - sta->ampdu_mlme.tid_rx_stop_requested))
> + sta->ampdu_mlme.tid_rx_stop_requested)) {
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> ___ieee80211_stop_rx_ba_session(
> sta, tid, WLAN_BACK_RECIPIENT,
> WLAN_REASON_UNSPECIFIED, true);
> + mutex_lock(&sta->ampdu_mlme.mtx);
> + }
>
> if (test_and_clear_bit(tid,
> - sta->ampdu_mlme.tid_rx_manage_offl))
> + sta->ampdu_mlme.tid_rx_manage_offl)) {
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> __ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
> IEEE80211_MAX_AMPDU_BUF,
> false, true);
> + mutex_lock(&sta->ampdu_mlme.mtx);
> + }
>
> if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
> - sta->ampdu_mlme.tid_rx_manage_offl))
> + sta->ampdu_mlme.tid_rx_manage_offl)) {
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> ___ieee80211_stop_rx_ba_session(
> sta, tid, WLAN_BACK_RECIPIENT,
> 0, false);
> + mutex_lock(&sta->ampdu_mlme.mtx);
> + }
>
> spin_lock_bh(&sta->lock);
>
> --
> Stefano
>
ACK, I have it running since 12 minutes.
The hang usually appears shortly after boot as I set
kernel.hung_task_timeout_secs=10
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply
* Re: hung task in mac80211
From: Sebastian Gottschall @ 2017-09-06 13:08 UTC (permalink / raw)
To: Johannes Berg, Stefano Brivio, Matteo Croce
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1504702990.13457.19.camel-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Am 06.09.2017 um 15:03 schrieb Johannes Berg:
> On Wed, 2017-09-06 at 14:48 +0200, Johannes Berg wrote:
>> I'm surprised nobody saw this before - though perhaps Sebastian's
>> useless report is the same.
> Oh, that's because this is only for the offloaded manager thing, and
> that's only ath10k.
>
> johannes
that explans the behaviour i have found with latest mac80211 and ath10k.
--
Mit freundlichen Grüssen / Regards
Sebastian Gottschall / CTO
NewMedia-NET GmbH - DD-WRT
Firmensitz: Berliner Ring 101, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall-t37Kgv3TaIPQT0dZR+AlfA@public.gmane.org
Tel.: +496251-582650 / Fax: +496251-5826565
^ permalink raw reply
* Re: hung task in mac80211
From: Stefano Brivio @ 2017-09-06 13:19 UTC (permalink / raw)
To: Johannes Berg; +Cc: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <1504702115.13457.16.camel@sipsolutions.net>
On Wed, 06 Sep 2017 14:48:35 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:
> I'll look in a bit - but
>
> > + mutex_unlock(&sta->ampdu_mlme.mtx);
> > ___ieee80211_stop_rx_ba_session(
> > sta, tid, WLAN_BACK_RECIPIENT,
> > WLAN_REASON_QSTA_TIMEOUT, true);
>
> This already has three underscores so shouldn't drop.
Right, of course.
> [...]
> > + mutex_unlock(&sta->ampdu_mlme.mtx);
> > __ieee80211_start_rx_ba_session(sta, 0, 0,
> > 0, 1, tid,
>
> maybe this one needs a ___ version then?
Either that, or as it's a single call, perhaps just the following?
Matter of taste I guess...
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index c92df492e898..377dd3c233d3 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -332,10 +332,13 @@ void ieee80211_ba_session_work(struct work_struct *work)
WLAN_REASON_UNSPECIFIED, true);
if (test_and_clear_bit(tid,
- sta->ampdu_mlme.tid_rx_manage_offl))
+ sta->ampdu_mlme.tid_rx_manage_offl)) {
+ mutex_unlock(&sta->ampdu_mlme.mtx);
__ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
IEEE80211_MAX_AMPDU_BUF,
false, true);
+ mutex_lock(&sta->ampdu_mlme.mtx);
+ }
if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
sta->ampdu_mlme.tid_rx_manage_offl))
--
Stefano
^ permalink raw reply related
* Re: hung task in mac80211
From: Johannes Berg @ 2017-09-06 13:21 UTC (permalink / raw)
To: Stefano Brivio; +Cc: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <20170906151922.4a320b1d@elisabeth>
On Wed, 2017-09-06 at 15:19 +0200, Stefano Brivio wrote:
> On Wed, 06 Sep 2017 14:48:35 +0200
> Johannes Berg <johannes@sipsolutions.net> wrote:
>
> > I'll look in a bit - but
> >
> > > + mutex_unlock(&sta->ampdu_mlme.mtx);
> > > ___ieee80211_stop_rx_ba_session(
> > > sta, tid, WLAN_BACK_RECIPIENT,
> > > WLAN_REASON_QSTA_TIMEOUT,
> > > true);
> >
> > This already has three underscores so shouldn't drop.
>
> Right, of course.
>
> > [...]
> > > + mutex_unlock(&sta->ampdu_mlme.mtx);
> > > __ieee80211_start_rx_ba_session(sta, 0,
> > > 0,
> > > 0, 1, tid,
> >
> > maybe this one needs a ___ version then?
>
> Either that, or as it's a single call, perhaps just the following?
> Matter of taste I guess...
I don't think it's a matter of taste - for me, in principle, dropping
locks for small sections of code where the larger section holds it is a
bug waiting to happen. It may (may, I don't even know) be OK here, but
in general it's something to avoid.
johannes
^ permalink raw reply
* Re: hung task in mac80211
From: Stefano Brivio @ 2017-09-06 13:27 UTC (permalink / raw)
To: Johannes Berg; +Cc: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <1504704060.13457.20.camel@sipsolutions.net>
On Wed, 06 Sep 2017 15:21:00 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2017-09-06 at 15:19 +0200, Stefano Brivio wrote:
> > On Wed, 06 Sep 2017 14:48:35 +0200
> > Johannes Berg <johannes@sipsolutions.net> wrote:
> >
> > > I'll look in a bit - but
> > >
> > > > + mutex_unlock(&sta->ampdu_mlme.mtx);
> > > > ___ieee80211_stop_rx_ba_session(
> > > > sta, tid, WLAN_BACK_RECIPIENT,
> > > > WLAN_REASON_QSTA_TIMEOUT,
> > > > true);
> > >
> > > This already has three underscores so shouldn't drop.
> >
> > Right, of course.
> >
> > > [...]
> > > > + mutex_unlock(&sta->ampdu_mlme.mtx);
> > > > __ieee80211_start_rx_ba_session(sta, 0,
> > > > 0,
> > > > 0, 1, tid,
> > >
> > > maybe this one needs a ___ version then?
> >
> > Either that, or as it's a single call, perhaps just the following?
> > Matter of taste I guess...
>
> I don't think it's a matter of taste - for me, in principle, dropping
> locks for small sections of code where the larger section holds it is a
> bug waiting to happen. It may (may, I don't even know) be OK here, but
> in general it's something to avoid.
Yes, that was based on the assumption that the initial part of
__ieee80211_start_rx_ba_session() can't really affect the AMPDU
state-machine in any way.
But sure, one small change there in the future and the assumption
doesn't hold anymore.
--
Stefano
^ permalink raw reply
* Re: hung task in mac80211
From: Johannes Berg @ 2017-09-06 13:30 UTC (permalink / raw)
To: Stefano Brivio; +Cc: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <20170906152709.673f230d@elisabeth>
On Wed, 2017-09-06 at 15:27 +0200, Stefano Brivio wrote:
>
> Yes, that was based on the assumption that the initial part of
> __ieee80211_start_rx_ba_session() can't really affect the AMPDU
> state-machine in any way.
That's not really the point, if that changes that function would have
to move the locking around, and nothing else.
The point is more that code in ieee80211_ba_session_work() could assume
the lock is held across the entire loop, since that's the way it's
written and looks like even with your patch.
So for example replacing the loop of tid = 0..NUM_TIDS-1 with a
list_for_each_entry() would already be unsafe with the dropping if the
list were to require the mutex for locking.
johannes
^ permalink raw reply
* [PATCH] isdn: isdnloop: fix logic error in isdnloop_sendbuf
From: Arnd Bergmann @ 2017-09-06 13:38 UTC (permalink / raw)
To: Karsten Keil
Cc: Arnd Bergmann, Joe Perches, David S. Miller, netdev, linux-kernel
gcc-7 found an ancient bug in the loop driver, leading to a condition that
is always false, meaning we ignore the contents of 'card->flags' here:
drivers/isdn/isdnloop/isdnloop.c:412:37: error: ?: using integer constants in boolean context, the expression will always evaluate to 'true' [-Werror=int-in-bool-context]
This changes the braces in the expression to ensure we actually
compare the flag bits, rather than comparing a constant. As Joe Perches
pointed out, an earlier patch of mine incorrectly assumed this was a
false-positive warning.
Cc: Joe Perches <joe@perches.com>
Link: https://patchwork.kernel.org/patch/9840289/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/isdn/isdnloop/isdnloop.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
index 6ffd13466b8c..e97232646ba1 100644
--- a/drivers/isdn/isdnloop/isdnloop.c
+++ b/drivers/isdn/isdnloop/isdnloop.c
@@ -409,7 +409,7 @@ isdnloop_sendbuf(int channel, struct sk_buff *skb, isdnloop_card *card)
return -EINVAL;
}
if (len) {
- if (!(card->flags & (channel) ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE))
+ if (!(card->flags & (channel ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE)))
return 0;
if (card->sndcount[channel] > ISDNLOOP_MAX_SQUEUE)
return 0;
--
2.9.0
^ permalink raw reply related
* Re: hung task in mac80211
From: Stefano Brivio @ 2017-09-06 13:40 UTC (permalink / raw)
To: Johannes Berg; +Cc: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <1504704610.23905.1.camel@sipsolutions.net>
On Wed, 06 Sep 2017 15:30:10 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:
> So for example replacing the loop of tid = 0..NUM_TIDS-1 with a
> list_for_each_entry() would already be unsafe with the dropping if the
> list were to require the mutex for locking.
Sure. Still, it would need another code change to break, but in general
I do agree indeed. :)
--
Stefano
^ permalink raw reply
* Re: [PATCH net 1/4] lan78xx: Fix for crash associated with System suspend
From: Andrew Lunn @ 2017-09-06 13:59 UTC (permalink / raw)
To: Nisar.Sayed; +Cc: davem, UNGLinuxDriver, netdev
In-Reply-To: <CE371C1263339941885964188A0225FA333370@CHN-SV-EXMX03.mchp-main.com>
On Wed, Sep 06, 2017 at 10:51:31AM +0000, Nisar.Sayed@microchip.com wrote:
> From: Nisar Sayed <Nisar.Sayed@microchip.com>
>
> Fix for crash associated with System suspend
>
> Since ndo_stop removes phydev which makes phydev NULL.
> Whenever system suspend is initiated or after "ifconfig <interface> down",
> if set_wol or get_wol is triggered phydev is NULL leads system crash.
> Hence phy_start/phy_stop for ndo_start/ndo_stop fixes the issues
> instead of adding/removing phydevice
Looking at this patch, there apears to be lots of different things
going on. Please can you split it up into multiple patches.
> Signed-off-by: Nisar Sayed <Nisar.Sayed@microchip.com>
> ---
> drivers/net/usb/lan78xx.c | 44 ++++++++++++++++++++++++++++----------------
> 1 file changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index b99a7fb..955ab3b 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2024,6 +2024,8 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> lan8835_fixup);
> if (ret < 0) {
> netdev_err(dev->net, "fail to register fixup\n");
> + phy_unregister_fixup_for_uid(PHY_KSZ9031RNX,
> + 0xfffffff0);
goto error; would be better. phy_unregister_fixup_for_uid() does not care if you try to unregister
something which has not been registered.
Also, this should be a separate patch.
> return ret;
> }
> /* add more external PHY fixup here if needed */
> @@ -2031,8 +2033,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> phydev->is_internal = false;
> } else {
> netdev_err(dev->net, "unknown ID found\n");
> - ret = -EIO;
> - goto error;
> + return -EIO;
> }
>
> /* if phyirq is not set, use polling mode in phylib */
> @@ -2051,7 +2052,10 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> if (ret) {
> netdev_err(dev->net, "can't attach PHY to %s\n",
> dev->mdiobus->id);
> - return -EIO;
> + ret = -EIO;
> + if (dev->chipid == ID_REV_CHIP_ID_7801_)
> + goto error;
> + return ret;
Why not add the if (dev->chipid == ID_REV_CHIP_ID_7801_) after the
error: label?
> }
>
> /* MAC doesn't support 1000T Half */
> @@ -2067,8 +2071,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>
> dev->fc_autoneg = phydev->autoneg;
>
> - phy_start(phydev);
> -
> netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
>
> return 0;
> @@ -2497,9 +2499,9 @@ static int lan78xx_open(struct net_device *net)
> if (ret < 0)
> goto done;
>
> - ret = lan78xx_phy_init(dev);
> - if (ret < 0)
> - goto done;
> + if (dev->domain_data.phyirq > 0)
> + phy_start_interrupts(dev->net->phydev);
This is unusual. I don't see any other MAC driver starting interrupts.
This needs explaining.
Andrew
^ permalink raw reply
* RE: [iproute PATCH 1/6] utils: Implement strlcpy() and strlcat()
From: David Laight @ 2017-09-06 13:59 UTC (permalink / raw)
To: 'Stephen Hemminger', Phil Sutter; +Cc: netdev@vger.kernel.org
In-Reply-To: <20170904112525.04bc66d5@xeon-e3>
From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: 04 September 2017 19:25
> On Mon, 4 Sep 2017 17:00:15 +0200
> Phil Sutter <phil@nwl.cc> wrote:
>
> > On Mon, Sep 04, 2017 at 02:49:20PM +0000, David Laight wrote:
> > > From: Phil Sutter
> > > > Sent: 01 September 2017 17:53
> > > > By making use of strncpy(), both implementations are really simple so
> > > > there is no need to add libbsd as additional dependency.
> > > >
> > > ...
> > > > +
> > > > +size_t strlcpy(char *dst, const char *src, size_t size)
> > > > +{
> > > > + if (size) {
> > > > + strncpy(dst, src, size - 1);
> > > > + dst[size - 1] = '\0';
> > > > + }
> > > > + return strlen(src);
> > > > +}
> > >
> > > Except that isn't really strlcpy().
> > > Better would be:
> > > len = strlen(src) + 1;
> > > if (len <= size)
> > > memcpy(dst, src, len);
> > > else if (size) {
> > > dst[size - 1] = 0;
> > > memcpy(dst, src, size - 1);
> > > }
> > > return len - 1;
> >
> > Please elaborate: Why isn't my version "really" strlcpy()? Why is your
> > proposed version better?
> >
> > Thanks, Phil
>
> Linux kernel:
> size_t strlcpy(char *dest, const char *src, size_t size)
> {
> size_t ret = strlen(src);
>
> if (size) {
> size_t len = (ret >= size) ? size - 1 : ret;
> memcpy(dest, src, len);
> dest[len] = '\0';
> }
> return ret;
> }
>
> FreeBSD:
> size_t
> strlcpy(char * __restrict dst, const char * __restrict src, size_t dsize)
> {
> const char *osrc = src;
> size_t nleft = dsize;
>
> /* Copy as many bytes as will fit. */
> if (nleft != 0) {
> while (--nleft != 0) {
> if ((*dst++ = *src++) == '\0')
> break;
> }
> }
>
> /* Not enough room in dst, add NUL and traverse rest of src. */
> if (nleft == 0) {
> if (dsize != 0)
> *dst = '\0'; /* NUL-terminate dst */
> while (*src++)
> ;
> }
>
> return(src - osrc - 1); /* count does not include NUL */
> }
>
>
> They all give the same results for some basic tests.
> Test FreeBSD Linux Iproute2
> "",0: 0 "JUNK" 0 "JUNK" 0 "JUNK"
> "",1: 0 "" 0 "" 0 ""
> "",8: 0 "" 0 "" 0 ""
> "foo",0: 3 "JUNK" 3 "JUNK" 3 "JUNK"
> "foo",3: 3 "fo" 3 "fo" 3 "fo"
> "foo",4: 3 "foo" 3 "foo" 3 "foo"
> "foo",8: 3 "foo" 3 "foo" 3 "foo"
> "longstring",0: 10 "JUNK" 10 "JUNK" 10 "JUNK"
> "longstring",8: 10 "longstr" 10 "longstr" 10 "longstr"
You need to look at the contents of the destination buffer after the
first '\0'.
strlcpy() shouldn't change it.
David
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Vivien Didelot @ 2017-09-06 13:56 UTC (permalink / raw)
To: Andrew Lunn, netdev, Florian Fainelli, Woojung.Huh, jbe,
sean.wang, john
In-Reply-To: <20170906004703.GB27385@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
> So there is the third issue. It affects just DSA, but it possible
> affects all DSA drivers.
>
> This patchset broken broadcast with the Marvell drivers. It could
> break broadcast on others drivers as well.
>
> What i found is that the Marvell chips don't flood broadcast frames
> between bridged ports. What appears to happen is there is a fdb miss,
> so it gets forwarded to the CPU port for the host to deal with. The
> software bridge when floods it out all ports of the bridge.
Do you have this issue on a single switch?
I do expect FDB (not MDB) miss on a multi-chip fabric, not on a single
chip though.
> But the set offload_fwd_mark patch changes this. The software bridge
> now assumes the hardware has already flooded broadcast out all ports
> of the switch as needed. So it does not do any flooding itself. As a
> result, on Marvell devices, broadcast packets don't get flooded at
> all.
>
> The issue can be fixed. I just need to add an mdb entry for the
> broadcast address to each port of the bridge in the switch, and the
> CPU port. But i don't know at what level to do this.
>
> Should this be done at the DSA level, or at the driver level? Do any
> chips do broadcast flooding in hardware already? Hence they currently
> see broadcast duplication? If i add a broadcast mdb at the DSA level,
> and the chip is already hard wired to flooding broadcast, is it going
> to double flood?
It should be done at the DSA level. DSA core must be the single entry
point to handle all the switch logic, calling into (dumb) DSA drivers.
If it is buried into a specific driver, we'll likely lose track of the
problem and make it harder to maintain.
Thanks!
Vivien
^ permalink raw reply
* Re: [PATCH net 2/4] lan78xx: Add fixed_phy device support for LAN7801 device
From: Andrew Lunn @ 2017-09-06 14:02 UTC (permalink / raw)
To: Nisar.Sayed; +Cc: davem, UNGLinuxDriver, netdev
In-Reply-To: <CE371C1263339941885964188A0225FA333381@CHN-SV-EXMX03.mchp-main.com>
On Wed, Sep 06, 2017 at 10:51:44AM +0000, Nisar.Sayed@microchip.com wrote:
> From: Nisar Sayed <Nisar.Sayed@microchip.com>
>
> Add fixed_phy device support for LAN7801 device
>
> When LAN7801 device connected to PHY Device which does not have
> MDIO/MDC access, fixex_phy device will be added.
Please try to find a way to do this without all the #ifdefs. They can
be acceptable in header files, but should be avoided in .c files.
Andrew
^ permalink raw reply
* Re: [PATCH net 3/4] lan78xx: Fix for eeprom read/write when device autosuspend
From: Andrew Lunn @ 2017-09-06 14:06 UTC (permalink / raw)
To: Nisar.Sayed; +Cc: davem, UNGLinuxDriver, netdev
In-Reply-To: <CE371C1263339941885964188A0225FA333391@CHN-SV-EXMX03.mchp-main.com>
Hi Nisar
> + else if ((ee->magic == LAN78XX_EEPROM_MAGIC) &&
> + (ee->offset >= 0 && ee->offset < MAX_EEPROM_SIZE) &&
> + (ee->len > 0 && (ee->offset + ee->len) <= MAX_EEPROM_SIZE))
> + ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee->len, data);
This change does not appear to have anything to do with auto suspend.
Please make it a separate patch.
Andrew
^ permalink raw reply
* Re: [PATCH] vsock: only load vmci transport on VMware hypervisor by default
From: Jorgen S. Hansen @ 2017-09-06 14:11 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Dexuan Cui, davem@davemloft.net, netdev@vger.kernel.org,
gregkh@linuxfoundation.org, devel@linuxdriverproject.org,
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, George Zhang,
Michal Kubecek, Asias He, Vitaly Kuznetsov, Cathy Avery,
jasowang@redhat.com, Rolf Neugebauer, Dave Scott, Marcelo Cerri,
apw@canonical.com
In-Reply-To: <20170831115455.GP13619@stefanha-x1.localdomain>
> On Aug 31, 2017, at 1:54 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Tue, Aug 29, 2017 at 03:37:07PM +0000, Jorgen S. Hansen wrote:
>>> On Aug 29, 2017, at 4:36 AM, Dexuan Cui <decui@microsoft.com> wrote:
>> If we allow multiple host side transports, virtio host side support and
>> vmci should be able to coexist regardless of the order of initialization.
>
> That sounds good to me.
>
> This means af_vsock.c needs to be aware of CID allocation. Currently the
> vhost_vsock.ko driver handles this itself (it keeps a list of CIDs and
> checks that they are not used twice). It should be possible to move
> that state into af_vsock.c so we have <cid, host_transport> pairs.
>
Yes, that was my thinking as well.
> I'm currently working on NFS over AF_VSOCK and sock_diag support (for
> ss(8) and netstat-like tools).
>
> Multi-transport support is lower priority for me at the moment. I'm
> happy to review patches though. If there is no progress on this by the
> end of the year then I will have time to work on it.
>
I’ll try to find time to write a more coherent proposal in the coming weeks,
and we can discuss that.
> Are either of you are in Prague, Czech Republic on October 25-27 for
> Linux Kernel Summit, Open Source Summit Europe, Embedded Linux
> Conference Europe, KVM Forum, or MesosCon Europe?
No, I won’t be there.
Thanks,
Jorgen
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Andrew Lunn @ 2017-09-06 14:16 UTC (permalink / raw)
To: Vivien Didelot
Cc: netdev, Florian Fainelli, Woojung.Huh, jbe, sean.wang, john
In-Reply-To: <87y3pskk1p.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
> > What i found is that the Marvell chips don't flood broadcast frames
> > between bridged ports. What appears to happen is there is a fdb miss,
> > so it gets forwarded to the CPU port for the host to deal with. The
> > software bridge when floods it out all ports of the bridge.
>
> Do you have this issue on a single switch?
Yes. I have two boards with a single switch in my test setup. They
fail the broadcast tests in the same way as boards with multiple
switches.
> It should be done at the DSA level. DSA core must be the single entry
> point to handle all the switch logic, calling into (dumb) DSA drivers.
> If it is buried into a specific driver, we'll likely lose track of the
> problem and make it harder to maintain.
I think we need to wait and see what driver writers report their chips
are doing. If this is just a Marvell issue, we probably should fix it
in the Marvell driver only.
If i remember correctly, Woojung said he was seeing duplication of
broadcasts. So it could be the Microchip hardware is forwarding
broadcast in the hardware.
Andrew
^ permalink raw reply
* Re: [RFC net-next] net: sch_clsact: add support for global per-netns classifier mode
From: Roopa Prabhu @ 2017-09-06 14:19 UTC (permalink / raw)
To: Jiri Pirko
Cc: Daniel Borkmann, Cong Wang, Nikolay Aleksandrov,
Linux Kernel Network Developers, David Ahern, Jamal Hadi Salim
In-Reply-To: <20170906072413.GC2523@nanopsycho>
On Wed, Sep 6, 2017 at 12:24 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Sep 06, 2017 at 06:04:17AM CEST, roopa@cumulusnetworks.com wrote:
>>On Tue, Sep 5, 2017 at 3:45 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>> On 09/06/2017 12:01 AM, Roopa Prabhu wrote:
>>>>
>>>> On Tue, Sep 5, 2017 at 11:18 AM, Cong Wang <xiyou.wangcong@gmail.com>
>>>> wrote:
>>>>>
>>>>> On Tue, Sep 5, 2017 at 5:48 AM, Nikolay Aleksandrov
>>>>> <nikolay@cumulusnetworks.com> wrote:
>>>>>>
>>>>>> Hi all,
>>>>>> This RFC adds a new mode for clsact which designates a device's egress
>>>>>> classifier as global per netns. The packets that are not classified for
>>>>>> a particular device will be classified using the global classifier.
>>>>>> We have needed a global classifier for some time now for various
>>>>>> purposes and setting the single bridge or loopback/vrf device as the
>>>
>>>
>>> Can you elaborate a bit more on the ... "we have needed a global
>>> classifier for some time now for various purposes".
>>
>>Most of our acl's are global or use a wildcard. eg iptables supports
>>global rules without an dev. We do end up having hundreds of netdevs.
>>Another use case for the future is use of tc for policy based routing
>>which requires global rules.
>
> That is not how TC works. There are devices, qdiscs, blocks, chains. The
> global approach does not fit. The block sharing gets you what you need,
> without need for any ugly hack.
We know how TC works. And we are only trying to see if the classifiers
and actions can be used
in other use-cases. Note that the patch was an RFC to start the
discussion and refine
it. We will explore your block sharing patches when they are in.
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: John Crispin @ 2017-09-06 14:27 UTC (permalink / raw)
To: Andrew Lunn, netdev, Florian Fainelli, Vivien Didelot,
Woojung.Huh, jbe, sean.wang
In-Reply-To: <20170906004703.GB27385@lunn.ch>
On 06/09/17 02:47, Andrew Lunn wrote:
> Should this be done at the DSA level, or at the driver level? Do any
> chips do broadcast flooding in hardware already? Hence they currently
> see broadcast duplication? If i add a broadcast mdb at the DSA level,
> and the chip is already hard wired to flooding broadcast, is it going
> to double flood?
Hi Andrew,
MT7530 and QCA8K only have a "unknown mac fwd to port X" feature. both
use the same HW table for FDB and MDB tables. so this should ideally be
fixed on DSA level rather than fixing up those 2 drivers.
John
^ permalink raw reply
* Re: hung task in mac80211
From: Stefano Brivio @ 2017-09-06 14:27 UTC (permalink / raw)
To: Johannes Berg; +Cc: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <1504702728.13457.17.camel@sipsolutions.net>
On Wed, 06 Sep 2017 14:58:48 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:
> +void __ieee80211_start_rx_ba_session(struct sta_info *sta,
> + u8 dialog_token, u16 timeout,
> + u16 start_seq_num, u16 ba_policy, u16 tid,
> + u16 buf_size, bool tx, bool auto_seq)
> +{
> + mutex_lock(&sta->ampdu_mlme.mtx);
> + ___ieee80211_start_rx_ba_session(sta, dialog_token, timeout,
> + start_seq_num, ba_policy, tid,
> + buf_size, tx, auto_seq);
> + mutex_unlock(&sta->ampdu_mlme.mtx);
> +}
> +
Sorry for the extended bothering :) but here, you're extending quite a
bit the scope of the lock also when__ieee80211_start_rx_ba_session() is
called by ieee80211_process_addba_request(). No idea what the hit can
be, but we can't safely assume it's nothing either. What about simply
introducing a 'ampdu_mlme_lock_held' argument instead? Something like:
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 2b36eff5d97e..35a9eff1ec66 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -248,7 +248,8 @@ static void ieee80211_send_addba_resp(struct ieee80211_sub_if_data *sdata, u8 *d
void __ieee80211_start_rx_ba_session(struct sta_info *sta,
u8 dialog_token, u16 timeout,
u16 start_seq_num, u16 ba_policy, u16 tid,
- u16 buf_size, bool tx, bool auto_seq)
+ u16 buf_size, bool tx, bool auto_seq,
+ bool ampdu_mlme_lock_held)
{
struct ieee80211_local *local = sta->sdata->local;
struct tid_ampdu_rx *tid_agg_rx;
@@ -311,7 +312,8 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
buf_size, sta->sta.addr);
/* examine state machine */
- mutex_lock(&sta->ampdu_mlme.mtx);
+ if (!ampdu_mlme_lock_held)
+ mutex_lock(&sta->ampdu_mlme.mtx);
if (test_bit(tid, sta->ampdu_mlme.agg_session_valid)) {
if (sta->ampdu_mlme.tid_rx_token[tid] == dialog_token) {
@@ -415,7 +417,8 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
__clear_bit(tid, sta->ampdu_mlme.unexpected_agg);
sta->ampdu_mlme.tid_rx_token[tid] = dialog_token;
}
- mutex_unlock(&sta->ampdu_mlme.mtx);
+ if (!ampdu_mlme_lock_held)
+ mutex_unlock(&sta->ampdu_mlme.mtx);
end_no_lock:
if (tx)
@@ -445,7 +448,7 @@ void ieee80211_process_addba_request(struct ieee80211_local *local,
__ieee80211_start_rx_ba_session(sta, dialog_token, timeout,
start_seq_num, ba_policy, tid,
- buf_size, true, false);
+ buf_size, true, false, false);
}
void ieee80211_manage_rx_ba_offl(struct ieee80211_vif *vif,
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index c92df492e898..59ba67e8942f 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -335,7 +335,7 @@ void ieee80211_ba_session_work(struct work_struct *work)
sta->ampdu_mlme.tid_rx_manage_offl))
__ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
IEEE80211_MAX_AMPDU_BUF,
- false, true);
+ false, true, true);
if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
sta->ampdu_mlme.tid_rx_manage_offl))
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 2197c62a0a6e..5d494ac65853 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1759,7 +1759,8 @@ void __ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
void __ieee80211_start_rx_ba_session(struct sta_info *sta,
u8 dialog_token, u16 timeout,
u16 start_seq_num, u16 ba_policy, u16 tid,
- u16 buf_size, bool tx, bool auto_seq);
+ u16 buf_size, bool tx, bool auto_seq,
+ bool ampdu_mlme_lock_held);
void ieee80211_sta_tear_down_BA_sessions(struct sta_info *sta,
enum ieee80211_agg_stop_reason reason);
void ieee80211_process_delba(struct ieee80211_sub_if_data *sdata,
--
Stefano
^ permalink raw reply related
* Re: hung task in mac80211
From: Johannes Berg @ 2017-09-06 14:30 UTC (permalink / raw)
To: Stefano Brivio; +Cc: Matteo Croce, linux-wireless, netdev, linux-kernel
In-Reply-To: <20170906162748.071b040e@elisabeth>
On Wed, 2017-09-06 at 16:27 +0200, Stefano Brivio wrote:
>
> Sorry for the extended bothering :) but here, you're extending quite
> a bit the scope of the lock also
> when__ieee80211_start_rx_ba_session() is called by
> ieee80211_process_addba_request().
I know, but it doesn't matter.
> No idea what the hit can be, but we can't safely assume it's
> nothing either.
We don't really have to assume anything, we can read the code :-)
Trust me, I probably wrote most of it. It's fine, just sanity checks.
> What about simply introducing a 'ampdu_mlme_lock_held' argument
> instead?
Eww, no.
johannes
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Vivien Didelot @ 2017-09-06 14:29 UTC (permalink / raw)
To: Andrew Lunn, netdev; +Cc: jiri, nikolay, Florian Fainelli, Andrew Lunn
In-Reply-To: <1504654510-31004-1-git-send-email-andrew@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
> Getting the frames to the bridge as requested turned up an issue or
> three. The offload_fwd_mark is not being set by DSA, so the bridge
> floods the received frames back to the switch ports, resulting in
> duplication since the hardware has already flooded the packet. Fixing
> that turned up an issue with the meaning of
> SWITCHDEV_ATTR_ID_PORT_PARENT_ID in DSA. A DSA fabric of three
> switches needs to look to the software bridge as a single
> switch. Otherwise the offload_fwd_mark does not work, and we get
> duplication on the non-ingress switch. But each switch returned a
> different value. And they were not unique.
[...]
> Is the SWITCHDEV_ATTR_ID_PORT_PARENT_ID change acceptable?
So here we need to return the switch fabric (tree) ID instead of a
single switch chip ID. I think we are not supposed to change it, but
there's definitly something wrong with them and we must fix it.
The same issue happens with the physical port IDs, where the net devices
of two disjoint trees on a system will have the same phys_*_id
attributes, because we do not include the tree ID in them.
(not sure if this affects your patchset though.)
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Mason @ 2017-09-06 14:33 UTC (permalink / raw)
To: Florian Fainelli, David Daney
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard, Thibaud Cornic
In-Reply-To: <4ea8b432-4968-1616-eff9-48a2689dd3ce@gmail.com>
On 31/08/2017 20:29, Florian Fainelli wrote:
> On 08/31/2017 11:12 AM, Mason wrote:
>> On 31/08/2017 19:53, Florian Fainelli wrote:
>>> On 08/31/2017 10:49 AM, Mason wrote:
>>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>>> And the race is between phy_detach() setting phydev->attached_dev = NULL
>>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>>> netif_carrier_off().
>>>>
>>>> I must be missing something.
>>>> (Since a thread cannot race against itself.)
>>>>
>>>> phy_disconnect calls phy_stop_machine which
>>>> 1) stops the work queue from running in a separate thread
>>>> 2) calls phy_state_machine *synchronously*
>>>> which runs the PHY_HALTED case with everything well-defined
>>>> end of phy_stop_machine
>>>>
>>>> phy_disconnect only then calls phy_detach()
>>>> which makes future calls of phy_state_machine perilous.
>>>>
>>>> This all happens in the same thread, so I'm not yet
>>>> seeing where the race happens?
>>>
>>> The race is as described in David's earlier email, so let's recap:
>>>
>>> Thread 1 Thread 2
>>> phy_disconnect()
>>> phy_stop_interrupts()
>>> phy_stop_machine()
>>> phy_state_machine()
>>> -> queue_delayed_work()
>>> phy_detach()
>>> phy_state_machine()
>>> -> netif_carrier_off()
>>>
>>> If phy_detach() finishes earlier than the workqueue had a chance to be
>>> scheduled and process PHY_HALTED again, then we trigger the NULL pointer
>>> de-reference.
>>>
>>> workqueues are not tasklets, the CPU scheduling them gets no guarantee
>>> they will run on the same CPU.
>>
>> Something does not add up.
>>
>> The synchronous call to phy_state_machine() does:
>>
>> case PHY_HALTED:
>> if (phydev->link) {
>> phydev->link = 0;
>> netif_carrier_off(phydev->attached_dev);
>> phy_adjust_link(phydev);
>> do_suspend = true;
>> }
>>
>> then sets phydev->link = 0; therefore subsequent calls to
>> phy_state_machin() will be no-op.
>
> Actually you are right, once phydev->link is set to 0 these would become
> no-ops. Still scratching my head as to what happens for David then...
>
>>
>> Also, queue_delayed_work() is only called in polling mode.
>> David stated that he's using interrupt mode.
>
> Right that's confusing too now. David can you check if you tree has:
>
> 49d52e8108a21749dc2114b924c907db43358984 ("net: phy: handle state
> correctly in phy_stop_machine")
Hello David,
A week ago, you wrote about my patch:
"This is broken. Please revert."
I assume you tested the revert locally, and that reverting did make
the crash disappear. Is that correct?
The reason I ask is because the analysis you provided contains some
flaws, as noted above. But, if reverting my patch did fix your issue,
then perhaps understanding *why* is unimportant.
I'm a bit baffled that it took less than 90 minutes for your request
to be approved, and the patch reverted in all branches, before I even
had a chance to comment.
Regards.
^ permalink raw reply
* Re: [PATCH v2 rfc 8/8] net: dsa: Fix SWITCHDEV_ATTR_ID_PORT_PARENT_ID
From: Vivien Didelot @ 2017-09-06 14:46 UTC (permalink / raw)
To: Andrew Lunn, netdev; +Cc: jiri, nikolay, Florian Fainelli, Andrew Lunn
In-Reply-To: <1504654510-31004-9-git-send-email-andrew@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
> SWITCHDEV_ATTR_ID_PORT_PARENT_ID is used by the software bridge when
> determining which ports to flood a packet out. If the packet
> originated from a switch, it assumes the switch has already flooded
> the packet out the switches ports, so the bridge should not flood the
> packet itself out switch ports. Ports on the same switch are expected
> to return the same parent ID when SWITCHDEV_ATTR_ID_PORT_PARENT_ID is
> called.
>
> DSA gets this wrong with clusters of switches. As far as the software
> bridge is concerned, the cluster is all one switch. A packet from any
> switch in the cluster can be assumed to of been flooded as needed out
> all ports of the cluster, not just the switch it originated
> from. Hence all ports of a cluster should return the same parent. The
> old implementation did not, each switch in the cluster had its own ID.
>
> Also wrong was that the ID was not unique if multiple DSA instances
> are in operation.
>
> Use the MAC address of the master interface as the parent ID. This is
> the same for all switches in a cluster, and should be unique if there
> are multiple clusters.
That is not correct. Support for multiple CPU ports is coming and in
this case, you can have two CPU host interfaces wired to two switch
ports of the same tree. So two different master MAC addresses.
Only the tree ID assigned by DSA core is unique to a given tree.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Matthias May @ 2017-09-06 14:46 UTC (permalink / raw)
To: Andrew Lunn, netdev, Florian Fainelli, Vivien Didelot,
Woojung.Huh, jbe, sean.wang, john
In-Reply-To: <20170906004703.GB27385@lunn.ch>
On 06/09/17 02:47, Andrew Lunn wrote:
>> The third and last issue will be explained in a followup email.
>
> Hi DSA hackers
>
> So there is the third issue. It affects just DSA, but it possible
> affects all DSA drivers.
>
> This patchset broken broadcast with the Marvell drivers. It could
> break broadcast on others drivers as well.
>
> What i found is that the Marvell chips don't flood broadcast frames
> between bridged ports. What appears to happen is there is a fdb miss,
> so it gets forwarded to the CPU port for the host to deal with. The
> software bridge when floods it out all ports of the bridge.
>
> But the set offload_fwd_mark patch changes this. The software bridge
> now assumes the hardware has already flooded broadcast out all ports
> of the switch as needed. So it does not do any flooding itself. As a
> result, on Marvell devices, broadcast packets don't get flooded at
> all.
>
> The issue can be fixed. I just need to add an mdb entry for the
> broadcast address to each port of the bridge in the switch, and the
> CPU port. But i don't know at what level to do this.
>
> Should this be done at the DSA level, or at the driver level? Do any
> chips do broadcast flooding in hardware already? Hence they currently
> see broadcast duplication? If i add a broadcast mdb at the DSA level,
> and the chip is already hard wired to flooding broadcast, is it going
> to double flood?
>
> Andrew
>
Hi Andrew
We are using the 88E6321.
In our setup we are using openvswitch and not a bridge, however the problem you describe seems to be the same.
We had to configure the switch to flood unknown multicast (Egress Floods = 0x3, bits 3:2, offset 0x4 in port control)
and
unset FloodBC (FloodBC = 0x0, bit 12, offset 0x5 in global 2) which defines if a broadcast should be considered as
multicast for the above config.
Regarding the looping problem:
Since OVS isn't aware of the fdb of the switch, we configure the ports representing the switch ports (in ovs) as
"protected" which prevents looping them back between (even when flooding) see [1].
I'm not sure if the bridge has a similar feature.
BR
Matthias
[1]http://openvswitch.org/support/dist-docs/ovs-vswitchd.conf.db.5.txt ctrl-f: "protected: boolean"
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox