* Re: [PATCH] Revert "net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"
From: Mathieu Malaterre @ 2018-06-18 18:29 UTC (permalink / raw)
To: schwab
Cc: eric.dumazet, David S. Miller, Eric Dumazet, LKML,
Christophe LEROY, Meelis Roos, netdev, linuxppc-dev
In-Reply-To: <87o9g8geu0.fsf@igel.home>
On Mon, Jun 18, 2018 at 7:54 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Jun 17 2018, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> > Oh this is silly, please try :
> >
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index c642304f178ce0a4e1358d59e45032a39f76fb3f..54dd9c18ecad817812898d6f335e1794a07dabbe 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -1845,10 +1845,9 @@ EXPORT_SYMBOL(___pskb_trim);
> > int pskb_trim_rcsum_slow(struct sk_buff *skb, unsigned int len)
> > {
> > if (skb->ip_summed == CHECKSUM_COMPLETE) {
> > - int delta = skb->len - len;
> > + __wsum csumdiff = skb_checksum(skb, len, skb->len - len, 0);
> >
> > - skb->csum = csum_sub(skb->csum,
> > - skb_checksum(skb, len, delta, 0));
> > + skb->csum = csum_block_sub(skb->csum, csumdiff, len);
> > }
> > return __pskb_trim(skb, len);
> > }
>
> That doesn't help either.
seconded (setup g4+sungem):
[ 100.272676] eth0: hw csum failure
[ 100.272710] CPU: 0 PID: 0 Comm: swapper Not tainted 4.17.0+ #6
[ 100.272716] Call Trace:
[ 100.272733] [dffedbd0] [c069ddb8]
__skb_checksum_complete+0xf0/0x108 (unreliable)
[ 100.272752] [dffedbf0] [c078ea28] __udp4_lib_rcv+0x238/0xf98
[ 100.272767] [dffedc70] [c0731630] ip_local_deliver_finish+0xa8/0x3c4
[ 100.272777] [dffedcb0] [c073243c] ip_local_deliver+0xf0/0x154
[ 100.272786] [dffedcf0] [c07328e8] ip_rcv+0x448/0x774
[ 100.272800] [dffedd50] [c06aeaec] __netif_receive_skb_core+0x5e8/0x1184
[ 100.272811] [dffedde0] [c06bba2c] napi_gro_receive+0x160/0x22c
[ 100.272828] [dffede10] [e1571590] gem_poll+0x7fc/0x1ac0 [sungem]
[ 100.272837] [dffedee0] [c06bacfc] net_rx_action+0x34c/0x618
[ 100.272849] [dffedf60] [c07fd28c] __do_softirq+0x16c/0x5f0
[ 100.272863] [dffedfd0] [c0064c7c] irq_exit+0x110/0x1a8
[ 100.272877] [dffedff0] [c0016170] call_do_irq+0x24/0x3c
[ 100.272890] [c0cf7e80] [c0009a84] do_IRQ+0x98/0x1a0
[ 100.272900] [c0cf7eb0] [c001b474] ret_from_except+0x0/0x14
[ 100.272911] --- interrupt: 501 at arch_cpu_idle+0x30/0x78
LR = arch_cpu_idle+0x30/0x78
[ 100.272920] [c0cf7f70] [c0cf6000] 0xc0cf6000 (unreliable)
[ 100.272935] [c0cf7f80] [c00a3868] do_idle+0xc4/0x158
[ 100.272944] [c0cf7fb0] [c00a3ab4] cpu_startup_entry+0x24/0x28
[ 100.272955] [c0cf7fc0] [c0998820] start_kernel+0x47c/0x490
[ 100.272963] [c0cf7ff0] [00003444] 0x3444
> Andreas.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
^ permalink raw reply
* Re: [PATCH] Revert "net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"
From: Mathieu Malaterre @ 2018-06-18 18:45 UTC (permalink / raw)
To: eric.dumazet
Cc: schwab, David S. Miller, Eric Dumazet, LKML, Christophe LEROY,
Meelis Roos, netdev, linuxppc-dev
In-Reply-To: <816ef746-5278-1d51-1d9d-55593e377681@gmail.com>
On Mon, Jun 18, 2018 at 8:18 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 06/18/2018 10:54 AM, Andreas Schwab wrote:
> > On Jun 17 2018, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> >> Oh this is silly, please try :
> >>
> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >> index c642304f178ce0a4e1358d59e45032a39f76fb3f..54dd9c18ecad817812898d6f335e1794a07dabbe 100644
> >> --- a/net/core/skbuff.c
> >> +++ b/net/core/skbuff.c
> >> @@ -1845,10 +1845,9 @@ EXPORT_SYMBOL(___pskb_trim);
> >> int pskb_trim_rcsum_slow(struct sk_buff *skb, unsigned int len)
> >> {
> >> if (skb->ip_summed == CHECKSUM_COMPLETE) {
> >> - int delta = skb->len - len;
> >> + __wsum csumdiff = skb_checksum(skb, len, skb->len - len, 0);
> >>
> >> - skb->csum = csum_sub(skb->csum,
> >> - skb_checksum(skb, len, delta, 0));
> >> + skb->csum = csum_block_sub(skb->csum, csumdiff, len);
> >> }
> >> return __pskb_trim(skb, len);
> >> }
> >
> > That doesn't help either.
> >
> > Andreas.
> >
>
> Then maybe NIC provided csum is not correct.
>
> It does not compute a checksum on all the frame, but part of it.
>
> You could use this patch to double check.
>
> diff --git a/drivers/net/ethernet/sun/sungem.c b/drivers/net/ethernet/sun/sungem.c
> index 7a16d40a72d13cf1d522e8a3a396c826fe76f9b9..277859ea73e35271a10b02011120fca248ec8e71 100644
> --- a/drivers/net/ethernet/sun/sungem.c
> +++ b/drivers/net/ethernet/sun/sungem.c
> @@ -857,6 +857,11 @@ static int gem_rx(struct gem *gp, int work_to_do)
>
> csum = (__force __sum16)htons((status & RXDCTRL_TCPCSUM) ^ 0xffff);
> skb->csum = csum_unfold(csum);
> + {
> + __wsum rsum = csum_partial(skb->data + ETH_HLEN, len - ETH_HLEN, 0);
> + if (csum != csum_fold(rsum))
> + pr_err_ratelimited("sungem wrong csum : %x/%x, len %u bytes\n", csum, csum_fold(rsum), len);
> + }
> skb->ip_summed = CHECKSUM_COMPLETE;
> skb->protocol = eth_type_trans(skb, gp->dev);
>
>
Here is what I get on my side
[ 53.628847] sungem: sungem wrong csum : 4e04/f97, len 64 bytes
[ 53.667063] sungem: sungem wrong csum : eea8/6eec, len 149 bytes
[ 58.648952] sungem: sungem wrong csum : 2095/3d06, len 64 bytes
[ 58.669414] sungem: sungem wrong csum : 5245/b50, len 149 bytes
[ 63.674451] sungem: sungem wrong csum : 2d8/5abd, len 149 bytes
[ 68.678233] sungem: sungem wrong csum : b8fc/a498, len 149 bytes
[ 73.685771] sungem: sungem wrong csum : 374/5a21, len 149 bytes
[ 78.689089] sungem: sungem wrong csum : d81/5014, len 149 bytes
[ 83.683261] sungem: sungem wrong csum : 4e04/f97, len 64 bytes
[ 83.690193] sungem: sungem wrong csum : c2f7/9a9d, len 149 bytes
[ 88.692511] sungem: sungem wrong csum : f4d8/68bc, len 149 bytes
[ 93.699915] sungem: sungem wrong csum : 1370/4a25, len 149 bytes
[ 98.703136] sungem: sungem wrong csum : e1b5/7bdf, len 149 bytes
[ 103.704230] sungem: sungem wrong csum : 5321/a74, len 149 bytes
[ 108.688912] sungem: sungem wrong csum : 2095/3d06, len 64 bytes
[ 108.706559] sungem: sungem wrong csum : ddbc/7fd8, len 149 bytes
[ 113.713189] sungem: sungem wrong csum : 5a65/330, len 149 bytes
[ 113.891697] sungem: sungem wrong csum : 4e04/f97, len 64 bytes
[ 118.717151] sungem: sungem wrong csum : f7c8/65cc, len 149 bytes
[ 123.722680] sungem: sungem wrong csum : 3d7a/201b, len 149 bytes
[ 128.726524] sungem: sungem wrong csum : c8fd/9497, len 149 bytes
[ 133.732045] sungem: sungem wrong csum : de0d/7f87, len 149 bytes
[ 135.529163] sungem: sungem wrong csum : 3089/b6dd, len 96 bytes
[ 135.529208] eth0: hw csum failure
[ 135.529220] CPU: 0 PID: 0 Comm: swapper Not tainted 4.17.0+ #7
[ 135.529226] Call Trace:
[ 135.529243] [dffedbe0] [c069ddac]
__skb_checksum_complete+0xf0/0x108 (unreliable)
>
>
>
^ permalink raw reply
* Re: [PATCH v3] bpf: attach type BPF_LIRC_MODE2 should not depend on CONFIG_CGROUP_BPF
From: kbuild test robot @ 2018-06-18 18:46 UTC (permalink / raw)
To: Sean Young
Cc: kbuild-all, Daniel Borkmann, Y Song, Matthias Reichl, linux-media,
LKML, Alexei Starovoitov, Mauro Carvalho Chehab, netdev,
Devin Heitmueller, Quentin Monnet
In-Reply-To: <20180618171216.gearpr755pm3wot7@gofer.mess.org>
[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]
Hi Sean,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc1 next-20180618]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sean-Young/bpf-attach-type-BPF_LIRC_MODE2-should-not-depend-on-CONFIG_CGROUP_BPF/20180619-023056
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
In file included from kernel///events/core.c:45:0:
>> include/linux/bpf.h:710:1: error: expected identifier or '(' before '{' token
{
^
vim +710 include/linux/bpf.h
707
708 int sockmap_get_from_fd(const union bpf_attr *attr, int type,
709 struct bpf_prog *prog);
> 710 {
711 return -EINVAL;
712 }
713 #endif
714
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6370 bytes --]
^ permalink raw reply
* [PATCH iproute2-next 1/1] tc: jsonify nat action
From: Keara Leibovitz @ 2018-06-18 18:57 UTC (permalink / raw)
To: dsahern; +Cc: stephen, netdev, kernel, Keara Leibovitz
Add json output support for nat action
Example output:
~$ $TC actions add action nat egress 10.10.10.1 20.20.20.2 index 2
~$ $TC actions add action nat ingress 100.100.100.1/32 200.200.200.2 \
continue index 99
~$ $TC -j actions ls action nat
[{
"total acts": 2
}, {
"actions": [{
"order": 0,
"type": "nat",
"direction": "egress",
"old_addr": "10.10.10.1/32",
"new_addr": "20.20.20.2",
"control_action": {
"type": "pass"
},
"index": 2,
"ref": 1,
"bind": 0
}, {
"order": 1,
"type": "nat",
"direction": "ingress",
"old_addr": "100.100.100.1/32",
"new_addr": "200.200.200.2",
"control_action": {
"type": "continue"
},
"index": 99,
"ref": 1,
"bind": 0
}]
}]
Signed-off-by: Keara Leibovitz <kleib@mojatatu.com>
---
tc/m_nat.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/tc/m_nat.c b/tc/m_nat.c
index 653792da91c0..ee0b7520a605 100644
--- a/tc/m_nat.c
+++ b/tc/m_nat.c
@@ -142,9 +142,8 @@ print_nat(struct action_util *au, FILE * f, struct rtattr *arg)
{
struct tc_nat *sel;
struct rtattr *tb[TCA_NAT_MAX + 1];
- char buf1[256];
- char buf2[256];
-
+ SPRINT_BUF(buf1);
+ SPRINT_BUF(buf2);
int len;
if (arg == NULL)
@@ -153,7 +152,7 @@ print_nat(struct action_util *au, FILE * f, struct rtattr *arg)
parse_rtattr_nested(tb, TCA_NAT_MAX, arg);
if (tb[TCA_NAT_PARMS] == NULL) {
- fprintf(f, "[NULL nat parameters]");
+ print_string(PRINT_FP, NULL, "%s", "[NULL nat parameters]");
return -1;
}
sel = RTA_DATA(tb[TCA_NAT_PARMS]);
@@ -161,15 +160,22 @@ print_nat(struct action_util *au, FILE * f, struct rtattr *arg)
len = ffs(sel->mask);
len = len ? 33 - len : 0;
- fprintf(f, " nat %s %s/%d %s", sel->flags & TCA_NAT_FLAG_EGRESS ?
- "egress" : "ingress",
- format_host_r(AF_INET, 4, &sel->old_addr, buf1, sizeof(buf1)),
- len,
- format_host_r(AF_INET, 4, &sel->new_addr, buf2, sizeof(buf2)));
- print_action_control(f, " ", sel->action, "");
+ print_string(PRINT_ANY, "type", " %s ", "nat");
+ print_string(PRINT_ANY, "direction", "%s",
+ sel->flags & TCA_NAT_FLAG_EGRESS ? "egress" : "ingress");
- fprintf(f, "\n\t index %u ref %d bind %d",
- sel->index, sel->refcnt, sel->bindcnt);
+ snprintf(buf2, sizeof(buf2), "%s/%d",
+ format_host_r(AF_INET, 4, &sel->old_addr, buf1, sizeof(buf1)),
+ len);
+ print_string(PRINT_ANY, "old_addr", " %s", buf2);
+ print_string(PRINT_ANY, "new_addr", " %s",
+ format_host_r(AF_INET, 4, &sel->new_addr, buf1, sizeof(buf1)));
+
+ print_action_control(f, " ", sel->action, "");
+ print_string(PRINT_FP, NULL, "%s", _SL_);
+ print_uint(PRINT_ANY, "index", "\t index %u", sel->index);
+ print_int(PRINT_ANY, "ref", " ref %d", sel->refcnt);
+ print_int(PRINT_ANY, "bind", " bind %d", sel->bindcnt);
if (show_stats) {
if (tb[TCA_NAT_TM]) {
@@ -179,7 +185,7 @@ print_nat(struct action_util *au, FILE * f, struct rtattr *arg)
}
}
- fprintf(f, "\n");
+ print_string(PRINT_FP, NULL, "%s", _SL_);
return 0;
}
--
2.7.4
^ permalink raw reply related
* Re: 4.14.(44->48) IPv6 RA issue?
From: Ido Schimmel @ 2018-06-18 19:09 UTC (permalink / raw)
To: valdis.kletnieks; +Cc: netdev, linux-kernel
In-Reply-To: <35958.1529334166@turing-police.cc.vt.edu>
On Mon, Jun 18, 2018 at 11:02:46AM -0400, valdis.kletnieks@vt.edu wrote:
> So I'm trying to troubleshoot an issue on an OpenWRT/Lede based
> router, where IPv6 connectivity totally fails. I've bisected it down to:
>
> git log --oneline 187da94808a634477b5e5a69109ea0c566dfa64b..73d8a6ab7668173d70adbed45b61be5256c505e
> 73d8a6ab7668 (refs/bisect/bad) base-files: fix UCI config parsing and callback handling
> e52f3e9b1376 kernel: bump 4.14 to 4.14.48
> 7590c3c58f5e (HEAD) scripts: Replace obsolete POSIX tmpnam in slugimage.pl with File::Temp function
> 987900f2de76 hostapd: properly build hostapd-only SSL variants
>
> and am pretty sure that it's the kernel bump (works with a 4.14.44 kernel,
> breaks with 4.14.48) as the other 3 commits don't go anywhere near IPv6 handling.
...
> Note that eth1 is the uplink towards my ISP. I've pointed a 'tcpdump -n -i eth1 ip6'
> at it, and see plenty of RA packets come in, but neighbor discovery never completes.
> Looking at the Changelogs for .45->.50 don't show any smoking-gun patches.
>
> This ring any bells, before I delve deeper into it?
If this is a router and forwarding is enabled, then you need to set
'accept_ra' to '2' [1]. Is this what you have configured? Seems that
OpenWRT recently started to explicitly set 'accept_ra' to '0' [2].
1. https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt
2. https://github.com/openwrt/openwrt/commit/bb46520159c0119e829900e29681feea6f297fe0
^ permalink raw reply
* Re: [PATCH] optoe: driver to read/write SFP/QSFP EEPROMs
From: Don Bollinger @ 2018-06-18 19:13 UTC (permalink / raw)
To: Florian Fainelli
Cc: Andrew Lunn, Tom Lendacky, Arnd Bergmann, Greg Kroah-Hartman,
linux-kernel, brandon_chuang, wally_wang, roy_lee, rick_burchett,
quentin.chang, steven.noble, jeffrey.townsend, scotte, roopa,
David Ahern, luke.williams, Guohan Lu, Russell King,
netdev@vger.kernel.org, don
In-Reply-To: <ef0baf6d-1d1f-ba5b-708a-d8179bd1ea3b@gmail.com>
On Thu, Jun 14, 2018 at 08:24:34PM -0700, Florian Fainelli wrote:
>
>
> On 06/14/2018 07:26 PM, Don Bollinger wrote:
> > On Tue, Jun 12, 2018 at 08:11:09PM +0200, Andrew Lunn wrote:
> >>> There's an SFP driver under drivers/net/phy. Can that driver be extended
> >>> to provide this support? Adding Russel King who developed sfp.c, as well
> >>> at the netdev mailing list.
> >>
> >> I agree, the current SFP code should be used.
> >>
> >> My observations seem to be there are two different ways {Q}SFP are used:
> >>
> >> 1) The Linux kernel has full control, as assumed by the devlink/SFP
> >> frame work. We parse the SFP data to find the capabilities of the SFP
> >> and use it to program the MAC to use the correct mode. The MAC can be
> >> a NIC, but it can also be a switch. DSA is gaining support for
> >> PHYLINK, so SFP modules should just work with most switches which DSA
> >> support. And there is no reason a plain switchdev switch can not use
> >> PHYLINK.
> >>
> >> 2) Firmware is in control of the PHY layer, but there is a wish to
> >> expose some of the data which is available via i2c from the {Q}SFP to
> >> linux.
> >>
> >> It appears this optoe supports this second case. It does not appear to
> >> support any in kernel API to actually make use of the SFP data in the
> >> kernel.
> >>
> >> We should not be duplicating code. We should share the SFP code for
> >> both use cases above. There is also a Linux standard API for getting
> >> access to this information. ethtool -m/--module-info. Anything which
> >> is exporting {Q}SFP data needs to use this API.
> >>
> >> Andrew
> >
> > Actually this is better described by a third use case. The target
> > switches are PHY-less (see various designs at
> > www.compute.org/wiki/Networking/SpecsAndDesigns). The AS5712 for example
> > says "The AS5712-54X is a PHY-Less design with the SFP+ and QSFP+
> > connections directly attaching to the Serdes interfaces of the Broadcom
> > BCM56854 720G Trident 2 switching silicon..."
> >
> > The electrical controls of the {Q}SFP devices (TxDisable for example)
> > are organized in a platform dependent way, through CPLD devices, and
> > managed by a platform specific CPLD driver.
> >
> > The i2c bus is muxed from the CPU to all of the {Q}SFP devices, which
> > are set up as standard linux i2c devices
> > (/sys/bus/i2c/devices/i2c-xxxx).
> >
> > There is no MDIO bus between the CPU and the {Q}SFP devices.
> >
> >> 2) Firmware is in control of the PHY layer, but there is a wish to
> >> expose some of the data which is available via i2c from the {Q}SFP to
> >> linux.
> >
> > So the switch silicon is in control of the PHY layer. The platform
> > driver is in control of the electrical interfaces. And the EEPROM data
> > is available via I2C.
> >
> > And, there isn't actually 'a wish to expose' the EEPROM data to linux
> > (the kernel). It turns out that none of the NOS partners I'm working
> > with use that data *in the kernel*. It is all managed from user space.
> >
> > More generally, I think sfp.c and optoe are not actually trying to
> > accomplish the same thing at all. sfp.c combines all three functions
> > (PHY, electrical control, EEPROM access). optoe is only providing EEPROM
> > access, and only to user space. This is a real need in the white box
> > switch environment, and is not met by sfp.c. optoe isn't better, sfp.c
> > isn't better, they're just different.
>
> sfp exposes standard ethtool hooks such as get_module_info() which pass
> the whole data blob to user-space, e.g: ethtool where all of this is
> better interpreted.
Yes. But ethtool depends on the underlying driver to access the data.
The current sfp.c implementation limits the amount of data that can be
accessed. optoe makes the entire EEPROM accessible. I think the right
solution is to call optoe for access to the EEPROM. A couple of lines
of code in sfp_i2c_read could call optoe to get the data instead of
formatting the i2c_transfer directly. That change would immediately
give the whole SFP framework access to all the address space of both SFP
and QSFP. (Same change to sfp_i2c_write.)
>
> >
> > Finally, sfp.c does not recognize that SFP devices have data beyond 512
> > bytes, accessible via a page register. It also does not recognize QSFP
> > devices at all. QSFP devices have only 256 bytes accessible (one i2c
> > address) before switching to paged access for the remaining data. The
> > first design requirement for optoe was to access all the available
> > pages, because there is information and controls that we (optics
> > vendors) want to make available to network management applications.
>
> Patches welcome if you wish to extend sfp.c to support QSFP devices for
> instances.
I would love to collaborate on this. I don't have an environment
(hardware or software) where I could build or test changes to the sfp
code.
>
> >
> > If sfp.c creates a standard linux i2c client for each SFP device, it
> > should be possible to create an optoe managed device 'under' sfp.c to
> > provide access to the full EEPROM address space:
>
> It's the other way around, SFP relies on a standard Linux i2c bus master
> to exist such that it can read the EEPROM from the standard slave
> address location, same goes with a possibly present PHY.
Great. Then plugging optoe in should be easy.
>
> > # echo optoe2 0x50 >/sys/bus/i2c/devices/i2c-xx/new_device
> > This might prove useful to user space consumers of that data. We could
> > also easily add a kernel API (eg the nvmem framework) to optoe to provide
> > kernel access. In other words, sfp.c could assign EEPROM management to
> > optoe, while managing the electrical interfaces. (This is actually
> > pretty close to how the platfom drivers work in the switch environment.)
> > sfp.c would get SFP page support and QSFP EEPROM access 'for free'.
>
> That sounds like a possibly good approach.
Thanks
>
> >
> >> There is also a Linux standard API for getting
> >> access to this information. ethtool -m/--module-info. Anything which
> >> is exporting {Q}SFP data needs to use this API.
> >
> > optoe simply provides direct access from user space to the full EEPROM
> > data. There is more data there than ethtool knows about, and in some
> > devices there are proprietary registers that ethtool will never know
> > about. optoe does not interpret any of the EEPROM content (except the
> > bare minimum to access pages correctly). optoe also does not get in the
> > way of ethtool. It could prove to be a handy way for ethtool to access
> > new EEPROM fields in the future. QSFP-DD/OSFP are coming soon, they
> > will have a different (incompatible) set of new fields to be decoded.
>
> sfp is the same it only passes the EEPROM information to user-space and
> interprets just what it needs to get the work done.
I offer include/linux/sfp.h as a counter example. Every byte, every bit
in the spec is spec'ed there. That's great, but exceeds the mandate of
optoe. Optoe is just there to get the data in and out of the EEPROM.
>
> >
> > Bottom Line: sfp.c is not a useful starting point for the switch
> > environment I'm working in. The underlying hardware architecture is
> > quite different. optoe is not a competing alternative. Its only
> > function is to provide user-space access to the EEPROM data in {Q}SFP
> > devices.
>
> I just don't understand why would we want optoe when the standard way to
> expose both EEPROM and diagnostics modules has been through ethtool's
> get_module_info since the basic paradigm is that a network port is a
> net_device instance in the kernel. If that basic device driver model
> does not exist, then it is unclear to me what are the benefits.
We want optoe to access regions of the EEPROM which are paged, and to
access QSFP which only has one I2C address and is also paged. It is
just the sfp_read/sfp_write function, but reading and writing the whole
EEPROM. Plugging it into sfp gives that broader access to the ethtool
interface and the rest of the net-device model.
>
> Would I be completely wrong if I wrote that you are likely working with
> a switch SDK which primarily runs in user-space and so with lack of a
> proper kernel-based device driver for your piece of hardware you are
> attempting to create a driver which is some sort of a "tap" for some
> specific portion of that larger hardware block?
Not completely wrong, but biased. The switch ASIC and the switch SDK do
not connect to the i2c bus (at least in the architecture I'm working
with). Whether or not it is in user-space isn't the point. That it
doesn't access i2c, hence doesn't access the EEPROM, is the reason we
have a 'proper kernel-based device driver'.
> --
> Florian
>
Don
^ permalink raw reply
* Re: WARNING: CPU: 3 PID: 0 at net/sched/sch_hfsc.c:1388 hfsc_dequeue+0x319/0x350 [sch_hfsc]
From: Cong Wang @ 2018-06-18 19:28 UTC (permalink / raw)
To: Marco Berizzi; +Cc: Linux Kernel Network Developers
In-Reply-To: <1938577710.309243.1528180788286@mail.libero.it>
[-- Attachment #1: Type: text/plain, Size: 867 bytes --]
On Mon, Jun 4, 2018 at 11:39 PM, Marco Berizzi <pupilla@libero.it> wrote:
>> Il 8 marzo 2018 alle 17.02 Marco Berizzi <pupilla@libero.it> ha scritto:
>>
>> > Marco Berizzi wrote:
>> >
>> > Hello everyone,
>> >
>> > Yesterday I got this error on a slackware linux 4.16-rc4 system
>> > running as a traffic shaping gateway and netfilter nat.
>> > The error has been arisen after a partial ISP network outage,
>> > so unfortunately it will not trivial for me to reproduce it again.
>>
>> Hello everyone,
>>
>> I'm getting this error twice/day, so fortunately I'm able to
>> reproduce it.
>
> I'm getting the same error also with 4.17 (4.15.18 was
> the latest working version):
Can you test the attached patch?
It almost certainly fixes the warning, but I am not sure if
it papers out any other real problem. Please make sure
hfsc still works as expected. :)
Thanks.
[-- Attachment #2: hfsc.diff --]
[-- Type: application/octet-stream, Size: 715 bytes --]
commit 3df066c835cdb6dd5ee093292e1c41ae584fafa3
Author: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon Jun 18 11:59:47 2018 -0700
net_sched: remove a bogus warning in hfsc
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 3ae9877ea205..3278a76f6861 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -1385,8 +1385,8 @@ hfsc_schedule_watchdog(struct Qdisc *sch)
if (next_time == 0 || next_time > q->root.cl_cfmin)
next_time = q->root.cl_cfmin;
}
- WARN_ON(next_time == 0);
- qdisc_watchdog_schedule(&q->watchdog, next_time);
+ if (next_time)
+ qdisc_watchdog_schedule(&q->watchdog, next_time);
}
static int
^ permalink raw reply related
* [PATCH net] net/tcp: Fix socket lookups with SO_BINDTODEVICE
From: dsahern @ 2018-06-18 19:30 UTC (permalink / raw)
To: netdev; +Cc: davem, lberger, renato, David Ahern
From: David Ahern <dsahern@gmail.com>
Similar to 69678bcd4d2d ("udp: fix SO_BINDTODEVICE"), TCP socket lookups
need to fail if dev_match is not true. Currently, a packet to a given port
can match a socket bound to device when it should not. In the VRF case,
this causes the lookup to hit a VRF socket and not a global socket
resulting in a response trying to go through the VRF when it should it.
Fixes: 3fa6f616a7a4d ("net: ipv4: add second dif to inet socket lookups")
Fixes: 4297a0ef08572 ("net: ipv6: add second dif to inet6 socket lookups")
Reported-by: Lou Berger <lberger@labn.net>
Diagnosed-by: Renato Westphal <renato@opensourcerouting.org>
Tested-by: Renato Westphal <renato@opensourcerouting.org>
Signed-off-by: David Ahern <dsahern@gmail.com>
---
net/ipv4/inet_hashtables.c | 4 ++--
net/ipv6/inet6_hashtables.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 31ff46daae97..3647167c8fa3 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -243,9 +243,9 @@ static inline int compute_score(struct sock *sk, struct net *net,
bool dev_match = (sk->sk_bound_dev_if == dif ||
sk->sk_bound_dev_if == sdif);
- if (exact_dif && !dev_match)
+ if (!dev_match)
return -1;
- if (sk->sk_bound_dev_if && dev_match)
+ if (sk->sk_bound_dev_if)
score += 4;
}
if (sk->sk_incoming_cpu == raw_smp_processor_id())
diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
index 2febe26de6a1..595ad408dba0 100644
--- a/net/ipv6/inet6_hashtables.c
+++ b/net/ipv6/inet6_hashtables.c
@@ -113,9 +113,9 @@ static inline int compute_score(struct sock *sk, struct net *net,
bool dev_match = (sk->sk_bound_dev_if == dif ||
sk->sk_bound_dev_if == sdif);
- if (exact_dif && !dev_match)
+ if (!dev_match)
return -1;
- if (sk->sk_bound_dev_if && dev_match)
+ if (sk->sk_bound_dev_if)
score++;
}
if (sk->sk_incoming_cpu == raw_smp_processor_id())
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v3 06/14] mtd: rawnand: marvell: remove the dmaengine compat need
From: Robert Jarzmik @ 2018-06-18 19:31 UTC (permalink / raw)
To: Daniel Mack
Cc: Ulf Hansson, alsa-devel, linux-kernel, linux-ide, linux-mtd,
Miquel Raynal, Mauro Carvalho Chehab, Vinod Koul,
Richard Weinberger, Takashi Iwai, Marek Vasut, linux-media,
Bartlomiej Zolnierkiewicz, Haojian Zhuang, Boris Brezillon,
Mark Brown, linux-arm-kernel, Nicolas Pitre, netdev, linux-mmc,
Liam Girdwood, dmaengine, Tejun Heo, Brian Norris,
David Woodhouse
In-Reply-To: <3a2a8951-f380-af99-bf97-6ff722404410@zonque.org>
Daniel Mack <daniel@zonque.org> writes:
> On Sunday, June 17, 2018 07:02 PM, Robert Jarzmik wrote:
>> As the pxa architecture switched towards the dmaengine slave map, the
>> old compatibility mechanism to acquire the dma requestor line number and
>> priority are not needed anymore.
>>
>> This patch simplifies the dma resource acquisition, using the more
>> generic function dma_request_slave_channel().
>>
>> Signed-off-by: Signed-off-by: Daniel Mack <daniel@zonque.org>
>
> Something went wrong here, but you can simply fix that when applying the series
> :)
Indeed, fixed before applying to the pxa/for-next tree.
--
Robert
^ permalink raw reply
* Re: [PATCH net-next v3 2/2] r8169: Reinstate ASPM Support
From: Heiner Kallweit @ 2018-06-18 19:33 UTC (permalink / raw)
To: Kai-Heng Feng, davem
Cc: ryankao, hayeswang, hau, romieu, bhelgaas, acelan.kao, netdev,
linux-pci, linux-kernel
In-Reply-To: <20180615053219.14053-2-kai.heng.feng@canonical.com>
On 15.06.2018 07:32, Kai-Heng Feng wrote:
> On Intel Coffe Lake platforms, ASPM support in r8169 is the last missing
> puzzle to let CPU's Package C-State reaches PC8.
> Without ASPM support, the CPU cannot reach beyond PC3. PC8 can save
> additional ~3W in comparison with PC3 on my testing platform, Dell G3
> 3779.
>
The patch itself looks good to me.
Still I think the commit message should be improved. Currently it sounds
like the patch affects Coffee Lake systems only. But disabled ASPM
prevents the system from reaching PC states beyond PC3 on more than
one platform. You just tested on Coffee Lake only.
And it would be good if you mention the RTL8168 chip version of
the system you tested on.
Finally one smaller remark below.
> This is based on the work from Chunhao Lin <hau@realtek.com>.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v3:
> - Change commit message wording.
> - Rename the function to rtl_hw_aspm_clkreq_enable().
>
> v2:
> - Remove module parameter.
> - Remove pci_disable_link_state().
>
> drivers/net/ethernet/realtek/r8169.c | 40 +++++++++++++++++++---------
> 1 file changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 9b55ce513a36..269ac7561368 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -5289,6 +5289,17 @@ static void rtl_pcie_state_l2l3_enable(struct rtl8169_private *tp, bool enable)
> RTL_W8(tp, Config3, data);
> }
>
> +static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
> +{
> + if (enable) {
> + RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
> + RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
> + } else {
> + RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> + RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> + }
> +}
> +
> static void rtl_hw_start_8168bb(struct rtl8169_private *tp)
> {
> RTL_W8(tp, Config3, RTL_R8(tp, Config3) & ~Beacon_en);
> @@ -5645,9 +5656,9 @@ static void rtl_hw_start_8168g_1(struct rtl8169_private *tp)
> rtl_hw_start_8168g(tp);
>
> /* disable aspm and clock request before access ephy */
> - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> + rtl_hw_aspm_clkreq_enable(tp, false);
> rtl_ephy_init(tp, e_info_8168g_1, ARRAY_SIZE(e_info_8168g_1));
> + rtl_hw_aspm_clkreq_enable(tp, true);
> }
>
> static void rtl_hw_start_8168g_2(struct rtl8169_private *tp)
> @@ -5680,9 +5691,9 @@ static void rtl_hw_start_8411_2(struct rtl8169_private *tp)
> rtl_hw_start_8168g(tp);
>
> /* disable aspm and clock request before access ephy */
> - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> + rtl_hw_aspm_clkreq_enable(tp, false);
> rtl_ephy_init(tp, e_info_8411_2, ARRAY_SIZE(e_info_8411_2));
> + rtl_hw_aspm_clkreq_enable(tp, true);
> }
>
> static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
> @@ -5699,8 +5710,7 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
> };
>
> /* disable aspm and clock request before access ephy */
> - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> + rtl_hw_aspm_clkreq_enable(tp, false);
> rtl_ephy_init(tp, e_info_8168h_1, ARRAY_SIZE(e_info_8168h_1));
>
> RTL_W32(tp, TxConfig, RTL_R32(tp, TxConfig) | TXCFG_AUTO_FIFO);
> @@ -5779,6 +5789,8 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
> r8168_mac_ocp_write(tp, 0xe63e, 0x0000);
> r8168_mac_ocp_write(tp, 0xc094, 0x0000);
> r8168_mac_ocp_write(tp, 0xc09e, 0x0000);
> +
> + rtl_hw_aspm_clkreq_enable(tp, true);
> }
>
> static void rtl_hw_start_8168ep(struct rtl8169_private *tp)
> @@ -5830,11 +5842,12 @@ static void rtl_hw_start_8168ep_1(struct rtl8169_private *tp)
> };
>
> /* disable aspm and clock request before access ephy */
> - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> + rtl_hw_aspm_clkreq_enable(tp, false);
> rtl_ephy_init(tp, e_info_8168ep_1, ARRAY_SIZE(e_info_8168ep_1));
>
> rtl_hw_start_8168ep(tp);
> +
> + rtl_hw_aspm_clkreq_enable(tp, true);
> }
>
> static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp)
> @@ -5846,14 +5859,15 @@ static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp)
> };
>
> /* disable aspm and clock request before access ephy */
> - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> + rtl_hw_aspm_clkreq_enable(tp, false);
> rtl_ephy_init(tp, e_info_8168ep_2, ARRAY_SIZE(e_info_8168ep_2));
>
> rtl_hw_start_8168ep(tp);
>
> RTL_W8(tp, DLLPR, RTL_R8(tp, DLLPR) & ~PFM_EN);
> RTL_W8(tp, MISC_1, RTL_R8(tp, MISC_1) & ~PFM_D3COLD_EN);
> +
> + rtl_hw_aspm_clkreq_enable(tp, true);
> }
>
> static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
> @@ -5867,8 +5881,7 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
> };
>
> /* disable aspm and clock request before access ephy */
> - RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
> - RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
> + rtl_hw_aspm_clkreq_enable(tp, false);
> rtl_ephy_init(tp, e_info_8168ep_3, ARRAY_SIZE(e_info_8168ep_3));
>
> rtl_hw_start_8168ep(tp);
> @@ -5888,6 +5901,8 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
> data = r8168_mac_ocp_read(tp, 0xe860);
> data |= 0x0080;
> r8168_mac_ocp_write(tp, 0xe860, data);
> +
> + rtl_hw_aspm_clkreq_enable(tp, true);
> }
>
> static void rtl_hw_start_8168(struct rtl8169_private *tp)
> @@ -7646,7 +7661,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> mii->reg_num_mask = 0x1f;
> mii->supports_gmii = cfg->has_gmii;
>
> -
This should go to patch 1.
> /* enable device (incl. PCI PM wakeup and hotplug setup) */
> rc = pcim_enable_device(pdev);
> if (rc < 0) {
>
^ permalink raw reply
* Re: [PATCH] optoe: driver to read/write SFP/QSFP EEPROMs
From: Don Bollinger @ 2018-06-18 19:41 UTC (permalink / raw)
To: Andrew Lunn
Cc: Tom Lendacky, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel,
brandon_chuang, wally_wang, roy_lee, rick_burchett, quentin.chang,
steven.noble, jeffrey.townsend, scotte, roopa, David Ahern,
luke.williams, Guohan Lu, Russell King, netdev@vger.kernel.org,
don
In-Reply-To: <20180615075417.GA28730@lunn.ch>
On Fri, Jun 15, 2018 at 09:54:17AM +0200, Andrew Lunn wrote:
> > Actually this is better described by a third use case. The target
> > switches are PHY-less (see various designs at
> > www.compute.org/wiki/Networking/SpecsAndDesigns). The AS5712 for example
> > says "The AS5712-54X is a PHY-Less design with the SFP+ and QSFP+
> > connections directly attaching to the Serdes interfaces of the Broadcom
> > BCM56854 720G Trident 2 switching silicon..."
>
> We consider the SFP+ and QSFP+ as being the PHY. You need something to
> control that PHY. Either it is firmware running in the switch, or it
> is the Linux kernel, via PHYLINK.
Actually in the environment I'm working in (at least 3 different NOS
vendors, and 3 different switch vendors), the {Q}SFP devices are
controlled by a platform specific driver that pokes CPLD registers. I'm
not sure where the actual control logic is, but it doesn't appear to use
any of the frameworks you are describing.
>
> > The i2c bus is muxed from the CPU to all of the {Q}SFP devices, which
> > are set up as standard linux i2c devices
> > (/sys/bus/i2c/devices/i2c-xxxx).
>
> Having a standard i2c bus driver is correct. This is what PHYLINK
> assumes. It knows about the different addresses the SFP uses on the
> i2c bus.
Great. Then plugging optoe into it should be easy.
>
> > There is no MDIO bus between the CPU and the {Q}SFP devices.
>
> There is no physical MDIO bus for SFP devices. If the SFP module
> implements copper 1G, there is often MDIO tunnelled over i2c. PHYLINK
> knows how to do this, and will instantiate a normal Linux MDIO bus
> driver, and then you can use the Linux kernel copper PHY state
> machines as normal.
>
> > And, there isn't actually 'a wish to expose' the EEPROM data to linux
> > (the kernel). It turns out that none of the NOS partners I'm working
> > with use that data *in the kernel*. It is all managed from user space.
>
> Ah. O.K. We can stop here then.
>
> If you are using Linux as a boot loader, i doubt you will find any
> network kernel developers who are willing to consider this driver. The
It isn't a boot loader. It is the kernel that is running on the switch
when it is doing its switch thing. The kernel hosts the drivers and the
switch SDK and all the apps that configure and manage the networking.
> kernel community as decided switchdev is how the Linux kernel supports
I'm sure switchdev works very well. It is not being used in the
environment I am trying to support. I've checked all 3 NOSs that have
adopted optoe, none of them have switchdev configured in their .config
file:
# CONFIG_NET_SWITCHDEV is not set
> switches. We are unlikely to add drivers for supporting user space
> drivers of switches.
That's not the request. I'm offering an improved driver to access {Q}SFP
EEPROMs. It can be easily called by your framework, so the sfp.c users
can also get improved access to the EEPROMs.
As designed it fits the need in the linux based community I'm
working with. It is in production in two NOSs on three switches. Less
complete variants of this driver are in production on all three NOSs
I've worked with, on dozens of platforms. This is real code, that fits
a real need, and would like the benefits of being maintained as part of
the mainline kernel.
>
> NACK.
>
> Andrew
>
Don
^ permalink raw reply
* RE: [PATCH] net: fix e1000.rst Documentation build errors
From: Brown, Aaron F @ 2018-06-18 19:42 UTC (permalink / raw)
To: Randy Dunlap, netdev@vger.kernel.org, David Miller,
linux-doc@vger.kernel.org
Cc: LKML, Kirsher, Jeffrey T
In-Reply-To: <4e7a8aa5-cd19-3f57-cc60-6e6224d02a69@infradead.org>
> From: Randy Dunlap [mailto:rdunlap@infradead.org]
> Sent: Saturday, June 16, 2018 5:36 PM
> To: netdev@vger.kernel.org; David Miller <davem@davemloft.net>; linux-
> doc@vger.kernel.org
> Cc: LKML <linux-kernel@vger.kernel.org>; Kirsher, Jeffrey T
> <jeffrey.t.kirsher@intel.com>; Brown, Aaron F <aaron.f.brown@intel.com>
> Subject: [PATCH] net: fix e1000.rst Documentation build errors
>
> From: Randy Dunlap <rdunlap@infradead.org>
>
> Fix Documentation build errors in e1000.rst. Several section titles and
> their underlines should not be indented.
>
> Documentation/networking/e1000.rst:358: (SEVERE/4) Unexpected section
> title.
>
> Fixes: 228046e76189 ("Documentation: e1000: Update kernel
> documentation")
>
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Aaron Brown <aaron.f.brown@intel.com>
> ---
> Is there a Sphinx version problem here? Tested-by: should indicate
> that there was no error like I am seeing.
The "Tested-by:" for this (and the e100 variant) was entirely focused on correctness of the documentation text, parameters match the driver, URLs were correct, etc... In retrospect "Tested-by:" is not exactly accurate and I probably should have left it at a simpler ack.
>
> Documentation/networking/e1000.rst | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> --- lnx-418-rc1.orig/Documentation/networking/e1000.rst
> +++ lnx-418-rc1/Documentation/networking/e1000.rst
> @@ -354,8 +354,8 @@ previously mentioned to force the adapte
> Additional Configurations
> =========================
>
> - Jumbo Frames
> - ------------
> +Jumbo Frames
> +------------
> Jumbo Frames support is enabled by changing the MTU to a value larger
> than
> the default of 1500. Use the ifconfig command to increase the MTU size.
> For example::
> @@ -389,8 +389,8 @@ Additional Configurations
> Intel(R) PRO/1000 Gigabit Server Adapter
> Intel(R) PRO/1000 PM Network Connection
>
> - ethtool
> - -------
> +ethtool
> +-------
> The driver utilizes the ethtool interface for driver configuration and
> diagnostics, as well as displaying statistical information. The ethtool
> version 1.6 or later is required for this functionality.
> @@ -398,8 +398,8 @@ Additional Configurations
> The latest release of ethtool can be found from
> https://www.kernel.org/pub/software/network/ethtool/
>
> - Enabling Wake on LAN* (WoL)
> - ---------------------------
> +Enabling Wake on LAN* (WoL)
> +---------------------------
> WoL is configured through the ethtool* utility.
>
> WoL will be enabled on the system during the next shut down or reboot.
>
^ permalink raw reply
* [PATCH v3] bitfield: fix *_encode_bits()
From: Johannes Berg @ 2018-06-18 19:56 UTC (permalink / raw)
To: linux-kernel, netdev; +Cc: Al Viro, Andy Shevchenko
There's a bug in *_encode_bits() in using ~field_multiplier() for
the check whether or not the constant value fits into the field,
this is wrong and clearly ~field_mask() was intended. This was
triggering for me for both constant and non-constant values.
Additionally, make this case actually into an compile error.
Declaring the extern function that will never exist with just a
warning is pointless as then later we'll just get a link error.
While at it, also fix the indentation in those lines I'm touching.
Finally, as suggested by Andy Shevchenko, add some tests and for
that introduce also u8 helpers. The tests don't compile without
the fix, showing that it's necessary.
Fixes: 00b0c9b82663 ("Add primitives for manipulating bitfields both in host- and fixed-endian.")
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
v2: replace stray tab by space
v3: u8 helpers, tests
If you don't mind, I'd like to take this through the networking
tree(s) as a fix since I have some pending code that depends on
it.
---
include/linux/bitfield.h | 7 +-
lib/Kconfig.debug | 7 ++
lib/Makefile | 1 +
lib/test_bitfield.c | 163 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 175 insertions(+), 3 deletions(-)
create mode 100644 lib/test_bitfield.c
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index cf2588d81148..65a6981eef7b 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -104,7 +104,7 @@
(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
})
-extern void __compiletime_warning("value doesn't fit into mask")
+extern void __compiletime_error("value doesn't fit into mask")
__field_overflow(void);
extern void __compiletime_error("bad bitfield mask")
__bad_mask(void);
@@ -121,8 +121,8 @@ static __always_inline u64 field_mask(u64 field)
#define ____MAKE_OP(type,base,to,from) \
static __always_inline __##type type##_encode_bits(base v, base field) \
{ \
- if (__builtin_constant_p(v) && (v & ~field_multiplier(field))) \
- __field_overflow(); \
+ if (__builtin_constant_p(v) && (v & ~field_mask(field))) \
+ __field_overflow(); \
return to((v & field_mask(field)) * field_multiplier(field)); \
} \
static __always_inline __##type type##_replace_bits(__##type old, \
@@ -143,6 +143,7 @@ static __always_inline base type##_get_bits(__##type v, base field) \
____MAKE_OP(le##size,u##size,cpu_to_le##size,le##size##_to_cpu) \
____MAKE_OP(be##size,u##size,cpu_to_be##size,be##size##_to_cpu) \
____MAKE_OP(u##size,u##size,,)
+____MAKE_OP(u8,u8,,)
__MAKE_OP(16)
__MAKE_OP(32)
__MAKE_OP(64)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index eb885942eb0f..b0870377b4d1 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1799,6 +1799,13 @@ config TEST_BITMAP
If unsure, say N.
+config TEST_BITFIELD
+ tristate "Test bitfield functions at runtime"
+ help
+ Enable this option to test the bitfield functions at boot.
+
+ If unsure, say N.
+
config TEST_UUID
tristate "Test functions located in the uuid module at runtime"
diff --git a/lib/Makefile b/lib/Makefile
index 84c6dcb31fbb..02ab4c1a64d1 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
obj-$(CONFIG_TEST_PRINTF) += test_printf.o
obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
+obj-$(CONFIG_TEST_BITFIELD) += test_bitfield.o
obj-$(CONFIG_TEST_UUID) += test_uuid.o
obj-$(CONFIG_TEST_PARMAN) += test_parman.o
obj-$(CONFIG_TEST_KMOD) += test_kmod.o
diff --git a/lib/test_bitfield.c b/lib/test_bitfield.c
new file mode 100644
index 000000000000..3b50067611d9
--- /dev/null
+++ b/lib/test_bitfield.c
@@ -0,0 +1,163 @@
+/*
+ * Test cases for bitfield helpers.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/bitfield.h>
+
+#define CHECK_ENC_GET_U(tp, v, field, res) do { \
+ { \
+ u##tp _res; \
+ \
+ _res = u##tp##_encode_bits(v, field); \
+ if (_res != res) { \
+ pr_warn("u" #tp "_encode_bits(" #v ", " #field ") is 0x%llx != " #res "\n",\
+ (u64)_res); \
+ return -EINVAL; \
+ } \
+ if (u##tp##_get_bits(_res, field) != v) \
+ return -EINVAL; \
+ } \
+ } while (0)
+
+#define CHECK_ENC_GET_LE(tp, v, field, res) do { \
+ { \
+ __le##tp _res; \
+ \
+ _res = le##tp##_encode_bits(v, field); \
+ if (_res != cpu_to_le##tp(res)) { \
+ pr_warn("le" #tp "_encode_bits(" #v ", " #field ") is 0x%llx != 0x%llx\n",\
+ (u64)le##tp##_to_cpu(_res), \
+ (u64)(res)); \
+ return -EINVAL; \
+ } \
+ if (le##tp##_get_bits(_res, field) != v) \
+ return -EINVAL; \
+ } \
+ } while (0)
+
+#define CHECK_ENC_GET_BE(tp, v, field, res) do { \
+ { \
+ __be##tp _res; \
+ \
+ _res = be##tp##_encode_bits(v, field); \
+ if (_res != cpu_to_be##tp(res)) { \
+ pr_warn("be" #tp "_encode_bits(" #v ", " #field ") is 0x%llx != 0x%llx\n",\
+ (u64)be##tp##_to_cpu(_res), \
+ (u64)(res)); \
+ return -EINVAL; \
+ } \
+ if (be##tp##_get_bits(_res, field) != v) \
+ return -EINVAL; \
+ } \
+ } while (0)
+
+#define CHECK_ENC_GET(tp, v, field, res) do { \
+ CHECK_ENC_GET_U(tp, v, field, res); \
+ CHECK_ENC_GET_LE(tp, v, field, res); \
+ CHECK_ENC_GET_BE(tp, v, field, res); \
+ } while (0)
+
+static int test_constants(void)
+{
+ /*
+ * NOTE
+ * This whole function compiles (or at least should, if everything
+ * is going according to plan) to nothing after optimisation.
+ */
+
+ CHECK_ENC_GET(16, 1, 0x000f, 0x0001);
+ CHECK_ENC_GET(16, 3, 0x00f0, 0x0030);
+ CHECK_ENC_GET(16, 5, 0x0f00, 0x0500);
+ CHECK_ENC_GET(16, 7, 0xf000, 0x7000);
+ CHECK_ENC_GET(16, 14, 0x000f, 0x000e);
+ CHECK_ENC_GET(16, 15, 0x00f0, 0x00f0);
+/*
+ * This should fail compilation:
+ * CHECK_ENC_GET(16, 16, 0x0f00, 0x1000);
+ */
+
+ CHECK_ENC_GET_U(8, 1, 0x0f, 0x01);
+ CHECK_ENC_GET_U(8, 3, 0xf0, 0x30);
+ CHECK_ENC_GET_U(8, 14, 0x0f, 0x0e);
+ CHECK_ENC_GET_U(8, 15, 0xf0, 0xf0);
+
+ CHECK_ENC_GET(32, 1, 0x00000f00, 0x00000100);
+ CHECK_ENC_GET(32, 3, 0x0000f000, 0x00003000);
+ CHECK_ENC_GET(32, 5, 0x000f0000, 0x00050000);
+ CHECK_ENC_GET(32, 7, 0x00f00000, 0x00700000);
+ CHECK_ENC_GET(32, 14, 0x0f000000, 0x0e000000);
+ CHECK_ENC_GET(32, 15, 0xf0000000, 0xf0000000);
+
+ CHECK_ENC_GET(64, 1, 0x00000f0000000000ull, 0x0000010000000000ull);
+ CHECK_ENC_GET(64, 3, 0x0000f00000000000ull, 0x0000300000000000ull);
+ CHECK_ENC_GET(64, 5, 0x000f000000000000ull, 0x0005000000000000ull);
+ CHECK_ENC_GET(64, 7, 0x00f0000000000000ull, 0x0070000000000000ull);
+ CHECK_ENC_GET(64, 14, 0x0f00000000000000ull, 0x0e00000000000000ull);
+ CHECK_ENC_GET(64, 15, 0xf000000000000000ull, 0xf000000000000000ull);
+
+ return 0;
+}
+
+#define CHECK(tp, mask) do { \
+ u64 v; \
+ \
+ for (v = 0; v < 1 << hweight32(mask); v++) \
+ if (tp##_encode_bits(v, mask) != v << __ffs64(mask)) \
+ return -EINVAL; \
+ } while (0)
+
+static int test_variables(void)
+{
+ CHECK(u8, 0x0f);
+ CHECK(u8, 0xf0);
+ CHECK(u8, 0x38);
+
+ CHECK(u16, 0x0038);
+ CHECK(u16, 0x0380);
+ CHECK(u16, 0x3800);
+ CHECK(u16, 0x8000);
+
+ CHECK(u32, 0x80000000);
+ CHECK(u32, 0x7f000000);
+ CHECK(u32, 0x07e00000);
+ CHECK(u32, 0x00018000);
+
+ CHECK(u64, 0x8000000000000000ull);
+ CHECK(u64, 0x7f00000000000000ull);
+ CHECK(u64, 0x0001800000000000ull);
+ CHECK(u64, 0x0000000080000000ull);
+ CHECK(u64, 0x000000007f000000ull);
+ CHECK(u64, 0x0000000018000000ull);
+ CHECK(u64, 0x0000001f8000000ull);
+
+ return 0;
+}
+
+static int __init test_bitfields(void)
+{
+ int ret = test_constants();
+
+ if (ret) {
+ pr_warn("constant tests failed!\n");
+ return ret;
+ }
+
+ ret = test_variables();
+ if (ret) {
+ pr_warn("variable tests failed!\n");
+ return ret;
+ }
+
+ pr_info("tests passed\n");
+
+ return 0;
+}
+module_init(test_bitfields)
+
+MODULE_AUTHOR("Johannes Berg <johannes@sipsolutions.net>");
+MODULE_LICENSE("GPL");
--
2.14.4
^ permalink raw reply related
* Re: [PATCH] tc, bpf: add option to dump bpf verifier as C program fragment
From: Jakub Kicinski @ 2018-06-18 20:18 UTC (permalink / raw)
To: Ophir Munk
Cc: netdev, Stephen Hemminger, David Ahern, Thomas Monjalon,
Olga Shern
In-Reply-To: <1529225321-15429-1-git-send-email-ophirmu@mellanox.com>
On Sun, 17 Jun 2018 08:48:41 +0000, Ophir Munk wrote:
> Similar to cbpf used within tcpdump utility with a "-d" option to dump
> the compiled packet-matching code in a human readable form - tc has the
> "verbose" option to dump ebpf verifier output.
> Another useful option of cbpf using tcpdump "-dd" option is to dump
> packet-matching code a C program fragment. Similar to this - this commit
> adds a new tc ebpf option named "code" to dump ebpf verifier as C program
> fragment.
>
> Existing "verbose" option sample output:
>
> Verifier analysis:
> 0: (61) r2 = *(u32 *)(r1 +52)
> 1: (18) r3 = 0xdeadbeef
> 3: (63) *(u32 *)(r10 -4) = r3
> .
> .
> 11: (63) *(u32 *)(r1 +52) = r2
> 12: (18) r0 = 0xffffffff
> 14: (95) exit
>
> New "code" option sample output:
>
> /* struct bpf_insn cls_q_code[] = { */
> {0x61, 2, 1, 52, 0x00000000},
> {0x18, 3, 0, 0, 0xdeadbeef},
> {0x00, 0, 0, 0, 0x00000000},
> .
> .
> {0x63, 1, 2, 52, 0x00000000},
> {0x18, 0, 0, 0, 0xffffffff},
> {0x00, 0, 0, 0, 0x00000000},
> {0x95, 0, 0, 0, 0x00000000},
>
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
Hmm... printing C arrays looks like hacky integration with some C
code... Would you not be better served by simply using libbpf in
whatever is consuming this output?
^ permalink raw reply
* Re: [RFC v2, net-next, PATCH 4/4] net/cpsw_switchdev: add switchdev mode of operation on cpsw driver
From: Ilias Apalodimas @ 2018-06-18 20:19 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, grygorii.strashko, ivan.khoronzhuk, nsekhar, jiri,
ivecera, f.fainelli, francois.ozog, yogeshs, spatton, Jose.Abreu
In-Reply-To: <20180618161627.GC5865@lunn.ch>
On Mon, Jun 18, 2018 at 06:16:27PM +0200, Andrew Lunn wrote:
> > @@ -2711,6 +2789,10 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
> > if (of_property_read_bool(node, "dual_emac"))
> > data->switch_mode = CPSW_DUAL_EMAC;
> >
> > + /* switchdev overrides DTS */
> > + if (IS_ENABLED(CONFIG_TI_CPSW_SWITCHDEV))
> > + data->switch_mode = CPSW_SWITCHDEV;
> > +
>
> I know you discussed this a bit with Jiri, but i still think if
> 'dual_mac" is found, you should do dual mac. The DT clearly requests
> dual mac, and doing anything else is going to cause confusion.
>
> The device tree binding is ambiguous what should happen when dual-mac
> is missing. So i would only enable swithdev mode in that case.
At the moment if no 'dual_emac;' is found on DTS the driver operates in "switch
mode". It only registers 1 ethernet interface with no ability (unless you patch
the kernel) to configure the switch. If we use DTS instead of a .config option
we should add parsing for something like 'switchdev;' in the DTS.
Jiri proposed using devlink, which makes sense, but i am not sure it's
applicable on this patchset. This will change the driver completely and will
totally break backwards compatibility.
Ideally i'd prefer something like:
1. Add a DTS option and continue the current behavior. I agree with you that
this will cause less confusion (in fact i prefer it for the current state of the
driver compared to the .config).
or
2. Keep the .config option which is better suited over DTS but might cause some
confusion.
>
> But ideally, it should be a new driver with a new binding.
TI is better suited to comment on this. The work proposed here is mostly to
accomodate future TSN related configuration for switches. This patchset has
been tested against Ivan's patchset for CBS and is working as expected
configuration wise.
(http://lkml.iu.edu/hypermail/linux/kernel/1806.1/05302.html)
Thanks
Ilias
^ permalink raw reply
* Re: [PATCH v3] bitfield: fix *_encode_bits()
From: Andy Shevchenko @ 2018-06-18 20:21 UTC (permalink / raw)
To: Johannes Berg; +Cc: Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <20180618195618.17536-1-johannes@sipsolutions.net>
On Mon, Jun 18, 2018 at 10:56 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> There's a bug in *_encode_bits() in using ~field_multiplier() for
> the check whether or not the constant value fits into the field,
> this is wrong and clearly ~field_mask() was intended. This was
> triggering for me for both constant and non-constant values.
>
> Additionally, make this case actually into an compile error.
> Declaring the extern function that will never exist with just a
> warning is pointless as then later we'll just get a link error.
>
> While at it, also fix the indentation in those lines I'm touching.
>
> Finally, as suggested by Andy Shevchenko, add some tests and for
> that introduce also u8 helpers. The tests don't compile without
> the fix, showing that it's necessary.
>
Thanks!
Few nitpicks / questions below, and I'm fine with the result!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Fixes: 00b0c9b82663 ("Add primitives for manipulating bitfields both in host- and fixed-endian.")
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
> v2: replace stray tab by space
> v3: u8 helpers, tests
>
> If you don't mind, I'd like to take this through the networking
> tree(s) as a fix since I have some pending code that depends on
> it.
> ---
> include/linux/bitfield.h | 7 +-
> lib/Kconfig.debug | 7 ++
> lib/Makefile | 1 +
> lib/test_bitfield.c | 163 +++++++++++++++++++++++++++++++++++++++++++++++
I think would be better to add test cases first, followed by fix. (1
patch -> 2 patches)
In this case Fixes tag would be only for the fix part and backporting
(if needed) will be much easier.
> 4 files changed, 175 insertions(+), 3 deletions(-)
> create mode 100644 lib/test_bitfield.c
>
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index cf2588d81148..65a6981eef7b 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -104,7 +104,7 @@
> (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
> })
>
> -extern void __compiletime_warning("value doesn't fit into mask")
> +extern void __compiletime_error("value doesn't fit into mask")
> __field_overflow(void);
> extern void __compiletime_error("bad bitfield mask")
> __bad_mask(void);
> @@ -121,8 +121,8 @@ static __always_inline u64 field_mask(u64 field)
> #define ____MAKE_OP(type,base,to,from) \
> static __always_inline __##type type##_encode_bits(base v, base field) \
> { \
> - if (__builtin_constant_p(v) && (v & ~field_multiplier(field))) \
> - __field_overflow(); \
> + if (__builtin_constant_p(v) && (v & ~field_mask(field))) \
> + __field_overflow(); \
> return to((v & field_mask(field)) * field_multiplier(field)); \
> } \
> static __always_inline __##type type##_replace_bits(__##type old, \
> @@ -143,6 +143,7 @@ static __always_inline base type##_get_bits(__##type v, base field) \
> ____MAKE_OP(le##size,u##size,cpu_to_le##size,le##size##_to_cpu) \
> ____MAKE_OP(be##size,u##size,cpu_to_be##size,be##size##_to_cpu) \
> ____MAKE_OP(u##size,u##size,,)
> +____MAKE_OP(u8,u8,,)
Is this one you need, or it's just for sake of tests?
For me looks like for consistency we may add fake conversion macros
for this, such as
#define cpu_to_le8(x) x
#define le8_to_cpu(x) x
...
#undef le8_to_cpu
#undef cpu_to_le8
And do in the same way like below
__MAKE_OP(8)
This might be third patch w/o Fixes tag as well.
> __MAKE_OP(16)
> __MAKE_OP(32)
> __MAKE_OP(64)
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index eb885942eb0f..b0870377b4d1 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1799,6 +1799,13 @@ config TEST_BITMAP
>
> If unsure, say N.
>
> +config TEST_BITFIELD
> + tristate "Test bitfield functions at runtime"
> + help
> + Enable this option to test the bitfield functions at boot.
> +
> + If unsure, say N.
> +
> config TEST_UUID
> tristate "Test functions located in the uuid module at runtime"
>
> diff --git a/lib/Makefile b/lib/Makefile
> index 84c6dcb31fbb..02ab4c1a64d1 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
> obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
> obj-$(CONFIG_TEST_PRINTF) += test_printf.o
> obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
> +obj-$(CONFIG_TEST_BITFIELD) += test_bitfield.o
> obj-$(CONFIG_TEST_UUID) += test_uuid.o
> obj-$(CONFIG_TEST_PARMAN) += test_parman.o
> obj-$(CONFIG_TEST_KMOD) += test_kmod.o
> diff --git a/lib/test_bitfield.c b/lib/test_bitfield.c
> new file mode 100644
> index 000000000000..3b50067611d9
> --- /dev/null
> +++ b/lib/test_bitfield.c
> @@ -0,0 +1,163 @@
Perhaps
// SPDX... GPL-2.0+
> +/*
> + * Test cases for bitfield helpers.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
Either module.h (if we can compile as a module) or just init.h otherwise.
> +#include <linux/bitfield.h>
> +
> +#define CHECK_ENC_GET_U(tp, v, field, res) do { \
> + { \
> + u##tp _res; \
> + \
> + _res = u##tp##_encode_bits(v, field); \
> + if (_res != res) { \
> + pr_warn("u" #tp "_encode_bits(" #v ", " #field ") is 0x%llx != " #res "\n",\
> + (u64)_res); \
> + return -EINVAL; \
> + } \
> + if (u##tp##_get_bits(_res, field) != v) \
> + return -EINVAL; \
> + } \
> + } while (0)
> +
> +#define CHECK_ENC_GET_LE(tp, v, field, res) do { \
> + { \
> + __le##tp _res; \
> + \
> + _res = le##tp##_encode_bits(v, field); \
> + if (_res != cpu_to_le##tp(res)) { \
> + pr_warn("le" #tp "_encode_bits(" #v ", " #field ") is 0x%llx != 0x%llx\n",\
> + (u64)le##tp##_to_cpu(_res), \
> + (u64)(res)); \
> + return -EINVAL; \
> + } \
> + if (le##tp##_get_bits(_res, field) != v) \
> + return -EINVAL; \
> + } \
> + } while (0)
> +
> +#define CHECK_ENC_GET_BE(tp, v, field, res) do { \
> + { \
> + __be##tp _res; \
> + \
> + _res = be##tp##_encode_bits(v, field); \
> + if (_res != cpu_to_be##tp(res)) { \
> + pr_warn("be" #tp "_encode_bits(" #v ", " #field ") is 0x%llx != 0x%llx\n",\
> + (u64)be##tp##_to_cpu(_res), \
> + (u64)(res)); \
> + return -EINVAL; \
> + } \
> + if (be##tp##_get_bits(_res, field) != v) \
> + return -EINVAL; \
> + } \
> + } while (0)
> +
> +#define CHECK_ENC_GET(tp, v, field, res) do { \
> + CHECK_ENC_GET_U(tp, v, field, res); \
> + CHECK_ENC_GET_LE(tp, v, field, res); \
> + CHECK_ENC_GET_BE(tp, v, field, res); \
> + } while (0)
> +
> +static int test_constants(void)
> +{
> + /*
> + * NOTE
> + * This whole function compiles (or at least should, if everything
> + * is going according to plan) to nothing after optimisation.
> + */
> +
> + CHECK_ENC_GET(16, 1, 0x000f, 0x0001);
> + CHECK_ENC_GET(16, 3, 0x00f0, 0x0030);
> + CHECK_ENC_GET(16, 5, 0x0f00, 0x0500);
> + CHECK_ENC_GET(16, 7, 0xf000, 0x7000);
> + CHECK_ENC_GET(16, 14, 0x000f, 0x000e);
> + CHECK_ENC_GET(16, 15, 0x00f0, 0x00f0);
> +/*
> + * This should fail compilation:
> + * CHECK_ENC_GET(16, 16, 0x0f00, 0x1000);
> + */
Perhaps we need some ifdeffery around this. It would allow you to try
w/o altering the source code.
#ifdef TEST_BITFIELD_COMPILE
...
#endif
> +
> + CHECK_ENC_GET_U(8, 1, 0x0f, 0x01);
> + CHECK_ENC_GET_U(8, 3, 0xf0, 0x30);
> + CHECK_ENC_GET_U(8, 14, 0x0f, 0x0e);
> + CHECK_ENC_GET_U(8, 15, 0xf0, 0xf0);
> +
> + CHECK_ENC_GET(32, 1, 0x00000f00, 0x00000100);
> + CHECK_ENC_GET(32, 3, 0x0000f000, 0x00003000);
> + CHECK_ENC_GET(32, 5, 0x000f0000, 0x00050000);
> + CHECK_ENC_GET(32, 7, 0x00f00000, 0x00700000);
> + CHECK_ENC_GET(32, 14, 0x0f000000, 0x0e000000);
> + CHECK_ENC_GET(32, 15, 0xf0000000, 0xf0000000);
> +
> + CHECK_ENC_GET(64, 1, 0x00000f0000000000ull, 0x0000010000000000ull);
> + CHECK_ENC_GET(64, 3, 0x0000f00000000000ull, 0x0000300000000000ull);
> + CHECK_ENC_GET(64, 5, 0x000f000000000000ull, 0x0005000000000000ull);
> + CHECK_ENC_GET(64, 7, 0x00f0000000000000ull, 0x0070000000000000ull);
> + CHECK_ENC_GET(64, 14, 0x0f00000000000000ull, 0x0e00000000000000ull);
> + CHECK_ENC_GET(64, 15, 0xf000000000000000ull, 0xf000000000000000ull);
> +
> + return 0;
> +}
> +
> +#define CHECK(tp, mask) do { \
> + u64 v; \
> + \
> + for (v = 0; v < 1 << hweight32(mask); v++) \
> + if (tp##_encode_bits(v, mask) != v << __ffs64(mask)) \
> + return -EINVAL; \
I guess you rather continue and print a statistics X passed out of Y.
Check how it's done, for example, in other test_* modules.
(test_printf.c comes first to my mind).
> + } while (0)
> +
> +static int test_variables(void)
> +{
> + CHECK(u8, 0x0f);
> + CHECK(u8, 0xf0);
> + CHECK(u8, 0x38);
> +
> + CHECK(u16, 0x0038);
> + CHECK(u16, 0x0380);
> + CHECK(u16, 0x3800);
> + CHECK(u16, 0x8000);
> +
> + CHECK(u32, 0x80000000);
> + CHECK(u32, 0x7f000000);
> + CHECK(u32, 0x07e00000);
> + CHECK(u32, 0x00018000);
> +
> + CHECK(u64, 0x8000000000000000ull);
> + CHECK(u64, 0x7f00000000000000ull);
> + CHECK(u64, 0x0001800000000000ull);
> + CHECK(u64, 0x0000000080000000ull);
> + CHECK(u64, 0x000000007f000000ull);
> + CHECK(u64, 0x0000000018000000ull);
> + CHECK(u64, 0x0000001f8000000ull);
> +
> + return 0;
> +}
> +
> +static int __init test_bitfields(void)
> +{
> + int ret = test_constants();
> +
> + if (ret) {
> + pr_warn("constant tests failed!\n");
> + return ret;
> + }
> +
> + ret = test_variables();
> + if (ret) {
> + pr_warn("variable tests failed!\n");
> + return ret;
> + }
> +
> + pr_info("tests passed\n");
> +
> + return 0;
> +}
> +module_init(test_bitfields)
> +
> +MODULE_AUTHOR("Johannes Berg <johannes@sipsolutions.net>");
> +MODULE_LICENSE("GPL");
> --
> 2.14.4
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3] bitfield: fix *_encode_bits()
From: Johannes Berg @ 2018-06-18 20:28 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <CAHp75VdY+PF0Edm_py+UiD3nz77HTQps_8uaRh+Sa3C+UKECKA@mail.gmail.com>
> I think would be better to add test cases first, followed by fix. (1
> patch -> 2 patches)
> In this case Fixes tag would be only for the fix part and backporting
> (if needed) will be much easier.
Can't, unless I introduce a compilation issue in the tests first? That
seems weird. But I guess I can do it the other way around.
> > @@ -143,6 +143,7 @@ static __always_inline base type##_get_bits(__##type v, base field) \
> > ____MAKE_OP(le##size,u##size,cpu_to_le##size,le##size##_to_cpu) \
> > ____MAKE_OP(be##size,u##size,cpu_to_be##size,be##size##_to_cpu) \
> > ____MAKE_OP(u##size,u##size,,)
> > +____MAKE_OP(u8,u8,,)
>
> Is this one you need, or it's just for sake of tests?
All three ;-)
We'll probably need it eventually (we do have bytes to take bits out
of), for consistency I think we want it, and I wanted to add it to the
tests too.
> For me looks like for consistency we may add fake conversion macros
> for this, such as
>
> #define cpu_to_le8(x) x
> #define le8_to_cpu(x) x
> ...
> #undef le8_to_cpu
> #undef cpu_to_le8
>
> And do in the same way like below
>
> __MAKE_OP(8)
I disagree with this. I don't see why we should have le8_encode_bits()
and be8_encode_bits() and friends, that makes no sense.
> Perhaps
> // SPDX... GPL-2.0+
Yeah, I guess I should have that.
> > +/*
> > + * Test cases for bitfield helpers.
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
>
> Either module.h (if we can compile as a module) or just init.h otherwise.
It can be a module ... guess I cargo-culted that from another test.
> > +/*
> > + * This should fail compilation:
> > + * CHECK_ENC_GET(16, 16, 0x0f00, 0x1000);
> > + */
>
> Perhaps we need some ifdeffery around this. It would allow you to try
> w/o altering the source code.
>
> #ifdef TEST_BITFIELD_COMPILE
> ...
> #endif
Yeah, I guess we could do that.
> I guess you rather continue and print a statistics X passed out of Y.
> Check how it's done, for example, in other test_* modules.
> (test_printf.c comes first to my mind).
I see it's done that way elsewhere, but I don't really see the point. It
makes the test code more complex, and if you fail here you'd better fix
it, and if you need a few iterations for that it's not really a problem?
johannes
^ permalink raw reply
* Re: [PATCH net-next] nfp: avoid using getnstimeofday64()
From: Jakub Kicinski @ 2018-06-18 20:31 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David S. Miller, y2038, Simon Horman, John Hurley,
Pieter Jansen van Vuuren, Jiri Pirko, oss-drivers, netdev,
linux-kernel
In-Reply-To: <20180618152051.1510142-1-arnd@arndb.de>
On Mon, 18 Jun 2018 17:20:17 +0200, Arnd Bergmann wrote:
> getnstimeofday64 is deprecated in favor of the ktime_get() family of
> functions. The direct replacement would be ktime_get_real_ts64(),
> but I'm picking the basic ktime_get() instead:
>
> - using a ktime_t simplifies the code compared to timespec64
> - using monotonic time instead of real time avoids issues caused
> by a concurrent settimeofday() or during a leap second adjustment.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Thank you!
^ permalink raw reply
* Re: [PATCH v3] bitfield: fix *_encode_bits()
From: Jakub Kicinski @ 2018-06-18 20:33 UTC (permalink / raw)
To: Johannes Berg; +Cc: Andy Shevchenko, Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <1529353683.3092.32.camel@sipsolutions.net>
On Mon, 18 Jun 2018 22:28:03 +0200, Johannes Berg wrote:
> > For me looks like for consistency we may add fake conversion macros
> > for this, such as
> >
> > #define cpu_to_le8(x) x
> > #define le8_to_cpu(x) x
> > ...
> > #undef le8_to_cpu
> > #undef cpu_to_le8
> >
> > And do in the same way like below
> >
> > __MAKE_OP(8)
>
> I disagree with this. I don't see why we should have le8_encode_bits()
> and be8_encode_bits() and friends, that makes no sense.
+1
^ permalink raw reply
* [PATCH v4 1/3] bitfield: fix *_encode_bits()
From: Johannes Berg @ 2018-06-18 20:37 UTC (permalink / raw)
To: linux-kernel, netdev; +Cc: Al Viro, Andy Shevchenko
There's a bug in *_encode_bits() in using ~field_multiplier() for
the check whether or not the constant value fits into the field,
this is wrong and clearly ~field_mask() was intended. This was
triggering for me for both constant and non-constant values.
Additionally, make this case actually into an compile error.
Declaring the extern function that will never exist with just a
warning is pointless as then later we'll just get a link error.
While at it, also fix the indentation in those lines I'm touching.
Finally, as suggested by Andy Shevchenko, add some tests and for
that introduce also u8 helpers. The tests don't compile without
the fix, showing that it's necessary.
Fixes: 00b0c9b82663 ("Add primitives for manipulating bitfields both in host- and fixed-endian.")
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
v2: replace stray tab by space
v3: u8 helpers, tests
v4: minor cleanup (Andy)
split into two patches (Andy)
If you don't mind, I'd like to take this through the networking
tree(s) as a fix since I have some pending code that depends on
it.
---
include/linux/bitfield.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index cf2588d81148..147a7bb341dd 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -104,7 +104,7 @@
(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
})
-extern void __compiletime_warning("value doesn't fit into mask")
+extern void __compiletime_error("value doesn't fit into mask")
__field_overflow(void);
extern void __compiletime_error("bad bitfield mask")
__bad_mask(void);
@@ -121,8 +121,8 @@ static __always_inline u64 field_mask(u64 field)
#define ____MAKE_OP(type,base,to,from) \
static __always_inline __##type type##_encode_bits(base v, base field) \
{ \
- if (__builtin_constant_p(v) && (v & ~field_multiplier(field))) \
- __field_overflow(); \
+ if (__builtin_constant_p(v) && (v & ~field_mask(field))) \
+ __field_overflow(); \
return to((v & field_mask(field)) * field_multiplier(field)); \
} \
static __always_inline __##type type##_replace_bits(__##type old, \
--
2.14.4
^ permalink raw reply related
* [PATCH v4 2/3] bitfield: add u8 helpers
From: Johannes Berg @ 2018-06-18 20:37 UTC (permalink / raw)
To: linux-kernel, netdev; +Cc: Al Viro, Andy Shevchenko
In-Reply-To: <20180618203750.28658-1-johannes@sipsolutions.net>
There's no reason why we shouldn't pack/unpack bits into/from
u8 values/registers/etc., so add u8 helpers.
Use the ____MAKE_OP() macro directly to avoid having nonsense
le8_encode_bits() and similar functions.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
include/linux/bitfield.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 147a7bb341dd..65a6981eef7b 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -143,6 +143,7 @@ static __always_inline base type##_get_bits(__##type v, base field) \
____MAKE_OP(le##size,u##size,cpu_to_le##size,le##size##_to_cpu) \
____MAKE_OP(be##size,u##size,cpu_to_be##size,be##size##_to_cpu) \
____MAKE_OP(u##size,u##size,,)
+____MAKE_OP(u8,u8,,)
__MAKE_OP(16)
__MAKE_OP(32)
__MAKE_OP(64)
--
2.14.4
^ permalink raw reply related
* [PATCH v4 3/3] bitfield: add tests
From: Johannes Berg @ 2018-06-18 20:37 UTC (permalink / raw)
To: linux-kernel, netdev; +Cc: Al Viro, Andy Shevchenko
In-Reply-To: <20180618203750.28658-1-johannes@sipsolutions.net>
Add tests for the bitfield helpers. The constant ones will all
be folded to nothing by the compiler (if everything is correct
in the header file), and the variable ones do some tests against
open-coding the necessary shifts.
A few test cases that should fail/warn compilation are provided
under ifdef.
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
lib/Kconfig.debug | 7 +++
lib/Makefile | 1 +
lib/test_bitfield.c | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 176 insertions(+)
create mode 100644 lib/test_bitfield.c
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index eb885942eb0f..b0870377b4d1 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1799,6 +1799,13 @@ config TEST_BITMAP
If unsure, say N.
+config TEST_BITFIELD
+ tristate "Test bitfield functions at runtime"
+ help
+ Enable this option to test the bitfield functions at boot.
+
+ If unsure, say N.
+
config TEST_UUID
tristate "Test functions located in the uuid module at runtime"
diff --git a/lib/Makefile b/lib/Makefile
index 84c6dcb31fbb..02ab4c1a64d1 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
obj-$(CONFIG_TEST_PRINTF) += test_printf.o
obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
+obj-$(CONFIG_TEST_BITFIELD) += test_bitfield.o
obj-$(CONFIG_TEST_UUID) += test_uuid.o
obj-$(CONFIG_TEST_PARMAN) += test_parman.o
obj-$(CONFIG_TEST_KMOD) += test_kmod.o
diff --git a/lib/test_bitfield.c b/lib/test_bitfield.c
new file mode 100644
index 000000000000..c9bf2d542244
--- /dev/null
+++ b/lib/test_bitfield.c
@@ -0,0 +1,168 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test cases for bitfield helpers.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/bitfield.h>
+
+#define CHECK_ENC_GET_U(tp, v, field, res) do { \
+ { \
+ u##tp _res; \
+ \
+ _res = u##tp##_encode_bits(v, field); \
+ if (_res != res) { \
+ pr_warn("u" #tp "_encode_bits(" #v ", " #field ") is 0x%llx != " #res "\n",\
+ (u64)_res); \
+ return -EINVAL; \
+ } \
+ if (u##tp##_get_bits(_res, field) != v) \
+ return -EINVAL; \
+ } \
+ } while (0)
+
+#define CHECK_ENC_GET_LE(tp, v, field, res) do { \
+ { \
+ __le##tp _res; \
+ \
+ _res = le##tp##_encode_bits(v, field); \
+ if (_res != cpu_to_le##tp(res)) { \
+ pr_warn("le" #tp "_encode_bits(" #v ", " #field ") is 0x%llx != 0x%llx\n",\
+ (u64)le##tp##_to_cpu(_res), \
+ (u64)(res)); \
+ return -EINVAL; \
+ } \
+ if (le##tp##_get_bits(_res, field) != v) \
+ return -EINVAL; \
+ } \
+ } while (0)
+
+#define CHECK_ENC_GET_BE(tp, v, field, res) do { \
+ { \
+ __be##tp _res; \
+ \
+ _res = be##tp##_encode_bits(v, field); \
+ if (_res != cpu_to_be##tp(res)) { \
+ pr_warn("be" #tp "_encode_bits(" #v ", " #field ") is 0x%llx != 0x%llx\n",\
+ (u64)be##tp##_to_cpu(_res), \
+ (u64)(res)); \
+ return -EINVAL; \
+ } \
+ if (be##tp##_get_bits(_res, field) != v) \
+ return -EINVAL; \
+ } \
+ } while (0)
+
+#define CHECK_ENC_GET(tp, v, field, res) do { \
+ CHECK_ENC_GET_U(tp, v, field, res); \
+ CHECK_ENC_GET_LE(tp, v, field, res); \
+ CHECK_ENC_GET_BE(tp, v, field, res); \
+ } while (0)
+
+static int test_constants(void)
+{
+ /*
+ * NOTE
+ * This whole function compiles (or at least should, if everything
+ * is going according to plan) to nothing after optimisation.
+ */
+
+ CHECK_ENC_GET(16, 1, 0x000f, 0x0001);
+ CHECK_ENC_GET(16, 3, 0x00f0, 0x0030);
+ CHECK_ENC_GET(16, 5, 0x0f00, 0x0500);
+ CHECK_ENC_GET(16, 7, 0xf000, 0x7000);
+ CHECK_ENC_GET(16, 14, 0x000f, 0x000e);
+ CHECK_ENC_GET(16, 15, 0x00f0, 0x00f0);
+
+ CHECK_ENC_GET_U(8, 1, 0x0f, 0x01);
+ CHECK_ENC_GET_U(8, 3, 0xf0, 0x30);
+ CHECK_ENC_GET_U(8, 14, 0x0f, 0x0e);
+ CHECK_ENC_GET_U(8, 15, 0xf0, 0xf0);
+
+ CHECK_ENC_GET(32, 1, 0x00000f00, 0x00000100);
+ CHECK_ENC_GET(32, 3, 0x0000f000, 0x00003000);
+ CHECK_ENC_GET(32, 5, 0x000f0000, 0x00050000);
+ CHECK_ENC_GET(32, 7, 0x00f00000, 0x00700000);
+ CHECK_ENC_GET(32, 14, 0x0f000000, 0x0e000000);
+ CHECK_ENC_GET(32, 15, 0xf0000000, 0xf0000000);
+
+ CHECK_ENC_GET(64, 1, 0x00000f0000000000ull, 0x0000010000000000ull);
+ CHECK_ENC_GET(64, 3, 0x0000f00000000000ull, 0x0000300000000000ull);
+ CHECK_ENC_GET(64, 5, 0x000f000000000000ull, 0x0005000000000000ull);
+ CHECK_ENC_GET(64, 7, 0x00f0000000000000ull, 0x0070000000000000ull);
+ CHECK_ENC_GET(64, 14, 0x0f00000000000000ull, 0x0e00000000000000ull);
+ CHECK_ENC_GET(64, 15, 0xf000000000000000ull, 0xf000000000000000ull);
+
+ return 0;
+}
+
+#define CHECK(tp, mask) do { \
+ u64 v; \
+ \
+ for (v = 0; v < 1 << hweight32(mask); v++) \
+ if (tp##_encode_bits(v, mask) != v << __ffs64(mask)) \
+ return -EINVAL; \
+ } while (0)
+
+static int test_variables(void)
+{
+ CHECK(u8, 0x0f);
+ CHECK(u8, 0xf0);
+ CHECK(u8, 0x38);
+
+ CHECK(u16, 0x0038);
+ CHECK(u16, 0x0380);
+ CHECK(u16, 0x3800);
+ CHECK(u16, 0x8000);
+
+ CHECK(u32, 0x80000000);
+ CHECK(u32, 0x7f000000);
+ CHECK(u32, 0x07e00000);
+ CHECK(u32, 0x00018000);
+
+ CHECK(u64, 0x8000000000000000ull);
+ CHECK(u64, 0x7f00000000000000ull);
+ CHECK(u64, 0x0001800000000000ull);
+ CHECK(u64, 0x0000000080000000ull);
+ CHECK(u64, 0x000000007f000000ull);
+ CHECK(u64, 0x0000000018000000ull);
+ CHECK(u64, 0x0000001f8000000ull);
+
+ return 0;
+}
+
+static int __init test_bitfields(void)
+{
+ int ret = test_constants();
+
+ if (ret) {
+ pr_warn("constant tests failed!\n");
+ return ret;
+ }
+
+ ret = test_variables();
+ if (ret) {
+ pr_warn("variable tests failed!\n");
+ return ret;
+ }
+
+#ifdef TEST_BITFIELD_COMPILE
+ /* these should fail compilation */
+ CHECK_ENC_GET(16, 16, 0x0f00, 0x1000);
+ u32_encode_bits(7, 0x06000000);
+
+ /* this should at least give a warning */
+ u16_encode_bits(0, 0x60000);
+#endif
+
+ pr_info("tests passed\n");
+
+ return 0;
+}
+module_init(test_bitfields)
+
+MODULE_AUTHOR("Johannes Berg <johannes@sipsolutions.net>");
+MODULE_LICENSE("GPL");
--
2.14.4
^ permalink raw reply related
* Re: [PATCH v3] bitfield: fix *_encode_bits()
From: Andy Shevchenko @ 2018-06-18 20:40 UTC (permalink / raw)
To: Johannes Berg; +Cc: Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <1529353683.3092.32.camel@sipsolutions.net>
On Mon, Jun 18, 2018 at 11:28 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>> I think would be better to add test cases first, followed by fix. (1
>> patch -> 2 patches)
>> In this case Fixes tag would be only for the fix part and backporting
>> (if needed) will be much easier.
>
> Can't, unless I introduce a compilation issue in the tests first? That
> seems weird. But I guess I can do it the other way around.
Works for me.
>
>> > @@ -143,6 +143,7 @@ static __always_inline base type##_get_bits(__##type v, base field) \
>> > ____MAKE_OP(le##size,u##size,cpu_to_le##size,le##size##_to_cpu) \
>> > ____MAKE_OP(be##size,u##size,cpu_to_be##size,be##size##_to_cpu) \
>> > ____MAKE_OP(u##size,u##size,,)
>> > +____MAKE_OP(u8,u8,,)
>>
>> Is this one you need, or it's just for sake of tests?
>
> All three ;-)
>
> We'll probably need it eventually (we do have bytes to take bits out
> of), for consistency I think we want it, and I wanted to add it to the
> tests too.
Okay, but I still think it makes sense to have this oneliner as a
separate patch.
> I disagree with this. I don't see why we should have le8_encode_bits()
> and be8_encode_bits() and friends, that makes no sense.
OK, it was just a proposal.
>> I guess you rather continue and print a statistics X passed out of Y.
>> Check how it's done, for example, in other test_* modules.
>> (test_printf.c comes first to my mind).
>
> I see it's done that way elsewhere, but I don't really see the point. It
> makes the test code more complex, and if you fail here you'd better fix
> it, and if you need a few iterations for that it's not really a problem?
The idea is to print what was the input, expected output and actual result.
Then you would see what exactly is broken.
I dunno how much we may take away from this certain test case, though
it would be better for my opinion.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 1/3] bitfield: fix *_encode_bits()
From: Andy Shevchenko @ 2018-06-18 20:41 UTC (permalink / raw)
To: Johannes Berg; +Cc: Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <20180618203750.28658-1-johannes@sipsolutions.net>
On Mon, Jun 18, 2018 at 11:37 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> There's a bug in *_encode_bits() in using ~field_multiplier() for
> the check whether or not the constant value fits into the field,
> this is wrong and clearly ~field_mask() was intended. This was
> triggering for me for both constant and non-constant values.
>
> Additionally, make this case actually into an compile error.
> Declaring the extern function that will never exist with just a
> warning is pointless as then later we'll just get a link error.
>
> While at it, also fix the indentation in those lines I'm touching.
>
> Finally, as suggested by Andy Shevchenko, add some tests and for
> that introduce also u8 helpers. The tests don't compile without
> the fix, showing that it's necessary.
>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Fixes: 00b0c9b82663 ("Add primitives for manipulating bitfields both in host- and fixed-endian.")
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
> v2: replace stray tab by space
> v3: u8 helpers, tests
> v4: minor cleanup (Andy)
> split into two patches (Andy)
>
> If you don't mind, I'd like to take this through the networking
> tree(s) as a fix since I have some pending code that depends on
> it.
> ---
> include/linux/bitfield.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index cf2588d81148..147a7bb341dd 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -104,7 +104,7 @@
> (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
> })
>
> -extern void __compiletime_warning("value doesn't fit into mask")
> +extern void __compiletime_error("value doesn't fit into mask")
> __field_overflow(void);
> extern void __compiletime_error("bad bitfield mask")
> __bad_mask(void);
> @@ -121,8 +121,8 @@ static __always_inline u64 field_mask(u64 field)
> #define ____MAKE_OP(type,base,to,from) \
> static __always_inline __##type type##_encode_bits(base v, base field) \
> { \
> - if (__builtin_constant_p(v) && (v & ~field_multiplier(field))) \
> - __field_overflow(); \
> + if (__builtin_constant_p(v) && (v & ~field_mask(field))) \
> + __field_overflow(); \
> return to((v & field_mask(field)) * field_multiplier(field)); \
> } \
> static __always_inline __##type type##_replace_bits(__##type old, \
> --
> 2.14.4
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 2/3] bitfield: add u8 helpers
From: Andy Shevchenko @ 2018-06-18 20:42 UTC (permalink / raw)
To: Johannes Berg; +Cc: Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <20180618203750.28658-2-johannes@sipsolutions.net>
On Mon, Jun 18, 2018 at 11:37 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> There's no reason why we shouldn't pack/unpack bits into/from
> u8 values/registers/etc., so add u8 helpers.
>
> Use the ____MAKE_OP() macro directly to avoid having nonsense
> le8_encode_bits() and similar functions.
>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
> include/linux/bitfield.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 147a7bb341dd..65a6981eef7b 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -143,6 +143,7 @@ static __always_inline base type##_get_bits(__##type v, base field) \
> ____MAKE_OP(le##size,u##size,cpu_to_le##size,le##size##_to_cpu) \
> ____MAKE_OP(be##size,u##size,cpu_to_be##size,be##size##_to_cpu) \
> ____MAKE_OP(u##size,u##size,,)
> +____MAKE_OP(u8,u8,,)
> __MAKE_OP(16)
> __MAKE_OP(32)
> __MAKE_OP(64)
> --
> 2.14.4
>
--
With Best Regards,
Andy Shevchenko
^ 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