* Re: [PATCH net-next 0/7] s390/qeth: updates 2018-08-09
From: David Miller @ 2018-08-09 21:04 UTC (permalink / raw)
To: jwi; +Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, ubraun
In-Reply-To: <20180809124804.20056-1-jwi@linux.ibm.com>
From: Julian Wiedmann <jwi@linux.ibm.com>
Date: Thu, 9 Aug 2018 14:47:57 +0200
> one more set of patches for net-next. This is all sorts of cleanups
> and more refactoring on the way to using netdev_priv. Please apply.
Looks simple enough, series applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next 0/3] qed*: Enhancements
From: David Miller @ 2018-08-09 21:05 UTC (permalink / raw)
To: manish.chopra; +Cc: netdev, ariel.elior, michal.kalderon
In-Reply-To: <20180809181351.32394-1-manish.chopra@cavium.com>
From: Manish Chopra <manish.chopra@cavium.com>
Date: Thu, 9 Aug 2018 11:13:48 -0700
> This patch series adds following support in drivers -
>
> 1. Egress mqprio offload.
> 2. Add destination IP based flow profile.
> 3. Ingress flower offload (for drop action).
>
> Please consider applying this series to "net-next".
Series applied, thank you.
^ permalink raw reply
* Re: [PATCH lora-next v2 3/8] net: lora: sx1301: convert to passing priv data throughout
From: Ben Whitten @ 2018-08-09 21:06 UTC (permalink / raw)
To: Andreas Färber
Cc: 潘建宏, hasnain.virk, netdev, Xue Liu, shess,
Ben Whitten
In-Reply-To: <3884a134-c916-7b30-1db3-1e2c8f0d9d5b@suse.de>
On Thu, 9 Aug 2018 at 21:43, Andreas Färber <afaerber@suse.de> wrote:
>
> Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> > Instead of passing around the spi device we instead pass around our
> > driver data directly.
> >
> > Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> > ---
> > drivers/net/lora/sx1301.c | 305 +++++++++++++++++++++++-----------------------
> > 1 file changed, 155 insertions(+), 150 deletions(-)
> >
> > diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> > index 3c09f5a..7324001 100644
> > --- a/drivers/net/lora/sx1301.c
> > +++ b/drivers/net/lora/sx1301.c
> > @@ -73,24 +73,26 @@ struct spi_sx1301 {
> > };
> >
> > struct sx1301_priv {
> > + struct device *dev;
> > + struct spi_device *spi;
>
> Obviously this is not a long-term solution, but as interim step it'll
> have to do.
>
> > struct lora_priv lora;
> > struct gpio_desc *rst_gpio;
> > u8 cur_page;
> > struct spi_controller *radio_a_ctrl, *radio_b_ctrl;
> > };
> >
> > -static int sx1301_read_burst(struct spi_device *spi, u8 reg, u8 *val, size_t len)
> > +static int sx1301_read_burst(struct sx1301_priv *priv, u8 reg, u8 *val, size_t len)
> > {
> > u8 addr = reg & 0x7f;
> > - return spi_write_then_read(spi, &addr, 1, val, len);
> > + return spi_write_then_read(priv->spi, &addr, 1, val, len);
> > }
> >
> > -static int sx1301_read(struct spi_device *spi, u8 reg, u8 *val)
> > +static int sx1301_read(struct sx1301_priv *priv, u8 reg, u8 *val)
> > {
> > - return sx1301_read_burst(spi, reg, val, 1);
> > + return sx1301_read_burst(priv, reg, val, 1);
> > }
> >
> > -static int sx1301_write_burst(struct spi_device *spi, u8 reg, const u8 *val, size_t len)
> > +static int sx1301_write_burst(struct sx1301_priv *priv, u8 reg, const u8 *val, size_t len)
> > {
> > u8 addr = reg | BIT(7);
> > struct spi_transfer xfr[2] = {
>
> This hunk did not apply for some reason, I've manually re-applied it.
>
> [...]
> > @@ -654,22 +646,35 @@ static int sx1301_probe(struct spi_device *spi)
> > priv->rst_gpio = rst;
> > priv->cur_page = 0xff;
> >
> > - spi_set_drvdata(spi, netdev);
> > + spi_set_drvdata(spi, priv);
>
> This change seems unnecessary and counter-productive for unregistration.
>
> Otherwise applying.
This is actually pretty critical, as it stands with the two spi masters we use
spi_get_drvdata on the parent device of the controller to recover the priv
struct for regmap.
We may have to include the netdev in the priv data, or do a container_of
dance to recover netdev in unregistration.
That said if we wrap things in devm then really our remove function could
be empty, as we have done with the allocation.
Regards,
Ben
^ permalink raw reply
* Re: [PATCH net-next] net: ipv6_gre: Fix GRO to work on IPv6 over GRE tap
From: David Miller @ 2018-08-09 21:08 UTC (permalink / raw)
To: tariqt; +Cc: netdev, eranbe, mariap
In-Reply-To: <1533717990-25365-1-git-send-email-tariqt@mellanox.com>
From: Tariq Toukan <tariqt@mellanox.com>
Date: Wed, 8 Aug 2018 11:46:30 +0300
> IPv6 GRO over GRE tap is not working while GRO is not set
> over the native interface.
...
> This patch removes the override of the hard_header_len, and
> assigns the calculated value to needed_headroom.
> This way, the comparison in gro_list_prepare is really of
> the mac headers, and if the packets have the same mac headers
> the same_flow will be set to 1.
>
> Performance testing: 45% higher bandwidth.
> Measuring bandwidth of single-stream IPv4 TCP traffic over IPv6
> GRE tap while GRO is not set on the native.
> NIC: ConnectX-4LX
> Before (GRO not working) : 7.2 Gbits/sec
> After (GRO working): 10.5 Gbits/sec
>
> Signed-off-by: Maria Pasechnik <mariap@mellanox.com>
> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH lora-next v2 6/8] net: lora: sx1301: replace version and size magic numbers with defines
From: Andreas Färber @ 2018-08-09 21:11 UTC (permalink / raw)
To: Ben Whitten
Cc: starnight, hasnain.virk, netdev, liuxuenetmail, shess,
Ben Whitten
In-Reply-To: <1533818018-29005-6-git-send-email-ben.whitten@lairdtech.com>
Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> We replace the hard coded numbers for size and version with meaningful
> names.
>
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
> drivers/net/lora/sx1301.c | 21 +++++++++++++--------
> drivers/net/lora/sx1301.h | 18 ++++++++++++++++++
> 2 files changed, 31 insertions(+), 8 deletions(-)
> create mode 100644 drivers/net/lora/sx1301.h
>
> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index 916ee40..8e81179 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
> @@ -21,6 +21,8 @@
> #include <linux/lora/dev.h>
> #include <linux/spi/spi.h>
>
> +#include "sx1301.h"
> +
> #define REG_PAGE_RESET 0
> #define REG_VERSION 1
> #define REG_MCU_PROM_ADDR 9
> @@ -293,7 +295,7 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
> u8 val, rst, select_mux;
> int ret;
>
> - if (fw->size != 8192) {
> + if (fw->size != SX1301_MCU_FW_BYTE) {
I think that should be BYTES, but we can still rename it later.
> dev_err(priv->dev, "Unexpected firmware size\n");
> return -EINVAL;
> }
[...]
> diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
> new file mode 100644
> index 0000000..b37ac56
> --- /dev/null
> +++ b/drivers/net/lora/sx1301.h
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
Header files must use /* ... */ or checkpatch.pl complains.
> +/*
> + * Semtech SX1301 lora concentrator
LoRa
> + *
> + * Copyright (c) 2018 Ben Whitten
Any reason for the multiple whitespaces?
> + */
> +
> +#ifndef _SX1301_
> +#define _SX1301_
> +
> +#define SX1301_CHIP_VERSION 103
> +
> +#define SX1301_MCU_FW_BYTE 8192
> +#define SX1301_MCU_ARB_FW_VERSION 1
> +#define SX1301_MCU_AGC_FW_VERSION 4
> +#define SX1301_MCU_AGC_CAL_FW_VERSION 2
> +
> +#endif
Applied.
Thanks,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply
* Re: [patch net-next] net: sched: fix block->refcnt decrement
From: David Miller @ 2018-08-09 21:12 UTC (permalink / raw)
To: jiri; +Cc: netdev, jhs, xiyou.wangcong, vladbu, mlxsw
In-Reply-To: <20180808120413.7667-1-jiri@resnulli.us>
From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 8 Aug 2018 14:04:13 +0200
> From: Jiri Pirko <jiri@mellanox.com>
>
> Currently the refcnt is never decremented in case the value is not 1.
> Fix it by adding decrement in case the refcnt is not 1.
>
> Reported-by: Vlad Buslov <vladbu@mellanox.com>
> Fixes: f71e0ca4db18 ("net: sched: Avoid implicit chain 0 creation")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Applied.
^ permalink raw reply
* [PATCH] bonding: avoid repeated display of same link status change
From: rama nichanamatlu @ 2018-08-09 21:12 UTC (permalink / raw)
To: netdev
From 9927a1c2a632d9479a80c63b7d9fda59ea8374bc Mon Sep 17 00:00:00 2001
From: Rama Nichanamatlu <rama.nichanamatlu@oracle.com>
Date: Tue, 31 Jul 2018 07:09:52 -0700
Subject: [PATCH] bonding: avoid repeated display of same link status change
When link status change needs to be committed and rtnl lock couldn't be
taken, avoid redisplay of same link status change message.
Signed-off-by: Rama Nichanamatlu <rama.nichanamatlu@oracle.com>
---
drivers/net/bonding/bond_main.c | 6 ++++--
include/net/bonding.h | 1 +
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c
b/drivers/net/bonding/bond_main.c
index 63e3844..3dd1091 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2086,7 +2086,7 @@ static int bond_miimon_inspect(struct bonding *bond)
bond_propose_link_state(slave, BOND_LINK_FAIL);
commit++;
slave->delay = bond->params.downdelay;
- if (slave->delay) {
+ if (slave->delay && !bond->rtnl_needed) {
netdev_info(bond->dev, "link status
down for %sinterface %s, disabling it in %d ms\n",
(BOND_MODE(bond) ==
BOND_MODE_ACTIVEBACKUP) ?
@@ -2126,7 +2126,7 @@ static int bond_miimon_inspect(struct bonding *bond)
commit++;
slave->delay = bond->params.updelay;
- if (slave->delay) {
+ if (slave->delay && !bond->rtnl_needed) {
netdev_info(bond->dev, "link status up
for interface %s, enabling it in %d ms\n",
slave->dev->name,
ignore_updelay ? 0 :
@@ -2300,9 +2300,11 @@ static void bond_mii_monitor(struct work_struct
*work)
if (!rtnl_trylock()) {
delay = 1;
should_notify_peers = false;
+ bond->rtnl_needed = true;
goto re_arm;
}
+ bond->rtnl_needed = false;
bond_for_each_slave(bond, slave, iter) {
bond_commit_link_state(slave,
BOND_SLAVE_NOTIFY_LATER);
}
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 808f1d1..4e76e5d 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -234,6 +234,7 @@ struct bonding {
struct dentry *debug_dir;
#endif /* CONFIG_DEBUG_FS */
struct rtnl_link_stats64 bond_stats;
+ u8 rtnl_needed;
};
#define bond_slave_get_rcu(dev) \
--
1.7.1
^ permalink raw reply related
* Re: [PATCH bpf-next] bpf: enable btf for use in all maps
From: Alexei Starovoitov @ 2018-08-09 21:14 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: ast, netdev, yhs
In-Reply-To: <20180809194220.17484-1-daniel@iogearbox.net>
On Thu, Aug 09, 2018 at 09:42:20PM +0200, Daniel Borkmann wrote:
> Commit a26ca7c982cb ("bpf: btf: Add pretty print support to
> the basic arraymap") enabled support for BTF and dumping via
> BPF fs for arraymap. However, both can be decoupled from each
> other such that all BPF maps can be supported for attaching
> BTF key/value information, while not all maps necessarily
> need to dump via map_seq_show_elem() callback.
>
> The check in array_map_check_btf() can be generalized as
> ultimatively the key and value size is the only contraint
> that needs to match for the map. The fact that the key needs
> to be of type int is optional; it could be any data type as
> long as it matches the 4 byte key size, just like hash table
> key or others could be of any data type as well.
>
> Minimal example of a hash table dump which then works out
> of the box for bpftool:
>
> # bpftool map dump id 19
> [{
> "key": {
> "": {
> "vip": 0,
> "vipv6": []
> },
> "port": 0,
> "family": 0,
> "proto": 0
> },
> "value": {
> "flags": 0,
> "vip_num": 0
> }
> }
> ]
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Yonghong Song <yhs@fb.com>
> ---
> include/linux/bpf.h | 4 +---
> kernel/bpf/arraymap.c | 27 ---------------------------
> kernel/bpf/inode.c | 3 ++-
> kernel/bpf/syscall.c | 24 ++++++++++++++++++++----
> 4 files changed, 23 insertions(+), 35 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index cd8790d..eb76e8e 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -48,8 +48,6 @@ struct bpf_map_ops {
> u32 (*map_fd_sys_lookup_elem)(void *ptr);
> void (*map_seq_show_elem)(struct bpf_map *map, void *key,
> struct seq_file *m);
> - int (*map_check_btf)(const struct bpf_map *map, const struct btf *btf,
> - u32 key_type_id, u32 value_type_id);
> };
>
> struct bpf_map {
> @@ -118,7 +116,7 @@ static inline bool bpf_map_offload_neutral(const struct bpf_map *map)
>
> static inline bool bpf_map_support_seq_show(const struct bpf_map *map)
> {
> - return map->ops->map_seq_show_elem && map->ops->map_check_btf;
> + return map->btf && map->ops->map_seq_show_elem;
> }
>
> extern const struct bpf_map_ops bpf_map_offload_ops;
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 2aa55d030..67f0bdf 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -358,32 +358,6 @@ static void array_map_seq_show_elem(struct bpf_map *map, void *key,
> rcu_read_unlock();
> }
>
> -static int array_map_check_btf(const struct bpf_map *map, const struct btf *btf,
> - u32 btf_key_id, u32 btf_value_id)
> -{
> - const struct btf_type *key_type, *value_type;
> - u32 key_size, value_size;
> - u32 int_data;
> -
> - key_type = btf_type_id_size(btf, &btf_key_id, &key_size);
> - if (!key_type || BTF_INFO_KIND(key_type->info) != BTF_KIND_INT)
> - return -EINVAL;
> -
> - int_data = *(u32 *)(key_type + 1);
> - /* bpf array can only take a u32 key. This check makes
> - * sure that the btf matches the attr used during map_create.
> - */
> - if (BTF_INT_BITS(int_data) != 32 || key_size != 4 ||
> - BTF_INT_OFFSET(int_data))
> - return -EINVAL;
I think most of these checks are still necessary for array type.
Relaxing BTF array key from BTF_KIND_INT to, for example, BTF_KIND_ENUM
is probably ok, but key being BTF_KIND_PTR or BTF_KIND_ARRAY doesn't makes sense.
For hash maps we probably need hash specific checks too. Otherwise
such sanity checks would need to be in kernel pretty printer and later
in user space too (bpftool and everything that will consume BTF),
since user space won't be able to trust kernel with sane key types.
^ permalink raw reply
* Re: [PATCH] net: dsa: rtl8366rb: Support port 4 (WAN)
From: David Miller @ 2018-08-09 21:15 UTC (permalink / raw)
To: linus.walleij
Cc: andrew, vivien.didelot, f.fainelli, netdev, openwrt-devel,
lede-dev
In-Reply-To: <20180808123855.18729-1-linus.walleij@linaro.org>
From: Linus Walleij <linus.walleij@linaro.org>
Date: Wed, 8 Aug 2018 14:38:55 +0200
> The totally undocumented IO mode needs to be set to enumerator
> 0 to enable port 4 also known as WAN in most configurations,
> for ordinary traffic. The 3 bits in the register come up as
> 010 after reset, but need to be set to 000.
>
> The Realtek source code contains a name for these bits, but
> no explanation of what the 8 different IO modes may be.
>
> Set it to zero for the time being and drop a comment so
> people know what is going on if they run into trouble. This
> "mode zero" works fine with the D-Link DIR-685 with
> RTL8366RB.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Applied to net-next, thank you.
^ permalink raw reply
* Re: [PATCH v2 net-next] net: phy: sfp: print debug message with text, not numbers
From: David Miller @ 2018-08-09 21:20 UTC (permalink / raw)
To: andrew; +Cc: netdev, rmk+kernel, f.fainelli
In-Reply-To: <1533819620-19535-1-git-send-email-andrew@lunn.ch>
From: Andrew Lunn <andrew@lunn.ch>
Date: Thu, 9 Aug 2018 15:00:20 +0200
> Convert the state numbers, device state, etc from numbers to strings
> when printing debug messages.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> v2: Fixed typo in subject line.
> Add Acked-by from Florian
Grrr, I committed and pushed out the version with the Subject header
typo.
I could revert and apply this one but that would just make things
look worse.
Sorry, will be more careful next time!
^ permalink raw reply
* Re: [PATCH lora-next v2 3/8] net: lora: sx1301: convert to passing priv data throughout
From: Andreas Färber @ 2018-08-09 21:21 UTC (permalink / raw)
To: Ben Whitten
Cc: 潘建宏, hasnain.virk, netdev, Xue Liu, shess,
Ben Whitten, Yannick Lanz
In-Reply-To: <CAF3==is4CLr5faEyzB2ADbsDXWD8A4eca73+CHZvPTkY81yqDQ@mail.gmail.com>
Am 09.08.2018 um 23:06 schrieb Ben Whitten:
> On Thu, 9 Aug 2018 at 21:43, Andreas Färber <afaerber@suse.de> wrote:
>> Am 09.08.2018 um 14:33 schrieb Ben Whitten:
>>> @@ -654,22 +646,35 @@ static int sx1301_probe(struct spi_device *spi)
>>> priv->rst_gpio = rst;
>>> priv->cur_page = 0xff;
>>>
>>> - spi_set_drvdata(spi, netdev);
>>> + spi_set_drvdata(spi, priv);
>>
>> This change seems unnecessary and counter-productive for unregistration.
>>
>> Otherwise applying.
>
> This is actually pretty critical, as it stands with the two spi masters we use
> spi_get_drvdata on the parent device of the controller to recover the priv
> struct for regmap.
>
> We may have to include the netdev in the priv data, or do a container_of
> dance to recover netdev in unregistration.
> That said if we wrap things in devm then really our remove function could
> be empty, as we have done with the allocation.
Thanks for quickly noticing. This should compensate:
diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 0ba841f8e7cd..43cd2308e41c 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -164,7 +164,8 @@ static int sx1301_soft_reset(struct sx1301_priv *priv)
static int sx1301_radio_set_cs(struct spi_controller *ctrl, bool enable)
{
struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl);
- struct sx1301_priv *priv = spi_get_drvdata(ssx->parent);
+ struct net_device *netdev = spi_get_drvdata(ssx->parent);
+ struct sx1301_priv *priv = netdev_priv(netdev);
u8 cs;
int ret;
@@ -204,7 +205,8 @@ static int sx1301_radio_spi_transfer_one(struct
spi_controller *ctrl,
struct spi_device *spi, struct spi_transfer *xfr)
{
struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl);
- struct sx1301_priv *priv = spi_get_drvdata(ssx->parent);
+ struct net_device *netdev = spi_get_drvdata(ssx->parent);
+ struct sx1301_priv *priv = netdev_priv(netdev);
const u8 *tx_buf = xfr->tx_buf;
u8 *rx_buf = xfr->rx_buf;
int ret;
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply related
* Re: [PATCH net-next v2 1/1] net/tls: Combined memory allocation for decryption request
From: David Miller @ 2018-08-09 21:23 UTC (permalink / raw)
To: vakul.garg; +Cc: netdev, borisp, aviadye, davejwatson
In-Reply-To: <20180808232623.18075-2-vakul.garg@nxp.com>
From: Vakul Garg <vakul.garg@nxp.com>
Date: Thu, 9 Aug 2018 04:56:23 +0530
> For preparing decryption request, several memory chunks are required
> (aead_req, sgin, sgout, iv, aad). For submitting the decrypt request to
> an accelerator, it is required that the buffers which are read by the
> accelerator must be dma-able and not come from stack. The buffers for
> aad and iv can be separately kmalloced each, but it is inefficient.
> This patch does a combined allocation for preparing decryption request
> and then segments into aead_req || sgin || sgout || iv || aad.
>
> Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
> ---
> This patch needs to be applied over Doron Roberts-Kedes's patch.
> net/tls: Calculate nsg for zerocopy path without skb_cow_data.
That's going to have many changes, I gave feedback on it yesterday.
Please do not post patches which have pre-requisites which are in
the process of changing or similar as that makes a lot more work
for me and you are also asking people to review changes on top
of code which is going to change.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next v2] net: allow to call netif_reset_xps_queues() under cpus_read_lock
From: David Miller @ 2018-08-09 21:26 UTC (permalink / raw)
To: avagin; +Cc: netdev, avagin, amritha.nambiar, mst, jasowang
In-Reply-To: <20180809030735.16173-1-avagin@openvz.org>
From: Andrei Vagin <avagin@openvz.org>
Date: Wed, 8 Aug 2018 20:07:35 -0700
> From: Andrei Vagin <avagin@gmail.com>
>
> The definition of static_key_slow_inc() has cpus_read_lock in place. In the
> virtio_net driver, XPS queues are initialized after setting the queue:cpu
> affinity in virtnet_set_affinity() which is already protected within
> cpus_read_lock. Lockdep prints a warning when we are trying to acquire
> cpus_read_lock when it is already held.
>
> This patch adds an ability to call __netif_set_xps_queue under
> cpus_read_lock().
...
Applied, thank you.
^ permalink raw reply
* Re: [PATCH net-next] cxgb4: update 1.20.8.0 as the latest firmware supported
From: David Miller @ 2018-08-09 21:27 UTC (permalink / raw)
To: ganeshgr; +Cc: netdev, nirranjan, indranil, dt
In-Reply-To: <1533798123-21071-1-git-send-email-ganeshgr@chelsio.com>
From: Ganesh Goudar <ganeshgr@chelsio.com>
Date: Thu, 9 Aug 2018 12:32:03 +0530
> Change t4fw_version.h to update latest firmware version
> number to 1.20.8.0.
>
> Signed-off-by: Ganesh Goudar <ganeshgr@chelsio.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: enable btf for use in all maps
From: Daniel Borkmann @ 2018-08-09 21:30 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: ast, netdev, yhs
In-Reply-To: <20180809211416.oznmx5jlnbagkk3w@ast-mbp>
On 08/09/2018 11:14 PM, Alexei Starovoitov wrote:
> On Thu, Aug 09, 2018 at 09:42:20PM +0200, Daniel Borkmann wrote:
>> Commit a26ca7c982cb ("bpf: btf: Add pretty print support to
>> the basic arraymap") enabled support for BTF and dumping via
>> BPF fs for arraymap. However, both can be decoupled from each
>> other such that all BPF maps can be supported for attaching
>> BTF key/value information, while not all maps necessarily
>> need to dump via map_seq_show_elem() callback.
>>
>> The check in array_map_check_btf() can be generalized as
>> ultimatively the key and value size is the only contraint
>> that needs to match for the map. The fact that the key needs
>> to be of type int is optional; it could be any data type as
>> long as it matches the 4 byte key size, just like hash table
>> key or others could be of any data type as well.
>>
>> Minimal example of a hash table dump which then works out
>> of the box for bpftool:
>>
>> # bpftool map dump id 19
>> [{
>> "key": {
>> "": {
>> "vip": 0,
>> "vipv6": []
>> },
>> "port": 0,
>> "family": 0,
>> "proto": 0
>> },
>> "value": {
>> "flags": 0,
>> "vip_num": 0
>> }
>> }
>> ]
>>
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: Yonghong Song <yhs@fb.com>
>> ---
>> include/linux/bpf.h | 4 +---
>> kernel/bpf/arraymap.c | 27 ---------------------------
>> kernel/bpf/inode.c | 3 ++-
>> kernel/bpf/syscall.c | 24 ++++++++++++++++++++----
>> 4 files changed, 23 insertions(+), 35 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index cd8790d..eb76e8e 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -48,8 +48,6 @@ struct bpf_map_ops {
>> u32 (*map_fd_sys_lookup_elem)(void *ptr);
>> void (*map_seq_show_elem)(struct bpf_map *map, void *key,
>> struct seq_file *m);
>> - int (*map_check_btf)(const struct bpf_map *map, const struct btf *btf,
>> - u32 key_type_id, u32 value_type_id);
>> };
>>
>> struct bpf_map {
>> @@ -118,7 +116,7 @@ static inline bool bpf_map_offload_neutral(const struct bpf_map *map)
>>
>> static inline bool bpf_map_support_seq_show(const struct bpf_map *map)
>> {
>> - return map->ops->map_seq_show_elem && map->ops->map_check_btf;
>> + return map->btf && map->ops->map_seq_show_elem;
>> }
>>
>> extern const struct bpf_map_ops bpf_map_offload_ops;
>> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
>> index 2aa55d030..67f0bdf 100644
>> --- a/kernel/bpf/arraymap.c
>> +++ b/kernel/bpf/arraymap.c
>> @@ -358,32 +358,6 @@ static void array_map_seq_show_elem(struct bpf_map *map, void *key,
>> rcu_read_unlock();
>> }
>>
>> -static int array_map_check_btf(const struct bpf_map *map, const struct btf *btf,
>> - u32 btf_key_id, u32 btf_value_id)
>> -{
>> - const struct btf_type *key_type, *value_type;
>> - u32 key_size, value_size;
>> - u32 int_data;
>> -
>> - key_type = btf_type_id_size(btf, &btf_key_id, &key_size);
>> - if (!key_type || BTF_INFO_KIND(key_type->info) != BTF_KIND_INT)
>> - return -EINVAL;
>> -
>> - int_data = *(u32 *)(key_type + 1);
>> - /* bpf array can only take a u32 key. This check makes
>> - * sure that the btf matches the attr used during map_create.
>> - */
>> - if (BTF_INT_BITS(int_data) != 32 || key_size != 4 ||
>> - BTF_INT_OFFSET(int_data))
>> - return -EINVAL;
>
> I think most of these checks are still necessary for array type.
> Relaxing BTF array key from BTF_KIND_INT to, for example, BTF_KIND_ENUM
> is probably ok, but key being BTF_KIND_PTR or BTF_KIND_ARRAY doesn't makes sense.
Hmm, so on 64 bit archs BTF_KIND_PTR would get rejected for array,
on 32 bit it may be allowed due to sizeof(void *) == 4. BTF_KIND_ARRAY
could be array of u8 foo[4], for example, or u16 foo[2]. But how would
it ultimately be different from e.g. having 'struct a' versus 'struct b'
where both are of same size and while actual key has 'struct a', the one
who writes the prog resp. loads the BTF into the kernel would lie about
it stating it's of type 'struct b' instead? It's basically trusting the
app that it advertised sane key types which kernel is propagating back.
Thanks,
Daniel
^ permalink raw reply
* Re: [Patch net-next] net_sched: fix a potential out-of-bound access
From: Cong Wang @ 2018-08-09 21:43 UTC (permalink / raw)
To: Vlad Buslov; +Cc: Linux Kernel Network Developers, Jiri Pirko
In-Reply-To: <vbflg9g2dx5.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
On Thu, Aug 9, 2018 at 12:32 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
> Before version V5 of my action API patchset this functionality was
> implemented in exactly the same way as in your patch. Unfortunately, it
> has a double-free bug. The problem is that if you have multiple
> actions(N) being deleted, and deleted succeeded for first K actions,
> this implementation will try to delete all N actions second time
> (including first K actions that were already deleted). That is why I
> added 'acts_deleted' variable that tracks actual amount of actions that
> were deleted successfully, and only delete last N-K actions in case of
> error.
Interesting, I didn't notice you call it for tcf_del_notify()'s failure too.
But this is easy to resolve, we can just set succeeded ones to NULL
and teach tcf_action_put_many() to scan the whole array but
skip NULL's.
>
> In order to fix that issue I did following code changes in V5:
> - Added 'acts_deleted' variable to delete only actions [K, N) in case of
> error.
> - Extended 'actions' array size by one to ensure that it always ends
> with NULL pointer.
Oh, I see, this is not how we use C, you can at least rollback
by passing acts_deleted as a parameter as the start of the array.
You picked the most confusing way to handle it.
I will send an updated patch.
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: enable btf for use in all maps
From: Alexei Starovoitov @ 2018-08-09 21:44 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: ast, netdev, yhs
In-Reply-To: <d851548d-e027-57ee-55c9-a61c48ffcf15@iogearbox.net>
On Thu, Aug 09, 2018 at 11:30:52PM +0200, Daniel Borkmann wrote:
> On 08/09/2018 11:14 PM, Alexei Starovoitov wrote:
> > On Thu, Aug 09, 2018 at 09:42:20PM +0200, Daniel Borkmann wrote:
> >> Commit a26ca7c982cb ("bpf: btf: Add pretty print support to
> >> the basic arraymap") enabled support for BTF and dumping via
> >> BPF fs for arraymap. However, both can be decoupled from each
> >> other such that all BPF maps can be supported for attaching
> >> BTF key/value information, while not all maps necessarily
> >> need to dump via map_seq_show_elem() callback.
> >>
> >> The check in array_map_check_btf() can be generalized as
> >> ultimatively the key and value size is the only contraint
> >> that needs to match for the map. The fact that the key needs
> >> to be of type int is optional; it could be any data type as
> >> long as it matches the 4 byte key size, just like hash table
> >> key or others could be of any data type as well.
> >>
> >> Minimal example of a hash table dump which then works out
> >> of the box for bpftool:
> >>
> >> # bpftool map dump id 19
> >> [{
> >> "key": {
> >> "": {
> >> "vip": 0,
> >> "vipv6": []
> >> },
> >> "port": 0,
> >> "family": 0,
> >> "proto": 0
> >> },
> >> "value": {
> >> "flags": 0,
> >> "vip_num": 0
> >> }
> >> }
> >> ]
> >>
> >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> >> Cc: Yonghong Song <yhs@fb.com>
> >> ---
> >> include/linux/bpf.h | 4 +---
> >> kernel/bpf/arraymap.c | 27 ---------------------------
> >> kernel/bpf/inode.c | 3 ++-
> >> kernel/bpf/syscall.c | 24 ++++++++++++++++++++----
> >> 4 files changed, 23 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >> index cd8790d..eb76e8e 100644
> >> --- a/include/linux/bpf.h
> >> +++ b/include/linux/bpf.h
> >> @@ -48,8 +48,6 @@ struct bpf_map_ops {
> >> u32 (*map_fd_sys_lookup_elem)(void *ptr);
> >> void (*map_seq_show_elem)(struct bpf_map *map, void *key,
> >> struct seq_file *m);
> >> - int (*map_check_btf)(const struct bpf_map *map, const struct btf *btf,
> >> - u32 key_type_id, u32 value_type_id);
> >> };
> >>
> >> struct bpf_map {
> >> @@ -118,7 +116,7 @@ static inline bool bpf_map_offload_neutral(const struct bpf_map *map)
> >>
> >> static inline bool bpf_map_support_seq_show(const struct bpf_map *map)
> >> {
> >> - return map->ops->map_seq_show_elem && map->ops->map_check_btf;
> >> + return map->btf && map->ops->map_seq_show_elem;
> >> }
> >>
> >> extern const struct bpf_map_ops bpf_map_offload_ops;
> >> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> >> index 2aa55d030..67f0bdf 100644
> >> --- a/kernel/bpf/arraymap.c
> >> +++ b/kernel/bpf/arraymap.c
> >> @@ -358,32 +358,6 @@ static void array_map_seq_show_elem(struct bpf_map *map, void *key,
> >> rcu_read_unlock();
> >> }
> >>
> >> -static int array_map_check_btf(const struct bpf_map *map, const struct btf *btf,
> >> - u32 btf_key_id, u32 btf_value_id)
> >> -{
> >> - const struct btf_type *key_type, *value_type;
> >> - u32 key_size, value_size;
> >> - u32 int_data;
> >> -
> >> - key_type = btf_type_id_size(btf, &btf_key_id, &key_size);
> >> - if (!key_type || BTF_INFO_KIND(key_type->info) != BTF_KIND_INT)
> >> - return -EINVAL;
> >> -
> >> - int_data = *(u32 *)(key_type + 1);
> >> - /* bpf array can only take a u32 key. This check makes
> >> - * sure that the btf matches the attr used during map_create.
> >> - */
> >> - if (BTF_INT_BITS(int_data) != 32 || key_size != 4 ||
> >> - BTF_INT_OFFSET(int_data))
> >> - return -EINVAL;
> >
> > I think most of these checks are still necessary for array type.
> > Relaxing BTF array key from BTF_KIND_INT to, for example, BTF_KIND_ENUM
> > is probably ok, but key being BTF_KIND_PTR or BTF_KIND_ARRAY doesn't makes sense.
>
> Hmm, so on 64 bit archs BTF_KIND_PTR would get rejected for array,
> on 32 bit it may be allowed due to sizeof(void *) == 4. BTF_KIND_ARRAY
> could be array of u8 foo[4], for example, or u16 foo[2]. But how would
> it ultimately be different from e.g. having 'struct a' versus 'struct b'
> where both are of same size and while actual key has 'struct a', the one
> who writes the prog resp. loads the BTF into the kernel would lie about
> it stating it's of type 'struct b' instead? It's basically trusting the
> app that it advertised sane key types which kernel is propagating back.
for hash map - yes. the kernel cannot yet catch the lie that
key == 'struct a' that user said in BTF is not what program used
(which used 'struct b' of the same size).
Eventually we will annotate all load/store in the program and will
make sure that memory access match what BTF said.
For array we can catch the lie today that key is not 4 byte int,
since it matters from pretty printing point of view.
If it's PTR or ARRAY or STRUCT, the printer will go nuts.
When userspace can trust kernel that array key is u32 it can print
int arr[10];
just like gdb does:
(gdb) p arr
$1 = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0}
(gdb) ptype arr
type = int [10]
What user printer suppose to do if kernel says that key=PTR or, worse, key=STRUCT ?
I cannot think of sane way of printing such array.
Even key=ENUM is not trivial to print, but I think it can be useful and
practical to use ENUM as a key, but for now I'd stick to INT only
like the check does today.
^ permalink raw reply
* Re: [PATCH lora-next v2 7/8] net: lora: sx1301: add initial registration for regmap
From: Andreas Färber @ 2018-08-09 21:58 UTC (permalink / raw)
To: Ben Whitten
Cc: starnight, hasnain.virk, netdev, liuxuenetmail, shess,
Ben Whitten
In-Reply-To: <1533818018-29005-7-git-send-email-ben.whitten@lairdtech.com>
Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> The register and bit-field definitions are taken from the SX1301
> datasheet version 2.01 dated June 2014 with the revision information
> 'First released version'.
>
> The reset state and RW capability of each field is not reflected in this
> patch however from the datasheet:
> "Bits and registers that are not documented are reserved. They may
> include calibration values. It is important not to modify these bits and
> registers. If specific bits must be changed in a register with reserved
> bits, the register must be read first, specific bits modified while
> masking reserved bits and then the register can be written."
>
> Then goes on to state:
> "Reserved bits should be written with their reset state, they may be
> read different states."
>
> Caching is currently disabled.
>
> The version is read back using regmap_read to verify regmap operation,
> in doing so needs to be moved after priv and regmap allocation.
>
> Further registers or fields are added as they are required in conversion.
>
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
> drivers/net/lora/Kconfig | 1 +
> drivers/net/lora/sx1301.c | 46 ++++++++++++++++++++++++++++++++++++++++++----
> drivers/net/lora/sx1301.h | 10 ++++++++++
> 3 files changed, 53 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/lora/Kconfig b/drivers/net/lora/Kconfig
> index bb57a01..79d23f2 100644
> --- a/drivers/net/lora/Kconfig
> +++ b/drivers/net/lora/Kconfig
> @@ -49,6 +49,7 @@ config LORA_SX1301
> tristate "Semtech SX1301 SPI driver"
> default y
> depends on SPI
> + select REGMAP_SPI
> help
> Semtech SX1301
>
> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index 8e81179..766df06 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
> @@ -20,11 +20,11 @@
> #include <linux/of_gpio.h>
> #include <linux/lora/dev.h>
> #include <linux/spi/spi.h>
> +#include <linux/regmap.h>
Misordered.
>
> #include "sx1301.h"
>
> #define REG_PAGE_RESET 0
> -#define REG_VERSION 1
> #define REG_MCU_PROM_ADDR 9
> #define REG_MCU_PROM_DATA 10
> #define REG_GPIO_SELECT_INPUT 27
> @@ -68,6 +68,35 @@
>
> #define REG_EMERGENCY_FORCE_HOST_CTRL BIT(0)
>
> +static const struct regmap_range_cfg sx1301_ranges[] = {
Let's rename to _regmap_ranges for consistency.
> @@ -81,6 +110,7 @@ struct sx1301_priv {
> struct gpio_desc *rst_gpio;
> u8 cur_page;
> struct spi_controller *radio_a_ctrl, *radio_b_ctrl;
> + struct regmap *regmap;
Note: We need a consistent style. Either whitespace or tabs, not both
depending on author. Same in an earlier patch. Problem with tabs is that
at some point it's always one tab too little, but we can try it.
Applied.
Thanks,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply
* Re: KCM - recvmsg() mangles packets?
From: Dominique Martinet @ 2018-08-09 22:06 UTC (permalink / raw)
To: Tom Herbert; +Cc: Linux Kernel Network Developers
In-Reply-To: <CAPDqMeptLoLZ2RU+T-d7YwkO2AeKV+nfow4C-LqqeZd9mo-vMg@mail.gmail.com>
Tom Herbert wrote on Thu, Aug 09, 2018:
> > diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
> > index 625acb27efcc..348ff5945591 100644
> > --- a/net/strparser/strparser.c
> > +++ b/net/strparser/strparser.c
> > @@ -222,6 +222,16 @@ static int __strp_recv(read_descriptor_t *desc, struct sk_buff *orig_skb,
> > if (!stm->strp.full_len) {
> > ssize_t len;
> >
> > + /* Can only parse if there is no offset */
> > + if (unlikely(stm->strp.offset)) {
> > + if (!pskb_pull(skb, stm->strp.offset)) {
> > + STRP_STATS_INCR(strp->stats.mem_fail);
> > + strp_parser_err(strp, -ENOMEM, desc);
> > + break;
> > + }
> > + stm->strp.offset = 0;
> > + }
> > +
>
> Seems okay to me for a fix.
Hmm, if you say so, I'll send this as a patch for broader comments right
away.
> Looks like strp.offset is only set in one place and read in one
> place. With this pull maybe that just can go away?
Good point, when strp.offset is set the full_len is also being init'd so
we will necessarily do the pull next...
But the way tls uses strparser is also kind of weird, since they modify
the strp_msg's offset and full_len, I wouldn't want to assume we can't
have full_len == 0 *again* later with a non zero offset...
On the other hand they do handle non-zero offset in their parse function
so they'd be ok with that... Ultimately it's probably closer to a design
choice than anything else.
I'll still send a v0 of the patch as is, because I feel it's easier to
understand that we pull because the existing parse_msg functions do not
handle it properly, and will write a note that I intend to move it up a
few lines as a comment.
Thanks,
--
Dominique Martinet
^ permalink raw reply
* Re: [PATCH net-next v2] net: allow to call netif_reset_xps_queues() under cpus_read_lock
From: Michael S. Tsirkin @ 2018-08-09 22:13 UTC (permalink / raw)
To: Andrei Vagin
Cc: David S. Miller, netdev, Andrei Vagin, Nambiar, Amritha,
Jason Wang
In-Reply-To: <20180809030735.16173-1-avagin@openvz.org>
On Wed, Aug 08, 2018 at 08:07:35PM -0700, Andrei Vagin wrote:
> From: Andrei Vagin <avagin@gmail.com>
>
> The definition of static_key_slow_inc() has cpus_read_lock in place. In the
> virtio_net driver, XPS queues are initialized after setting the queue:cpu
> affinity in virtnet_set_affinity() which is already protected within
> cpus_read_lock. Lockdep prints a warning when we are trying to acquire
> cpus_read_lock when it is already held.
>
> This patch adds an ability to call __netif_set_xps_queue under
> cpus_read_lock().
>
> ============================================
> WARNING: possible recursive locking detected
> 4.18.0-rc3-next-20180703+ #1 Not tainted
> --------------------------------------------
> swapper/0/1 is trying to acquire lock:
> 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: static_key_slow_inc+0xe/0x20
>
> but task is already holding lock:
> 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: init_vqs+0x513/0x5a0
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(cpu_hotplug_lock.rw_sem);
> lock(cpu_hotplug_lock.rw_sem);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> 3 locks held by swapper/0/1:
> #0: 00000000244bc7da (&dev->mutex){....}, at: __driver_attach+0x5a/0x110
> #1: 00000000cf973d46 (cpu_hotplug_lock.rw_sem){++++}, at: init_vqs+0x513/0x5a0
> #2: 000000005cd8463f (xps_map_mutex){+.+.}, at: __netif_set_xps_queue+0x8d/0xc60
>
> v2: move cpus_read_lock() out of __netif_set_xps_queue()
FYI you change log should go after -- below, not before it.
> Cc: "Nambiar, Amritha" <amritha.nambiar@intel.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Fixes: 8af2c06ff4b1 ("net-sysfs: Add interface for Rx queue(s) map per Tx queue")
>
> Signed-off-by: Andrei Vagin <avagin@gmail.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/virtio_net.c | 4 +++-
> net/core/dev.c | 20 +++++++++++++++-----
> net/core/net-sysfs.c | 4 ++++
> 3 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 62311dde6e71..39a7f4452587 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1903,9 +1903,11 @@ static void virtnet_set_affinity(struct virtnet_info *vi)
>
> i = 0;
> for_each_online_cpu(cpu) {
> + const unsigned long *mask = cpumask_bits(cpumask_of(cpu));
> +
> virtqueue_set_affinity(vi->rq[i].vq, cpu);
> virtqueue_set_affinity(vi->sq[i].vq, cpu);
> - netif_set_xps_queue(vi->dev, cpumask_of(cpu), i);
> + __netif_set_xps_queue(vi->dev, mask, i, false);
> i++;
> }
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index f68122f0ab02..325fc5088370 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2176,6 +2176,7 @@ static void netif_reset_xps_queues(struct net_device *dev, u16 offset,
> if (!static_key_false(&xps_needed))
> return;
>
> + cpus_read_lock();
> mutex_lock(&xps_map_mutex);
>
> if (static_key_false(&xps_rxqs_needed)) {
> @@ -2199,10 +2200,11 @@ static void netif_reset_xps_queues(struct net_device *dev, u16 offset,
>
> out_no_maps:
> if (static_key_enabled(&xps_rxqs_needed))
> - static_key_slow_dec(&xps_rxqs_needed);
> + static_key_slow_dec_cpuslocked(&xps_rxqs_needed);
>
> - static_key_slow_dec(&xps_needed);
> + static_key_slow_dec_cpuslocked(&xps_needed);
> mutex_unlock(&xps_map_mutex);
> + cpus_read_unlock();
> }
>
> static void netif_reset_xps_queues_gt(struct net_device *dev, u16 index)
> @@ -2250,6 +2252,7 @@ static struct xps_map *expand_xps_map(struct xps_map *map, int attr_index,
> return new_map;
> }
>
> +/* Must be called under cpus_read_lock */
> int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
> u16 index, bool is_rxqs_map)
> {
> @@ -2317,9 +2320,9 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
> if (!new_dev_maps)
> goto out_no_new_maps;
>
> - static_key_slow_inc(&xps_needed);
> + static_key_slow_inc_cpuslocked(&xps_needed);
> if (is_rxqs_map)
> - static_key_slow_inc(&xps_rxqs_needed);
> + static_key_slow_inc_cpuslocked(&xps_rxqs_needed);
>
> for (j = -1; j = netif_attrmask_next(j, possible_mask, nr_ids),
> j < nr_ids;) {
> @@ -2448,11 +2451,18 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
> kfree(new_dev_maps);
> return -ENOMEM;
> }
> +EXPORT_SYMBOL_GPL(__netif_set_xps_queue);
>
> int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
> u16 index)
> {
> - return __netif_set_xps_queue(dev, cpumask_bits(mask), index, false);
> + int ret;
> +
> + cpus_read_lock();
> + ret = __netif_set_xps_queue(dev, cpumask_bits(mask), index, false);
> + cpus_read_unlock();
> +
> + return ret;
> }
> EXPORT_SYMBOL(netif_set_xps_queue);
>
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 0a95bcf64cdc..bd67c4d0fcfd 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -26,6 +26,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/of.h>
> #include <linux/of_net.h>
> +#include <linux/cpu.h>
>
> #include "net-sysfs.h"
>
> @@ -1400,7 +1401,10 @@ static ssize_t xps_rxqs_store(struct netdev_queue *queue, const char *buf,
> return err;
> }
>
> + cpus_read_lock();
> err = __netif_set_xps_queue(dev, mask, index, true);
> + cpus_read_unlock();
> +
> kfree(mask);
> return err ? : len;
> }
> --
> 2.17.1
^ permalink raw reply
* Re: [PATCH lora-next v2 8/8] net: lora: sx1301: convert driver over to regmap reads and writes
From: Andreas Färber @ 2018-08-09 22:34 UTC (permalink / raw)
To: Ben Whitten
Cc: starnight, hasnain.virk, netdev, liuxuenetmail, shess,
Ben Whitten, Yannick Lanz
In-Reply-To: <1533818018-29005-8-git-send-email-ben.whitten@lairdtech.com>
Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> The reads and writes are replaced with regmap versions and unneeded
> functions, variable, and defines removed.
>
> Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> ---
> drivers/net/lora/sx1301.c | 204 +++++++++++++++-------------------------------
> drivers/net/lora/sx1301.h | 30 +++++++
> 2 files changed, 95 insertions(+), 139 deletions(-)
>
> diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> index 766df06..4db5a43 100644
> --- a/drivers/net/lora/sx1301.c
> +++ b/drivers/net/lora/sx1301.c
[...]
> @@ -140,50 +115,9 @@ static int sx1301_write(struct sx1301_priv *priv, u8 reg, u8 val)
> return sx1301_write_burst(priv, reg, &val, 1);
> }
_write and _read are now unused, causing warnings. Dropping.
The _burst versions are still in use for firmware load, and I saw a
discussion indicating that regmap is lacking the capability to not
increment the reg for bulk reads at the moment. So we still can't
cleanly switch to regmap entirely and thereby remain bound to SPI.
[...]
> @@ -235,8 +169,8 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
> {
> struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl);
> struct sx1301_priv *priv = spi_get_drvdata(ssx->parent);
> - const u8 *tx_buf = xfr->tx_buf;
> - u8 *rx_buf = xfr->rx_buf;
> + const unsigned int *tx_buf = xfr->tx_buf;
> + unsigned int *rx_buf = xfr->rx_buf;
These are wrong both for Little Endian and even worse for Big Endian.
> int ret;
>
> if (xfr->len == 0 || xfr->len > 3)
> @@ -245,13 +179,13 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
> dev_dbg(&spi->dev, "transferring one (%u)\n", xfr->len);
>
> if (tx_buf) {
> - ret = sx1301_page_write(priv, ssx->page, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0);
> + ret = regmap_write(priv->regmap, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0);
> if (ret) {
> dev_err(&spi->dev, "SPI radio address write failed\n");
> return ret;
> }
>
> - ret = sx1301_page_write(priv, ssx->page, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
> + ret = regmap_write(priv->regmap, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
> if (ret) {
> dev_err(&spi->dev, "SPI radio data write failed\n");
> return ret;
> @@ -271,7 +205,7 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
> }
>
> if (rx_buf) {
> - ret = sx1301_page_read(priv, ssx->page, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
> + ret = regmap_read(priv->regmap, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
> if (ret) {
> dev_err(&spi->dev, "SPI radio data read failed\n");
> return ret;
Fixing by adding a local variable instead:
@@ -239,6 +163,7 @@ static int sx1301_radio_spi_transfer_one(struct
spi_controll
er *ctrl,
struct sx1301_priv *priv = netdev_priv(netdev);
const u8 *tx_buf = xfr->tx_buf;
u8 *rx_buf = xfr->rx_buf;
+ unsigned int val;
int ret;
if (xfr->len == 0 || xfr->len > 3)
[...]
@@ -273,27 +198,28 @@ static int sx1301_radio_spi_transfer_one(struct
spi_controller *ctrl,
}
if (rx_buf) {
- ret = sx1301_page_read(priv, ssx->page, ssx->regs +
REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
+ ret = regmap_read(priv->regmap, ssx->regs +
REG_RADIO_X_DATA_READBACK, &val);
if (ret) {
dev_err(&spi->dev, "SPI radio data read failed\n");
return ret;
}
+ rx_buf[xfr->len - 1] = val & 0xff;
}
return 0;
[...]
> diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
> index 2fc283f..b21e5c6 100644
> --- a/drivers/net/lora/sx1301.h
> +++ b/drivers/net/lora/sx1301.h
> @@ -18,11 +18,41 @@
> /* Page independent */
> #define SX1301_PAGE 0x00
> #define SX1301_VER 0x01
> +#define SX1301_MPA 0x09
Those are the official register names? I find these much harder to read
than my guessed names. Could we keep the long names as aliases?
> +#define SX1301_MPD 0x0A
> +#define SX1301_GEN 0x10
> +#define SX1301_CKEN 0x11
> +#define SX1301_GPSO 0x1C
> +#define SX1301_GPMODE 0x1D
> +#define SX1301_AGCSTS 0x20
>
> #define SX1301_VIRT_BASE 0x100
> #define SX1301_PAGE_LEN 0x80
> #define SX1301_PAGE_BASE(n) (SX1301_VIRT_BASE + (SX1301_PAGE_LEN * n))
>
> +/* Page 0 */
> +#define SX1301_CHRS (SX1301_PAGE_BASE(0) + 0x23)
> +#define SX1301_FORCE_CTRL (SX1301_PAGE_BASE(0) + 0x69)
> +#define SX1301_MCU_CTRL (SX1301_PAGE_BASE(0) + 0x6A)
> +
> +/* Page 2 */
> +#define SX1301_RADIO_A_SPI_DATA (SX1301_PAGE_BASE(2) + 0x21)
> +#define SX1301_RADIO_A_SPI_DATA_RB (SX1301_PAGE_BASE(2) + 0x22)
> +#define SX1301_RADIO_A_SPI_ADDR (SX1301_PAGE_BASE(2) + 0x23)
> +#define SX1301_RADIO_A_SPI_CS (SX1301_PAGE_BASE(2) + 0x25)
> +#define SX1301_RADIO_B_SPI_DATA (SX1301_PAGE_BASE(2) + 0x26)
> +#define SX1301_RADIO_B_SPI_DATA_RB (SX1301_PAGE_BASE(2) + 0x27)
> +#define SX1301_RADIO_B_SPI_ADDR (SX1301_PAGE_BASE(2) + 0x28)
> +#define SX1301_RADIO_B_SPI_CS (SX1301_PAGE_BASE(2) + 0x2A)
> +#define SX1301_RADIO_CFG (SX1301_PAGE_BASE(2) + 0x2B)
> +#define SX1301_DBG_ARB_MCU_RAM_DATA (SX1301_PAGE_BASE(2) + 0x40)
> +#define SX1301_DBG_AGC_MCU_RAM_DATA (SX1301_PAGE_BASE(2) + 0x41)
> +#define SX1301_DBG_ARB_MCU_RAM_ADDR (SX1301_PAGE_BASE(2) + 0x50)
> +#define SX1301_DBG_AGC_MCU_RAM_ADDR (SX1301_PAGE_BASE(2) + 0x51)
> +
> +/* Page 3 */
> +#define SX1301_EMERGENCY_FORCE_HOST_CTRL (SX1301_PAGE_BASE(3) + 0x7F)
> +
> #define SX1301_MAX_REGISTER (SX1301_PAGE_BASE(3) + 0x7F)
>
> #endif
Applying so that we can continue based on regmap.
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply
* [PATCH v0] strparser: remove any offset before parsing messages
From: Dominique Martinet @ 2018-08-09 22:40 UTC (permalink / raw)
To: Doron Roberts-Kedes, Tom Herbert, Dave Watson
Cc: Dominique Martinet, David S. Miller, netdev, linux-kernel
Offset is not well handled by strparser users right now.
Out of the current strparser users, we have:
- tls, that handles offset properly in parse and rcv callbacks
- kcm, that handles offset in rcv but not in parse
- bpf sockmap, that does not seem to handle offset anywhere
Calling pskb_pull() on the skb before parsing ensures that the offset
will be 0 everywhere in practice unless the user modifies it themselves
like tls, as a workaround for the other two protocols.
This fixes a bug whilch can be exhibited by implementing a simpe kcm
parser that looks for the packet size in the first word of the packet,
and sending two such packets in a single write() call on the other side:
the second message will be cut at the length of the first message.
Since this is a stream protocol, all the following messages will also
be corrupt since it will start looking for the next offset at a wrong
position.
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
Discussions on the bug along with a (bad) reproducer can be found here:
http://lkml.kernel.org/m/20180803182830.GB29193@nautica
(now the problem is better understood though it's much simpler to send
two messages at once than to spam and wait for tcp aggregation to do it)
Two notes:
- I've marked this patch v0 as we could move the pskb_pull() up to
where strp.offset is set, and just always leave it at 0 in the strparser
code.
This will let applications that are fine dealing with a non-zero offset
deal with it as they seem fit (tls writes into the offset and full_len
fields behind the back of the stream parser), while still being safe for
kcm/sockmap
- Even with that modification I'm not totally happy with
single-handedly eating the offset for strparser users which could handle
it, but I'm not really familiar with the cost this really has in
practice...
A better fix would be to handle the offset properly in the callbacks,
but frankly at least for kcm I don't see how (maybe because I'm not
familiar with how bpf programs work)
Another idea I had would be to write flags when registering the protocol
e.g. strp->cb.flags & STRP_CAN_PARSE_WITH_OFFSET or something like that,
but without an idea of the cost of that pull I don't know if it's worth
doing.
Anyway, comments welcome.
net/strparser/strparser.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index 625acb27efcc..d7a3b81c3481 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -222,6 +222,16 @@ static int __strp_recv(read_descriptor_t *desc, struct sk_buff *orig_skb,
if (!stm->strp.full_len) {
ssize_t len;
+ /* Can only parse if there is no offset */
+ if (unlikely(stm->strp.offset)) {
+ if (!pskb_pull(skb, stm->strp.offset)) {
+ STRP_STATS_INCR(strp->stats.mem_fail);
+ strp_parser_err(strp, -ENOMEM, desc);
+ break;
+ }
+ stm->strp.offset = 0;
+ }
+
len = (*strp->cb.parse_msg)(strp, head);
if (!len) {
@@ -249,8 +259,7 @@ static int __strp_recv(read_descriptor_t *desc, struct sk_buff *orig_skb,
STRP_STATS_INCR(strp->stats.msg_too_big);
strp_parser_err(strp, -EMSGSIZE, desc);
break;
- } else if (len <= (ssize_t)head->len -
- skb->len - stm->strp.offset) {
+ } else if (len <= (ssize_t)head->len - skb->len) {
/* Length must be into new skb (and also
* greater than zero)
*/
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf-next] bpf: enable btf for use in all maps
From: Daniel Borkmann @ 2018-08-09 22:43 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: ast, netdev, yhs
In-Reply-To: <20180809214430.gd4zwsnmbwmq7b26@ast-mbp>
On 08/09/2018 11:44 PM, Alexei Starovoitov wrote:
> On Thu, Aug 09, 2018 at 11:30:52PM +0200, Daniel Borkmann wrote:
>> On 08/09/2018 11:14 PM, Alexei Starovoitov wrote:
>>> On Thu, Aug 09, 2018 at 09:42:20PM +0200, Daniel Borkmann wrote:
>>>> Commit a26ca7c982cb ("bpf: btf: Add pretty print support to
>>>> the basic arraymap") enabled support for BTF and dumping via
>>>> BPF fs for arraymap. However, both can be decoupled from each
>>>> other such that all BPF maps can be supported for attaching
>>>> BTF key/value information, while not all maps necessarily
>>>> need to dump via map_seq_show_elem() callback.
>>>>
>>>> The check in array_map_check_btf() can be generalized as
>>>> ultimatively the key and value size is the only contraint
>>>> that needs to match for the map. The fact that the key needs
>>>> to be of type int is optional; it could be any data type as
>>>> long as it matches the 4 byte key size, just like hash table
>>>> key or others could be of any data type as well.
>>>>
>>>> Minimal example of a hash table dump which then works out
>>>> of the box for bpftool:
>>>>
>>>> # bpftool map dump id 19
>>>> [{
>>>> "key": {
>>>> "": {
>>>> "vip": 0,
>>>> "vipv6": []
>>>> },
>>>> "port": 0,
>>>> "family": 0,
>>>> "proto": 0
>>>> },
>>>> "value": {
>>>> "flags": 0,
>>>> "vip_num": 0
>>>> }
>>>> }
>>>> ]
>>>>
>>>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>>>> Cc: Yonghong Song <yhs@fb.com>
>>>> ---
>>>> include/linux/bpf.h | 4 +---
>>>> kernel/bpf/arraymap.c | 27 ---------------------------
>>>> kernel/bpf/inode.c | 3 ++-
>>>> kernel/bpf/syscall.c | 24 ++++++++++++++++++++----
>>>> 4 files changed, 23 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>>> index cd8790d..eb76e8e 100644
>>>> --- a/include/linux/bpf.h
>>>> +++ b/include/linux/bpf.h
>>>> @@ -48,8 +48,6 @@ struct bpf_map_ops {
>>>> u32 (*map_fd_sys_lookup_elem)(void *ptr);
>>>> void (*map_seq_show_elem)(struct bpf_map *map, void *key,
>>>> struct seq_file *m);
>>>> - int (*map_check_btf)(const struct bpf_map *map, const struct btf *btf,
>>>> - u32 key_type_id, u32 value_type_id);
>>>> };
>>>>
>>>> struct bpf_map {
>>>> @@ -118,7 +116,7 @@ static inline bool bpf_map_offload_neutral(const struct bpf_map *map)
>>>>
>>>> static inline bool bpf_map_support_seq_show(const struct bpf_map *map)
>>>> {
>>>> - return map->ops->map_seq_show_elem && map->ops->map_check_btf;
>>>> + return map->btf && map->ops->map_seq_show_elem;
>>>> }
>>>>
>>>> extern const struct bpf_map_ops bpf_map_offload_ops;
>>>> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
>>>> index 2aa55d030..67f0bdf 100644
>>>> --- a/kernel/bpf/arraymap.c
>>>> +++ b/kernel/bpf/arraymap.c
>>>> @@ -358,32 +358,6 @@ static void array_map_seq_show_elem(struct bpf_map *map, void *key,
>>>> rcu_read_unlock();
>>>> }
>>>>
>>>> -static int array_map_check_btf(const struct bpf_map *map, const struct btf *btf,
>>>> - u32 btf_key_id, u32 btf_value_id)
>>>> -{
>>>> - const struct btf_type *key_type, *value_type;
>>>> - u32 key_size, value_size;
>>>> - u32 int_data;
>>>> -
>>>> - key_type = btf_type_id_size(btf, &btf_key_id, &key_size);
>>>> - if (!key_type || BTF_INFO_KIND(key_type->info) != BTF_KIND_INT)
>>>> - return -EINVAL;
>>>> -
>>>> - int_data = *(u32 *)(key_type + 1);
>>>> - /* bpf array can only take a u32 key. This check makes
>>>> - * sure that the btf matches the attr used during map_create.
>>>> - */
>>>> - if (BTF_INT_BITS(int_data) != 32 || key_size != 4 ||
>>>> - BTF_INT_OFFSET(int_data))
>>>> - return -EINVAL;
>>>
>>> I think most of these checks are still necessary for array type.
>>> Relaxing BTF array key from BTF_KIND_INT to, for example, BTF_KIND_ENUM
>>> is probably ok, but key being BTF_KIND_PTR or BTF_KIND_ARRAY doesn't makes sense.
>>
>> Hmm, so on 64 bit archs BTF_KIND_PTR would get rejected for array,
>> on 32 bit it may be allowed due to sizeof(void *) == 4. BTF_KIND_ARRAY
>> could be array of u8 foo[4], for example, or u16 foo[2]. But how would
>> it ultimately be different from e.g. having 'struct a' versus 'struct b'
>> where both are of same size and while actual key has 'struct a', the one
>> who writes the prog resp. loads the BTF into the kernel would lie about
>> it stating it's of type 'struct b' instead? It's basically trusting the
>> app that it advertised sane key types which kernel is propagating back.
>
> for hash map - yes. the kernel cannot yet catch the lie that
> key == 'struct a' that user said in BTF is not what program used
> (which used 'struct b' of the same size).
> Eventually we will annotate all load/store in the program and will
> make sure that memory access match what BTF said.
But in that case, would you reject the program? E.g. from prog point of
view, it's just a buffer of x bytes, so key could be casted to different
struct/types potentially and used for lookup; similar with value if you
would go the route to annotate all access into it. I don't think this
serves as a security feature (since you might as well just load the prog
without BTF just fine), but it can be used to help verifier to perform
rewrites like in tracing for implicit bpf_probe_read() based on member
access. But also in that case, if you might have e.g. stale or wrong BTF
data e.g. of the whole kernel or some application it would follow/walk
that one instead. But such user error would be "acceptable" since it serves
as a hint, roughly similar to (explicitly) walking the data structures
based on the headers today, you do have better control in terms of header
mismatches in that you can ship the BTF directly from the build, but there's
still no guarantee in that sense that you "verified" that these bytes
originally were mapped to struct foo somewhere in a C program.
> For array we can catch the lie today that key is not 4 byte int,
> since it matters from pretty printing point of view.
> If it's PTR or ARRAY or STRUCT, the printer will go nuts.
In that case, would you enforce a hash map key size of 4 also to INT-only
instead of e.g. allowing STRUCT and various others? I don't think it's
that much different from pretty printer PoV (e.g. see bpftool transparently
handling them via btf_dumper_type()), it's some key with 4 bytes of memory
and a type matching these 4 bytes in size for pretty printer.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH net-next,v4] net/tls: Calculate nsg for zerocopy path without skb_cow_data.
From: Doron Roberts-Kedes @ 2018-08-09 22:43 UTC (permalink / raw)
To: David Miller; +Cc: davejwatson, vakul.garg, borisp, aviadye, netdev
In-Reply-To: <20180808.121430.2168057232319674612.davem@davemloft.net>
On Wed, Aug 08, 2018 at 12:14:30PM -0700, David Miller wrote:
> From: Doron Roberts-Kedes <doronrk@fb.com>
> Date: Tue, 7 Aug 2018 11:09:39 -0700
>
> > +static int __skb_nsg(struct sk_buff *skb, int offset, int len,
> > + unsigned int recursion_level)
> > +{
> > + int start = skb_headlen(skb);
> > + int i, copy = start - offset;
> > + struct sk_buff *frag_iter;
> > + int elt = 0;
> > +
> > + if (unlikely(recursion_level >= 24))
> > + return -EMSGSIZE;
>
> This recursion is kinda crazy.
>
> Even skb_cow_data() doesn't recurse like this (of course because it copies
> into linear buffers).
>
> There has to be a way to simplify this. Fragment lists are such a rarely
> used SKB geometry, and few if any devices support it for transmission
> (so the fraglist will get undone at transmit time anyways).
>
Interesting. Just wanted to clarify whether the issue is the use of
recursion or the fact that the function is handling the frag_list at
all. This is the rx path, so my understanding was that we need to handle
the frag_list. Please let me know if I'm misunderstanding your point
about the rare use of fragment lists.
If the issue is the recursion, I can rewrite the function to not use
recursion, but skb_to_sgvec uses a similar pattern and is invoked
immediately afterwards.
Taking a step back, is there an existing solution for what this function
is trying to do? I was surprised to find that there did not seem to
exist a function for determining the number of scatterlist elements
required to map an skb without COW.
^ permalink raw reply
* Re: [PATCH lora-next v2 8/8] net: lora: sx1301: convert driver over to regmap reads and writes
From: Ben Whitten @ 2018-08-09 22:47 UTC (permalink / raw)
To: Andreas Färber
Cc: 潘建宏, hasnain.virk, netdev, Xue Liu, shess,
Ben Whitten, yannick.lanz
In-Reply-To: <e7b112d9-d8ac-1654-fc97-9289180d6a02@suse.de>
On Thu, 9 Aug 2018 at 23:34, Andreas Färber <afaerber@suse.de> wrote:
>
> Am 09.08.2018 um 14:33 schrieb Ben Whitten:
> > The reads and writes are replaced with regmap versions and unneeded
> > functions, variable, and defines removed.
> >
> > Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
> > ---
> > drivers/net/lora/sx1301.c | 204 +++++++++++++++-------------------------------
> > drivers/net/lora/sx1301.h | 30 +++++++
> > 2 files changed, 95 insertions(+), 139 deletions(-)
> >
> > diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
> > index 766df06..4db5a43 100644
> > --- a/drivers/net/lora/sx1301.c
> > +++ b/drivers/net/lora/sx1301.c
> [...]
> > @@ -140,50 +115,9 @@ static int sx1301_write(struct sx1301_priv *priv, u8 reg, u8 val)
> > return sx1301_write_burst(priv, reg, &val, 1);
> > }
>
> _write and _read are now unused, causing warnings. Dropping.
>
> The _burst versions are still in use for firmware load, and I saw a
> discussion indicating that regmap is lacking the capability to not
> increment the reg for bulk reads at the moment. So we still can't
> cleanly switch to regmap entirely and thereby remain bound to SPI.
>
> [...]
> > @@ -235,8 +169,8 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
> > {
> > struct spi_sx1301 *ssx = spi_controller_get_devdata(ctrl);
> > struct sx1301_priv *priv = spi_get_drvdata(ssx->parent);
> > - const u8 *tx_buf = xfr->tx_buf;
> > - u8 *rx_buf = xfr->rx_buf;
> > + const unsigned int *tx_buf = xfr->tx_buf;
> > + unsigned int *rx_buf = xfr->rx_buf;
>
> These are wrong both for Little Endian and even worse for Big Endian.
>
> > int ret;
> >
> > if (xfr->len == 0 || xfr->len > 3)
> > @@ -245,13 +179,13 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
> > dev_dbg(&spi->dev, "transferring one (%u)\n", xfr->len);
> >
> > if (tx_buf) {
> > - ret = sx1301_page_write(priv, ssx->page, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0);
> > + ret = regmap_write(priv->regmap, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0);
> > if (ret) {
> > dev_err(&spi->dev, "SPI radio address write failed\n");
> > return ret;
> > }
> >
> > - ret = sx1301_page_write(priv, ssx->page, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
> > + ret = regmap_write(priv->regmap, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
> > if (ret) {
> > dev_err(&spi->dev, "SPI radio data write failed\n");
> > return ret;
> > @@ -271,7 +205,7 @@ static int sx1301_radio_spi_transfer_one(struct spi_controller *ctrl,
> > }
> >
> > if (rx_buf) {
> > - ret = sx1301_page_read(priv, ssx->page, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
> > + ret = regmap_read(priv->regmap, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
> > if (ret) {
> > dev_err(&spi->dev, "SPI radio data read failed\n");
> > return ret;
>
> Fixing by adding a local variable instead:
>
> @@ -239,6 +163,7 @@ static int sx1301_radio_spi_transfer_one(struct
> spi_controll
> er *ctrl,
> struct sx1301_priv *priv = netdev_priv(netdev);
> const u8 *tx_buf = xfr->tx_buf;
> u8 *rx_buf = xfr->rx_buf;
> + unsigned int val;
> int ret;
>
> if (xfr->len == 0 || xfr->len > 3)
> [...]
> @@ -273,27 +198,28 @@ static int sx1301_radio_spi_transfer_one(struct
> spi_controller *ctrl,
> }
>
> if (rx_buf) {
> - ret = sx1301_page_read(priv, ssx->page, ssx->regs +
> REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
> + ret = regmap_read(priv->regmap, ssx->regs +
> REG_RADIO_X_DATA_READBACK, &val);
> if (ret) {
> dev_err(&spi->dev, "SPI radio data read failed\n");
> return ret;
> }
> + rx_buf[xfr->len - 1] = val & 0xff;
> }
>
> return 0;
>
> [...]
> > diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
> > index 2fc283f..b21e5c6 100644
> > --- a/drivers/net/lora/sx1301.h
> > +++ b/drivers/net/lora/sx1301.h
> > @@ -18,11 +18,41 @@
> > /* Page independent */
> > #define SX1301_PAGE 0x00
> > #define SX1301_VER 0x01
> > +#define SX1301_MPA 0x09
>
> Those are the official register names? I find these much harder to read
> than my guessed names. Could we keep the long names as aliases?
Yes these are the official register names, aliases to improve readability
sound like a good plan as all the official names are terse.
> > +#define SX1301_MPD 0x0A
> > +#define SX1301_GEN 0x10
> > +#define SX1301_CKEN 0x11
> > +#define SX1301_GPSO 0x1C
> > +#define SX1301_GPMODE 0x1D
> > +#define SX1301_AGCSTS 0x20
> >
> > #define SX1301_VIRT_BASE 0x100
> > #define SX1301_PAGE_LEN 0x80
> > #define SX1301_PAGE_BASE(n) (SX1301_VIRT_BASE + (SX1301_PAGE_LEN * n))
> >
> > +/* Page 0 */
> > +#define SX1301_CHRS (SX1301_PAGE_BASE(0) + 0x23)
> > +#define SX1301_FORCE_CTRL (SX1301_PAGE_BASE(0) + 0x69)
> > +#define SX1301_MCU_CTRL (SX1301_PAGE_BASE(0) + 0x6A)
> > +
> > +/* Page 2 */
> > +#define SX1301_RADIO_A_SPI_DATA (SX1301_PAGE_BASE(2) + 0x21)
> > +#define SX1301_RADIO_A_SPI_DATA_RB (SX1301_PAGE_BASE(2) + 0x22)
> > +#define SX1301_RADIO_A_SPI_ADDR (SX1301_PAGE_BASE(2) + 0x23)
> > +#define SX1301_RADIO_A_SPI_CS (SX1301_PAGE_BASE(2) + 0x25)
> > +#define SX1301_RADIO_B_SPI_DATA (SX1301_PAGE_BASE(2) + 0x26)
> > +#define SX1301_RADIO_B_SPI_DATA_RB (SX1301_PAGE_BASE(2) + 0x27)
> > +#define SX1301_RADIO_B_SPI_ADDR (SX1301_PAGE_BASE(2) + 0x28)
> > +#define SX1301_RADIO_B_SPI_CS (SX1301_PAGE_BASE(2) + 0x2A)
> > +#define SX1301_RADIO_CFG (SX1301_PAGE_BASE(2) + 0x2B)
> > +#define SX1301_DBG_ARB_MCU_RAM_DATA (SX1301_PAGE_BASE(2) + 0x40)
> > +#define SX1301_DBG_AGC_MCU_RAM_DATA (SX1301_PAGE_BASE(2) + 0x41)
> > +#define SX1301_DBG_ARB_MCU_RAM_ADDR (SX1301_PAGE_BASE(2) + 0x50)
> > +#define SX1301_DBG_AGC_MCU_RAM_ADDR (SX1301_PAGE_BASE(2) + 0x51)
> > +
> > +/* Page 3 */
> > +#define SX1301_EMERGENCY_FORCE_HOST_CTRL (SX1301_PAGE_BASE(3) + 0x7F)
> > +
> > #define SX1301_MAX_REGISTER (SX1301_PAGE_BASE(3) + 0x7F)
> >
> > #endif
>
> Applying so that we can continue based on regmap.
Thanks!
Ben Whitten
^ 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