* Re: [RFC PATCH 00/11] bpf, trace, dtrace: DTrace BPF program type implementation and sample use
From: Kris Van Hees @ 2019-06-18 1:25 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kris Van Hees, netdev, bpf, dtrace-devel, linux-kernel, rostedt,
mhiramat, acme, ast, daniel, peterz
In-Reply-To: <20190523202842.ij2quhpmem3nabii@ast-mbp.dhcp.thefacebook.com>
On Thu, May 23, 2019 at 01:28:44PM -0700, Alexei Starovoitov wrote:
<< stuff skipped because it is not relevant to the technical discussion... >>
> > > In particular you brought up a good point that there is a use case
> > > for sharing a piece of bpf program between kprobe and tracepoint events.
> > > The better way to do that is via bpf2bpf call.
> > > Example:
> > > void bpf_subprog(arbitrary args)
> > > {
> > > }
> > >
> > > SEC("kprobe/__set_task_comm")
> > > int bpf_prog_kprobe(struct pt_regs *ctx)
> > > {
> > > bpf_subprog(...);
> > > }
> > >
> > > SEC("tracepoint/sched/sched_switch")
> > > int bpf_prog_tracepoint(struct sched_switch_args *ctx)
> > > {
> > > bpf_subprog(...);
> > > }
> > >
> > > Such configuration is not supported by the verifier yet.
> > > We've been discussing it for some time, but no work has started,
> > > since there was no concrete use case.
> > > If you can work on adding support for it everyone will benefit.
> > >
> > > Could you please consider doing that as a step forward?
> >
> > This definitely looks to be an interesting addition and I am happy to look into
> > that further. I have a few questions that I hope you can shed light on...
> >
> > 1. What context would bpf_subprog execute with? If it can be called from
> > multiple different prog types, would it see whichever context the caller
> > is executing with? Or would you envision bpf_subprog to not be allowed to
> > access the execution context because it cannot know which one is in use?
>
> bpf_subprog() won't be able to access 'ctx' pointer _if_ it's ambiguous.
> The verifier already smart enough to track all the data flow, so it's fine to
> pass 'struct pt_regs *ctx' as long as it's accessed safely.
> For example:
> void bpf_subprog(int kind, struct pt_regs *ctx1, struct sched_switch_args *ctx2)
> {
> if (kind == 1)
> bpf_printk("%d", ctx1->pc);
> if (kind == 2)
> bpf_printk("%d", ctx2->next_pid);
> }
>
> SEC("kprobe/__set_task_comm")
> int bpf_prog_kprobe(struct pt_regs *ctx)
> {
> bpf_subprog(1, ctx, NULL);
> }
>
> SEC("tracepoint/sched/sched_switch")
> int bpf_prog_tracepoint(struct sched_switch_args *ctx)
> {
> bpf_subprog(2, NULL, ctx);
> }
>
> The verifier should be able to prove that the above is correct.
> It can do so already if s/ctx1/map_value1/, s/ctx2/map_value2/
> What's missing is an ability to have more than one 'starting' or 'root caller'
> program.
>
> Now replace SEC("tracepoint/sched/sched_switch") with SEC("cgroup/ingress")
> and it's becoming clear that BPF_PROG_TYPE_PROBE approach is not good enough, right?
> Folks are already sharing the bpf progs between kprobe and networking.
> Currently it's done via code duplication and actual sharing happens via maps.
> That's not ideal, hence we've been discussing 'shared library' approach for
> quite some time. We need a way to support common bpf functions that can be called
> from networking and from tracing programs.
>
> > 2. Given that BPF programs are loaded with a specification of the prog type,
> > how would one load a code construct as the one you outline above? How can
> > you load a BPF function and have it be used as subprog from programs that
> > are loaded separately? I.e. in the sample above, if bpf_subprog is loaded
> > as part of loading bpf_prog_kprobe (prog type KPROBE), how can it be
> > referenced from bpf_prog_tracepoint (prog type TRACEPOINT) which would be
> > loaded separately?
>
> The api to support shared libraries was discussed, but not yet implemented.
> We've discussed 'FD + name' approach.
> FD identifies a loaded program (which is root program + a set of subprogs)
> and other programs can be loaded at any time later. The BPF_CALL instructions
> in such later program would refer to older subprogs via FD + name.
> Note that both tracing and networking progs can be part of single elf file.
> libbpf has to be smart to load progs into kernel step by step
> and reusing subprogs that are already loaded.
>
> Note that libbpf work for such feature can begin _without_ kernel changes.
> libbpf can pass bpf_prog_kprobe+bpf_subprog as a single program first,
> then pass bpf_prog_tracepoint+bpf_subprog second (as a separate program).
> The bpf_subprog will be duplicated and JITed twice, but sharing will happen
> because data structures (maps, global and static data) will be shared.
> This way the support for 'pseudo shared libraries' can begin.
> (later accompanied by FD+name kernel support)
As far as I can determine, the current libbpd implementation is already able
to do the duplication of the called function, even when the ELF object contains
programs of differemt program types. I.e. the example you give at the top
of the email actually seems to work already. Right?
In that case, I am a bit unsure what more can be done on the side of libbpf
without needing kernel changes?
> There are other things we discsused. Ideally the body of bpf_subprog()
> wouldn't need to be kept around for future verification when this bpf
> function is called by a different program. The idea was to
> use BTF and similar mechanism to ongoing 'bounded loop' work.
> So the verifier can analyze bpf_subprog() once and reuse that knowledge
> for dynamic linking with progs that will be loaded later.
> This is more long term work.
> A simple short term would be to verify the full call chain every time
> the subprog (bpf function) is reused.
^ permalink raw reply
* Re: [RFC PATCH 00/11] bpf, trace, dtrace: DTrace BPF program type implementation and sample use
From: Alexei Starovoitov @ 2019-06-18 1:32 UTC (permalink / raw)
To: Kris Van Hees
Cc: Network Development, bpf, dtrace-devel, LKML, Steven Rostedt,
Masami Hiramatsu, Arnaldo Carvalho de Melo, Alexei Starovoitov,
Daniel Borkmann, Peter Zijlstra
In-Reply-To: <20190618012509.GF8794@oracle.com>
On Mon, Jun 17, 2019 at 6:25 PM Kris Van Hees <kris.van.hees@oracle.com> wrote:
>
> On Thu, May 23, 2019 at 01:28:44PM -0700, Alexei Starovoitov wrote:
>
> << stuff skipped because it is not relevant to the technical discussion... >>
>
> > > > In particular you brought up a good point that there is a use case
> > > > for sharing a piece of bpf program between kprobe and tracepoint events.
> > > > The better way to do that is via bpf2bpf call.
> > > > Example:
> > > > void bpf_subprog(arbitrary args)
> > > > {
> > > > }
> > > >
> > > > SEC("kprobe/__set_task_comm")
> > > > int bpf_prog_kprobe(struct pt_regs *ctx)
> > > > {
> > > > bpf_subprog(...);
> > > > }
> > > >
> > > > SEC("tracepoint/sched/sched_switch")
> > > > int bpf_prog_tracepoint(struct sched_switch_args *ctx)
> > > > {
> > > > bpf_subprog(...);
> > > > }
> > > >
> > > > Such configuration is not supported by the verifier yet.
> > > > We've been discussing it for some time, but no work has started,
> > > > since there was no concrete use case.
> > > > If you can work on adding support for it everyone will benefit.
> > > >
> > > > Could you please consider doing that as a step forward?
> > >
> > > This definitely looks to be an interesting addition and I am happy to look into
> > > that further. I have a few questions that I hope you can shed light on...
> > >
> > > 1. What context would bpf_subprog execute with? If it can be called from
> > > multiple different prog types, would it see whichever context the caller
> > > is executing with? Or would you envision bpf_subprog to not be allowed to
> > > access the execution context because it cannot know which one is in use?
> >
> > bpf_subprog() won't be able to access 'ctx' pointer _if_ it's ambiguous.
> > The verifier already smart enough to track all the data flow, so it's fine to
> > pass 'struct pt_regs *ctx' as long as it's accessed safely.
> > For example:
> > void bpf_subprog(int kind, struct pt_regs *ctx1, struct sched_switch_args *ctx2)
> > {
> > if (kind == 1)
> > bpf_printk("%d", ctx1->pc);
> > if (kind == 2)
> > bpf_printk("%d", ctx2->next_pid);
> > }
> >
> > SEC("kprobe/__set_task_comm")
> > int bpf_prog_kprobe(struct pt_regs *ctx)
> > {
> > bpf_subprog(1, ctx, NULL);
> > }
> >
> > SEC("tracepoint/sched/sched_switch")
> > int bpf_prog_tracepoint(struct sched_switch_args *ctx)
> > {
> > bpf_subprog(2, NULL, ctx);
> > }
> >
> > The verifier should be able to prove that the above is correct.
> > It can do so already if s/ctx1/map_value1/, s/ctx2/map_value2/
> > What's missing is an ability to have more than one 'starting' or 'root caller'
> > program.
> >
> > Now replace SEC("tracepoint/sched/sched_switch") with SEC("cgroup/ingress")
> > and it's becoming clear that BPF_PROG_TYPE_PROBE approach is not good enough, right?
> > Folks are already sharing the bpf progs between kprobe and networking.
> > Currently it's done via code duplication and actual sharing happens via maps.
> > That's not ideal, hence we've been discussing 'shared library' approach for
> > quite some time. We need a way to support common bpf functions that can be called
> > from networking and from tracing programs.
> >
> > > 2. Given that BPF programs are loaded with a specification of the prog type,
> > > how would one load a code construct as the one you outline above? How can
> > > you load a BPF function and have it be used as subprog from programs that
> > > are loaded separately? I.e. in the sample above, if bpf_subprog is loaded
> > > as part of loading bpf_prog_kprobe (prog type KPROBE), how can it be
> > > referenced from bpf_prog_tracepoint (prog type TRACEPOINT) which would be
> > > loaded separately?
> >
> > The api to support shared libraries was discussed, but not yet implemented.
> > We've discussed 'FD + name' approach.
> > FD identifies a loaded program (which is root program + a set of subprogs)
> > and other programs can be loaded at any time later. The BPF_CALL instructions
> > in such later program would refer to older subprogs via FD + name.
> > Note that both tracing and networking progs can be part of single elf file.
> > libbpf has to be smart to load progs into kernel step by step
> > and reusing subprogs that are already loaded.
> >
> > Note that libbpf work for such feature can begin _without_ kernel changes.
> > libbpf can pass bpf_prog_kprobe+bpf_subprog as a single program first,
> > then pass bpf_prog_tracepoint+bpf_subprog second (as a separate program).
> > The bpf_subprog will be duplicated and JITed twice, but sharing will happen
> > because data structures (maps, global and static data) will be shared.
> > This way the support for 'pseudo shared libraries' can begin.
> > (later accompanied by FD+name kernel support)
>
> As far as I can determine, the current libbpd implementation is already able
> to do the duplication of the called function, even when the ELF object contains
> programs of differemt program types. I.e. the example you give at the top
> of the email actually seems to work already. Right?
Have you tried it?
> In that case, I am a bit unsure what more can be done on the side of libbpf
> without needing kernel changes?
it's a bit weird to discuss hypothetical kernel changes when the first step
of changing libbpf wasn't even attempted.
^ permalink raw reply
* arm32 build failure after 992aa864dca068554802a65a467a2640985cc213
From: Nathan Chancellor @ 2019-06-18 1:46 UTC (permalink / raw)
To: Shalom Toledo, Ido Schimmel, Jiri Pirko
Cc: Petr Machata, David S. Miller, netdev, linux-kernel
Hi all,
A 32-bit ARM allyesconfig fails to link after commit 992aa864dca0
("mlxsw: spectrum_ptp: Add implementation for physical hardware clock
operations") because of 64-bit division:
arm-linux-gnueabi-ld:
drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.o: in function
`mlxsw_sp1_ptp_phc_settime':
spectrum_ptp.c:(.text+0x39c): undefined reference to `__aeabi_uldivmod'
The following diff fixes it but I have no idea if it is proper or not
(hence reaching out before sending it, in case one of you has a more
proper idea).
Cheers,
Nathan
---
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
index 2a9bbc90225e..65686f0b6834 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
@@ -87,7 +87,7 @@ mlxsw_sp1_ptp_phc_settime(struct mlxsw_sp_ptp_clock *clock, u64 nsec)
u32 next_sec;
int err;
- next_sec = nsec / NSEC_PER_SEC + 1;
+ next_sec = (u32)div64_u64(nsec, NSEC_PER_SEC + 1);
next_sec_in_nsec = next_sec * NSEC_PER_SEC;
spin_lock(&clock->lock);
^ permalink raw reply related
* Re: [PATCH net-next 0/2] net: mediatek: Add MT7621 TRGMII mode support
From: Andrew Lunn @ 2019-06-18 1:53 UTC (permalink / raw)
To: René van Dorst
Cc: Sean Wang, Florian Fainelli, David S . Miller, Matthias Brugger,
Vivien Didelot, netdev, john, linux-mediatek, linux-mips
In-Reply-To: <20190617232004.Horde.mAVymZdeb9Jjf29W2PeOggU@www.vdorst.com>
> By adding some extra speed states in the code it seems to work.
>
> + if (state->speed == 1200)
> + mcr |= PMCR_FORCE_SPEED_1000;
Hi René
Is TRGMII always 1.2G? Or can you set it to 1000 or 1200? This
PMCR_FORCE_SPEED_1000 feels wrong.
> >We could consider adding 1200BaseT/Full?
>
> I don't have any opinion about this.
> It is great that it shows nicely in ethtool but I think supporting more
> speeds in phy_speed_to_str() is enough.
>
> Also you may want to add other SOCs trgmii ranges too:
> - 1200BaseT/Full for mt7621 only
> - 2000BaseT/Full for mt7623 and mt7683
> - 2600BaseT/Full for mt7623 only
Are these standardised in any way? Or MTK proprietary? Also, is the T
in BaseT correct? These speeds work over copper cables? Or should we
be talking about 1200BaseKX?
Thanks
Andrew
^ permalink raw reply
* Re: [RFC PATCH 00/11] bpf, trace, dtrace: DTrace BPF program type implementation and sample use
From: Kris Van Hees @ 2019-06-18 1:54 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kris Van Hees, Network Development, bpf, dtrace-devel, LKML,
Steven Rostedt, Masami Hiramatsu, Arnaldo Carvalho de Melo,
Alexei Starovoitov, Daniel Borkmann, Peter Zijlstra
In-Reply-To: <CAADnVQJoH4WOQ0t7ZhLgh4kh2obxkFs0UGDRas0y4QSqh1EMsg@mail.gmail.com>
On Mon, Jun 17, 2019 at 06:32:22PM -0700, Alexei Starovoitov wrote:
> On Mon, Jun 17, 2019 at 6:25 PM Kris Van Hees <kris.van.hees@oracle.com> wrote:
> >
> > On Thu, May 23, 2019 at 01:28:44PM -0700, Alexei Starovoitov wrote:
> >
> > << stuff skipped because it is not relevant to the technical discussion... >>
> >
> > > > > In particular you brought up a good point that there is a use case
> > > > > for sharing a piece of bpf program between kprobe and tracepoint events.
> > > > > The better way to do that is via bpf2bpf call.
> > > > > Example:
> > > > > void bpf_subprog(arbitrary args)
> > > > > {
> > > > > }
> > > > >
> > > > > SEC("kprobe/__set_task_comm")
> > > > > int bpf_prog_kprobe(struct pt_regs *ctx)
> > > > > {
> > > > > bpf_subprog(...);
> > > > > }
> > > > >
> > > > > SEC("tracepoint/sched/sched_switch")
> > > > > int bpf_prog_tracepoint(struct sched_switch_args *ctx)
> > > > > {
> > > > > bpf_subprog(...);
> > > > > }
> > > > >
> > > > > Such configuration is not supported by the verifier yet.
> > > > > We've been discussing it for some time, but no work has started,
> > > > > since there was no concrete use case.
> > > > > If you can work on adding support for it everyone will benefit.
> > > > >
> > > > > Could you please consider doing that as a step forward?
> > > >
> > > > This definitely looks to be an interesting addition and I am happy to look into
> > > > that further. I have a few questions that I hope you can shed light on...
> > > >
> > > > 1. What context would bpf_subprog execute with? If it can be called from
> > > > multiple different prog types, would it see whichever context the caller
> > > > is executing with? Or would you envision bpf_subprog to not be allowed to
> > > > access the execution context because it cannot know which one is in use?
> > >
> > > bpf_subprog() won't be able to access 'ctx' pointer _if_ it's ambiguous.
> > > The verifier already smart enough to track all the data flow, so it's fine to
> > > pass 'struct pt_regs *ctx' as long as it's accessed safely.
> > > For example:
> > > void bpf_subprog(int kind, struct pt_regs *ctx1, struct sched_switch_args *ctx2)
> > > {
> > > if (kind == 1)
> > > bpf_printk("%d", ctx1->pc);
> > > if (kind == 2)
> > > bpf_printk("%d", ctx2->next_pid);
> > > }
> > >
> > > SEC("kprobe/__set_task_comm")
> > > int bpf_prog_kprobe(struct pt_regs *ctx)
> > > {
> > > bpf_subprog(1, ctx, NULL);
> > > }
> > >
> > > SEC("tracepoint/sched/sched_switch")
> > > int bpf_prog_tracepoint(struct sched_switch_args *ctx)
> > > {
> > > bpf_subprog(2, NULL, ctx);
> > > }
> > >
> > > The verifier should be able to prove that the above is correct.
> > > It can do so already if s/ctx1/map_value1/, s/ctx2/map_value2/
> > > What's missing is an ability to have more than one 'starting' or 'root caller'
> > > program.
> > >
> > > Now replace SEC("tracepoint/sched/sched_switch") with SEC("cgroup/ingress")
> > > and it's becoming clear that BPF_PROG_TYPE_PROBE approach is not good enough, right?
> > > Folks are already sharing the bpf progs between kprobe and networking.
> > > Currently it's done via code duplication and actual sharing happens via maps.
> > > That's not ideal, hence we've been discussing 'shared library' approach for
> > > quite some time. We need a way to support common bpf functions that can be called
> > > from networking and from tracing programs.
> > >
> > > > 2. Given that BPF programs are loaded with a specification of the prog type,
> > > > how would one load a code construct as the one you outline above? How can
> > > > you load a BPF function and have it be used as subprog from programs that
> > > > are loaded separately? I.e. in the sample above, if bpf_subprog is loaded
> > > > as part of loading bpf_prog_kprobe (prog type KPROBE), how can it be
> > > > referenced from bpf_prog_tracepoint (prog type TRACEPOINT) which would be
> > > > loaded separately?
> > >
> > > The api to support shared libraries was discussed, but not yet implemented.
> > > We've discussed 'FD + name' approach.
> > > FD identifies a loaded program (which is root program + a set of subprogs)
> > > and other programs can be loaded at any time later. The BPF_CALL instructions
> > > in such later program would refer to older subprogs via FD + name.
> > > Note that both tracing and networking progs can be part of single elf file.
> > > libbpf has to be smart to load progs into kernel step by step
> > > and reusing subprogs that are already loaded.
> > >
> > > Note that libbpf work for such feature can begin _without_ kernel changes.
> > > libbpf can pass bpf_prog_kprobe+bpf_subprog as a single program first,
> > > then pass bpf_prog_tracepoint+bpf_subprog second (as a separate program).
> > > The bpf_subprog will be duplicated and JITed twice, but sharing will happen
> > > because data structures (maps, global and static data) will be shared.
> > > This way the support for 'pseudo shared libraries' can begin.
> > > (later accompanied by FD+name kernel support)
> >
> > As far as I can determine, the current libbpd implementation is already able
> > to do the duplication of the called function, even when the ELF object contains
> > programs of differemt program types. I.e. the example you give at the top
> > of the email actually seems to work already. Right?
>
> Have you tried it?
Yes, of course. I wouldn't want to make an unfounded claim.
> > In that case, I am a bit unsure what more can be done on the side of libbpf
> > without needing kernel changes?
>
> it's a bit weird to discuss hypothetical kernel changes when the first step
> of changing libbpf wasn't even attempted.
It is not hypothetical. The folowing example works fine:
static int noinline bpf_action(void *ctx, long fd, long buf, long count)
{
int cpu = bpf_get_smp_processor_id();
struct data {
u64 arg0;
u64 arg1;
u64 arg2;
} rec;
memset(&rec, 0, sizeof(rec));
rec.arg0 = fd;
rec.arg1 = buf;
rec.arg2 = count;
bpf_perf_event_output(ctx, &buffers, cpu, &rec, sizeof(rec));
return 0;
}
SEC("kprobe/ksys_write")
int bpf_kprobe(struct pt_regs *ctx)
{
return bpf_action(ctx, ctx->di, ctx->si, ctx->dx);
}
SEC("tracepoint/syscalls/sys_enter_write")
int bpf_tp(struct syscalls_enter_write_args *ctx)
{
return bpf_action(ctx, ctx->fd, ctx->buf, ctx->count);
}
char _license[] SEC("license") = "GPL";
u32 _version SEC("version") = LINUX_VERSION_CODE;
^ permalink raw reply
* Re: [PATCH net-next 0/2] net: mediatek: Add MT7621 TRGMII mode support
From: Florian Fainelli @ 2019-06-18 2:21 UTC (permalink / raw)
To: Andrew Lunn, René van Dorst
Cc: Sean Wang, David S . Miller, Matthias Brugger, Vivien Didelot,
netdev, john, linux-mediatek, linux-mips
In-Reply-To: <20190618015309.GA18088@lunn.ch>
On 6/17/2019 6:53 PM, Andrew Lunn wrote:
>> By adding some extra speed states in the code it seems to work.
>>
>> + if (state->speed == 1200)
>> + mcr |= PMCR_FORCE_SPEED_1000;
>
> Hi René
>
> Is TRGMII always 1.2G? Or can you set it to 1000 or 1200? This
> PMCR_FORCE_SPEED_1000 feels wrong.
It is not uncommon to have to "force" 1G to get a higher speed, there is
something similar with B53 switches configuring the CPU ports at 2GB/sec
(proprietary too and not standardized either).
>
>>> We could consider adding 1200BaseT/Full?
>>
>> I don't have any opinion about this.
>> It is great that it shows nicely in ethtool but I think supporting more
>> speeds in phy_speed_to_str() is enough.
>>
>> Also you may want to add other SOCs trgmii ranges too:
>> - 1200BaseT/Full for mt7621 only
>> - 2000BaseT/Full for mt7623 and mt7683
>> - 2600BaseT/Full for mt7623 only
>
> Are these standardised in any way? Or MTK proprietary? Also, is the T
> in BaseT correct? These speeds work over copper cables? Or should we
> be talking about 1200BaseKX?
Looks like this is MTK proprietary:
http://lists.infradead.org/pipermail/linux-mediatek/2016-September/007083.html
https://patchwork.kernel.org/patch/9341129/
--
Florian
^ permalink raw reply
* Re: [PATCH net 2/4] tcp: tcp_fragment() should apply sane memory limits
From: Eric Dumazet @ 2019-06-18 2:28 UTC (permalink / raw)
To: Christoph Paasch, Eric Dumazet
Cc: David S . Miller, netdev, Greg Kroah-Hartman, Jonathan Looney,
Neal Cardwell, Tyler Hicks, Yuchung Cheng, Bruce Curtis,
Jonathan Lemon, Dustin Marquess
In-Reply-To: <CALMXkpYVRxgeqarp4gnmX7GqYh1sWOAt6UaRFqYBOaaNFfZ5sw@mail.gmail.com>
On 6/17/19 5:18 PM, Christoph Paasch wrote:
>
> Hi Eric, I now have a packetdrill test that started failing (see
> below). Admittedly, a bit weird test with the SO_SNDBUF forced so low.
>
> Nevertheless, previously this test would pass, now it stalls after the
> write() because tcp_fragment() returns -ENOMEM. Your commit-message
> mentions that this could trigger when one sets SO_SNDBUF low. But,
> here we have a complete stall of the connection and we never recover.
>
> I don't know if we care about this, but there it is :-)
I guess it is WAI :)
Honestly I am not sure we want to add code just to allow these degenerated use cases.
Upstream kernels could check if rtx queue is empty or not, but this check will be not trivial to backport
Can you test :
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 00c01a01b547ec67c971dc25a74c9258563cf871..06576540133806222f43d4a9532c5a929a2965b0 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1296,7 +1296,8 @@ int tcp_fragment(struct sock *sk, enum tcp_queue tcp_queue,
if (nsize < 0)
nsize = 0;
- if (unlikely((sk->sk_wmem_queued >> 1) > sk->sk_sndbuf)) {
+ if (unlikely((sk->sk_wmem_queued >> 1) > sk->sk_sndbuf &&
+ !tcp_rtx_queue_empty(sk))) {
NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPWQUEUETOOBIG);
return -ENOMEM;
}
^ permalink raw reply related
* Re: [EXT] [PATCH net-next 06/16] qlge: Remove useless dma synchronization calls
From: Benjamin Poirier @ 2019-06-18 2:51 UTC (permalink / raw)
To: Manish Chopra; +Cc: GR-Linux-NIC-Dev, netdev@vger.kernel.org
In-Reply-To: <BYAPR18MB2696CEF6D42DAE1E467582ABABEB0@BYAPR18MB2696.namprd18.prod.outlook.com>
On 2019/06/17 09:44, Manish Chopra wrote:
[...]
> > --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> > +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> > @@ -1110,9 +1110,6 @@ static void ql_update_lbq(struct ql_adapter *qdev,
> > struct rx_ring *rx_ring)
> > dma_unmap_addr_set(lbq_desc, mapaddr, map);
> > *lbq_desc->addr = cpu_to_le64(map);
> >
> > - pci_dma_sync_single_for_device(qdev->pdev, map,
> > - qdev->lbq_buf_size,
> > - PCI_DMA_FROMDEVICE);
> > clean_idx++;
> > if (clean_idx == rx_ring->lbq_len)
> > clean_idx = 0;
> > @@ -1598,10 +1595,6 @@ static void ql_process_mac_rx_skb(struct
> > ql_adapter *qdev,
> >
> > skb_put_data(new_skb, skb->data, length);
> >
> > - pci_dma_sync_single_for_device(qdev->pdev,
> > - dma_unmap_addr(sbq_desc, mapaddr),
> > - SMALL_BUF_MAP_SIZE,
> > - PCI_DMA_FROMDEVICE);
>
> This was introduced in commit 2c9a266afefe ("qlge: Fix receive packets drop").
> So hoping that it is fine, the buffer shouldn't be synced for the device back after the synced for CPU call in context of any ownership etc. ?
No, dma_sync_*_for_cpu() and dma_sync_*_for_device() calls don't have to
be paired; they are not like lock acquire and release calls.
In the cases the current patch is concerned with, the cpu does not write
any data for the device in the rx buffers. Therefore, there is no need
for those pci_dma_sync_single_for_device() calls.
^ permalink raw reply
* Re: [RFC PATCH 00/11] bpf, trace, dtrace: DTrace BPF program type implementation and sample use
From: Alexei Starovoitov @ 2019-06-18 3:01 UTC (permalink / raw)
To: Kris Van Hees
Cc: Network Development, bpf, dtrace-devel, LKML, Steven Rostedt,
Masami Hiramatsu, Arnaldo Carvalho de Melo, Alexei Starovoitov,
Daniel Borkmann, Peter Zijlstra
In-Reply-To: <20190618015442.GG8794@oracle.com>
On Mon, Jun 17, 2019 at 6:54 PM Kris Van Hees <kris.van.hees@oracle.com> wrote:
>
> It is not hypothetical. The folowing example works fine:
>
> static int noinline bpf_action(void *ctx, long fd, long buf, long count)
> {
> int cpu = bpf_get_smp_processor_id();
> struct data {
> u64 arg0;
> u64 arg1;
> u64 arg2;
> } rec;
>
> memset(&rec, 0, sizeof(rec));
>
> rec.arg0 = fd;
> rec.arg1 = buf;
> rec.arg2 = count;
>
> bpf_perf_event_output(ctx, &buffers, cpu, &rec, sizeof(rec));
>
> return 0;
> }
>
> SEC("kprobe/ksys_write")
> int bpf_kprobe(struct pt_regs *ctx)
> {
> return bpf_action(ctx, ctx->di, ctx->si, ctx->dx);
> }
>
> SEC("tracepoint/syscalls/sys_enter_write")
> int bpf_tp(struct syscalls_enter_write_args *ctx)
> {
> return bpf_action(ctx, ctx->fd, ctx->buf, ctx->count);
> }
>
> char _license[] SEC("license") = "GPL";
> u32 _version SEC("version") = LINUX_VERSION_CODE;
Great. Then you're all set to proceed with user space dtrace tooling, right?
What you'll discover thought that it works only for simplest things
like above. libbpf assumes that everything in single elf will be used
and passes the whole thing to the kernel.
The verifer removes dead code only from single program.
It disallows unused functions. Hence libbpf needs to start doing
more "linker work" than it does today.
When it loads .o it needs to pass to the kernel only the functions
that are used by the program.
This work should be straightforward to implement.
Unfortunately no one had time to do it.
It's also going to be the first step to multi-elf support.
libbpf would need to do the same "linker work" across .o-s.
^ permalink raw reply
* Re: [PATCH net 2/4] tcp: tcp_fragment() should apply sane memory limits
From: Christoph Paasch @ 2019-06-18 3:19 UTC (permalink / raw)
To: Eric Dumazet
Cc: Eric Dumazet, David S . Miller, netdev, Greg Kroah-Hartman,
Jonathan Looney, Neal Cardwell, Tyler Hicks, Yuchung Cheng,
Bruce Curtis, Jonathan Lemon, Dustin Marquess
In-Reply-To: <03cbcfdf-58a4-dbca-45b1-8b17f229fa1d@gmail.com>
On Mon, Jun 17, 2019 at 7:28 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 6/17/19 5:18 PM, Christoph Paasch wrote:
> >
> > Hi Eric, I now have a packetdrill test that started failing (see
> > below). Admittedly, a bit weird test with the SO_SNDBUF forced so low.
> >
> > Nevertheless, previously this test would pass, now it stalls after the
> > write() because tcp_fragment() returns -ENOMEM. Your commit-message
> > mentions that this could trigger when one sets SO_SNDBUF low. But,
> > here we have a complete stall of the connection and we never recover.
> >
> > I don't know if we care about this, but there it is :-)
>
> I guess it is WAI :)
>
> Honestly I am not sure we want to add code just to allow these degenerated use cases.
>
> Upstream kernels could check if rtx queue is empty or not, but this check will be not trivial to backport
>
>
> Can you test :
Yes, this does the trick for my packetdrill-test.
I wonder, is there a way we could end up in a situation where we can't
retransmit anymore?
For example, sk_wmem_queued has grown so much that the new test fails.
Then, if we legitimately need to fragment in __tcp_retransmit_skb() we
won't be able to do so. So we will never retransmit. And if no ACK
comes back in to make some room we are stuck, no?
Christoph
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 00c01a01b547ec67c971dc25a74c9258563cf871..06576540133806222f43d4a9532c5a929a2965b0 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1296,7 +1296,8 @@ int tcp_fragment(struct sock *sk, enum tcp_queue tcp_queue,
> if (nsize < 0)
> nsize = 0;
>
> - if (unlikely((sk->sk_wmem_queued >> 1) > sk->sk_sndbuf)) {
> + if (unlikely((sk->sk_wmem_queued >> 1) > sk->sk_sndbuf &&
> + !tcp_rtx_queue_empty(sk))) {
> NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPWQUEUETOOBIG);
> return -ENOMEM;
> }
^ permalink raw reply
* Re: [RFC PATCH 00/11] bpf, trace, dtrace: DTrace BPF program type implementation and sample use
From: Kris Van Hees @ 2019-06-18 3:19 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kris Van Hees, Network Development, bpf, dtrace-devel, LKML,
Steven Rostedt, Masami Hiramatsu, Arnaldo Carvalho de Melo,
Alexei Starovoitov, Daniel Borkmann, Peter Zijlstra
In-Reply-To: <CAADnVQ+zAwoH_mjJLhfEgXHHz+3WYkzhEm-mEObP0koLiSvknw@mail.gmail.com>
On Mon, Jun 17, 2019 at 08:01:52PM -0700, Alexei Starovoitov wrote:
> On Mon, Jun 17, 2019 at 6:54 PM Kris Van Hees <kris.van.hees@oracle.com> wrote:
> >
> > It is not hypothetical. The folowing example works fine:
> >
> > static int noinline bpf_action(void *ctx, long fd, long buf, long count)
> > {
> > int cpu = bpf_get_smp_processor_id();
> > struct data {
> > u64 arg0;
> > u64 arg1;
> > u64 arg2;
> > } rec;
> >
> > memset(&rec, 0, sizeof(rec));
> >
> > rec.arg0 = fd;
> > rec.arg1 = buf;
> > rec.arg2 = count;
> >
> > bpf_perf_event_output(ctx, &buffers, cpu, &rec, sizeof(rec));
> >
> > return 0;
> > }
> >
> > SEC("kprobe/ksys_write")
> > int bpf_kprobe(struct pt_regs *ctx)
> > {
> > return bpf_action(ctx, ctx->di, ctx->si, ctx->dx);
> > }
> >
> > SEC("tracepoint/syscalls/sys_enter_write")
> > int bpf_tp(struct syscalls_enter_write_args *ctx)
> > {
> > return bpf_action(ctx, ctx->fd, ctx->buf, ctx->count);
> > }
> >
> > char _license[] SEC("license") = "GPL";
> > u32 _version SEC("version") = LINUX_VERSION_CODE;
>
> Great. Then you're all set to proceed with user space dtrace tooling, right?
I can indeed proceed with the initial basics, yes, and have started. I hope
to have a first bare bones patch for review sometime next week.
> What you'll discover thought that it works only for simplest things
> like above. libbpf assumes that everything in single elf will be used
> and passes the whole thing to the kernel.
> The verifer removes dead code only from single program.
> It disallows unused functions. Hence libbpf needs to start doing
> more "linker work" than it does today.
> When it loads .o it needs to pass to the kernel only the functions
> that are used by the program.
> This work should be straightforward to implement.
> Unfortunately no one had time to do it.
Ah yes, I see what you mean. I'll work on that next since I will definitely
be needing that.
> It's also going to be the first step to multi-elf support.
> libbpf would need to do the same "linker work" across .o-s.
^ permalink raw reply
* Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000
From: Paul Walmsley @ 2019-06-18 3:26 UTC (permalink / raw)
To: Alistair Francis
Cc: troy.benjegerdes@sifive.com, jamez@wit.com,
linux-riscv@lists.infradead.org, davem@davemloft.net,
schwab@suse.de, nicolas.ferre@microchip.com, mark.rutland@arm.com,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
aou@eecs.berkeley.edu, sachin.ghadi@sifive.com,
netdev@vger.kernel.org, ynezz@true.cz, palmer@sifive.com,
yash.shah@sifive.com, robh+dt@kernel.org
In-Reply-To: <d2836a90b92f3522a398d57ab8555d08956a0d1f.camel@wdc.com>
[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]
On Mon, 17 Jun 2019, Alistair Francis wrote:
> > The legacy M-mode U-boot handles the phy reset already, and I’ve been
> > able to load upstream S-mode uboot as a payload via TFTP, and then
> > load and boot a 4.19 kernel.
> >
> > It would be nice to get this all working with 5.x, however there are
> > still
> > several missing pieces to really have it work well.
>
> Let me know what is still missing/doesn't work and I can add it. At the
> moment the only known issue I know of is a missing SD card driver in U-
> Boot.
The DT data has changed between the non-upstream data that people
developed against previously, vs. the DT data that just went upstream
here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=72296bde4f4207566872ee355950a59cbc29f852
and
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c35f1b87fc595807ff15d2834d241f9771497205
So Upstream U-Boot is going to need several patches to get things working
again. Clock identifiers and Ethernet are two known areas.
- Paul
^ permalink raw reply
* Re: [PATCH net 2/4] tcp: tcp_fragment() should apply sane memory limits
From: Eric Dumazet @ 2019-06-18 3:44 UTC (permalink / raw)
To: Christoph Paasch, Eric Dumazet
Cc: Eric Dumazet, David S . Miller, netdev, Greg Kroah-Hartman,
Jonathan Looney, Neal Cardwell, Tyler Hicks, Yuchung Cheng,
Bruce Curtis, Jonathan Lemon, Dustin Marquess
In-Reply-To: <CALMXkpZ4isoXpFp_5=nVUcWrt5TofYVhpdAjv7LkCH7RFW1tYw@mail.gmail.com>
On 6/17/19 8:19 PM, Christoph Paasch wrote:
>
> Yes, this does the trick for my packetdrill-test.
>
> I wonder, is there a way we could end up in a situation where we can't
> retransmit anymore?
> For example, sk_wmem_queued has grown so much that the new test fails.
> Then, if we legitimately need to fragment in __tcp_retransmit_skb() we
> won't be able to do so. So we will never retransmit. And if no ACK
> comes back in to make some room we are stuck, no?
Well, RTO will eventually fire.
Really TCP can not work well with tiny sndbuf limits.
There is really no point trying to be nice.
There is precedent in TCP stack where we always allow one packet in RX or TX queue
even with tiny rcv/sndbuf limits (or global memory pressure)
We only need to make sure to allow having at least one packet in rtx queue as well.
^ permalink raw reply
* Re: [PATCH net 2/4] tcp: tcp_fragment() should apply sane memory limits
From: Christoph Paasch @ 2019-06-18 3:53 UTC (permalink / raw)
To: Eric Dumazet
Cc: Eric Dumazet, David S . Miller, netdev, Greg Kroah-Hartman,
Jonathan Looney, Neal Cardwell, Tyler Hicks, Yuchung Cheng,
Bruce Curtis, Jonathan Lemon, Dustin Marquess
In-Reply-To: <aa0af451-5e7c-7d83-ef25-095a67cd23a1@gmail.com>
On Mon, Jun 17, 2019 at 8:44 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 6/17/19 8:19 PM, Christoph Paasch wrote:
> >
> > Yes, this does the trick for my packetdrill-test.
> >
> > I wonder, is there a way we could end up in a situation where we can't
> > retransmit anymore?
> > For example, sk_wmem_queued has grown so much that the new test fails.
> > Then, if we legitimately need to fragment in __tcp_retransmit_skb() we
> > won't be able to do so. So we will never retransmit. And if no ACK
> > comes back in to make some room we are stuck, no?
>
> Well, RTO will eventually fire.
But even the RTO would have to go through __tcp_retransmit_skb(), and
let's say the MTU of the interface changed and thus we need to
fragment. tcp_fragment() would keep on failing then, no? Sure,
eventually we will ETIMEOUT but that's a long way to go.
> Really TCP can not work well with tiny sndbuf limits.
>
> There is really no point trying to be nice.
Sure, fair enough :-)
Christoph
>
> There is precedent in TCP stack where we always allow one packet in RX or TX queue
> even with tiny rcv/sndbuf limits (or global memory pressure)
>
> We only need to make sure to allow having at least one packet in rtx queue as well.
^ permalink raw reply
* Re: [PATCH v2 2/2] macb: Add support for SiFive FU540-C000
From: Yash Shah @ 2019-06-18 4:04 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, devicetree, netdev, linux-kernel, linux-riscv,
Rob Herring, Mark Rutland, Nicolas Ferre, Palmer Dabbelt,
Albert Ou, Paul Walmsley, Petr Štetiar, Sachin Ghadi
In-Reply-To: <20190617155834.GK25211@lunn.ch>
On Mon, Jun 17, 2019 at 9:28 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Jun 17, 2019 at 09:49:27AM +0530, Yash Shah wrote:
...
> > static const struct macb_config at91sam9260_config = {
> > .caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
> > .clk_init = macb_clk_init,
> > @@ -3992,6 +4112,9 @@ static int at91ether_init(struct platform_device *pdev)
> > { .compatible = "cdns,emac", .data = &emac_config },
> > { .compatible = "cdns,zynqmp-gem", .data = &zynqmp_config},
> > { .compatible = "cdns,zynq-gem", .data = &zynq_config },
> > +#ifdef CONFIG_MACB_SIFIVE_FU540
> > + { .compatible = "sifive,fu540-macb", .data = &fu540_c000_config },
> > +#endif
>
> This #ifdef should not be needed.
>
> > { /* sentinel */ }
> > };
> > MODULE_DEVICE_TABLE(of, macb_dt_ids);
> > @@ -4199,6 +4322,9 @@ static int macb_probe(struct platform_device *pdev)
> >
> > err_disable_clocks:
> > clk_disable_unprepare(tx_clk);
> > +#ifdef CONFIG_MACB_SIFIVE_FU540
> > + clk_unregister(tx_clk);
> > +#endif
>
> So long as tx_clk is NULL, you can call clk_unregister(). So please
> remove the #ifdef.
>
>
> > clk_disable_unprepare(hclk);
> > clk_disable_unprepare(pclk);
> > clk_disable_unprepare(rx_clk);
> > @@ -4233,6 +4359,9 @@ static int macb_remove(struct platform_device *pdev)
> > pm_runtime_dont_use_autosuspend(&pdev->dev);
> > if (!pm_runtime_suspended(&pdev->dev)) {
> > clk_disable_unprepare(bp->tx_clk);
> > +#ifdef CONFIG_MACB_SIFIVE_FU540
> > + clk_unregister(bp->tx_clk);
> > +#endif
>
> Same here.
>
> In general try to avoid #ifdef in C code.
Will remove all the #ifdef in v3.
Thanks for your comments.
- Yash
^ permalink raw reply
* Re: [PATCH net 2/4] tcp: tcp_fragment() should apply sane memory limits
From: Eric Dumazet @ 2019-06-18 4:08 UTC (permalink / raw)
To: Christoph Paasch, Eric Dumazet
Cc: Eric Dumazet, David S . Miller, netdev, Greg Kroah-Hartman,
Jonathan Looney, Neal Cardwell, Tyler Hicks, Yuchung Cheng,
Bruce Curtis, Jonathan Lemon, Dustin Marquess
In-Reply-To: <CALMXkpYs8KN0DmXV+37grbS0Y4Q-DAM-_GVZy+qWi2dtV+cDPA@mail.gmail.com>
On 6/17/19 8:53 PM, Christoph Paasch wrote:
> On Mon, Jun 17, 2019 at 8:44 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>
>>
>> On 6/17/19 8:19 PM, Christoph Paasch wrote:
>>>
>>> Yes, this does the trick for my packetdrill-test.
>>>
>>> I wonder, is there a way we could end up in a situation where we can't
>>> retransmit anymore?
>>> For example, sk_wmem_queued has grown so much that the new test fails.
>>> Then, if we legitimately need to fragment in __tcp_retransmit_skb() we
>>> won't be able to do so. So we will never retransmit. And if no ACK
>>> comes back in to make some room we are stuck, no?
>>
>> Well, RTO will eventually fire.
>
> But even the RTO would have to go through __tcp_retransmit_skb(), and
> let's say the MTU of the interface changed and thus we need to
> fragment. tcp_fragment() would keep on failing then, no? Sure,
> eventually we will ETIMEOUT but that's a long way to go.
Also I want to point that normal skb split for not-yet transmitted skbs
does not use tcp_fragment(), with one exception (the one you hit)
Only the first skb in write queue can possibly have payload in skb->head
and might go through tcp_fragment()
Other splits will use tso_fragment() which does not enforce sk_wmem_queued limits (yet)
So things like TLP should work.
^ permalink raw reply
* Re: [PATCH v3] net: ipv4: move tcp_fastopen server side code to SipHash library
From: Eric Biggers @ 2019-06-18 4:14 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: netdev, linux-crypto, herbert, edumazet, davem, kuznet, yoshfuji,
jbaron, cpaasch, David.Laight, ycheng
In-Reply-To: <20190617080933.32152-1-ard.biesheuvel@linaro.org>
On Mon, Jun 17, 2019 at 10:09:33AM +0200, Ard Biesheuvel wrote:
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index c23019a3b264..9ea0e71f5c6a 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -58,12 +58,7 @@ static inline unsigned int tcp_optlen(const struct sk_buff *skb)
>
> /* TCP Fast Open Cookie as stored in memory */
> struct tcp_fastopen_cookie {
> - union {
> - u8 val[TCP_FASTOPEN_COOKIE_MAX];
> -#if IS_ENABLED(CONFIG_IPV6)
> - struct in6_addr addr;
> -#endif
> - };
> + u64 val[TCP_FASTOPEN_COOKIE_MAX / sizeof(u64)];
> s8 len;
> bool exp; /* In RFC6994 experimental option format */
> };
Is it okay that the cookies will depend on CPU endianness?
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 96e0e53ff440..184930b02779 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1628,9 +1628,9 @@ bool tcp_fastopen_defer_connect(struct sock *sk, int *err);
>
> /* Fastopen key context */
> struct tcp_fastopen_context {
> - struct crypto_cipher *tfm[TCP_FASTOPEN_KEY_MAX];
> - __u8 key[TCP_FASTOPEN_KEY_BUF_LENGTH];
> - struct rcu_head rcu;
> + __u8 key[TCP_FASTOPEN_KEY_MAX][TCP_FASTOPEN_KEY_LENGTH];
> + int num;
> + struct rcu_head rcu;
> };
Why not use 'siphash_key_t' here? Then the (potentially alignment-violating)
cast in __tcp_fastopen_cookie_gen_cipher() wouldn't be needed.
> int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
> void *primary_key, void *backup_key,
> unsigned int len)
> @@ -115,11 +75,20 @@ int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
> struct fastopen_queue *q;
> int err = 0;
>
> - ctx = tcp_fastopen_alloc_ctx(primary_key, backup_key, len);
> - if (IS_ERR(ctx)) {
> - err = PTR_ERR(ctx);
> + ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx) {
> + err = -ENOMEM;
> goto out;
> }
> +
> + memcpy(ctx->key[0], primary_key, len);
> + if (backup_key) {
> + memcpy(ctx->key[1], backup_key, len);
> + ctx->num = 2;
> + } else {
> + ctx->num = 1;
> + }
> +
> spin_lock(&net->ipv4.tcp_fastopen_ctx_lock);
> if (sk) {
> q = &inet_csk(sk)->icsk_accept_queue.fastopenq;
Shouldn't there be a check that 'len == TCP_FASTOPEN_KEY_LENGTH'? I see that
all callers pass that, but it seems unnecessarily fragile for this to accept
short lengths and leave uninitialized memory in that case.
- Eric
^ permalink raw reply
* [PATCH V2] can: flexcan: fix stop mode acknowledgment
From: Joakim Zhang @ 2019-06-18 5:02 UTC (permalink / raw)
To: mkl@pengutronix.de, linux-can@vger.kernel.org
Cc: dl-linux-imx, wg@grandegger.com, netdev@vger.kernel.org,
Joakim Zhang
To enter stop mode, the CPU should manually assert a global Stop Mode
request and check the acknowledgment asserted by FlexCAN. The CPU must
only consider the FlexCAN in stop mode when both request and
acknowledgment conditions are satisfied.
Fixes: de3578c198c6 ("can: flexcan: add self wakeup support")
Reported-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
ChangeLog:
V1->V2:
* regmap_read()-->regmap_read_poll_timeout()
---
drivers/net/can/flexcan.c | 35 +++++++++++++++++++++++++++--------
1 file changed, 27 insertions(+), 8 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index e35083ff31ee..43245a5655c7 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -404,9 +404,10 @@ static void flexcan_enable_wakeup_irq(struct flexcan_priv *priv, bool enable)
priv->write(reg_mcr, ®s->mcr);
}
-static inline void flexcan_enter_stop_mode(struct flexcan_priv *priv)
+static inline int flexcan_enter_stop_mode(struct flexcan_priv *priv)
{
struct flexcan_regs __iomem *regs = priv->regs;
+ unsigned int ackval;
u32 reg_mcr;
reg_mcr = priv->read(®s->mcr);
@@ -416,20 +417,37 @@ static inline void flexcan_enter_stop_mode(struct flexcan_priv *priv)
/* enable stop request */
regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
1 << priv->stm.req_bit, 1 << priv->stm.req_bit);
+
+ /* get stop acknowledgment */
+ if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
+ ackval, ackval & (1 << priv->stm.ack_bit),
+ 0, FLEXCAN_TIMEOUT_US))
+ return -ETIMEDOUT;
+
+ return 0;
}
-static inline void flexcan_exit_stop_mode(struct flexcan_priv *priv)
+static inline int flexcan_exit_stop_mode(struct flexcan_priv *priv)
{
struct flexcan_regs __iomem *regs = priv->regs;
+ unsigned int ackval;
u32 reg_mcr;
+ reg_mcr = priv->read(®s->mcr);
+ reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
+ priv->write(reg_mcr, ®s->mcr);
+
/* remove stop request */
regmap_update_bits(priv->stm.gpr, priv->stm.req_gpr,
1 << priv->stm.req_bit, 0);
- reg_mcr = priv->read(®s->mcr);
- reg_mcr &= ~FLEXCAN_MCR_SLF_WAK;
- priv->write(reg_mcr, ®s->mcr);
+ /* get stop acknowledgment */
+ if (regmap_read_poll_timeout(priv->stm.gpr, priv->stm.ack_gpr,
+ ackval, !(ackval & (1 << priv->stm.ack_bit)),
+ 0, FLEXCAN_TIMEOUT_US))
+ return -ETIMEDOUT;
+
+ return 0;
}
static inline void flexcan_error_irq_enable(const struct flexcan_priv *priv)
@@ -1652,7 +1670,7 @@ static int __maybe_unused flexcan_suspend(struct device *device)
*/
if (device_may_wakeup(device)) {
enable_irq_wake(dev->irq);
- flexcan_enter_stop_mode(priv);
+ err = flexcan_enter_stop_mode(priv);
} else {
err = flexcan_chip_disable(priv);
if (err)
@@ -1725,13 +1743,14 @@ static int __maybe_unused flexcan_noirq_resume(struct device *device)
{
struct net_device *dev = dev_get_drvdata(device);
struct flexcan_priv *priv = netdev_priv(dev);
+ int err = 0;
if (netif_running(dev) && device_may_wakeup(device)) {
flexcan_enable_wakeup_irq(priv, false);
- flexcan_exit_stop_mode(priv);
+ err = flexcan_exit_stop_mode(priv);
}
- return 0;
+ return err;
}
static const struct dev_pm_ops flexcan_pm_ops = {
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 2/2] perf trace: Handle NULL pointer dereference in trace__syscall_info()
From: Leo Yan @ 2019-06-18 6:39 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, netdev, bpf
In-Reply-To: <20190617173203.GA23094@kernel.org>
On Mon, Jun 17, 2019 at 02:32:03PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jun 17, 2019 at 05:11:40PM +0800, Leo Yan escreveu:
> > trace__init_bpf_map_syscall_args() invokes trace__syscall_info() to
> > retrieve system calls information, it always passes NULL for 'evsel'
> > argument; when id is an invalid value then the logging will try to
> > output event name, this triggers NULL pointer dereference.
> >
> > This patch directly uses string "unknown" for event name when 'evsel'
> > is NULL pointer.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> > tools/perf/builtin-trace.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index 5cd74651db4c..49dfb2fd393b 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> > @@ -1764,7 +1764,7 @@ static struct syscall *trace__syscall_info(struct trace *trace,
> > static u64 n;
> >
> > pr_debug("Invalid syscall %d id, skipping (%s, %" PRIu64 ")\n",
> > - id, perf_evsel__name(evsel), ++n);
> > + id, evsel ? perf_evsel__name(evsel) : "unknown", ++n);
> > return NULL;
>
> What do you think of this instead?
Yes, I agree the below change is right thing to do. FWIW:
Reviewed-by: Leo Yan <leo.yan@linaro.org>
BTW, my patch followed the code in [1], after apply below your change,
could consider to simplify code in [1] for without checking 'evsel' is
NULL pointer anymore.
Thanks,
Leo
[1] https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/builtin-report.c?h=perf/core#n301
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 68beef8f47ff..1d6af95b9207 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -590,6 +590,9 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
> {
> char bf[128];
>
> + if (!evsel)
> + goto out_unknown;
> +
> if (evsel->name)
> return evsel->name;
>
> @@ -629,7 +632,10 @@ const char *perf_evsel__name(struct perf_evsel *evsel)
>
> evsel->name = strdup(bf);
>
> - return evsel->name ?: "unknown";
> + if (evsel->name)
> + return evsel->name;
> +out_unknown:
> + return "unknown";
> }
>
> const char *perf_evsel__group_name(struct perf_evsel *evsel)
^ permalink raw reply
* Re: [PATCH 1/2] perf trace: Use pr_debug() instead of fprintf() for logging
From: Leo Yan @ 2019-06-18 6:24 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Alexander Shishkin, Jiri Olsa, Namhyung Kim, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, netdev, bpf
In-Reply-To: <20190617152412.GJ1402@kernel.org>
On Mon, Jun 17, 2019 at 12:24:12PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jun 17, 2019 at 05:11:39PM +0800, Leo Yan escreveu:
> > In the function trace__syscall_info(), it explicitly checks verbose
> > level and print out log with fprintf(). Actually, we can use
> > pr_debug() to do the same thing for debug logging.
> >
> > This patch uses pr_debug() instead of fprintf() for debug logging; it
> > includes a minor fixing for 'space before tab in indent', which
> > dismisses git warning when apply it.
>
> But those are not fprintf(stdout,), they explicitely redirect to the
> output file that the user may have specified using 'perf trace --output
> filename.trace' :-)
Thanks for pointing out, sorry for noise. Please drop this patch.
Thanks,
Leo Yan
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> > tools/perf/builtin-trace.c | 21 +++++++++------------
> > 1 file changed, 9 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index bd1f00e7a2eb..5cd74651db4c 100644
> > --- a/tools/perf/builtin-trace.c
> > +++ b/tools/perf/builtin-trace.c
> > @@ -1760,12 +1760,11 @@ static struct syscall *trace__syscall_info(struct trace *trace,
> > * grep "NR -1 " /t/trace_pipe
> > *
> > * After generating some load on the machine.
> > - */
> > - if (verbose > 1) {
> > - static u64 n;
> > - fprintf(trace->output, "Invalid syscall %d id, skipping (%s, %" PRIu64 ") ...\n",
> > - id, perf_evsel__name(evsel), ++n);
> > - }
> > + */
> > + static u64 n;
> > +
> > + pr_debug("Invalid syscall %d id, skipping (%s, %" PRIu64 ")\n",
> > + id, perf_evsel__name(evsel), ++n);
> > return NULL;
> > }
> >
> > @@ -1779,12 +1778,10 @@ static struct syscall *trace__syscall_info(struct trace *trace,
> > return &trace->syscalls.table[id];
> >
> > out_cant_read:
> > - if (verbose > 0) {
> > - fprintf(trace->output, "Problems reading syscall %d", id);
> > - if (id <= trace->syscalls.max && trace->syscalls.table[id].name != NULL)
> > - fprintf(trace->output, "(%s)", trace->syscalls.table[id].name);
> > - fputs(" information\n", trace->output);
> > - }
> > + pr_debug("Problems reading syscall %d", id);
> > + if (id <= trace->syscalls.max && trace->syscalls.table[id].name != NULL)
> > + pr_debug("(%s)", trace->syscalls.table[id].name);
> > + pr_debug(" information\n");
> > return NULL;
> > }
> >
> > --
> > 2.17.1
>
> --
>
> - Arnaldo
^ permalink raw reply
* Re: [PATCH v3] net: ipv4: move tcp_fastopen server side code to SipHash library
From: Ard Biesheuvel @ 2019-06-18 6:56 UTC (permalink / raw)
To: Eric Biggers
Cc: <netdev@vger.kernel.org>,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
Eric Dumazet, David S. Miller, kuznet, yoshfuji, Jason Baron,
cpaasch, David Laight, Yuchung Cheng
In-Reply-To: <20190618041408.GB2266@sol.localdomain>
On Tue, 18 Jun 2019 at 06:14, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Mon, Jun 17, 2019 at 10:09:33AM +0200, Ard Biesheuvel wrote:
> > diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> > index c23019a3b264..9ea0e71f5c6a 100644
> > --- a/include/linux/tcp.h
> > +++ b/include/linux/tcp.h
> > @@ -58,12 +58,7 @@ static inline unsigned int tcp_optlen(const struct sk_buff *skb)
> >
> > /* TCP Fast Open Cookie as stored in memory */
> > struct tcp_fastopen_cookie {
> > - union {
> > - u8 val[TCP_FASTOPEN_COOKIE_MAX];
> > -#if IS_ENABLED(CONFIG_IPV6)
> > - struct in6_addr addr;
> > -#endif
> > - };
> > + u64 val[TCP_FASTOPEN_COOKIE_MAX / sizeof(u64)];
> > s8 len;
> > bool exp; /* In RFC6994 experimental option format */
> > };
>
> Is it okay that the cookies will depend on CPU endianness?
>
That depends on whether keys shared between hosts with different
endiannesses are expected to produce cookies that can be shared.
> > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > index 96e0e53ff440..184930b02779 100644
> > --- a/include/net/tcp.h
> > +++ b/include/net/tcp.h
> > @@ -1628,9 +1628,9 @@ bool tcp_fastopen_defer_connect(struct sock *sk, int *err);
> >
> > /* Fastopen key context */
> > struct tcp_fastopen_context {
> > - struct crypto_cipher *tfm[TCP_FASTOPEN_KEY_MAX];
> > - __u8 key[TCP_FASTOPEN_KEY_BUF_LENGTH];
> > - struct rcu_head rcu;
> > + __u8 key[TCP_FASTOPEN_KEY_MAX][TCP_FASTOPEN_KEY_LENGTH];
> > + int num;
> > + struct rcu_head rcu;
> > };
>
> Why not use 'siphash_key_t' here? Then the (potentially alignment-violating)
> cast in __tcp_fastopen_cookie_gen_cipher() wouldn't be needed.
>
These data structures are always kmalloc'ed so the alignment is never
violated in practice. But I do take your point. My idea at the time of
the first RFC was that the actual MAC algo should be an implementation
detail, and so the key is just a buffer. However, after Eric pointed
out that setting the same key across different hosts should produce
compatible cookies (module the upgrade scenario), it is true that the
algorithm is an externally visible property, so it might be better to
change this into siphash_key_t[] here.
> > int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
> > void *primary_key, void *backup_key,
> > unsigned int len)
> > @@ -115,11 +75,20 @@ int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
> > struct fastopen_queue *q;
> > int err = 0;
> >
> > - ctx = tcp_fastopen_alloc_ctx(primary_key, backup_key, len);
> > - if (IS_ERR(ctx)) {
> > - err = PTR_ERR(ctx);
> > + ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
> > + if (!ctx) {
> > + err = -ENOMEM;
> > goto out;
> > }
> > +
> > + memcpy(ctx->key[0], primary_key, len);
> > + if (backup_key) {
> > + memcpy(ctx->key[1], backup_key, len);
> > + ctx->num = 2;
> > + } else {
> > + ctx->num = 1;
> > + }
> > +
> > spin_lock(&net->ipv4.tcp_fastopen_ctx_lock);
> > if (sk) {
> > q = &inet_csk(sk)->icsk_accept_queue.fastopenq;
>
> Shouldn't there be a check that 'len == TCP_FASTOPEN_KEY_LENGTH'? I see that
> all callers pass that, but it seems unnecessarily fragile for this to accept
> short lengths and leave uninitialized memory in that case.
>
Sure, I can add back the error handling path the previously handled
any errors from crypto_cipher_setkey() [which would perform the input
length checking in that case]
I'll spin an incremental patch covering the above.
^ permalink raw reply
* [PATCH] ipsec: select CRYPTO_HASH for xfrm_algo
From: Arnd Bergmann @ 2019-06-18 7:14 UTC (permalink / raw)
To: Steffen Klassert, Herbert Xu, David S. Miller
Cc: Arnd Bergmann, Florian Westphal, netdev, linux-kernel
kernelci.org reports failed builds on arc because of what looks
like an old missed 'select' statement:
net/xfrm/xfrm_algo.o: In function `xfrm_probe_algs':
xfrm_algo.c:(.text+0x1e8): undefined reference to `crypto_has_ahash'
I don't see this in randconfig builds on other architectures, but
it's fairly clear we want to select the hash code for it, like we
do for all its other users.
Fixes: 17bc19702221 ("ipsec: Use skcipher and ahash when probing algorithms")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
net/xfrm/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/xfrm/Kconfig b/net/xfrm/Kconfig
index c967fc3c38c8..a5c967efe5f4 100644
--- a/net/xfrm/Kconfig
+++ b/net/xfrm/Kconfig
@@ -15,6 +15,7 @@ config XFRM_ALGO
tristate
select XFRM
select CRYPTO
+ select CRYPTO_HASH
if INET
config XFRM_USER
--
2.20.0
^ permalink raw reply related
* Re: Cleanup of -Wunused-const-variable in drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c
From: Maxime Chevallier @ 2019-06-18 6:39 UTC (permalink / raw)
To: Nathan Huckleberry; +Cc: davem, netdev, clang-built-linux
In-Reply-To: <CAJkfWY5ZuDsmV6u1p=DPZF84ijYS3Mu2NeySGgfCXgLGnruu_A@mail.gmail.com>
Hello Nathan,
On Thu, 13 Jun 2019 10:53:05 -0700
Nathan Huckleberry <nhuck@google.com> wrote:
>Hey all,
>
>I'm looking into cleaning up ignored warnings in the kernel so we can
>remove compiler flags to ignore warnings.
>
>There's an unused variable 'mvpp2_dbgfs_prs_pmap_fops' in
>mvpp2_debugfs.c. It looks like this code is for dumping useful
>information into userspace. I'd like to either remove the variable or
>dump it to userspace in the same way the other variables are.
Thanks for reporting this.
The ops should actually be used, fixing the warning should be as simple
as adding this into mvpp2_dbgfs_prs_entry_init :
+ debugfs_create_file("pmap", 0444, prs_entry_dir, entry,
+ &mvpp2_dbgfs_prs_pmap_fops);
+
>Wanted to reach out for opinions on the best course of action before
>submitting a patch.
Can you submit a patch, or do you prefer me to do it ?
Thanks,
Maxime
^ permalink raw reply
* Re: [PATCH ipsec] xfrm: fix sa selector validation
From: Steffen Klassert @ 2019-06-18 7:27 UTC (permalink / raw)
To: Herbert Xu; +Cc: Nicolas Dichtel, davem, netdev, Anirudh Gupta
In-Reply-To: <20190614161148.vti6mhvnxfwweznc@gondor.apana.org.au>
On Sat, Jun 15, 2019 at 12:11:48AM +0800, Herbert Xu wrote:
> On Fri, Jun 14, 2019 at 11:13:55AM +0200, Nicolas Dichtel wrote:
> > After commit b38ff4075a80, the following command does not work anymore:
> > $ ip xfrm state add src 10.125.0.2 dst 10.125.0.1 proto esp spi 34 reqid 1 \
> > mode tunnel enc 'cbc(aes)' 0xb0abdba8b782ad9d364ec81e3a7d82a1 auth-trunc \
> > 'hmac(sha1)' 0xe26609ebd00acb6a4d51fca13e49ea78a72c73e6 96 flag align4
> >
> > In fact, the selector is not mandatory, allow the user to provide an empty
> > selector.
> >
> > Fixes: b38ff4075a80 ("xfrm: Fix xfrm sel prefix length validation")
> > CC: Anirudh Gupta <anirudh.gupta@sophos.com>
> > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Patch applied, thanks everyone!
^ permalink raw reply
* Re: [PATCH net-next 0/2] net: sched: act_ctinfo: fixes
From: Kevin 'ldir' Darbyshire-Bryant @ 2019-06-18 7:33 UTC (permalink / raw)
To: David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <20190617.140106.2136391777805798865.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 743 bytes --]
> On 17 Jun 2019, at 22:01, David Miller <davem@davemloft.net> wrote:
>
> From: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
> Date: Mon, 17 Jun 2019 11:03:25 +0100
>
>>
<snipped>
>> If I ever get to a developer conference please feel free to
>> tar/feather/apply cone of shame.
>
> :-) In kernel networking development we prefer brown paper bags over
> cones of shame, just FYI :) :) :)
LOL - I’ll bear that in mind :-) I thought fixing the code and admitting
my incompetence was the best policy…I’d be found out at some point anyway :-)
>
> Series applied, thanks.
Excellent. Hopefully that will be it.
Cheers,
Kevin D-B
gpg: 012C ACB2 28C6 C53E 9775 9123 B3A2 389B 9DE2 334A
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox