Re: [PATCH net-next] virtio_net: Fix error code in __virtnet_get_hw_stats()
On Fri, May 10, 2024 at 03:50:45PM +0300, Dan Carpenter wrote:
> The virtnet_send_command_reply() function returns true on success or
> false on failure. The "ok" variable is true/false depending on whether
> it succeeds or not. It's up to the caller to translate the true/false
> into -EINVAL on failure or zero for success.
>
> The bug is that __virtnet_get_hw_stats() returns false for both
> errors and success. It's not a bug, but it is confusing that the caller
> virtnet_get_hw_stats() uses an "ok" variable to store negative error
> codes.
>
> Fix the bug and clean things up so that it's clear that
> __virtnet_get_hw_stats() returns zero on success or negative error codes
> on failure.
>
> Fixes: 941168f8b40e ("virtio_net: support device stats")
> Signed-off-by: Dan Carpenter
Hi Dan, all,
Strictly this is doing two things. But I agree that the "ok" variable in
virtnet_get_hw_stats() was very confusing, and I'm not sure how long it
would have taken me to grasp the fix without that change being here too.
Reviewed-by: Simon Horman
Re: [PATCH net-next v7 1/7] net: introduce rstreason to detect why the RST is sent
On Tue, Apr 23, 2024 at 10:17:31AM +0800, Jason Xing wrote:
> On Tue, Apr 23, 2024 at 10:14 AM Jason Xing wrote:
> >
> > Hi Simon,
> >
> > On Tue, Apr 23, 2024 at 2:28 AM Simon Horman wrote:
> > >
> > > On Mon, Apr 22, 2024 at 11:01:03AM +0800, Jason Xing wrote:
> > >
> > > ...
> > >
> > > > diff --git a/include/net/rstreason.h b/include/net/rstreason.h
> > >
> > > ...
> > >
> > > > +/**
> > > > + * There are three parts in order:
> > > > + * 1) reset reason in MPTCP: only for MPTCP use
> > > > + * 2) skb drop reason: relying on drop reasons for such as passive
> > > > reset
> > > > + * 3) independent reset reason: such as active reset reasons
> > > > + */
> > >
> > > Hi Jason,
> > >
> > > A minor nit from my side.
> > >
> > > '/**' denotes the beginning of a Kernel doc,
> > > but other than that, this comment is not a Kernel doc.
> > >
> > > FWIIW, I would suggest providing a proper Kernel doc for enum
> > > sk_rst_reason.
> > > But another option would be to simply make this a normal comment,
> > > starting with "/* There are"
> >
> > Thanks Simon. I'm trying to use the kdoc way to make it right :)
> >
> > How about this one:
> > /**
> > * enum sk_rst_reason - the reasons of socket reset
> > *
> > * The reason of skb drop, which is used in DCCP/TCP/MPTCP protocols.
>
> s/skb drop/sk reset/
>
> Sorry, I cannot withdraw my previous email in time.
>
> > *
> > * There are three parts in order:
> > * 1) skb drop reasons: relying on drop reasons for such as passive
> > reset
> > * 2) independent reset reasons: such as active reset reasons
> > * 3) reset reasons in MPTCP: only for MPTCP use
> > */
> > ?
> >
> > I chose to mimic what enum skb_drop_reason does in the
> > include/net/dropreason-core.h file.
> >
> > > +enum sk_rst_reason {
> > > + /**
> > > +* Copy from include/uapi/linux/mptcp.h.
> > > +* These reset fields will not be changed since they adhere to
> > > +* RFC 8684. So do not touch them. I'm going to list each
> > > definition
> > > +* of them respectively.
> > > +*/
> >
> > Thanks to you, I found another similar point where I smell something
> > wrong as in the above code. I'm going to replace '/**' with '/*' since
> > it's only a comment, not a kdoc.
Likewise, thanks Jason.
I haven't had time to look at v8 properly,
but I see that kernel-doc is happy with the changed
you have made there as discussed above.
Re: [PATCH net-next v7 1/7] net: introduce rstreason to detect why the RST is sent
On Mon, Apr 22, 2024 at 11:01:03AM +0800, Jason Xing wrote:
...
> diff --git a/include/net/rstreason.h b/include/net/rstreason.h
...
> +/**
> + * There are three parts in order:
> + * 1) reset reason in MPTCP: only for MPTCP use
> + * 2) skb drop reason: relying on drop reasons for such as passive reset
> + * 3) independent reset reason: such as active reset reasons
> + */
Hi Jason,
A minor nit from my side.
'/**' denotes the beginning of a Kernel doc,
but other than that, this comment is not a Kernel doc.
FWIIW, I would suggest providing a proper Kernel doc for enum sk_rst_reason.
But another option would be to simply make this a normal comment,
starting with "/* There are"
> +enum sk_rst_reason {
...
Re: [PATCH net-next v5] net/ipv4: add tracepoint for icmp_send
On Thu, Apr 11, 2024 at 06:01:54PM +0800, xu.xi...@zte.com.cn wrote:
> From: hepeilin
nit: it's nicer if this From line matches one of the Signed-off-by lines
From: Peilin He
> Introduce a tracepoint for icmp_send, which can help users to get more
> detail information conveniently when icmp abnormal events happen.
>
> 1. Giving an usecase example:
> =
> When an application experiences packet loss due to an unreachable UDP
> destination port, the kernel will send an exception message through the
> icmp_send function. By adding a trace point for icmp_send, developers or
> system administrators can obtain detailed information about the UDP
> packet loss, including the type, code, source address, destination address,
> source port, and destination port. This facilitates the trouble-shooting
> of UDP packet loss issues especially for those network-service
> applications.
>
> 2. Operation Instructions:
> ==
> Switch to the tracing directory.
> cd /sys/kernel/tracing
> Filter for destination port unreachable.
> echo "type==3 && code==3" > events/icmp/icmp_send/filter
> Enable trace event.
> echo 1 > events/icmp/icmp_send/enable
>
> 3. Result View:
>
> udp_client_erro-11370 [002] ...s.12 124.728002:
> icmp_send: icmp_send: type=3, code=3.
> From 127.0.0.1:41895 to 127.0.0.1: ulen=23
> skbaddr=589b167a
>
> v4->v5:
> Some fixes according to
> https://lore.kernel.org/all/CAL+tcoDeXXh+zcRk4PHnUk8ELnx=ce2pccqs7sfm0y9ak-e...@mail.gmail.com/
> 1.Adjust the position of trace_icmp_send() to before icmp_push_reply().
>
> v3->v4:
> Some fixes according to
> https://lore.kernel.org/all/CANn89i+EFEr7VHXNdOi59Ba_R1nFKSBJzBzkJFVgCTdXBx=y...@mail.gmail.com/
> 1.Add legality check for UDP header in SKB.
> 2.Target this patch for net-next.
>
> Changelog
>
> v2->v3:
> Some fixes according to
> https://lore.kernel.org/all/20240319102549.7f7f6...@gandalf.local.home/
> 1. Change the tracking directory to/sys/kernel/tracking.
> 2. Adjust the layout of the TP-STRUCT_entry parameter structure.
>
> v1->v2:
> Some fixes according to
> https://lore.kernel.org/all/CANn89iL-y9e_VFpdw=sztrnkru_tnuwqhufqtjvjsv-nz1x...@mail.gmail.com/
> 1. adjust the trace_icmp_send() to more protocols than UDP.
> 2. move the calling of trace_icmp_send after sanity checks
> in __icmp_send().
>
> Signed-off-by: Peilin He
nit: There should be a space between 'He' and '<'
> Reviewed-by: xu xin
This has been posted by xu xin, thus it is appropriate for
a Signed-off-by line from xu xin to be present.
> Reviewed-by: Yunkai Zhang
> Cc: Yang Yang
> Cc: Liu Chun
> Cc: Xuexin Jiang
Hi,
Unfortunately this patch does not apply to next-next.
Please rebase and repost after waiting a suitable time for
other review.
--
pw-bot: changes-requested
> ---
> include/trace/events/icmp.h | 65 +
> net/ipv4/icmp.c | 4 +++
> 2 files changed, 69 insertions(+)
> create mode 100644 include/trace/events/icmp.h
>
> diff --git a/include/trace/events/icmp.h b/include/trace/events/icmp.h
> new file mode 100644
> index 0..7d5190f48
> --- /dev/null
> +++ b/include/trace/events/icmp.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM icmp
> +
> +#if !defined(_TRACE_ICMP_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_ICMP_H
> +
> +#include
> +#include
> +
> +TRACE_EVENT(icmp_send,
> +
> + TP_PROTO(const struct sk_buff *skb, int type, int code),
> +
> + TP_ARGS(skb, type, code),
> +
> + TP_STRUCT__entry(
> + __field(const void *, skbaddr)
> + __field(int, type)
> + __field(int, code)
> + __array(__u8, saddr, 4)
> + __array(__u8, daddr, 4)
> + __field(__u16, sport)
> + __field(__u16, dport)
> + __field(unsigned short, ulen)
> + ),
> +
> + TP_fast_assign(
> + struct iphdr *iph = ip_hdr(skb);
> + int proto_4 = iph->protocol;
> + __be32 *p32;
> +
> + __entry->skbaddr = skb;
> + __entry->type = type;
> + __entry->code = code;
> +
> + struct udphdr *uh = udp_hdr(skb);
> + if (proto_4 != IPPROTO_UDP || (u8 *)uh < skb->head ||
> + (u8 *)uh + sizeof(struct udphdr) >
> skb_tail_pointer(skb)) {
> + __entry->sport = 0;
> + __entry->dport = 0;
> + __entry->ulen = 0;
> + } else {
> + __entry->sport = ntohs(uh->source);
> + __entry->dport = ntohs(uh->dest);
> +
Re: [RFC PATCH v2 1/4] tracing: add __print_sym() to replace __print_symbolic()
On Tue, Mar 26, 2024 at 08:15:56PM +0100, Johannes Berg wrote: > From: Johannes Berg > > The way __print_symbolic() works is limited and inefficient > in multiple ways: > - you can only use it with a static list of symbols, but >e.g. the SKB dropreasons are now a dynamic list > > - it builds the list in memory _three_ times, so it takes >a lot of memory: >- The print_fmt contains the list (since it's passed to > the macro there). This actually contains the names > _twice_, which is fixed up at runtime. >- TRACE_DEFINE_ENUM() puts a 24-byte struct trace_eval_map > for every entry, plus the string pointed to by it, which > cannot be deduplicated with the strings in the print_fmt >- The in-kernel symbolic printing creates yet another list > of struct trace_print_flags for trace_print_symbols_seq() > > - it also requires runtime fixup during init, which is a lot >of string parsing due to the print_fmt fixup > > Introduce __print_sym() to - over time - replace the old one. > We can easily extend this also to __print_flags later, but I > cared only about the SKB dropreasons for now, which has only > __print_symbolic(). > > This new __print_sym() requires only a single list of items, > created by TRACE_DEFINE_SYM_LIST(), or can even use another > already existing list by using TRACE_DEFINE_SYM_FNS() with > lookup and show methods. > > Then, instead of doing an init-time fixup, just do this at the > time when userspace reads the print_fmt. This way, dynamically > updated lists are possible. > > For userspace, nothing actually changes, because the print_fmt > is shown exactly the same way the old __print_symbolic() was. > > This adds about 4k .text in my test builds, but that'll be > more than paid for by the actual conversions. > > Signed-off-by: Johannes Berg Hi Johannes, I'm seeing some allmodconfig build problems with this applied on top of net-next. In file included from ./include/trace/trace_events.h:27, from ./include/trace/define_trace.h:102, from ./include/trace/events/module.h:134, from kernel/module/main.c:64: ./include/trace/stages/init.h:30: warning: "TRACE_DEFINE_SYM_FNS" redefined 30 | #define TRACE_DEFINE_SYM_FNS(_symbol_id, _lookup, _show) \ | In file included from ./include/linux/trace_events.h:11, from kernel/module/main.c:14: ./include/linux/tracepoint.h:130: note: this is the location of the previous definition 130 | #define TRACE_DEFINE_SYM_FNS(...) | ./include/trace/stages/init.h:54: warning: "TRACE_DEFINE_SYM_LIST" redefined 54 | #define TRACE_DEFINE_SYM_LIST(_symbol_id, ...) \ | ./include/linux/tracepoint.h:131: note: this is the location of the previous definition 131 | #define TRACE_DEFINE_SYM_LIST(...) |
Re: [PATCH] net: hns3: tracing: fix hclgevf trace event strings
On Thu, Mar 14, 2024 at 12:00:27PM -0400, Steven Rostedt wrote:
> On Thu, 14 Mar 2024 15:39:28 +0100
> Paolo Abeni wrote:
>
> > On Wed, 2024-03-13 at 09:34 -0400, Steven Rostedt wrote:
...
> > > Fixes: d8355240cf8fb ("net: hns3: add trace event support for PF/VF
> > > mailbox")
> >
> > checkpactch in strict mode complains the hash is not 12 char long.
>
> Hmm, I wonder why my git blame gives me 13 characters in the sha. (I cut
> and pasted it from git blame). My git config has:
>
> [core]
> abbrev = 12
I wonder if there is a collusion at 12 chars in your local tree.
...
Re: [PATCH net] ipvs: Simplify the allocation of ip_vs_conn slab caches
On Thu, Jan 18, 2024 at 10:22:05AM +0800, Kunwu Chan wrote:
> Hi Simon,
>
> Thanks for your reply.
>
> On 2024/1/17 17:29, Simon Horman wrote:
> > On Wed, Jan 17, 2024 at 03:20:45PM +0800, Kunwu Chan wrote:
> > > Use the new KMEM_CACHE() macro instead of direct kmem_cache_create
> > > to simplify the creation of SLAB caches.
> > >
> > > Signed-off-by: Kunwu Chan
> >
> > Hi Kunwu Chan,
> >
> > I think this is more of a cleanup than a fix,
> > so it should probably be targeted at 'nf-next' rather than 'net'.
> Thanks, I'm confused about when to use "nf-next" or "net" or "net-next".
> "nf-next" means fixing errors for linux-next.git and linux-stable.git, while
> "nf" or "next" just means linux-next.git?
Hi Kunwu,
nf is for fixes for Netfilter (which includes IPVS). The target tree is nf.git
nf-next is for non-fixes for Netfilter. The target tree if nf-next.git
net is for fixes for Networking code, which does not have a more specific
tree (as is the case for Netfilter). The target tree is net.git.
Liikewise, net-next is for non-fixes for Networking code.
The target tree is net-next.git
The MAINTAINERS file, and get_maintainers.pl script are useful here.
nf is merged into net on request from the Netfilter maintainers,
this is it's path to released kernels.
Likewise, nf-next is merged into net-next.
>
> >
> > If it is a fix, then I would suggest targeting it at 'nf'
> > and providing a Fixes tag.
> I'll keep it in mind in the future.
> >
> > The above notwithstanding, this looks good to me.
> >
> > Acked-by: Simon Horman
> >
> > > ---
> > > net/netfilter/ipvs/ip_vs_conn.c | 4 +---
> > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/net/netfilter/ipvs/ip_vs_conn.c
> > > b/net/netfilter/ipvs/ip_vs_conn.c
> > > index a743db073887..98d7dbe3d787 100644
> > > --- a/net/netfilter/ipvs/ip_vs_conn.c
> > > +++ b/net/netfilter/ipvs/ip_vs_conn.c
> > > @@ -1511,9 +1511,7 @@ int __init ip_vs_conn_init(void)
> > > return -ENOMEM;
> > > /* Allocate ip_vs_conn slab cache */
> > > - ip_vs_conn_cachep = kmem_cache_create("ip_vs_conn",
> > > - sizeof(struct ip_vs_conn), 0,
> > > - SLAB_HWCACHE_ALIGN, NULL);
> > > + ip_vs_conn_cachep = KMEM_CACHE(ip_vs_conn, SLAB_HWCACHE_ALIGN);
> > > if (!ip_vs_conn_cachep) {
> > > kvfree(ip_vs_conn_tab);
> > > return -ENOMEM;
> --
> Thanks,
> Kunwu
>
Re: [PATCH net] ipvs: Simplify the allocation of ip_vs_conn slab caches
On Wed, Jan 17, 2024 at 03:20:45PM +0800, Kunwu Chan wrote:
> Use the new KMEM_CACHE() macro instead of direct kmem_cache_create
> to simplify the creation of SLAB caches.
>
> Signed-off-by: Kunwu Chan
Hi Kunwu Chan,
I think this is more of a cleanup than a fix,
so it should probably be targeted at 'nf-next' rather than 'net'.
If it is a fix, then I would suggest targeting it at 'nf'
and providing a Fixes tag.
The above notwithstanding, this looks good to me.
Acked-by: Simon Horman
> ---
> net/netfilter/ipvs/ip_vs_conn.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
> index a743db073887..98d7dbe3d787 100644
> --- a/net/netfilter/ipvs/ip_vs_conn.c
> +++ b/net/netfilter/ipvs/ip_vs_conn.c
> @@ -1511,9 +1511,7 @@ int __init ip_vs_conn_init(void)
> return -ENOMEM;
>
> /* Allocate ip_vs_conn slab cache */
> - ip_vs_conn_cachep = kmem_cache_create("ip_vs_conn",
> - sizeof(struct ip_vs_conn), 0,
> - SLAB_HWCACHE_ALIGN, NULL);
> + ip_vs_conn_cachep = KMEM_CACHE(ip_vs_conn, SLAB_HWCACHE_ALIGN);
> if (!ip_vs_conn_cachep) {
> kvfree(ip_vs_conn_tab);
> return -ENOMEM;
Re: [PATCH net] net: ipvs: avoid stat macros calls from preemptible context
On Mon, Jan 15, 2024 at 05:39:22PM +0300, Fedor Pchelkin wrote:
> Inside decrement_ttl() upon discovering that the packet ttl has exceeded,
> __IP_INC_STATS and __IP6_INC_STATS macros can be called from preemptible
> context having the following backtrace:
>
> check_preemption_disabled: 48 callbacks suppressed
> BUG: using __this_cpu_add() in preemptible [] code: curl/1177
> caller is decrement_ttl+0x217/0x830
> CPU: 5 PID: 1177 Comm: curl Not tainted 6.7.0+ #34
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 04/01/2014
> Call Trace:
>
> dump_stack_lvl+0xbd/0xe0
> check_preemption_disabled+0xd1/0xe0
> decrement_ttl+0x217/0x830
> __ip_vs_get_out_rt+0x4e0/0x1ef0
> ip_vs_nat_xmit+0x205/0xcd0
> ip_vs_in_hook+0x9b1/0x26a0
> nf_hook_slow+0xc2/0x210
> nf_hook+0x1fb/0x770
> __ip_local_out+0x33b/0x640
> ip_local_out+0x2a/0x490
> __ip_queue_xmit+0x990/0x1d10
> __tcp_transmit_skb+0x288b/0x3d10
> tcp_connect+0x3466/0x5180
> tcp_v4_connect+0x1535/0x1bb0
> __inet_stream_connect+0x40d/0x1040
> inet_stream_connect+0x57/0xa0
> __sys_connect_file+0x162/0x1a0
> __sys_connect+0x137/0x160
> __x64_sys_connect+0x72/0xb0
> do_syscall_64+0x6f/0x140
> entry_SYSCALL_64_after_hwframe+0x6e/0x76
> RIP: 0033:0x7fe6dbbc34e0
>
> Use the corresponding preemption-aware variants: IP_INC_STATS and
> IP6_INC_STATS.
>
> Found by Linux Verification Center (linuxtesting.org).
>
> Fixes: 8d8e20e2d7bb ("ipvs: Decrement ttl")
> Signed-off-by: Fedor Pchelkin
Acked-by: Simon Horman
Re: [PATCH] vhost-vdpa: Remove usage of the deprecated ida_simple_xx() API
On Mon, Jan 15, 2024 at 09:35:50PM +0100, Christophe JAILLET wrote: > ida_alloc() and ida_free() should be preferred to the deprecated > ida_simple_get() and ida_simple_remove(). > > Note that the upper limit of ida_simple_get() is exclusive, buInputt the one > of > ida_alloc_max() is inclusive. So a -1 has been added when needed. > > Signed-off-by: Christophe JAILLET Reviewed-by: Simon Horman
Re: [PATCH V1] net: qrtr: ns: Ignore ENODEV failures in ns
On Tue, Dec 26, 2023 at 04:20:03PM -0800, Chris Lew wrote:
>
>
> On 12/23/2023 5:56 AM, Simon Horman wrote:
> > [Dropped bjorn.anders...@kernel.org, as the correct address seems
> > to be anders...@kernel.org, which is already in the CC list.
> > kernel.org rejected sending this email without that update.]
> >
> > On Thu, Dec 21, 2023 at 03:36:50PM +0530, Sarannya S wrote:
> > > From: Chris Lew
> > >
> > > Ignore the ENODEV failures returned by kernel_sendmsg(). These errors
> > > indicate that either the local port has been closed or the remote has
> > > gone down. Neither of these scenarios are fatal and will eventually be
> > > handled through packets that are later queued on the control port.
> > >
> > > Signed-off-by: Chris Lew
> > > Signed-off-by: Sarannya Sasikumar
> > > ---
> > > net/qrtr/ns.c | 11 +++
> > > 1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> > > index abb0c70..8234339 100644
> > > --- a/net/qrtr/ns.c
> > > +++ b/net/qrtr/ns.c
> > > @@ -157,7 +157,7 @@ static int service_announce_del(struct sockaddr_qrtr
> > > *dest,
> > > msg.msg_namelen = sizeof(*dest);
> > > ret = kernel_sendmsg(qrtr_ns.sock, , , 1, sizeof(pkt));
> > > - if (ret < 0)
> > > + if (ret < 0 && ret != -ENODEV)
> > > pr_err("failed to announce del service\n");
> > > return ret;
> >
> > Hi,
> >
> > The caller of service_announce_del() ignores it's return value.
> > So the only action on error is the pr_err() call above, and so
> > with this patch -ENODEV is indeed ignored.
> >
> > However, I wonder if it would make things clearer to the reader (me?)
> > if the return type of service_announce_del was updated void. Because
> > as things stand -ENODEV may be returned, which implies something might
> > handle that, even though it doe not.
> >
> > The above notwithstanding, this change looks good to me.
> >
> > Reviewed-by: Simon Horman
> >
> > ...
>
> Hi Simon, thanks for the review and suggestion. We weren't sure whether we
> should change the function prototype on these patches on the chance that
> there will be something that listens and handles this in the future. I think
> it's a good idea to change it to void and we can change it back if there is
> such a usecase in the future.
Hi Chris,
yes, I think that would be a good approach.
Re: [PATCH V1] net: qrtr: ns: Return 0 if server port is not present
[Dropped bjorn.anders...@kernel.org, as the correct address seems to be anders...@kernel.org, which is already in the CC list. kernel.org rejected sending this email without that update.] On Thu, Dec 21, 2023 at 03:36:51PM +0530, Sarannya S wrote: > When a 'DEL_CLIENT' message is received from the remote, the corresponding > server port gets deleted. A DEL_SERVER message is then announced for this > server. As part of handling the subsequent DEL_SERVER message, the name- > server attempts to delete the server port which results in a '-ENOENT' error. > The return value from server_del() is then propagated back to qrtr_ns_worker, > causing excessive error prints. > To address this, return 0 from control_cmd_del_server() without checking the > return value of server_del(), since the above scenario is not an error case > and hence server_del() doesn't have any other error return value. > > Signed-off-by: Sarannya Sasikumar Thanks, I have a suggestion below. But that notwithstanding this change looks good to me. Reviewed-by: Simon Horman > --- > net/qrtr/ns.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c > index b1db0b5..abb0c70 100644 > --- a/net/qrtr/ns.c > +++ b/net/qrtr/ns.c > @@ -512,7 +512,9 @@ static int ctrl_cmd_del_server(struct sockaddr_qrtr *from, > if (!node) > return -ENOENT; > > - return server_del(node, port, true); > + server_del(node, port, true); > + > + return 0; > } With this change the return value of server_del() now seems to be ignored by all callers. Perhaps it would make sense to update it to return void? > > static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from, > -- > 2.7.4 > >
Re: [PATCH V1] net: qrtr: ns: Ignore ENODEV failures in ns
[Dropped bjorn.anders...@kernel.org, as the correct address seems
to be anders...@kernel.org, which is already in the CC list.
kernel.org rejected sending this email without that update.]
On Thu, Dec 21, 2023 at 03:36:50PM +0530, Sarannya S wrote:
> From: Chris Lew
>
> Ignore the ENODEV failures returned by kernel_sendmsg(). These errors
> indicate that either the local port has been closed or the remote has
> gone down. Neither of these scenarios are fatal and will eventually be
> handled through packets that are later queued on the control port.
>
> Signed-off-by: Chris Lew
> Signed-off-by: Sarannya Sasikumar
> ---
> net/qrtr/ns.c | 11 +++
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index abb0c70..8234339 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c
> @@ -157,7 +157,7 @@ static int service_announce_del(struct sockaddr_qrtr
> *dest,
> msg.msg_namelen = sizeof(*dest);
>
> ret = kernel_sendmsg(qrtr_ns.sock, , , 1, sizeof(pkt));
> - if (ret < 0)
> + if (ret < 0 && ret != -ENODEV)
> pr_err("failed to announce del service\n");
>
> return ret;
Hi,
The caller of service_announce_del() ignores it's return value.
So the only action on error is the pr_err() call above, and so
with this patch -ENODEV is indeed ignored.
However, I wonder if it would make things clearer to the reader (me?)
if the return type of service_announce_del was updated void. Because
as things stand -ENODEV may be returned, which implies something might
handle that, even though it doe not.
The above notwithstanding, this change looks good to me.
Reviewed-by: Simon Horman
...
Re: [PATCH net-next v3 3/3] net: add netmem_ref to skb_frag_t
On Wed, Dec 20, 2023 at 01:45:02PM -0800, Mina Almasry wrote:
> Use netmem_ref instead of page in skb_frag_t. Currently netmem_ref
> is always a struct page underneath, but the abstraction allows efforts
> to add support for skb frags not backed by pages.
>
> There is unfortunately 1 instance where the skb_frag_t is assumed to be
> a bio_vec in kcm. For this case, add a debug assert that the skb frag is
> indeed backed by a page, and do a cast.
>
> Add skb[_frag]_fill_netmem_*() and skb_add_rx_frag_netmem() helpers so
> that the API can be used to create netmem skbs.
>
> Signed-off-by: Mina Almasry
...
> diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
> index 65d1f6755f98..3180a54b2c68 100644
> --- a/net/kcm/kcmsock.c
> +++ b/net/kcm/kcmsock.c
> @@ -636,9 +636,15 @@ static int kcm_write_msgs(struct kcm_sock *kcm)
> for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
> msize += skb_shinfo(skb)->frags[i].bv_len;
>
> + /* The cast to struct bio_vec* here assumes the frags are
> + * struct page based. WARN if there is no page in this skb.
> + */
> + DEBUG_NET_WARN_ON_ONCE(
> + !skb_frag_page(_shinfo(skb)->frags[0]));
> +
> iov_iter_bvec(_iter, ITER_SOURCE,
> - skb_shinfo(skb)->frags, skb_shinfo(skb)->nr_frags,
> - msize);
> + (const struct bio_vec *)skb_shinfo(skb)->frags,
> + skb_shinfo(skb)->nr_frags, msize);
> iov_iter_advance(_iter, txm->frag_offset);
>
> do {
Hi Mina,
something isn't quite right here.
...//kcmsock.c:637:39: error: no member named 'bv_len' in 'struct skb_frag'
637 | msize += skb_shinfo(skb)->frags[i].bv_len;
| ~ ^
--
pw-bot: changes-requested
Re: [PATCH] neighbor: tracing: Move pin6 inside CONFIG_IPV6=y section
On Mon, Oct 16, 2023 at 02:49:04PM +0200, Geert Uytterhoeven wrote:
> When CONFIG_IPV6=n, and building with W=1:
>
> In file included from include/trace/define_trace.h:102,
>from include/trace/events/neigh.h:255,
>from net/core/net-traces.c:51:
> include/trace/events/neigh.h: In function
> ‘trace_event_raw_event_neigh_create’:
> include/trace/events/neigh.h:42:34: error: variable ‘pin6’ set but not
> used [-Werror=unused-but-set-variable]
>42 | struct in6_addr *pin6;
> | ^~~~
> include/trace/trace_events.h:402:11: note: in definition of macro
> ‘DECLARE_EVENT_CLASS’
> 402 | { assign; }
> \
> | ^~
> include/trace/trace_events.h:44:30: note: in expansion of macro ‘PARAMS’
>44 | PARAMS(assign), \
> | ^~
> include/trace/events/neigh.h:23:1: note: in expansion of macro
> ‘TRACE_EVENT’
>23 | TRACE_EVENT(neigh_create,
> | ^~~
> include/trace/events/neigh.h:41:9: note: in expansion of macro
> ‘TP_fast_assign’
>41 | TP_fast_assign(
> | ^~
> In file included from include/trace/define_trace.h:103,
>from include/trace/events/neigh.h:255,
>from net/core/net-traces.c:51:
> include/trace/events/neigh.h: In function ‘perf_trace_neigh_create’:
> include/trace/events/neigh.h:42:34: error: variable ‘pin6’ set but not
> used [-Werror=unused-but-set-variable]
>42 | struct in6_addr *pin6;
> | ^~~~
> include/trace/perf.h:51:11: note: in definition of macro
> ‘DECLARE_EVENT_CLASS’
>51 | { assign; }
> \
> | ^~
> include/trace/trace_events.h:44:30: note: in expansion of macro ‘PARAMS’
>44 | PARAMS(assign), \
> | ^~
> include/trace/events/neigh.h:23:1: note: in expansion of macro
> ‘TRACE_EVENT’
>23 | TRACE_EVENT(neigh_create,
> | ^~~
> include/trace/events/neigh.h:41:9: note: in expansion of macro
> ‘TP_fast_assign’
>41 | TP_fast_assign(
> | ^~
>
> Indeed, the variable pin6 is declared and initialized unconditionally,
> while it is only used and needlessly re-initialized when support for
> IPv6 is enabled.
>
> Fix this by dropping the unused variable initialization, and moving the
> variable declaration inside the existing section protected by a check
> for CONFIG_IPV6.
>
> Fixes: fc651001d2c5ca4f ("neighbor: Add tracepoint to __neigh_create")
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Simon Horman
Tested-by: Simon Horman # build-tested
Re: [PATCH v4 net] net: hv_netvsc: fix netvsc_send_completion to avoid multiple message length checks
On Thu, Sep 07, 2023 at 12:56:47PM -0700, Sonia Sharma wrote:
> From: Sonia Sharma
>
> The switch statement in netvsc_send_completion() is incorrectly validating
> the length of incoming network packets by falling through to the next case.
> Avoid the fallthrough. Instead break after a case match and then process
> the complete() call.
> The current code has not caused any known failures. But nonetheless, the
> code should be corrected as a different ordering of the switch cases might
> cause a length check to fail when it should not.
>
> Fixes: 44144185951a0f ("hv_netvsc: Add validation for untrusted Hyper-V
> values")
As the current code is correct - it works - I feel that this is more of a
clean-up than a fix. As such I suggest dropping the fixes tag and
retargeting at net-next (which is due to re-open in the coming days).
> Signed-off-by: Sonia Sharma
>
> ---
> Changes in v3:
> * added return statement in default case as pointed by Michael Kelley.
> Changes in v4:
> * added fixes tag
> * modified commit message to explain the issue fixed by patch.
> ---
> drivers/net/hyperv/netvsc.c | 18 ++
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 82e9796c8f5e..0f7e4d36 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -851,7 +851,7 @@ static void netvsc_send_completion(struct net_device
> *ndev,
> msglen);
> return;
> }
> - fallthrough;
> + break;
>
> case NVSP_MSG1_TYPE_SEND_RECV_BUF_COMPLETE:
> if (msglen < sizeof(struct nvsp_message_header) +
> @@ -860,7 +860,7 @@ static void netvsc_send_completion(struct net_device
> *ndev,
> msglen);
> return;
> }
> - fallthrough;
> + break;
>
> case NVSP_MSG1_TYPE_SEND_SEND_BUF_COMPLETE:
> if (msglen < sizeof(struct nvsp_message_header) +
> @@ -869,7 +869,7 @@ static void netvsc_send_completion(struct net_device
> *ndev,
> msglen);
> return;
> }
> - fallthrough;
> + break;
>
> case NVSP_MSG5_TYPE_SUBCHANNEL:
> if (msglen < sizeof(struct nvsp_message_header) +
> @@ -878,10 +878,6 @@ static void netvsc_send_completion(struct net_device
> *ndev,
> msglen);
> return;
> }
> - /* Copy the response back */
> - memcpy(_device->channel_init_pkt, nvsp_packet,
> -sizeof(struct nvsp_message));
> - complete(_device->channel_init_wait);
> break;
>
> case NVSP_MSG1_TYPE_SEND_RNDIS_PKT_COMPLETE:
> @@ -904,13 +900,19 @@ static void netvsc_send_completion(struct net_device
> *ndev,
>
> netvsc_send_tx_complete(ndev, net_device, incoming_channel,
> desc, budget);
> - break;
> + return;
>
> default:
> netdev_err(ndev,
> "Unknown send completion type %d received!!\n",
> nvsp_packet->hdr.msg_type);
> + return;
> }
> +
> + /* Copy the response back */
> + memcpy(_device->channel_init_pkt, nvsp_packet,
> + sizeof(struct nvsp_message));
nit: the indentation of the line above is not correct.
memcpy(_device->channel_init_pkt, nvsp_packet,
sizeof(struct nvsp_message));
> + complete(_device->channel_init_wait);
> }
>
> static u32 netvsc_get_next_send_section(struct netvsc_device *net_device)
> --
> 2.25.1
>
>
Re: [PATCH 08/19] netfilter: ipvs: A spello fix
+ netfilter-de...@vger.kernel.org On Sat, Mar 27, 2021 at 04:43:01AM +0530, Bhaskar Chowdhury wrote: > s/registerd/registered/ > > Signed-off-by: Bhaskar Chowdhury Reviewed-by: Simon Horman > --- > net/netfilter/ipvs/ip_vs_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c > index 0c132ff9b446..128690c512df 100644 > --- a/net/netfilter/ipvs/ip_vs_core.c > +++ b/net/netfilter/ipvs/ip_vs_core.c > @@ -2398,7 +2398,7 @@ static int __net_init __ip_vs_init(struct net *net) > if (ipvs == NULL) > return -ENOMEM; > > - /* Hold the beast until a service is registerd */ > + /* Hold the beast until a service is registered */ > ipvs->enable = 0; > ipvs->net = net; > /* Counters used for creating unique names */ > -- > 2.26.2 >
Re: [PATCH] netfilter: ipvs: A spello fix
+ netfilter-de...@vger.kernel.org On Sat, Mar 27, 2021 at 04:42:48AM +0530, Bhaskar Chowdhury wrote: > s/registerd/registered/ > > Signed-off-by: Bhaskar Chowdhury Reviewed-by: Simon Horman > --- > net/netfilter/ipvs/ip_vs_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c > index 0c132ff9b446..128690c512df 100644 > --- a/net/netfilter/ipvs/ip_vs_core.c > +++ b/net/netfilter/ipvs/ip_vs_core.c > @@ -2398,7 +2398,7 @@ static int __net_init __ip_vs_init(struct net *net) > if (ipvs == NULL) > return -ENOMEM; > > - /* Hold the beast until a service is registerd */ > + /* Hold the beast until a service is registered */ > ipvs->enable = 0; > ipvs->net = net; > /* Counters used for creating unique names */ > -- > 2.26.2 >
Re: [PATCH] drivers: net: ethernet: struct sk_buff is declared duplicately
On Thu, Mar 25, 2021 at 02:35:55PM +0800, Wan Jiabing wrote: > struct sk_buff has been declared. Remove the duplicate. > > Signed-off-by: Wan Jiabing Thanks, nice catch. Reviewed-by: Simon Horman
Re: [PATCH RESEND][next] nfp: Fix fall-through warnings for Clang
On Fri, Mar 05, 2021 at 03:49:37AM -0600, Gustavo A. R. Silva wrote: > In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning > by explicitly adding a break statement instead of letting the code fall > through to the next case. > > Link: https://github.com/KSPP/linux/issues/115 > Signed-off-by: Gustavo A. R. Silva Thanks Gustavo, this looks good to me. Acked-by: Simon Horman > --- > drivers/net/ethernet/netronome/nfp/nfp_net_repr.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c > b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c > index b3cabc274121..3b8e675087de 100644 > --- a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c > +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c > @@ -103,6 +103,7 @@ nfp_repr_get_stats64(struct net_device *netdev, struct > rtnl_link_stats64 *stats) > case NFP_PORT_PF_PORT: > case NFP_PORT_VF_PORT: > nfp_repr_vnic_get_stats64(repr->port, stats); > + break; > default: > break; > } > -- > 2.27.0 >
Re: [PATCH] nfp: remove h from printk format specifier
On Wed, Dec 23, 2020 at 12:20:53PM -0800, t...@redhat.com wrote:
> From: Tom Rix
>
> This change fixes the checkpatch warning described in this commit
> commit cbacb5ab0aa0 ("docs: printk-formats: Stop encouraging use of
> unnecessary %h[xudi] and %hh[xudi]")
>
> Standard integer promotion is already done and %hx and %hhx is useless
> so do not encourage the use of %hh[xudi] or %h[xudi].
>
> Signed-off-by: Tom Rix
Hi Tom,
This patch looks appropriate for net-next, which is currently closed.
The changes look fine, but I'm curious to know if its intentionally that
the following was left alone in
ethernet/netronome/nfp/nfp_net_ethtool.c:nfp_net_get_nspinfo()
snprintf(version, ETHTOOL_FWVERS_LEN, "%hu.%hu"
If the above was not intentional then perhaps you could respin with that
updated and resubmit when net-next re-opens. Feel free to add:
Reviewed-by: Simon Horman
Re: [PATCH] ethernet: select CONFIG_CRC32 as needed
On Fri, Dec 04, 2020 at 12:20:37AM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann > > A number of ethernet drivers require crc32 functionality to be > avaialable in the kernel, causing a link error otherwise: > > arm-linux-gnueabi-ld: drivers/net/ethernet/agere/et131x.o: in function > `et1310_setup_device_for_multicast': > et131x.c:(.text+0x5918): undefined reference to `crc32_le' > arm-linux-gnueabi-ld: drivers/net/ethernet/cadence/macb_main.o: in function > `macb_start_xmit': > macb_main.c:(.text+0x4b88): undefined reference to `crc32_le' > arm-linux-gnueabi-ld: drivers/net/ethernet/faraday/ftgmac100.o: in function > `ftgmac100_set_rx_mode': > ftgmac100.c:(.text+0x2b38): undefined reference to `crc32_le' > arm-linux-gnueabi-ld: drivers/net/ethernet/freescale/fec_main.o: in function > `set_multicast_list': > fec_main.c:(.text+0x6120): undefined reference to `crc32_le' > arm-linux-gnueabi-ld: drivers/net/ethernet/freescale/fman/fman_dtsec.o: in > function `dtsec_add_hash_mac_address': > fman_dtsec.c:(.text+0x830): undefined reference to `crc32_le' > arm-linux-gnueabi-ld: > drivers/net/ethernet/freescale/fman/fman_dtsec.o:fman_dtsec.c:(.text+0xb68): > more undefined references to `crc32_le' follow > arm-linux-gnueabi-ld: > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_hwinfo.o: in function > `nfp_hwinfo_read': > nfp_hwinfo.c:(.text+0x250): undefined reference to `crc32_be' > arm-linux-gnueabi-ld: nfp_hwinfo.c:(.text+0x288): undefined reference to > `crc32_be' > arm-linux-gnueabi-ld: > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_resource.o: in function > `nfp_resource_acquire': > nfp_resource.c:(.text+0x144): undefined reference to `crc32_be' > arm-linux-gnueabi-ld: nfp_resource.c:(.text+0x158): undefined reference to > `crc32_be' > arm-linux-gnueabi-ld: drivers/net/ethernet/nxp/lpc_eth.o: in function > `lpc_eth_set_multicast_list': > lpc_eth.c:(.text+0x1934): undefined reference to `crc32_le' > arm-linux-gnueabi-ld: drivers/net/ethernet/rocker/rocker_ofdpa.o: in function > `ofdpa_flow_tbl_do': > rocker_ofdpa.c:(.text+0x2e08): undefined reference to `crc32_le' > arm-linux-gnueabi-ld: drivers/net/ethernet/rocker/rocker_ofdpa.o: in function > `ofdpa_flow_tbl_del': > rocker_ofdpa.c:(.text+0x3074): undefined reference to `crc32_le' > arm-linux-gnueabi-ld: drivers/net/ethernet/rocker/rocker_ofdpa.o: in function > `ofdpa_port_fdb': > arm-linux-gnueabi-ld: > drivers/net/ethernet/mellanox/mlx5/core/steering/dr_ste.o: in function > `mlx5dr_ste_calc_hash_index': > dr_ste.c:(.text+0x354): undefined reference to `crc32_le' > arm-linux-gnueabi-ld: drivers/net/ethernet/microchip/lan743x_main.o: in > function `lan743x_netdev_set_multicast': > lan743x_main.c:(.text+0x5dc4): undefined reference to `crc32_le' > > Add the missing 'select CRC32' entries in Kconfig for each of them. > > Signed-off-by: Arnd Bergmann > --- > drivers/net/ethernet/agere/Kconfig | 1 + > drivers/net/ethernet/cadence/Kconfig| 1 + > drivers/net/ethernet/faraday/Kconfig| 1 + > drivers/net/ethernet/freescale/Kconfig | 1 + > drivers/net/ethernet/freescale/fman/Kconfig | 1 + > drivers/net/ethernet/mellanox/mlx5/core/Kconfig | 1 + > drivers/net/ethernet/microchip/Kconfig | 1 + > drivers/net/ethernet/netronome/Kconfig | 1 + > drivers/net/ethernet/nxp/Kconfig| 1 + > drivers/net/ethernet/rocker/Kconfig | 1 + > 10 files changed, 10 insertions(+) Hi Arnd, I'm slightly curious to know how you configured the kernel to build the Netronome NFP driver but not CRC32 but nonetheless I have no objection to this change. For the Netronome portion: Acked-by: Simon Horman
Re: [PATCH][next] nfp: tls: Fix unreachable code issue
On Tue, Nov 17, 2020 at 11:13:47AM -0600, Gustavo A. R. Silva wrote: > Fix the following unreachable code issue: > >drivers/net/ethernet/netronome/nfp/crypto/tls.c: In function > 'nfp_net_tls_add': >include/linux/compiler_attributes.h:208:41: warning: statement will never > be executed [-Wswitch-unreachable] > 208 | # define fallthrough > __attribute__((__fallthrough__)) > | ^ >drivers/net/ethernet/netronome/nfp/crypto/tls.c:299:3: note: in expansion > of macro 'fallthrough' > 299 | fallthrough; > | ^~~ > > Reported-by: kernel test robot > Signed-off-by: Gustavo A. R. Silva Reviewed-by: Simon Horman
Re: [PATCH] nfp: Fix passing zero to 'PTR_ERR'
On Thu, Nov 12, 2020 at 10:58:52PM +0800, YueHaibing wrote:
> nfp_cpp_from_nfp6000_pcie() returns ERR_PTR() and never returns
> NULL. The NULL test should be removed, also return correct err.
>
> Fixes: 63461a028f76 ("nfp: add the PF driver")
> Signed-off-by: YueHaibing
Thanks, this does indeed seem to be the case.
Reviewed-by: Simon Horman
Re: [PATCH] ipvs: adjust the debug order of src and dst
On Wed, Sep 23, 2020 at 01:49:59PM +0800, longguang.yue wrote: > From: ylg > > adjust the debug order of src and dst when tcp state changes > > Signed-off-by: ylg Hi, This sounds reasonable to me but please provide your real name in the Signed-off-by name, which should be consistent with the From field at the top of the commit message (or, if absent of the email). Thanks! > --- > net/netfilter/ipvs/ip_vs_proto_tcp.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_proto_tcp.c > b/net/netfilter/ipvs/ip_vs_proto_tcp.c > index dc2e7da2742a..6567eb45a234 100644 > --- a/net/netfilter/ipvs/ip_vs_proto_tcp.c > +++ b/net/netfilter/ipvs/ip_vs_proto_tcp.c > @@ -548,10 +548,10 @@ set_tcp_state(struct ip_vs_proto_data *pd, struct > ip_vs_conn *cp, > th->fin ? 'F' : '.', > th->ack ? 'A' : '.', > th->rst ? 'R' : '.', > - IP_VS_DBG_ADDR(cp->daf, >daddr), > - ntohs(cp->dport), > IP_VS_DBG_ADDR(cp->af, >caddr), > ntohs(cp->cport), > + IP_VS_DBG_ADDR(cp->daf, >daddr), > + ntohs(cp->dport), > tcp_state_name(cp->state), > tcp_state_name(new_state), > refcount_read(>refcnt)); > -- > 2.20.1 (Apple Git-117) >
Re: [PATCH net-next] ipvs: Remove unused macros
On Fri, Sep 18, 2020 at 09:16:56PM +0800, YueHaibing wrote:
> They are not used since commit e4ff67513096 ("ipvs: add
> sync_maxlen parameter for the sync daemon")
>
> Signed-off-by: YueHaibing
Thanks, this look good to me.
Acked-by: Simon Horman
Pablo, please consider this for nf-next.
> ---
> net/netfilter/ipvs/ip_vs_sync.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> index 2b8abbfe018c..16b48064f715 100644
> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c
> @@ -242,9 +242,6 @@ struct ip_vs_sync_thread_data {
>|IPVS Sync Connection (1) |
> */
>
> -#define SYNC_MESG_HEADER_LEN 4
> -#define MAX_CONNS_PER_SYNCBUFF 255 /* nr_conns in ip_vs_sync_mesg is 8
> bit */
> -
> /* Version 0 header */
> struct ip_vs_sync_mesg_v0 {
> __u8nr_conns;
> --
> 2.17.1
>
Re: [oss-drivers] [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
On Wed, Sep 09, 2020 at 01:06:39PM -0700, Joe Perches wrote:
> fallthrough to a separate case/default label break; isn't very readable.
>
> Convert pseudo-keyword fallthrough; statements to a simple break; when
> the next label is case or default and the only statement in the next
> label block is break;
>
> Found using:
>
> $ grep-2.5.4 -rP --include=*.[ch] -n
> "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
>
> Miscellanea:
>
> o Move or coalesce a couple label blocks above a default: block.
>
> Signed-off-by: Joe Perches
...
> diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c
> b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c
> index 252fe06f58aa..1d5b87079104 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c
> @@ -345,7 +345,7 @@ static int matching_bar(struct nfp_bar *bar, u32 tgt, u32
> act, u32 tok,
> baract = NFP_CPP_ACTION_RW;
> if (act == 0)
> act = NFP_CPP_ACTION_RW;
> - fallthrough;
> + break;
> case NFP_PCIE_BAR_PCIE2CPP_MapType_FIXED:
> break;
> default:
This is a cascading fall-through handling all map types.
I don't think this change improves readability.
...
Re: [PATCH] drivers/net/ethernet: fix a typo for stmmac_pltfr_suspend
Hi Wang, On Wed, Sep 09, 2020 at 08:19:21PM +0800, Wang Qing wrote: > Change the comment typo: "direcly" -> "directly". > > Signed-off-by: Wang Qing > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) git log tells me that the correct prefix for this patch is probably "net: stmmac:" rather than "drivers/net/ethernet:' Probably this patch is targeted at net-next and should include net-next in the subject like this: [PATCH net-next] ... > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > index f32317f..b666bb9 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > @@ -711,7 +711,7 @@ EXPORT_SYMBOL_GPL(stmmac_pltfr_remove); > /** > * stmmac_pltfr_suspend > * @dev: device pointer > - * Description: this function is invoked when suspend the driver and it > direcly > + * Description: this function is invoked when suspend the driver and it > directly > * call the main suspend function and then, if required, on some platform, it > * can call an exit helper. > */ > -- > 2.7.4 >
Re: [PATCH] net/netfilter: fix a typo for nf_conntrack_proto_dccp.c
Hi Wang, On Wed, Sep 09, 2020 at 08:12:44PM +0800, Wang Qing wrote: > Change the comment typo: "direcly" -> "directly". > > Signed-off-by: Wang Qing git log tells me that the correct prefix for this patch is "netfilter: conntrack:" rather than "net/netfilter:" Probably this patch is targeted at nf-next and should include nf-next in the subject like this: [PATCH nf-next] ... > --- > net/netfilter/nf_conntrack_proto_dccp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/netfilter/nf_conntrack_proto_dccp.c > b/net/netfilter/nf_conntrack_proto_dccp.c > index b3f4a33..d9bb0ce > --- a/net/netfilter/nf_conntrack_proto_dccp.c > +++ b/net/netfilter/nf_conntrack_proto_dccp.c > @@ -340,7 +340,7 @@ dccp_state_table[CT_DCCP_ROLE_MAX + 1][DCCP_PKT_SYNCACK + > 1][CT_DCCP_MAX + 1] = >* sNO -> sIV No connection >* sRQ -> sIV No connection >* sRS -> sIV No connection > - * sPO -> sOP -> sCGMove direcly to CLOSING > + * sPO -> sOP -> sCGMove directly to CLOSING >* sOP -> sCG Move to CLOSING >* sCR -> sIV Close after CloseReq is invalid >* sCG -> sCG Retransmit > -- > 2.7.4 >
Re: [PATCH net-next v2 3/3] ipmr: Use full VIF ID in netlink cache reports
On Tue, Sep 08, 2020 at 10:04:08AM +1200, Paul Davey wrote: > Insert the full 16 bit VIF ID into ipmr Netlink cache reports. > > The VIF_ID attribute has 32 bits of space so can store the full VIF ID > extracted from the high and low byte fields in the igmpmsg. > > Signed-off-by: Paul Davey > --- > net/ipv4/ipmr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c > index 4809318f591b..939792a38814 100644 > --- a/net/ipv4/ipmr.c > +++ b/net/ipv4/ipmr.c > @@ -2432,7 +2432,7 @@ static void igmpmsg_netlink_event(struct mr_table *mrt, > struct sk_buff *pkt) > rtgenm = nlmsg_data(nlh); > rtgenm->rtgen_family = RTNL_FAMILY_IPMR; > if (nla_put_u8(skb, IPMRA_CREPORT_MSGTYPE, msg->im_msgtype) || > - nla_put_u32(skb, IPMRA_CREPORT_VIF_ID, msg->im_vif) || > + nla_put_u32(skb, IPMRA_CREPORT_VIF_ID, msg->im_vif | > (msg->im_vif_hi << 8)) || nit: the inner parentheses seem unnecessary Otherwise, FWIIW, this series looks good to me. > nla_put_in_addr(skb, IPMRA_CREPORT_SRC_ADDR, > msg->im_src.s_addr) || > nla_put_in_addr(skb, IPMRA_CREPORT_DST_ADDR, > -- > 2.28.0 >
Re: [oss-drivers] [PATCH 16/20] ethernet: netronome: convert tasklets to use new tasklet_setup() API
On Mon, Aug 17, 2020 at 01:54:30PM +0530, Allen Pais wrote:
> In preparation for unconditionally passing the
> struct tasklet_struct pointer to all tasklet
> callbacks, switch to using the new tasklet_setup()
> and from_tasklet() to pass the tasklet pointer explicitly.
>
> Signed-off-by: Romain Perier
> Signed-off-by: Allen Pais
Reviewed-by: Simon Horman
But:
This series should be targeted at net-next, and thus have net-next in its
subject
[PATCH net-next x/y] ...
And it should be posted when net-next is open: it is currently closed.
http://vger.kernel.org/~davem/net-next.html
> ---
> drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> index 39ee23e8c0bf..1dcd24d899f5 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> @@ -2287,9 +2287,9 @@ static bool nfp_ctrl_rx(struct nfp_net_r_vector *r_vec)
> return budget;
> }
>
> -static void nfp_ctrl_poll(unsigned long arg)
> +static void nfp_ctrl_poll(struct tasklet_struct *t)
> {
> - struct nfp_net_r_vector *r_vec = (void *)arg;
> + struct nfp_net_r_vector *r_vec = from_tasklet(r_vec, t, tasklet);
>
> spin_lock(_vec->lock);
> nfp_net_tx_complete(r_vec->tx_ring, 0);
> @@ -2337,8 +2337,7 @@ static void nfp_net_vecs_init(struct nfp_net *nn)
>
> __skb_queue_head_init(_vec->queue);
> spin_lock_init(_vec->lock);
> - tasklet_init(_vec->tasklet, nfp_ctrl_poll,
> - (unsigned long)r_vec);
> + tasklet_setup(_vec->tasklet, nfp_ctrl_poll);
> tasklet_disable(_vec->tasklet);
> }
>
> --
> 2.17.1
>
Re: [Linux-kernel-mentees] [PATCH net-next v2] ipvs: Fix uninit-value in do_ip_vs_set_ctl()
On Tue, Aug 11, 2020 at 01:29:04PM +0300, Julian Anastasov wrote: > > Hello, > > On Tue, 11 Aug 2020, Peilin Ye wrote: > > > do_ip_vs_set_ctl() is referencing uninitialized stack value when `len` is > > zero. Fix it. > > > > Reported-by: syzbot+23b5f9e7caf61d9a3...@syzkaller.appspotmail.com > > Link: > > https://syzkaller.appspot.com/bug?id=46ebfb92a8a812621a001ef04d90dfa459520fe2 > > Suggested-by: Julian Anastasov > > Signed-off-by: Peilin Ye > > Looks good to me, thanks! > > Acked-by: Julian Anastasov Pablo, could you consider this for nf-next or should we repost when net-next re-opens? Reviewed-by: Simon Horman
Re: [PATCH] Replace HTTP links with HTTPS ones: kdump
On Sat, Jun 27, 2020 at 12:31:51PM +0200, Alexander A. Klimov wrote: > Rationale: > Reduces attack surface on kernel devs opening the links for MITM > as HTTPS traffic is much harder to manipulate. > > Deterministic algorithm: > For each file: > If not .svg: > For each line: > If doesn't contain `\bxmlns\b`: > For each link, `\bhttp://[^# \t\r\n]*(?:\w|/)`: > If both the HTTP and HTTPS versions > return 200 OK and serve the same content: > Replace HTTP with HTTPS. > > Signed-off-by: Alexander A. Klimov Reviewed-by: Simon Horman
Re: [v2,net-next] net: qos offload add flow status with dropped count
On Fri, Jun 19, 2020 at 02:01:07PM +0800, Po Liu wrote: > From: Po Liu > > This patch adds a drop frames counter to tc flower offloading. > Reporting h/w dropped frames is necessary for some actions. > Some actions like police action and the coming introduced stream gate > action would produce dropped frames which is necessary for user. Status > update shows how many filtered packets increasing and how many dropped > in those packets. > > v2: Changes > - Update commit comments suggest by Jiri Pirko. > > Signed-off-by: Po Liu > --- > This patch is continue the thread 20200324034745.30979-1-po@nxp.com > > drivers/net/dsa/sja1105/sja1105_vl.c | 2 +- > drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 2 +- > drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c | 2 +- > .../net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c| 2 +- > drivers/net/ethernet/freescale/enetc/enetc_qos.c | 7 +-- > drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c| 2 +- > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 4 ++-- > drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 2 +- > drivers/net/ethernet/mscc/ocelot_flower.c | 2 +- > drivers/net/ethernet/netronome/nfp/flower/offload.c | 2 +- > drivers/net/ethernet/netronome/nfp/flower/qos_conf.c | 2 +- > include/net/act_api.h | 11 ++- > include/net/flow_offload.h| 5 - > include/net/pkt_cls.h | 5 +++-- > net/sched/act_api.c | 10 -- > net/sched/act_ct.c| 6 +++--- > net/sched/act_gact.c | 7 --- > net/sched/act_gate.c | 6 +++--- > net/sched/act_mirred.c| 6 +++--- > net/sched/act_pedit.c | 6 +++--- > net/sched/act_police.c| 4 ++-- > net/sched/act_skbedit.c | 5 +++-- > net/sched/act_vlan.c | 6 +++--- > net/sched/cls_flower.c| 1 + > net/sched/cls_matchall.c | 3 ++- > 25 files changed, 60 insertions(+), 50 deletions(-) Netronome portion: Reviewed-by: Simon Horman
Re: [ovs-dev] [PATCH 1/1] openvswitch: fix infoleak in conntrack
On Mon, Jun 15, 2020 at 07:13:01PM -0700, Xidong Wang wrote:
> From: xidongwang
>
> The stack object “zone_limit” has 3 members. In function
> ovs_ct_limit_get_default_limit(), the member "count" is
> not initialized and sent out via “nla_put_nohdr”.
Hi Xidong,
thanks for your patch.
It appears that the patch is a fix. So I think that subject should be
targeted at the net tree and thus the subject should include
"[PATCH net]". (The other option being to target the net-next tree
in which case the subject should include "[PATCH net-next]".)
Also, as a fix it would be useful to include a fixes tag that references
the patch that introduced the problem. This is to facilitate backporting
to -stable branches of released kernels. In this case the following seems
appropriate.
Fixes: 11efd5cb04a1 ("openvswitch: Support conntrack zone limit")
> Signed-off-by: xidongwang
> ---
> net/openvswitch/conntrack.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 4340f25..1b7820a 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -2020,6 +2020,7 @@ static int ovs_ct_limit_get_default_limit(struct
> ovs_ct_limit_info *info,
> {
> struct ovs_zone_limit zone_limit;
> int err;
There should be a blank line here.
> + memset(_limit, 0, sizeof(zone_limit));
Moreover, initializing the entire structure to zero only to overwrite
most of its fields immediately below seems a bit inefficient.
Perhaps it would be better to just initialise count.
> zone_limit.zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE;
> zone_limit.limit = info->default_limit;
zone_limit.count = 0;
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [PATCH] iommu/renesas: fix unused-function warning
On Sat, May 09, 2020 at 12:02:16AM +0200, Arnd Bergmann wrote:
> gcc warns because the only reference to ipmmu_find_group
> is inside of an #ifdef:
>
> drivers/iommu/ipmmu-vmsa.c:878:28: error: 'ipmmu_find_group' defined but not
> used [-Werror=unused-function]
>
> Change the #ifdef to an equivalent IS_ENABLED().
>
> Fixes: 6580c8a78424 ("iommu/renesas: Convert to probe/release_device()
> call-backs")
> Signed-off-by: Arnd Bergmann
Reviewed-by: Simon Horman
> ---
> drivers/iommu/ipmmu-vmsa.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index fb7e702dee23..4c2972f3153b 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -903,11 +903,8 @@ static const struct iommu_ops ipmmu_ops = {
> .probe_device = ipmmu_probe_device,
> .release_device = ipmmu_release_device,
> .probe_finalize = ipmmu_probe_finalize,
> -#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
> - .device_group = generic_device_group,
> -#else
> - .device_group = ipmmu_find_group,
> -#endif
> + .device_group = IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_IOMMU_DMA)
> + ? generic_device_group : ipmmu_find_group,
> .pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
> .of_xlate = ipmmu_of_xlate,
> };
> --
> 2.26.0
>
Re: [PATCH 37/38] docs: networking: convert ipvs-sysctl.txt to ReST
On Tue, Apr 28, 2020 at 12:01:52AM +0200, Mauro Carvalho Chehab wrote: > - add SPDX header; > - add a document title; > - mark lists as such; > - mark code blocks and literals as such; > - adjust identation, whitespaces and blank lines; > - add to networking/index.rst. > > Signed-off-by: Mauro Carvalho Chehab Acked-by: Simon Horman
Re: [PATCH net-next 4/4] bonding: balance ICMP echoes in layer3+4 mode
On Wed, Oct 23, 2019 at 06:58:16PM +0200, Matteo Croce wrote: > On Wed, Oct 23, 2019 at 12:01 PM Simon Horman > wrote: > > > > On Mon, Oct 21, 2019 at 10:09:48PM +0200, Matteo Croce wrote: > > > The bonding uses the L4 ports to balance flows between slaves. > > > As the ICMP protocol has no ports, those packets are sent all to the > > > same device: > > > > > > # tcpdump -qltnni veth0 ip |sed 's/^/0: /' & > > > # tcpdump -qltnni veth1 ip |sed 's/^/1: /' & > > > # ping -qc1 192.168.0.2 > > > 1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 315, seq 1, > > > length 64 > > > 1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 315, seq 1, > > > length 64 > > > # ping -qc1 192.168.0.2 > > > 1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 316, seq 1, > > > length 64 > > > 1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 316, seq 1, > > > length 64 > > > # ping -qc1 192.168.0.2 > > > 1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 317, seq 1, > > > length 64 > > > 1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 317, seq 1, > > > length 64 > > > > > > But some ICMP packets have an Identifier field which is > > > used to match packets within sessions, let's use this value in the hash > > > function to balance these packets between bond slaves: > > > > > > # ping -qc1 192.168.0.2 > > > 0: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 303, seq 1, > > > length 64 > > > 0: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 303, seq 1, > > > length 64 > > > # ping -qc1 192.168.0.2 > > > 1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 304, seq 1, > > > length 64 > > > 1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 304, seq 1, > > > length 64 > > > > > > Signed-off-by: Matteo Croce > > > > I see where this patch is going but it is unclear to me what problem it is > > solving. I would expect ICMP traffic to be low volume and thus able to be > > handled by a single lower-device of a bond. > > > > ... > > Hi, > > The problem is not balancing the volume, even if it could increase due > to IoT devices pinging some well known DNS servers to check for > connection. > If a bonding slave is down, people using pings to check for > connectivity could fail to detect a broken link if all the packets are > sent to the alive link. > Anyway, although I didn't measure it, the computational overhead of > this changeset should be minimal, and only affect ICMP packets when > the ICMP dissector is used. So the idea is that by using different id values ping could be used to probe all lower-devices of a bond? If so then I understand why you want this and have no particular objection.
Re: [PATCH net-next 3/4] flow_dissector: extract more ICMP information
On Wed, Oct 23, 2019 at 12:53:37PM +0200, Matteo Croce wrote:
> On Wed, Oct 23, 2019 at 12:00 PM Simon Horman
> wrote:
> > On Mon, Oct 21, 2019 at 10:09:47PM +0200, Matteo Croce wrote:
> > > + switch (ih->type) {
> > > + case ICMP_ECHO:
> > > + case ICMP_ECHOREPLY:
> > > + case ICMP_TIMESTAMP:
> > > + case ICMP_TIMESTAMPREPLY:
> > > + case ICMPV6_ECHO_REQUEST:
> > > + case ICMPV6_ECHO_REPLY:
> > > + /* As we use 0 to signal that the Id field is not present,
> > > + * avoid confusion with packets without such field
> > > + */
> > > + key_icmp->id = ih->un.echo.id ? : 1;
> >
> > Its not obvious to me why the kernel should treat id-zero as a special
> > value if it is not special on the wire.
> >
> > Perhaps a caller who needs to know if the id is present can
> > check the ICMP type as this code does, say using a helper.
> >
>
> Hi,
>
> The problem is that the 0-0 Type-Code pair identifies the echo replies.
> So instead of adding a bool is_present value I hardcoded the info in
> the ID field making it always non null, at the expense of a possible
> collision, which is harmless.
Sorry, I feel that I'm missing something here.
My reading of the code above is that for the cased types above
(echo, echo reply, ...) the id is present. Otherwise it is not.
My idea would be to put a check for those types in a helper.
I do agree that the override you have used is harmless enough
in the context of the only user of the id which appears in
the following patch of this series.
Some other things I noticed in this patch on a second pass:
* I think you can remove the icmp field from struct flow_dissector_key_ports
* I think that adding icmp to struct flow_keys should be accompanied by
adding ICMP to flow_keys_dissector_symmetric_keys. But I think this is
not desirable outside of the bonding use-case and rather
the bonding driver should define its own structures that
includes the keys it needs - basically copies of struct flow_keys
and flow_keys_dissector_symmetric_keys with some modifications.
* Modifying flow_keys_have_l4 affects the behaviour of
skb_get_hash_flowi6() but there is not a corresponding update
to flow_keys_have_l4(). I didn't look at all the other call sites
but it strikes me that this is a) a wide-spread behavioural change
and b) is perhaps not required for the bond-use case.
Re: [RFC 1/2] vhost: IFC VF hardware operation layer
On Wed, Oct 23, 2019 at 06:36:13PM +0800, Jason Wang wrote:
>
> On 2019/10/23 下午6:13, Simon Horman wrote:
> > On Tue, Oct 22, 2019 at 09:32:36AM +0800, Jason Wang wrote:
> > > On 2019/10/22 上午12:31, Simon Horman wrote:
> > > > On Mon, Oct 21, 2019 at 05:55:33PM +0800, Zhu, Lingshan wrote:
> > > > > On 10/16/2019 5:53 PM, Simon Horman wrote:
> > > > > > Hi Zhu,
> > > > > >
> > > > > > thanks for your patch.
> > > > > >
> > > > > > On Wed, Oct 16, 2019 at 09:10:40AM +0800, Zhu Lingshan wrote:
> > > > ...
> > > >
> > > > > > > +static void ifcvf_read_dev_config(struct ifcvf_hw *hw, u64
> > > > > > > offset,
> > > > > > > +void *dst, int length)
> > > > > > > +{
> > > > > > > + int i;
> > > > > > > + u8 *p;
> > > > > > > + u8 old_gen, new_gen;
> > > > > > > +
> > > > > > > + do {
> > > > > > > + old_gen = ioread8(>common_cfg->config_generation);
> > > > > > > +
> > > > > > > + p = dst;
> > > > > > > + for (i = 0; i < length; i++)
> > > > > > > + *p++ = ioread8((u8 *)hw->dev_cfg + offset + i);
> > > > > > > +
> > > > > > > + new_gen = ioread8(>common_cfg->config_generation);
> > > > > > > + } while (old_gen != new_gen);
> > > > > > Would it be wise to limit the number of iterations of the loop
> > > > > > above?
> > > > > Thanks but I don't quite get it. This is used to make sure the
> > > > > function
> > > > > would get the latest config.
> > > > I am worried about the possibility that it will loop forever.
> > > > Could that happen?
> > > >
> > > > ...
> > > My understanding is that the function here is similar to virtio config
> > > generation [1]. So this can only happen for a buggy hardware.
> > Ok, so this circles back to my original question.
> > Should we put a bound on the number of times the loop runs
> > or should we accept that the kernel locks up if the HW is buggy?
> >
>
> I'm not sure, and similar logic has been used by virtio-pci drivers for
> years. Consider this logic is pretty simple and it should not be the only
> place that virito hardware can lock kernel, we can keep it as is.
Ok, I accept that there isn't much use fixing this if its idomatic and
there are other places virtio hardware can lock up the kernel.
> Actually, there's no need for hardware to implement generation logic, it
> could be emulated by software or even ignored. In new version of
> virtio-mdev, get_generation() is optional, when it was not implemented, 0 is
> simply returned by virtio-mdev transport.
>
> Thanks
>
Re: [RFC 1/2] vhost: IFC VF hardware operation layer
On Tue, Oct 22, 2019 at 09:32:36AM +0800, Jason Wang wrote:
>
> On 2019/10/22 上午12:31, Simon Horman wrote:
> > On Mon, Oct 21, 2019 at 05:55:33PM +0800, Zhu, Lingshan wrote:
> > > On 10/16/2019 5:53 PM, Simon Horman wrote:
> > > > Hi Zhu,
> > > >
> > > > thanks for your patch.
> > > >
> > > > On Wed, Oct 16, 2019 at 09:10:40AM +0800, Zhu Lingshan wrote:
> > ...
> >
> > > > > +static void ifcvf_read_dev_config(struct ifcvf_hw *hw, u64 offset,
> > > > > +void *dst, int length)
> > > > > +{
> > > > > + int i;
> > > > > + u8 *p;
> > > > > + u8 old_gen, new_gen;
> > > > > +
> > > > > + do {
> > > > > + old_gen = ioread8(>common_cfg->config_generation);
> > > > > +
> > > > > + p = dst;
> > > > > + for (i = 0; i < length; i++)
> > > > > + *p++ = ioread8((u8 *)hw->dev_cfg + offset + i);
> > > > > +
> > > > > + new_gen = ioread8(>common_cfg->config_generation);
> > > > > + } while (old_gen != new_gen);
> > > > Would it be wise to limit the number of iterations of the loop above?
> > > Thanks but I don't quite get it. This is used to make sure the function
> > > would get the latest config.
> > I am worried about the possibility that it will loop forever.
> > Could that happen?
> >
> > ...
>
>
> My understanding is that the function here is similar to virtio config
> generation [1]. So this can only happen for a buggy hardware.
Ok, so this circles back to my original question.
Should we put a bound on the number of times the loop runs
or should we accept that the kernel locks up if the HW is buggy?
>
> Thanks
>
> [1]
> https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html
> Section 2.4.1
>
>
> >
> > > > > +static void io_write64_twopart(u64 val, u32 *lo, u32 *hi)
> > > > > +{
> > > > > + iowrite32(val & ((1ULL << 32) - 1), lo);
> > > > > + iowrite32(val >> 32, hi);
> > > > > +}
> > > > I see this macro is also in virtio_pci_modern.c
> > > >
> > > > Assuming lo and hi aren't guaranteed to be sequential
> > > > and thus iowrite64_hi_lo() cannot be used perhaps
> > > > it would be good to add a common helper somewhere.
> > > Thanks, I will try after this IFC patchwork, I will cc you.
> > Thanks.
> >
> > ...
>
Re: [PATCH net-next 4/4] bonding: balance ICMP echoes in layer3+4 mode
On Mon, Oct 21, 2019 at 10:09:48PM +0200, Matteo Croce wrote: > The bonding uses the L4 ports to balance flows between slaves. > As the ICMP protocol has no ports, those packets are sent all to the > same device: > > # tcpdump -qltnni veth0 ip |sed 's/^/0: /' & > # tcpdump -qltnni veth1 ip |sed 's/^/1: /' & > # ping -qc1 192.168.0.2 > 1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 315, seq 1, length > 64 > 1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 315, seq 1, length 64 > # ping -qc1 192.168.0.2 > 1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 316, seq 1, length > 64 > 1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 316, seq 1, length 64 > # ping -qc1 192.168.0.2 > 1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 317, seq 1, length > 64 > 1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 317, seq 1, length 64 > > But some ICMP packets have an Identifier field which is > used to match packets within sessions, let's use this value in the hash > function to balance these packets between bond slaves: > > # ping -qc1 192.168.0.2 > 0: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 303, seq 1, length > 64 > 0: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 303, seq 1, length 64 > # ping -qc1 192.168.0.2 > 1: IP 192.168.0.1 > 192.168.0.2: ICMP echo request, id 304, seq 1, length > 64 > 1: IP 192.168.0.2 > 192.168.0.1: ICMP echo reply, id 304, seq 1, length 64 > > Signed-off-by: Matteo Croce I see where this patch is going but it is unclear to me what problem it is solving. I would expect ICMP traffic to be low volume and thus able to be handled by a single lower-device of a bond. ...
Re: [PATCH net-next 3/4] flow_dissector: extract more ICMP information
On Mon, Oct 21, 2019 at 10:09:47PM +0200, Matteo Croce wrote:
> The ICMP flow dissector currently parses only the Type and Code fields.
> Some ICMP packets (echo, timestamp) have a 16 bit Identifier field which
> is used to correlate packets.
> Add such field in flow_dissector_key_icmp and replace skb_flow_get_be16()
> with a more complex function which populate this field.
>
> Signed-off-by: Matteo Croce
> ---
> include/net/flow_dissector.h | 10 +-
> net/core/flow_dissector.c| 64 ++--
> 2 files changed, 49 insertions(+), 25 deletions(-)
>
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index 7747af3cc500..86c6bf5eab31 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -6,6 +6,8 @@
> #include
> #include
>
> +struct sk_buff;
> +
> /**
> * struct flow_dissector_key_control:
> * @thoff: Transport header offset
> @@ -160,6 +162,7 @@ struct flow_dissector_key_ports {
> * icmp: ICMP type (high) and code (low)
> * type: ICMP type
> * code: ICMP code
> + * id: session identifier
> */
> struct flow_dissector_key_icmp {
> union {
> @@ -169,6 +172,7 @@ struct flow_dissector_key_icmp {
> u8 code;
> };
> };
> + u16 id;
> };
>
> /**
> @@ -282,6 +286,7 @@ struct flow_keys {
> struct flow_dissector_key_vlan cvlan;
> struct flow_dissector_key_keyid keyid;
> struct flow_dissector_key_ports ports;
> + struct flow_dissector_key_icmp icmp;
> /* 'addrs' must be the last member */
> struct flow_dissector_key_addrs addrs;
> };
> @@ -312,10 +317,13 @@ void make_flow_keys_digest(struct flow_keys_digest
> *digest,
>
> static inline bool flow_keys_have_l4(const struct flow_keys *keys)
> {
> - return (keys->ports.ports || keys->tags.flow_label);
> + return keys->ports.ports || keys->tags.flow_label || keys->icmp.id;
> }
>
> u32 flow_hash_from_keys(struct flow_keys *keys);
> +void skb_flow_get_icmp_tci(const struct sk_buff *skb,
> +struct flow_dissector_key_icmp *key_icmp,
> +void *data, int thoff, int hlen);
>
> static inline bool dissector_uses_key(const struct flow_dissector
> *flow_dissector,
> enum flow_dissector_key_id key_id)
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 6443fac65ce8..90dcf6f2ef19 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -147,27 +147,6 @@ int skb_flow_dissector_bpf_prog_detach(const union
> bpf_attr *attr)
> mutex_unlock(_dissector_mutex);
> return 0;
> }
> -/**
> - * skb_flow_get_be16 - extract be16 entity
> - * @skb: sk_buff to extract from
> - * @poff: offset to extract at
> - * @data: raw buffer pointer to the packet
> - * @hlen: packet header length
> - *
> - * The function will try to retrieve a be32 entity at
> - * offset poff
> - */
> -static __be16 skb_flow_get_be16(const struct sk_buff *skb, int poff,
> - void *data, int hlen)
> -{
> - __be16 *u, _u;
> -
> - u = __skb_header_pointer(skb, poff, sizeof(_u), data, hlen, &_u);
> - if (u)
> - return *u;
> -
> - return 0;
> -}
>
> /**
> * __skb_flow_get_ports - extract the upper layer ports and return them
> @@ -203,8 +182,44 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb,
> int thoff, u8 ip_proto,
> }
> EXPORT_SYMBOL(__skb_flow_get_ports);
>
> -/* If FLOW_DISSECTOR_KEY_ICMP is set, get the Type and Code from an ICMP
> packet
> - * using skb_flow_get_be16().
> +/**
> + * skb_flow_get_icmp_tci - extract ICMP(6) Type, Code and Identifier fields
> + * @skb: sk_buff to extract from
> + * @key_icmp: struct flow_dissector_key_icmp to fill
> + * @data: raw buffer pointer to the packet
> + * @toff: offset to extract at
> + * @hlen: packet header length
> + */
> +void skb_flow_get_icmp_tci(const struct sk_buff *skb,
> +struct flow_dissector_key_icmp *key_icmp,
> +void *data, int thoff, int hlen)
> +{
> + struct icmphdr *ih, _ih;
> +
> + ih = __skb_header_pointer(skb, thoff, sizeof(_ih), data, hlen, &_ih);
> + if (!ih)
> + return;
> +
> + key_icmp->type = ih->type;
> + key_icmp->code = ih->code;
> + key_icmp->id = 0;
> + switch (ih->type) {
> + case ICMP_ECHO:
> + case ICMP_ECHOREPLY:
> + case ICMP_TIMESTAMP:
> + case ICMP_TIMESTAMPREPLY:
> + case ICMPV6_ECHO_REQUEST:
> + case ICMPV6_ECHO_REPLY:
> + /* As we use 0 to signal that the Id field is not present,
> + * avoid confusion with packets without such field
> + */
> + key_icmp->id = ih->un.echo.id ? : 1;
Its not obvious to me why the kernel should treat id-zero as a special
value if it is not special on the wire.
Perhaps a caller who
Re: [PATCH net-next 2/4] flow_dissector: skip the ICMP dissector for non ICMP packets
On Mon, Oct 21, 2019 at 10:09:46PM +0200, Matteo Croce wrote:
> FLOW_DISSECTOR_KEY_ICMP is checked for every packet, not only ICMP ones.
> Even if the test overhead is probably negligible, move the
> ICMP dissector code under the big 'switch(ip_proto)' so it gets called
> only for ICMP packets.
>
> Signed-off-by: Matteo Croce
Reviewed-by: Simon Horman
> ---
> net/core/flow_dissector.c | 34 +-
> 1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index affde70dad47..6443fac65ce8 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -203,6 +203,25 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb,
> int thoff, u8 ip_proto,
> }
> EXPORT_SYMBOL(__skb_flow_get_ports);
>
> +/* If FLOW_DISSECTOR_KEY_ICMP is set, get the Type and Code from an ICMP
> packet
> + * using skb_flow_get_be16().
> + */
> +static void __skb_flow_dissect_icmp(const struct sk_buff *skb,
> + struct flow_dissector *flow_dissector,
> + void *target_container,
> + void *data, int thoff, int hlen)
> +{
> + struct flow_dissector_key_icmp *key_icmp;
> +
> + if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ICMP))
> + return;
> +
> + key_icmp = skb_flow_dissector_target(flow_dissector,
> + FLOW_DISSECTOR_KEY_ICMP,
> + target_container);
> + key_icmp->icmp = skb_flow_get_be16(skb, thoff, data, hlen);
> +}
> +
> void skb_flow_dissect_meta(const struct sk_buff *skb,
> struct flow_dissector *flow_dissector,
> void *target_container)
> @@ -853,7 +872,6 @@ bool __skb_flow_dissect(const struct net *net,
> struct flow_dissector_key_basic *key_basic;
> struct flow_dissector_key_addrs *key_addrs;
> struct flow_dissector_key_ports *key_ports;
> - struct flow_dissector_key_icmp *key_icmp;
> struct flow_dissector_key_tags *key_tags;
> struct flow_dissector_key_vlan *key_vlan;
> struct bpf_prog *attached = NULL;
> @@ -1295,6 +1313,12 @@ bool __skb_flow_dissect(const struct net *net,
> data, nhoff, hlen);
> break;
>
> + case IPPROTO_ICMP:
> + case IPPROTO_ICMPV6:
> + __skb_flow_dissect_icmp(skb, flow_dissector, target_container,
> + data, nhoff, hlen);
> + break;
> +
> default:
> break;
> }
> @@ -1308,14 +1332,6 @@ bool __skb_flow_dissect(const struct net *net,
> data, hlen);
> }
>
> - if (dissector_uses_key(flow_dissector,
> -FLOW_DISSECTOR_KEY_ICMP)) {
> - key_icmp = skb_flow_dissector_target(flow_dissector,
> - FLOW_DISSECTOR_KEY_ICMP,
> - target_container);
> - key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data, hlen);
> - }
> -
> /* Process result of IP proto processing */
> switch (fdret) {
> case FLOW_DISSECT_RET_PROTO_AGAIN:
> --
> 2.21.0
>
Re: [PATCH net-next 1/4] flow_dissector: add meaningful comments
On Mon, Oct 21, 2019 at 10:09:45PM +0200, Matteo Croce wrote:
> Documents two piece of code which can't be understood at a glance.
>
> Signed-off-by: Matteo Croce
Reviewed-by: Simon Horman
> ---
> include/net/flow_dissector.h | 1 +
> net/core/flow_dissector.c| 6 ++
> 2 files changed, 7 insertions(+)
>
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index 90bd210be060..7747af3cc500 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -282,6 +282,7 @@ struct flow_keys {
> struct flow_dissector_key_vlan cvlan;
> struct flow_dissector_key_keyid keyid;
> struct flow_dissector_key_ports ports;
> + /* 'addrs' must be the last member */
> struct flow_dissector_key_addrs addrs;
> };
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 7c09d87d3269..affde70dad47 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -1374,6 +1374,9 @@ static inline size_t flow_keys_hash_length(const struct
> flow_keys *flow)
> {
> size_t diff = FLOW_KEYS_HASH_OFFSET + sizeof(flow->addrs);
> BUILD_BUG_ON((sizeof(*flow) - FLOW_KEYS_HASH_OFFSET) % sizeof(u32));
> + /* flow.addrs MUST be the last member in struct flow_keys because
> + * different L3 protocols have different address length
> + */
> BUILD_BUG_ON(offsetof(typeof(*flow), addrs) !=
>sizeof(*flow) - sizeof(flow->addrs));
>
> @@ -1421,6 +1424,9 @@ __be32 flow_get_u32_dst(const struct flow_keys *flow)
> }
> EXPORT_SYMBOL(flow_get_u32_dst);
>
> +/* Sort the source and destination IP (and the ports if the IP are the same),
> + * to have consistent hash within the two directions
> + */
> static inline void __flow_hash_consistentify(struct flow_keys *keys)
> {
> int addr_diff, i;
> --
> 2.21.0
>
Re: [RFC 1/2] vhost: IFC VF hardware operation layer
On Mon, Oct 21, 2019 at 05:55:33PM +0800, Zhu, Lingshan wrote:
>
> On 10/16/2019 5:53 PM, Simon Horman wrote:
> > Hi Zhu,
> >
> > thanks for your patch.
> >
> > On Wed, Oct 16, 2019 at 09:10:40AM +0800, Zhu Lingshan wrote:
...
> > > +static void ifcvf_read_dev_config(struct ifcvf_hw *hw, u64 offset,
> > > +void *dst, int length)
> > > +{
> > > + int i;
> > > + u8 *p;
> > > + u8 old_gen, new_gen;
> > > +
> > > + do {
> > > + old_gen = ioread8(>common_cfg->config_generation);
> > > +
> > > + p = dst;
> > > + for (i = 0; i < length; i++)
> > > + *p++ = ioread8((u8 *)hw->dev_cfg + offset + i);
> > > +
> > > + new_gen = ioread8(>common_cfg->config_generation);
> > > + } while (old_gen != new_gen);
> > Would it be wise to limit the number of iterations of the loop above?
> Thanks but I don't quite get it. This is used to make sure the function
> would get the latest config.
I am worried about the possibility that it will loop forever.
Could that happen?
...
> > > +static void io_write64_twopart(u64 val, u32 *lo, u32 *hi)
> > > +{
> > > + iowrite32(val & ((1ULL << 32) - 1), lo);
> > > + iowrite32(val >> 32, hi);
> > > +}
> > I see this macro is also in virtio_pci_modern.c
> >
> > Assuming lo and hi aren't guaranteed to be sequential
> > and thus iowrite64_hi_lo() cannot be used perhaps
> > it would be good to add a common helper somewhere.
> Thanks, I will try after this IFC patchwork, I will cc you.
Thanks.
...
Re: [PATCH] RFC: Bluetooth: missed cpu_to_le16 conversion in hci_init4_req
On Wed, Oct 16, 2019 at 08:42:48PM +0200, Marcel Holtmann wrote:
> Hi Simon,
>
> >> It looks like in hci_init4_req() the request is being
> >> initialised from cpu-endian data but the packet is specified
> >> to be little-endian. This causes an warning from sparse due
> >> to __le16 to u16 conversion.
> >>
> >> Fix this by using cpu_to_le16() on the two fields in the packet.
> >>
> >> net/bluetooth/hci_core.c:845:27: warning: incorrect type in assignment
> >> (different base types)
> >> net/bluetooth/hci_core.c:845:27:expected restricted __le16 [usertype]
> >> tx_len
> >> net/bluetooth/hci_core.c:845:27:got unsigned short [usertype]
> >> le_max_tx_len
> >> net/bluetooth/hci_core.c:846:28: warning: incorrect type in assignment
> >> (different base types)
> >> net/bluetooth/hci_core.c:846:28:expected restricted __le16 [usertype]
> >> tx_time
> >> net/bluetooth/hci_core.c:846:28:got unsigned short [usertype]
> >> le_max_tx_time
> >>
> >> Signed-off-by: Ben Dooks
> >> ---
> >> Cc: Marcel Holtmann
> >> Cc: Johan Hedberg
> >> Cc: "David S. Miller"
> >> Cc: linux-blueto...@vger.kernel.org
> >> Cc: net...@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org
> >> ---
> >> net/bluetooth/hci_core.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> >> index 04bc79359a17..b2559d4bed81 100644
> >> --- a/net/bluetooth/hci_core.c
> >> +++ b/net/bluetooth/hci_core.c
> >> @@ -842,8 +842,8 @@ static int hci_init4_req(struct hci_request *req,
> >> unsigned long opt)
> >>if (hdev->le_features[0] & HCI_LE_DATA_LEN_EXT) {
> >>struct hci_cp_le_write_def_data_len cp;
> >>
> >> - cp.tx_len = hdev->le_max_tx_len;
> >> - cp.tx_time = hdev->le_max_tx_time;
> >> + cp.tx_len = cpu_to_le16(hdev->le_max_tx_len);
> >> + cp.tx_time = cpu_to_le16(hdev->le_max_tx_time);
> >
> > I would suggest that the naming of the le_ fields of struct hci_dev
> > implies that the values stored in those fields should be little endian
> > (but those that are more than bone byte wide are not).
>
> the le_ stands for Low Energy and not for Little Endian.
Thanks, in that case I have no objections.
Re: [PATCH] usb: renesas_usbhs: fix __le16 warnings
On Thu, Oct 17, 2019 at 08:57:26AM +0200, Geert Uytterhoeven wrote:
> Hi Shimoda-san, Simon,
>
> On Thu, Oct 17, 2019 at 4:18 AM Yoshihiro Shimoda
> wrote:
> > > From: Simon Horman, Sent: Wednesday, October 16, 2019 9:27 PM
> >
> > > > diff --git a/drivers/usb/renesas_usbhs/common.c
> > > > b/drivers/usb/renesas_usbhs/common.c
> > > > index 4c3de777ef6c..a3c30b609433 100644
> > > > --- a/drivers/usb/renesas_usbhs/common.c
> > > > +++ b/drivers/usb/renesas_usbhs/common.c
> > > > @@ -162,17 +162,17 @@ void usbhs_usbreq_get_val(struct usbhs_priv
> > > > *priv, struct usb_ctrlrequest *req)
> > > > req->bRequest = (val >> 8) & 0xFF;
> > > > req->bRequestType = (val >> 0) & 0xFF;
> > > >
> > > > - req->wValue = usbhs_read(priv, USBVAL);
> > > > - req->wIndex = usbhs_read(priv, USBINDX);
> > > > - req->wLength= usbhs_read(priv, USBLENG);
> > > > + req->wValue = cpu_to_le16(usbhs_read(priv, USBVAL));
> > > > + req->wIndex = cpu_to_le16(usbhs_read(priv, USBINDX));
> > > > + req->wLength= cpu_to_le16(usbhs_read(priv, USBLENG));
> > >
> > > usbhs_read is backed by readl which performs
> > > a le->cpu conversion. Rather than have a double conversion
> > > perhaps it would be nicer to introduce usbhs_read_le.
> > > Likewise for write.
> >
> > I'm afraid but, I could not understand these comments.
> > At the moment, the usbhs_{read,write}() call io{read,write}16(),
> > not {read,write}l().
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/renesas_usbhs/common.c?h=v5.4-rc3#n62
>
> ioread16() and readw() don't do byteswapping on ARM, as ARM is
> little-endian. Likewise, cpu_to_le16() is a no-op on ARM.
>
> Double swapping would matter only on a big-endian platform, and could
> indeed be avoided by introducing usbhs_read_le*() functions that are
> just wrappers around __raw_read16() on big-endian.
> However, until the Renesas USBHS IP core ends up on a big-endian
> platform, it's not worth doing that, IMHO.
Yes, that is all true.
I'm fine with this patch as it is.
>
> > > > }
> > > >
> > > > void usbhs_usbreq_set_val(struct usbhs_priv *priv, struct
> > > > usb_ctrlrequest *req)
> > > > {
> > > > usbhs_write(priv, USBREQ, (req->bRequest << 8) |
> > > > req->bRequestType);
> > > > - usbhs_write(priv, USBVAL, req->wValue);
> > > > - usbhs_write(priv, USBINDX, req->wIndex);
> > > > - usbhs_write(priv, USBLENG, req->wLength);
> > > > + usbhs_write(priv, USBVAL, le16_to_cpu(req->wValue));
> > > > + usbhs_write(priv, USBINDX, le16_to_cpu(req->wIndex));
> > > > + usbhs_write(priv, USBLENG, le16_to_cpu(req->wLength));
> > > >
> > > > usbhs_bset(priv, DCPCTR, SUREQ, SUREQ);
> > > > }
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
>
Re: [PATCH] usb: renesas_usbhs: fix __le16 warnings
On Tue, Oct 15, 2019 at 04:50:44PM +0100, Ben Dooks (Codethink) wrote:
> Fix the warnings generated by casting to/from __le16 without
> using the correct functions.
>
> Fixes the following sparse warnings:
>
> drivers/usb/renesas_usbhs/common.c:165:25: warning: incorrect type in
> assignment (different base types)
> drivers/usb/renesas_usbhs/common.c:165:25:expected restricted __le16
> [usertype] wValue
> drivers/usb/renesas_usbhs/common.c:165:25:got unsigned short
> drivers/usb/renesas_usbhs/common.c:166:25: warning: incorrect type in
> assignment (different base types)
> drivers/usb/renesas_usbhs/common.c:166:25:expected restricted __le16
> [usertype] wIndex
> drivers/usb/renesas_usbhs/common.c:166:25:got unsigned short
> drivers/usb/renesas_usbhs/common.c:167:25: warning: incorrect type in
> assignment (different base types)
> drivers/usb/renesas_usbhs/common.c:167:25:expected restricted __le16
> [usertype] wLength
> drivers/usb/renesas_usbhs/common.c:167:25:got unsigned short
> drivers/usb/renesas_usbhs/common.c:173:39: warning: incorrect type in
> argument 3 (different base types)
> drivers/usb/renesas_usbhs/common.c:173:39:expected unsigned short
> [usertype] data
> drivers/usb/renesas_usbhs/common.c:173:39:got restricted __le16
> [usertype] wValue
> drivers/usb/renesas_usbhs/common.c:174:39: warning: incorrect type in
> argument 3 (different base types)
> drivers/usb/renesas_usbhs/common.c:174:39:expected unsigned short
> [usertype] data
> drivers/usb/renesas_usbhs/common.c:174:39:got restricted __le16
> [usertype] wIndex
> drivers/usb/renesas_usbhs/common.c:175:39: warning: incorrect type in
> argument 3 (different base types)
> drivers/usb/renesas_usbhs/common.c:175:39:expected unsigned short
> [usertype] data
>
> Note. I belive this to be correct, and should be a no-op on arm.
>
> Signed-off-by: Ben Dooks
> ---
> Cc: Greg Kroah-Hartman
> Cc: Yoshihiro Shimoda
> Cc: Simon Horman
> Cc: Geert Uytterhoeven
> Cc: linux-...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/usb/renesas_usbhs/common.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/renesas_usbhs/common.c
> b/drivers/usb/renesas_usbhs/common.c
> index 4c3de777ef6c..a3c30b609433 100644
> --- a/drivers/usb/renesas_usbhs/common.c
> +++ b/drivers/usb/renesas_usbhs/common.c
> @@ -162,17 +162,17 @@ void usbhs_usbreq_get_val(struct usbhs_priv *priv,
> struct usb_ctrlrequest *req)
> req->bRequest = (val >> 8) & 0xFF;
> req->bRequestType = (val >> 0) & 0xFF;
>
> - req->wValue = usbhs_read(priv, USBVAL);
> - req->wIndex = usbhs_read(priv, USBINDX);
> - req->wLength= usbhs_read(priv, USBLENG);
> + req->wValue = cpu_to_le16(usbhs_read(priv, USBVAL));
> + req->wIndex = cpu_to_le16(usbhs_read(priv, USBINDX));
> + req->wLength= cpu_to_le16(usbhs_read(priv, USBLENG));
usbhs_read is backed by readl which performs
a le->cpu conversion. Rather than have a double conversion
perhaps it would be nicer to introduce usbhs_read_le.
Likewise for write.
> }
>
> void usbhs_usbreq_set_val(struct usbhs_priv *priv, struct usb_ctrlrequest
> *req)
> {
> usbhs_write(priv, USBREQ, (req->bRequest << 8) | req->bRequestType);
> - usbhs_write(priv, USBVAL, req->wValue);
> - usbhs_write(priv, USBINDX, req->wIndex);
> - usbhs_write(priv, USBLENG, req->wLength);
> + usbhs_write(priv, USBVAL, le16_to_cpu(req->wValue));
> + usbhs_write(priv, USBINDX, le16_to_cpu(req->wIndex));
> + usbhs_write(priv, USBLENG, le16_to_cpu(req->wLength));
>
> usbhs_bset(priv, DCPCTR, SUREQ, SUREQ);
> }
> --
> 2.23.0
>
Re: [PATCH] RFC: Bluetooth: missed cpu_to_le16 conversion in hci_init4_req
On Wed, Oct 16, 2019 at 12:39:43PM +0100, Ben Dooks (Codethink) wrote:
> It looks like in hci_init4_req() the request is being
> initialised from cpu-endian data but the packet is specified
> to be little-endian. This causes an warning from sparse due
> to __le16 to u16 conversion.
>
> Fix this by using cpu_to_le16() on the two fields in the packet.
>
> net/bluetooth/hci_core.c:845:27: warning: incorrect type in assignment
> (different base types)
> net/bluetooth/hci_core.c:845:27:expected restricted __le16 [usertype]
> tx_len
> net/bluetooth/hci_core.c:845:27:got unsigned short [usertype]
> le_max_tx_len
> net/bluetooth/hci_core.c:846:28: warning: incorrect type in assignment
> (different base types)
> net/bluetooth/hci_core.c:846:28:expected restricted __le16 [usertype]
> tx_time
> net/bluetooth/hci_core.c:846:28:got unsigned short [usertype]
> le_max_tx_time
>
> Signed-off-by: Ben Dooks
> ---
> Cc: Marcel Holtmann
> Cc: Johan Hedberg
> Cc: "David S. Miller"
> Cc: linux-blueto...@vger.kernel.org
> Cc: net...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> net/bluetooth/hci_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 04bc79359a17..b2559d4bed81 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -842,8 +842,8 @@ static int hci_init4_req(struct hci_request *req,
> unsigned long opt)
> if (hdev->le_features[0] & HCI_LE_DATA_LEN_EXT) {
> struct hci_cp_le_write_def_data_len cp;
>
> - cp.tx_len = hdev->le_max_tx_len;
> - cp.tx_time = hdev->le_max_tx_time;
> + cp.tx_len = cpu_to_le16(hdev->le_max_tx_len);
> + cp.tx_time = cpu_to_le16(hdev->le_max_tx_time);
I would suggest that the naming of the le_ fields of struct hci_dev
implies that the values stored in those fields should be little endian
(but those that are more than bone byte wide are not).
In any case, the question arises as to if this has ever worked on big
endian machines.
> hci_req_add(req, HCI_OP_LE_WRITE_DEF_DATA_LEN, sizeof(cp), );
> }
>
> --
> 2.23.0
>
[PATCH] MAINTAINERS: Add Marek and Shimoda-san as R-Car PCIE co-maintainers
At the end of the v5.3 upstream development cycle I stepped down from my role at Renesas. Pass maintainership of the R-Car PCIE to Marek and Shimoda-san. Signed-off-by: Simon Horman --- MAINTAINERS | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 783569e3c4b4..b61ade7afd64 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12337,7 +12337,8 @@ F: Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt F: drivers/pci/controller/pci-tegra.c PCI DRIVER FOR RENESAS R-CAR -M: Simon Horman +M: Marek Vasut +M: Yoshihiro Shimoda L: linux-...@vger.kernel.org L: linux-renesas-...@vger.kernel.org S: Maintained -- 2.11.0
Re: [RFC 2/2] vhost: IFC VF vdpa layer
Hi Zhu,
thanks for your patch.
On Wed, Oct 16, 2019 at 09:03:18AM +0800, Zhu Lingshan wrote:
> This commit introduced IFC VF operations for vdpa, which complys to
> vhost_mdev interfaces, handles IFC VF initialization,
> configuration and removal.
>
> Signed-off-by: Zhu Lingshan
> ---
> drivers/vhost/ifcvf/ifcvf_main.c | 541
> +++
> 1 file changed, 541 insertions(+)
> create mode 100644 drivers/vhost/ifcvf/ifcvf_main.c
>
> diff --git a/drivers/vhost/ifcvf/ifcvf_main.c
> b/drivers/vhost/ifcvf/ifcvf_main.c
> new file mode 100644
> index ..c48a29969a85
> --- /dev/null
> +++ b/drivers/vhost/ifcvf/ifcvf_main.c
> @@ -0,0 +1,541 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2019 Intel Corporation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "ifcvf_base.h"
> +
> +#define VERSION_STRING "0.1"
> +#define DRIVER_AUTHOR"Intel Corporation"
> +#define IFCVF_DRIVER_NAME"ifcvf"
> +
> +static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
> +{
> + struct vring_info *vring = arg;
> +
> + if (vring->cb.callback)
> + return vring->cb.callback(vring->cb.private);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static u64 ifcvf_mdev_get_features(struct mdev_device *mdev)
> +{
> + return IFC_SUPPORTED_FEATURES;
> +}
> +
> +static int ifcvf_mdev_set_features(struct mdev_device *mdev, u64 features)
> +{
> + struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> + struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
Perhaps a helper that takes a struct mdev_device would be useful.
The pattern above seems to be repeated many times.
> +
> + vf->req_features = features;
> +
> + return 0;
> +}
> +
> +static u64 ifcvf_mdev_get_vq_state(struct mdev_device *mdev, u16 qid)
> +{
> + struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> + struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> + return vf->vring[qid].last_avail_idx;
> +}
> +
> +static int ifcvf_mdev_set_vq_state(struct mdev_device *mdev, u16 qid, u64
> num)
> +{
> + struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> + struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> + vf->vring[qid].last_used_idx = num;
> + vf->vring[qid].last_avail_idx = num;
> +
> + return 0;
> +}
> +
> +static int ifcvf_mdev_set_vq_address(struct mdev_device *mdev, u16 idx,
> + u64 desc_area, u64 driver_area,
> + u64 device_area)
> +{
> + struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> + struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> + vf->vring[idx].desc = desc_area;
> + vf->vring[idx].avail = driver_area;
> + vf->vring[idx].used = device_area;
> +
> + return 0;
> +}
> +
> +static void ifcvf_mdev_set_vq_num(struct mdev_device *mdev, u16 qid, u32 num)
> +{
> + struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> + struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> + vf->vring[qid].size = num;
> +}
> +
> +static void ifcvf_mdev_set_vq_ready(struct mdev_device *mdev,
> + u16 qid, bool ready)
u16 should be vertically whitespace aligned with struct mdev_device.
> +{
> +
> + struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> + struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> + vf->vring[qid].ready = ready;
> +}
> +
> +static bool ifcvf_mdev_get_vq_ready(struct mdev_device *mdev, u16 qid)
> +{
> +
> + struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> + struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> + return vf->vring[qid].ready;
> +}
> +
> +static void ifcvf_mdev_set_vq_cb(struct mdev_device *mdev, u16 idx,
> + struct virtio_mdev_callback *cb)
> +{
> + struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> + struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> + vf->vring[idx].cb = *cb;
> +}
> +
> +static void ifcvf_mdev_kick_vq(struct mdev_device *mdev, u16 idx)
> +{
> + struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> + struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> + ifcvf_notify_queue(vf, idx);
> +}
> +
> +static u8 ifcvf_mdev_get_status(struct mdev_device *mdev)
> +{
> + struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> + struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> + return vf->status;
> +}
> +
> +static u32 ifcvf_mdev_get_generation(struct mdev_device *mdev)
> +{
> + struct ifcvf_adapter *adapter = mdev_get_drvdata(mdev);
> + struct ifcvf_hw *vf = IFC_PRIVATE_TO_VF(adapter);
> +
> + return vf->generation;
> +}
> +
> +static int ifcvf_mdev_get_version(struct mdev_device *mdev)
> +{
> + return VIRTIO_MDEV_VERSION;
> +}
> +
> +static u32 ifcvf_mdev_get_device_id(struct mdev_device *mdev)
> +{
> + return IFCVF_DEVICE_ID;
> +}
> +
Re: [RFC 1/2] vhost: IFC VF hardware operation layer
Hi Zhu,
thanks for your patch.
On Wed, Oct 16, 2019 at 09:10:40AM +0800, Zhu Lingshan wrote:
> This commit introduced ifcvf_base layer, which handles IFC VF NIC
> hardware operations and configurations.
>
> Signed-off-by: Zhu Lingshan
> ---
> drivers/vhost/ifcvf/ifcvf_base.c | 390
> +++
> drivers/vhost/ifcvf/ifcvf_base.h | 137 ++
> 2 files changed, 527 insertions(+)
> create mode 100644 drivers/vhost/ifcvf/ifcvf_base.c
> create mode 100644 drivers/vhost/ifcvf/ifcvf_base.h
>
> diff --git a/drivers/vhost/ifcvf/ifcvf_base.c
> b/drivers/vhost/ifcvf/ifcvf_base.c
> new file mode 100644
> index ..b85e14c9bdcf
> --- /dev/null
> +++ b/drivers/vhost/ifcvf/ifcvf_base.c
> @@ -0,0 +1,390 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2019 Intel Corporation.
> + */
> +
> +#include "ifcvf_base.h"
> +
> +static void *get_cap_addr(struct ifcvf_hw *hw, struct virtio_pci_cap *cap)
> +{
> + u8 bar = cap->bar;
> + u32 length = cap->length;
> + u32 offset = cap->offset;
The type of the length and offset fields of virtio_pci_cap is __le32.
A conversion, such as le32_to_cpu(), is required in order to access
these fields in host byte order.
> + struct ifcvf_adapter *ifcvf =
> + container_of(hw, struct ifcvf_adapter, vf);
As mentioned elsewhere, please use reverse Christmas tree to order local
variables throughout the patch.
Please consider declaring and assigning ifcvf on separate lines
if combining these stretches over more than one line.
> +
> + if (bar >= IFCVF_PCI_MAX_RESOURCE) {
> + IFC_ERR(ifcvf->dev,
> + "Invalid bar number %u to get capabilities.\n", bar);
> + return NULL;
> + }
> +
> + if (offset + length < offset) {
> + IFC_ERR(ifcvf->dev, "offset(%u) + length(%u) overflows\n",
> + offset, length);
> + return NULL;
> + }
> +
> + if (offset + length > hw->mem_resource[cap->bar].len) {
> + IFC_ERR(ifcvf->dev,
> + "offset(%u) + len(%u) overflows bar%u to get
> capabilities.\n",
> + offset, length, bar);
> + return NULL;
> + }
> +
> + return hw->mem_resource[bar].addr + offset;
> +}
> +
> +int ifcvf_read_config_range(struct pci_dev *dev,
> + uint32_t *val, int size, int where)
> +{
> + int i;
> +
> + for (i = 0; i < size; i += 4) {
> + if (pci_read_config_dword(dev, where + i, val + i / 4) < 0)
> + return -1;
> + }
I don't think the {} is needed here.
An error code, such as -EINVAL, should be returned rather than -1.
Likewise elsewhere.
> + return 0;
> +}
> +
> +int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *dev)
> +{
> + int ret;
> + u8 pos;
> + struct virtio_pci_cap cap;
> + u32 i;
> + u16 notify_off;
> +
> + ret = pci_read_config_byte(dev, PCI_CAPABILITY_LIST, );
> +
> + if (ret < 0) {
> + IFC_ERR(>dev, "failed to read PCI capability list.\n");
> + return -EIO;
> + }
> +
> + while (pos) {
> + ret = ifcvf_read_config_range(dev, (u32 *),
> + sizeof(cap), pos);
sizeof should be white-space aligned vertically with dev.
Likewise elsewhere.
> +
> + if (ret < 0) {
> + IFC_ERR(>dev, "failed to get PCI capability at %x",
> + pos);
> + break;
> + }
> +
> + if (cap.cap_vndr != PCI_CAP_ID_VNDR)
> + goto next;
> +
> + IFC_INFO(>dev, "read PCI config:\n"
> + "config type: %u.\n"
> + "PCI bar: %u.\n"
> + "PCI bar offset: %u.\n"
> + "PCI config len: %u.\n",
> + cap.cfg_type, cap.bar,
> + cap.offset, cap.length);
> +
> + switch (cap.cfg_type) {
> + case VIRTIO_PCI_CAP_COMMON_CFG:
> + hw->common_cfg = get_cap_addr(hw, );
> + IFC_INFO(>dev, "hw->common_cfg = %p.\n",
> + hw->common_cfg);
> + break;
> + case VIRTIO_PCI_CAP_NOTIFY_CFG:
> + pci_read_config_dword(dev, pos + sizeof(cap),
> + >notify_off_multiplier);
> + hw->notify_bar = cap.bar;
> + hw->notify_base = get_cap_addr(hw, );
> + IFC_INFO(>dev, "hw->notify_base = %p.\n",
> + hw->notify_base);
> + break;
> + case VIRTIO_PCI_CAP_ISR_CFG:
> + hw->isr = get_cap_addr(hw, );
> + IFC_INFO(>dev, "hw->isr = %p.\n",
Re: [PATCH net-next] can: ifi: use devm_platform_ioremap_resource() to simplify code
On Tue, Oct 15, 2019 at 10:20:46PM +0800, YueHaibing wrote: > Use devm_platform_ioremap_resource() to simplify the code a bit. > This is detected by coccinelle. > > Signed-off-by: YueHaibing Reviewed-by: Simon Horman > --- > drivers/net/can/ifi_canfd/ifi_canfd.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c > b/drivers/net/can/ifi_canfd/ifi_canfd.c > index fedd927..04d59be 100644 > --- a/drivers/net/can/ifi_canfd/ifi_canfd.c > +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c > @@ -942,13 +942,11 @@ static int ifi_canfd_plat_probe(struct platform_device > *pdev) > struct device *dev = >dev; > struct net_device *ndev; > struct ifi_canfd_priv *priv; > - struct resource *res; > void __iomem *addr; > int irq, ret; > u32 id, rev; > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - addr = devm_ioremap_resource(dev, res); > + addr = devm_platform_ioremap_resource(pdev, 0); > irq = platform_get_irq(pdev, 0); > if (IS_ERR(addr) || irq < 0) > return -EINVAL; > -- > 2.7.4 > >
Re: [PATCH] can: m_can: fix boolreturn.cocci warnings
On Tue, Oct 15, 2019 at 06:37:54AM +, Jeroen Hofstee wrote:
> Hi,
>
> On 10/15/19 7:57 AM, Simon Horman wrote:
> > On Mon, Oct 14, 2019 at 11:04:28PM +0800, kbuild test robot wrote:
> >> From: kbuild test robot
> >>
> >> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function
> >> 'is_protocol_err' with return type bool
> >>
> >> Return statements in functions returning bool should use
> >> true/false instead of 1/0.
> >> Generated by: scripts/coccinelle/misc/boolreturn.cocci
> >>
> >> Fixes: 46946163ac61 ("can: m_can: add support for handling arbitration
> >> error")
> >> CC: Pankaj Sharma
> >> Signed-off-by: kbuild test robot
> >> ---
> >>
> >> url:
> >> https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
> >>
> >> m_can.c |4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> --- a/drivers/net/can/m_can/m_can.c
> >> +++ b/drivers/net/can/m_can/m_can.c
> >> @@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
> >> static inline bool is_protocol_err(u32 irqstatus)
> >> {
> >>if (irqstatus & IR_ERR_LEC_31X)
> >> - return 1;
> >> + return true;
> >>else
> >> - return 0;
> >> + return false;
> >> }
> >>
> >> static int m_can_handle_protocol_error(struct net_device *dev, u32
> >> irqstatus)
> >>
> > <2c>
> > Perhaps the following is a nicer way to express this (completely untested):
> >
> > return !!(irqstatus & IR_ERR_LEC_31X);
> >
>
>
> Really, !! for bool / _Bool types? why not simply:
>
> static inline bool is_protocol_err(u32 irqstatus)
> return irqstatus & IR_ERR_LEC_31X;
> }
Good point, silly me.
Re: [PATCH] can: m_can: fix boolreturn.cocci warnings
On Mon, Oct 14, 2019 at 11:04:28PM +0800, kbuild test robot wrote:
> From: kbuild test robot
>
> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function
> 'is_protocol_err' with return type bool
>
> Return statements in functions returning bool should use
> true/false instead of 1/0.
> Generated by: scripts/coccinelle/misc/boolreturn.cocci
>
> Fixes: 46946163ac61 ("can: m_can: add support for handling arbitration error")
> CC: Pankaj Sharma
> Signed-off-by: kbuild test robot
> ---
>
> url:
> https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
>
> m_can.c |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
> static inline bool is_protocol_err(u32 irqstatus)
> {
> if (irqstatus & IR_ERR_LEC_31X)
> - return 1;
> + return true;
> else
> - return 0;
> + return false;
> }
>
> static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
>
<2c>
Perhaps the following is a nicer way to express this (completely untested):
return !!(irqstatus & IR_ERR_LEC_31X);
Re: [PATCH] MAINTAINERS: Remove Simon as Renesas SoC Co-Maintainer
On Thu, Oct 10, 2019 at 02:30:46PM +0200, Geert Uytterhoeven wrote: > At the end of the v5.3 upstream kernel development cycle, Simon stepped > down from his role as Renesas SoC maintainer. > > Remove his maintainership, git repository, and branch from the > MAINTAINERS file, and add an entry to the CREDITS file to honor his > work. > > Signed-off-by: Geert Uytterhoeven Acked-by: Simon Horman > --- > CREDITS | 4 > MAINTAINERS | 4 > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/CREDITS b/CREDITS > index 8b67a85844b55d88..031605d46b4d5cc1 100644 > --- a/CREDITS > +++ b/CREDITS > @@ -1637,6 +1637,10 @@ S: Panoramastrasse 18 > S: D-69126 Heidelberg > S: Germany > > +N: Simon Horman > +M: ho...@verge.net.au > +D: Renesas ARM/ARM64 SoC maintainer > + > N: Christopher Horn > E: ch...@warwick.net > D: Miscellaneous sysctl hacks > diff --git a/MAINTAINERS b/MAINTAINERS > index 94ce075907a0b9aa..d44d6732510df746 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2165,12 +2165,10 @@ F:arch/arm64/boot/dts/realtek/ > F: Documentation/devicetree/bindings/arm/realtek.yaml > > ARM/RENESAS ARM64 ARCHITECTURE > -M: Simon Horman > M: Geert Uytterhoeven > M: Magnus Damm > L: linux-renesas-...@vger.kernel.org > Q: http://patchwork.kernel.org/project/linux-renesas-soc/list/ > -T: git git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git next > T: git > git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git next > S: Supported > F: arch/arm64/boot/dts/renesas/ > @@ -2282,12 +2280,10 @@ S:Maintained > F: drivers/media/platform/s5p-mfc/ > > ARM/SHMOBILE ARM ARCHITECTURE > -M: Simon Horman > M: Geert Uytterhoeven > M: Magnus Damm > L: linux-renesas-...@vger.kernel.org > Q: http://patchwork.kernel.org/project/linux-renesas-soc/list/ > -T: git git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git next > T: git > git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git next > S: Supported > F: arch/arm/boot/dts/emev2* > -- > 2.17.1 >
Re: [PATCH v6 0/3] selftests: netfilter: introduce test cases for ipvs
On Thu, Oct 10, 2019 at 10:50:52PM +0800, Haishuang Yan wrote: > This series patch include test cases for ipvs. > > The test topology is who as below: > +--+ > | | | > | ns0 | ns1 | > | --- | ------| > | | veth01 | - | veth10 || veth12 || > | ---peer ------| > | | || | > | --- || | > | | br0| |- peer |--| > | --- || | > | | || | > | -- peer -- --- | > | | veth02 | - | veth20 | | veth12 | | > | -- | -- --- | > | | ns2 | > | | | > +--+ > > Test results: > # selftests: netfilter: ipvs.sh > # Testing DR mode... > # Testing NAT mode... > # Testing Tunnel mode... > # ipvs.sh: PASS > ok 6 selftests: netfilter: ipvs.sh > > Signed-off-by: Haishuang Yan Thanks, applied to ipvs-next.
Re: [PATCH v5 1/3] selftests: netfilter: add ipvs test script
On Wed, Oct 09, 2019 at 07:16:28PM +0800, Haishuang Yan wrote: > Test virutal server via directing routing for IPv4. > > Tested: > > # selftests: netfilter: ipvs.sh > # Testing DR mode... > # ipvs.sh: PASS > ok 6 selftests: netfilter: ipvs.sh > > Signed-off-by: Haishuang Yan > --- > v5: use cmp to compare two file contents suggested by Simon Horman > v4: use #!/bin/bash -p suggested by Duncan Roe > v3: use bash style > v2: optimize test script > --- > tools/testing/selftests/netfilter/Makefile | 2 +- > tools/testing/selftests/netfilter/ipvs.sh | 178 > + > 2 files changed, 179 insertions(+), 1 deletion(-) > create mode 100755 tools/testing/selftests/netfilter/ipvs.sh > > diff --git a/tools/testing/selftests/netfilter/Makefile > b/tools/testing/selftests/netfilter/Makefile > index 4144984..de1032b 100644 > --- a/tools/testing/selftests/netfilter/Makefile > +++ b/tools/testing/selftests/netfilter/Makefile > @@ -2,6 +2,6 @@ > # Makefile for netfilter selftests > > TEST_PROGS := nft_trans_stress.sh nft_nat.sh bridge_brouter.sh \ > - conntrack_icmp_related.sh nft_flowtable.sh > + conntrack_icmp_related.sh nft_flowtable.sh ipvs.sh > > include ../lib.mk > diff --git a/tools/testing/selftests/netfilter/ipvs.sh > b/tools/testing/selftests/netfilter/ipvs.sh > new file mode 100755 > index 000..f844c0a > --- /dev/null > +++ b/tools/testing/selftests/netfilter/ipvs.sh > @@ -0,0 +1,178 @@ > +#!/bin/bash -p Please change this to /bin/sh
Re: [PATCH v4 1/3] selftests: netfilter: add ipvs test script
On Sat, Oct 05, 2019 at 10:37:43PM +0800, Haishuang Yan wrote:
> Test virutal server via directing routing for IPv4.
>
> Tested:
>
> # selftests: netfilter: ipvs.sh
> # Testing DR mode...
> # ipvs.sh: PASS
> ok 6 selftests: netfilter: ipvs.sh
>
> Signed-off-by: Haishuang Yan
> ---
> v4: use #!/bin/bash -p suggested by Duncan Roe
> v3: use bash style
> v2: optimize test script
> ---
> tools/testing/selftests/netfilter/Makefile | 2 +-
> tools/testing/selftests/netfilter/ipvs.sh | 184
> +
> 2 files changed, 185 insertions(+), 1 deletion(-)
> create mode 100755 tools/testing/selftests/netfilter/ipvs.sh
>
> diff --git a/tools/testing/selftests/netfilter/Makefile
> b/tools/testing/selftests/netfilter/Makefile
> index 4144984..de1032b 100644
> --- a/tools/testing/selftests/netfilter/Makefile
> +++ b/tools/testing/selftests/netfilter/Makefile
> @@ -2,6 +2,6 @@
> # Makefile for netfilter selftests
>
> TEST_PROGS := nft_trans_stress.sh nft_nat.sh bridge_brouter.sh \
> - conntrack_icmp_related.sh nft_flowtable.sh
> + conntrack_icmp_related.sh nft_flowtable.sh ipvs.sh
>
> include ../lib.mk
> diff --git a/tools/testing/selftests/netfilter/ipvs.sh
> b/tools/testing/selftests/netfilter/ipvs.sh
> new file mode 100755
> index 000..f6da1bd
> --- /dev/null
> +++ b/tools/testing/selftests/netfilter/ipvs.sh
> @@ -0,0 +1,184 @@
> +#!/bin/bash -p
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# End-to-end ipvs test suite
> +# Topology:
> +#--+
> +# | |
> +# ns0 | ns1 |
> +# --- | ------|
> +# | veth01 | - | veth10 || veth12 ||
> +# ---peer ------|
> +# | || |
> +# --- || |
> +# | br0| |- peer |--|
> +# --- || |
> +# | || |
> +# -- peer -- --- |
> +# | veth02 | - | veth20 | | veth21 | |
> +# -- | -- --- |
> +# | ns2 |
> +# | |
> +#--+
> +#
> +# We assume that all network driver are loaded
> +#
> +
> +# Kselftest framework requirement - SKIP code is 4.
> +ksft_skip=4
> +ret=0
> +GREEN='\033[0;92m'
> +RED='\033[0;31m'
> +NC='\033[0m' # No Color
> +
> +readonly port=8080
> +
> +readonly vip_v4=207.175.44.110
> +readonly cip_v4=10.0.0.2
> +readonly gip_v4=10.0.0.1
> +readonly dip_v4=172.16.0.1
> +readonly rip_v4=172.16.0.2
> +readonly sip_v4=10.0.0.3
> +
> +readonly infile="$(mktemp)"
> +readonly outfile="$(mktemp)"
> +
> +sysipvsnet=/proc/sys/net/ipv4/vs/
> +if [ ! -d /proc/sys/net/ipv4/vs/ ]; then
> +modprobe -q ip_vs
> +if [ $? -ne 0 ]; then
> +echo "SKIP: Could not run test without ipvs module"
> + exit $ksft_skip
There are some leading spaces before the tab on the line above.
> +fi
> +fi
> +
> +ip -Version > /dev/null 2>&1
> +if [ $? -ne 0 ]; then
> + echo "SKIP: Could not run test without ip tool"
> + exit $ksft_skip
> +fi
> +
> +ipvsadm -v > /dev/null 2>&1
> +if [ $? -ne 0 ]; then
> + echo "SKIP: Could not run test without ipvsadm"
> + exit $ksft_skip
> +fi
> +
> +nc --version > /dev/null 2>&1
> +if [ $? -ne 0 ]; then
> + echo "SKIP: Could not run test without ncat"
> + exit $ksft_skip
> +fi
> +
> +setup() {
> +ip netns add ns0
> +ip netns add ns1
> +ip netns add ns2
> +
> +ip link add veth01 netns ns0 type veth peer name veth10 netns ns1
> +ip link add veth02 netns ns0 type veth peer name veth20 netns ns2
> +ip link add veth12 netns ns1 type veth peer name veth21 netns ns2
> +
> +ip netns exec ns0 ip link set veth01 up
> +ip netns exec ns0 ip link set veth02 up
> +ip netns exec ns0 ip link add br0 type bridge
> +ip netns exec ns0 ip link set veth01 master br0
> +ip netns exec ns0 ip link set veth02 master br0
> +ip netns exec ns0 ip link set br0 up
> +ip netns exec ns0 ip addr add ${cip_v4}/24 dev br0
> +
> +ip netns exec ns1 ip link set lo up
> +ip netns exec ns1 ip link set veth10 up
> +ip netns exec ns1 ip addr add ${gip_v4}/24 dev veth10
> +ip netns exec ns1 ip link set veth12 up
> +ip netns exec ns1 ip addr add ${dip_v4}/24 dev veth12
> +
> +ip netns exec ns2 ip link set lo up
> +ip netns exec ns2 ip link set veth21 up
> +ip netns exec ns2 ip addr add ${rip_v4}/24 dev veth21
> +ip netns
Re: [PATCH] r8152: Set macpassthru in reset_resume callback
On Sat, Oct 05, 2019 at 07:54:15PM +0800, Kai-Heng Feng wrote: > > > > On Oct 5, 2019, at 19:46, Simon Horman wrote: > > > > On Fri, Oct 04, 2019 at 08:51:04PM +0800, Kai-Heng Feng wrote: > >> r8152 may fail to establish network connection after resume from system > >> suspend. > >> > >> If the USB port connects to r8152 lost its power during system suspend, > >> the MAC address was written before is lost. The reason is that The MAC > >> address doesn't get written again in its reset_resume callback. > >> > >> So let's set MAC address again in reset_resume callback. Also remove > >> unnecessary lock as no other locking attempt will happen during > >> reset_resume. > > > > This is two separate seemingly unrelated, other than locality in the code, > > changes. One is a fix, the other seems to be a cleanup. Perhaps they would > > be better addressed in separate patches. > > rtl8152_set_mac_address() which gets called by set_ethernet_addr(), also > holds the same mutex. > So this is more then a cleanup and I should mention it in commit log. Thanks, I agree that is a good idea. > Kai-Heng > > > > >> Signed-off-by: Kai-Heng Feng > >> --- > >> drivers/net/usb/r8152.c | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > >> index 08726090570e..cee9fef925cd 100644 > >> --- a/drivers/net/usb/r8152.c > >> +++ b/drivers/net/usb/r8152.c > >> @@ -4799,10 +4799,9 @@ static int rtl8152_reset_resume(struct > >> usb_interface *intf) > >>struct r8152 *tp = usb_get_intfdata(intf); > >> > >>clear_bit(SELECTIVE_SUSPEND, >flags); > >> - mutex_lock(>control); > >>tp->rtl_ops.init(tp); > >>queue_delayed_work(system_long_wq, >hw_phy_work, 0); > >> - mutex_unlock(>control); > >> + set_ethernet_addr(tp); > >>return rtl8152_resume(intf); > >> } > >> > >> -- > >> 2.17.1 > >> >
Re: [PATCH] r8152: Set macpassthru in reset_resume callback
On Fri, Oct 04, 2019 at 08:51:04PM +0800, Kai-Heng Feng wrote: > r8152 may fail to establish network connection after resume from system > suspend. > > If the USB port connects to r8152 lost its power during system suspend, > the MAC address was written before is lost. The reason is that The MAC > address doesn't get written again in its reset_resume callback. > > So let's set MAC address again in reset_resume callback. Also remove > unnecessary lock as no other locking attempt will happen during > reset_resume. This is two separate seemingly unrelated, other than locality in the code, changes. One is a fix, the other seems to be a cleanup. Perhaps they would be better addressed in separate patches. > Signed-off-by: Kai-Heng Feng > --- > drivers/net/usb/r8152.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > index 08726090570e..cee9fef925cd 100644 > --- a/drivers/net/usb/r8152.c > +++ b/drivers/net/usb/r8152.c > @@ -4799,10 +4799,9 @@ static int rtl8152_reset_resume(struct usb_interface > *intf) > struct r8152 *tp = usb_get_intfdata(intf); > > clear_bit(SELECTIVE_SUSPEND, >flags); > - mutex_lock(>control); > tp->rtl_ops.init(tp); > queue_delayed_work(system_long_wq, >hw_phy_work, 0); > - mutex_unlock(>control); > + set_ethernet_addr(tp); > return rtl8152_resume(intf); > } > > -- > 2.17.1 >
Re: [PATCH v3 0/3] selftests: netfilter: introduce test cases for ipvs
On Wed, Oct 02, 2019 at 11:27:26AM +1000, Duncan Roe wrote: > On Tue, Oct 01, 2019 at 09:34:13PM +0300, Julian Anastasov wrote: > > > > Hello, > > > > On Tue, 1 Oct 2019, Haishuang Yan wrote: > > > > > This series patch include test cases for ipvs. > > > > > > The test topology is who as below: > > > +--+ > > > | | | > > > | ns0 | ns1 | > > > | --- | ------| > > > | | veth01 | - | veth10 || veth12 || > > > | ---peer ------| > > > | | || | > > > | --- || | > > > | | br0| |- peer |--| > > > | --- || | > > > | | || | > > > | -- peer -- --- | > > > | | veth02 | - | veth20 | | veth12 | | > > > | -- | -- --- | > > > | | ns2 | > > > | | | > > > +--+ > > > > > > Test results: > > > # selftests: netfilter: ipvs.sh > > > # Testing DR mode... > > > # Testing NAT mode... > > > # Testing Tunnel mode... > > > # ipvs.sh: PASS > > > ok 6 selftests: netfilter: ipvs.sh > > > > > > Haishuang Yan (3): > > > selftests: netfilter: add ipvs test script > > > selftests: netfilter: add ipvs nat test case > > > selftests: netfilter: add ipvs tunnel test case > > > > Acked-by: Julian Anastasov > > > > > tools/testing/selftests/netfilter/Makefile | 2 +- > > > tools/testing/selftests/netfilter/ipvs.sh | 234 > > > + > > > 2 files changed, 235 insertions(+), 1 deletion(-) > > > create mode 100755 tools/testing/selftests/netfilter/ipvs.sh > > > > Regards > > > > -- > > Julian Anastasov > > I still prefer #!/bin/sh in 1/3. You never know what's in someone's > environment That would be my preference too.
Re: [PATCH v2 0/2] ipvs: speedup ipvs netns dismantle
On Mon, Sep 30, 2019 at 10:08:23PM +0300, Julian Anastasov wrote: > > Hello, > > On Fri, 27 Sep 2019, Haishuang Yan wrote: > > > Implement exit_batch() method to dismantle more ipvs netns > > per round. > > > > Tested: > > $ cat add_del_unshare.sh > > #!/bin/bash > > > > for i in `seq 1 100` > > do > > (for j in `seq 1 40` ; do unshare -n ipvsadm -A -t 172.16.$i.$j:80 > > >/dev/null ; done) & > > done > > wait; grep net_namespace /proc/slabinfo > > > > Befor patch: > > $ time sh add_del_unshare.sh > > net_namespace 4020 4020 473668 : tunables000 > > : slabdata670670 0 > > > > real0m8.086s > > user0m2.025s > > sys 0m36.956s > > > > After patch: > > $ time sh add_del_unshare.sh > > net_namespace 4020 4020 473668 : tunables000 > > : slabdata670670 0 > > > > real0m7.623s > > user0m2.003s > > sys 0m32.935s > > > > Haishuang Yan (2): > > ipvs: batch __ip_vs_cleanup > > ipvs: batch __ip_vs_dev_cleanup > > > > include/net/ip_vs.h | 2 +- > > net/netfilter/ipvs/ip_vs_core.c | 47 > > - > > net/netfilter/ipvs/ip_vs_ctl.c | 12 --- > > 3 files changed, 38 insertions(+), 23 deletions(-) > > Both patches in v2 look good to me, thanks! > > Acked-by: Julian Anastasov > > This is for the -next kernels... Thanks, applied to ipvs-next.
Re: [PATCH v2 1/3] selftests: netfilter: add ipvs test script
On Fri, Sep 27, 2019 at 02:21:04PM +0800, Haishuang Yan wrote:
> Test virutal server via directing routing for IPv4.
>
> Tested:
>
> # selftests: netfilter: ipvs.sh
> # Testing DR mode...
> # ipvs.sh: PASS
> ok 6 selftests: netfilter: ipvs.sh
>
> Signed-off-by: Haishuang Yan
> ---
> v2: optimize test script
> ---
> tools/testing/selftests/netfilter/Makefile | 2 +-
> tools/testing/selftests/netfilter/ipvs.sh | 184
> +
> 2 files changed, 185 insertions(+), 1 deletion(-)
> create mode 100755 tools/testing/selftests/netfilter/ipvs.sh
>
> diff --git a/tools/testing/selftests/netfilter/Makefile
> b/tools/testing/selftests/netfilter/Makefile
> index 4144984..de1032b 100644
> --- a/tools/testing/selftests/netfilter/Makefile
> +++ b/tools/testing/selftests/netfilter/Makefile
> @@ -2,6 +2,6 @@
> # Makefile for netfilter selftests
>
> TEST_PROGS := nft_trans_stress.sh nft_nat.sh bridge_brouter.sh \
> - conntrack_icmp_related.sh nft_flowtable.sh
> + conntrack_icmp_related.sh nft_flowtable.sh ipvs.sh
>
> include ../lib.mk
> diff --git a/tools/testing/selftests/netfilter/ipvs.sh
> b/tools/testing/selftests/netfilter/ipvs.sh
> new file mode 100755
> index 000..658c06b
> --- /dev/null
> +++ b/tools/testing/selftests/netfilter/ipvs.sh
> @@ -0,0 +1,184 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# End-to-end ipvs test suite
> +# Topology:
> +#--+
> +# | |
> +# ns0 | ns1 |
> +# --- | ------|
> +# | veth01 | - | veth10 || veth12 ||
> +# ---peer ------|
> +# | || |
> +# --- || |
> +# | br0| |- peer |--|
> +# --- || |
> +# | || |
> +# -- peer -- --- |
> +# | veth02 | - | veth20 | | veth21 | |
> +# -- | -- --- |
> +# | ns2 |
> +# | |
> +#--+
> +#
> +# We assume that all network driver are loaded
> +#
> +
> +# Kselftest framework requirement - SKIP code is 4.
> +ksft_skip=4
> +ret=0
> +GREEN='\033[0;92m'
> +RED='\033[0;31m'
> +NC='\033[0m' # No Color
> +
> +readonly port=8080
> +
> +readonly vip_v4=207.175.44.110
> +readonly cip_v4=10.0.0.2
> +readonly gip_v4=10.0.0.1
> +readonly dip_v4=172.16.0.1
> +readonly rip_v4=172.16.0.2
> +readonly sip_v4=10.0.0.3
> +
> +readonly infile="$(mktemp)"
> +readonly outfile="$(mktemp)"
> +
> +sysipvsnet=/proc/sys/net/ipv4/vs/
> +if [ ! -d /proc/sys/net/ipv4/vs/ ]; then
> +modprobe -q ip_vs
> +if [ $? -ne 0 ]; then
> +echo "SKIP: Could not run test without ipvs module"
> + exit $ksft_skip
> +fi
> +fi
> +
> +ip -Version > /dev/null 2>&1
> +if [ $? -ne 0 ]; then
> + echo "SKIP: Could not run test without ip tool"
> + exit $ksft_skip
> +fi
> +
> +ipvsadm -v > /dev/null 2>&1
> +if [ $? -ne 0 ]; then
> + echo "SKIP: Could not run test without ipvsadm"
> + exit $ksft_skip
> +fi
> +
> +nc --version > /dev/null 2>&1
> +if [ $? -ne 0 ]; then
> + echo "SKIP: Could not run test without ncat"
> + exit $ksft_skip
> +fi
> +
> +setup() {
> +ip netns add ns0
> +ip netns add ns1
> +ip netns add ns2
> +
> +ip link add veth01 netns ns0 type veth peer name veth10 netns ns1
> +ip link add veth02 netns ns0 type veth peer name veth20 netns ns2
> +ip link add veth12 netns ns1 type veth peer name veth21 netns ns2
> +
> +ip netns exec ns0 ip link set veth01 up
> +ip netns exec ns0 ip link set veth02 up
> +ip netns exec ns0 ip link add br0 type bridge
> +ip netns exec ns0 ip link set veth01 master br0
> +ip netns exec ns0 ip link set veth02 master br0
> +ip netns exec ns0 ip link set br0 up
> +ip netns exec ns0 ip addr add ${cip_v4}/24 dev br0
> +
> +ip netns exec ns1 ip link set lo up
> +ip netns exec ns1 ip link set veth10 up
> +ip netns exec ns1 ip addr add ${gip_v4}/24 dev veth10
> +ip netns exec ns1 ip link set veth12 up
> +ip netns exec ns1 ip addr add ${dip_v4}/24 dev veth12
> +
> +ip netns exec ns2 ip link set lo up
> +ip netns exec ns2 ip link set veth21 up
> +ip netns exec ns2 ip addr add ${rip_v4}/24 dev veth21
> +ip netns exec ns2 ip link set veth20 up
> +ip netns exec ns2 ip addr add ${sip_v4}/24 dev veth20
> +}
> +
> +cleanup() {
> +for i in 0 1 2
> +
Re: [PATCH] soc: renesas: rcar-sysc: fix memory leak in rcar_sysc_pd_init
Ni Navid,
thanks for your patch.
On Wed, Sep 25, 2019 at 04:03:53PM -0500, Navid Emamdoost wrote:
> In rcar_sysc_pd_init when looping over info->areas errors may happen but
> the error handling path does not clean up the intermediate allocated
> memories.
>
> This patch changes the error handling path in major and a little the loop
> itself. Inside the loop if an error happens the current pd will be
> released and then it goes to error handling path where it releases any
> previously allocated domains.
>
> Signed-off-by: Navid Emamdoost
> ---
> drivers/soc/renesas/rcar-sysc.c | 27 ---
> 1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/soc/renesas/rcar-sysc.c b/drivers/soc/renesas/rcar-sysc.c
> index 59b5e6b10272..f9613c1ee0a0 100644
> --- a/drivers/soc/renesas/rcar-sysc.c
> +++ b/drivers/soc/renesas/rcar-sysc.c
> @@ -330,10 +330,10 @@ static int __init rcar_sysc_pd_init(void)
> {
> const struct rcar_sysc_info *info;
> const struct of_device_id *match;
> - struct rcar_pm_domains *domains;
> + struct rcar_pm_domains *domains = NULL;
> struct device_node *np;
> void __iomem *base;
> - unsigned int i;
> + unsigned int i, num_areas = 0;
> int error;
Please preserve reverse xmas tree sorting of local variables.
> np = of_find_matching_node_and_match(NULL, rcar_sysc_matches, );
> @@ -382,6 +382,7 @@ static int __init rcar_sysc_pd_init(void)
> pd = kzalloc(sizeof(*pd) + strlen(area->name) + 1, GFP_KERNEL);
> if (!pd) {
> error = -ENOMEM;
> + num_areas = i;
> goto out_put;
> }
>
> @@ -393,8 +394,11 @@ static int __init rcar_sysc_pd_init(void)
> pd->flags = area->flags;
>
> error = rcar_sysc_pd_setup(pd);
> - if (error)
> + if (error) {
> + kfree(pd);
> + num_areas = i;
> goto out_put;
> + }
>
> domains->domains[area->isr_bit] = >genpd;
>
> @@ -406,13 +410,30 @@ static int __init rcar_sysc_pd_init(void)
> if (error) {
> pr_warn("Failed to add PM subdomain %s to parent %u\n",
> area->name, area->parent);
> + kfree(pd);
> + num_areas = i;
> goto out_put;
> }
> }
>
> error = of_genpd_add_provider_onecell(np, >onecell_data);
> + of_node_put(np);
> +
> + return error;
>
> out_put:
> + if (domains) {
> + for (i = 0; i < num_areas; i++) {
> + const struct rcar_sysc_area *area = >areas[i];
> +
> + if (!area->name) {
> + /* Skip NULLified area */
> + continue;
> + }
> + kfree(domains->domains[area->isr_bit]);
This cleanup doesn't feel correct to me.
For one I think the allocated memory is at
to_rcar_pd(domains->domains[area->isr_bit]);
And for antoher I wonder if it is also necessary to unwind initialisation done
by rcar_sysc_pd_setup() and pm_genpd_add_subdomain();
I think this leads us to the heart of why such unwinding is not present
and that is, I suspect, that its reasonably complex and in the event of
failure the system is very likely unusable. So leaking a bit of memory,
while unpleasent, doesn't effect the user experience.
> + }
> + kfree(domains);
> + }
> of_node_put(np);
> return error;
I think it would be more in keeping with kernel coding style to add
some extra labels for different error paths. I also think you can
utilise the fact that i is already set to the number of allocated areas.
Something like this (completely untested):
out_free_areas:
while (--i > 0) {
/* Cleanup of 'i' goes here */
}
out_free_domains:
kfree(domains);
out_put:
of_node_put(np);
return error;
> }
> --
> 2.17.1
>
Re: [PATCH v7 1/7] nfc: pn533: i2c: "pn532" as dt compatible string
On Tue, Sep 10, 2019 at 11:31:21AM +0200, Lars Poeschel wrote:
> It is favourable to have one unified compatible string for devices that
> have multiple interfaces. So this adds simply "pn532" as the devicetree
> binding compatible string and makes a note that the old ones are
> deprecated.
Do you also need to update
Documentation/devicetree/bindings/net/nfc/pn533-i2c.txt
to both document the new compat string and deprecate the old ones?
> Cc: Johan Hovold
> Signed-off-by: Lars Poeschel
> ---
> Changes in v6:
> - Rebased the patch series on v5.3-rc5
>
> Changes in v3:
> - This patch is new in v3
>
> drivers/nfc/pn533/i2c.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
> index 1832cd921ea7..1abd40398a5a 100644
> --- a/drivers/nfc/pn533/i2c.c
> +++ b/drivers/nfc/pn533/i2c.c
> @@ -245,6 +245,11 @@ static int pn533_i2c_remove(struct i2c_client *client)
> }
>
> static const struct of_device_id of_pn533_i2c_match[] = {
> + { .compatible = "nxp,pn532", },
> + /*
> + * NOTE: The use of the compatibles with the trailing "...-i2c" is
> + * deprecated and will be removed.
> + */
> { .compatible = "nxp,pn533-i2c", },
> { .compatible = "nxp,pn532-i2c", },
> {},
> --
> 2.23.0
>
Re: [PATCH] arm64: dts: renesas: r8a77970: Fix PWM3
On Thu, Sep 12, 2019 at 11:31:43AM +0100, Kieran Bingham wrote:
> The pwm3 was incorrectly added with a compatible reference to the
> renesas,pwm-r8a7790 (H2) due to a single characther ommision.
>
> Fix the compatible string.
>
> Fixes: de625477c632 ("arm64: dts: renesas: r8a779{7|8}0: add PWM support")
> Signed-off-by: Kieran Bingham
Reviewed-by: Simon Horman
> ---
> arch/arm64/boot/dts/renesas/r8a77970.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> index 2c4ab70e2a39..74c2c0024e45 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> @@ -652,7 +652,7 @@
> };
>
> pwm3: pwm@e6e33000 {
> - compatible = "renesas,pwm-r8a7790", "renesas,pwm-rcar";
> + compatible = "renesas,pwm-r8a77970", "renesas,pwm-rcar";
> reg = <0 0xe6e33000 0 8>;
> #pwm-cells = <2>;
> clocks = < CPG_MOD 523>;
> --
> 2.20.1
>
Re: [PATCH] arm: dts: renesas: r8a77980: Remove r8a77970 DU compatible
On Thu, Sep 12, 2019 at 11:37:34AM +0100, Kieran Bingham wrote:
> The r8a77970 was added with an compatible string for a differnet device
> rather than adding the correct compatible to the driver.
>
> Remove the unnecessary compatible which is for a different platform.
>
> Signed-off-by: Kieran Bingham
Reviewed-by: Simon Horman
> ---
> Please note, this patch should not be integrated until the renesas,du-r8a77980
> compatible string makes it into the DU [0].
>
> [0]
> https://lore.kernel.org/linux-renesas-soc/a9cc2193-0a18-0490-c273-c64bd7099...@ideasonboard.com/T/#t
>
> arch/arm64/boot/dts/renesas/r8a77980.dtsi | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77980.dtsi
> b/arch/arm64/boot/dts/renesas/r8a77980.dtsi
> index 042f4089e546..c6195377d181 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77980.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77980.dtsi
> @@ -1487,8 +1487,7 @@
> };
>
> du: display@feb0 {
> - compatible = "renesas,du-r8a77980",
> - "renesas,du-r8a77970";
> + compatible = "renesas,du-r8a77980";
> reg = <0 0xfeb0 0 0x8>;
> interrupts = ;
> clocks = < CPG_MOD 724>;
> --
> 2.20.1
>
Re: [PATCH 5/6] dt-bindings: timer: renesas: tmu: Document r8a774a1 bindings
On Fri, Aug 30, 2019 at 10:37:54AM +, Fabrizio Castro wrote: > Dear All, > > This patch has been reviewed by Geert, Simon, and Rob, so I think it's ok to > apply. > Is anybody willing to take this patch? <2c> I think Geert can take this > Thanks, > Fab > > > From: Fabrizio Castro > > Sent: 11 June 2019 14:07 > > Subject: [PATCH 5/6] dt-bindings: timer: renesas: tmu: Document r8a774a1 > > bindings > > > > Document RZ/G2M (R8A774A1) SoC in the Renesas TMU bindings. > > > > Signed-off-by: Fabrizio Castro > > --- > > Documentation/devicetree/bindings/timer/renesas,tmu.txt | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/Documentation/devicetree/bindings/timer/renesas,tmu.txt > > b/Documentation/devicetree/bindings/timer/renesas,tmu.txt > > index 13ad074..9dff7e5 100644 > > --- a/Documentation/devicetree/bindings/timer/renesas,tmu.txt > > +++ b/Documentation/devicetree/bindings/timer/renesas,tmu.txt > > @@ -10,6 +10,7 @@ Required Properties: > > > >- compatible: must contain one or more of the following: > > - "renesas,tmu-r8a7740" for the r8a7740 TMU > > +- "renesas,tmu-r8a774a1" for the r8a774A1 TMU > > - "renesas,tmu-r8a774c0" for the r8a774C0 TMU > > - "renesas,tmu-r8a7778" for the r8a7778 TMU > > - "renesas,tmu-r8a7779" for the r8a7779 TMU > > -- > > 2.7.4 >
Re: [PATCH] thermal: rcar_gen3_thermal: Fix Wshift-negative-value
On Thu, Aug 29, 2019 at 03:11:24PM +0200, Wolfram Sang wrote:
> On Wed, Aug 28, 2019 at 04:52:20PM +0800, Zhang Rui wrote:
> > On Fri, 2019-06-14 at 12:52 +0200, Daniel Lezcano wrote:
> > > Hi Nathan,
> > >
> > > On 13/06/2019 23:12, Nathan Huckleberry wrote:
> > > > Clang produces the following warning
> > > >
> > > > vers/thermal/rcar_gen3_thermal.c:147:33: warning: shifting a
> > > > negative
> > > > signed value is undefined [-Wshift-negative-value] / (ptat[0] -
> > > > ptat[2])) +
> > > > FIXPT_INT(TJ_3); ^~~
> > > > drivers/thermal/rcar_gen3_thermal.c:126:29
> > > > note: expanded from macro 'FIXPT_INT' #define FIXPT_INT(_x) ((_x)
> > > > <<
> > > > FIXPT_SHIFT) ^ drivers/thermal/rcar_gen3_thermal.c:150:18:
> > > > warning:
> > > > shifting a negative signed value is undefined [-Wshift-negative-
> > > > value]
> > > > tsc->tj_t - FIXPT_INT(TJ_3)); ^~~~
> > > >
> > > > Upon further investigating it looks like there is no real reason
> > > > for
> > > > TJ_3 to be -41. Usages subtract -41, code would be cleaner to just
> > > > add.
> > >
> > > All the code seems broken regarding the negative value shifting as
> > > the
> > > macros pass an integer:
> > >
> > > eg.
> > > tsc->coef.a2 = FIXPT_DIV(FIXPT_INT(thcode[1] - thcode[0]),
> > > tsc->tj_t - FIXPT_INT(ths_tj_1));
> > >
> > > thcode[1] is always < than thcode[0],
> > >
> > > thcode[1] - thcode[0] < 0
> > >
> > > FIXPT_INT(thcode[1] - thcode[0]) is undefined
> > >
> > >
> > > Is it done on purpose FIXPT_DIV(FIXPT_INT(thcode[1] - thcode[0]) ?
> > >
> > > Try developing the macro with the coef.a2 computation ...
> > >
> > > The code quality of this driver could be better, it deserves a rework
> > > IMHO ...
> > >
> > > I suggest to revert:
> > >
> > > 4eb39f79ef443fa566d36bd43f1f578d5c140305
> > > bdc4480a669d476814061b4da6bb006f7048c8e5
> > > 6a310f8f97bb8bc2e2bb9db6f49a1b8678c8d144
> > >
> > > Rework the coefficient computation and re-introduce what was reverted
> > > in
> > > a nicer way.
> >
> > Sounds reasonable to me.
> >
> > Yoshihiro,
> > can you please clarify on this? Or else I will revert the above commits
> > first?
> >
> > Also CC Wolfram Sang, the driver author.
>
> CCing Simon Horman who worked with Kaneko-san on these changes.
Hi,
I think that what has happened here is that these patches and moreover the
driver has been through quite a few hands and I agree that zooming out and
cleaning things up would make a lot of sense. Personally I would take the
approach of incrementally cleaning things up. But I don't feel strongly
about this.
As for the specific question about a negative constant, I don't know of a
specific reason that approach was taken and I don't recall investigating it
at the time.
Kind regards,
Simon
>
> > thanks,
> > rui
> > >
> > >
> > > > Fixes: 4eb39f79ef44 ("thermal: rcar_gen3_thermal: Update value of
> > > > Tj_1")
> > > > Cc: clang-built-li...@googlegroups.com
> > > > Link: https://github.com/ClangBuiltLinux/linux/issues/531
> > > > Signed-off-by: Nathan Huckleberry
> > > > ---
> > > > drivers/thermal/rcar_gen3_thermal.c | 8
> > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/thermal/rcar_gen3_thermal.c
> > > > b/drivers/thermal/rcar_gen3_thermal.c
> > > > index a56463308694..f4b4558c08e9 100644
> > > > --- a/drivers/thermal/rcar_gen3_thermal.c
> > > > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > > > @@ -131,7 +131,7 @@ static inline void
> > > > rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
> > > > #define RCAR3_THERMAL_GRAN 500 /* mili Celsius */
> > > >
> > > > /* no idea where these constants come from */
Regarding the line above, I believe the constant comes
from the documentation.
> > > > -#define TJ_3 -41
> > > > +#define TJ_3 41
> > > >
> > > > static void rcar_gen3_thermal_calc_coefs(struct
> > > > rcar_gen3_thermal_tsc *tsc,
> > > >
Re: [PATCH] ARM: ARM_ERRATA_775420: Spelling s/date/data/
On Wed, Aug 28, 2019 at 03:31:51PM +0200, Geert Uytterhoeven wrote: > Caching dates is never a good idea ;-) > > Signed-off-by: Geert Uytterhoeven Reviewed-by: Simon Horman > --- > arch/arm/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index dcf46f0e45c24a5f..eb18279a63027c08 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -1040,7 +1040,7 @@ config ARM_ERRATA_775420 > depends on CPU_V7 > help >This option enables the workaround for the 775420 Cortex-A9 (r2p2, > - r2p6,r2p8,r2p10,r3p0) erratum. In case a date cache maintenance > + r2p6,r2p8,r2p10,r3p0) erratum. In case a data cache maintenance >operation aborts with MMU exception, it might cause the processor >to deadlock. This workaround puts DSB before executing ISB if >an abort may occur on cache maintenance. > -- > 2.17.1 >
Re: [PATCH v3 3/3] clocksource/drivers/ostm: Use unique device name instead of ostm
On Fri, Aug 16, 2019 at 03:18:06PM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
>
> On Fri, Aug 16, 2019 at 3:11 PM Simon Horman wrote:
> > On Wed, Aug 07, 2019 at 10:46:35AM +0200, Geert Uytterhoeven wrote:
> > > Currently all OSTM devices are called "ostm", also in kernel messages.
> > >
> > > As there can be multiple instances in an SoC, this can confuse the user.
> > > Hence construct a unique name from the DT node name, like is done for
> > > platform devices.
> > >
> > > On RSK+RZA1, the boot log changes like:
> > >
> > > -clocksource: ostm: mask: 0x max_cycles: 0x,
> > > max_idle_ns: 57352151442 ns
> > > +clocksource: ostm fcfec000.timer: mask: 0x max_cycles:
> > > 0x, max_idle_ns: 57352151442 ns
> > > sched_clock: 32 bits at 33MHz, resolution 30ns, wraps every
> > > 64440619504ns
> > > -ostm: used for clocksource
> > > -ostm: used for clock events
> > > +ostm fcfec000.timer: used for clocksource
> > > +ostm fcfec400.timer: used for clock events
> > >
> > > Signed-off-by: Geert Uytterhoeven
> > > ---
> > > v3:
> > > - Make the name format similar to the one used for platform devices,
> > > - Use kasprintf() instead of buffer size guessing,
> > > - Use a real example from rskrza1.
> > >
> > > v2 (by Jacopo):
> > > - Use np->full_name.
> > > ---
> > > drivers/clocksource/renesas-ostm.c | 45 --
> > > 1 file changed, 30 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/clocksource/renesas-ostm.c
> > > b/drivers/clocksource/renesas-ostm.c
> > > index 1e22e54d7b0df40d..659e3ec7b86714e3 100644
> > > --- a/drivers/clocksource/renesas-ostm.c
> > > +++ b/drivers/clocksource/renesas-ostm.c
>
> > > @@ -195,22 +195,35 @@ static int __init ostm_init(struct device_node *np)
> > > if (!ostm)
> > > return -ENOMEM;
> > >
> > > - ostm->base = of_iomap(np, 0);
> > > - if (!ostm->base) {
> > > + ret = of_address_to_resource(np, 0, );
> > > + if (ret) {
> > > + pr_err("ostm: Failed to obtain I/O memory\n");
> > > + goto err_free;
> > > + }
> > > +
> > > + ostm->name = kasprintf(GFP_KERNEL, "ostm %llx.%s",
> > > +(unsigned long long)res.start, np->name);
> >
> > I'm not sure, but looking at printk-formats.rst it seems that
> > %pa[p] as a format specifier for resource_size_t. If so it may
> > allow dropping the cast above.
>
> Using "%pa" adds a "0x" prefix, which we don't want here.
Thanks, got it.
Re: [PATCH 6/7] clocksource/drivers/sh_cmt: r8a7740 and sh73a0 SoC-specific match
On Tue, Aug 20, 2019 at 09:34:13PM +0900, Magnus Damm wrote:
> Hi Simon,
>
> On Wed, Jul 24, 2019 at 8:12 PM Simon Horman wrote:
> >
> > On Thu, Jul 18, 2019 at 08:45:24PM +0900, Magnus Damm wrote:
> > > From: Magnus Damm
> > >
> > > Add SoC-specific matching for CMT1 on r8a7740 and sh73a0.
> > >
> > > This allows us to move away from the old DT bindings such as
> > > - "renesas,cmt-48-sh73a0"
> > > - "renesas,cmt-48-r8a7740"
> > > - "renesas,cmt-48"
> > > in favour for the now commonly used format "renesas,-"
> > >
> > > Signed-off-by: Magnus Damm
> >
> > Reviewed-by: Simon Horman
>
> Thanks!
>
> > > ---
> > >
> > > drivers/clocksource/sh_cmt.c |8
> > > 1 file changed, 8 insertions(+)
> > >
> > > --- 0001/drivers/clocksource/sh_cmt.c
> > > +++ work/drivers/clocksource/sh_cmt.c 2019-07-18 19:29:06.005414716 +0900
> > > @@ -928,6 +928,14 @@ static const struct of_device_id sh_cmt_
> > > .data = _cmt_info[SH_CMT0_RCAR_GEN2]
> > > },
> > > {
> > > + .compatible = "renesas,r8a7740-cmt1",
> > > + .data = _cmt_info[SH_CMT_48BIT]
> >
> > Perhaps as a follow-up SH_CMT_48BIT could be renamed.
>
> I was actually considering implementing proper 48-bit support, and
> reworking those names might be a good idea at that point.
Sure, I don't think there is any urgency on updating the name :)
Re: [PATCH v3 2/3] clocksource/drivers/renesas-ostm: Fix probe error path
On Wed, Aug 07, 2019 at 08:13:27PM +0200, Daniel Lezcano wrote: > On 07/08/2019 10:46, Geert Uytterhoeven wrote: > > Fix various issues in the error path of ostm_init(): > > 1. Drop error message printing on of_iomap() failure, as the memory > > allocation core already takes of that, > > 2. Handle irq_of_parse_and_map() failures correctly: it returns > > unsigned int, hence make irq unsigned int, and zero is an error, > > 3. Propagate/return appropriate error codes instead of -EFAULT. > > 4. Add a missing clk_put(), > > 5. Split the single cleanup block in separate phases, to avoid > > clk_put() calling WARN() when passed an error pointer. > > Does it make sense to convert to timer-of API? I don't have an opinion on that at this time, but as an improvement this patch looks good to me. Reviewed-by: Simon Horman > > Signed-off-by: Geert Uytterhoeven
Re: [PATCH v3 3/3] clocksource/drivers/ostm: Use unique device name instead of ostm
On Fri, Aug 16, 2019 at 03:11:29PM +0200, Simon Horman wrote:
> On Wed, Aug 07, 2019 at 10:46:35AM +0200, Geert Uytterhoeven wrote:
> > Currently all OSTM devices are called "ostm", also in kernel messages.
> >
> > As there can be multiple instances in an SoC, this can confuse the user.
> > Hence construct a unique name from the DT node name, like is done for
> > platform devices.
> >
> > On RSK+RZA1, the boot log changes like:
> >
> > -clocksource: ostm: mask: 0x max_cycles: 0x,
> > max_idle_ns: 57352151442 ns
> > +clocksource: ostm fcfec000.timer: mask: 0x max_cycles:
> > 0x, max_idle_ns: 57352151442 ns
> > sched_clock: 32 bits at 33MHz, resolution 30ns, wraps every
> > 64440619504ns
> > -ostm: used for clocksource
> > -ostm: used for clock events
> > +ostm fcfec000.timer: used for clocksource
> > +ostm fcfec400.timer: used for clock events
> >
> > Signed-off-by: Geert Uytterhoeven
The nit in my previous email notwithstanding
Reviewed-by: Simon Horman
> > ---
> > v3:
> > - Make the name format similar to the one used for platform devices,
> > - Use kasprintf() instead of buffer size guessing,
> > - Use a real example from rskrza1.
> >
> > v2 (by Jacopo):
> > - Use np->full_name.
> > ---
> > drivers/clocksource/renesas-ostm.c | 45 --
> > 1 file changed, 30 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/clocksource/renesas-ostm.c
> > b/drivers/clocksource/renesas-ostm.c
> > index 1e22e54d7b0df40d..659e3ec7b86714e3 100644
> > --- a/drivers/clocksource/renesas-ostm.c
> > +++ b/drivers/clocksource/renesas-ostm.c
> > @@ -25,6 +25,7 @@
> > */
> >
> > struct ostm_device {
> > + const char *name;
> > void __iomem *base;
> > unsigned long ticks_per_jiffy;
> > struct clock_event_device ced;
> > @@ -79,9 +80,8 @@ static int __init ostm_init_clksrc(struct ostm_device
> > *ostm, unsigned long rate)
> > writeb(CTL_FREERUN, ostm->base + OSTM_CTL);
> > writeb(TS, ostm->base + OSTM_TS);
> >
> > - return clocksource_mmio_init(ostm->base + OSTM_CNT,
> > - "ostm", rate,
> > - 300, 32, clocksource_mmio_readl_up);
> > + return clocksource_mmio_init(ostm->base + OSTM_CNT, ostm->name, rate,
> > +300, 32, clocksource_mmio_readl_up);
> > }
> >
> > static u64 notrace ostm_read_sched_clock(void)
> > @@ -161,15 +161,14 @@ static int __init ostm_init_clkevt(struct ostm_device
> > *ostm, unsigned int irq,
> > struct clock_event_device *ced = >ced;
> > int ret = -ENXIO;
> >
> > - ret = request_irq(irq, ostm_timer_interrupt,
> > - IRQF_TIMER | IRQF_IRQPOLL,
> > - "ostm", ostm);
> > + ret = request_irq(irq, ostm_timer_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
> > + ostm->name, ostm);
> > if (ret) {
> > - pr_err("ostm: failed to request irq\n");
> > + pr_err("%s: Failed to request irq\n", ostm->name);
> > return ret;
> > }
> >
> > - ced->name = "ostm";
> > + ced->name = ostm->name;
> > ced->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC;
> > ced->set_state_shutdown = ostm_shutdown;
> > ced->set_state_periodic = ostm_set_periodic;
> > @@ -187,6 +186,7 @@ static int __init ostm_init(struct device_node *np)
> > {
> > struct clk *ostm_clk = NULL;
> > struct ostm_device *ostm;
> > + struct resource res;
> > unsigned long rate;
> > unsigned int irq;
> > int ret;
> > @@ -195,22 +195,35 @@ static int __init ostm_init(struct device_node *np)
> > if (!ostm)
> > return -ENOMEM;
> >
> > - ostm->base = of_iomap(np, 0);
> > - if (!ostm->base) {
> > + ret = of_address_to_resource(np, 0, );
> > + if (ret) {
> > + pr_err("ostm: Failed to obtain I/O memory\n");
> > + goto err_free;
> > + }
> > +
> > + ostm->name = kasprintf(GFP_KERNEL, "ostm %llx.%s",
> > + (unsigned long long)res.start, np->name);
>
> I'm not sure, but looking at printk-formats.rst it seems that
> %pa[p] as a format specifier for resource_size_t. If so it may
Re: [PATCH v3 3/3] clocksource/drivers/ostm: Use unique device name instead of ostm
On Wed, Aug 07, 2019 at 10:46:35AM +0200, Geert Uytterhoeven wrote:
> Currently all OSTM devices are called "ostm", also in kernel messages.
>
> As there can be multiple instances in an SoC, this can confuse the user.
> Hence construct a unique name from the DT node name, like is done for
> platform devices.
>
> On RSK+RZA1, the boot log changes like:
>
> -clocksource: ostm: mask: 0x max_cycles: 0x, max_idle_ns:
> 57352151442 ns
> +clocksource: ostm fcfec000.timer: mask: 0x max_cycles:
> 0x, max_idle_ns: 57352151442 ns
> sched_clock: 32 bits at 33MHz, resolution 30ns, wraps every 64440619504ns
> -ostm: used for clocksource
> -ostm: used for clock events
> +ostm fcfec000.timer: used for clocksource
> +ostm fcfec400.timer: used for clock events
>
> Signed-off-by: Geert Uytterhoeven
> ---
> v3:
> - Make the name format similar to the one used for platform devices,
> - Use kasprintf() instead of buffer size guessing,
> - Use a real example from rskrza1.
>
> v2 (by Jacopo):
> - Use np->full_name.
> ---
> drivers/clocksource/renesas-ostm.c | 45 --
> 1 file changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/clocksource/renesas-ostm.c
> b/drivers/clocksource/renesas-ostm.c
> index 1e22e54d7b0df40d..659e3ec7b86714e3 100644
> --- a/drivers/clocksource/renesas-ostm.c
> +++ b/drivers/clocksource/renesas-ostm.c
> @@ -25,6 +25,7 @@
> */
>
> struct ostm_device {
> + const char *name;
> void __iomem *base;
> unsigned long ticks_per_jiffy;
> struct clock_event_device ced;
> @@ -79,9 +80,8 @@ static int __init ostm_init_clksrc(struct ostm_device
> *ostm, unsigned long rate)
> writeb(CTL_FREERUN, ostm->base + OSTM_CTL);
> writeb(TS, ostm->base + OSTM_TS);
>
> - return clocksource_mmio_init(ostm->base + OSTM_CNT,
> - "ostm", rate,
> - 300, 32, clocksource_mmio_readl_up);
> + return clocksource_mmio_init(ostm->base + OSTM_CNT, ostm->name, rate,
> + 300, 32, clocksource_mmio_readl_up);
> }
>
> static u64 notrace ostm_read_sched_clock(void)
> @@ -161,15 +161,14 @@ static int __init ostm_init_clkevt(struct ostm_device
> *ostm, unsigned int irq,
> struct clock_event_device *ced = >ced;
> int ret = -ENXIO;
>
> - ret = request_irq(irq, ostm_timer_interrupt,
> - IRQF_TIMER | IRQF_IRQPOLL,
> - "ostm", ostm);
> + ret = request_irq(irq, ostm_timer_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
> + ostm->name, ostm);
> if (ret) {
> - pr_err("ostm: failed to request irq\n");
> + pr_err("%s: Failed to request irq\n", ostm->name);
> return ret;
> }
>
> - ced->name = "ostm";
> + ced->name = ostm->name;
> ced->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC;
> ced->set_state_shutdown = ostm_shutdown;
> ced->set_state_periodic = ostm_set_periodic;
> @@ -187,6 +186,7 @@ static int __init ostm_init(struct device_node *np)
> {
> struct clk *ostm_clk = NULL;
> struct ostm_device *ostm;
> + struct resource res;
> unsigned long rate;
> unsigned int irq;
> int ret;
> @@ -195,22 +195,35 @@ static int __init ostm_init(struct device_node *np)
> if (!ostm)
> return -ENOMEM;
>
> - ostm->base = of_iomap(np, 0);
> - if (!ostm->base) {
> + ret = of_address_to_resource(np, 0, );
> + if (ret) {
> + pr_err("ostm: Failed to obtain I/O memory\n");
> + goto err_free;
> + }
> +
> + ostm->name = kasprintf(GFP_KERNEL, "ostm %llx.%s",
> +(unsigned long long)res.start, np->name);
I'm not sure, but looking at printk-formats.rst it seems that
%pa[p] as a format specifier for resource_size_t. If so it may
allow dropping the cast above.
> + if (!ostm->name) {
> ret = -ENOMEM;
> goto err_free;
> }
>
> + ostm->base = ioremap(res.start, resource_size());
> + if (!ostm->base) {
> + ret = -ENOMEM;
> + goto err_free_name;
> + }
> +
> irq = irq_of_parse_and_map(np, 0);
> if (!irq) {
> - pr_err("ostm: Failed to get irq\n");
> + pr_err("%s: Failed to get irq\n", ostm->name);
> ret = -EINVAL;
> goto err_unmap;
> }
>
> ostm_clk = of_clk_get(np, 0);
> if (IS_ERR(ostm_clk)) {
> - pr_err("ostm: Failed to get clock\n");
> + pr_err("%s: Failed to get clock\n", ostm->name);
> ostm_clk = NULL;
> ret = PTR_ERR(ostm_clk);
> goto err_unmap;
> @@ -218,7 +231,7 @@ static int __init ostm_init(struct device_node *np)
>
> ret = clk_prepare_enable(ostm_clk);
> if (ret) {
> - pr_err("ostm: Failed to
Re: [PATCH v3 1/3] clocksource/drivers/renesas-ostm: Use DIV_ROUND_CLOSEST() helper
On Wed, Aug 07, 2019 at 10:46:33AM +0200, Geert Uytterhoeven wrote: > Use the DIV_ROUND_CLOSEST() helper instead of open-coding the same > operation. > > Signed-off-by: Geert Uytterhoeven Reviewed-by: Simon Horman > --- > v3: > - New. > --- > drivers/clocksource/renesas-ostm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clocksource/renesas-ostm.c > b/drivers/clocksource/renesas-ostm.c > index 61d5f3b539ce23df..37c39b901bb12b38 100644 > --- a/drivers/clocksource/renesas-ostm.c > +++ b/drivers/clocksource/renesas-ostm.c > @@ -221,7 +221,7 @@ static int __init ostm_init(struct device_node *np) > } > > rate = clk_get_rate(ostm_clk); > - ostm->ticks_per_jiffy = (rate + HZ / 2) / HZ; > + ostm->ticks_per_jiffy = DIV_ROUND_CLOSEST(rate, HZ); > > /* >* First probed device will be used as system clocksource. Any > -- > 2.17.1 >
Re: [PATCH] MAINTAINERS: Add Geert as Renesas SoC Co-Maintainer
On Mon, Jul 29, 2019 at 07:56:58PM +0200, Geert Uytterhoeven wrote: > At the end of the v5.3 upstream kernel development cycle, Simon will be > stepping down from his role as Renesas SoC maintainer. Starting with > the v5.4 development cycle, Geert is taking over this role. > > Add Geert as a co-maintainer, and add his git repository and branch. > > Signed-off-by: Geert Uytterhoeven > --- > Stephen: Can you please add my branch to linux-next, after Simon's >branch, which may still receive fixes for v5.3? > >Thanks! > --- > MAINTAINERS | 4 > 1 file changed, 4 insertions(+) For the record: Acked-by: Simon Horman > diff --git a/MAINTAINERS b/MAINTAINERS > index 6426db5198f05377..6de667021591fb52 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2155,10 +2155,12 @@ F: > Documentation/devicetree/bindings/arm/realtek.txt > > ARM/RENESAS ARM64 ARCHITECTURE > M: Simon Horman > +M: Geert Uytterhoeven > M: Magnus Damm > L: linux-renesas-...@vger.kernel.org > Q: http://patchwork.kernel.org/project/linux-renesas-soc/list/ > T: git git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git next > +T: git > git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git next > S: Supported > F: arch/arm64/boot/dts/renesas/ > F: Documentation/devicetree/bindings/arm/renesas.yaml > @@ -2269,10 +2271,12 @@ F:drivers/media/platform/s5p-mfc/ > > ARM/SHMOBILE ARM ARCHITECTURE > M: Simon Horman > +M: Geert Uytterhoeven > M: Magnus Damm > L: linux-renesas-...@vger.kernel.org > Q: http://patchwork.kernel.org/project/linux-renesas-soc/list/ > T: git git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git next > +T: git > git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git next > S: Supported > F: arch/arm/boot/dts/emev2* > F: arch/arm/boot/dts/gr-peach* > -- > 2.17.1 >
Re: [PATCH 7/7] clocksource/drivers/sh_cmt: Document "cmt-48" as deprecated
On Thu, Jul 18, 2019 at 08:45:38PM +0900, Magnus Damm wrote: > From: Magnus Damm > > Update the CMT driver to mark "renesas,cmt-48" as deprecated. > > Instead of documenting a theoretical hardware device based on current software > support level, define DT bindings top-down based on available data sheet > information and make use of part numbers in the DT compat string. > > In case of the only in-tree users r8a7740 and sh73a0 the compat strings > "renesas,r8a7740-cmt1" and "renesas,sh73a0-cmt1" may be used instead. > > Signed-off-by: Magnus Damm Reviewed-by: Simon Horman
Re: [PATCH 5/7] dt-bindings: timer: renesas, cmt: Update R-Car Gen3 CMT1 usage
On Thu, Jul 18, 2019 at 08:45:11PM +0900, Magnus Damm wrote: > From: Magnus Damm > > The R-Car Gen3 SoCs so far come with a total for 4 on-chip CMT devices: > - CMT0 > - CMT1 > - CMT2 > - CMT3 > > CMT0 includes two rather basic 32-bit timer channels. The rest of the on-chip > CMT devices support 48-bit counters and have 8 channels each. > > Based on the data sheet information "CMT2/3 are exactly same as CMT1" > it seems that CMT2 and CMT3 now use the CMT1 compat string in the DTSI. > > Clarify this in the DT binding documentation by describing R-Car Gen3 and > RZ/G2 CMT1 as "48-bit CMT devices". > > Signed-off-by: Magnus Damm > Reviewed-by: Geert Uytterhoeven > Reviewed-by: Rob Herring Reviewed-by: Simon Horman
Re: [PATCH 2/7] dt-bindings: timer: renesas, cmt: Update CMT1 on sh73a0 and r8a7740
On Thu, Jul 18, 2019 at 08:44:25PM +0900, Magnus Damm wrote: > From: Magnus Damm > > This patch reworks the DT binding documentation for the 6-channel > 48-bit CMTs known as CMT1 on r8a7740 and sh73a0. > > After the update the same style of DT binding as the rest of the upstream > SoCs will now also be used by r8a7740 and sh73a0. The DT binding "cmt-48" > is removed from the DT binding documentation, however software support for > this deprecated binding will still remain in the CMT driver for some time. > > Signed-off-by: Magnus Damm > Reviewed-by: Rob Herring Reviewed-by: Simon Horman
Re: [PATCH 3/7] dt-bindings: timer: renesas, cmt: Add CMT0 and CMT1 to r8a7792
On Thu, Jul 18, 2019 at 08:44:39PM +0900, Magnus Damm wrote: > From: Magnus Damm > > This patch adds DT binding documentation for the CMT devices on > the R-Car Gen2 V2H (r8a7792) SoC. > > Signed-off-by: Magnus Damm > Reviewed-by: Geert Uytterhoeven > Reviewed-by: Rob Herring Reviewed-by: Simon Horman
Re: [PATCH 4/7] dt-bindings: timer: renesas, cmt: Add CMT0 and CMT1 to r8a77995
On Thu, Jul 18, 2019 at 08:44:53PM +0900, Magnus Damm wrote: > From: Magnus Damm > > This patch adds DT binding documentation for the CMT devices on > the R-Car Gen3 D3 (r8a77995) SoC. > > Signed-off-by: Magnus Damm > Reviewed-by: Geert Uytterhoeven > Reviewed-by: Rob Herring Reviewed-by: Simon Horman
Re: [PATCH 1/7] dt-bindings: timer: renesas, cmt: Add CMT0234 to sh73a0 and r8a7740
On Thu, Jul 18, 2019 at 08:44:12PM +0900, Magnus Damm wrote: > From: Magnus Damm > > Document the on-chip CMT devices included in r8a7740 and sh73a0. > > Included in this patch is DT binding documentation for 32-bit CMTs > CMT0, CMT2, CMT3 and CMT4. They all contain a single channel and are > quite similar however some minor differences still exist: > - "Counter input clock" (clock input and on-device divider) > One example is that RCLK 1/1 is supported by CMT2, CMT3 and CMT4. > - "Wakeup request" (supported by CMT0 and CMT2) > > Because of this one unique compat string per CMT device is selected. > > Signed-off-by: Magnus Damm > Reviewed-by: Rob Herring Reviewed-by: Simon Horman
Re: [PATCH 6/7] clocksource/drivers/sh_cmt: r8a7740 and sh73a0 SoC-specific match
On Thu, Jul 18, 2019 at 08:45:24PM +0900, Magnus Damm wrote:
> From: Magnus Damm
>
> Add SoC-specific matching for CMT1 on r8a7740 and sh73a0.
>
> This allows us to move away from the old DT bindings such as
> - "renesas,cmt-48-sh73a0"
> - "renesas,cmt-48-r8a7740"
> - "renesas,cmt-48"
> in favour for the now commonly used format "renesas,-"
>
> Signed-off-by: Magnus Damm
Reviewed-by: Simon Horman
> ---
>
> drivers/clocksource/sh_cmt.c |8
> 1 file changed, 8 insertions(+)
>
> --- 0001/drivers/clocksource/sh_cmt.c
> +++ work/drivers/clocksource/sh_cmt.c 2019-07-18 19:29:06.005414716 +0900
> @@ -928,6 +928,14 @@ static const struct of_device_id sh_cmt_
> .data = _cmt_info[SH_CMT0_RCAR_GEN2]
> },
> {
> + .compatible = "renesas,r8a7740-cmt1",
> + .data = _cmt_info[SH_CMT_48BIT]
Perhaps as a follow-up SH_CMT_48BIT could be renamed.
> + },
> + {
> + .compatible = "renesas,sh73a0-cmt1",
> + .data = _cmt_info[SH_CMT_48BIT]
> + },
> + {
> .compatible = "renesas,rcar-gen2-cmt0",
> .data = _cmt_info[SH_CMT0_RCAR_GEN2]
> },
>
Re: [PATCH] ipvs: remove unnecessary space
On Wed, Jul 10, 2019 at 03:45:52PM +0800, yangxingwu wrote: > this patch removes the extra space. > > Signed-off-by: yangxingwu Thanks, this looks good to me. Acked-by: Simon Horman Pablo, please consider including this in nf-next. > --- > net/netfilter/ipvs/ip_vs_mh.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_mh.c b/net/netfilter/ipvs/ip_vs_mh.c > index 94d9d34..98e358e 100644 > --- a/net/netfilter/ipvs/ip_vs_mh.c > +++ b/net/netfilter/ipvs/ip_vs_mh.c > @@ -174,8 +174,8 @@ static int ip_vs_mh_populate(struct ip_vs_mh_state *s, > return 0; > } > > - table = kcalloc(BITS_TO_LONGS(IP_VS_MH_TAB_SIZE), > - sizeof(unsigned long), GFP_KERNEL); > + table = kcalloc(BITS_TO_LONGS(IP_VS_MH_TAB_SIZE), > + sizeof(unsigned long), GFP_KERNEL); > if (!table) > return -ENOMEM; > > -- > 1.8.3.1 >
Re: [PATCH] ipvs: Delete some unused space characters in Kconfig
On Sun, Jul 07, 2019 at 12:16:49PM +0800, xianfengting...@163.com wrote: > From: Hu Haowen > > The space characters at the end of lines are always unused and > not easy to find. This patch deleted some of them I have found > in Kconfig. > > Signed-off-by: Hu Haowen > --- > > This is my first patch to the Linux kernel, so please forgive > me if anything went wrong. Acked-by: Simon Horman Thanks Hu, this looks good to me. Pablo, please consider this for inclusion in nf-next. > > net/netfilter/ipvs/Kconfig | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/net/netfilter/ipvs/Kconfig b/net/netfilter/ipvs/Kconfig > index f6f1a0d..54afad5 100644 > --- a/net/netfilter/ipvs/Kconfig > +++ b/net/netfilter/ipvs/Kconfig > @@ -120,7 +120,7 @@ configIP_VS_RR > > If you want to compile it in kernel, say Y. To compile it as a > module, choose M here. If unsure, say N. > - > + > config IP_VS_WRR > tristate "weighted round-robin scheduling" > ---help--- > @@ -138,7 +138,7 @@ configIP_VS_LC > tristate "least-connection scheduling" > ---help--- > The least-connection scheduling algorithm directs network > - connections to the server with the least number of active > + connections to the server with the least number of active > connections. > > If you want to compile it in kernel, say Y. To compile it as a > @@ -193,7 +193,7 @@ config IP_VS_LBLCR > tristate "locality-based least-connection with replication scheduling" > ---help--- > The locality-based least-connection with replication scheduling > - algorithm is also for destination IP load balancing. It is > + algorithm is also for destination IP load balancing. It is > usually used in cache cluster. It differs from the LBLC scheduling > as follows: the load balancer maintains mappings from a target > to a set of server nodes that can serve the target. Requests for > @@ -250,8 +250,8 @@ configIP_VS_SED > tristate "shortest expected delay scheduling" > ---help--- > The shortest expected delay scheduling algorithm assigns network > - connections to the server with the shortest expected delay. The > - expected delay that the job will experience is (Ci + 1) / Ui if > + connections to the server with the shortest expected delay. The > + expected delay that the job will experience is (Ci + 1) / Ui if > sent to the ith server, in which Ci is the number of connections > on the ith server and Ui is the fixed service rate (weight) > of the ith server. > -- > 2.7.4 > >
Re: [PATCH v2] dt-binding: mmc: rename tmio_mmc.txt to renesas,sdhi.txt
On Mon, Jun 24, 2019 at 04:03:45PM +0900, Masahiro Yamada wrote:
> As commit b6147490e6aa ("mmc: tmio: split core functionality, DMA and
> MFD glue") said, these MMC controllers use the IP from Panasonic.
>
> TMIO (Toshiba Mobile IO) MMC was the first upstreamed user of this IP.
> The common driver code was split and expanded as 'tmio-mmc-core', then
> it became historical misnomer since 'tmio' is not the name of this IP.
>
> In the discussion [1], we decide to keep this name as-is at least in
> Linux driver level because renaming everything is a big churn.
>
> However, DT should not be oriented to a particular project even though
> it is mainly developed in Linux communities.
>
> This is the misfortune only in Linux. Let's stop exporting it to other
> projects, where there is no good reason to call this hardware "TMIO".
> Rename the file to renesas,sdhi.txt. In fact, all the information in
> this file is specific to the Renesas platform.
>
> This commit also removes the first paragraph entirely. The DT-binding
> should describe the hardware. It is strange to talk about Linux driver
> internals such as how the drivers are probed, how platform data are
> handed off, etc.
>
> [1] https://www.spinics.net/lists/linux-mmc/msg46952.html
>
> Signed-off-by: Masahiro Yamada
Reviewed-by: Simon Horman
> ---
>
> Changes in v2:
> - Rename to renesas,sdhi.txt instead of renesas_sdhi.txt
>
> .../bindings/mmc/{tmio_mmc.txt => renesas,sdhi.txt} | 11 +--
> 1 file changed, 1 insertion(+), 10 deletions(-)
> rename Documentation/devicetree/bindings/mmc/{tmio_mmc.txt =>
> renesas,sdhi.txt} (87%)
>
> diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> b/Documentation/devicetree/bindings/mmc/renesas,sdhi.txt
> similarity index 87%
> rename from Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> rename to Documentation/devicetree/bindings/mmc/renesas,sdhi.txt
> index 2b4f17ca9087..dd08d038a65c 100644
> --- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/renesas,sdhi.txt
> @@ -1,13 +1,4 @@
> -* Toshiba Mobile IO SD/MMC controller
> -
> -The tmio-mmc driver doesn't probe its devices actively, instead its binding
> to
> -devices is managed by either MFD drivers or by the sh_mobile_sdhi platform
> -driver. Those drivers supply the tmio-mmc driver with platform data, that
> either
> -describe hardware capabilities, known to them, or are obtained by them from
> -their own platform data or from their DT information. In the latter case all
> -compulsory and any optional properties, common to all SD/MMC drivers, as
> -described in mmc.txt, can be used. Additionally the following
> tmio_mmc-specific
> -optional bindings can be used.
> +* Renesas SDHI SD/MMC controller
>
> Required properties:
> - compatible: should contain one or more of the following:
> --
> 2.17.1
>
Re: [PATCH] mmc: remove another TMIO MMC variant usdhi6rol0.c
On Fri, Jun 21, 2019 at 03:05:11PM +0900, Masahiro Yamada wrote:
> Renesas upstreamed two different drivers for (almost) the same hardware.
> usdhi6rol0.c is (what we call) "TMIO MMC", which I am 100% sure from the
> the register macros in usdhi6rol0.c.
>
> As commit b6147490e6aa ("mmc: tmio: split core functionality, DMA and
> MFD glue") said, the MMC controllers called tmio_mmc uses the IP from
> Panasonic. ('TMIO MMC' is a historical misnomer)
>
> The macros in usdhi6rol0.c exactly match to the original datasheet
> published by Panasonic. (except the 'USDHI6_' prefix, of course).
> I formerly worked for Panasonic, and Socionext was split out from
> Panasonic. I can get access to the IP datasheet.
>
> [The register comparison]
>
> tmio_mmc.h usdhi6rol0.c
>
> 0x00 CTL_SD_CMD 0x000 USDHI6_SD_CMD
> 0x004 USDHI6_SD_PORT_SEL
> 0x04 CTL_ARG_REG0x008 USDHI6_SD_ARG
> 0x08 CTL_STOP_INTERNAL_ACTION 0x010 USDHI6_SD_STOP
> 0x0a CTL_XFER_BLK_COUNT 0x014 USDHI6_SD_SECCNT
> 0x0c CTL_RESPONSE 0x018 USDHI6_SD_RSP10
> 0x020 USDHI6_SD_RSP32
> 0x028 USDHI6_SD_RSP54
> 0x030 USDHI6_SD_RSP76
> 0x1c CTL_STATUS 0x038 USDHI6_SD_INFO1
> 0x03c USDHI6_SD_INFO2
> 0x20 CTL_IRQ_MASK 0x040 USDHI6_SD_INFO1_MASK
> 0x044 USDHI6_SD_INFO2_MASK
> 0x24 CTL_SD_CARD_CLK_CTL0x048 USDHI6_SD_CLK_CTRL
> 0x26 CTL_SD_XFER_LEN0x04c USDHI6_SD_SIZE
> 0x28 CTL_SD_MEM_CARD_OPT0x050 USDHI6_SD_OPTION
> 0x2c CTL_SD_ERROR_DETAIL_STATUS 0x058 USDHI6_SD_ERR_STS1
> 0x05c USDHI6_SD_ERR_STS2
> 0x30 CTL_SD_DATA_PORT 0x060 USDHI6_SD_BUF0
> 0x34 CTL_TRANSACTION_CTL0x068 USDHI6_SDIO_MODE
> 0x36 CTL_SDIO_STATUS0x06c USDHI6_SDIO_INFO1
> 0x38 CTL_SDIO_IRQ_MASK 0x070 USDHI6_SDIO_INFO1_MASK
> 0xd8 CTL_DMA_ENABLE 0x1b0 USDHI6_CC_EXT_MODE
> 0xe0 CTL_RESET_SD 0x1c0 USDHI6_SOFT_RESET
> 0xe2 CTL_VERSION0x1c4 USDHI6_VERSION
> 0x1c8 USDHI6_HOST_MODE
> 0xe6 CTL_SDIF_MODE 0x1cc USDHI6_SDIF_MODE
>
> The offsets for tmio_mmc.h are half of those of usdhi6rol0.c because
> tmio_mmc was originally written for 16-bit bus platforms. The register
> stride is adjusted by ->bus_shift for modern SoCs.
>
> The register names for usdhi6rol0.c are taken from the IP datasheet
> (with USDHI6_ prefixed). On the other hand, tmio_mmc largely renamed
> the registers.
>
> You may think some registers are missing on the tmio_mmc side.
> Actually, the registers exists. For example, tmio_mmc merged
> 'SD_INFO1' and 'SD_INFO2' into the single macro 'CTL_STATUS', then
> get access to it by the crappy helper, sd_ctrl_write32_as_16_and_16().
>
> As a summary, maintaining two drivers for the same hardware leads to
> maintenance nightmare.
>
> The naming and the code quality for TMIO is unfortunate, but we cannot
> kill it since it is widely used. On the other hand, usdhi6rol0.c just
> supports a single platform. In fact, there is no DT in upstream for
> "renesas,usdhi6rol0":
I am fine with removing this driver on the basis that it is not used
upstream. I agree it would be good to get an Ack from @renesas.com.
>
> $ git grep renesas,usdhi6rol0
> Documentation/devicetree/bindings/mmc/usdhi6rol0.txt:
> "renesas,usdhi6rol0"
> Documentation/devicetree/bindings/mmc/usdhi6rol0.txt: compatible =
> "renesas,usdhi6rol0";
> drivers/mmc/host/usdhi6rol0.c: {.compatible = "renesas,usdhi6rol0"},
>
> Delete this driver now. Please re-implement it based on tmio_mmc_core.c
> if needed.
>
> Perhaps, some code snippets in this driver might be useful for cleaning
> tmio_mmc. It will stay in git history forever, and you can dig for it
> whenever you need it.
>
> Signed-off-by: Masahiro Yamada
...
Re: [PATCH] dt-binding: mmc: rename tmio_mmc.txt to renesas_sdhi.txt
On Fri, Jun 21, 2019 at 12:50:10PM +0900, Masahiro Yamada wrote:
> As commit b6147490e6aa ("mmc: tmio: split core functionality, DMA and
> MFD glue") said, these MMC controllers use the IP from Panasonic.
>
> TMIO (Toshiba Mobile IO) MMC was the first upstreamed user of this IP.
> The common driver code was split and expanded as 'tmio-mmc-core', then
> it become historical misnomer since 'tmio' is not the name of this IP
> in the first place.
>
> In the discussion [1], we decide to keep calling these MMC variants
> 'TMIO MMC' at least in Linux driver level because renaming all of them
> is a big churn.
>
> However, DT should not be oriented to a particular project even though
> it is developed in Linux communities.
>
> Let's stop exporting this unfortunate things to other projects, where
> there is no good reason to call this "TMIO". Rename the file to
> renesas_sdhi.txt. In fact, all the information in this file is specific
> to the Renesas platform.
>
> This commit also removes the first paragraph entirely. The DT-binding
> should describe the hardware. It is really strange to talk about Linux
> driver internals such as how the drivers are probed, how platform data
> are handed off, etc.
>
> [1] https://www.spinics.net/lists/linux-mmc/msg46952.html
>
> Signed-off-by: Masahiro Yamada
> ---
>
> I sent this before, but it was dismissed somehow.
> I am resending this.
Hi Yamada-san,
I for one am fine with this change.
My minor nit is that I think that renesas,sdhi.txt would be a slightly
better filename in terms of consistency with other renesas bindings
documentation filenames.
>
>
> .../bindings/mmc/{tmio_mmc.txt => renesas_sdhi.txt} | 11 +--
> 1 file changed, 1 insertion(+), 10 deletions(-)
> rename Documentation/devicetree/bindings/mmc/{tmio_mmc.txt =>
> renesas_sdhi.txt} (87%)
>
> diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> b/Documentation/devicetree/bindings/mmc/renesas_sdhi.txt
> similarity index 87%
> rename from Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> rename to Documentation/devicetree/bindings/mmc/renesas_sdhi.txt
> index 2b4f17ca9087..dd08d038a65c 100644
> --- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/renesas_sdhi.txt
> @@ -1,13 +1,4 @@
> -* Toshiba Mobile IO SD/MMC controller
> -
> -The tmio-mmc driver doesn't probe its devices actively, instead its binding
> to
> -devices is managed by either MFD drivers or by the sh_mobile_sdhi platform
> -driver. Those drivers supply the tmio-mmc driver with platform data, that
> either
> -describe hardware capabilities, known to them, or are obtained by them from
> -their own platform data or from their DT information. In the latter case all
> -compulsory and any optional properties, common to all SD/MMC drivers, as
> -described in mmc.txt, can be used. Additionally the following
> tmio_mmc-specific
> -optional bindings can be used.
> +* Renesas SDHI SD/MMC controller
>
> Required properties:
> - compatible: should contain one or more of the following:
> --
> 2.17.1
>
Re: [PATCH repost] dt-bindings: timer: renesas, cmt: Document r8a779{5|65|90} CMT support
On Thu, Jun 13, 2019 at 01:09:08PM +0200, Daniel Lezcano wrote: > On 13/06/2019 12:12, Simon Horman wrote: > > On Thu, May 09, 2019 at 02:29:49PM +0200, Simon Horman wrote: > >> From: Cao Van Dong > >> > >> Document SoC specific bindings for R-Car H3/M3-N/E3 SoCs. > > > > Hi Daniel and Thomas, > > > > would you object to me taking this patch through the renesas tree. > > It has been outstanding for some time now. > > Acked-by: Daniel Lezcano Thanks, applied to the renesas tree for inclusion in v5.3.
Re: [PATCH repost 0/5] Repost CAN and CANFD dt-bindings
On Wed, Jun 12, 2019 at 09:49:08AM -0700, David Miller wrote: > From: Simon Horman > Date: Wed, 12 Jun 2019 14:20:20 +0200 > > > are you comfortable with me taking these patches > > through the renesas tree? Or perhaps should they be reposted > > to you for inclusion in net-next? > > > > They have been stuck for a long time now. > > They can go through the renesas tree, no problem. > > Acked-by: David S. Miller Thanks Dave, I have applied these to the renesas tree for inclusion in v5.3.
Re: [PATCH v3] phy: renesas: rcar-gen3-usb2: fix imbalance powered flag
On Mon, Jun 10, 2019 at 03:23:55PM +0900, Yoshihiro Shimoda wrote:
> The powered flag should be set for any other phys anyway. Also
> the flag should be locked by the channel. Otherwise, after we have
> revised the device tree for the usb phy, the following warning
> happened during a second system suspend. And if the driver doesn't
> lock the flag, an imbalance is possible when enabling the regulator
> during system resume. So, this patch fixes the issues.
>
> < The warning >
> [ 56.026531] unbalanced disables for USB20_VBUS0
> [ 56.031108] WARNING: CPU: 3 PID: 513 at drivers/regulator/core.c:2593
> _regula
> tor_disable+0xe0/0x1c0
> [ 56.040146] Modules linked in: rcar_du_drm rcar_lvds drm_kms_helper drm
> drm_p
> anel_orientation_quirks vsp1 videobuf2_vmalloc videobuf2_dma_contig
> videobuf2_me
> mops videobuf2_v4l2 videobuf2_common videodev snd_soc_rcar renesas_usbhs
> snd_soc
> _audio_graph_card media snd_soc_simple_card_utils crct10dif_ce renesas_usb3
> snd_
> soc_ak4613 rcar_fcp pwm_rcar usb_dmac phy_rcar_gen3_usb3 pwm_bl ipv6
> [ 56.074047] CPU: 3 PID: 513 Comm: kworker/u16:19 Not tainted
> 5.2.0-rc3-1-
> g5f20a19 #6
> [ 56.082129] Hardware name: Renesas Salvator-X board based on r8a7795
> ES2.0+ (
> DT)
> [ 56.089524] Workqueue: events_unbound async_run_entry_fn
> [ 56.094832] pstate: 4005 (nZcv daif -PAN -UAO)
> [ 56.099617] pc : _regulator_disable+0xe0/0x1c0
> [ 56.104054] lr : _regulator_disable+0xe0/0x1c0
> [ 56.108489] sp : 121c3ae0
> [ 56.111796] x29: 121c3ae0 x28:
> [ 56.117102] x27: x26: 10fe0e60
> [ 56.122407] x25: 0002 x24: 0001
> [ 56.127712] x23: 0002 x22: 8006f99d4000
> [ 56.133017] x21: 8006f99cc000 x20: 8006f9846800
> [ 56.138322] x19: 8006f9846800 x18:
> [ 56.143626] x17: x16:
> [ 56.148931] x15: 112f96c8 x14: 921c37f7
> [ 56.154235] x13: 121c3805 x12: 11312000
> [ 56.159540] x11: 05f5e0ff x10: 112f9f20
> [ 56.164844] x9 : 112d3018 x8 : 01ad
> [ 56.170149] x7 : ffcc x6 : 8006ff768180
> [ 56.175453] x5 : 8006ff768180 x4 :
> [ 56.180758] x3 : 8006ff76ef10 x2 : 8006ff768180
> [ 56.186062] x1 : 3d2eccbaead8fb00 x0 :
> [ 56.191367] Call trace:
> [ 56.193808] _regulator_disable+0xe0/0x1c0
> [ 56.197899] regulator_disable+0x40/0x78
> [ 56.201820] rcar_gen3_phy_usb2_power_off+0x3c/0x50
> [ 56.206692] phy_power_off+0x48/0xd8
> [ 56.210263] usb_phy_roothub_power_off+0x30/0x50
> [ 56.214873] usb_phy_roothub_suspend+0x1c/0x50
> [ 56.219311] hcd_bus_suspend+0x13c/0x168
> [ 56.223226] generic_suspend+0x4c/0x58
> [ 56.226969] usb_suspend_both+0x1ac/0x238
> [ 56.230972] usb_suspend+0xcc/0x170
> [ 56.234455] usb_dev_suspend+0x10/0x18
> [ 56.238199] dpm_run_callback.isra.6+0x20/0x68
> [ 56.242635] __device_suspend+0x110/0x308
> [ 56.246637] async_suspend+0x24/0xa8
> [ 56.250205] async_run_entry_fn+0x40/0xf8
> [ 56.254210] process_one_work+0x1e0/0x320
> [ 56.258211] worker_thread+0x40/0x450
> [ 56.261867] kthread+0x124/0x128
> [ 56.265094] ret_from_fork+0x10/0x18
> [ 56.268661] ---[ end trace 86d7ec5de5c517af ]---
> [ 56.273290] phy phy-ee080200.usb-phy.10: phy poweroff failed --> -5
>
> Reported-by: Geert Uytterhoeven
> Fixes: 549b6b55b005 ("phy: renesas: rcar-gen3-usb2: enable/disable
> independent irqs")
> Signed-off-by: Yoshihiro Shimoda
> Reviewed-by: Geert Uytterhoeven
> Tested-by: Geert Uytterhoeven
Reviewed-by: Simon Horman
Re: [PATCH 05/34] leds: leds-tca6507: simplify getting the adapter of a client
On Sat, Jun 08, 2019 at 12:55:44PM +0200, Wolfram Sang wrote: > We have a dedicated pointer for that, so use it. Much easier to read and > less computation involved. > > Signed-off-by: Wolfram Sang Reviewed-by: Simon Horman > --- > > Please apply to your subsystem tree. > > drivers/leds/leds-tca6507.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c > index c59035e157d1..58be20cae183 100644 > --- a/drivers/leds/leds-tca6507.c > +++ b/drivers/leds/leds-tca6507.c > @@ -758,7 +758,7 @@ static int tca6507_probe(struct i2c_client *client, > int err; > int i = 0; > > - adapter = to_i2c_adapter(client->dev.parent); > + adapter = client->adapter; > pdata = dev_get_platdata(>dev); > > if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) > -- > 2.19.1 >
Re: [PATCH 01/34] clk: clk-cdce706: simplify getting the adapter of a client
On Sat, Jun 08, 2019 at 12:55:40PM +0200, Wolfram Sang wrote:
> We have a dedicated pointer for that, so use it. Much easier to read and
> less computation involved.
>
> Signed-off-by: Wolfram Sang
Reviewed-by: Simon Horman
> ---
>
> Please apply to your subsystem tree.
>
> drivers/clk/clk-cdce706.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-cdce706.c b/drivers/clk/clk-cdce706.c
> index f21d9092564f..476d29c013e5 100644
> --- a/drivers/clk/clk-cdce706.c
> +++ b/drivers/clk/clk-cdce706.c
> @@ -633,7 +633,7 @@ of_clk_cdce_get(struct of_phandle_args *clkspec, void
> *data)
> static int cdce706_probe(struct i2c_client *client,
>const struct i2c_device_id *id)
> {
> - struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> + struct i2c_adapter *adapter = client->adapter;
> struct cdce706_dev_data *cdce;
> int ret;
>
> --
> 2.19.1
>
Re: [PATCH 10/34] media: i2c: ov2640: simplify getting the adapter of a client
On Sat, Jun 08, 2019 at 12:55:49PM +0200, Wolfram Sang wrote:
> We have a dedicated pointer for that, so use it. Much easier to read and
> less computation involved.
>
> Signed-off-by: Wolfram Sang
Reviewed-by: Simon Horman
> ---
>
> Please apply to your subsystem tree.
>
> drivers/media/i2c/ov2640.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> index 83031cfc7914..30e7e6b2b293 100644
> --- a/drivers/media/i2c/ov2640.c
> +++ b/drivers/media/i2c/ov2640.c
> @@ -1197,7 +1197,7 @@ static int ov2640_probe(struct i2c_client *client,
> const struct i2c_device_id *did)
> {
> struct ov2640_priv *priv;
> - struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> + struct i2c_adapter *adapter = client->adapter;
> int ret;
>
> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> --
> 2.19.1
>
Re: [PATCH 07/34] media: i2c: mt9m001: simplify getting the adapter of a client
On Sat, Jun 08, 2019 at 12:55:46PM +0200, Wolfram Sang wrote: > We have a dedicated pointer for that, so use it. Much easier to read and > less computation involved. > > Signed-off-by: Wolfram Sang Reviewed-by: Simon Horman
Re: [PATCH 08/34] media: i2c: mt9m111: simplify getting the adapter of a client
On Sat, Jun 08, 2019 at 12:55:47PM +0200, Wolfram Sang wrote:
> We have a dedicated pointer for that, so use it. Much easier to read and
> less computation involved.
>
> Signed-off-by: Wolfram Sang
Reviewed-by: Simon Horman
> ---
>
> Please apply to your subsystem tree.
>
> drivers/media/i2c/mt9m111.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 5168bb5880c4..a9da43316504 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -1235,7 +1235,7 @@ static int mt9m111_probe(struct i2c_client *client,
>const struct i2c_device_id *did)
> {
> struct mt9m111 *mt9m111;
> - struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> + struct i2c_adapter *adapter = client->adapter;
> int ret;
>
> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> --
> 2.19.1
>
Re: [PATCH 06/34] media: i2c: ak881x: simplify getting the adapter of a client
On Sat, Jun 08, 2019 at 12:55:45PM +0200, Wolfram Sang wrote:
> We have a dedicated pointer for that, so use it. Much easier to read and
> less computation involved.
>
> Signed-off-by: Wolfram Sang
Reviewed-by: Simon Horman
> ---
>
> Please apply to your subsystem tree.
>
> drivers/media/i2c/ak881x.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ak881x.c b/drivers/media/i2c/ak881x.c
> index 30f9db1351b9..09860603da64 100644
> --- a/drivers/media/i2c/ak881x.c
> +++ b/drivers/media/i2c/ak881x.c
> @@ -232,7 +232,7 @@ static const struct v4l2_subdev_ops ak881x_subdev_ops = {
> static int ak881x_probe(struct i2c_client *client,
> const struct i2c_device_id *did)
> {
> - struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> + struct i2c_adapter *adapter = client->adapter;
> struct ak881x *ak881x;
> u8 ifmode, data;
>
> --
> 2.19.1
>
