* Re: [RESEND PATCH v2 0/5] net: phy: mscc: add support for VSC8584 and VSC8574 Microsemi quad-port PHYs
From: Paul Burton @ 2018-10-09 17:59 UTC (permalink / raw)
To: Quentin Schulz
Cc: alexandre.belloni@bootlin.com, ralf@linux-mips.org,
jhogan@kernel.org, robh+dt@kernel.org, mark.rutland@arm.com,
davem@davemloft.net, andrew@lunn.ch, f.fainelli@gmail.com,
allan.nielsen@microchip.com, linux-mips@linux-mips.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, thomas.petazzoni@bootlin.com,
antoine.tenart@bootlin.com
In-Reply-To: <20181008101445.25946-1-quentin.schulz@bootlin.com>
Hi Quentin,
On Mon, Oct 08, 2018 at 12:14:40PM +0200, Quentin Schulz wrote:
> RESEND: rebased on top of latest net-next and on top of latest version of
> "net: phy: mscc: various improvements to Microsemi PHY driver" patch
> series.
>
>%
>
> I suggest patches 1 to 3 go through net tree and patches 4 and 5 go
> through MIPS tree. Patches going through net tree and those going through
> MIPS tree do not depend on one another.
Patches 4 & 5 applied to mips-next for 4.20, thanks!
Paul
^ permalink raw reply
* Re: re iproute2 - don't return error on success fix
From: Or Gerlitz @ 2018-10-09 10:54 UTC (permalink / raw)
To: Phil Sutter, Stephen Hemminger, David Ahern, Linux Netdev List,
Roi Dayan
In-Reply-To: <20181008124141.GB14666@orbyte.nwl.cc>
On Mon, Oct 8, 2018 at 3:41 PM Phil Sutter <phil@nwl.cc> wrote:
> > $ ./tc/tc filter add dev enp33s0f0 protocol ip parent ffff: flower
> > skip_sw ip_flags firstfrag action drop && echo "success" || echo
> > "failed"
> > RTNETLINK answers: Operation not supported
> > success
>
> Interestingly, your output lacks the "We have an error" message mine
> shows. So in your case, the call to rtnl_talk_iov() from
> tc_filter_modify() does not return < 0.
>
> In my case, commands always fail with 'skip_sw' since I only test on a
> dummy interface. So maybe we hit different error paths? Could you please
> check what happens for you in __rtnl_talk_iov()? I guess the "RTNETLINK
> answers:" message should come from rtnl_talk_error(). In my case it does
> at least, and I haven't found an alternative place where that message
> could come from.
I have applied the patch Vlad sent today and it solved the problem:
# ./tc/tc filter add dev enp33s0f0 protocol ip parent ffff: flower
skip_sw ip_flags firstfrag action drop && echo "success" || echo
"failed"
RTNETLINK answers: Operation not supported
We have an error talking to the kernel, -1
failed
^ permalink raw reply
* Re: [PATCH v3 1/2] Driver core: add bus_find_device_by_fwnode
From: Wolfram Sang @ 2018-10-09 11:02 UTC (permalink / raw)
To: Silesh C V
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
linux-arm-kernel, linux-i2c, linux-rdma, netdev, devicetree,
linux-spi, Mathieu Poirier, Lijun Ou, Wei Hu(Xavier),
Yisen Zhuang, Salil Mehta, Srinivas Kandagatla, Andrew Lunn,
Florian Fainelli, Rob Herring, Frank Rowand, Mark Brown,
David S. Miller
In-Reply-To: <1539080245-25818-1-git-send-email-svellattu@mvista.com>
[-- Attachment #1: Type: text/plain, Size: 822 bytes --]
On Tue, Oct 09, 2018 at 03:47:24PM +0530, Silesh C V wrote:
> Some drivers need to find the device on a bus having a specific firmware
> node. Currently, such drivers have their own implementations to do this.
> Provide a helper similar to bus_find_device_by_name so that each driver
> does not have to reinvent this.
>
> Signed-off-by: Silesh C V <svellattu@mvista.com>
Looks good in general, however:
We recently had this discussion in I2C world about using the parent if
the (logical) device has a NULL fw_node [1]. I don't know if the other
subsystems you modify use logical devices as well? If no, it seems we
need an additional check for the parent in the I2C core only. If yes,
this might be considered in your patchset?
Thanks,
Wolfram
[1] http://patchwork.ozlabs.org/patch/974584/
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] dt-bindings: Add bindings for aliases node
From: Brian Norris @ 2018-10-09 18:31 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Matthias Kaehlcke, Rob Herring, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, linux-wireless, linux-spi, netdev,
swboyd, Florian Fainelli
In-Reply-To: <CAMuHMdWe5JYp5eC2PvuG4xEud908Crhm+BhrtxPJ4Uo1-LCc4g@mail.gmail.com>
On Tue, Oct 09, 2018 at 09:22:07AM +0200, Geert Uytterhoeven wrote:
> Please note these aliases become cumbersome once you start considering
> (dynamic) DT overlays. That's why I made them optional in the sh-sci
> serial driver, cfr. commit 7678f4c20fa7670f ("serial: sh-sci: Add support
> for dynamic instances").
Note that as I understand it, the entire point of documenting this sort
of thing is to help solidify the interface between a DT aware boot
program (e.g., bootloader) and a device tree which is provided
separately, to avoid memorizing node/path hierarchy. It doesn't need to
(and doesn't, as I read it) enforce an OS's device naming policy.
> Relevant parts of the commit description are:
>
> On DT platforms, the sh-sci driver requires the presence of "serialN"
> aliases in DT, from which instance IDs are derived. If a DT alias is
> missing, the drivers fails to probe the corresponding serial port.
>
> This becomes cumbersome when considering DT overlays, as currently
> there is no upstream support for dynamically updating the /aliases node
> in DT.
That part is not a DT spec problem :)
> Furthermore, even in the presence of such support, hardcoded
> instance IDs in independent overlays are prone to conflicts.
>
> Hence add support for dynamic instance IDs, to be used in the absence of
> a DT alias. This makes serial ports behave similar to I2C and SPI
> buses, which already support dynamic instances.
This seems to be a much different sort of problem. People always love
having predictable IDs given by the OS (myself included), but that's
just plain hard to do and impossible in some cases. I don't think that's
what this document is about though.
IOW, this document seems pretty consistent with the above: it doesn't
require the usage of aliases (and it seems silly to have a driver
*require* an alias) -- it just documents how one should name such an
alias if you expect multiple independent software components to
understand it.
> To clarify my point: R-Car M2-W has 4 different types of serial ports, for a
> total of 18 ports, and the two ports on a board labeled 0 and 1 may not
> correspond to the physical first two ports (what's "first" in a collection of
> 4 different types?).
>
> Aliases may be fine for referring to the main serial console (labeled
> port 0 on the device, too), and the primary Ethernet interface (so U-Boot
> knows where to add the "local-mac-address" property), but beyond that,
> I think they should be avoided.
That's fair enough. Just because the solution isn't an all-purpose tool
doesn't mean it shouldn't be documented. The general concept is already
in ePAPR, but it's just not very specific about property names.
> Just my two^H^H^Hfive €c.
Thanks,
Brian
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v3 net-next] ravb: do not write 1 to reserved bits
From: Simon Horman @ 2018-10-09 11:53 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Sergei Shtylyov, David S. Miller, Magnus Damm, netdev,
Linux-Renesas
In-Reply-To: <CAMuHMdU_6uBpHOFshgxc61GOG1o1ibAoi5T1n_ZdYZ+=i8H5Sw@mail.gmail.com>
On Tue, Oct 09, 2018 at 11:28:40AM +0200, Geert Uytterhoeven wrote:
> Hi Sergei,
>
> On Tue, Sep 18, 2018 at 6:55 PM Sergei Shtylyov
> <sergei.shtylyov@cogentembedded.com> wrote:
> > On 09/18/2018 01:22 PM, Simon Horman wrote:
> > > From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> > > EtherAVB hardware requires 0 to be written to status register bits in
> > > order to clear them, however, care must be taken not to:
> > >
> > > 1. Clear other bits, by writing zero to them
> > > 2. Write one to reserved bits
> > >
> > > This patch corrects the ravb driver with respect to the second point above.
> > > This is done by defining reserved bit masks for the affected registers and,
> > > after auditing the code, ensure all sites that may write a one to a
> > > reserved bit use are suitably masked.
> > >
> > > Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > [...]
> >
> > Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >
> > > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> > > index 1470fc12282b..9b6bf557a2f5 100644
> > > --- a/drivers/net/ethernet/renesas/ravb.h
> > > +++ b/drivers/net/ethernet/renesas/ravb.h
> > > @@ -428,6 +428,7 @@ enum EIS_BIT {
> > > EIS_CULF1 = 0x00000080,
> > > EIS_TFFF = 0x00000100,
> > > EIS_QFS = 0x00010000,
> > > + EIS_RESERVED = (GENMASK(31, 17) | GENMASK(15, 11)),
> >
> > Well, I'm not a big fan of BIT() and GENMASK() -- they still lack a macro
> > to #define a bit/field value. But if you prefer to use them, OK, let's be so...
>
> FIELD_PREP()?
>
> Perhaps the other bit definitions should be converted to BIT()?
> That way it becomes much easier to match valid EIS_* bits with EIS_RESERVED.
+1
^ permalink raw reply
* Re: [PATCH v8 10/15] octeontx2-af: Reconfig MSIX base with IOVA
From: Arnd Bergmann @ 2018-10-09 12:00 UTC (permalink / raw)
To: Sunil Kovvuri; +Cc: Networking, David Miller, linux-soc, gakula, sgoutham
In-Reply-To: <CA+sq2Cc_oxj6cexz1XuJ02vSvPEdjxRcetig5Z0tceHvEWWNig@mail.gmail.com>
On Tue, Oct 9, 2018 at 11:20 AM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote:
>
> On Tue, Oct 9, 2018 at 1:27 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tue, Oct 9, 2018 at 9:03 AM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote:
> > > On Mon, Oct 8, 2018 at 5:38 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > On Sun, Oct 7, 2018 at 5:01 PM <sunil.kovvuri@gmail.com> wrote:
> > I think if you enable CONFIG_DEBUG_VIRTUAL, the virt_to_page()
> > above should trigger a warning in
> >
> > phys_addr_t __virt_to_phys(unsigned long x)
> > {
> > WARN(!__is_lm_address(x),
> > "virt_to_phys used for non-linear address: %pK (%pS)\n",
> > (void *)x,
> > (void *)x);
> >
> > return __virt_to_phys_nodebug(x);
> > }
> >
> > Can you verify that?
> >
> > Arnd
>
> No, it isn't, as for 64bit systems CONFIG_SPARSEMEM_VMEMMAP is enabled.
But that is also user-selectable, right? It still seems to be really
fragile to rely on non-documented behavior of virt_to_phys()
here, if that only works for some configurations.
> But is there any alternative to what is being done ?
I'm not completely sure, but there may be a way to do this correctly
using the iommu API instead of the dma-mapping API. What you do
here is fairly rare, but not unprecedented.
Arnd
^ permalink raw reply
* Re: [PATCH] ath10k: htt_rx: Fix signedness bug in ath10k_update_per_peer_tx_stats
From: Gustavo A. R. Silva @ 2018-10-09 19:19 UTC (permalink / raw)
To: Anilkumar Kolli
Cc: Ben Greear, Kalle Valo, David S. Miller, ath10k, linux-wireless,
netdev, linux-kernel, linux-wireless-owner
In-Reply-To: <af44d03892b3d51df391760af7af0382@codeaurora.org>
>
> I have sent a patch to address this,
> https://patchwork.kernel.org/patch/10611943/
>
That's great.
Thanks for letting me know. :)
--
Gustavo
^ permalink raw reply
* Re: [PATCH net-next] rxrpc: Remove set but not used variable 'ioc'
From: YueHaibing @ 2018-10-09 12:04 UTC (permalink / raw)
To: David Howells; +Cc: davem, linux-afs, netdev, kernel-janitors
In-Reply-To: <11853.1539079996@warthog.procyon.org.uk>
On 2018/10/9 18:13, David Howells wrote:
> YueHaibing <yuehaibing@huawei.com> wrote:
>
>> net/rxrpc/output.c: In function 'rxrpc_reject_packets':
>> net/rxrpc/output.c:527:11: warning:
>> variable 'ioc' set but not used [-Wunused-but-set-variable]
>>
>> It never used since introduction in
>
> I wonder why my compiler doesn't show this warning.
Just use make W=1
>
> Anyway, NAK: just removing the variable is the wrong fix - you need to look at
> the code more closely. The actual fix is to pass it to kernel_sendmsg()
> instead of 2.
I didn't notice this, Thank you for correction.
>
> But thanks anyway! Do you want to respin your patch?
Sure, I will fix it.
>
>> commit ece64fec164f ("rxrpc: Emit BUSY packets when supposed to rather than ABORTs")
>
> Btw, this should be a 'Fixes: <commit> ("subject")' line and the patch needs
> to go to net, not net-next.
>
> David
>
> .
>
^ permalink raw reply
* icmp redirect sent when an active ipsec child_sa exist
From: Marco Berizzi @ 2018-10-09 12:09 UTC (permalink / raw)
To: netdev
Hi Folks,
I'm running strongSwan on Slackware with linux 4.18
This is the output from 'ip route show':
default via isp_router dev eth0 metric 1
10.0.0.0/8 via 10.68.63.3 dev eth1
10.68.63.0/26 dev eth1 proto kernel scope link src 10.68.63.62
127.0.0.0/8 dev lo scope link
172.16.0.0/12 via 10.68.63.3 dev eth1
192.168.0.0/16 via 10.68.63.3 dev eth1
and 'ip a s':
3: eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 [trimmed]
link/ether fc:4d:d4:2e:dc:6a brd ff:ff:ff:ff:ff:ff
inet 10.68.63.62/26 brd 10.68.63.63 scope global eth1
valid_lft forever preferred_lft forever
Connected to the eth1 there is also a 10.68.63.55 host:
the default gateway for this host is 10.68.63.62
This is the relevant configuration from stronSwan:
children {
networks {
local_ts = 10.68.63.54/31
remote_ts = 10.82.43.0/24
start_action = trap
}
}
The value trap installs a trap policy, which is going
to trigger the tunnel as soon as matching traffic will
be detected.
When I issue a ping from 10.68.63.55 host to a 10.82.43.0/24
network's host, I get this behaviour:
12:24:40.219185 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
10.68.63.55 > 10.82.43.10: ICMP echo request, id 12199, seq 43, length 64
12:24:41.235072 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
10.68.63.55 > 10.82.43.10: ICMP echo request, id 12199, seq 44, length 64
12:24:42.251095 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
10.68.63.55 > 10.82.43.10: ICMP echo request, id 12199, seq 45, length 64
12:24:43.267007 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
10.68.63.55 > 10.82.43.10: ICMP echo request, id 12199, seq 46, length 64
12:24:44.282948 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
10.68.63.55 > 10.82.43.10: ICMP echo request, id 12199, seq 47, length 64
12:24:45.298873 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
10.68.63.55 > 10.82.43.10: ICMP echo request, id 12199, seq 48, length 64
12:24:46.314840 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
10.68.63.55 > 10.82.43.10: ICMP echo request, id 12199, seq 49, length 64
12:24:47.330767 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
10.68.63.55 > 10.82.43.10: ICMP echo request, id 12199, seq 50, length 64
12:24:48.346708 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
10.68.63.55 > 10.82.43.10: ICMP echo request, id 12199, seq 51, length 64
12:24:49.362627 IP (tos 0x0, ttl 64, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
10.68.63.55 > 10.82.43.10: ICMP echo request, id 12199, seq 52, length 64
12:24:49.362686 IP (tos 0xc0, ttl 64, id 11631, offset 0, flags [none], proto ICMP (1), length 112)
10.68.63.62 > 10.68.63.55: ICMP redirect 10.82.43.10 to host 10.68.63.3, length 92
IP (tos 0x0, ttl 63, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
10.68.63.55 > 10.82.43.10: ICMP echo request, id 12199, seq 52, length 64
12:24:50.379051 IP (tos 0x0, ttl 63, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
10.68.63.55 > 10.82.43.10: ICMP echo request, id 12199, seq 53, length 64
12:24:50.379113 IP (tos 0xc0, ttl 64, id 12190, offset 0, flags [none], proto ICMP (1), length 112)
10.68.63.62 > 10.68.63.55: ICMP redirect 10.82.43.10 to host 10.68.63.3, length 92
IP (tos 0x0, ttl 62, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
10.68.63.55 > 10.82.43.10: ICMP echo request, id 12199, seq 53, length 64
12:24:51.398707 IP (tos 0x0, ttl 63, id 0, offset 0, flags [DF], proto ICMP (1), length 84)
10.68.63.55 > 10.82.43.10: ICMP echo request, id 12199, seq 54, length 64
Linux is sending those icmp redirects *after* the
CHILD_SA has been established. When the CHILD_SA
is not yet established linux will not send any icmp
redirect.
Is this the expected behavior?
Obviously if I remove the following static route:
10.0.0.0/8 via 10.68.63.3 dev eth1
linux will not send anymore the icmp redirect.
Any feedback are welcome.
Thanks in advance
^ permalink raw reply
* [PATCH v2 lora-next 0/4] migration to regmap and clk framework
From: Ben Whitten @ 2018-10-09 12:52 UTC (permalink / raw)
To: afaerber; +Cc: starnight, hasnain.virk, netdev, liuxuenetmail, shess,
Ben Whitten
This series completes the sx1301 migration to regmap by replaceing the SPI
burst read and write with regmap raw reads and writes, this solution is a work
around until there is regmap_noinc_read/write API merged as it sepends on the
fact that we have the regcache turned off.
regmap_noinc_read is available in 4.19 however a write varient isn't.
The various fields of the sx1301 are converted to regmap to pass the read
modify write down to the regmap layer and reduce our code.
The downside is that in the cases of the hand optimised multiple field writes
at once within one register, however if it is desirable these can be replaced
with regmap_update_bits.
Apply the same regmap treatment for the radio device and allow it to register as
a clock provider, this clock is enabled when the lora link is brought up as it
is required prior to calibration of the concentrator.
modprobe lora-sx125x
lora-dev: init
sx1301 spi0.0: SX1301 module probed
sx125x_con spi0.0-a: SX125x version: 21
sx125x_con spi0.0-a: SX125x module probed
sx125x_con spi0.0-b: SX125x version: 21
sx125x_con spi0.0-b: registering clkout
sx125x_con spi0.0-b: SX125x module probed
ip link set lora0 up
sx125x_con spi0.0-b: enabling clkout
Ben Whitten (4):
net: lora: sx1301: convert burst spi functions to regmap raw
net: lora: sx1301: convert to using regmap fields for bit ops
net: lora: sx125x: convert to regmap fields
net: lora: sx125x sx1301: allow radio to register as a clk provider
drivers/net/lora/sx125x.c | 159 ++++++++++++++++++++++++++--
drivers/net/lora/sx1301.c | 265 ++++++++++++++++------------------------------
drivers/net/lora/sx1301.h | 50 ++++++++-
3 files changed, 287 insertions(+), 187 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH v2 lora-next 1/4] net: lora: sx1301: convert burst spi functions to regmap raw
From: Ben Whitten @ 2018-10-09 12:52 UTC (permalink / raw)
To: afaerber; +Cc: starnight, hasnain.virk, netdev, liuxuenetmail, shess,
Ben Whitten
In-Reply-To: <1539089532-2481-1-git-send-email-ben.whitten@lairdtech.com>
As we have caching disabled we can access the regmap using raw for our
firmware reading and writing bursts.
We also remove the now defunct spi element from the structure as this
completes the move to regmap.
Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com>
---
drivers/net/lora/sx1301.c | 26 +++++++++++++++++---------
drivers/net/lora/sx1301.h | 2 --
2 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index fd29258..5ab0e2d 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -76,19 +76,28 @@ static struct regmap_config sx1301_regmap_config = {
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(priv->spi, &addr, 1, val, len);
+ size_t max;
+
+ max = regmap_get_raw_read_max(priv->regmap);
+ if (max && max < len) {
+ dev_err(priv->dev, "Burst greater then max raw read\n");
+ return -EINVAL;
+ }
+
+ return regmap_raw_read(priv->regmap, reg, val, 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] = {
- { .tx_buf = &addr, .len = 1 },
- { .tx_buf = val, .len = len },
- };
+ size_t max;
+
+ max = regmap_get_raw_write_max(priv->regmap);
+ if (max && max < len) {
+ dev_err(priv->dev, "Burst greater then max raw write\n");
+ return -EINVAL;
+ }
- return spi_sync_transfer(priv->spi, xfr, 2);
+ return regmap_raw_write(priv->regmap, reg, val, len);
}
static int sx1301_soft_reset(struct sx1301_priv *priv)
@@ -566,7 +575,6 @@ static int sx1301_probe(struct spi_device *spi)
spi_set_drvdata(spi, netdev);
priv->dev = &spi->dev;
- priv->spi = spi;
priv->regmap = devm_regmap_init_spi(spi, &sx1301_regmap_config);
if (IS_ERR(priv->regmap)) {
diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
index e939c02..e6400f8 100644
--- a/drivers/net/lora/sx1301.h
+++ b/drivers/net/lora/sx1301.h
@@ -12,7 +12,6 @@
#include <linux/regmap.h>
#include <linux/gpio/consumer.h>
#include <linux/lora/dev.h>
-#include <linux/spi/spi.h>
#define SX1301_CHIP_VERSION 103
@@ -64,7 +63,6 @@
struct sx1301_priv {
struct lora_dev_priv lora;
struct device *dev;
- struct spi_device *spi;
struct gpio_desc *rst_gpio;
struct regmap *regmap;
};
--
2.7.4
^ permalink raw reply related
* [PATCH v2 lora-next 2/4] net: lora: sx1301: convert to using regmap fields for bit ops
From: Ben Whitten @ 2018-10-09 12:52 UTC (permalink / raw)
To: afaerber; +Cc: starnight, hasnain.virk, netdev, liuxuenetmail, shess,
Ben Whitten
In-Reply-To: <1539089532-2481-1-git-send-email-ben.whitten@lairdtech.com>
From: Ben Whitten <ben.whitten@gmail.com>
We convert to using regmap fields to allow bit access to the registers
where regmap handles the read update write.
Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
drivers/net/lora/sx1301.c | 240 +++++++++++++---------------------------------
drivers/net/lora/sx1301.h | 46 +++++++++
2 files changed, 113 insertions(+), 173 deletions(-)
diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index 5ab0e2d..eb89af6 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -24,27 +24,6 @@
#include "sx1301.h"
-#define REG_PAGE_RESET_SOFT_RESET BIT(7)
-
-#define REG_16_GLOBAL_EN BIT(3)
-
-#define REG_17_CLK32M_EN BIT(0)
-
-#define REG_0_105_FORCE_HOST_RADIO_CTRL BIT(1)
-#define REG_0_105_FORCE_HOST_FE_CTRL BIT(2)
-#define REG_0_105_FORCE_DEC_FILTER_GAIN BIT(3)
-
-#define REG_0_MCU_RST_0 BIT(0)
-#define REG_0_MCU_RST_1 BIT(1)
-#define REG_0_MCU_SELECT_MUX_0 BIT(2)
-#define REG_0_MCU_SELECT_MUX_1 BIT(3)
-
-#define REG_2_43_RADIO_A_EN BIT(0)
-#define REG_2_43_RADIO_B_EN BIT(1)
-#define REG_2_43_RADIO_RST BIT(2)
-
-#define REG_EMERGENCY_FORCE_HOST_CTRL BIT(0)
-
static const struct regmap_range_cfg sx1301_regmap_ranges[] = {
{
.name = "Pages",
@@ -74,6 +53,12 @@ static struct regmap_config sx1301_regmap_config = {
.max_register = SX1301_MAX_REGISTER,
};
+static int sx1301_field_write(struct sx1301_priv *priv,
+ enum sx1301_fields field_id, u8 val)
+{
+ return regmap_field_write(priv->regmap_fields[field_id], val);
+}
+
static int sx1301_read_burst(struct sx1301_priv *priv, u8 reg, u8 *val, size_t len)
{
size_t max;
@@ -100,11 +85,6 @@ static int sx1301_write_burst(struct sx1301_priv *priv, u8 reg, const u8 *val, s
return regmap_raw_write(priv->regmap, reg, val, len);
}
-static int sx1301_soft_reset(struct sx1301_priv *priv)
-{
- return regmap_write(priv->regmap, SX1301_PAGE, REG_PAGE_RESET_SOFT_RESET);
-}
-
static int sx1301_agc_ram_read(struct sx1301_priv *priv, u8 addr, unsigned int *val)
{
int ret;
@@ -146,7 +126,7 @@ static int sx1301_arb_ram_read(struct sx1301_priv *priv, u8 addr, unsigned int *
static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct firmware *fw)
{
u8 *buf;
- u8 rst, select_mux;
+ enum sx1301_fields rst, select_mux;
unsigned int val;
int ret;
@@ -157,29 +137,26 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
switch (mcu) {
case 0:
- rst = REG_0_MCU_RST_0;
- select_mux = REG_0_MCU_SELECT_MUX_0;
+ rst = F_MCU_RST_0;
+ select_mux = F_MCU_SELECT_MUX_0;
break;
case 1:
- rst = REG_0_MCU_RST_1;
- select_mux = REG_0_MCU_SELECT_MUX_1;
+ rst = F_MCU_RST_1;
+ select_mux = F_MCU_SELECT_MUX_1;
break;
default:
return -EINVAL;
}
- ret = regmap_read(priv->regmap, SX1301_MCU_CTRL, &val);
+ ret = sx1301_field_write(priv, rst, 1);
if (ret) {
- dev_err(priv->dev, "MCU read failed\n");
+ dev_err(priv->dev, "MCU reset failed\n");
return ret;
}
- val |= rst;
- val &= ~select_mux;
-
- ret = regmap_write(priv->regmap, SX1301_MCU_CTRL, val);
+ ret = sx1301_field_write(priv, select_mux, 0);
if (ret) {
- dev_err(priv->dev, "MCU reset / select mux write failed\n");
+ dev_err(priv->dev, "MCU RAM select mux failed\n");
return ret;
}
@@ -220,17 +197,9 @@ static int sx1301_load_firmware(struct sx1301_priv *priv, int mcu, const struct
kfree(buf);
- ret = regmap_read(priv->regmap, SX1301_MCU_CTRL, &val);
+ ret = sx1301_field_write(priv, select_mux, 1);
if (ret) {
- dev_err(priv->dev, "MCU read (1) failed\n");
- return ret;
- }
-
- val |= select_mux;
-
- ret = regmap_write(priv->regmap, SX1301_MCU_CTRL, val);
- if (ret) {
- dev_err(priv->dev, "MCU reset / select mux write (1) failed\n");
+ dev_err(priv->dev, "MCU RAM release mux failed\n");
return ret;
}
@@ -256,17 +225,9 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
return ret;
}
- ret = regmap_read(priv->regmap, SX1301_FORCE_CTRL, &val);
+ ret = sx1301_field_write(priv, F_FORCE_HOST_RADIO_CTRL, 0);
if (ret) {
- dev_err(priv->dev, "0|105 read failed\n");
- return ret;
- }
-
- val &= ~REG_0_105_FORCE_HOST_RADIO_CTRL;
-
- ret = regmap_write(priv->regmap, SX1301_FORCE_CTRL, val);
- if (ret) {
- dev_err(priv->dev, "0|105 write failed\n");
+ dev_err(priv->dev, "force host control failed\n");
return ret;
}
@@ -280,17 +241,9 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
return ret;
}
- ret = regmap_read(priv->regmap, SX1301_MCU_CTRL, &val);
+ ret = sx1301_field_write(priv, F_MCU_RST_1, 0);
if (ret) {
- dev_err(priv->dev, "MCU read (0) failed\n");
- return ret;
- }
-
- val &= ~REG_0_MCU_RST_1;
-
- ret = regmap_write(priv->regmap, SX1301_MCU_CTRL, val);
- if (ret) {
- dev_err(priv->dev, "MCU write (0) failed\n");
+ dev_err(priv->dev, "MCU 1 reset failed\n");
return ret;
}
@@ -308,34 +261,18 @@ static int sx1301_agc_calibrate(struct sx1301_priv *priv)
return -ENXIO;
}
- ret = regmap_read(priv->regmap, SX1301_EMERGENCY_FORCE_HOST_CTRL, &val);
+ ret = sx1301_field_write(priv, F_EMERGENCY_FORCE_HOST_CTRL, 0);
if (ret) {
- dev_err(priv->dev, "emergency force read failed\n");
- return ret;
- }
-
- val &= ~REG_EMERGENCY_FORCE_HOST_CTRL;
-
- ret = regmap_write(priv->regmap, SX1301_EMERGENCY_FORCE_HOST_CTRL, val);
- if (ret) {
- dev_err(priv->dev, "emergency force write failed\n");
+ dev_err(priv->dev, "emergency force failed\n");
return ret;
}
dev_err(priv->dev, "starting calibration...\n");
msleep(2300);
- ret = regmap_read(priv->regmap, SX1301_EMERGENCY_FORCE_HOST_CTRL, &val);
- if (ret) {
- dev_err(priv->dev, "emergency force read (1) failed\n");
- return ret;
- }
-
- val |= REG_EMERGENCY_FORCE_HOST_CTRL;
-
- ret = regmap_write(priv->regmap, SX1301_EMERGENCY_FORCE_HOST_CTRL, val);
+ ret = sx1301_field_write(priv, F_EMERGENCY_FORCE_HOST_CTRL, 1);
if (ret) {
- dev_err(priv->dev, "emergency force write (1) failed\n");
+ dev_err(priv->dev, "emergency force release failed\n");
return ret;
}
@@ -382,19 +319,15 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
if (ret)
return ret;
- ret = regmap_read(priv->regmap, SX1301_FORCE_CTRL, &val);
- if (ret) {
- dev_err(priv->dev, "0|105 read failed\n");
+ ret = sx1301_field_write(priv, F_FORCE_HOST_RADIO_CTRL, 0);
+ if (ret)
return ret;
- }
-
- val &= ~(REG_0_105_FORCE_HOST_RADIO_CTRL | REG_0_105_FORCE_HOST_FE_CTRL | REG_0_105_FORCE_DEC_FILTER_GAIN);
-
- ret = regmap_write(priv->regmap, SX1301_FORCE_CTRL, val);
- if (ret) {
- dev_err(priv->dev, "0|105 write failed\n");
+ ret = sx1301_field_write(priv, F_FORCE_HOST_FE_CTRL, 0);
+ if (ret)
+ return ret;
+ ret = sx1301_field_write(priv, F_FORCE_DEC_FILTER_GAIN, 0);
+ if (ret)
return ret;
- }
ret = regmap_write(priv->regmap, SX1301_CHRS, 0);
if (ret) {
@@ -402,17 +335,15 @@ static int sx1301_load_all_firmware(struct sx1301_priv *priv)
return ret;
}
- ret = regmap_read(priv->regmap, SX1301_MCU_CTRL, &val);
+ ret = sx1301_field_write(priv, F_MCU_RST_0, 0);
if (ret) {
- dev_err(priv->dev, "MCU read (0) failed\n");
+ dev_err(priv->dev, "MCU 0 release failed\n");
return ret;
}
- val &= ~(REG_0_MCU_RST_1 | REG_0_MCU_RST_0);
-
- ret = regmap_write(priv->regmap, SX1301_MCU_CTRL, val);
+ ret = sx1301_field_write(priv, F_MCU_RST_1, 0);
if (ret) {
- dev_err(priv->dev, "MCU write (0) failed\n");
+ dev_err(priv->dev, "MCU 1 release failed\n");
return ret;
}
@@ -464,7 +395,6 @@ static netdev_tx_t sx130x_loradev_start_xmit(struct sk_buff *skb, struct net_dev
static int sx130x_loradev_open(struct net_device *netdev)
{
struct sx1301_priv *priv = netdev_priv(netdev);
- unsigned int val;
int ret;
netdev_dbg(netdev, "%s", __func__);
@@ -474,31 +404,15 @@ static int sx130x_loradev_open(struct net_device *netdev)
return -ENXIO;
}
- ret = regmap_read(priv->regmap, SX1301_GEN, &val);
+ ret = sx1301_field_write(priv, F_GLOBAL_EN, 1);
if (ret) {
- netdev_err(netdev, "16 read (1) failed\n");
+ dev_err(priv->dev, "enable global clocks failed\n");
return ret;
}
- val |= REG_16_GLOBAL_EN;
-
- ret = regmap_write(priv->regmap, SX1301_GEN, val);
+ ret = sx1301_field_write(priv, F_CLK32M_EN, 1);
if (ret) {
- netdev_err(netdev, "16 write (1) failed\n");
- return ret;
- }
-
- ret = regmap_read(priv->regmap, SX1301_CKEN, &val);
- if (ret) {
- netdev_err(netdev, "17 read (1) failed\n");
- return ret;
- }
-
- val |= REG_17_CLK32M_EN;
-
- ret = regmap_write(priv->regmap, SX1301_CKEN, val);
- if (ret) {
- netdev_err(netdev, "17 write (1) failed\n");
+ dev_err(priv->dev, "enable 32M clock failed\n");
return ret;
}
@@ -545,6 +459,7 @@ static int sx1301_probe(struct spi_device *spi)
struct sx1301_priv *priv;
struct gpio_desc *rst;
int ret;
+ int i;
unsigned int ver;
unsigned int val;
@@ -583,6 +498,19 @@ static int sx1301_probe(struct spi_device *spi)
return ret;
}
+ for (i = 0; i < ARRAY_SIZE(sx1301_regmap_fields); i++) {
+ const struct reg_field *reg_fields = sx1301_regmap_fields;
+
+ priv->regmap_fields[i] = devm_regmap_field_alloc(&spi->dev,
+ priv->regmap,
+ reg_fields[i]);
+ if (IS_ERR(priv->regmap_fields[i])) {
+ ret = PTR_ERR(priv->regmap_fields[i]);
+ dev_err(&spi->dev, "Cannot allocate regmap field: %d\n", ret);
+ return ret;
+ }
+ }
+
ret = regmap_read(priv->regmap, SX1301_VER, &ver);
if (ret) {
dev_err(&spi->dev, "version read failed\n");
@@ -600,83 +528,49 @@ static int sx1301_probe(struct spi_device *spi)
return ret;
}
- ret = sx1301_soft_reset(priv);
+ ret = sx1301_field_write(priv, F_SOFT_RESET, 1);
if (ret) {
dev_err(&spi->dev, "soft reset failed\n");
return ret;
}
- ret = regmap_read(priv->regmap, SX1301_GEN, &val);
+ ret = sx1301_field_write(priv, F_GLOBAL_EN, 0);
if (ret) {
- dev_err(&spi->dev, "16 read failed\n");
+ dev_err(&spi->dev, "gate global clocks failed\n");
return ret;
}
- val &= ~REG_16_GLOBAL_EN;
-
- ret = regmap_write(priv->regmap, SX1301_GEN, val);
- if (ret) {
- dev_err(&spi->dev, "16 write failed\n");
- return ret;
- }
-
- ret = regmap_read(priv->regmap, SX1301_CKEN, &val);
+ ret = sx1301_field_write(priv, F_CLK32M_EN, 0);
if (ret) {
- dev_err(&spi->dev, "17 read failed\n");
+ dev_err(&spi->dev, "gate 32M clock failed\n");
return ret;
}
- val &= ~REG_17_CLK32M_EN;
-
- ret = regmap_write(priv->regmap, SX1301_CKEN, val);
+ ret = sx1301_field_write(priv, F_RADIO_A_EN, 1);
if (ret) {
- dev_err(&spi->dev, "17 write failed\n");
+ dev_err(&spi->dev, "radio a enable failed\n");
return ret;
}
- ret = regmap_read(priv->regmap, SX1301_RADIO_CFG, &val);
- if (ret) {
- dev_err(&spi->dev, "2|43 read failed\n");
- return ret;
- }
-
- val |= REG_2_43_RADIO_B_EN | REG_2_43_RADIO_A_EN;
-
- ret = regmap_write(priv->regmap, SX1301_RADIO_CFG, val);
+ ret = sx1301_field_write(priv, F_RADIO_B_EN, 1);
if (ret) {
- dev_err(&spi->dev, "2|43 write failed\n");
+ dev_err(&spi->dev, "radio b enable failed\n");
return ret;
}
msleep(500);
- ret = regmap_read(priv->regmap, SX1301_RADIO_CFG, &val);
+ ret = sx1301_field_write(priv, F_RADIO_RST, 1);
if (ret) {
- dev_err(&spi->dev, "2|43 read failed\n");
- return ret;
- }
-
- val |= REG_2_43_RADIO_RST;
-
- ret = regmap_write(priv->regmap, SX1301_RADIO_CFG, val);
- if (ret) {
- dev_err(&spi->dev, "2|43 write failed\n");
+ dev_err(&spi->dev, "radio asert reset failed\n");
return ret;
}
msleep(5);
- ret = regmap_read(priv->regmap, SX1301_RADIO_CFG, &val);
- if (ret) {
- dev_err(&spi->dev, "2|43 read failed\n");
- return ret;
- }
-
- val &= ~REG_2_43_RADIO_RST;
-
- ret = regmap_write(priv->regmap, SX1301_RADIO_CFG, val);
+ ret = sx1301_field_write(priv, F_RADIO_RST, 0);
if (ret) {
- dev_err(&spi->dev, "2|43 write failed\n");
+ dev_err(&spi->dev, "radio deasert reset failed\n");
return ret;
}
diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
index e6400f8..0bbd948 100644
--- a/drivers/net/lora/sx1301.h
+++ b/drivers/net/lora/sx1301.h
@@ -60,11 +60,57 @@
#define SX1301_MAX_REGISTER (SX1301_PAGE_BASE(3) + 0x7F)
+enum sx1301_fields {
+ F_SOFT_RESET,
+ F_GLOBAL_EN,
+ F_CLK32M_EN,
+ F_RADIO_A_EN,
+ F_RADIO_B_EN,
+ F_RADIO_RST,
+
+ F_MCU_RST_0,
+ F_MCU_RST_1,
+ F_MCU_SELECT_MUX_0,
+ F_MCU_SELECT_MUX_1,
+
+ F_FORCE_HOST_RADIO_CTRL,
+ F_FORCE_HOST_FE_CTRL,
+ F_FORCE_DEC_FILTER_GAIN,
+
+ F_EMERGENCY_FORCE_HOST_CTRL,
+};
+
+static const struct reg_field sx1301_regmap_fields[] = {
+ /* PAGE */
+ [F_SOFT_RESET] = REG_FIELD(SX1301_PAGE, 7, 7),
+ /* GEN */
+ [F_GLOBAL_EN] = REG_FIELD(SX1301_GEN, 3, 3),
+ /* CKEN */
+ [F_CLK32M_EN] = REG_FIELD(SX1301_CKEN, 0, 0),
+ /* RADIO_CFG */
+ [F_RADIO_A_EN] = REG_FIELD(SX1301_RADIO_CFG, 0, 0),
+ [F_RADIO_B_EN] = REG_FIELD(SX1301_RADIO_CFG, 1, 1),
+ [F_RADIO_RST] = REG_FIELD(SX1301_RADIO_CFG, 2, 2),
+ /* MCU_CTRL */
+ [F_MCU_RST_0] = REG_FIELD(SX1301_MCU_CTRL, 0, 0),
+ [F_MCU_RST_1] = REG_FIELD(SX1301_MCU_CTRL, 1, 1),
+ [F_MCU_SELECT_MUX_0] = REG_FIELD(SX1301_MCU_CTRL, 2, 2),
+ [F_MCU_SELECT_MUX_1] = REG_FIELD(SX1301_MCU_CTRL, 3, 3),
+ /* FORCE_CTRL */
+ [F_FORCE_HOST_RADIO_CTRL] = REG_FIELD(SX1301_FORCE_CTRL, 1, 1),
+ [F_FORCE_HOST_FE_CTRL] = REG_FIELD(SX1301_FORCE_CTRL, 2, 2),
+ [F_FORCE_DEC_FILTER_GAIN] = REG_FIELD(SX1301_FORCE_CTRL, 3, 3),
+ /* EMERGENCY_FORCE_HOST_CTRL */
+ [F_EMERGENCY_FORCE_HOST_CTRL] =
+ REG_FIELD(SX1301_EMERGENCY_FORCE_HOST_CTRL, 0, 0),
+};
+
struct sx1301_priv {
struct lora_dev_priv lora;
struct device *dev;
struct gpio_desc *rst_gpio;
struct regmap *regmap;
+ struct regmap_field *regmap_fields[ARRAY_SIZE(sx1301_regmap_fields)];
};
int __init sx130x_radio_init(void);
--
2.7.4
^ permalink raw reply related
* [PATCH v2 lora-next 3/4] net: lora: sx125x: convert to regmap fields
From: Ben Whitten @ 2018-10-09 12:52 UTC (permalink / raw)
To: afaerber; +Cc: starnight, hasnain.virk, netdev, liuxuenetmail, shess,
Ben Whitten
In-Reply-To: <1539089532-2481-1-git-send-email-ben.whitten@lairdtech.com>
From: Ben Whitten <ben.whitten@gmail.com>
We convert to using regmap fields to allow regmap to take care of read
modify writes and bit shifting for ofset fields.
Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
drivers/net/lora/sx125x.c | 59 ++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 51 insertions(+), 8 deletions(-)
diff --git a/drivers/net/lora/sx125x.c b/drivers/net/lora/sx125x.c
index dc13d1a..36b61b1 100644
--- a/drivers/net/lora/sx125x.c
+++ b/drivers/net/lora/sx125x.c
@@ -25,11 +25,25 @@
#include "sx125x.h"
-#define REG_CLK_SELECT_TX_DAC_CLK_SELECT_CLK_IN BIT(0)
-#define REG_CLK_SELECT_CLK_OUT BIT(1)
+enum sx125x_fields {
+ F_CLK_OUT,
+ F_TX_DAC_CLK_SEL,
+ F_SX1257_XOSC_GM_STARTUP,
+ F_SX1257_XOSC_DISABLE_CORE,
+};
+
+static const struct reg_field sx125x_regmap_fields[] = {
+ /* CLK_SELECT */
+ [F_CLK_OUT] = REG_FIELD(SX125X_CLK_SELECT, 1, 1),
+ [F_TX_DAC_CLK_SEL] = REG_FIELD(SX125X_CLK_SELECT, 0, 0),
+ /* XOSC */ /* TODO maybe make this dynamic */
+ [F_SX1257_XOSC_GM_STARTUP] = REG_FIELD(SX1257_XOSC, 0, 3),
+ [F_SX1257_XOSC_DISABLE_CORE] = REG_FIELD(SX1257_XOSC, 5, 5),
+};
struct sx125x_priv {
struct regmap *regmap;
+ struct regmap_field *regmap_fields[ARRAY_SIZE(sx125x_regmap_fields)];
};
static struct regmap_config __maybe_unused sx125x_regmap_config = {
@@ -44,11 +58,18 @@ static struct regmap_config __maybe_unused sx125x_regmap_config = {
.max_register = SX125X_MAX_REGISTER,
};
+static int sx125x_field_write(struct sx125x_priv *priv,
+ enum sx125x_fields field_id, u8 val)
+{
+ return regmap_field_write(priv->regmap_fields[field_id], val);
+}
+
static int __maybe_unused sx125x_regmap_probe(struct device *dev, struct regmap *regmap, unsigned int radio)
{
struct sx125x_priv *priv;
unsigned int val;
int ret;
+ int i;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
@@ -56,6 +77,18 @@ static int __maybe_unused sx125x_regmap_probe(struct device *dev, struct regmap
dev_set_drvdata(dev, priv);
priv->regmap = regmap;
+ for (i = 0; i < ARRAY_SIZE(sx125x_regmap_fields); i++) {
+ const struct reg_field *reg_fields = sx125x_regmap_fields;
+
+ priv->regmap_fields[i] = devm_regmap_field_alloc(dev,
+ priv->regmap,
+ reg_fields[i]);
+ if (IS_ERR(priv->regmap_fields[i])) {
+ ret = PTR_ERR(priv->regmap_fields[i]);
+ dev_err(dev, "Cannot allocate regmap field: %d\n", ret);
+ return ret;
+ }
+ }
if (true) {
ret = regmap_read(priv->regmap, SX1255_VERSION, &val);
@@ -66,24 +99,34 @@ static int __maybe_unused sx125x_regmap_probe(struct device *dev, struct regmap
dev_info(dev, "SX125x version: %02x\n", val);
}
- val = REG_CLK_SELECT_TX_DAC_CLK_SELECT_CLK_IN;
if (radio == 1) { /* HACK */
- val |= REG_CLK_SELECT_CLK_OUT;
+ ret = sx125x_field_write(priv, F_CLK_OUT, 1);
+ if (ret) {
+ dev_err(dev, "enabling clock output failed\n");
+ return ret;
+ }
+
dev_info(dev, "enabling clock output\n");
}
- ret = regmap_write(priv->regmap, SX125X_CLK_SELECT, val);
+ ret = sx125x_field_write(priv, F_TX_DAC_CLK_SEL, 1);
if (ret) {
- dev_err(dev, "clk write failed\n");
+ dev_err(dev, "clock select failed\n");
return ret;
}
dev_dbg(dev, "clk written\n");
if (true) {
- ret = regmap_write(priv->regmap, SX1257_XOSC, 13 + 2 * 16);
+ ret = sx125x_field_write(priv, F_SX1257_XOSC_DISABLE_CORE, 1);
+ if (ret) {
+ dev_err(dev, "xosc disable failed\n");
+ return ret;
+ }
+
+ ret = sx125x_field_write(priv, F_SX1257_XOSC_GM_STARTUP, 13);
if (ret) {
- dev_err(dev, "xosc write failed\n");
+ dev_err(dev, "xosc startup adjust failed\n");
return ret;
}
}
--
2.7.4
^ permalink raw reply related
* [PATCH v2 lora-next 4/4] net: lora: sx125x sx1301: allow radio to register as a clk provider
From: Ben Whitten @ 2018-10-09 12:52 UTC (permalink / raw)
To: afaerber; +Cc: starnight, hasnain.virk, netdev, liuxuenetmail, shess,
Ben Whitten
In-Reply-To: <1539089532-2481-1-git-send-email-ben.whitten@lairdtech.com>
From: Ben Whitten <ben.whitten@gmail.com>
The 32M is run from the radio, before we just enabled it based on
the radio number but now we can use the clk framework to request the
clk is started when we need it.
The 32M clock produced from the radio is really a gated version of
tcxo which is a fixed clock provided by hardware, and isn't captured
in this patch.
The sx1301 brings the clock up prior to calibration once the radios
have probed themselves.
A sample dts showing the clk link:
sx1301: sx1301@0 {
...
clocks = <&radio1 0>;
clock-names = "clk32m";
radio-spi {
radio0: radio-a@0 {
...
};
radio1: radio-b@1 {
#clock-cells = <0>;
clock-output-names = "clk32m";
};
};
};
Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
drivers/net/lora/sx125x.c | 112 ++++++++++++++++++++++++++++++++++++++++++----
drivers/net/lora/sx1301.c | 13 ++++++
drivers/net/lora/sx1301.h | 2 +
3 files changed, 119 insertions(+), 8 deletions(-)
diff --git a/drivers/net/lora/sx125x.c b/drivers/net/lora/sx125x.c
index 36b61b1..b7ca782 100644
--- a/drivers/net/lora/sx125x.c
+++ b/drivers/net/lora/sx125x.c
@@ -9,6 +9,8 @@
* Copyright (c) 2013 Semtech-Cycleo
*/
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
@@ -42,10 +44,16 @@ static const struct reg_field sx125x_regmap_fields[] = {
};
struct sx125x_priv {
+ struct clk *clkout;
+ struct clk_hw clkout_hw;
+
+ struct device *dev;
struct regmap *regmap;
struct regmap_field *regmap_fields[ARRAY_SIZE(sx125x_regmap_fields)];
};
+#define to_clkout(_hw) container_of(_hw, struct sx125x_priv, clkout_hw)
+
static struct regmap_config __maybe_unused sx125x_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
@@ -64,6 +72,96 @@ static int sx125x_field_write(struct sx125x_priv *priv,
return regmap_field_write(priv->regmap_fields[field_id], val);
}
+static int sx125x_field_read(struct sx125x_priv *priv,
+ enum sx125x_fields field_id, unsigned int *val)
+{
+ return regmap_field_read(priv->regmap_fields[field_id], val);
+}
+
+static int sx125x_clkout_enable(struct clk_hw *hw)
+{
+ struct sx125x_priv *priv = to_clkout(hw);
+
+ dev_info(priv->dev, "enabling clkout\n");
+ return sx125x_field_write(priv, F_CLK_OUT, 1);
+}
+
+static void sx125x_clkout_disable(struct clk_hw *hw)
+{
+ struct sx125x_priv *priv = to_clkout(hw);
+ int ret;
+
+ dev_info(priv->dev, "disabling clkout\n");
+ ret = sx125x_field_write(priv, F_CLK_OUT, 0);
+ if (ret)
+ dev_err(priv->dev, "error disabling clkout\n");
+}
+
+static int sx125x_clkout_is_enabled(struct clk_hw *hw)
+{
+ struct sx125x_priv *priv = to_clkout(hw);
+ unsigned int enabled;
+ int ret;
+
+ ret = sx125x_field_read(priv, F_CLK_OUT, &enabled);
+ if (ret) {
+ dev_err(priv->dev, "error reading clk enable\n");
+ return 0;
+ }
+ return enabled;
+}
+
+static const struct clk_ops sx125x_clkout_ops = {
+ .enable = sx125x_clkout_enable,
+ .disable = sx125x_clkout_disable,
+ .is_enabled = sx125x_clkout_is_enabled,
+};
+
+static int sx125x_register_clock_provider(struct sx125x_priv *priv)
+{
+ struct device *dev = priv->dev;
+ struct clk_init_data init;
+ const char *parent;
+ int ret;
+
+ /* Disable CLKOUT */
+ ret = sx125x_field_write(priv, F_CLK_OUT, 0);
+ if (ret) {
+ dev_err(dev, "unable to disable clkout\n");
+ return ret;
+ }
+
+ /* Register clock provider if expected in DTB */
+ if (!of_find_property(dev->of_node, "#clock-cells", NULL))
+ return 0;
+
+ dev_info(dev, "registering clkout\n");
+
+ parent = of_clk_get_parent_name(dev->of_node, 0);
+ if (!parent) {
+ dev_err(dev, "Unable to find parent clk\n");
+ return -ENODEV;
+ }
+
+ init.ops = &sx125x_clkout_ops;
+ init.flags = CLK_IS_BASIC;
+ init.parent_names = &parent;
+ init.num_parents = 1;
+ priv->clkout_hw.init = &init;
+
+ of_property_read_string_index(dev->of_node, "clock-output-names", 0,
+ &init.name);
+
+ priv->clkout = devm_clk_register(dev, &priv->clkout_hw);
+ if (IS_ERR(priv->clkout)) {
+ dev_err(dev, "failed to register clkout\n");
+ return PTR_ERR(priv->clkout);
+ }
+ ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get,
+ &priv->clkout_hw);
+ return ret;
+}
+
static int __maybe_unused sx125x_regmap_probe(struct device *dev, struct regmap *regmap, unsigned int radio)
{
struct sx125x_priv *priv;
@@ -76,6 +174,7 @@ static int __maybe_unused sx125x_regmap_probe(struct device *dev, struct regmap
return -ENOMEM;
dev_set_drvdata(dev, priv);
+ priv->dev = dev;
priv->regmap = regmap;
for (i = 0; i < ARRAY_SIZE(sx125x_regmap_fields); i++) {
const struct reg_field *reg_fields = sx125x_regmap_fields;
@@ -99,16 +198,13 @@ static int __maybe_unused sx125x_regmap_probe(struct device *dev, struct regmap
dev_info(dev, "SX125x version: %02x\n", val);
}
- if (radio == 1) { /* HACK */
- ret = sx125x_field_write(priv, F_CLK_OUT, 1);
- if (ret) {
- dev_err(dev, "enabling clock output failed\n");
- return ret;
- }
-
- dev_info(dev, "enabling clock output\n");
+ ret = sx125x_register_clock_provider(priv);
+ if (ret) {
+ dev_err(dev, "failed to register clkout provider: %d\n", ret);
+ return ret;
}
+ /* TODO Only needs setting on radio on the TX path */
ret = sx125x_field_write(priv, F_TX_DAC_CLK_SEL, 1);
if (ret) {
dev_err(dev, "clock select failed\n");
diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c
index eb89af6..44a7d0b 100644
--- a/drivers/net/lora/sx1301.c
+++ b/drivers/net/lora/sx1301.c
@@ -10,6 +10,7 @@
*/
#include <linux/bitops.h>
+#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/firmware.h>
#include <linux/lora.h>
@@ -404,6 +405,18 @@ static int sx130x_loradev_open(struct net_device *netdev)
return -ENXIO;
}
+ priv->clk32m = devm_clk_get(priv->dev, "clk32m");
+ if (IS_ERR(priv->clk32m)) {
+ dev_err(priv->dev, "failed to get clk32m\n");
+ return PTR_ERR(priv->clk32m);
+ }
+
+ ret = clk_prepare_enable(priv->clk32m);
+ if (ret) {
+ dev_err(priv->dev, "failed to enable clk32m: %d\n", ret);
+ return ret;
+ }
+
ret = sx1301_field_write(priv, F_GLOBAL_EN, 1);
if (ret) {
dev_err(priv->dev, "enable global clocks failed\n");
diff --git a/drivers/net/lora/sx1301.h b/drivers/net/lora/sx1301.h
index 0bbd948..a1a2e38 100644
--- a/drivers/net/lora/sx1301.h
+++ b/drivers/net/lora/sx1301.h
@@ -9,6 +9,7 @@
#ifndef _SX1301_
#define _SX1301_
+#include <linux/clk.h>
#include <linux/regmap.h>
#include <linux/gpio/consumer.h>
#include <linux/lora/dev.h>
@@ -108,6 +109,7 @@ static const struct reg_field sx1301_regmap_fields[] = {
struct sx1301_priv {
struct lora_dev_priv lora;
struct device *dev;
+ struct clk *clk32m;
struct gpio_desc *rst_gpio;
struct regmap *regmap;
struct regmap_field *regmap_fields[ARRAY_SIZE(sx1301_regmap_fields)];
--
2.7.4
^ permalink raw reply related
* Re: [RFC PATCH 02/11] dt-bindings: phy: add cpsw port interface mode selection phy bindings
From: Grygorii Strashko @ 2018-10-09 20:10 UTC (permalink / raw)
To: Tony Lindgren
Cc: David S. Miller, netdev, Rob Herring, Kishon Vijay Abraham I,
Sekhar Nori, linux-kernel, linux-omap, devicetree
In-Reply-To: <20181009144000.GL5662@atomide.com>
On 10/09/2018 09:40 AM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [181008 23:54]:
>> +Examples:
>> + phy_gmii_sel: phy-gmii-sel {
>> + compatible = "ti,am3352-phy-gmii-sel";
>> + syscon-scm = <&scm_conf>;
>> + #phy-cells = <2>;
>> + };
>
> Now that this driver can live in it's proper place in the
right
> dts, you may want to consider just using standard reg
> property for it instead of the syscon-scm. And also get
> rid of the syscon reads and writes.
Could you help clarify how to get syscon in this case?
syscon_node_to_regmap(dev->parent->of_node)?
Also, there are could be more then one gmii_sel registers in SCM in the future,
so I hidden offsets in of_match data.
As result, "reg" not needed at all now.
--
regards,
-grygorii
^ permalink raw reply
* Re: [PATCH bpf-next 3/6] bpf: add MAP_LOOKUP_AND_DELETE_ELEM syscall
From: Mauricio Vasquez @ 2018-10-09 12:56 UTC (permalink / raw)
To: Song Liu; +Cc: Alexei Starovoitov, Daniel Borkmann, Networking
In-Reply-To: <CAPhsuW4xb=srya5ghyjvKtcnTTYvje-c=vbJa-abUBj6CjC6_A@mail.gmail.com>
On 10/08/2018 08:13 PM, Song Liu wrote:
> On Mon, Oct 8, 2018 at 12:12 PM Mauricio Vasquez B
> <mauricio.vasquez@polito.it> wrote:
>> The following patch implements a bpf queue/stack maps that
>> provides the peek/pop/push functions. There is not a direct
>> relationship between those functions and the current maps
>> syscalls, hence a new MAP_LOOKUP_AND_DELETE_ELEM syscall is added,
>> this is mapped to the pop operation in the queue/stack maps
>> and it is still to implement in other kind of maps.
> Do we need this system call for other maps (non-stack/queue)?
> If not, maybe we can just call it POP, and only implement it for
> stack and queue?
>
Yes, this system call could also benefit other maps. The first idea was
to add pop/push/peek system calls as well, but them Alexei realized it
was too specific for queue/stack maps and we decided to go ahead with
this solution that is more general.
>> Signed-off-by: Mauricio Vasquez B <mauricio.vasquez@polito.it>
>> ---
>> include/linux/bpf.h | 1 +
>> include/uapi/linux/bpf.h | 1 +
>> kernel/bpf/syscall.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 83 insertions(+)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 027697b6a22f..98c7eeb6d138 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -39,6 +39,7 @@ struct bpf_map_ops {
>> void *(*map_lookup_elem)(struct bpf_map *map, void *key);
>> int (*map_update_elem)(struct bpf_map *map, void *key, void *value, u64 flags);
>> int (*map_delete_elem)(struct bpf_map *map, void *key);
>> + void *(*map_lookup_and_delete_elem)(struct bpf_map *map, void *key);
>>
>> /* funcs called by prog_array and perf_event_array map */
>> void *(*map_fd_get_ptr)(struct bpf_map *map, struct file *map_file,
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index f9187b41dff6..3bb94aa2d408 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -103,6 +103,7 @@ enum bpf_cmd {
>> BPF_BTF_LOAD,
>> BPF_BTF_GET_FD_BY_ID,
>> BPF_TASK_FD_QUERY,
>> + BPF_MAP_LOOKUP_AND_DELETE_ELEM,
>> };
>>
>> enum bpf_map_type {
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index eb75e8af73ff..c33d9303f72f 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -975,6 +975,84 @@ static int map_get_next_key(union bpf_attr *attr)
>> return err;
>> }
>>
>> +#define BPF_MAP_LOOKUP_AND_DELETE_ELEM_LAST_FIELD value
>> +
>> +static int map_lookup_and_delete_elem(union bpf_attr *attr)
>> +{
>> + void __user *ukey = u64_to_user_ptr(attr->key);
>> + void __user *uvalue = u64_to_user_ptr(attr->value);
>> + int ufd = attr->map_fd;
>> + struct bpf_map *map;
>> + void *key, *value, *ptr;
>> + u32 value_size;
>> + struct fd f;
>> + int err;
>> +
>> + if (CHECK_ATTR(BPF_MAP_LOOKUP_AND_DELETE_ELEM))
>> + return -EINVAL;
>> +
>> + f = fdget(ufd);
>> + map = __bpf_map_get(f);
>> + if (IS_ERR(map))
>> + return PTR_ERR(map);
>> +
>> + if (!(f.file->f_mode & FMODE_CAN_WRITE)) {
>> + err = -EPERM;
>> + goto err_put;
>> + }
>> +
>> + key = __bpf_copy_key(ukey, map->key_size);
>> + if (IS_ERR(key)) {
>> + err = PTR_ERR(key);
>> + goto err_put;
>> + }
>> +
>> + value_size = map->value_size;
>> +
>> + err = -ENOMEM;
>> + value = kmalloc(value_size, GFP_USER | __GFP_NOWARN);
>> + if (!value)
>> + goto free_key;
>> +
>> + err = -EFAULT;
>> + if (copy_from_user(value, uvalue, value_size) != 0)
>> + goto free_value;
>> +
>> + /* must increment bpf_prog_active to avoid kprobe+bpf triggering from
>> + * inside bpf map update or delete otherwise deadlocks are possible
>> + */
>> + preempt_disable();
>> + __this_cpu_inc(bpf_prog_active);
>> + if (!map->ops->map_lookup_and_delete_elem) {
> Why do have have this check here? Shouldn't it be check much earlier?
> If we really need it here, we need at least add the following:
In this particular patch the check can be done much earlier, but in next
patch we need it on this position, so I leave it here to avoid moving
around on next patch.
> __this_cpu_dec(bpf_prog_active);
> preempt_enable();
You are right, I missed that. Will fix for next version.
>
>
>> + err = -ENOTSUPP;
>> + goto free_value;
>> + }
>> + rcu_read_lock();
>> + ptr = map->ops->map_lookup_and_delete_elem(map, key);
>> + if (ptr)
>> + memcpy(value, ptr, value_size);
>> + rcu_read_unlock();
>> + err = ptr ? 0 : -ENOENT;
>> + __this_cpu_dec(bpf_prog_active);
>> + preempt_enable();
>> +
>> + if (err)
>> + goto free_value;
>> +
>> + if (copy_to_user(uvalue, value, value_size) != 0)
>> + goto free_value;
>> +
>> + err = 0;
>> +
>> +free_value:
>> + kfree(value);
>> +free_key:
>> + kfree(key);
>> +err_put:
>> + fdput(f);
>> + return err;
>> +}
>> +
>> static const struct bpf_prog_ops * const bpf_prog_types[] = {
>> #define BPF_PROG_TYPE(_id, _name) \
>> [_id] = & _name ## _prog_ops,
>> @@ -2448,6 +2526,9 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
>> case BPF_TASK_FD_QUERY:
>> err = bpf_task_fd_query(&attr, uattr);
>> break;
>> + case BPF_MAP_LOOKUP_AND_DELETE_ELEM:
>> + err = map_lookup_and_delete_elem(&attr);
>> + break;
>> default:
>> err = -EINVAL;
>> break;
>>
^ permalink raw reply
* Re: [RFC PATCH 03/11] phy: ti: introduce phy-gmii-sel driver
From: Grygorii Strashko @ 2018-10-09 20:22 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, netdev, Tony Lindgren, Rob Herring,
Kishon Vijay Abraham I, Sekhar Nori, linux-kernel, linux-omap,
devicetree
In-Reply-To: <20181009003935.GA23588@lunn.ch>
Hi Andrew,
On 10/08/2018 07:39 PM, Andrew Lunn wrote:
> On Mon, Oct 08, 2018 at 06:49:41PM -0500, Grygorii Strashko wrote:
>> +static int phy_gmii_sel_mode(struct phy *phy, phy_interface_t intf_mode)
>> +{
>> + struct phy_gmii_sel_phy_priv *if_phy = phy_get_drvdata(phy);
>> + const struct phy_gmii_sel_soc_data *soc_data = if_phy->priv->soc_data;
>> + struct device *dev = if_phy->priv->dev;
>> + struct regmap_field *regfield;
>> + int ret, rgmii_id = 0;
>> + u32 mode = 0;
>> +
>> + if_phy->phy_if_mode = intf_mode;
>> +
>> + switch (if_phy->phy_if_mode) {
>> + case PHY_INTERFACE_MODE_RMII:
>> + mode = AM33XX_GMII_SEL_MODE_RMII;
>> + break;
>> +
>> + case PHY_INTERFACE_MODE_RGMII:
>> + mode = AM33XX_GMII_SEL_MODE_RGMII;
>> + break;
>> +
>> + case PHY_INTERFACE_MODE_RGMII_ID:
>> + case PHY_INTERFACE_MODE_RGMII_RXID:
>> + case PHY_INTERFACE_MODE_RGMII_TXID:
>> + mode = AM33XX_GMII_SEL_MODE_RGMII;
>> + rgmii_id = 1;
>> + break;
>
> Hi Grygorii
>
> It looks like the MAC can do AM33XX_GMII_SEL_MODE_RGMII and
> AM33XX_GMII_SEL_MODE_RGMII_ID. I don't think it can do
> AM33XX_GMII_SEL_MODE_RGMII_RXID or AM33XX_GMII_SEL_MODE_RGMII_TXID?
Sry, but would prefer not to thought this logic as part of this series as i moved it here
unchanged rom cpsw-phy-sel.c (except adding possibility to update only supported field)
and any changes here would require separate review (including all existing TI DT boards)
and testing.
I
> would prefer it return -EINVAL when asked to do something it cannot
> do.
Just to clarify rgmii_id = 1 means *disable* CPSW Internal Delay Mode.
>
>> +
>> + default:
>> + dev_warn(dev,
>> + "port%u: unsupported mode: \"%s\". Defaulting to MII.\n",
>> + if_phy->id, phy_modes(rgmii_id));
>> + /* fall through */
>
> Returning -EINVAL would be better. Otherwise the DT might never get
> fixed.
ok
>
>> + case PHY_INTERFACE_MODE_MII:
>> + mode = AM33XX_GMII_SEL_MODE_MII;
>> + break;
>> + };
>> +
>> + dev_dbg(dev, "%s id:%u mode:%u rgmii_id:%d rmii_clk_ext:%d\n",
>> + __func__, if_phy->id, mode, rgmii_id,
>> + if_phy->rmii_clock_external);
>> +
>> + regfield = if_phy->fields[PHY_GMII_SEL_PORT_MODE];
>> + ret = regmap_field_write(regfield, mode);
>> +
>> + if (soc_data->features & BIT(PHY_GMII_SEL_RGMII_ID_MODE) &&
>> + if_phy->fields[PHY_GMII_SEL_RGMII_ID_MODE]) {
>> + regfield = if_phy->fields[PHY_GMII_SEL_RGMII_ID_MODE];
>> + ret |= regmap_field_write(regfield, rgmii_id);
>> + }
>> +
>> + if (soc_data->features & BIT(PHY_GMII_SEL_RMII_IO_CLK_EN) &&
>> + if_phy->fields[PHY_GMII_SEL_RMII_IO_CLK_EN]) {
>> + regfield = if_phy->fields[PHY_GMII_SEL_RMII_IO_CLK_EN];
>> + ret |= regmap_field_write(regfield,
>> + if_phy->rmii_clock_external);
>> + }
>> +
>> + if (ret) {
>> + dev_err(dev, "port%u: set mode fail %d", if_phy->id, ret);
>> + return -EIO;
>> + }
>
> I would prefer each write had its own error check. The fact you don't
> return ret means you know ret could be -EINVAL|-EOIO, making
> -EMORECOFFEE.
:) right, sry.
--
regards,
-grygorii
^ permalink raw reply
* Re: [PATCH bpf-next 4/6] bpf: add queue and stack maps
From: Mauricio Vasquez @ 2018-10-09 13:05 UTC (permalink / raw)
To: Song Liu; +Cc: Alexei Starovoitov, Daniel Borkmann, Networking
In-Reply-To: <CAPhsuW6B_VJ8t8VoDhmk=H9J6PwkczscPqsB4fW_cFr1K72z6g@mail.gmail.com>
On 10/08/2018 08:36 PM, Song Liu wrote:
> On Mon, Oct 8, 2018 at 12:12 PM Mauricio Vasquez B
> <mauricio.vasquez@polito.it> wrote:
>> Queue/stack maps implement a FIFO/LIFO data storage for ebpf programs.
>> These maps support peek, pop and push operations that are exposed to eBPF
>> programs through the new bpf_map[peek/pop/push] helpers. Those operations
>> are exposed to userspace applications through the already existing
>> syscalls in the following way:
>>
>> BPF_MAP_LOOKUP_ELEM -> peek
>> BPF_MAP_LOOKUP_AND_DELETE_ELEM -> pop
>> BPF_MAP_UPDATE_ELEM -> push
>>
>> Queue/stack maps are implemented using a buffer, tail and head indexes,
>> hence BPF_F_NO_PREALLOC is not supported.
>>
>> As opposite to other maps, queue and stack do not use RCU for protecting
>> maps values, the bpf_map[peek/pop] have a ARG_PTR_TO_UNINIT_MAP_VALUE
>> argument that is a pointer to a memory zone where to save the value of a
>> map. Basically the same as ARG_PTR_TO_UNINIT_MEM, but the size has not
>> be passed as an extra argument.
>>
>> Our main motivation for implementing queue/stack maps was to keep track
>> of a pool of elements, like network ports in a SNAT, however we forsee
>> other use cases, like for exampling saving last N kernel events in a map
>> and then analysing from userspace.
>>
>> Signed-off-by: Mauricio Vasquez B <mauricio.vasquez@polito.it>
>> ---
>> include/linux/bpf.h | 7 +
>> include/linux/bpf_types.h | 2
>> include/uapi/linux/bpf.h | 35 ++++-
>> kernel/bpf/Makefile | 2
>> kernel/bpf/core.c | 3
>> kernel/bpf/helpers.c | 43 ++++++
>> kernel/bpf/queue_stack_maps.c | 288 +++++++++++++++++++++++++++++++++++++++++
>> kernel/bpf/syscall.c | 30 +++-
>> kernel/bpf/verifier.c | 28 +++-
>> net/core/filter.c | 6 +
>> 10 files changed, 426 insertions(+), 18 deletions(-)
>> create mode 100644 kernel/bpf/queue_stack_maps.c
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 98c7eeb6d138..cad3bc5cffd1 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -40,6 +40,9 @@ struct bpf_map_ops {
>> int (*map_update_elem)(struct bpf_map *map, void *key, void *value, u64 flags);
>> int (*map_delete_elem)(struct bpf_map *map, void *key);
>> void *(*map_lookup_and_delete_elem)(struct bpf_map *map, void *key);
>> + int (*map_push_elem)(struct bpf_map *map, void *value, u64 flags);
>> + int (*map_pop_elem)(struct bpf_map *map, void *value);
>> + int (*map_peek_elem)(struct bpf_map *map, void *value);
>>
>> /* funcs called by prog_array and perf_event_array map */
>> void *(*map_fd_get_ptr)(struct bpf_map *map, struct file *map_file,
>> @@ -139,6 +142,7 @@ enum bpf_arg_type {
>> ARG_CONST_MAP_PTR, /* const argument used as pointer to bpf_map */
>> ARG_PTR_TO_MAP_KEY, /* pointer to stack used as map key */
>> ARG_PTR_TO_MAP_VALUE, /* pointer to stack used as map value */
>> + ARG_PTR_TO_UNINIT_MAP_VALUE, /* pointer to valid memory used to store a map value */
> How about we put ARG_PTR_TO_UNINIT_MAP_VALUE and related logic to a
> separate patch?
I thought it too, but this is a really small change (6 additions, 3
deletions). Does it worth a separated patch?
>
>> /* the following constraints used to prototype bpf_memcmp() and other
>> * functions that access data on eBPF program stack
>> @@ -825,6 +829,9 @@ static inline int bpf_fd_reuseport_array_update_elem(struct bpf_map *map,
>> extern const struct bpf_func_proto bpf_map_lookup_elem_proto;
>> extern const struct bpf_func_proto bpf_map_update_elem_proto;
>> extern const struct bpf_func_proto bpf_map_delete_elem_proto;
>> +extern const struct bpf_func_proto bpf_map_push_elem_proto;
>> +extern const struct bpf_func_proto bpf_map_pop_elem_proto;
>> +extern const struct bpf_func_proto bpf_map_peek_elem_proto;
>>
>> extern const struct bpf_func_proto bpf_get_prandom_u32_proto;
>> extern const struct bpf_func_proto bpf_get_smp_processor_id_proto;
>> diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
>> index 658509daacd4..a2ec73aa1ec7 100644
>> --- a/include/linux/bpf_types.h
>> +++ b/include/linux/bpf_types.h
>> @@ -69,3 +69,5 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_XSKMAP, xsk_map_ops)
>> BPF_MAP_TYPE(BPF_MAP_TYPE_REUSEPORT_SOCKARRAY, reuseport_array_ops)
>> #endif
>> #endif
>> +BPF_MAP_TYPE(BPF_MAP_TYPE_QUEUE, queue_map_ops)
>> +BPF_MAP_TYPE(BPF_MAP_TYPE_STACK, stack_map_ops)
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 3bb94aa2d408..bfa042273fad 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -129,6 +129,8 @@ enum bpf_map_type {
>> BPF_MAP_TYPE_CGROUP_STORAGE,
>> BPF_MAP_TYPE_REUSEPORT_SOCKARRAY,
>> BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE,
>> + BPF_MAP_TYPE_QUEUE,
>> + BPF_MAP_TYPE_STACK,
>> };
>>
>> enum bpf_prog_type {
>> @@ -463,6 +465,28 @@ union bpf_attr {
>> * Return
>> * 0 on success, or a negative error in case of failure.
>> *
>> + * int bpf_map_push_elem(struct bpf_map *map, const void *value, u64 flags)
>> + * Description
>> + * Push an element *value* in *map*. *flags* is one of:
>> + *
>> + * **BPF_EXIST**
>> + * If the queue/stack is full, the oldest element is removed to
>> + * make room for this.
>> + * Return
>> + * 0 on success, or a negative error in case of failure.
>> + *
>> + * int bpf_map_pop_elem(struct bpf_map *map, void *value)
>> + * Description
>> + * Pop an element from *map*.
>> + * Return
>> + * 0 on success, or a negative error in case of failure.
>> + *
>> + * int bpf_map_peek_elem(struct bpf_map *map, void *value)
>> + * Description
>> + * Get an element from *map* without removing it.
>> + * Return
>> + * 0 on success, or a negative error in case of failure.
>> + *
>> * int bpf_probe_read(void *dst, u32 size, const void *src)
>> * Description
>> * For tracing programs, safely attempt to read *size* bytes from
>> @@ -790,14 +814,14 @@ union bpf_attr {
>> *
>> * int ret;
>> * struct bpf_tunnel_key key = {};
>> - *
>> + *
>> * ret = bpf_skb_get_tunnel_key(skb, &key, sizeof(key), 0);
>> * if (ret < 0)
>> * return TC_ACT_SHOT; // drop packet
>> - *
>> + *
>> * if (key.remote_ipv4 != 0x0a000001)
>> * return TC_ACT_SHOT; // drop packet
>> - *
>> + *
>> * return TC_ACT_OK; // accept packet
>> *
>> * This interface can also be used with all encapsulation devices
>> @@ -2304,7 +2328,10 @@ union bpf_attr {
>> FN(skb_ancestor_cgroup_id), \
>> FN(sk_lookup_tcp), \
>> FN(sk_lookup_udp), \
>> - FN(sk_release),
>> + FN(sk_release), \
>> + FN(map_push_elem), \
>> + FN(map_pop_elem), \
>> + FN(map_peek_elem),
>>
>> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>> * function eBPF program intends to call
>> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
>> index 0488b8258321..17afae9e65f3 100644
>> --- a/kernel/bpf/Makefile
>> +++ b/kernel/bpf/Makefile
>> @@ -3,7 +3,7 @@ obj-y := core.o
>>
>> obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o inode.o helpers.o tnum.o
>> obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o lpm_trie.o map_in_map.o
>> -obj-$(CONFIG_BPF_SYSCALL) += local_storage.o
>> +obj-$(CONFIG_BPF_SYSCALL) += local_storage.o queue_stack_maps.o
>> obj-$(CONFIG_BPF_SYSCALL) += disasm.o
>> obj-$(CONFIG_BPF_SYSCALL) += btf.o
>> ifeq ($(CONFIG_NET),y)
>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
>> index 3f5bf1af0826..8d2db076d123 100644
>> --- a/kernel/bpf/core.c
>> +++ b/kernel/bpf/core.c
>> @@ -1783,6 +1783,9 @@ BPF_CALL_0(bpf_user_rnd_u32)
>> const struct bpf_func_proto bpf_map_lookup_elem_proto __weak;
>> const struct bpf_func_proto bpf_map_update_elem_proto __weak;
>> const struct bpf_func_proto bpf_map_delete_elem_proto __weak;
>> +const struct bpf_func_proto bpf_map_push_elem_proto __weak;
>> +const struct bpf_func_proto bpf_map_pop_elem_proto __weak;
>> +const struct bpf_func_proto bpf_map_peek_elem_proto __weak;
>>
>> const struct bpf_func_proto bpf_get_prandom_u32_proto __weak;
>> const struct bpf_func_proto bpf_get_smp_processor_id_proto __weak;
>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>> index 6502115e8f55..ab0d5e3f9892 100644
>> --- a/kernel/bpf/helpers.c
>> +++ b/kernel/bpf/helpers.c
>> @@ -76,6 +76,49 @@ const struct bpf_func_proto bpf_map_delete_elem_proto = {
>> .arg2_type = ARG_PTR_TO_MAP_KEY,
>> };
>>
>> +BPF_CALL_3(bpf_map_push_elem, struct bpf_map *, map, void *, value, u64, flags)
>> +{
>> + return map->ops->map_push_elem(map, value, flags);
>> +}
>> +
>> +const struct bpf_func_proto bpf_map_push_elem_proto = {
>> + .func = bpf_map_push_elem,
>> + .gpl_only = false,
>> + .pkt_access = true,
>> + .ret_type = RET_INTEGER,
>> + .arg1_type = ARG_CONST_MAP_PTR,
>> + .arg2_type = ARG_PTR_TO_MAP_VALUE,
>> + .arg3_type = ARG_ANYTHING,
>> +};
>> +
>> +BPF_CALL_2(bpf_map_pop_elem, struct bpf_map *, map, void *, value)
>> +{
>> + return map->ops->map_pop_elem(map, value);
>> +}
>> +
>> +const struct bpf_func_proto bpf_map_pop_elem_proto = {
>> + .func = bpf_map_pop_elem,
>> + .gpl_only = false,
>> + .pkt_access = true,
>> + .ret_type = RET_INTEGER,
>> + .arg1_type = ARG_CONST_MAP_PTR,
>> + .arg2_type = ARG_PTR_TO_UNINIT_MAP_VALUE,
>> +};
>> +
>> +BPF_CALL_2(bpf_map_peek_elem, struct bpf_map *, map, void *, value)
>> +{
>> + return map->ops->map_peek_elem(map, value);
>> +}
>> +
>> +const struct bpf_func_proto bpf_map_peek_elem_proto = {
>> + .func = bpf_map_pop_elem,
>> + .gpl_only = false,
>> + .pkt_access = true,
>> + .ret_type = RET_INTEGER,
>> + .arg1_type = ARG_CONST_MAP_PTR,
>> + .arg2_type = ARG_PTR_TO_UNINIT_MAP_VALUE,
>> +};
>> +
>> const struct bpf_func_proto bpf_get_prandom_u32_proto = {
>> .func = bpf_user_rnd_u32,
>> .gpl_only = false,
>> diff --git a/kernel/bpf/queue_stack_maps.c b/kernel/bpf/queue_stack_maps.c
>> new file mode 100644
>> index 000000000000..12a93fb37449
>> --- /dev/null
>> +++ b/kernel/bpf/queue_stack_maps.c
>> @@ -0,0 +1,288 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * queue_stack_maps.c: BPF queue and stack maps
>> + *
>> + * Copyright (c) 2018 Politecnico di Torino
>> + */
>> +#include <linux/bpf.h>
>> +#include <linux/list.h>
>> +#include <linux/slab.h>
>> +#include "percpu_freelist.h"
>> +
>> +#define QUEUE_STACK_CREATE_FLAG_MASK \
>> + (BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
>> +
>> +
>> +struct bpf_queue_stack {
>> + struct bpf_map map;
>> + raw_spinlock_t lock;
>> + u32 head, tail;
>> + u32 size; /* max_entries + 1 */
>> +
>> + char elements[0] __aligned(8);
>> +};
>> +
>> +static struct bpf_queue_stack *bpf_queue_stack(struct bpf_map *map)
>> +{
>> + return container_of(map, struct bpf_queue_stack, map);
>> +}
>> +
>> +static bool queue_stack_map_is_empty(struct bpf_queue_stack *qs)
>> +{
>> + return qs->head == qs->tail;
>> +}
>> +
>> +static bool queue_stack_map_is_full(struct bpf_queue_stack *qs)
>> +{
>> + u32 head = qs->head + 1;
>> +
>> + if (unlikely(head >= qs->size))
>> + head = 0;
>> +
>> + return head == qs->tail;
>> +}
>> +
>> +/* Called from syscall */
>> +static int queue_stack_map_alloc_check(union bpf_attr *attr)
>> +{
>> + /* check sanity of attributes */
>> + if (attr->max_entries == 0 || attr->key_size != 0 ||
>> + attr->map_flags & ~QUEUE_STACK_CREATE_FLAG_MASK)
>> + return -EINVAL;
>> +
>> + if (attr->value_size > KMALLOC_MAX_SIZE)
>> + /* if value_size is bigger, the user space won't be able to
>> + * access the elements.
>> + */
>> + return -E2BIG;
>> +
>> + return 0;
>> +}
>> +
>> +static struct bpf_map *queue_stack_map_alloc(union bpf_attr *attr)
>> +{
>> + int ret, numa_node = bpf_map_attr_numa_node(attr);
>> + struct bpf_queue_stack *qs;
>> + u32 size, value_size;
>> + u64 queue_size, cost;
>> +
>> + size = attr->max_entries + 1;
>> + value_size = attr->value_size;
>> +
>> + queue_size = sizeof(*qs) + (u64) value_size * size;
>> +
>> + cost = queue_size;
>> + if (cost >= U32_MAX - PAGE_SIZE)
>> + return ERR_PTR(-E2BIG);
>> +
>> + cost = round_up(cost, PAGE_SIZE) >> PAGE_SHIFT;
>> +
>> + ret = bpf_map_precharge_memlock(cost);
>> + if (ret < 0)
>> + return ERR_PTR(ret);
>> +
>> + qs = bpf_map_area_alloc(queue_size, numa_node);
>> + if (!qs)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + memset(qs, 0, sizeof(*qs));
>> +
>> + bpf_map_init_from_attr(&qs->map, attr);
>> +
>> + qs->map.pages = cost;
>> + qs->size = size;
>> +
>> + raw_spin_lock_init(&qs->lock);
>> +
>> + return &qs->map;
>> +}
>> +
>> +/* Called when map->refcnt goes to zero, either from workqueue or from syscall */
>> +static void queue_stack_map_free(struct bpf_map *map)
>> +{
>> + struct bpf_queue_stack *qs = bpf_queue_stack(map);
>> +
>> + /* at this point bpf_prog->aux->refcnt == 0 and this map->refcnt == 0,
>> + * so the programs (can be more than one that used this map) were
>> + * disconnected from events. Wait for outstanding critical sections in
>> + * these programs to complete
>> + */
>> + synchronize_rcu();
>> +
>> + bpf_map_area_free(qs);
>> +}
>> +
>> +static int __queue_map_get(struct bpf_map *map, void *value, bool delete)
>> +{
>> + struct bpf_queue_stack *qs = bpf_queue_stack(map);
>> + unsigned long flags;
>> + int err = 0;
>> + void *ptr;
>> +
>> + raw_spin_lock_irqsave(&qs->lock, flags);
>> +
>> + if (queue_stack_map_is_empty(qs)) {
>> + err = -ENOENT;
>> + goto out;
>> + }
>> +
>> + ptr = &qs->elements[qs->tail * qs->map.value_size];
>> + memcpy(value, ptr, qs->map.value_size);
>> +
>> + if (delete) {
>> + if (unlikely(++qs->tail >= qs->size))
>> + qs->tail = 0;
>> + }
>> +
>> +out:
>> + raw_spin_unlock_irqrestore(&qs->lock, flags);
>> + return err;
>> +}
>> +
>> +
>> +static int __stack_map_get(struct bpf_map *map, void *value, bool delete)
>> +{
>> + struct bpf_queue_stack *qs = bpf_queue_stack(map);
>> + unsigned long flags;
>> + int err = 0;
>> + void *ptr;
>> + u32 index;
>> +
>> + raw_spin_lock_irqsave(&qs->lock, flags);
>> +
>> + if (queue_stack_map_is_empty(qs)) {
>> + err = -ENOENT;
>> + goto out;
>> + }
>> +
>> + index = qs->head - 1;
>> + if (unlikely(index >= qs->size))
>> + index = qs->size - 1;
>> +
>> + ptr = &qs->elements[index * qs->map.value_size];
>> + memcpy(value, ptr, qs->map.value_size);
>> +
>> + if (delete)
>> + qs->head = index;
>> +
>> +out:
>> + raw_spin_unlock_irqrestore(&qs->lock, flags);
>> + return err;
>> +}
>> +
>> +/* Called from syscall or from eBPF program */
>> +static int queue_map_peek_elem(struct bpf_map *map, void *value)
>> +{
>> + return __queue_map_get(map, value, false);
>> +}
>> +
>> +/* Called from syscall or from eBPF program */
>> +static int stack_map_peek_elem(struct bpf_map *map, void *value)
>> +{
>> + return __stack_map_get(map, value, false);
>> +}
>> +
>> +/* Called from syscall or from eBPF program */
>> +static int queue_map_pop_elem(struct bpf_map *map, void *value)
>> +{
>> + return __queue_map_get(map, value, true);
>> +}
>> +
>> +/* Called from syscall or from eBPF program */
>> +static int stack_map_pop_elem(struct bpf_map *map, void *value)
>> +{
>> + return __stack_map_get(map, value, true);
>> +}
>> +
>> +/* Called from syscall or from eBPF program */
>> +static int queue_stack_map_push_elem(struct bpf_map *map, void *value,
>> + u64 flags)
>> +{
>> + struct bpf_queue_stack *qs = bpf_queue_stack(map);
>> + unsigned long irq_flags;
>> + int err = 0;
>> + void *dst;
>> +
>> + /* BPF_EXIST is used to force making room for a new element in case the
>> + * map is full
>> + */
>> + bool replace = (flags & BPF_EXIST);
>> +
>> + /* Check supported flags for queue and stack maps */
>> + if (flags & BPF_NOEXIST || flags > BPF_EXIST)
>> + return -EINVAL;
>> +
>> + raw_spin_lock_irqsave(&qs->lock, irq_flags);
>> +
>> + if (queue_stack_map_is_full(qs)) {
>> + if (!replace) {
>> + err = -E2BIG;
>> + goto out;
>> + }
>> + /* advance tail pointer to overwrite oldest element */
>> + if (unlikely(++qs->tail >= qs->size))
>> + qs->tail = 0;
>> + }
>> +
>> + dst = &qs->elements[qs->head * qs->map.value_size];
>> + memcpy(dst, value, qs->map.value_size);
>> +
>> + if (unlikely(++qs->head >= qs->size))
>> + qs->head = 0;
>> +
>> +out:
>> + raw_spin_unlock_irqrestore(&qs->lock, irq_flags);
>> + return err;
>> +}
>> +
>> +/* Called from syscall or from eBPF program */
>> +static void *queue_stack_map_lookup_elem(struct bpf_map *map, void *key)
>> +{
>> + return NULL;
>> +}
>> +
>> +/* Called from syscall or from eBPF program */
>> +static int queue_stack_map_update_elem(struct bpf_map *map, void *key,
>> + void *value, u64 flags)
>> +{
>> + return -EINVAL;
>> +}
>> +
>> +/* Called from syscall or from eBPF program */
>> +static int queue_stack_map_delete_elem(struct bpf_map *map, void *key)
>> +{
>> + return -EINVAL;
>> +}
>> +
>> +/* Called from syscall */
>> +static int queue_stack_map_get_next_key(struct bpf_map *map, void *key,
>> + void *next_key)
>> +{
>> + return -EINVAL;
>> +}
>> +
>> +const struct bpf_map_ops queue_map_ops = {
>> + .map_alloc_check = queue_stack_map_alloc_check,
>> + .map_alloc = queue_stack_map_alloc,
>> + .map_free = queue_stack_map_free,
>> + .map_lookup_elem = queue_stack_map_lookup_elem,
>> + .map_update_elem = queue_stack_map_update_elem,
>> + .map_delete_elem = queue_stack_map_delete_elem,
>> + .map_push_elem = queue_stack_map_push_elem,
>> + .map_pop_elem = queue_map_pop_elem,
>> + .map_peek_elem = queue_map_peek_elem,
>> + .map_get_next_key = queue_stack_map_get_next_key,
>> +};
>> +
>> +const struct bpf_map_ops stack_map_ops = {
>> + .map_alloc_check = queue_stack_map_alloc_check,
>> + .map_alloc = queue_stack_map_alloc,
>> + .map_free = queue_stack_map_free,
>> + .map_lookup_elem = queue_stack_map_lookup_elem,
>> + .map_update_elem = queue_stack_map_update_elem,
>> + .map_delete_elem = queue_stack_map_delete_elem,
>> + .map_push_elem = queue_stack_map_push_elem,
>> + .map_pop_elem = stack_map_pop_elem,
>> + .map_peek_elem = stack_map_peek_elem,
>> + .map_get_next_key = queue_stack_map_get_next_key,
>> +};
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index c33d9303f72f..c135d205fd09 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -727,6 +727,9 @@ static int map_lookup_elem(union bpf_attr *attr)
>> err = bpf_fd_htab_map_lookup_elem(map, key, value);
>> } else if (map->map_type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY) {
>> err = bpf_fd_reuseport_array_lookup_elem(map, key, value);
>> + } else if (map->map_type == BPF_MAP_TYPE_QUEUE ||
>> + map->map_type == BPF_MAP_TYPE_STACK) {
>> + err = map->ops->map_peek_elem(map, value);
>> } else {
>> rcu_read_lock();
>> ptr = map->ops->map_lookup_elem(map, key);
>> @@ -841,6 +844,9 @@ static int map_update_elem(union bpf_attr *attr)
>> /* rcu_read_lock() is not needed */
>> err = bpf_fd_reuseport_array_update_elem(map, key, value,
>> attr->flags);
>> + } else if (map->map_type == BPF_MAP_TYPE_QUEUE ||
>> + map->map_type == BPF_MAP_TYPE_STACK) {
>> + err = map->ops->map_push_elem(map, value, attr->flags);
>> } else {
>> rcu_read_lock();
>> err = map->ops->map_update_elem(map, key, value, attr->flags);
>> @@ -1023,16 +1029,22 @@ static int map_lookup_and_delete_elem(union bpf_attr *attr)
>> */
>> preempt_disable();
>> __this_cpu_inc(bpf_prog_active);
>> - if (!map->ops->map_lookup_and_delete_elem) {
>> - err = -ENOTSUPP;
>> - goto free_value;
>> + if (map->map_type == BPF_MAP_TYPE_QUEUE ||
>> + map->map_type == BPF_MAP_TYPE_STACK) {
>> + err = map->ops->map_pop_elem(map, value);
>> + } else {
>> + if (!map->ops->map_lookup_and_delete_elem) {
>> + err = -ENOTSUPP;
>> + goto free_value;
> similar to previous patch: either we move this check, or we add
> __this_cpu_dec() and preempt_enable().
In this case the check cannot be moved, I'll change it to fix the
problem and make it more readable.
>
> Thanks,
> Song
>
^ permalink raw reply
* Re: [PATCH] qtnfmac: avoid uninitialized variable access
From: Sergey Matyukevich @ 2018-10-09 20:25 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Igor Mitsyanko, Igor Mitsyanko, Avinash Patil, Sergey Matyukevich,
Kalle Valo, David S. Miller, Andrey Shevchenko,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20181009155757.494212-1-arnd@arndb.de>
Hello Arnd,
> When qtnf_trans_send_cmd_with_resp() fails, we have not yet initialized
> 'resp', as pointed out by a valid gcc warning:
>
> drivers/net/wireless/quantenna/qtnfmac/commands.c: In function 'qtnf_cmd_send_with_reply':
> drivers/net/wireless/quantenna/qtnfmac/commands.c:133:54: error: 'resp' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> Since 'resp_skb' is also not set here, we can skip all further
> processing and just print the warning and return the failure code.
>
> Fixes: c6ed298ffe09 ("qtnfmac: cleanup and unify command error handling")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Thanks for the patch! And for reminding me that I forgot to enable
gcc warnings in CI builds in addition to sparse checks.
Reviewed-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
Regards,
Sergey
^ permalink raw reply
* [PATCH net] net/sched: cls_api: add missing validation of netlink attributes
From: Davide Caratti @ 2018-10-09 13:10 UTC (permalink / raw)
To: David S. Miller, David Ahern, Jamal Hadi Salim; +Cc: netdev
Similarly to what has been done in 8b4c3cdd9dd8 ("net: sched: Add policy
validation for tc attributes"), add validation for TCA_CHAIN and TCA_KIND
netlink attributes.
tested with:
# ./tdc.py -c filter
Fixes: 5bc1701881e39 ("net: sched: introduce multichain support for filters")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/cls_api.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 0a75cb2e5e7b..fb1afc0e130d 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -37,6 +37,11 @@ static LIST_HEAD(tcf_proto_base);
/* Protects list of registered TC modules. It is pure SMP lock. */
static DEFINE_RWLOCK(cls_mod_lock);
+const struct nla_policy cls_tca_policy[TCA_MAX + 1] = {
+ [TCA_KIND] = { .type = NLA_STRING },
+ [TCA_CHAIN] = { .type = NLA_U32 },
+};
+
/* Find classifier type by string name */
static const struct tcf_proto_ops *__tcf_proto_lookup_ops(const char *kind)
@@ -1211,7 +1216,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
replay:
tp_created = 0;
- err = nlmsg_parse(n, sizeof(*t), tca, TCA_MAX, NULL, extack);
+ err = nlmsg_parse(n, sizeof(*t), tca, TCA_MAX, cls_tca_policy, extack);
if (err < 0)
return err;
@@ -1360,7 +1365,7 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN))
return -EPERM;
- err = nlmsg_parse(n, sizeof(*t), tca, TCA_MAX, NULL, extack);
+ err = nlmsg_parse(n, sizeof(*t), tca, TCA_MAX, cls_tca_policy, extack);
if (err < 0)
return err;
@@ -1475,7 +1480,7 @@ static int tc_get_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
void *fh = NULL;
int err;
- err = nlmsg_parse(n, sizeof(*t), tca, TCA_MAX, NULL, extack);
+ err = nlmsg_parse(n, sizeof(*t), tca, TCA_MAX, cls_tca_policy, extack);
if (err < 0)
return err;
@@ -1838,7 +1843,7 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
return -EPERM;
replay:
- err = nlmsg_parse(n, sizeof(*t), tca, TCA_MAX, NULL, extack);
+ err = nlmsg_parse(n, sizeof(*t), tca, TCA_MAX, cls_tca_policy, extack);
if (err < 0)
return err;
@@ -1949,7 +1954,8 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
if (nlmsg_len(cb->nlh) < sizeof(*tcm))
return skb->len;
- err = nlmsg_parse(cb->nlh, sizeof(*tcm), tca, TCA_MAX, NULL, NULL);
+ err = nlmsg_parse(cb->nlh, sizeof(*tcm), tca, TCA_MAX, cls_tca_policy,
+ NULL);
if (err)
return err;
--
2.17.1
^ permalink raw reply related
* Re: [RFC PATCH 05/11] net: ethernet: ti: cpsw: add support for port interface mode selection phy
From: Grygorii Strashko @ 2018-10-09 20:28 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, netdev, Tony Lindgren, Rob Herring,
Kishon Vijay Abraham I, Sekhar Nori, linux-kernel, linux-omap,
devicetree
In-Reply-To: <20181009005048.GB23588@lunn.ch>
Hi Andrew,
On 10/08/2018 07:50 PM, Andrew Lunn wrote:
>> /* Configure GMII_SEL register */
>> - cpsw_phy_sel(cpsw->dev, slave->phy->interface, slave->slave_num);
>> + if (!IS_ERR(slave->data->ifphy))
>> + phy_set_netif_mode(slave->data->ifphy, slave->data->phy_if);
>
> Is slave->data->phy_if also passed to phy_connect()? So you are going
> to end up with both the MAC and the PHY inserting RGMII delays, and it
> not working.
No. This logic not changed comparing to how it was.
* "rgmii" (RX and TX delays are added by the MAC when required)
rgmii_id = 0 --> CPSW: 0 : Internal Delay, PHY - no delay
* "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY, the
MAC should not add the RX or TX delays in this case)
* "rgmii-rxid" (RGMII with internal RX delay provided by the PHY, the MAC
should not add an RX delay in this case)
* "rgmii-txid" (RGMII with internal TX delay provided by the PHY, the MAC
should not add an TX delay in this case)
rgmii_id = 1 --> CPSW: 1 : No Internal Delay, PHY/board - delay
>
> You need to somehow decide if the MAC is going to do the delay, or the
> PHY. But not both.
Again, this series does not change logic - only interfaces and DT.
Thank you for review.
--
regards,
-grygorii
^ permalink raw reply
* [iproute PATCH] bridge: fdb: Fix for missing keywords in non-JSON output
From: Phil Sutter @ 2018-10-09 12:44 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
While migrating to JSON print library, some keywords were dropped from
standard output by accident. Add them back to unbreak output parsers.
Fixes: c7c1a1ef51aea ("bridge: colorize output and use JSON print library")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
bridge/fdb.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/bridge/fdb.c b/bridge/fdb.c
index 4dbc894ceab93..6487fac579c20 100644
--- a/bridge/fdb.c
+++ b/bridge/fdb.c
@@ -182,7 +182,7 @@ int print_fdb(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
if (!is_json_context())
fprintf(fp, "dev ");
print_color_string(PRINT_ANY, COLOR_IFNAME,
- "ifname", "%s ",
+ "ifname", "dev %s ",
ll_index_to_name(r->ndm_ifindex));
}
@@ -199,7 +199,7 @@ int print_fdb(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
print_color_string(PRINT_ANY,
ifa_family_color(family),
- "dst", "%s ", dst);
+ "dst", "dst %s ", dst);
}
if (vid)
@@ -246,7 +246,7 @@ int print_fdb(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg)
if (tb[NDA_MASTER])
- print_string(PRINT_ANY, "master", "%s ",
+ print_string(PRINT_ANY, "master", "master %s ",
ll_index_to_name(rta_getattr_u32(tb[NDA_MASTER])));
print_string(PRINT_ANY, "state", "%s\n",
--
2.19.0
^ permalink raw reply related
* Re: [RFC PATCH 02/11] dt-bindings: phy: add cpsw port interface mode selection phy bindings
From: Tony Lindgren @ 2018-10-09 20:30 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Rob Herring, Kishon Vijay Abraham I,
Sekhar Nori, linux-kernel, linux-omap, devicetree
In-Reply-To: <3fa09831-0f70-dfcc-3fd9-f877215a4631@ti.com>
* Grygorii Strashko <grygorii.strashko@ti.com> [181009 20:10]:
>
>
> On 10/09/2018 09:40 AM, Tony Lindgren wrote:
> > * Grygorii Strashko <grygorii.strashko@ti.com> [181008 23:54]:
> >> +Examples:
> >> + phy_gmii_sel: phy-gmii-sel {
> >> + compatible = "ti,am3352-phy-gmii-sel";
> >> + syscon-scm = <&scm_conf>;
> >> + #phy-cells = <2>;
> >> + };
> >
> > Now that this driver can live in it's proper place in the
>
> right
>
> > dts, you may want to consider just using standard reg
> > property for it instead of the syscon-scm. And also get
> > rid of the syscon reads and writes.
>
> Could you help clarify how to get syscon in this case?
> syscon_node_to_regmap(dev->parent->of_node)?
Hmm I don't think you need syscon at all now. You can just
ioremap the register(s) and use readl/writel and that's it.
Or use regmap without syscon if you prefer that.
The ioremap in this case should be hitting cached ranges
anyways, so no extra overhead there.
> Also, there are could be more then one gmii_sel registers in SCM in the future,
> so I hidden offsets in of_match data.
> As result, "reg" not needed at all now.
But then you have to patch driver for various SoCs
instead of just configuring the standard reg property
in the dts file :)
Regards,
Tony
^ permalink raw reply
* [PATCH] ethtool: fix a missing-check bug
From: Wenwen Wang @ 2018-10-09 13:15 UTC (permalink / raw)
To: Wenwen Wang
Cc: Kangjie Lu, David S. Miller, Florian Fainelli, Kees Cook,
Ilya Lesokhin, Edward Cree, Yury Norov, Alan Brady,
Eugenia Emantayev, Stephen Hemminger,
open list:NETWORKING [GENERAL], open list
In ethtool_get_rxnfc(), the eth command 'cmd' is compared against
'ETHTOOL_GRXFH' to see whether it is necessary to adjust the variable
'info_size'. Then the whole structure of 'info' is copied from the
user-space buffer 'useraddr' with 'info_size' bytes. In the following
execution, 'info' may be copied again from the buffer 'useraddr' depending
on the 'cmd' and the 'info.flow_type'. However, after these two copies,
there is no check between 'cmd' and 'info.cmd'. In fact, 'cmd' is also
copied from the buffer 'useraddr' in dev_ethtool(), which is the caller
function of ethtool_get_rxnfc(). Given that 'useraddr' is in the user
space, a malicious user can race to change the eth command in the buffer
between these copies. By doing so, the attacker can supply inconsistent
data and cause undefined behavior because in the following execution 'info'
will be passed to ops->get_rxnfc().
This patch adds a necessary check on 'info.cmd' and 'cmd' to confirm that
they are still same after the two copies in ethtool_get_rxnfc(). Otherwise,
an error code EINVAL will be returned.
Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
net/core/ethtool.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index c9993c6..0136625 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1015,6 +1015,9 @@ static noinline_for_stack int ethtool_get_rxnfc(struct net_device *dev,
return -EINVAL;
}
+ if (info.cmd != cmd)
+ return -EINVAL;
+
if (info.cmd == ETHTOOL_GRXCLSRLALL) {
if (info.rule_cnt > 0) {
if (info.rule_cnt <= KMALLOC_MAX_SIZE / sizeof(u32))
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next 04/19] net: usb: aqc111: Various callbacks implementation
From: Bjørn Mork @ 2018-10-09 13:27 UTC (permalink / raw)
To: Oliver Neukum
Cc: Igor Russkikh, David S . Miller, Dmitry Bezrukov,
linux-usb@vger.kernel.org, netdev@vger.kernel.org
In-Reply-To: <1539006434.10342.14.camel@suse.com>
Oliver Neukum <oneukum@suse.com> writes:
> On Fr, 2018-10-05 at 10:24 +0000, Igor Russkikh wrote:
>> From: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
>>
>> Reset, stop callbacks, driver unbind callback.
>> More register defines required for these callbacks.
>>
>> Signed-off-by: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
>> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
>> ---
>> drivers/net/usb/aqc111.c | 48 ++++++++++++++++++++++
>> drivers/net/usb/aqc111.h | 101 +++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 149 insertions(+)
>>
>> diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
>> index 7f3e5a615750..22bb259d71fb 100644
>> --- a/drivers/net/usb/aqc111.c
>> +++ b/drivers/net/usb/aqc111.c
>> @@ -169,12 +169,60 @@ static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf)
>>
>> static void aqc111_unbind(struct usbnet *dev, struct usb_interface *intf)
>> {
>> + u8 reg8;
>> + u16 reg16;
>> +
>> + /* Force bz */
>> + reg16 = SFR_PHYPWR_RSTCTL_BZ;
>> + aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_PHYPWR_RSTCTL,
>> + 2, 2, ®16);
>
> No, I am sorry, you are doing DMA on the kernel stack. That is not
> allowed. These functions will all have to be fixed.
Huh? No, he doesn't. That's the whole point with
usbnet_read_cmd_nopm(), isn't it?
Bjørn
^ 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