* Re: Regression caused by commit 7bb05b85bc2d ("r8169: don't use MSI-X on RTL8106e")
From: Jian-Hong Pan @ 2018-09-27 9:03 UTC (permalink / raw)
To: andy.shevchenko
Cc: Kai-Heng Feng, Heiner Kallweit, Thomas Gleixner,
Linux Netdev List, Linux Kernel, Linux Upstreaming Team,
Daniel Drake, Steve Dodd
In-Reply-To: <CAHp75VcCLvb+LvE49YNWnptTMFpUNuj+Jao7udhQeLfOuHh2vg@mail.gmail.com>
Andy Shevchenko <andy.shevchenko@gmail.com> 於 2018年9月22日 週六 上午1:08寫道：
>
> On Thu, Sep 13, 2018 at 8:53 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
> >
> > 2018-09-12 16:19 GMT+08:00 Kai-Heng Feng <kai.heng.feng@canonical.com>:
> > > at 14:32, Thomas Gleixner <tglx@linutronix.de> wrote:
> > >
> > >> On Wed, 12 Sep 2018, Kai-Heng Feng wrote:
> > >>
> > >>> There's a Dell machine with RTL8106e stops to work after S3 since the
> > >>> commit introduced. So I am wondering if it's possible to revert the
> > >>> commit and use DMI/subsystem id based quirk table?
> > >>
> > >>
> > >> Probably.
>
> Have you seen this thread:
> https://patchwork.ozlabs.org/cover/968924/
>
> and this one:
> https://patchwork.kernel.org/patch/10583229/
Ya! It is the one. And it is discussed in bugzilla
https://bugzilla.kernel.org/show_bug.cgi?id=201181
Now, the revert patch is submitted https://lkml.org/lkml/2018/9/27/224
However, still thanks for your information. :)
Regards,
Jian-Hong Pan
> ?
>
> > >
> > >
> > > Hopefully Jian-Hong can cook up a quirk table for the issue.
> >
> > Module r8169 gets nothing in the PCI BAR after system resumes which
> > makes MSI-X fail on some ASUS laptops equipped with RTL8106e chip.
> > https://www.spinics.net/lists/linux-pci/msg75598.html
> >
> > Actually, I am waiting for the patch "PCI: Reprogram bridge prefetch
> > registers on resume" being merged.
> > https://marc.info/?l=linux-pm&m=153680987814299&w=2
> >
> > It resolves the drivers which get nothing in PCI BAR after system resumes.
> >
> > After that, I can remove the falling back code of RTL8106e.
> >
> > Heiner, any comment?
> >
> > Regards,
> > Jian-Hong Pan
> >
> > >>
> > >>> It's because of commit bc976233a872 ("genirq/msi, x86/vector: Prevent
> > >>> reservation mode for non maskable MSI") cleared the reservation mode, and
> > >>> I
> > >>> can see this after S3:
> > >>>
> > >>> [ 94.872838] do_IRQ: 3.33 No irq handler for vector
> > >>
> > >>
> > >> It's not because of that commit, really. There is a interrupt sent after
> > >> resume to the wrong vector for whatever reason. The MSI vector cannot be
> > >> masked it seems in the device, but the driver should quiescen the device
> > >> to
> > >> a point where it does not send interrupts.
> > >
> > >
> > > Understood.
> > >
> > >>
> > >>> If the device uses MSI-X instead of MSI, the issue doesn't happen because
> > >>> of
> > >>> reservation mode.
> > >>
> > >>
> > >> Reservation mode has absolutely nothing to do with that. What prevents the
> > >> issue is the fact that MSI-X can be masked by the IRQ core.
> > >
> > >
> > > So in this case I think keep the device using MSI-X is a better route, it's
> > > MSI-X capable anyway.
> > >
> > >>
> > >>> Is it something should be handled by x86 BIOS? Because I don't see this
> > >>> issue
> > >>> when I use Suspend-to-Idle, which doesn't use BIOS to do suspend.
> > >>
> > >>
> > >> Suspend to idle works completely different and I don't see the BIOS at
> > >> fault here. it's more an issue of MSI not being maskable on that device,
> > >> which can't be fixed in BIOS or it's some half quiescened state which is
> > >> used when suspending and that's a pure driver issue.
> > >
> > >
> > > Understood.
> > > Thanks for all the info!
> > >
> > > Kai-Heng
> > >
> > >>
> > >> Thanks,
> > >>
> > >> tglx
> > >
> > >
> > >
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply
* Re: [PATCH net] net/ipv6: Remove extra call to ip6_convert_metrics for multipath case
From: David Miller @ 2018-09-27 2:58 UTC (permalink / raw)
To: dsahern; +Cc: netdev, dsahern
In-Reply-To: <20180927003514.18435-1-dsahern@kernel.org>
From: dsahern@kernel.org
Date: Wed, 26 Sep 2018 17:35:14 -0700
> From: David Ahern <dsahern@gmail.com>
>
> The change to move metrics from the dst to rt6_info moved the call
> to ip6_convert_metrics from ip6_route_add to ip6_route_info_create. In
> doing so it makes the call in ip6_route_info_append redundant and
> actually leaks the metrics installed as part of the ip6_route_info_create.
> Remove the now unnecessary call.
>
> Fixes: d4ead6b34b67f ("net/ipv6: move metrics from dst to rt6_info")
> Signed-off-by: David Ahern <dsahern@gmail.com>
Applied and queued up for -stable, thanks David.
^ permalink raw reply
* Re: [PATCH net v2 0/2] net: phy: fix WoL handling when suspending the PHY
From: David Miller @ 2018-09-27 3:04 UTC (permalink / raw)
To: hkallweit1; +Cc: andrew, f.fainelli, nic_swsd, netdev
In-Reply-To: <43386295-806b-ff32-4c7c-56aaeb87b4db@gmail.com>
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Mon, 24 Sep 2018 21:58:04 +0200
> phy_suspend doesn't always recognize that WoL is enabled and therefore
> suspends the PHY when it should not. First idea to address the issue
> was to reuse checks used in mdio_bus_phy_may_suspend which check
> whether relevant devices are wakeup-enabled.
> Florian raised some concerns because drivers may enable wakeup even if
> WoL isn't enabled (e.g. certain USB network drivers).
>
> The new approach focuses on reducing the risk to break existing stuff.
> We add a flag wol_enabled to struct net_device which is set in
> ethtool_set_wol(). Then this flag is checked in phy_suspend().
> This doesn't cover 100% of the cases yet (e.g. if WoL is enabled w/o
> explicit configuration), but it covers the most relevant cases with
> very little risk of regressions.
>
> v2:
> - Fix a typo
Series applied, thanks.
Please deal with the extra 4 byte size of net_device in net-next.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 net-next 07/12] net: ethernet: Add helper to remove a supported link mode
From: David Miller @ 2018-09-27 3:08 UTC (permalink / raw)
To: andrew; +Cc: horms, netdev, f.fainelli, sergei.shtylyov, linux-renesas-soc
In-Reply-To: <20180924155023.GB10667@lunn.ch>
From: Andrew Lunn <andrew@lunn.ch>
Date: Mon, 24 Sep 2018 17:50:23 +0200
> I submitted it to netdev in the usual way. I hope DaveM will accept
> and merge it.
Andrew did I miss your patch somehow?
If so, sorry, please resend.
^ permalink raw reply
* Re: bond: take rcu lock in bond_poll_controller
From: David Miller @ 2018-09-27 3:15 UTC (permalink / raw)
To: davej; +Cc: netdev
In-Reply-To: <20180924192317.wghbs34ld7klqpi6@codemonkey.org.uk>
From: Dave Jones <davej@codemonkey.org.uk>
Date: Mon, 24 Sep 2018 15:23:17 -0400
> Callers of bond_for_each_slave_rcu are expected to hold the rcu lock,
> otherwise a trace like below is shown
...
> Signed-off-by: Dave Jones <davej@codemonkey.org.uk>
Hey Dave, after some recent changes by Eric Dumazet this no longer
applies.
Please respin against 'net'.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next] net: phy: improve handling delayed work
From: David Miller @ 2018-09-27 3:20 UTC (permalink / raw)
To: hkallweit1; +Cc: f.fainelli, andrew, netdev
In-Reply-To: <002b6ead-9d72-0995-4ac7-369a4422b026@gmail.com>
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Mon, 24 Sep 2018 23:36:50 +0200
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index a1f8e4816..40a7dc101 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
...
> @@ -635,6 +635,13 @@ int phy_speed_up(struct phy_device *phydev)
> }
> EXPORT_SYMBOL_GPL(phy_speed_up);
>
> +static inline void phy_queue_state_machine(struct phy_device *phydev,
> + unsigned int secs)
> +{
Please do not use the inline keyword in foo.c files, let the compiler
decide.
^ permalink raw reply
* Re: [PATCH] net/ncsi: Add NCSI OEM command for FB Tiogapass
From: Joel Stanley @ 2018-09-27 3:20 UTC (permalink / raw)
To: Justin.Lee1
Cc: Amithash Prasad, vijaykhemka, Sam Mendoza-Jonas, linux-aspeed,
OpenBMC Maillist, sdasari, netdev, Christian Svensson
In-Reply-To: <5e9c8e3f8fd54845b15cd460efe9090d@AUSX13MPS306.AMER.DELL.COM>
On Thu, 27 Sep 2018 at 00:39, <Justin.Lee1@dell.com> wrote:
>
> > > As I understand Justin's version adds a generic handler, using the NCSI
> > > Netlink interface to pass OEM commands and responses to and from
> > > userspace, which does the actual packet handling.
> > Thanks for the direction Sam! Justin, if you don't mind, can you share the patches you have to add the support? This actually would solve a few other things we are trying to accomplish.
>
>
> Basically, I add a new flag to indicate the request is coming from the Netlink interface to allow the command handler and response handler to react.
> #define NCSI_REQ_FLAG_NETLINK_DRIVEN 2
>
> The work flow is as below.
>
> Request:
> User space application -> Netlink interface (msg) -> new Netlink handler - ncsi_send_cmd_nl() - ncsi_xmit_cmd()
>
> Response:
> Response received - ncsi_rcv_rsp() -> internal response handler - ncsi_rsp_handler_xxx() -> ncsi_send_netlink_rsp () -> Netlink interface (msg) -> user space application
> Command timeout - ncsi_request_timeout() -> ncsi_send_netlink_timeout () -> Netlink interface (msg with zero data length) -> user space application
>
> Error:
> Detected error -> ncsi_send_netlink_err () -> Netlink interface (err msg) -> user space application
>
> I will clean up some code and send out the patch.
Thanks for the overview. We look forward to your patches; please
include the same cc list as this series.
I think it makes sense to have some OEM NCSI handing purely in the
kenrel. This would allow eg. the MAC address of an interface to be
correct at boot, without requiring userspace to come up first.
I have also heard stories of temperature sensors over NCSI. Those
would make sense to be hwmon drivers, which again would mean handling
them in the kernel.
Justin, Vijay, can you please list the known NCSI OEM
commands/extensions that we plan on implementing?
Cheers,
Joel
^ permalink raw reply
* Re: [PATCH net] bonding: pass link-local packets to bonding master also.
From: David Miller @ 2018-09-27 3:24 UTC (permalink / raw)
To: mahesh; +Cc: j.vosburgh, andy, vfalico, netdev, edumazet, maheshb
In-Reply-To: <20180924213942.234209-1-mahesh@bandewar.net>
From: Mahesh Bandewar <mahesh@bandewar.net>
Date: Mon, 24 Sep 2018 14:39:42 -0700
> From: Mahesh Bandewar <maheshb@google.com>
>
> Commit b89f04c61efe ("bonding: deliver link-local packets with
> skb->dev set to link that packets arrived on") changed the behavior
> of how link-local-multicast packets are processed. The change in
> the behavior broke some legacy use cases where these packets are
> expected to arrive on bonding master device also.
>
> This patch passes the packet to the stack with the link it arrived
> on as well as passes to the bonding-master device to preserve the
> legacy use case.
>
> Fixes: b89f04c61efe ("bonding: deliver link-local packets with skb->dev set to link that packets arrived on")
> Reported-by: Michal Soltys <soltys@ziu.info>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH net] bonding: avoid possible dead-lock
From: David Miller @ 2018-09-27 3:24 UTC (permalink / raw)
To: mahesh; +Cc: j.vosburgh, andy, vfalico, netdev, edumazet, maheshb
In-Reply-To: <20180924214011.234426-1-mahesh@bandewar.net>
From: Mahesh Bandewar <mahesh@bandewar.net>
Date: Mon, 24 Sep 2018 14:40:11 -0700
> From: Mahesh Bandewar <maheshb@google.com>
>
> Syzkaller reported this on a slightly older kernel but it's still
> applicable to the current kernel -
...
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH] qed: Avoid implicit enum conversion in qed_iwarp_parse_rx_pkt
From: David Miller @ 2018-09-27 3:24 UTC (permalink / raw)
To: natechancellor; +Cc: Ariel.Elior, everest-linux-l2, netdev, linux-kernel
In-Reply-To: <20180924214212.22522-1-natechancellor@gmail.com>
From: Nathan Chancellor <natechancellor@gmail.com>
Date: Mon, 24 Sep 2018 14:42:12 -0700
> Clang warns when one enumerated type is implicitly converted to another.
>
> drivers/net/ethernet/qlogic/qed/qed_iwarp.c:1713:25: warning: implicit
> conversion from enumeration type 'enum tcp_ip_version' to different
> enumeration type 'enum qed_tcp_ip_version' [-Wenum-conversion]
> cm_info->ip_version = TCP_IPV4;
> ~ ^~~~~~~~
> drivers/net/ethernet/qlogic/qed/qed_iwarp.c:1733:25: warning: implicit
> conversion from enumeration type 'enum tcp_ip_version' to different
> enumeration type 'enum qed_tcp_ip_version' [-Wenum-conversion]
> cm_info->ip_version = TCP_IPV6;
> ~ ^~~~~~~~
> 2 warnings generated.
>
> Use the appropriate values from the expected type, qed_tcp_ip_version:
>
> TCP_IPV4 = QED_TCP_IPV4 = 0
> TCP_IPV6 = QED_TCP_IPV6 = 1
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/125
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH] qed: Avoid constant logical operation warning in qed_vf_pf_acquire
From: David Miller @ 2018-09-27 3:24 UTC (permalink / raw)
To: natechancellor; +Cc: Ariel.Elior, everest-linux-l2, netdev, linux-kernel
In-Reply-To: <20180924221703.26454-1-natechancellor@gmail.com>
From: Nathan Chancellor <natechancellor@gmail.com>
Date: Mon, 24 Sep 2018 15:17:03 -0700
> Clang warns when a constant is used in a boolean context as it thinks a
> bitwise operation may have been intended.
>
> drivers/net/ethernet/qlogic/qed/qed_vf.c:415:27: warning: use of logical
> '&&' with constant operand [-Wconstant-logical-operand]
> if (!p_iov->b_pre_fp_hsi &&
> ^
> drivers/net/ethernet/qlogic/qed/qed_vf.c:415:27: note: use '&' for a
> bitwise operation
> if (!p_iov->b_pre_fp_hsi &&
> ^~
> &
> drivers/net/ethernet/qlogic/qed/qed_vf.c:415:27: note: remove constant
> to silence this warning
> if (!p_iov->b_pre_fp_hsi &&
> ~^~
> 1 warning generated.
>
> This has been here since commit 1fe614d10f45 ("qed: Relax VF firmware
> requirements") and I am not entirely sure why since 0 isn't a special
> case. Just remove the statement causing Clang to warn since it isn't
> required.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/126
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH v2 net] net: phy: sfp: Fix unregistering of HWMON SFP device
From: David Miller @ 2018-09-27 3:25 UTC (permalink / raw)
To: andrew; +Cc: netdev, rmk+kernel, f.fainelli
In-Reply-To: <1537833000-23960-1-git-send-email-andrew@lunn.ch>
From: Andrew Lunn <andrew@lunn.ch>
Date: Tue, 25 Sep 2018 01:50:00 +0200
> A HWMON device is only registered is the SFP module supports the
> diagnostic page and is complient to SFF8472. Don't unconditionally
> unregister the hwmon device when the SFP module is remove, otherwise
> we access data structures which don't exist.
>
> Reported-by: Florian Fainelli <f.fainelli@gmail.com>
> Fixes: 1323061a018a ("net: phy: sfp: Add HWMON support for module sensors")
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Applied.
^ permalink raw reply
* Re: [REBASE PATCH net-next v9 0/4] net: vhost: improve performance when enable busyloop
From: David Miller @ 2018-09-27 3:26 UTC (permalink / raw)
To: xiangxia.m.yue; +Cc: jasowang, mst, makita.toshiaki, netdev, virtualization
In-Reply-To: <1537879012-20859-1-git-send-email-xiangxia.m.yue@gmail.com>
From: xiangxia.m.yue@gmail.com
Date: Tue, 25 Sep 2018 05:36:48 -0700
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> This patches improve the guest receive performance.
> On the handle_tx side, we poll the sock receive queue
> at the same time. handle_rx do that in the same way.
>
> For more performance report, see patch 4
Series applied, thank you.
^ permalink raw reply
* Re: [PATCH net] vxlan: fill ttl inherit info
From: David Miller @ 2018-09-27 3:29 UTC (permalink / raw)
To: liuhangbin; +Cc: netdev
In-Reply-To: <1537929342-10047-1-git-send-email-liuhangbin@gmail.com>
From: Hangbin Liu <liuhangbin@gmail.com>
Date: Wed, 26 Sep 2018 10:35:42 +0800
> When add vxlan ttl inherit support, I forgot to fill it when dump
> vlxan info. Fix it now.
>
> Fixes: 72f6d71e491e6 ("vxlan: add ttl inherit support")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH 1/2] net-ipv4: remove 2 always zero parameters from ipv4_update_pmtu()
From: David Miller @ 2018-09-27 3:31 UTC (permalink / raw)
To: zenczykowski; +Cc: maze, edumazet, netdev, dsahern
In-Reply-To: <20180926035627.221286-1-zenczykowski@gmail.com>
From: "Maciej Żenczykowski" <zenczykowski@gmail.com>
Date: Tue, 25 Sep 2018 20:56:26 -0700
> From: Maciej Żenczykowski <maze@google.com>
>
> (the parameters in question are mark and flow_flags)
>
> Reviewed-by: David Ahern <dsahern@gmail.com>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
Applied.
^ permalink raw reply
* Re: [PATCH 2/2] net-ipv4: remove 2 always zero parameters from ipv4_redirect()
From: David Miller @ 2018-09-27 3:31 UTC (permalink / raw)
To: zenczykowski; +Cc: maze, edumazet, netdev, dsahern
In-Reply-To: <20180926035627.221286-2-zenczykowski@gmail.com>
From: "Maciej Żenczykowski" <zenczykowski@gmail.com>
Date: Tue, 25 Sep 2018 20:56:27 -0700
> From: Maciej Żenczykowski <maze@google.com>
>
> (the parameters in question are mark and flow_flags)
>
> Reviewed-by: David Ahern <dsahern@gmail.com>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
Applied.
^ permalink raw reply
* Re: [PATCH net v2] bnxt_en: Fix TX timeout during netpoll.
From: David Miller @ 2018-09-27 3:33 UTC (permalink / raw)
To: michael.chan; +Cc: songliubraving, edumazet, netdev
In-Reply-To: <1537936864-23472-1-git-send-email-michael.chan@broadcom.com>
From: Michael Chan <michael.chan@broadcom.com>
Date: Wed, 26 Sep 2018 00:41:04 -0400
> The current netpoll implementation in the bnxt_en driver has problems
> that may miss TX completion events. bnxt_poll_work() in effect is
> only handling at most 1 TX packet before exiting. In addition,
> there may be in flight TX completions that ->poll() may miss even
> after we fix bnxt_poll_work() to handle all visible TX completions.
> netpoll may not call ->poll() again and HW may not generate IRQ
> because the driver does not ARM the IRQ when the budget (0 for netpoll)
> is reached.
>
> We fix it by handling all TX completions and to always ARM the IRQ
> when we exit ->poll() with 0 budget.
>
> Also, the logic to ACK the completion ring in case it is almost filled
> with TX completions need to be adjusted to take care of the 0 budget
> case, as discussed with Eric Dumazet <edumazet@google.com>
>
> Reported-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
Applied and queued up for -stable, thanks Michael.
^ permalink raw reply
* Re: [PATCH] net-tcp: /proc/sys/net/ipv4/tcp_probe_interval is a u32 not int
From: David Miller @ 2018-09-27 3:34 UTC (permalink / raw)
To: zenczykowski; +Cc: maze, edumazet, netdev
In-Reply-To: <20180926045928.94911-1-zenczykowski@gmail.com>
From: "Maciej Żenczykowski" <zenczykowski@gmail.com>
Date: Tue, 25 Sep 2018 21:59:28 -0700
> From: Maciej Żenczykowski <maze@google.com>
>
> (fix documentation and sysctl access to treat it as such)
>
> Tested:
> # zcat /proc/config.gz | egrep ^CONFIG_HZ
> CONFIG_HZ_1000=y
> CONFIG_HZ=1000
> # echo $[(1<<32)/1000 + 1] | tee /proc/sys/net/ipv4/tcp_probe_interval
> 4294968
> tee: /proc/sys/net/ipv4/tcp_probe_interval: Invalid argument
> # echo $[(1<<32)/1000] | tee /proc/sys/net/ipv4/tcp_probe_interval
> 4294967
> # echo 0 | tee /proc/sys/net/ipv4/tcp_probe_interval
> # echo -1 | tee /proc/sys/net/ipv4/tcp_probe_interval
> -1
> tee: /proc/sys/net/ipv4/tcp_probe_interval: Invalid argument
>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
Applied.
^ permalink raw reply
* [PATCH net-next] net: dsa: b53: Fix build with B53_SRAB enabled and B53_SERDES=m
From: Arnd Bergmann @ 2018-09-27 10:02 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, Vivien Didelot, David S. Miller
Cc: Arnd Bergmann, netdev, linux-kernel
When B53_SERDES is a loadable module, a built-in srab driver still
cannot reach it, so the previous fix is incomplete:
b53_srab.c:(.text+0x3f4): undefined reference to `b53_serdes_init'
drivers/net/dsa/b53/b53_srab.o:(.rodata+0xe64): undefined reference to `b53_serdes_link_state'
drivers/net/dsa/b53/b53_srab.o:(.rodata+0xe74): undefined reference to `b53_serdes_link_set'
drivers/net/dsa/b53/b53_srab.o:(.rodata+0xe88): undefined reference to `b53_serdes_an_restart'
drivers/net/dsa/b53/b53_srab.o:(.rodata+0xea0): undefined reference to `b53_serdes_phylink_validate'
drivers/net/dsa/b53/b53_srab.o:(.rodata+0xea4): undefined reference to `b53_serdes_config'
Add a Kconfig dependency that forces srab to also be a module
in this case, but allow it to be built-in when serdes is
disabled or built-in.
Fixes: 7a8c7f5c30f9 ("net: dsa: b53: Fix build with B53_SRAB enabled and not B53_SERDES")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/dsa/b53/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/dsa/b53/Kconfig b/drivers/net/dsa/b53/Kconfig
index e83ebfafd881..d32469283f97 100644
--- a/drivers/net/dsa/b53/Kconfig
+++ b/drivers/net/dsa/b53/Kconfig
@@ -31,6 +31,7 @@ config B53_MMAP_DRIVER
config B53_SRAB_DRIVER
tristate "B53 SRAB connected switch driver"
depends on B53 && HAS_IOMEM
+ depends on B53_SERDES || !B53_SERDES
default ARCH_BCM_IPROC
help
Select to enable support for memory-mapped Switch Register Access
--
2.18.0
^ permalink raw reply related
* Re: [PATCH net v2] bnxt_en: Fix TX timeout during netpoll.
From: Song Liu @ 2018-09-27 3:49 UTC (permalink / raw)
To: David Miller
Cc: michael.chan@broadcom.com, edumazet@google.com,
netdev@vger.kernel.org
In-Reply-To: <20180926.203304.405531282068457495.davem@davemloft.net>
> On Sep 26, 2018, at 8:33 PM, David Miller <davem@davemloft.net> wrote:
>
> From: Michael Chan <michael.chan@broadcom.com>
> Date: Wed, 26 Sep 2018 00:41:04 -0400
>
>> The current netpoll implementation in the bnxt_en driver has problems
>> that may miss TX completion events. bnxt_poll_work() in effect is
>> only handling at most 1 TX packet before exiting. In addition,
>> there may be in flight TX completions that ->poll() may miss even
>> after we fix bnxt_poll_work() to handle all visible TX completions.
>> netpoll may not call ->poll() again and HW may not generate IRQ
>> because the driver does not ARM the IRQ when the budget (0 for netpoll)
>> is reached.
>>
>> We fix it by handling all TX completions and to always ARM the IRQ
>> when we exit ->poll() with 0 budget.
>>
>> Also, the logic to ACK the completion ring in case it is almost filled
>> with TX completions need to be adjusted to take care of the 0 budget
>> case, as discussed with Eric Dumazet <edumazet@google.com>
>>
>> Reported-by: Song Liu <songliubraving@fb.com>
>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>
> Applied and queued up for -stable, thanks Michael.
Hi David,
We also need this patch from Eric:
https://marc.info/?l=linux-netdev&m=153780304905946
Thanks,
Song
^ permalink raw reply
* Re: [PATCH net v2] bnxt_en: Fix TX timeout during netpoll.
From: Eric Dumazet @ 2018-09-27 3:50 UTC (permalink / raw)
To: Song Liu; +Cc: David Miller, Michael Chan, netdev
In-Reply-To: <436AA90E-83A6-45A0-80B6-9ED843EE254E@fb.com>
On Wed, Sep 26, 2018 at 8:49 PM Song Liu <songliubraving@fb.com> wrote:
>
> We also need this patch from Eric:
>
> https://marc.info/?l=linux-netdev&m=153780304905946
>
I will submit this formally tomorrow, thanks.
^ permalink raw reply
* Re: [PATCH net-next v2] wireless-drivers: rtnetlink wifi simulation device
From: Johannes Berg @ 2018-09-27 10:10 UTC (permalink / raw)
To: Cody Schuffelen
Cc: Kalle Valo, David S . Miller, linux-kernel, linux-wireless,
netdev, kernel-team
In-Reply-To: <20180926194324.71290-1-schuffelen@google.com>
On Wed, 2018-09-26 at 12:43 -0700, Cody Schuffelen wrote:
> ip link set eth0 down
> ip link set eth0 name buried_eth0
> ip link set buried_eth0 up
> ip link add link buried_eth0 name wlan0 type virt_wifi
>
> eth0 is renamed to buried_eth0 to avoid a network manager trying to
> manage it,
I feel you should remove this "buried" thing at least from the plain
instructions - first of all, I'm not convinced it actually *works* in
general (network managers are able to find it anyway, right? perhaps
android's has some rules?), and it's not really necessary from a kernel
POV.
Perhaps add a note like "you may have to rename or otherwise hide the
eth0 from your connection manager" (also "NetworkManager" is a specific
software, so "network manager" is confusing - I actually first thought
you meant NM).
Anyway, just a thought.
> Thanks for the detailed review! I believe I've addressed all comments.
Thanks!
> The problem we've experienced with routing the hwsim virtual medium from the
> inside the VM to outside the VM is this required running hwsim on the host
> kernel as well. This is too intrusive for our use-case, whereas wifi that
> exists completely inside the VM kernel is much more palatable.
Fair enough. I still think it'd be worthwhile in the long run because
then you could start multiple APs to simulate roaming etc. without
having to implement all of this here.
IOW - I don't think we should extend this much. As is, I think it's
fine, but I wouldn't want to see extensions like "have two scan result",
"let userspace control the RSSI of the scan results to force 'roaming'"
etc.
> +static struct ieee80211_channel channel_2ghz = {
> + .band = NL80211_BAND_2GHZ,
> + .center_freq = 5500,
> + .hw_value = 5500,
That doesn't seem right - a 2 GHz channel that's really 5.5 GHz?
> + .max_power = 5500,
That seems more like a copy/paste bug rather than intentional?
> +static struct ieee80211_rate bitrates_2ghz[] = {
> + {
> + .bitrate = 10,
> + }, {
> + .bitrate = 20,
> + }, {
> + .bitrate = 55,
> + }, {
> + .bitrate = 60,
> + }, {
> + .bitrate = 110,
> + }, {
> + .bitrate = 120,
> + }, {
> + .bitrate = 240,
> + },
> +};
That's ... strangely formatted? I guess we'd usually write
{ .bitrate = 10 },
{ .bitrate = 20 },
...
> +static struct ieee80211_supported_band band_2ghz = {
> + .channels = &channel_2ghz,
> + .bitrates = bitrates_2ghz,
> + .band = NL80211_BAND_2GHZ,
> + .n_channels = 1,
> + .n_bitrates = 7,
Please use ARRAY_SIZE()
> + .ht_cap = {
> + .ht_supported = true,
> + },
> + .vht_cap = {
> + .vht_supported = true,
> + },
That looks _really_ bare - you may want to copy something sane to here.
> +};
> +
> +static struct ieee80211_channel channel_5ghz = {
> + .max_power = 5500,
same here - max_power 5500?
> + .max_reg_power = 9999,
> +};
> +
> +static struct ieee80211_rate bitrates_5ghz[] = {
> + {
> + .bitrate = 60,
> + }, {
> + .bitrate = 120,
> + }, {
> + .bitrate = 240,
> + },
> +};
Same comment here regarding formatting.
> +static struct ieee80211_supported_band band_5ghz = {
> + .channels = &channel_5ghz,
> + .bitrates = bitrates_5ghz,
> + .band = NL80211_BAND_5GHZ,
> + .n_channels = 1,
> + .n_bitrates = 3,
and ARRAY_SIZE
> + .ht_cap = {
> + .ht_supported = true,
> + },
> + .vht_cap = {
> + .vht_supported = true,
> + },
and HT/VHT
> +/** Assigned at module init. Guaranteed locally-administered and unicast. */
> +static u8 fake_router_bssid[ETH_ALEN] = {};
Maybe make that __ro_after_init?
It doesn't matter that much, but is a good signal.
> +static void virt_wifi_scan_result(struct work_struct *work)
> +{
> + char ssid[] = "__VirtWifi";
> + struct cfg80211_bss *informed_bss;
> + struct virt_wifi_priv *priv =
> + container_of(work, struct virt_wifi_priv,
> + scan_result.work);
> + struct wiphy *wiphy = priv_to_wiphy(priv);
> + struct cfg80211_inform_bss mock_inform_bss = {
> + .chan = &channel_5ghz,
> + .scan_width = NL80211_BSS_CHAN_WIDTH_20,
> + .signal = -60,
> + .boottime_ns = ktime_get_boot_ns(),
> + };
> +
> + ssid[0] = WLAN_EID_SSID;
> + /* size of the array minus null terminator, length byte, tag byte */
> + ssid[1] = sizeof(ssid) - 3;
Seems like you could make that const?
struct {
u8 tag;
u8 len;
u8 ssid[8];
} ssid = { .tag = 0, .len = 8, .ssid = "VirtWifi" };
or something like that?
Hmm, maybe that doesn't work.. whatever, not really important.
> + informed_bss = cfg80211_inform_bss_data(wiphy, &mock_inform_bss,
> + CFG80211_BSS_FTYPE_PRESP,
> + fake_router_bssid,
> + mock_inform_bss.boottime_ns,
> + WLAN_CAPABILITY_ESS, 0, ssid,
> + /* Truncate before the null. */
> + sizeof(ssid) - 1, GFP_KERNEL);
> + cfg80211_put_bss(wiphy, informed_bss);
> +
> + informed_bss = cfg80211_inform_bss_data(wiphy, &mock_inform_bss,
> + CFG80211_BSS_FTYPE_BEACON,
> + fake_router_bssid,
> + mock_inform_bss.boottime_ns,
> + WLAN_CAPABILITY_ESS, 0, ssid,
> + /* Truncate before the null. */
> + sizeof(ssid) - 1, GFP_KERNEL);
> + cfg80211_put_bss(wiphy, informed_bss);
Hmm, what's the point of doing it twice? You don't really need to
receive both PRESP/BEACON, just one is sufficient.
> + schedule_delayed_work(&priv->scan_complete, HZ * 2);
> +}
I don't think you need to make that async again.
> +static int virt_wifi_connect(struct wiphy *wiphy, struct net_device *netdev,
> + struct cfg80211_connect_params *sme)
> +{
> + struct virt_wifi_priv *priv = wiphy_priv(wiphy);
> + bool could_schedule;
> +
> + if (priv->being_deleted)
> + return -EBUSY;
> +
> + if (sme->bssid && !ether_addr_equal(sme->bssid, fake_router_bssid))
> + return -EINVAL;
If you wanted to be more "real", you'd accept this and then check it in
the connect worker, rejecting it there if mismatched.
> +static int virt_wifi_get_station(struct wiphy *wiphy, struct net_device *dev,
> + const u8 *mac, struct station_info *sinfo)
> +{
> + wiphy_debug(wiphy, "get_station\n");
> + sinfo->filled = BIT(NL80211_STA_INFO_TX_PACKETS) |
> + BIT(NL80211_STA_INFO_TX_FAILED) | BIT(NL80211_STA_INFO_SIGNAL) |
> + BIT(NL80211_STA_INFO_TX_BITRATE);
> + sinfo->tx_packets = 1;
> + sinfo->tx_failed = 0;
> + sinfo->signal = -60;
> + sinfo->txrate = (struct rate_info) {
> + .legacy = 10, /* units are 100kbit/s */
> + };
> + return 0;
> +}
You should check that mac is fake_router_bssid, and otherwise return not
found (-ENOENT), IIRC.
> +static netdev_tx_t virt_wifi_start_xmit(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + struct virt_wifi_netdev_priv *priv = netdev_priv(dev);
> + struct virt_wifi_priv *w_priv = wiphy_priv(dev->ieee80211_ptr->wiphy);
> +
> + if (!w_priv->is_connected)
> + return NETDEV_TX_BUSY;
I think you should just drop the frame.
> +/* Called under rcu_read_lock() from netif_receive_skb */
> +static rx_handler_result_t virt_wifi_rx_handler(struct sk_buff **pskb)
FWIW, the stuff beyond this point, especially the netlink, I'm less
familiar with.
> +/** Called with rtnl lock held. */
/** is usually used only for kernel-doc
johannes
^ permalink raw reply
* Re: [PATCH] net/ncsi: Add NCSI OEM command for FB Tiogapass
From: Samuel Mendoza-Jonas @ 2018-09-27 3:54 UTC (permalink / raw)
To: Vijay Khemka, linux-aspeed @ lists . ozlabs . org,
openbmc @ lists . ozlabs . org, Sai Dasari, Amithash Prasad,
Justin.Lee1, netdev@vger.kernel.org
In-Reply-To: <81718e3fd6c883917aca540c032a7065fd00e79a.camel@mendozajonas.com>
On Thu, 2018-09-27 at 13:43 +1000, Samuel Mendoza-Jonas wrote:
> On Mon, 2018-09-24 at 17:08 -0700, Vijay Khemka wrote:
> > This patch adds OEM command to get mac address from NCSI device and and
> > configure the same to the network card.
> >
> > ncsi_cmd_arg - Modified this structure to include bigger payload data.
> > ncsi_cmd_handler_oem: This function handles oem command request
> > ncsi_rsp_handler_oem: This function handles response for OEM command.
> > get_mac_address_oem_mlx: This function will send OEM command to get
> > mac address for Mellanox card
> > set_mac_affinity_mlx: This will send OEM command to set Mac affinity
> > for Mellanox card
> >
> > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
>
> Hi Vijay,
>
> Having had a chance to take a closer look, there is probably room for
> both this patchset and Justin's potential changes to coexist; while
> Justin's is a more general solution for sending arbitrary commands, the
> approach of this patch is also useful for handling commands we want
> included in the configure process (such as get-mac-address).
>
> Some comments below:
Whoops, forgot to re-add netdev.
>
> > ---
> > net/ncsi/Kconfig | 3 ++
> > net/ncsi/internal.h | 11 +++++--
> > net/ncsi/ncsi-cmd.c | 24 +++++++++++++--
> > net/ncsi/ncsi-manage.c | 68 ++++++++++++++++++++++++++++++++++++++++++
> > net/ncsi/ncsi-pkt.h | 16 ++++++++++
> > net/ncsi/ncsi-rsp.c | 33 +++++++++++++++++++-
> > 6 files changed, 149 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
> > index 08a8a6031fd7..b8bf89fea7c8 100644
> > --- a/net/ncsi/Kconfig
> > +++ b/net/ncsi/Kconfig
> > @@ -10,3 +10,6 @@ config NET_NCSI
> > support. Enable this only if your system connects to a network
> > device via NCSI and the ethernet driver you're using supports
> > the protocol explicitly.
> > +config NCSI_OEM_CMD_GET_MAC
> > + bool "Get NCSI OEM MAC Address"
> > + depends on NET_NCSI
>
> For the moment this isn't too bad but I wonder if in the future it would
> be more flexible to have something like NCSI_OEM_CMD_MELLANOX etc, so we
> could selectively enable a class of OEM commands based on vendor rather
> than per-command.
>
> > diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> > index 8055e3965cef..da17958e6a4b 100644
> > --- a/net/ncsi/internal.h
> > +++ b/net/ncsi/internal.h
> > @@ -68,6 +68,10 @@ enum {
> > NCSI_MODE_MAX
> > };
> >
> > +#define NCSI_OEM_MFR_MLX_ID 0x8119
> > +#define NCSI_OEM_MLX_CMD_GET_MAC 0x1b00
> > +#define NCSI_OEM_MLX_CMD_SET_AFFINITY 0x010700
>
> I gather this is part of the OEM command but it would be good to describe
> what these bits mean. Is this command documented anywhere by Mellanox?
>
> > +
> > struct ncsi_channel_version {
> > u32 version; /* Supported BCD encoded NCSI version */
> > u32 alpha2; /* Supported BCD encoded NCSI version */
> > @@ -236,6 +240,7 @@ enum {
> > ncsi_dev_state_probe_dp,
> > ncsi_dev_state_config_sp = 0x0301,
> > ncsi_dev_state_config_cis,
> > + ncsi_dev_state_config_oem_gma,
> > ncsi_dev_state_config_clear_vids,
> > ncsi_dev_state_config_svf,
> > ncsi_dev_state_config_ev,
> > @@ -301,9 +306,9 @@ struct ncsi_cmd_arg {
> > unsigned short payload; /* Command packet payload length */
> > unsigned int req_flags; /* NCSI request properties */
> > union {
> > - unsigned char bytes[16]; /* Command packet specific data */
> > - unsigned short words[8];
> > - unsigned int dwords[4];
> > + unsigned char bytes[64]; /* Command packet specific data */
> > + unsigned short words[32];
> > + unsigned int dwords[16];
> > };
> > };
> >
> > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> > index 7567ca63aae2..3205e22c1734 100644
> > --- a/net/ncsi/ncsi-cmd.c
> > +++ b/net/ncsi/ncsi-cmd.c
> > @@ -211,6 +211,25 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
> > return 0;
> > }
> >
> > +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> > + struct ncsi_cmd_arg *nca)
> > +{
> > + struct ncsi_cmd_oem_pkt *cmd;
> > + unsigned int len;
> > +
> > + len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> > + if (nca->payload < 26)
> > + len += 26;
>
> This will have already happened in ncsi_alloc_command(), is this check
> needed?
>
> > + else
> > + len += nca->payload;
> > +
> > + cmd = skb_put_zero(skb, len);
> > + memcpy(cmd->data, nca->bytes, nca->payload);
> > + ncsi_cmd_build_header(&cmd->cmd.common, nca);
> > +
> > + return 0;
> > +}
> > +
> > static struct ncsi_cmd_handler {
> > unsigned char type;
> > int payload;
> > @@ -244,7 +263,7 @@ static struct ncsi_cmd_handler {
> > { NCSI_PKT_CMD_GNS, 0, ncsi_cmd_handler_default },
> > { NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
> > { NCSI_PKT_CMD_GPS, 0, ncsi_cmd_handler_default },
> > - { NCSI_PKT_CMD_OEM, 0, NULL },
> > + { NCSI_PKT_CMD_OEM, -1, ncsi_cmd_handler_oem },
> > { NCSI_PKT_CMD_PLDM, 0, NULL },
> > { NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
> > };
> > @@ -317,7 +336,8 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
> > }
> >
> > /* Get packet payload length and allocate the request */
> > - nca->payload = nch->payload;
> > + if (nch->payload >= 0)
> > + nca->payload = nch->payload;
>
> I think with this there is a chance of nca->payload being uninitialised
> and then used in ncsi_alloc_command(). Can you describe what the aim here
> is?
>
> > nr = ncsi_alloc_command(nca);
> > if (!nr)
> > return -ENOMEM;
> > diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> > index 091284760d21..3b2b86560cc8 100644
> > --- a/net/ncsi/ncsi-manage.c
> > +++ b/net/ncsi/ncsi-manage.c
> > @@ -635,6 +635,58 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
> > return 0;
> > }
> >
> > +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
> > +/* NCSI Facebook OEM APIs */
>
> Are these Facebook OEM commands or Mellanox OEM commands?
>
> > +static void get_mac_address_oem_mlx(struct ncsi_dev_priv *ndp)
> > +{
> > + struct ncsi_cmd_arg nca;
> > + int ret = 0;
> > +
> > + memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
> > + nca.ndp = ndp;
> > + nca.channel = ndp->active_channel->id;
> > + nca.package = ndp->active_package->id;
> > + nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> > + nca.type = NCSI_PKT_CMD_OEM;
> > + nca.payload = 8;
> > +
> > + nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
> > + nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_GET_MAC);
> > +
> > + ret = ncsi_xmit_cmd(&nca);
> > + if (ret)
> > + netdev_err(ndp->ndev.dev,
> > + "NCSI: Failed to transmit cmd 0x%x during probe\n",
> > + nca.type);
> > +}
> > +
> > +static void set_mac_affinity_mlx(struct ncsi_dev_priv *ndp)
> > +{
> > + struct ncsi_cmd_arg nca;
> > + int ret = 0;
> > +
> > + memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
>
> Ah I see we initialise nca, and thus payload here - the path between
> these two points in the driver isn't super obvious (not this patch's
> fault :) ), it might be better to explicitly mention/handle this where we
> use payload later on.
>
> > + nca.ndp = ndp;
> > + nca.channel = ndp->active_channel->id;
> > + nca.package = ndp->active_package->id;
>
> These should probably be set in ncsi_configure_channel (eg. see the CIS
> state before these are called).
>
> > + nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> > + nca.type = NCSI_PKT_CMD_OEM;
> > + nca.payload = 60;
> > +
> > + nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
> > + nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_SET_AFFINITY);
> > +
> > + memcpy(&(nca.bytes[8]), ndp->ndev.dev->dev_addr, ETH_ALEN);
> > + nca.bytes[14] = 0x09;
> > +
> > + ret = ncsi_xmit_cmd(&nca);
> > + if (ret)
> > + netdev_err(ndp->ndev.dev,
> > + "NCSI: Failed to transmit cmd 0x%x during probe\n",
> > + nca.type);
> > +}
> > +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
> > +
> > static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> > {
> > struct ncsi_dev *nd = &ndp->ndev;
> > @@ -685,6 +737,22 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> > goto error;
> > }
> >
> > +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
> > + /* Check Manufacture id if it is Mellanox then
> > + * get and set mac address. To Do: Add code for
> > + * other types of card if required
> > + */
> > + if (nc->version.mf_id == NCSI_OEM_MFR_MLX_ID)
> > + nd->state = ncsi_dev_state_config_oem_gma;
> > + else
> > + nd->state = ncsi_dev_state_config_clear_vids;
> > + break;
> > + case ncsi_dev_state_config_oem_gma:
> > + ndp->pending_req_num = 2;
> > + get_mac_address_oem_mlx(ndp);
> > + msleep(500);
>
> Is this msleep() required?
>
> > + set_mac_affinity_mlx(ndp);
>
> Since this *sets* the MAC address, should we name the state and config
> option more accurately? How does this OEM command interact with the NCSI
> set-mac-address command?
> Does this command depend on the previous get_mac_address_oem_mlx()
> command succeeding?
>
> > +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
> > nd->state = ncsi_dev_state_config_clear_vids;
> > break;
> > case ncsi_dev_state_config_clear_vids:
> > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> > index 91b4b66438df..0653a893eb12 100644
> > --- a/net/ncsi/ncsi-pkt.h
> > +++ b/net/ncsi/ncsi-pkt.h
> > @@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
> > unsigned char pad[22];
> > };
> >
> > +/* Oem Request Command */
>
> In general, s/Oem/OEM
>
> > +struct ncsi_cmd_oem_pkt {
> > + struct ncsi_cmd_pkt_hdr cmd; /* Command header */
> > + unsigned char data[64]; /* OEM Payload Data */
> > + __be32 checksum; /* Checksum */
> > +};
> > +
> > +/* Oem Response Packet */
> > +struct ncsi_rsp_oem_pkt {
> > + struct ncsi_rsp_pkt_hdr rsp; /* Command header */
> > + __be32 mfr_id; /* Manufacture ID */
> > + __be32 oem_cmd; /* oem command */
> > + unsigned char data[32]; /* Payload data */
> > + __be32 checksum; /* Checksum */
> > +};
> > +
> > /* Get Link Status */
> > struct ncsi_rsp_gls_pkt {
> > struct ncsi_rsp_pkt_hdr rsp; /* Response header */
> > diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> > index 930c1d3796f0..3b94c96b9c7f 100644
> > --- a/net/ncsi/ncsi-rsp.c
> > +++ b/net/ncsi/ncsi-rsp.c
> > @@ -596,6 +596,37 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
> > return 0;
> > }
> >
> > +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
> > +{
> > + struct ncsi_rsp_oem_pkt *rsp;
> > + struct ncsi_dev_priv *ndp = nr->ndp;
> > + struct net_device *ndev = ndp->ndev.dev;
> > + int ret = 0;
> > + unsigned int oem_cmd, mfr_id;
> > + const struct net_device_ops *ops = ndev->netdev_ops;
> > + struct sockaddr saddr;
> > +
> > + /* Get the response header */
> > + rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> > +
> > + oem_cmd = ntohl(rsp->oem_cmd);
> > + mfr_id = ntohl(rsp->mfr_id);
> > +
> > + /* Check for Mellanox manufacturer id */
> > + if (mfr_id != NCSI_OEM_MFR_MLX_ID)
> > + return 0;
> > +
> > + if (oem_cmd == NCSI_OEM_MLX_CMD_GET_MAC) {
> > + saddr.sa_family = ndev->type;
> > + ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> > + memcpy(saddr.sa_data, &(rsp->data[4]), ETH_ALEN);
> > + ret = ops->ndo_set_mac_address(ndev, &saddr);
> > + if (ret < 0)
> > + netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
> > + }
> > + return ret;
> > +}
> > +
> > static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
> > {
> > struct ncsi_rsp_gvi_pkt *rsp;
> > @@ -932,7 +963,7 @@ static struct ncsi_rsp_handler {
> > { NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
> > { NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
> > { NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
> > - { NCSI_PKT_RSP_OEM, 0, NULL },
> > + { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
> > { NCSI_PKT_RSP_PLDM, 0, NULL },
> > { NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
> > };
^ permalink raw reply
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send
From: Stephen Hemminger @ 2018-09-27 10:23 UTC (permalink / raw)
To: Mohammed Gamal
Cc: Haiyang Zhang, Stephen Hemminger, netdev@vger.kernel.org,
otubo@redhat.com, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, vkuznets
In-Reply-To: <1538038625.19334.2.camel@redhat.com>
On Thu, 27 Sep 2018 10:57:05 +0200
Mohammed Gamal <mgamal@redhat.com> wrote:
> On Wed, 2018-09-26 at 17:13 +0000, Haiyang Zhang wrote:
> > > -----Original Message-----
> > > From: Mohammed Gamal <mgamal@redhat.com>
> > > Sent: Wednesday, September 26, 2018 12:34 PM
> > > To: Stephen Hemminger <sthemmin@microsoft.com>; netdev@vger.kernel.
> > > org
> > > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > > <haiyangz@microsoft.com>; vkuznets <vkuznets@redhat.com>;
> > > otubo@redhat.com; cavery <cavery@redhat.com>; linux-
> > > kernel@vger.kernel.org; devel@linuxdriverproject.org; Mohammed
> > > Gamal
> > > <mgamal@redhat.com>
> > > Subject: [PATCH] hv_netvsc: Make sure out channel is fully opened
> > > on send
> > >
> > > Dring high network traffic changes to network interface parameters
> > > such as
> > > number of channels or MTU can cause a kernel panic with a NULL
> > > pointer
> > > dereference. This is due to netvsc_device_remove() being called and
> > > deallocating the channel ring buffers, which can then be accessed
> > > by
> > > netvsc_send_pkt() before they're allocated on calling
> > > netvsc_device_add()
> > >
> > > The patch fixes this problem by checking the channel state and
> > > returning
> > > ENODEV if not yet opened. We also move the call to
> > > hv_ringbuf_avail_percent()
> > > which may access the uninitialized ring buffer.
> > >
> > > Signed-off-by: Mohammed Gamal <mgamal@redhat.com>
> > > ---
> > > drivers/net/hyperv/netvsc.c | 7 ++++++-
> > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/hyperv/netvsc.c
> > > b/drivers/net/hyperv/netvsc.c index
> > > fe01e14..75f1b31 100644
> > > --- a/drivers/net/hyperv/netvsc.c
> > > +++ b/drivers/net/hyperv/netvsc.c
> > > @@ -825,7 +825,12 @@ static inline int netvsc_send_pkt(
> > > struct netdev_queue *txq = netdev_get_tx_queue(ndev,
> > > packet->q_idx);
> > > u64 req_id;
> > > int ret;
> > > - u32 ring_avail =
> > > hv_get_avail_to_write_percent(&out_channel-
> > > > outbound);
> > >
> > > + u32 ring_avail;
> > > +
> > > + if (out_channel->state != CHANNEL_OPENED_STATE)
> > > + return -ENODEV;
> > > +
> > > + ring_avail = hv_get_avail_to_write_percent(&out_channel-
> > > >outbound);
> >
> > When you reproducing the NULL ptr panic, does your kernel include the
> > following patch?
> > hv_netvsc: common detach logic
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/c
> > ommit/?id=7b2ee50c0cd513a176a26a71f2989facdd75bfea
> >
> Yes it is included. And the commit did reduce the occurrence of this
> race condition, but it still nevertheless occurs albeit rarely.
>
> > We call netif_tx_disable(ndev) and netif_device_detach(ndev) before
> > doing the changes
> > on MTU or #channels. So there should be no call to start_xmit() when
> > channel is not ready.
> >
> > If you see the check for CHANNEL_OPENED_STATE is still necessary on
> > upstream kernel (including
> > the patch " common detach logic "), we should debug further on the
> > code and find out the
> > root cause.
> >
> > Thanks,
> > - Haiyang
> >
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Is there some workload, that can be used to reproduce this?
The stress test from Vitaly with changing parameters while running network traffic
passes now.
Can you reproduce this with the upstream current kernel?
Adding the check in start xmit is still racy, and won't cure the problem.
Another solution would be to add a grace period in the netvsc detach logic.
^ permalink raw reply
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send
From: Mohammed Gamal @ 2018-09-27 10:31 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Haiyang Zhang, Stephen Hemminger, netdev@vger.kernel.org,
otubo@redhat.com, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, vkuznets
In-Reply-To: <20180927122355.470df119@shemminger-XPS-13-9360>
On Thu, 2018-09-27 at 12:23 +0200, Stephen Hemminger wrote:
> On Thu, 27 Sep 2018 10:57:05 +0200
> Mohammed Gamal <mgamal@redhat.com> wrote:
>
> > On Wed, 2018-09-26 at 17:13 +0000, Haiyang Zhang wrote:
> > > > -----Original Message-----
> > > > From: Mohammed Gamal <mgamal@redhat.com>
> > > > Sent: Wednesday, September 26, 2018 12:34 PM
> > > > To: Stephen Hemminger <sthemmin@microsoft.com>; netdev@vger.ker
> > > > nel.
> > > > org
> > > > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > > > <haiyangz@microsoft.com>; vkuznets <vkuznets@redhat.com>;
> > > > otubo@redhat.com; cavery <cavery@redhat.com>; linux-
> > > > kernel@vger.kernel.org; devel@linuxdriverproject.org; Mohammed
> > > > Gamal
> > > > <mgamal@redhat.com>
> > > > Subject: [PATCH] hv_netvsc: Make sure out channel is fully
> > > > opened
> > > > on send
> > > >
> > > > Dring high network traffic changes to network interface
> > > > parameters
> > > > such as
> > > > number of channels or MTU can cause a kernel panic with a NULL
> > > > pointer
> > > > dereference. This is due to netvsc_device_remove() being called
> > > > and
> > > > deallocating the channel ring buffers, which can then be
> > > > accessed
> > > > by
> > > > netvsc_send_pkt() before they're allocated on calling
> > > > netvsc_device_add()
> > > >
> > > > The patch fixes this problem by checking the channel state and
> > > > returning
> > > > ENODEV if not yet opened. We also move the call to
> > > > hv_ringbuf_avail_percent()
> > > > which may access the uninitialized ring buffer.
> > > >
> > > > Signed-off-by: Mohammed Gamal <mgamal@redhat.com>
> > > > ---
> > > > drivers/net/hyperv/netvsc.c | 7 ++++++-
> > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/hyperv/netvsc.c
> > > > b/drivers/net/hyperv/netvsc.c index
> > > > fe01e14..75f1b31 100644
> > > > --- a/drivers/net/hyperv/netvsc.c
> > > > +++ b/drivers/net/hyperv/netvsc.c
> > > > @@ -825,7 +825,12 @@ static inline int netvsc_send_pkt(
> > > > struct netdev_queue *txq = netdev_get_tx_queue(ndev,
> > > > packet->q_idx);
> > > > u64 req_id;
> > > > int ret;
> > > > - u32 ring_avail =
> > > > hv_get_avail_to_write_percent(&out_channel-
> > > > > outbound);
> > > >
> > > > + u32 ring_avail;
> > > > +
> > > > + if (out_channel->state != CHANNEL_OPENED_STATE)
> > > > + return -ENODEV;
> > > > +
> > > > + ring_avail =
> > > > hv_get_avail_to_write_percent(&out_channel-
> > > > > outbound);
> > >
> > > When you reproducing the NULL ptr panic, does your kernel include
> > > the
> > > following patch?
> > > hv_netvsc: common detach logic
> > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.g
> > > it/c
> > > ommit/?id=7b2ee50c0cd513a176a26a71f2989facdd75bfea
> > >
> >
> > Yes it is included. And the commit did reduce the occurrence of
> > this
> > race condition, but it still nevertheless occurs albeit rarely.
> >
> > > We call netif_tx_disable(ndev) and netif_device_detach(ndev)
> > > before
> > > doing the changes
> > > on MTU or #channels. So there should be no call to start_xmit()
> > > when
> > > channel is not ready.
> > >
> > > If you see the check for CHANNEL_OPENED_STATE is still necessary
> > > on
> > > upstream kernel (including
> > > the patch " common detach logic "), we should debug further on
> > > the
> > > code and find out the
> > > root cause.
> > >
> > > Thanks,
> > > - Haiyang
> > >
> >
> > _______________________________________________
> > devel mailing list
> > devel@linuxdriverproject.org
> > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-
> > devel
>
> Is there some workload, that can be used to reproduce this?
> The stress test from Vitaly with changing parameters while running
> network traffic
> passes now.
>
> Can you reproduce this with the upstream current kernel?
>
> Adding the check in start xmit is still racy, and won't cure the
> problem.
>
> Another solution would be to add a grace period in the netvsc detach
> logic.
>
Steps to reproduce are listed here:
https://bugzilla.redhat.com/show_bug.cgi?id=1632653
We've also managed to reproduce the same issue upstream. It's more
likely to be reproduced on Windows 2012R2 than 2016.
Regards,
Mohammed
^ 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