* [PATCH net-next 00/14] remove copies of the NAPI_POLL_WEIGHT define
From: Jakub Kicinski @ 2022-04-27 15:40 UTC (permalink / raw)
To: davem, pabeni; +Cc: netdev, Jakub Kicinski
netif_napi_add() takes weight as the last argument. The value of
that parameter is hard to come up with and depends on many factors,
so driver authors are encouraged to use NAPI_POLL_WEIGHT.
We should probably move weight to an "advanced" version of the API
(__netif_napi_add()?) and simplify the life of most driver authors.
In preparation for such API changes this series removes local
defines equivalent to NAPI_POLL_WEIGHT from drivers, so that a simple
coccinelle / spatch script does not get thrown off by them.
Jakub Kicinski (14):
eth: remove copies of the NAPI_POLL_WEIGHT define
eth: remove NAPI_WEIGHT defines
eth: cpsw: remove a copy of the NAPI_POLL_WEIGHT define
eth: pch_gbe: remove a copy of the NAPI_POLL_WEIGHT define
eth: mtk_eth_soc: remove a copy of the NAPI_POLL_WEIGHT define
usb: lan78xx: remove a copy of the NAPI_POLL_WEIGHT define
slic: remove a copy of the NAPI_POLL_WEIGHT define
eth: bgnet: remove a copy of the NAPI_POLL_WEIGHT define
eth: atlantic: remove a copy of the NAPI_POLL_WEIGHT define
eth: benet: remove a copy of the NAPI_POLL_WEIGHT define
eth: gfar: remove a copy of the NAPI_POLL_WEIGHT define
eth: vxge: remove a copy of the NAPI_POLL_WEIGHT define
eth: spider: remove a copy of the NAPI_POLL_WEIGHT define
eth: velocity: remove a copy of the NAPI_POLL_WEIGHT define
drivers/net/ethernet/alacritech/slic.h | 2 --
drivers/net/ethernet/alacritech/slicoss.c | 2 +-
drivers/net/ethernet/aquantia/atlantic/aq_cfg.h | 2 --
drivers/net/ethernet/aquantia/atlantic/aq_ptp.c | 2 +-
drivers/net/ethernet/aquantia/atlantic/aq_vec.c | 2 +-
drivers/net/ethernet/broadcom/bgmac.c | 2 +-
drivers/net/ethernet/broadcom/bgmac.h | 2 --
drivers/net/ethernet/cortina/gemini.c | 4 +---
drivers/net/ethernet/emulex/benet/be.h | 3 +--
drivers/net/ethernet/emulex/benet/be_main.c | 2 +-
drivers/net/ethernet/freescale/gianfar.c | 2 +-
drivers/net/ethernet/freescale/gianfar.h | 3 ---
drivers/net/ethernet/marvell/skge.c | 3 +--
drivers/net/ethernet/marvell/sky2.c | 3 +--
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 4 ++--
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 1 -
drivers/net/ethernet/mediatek/mtk_star_emac.c | 3 +--
drivers/net/ethernet/neterion/vxge/vxge-main.c | 2 +-
drivers/net/ethernet/neterion/vxge/vxge-main.h | 2 --
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 12 +++++-------
drivers/net/ethernet/smsc/smsc9420.c | 2 +-
drivers/net/ethernet/smsc/smsc9420.h | 1 -
drivers/net/ethernet/ti/cpsw.c | 4 ++--
drivers/net/ethernet/ti/cpsw_new.c | 4 ++--
drivers/net/ethernet/ti/cpsw_priv.c | 12 ++++++------
drivers/net/ethernet/ti/cpsw_priv.h | 1 -
drivers/net/ethernet/ti/davinci_emac.c | 3 +--
drivers/net/ethernet/ti/netcp_core.c | 5 ++---
drivers/net/ethernet/toshiba/spider_net.c | 2 +-
drivers/net/ethernet/toshiba/spider_net.h | 1 -
drivers/net/ethernet/via/via-velocity.c | 3 +--
drivers/net/ethernet/via/via-velocity.h | 1 -
drivers/net/usb/lan78xx.c | 4 +---
drivers/net/xen-netback/interface.c | 3 +--
drivers/staging/unisys/visornic/visornic_main.c | 4 ++--
35 files changed, 39 insertions(+), 69 deletions(-)
--
2.34.1
^ permalink raw reply
* [PATCH net-next 02/14] eth: remove NAPI_WEIGHT defines
From: Jakub Kicinski @ 2022-04-27 15:40 UTC (permalink / raw)
To: davem, pabeni
Cc: netdev, Jakub Kicinski, steve.glendinning, david.kershner, gregkh,
liujunqi, sparmaintainer, linux-staging
In-Reply-To: <20220427154111.529975-1-kuba@kernel.org>
Defining local versions of NAPI_POLL_WEIGHT with the same
values in the drivers just makes refactoring harder.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: steve.glendinning@shawell.net
CC: david.kershner@unisys.com
CC: gregkh@linuxfoundation.org
CC: liujunqi@pku.edu.cn
CC: sparmaintainer@unisys.com
CC: linux-staging@lists.linux.dev
---
drivers/net/ethernet/smsc/smsc9420.c | 2 +-
drivers/net/ethernet/smsc/smsc9420.h | 1 -
drivers/staging/unisys/visornic/visornic_main.c | 4 ++--
3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc9420.c b/drivers/net/ethernet/smsc/smsc9420.c
index d937af18973e..0c68c7f8056d 100644
--- a/drivers/net/ethernet/smsc/smsc9420.c
+++ b/drivers/net/ethernet/smsc/smsc9420.c
@@ -1585,7 +1585,7 @@ smsc9420_probe(struct pci_dev *pdev, const struct pci_device_id *id)
dev->netdev_ops = &smsc9420_netdev_ops;
dev->ethtool_ops = &smsc9420_ethtool_ops;
- netif_napi_add(dev, &pd->napi, smsc9420_rx_poll, NAPI_WEIGHT);
+ netif_napi_add(dev, &pd->napi, smsc9420_rx_poll, NAPI_POLL_WEIGHT);
result = register_netdev(dev);
if (result) {
diff --git a/drivers/net/ethernet/smsc/smsc9420.h b/drivers/net/ethernet/smsc/smsc9420.h
index 409e82b2018a..876410a256c6 100644
--- a/drivers/net/ethernet/smsc/smsc9420.h
+++ b/drivers/net/ethernet/smsc/smsc9420.h
@@ -15,7 +15,6 @@
/* interrupt deassertion in multiples of 10us */
#define INT_DEAS_TIME (50)
-#define NAPI_WEIGHT (64)
#define SMSC_BAR (3)
#ifdef __BIG_ENDIAN
diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 643432458105..81ac3ac05192 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -26,7 +26,6 @@
* = 163840 bytes
*/
#define MAX_BUF 163840
-#define NAPI_WEIGHT 64
/* GUIDS for director channel type supported by this driver. */
/* {8cd5994d-c58e-11da-95a9-00e08161165f} */
@@ -1884,7 +1883,8 @@ static int visornic_probe(struct visor_device *dev)
/* TODO: Setup Interrupt information */
/* Let's start our threads to get responses */
- netif_napi_add(netdev, &devdata->napi, visornic_poll, NAPI_WEIGHT);
+ netif_napi_add(netdev, &devdata->napi, visornic_poll,
+ NAPI_POLL_WEIGHT);
channel_offset = offsetof(struct visor_io_channel,
channel_header.features);
--
2.34.1
^ permalink raw reply related
* Re: [PATCH net] usbnet: smsc95xx: Fix deadlock on runtime resume
From: Lukas Wunner @ 2022-04-27 15:38 UTC (permalink / raw)
To: Andrew Lunn
Cc: Steve Glendinning, UNGLinuxDriver, Oliver Neukum, David S. Miller,
Jakub Kicinski, Paolo Abeni, netdev, linux-usb, Andre Edich,
Oleksij Rempel, Martyn Welch, Gabriel Hojda, Christoph Fritz,
Lino Sanfilippo, Philipp Rosenberger, Heiner Kallweit,
Russell King
In-Reply-To: <YmlgQhauzZ/tkX/v@lunn.ch>
On Wed, Apr 27, 2022 at 05:24:50PM +0200, Andrew Lunn wrote:
> On Wed, Apr 27, 2022 at 08:41:49AM +0200, Lukas Wunner wrote:
> > Commit 05b35e7eb9a1 ("smsc95xx: add phylib support") amended
> > smsc95xx_resume() to call phy_init_hw(). That function waits for the
> > device to runtime resume even though it is placed in the runtime resume
> > path, causing a deadlock.
>
> You have looked at this code, tried a few different things, so this is
> probably a dumb question.
>
> Do you actually need to call phy_init_hw()?
>
> mdio_bus_phy_resume() will call phy_init_hw(). So long as you first
> resume the MAC and then the PHY, shouldn't this just work?
mdio_bus_phy_resume() is only called for system sleep. But this is about
*runtime* PM.
mdio_bus_phy_pm_ops does not define runtime PM callbacks. So runtime PM
is disabled on PHYs, no callback is invoked for them when the MAC runtime
suspends, hence the onus is on the MAC to runtime suspend the PHY (which
is a child of the MAC). Same on runtime resume.
Let's say I enable runtime PM on the PHY and use pm_runtime_force_suspend()
to suspend the PHY from the MAC's ->runtime_suspend callback. At that
point the MAC already has status RPM_SUSPENDING. Boom, deadlock.
The runtime PM core lacks the capability to declare that children should
be force runtime suspended before a device can runtime suspend, that's
the problem.
Thanks,
Lukas
^ permalink raw reply
* Re: [PATCH net-next v2 1/2] net: phy: microchip: update LAN88xx phy ID and phy ID mask.
From: Andrew Lunn @ 2022-04-27 15:36 UTC (permalink / raw)
To: Yuiko Oshino; +Cc: woojung.huh, davem, netdev, ravi.hegde, UNGLinuxDriver
In-Reply-To: <20220427121957.13099-2-yuiko.oshino@microchip.com>
On Wed, Apr 27, 2022 at 05:19:56AM -0700, Yuiko Oshino wrote:
> update LAN88xx phy ID and phy ID mask because the existing code conflicts with the LAN8742 phy.
>
> The current phy IDs on the available hardware.
> LAN8742 0x0007C130, 0x0007C131
> LAN88xx 0x0007C132
>
> Signed-off-by: Yuiko Oshino <yuiko.oshino@microchip.com>
> ---
> drivers/net/phy/microchip.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/phy/microchip.c b/drivers/net/phy/microchip.c
> index 9f1f2b6c97d4..131caf659ed2 100644
> --- a/drivers/net/phy/microchip.c
> +++ b/drivers/net/phy/microchip.c
> @@ -344,8 +344,8 @@ static int lan88xx_config_aneg(struct phy_device *phydev)
>
> static struct phy_driver microchip_phy_driver[] = {
> {
> - .phy_id = 0x0007c130,
> - .phy_id_mask = 0xfffffff0,
> + .phy_id = 0x0007c132,
> + .phy_id_mask = 0xfffffff2,
Just so my comment on the previous version does not get lost, is this
the correct mask, because it is very odd. I think you really want
0xfffffffe?
Andrew
^ permalink raw reply
* Re: [PATCH v2 net-next] net: generalize skb freeing deferral to per-cpu lists
From: Ido Schimmel @ 2022-04-27 15:34 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
Eric Dumazet
In-Reply-To: <20220422201237.416238-1-eric.dumazet@gmail.com>
On Fri, Apr 22, 2022 at 01:12:37PM -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Logic added in commit f35f821935d8 ("tcp: defer skb freeing after socket
> lock is released") helped bulk TCP flows to move the cost of skbs
> frees outside of critical section where socket lock was held.
>
> But for RPC traffic, or hosts with RFS enabled, the solution is far from
> being ideal.
>
> For RPC traffic, recvmsg() has to return to user space right after
> skb payload has been consumed, meaning that BH handler has no chance
> to pick the skb before recvmsg() thread. This issue is more visible
> with BIG TCP, as more RPC fit one skb.
>
> For RFS, even if BH handler picks the skbs, they are still picked
> from the cpu on which user thread is running.
>
> Ideally, it is better to free the skbs (and associated page frags)
> on the cpu that originally allocated them.
>
> This patch removes the per socket anchor (sk->defer_list) and
> instead uses a per-cpu list, which will hold more skbs per round.
>
> This new per-cpu list is drained at the end of net_action_rx(),
> after incoming packets have been processed, to lower latencies.
>
> In normal conditions, skbs are added to the per-cpu list with
> no further action. In the (unlikely) cases where the cpu does not
> run net_action_rx() handler fast enough, we use an IPI to raise
> NET_RX_SOFTIRQ on the remote cpu.
>
> Also, we do not bother draining the per-cpu list from dev_cpu_dead()
> This is because skbs in this list have no requirement on how fast
> they should be freed.
>
> Note that we can add in the future a small per-cpu cache
> if we see any contention on sd->defer_lock.
>
> Tested on a pair of hosts with 100Gbit NIC, RFS enabled,
> and /proc/sys/net/ipv4/tcp_rmem[2] tuned to 16MB to work around
> page recycling strategy used by NIC driver (its page pool capacity
> being too small compared to number of skbs/pages held in sockets
> receive queues)
>
> Note that this tuning was only done to demonstrate worse
> conditions for skb freeing for this particular test.
> These conditions can happen in more general production workload.
[...]
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Eric, with this patch I'm seeing memory leaks such as these [1][2] after
boot. The system is using the igb driver for its management interface
[3]. The leaks disappear after reverting the patch.
Any ideas?
Let me know if you need more info. I can easily test a patch.
Thanks
[1]
# cat /sys/kernel/debug/kmemleak
unreferenced object 0xffff888170143740 (size 216):
comm "softirq", pid 0, jiffies 4294825261 (age 95.244s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 17 0f 81 88 ff ff 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffff82571fc0>] napi_skb_cache_get+0xf0/0x180
[<ffffffff8257206a>] __napi_build_skb+0x1a/0x60
[<ffffffff8257b0f3>] napi_build_skb+0x23/0x350
[<ffffffffa0469592>] igb_poll+0x2b72/0x5880 [igb]
[<ffffffff825f9584>] __napi_poll.constprop.0+0xb4/0x480
[<ffffffff825f9d5a>] net_rx_action+0x40a/0xc60
[<ffffffff82e00295>] __do_softirq+0x295/0x9fe
[<ffffffff81185bcc>] __irq_exit_rcu+0x11c/0x180
[<ffffffff8118622a>] irq_exit_rcu+0xa/0x20
[<ffffffff82bbed39>] common_interrupt+0xa9/0xc0
[<ffffffff82c00b5e>] asm_common_interrupt+0x1e/0x40
[<ffffffff824a186e>] cpuidle_enter_state+0x27e/0xcb0
[<ffffffff824a236f>] cpuidle_enter+0x4f/0xa0
[<ffffffff8126a290>] do_idle+0x3b0/0x4b0
[<ffffffff8126a869>] cpu_startup_entry+0x19/0x20
[<ffffffff810f4725>] start_secondary+0x265/0x340
[2]
# cat /sys/kernel/debug/kmemleak
unreferenced object 0xffff88810ce3aac0 (size 216):
comm "softirq", pid 0, jiffies 4294861408 (age 64.607s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 c0 7b 07 81 88 ff ff 00 00 00 00 00 00 00 00 ..{.............
backtrace:
[<ffffffff82575539>] __alloc_skb+0x229/0x360
[<ffffffff8290bd3c>] __tcp_send_ack.part.0+0x6c/0x760
[<ffffffff8291a062>] tcp_send_ack+0x82/0xa0
[<ffffffff828cb6db>] __tcp_ack_snd_check+0x15b/0xa00
[<ffffffff828f17fe>] tcp_rcv_established+0x198e/0x2120
[<ffffffff829363b5>] tcp_v4_do_rcv+0x665/0x9a0
[<ffffffff8293d8ae>] tcp_v4_rcv+0x2c1e/0x32f0
[<ffffffff828610b3>] ip_protocol_deliver_rcu+0x53/0x2c0
[<ffffffff828616eb>] ip_local_deliver+0x3cb/0x620
[<ffffffff8285e66f>] ip_sublist_rcv_finish+0x9f/0x2c0
[<ffffffff82860895>] ip_list_rcv_finish.constprop.0+0x525/0x6f0
[<ffffffff82861f88>] ip_list_rcv+0x318/0x460
[<ffffffff825f5e61>] __netif_receive_skb_list_core+0x541/0x8f0
[<ffffffff825f8043>] netif_receive_skb_list_internal+0x763/0xdc0
[<ffffffff826c3025>] napi_gro_complete.constprop.0+0x5a5/0x700
[<ffffffff826c44ed>] dev_gro_receive+0xf2d/0x23f0
unreferenced object 0xffff888175e1afc0 (size 216):
comm "sshd", pid 1024, jiffies 4294861424 (age 64.591s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 c0 7b 07 81 88 ff ff 00 00 00 00 00 00 00 00 ..{.............
backtrace:
[<ffffffff82575539>] __alloc_skb+0x229/0x360
[<ffffffff8258201c>] alloc_skb_with_frags+0x9c/0x720
[<ffffffff8255f333>] sock_alloc_send_pskb+0x7b3/0x940
[<ffffffff82876af4>] __ip_append_data+0x1874/0x36d0
[<ffffffff8287f283>] ip_make_skb+0x263/0x2e0
[<ffffffff82978afa>] udp_sendmsg+0x1c8a/0x29d0
[<ffffffff829af94e>] inet_sendmsg+0x9e/0xe0
[<ffffffff8255082d>] __sys_sendto+0x23d/0x360
[<ffffffff82550a31>] __x64_sys_sendto+0xe1/0x1b0
[<ffffffff82bbde05>] do_syscall_64+0x35/0x80
[<ffffffff82c00068>] entry_SYSCALL_64_after_hwframe+0x44/0xae
[3]
# ethtool -i enp8s0
driver: igb
version: 5.18.0-rc3-custom-91743-g481c1b
firmware-version: 3.25, 0x80000708, 1.1824.0
expansion-rom-version:
bus-info: 0000:08:00.0
supports-statistics: yes
supports-test: yes
supports-eeprom-access: yes
supports-register-dump: yes
supports-priv-flags: yes
^ permalink raw reply
* Re: [REVIEW REQUEST3 PATCH net-next 1/2] net: phy: microchip: update LAN88xx phy ID and phy ID mask.
From: Andrew Lunn @ 2022-04-27 15:30 UTC (permalink / raw)
To: Yuiko Oshino; +Cc: woojung.huh, davem, netdev, ravi.hegde, UNGLinuxDriver
In-Reply-To: <20220427115142.12642-2-yuiko.oshino@microchip.com>
On Wed, Apr 27, 2022 at 04:51:41AM -0700, Yuiko Oshino wrote:
> update LAN88xx phy ID and phy ID mask because the existing code conflicts with the LAN8742 phy.
>
> The current phy IDs on the available hardware.
> LAN8742 0x0007C130, 0x0007C131
> LAN88xx 0x0007C132
>
> Signed-off-by: Yuiko Oshino <yuiko.oshino@microchip.com>
> ---
> drivers/net/phy/microchip.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/phy/microchip.c b/drivers/net/phy/microchip.c
> index 9f1f2b6c97d4..131caf659ed2 100644
> --- a/drivers/net/phy/microchip.c
> +++ b/drivers/net/phy/microchip.c
> @@ -344,8 +344,8 @@ static int lan88xx_config_aneg(struct phy_device *phydev)
>
> static struct phy_driver microchip_phy_driver[] = {
> {
> - .phy_id = 0x0007c130,
> - .phy_id_mask = 0xfffffff0,
> + .phy_id = 0x0007c132,
> + .phy_id_mask = 0xfffffff2,
That is a very odd mask. Is that really correct?
Please also look at the patch subject line, it is not correct.
Andrew
^ permalink raw reply
* Re: [PATCH net] usbnet: smsc95xx: Fix deadlock on runtime resume
From: Andrew Lunn @ 2022-04-27 15:24 UTC (permalink / raw)
To: Lukas Wunner
Cc: Steve Glendinning, UNGLinuxDriver, Oliver Neukum, David S. Miller,
Jakub Kicinski, Paolo Abeni, netdev, linux-usb, Andre Edich,
Oleksij Rempel, Martyn Welch, Gabriel Hojda, Christoph Fritz,
Lino Sanfilippo, Philipp Rosenberger, Heiner Kallweit,
Russell King
In-Reply-To: <6710d8c18ff54139cdc538763ba544187c5a0cee.1651041411.git.lukas@wunner.de>
On Wed, Apr 27, 2022 at 08:41:49AM +0200, Lukas Wunner wrote:
> Commit 05b35e7eb9a1 ("smsc95xx: add phylib support") amended
> smsc95xx_resume() to call phy_init_hw(). That function waits for the
> device to runtime resume even though it is placed in the runtime resume
> path, causing a deadlock.
Hi Lukas
You have looked at this code, tried a few different things, so this is
probably a dumb question.
Do you actually need to call phy_init_hw()?
mdio_bus_phy_resume() will call phy_init_hw(). So long as you first
resume the MAC and then the PHY, shouldn't this just work?
Andrew
^ permalink raw reply
* Re: [PATCH RFC 4/5] net/tls: Add support for PF_TLSH (a TLS handshake listener)
From: Chuck Lever III @ 2022-04-27 15:24 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Hannes Reinecke, Sagi Grimberg, netdev, Linux NFS Mailing List,
linux-nvme@lists.infradead.org, CIFS, linux-fsdevel,
Alexander Krizhanovsky, Boris Pismenny, simo@redhat.com
In-Reply-To: <20220426170334.3781cd0e@kernel.org>
> On Apr 26, 2022, at 8:03 PM, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 26 Apr 2022 17:58:39 +0200 Hannes Reinecke wrote:
>>
>> - Establishing sockets from userspace will cause issues during
>> reconnection, as then someone (aka the kernel) will have to inform
>> userspace that a new connection will need to be established.
>> (And that has to happen while the root filesystem is potentially
>> inaccessible, so you can't just call arbitrary commands here)
>> (Especially call_usermodehelper() is out of the game)
>
> Indeed, we may need _some_ form of a notification mechanism and that's
> okay. Can be a (more generic) socket, can be something based on existing
> network storage APIs (IDK what you have there).
>
> My thinking was that establishing the session in user space would be
> easiest. We wouldn't need all the special getsockopt()s which AFAIU
> work around part of the handshake being done in the kernel, and which,
> I hope we can agree, are not beautiful.
In the prototype, the new socket options on AF_TLSH sockets
include:
#define TLSH_PRIORITIES 1 /* Retrieve TLS priorities string */
#define TLSH_PEERID 2 /* Retrieve peer identity */
#define TLSH_HANDSHAKE_TYPE 3 /* Retrieve handshake type */
#define TLSH_X509_CERTIFICATE 4 /* Retrieve x.509 certificate */
PRIORITIES is the TLS priorities string that the GnuTLS library
uses to parametrize the handshake (which TLS versions, ciphers,
and so on).
PEERID is a keyring serial number for the key that contains the
a Pre-Shared Key (for PSK handshakes) or the private key (for
x.509 handshakes).
HANDSHAKE_TYPE is an integer that represents the type of handshake
being requested: ClientHello, ServerHello, Rekey, and so on. This
option enables the repertoire of handshake types to be expanded.
X509_CERTIFICATE is a keyring serial number for the key that
contains an x.509 certificate.
When each handshake is complete, the handshake agent instantiates
the IV into the passed-in socket using existing kTLS socket options
before it returns the endpoint to the kernel.
There is nothing in these options that indicates to the handshake
agent what upper layer protocol is going to be used inside the TLS
session.
----
The new AF_TLSH socket options are not there because the handshake
is split between the kernel and user space. They are there because
the initial requester is (eg, in the case of NFS) mount.nfs, another
user space program. mount.nfs has to package up an x.509 cert or
pre-shared key and place it on a keyring to make it available to
the handshake agent.
The basic issue is that the administrative interfaces that
parametrize the handshakes are quite distant from the in-kernel
consumers that make handshake requests.
----
Further, in order to support server side TLS handshakes in the
kernel, we really do have to pass a kernel-created socket up to
user space. NFSD (and maybe the NVMe target) use in-kernel listeners
to accept incoming connections. Each new endpoint is created in the
kernel.
So if you truly seek generality in this facility, the user
space componentry must work with passed-in sockets rather than
creating them in user space.
--
Chuck Lever
^ permalink raw reply
* Re: [PATCH net] usbnet: smsc95xx: Fix deadlock on runtime resume
From: Lukas Wunner @ 2022-04-27 15:10 UTC (permalink / raw)
To: Alan Stern
Cc: Steve Glendinning, UNGLinuxDriver, Oliver Neukum, David S. Miller,
Jakub Kicinski, Paolo Abeni, netdev, linux-usb, Andre Edich,
Oleksij Rempel, Martyn Welch, Gabriel Hojda, Christoph Fritz,
Lino Sanfilippo, Philipp Rosenberger, Heiner Kallweit,
Andrew Lunn, Russell King
In-Reply-To: <YmlMaE53+EhRz5it@rowland.harvard.edu>
On Wed, Apr 27, 2022 at 10:00:08AM -0400, Alan Stern wrote:
> On Wed, Apr 27, 2022 at 08:41:49AM +0200, Lukas Wunner wrote:
> > Commit 05b35e7eb9a1 ("smsc95xx: add phylib support") amended
> > smsc95xx_resume() to call phy_init_hw(). That function waits for the
> > device to runtime resume even though it is placed in the runtime resume
> > path, causing a deadlock.
> >
> > The problem is that phy_init_hw() calls down to smsc95xx_mdiobus_read(),
> > which never uses the _nopm variant of usbnet_read_cmd(). Amend it to
> > autosense that it's called from the runtime resume/suspend path and use
> > the _nopm variant if so.
[...]
> > --- a/drivers/net/usb/smsc95xx.c
> > +++ b/drivers/net/usb/smsc95xx.c
> > @@ -285,11 +285,21 @@ static void smsc95xx_mdio_write_nopm(struct usbnet *dev, int idx, int regval)
> > __smsc95xx_mdio_write(dev, pdata->phydev->mdio.addr, idx, regval, 1);
> > }
> >
> > +static bool smsc95xx_in_pm(struct usbnet *dev)
> > +{
> > +#ifdef CONFIG_PM
> > + return dev->udev->dev.power.runtime_status == RPM_RESUMING ||
> > + dev->udev->dev.power.runtime_status == RPM_SUSPENDING;
> > +#else
> > + return false;
> > +#endif
> > +}
>
> This does not do what you want. You want to know if this function is
> being called in the resume pathway, but all it really tells you is
> whether the function is being called while a resume is in progress (and
> it doesn't even do that very precisely because the code does not use the
> runtime-pm spinlock). The resume could be running in a different
> thread, in which case you most definitely _would_ want to want for it to
> complete.
I'm aware of that. I've explored various approaches and none solved
the problem perfectly. This one seems good enough for all practical
purposes.
One approach I've considered is to use current_work() to determine if
we're called from dev->power.work. But that only works if the runtime
resume/suspend is asynchronous (RPM_ASYNC is set). In this case, the
runtime resume is synchronous and called from a different work item
(hub_event). So the approach is not feasible.
Another approach is to assign a dev_pm_domain to the usb_device, whose
->runtime_resume hook first calls usb_runtime_resume() (so that the
usb_device and usb_interface has status RPM_ACTIVE), *then* calls
phy_init_hw(). Problem is, this only works for runtime resume
and we need a solution for runtime suspend as well. (The device already
has status RPM_SUSPENDING when the dev_pm_domain's ->runtime_suspend hook
is invoked.) So not a feasible approach either.
Fudging the runtime_status in the ->runtime_suspend and ->runtime_resume
hooks via pm_runtime_set_active() / _set_suspended() is rejected by the
runtime PM core.
I've even considered walking up the callstack via _RET_IP_ to determine
if one of the callers is smsc95xx_resume() / _suspend(). But I'm not
sure that's reliable and portable across all arches.
And I don't want to clutter phylib with _nopm variants either.
So the approach I've chosen here, while not perfect, does its job,
is simple and uses very little code. If you've got a better idea,
please let me know.
Thanks,
Lukas
^ permalink raw reply
* Re: [PATCH memcg v4] net: set proper memcg for net_init hooks allocations
From: Shakeel Butt @ 2022-04-27 15:06 UTC (permalink / raw)
To: Michal Koutný
Cc: Vasily Averin, Vlastimil Babka, Roman Gushchin, kernel,
Florian Westphal, LKML, Michal Hocko, Cgroups, netdev,
David S. Miller, Jakub Kicinski, Paolo Abeni
In-Reply-To: <20220427122232.GA9823@blackbody.suse.cz>
On Wed, Apr 27, 2022 at 5:22 AM Michal Koutný <mkoutny@suse.com> wrote:
>
> On Tue, Apr 26, 2022 at 10:23:32PM -0700, Shakeel Butt <shakeelb@google.com> wrote:
> > [...]
> > >
> > > +static inline struct mem_cgroup *get_mem_cgroup_from_obj(void *p)
> > > +{
> > > + struct mem_cgroup *memcg;
> > > +
> >
> > Do we need memcg_kmem_enabled() check here or maybe
> > mem_cgroup_from_obj() should be doing memcg_kmem_enabled() instead of
> > mem_cgroup_disabled() as we can have "cgroup.memory=nokmem" boot
> > param.
>
> I reckon such a guard is on the charge side and readers should treat
> NULL and root_mem_group equally. Or is there a case when these two are
> different?
>
> (I can see it's different semantics when stored in current->active_memcg
> (and active_memcg() getter) but for such "outer" callers like here it
> seems equal.)
I was more thinking about possible shortcut optimization and unrelated
to this patch.
Vasily, can you please add documentation for get_mem_cgroup_from_obj()
similar to get_mem_cgroup_from_mm()? Also for mem_cgroup_or_root().
Please note that root_mem_cgroup can be NULL during early boot.
^ permalink raw reply
* [PATCH] net: mvpp2: add delay at the end of .mac_prepare
From: Baruch Siach @ 2022-04-27 15:05 UTC (permalink / raw)
To: Marcin Wojtas, Russell King; +Cc: netdev, Baruch Siach
From: Baruch Siach <baruch.siach@siklu.com>
Without this delay PHY mode switch from XLG to SGMII fails in a weird
way. Rx side works. However, Tx appears to work as far as the MAC is
concerned, but packets don't show up on the wire.
Tested with Marvell 10G 88X3310 PHY.
Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
---
Not sure this is the right fix. Let me know if you have any better
suggestion for me to test.
The same issue and fix reproduce with both v5.18-rc4 and v5.10.110.
---
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 1a835b48791b..8823efe396b1 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -6432,6 +6432,8 @@ static int mvpp2_mac_prepare(struct phylink_config *config, unsigned int mode,
}
}
+ mdelay(10);
+
return 0;
}
--
2.35.1
^ permalink raw reply related
* Re: [PATCH bpf-next 2/4] libbpf: Add helpers for pinning bpf prog through bpf object skeleton
From: Yafang Shao @ 2022-04-27 14:45 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, netdev, bpf
In-Reply-To: <CAEf4BzbPDhYw6DL6OySyQY1CgBCp0=RUO1FSc8CGYraJx6NMCQ@mail.gmail.com>
On Wed, Apr 27, 2022 at 7:16 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Apr 26, 2022 at 8:59 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > On Mon, Apr 25, 2022 at 9:57 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > >
> > > On 4/23/22 4:00 PM, Yafang Shao wrote:
> > > > Currently there're helpers for allowing to open/load/attach BPF object
> > > > through BPF object skeleton. Let's also add helpers for pinning through
> > > > BPF object skeleton. It could simplify BPF userspace code which wants to
> > > > pin the progs into bpffs.
> > >
> > > Please elaborate some more on your use case/rationale for the commit message,
> > > do you have orchestration code that will rely on these specifically?
> > >
> >
> > We have a bpf manager on our production environment to maintain the
> > bpf programs, some of which need to be pinned in bpffs, for example
> > tracing bpf programs, perf_event programs and other bpf hooks added by
> > ourselves for performance tuning. These bpf programs don't need a
> > user agent, while they really work like a kernel module, that is why
> > we pin them. For these kinds of bpf programs, the bpf manager can help
> > to simplify the development and deployment. Take the improvement on
> > development for example, the user doesn't need to write userspace
> > code while he focuses on the kernel side only, and then bpf manager
> > will do all the other things. Below is a simple example,
> > Step1, gen the skeleton for the user provided bpf object file,
> > $ bpftool gen skeleton test.bpf.o > simple.skel.h
> > Step2, Compile the bpf object file into a runnable binary
> > #include "simple.skel.h"
> >
> > #define SIMPLE_BPF_PIN(name, path) \
> > ({ \
> > struct name##_bpf *obj; \
> > int err = 0; \
> > \
> > obj = name##_bpf__open(); \
> > if (!obj) { \
> > err = -errno; \
> > goto cleanup; \
> > } \
> > \
> > err = name##_bpf__load(obj); \
> > if (err) \
> > goto cleanup; \
> > \
> > err = name##_bpf__attach(obj); \
> > if (err) \
> > goto cleanup; \
> > \
> > err = name##_bpf__pin_prog(obj, path); \
> > if (err) \
> > goto cleanup; \
> > \
> > goto end; \
> > \
> > cleanup: \
> > name##_bpf__destroy(obj); \
> > end: \
> > err; \
> > })
> >
> > SIMPLE_BPF_PIN(test, "/sys/fs/bpf");
> >
> > As the userspace code of FD-based bpf objects are all
> > the same, so we can abstract them as above. The pathset means to add
> > the non-exist "name##_bpf__pin_prog(obj, path)" for it.
> >
>
> Your BPF manager is user-space code that you control, right? I'm not
> sure how skeleton is helpful here given your BPF manager is generic
> and doesn't work with any specific skeleton, if I understand the idea.
> But let's assume that you use skeleton to also embed BPF ELF bytes and
> pass them to your manager for "activation". Once you open and load
> bpf_object, your BPF manager can generically iterate all BPF programs
> using bpf_object_for_each_program(), attempt to attach them with
> bpf_program__attach() (see how bpf_object__attach_skeleton is handling
> non-auto-attachable programs) and immediately pin the link (no need to
> even store it, you can destroy it after pinning immediately). All this
> is using generic libbpf APIs and requires no code generation.
Many thanks for the detailed explanation. Your suggestion can also
work, but with the skeleton we can also generate a binary which can
run independently. (Technically speaking, the binary is the same as
'./bpf_install target.bpf.o').
> But keep
> in mind that not all struct bpf_link in libbpf are pinnable (not all
> links have FD-based BPF link in kernel associated with them), so
> you'll have to deal with that somehow (and what you didn't do in this
> patch for libbpf implementation).
>
Right, I have found it. If I understand it correctly, only the link
types defined in enum bpf_link_type (which is in
include/uapi/linux/bpf.h) are pinnable, right?
BTW, is it possible to support pinning all struct bpf_link in libbpf ?
--
Regards
Yafang
^ permalink raw reply
* Re: [PATCH RFC 4/5] net/tls: Add support for PF_TLSH (a TLS handshake listener)
From: Chuck Lever III @ 2022-04-27 14:42 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, Linux NFS Mailing List, linux-nvme@lists.infradead.org,
linux-cifs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
ak@tempesta-tech.com, borisp@nvidia.com, simo@redhat.com
In-Reply-To: <20220426164712.068e365c@kernel.org>
> On Apr 26, 2022, at 7:47 PM, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 26 Apr 2022 15:58:29 +0000 Chuck Lever III wrote:
>>> On Apr 26, 2022, at 10:55 AM, Jakub Kicinski <kuba@kernel.org> wrote:
>>>> The RPC-with-TLS standard allows unencrypted RPC traffic on the connection
>>>> before sending ClientHello. I think we'd like to stick with creating the
>>>> socket in the kernel, for this reason and for the reasons Hannes mentions
>>>> in his reply.
>>>
>>> Umpf, I presume that's reviewed by security people in IETF so I guess
>>> it's done right this time (tm).
>>
>>> Your wording seems careful not to imply that you actually need that,
>>> tho. Am I over-interpreting?
>>
>> RPC-with-TLS requires one RPC as a "starttls" token. That could be
>> done in user space as part of the handshake, but it is currently
>> done in the kernel to enable the user agent to be shared with other
>> kernel consumers of TLS. Keep in mind that we already have two
>> real consumers: NVMe and RPC-with-TLS; and possibly QUIC.
>>
>> You asserted earlier that creating sockets in user space "scales
>> better" but did not provide any data. Can we see some? How well
>> does it need to scale for storage protocols that use long-lived
>> connections?
>
> I meant scale with the number of possible crypto protocols,
> I mentioned three there.
I'm looking at previous emails. The "three crypto protocols"
don't stand out to me. Which ones?
The prototype has a "handshake type" option that enables the kernel
to request handshakes for different transport layer security
protocols. Is that the kind of scalability you mean?
For TLS, we expect to have at least:
- ClientHello
- X509
- PSK
- ServerHello
- Re-key
It should be straightforward to add the ability to service
other handshake types.
>> Also, why has no-one mentioned the NBD on TLS implementation to
>> us before? I will try to review that code soon.
>
> Oops, maybe that thing had never seen the light of a public mailing
> list then :S Dave Watson was working on it at Facebook, but he since
> moved to greener pastures.
>
>>> This set does not even have selftests.
>>
>> I can include unit tests with the prototype. Someone needs to
>> educate me on what is the preferred unit test paradigm for this
>> type of subsystem. Examples in the current kernel code base would
>> help too.
>
> Whatever level of testing makes you as an engineer comfortable
> with saying "this test suite is sufficient"? ;)
>
> For TLS we have tools/testing/selftests/net/tls.c - it's hardly
> an example of excellence but, you know, it catches bugs here and
> there.
My question wasn't clear, sorry. I meant, what framework is
appropriate to use for unit tests in this area?
>>> Plus there are more protocols being actively worked on (QUIC, PSP etc.)
>>> Having per ULP special sauce to invoke a user space helper is not the
>>> paradigm we chose, and the time as inopportune as ever to change that.
>>
>> When we started discussing TLS handshake requirements with some
>> community members several years ago, creating the socket in
>> kernel and passing it up to a user agent was the suggested design.
>> Has that recommendation changed since then?
>
> Hm, do you remember who you discussed it with? Would be good
> to loop those folks in.
Yes, I remember. Trond Myklebust discussed this with Dave Miller
during a hallway conversation at a conference (probably Plumbers)
in 2018 or 2019.
Trond is Cc'd on this thread via linux-nfs@ and Dave is Cc'd via
netdev@.
I also traded email with Boris Pismenny about this a year ago,
and if memory serves he also recommended passing an existing
socket up to user space. He is Cc'd on this directly.
> I wasn't involved at the beginning of the
> TLS work, I know second hand that HW offload and nbd were involved
> and that the design went thru some serious re-architecting along
> the way. In the beginning there was a separate socket for control
> records, and that was nacked.
>
> But also (and perhaps most importantly) I'm not really objecting
> to creating the socket in the kernel. I'm primarily objecting to
> a second type of a special TLS socket which has TLS semantics.
I don't understand your objection. Can you clarify?
AF_TLSH is a listen-only socket. It's just a rendezvous point
for passing a kernel socket up to user space. It doesn't have
any particular "TLS semantics". It's the user space agent
listening on that endpoint that implements particular handshake
behaviors.
In fact, if the name AF_TLSH gives you hives, that can be made
more generic. However, that makes it harder for the kernel to
figure out which listening endpoint handles handshake requests.
>> I'd prefer an in-kernel handshake implementation over a user
>> space one (even one that is sharable amongst transports and ULPs
>> as my proposal is intended to be). However, so far we've been told
>> that an in-kernel handshake implementation is a non-starter.
>>
>> But in the abstract, we agree that having a single TLS handshake
>> mechanism for kernel consumers is preferable.
>
> For some definition of "we" which doesn't not include me?
The double negative made me blink a couple of times.
I'm working with folks from the Linux NFS community, the
Linux block community, and the Linux SMB community. We
would be happy to include you in our effort, if you would
like to be more involved.
--
Chuck Lever
^ permalink raw reply
* Re: [PATCH net-next v1 1/4] net: phy: broadcom: Add PTP support for some Broadcom PHYs.
From: Richard Cochran @ 2022-04-27 14:40 UTC (permalink / raw)
To: Jonathan Lemon
Cc: f.fainelli, bcm-kernel-feedback-list, andrew, hkallweit1, linux,
netdev, kernel-team
In-Reply-To: <20220424022356.587949-2-jonathan.lemon@gmail.com>
On Sat, Apr 23, 2022 at 07:23:53PM -0700, Jonathan Lemon wrote:
> +static int bcm_ptp_adjtime_locked(struct bcm_ptp_private *priv,
> + s64 delta_ns)
> +{
> + struct timespec64 ts;
> + int err;
> +
> + err = bcm_ptp_gettime_locked(priv, &ts, NULL);
> + if (!err) {
> + timespec64_add_ns(&ts, delta_ns);
When delta_ns is negative, this takes a long time to complete.
> + err = bcm_ptp_settime_locked(priv, &ts);
> + }
> + return err;
> +}
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH nf-next v2 1/1] netfilter: conntrack: skip verification of zero UDP checksum
From: Pablo Neira Ayuso @ 2022-04-27 14:36 UTC (permalink / raw)
To: Kevin Mitchell
Cc: gal, Jozsef Kadlecsik, Florian Westphal, David S. Miller,
Jakub Kicinski, Paolo Abeni, Hideaki YOSHIFUJI, David Ahern,
netfilter-devel, coreteam, netdev, linux-kernel
In-Reply-To: <20220408043341.416219-1-kevmitch@arista.com>
On Thu, Apr 07, 2022 at 09:33:40PM -0700, Kevin Mitchell wrote:
> The checksum is optional for UDP packets in IPv4. However nf_reject
> would previously require a valid checksum to elicit a response such as
> ICMP_DEST_UNREACH.
>
> Add some logic to nf_reject_verify_csum to determine if a UDP packet has
> a zero checksum and should therefore not be verified. Explicitly require
> a valid checksum for IPv6 consistent RFC 2460 and with the non-netfilter
> stack (see udp6_csum_zero_error).
>
> Signed-off-by: Kevin Mitchell <kevmitch@arista.com>
> ---
> include/net/netfilter/nf_reject.h | 27 +++++++++++++++++++++++----
> net/ipv4/netfilter/nf_reject_ipv4.c | 10 +++++++---
> net/ipv6/netfilter/nf_reject_ipv6.c | 4 ++--
> 3 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/netfilter/nf_reject.h b/include/net/netfilter/nf_reject.h
> index 9051c3a0c8e7..f248c1ff8b22 100644
> --- a/include/net/netfilter/nf_reject.h
> +++ b/include/net/netfilter/nf_reject.h
> @@ -5,12 +5,34 @@
> #include <linux/types.h>
> #include <uapi/linux/in.h>
>
> -static inline bool nf_reject_verify_csum(__u8 proto)
> +static inline bool nf_reject_verify_csum(struct sk_buff *skb, int dataoff,
> + __u8 proto)
> {
> /* Skip protocols that don't use 16-bit one's complement checksum
> * of the entire payload.
> */
> switch (proto) {
> + /* Protocols with optional checksums. */
> + case IPPROTO_UDP: {
> + const struct udphdr *udp_hdr;
> + struct udphdr _udp_hdr;
> +
> + /* Checksum is required in IPv6
> + * see RFC 2460 section 8.1
> + */
Right, but follow up work say otherwise?
https://www.rfc-editor.org/rfc/rfc6935
https://www.rfc-editor.org/rfc/rfc6936
Moreover, conntrack and NAT already allow for UDP zero checksum in IPv6.
I'm inclined to stick to the existing behaviour for consistency, ie.
allow for zero checksum in IPv6 UDP.
^ permalink raw reply
* Re: [PATCH iproute2-next v3 0/2] f_flower: match on the number of vlan tags
From: Boris Sukholitko @ 2022-04-27 14:32 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, David Ahern, Ilya Lifshits, Jamal Hadi Salim
In-Reply-To: <20220426081142.71d58c1b@hermes.local>
[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]
Hi Stephen,
On Tue, Apr 26, 2022 at 08:11:42AM -0700, Stephen Hemminger wrote:
> On Tue, 26 Apr 2022 12:14:15 +0300
> Boris Sukholitko <boris.sukholitko@broadcom.com> wrote:
>
> > Hi,
> >
> > Our customers in the fiber telecom world have network configurations
> > where they would like to control their traffic according to the number
> > of tags appearing in the packet.
> >
> > For example, TR247 GPON conformance test suite specification mostly
> > talks about untagged, single, double tagged packets and gives lax
> > guidelines on the vlan protocol vs. number of vlan tags.
> >
> > This is different from the common IT networks where 802.1Q and 802.1ad
> > protocols are usually describe single and double tagged packet. GPON
> > configurations that we work with have arbitrary mix the above protocols
> > and number of vlan tags in the packet.
> >
> > The following patch series implement number of vlans flower filter. They
> > add num_of_vlans flower filter as an alternative to vlan ethtype protocol
> > matching. The end result is that the following command becomes possible:
> >
> > tc filter add dev eth1 ingress flower \
> > num_of_vlans 1 vlan_prio 5 action drop
> >
> > Also, from our logs, we have redirect rules such that:
> >
> > tc filter add dev $GPON ingress flower num_of_vlans $N \
> > action mirred egress redirect dev $DEV
> >
> > where N can range from 0 to 3 and $DEV is the function of $N.
> >
> > Also there are rules setting skb mark based on the number of vlans:
> >
> > tc filter add dev $GPON ingress flower num_of_vlans $N vlan_prio \
> > $P action skbedit mark $M
> >
> > Thanks,
> > Boris.
> >
> > - v3: rebased to the latest iproute2-next
> > - v2: add missing f_flower subject prefix
> >
> > Boris Sukholitko (2):
> > f_flower: Add num of vlans parameter
> > f_flower: Check args with num_of_vlans
> >
> > tc/f_flower.c | 57 ++++++++++++++++++++++++++++++++++++---------------
> > 1 file changed, 41 insertions(+), 16 deletions(-)
>
> Can you do this with BPF? instead of kernel change?
You may have missed my reply to this question at:
https://lore.kernel.org/netdev/20220412104514.GB27480@noodle/
There is also Jamal's reply further at the thread:
https://lore.kernel.org/netdev/b2c83f63-a2e9-92a2-f262-3aae3491dfc3@mojatatu.com/
Thanks,
Boris.
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]
^ permalink raw reply
* RE: [PATCH net 1/2] ptp: ptp_clockmatrix: Add PTP_CLK_REQ_EXTTS support
From: Min Li @ 2022-04-27 14:32 UTC (permalink / raw)
To: Jakub Kicinski
Cc: richardcochran@gmail.com, lee.jones@linaro.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org
In-Reply-To: <20220426142150.47b057a3@kernel.org>
> On Tue, 26 Apr 2022 15:32:53 -0400 Min Li wrote:
> > Use TOD_READ_SECONDARY for extts to keep TOD_READ_PRIMARY for
> gettime
> > and settime exclusively
>
> Does not build on 32 bit.
>
Hi Jakub
I compiled with arm-linux-gnueabi-
By 32 bit, did you mean x86?
Min
^ permalink raw reply
* Re: [PATCH net-next 00/11] mlxsw: extend line card model by devices and info
From: Jakub Kicinski @ 2022-04-27 14:14 UTC (permalink / raw)
To: Jiri Pirko
Cc: Ido Schimmel, Ido Schimmel, netdev, davem, pabeni, jiri, petrm,
dsahern, andrew, mlxsw
In-Reply-To: <YmjyRgYYRU/ZaF9X@nanopsycho>
On Wed, 27 Apr 2022 09:35:34 +0200 Jiri Pirko wrote:
> >> The relationship-by-name sounds a bit fragile to me. The names of
> >> components are up to the individual drivers.
> >
> >I asked you how the automation will operate. You must answer questions
> >if you want to have a discussion. Automation is the relevant part.
>
> Automation, not sure. It would probably just see type of gearbox and
> flash it. Not sure I understand the question, perhaps you could explain?
> Plus, the possibility is to auto-flash the GB from driver directly.
>
>
> >You're not designing an interface for SDK users but for end users.
>
> Sure, that is the aim of this API. Human end user. That is why I wanted
> the user to see the relationships between devlink dev, line cards and
> the gearboxes on them. If you want to limit the visibility, sure, just
> tell me how.
Okay, we have completely different views on what the goals should be.
Perhaps that explains the differences in the design.
Of the three API levels (SDK, automation, human) I think automation
is the only one that's interesting to us in Linux. SDK interfaces are
necessarily too low level as they expose too much of internal details
to standardize. Humans are good with dealing with uncertainty and
diverse so there's no a good benchmark.
The benchmark for automation is - can a machine use this API across
different vendors to reliably achieve its goals. For FW info/flashing
the goal is keeping the FW versions up to date. This is documented:
https://www.kernel.org/doc/html/latest/networking/devlink/devlink-flash.html#firmware-version-management
What would the pseudo code look like with "line cards" in the picture?
Apply RFC1925 truth 12.
> >> There is no new command for that, only one nested attribute which
> >> carries the device list added to the existing command. They are no new
> >> objects, they are just few nested values.
> >
> >DEVLINK_CMD_LINECARD_INFO_GET
>
> Okay, that is not only to expose devices. That is also to expose info
> about linecards, like HW revision, INI version etc. Where else to put
> it? I can perhaps embed it into devlink dev info, but I thought separate
> command would be more suitable. object cmd, object info cmd. It is
> more clear I believe.
> >> If so, how does the user know if/when to flash it?
> >> If not, where would you list it if devices nest is not the correct place?
> >
> >Let me mock up what I had in mind for you since it did not come thru
> >in the explanation:
> >
> >$ devlink dev info show pci/0000:01:00.0
> > versions:
> > fixed:
> > hw.revision 0
> > lc2.hw.revision a
> > lc8.hw.revision b
> > running:
> > ini.version 4
> > lc2.gearbox 1.1.3
> > lc8.gearbox 1.2.3
>
> Would be rather:
>
> lc2.gearbox0 1.1.3
> lc2.gearbox1 1.2.4
I thought you said your gearboxes all the the same FW?
Theoretically, yes. Theoretically, I can also have nested "line cards".
> lc8.gearbox0 1.2.3
>
> Okay, I see. So instead of having clear api with relationships and
> clear human+machine readability we have squahed indexes into strings.
> I fail to see the benefit, other than no-api-extension :/ On contrary.
Show me the real life use for all the "clear api with relationships"
and I'll shut up.
I would not take falling back to physical (HW) hierarchy for the API
design as a point of pride. Seems lazy if I'm completely honest.
Someone else's HW may have a different hierarchy, and you're just
forcing the automation engineer iterate over irrelevant structures
("devices").
My hunch is that automation will not want to deal with line cards
separately, and flash the entire devices in one go to a tested and
verified bundle blob provided by the vendor. If they do want to poke
at line cards - the information is still there in what I described.
> >$ devlink lc show pci/0000:01:00.0 lc 8
> >pci/0000:01:00.0:
> > lc 8 state active type 16x100G
> > supported_types:
> > 16x100G
> > versions:
> > lc8.hw.revision (a)
> > lc8.gearbox (1.2.3)
> >
> >Where the data in the brackets is optionally fetched thru the existing
> >"dev info" API, but rendered together by the user space.
>
> Quite odd. I find it questionable to say at least to mix multiple
> command netlink outputs into one output.
Really? So we're going to be designing kernel APIs so that each message
contains complete information and can't contain references now?
> The processing of it would be a small nightmare considering the way
> how the netlink message processing works in iproute2 :/
^ permalink raw reply
* Re: [PATCH] memcg: accounting for objects allocated for new netdevice
From: Michal Koutný @ 2022-04-27 14:01 UTC (permalink / raw)
To: Vasily Averin
Cc: Roman Gushchin, Vlastimil Babka, Shakeel Butt, kernel,
Florian Westphal, linux-kernel, Michal Hocko, cgroups, netdev,
David S. Miller, Jakub Kicinski, Paolo Abeni, Greg Kroah-Hartman,
Tejun Heo, Luis Chamberlain, Kees Cook, Iurii Zaikin,
linux-fsdevel
In-Reply-To: <7e867cb0-89d6-402c-33d2-9b9ba0ba1523@openvz.org>
Hello Vasily.
On Wed, Apr 27, 2022 at 01:37:50PM +0300, Vasily Averin <vvs@openvz.org> wrote:
> diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
> index cfa79715fc1a..2881aeeaa880 100644
> --- a/fs/kernfs/mount.c
> +++ b/fs/kernfs/mount.c
> @@ -391,7 +391,7 @@ void __init kernfs_init(void)
> {
> kernfs_node_cache = kmem_cache_create("kernfs_node_cache",
> sizeof(struct kernfs_node),
> - 0, SLAB_PANIC, NULL);
> + 0, SLAB_PANIC | SLAB_ACCOUNT, NULL);
kernfs accounting you say?
kernfs backs up also cgroups, so the parent-child accounting comes to my
mind.
See the temporary switch to parent memcg in mem_cgroup_css_alloc().
(I mean this makes some sense but I'd suggest unlumping the kernfs into
a separate path for possible discussion and its not-only-netdevice
effects.)
Thanks,
Michal
^ permalink raw reply
* Re: [PATCH net] usbnet: smsc95xx: Fix deadlock on runtime resume
From: Alan Stern @ 2022-04-27 14:00 UTC (permalink / raw)
To: Lukas Wunner
Cc: Steve Glendinning, UNGLinuxDriver, Oliver Neukum, David S. Miller,
Jakub Kicinski, Paolo Abeni, netdev, linux-usb, Andre Edich,
Oleksij Rempel, Martyn Welch, Gabriel Hojda, Christoph Fritz,
Lino Sanfilippo, Philipp Rosenberger, Heiner Kallweit,
Andrew Lunn, Russell King
In-Reply-To: <6710d8c18ff54139cdc538763ba544187c5a0cee.1651041411.git.lukas@wunner.de>
On Wed, Apr 27, 2022 at 08:41:49AM +0200, Lukas Wunner wrote:
> Commit 05b35e7eb9a1 ("smsc95xx: add phylib support") amended
> smsc95xx_resume() to call phy_init_hw(). That function waits for the
> device to runtime resume even though it is placed in the runtime resume
> path, causing a deadlock.
>
> The problem is that phy_init_hw() calls down to smsc95xx_mdiobus_read(),
> which never uses the _nopm variant of usbnet_read_cmd(). Amend it to
> autosense that it's called from the runtime resume/suspend path and use
> the _nopm variant if so.
...
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index 4ef61f6b85df..82b8feaa5162 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -285,11 +285,21 @@ static void smsc95xx_mdio_write_nopm(struct usbnet *dev, int idx, int regval)
> __smsc95xx_mdio_write(dev, pdata->phydev->mdio.addr, idx, regval, 1);
> }
>
> +static bool smsc95xx_in_pm(struct usbnet *dev)
> +{
> +#ifdef CONFIG_PM
> + return dev->udev->dev.power.runtime_status == RPM_RESUMING ||
> + dev->udev->dev.power.runtime_status == RPM_SUSPENDING;
> +#else
> + return false;
> +#endif
> +}
This does not do what you want. You want to know if this function is
being called in the resume pathway, but all it really tells you is
whether the function is being called while a resume is in progress (and
it doesn't even do that very precisely because the code does not use the
runtime-pm spinlock). The resume could be running in a different
thread, in which case you most definitely _would_ want to want for it to
complete.
Alan Stern
^ permalink raw reply
* Re: [PATCH net] netfilter: conn: fix udp offload timeout sysctl
From: Pablo Neira Ayuso @ 2022-04-27 13:49 UTC (permalink / raw)
To: Volodymyr Mytnyk
Cc: netdev, Taras Chornyi, Serhiy Pshyk, Oz Shlomo, Jozsef Kadlecsik,
Florian Westphal, David S. Miller, Jakub Kicinski, Paolo Abeni,
Paul Blakey, netfilter-devel, coreteam, linux-kernel
In-Reply-To: <1651057740-12713-1-git-send-email-volodymyr.mytnyk@plvision.eu>
On Wed, Apr 27, 2022 at 02:09:00PM +0300, Volodymyr Mytnyk wrote:
> `nf_flowtable_udp_timeout` sysctl option is available only
> if CONFIG_NFT_FLOW_OFFLOAD enabled. But infra for this flow
> offload UDP timeout was added under CONFIG_NF_FLOW_TABLE
> config option. So, if you have CONFIG_NFT_FLOW_OFFLOAD
> disabled and CONFIG_NF_FLOW_TABLE enabled, the
> `nf_flowtable_udp_timeout` is not present in sysfs.
> Please note, that TCP flow offload timeout sysctl option
> is present even CONFIG_NFT_FLOW_OFFLOAD is disabled.
>
> I suppose it was a typo in commit that adds UDP flow offload
> timeout and CONFIG_NF_FLOW_TABLE should be used instead.
Applied, thanks
^ permalink raw reply
* Re: [PATCH ethtool-next v2 2/2] ethtool: add support to get/set tx push by ethtool -G/g
From: sundeep subbaraya @ 2022-04-27 13:46 UTC (permalink / raw)
To: Guangbin Huang
Cc: mkubecek, David Miller, Jakub Kicinski, netdev, lipeng321,
Subbaraya Sundeep
In-Reply-To: <20220421084646.15458-3-huangguangbin2@huawei.com>
Hi Guangbin,
On Thu, Apr 21, 2022 at 3:47 PM Guangbin Huang
<huangguangbin2@huawei.com> wrote:
>
> From: Jie Wang <wangjie125@huawei.com>
>
> Currently tx push is a standard feature for NICs such as Mellanox, HNS3.
> But there is no command to set or get this feature.
>
> So this patch adds support for "ethtool -G <dev> tx-push on|off" and
> "ethtool -g <dev>" to set/get tx push mode.
>
> Signed-off-by: Jie Wang <wangjie125@huawei.com>
> ---
> ethtool.8.in | 4 ++++
> ethtool.c | 1 +
> netlink/rings.c | 7 +++++++
> 3 files changed, 12 insertions(+)
>
> diff --git a/ethtool.8.in b/ethtool.8.in
> index 12940e1..a87f31f 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -199,6 +199,7 @@ ethtool \- query or control network driver and hardware settings
> .BN rx\-jumbo
> .BN tx
> .BN rx\-buf\-len
> +.BN tx\-push
> .HP
> .B ethtool \-i|\-\-driver
> .I devname
> @@ -573,6 +574,9 @@ Changes the number of ring entries for the Tx ring.
> .TP
> .BI rx\-buf\-len \ N
> Changes the size of a buffer in the Rx ring.
> +.TP
> +.BI tx\-push \ on|off
> +Specifies whether TX push should be enabled.
> .RE
> .TP
> .B \-i \-\-driver
> diff --git a/ethtool.c b/ethtool.c
> index 4f5c234..4d2a475 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -5733,6 +5733,7 @@ static const struct option args[] = {
> " [ rx-jumbo N ]\n"
> " [ tx N ]\n"
> " [ rx-buf-len N]\n"
> + " [ tx-push on|off]\n"
> },
> {
> .opts = "-k|--show-features|--show-offload",
> diff --git a/netlink/rings.c b/netlink/rings.c
> index 119178e..a53eed5 100644
> --- a/netlink/rings.c
> +++ b/netlink/rings.c
> @@ -47,6 +47,7 @@ int rings_reply_cb(const struct nlmsghdr *nlhdr, void *data)
> show_u32(tb[ETHTOOL_A_RINGS_RX_JUMBO], "RX Jumbo:\t");
> show_u32(tb[ETHTOOL_A_RINGS_TX], "TX:\t\t");
> show_u32(tb[ETHTOOL_A_RINGS_RX_BUF_LEN], "RX Buf Len:\t\t");
> + show_bool("tx-push", "TX Push:\t%s\n", tb[ETHTOOL_A_RINGS_TX_PUSH]);
>
> return MNL_CB_OK;
> }
> @@ -105,6 +106,12 @@ static const struct param_parser sring_params[] = {
> .handler = nl_parse_direct_u32,
> .min_argc = 1,
> },
> + {
> + .arg = "tx-push",
> + .type = ETHTOOL_A_RINGS_TX_PUSH,
> + .handler = nl_parse_u8bool,
> + .min_argc = 0,
Why min_argc is 0 ? Thanks for syncing kernel headers. I have patch for adding
cqe-size command and will send after these are merged.
Sundeep
> + },
> {}
> };
>
> --
> 2.33.0
>
^ permalink raw reply
* Re: [PATCH v2 3/5] hv_sock: Add validation for untrusted Hyper-V values
From: Stefano Garzarella @ 2022-04-27 13:38 UTC (permalink / raw)
To: Andrea Parri (Microsoft)
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Dexuan Cui, Michael Kelley, David Miller, Jakub Kicinski,
Paolo Abeni, linux-hyperv, virtualization, netdev, linux-kernel
In-Reply-To: <20220427131225.3785-4-parri.andrea@gmail.com>
On Wed, Apr 27, 2022 at 03:12:23PM +0200, Andrea Parri (Microsoft) wrote:
>For additional robustness in the face of Hyper-V errors or malicious
>behavior, validate all values that originate from packets that Hyper-V
>has sent to the guest in the host-to-guest ring buffer. Ensure that
>invalid values cannot cause data being copied out of the bounds of the
>source buffer in hvs_stream_dequeue().
>
>Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
>Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>---
> include/linux/hyperv.h | 5 +++++
> net/vmw_vsock/hyperv_transport.c | 10 ++++++++--
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
>diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
>index fe2e0179ed51e..55478a6810b60 100644
>--- a/include/linux/hyperv.h
>+++ b/include/linux/hyperv.h
>@@ -1663,6 +1663,11 @@ static inline u32 hv_pkt_datalen(const struct vmpacket_descriptor *desc)
> return (desc->len8 << 3) - (desc->offset8 << 3);
> }
>
>+/* Get packet length associated with descriptor */
>+static inline u32 hv_pkt_len(const struct vmpacket_descriptor *desc)
>+{
>+ return desc->len8 << 3;
>+}
>
> struct vmpacket_descriptor *
> hv_pkt_iter_first_raw(struct vmbus_channel *channel);
>diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
>index 8c37d07017fc4..fd98229e3db30 100644
>--- a/net/vmw_vsock/hyperv_transport.c
>+++ b/net/vmw_vsock/hyperv_transport.c
>@@ -577,12 +577,18 @@ static bool hvs_dgram_allow(u32 cid, u32 port)
> static int hvs_update_recv_data(struct hvsock *hvs)
> {
> struct hvs_recv_buf *recv_buf;
>- u32 payload_len;
>+ u32 pkt_len, payload_len;
>+
>+ pkt_len = hv_pkt_len(hvs->recv_desc);
>+
>+ if (pkt_len < HVS_HEADER_LEN)
>+ return -EIO;
>
> recv_buf = (struct hvs_recv_buf *)(hvs->recv_desc + 1);
> payload_len = recv_buf->hdr.data_size;
>
>- if (payload_len > HVS_MTU_SIZE)
>+ if (payload_len > pkt_len - HVS_HEADER_LEN ||
>+ payload_len > HVS_MTU_SIZE)
> return -EIO;
>
> if (payload_len == 0)
>--
>2.25.1
>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
^ permalink raw reply
* Re: [PATCH v2 2/5] hv_sock: Copy packets sent by Hyper-V out of the ring buffer
From: Stefano Garzarella @ 2022-04-27 13:38 UTC (permalink / raw)
To: Andrea Parri (Microsoft)
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Dexuan Cui, Michael Kelley, David Miller, Jakub Kicinski,
Paolo Abeni, linux-hyperv, virtualization, netdev, linux-kernel
In-Reply-To: <20220427131225.3785-3-parri.andrea@gmail.com>
On Wed, Apr 27, 2022 at 03:12:22PM +0200, Andrea Parri (Microsoft) wrote:
>Pointers to VMbus packets sent by Hyper-V are used by the hv_sock driver
>within the guest VM. Hyper-V can send packets with erroneous values or
>modify packet fields after they are processed by the guest. To defend
>against these scenarios, copy the incoming packet after validating its
>length and offset fields using hv_pkt_iter_{first,next}(). Use
>HVS_PKT_LEN(HVS_MTU_SIZE) to initialize the buffer which holds the
>copies of the incoming packets. In this way, the packet can no longer
>be modified by the host.
>
>Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
>Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>---
> net/vmw_vsock/hyperv_transport.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
>index 943352530936e..8c37d07017fc4 100644
>--- a/net/vmw_vsock/hyperv_transport.c
>+++ b/net/vmw_vsock/hyperv_transport.c
>@@ -78,6 +78,9 @@ struct hvs_send_buf {
> ALIGN((payload_len), 8) + \
> VMBUS_PKT_TRAILER_SIZE)
>
>+/* Upper bound on the size of a VMbus packet for hv_sock */
>+#define HVS_MAX_PKT_SIZE HVS_PKT_LEN(HVS_MTU_SIZE)
>+
> union hvs_service_id {
> guid_t srv_id;
>
>@@ -378,6 +381,8 @@ static void hvs_open_connection(struct vmbus_channel *chan)
> rcvbuf = ALIGN(rcvbuf, HV_HYP_PAGE_SIZE);
> }
>
>+ chan->max_pkt_size = HVS_MAX_PKT_SIZE;
>+
> ret = vmbus_open(chan, sndbuf, rcvbuf, NULL, 0, hvs_channel_cb,
> conn_from_host ? new : sk);
> if (ret != 0) {
>@@ -602,7 +607,7 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk, struct msghdr *msg,
> return -EOPNOTSUPP;
>
> if (need_refill) {
>- hvs->recv_desc = hv_pkt_iter_first_raw(hvs->chan);
>+ hvs->recv_desc = hv_pkt_iter_first(hvs->chan);
> if (!hvs->recv_desc)
> return -ENOBUFS;
> ret = hvs_update_recv_data(hvs);
>@@ -618,7 +623,7 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk, struct msghdr *msg,
>
> hvs->recv_data_len -= to_read;
> if (hvs->recv_data_len == 0) {
>- hvs->recv_desc = hv_pkt_iter_next_raw(hvs->chan, hvs->recv_desc);
>+ hvs->recv_desc = hv_pkt_iter_next(hvs->chan, hvs->recv_desc);
> if (hvs->recv_desc) {
> ret = hvs_update_recv_data(hvs);
> if (ret)
>--
>2.25.1
>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
^ permalink raw reply
* Re: [PATCH net-next v6 02/13] net: wwan: t7xx: Add control DMA interface
From: Sergey Ryazanov @ 2022-04-27 13:17 UTC (permalink / raw)
To: Loic Poulain
Cc: Ricardo Martinez, netdev, linux-wireless, Jakub Kicinski,
David Miller, Johannes Berg, M Chetan Kumar,
Devegowda, Chandrashekar, Intel Corporation, chiranjeevi.rapolu,
Haijun Liu (刘海军), Hanania, Amir,
Andy Shevchenko, Sharma, Dinesh, Lee, Eliot,
Jarvinen, Ilpo Johannes, Veleta, Moises, Bossart, Pierre-louis,
Sethuraman, Muralidharan, Mishra, Soumya Prakash,
Kancharla, Sreehari, Sahu, Madhusmita
In-Reply-To: <CAMZdPi90Joo8+_44ceqS3k8ez08W_AX-eWs42F0ztDN67WR2Pw@mail.gmail.com>
On Wed, Apr 27, 2022 at 3:35 PM Loic Poulain <loic.poulain@linaro.org> wrote:
> On Tue, 26 Apr 2022 at 02:19, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>> On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez
>> <ricardo.martinez@linux.intel.com> wrote:
>>> ...
>>> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
>>> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
>>>
>>> From a WWAN framework perspective:
>>> Reviewed-by: Loic Poulain <loic.poulain@linaro.org>
>>>
>>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>
>> This line with "From a WWAN framework perspective" looks confusing to
>> me. Anyone not familiar with all of the iterations will be in doubt as
>> to whether it belongs only to Loic's review or to both of them.
>>
>> How about to format this block like this:
>>
>>> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
>>> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
>>> Reviewed-by: Loic Poulain <loic.poulain@linaro.org> (WWAN framework)
>>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>
>> or like this:
>>
>>> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
>>> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
>>> Reviewed-by: Loic Poulain <loic.poulain@linaro.org> # WWAN framework
>>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>
>> Parentheses vs. comment sign. I saw people use both of these formats,
>> I just do not know which is better. What do you think?
>
> My initial comment was to highlight that someone else should double
> check the network code, but it wasn't expected to end up in the commit
> message. Maybe simply drop this extra comment?
Yep, this drastically solves the problem with comment format :) I do not mind.
--
Sergey
^ 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