* Re: [PATCH bpf-net] bpf: Change bpf_fib_lookup to return lookup status
From: David Ahern @ 2018-06-18 21:35 UTC (permalink / raw)
To: Martin KaFai Lau; +Cc: dsahern, netdev, borkmann, ast, davem
In-Reply-To: <20180618205537.2j645mfujdsqxf2b@kafai-mbp.dhcp.thefacebook.com>
On 6/18/18 2:55 PM, Martin KaFai Lau wrote:
>> /* rc > 0 case */
>> switch(rc) {
>> case BPF_FIB_LKUP_RET_BLACKHOLE:
>> case BPF_FIB_LKUP_RET_UNREACHABLE:
>> case BPF_FIB_LKUP_RET_PROHIBIT:
>> return XDP_DROP;
>> }
>>
>> For the others it becomes a question of do we share why the stack needs
>> to be involved? Maybe the program wants to collect stats to show traffic
>> patterns that can be improved (BPF_FIB_LKUP_RET_FRAG_NEEDED) or support
>> in the kernel needs to be improved (BPF_FIB_LKUP_RET_UNSUPP_LWT) or an
>> interface is misconfigured (BPF_FIB_LKUP_RET_FWD_DISABLED).
> Thanks for the explanation.
>
> Agree on the bpf able to collect stats will be useful.
>
> I am wondering, if a new BPF_FIB_LKUP_RET_XYZ is added later,
> how may the old xdp_prog work/not-work? As of now, the return value
> is straight forward, FWD, PASS (to stack) or DROP (error).
> With this change, the xdp_prog needs to match/switch() the
> BPF_FIB_LKUP_RET_* to at least PASS and DROP.
IMO, programs should only call XDP_DROP for known reasons - like the 3
above. Anything else punt to the stack.
If a new RET_XYZ comes along:
1. the new XYZ is a new ACL response where the packet is to be dropped.
If the program does not understand XYZ and punts to the stack
(recommendation), then a second lookup is done during normal packet
processing and the stack drops it.
2. the new XYZ is a new path in the kernel that is unsupported with
respect to XDP forwarding, nothing new for the program to do.
Either way I would expect stats on BPF_FIB_LKUP_RET_* to give a hint to
the program writer.
Worst case of punting packets to the stack for any rc != 0 means the
stack is doing 2 lookups - 1 in XDP based on its lookup parameters and 1
in normal stack processing - to handle the packet.
>
>>
>> Arguably BPF_FIB_LKUP_RET_NO_NHDEV is not needed. See below.
>>
>>>> @@ -2612,6 +2613,19 @@ struct bpf_raw_tracepoint_args {
>>>> #define BPF_FIB_LOOKUP_DIRECT BIT(0)
>>>> #define BPF_FIB_LOOKUP_OUTPUT BIT(1)
>>>>
>>>> +enum {
>>>> + BPF_FIB_LKUP_RET_SUCCESS, /* lookup successful */
>>>> + BPF_FIB_LKUP_RET_BLACKHOLE, /* dest is blackholed */
>>>> + BPF_FIB_LKUP_RET_UNREACHABLE, /* dest is unreachable */
>>>> + BPF_FIB_LKUP_RET_PROHIBIT, /* dest not allowed */
>>>> + BPF_FIB_LKUP_RET_NOT_FWDED, /* pkt is not forwardded */
>>> BPF_FIB_LKUP_RET_NOT_FWDED is a catch all?
>>>
>>
>> Destination is local. More precisely, the FIB lookup is not unicast so
>> not forwarded. It could be RTN_LOCAL, RTN_BROADCAST, RTN_ANYCAST, or
>> RTN_MULTICAST. The next ones -- blackhole, reachable, prohibit -- are
>> called out.
> I think it also includes the tbid not found case.
Another one of those "should never happen scenarios". The user does not
specify the table; it is retrieved based on device association. Table
defaults to the main table - which always exists - and any VRF
enslavement of a device happens after the VRF device creates the table.
>
>>
>>>> @@ -4252,16 +4277,19 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>>>> if (check_mtu) {
>>>> mtu = ipv6_stub->ip6_mtu_from_fib6(f6i, dst, src);
>>>> if (params->tot_len > mtu)
>>>> - return 0;
>>>> + return BPF_FIB_LKUP_RET_FRAG_NEEDED;
>>>> }
>>>>
>>>> if (f6i->fib6_nh.nh_lwtstate)
>>>> - return 0;
>>>> + return BPF_FIB_LKUP_RET_UNSUPP_LWT;
>>>>
>>>> if (f6i->fib6_flags & RTF_GATEWAY)
>>>> *dst = f6i->fib6_nh.nh_gw;
>>>>
>>>> dev = f6i->fib6_nh.nh_dev;
>>>> + if (unlikely(!dev))
>>>> + return BPF_FIB_LKUP_RET_NO_NHDEV;
>>> Is this a bug fix?
>>>
>>
>> Difference between IPv4 and IPv6. Making them consistent.
>>
>> It is a major BUG in the kernel to reach this point in either protocol
>> to have a unicast route not tied to a device. IPv4 has checks; v6 does
>> not. I figured this being new code, why not make bpf_ipv{4,6}_fib_lookup
>> as close to the same as possible.
> Make sense. A comment in the commit log will be useful if there is a
> re-spin.
>
ok.
^ permalink raw reply
* Debido al no el pago de sus impuestos se hara efectivo un embargo bancario
From: Dian @ 2018-06-18 20:02 UTC (permalink / raw)
[-- Attachment #1: Type: text/plain, Size: 436 bytes --]
Estimado contribuyente:
Es nuestro deber informarle que debido al no pago de sus impuestos se hara efectivo un embargo bancario en el dia de hoy.
Por la seguridad de sus datos hemos adjuntado un documento con su deuda a la fecha con una clave la cual es :
421e68dd993c4a8bb9e3d5e6c066946r
Este documento contiene su deuda a la fecha y todos los datos del proceso detalladamente.
esperamos pronta respuesta
[-- Attachment #2: estado de cuenta.rar --]
[-- Type: *, Size: 73692 bytes --]
^ permalink raw reply
* Re: [bpf PATCH v2 2/6] bpf: sockmap only allow ESTABLISHED sock state
From: Martin KaFai Lau @ 2018-06-18 21:17 UTC (permalink / raw)
To: John Fastabend; +Cc: ast, daniel, netdev
In-Reply-To: <783d9ac4-3291-04cf-98a7-a05f31a833e4@gmail.com>
On Mon, Jun 18, 2018 at 07:50:19AM -0700, John Fastabend wrote:
> On 06/14/2018 05:18 PM, Martin KaFai Lau wrote:
> > On Thu, Jun 14, 2018 at 09:44:52AM -0700, John Fastabend wrote:
> >> Per the note in the TLS ULP (which is actually a generic statement
> >> regarding ULPs)
> >>
> >> /* The TLS ulp is currently supported only for TCP sockets
> >> * in ESTABLISHED state.
> >> * Supporting sockets in LISTEN state will require us
> >> * to modify the accept implementation to clone rather then
> >> * share the ulp context.
> >> */
> > Can you explain how that apply to bpf_tcp ulp?
> >
> > My understanding is the "ulp context" referred in TLS ulp is
> > the tls_context stored in icsk_ulp_data but I don't see bpf_tcp's
> > ulp is using icsk_ulp_data.
> >
> > Others LGTM.
> >
>
> So I think you are right we could probably allow it
> here but I am thinking I'll leave the check for now
> anyways for a couple reasons. First, we will shortly
> add support to allow ULP types to coexist. At the moment
> the two ULP types can not coexist. When this happens it
> looks like we will need to restrict to only ESTABLISHED
> types or somehow make all ULPs work in all states.
>
> Second, I don't have any use cases (nor can I think of
> any) for the sock{map|hash} ULP to be running on a non
> ESTABLISHED socket. Its not clear to me that having the
> sendmsg/sendpage hooks for a LISTEN socket makes sense.
> I would rather restrict it now and if we add something
> later where it makes sense to run on non-ESTABLISHED
> socks we can remove the check.
Make sense if there is no use case. It will be helpful if the commit log
is updated accordingly. Thanks!
Acked-by: Martin KaFai Lau <kafai@fb.com>
^ permalink raw reply
* Re: [PATCH bpf-net] bpf: Change bpf_fib_lookup to return lookup status
From: Martin KaFai Lau @ 2018-06-18 20:55 UTC (permalink / raw)
To: David Ahern; +Cc: dsahern, netdev, borkmann, ast, davem
In-Reply-To: <780331bd-947a-83fe-6e62-c0efc05cfc04@gmail.com>
On Mon, Jun 18, 2018 at 12:27:07PM -0600, David Ahern wrote:
> On 6/18/18 12:11 PM, Martin KaFai Lau wrote:
> > On Sun, Jun 17, 2018 at 08:18:19AM -0700, dsahern@kernel.org wrote:
> >> From: David Ahern <dsahern@gmail.com>
> >>
> >> For ACLs implemented using either FIB rules or FIB entries, the BPF
> >> program needs the FIB lookup status to be able to drop the packet.
> > Except BPF_FIB_LKUP_RET_SUCCESS and BPF_FIB_LKUP_RET_NO_NEIGH, can you
> > give an example on how the xdp_prog may decide XDP_PASS vs XDP_DROP based
> > on other BPF_FIB_LKUP_RET_*?
> >
>
> rc = bpf_fib_lookup(ctx, &fib_params, sizeof(fib_params), flags);
> if (rc == 0)
> packet is forwarded, do the redirect
>
> /* the program is misconfigured -- wrong parameters in struct or flags */
> if (rc < 0)
> ....
>
> /* rc > 0 case */
> switch(rc) {
> case BPF_FIB_LKUP_RET_BLACKHOLE:
> case BPF_FIB_LKUP_RET_UNREACHABLE:
> case BPF_FIB_LKUP_RET_PROHIBIT:
> return XDP_DROP;
> }
>
> For the others it becomes a question of do we share why the stack needs
> to be involved? Maybe the program wants to collect stats to show traffic
> patterns that can be improved (BPF_FIB_LKUP_RET_FRAG_NEEDED) or support
> in the kernel needs to be improved (BPF_FIB_LKUP_RET_UNSUPP_LWT) or an
> interface is misconfigured (BPF_FIB_LKUP_RET_FWD_DISABLED).
Thanks for the explanation.
Agree on the bpf able to collect stats will be useful.
I am wondering, if a new BPF_FIB_LKUP_RET_XYZ is added later,
how may the old xdp_prog work/not-work? As of now, the return value
is straight forward, FWD, PASS (to stack) or DROP (error).
With this change, the xdp_prog needs to match/switch() the
BPF_FIB_LKUP_RET_* to at least PASS and DROP.
>
> Arguably BPF_FIB_LKUP_RET_NO_NHDEV is not needed. See below.
>
> >> @@ -2612,6 +2613,19 @@ struct bpf_raw_tracepoint_args {
> >> #define BPF_FIB_LOOKUP_DIRECT BIT(0)
> >> #define BPF_FIB_LOOKUP_OUTPUT BIT(1)
> >>
> >> +enum {
> >> + BPF_FIB_LKUP_RET_SUCCESS, /* lookup successful */
> >> + BPF_FIB_LKUP_RET_BLACKHOLE, /* dest is blackholed */
> >> + BPF_FIB_LKUP_RET_UNREACHABLE, /* dest is unreachable */
> >> + BPF_FIB_LKUP_RET_PROHIBIT, /* dest not allowed */
> >> + BPF_FIB_LKUP_RET_NOT_FWDED, /* pkt is not forwardded */
> > BPF_FIB_LKUP_RET_NOT_FWDED is a catch all?
> >
>
> Destination is local. More precisely, the FIB lookup is not unicast so
> not forwarded. It could be RTN_LOCAL, RTN_BROADCAST, RTN_ANYCAST, or
> RTN_MULTICAST. The next ones -- blackhole, reachable, prohibit -- are
> called out.
I think it also includes the tbid not found case.
>
> >> @@ -4252,16 +4277,19 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
> >> if (check_mtu) {
> >> mtu = ipv6_stub->ip6_mtu_from_fib6(f6i, dst, src);
> >> if (params->tot_len > mtu)
> >> - return 0;
> >> + return BPF_FIB_LKUP_RET_FRAG_NEEDED;
> >> }
> >>
> >> if (f6i->fib6_nh.nh_lwtstate)
> >> - return 0;
> >> + return BPF_FIB_LKUP_RET_UNSUPP_LWT;
> >>
> >> if (f6i->fib6_flags & RTF_GATEWAY)
> >> *dst = f6i->fib6_nh.nh_gw;
> >>
> >> dev = f6i->fib6_nh.nh_dev;
> >> + if (unlikely(!dev))
> >> + return BPF_FIB_LKUP_RET_NO_NHDEV;
> > Is this a bug fix?
> >
>
> Difference between IPv4 and IPv6. Making them consistent.
>
> It is a major BUG in the kernel to reach this point in either protocol
> to have a unicast route not tied to a device. IPv4 has checks; v6 does
> not. I figured this being new code, why not make bpf_ipv{4,6}_fib_lookup
> as close to the same as possible.
Make sense. A comment in the commit log will be useful if there is a
re-spin.
^ permalink raw reply
* Re: [PATCH 3/4] net/ncsi: Use netdev_dbg for debug messages
From: Joe Perches @ 2018-06-18 20:53 UTC (permalink / raw)
To: Joel Stanley, Samuel Mendoza-Jonas, David S . Miller; +Cc: netdev
In-Reply-To: <20180618071916.6765-4-joel@jms.id.au>
On Mon, 2018-06-18 at 16:49 +0930, Joel Stanley wrote:
> This moves all of the netdev_printk(KERN_DEBUG, ...) messages over to
> netdev_dbg. There is no change in behaviour.
Not quite, but I think the patch is fine anyway.
netdev_printk(KERN_DEBUG ... is always emitted as
long as the console level includes debug output.
netdev_dbg is not included in object code unless
DEBUG is defined or CONFIG_DYNAMIC_DEBUG is set.
And then, it is not emitted into the log unless
DEBUG is set or this specific netdev_dbg is enabled
via the dynamic debug control file.
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> net/ncsi/ncsi-aen.c | 6 +++---
> net/ncsi/ncsi-manage.c | 33 +++++++++++++++------------------
> 2 files changed, 18 insertions(+), 21 deletions(-)
>
> diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
> index f899ed61bb57..25e483e8278b 100644
> --- a/net/ncsi/ncsi-aen.c
> +++ b/net/ncsi/ncsi-aen.c
> @@ -148,9 +148,9 @@ static int ncsi_aen_handler_hncdsc(struct ncsi_dev_priv *ndp,
> hncdsc = (struct ncsi_aen_hncdsc_pkt *)h;
> ncm->data[3] = ntohl(hncdsc->status);
> spin_unlock_irqrestore(&nc->lock, flags);
> - netdev_printk(KERN_DEBUG, ndp->ndev.dev,
> - "NCSI: host driver %srunning on channel %u\n",
> - ncm->data[3] & 0x1 ? "" : "not ", nc->id);
> + netdev_dbg(ndp->ndev.dev,
> + "NCSI: host driver %srunning on channel %u\n",
> + ncm->data[3] & 0x1 ? "" : "not ", nc->id);
>
> return 0;
> }
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 716493a61ba6..091284760d21 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -788,8 +788,8 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> }
> break;
> case ncsi_dev_state_config_done:
> - netdev_printk(KERN_DEBUG, ndp->ndev.dev,
> - "NCSI: channel %u config done\n", nc->id);
> + netdev_dbg(ndp->ndev.dev, "NCSI: channel %u config done\n",
> + nc->id);
> spin_lock_irqsave(&nc->lock, flags);
> if (nc->reconfigure_needed) {
> /* This channel's configuration has been updated
> @@ -804,8 +804,7 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> spin_unlock_irqrestore(&ndp->lock, flags);
>
> - netdev_printk(KERN_DEBUG, dev,
> - "Dirty NCSI channel state reset\n");
> + netdev_dbg(dev, "Dirty NCSI channel state reset\n");
> ncsi_process_next_channel(ndp);
> break;
> }
> @@ -908,9 +907,9 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
> }
>
> ncm = &found->modes[NCSI_MODE_LINK];
> - netdev_printk(KERN_DEBUG, ndp->ndev.dev,
> - "NCSI: Channel %u added to queue (link %s)\n",
> - found->id, ncm->data[2] & 0x1 ? "up" : "down");
> + netdev_dbg(ndp->ndev.dev,
> + "NCSI: Channel %u added to queue (link %s)\n",
> + found->id, ncm->data[2] & 0x1 ? "up" : "down");
>
> out:
> spin_lock_irqsave(&ndp->lock, flags);
> @@ -1316,9 +1315,9 @@ static int ncsi_kick_channels(struct ncsi_dev_priv *ndp)
> if ((ndp->ndev.state & 0xff00) ==
> ncsi_dev_state_config ||
> !list_empty(&nc->link)) {
> - netdev_printk(KERN_DEBUG, nd->dev,
> - "NCSI: channel %p marked dirty\n",
> - nc);
> + netdev_dbg(nd->dev,
> + "NCSI: channel %p marked dirty\n",
> + nc);
> nc->reconfigure_needed = true;
> }
> spin_unlock_irqrestore(&nc->lock, flags);
> @@ -1336,8 +1335,7 @@ static int ncsi_kick_channels(struct ncsi_dev_priv *ndp)
> list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> spin_unlock_irqrestore(&ndp->lock, flags);
>
> - netdev_printk(KERN_DEBUG, nd->dev,
> - "NCSI: kicked channel %p\n", nc);
> + netdev_dbg(nd->dev, "NCSI: kicked channel %p\n", nc);
> n++;
> }
> }
> @@ -1368,8 +1366,8 @@ int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
> list_for_each_entry_rcu(vlan, &ndp->vlan_vids, list) {
> n_vids++;
> if (vlan->vid == vid) {
> - netdev_printk(KERN_DEBUG, dev,
> - "NCSI: vid %u already registered\n", vid);
> + netdev_dbg(dev, "NCSI: vid %u already registered\n",
> + vid);
> return 0;
> }
> }
> @@ -1388,7 +1386,7 @@ int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
> vlan->vid = vid;
> list_add_rcu(&vlan->list, &ndp->vlan_vids);
>
> - netdev_printk(KERN_DEBUG, dev, "NCSI: Added new vid %u\n", vid);
> + netdev_dbg(dev, "NCSI: Added new vid %u\n", vid);
>
> found = ncsi_kick_channels(ndp) != 0;
>
> @@ -1417,8 +1415,7 @@ int ncsi_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
> /* Remove the VLAN id from our internal list */
> list_for_each_entry_safe(vlan, tmp, &ndp->vlan_vids, list)
> if (vlan->vid == vid) {
> - netdev_printk(KERN_DEBUG, dev,
> - "NCSI: vid %u found, removing\n", vid);
> + netdev_dbg(dev, "NCSI: vid %u found, removing\n", vid);
> list_del_rcu(&vlan->list);
> found = true;
> kfree(vlan);
> @@ -1545,7 +1542,7 @@ void ncsi_stop_dev(struct ncsi_dev *nd)
> }
> }
>
> - netdev_printk(KERN_DEBUG, ndp->ndev.dev, "NCSI: Stopping device\n");
> + netdev_dbg(ndp->ndev.dev, "NCSI: Stopping device\n");
> ncsi_report_link(ndp, true);
> }
> EXPORT_SYMBOL_GPL(ncsi_stop_dev);
^ permalink raw reply
* Re: [PATCH 1/4] net/ncsi: Silence debug messages
From: Joe Perches @ 2018-06-18 20:49 UTC (permalink / raw)
To: Joel Stanley, Samuel Mendoza-Jonas, David S . Miller; +Cc: netdev
In-Reply-To: <20180618071916.6765-2-joel@jms.id.au>
On Mon, 2018-06-18 at 16:49 +0930, Joel Stanley wrote:
> In normal operation we see this series of messages as the host drives
> the network device:
>
> ftgmac100 1e660000.ethernet eth0: NCSI: LSC AEN - channel 0 state down
> ftgmac100 1e660000.ethernet eth0: NCSI: suspending channel 0
[...]
> This makes all of these messages netdev_dbg. They are still useful to
> debug eg. misbehaving network device firmware, but we do not need them
> filling up the kernel logs in normal operation.
trivia:
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
[]
> @@ -1735,7 +1735,7 @@ static void ftgmac100_ncsi_handler(struct ncsi_dev *nd)
> if (unlikely(nd->state != ncsi_dev_state_functional))
> return;
>
> - netdev_info(nd->dev, "NCSI interface %s\n",
> + netdev_dbg(nd->dev, "NCSI interface %s\n",
> nd->link_up ? "up" : "down");
It's nicer to realign the multiple line statements
to the open parenthesis
> diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
[]
> @@ -73,8 +73,8 @@ static int ncsi_aen_handler_lsc(struct ncsi_dev_priv *ndp,
> ncm->data[2] = data;
> ncm->data[4] = ntohl(lsc->oem_status);
>
> - netdev_info(ndp->ndev.dev, "NCSI: LSC AEN - channel %u state %s\n",
> - nc->id, data & 0x1 ? "up" : "down");
> + netdev_dbg(ndp->ndev.dev, "NCSI: LSC AEN - channel %u state %s\n",
> + nc->id, data & 0x1 ? "up" : "down");
as was done for the rest of these...
^ permalink raw reply
* Re: [PATCH rdma-next v2 06/20] IB/uverbs: Add PTR_IN attributes that are allocated/copied automatically
From: Jason Gunthorpe @ 2018-06-18 20:48 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Joonas Lahtinen,
Yishai Hadas, Saeed Mahameed, linux-netdev
In-Reply-To: <20180617100006.30663-7-leon@kernel.org>
On Sun, Jun 17, 2018 at 12:59:52PM +0300, Leon Romanovsky wrote:
> From: Matan Barak <matanb@mellanox.com>
>
> Adding UVERBS_ATTR_SPEC_F_ALLOC_AND_COPY flag to PTR_IN attributes.
> By using this flag, the parse automatically allocates and copies the
> user-space data. This data is accessible by using uverbs_attr_get_len
> and uverbs_attr_get_alloced_ptr inline accessor functions from the
> handler.
>
> Signed-off-by: Matan Barak <matanb@mellanox.com>
> Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/infiniband/core/uverbs_ioctl.c | 25 ++++++++++++++++++++++++-
> include/rdma/uverbs_ioctl.h | 25 +++++++++++++++++++++++++
> 2 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
> index 8cc3e8dad9b5..ee15c9ca788b 100644
> +++ b/drivers/infiniband/core/uverbs_ioctl.c
> @@ -114,7 +114,26 @@ static int uverbs_process_attr(struct ib_device *ibdev,
> uattr->attr_data.reserved)
> return -EINVAL;
>
> - e->ptr_attr.data = uattr->data;
> + if (val_spec->flags & UVERBS_ATTR_SPEC_F_ALLOC_AND_COPY &&
> + uattr->len > sizeof(((struct ib_uverbs_attr *)0)->data)) {
Why open-code uverbs_attr_ptr_is_inline() ?
> diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
> index bd6bba3a6e04..0e6f782727bd 100644
> +++ b/include/rdma/uverbs_ioctl.h
> @@ -65,6 +65,8 @@ enum {
> UVERBS_ATTR_SPEC_F_MANDATORY = 1U << 0,
> /* Support extending attributes by length, validate all unknown size == zero */
> UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO = 1U << 1,
> + /* Valid only for PTR_IN. Allocate and copy the data inside the parser */
> + UVERBS_ATTR_SPEC_F_ALLOC_AND_COPY = 1U << 2,
> };
>
> /* Specification of a single attribute inside the ioctl message */
> @@ -431,6 +433,17 @@ static inline struct ib_uobject *uverbs_attr_get_uobject(const struct uverbs_att
> return attr->obj_attr.uobject;
> }
>
> +static inline int uverbs_attr_get_len(const struct uverbs_attr_bundle *attrs_bundle,
> + u16 idx)
> +{
> + const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx);
> +
> + if (IS_ERR(attr))
> + return PTR_ERR(attr);
> +
> + return attr->ptr_attr.len;
> +}
> +
> static inline int uverbs_copy_to(const struct uverbs_attr_bundle *attrs_bundle,
> size_t idx, const void *from, size_t size)
> {
> @@ -457,6 +470,18 @@ static inline bool uverbs_attr_ptr_is_inline(const struct uverbs_attr *attr)
> return attr->ptr_attr.len <= sizeof(attr->ptr_attr.data);
> }
>
> +static inline void *uverbs_attr_get_alloced_ptr(const struct uverbs_attr_bundle *attrs_bundle,
> + u16 idx)
> +{
> + const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx);
> +
> + if (IS_ERR(attr))
> + return (void *)attr;
> +
> + return uverbs_attr_ptr_is_inline(attr) ? u64_to_ptr(void *, attr->ptr_attr.data) :
> + u64_to_ptr(void, attr->ptr_attr.data);
WTF is this:
u64_to_ptr(void *, attr->ptr_attr.data)
That returns attr->ptr_attr.data casted to a void **, then casts it to
a void * - which is identical to u64_to_ptr(void, attr->ptr_attr.data)
It should be &attr->ptr_attr.data. And the return should be const, the
caller shouldn't be mutating the copy.
All this use of u64_to_ptr is ugly needless obfuscation here, and
look, it causes bugs. Use a union.
Like this:
diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index 8cc3e8dad9b506..82c5d33195dfc7 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -114,9 +114,27 @@ static int uverbs_process_attr(struct ib_device *ibdev,
uattr->attr_data.reserved)
return -EINVAL;
- e->ptr_attr.data = uattr->data;
e->ptr_attr.len = uattr->len;
e->ptr_attr.flags = uattr->flags;
+
+ if (val_spec->flags & UVERBS_ATTR_SPEC_F_ALLOC_AND_COPY &&
+ !uverbs_attr_ptr_is_inline(e)) {
+ void *p;
+
+ p = kvmalloc(uattr->len, GFP_KERNEL);
+ if (!p)
+ return -ENOMEM;
+
+ e->ptr_attr.ptr = p;
+
+ if (copy_from_user(p, u64_to_user_ptr(uattr->data),
+ uattr->len)) {
+ kvfree(p);
+ return -EFAULT;
+ }
+ } else {
+ e->ptr_attr.data = uattr->data;
+ }
break;
case UVERBS_ATTR_TYPE_IDR:
@@ -201,6 +219,10 @@ static int uverbs_finalize_attrs(struct uverbs_attr_bundle *attrs_bundle,
commit);
if (!ret)
ret = current_ret;
+ } else if (spec->type == UVERBS_ATTR_TYPE_PTR_IN &&
+ spec->flags & UVERBS_ATTR_SPEC_F_ALLOC_AND_COPY &&
+ !uverbs_attr_ptr_is_inline(attr)) {
+ kvfree(attr->ptr_attr.ptr);
}
}
}
diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index bd6bba3a6e04a1..6e1c322ff5c015 100644
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -65,6 +65,8 @@ enum {
UVERBS_ATTR_SPEC_F_MANDATORY = 1U << 0,
/* Support extending attributes by length, validate all unknown size == zero */
UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO = 1U << 1,
+ /* Valid only for PTR_IN. Allocate and copy the data inside the parser */
+ UVERBS_ATTR_SPEC_F_ALLOC_AND_COPY = 1U << 2,
};
/* Specification of a single attribute inside the ioctl message */
@@ -323,7 +325,15 @@ struct uverbs_object_tree_def {
*/
struct uverbs_ptr_attr {
- u64 data;
+ /*
+ * If UVERBS_ATTR_SPEC_F_ALLOC_AND_COPY is set then the 'ptr' is
+ * used.
+ */
+ union
+ {
+ void *ptr;
+ u64 data;
+ };
u16 len;
/* Combination of bits from enum UVERBS_ATTR_F_XXXX */
u16 flags;
@@ -431,6 +441,17 @@ static inline struct ib_uobject *uverbs_attr_get_uobject(const struct uverbs_att
return attr->obj_attr.uobject;
}
+static inline int uverbs_attr_get_len(const struct uverbs_attr_bundle *attrs_bundle,
+ u16 idx)
+{
+ const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx);
+
+ if (IS_ERR(attr))
+ return PTR_ERR(attr);
+
+ return attr->ptr_attr.len;
+}
+
static inline int uverbs_copy_to(const struct uverbs_attr_bundle *attrs_bundle,
size_t idx, const void *from, size_t size)
{
@@ -457,6 +478,18 @@ static inline bool uverbs_attr_ptr_is_inline(const struct uverbs_attr *attr)
return attr->ptr_attr.len <= sizeof(attr->ptr_attr.data);
}
+static inline const void *uverbs_attr_get_alloced_ptr(
+ const struct uverbs_attr_bundle *attrs_bundle, u16 idx)
+{
+ const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx);
+
+ if (IS_ERR(attr))
+ return (void *)attr;
+
+ return uverbs_attr_ptr_is_inline(attr) ? &attr->ptr_attr.data :
+ attr->ptr_attr.ptr;
+}
+
static inline int _uverbs_copy_from(void *to,
const struct uverbs_attr_bundle *attrs_bundle,
size_t idx,
^ permalink raw reply related
* [PATCH v5 3/3] bitfield: add tests
From: Johannes Berg @ 2018-06-18 20:48 UTC (permalink / raw)
To: linux-kernel, netdev; +Cc: Al Viro, Andy Shevchenko
In-Reply-To: <20180618204816.30806-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>
Reviewed-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..5b8f4108662d
--- /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
* [PATCH v5 2/3] bitfield: add u8 helpers
From: Johannes Berg @ 2018-06-18 20:48 UTC (permalink / raw)
To: linux-kernel, netdev; +Cc: Al Viro, Andy Shevchenko
In-Reply-To: <20180618204816.30806-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.
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
^ permalink raw reply related
* [PATCH v5 1/3] bitfield: fix *_encode_bits()
From: Johannes Berg @ 2018-06-18 20:48 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.")
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
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
* Re: [PATCH v4 3/3] bitfield: add tests
From: Johannes Berg @ 2018-06-18 20:47 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <CAHp75VfeMBr57Frzvm2vJ2irJQat-6a+_dN9mtftAnwfPT=Bww@mail.gmail.com>
On Mon, 2018-06-18 at 23:44 +0300, Andy Shevchenko wrote:
> On Mon, Jun 18, 2018 at 11:37 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > 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.
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> Though license mismatch. Either GPL-2.0+ for SPDX, or "GPL v2" for _LICENSE().
Sigh. Yeah, I guess I'll resend. That's what I get for copy/pasting.
johannes
^ permalink raw reply
* Re: [PATCH v3] bitfield: fix *_encode_bits()
From: Andy Shevchenko @ 2018-06-18 20:46 UTC (permalink / raw)
To: Johannes Berg; +Cc: Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <1529354549.3092.36.camel@sipsolutions.net>
On Mon, Jun 18, 2018 at 11:42 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2018-06-18 at 23:40 +0300, Andy Shevchenko wrote:
>
>> The idea is to print what was the input, expected output and actual result.
>> Then you would see what exactly is broken.
>
> Yeah, I guess we could. I did some of that work.
>
>> I dunno how much we may take away from this certain test case, though
>> it would be better for my opinion.
>
> TBH though, I'm not sure I want to do this (right now at least). I don't
> think we gain anything from it, it's so basic ... so to me it's just
> pointless extra code.
I'm not insisting I'm trying to specify rationale behind it.
We may add this later at some point.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 3/3] bitfield: add tests
From: Andy Shevchenko @ 2018-06-18 20:44 UTC (permalink / raw)
To: Johannes Berg; +Cc: Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <20180618203750.28658-3-johannes@sipsolutions.net>
On Mon, Jun 18, 2018 at 11:37 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> 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.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Though license mismatch. Either GPL-2.0+ for SPDX, or "GPL v2" for _LICENSE().
>
> 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
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3] bitfield: fix *_encode_bits()
From: Johannes Berg @ 2018-06-18 20:42 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <CAHp75VesXgrh-XR-3bFMOF6eE0K=NyRvyY1tw1yOX44rJS9fag@mail.gmail.com>
On Mon, 2018-06-18 at 23:40 +0300, Andy Shevchenko wrote:
> The idea is to print what was the input, expected output and actual result.
> Then you would see what exactly is broken.
Yeah, I guess we could. I did some of that work.
> I dunno how much we may take away from this certain test case, though
> it would be better for my opinion.
TBH though, I'm not sure I want to do this (right now at least). I don't
think we gain anything from it, it's so basic ... so to me it's just
pointless extra code.
johannes
^ 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
* 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 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
* [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
* [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 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
* 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
* 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: 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 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: [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
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