Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH bpf-next] bpf: Allow bpf_jit_enable = 2 with BPF_JIT_ALWAYS_ON config
From: Daniel Borkmann @ 2018-04-25  9:12 UTC (permalink / raw)
  To: Leo Yan, David S. Miller, Alexei Starovoitov, Kirill Tkhai,
	netdev, linux-kernel
In-Reply-To: <1524644322-9263-1-git-send-email-leo.yan@linaro.org>

On 04/25/2018 10:18 AM, Leo Yan wrote:
> After enabled BPF_JIT_ALWAYS_ON config, bpf_jit_enable always equals to
> 1; it is impossible to set 'bpf_jit_enable = 2' and the kernel has no
> chance to call bpf_jit_dump().
> 
> This patch relaxes bpf_jit_enable range to [1..2] when kernel config
> BPF_JIT_ALWAYS_ON is enabled so can invoke jit dump.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Is there a specific reason why you need this here instead of retrieving the
dump from the newer interface available from bpftool (tools/bpf/bpftool/)?
The bpf_jit_enable = 2 is not recommended these days since it dumps into the
kernel log which is often readable from unpriv as well. bpftool makes use
of the BPF_OBJ_GET_INFO_BY_FD interface via bpf syscall to get the JIT dump
instead when bpf_jit_enable is set.

> ---
>  net/core/sysctl_net_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
> index b1a2c5e..6a39b22 100644
> --- a/net/core/sysctl_net_core.c
> +++ b/net/core/sysctl_net_core.c
> @@ -371,7 +371,7 @@ static int proc_dointvec_minmax_bpf_enable(struct ctl_table *table, int write,
>  		.proc_handler	= proc_dointvec_minmax_bpf_enable,
>  # ifdef CONFIG_BPF_JIT_ALWAYS_ON
>  		.extra1		= &one,
> -		.extra2		= &one,
> +		.extra2		= &two,
>  # else
>  		.extra1		= &zero,
>  		.extra2		= &two,
> 

^ permalink raw reply

* Re: [PATCH net-next 3/4] nfp: flower: support offloading multiple rules with same cookie
From: Or Gerlitz @ 2018-04-25  9:13 UTC (permalink / raw)
  To: John Hurley
  Cc: Jakub Kicinski, David Miller, Linux Netdev List, oss-drivers,
	ASAP_Direct_Dev
In-Reply-To: <CAK+XE==18NFnRYDZKhREmbVawbeUHj1W=HeymLzBBHwnXSfOKg@mail.gmail.com>

On Wed, Apr 25, 2018 at 12:02 PM, John Hurley <john.hurley@netronome.com> wrote:
> On Wed, Apr 25, 2018 at 9:56 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>> On Wed, Apr 25, 2018 at 11:51 AM, John Hurley <john.hurley@netronome.com> wrote:
>>> On Wed, Apr 25, 2018 at 7:31 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>>> On Wed, Apr 25, 2018 at 7:17 AM, Jakub Kicinski
>>>> <jakub.kicinski@netronome.com> wrote:
>>>>> From: John Hurley <john.hurley@netronome.com>
>>>>>
>>>>> When multiple netdevs are attached to a tc offload block and register for
>>>>> callbacks, a rule added to the block will be propogated to all netdevs.
>>>>> Previously these were detected as duplicates (based on cookie) and
>>>>> rejected. Modify the rule nfp lookup function to optionally include an
>>>>> ingress netdev and a host context along with the cookie value when
>>>>> searching for a rule. When a new rule is passed to the driver, the netdev
>>>>> the rule is to be attached to is considered when searching for dublicates.
>>>>
>>>> so if the same rule (cookie) is provided to the driver through multiple ingress
>>>> devices you will not reject it -- what is the use case for that, is it
>>>> block sharing?
>>>
>>> Hi Or,
>>> Yes, block sharing is the current use-case.
>>> Simple example for clarity....
>>> Here we want to offload the filter to both ingress devs nfp_0 and nfp_1:
>>>
>>> tc qdisc add dev nfp_p0 ingress_block 22 ingress
>>> tc qdisc add dev nfp_p1 ingress_block 22 ingress
>>> tc filter add block 22 protocol ip parent ffff: flower skip_sw
>>> ip_proto tcp action drop
>>
>> cool!
>>
>> Just out of curiosity, do you actually share this HW rule or you duplicate it?
>
> It's duplicated. At HW level the ingress port is part of the match so technically it's
> a different rule.

I see, we have also a match on the ingress port as part of the HW API, which
means we will have to apply a similar practice if we want to support
block sharing quickly.

Just to make sure, under tc block sharing the tc stack calls for hw
offloading of the
same rule (same cookie) multiple times, each with different ingress
device, right?


Or.

^ permalink raw reply

* Re: [PATCH net-next 4/4] nfp: flower: ignore duplicate cb requests for same rule
From: Or Gerlitz @ 2018-04-25  9:17 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: David Miller, Linux Netdev List, oss-drivers, John Hurley
In-Reply-To: <20180425041704.26882-5-jakub.kicinski@netronome.com>

On Wed, Apr 25, 2018 at 7:17 AM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> From: John Hurley <john.hurley@netronome.com>
>
> If a flower rule has a repr both as ingress and egress port then 2
> callbacks may be generated for the same rule request.
>
> Add an indicator to each flow as to whether or not it was added from an
> ingress registered cb. If so then ignore add/del/stat requests to it from
> an egress cb.

So on add() you ignore (return success) - I wasn't sure from the patch
what do you do for stat()/del() -- success? why not err? as you know I am
working on the same patch for mlx5, lets align here please.

Or.

^ permalink raw reply

* Re: [PATCH 2/8] dmaengine: shdmac: Change platform check to CONFIG_ARCH_RENESAS
From: Vinod Koul @ 2018-04-25  9:18 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: alsa-devel, Kuninori Morimoto, Catalin Marinas, Will Deacon,
	Liam Girdwood, Laurent Pinchart, devel, Mauro Carvalho Chehab,
	Magnus Damm, Russell King, linux-media, Arnd Bergmann, Mark Brown,
	Dan Williams, Jaroslav Kysela, linux-arm-kernel, Sergei Shtylyov,
	Greg Kroah-Hartman, Takashi Iwai, linux-kernel, linux-renesas-soc,
	Simon Horman, netdev, dmaengine
In-Reply-To: <1524230914-10175-3-git-send-email-geert+renesas@glider.be>

On Fri, Apr 20, 2018 at 03:28:28PM +0200, Geert Uytterhoeven wrote:
> Since commit 9b5ba0df4ea4f940 ("ARM: shmobile: Introduce ARCH_RENESAS")
> is CONFIG_ARCH_RENESAS a more appropriate platform check than the legacy
> CONFIG_ARCH_SHMOBILE, hence use the former.
> 
> Renesas SuperH SH-Mobile SoCs are still covered by the CONFIG_CPU_SH4
> check, just like before support for Renesas ARM SoCs was added.
> 
> Instead of blindly changing all the #ifdefs, switch the main code block
> in sh_dmae_probe() to IS_ENABLED(), as this allows to remove all the
> remaining #ifdefs.
> 
> This will allow to drop ARCH_SHMOBILE on ARM in the near future.

Applied, thanks

-- 
~Vinod

^ permalink raw reply

* Re: [net-next v3] ipv6: sr: Compute flowlabel for outer IPv6 header of seg6 encap mode
From: David Lebrun @ 2018-04-25  9:23 UTC (permalink / raw)
  To: Ahmed Abdelsalam, davem, kuznet, yoshfuji, netdev, linux-kernel
In-Reply-To: <1524594196-12383-1-git-send-email-amsalam20@gmail.com>

On 04/24/2018 07:23 PM, Ahmed Abdelsalam wrote:
> 
> Signed-off-by: Ahmed Abdelsalam <amsalam20@gmail.com>

Acked-by: David Lebrun <dlebrun@google.com>

^ permalink raw reply

* Re: [net 1/6] ixgbevf: ensure xdp_ring resources are free'd on error exit
From: Sergei Shtylyov @ 2018-04-25  9:25 UTC (permalink / raw)
  To: Jeff Kirsher, davem; +Cc: Colin Ian King, netdev, nhorman, sassmann, jogreene
In-Reply-To: <20180424192911.22786-2-jeffrey.t.kirsher@intel.com>

Hello!

On 4/24/2018 10:29 PM, Jeff Kirsher wrote:

> From: Colin Ian King <colin.king@canonical.com>
> 
> The current error handling for failed resource setup for xdp_ring
> data is a break out of the loop and returning 0 indicated everything
> was OK, when in fact it is not.  Fix this by exiting via the
> error exit label err_setup_tx that will clean up the resources
> correctly and return and error status.

    s/and/an/&

> Detected by CoverityScan, CID#1466879 ("Logically dead code")
> 
> Fixes: 21092e9ce8b1 ("ixgbevf: Add support for XDP_TX action")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
[...]

MBR, Sergei

^ permalink raw reply

* Re: [PATCH bpf-next] bpf: Allow bpf_jit_enable = 2 with BPF_JIT_ALWAYS_ON config
From: Leo Yan @ 2018-04-25  9:25 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: David S. Miller, Alexei Starovoitov, Kirill Tkhai, netdev,
	linux-kernel
In-Reply-To: <a90e398e-c012-2630-6909-6d413e03cc96@iogearbox.net>

Hi Daniel,

On Wed, Apr 25, 2018 at 11:12:21AM +0200, Daniel Borkmann wrote:
> On 04/25/2018 10:18 AM, Leo Yan wrote:
> > After enabled BPF_JIT_ALWAYS_ON config, bpf_jit_enable always equals to
> > 1; it is impossible to set 'bpf_jit_enable = 2' and the kernel has no
> > chance to call bpf_jit_dump().
> > 
> > This patch relaxes bpf_jit_enable range to [1..2] when kernel config
> > BPF_JIT_ALWAYS_ON is enabled so can invoke jit dump.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> 
> Is there a specific reason why you need this here instead of retrieving the
> dump from the newer interface available from bpftool (tools/bpf/bpftool/)?
> The bpf_jit_enable = 2 is not recommended these days since it dumps into the
> kernel log which is often readable from unpriv as well. bpftool makes use
> of the BPF_OBJ_GET_INFO_BY_FD interface via bpf syscall to get the JIT dump
> instead when bpf_jit_enable is set.

Thanks for reviewing.

When I read the doc Documentation/networking/filter.txt and the
section "JIT compiler" it suggests as below.  So I tried to set
'bpf_jit_enable = 2' to dump JIT code, but it failed.

If we have concern for security issue, should we remove support for
'bpf_jit_enable = 2' and modify the doc to reflect this change?

---8<---

For JIT developers, doing audits etc, each compile run can output the generated
opcode image into the kernel log via: 

  echo 2 > /proc/sys/net/core/bpf_jit_enable

Example output from dmesg:

[ 3389.935842] flen=6 proglen=70 pass=3 image=ffffffffa0069c8f
[ 3389.935847] JIT code: 00000000: 55 48 89 e5 48 83 ec 60 48 89 5d f8 44 8b 4f 68
[ 3389.935849] JIT code: 00000010: 44 2b 4f 6c 4c 8b 87 d8 00 00 00 be 0c 00 00 00
[ 3389.935850] JIT code: 00000020: e8 1d 94 ff e0 3d 00 08 00 00 75 16 be 17 00 00
[ 3389.935851] JIT code: 00000030: 00 e8 28 94 ff e0 83 f8 01 75 07 b8 ff ff 00 00
[ 3389.935852] JIT code: 00000040: eb 02 31 c0 c9 c3

> > ---
> >  net/core/sysctl_net_core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
> > index b1a2c5e..6a39b22 100644
> > --- a/net/core/sysctl_net_core.c
> > +++ b/net/core/sysctl_net_core.c
> > @@ -371,7 +371,7 @@ static int proc_dointvec_minmax_bpf_enable(struct ctl_table *table, int write,
> >  		.proc_handler	= proc_dointvec_minmax_bpf_enable,
> >  # ifdef CONFIG_BPF_JIT_ALWAYS_ON
> >  		.extra1		= &one,
> > -		.extra2		= &one,
> > +		.extra2		= &two,
> >  # else
> >  		.extra1		= &zero,
> >  		.extra2		= &two,
> > 
> 

^ permalink raw reply

* Re: [net 2/6] igb: Fix the transmission mode of queue 0 for Qav mode
From: Sergei Shtylyov @ 2018-04-25  9:27 UTC (permalink / raw)
  To: Jeff Kirsher, davem
  Cc: Vinicius Costa Gomes, netdev, nhorman, sassmann, jogreene
In-Reply-To: <20180424192911.22786-3-jeffrey.t.kirsher@intel.com>

On 4/24/2018 10:29 PM, Jeff Kirsher wrote:

> From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> 
> When Qav mode is enabled, queue 0 should be kept on Stream Reservation

    s/on/in/?

> mode. From the i210 datasheet, section 8.12.19:
> 
> "Note: Queue0 QueueMode must be set to 1b when TransmitMode is set to
> Qav." ("QueueMode 1b" represents the Stream Reservation mode)
> 
> The solution is to give queue 0 the all the credits it might need, so
> it has priority over queue 1.
> 
> A situation where this can happen is when cbs is "installed" only on
> queue 1, leaving queue 0 alone. For example:
> 
> $ tc qdisc replace dev enp2s0 handle 100: parent root mqprio num_tc 3 \
>       	   map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 queues 1@0 1@1 2@2 hw 0
> 
> $ tc qdisc replace dev enp2s0 parent 100:2 cbs locredit -1470 \
>       	   hicredit 30 sendslope -980000 idleslope 20000 offload 1
> 
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
[...]

MBR, Sergei

^ permalink raw reply

* Re: [PATCH net-next 3/4] nfp: flower: support offloading multiple rules with same cookie
From: John Hurley @ 2018-04-25  9:27 UTC (permalink / raw)
  To: Or Gerlitz
  Cc: Jakub Kicinski, David Miller, Linux Netdev List, oss-drivers,
	ASAP_Direct_Dev
In-Reply-To: <CAJ3xEMg0VPbM1bZG-fiNkpgabWBVw2U6uibrXxz+dMaOophVWA@mail.gmail.com>

On Wed, Apr 25, 2018 at 10:13 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Wed, Apr 25, 2018 at 12:02 PM, John Hurley <john.hurley@netronome.com> wrote:
>> On Wed, Apr 25, 2018 at 9:56 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>> On Wed, Apr 25, 2018 at 11:51 AM, John Hurley <john.hurley@netronome.com> wrote:
>>>> On Wed, Apr 25, 2018 at 7:31 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>>>> On Wed, Apr 25, 2018 at 7:17 AM, Jakub Kicinski
>>>>> <jakub.kicinski@netronome.com> wrote:
>>>>>> From: John Hurley <john.hurley@netronome.com>
>>>>>>
>>>>>> When multiple netdevs are attached to a tc offload block and register for
>>>>>> callbacks, a rule added to the block will be propogated to all netdevs.
>>>>>> Previously these were detected as duplicates (based on cookie) and
>>>>>> rejected. Modify the rule nfp lookup function to optionally include an
>>>>>> ingress netdev and a host context along with the cookie value when
>>>>>> searching for a rule. When a new rule is passed to the driver, the netdev
>>>>>> the rule is to be attached to is considered when searching for dublicates.
>>>>>
>>>>> so if the same rule (cookie) is provided to the driver through multiple ingress
>>>>> devices you will not reject it -- what is the use case for that, is it
>>>>> block sharing?
>>>>
>>>> Hi Or,
>>>> Yes, block sharing is the current use-case.
>>>> Simple example for clarity....
>>>> Here we want to offload the filter to both ingress devs nfp_0 and nfp_1:
>>>>
>>>> tc qdisc add dev nfp_p0 ingress_block 22 ingress
>>>> tc qdisc add dev nfp_p1 ingress_block 22 ingress
>>>> tc filter add block 22 protocol ip parent ffff: flower skip_sw
>>>> ip_proto tcp action drop
>>>
>>> cool!
>>>
>>> Just out of curiosity, do you actually share this HW rule or you duplicate it?
>>
>> It's duplicated. At HW level the ingress port is part of the match so technically it's
>> a different rule.
>
> I see, we have also a match on the ingress port as part of the HW API, which
> means we will have to apply a similar practice if we want to support
> block sharing quickly.
>
> Just to make sure, under tc block sharing the tc stack calls for hw
> offloading of the
> same rule (same cookie) multiple times, each with different ingress
> device, right?
>
>
> Or.

So in the example above, when each qdisc add is called, a callback
will be registered to the block.
For each callback, the dev used is passed as priv data (presumably you
do similar).
When the filter is added, the block code triggers all callbacks with
the same rule data [1].
We differentiate the callbacks with the priv data (ingress dev).

[1] https://elixir.bootlin.com/linux/v4.17-rc2/source/net/sched/cls_api.c#L741

^ permalink raw reply

* [PATCH iproute2] ingress: Don't break JSON output
From: Toke Høiland-Jørgensen @ 2018-04-25  9:29 UTC (permalink / raw)
  To: netdev; +Cc: Toke Høiland-Jørgensen

The dash printed by the ingress qdisc breaks JSON output, so only print it
in regular output mode.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 tc/q_ingress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tc/q_ingress.c b/tc/q_ingress.c
index 1e422298..93313c9c 100644
--- a/tc/q_ingress.c
+++ b/tc/q_ingress.c
@@ -40,7 +40,7 @@ static int ingress_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 static int ingress_print_opt(struct qdisc_util *qu, FILE *f,
 			     struct rtattr *opt)
 {
-	fprintf(f, "---------------- ");
+	print_string(PRINT_FP, NULL, "---------------- ", NULL);
 	return 0;
 }
 
-- 
2.17.0

^ permalink raw reply related

* Re: [PATCH net-next] sctp: fix const parameter violation in sctp_make_sack
From: Xin Long @ 2018-04-25  9:44 UTC (permalink / raw)
  To: Marcelo Ricardo Leitner
  Cc: network dev, linux-sctp, Vlad Yasevich, Neil Horman
In-Reply-To: <b9065b3cfb0a2bf3c83008ff53967f473e081766.1524603855.git.marcelo.leitner@gmail.com>

On Wed, Apr 25, 2018 at 5:17 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> sctp_make_sack() make changes to the asoc and this cast is just
> bypassing the const attribute. As there is no need to have the const
> there, just remove it and fix the violation.
>
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
>
> This one can go to net or net-next, but targetting net-next here just to
> keep it together with the rest (which I'll post as patches get in).
>
>  include/net/sctp/sm.h    | 2 +-
>  net/sctp/sm_make_chunk.c | 9 ++++-----
>  2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
> index 2d0e782c90551377ad654bcef1224bbdb75ba394..f4b657478a304050851f33d92c71162a4a4a2e50 100644
> --- a/include/net/sctp/sm.h
> +++ b/include/net/sctp/sm.h
> @@ -207,7 +207,7 @@ struct sctp_chunk *sctp_make_datafrag_empty(const struct sctp_association *asoc,
>                                             int len, __u8 flags, gfp_t gfp);
>  struct sctp_chunk *sctp_make_ecne(const struct sctp_association *asoc,
>                                   const __u32 lowest_tsn);
> -struct sctp_chunk *sctp_make_sack(const struct sctp_association *asoc);
> +struct sctp_chunk *sctp_make_sack(struct sctp_association *asoc);
>  struct sctp_chunk *sctp_make_shutdown(const struct sctp_association *asoc,
>                                       const struct sctp_chunk *chunk);
>  struct sctp_chunk *sctp_make_shutdown_ack(const struct sctp_association *asoc,
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 5a4fb1dc8400a0316177ce65be8126857297eb5e..db93eabd6ef500ab20be6e091ee06bd3b60923c9 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -779,10 +779,9 @@ struct sctp_chunk *sctp_make_datafrag_empty(const struct sctp_association *asoc,
>   * association.  This reports on which TSN's we've seen to date,
>   * including duplicates and gaps.
>   */
> -struct sctp_chunk *sctp_make_sack(const struct sctp_association *asoc)
> +struct sctp_chunk *sctp_make_sack(struct sctp_association *asoc)
>  {
>         struct sctp_tsnmap *map = (struct sctp_tsnmap *)&asoc->peer.tsn_map;
> -       struct sctp_association *aptr = (struct sctp_association *)asoc;
>         struct sctp_gap_ack_block gabs[SCTP_MAX_GABS];
>         __u16 num_gabs, num_dup_tsns;
>         struct sctp_transport *trans;
> @@ -857,7 +856,7 @@ struct sctp_chunk *sctp_make_sack(const struct sctp_association *asoc)
>
>         /* Add the duplicate TSN information.  */
>         if (num_dup_tsns) {
> -               aptr->stats.idupchunks += num_dup_tsns;
> +               asoc->stats.idupchunks += num_dup_tsns;
>                 sctp_addto_chunk(retval, sizeof(__u32) * num_dup_tsns,
>                                  sctp_tsnmap_get_dups(map));
>         }
> @@ -869,11 +868,11 @@ struct sctp_chunk *sctp_make_sack(const struct sctp_association *asoc)
>          * association so no transport will match after a wrap event like this,
>          * Until the next sack
>          */
> -       if (++aptr->peer.sack_generation == 0) {
> +       if (++asoc->peer.sack_generation == 0) {
>                 list_for_each_entry(trans, &asoc->peer.transport_addr_list,
>                                     transports)
>                         trans->sack_generation = 0;
> -               aptr->peer.sack_generation = 1;
> +               asoc->peer.sack_generation = 1;
>         }
>  nodata:
>         return retval;
> --
> 2.14.3
>
Reviewed-by: Xin Long <lucien.xin@gmail.com>

^ permalink raw reply

* Re: [PATCH net-next 4/4] nfp: flower: ignore duplicate cb requests for same rule
From: John Hurley @ 2018-04-25  9:45 UTC (permalink / raw)
  To: Or Gerlitz; +Cc: Jakub Kicinski, David Miller, Linux Netdev List, oss-drivers
In-Reply-To: <CAJ3xEMgYg2xKuBM34BhgamifreCHtB7+2V9T9kiQM+_xAPm_Lw@mail.gmail.com>

On Wed, Apr 25, 2018 at 10:17 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Wed, Apr 25, 2018 at 7:17 AM, Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
>> From: John Hurley <john.hurley@netronome.com>
>>
>> If a flower rule has a repr both as ingress and egress port then 2
>> callbacks may be generated for the same rule request.
>>
>> Add an indicator to each flow as to whether or not it was added from an
>> ingress registered cb. If so then ignore add/del/stat requests to it from
>> an egress cb.
>
> So on add() you ignore (return success) - I wasn't sure from the patch
> what do you do for stat()/del() -- success? why not err? as you know I am
> working on the same patch for mlx5, lets align here please.
>
> Or.

ok, this is way Ive implemented the calls...

add:
- if egress cb duplicate but the flow has been added on ingress cb
then ignore (return success)
- if egress cb duplicate and flow not added on ingress then err (this
is a 'true duplicate')

stats:
- if egress cb but the flow has an ingress cb then ignore - stat
request will have been covered on ingress cb so shouldn't error, just
don't repeat

del:
- if egress cb and flow no longer exists then assume it was deleted on
ingress so ignore (return success)
- if ingress cb and flow no longer exists then (as ingress cb is hit
first) this is a bad request whereby trying to delete a flow that was
never added - return err

^ permalink raw reply

* Re: [PATCH net-next] sctp: fix identification of new acks for SFR-CACC
From: Xin Long @ 2018-04-25  9:45 UTC (permalink / raw)
  To: Marcelo Ricardo Leitner
  Cc: network dev, linux-sctp, Vlad Yasevich, Neil Horman
In-Reply-To: <4e837f7eaf67a1a964d1b3418aa3cf759f512535.1524603870.git.marcelo.leitner@gmail.com>

On Wed, Apr 25, 2018 at 5:17 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> It's currently written as:
>
> if (!tchunk->tsn_gap_acked) {   [1]
>         tchunk->tsn_gap_acked = 1;
>         ...
> }
>
> if (TSN_lte(tsn, sack_ctsn)) {
>         if (!tchunk->tsn_gap_acked) {
>                 /* SFR-CACC processing */
>                 ...
>         }
> }
>
> Which causes the SFR-CACC processing on ack reception to never process,
> as tchunk->tsn_gap_acked is always true by then. Block [1] was
> moved to that position by the commit marked below.
>
> This patch fixes it by doing SFR-CACC processing earlier, before
> tsn_gap_acked is set to true.
>
> Fixes: 31b02e154940 ("sctp: Failover transmitted list on transport delete")
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
>
> Even though this is a -stable candidate, please apply it to net-next
> to avoid conflicts with subsequent patches in my queue. Thanks.
>
>  net/sctp/outqueue.c | 48 +++++++++++++++++++++++-------------------------
>  1 file changed, 23 insertions(+), 25 deletions(-)
>
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index f211b3db6a3543073e113da121bb28518b0af491..dee7cbd5483149024f2f3195db2fe4d473b1a00a 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -1457,7 +1457,7 @@ static void sctp_check_transmitted(struct sctp_outq *q,
>                          * the outstanding bytes for this chunk, so only
>                          * count bytes associated with a transport.
>                          */
> -                       if (transport) {
> +                       if (transport && !tchunk->tsn_gap_acked) {
>                                 /* If this chunk is being used for RTT
>                                  * measurement, calculate the RTT and update
>                                  * the RTO using this value.
> @@ -1469,14 +1469,34 @@ static void sctp_check_transmitted(struct sctp_outq *q,
>                                  * first instance of the packet or a later
>                                  * instance).
>                                  */
> -                               if (!tchunk->tsn_gap_acked &&
> -                                   !sctp_chunk_retransmitted(tchunk) &&
> +                               if (!sctp_chunk_retransmitted(tchunk) &&
>                                     tchunk->rtt_in_progress) {
>                                         tchunk->rtt_in_progress = 0;
>                                         rtt = jiffies - tchunk->sent_at;
>                                         sctp_transport_update_rto(transport,
>                                                                   rtt);
>                                 }
> +
> +                               if (TSN_lte(tsn, sack_ctsn)) {
> +                                       /*
> +                                        * SFR-CACC algorithm:
> +                                        * 2) If the SACK contains gap acks
> +                                        * and the flag CHANGEOVER_ACTIVE is
> +                                        * set the receiver of the SACK MUST
> +                                        * take the following action:
> +                                        *
> +                                        * B) For each TSN t being acked that
> +                                        * has not been acked in any SACK so
> +                                        * far, set cacc_saw_newack to 1 for
> +                                        * the destination that the TSN was
> +                                        * sent to.
> +                                        */
> +                                       if (sack->num_gap_ack_blocks &&
> +                                           q->asoc->peer.primary_path->cacc.
> +                                           changeover_active)
> +                                               transport->cacc.cacc_saw_newack
> +                                                       = 1;
> +                               }
>                         }
>
>                         /* If the chunk hasn't been marked as ACKED,
> @@ -1508,28 +1528,6 @@ static void sctp_check_transmitted(struct sctp_outq *q,
>                                 restart_timer = 1;
>                                 forward_progress = true;
>
> -                               if (!tchunk->tsn_gap_acked) {
> -                                       /*
> -                                        * SFR-CACC algorithm:
> -                                        * 2) If the SACK contains gap acks
> -                                        * and the flag CHANGEOVER_ACTIVE is
> -                                        * set the receiver of the SACK MUST
> -                                        * take the following action:
> -                                        *
> -                                        * B) For each TSN t being acked that
> -                                        * has not been acked in any SACK so
> -                                        * far, set cacc_saw_newack to 1 for
> -                                        * the destination that the TSN was
> -                                        * sent to.
> -                                        */
> -                                       if (transport &&
> -                                           sack->num_gap_ack_blocks &&
> -                                           q->asoc->peer.primary_path->cacc.
> -                                           changeover_active)
> -                                               transport->cacc.cacc_saw_newack
> -                                                       = 1;
> -                               }
> -
>                                 list_add_tail(&tchunk->transmitted_list,
>                                               &q->sacked);
>                         } else {
> --
> 2.14.3
>
Reviewed-by: Xin Long <lucien.xin@gmail.com>

^ permalink raw reply

* [PATCH net-next] sctp: remove the unused sctp_assoc_is_match function
From: Xin Long @ 2018-04-25  9:46 UTC (permalink / raw)
  To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem

After Commit 4f0087812648 ("sctp: apply rhashtable api to send/recv
path"), there's no place using sctp_assoc_is_match, so remove it.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/structs.h |  4 ----
 net/sctp/associola.c       | 25 -------------------------
 2 files changed, 29 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index a0ec462..05594b2 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -2091,10 +2091,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 				  enum sctp_transport_cmd command,
 				  sctp_sn_error_t error);
 struct sctp_transport *sctp_assoc_lookup_tsn(struct sctp_association *, __u32);
-struct sctp_transport *sctp_assoc_is_match(struct sctp_association *,
-					   struct net *,
-					   const union sctp_addr *,
-					   const union sctp_addr *);
 void sctp_assoc_migrate(struct sctp_association *, struct sock *);
 int sctp_assoc_update(struct sctp_association *old,
 		      struct sctp_association *new);
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 837806d..a8f3b08 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -988,31 +988,6 @@ struct sctp_transport *sctp_assoc_lookup_tsn(struct sctp_association *asoc,
 	return match;
 }
 
-/* Is this the association we are looking for? */
-struct sctp_transport *sctp_assoc_is_match(struct sctp_association *asoc,
-					   struct net *net,
-					   const union sctp_addr *laddr,
-					   const union sctp_addr *paddr)
-{
-	struct sctp_transport *transport;
-
-	if ((htons(asoc->base.bind_addr.port) == laddr->v4.sin_port) &&
-	    (htons(asoc->peer.port) == paddr->v4.sin_port) &&
-	    net_eq(sock_net(asoc->base.sk), net)) {
-		transport = sctp_assoc_lookup_paddr(asoc, paddr);
-		if (!transport)
-			goto out;
-
-		if (sctp_bind_addr_match(&asoc->base.bind_addr, laddr,
-					 sctp_sk(asoc->base.sk)))
-			goto out;
-	}
-	transport = NULL;
-
-out:
-	return transport;
-}
-
 /* Do delayed input processing.  This is scheduled by sctp_rcv(). */
 static void sctp_assoc_bh_rcv(struct work_struct *work)
 {
-- 
2.1.0

^ permalink raw reply related

* [PATCH stable v4.4+] r8152: add Linksys USB3GIGV1 id
From: Krzysztof Kozlowski @ 2018-04-25  9:54 UTC (permalink / raw)
  To: Oliver Neukum, David S. Miller, Krzysztof Kozlowski, linux-usb,
	netdev, linux-kernel
  Cc: Grant Grundler

commit 90841047a01b452cc8c3f9b990698b264143334a upstream

This linksys dongle by default comes up in cdc_ether mode.
This patch allows r8152 to claim the device:
   Bus 002 Device 002: ID 13b1:0041 Linksys

Signed-off-by: Grant Grundler <grundler@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: David S. Miller <davem@davemloft.net>
[krzk: Rebase on v4.4]
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/net/usb/cdc_ether.c | 10 ++++++++++
 drivers/net/usb/r8152.c     |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 6578127db847..f71abe50ea6f 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -461,6 +461,7 @@ static const struct driver_info wwan_info = {
 #define REALTEK_VENDOR_ID	0x0bda
 #define SAMSUNG_VENDOR_ID	0x04e8
 #define LENOVO_VENDOR_ID	0x17ef
+#define LINKSYS_VENDOR_ID	0x13b1
 #define NVIDIA_VENDOR_ID	0x0955
 #define HP_VENDOR_ID		0x03f0
 
@@ -650,6 +651,15 @@ static const struct usb_device_id	products[] = {
 	.driver_info = 0,
 },
 
+#if IS_ENABLED(CONFIG_USB_RTL8152)
+/* Linksys USB3GIGV1 Ethernet Adapter */
+{
+	USB_DEVICE_AND_INTERFACE_INFO(LINKSYS_VENDOR_ID, 0x0041, USB_CLASS_COMM,
+			USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
+	.driver_info = 0,
+},
+#endif
+
 /* Lenovo Thinkpad USB 3.0 Ethernet Adapters (based on Realtek RTL8153) */
 {
 	USB_DEVICE_AND_INTERFACE_INFO(LENOVO_VENDOR_ID, 0x7205, USB_CLASS_COMM,
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 89950f5cea71..b2c1a435357f 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -506,6 +506,7 @@ enum rtl8152_flags {
 #define VENDOR_ID_REALTEK		0x0bda
 #define VENDOR_ID_SAMSUNG		0x04e8
 #define VENDOR_ID_LENOVO		0x17ef
+#define VENDOR_ID_LINKSYS		0x13b1
 #define VENDOR_ID_NVIDIA		0x0955
 
 #define MCU_TYPE_PLA			0x0100
@@ -4376,6 +4377,7 @@ static struct usb_device_id rtl8152_table[] = {
 	{REALTEK_USB_DEVICE(VENDOR_ID_SAMSUNG, 0xa101)},
 	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7205)},
 	{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x304f)},
+	{REALTEK_USB_DEVICE(VENDOR_ID_LINKSYS, 0x0041)},
 	{REALTEK_USB_DEVICE(VENDOR_ID_NVIDIA,  0x09ff)},
 	{}
 };
-- 
2.7.4

^ permalink raw reply related

* Re: [PATCH bpf-next 1/5] samples/bpf: Fix typo in comment
From: Leo Yan @ 2018-04-25 10:01 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: Daniel Thompson, Alexei Starovoitov, Daniel Borkmann, netdev,
	linux-kernel
In-Reply-To: <20180420155213.2867fcf5@redhat.com>

On Fri, Apr 20, 2018 at 03:52:13PM +0200, Jesper Dangaard Brouer wrote:
> On Fri, 20 Apr 2018 14:21:16 +0100
> Daniel Thompson <daniel.thompson@linaro.org> wrote:
> 
> > On Fri, Apr 20, 2018 at 02:10:04PM +0200, Jesper Dangaard Brouer wrote:
> > > 
> > > On Thu, 19 Apr 2018 09:34:02 +0800 Leo Yan <leo.yan@linaro.org> wrote:
> > >   
> > > > Fix typo by replacing 'iif' with 'if'.
> > > > 
> > > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > > ---
> > > >  samples/bpf/bpf_load.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
> > > > index bebe418..28e4678 100644
> > > > --- a/samples/bpf/bpf_load.c
> > > > +++ b/samples/bpf/bpf_load.c
> > > > @@ -393,7 +393,7 @@ static int load_elf_maps_section(struct bpf_map_data *maps, int maps_shndx,
> > > >  			continue;
> > > >  		if (sym[nr_maps].st_shndx != maps_shndx)
> > > >  			continue;
> > > > -		/* Only increment iif maps section */
> > > > +		/* Only increment if maps section */
> > > >  		nr_maps++;
> > > >  	}  
> > > 
> > > This was actually not a typo from my side.
> > > 
> > > With 'iif' I mean 'if and only if' ... but it doesn't matter much.  
> > 
> > I think 'if and only if' is more commonly abbreviated 'iff' isn't it?
> 
> Ah, yes![1]  -- then it *is* actually a typo! - LOL
> 
> I'm fine with changing this to "if" :-)

Thanks for the reviewing, Daniel & Jesper.
I also learn it from the discussion :)

> [1] https://en.wikipedia.org/wiki/If_and_only_if
> 
> -- 
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer

^ permalink raw reply

* Re: [PATCH 0/3] bpf: Store/dump license string for loaded program
From: Jiri Olsa @ 2018-04-25 10:17 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, lkml, netdev,
	Quentin Monnet
In-Reply-To: <20180423201134.r56ts7usec4jjnbh@ast-mbp>

On Mon, Apr 23, 2018 at 02:11:36PM -0600, Alexei Starovoitov wrote:
> On Mon, Apr 23, 2018 at 08:59:24AM +0200, Jiri Olsa wrote:
> > hi,
> > sending the change to store and dump the license
> > info for loaded BPF programs. It's important for
> > us get the license info, when investigating on
> > screwed up machine.
> 
> hmm. boolean flag whether bpf prog is gpl or not
> is already exposed via bpf_prog_info.

hum, I can't see that (on bpf-next/master)
would the attached change be ok with you?

jirka


---
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index e6679393b687..2ce9c9d41c2b 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1062,6 +1062,7 @@ struct bpf_prog_info {
 	__u32 ifindex;
 	__u64 netns_dev;
 	__u64 netns_ino;
+	__u16 gpl_compatible:1;
 } __attribute__((aligned(8)));
 
 struct bpf_map_info {
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index fe23dc5a3ec4..7bb4ff1c770a 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1914,6 +1914,7 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
 	info.load_time = prog->aux->load_time;
 	info.created_by_uid = from_kuid_munged(current_user_ns(),
 					       prog->aux->user->uid);
+	info.gpl_compatible = prog->gpl_compatible;
 
 	memcpy(info.tag, prog->tag, sizeof(prog->tag));
 	memcpy(info.name, prog->aux->name, sizeof(prog->aux->name));

^ permalink raw reply related

* Re: [PATCH stable v4.4+] r8152: add Linksys USB3GIGV1 id
From: Greg KH @ 2018-04-25 10:30 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Oliver Neukum, David S. Miller, linux-usb, netdev, linux-kernel,
	Grant Grundler
In-Reply-To: <1524650077-13270-1-git-send-email-krzk@kernel.org>

On Wed, Apr 25, 2018 at 11:54:37AM +0200, Krzysztof Kozlowski wrote:
> commit 90841047a01b452cc8c3f9b990698b264143334a upstream
> 
> This linksys dongle by default comes up in cdc_ether mode.
> This patch allows r8152 to claim the device:
>    Bus 002 Device 002: ID 13b1:0041 Linksys
> 
> Signed-off-by: Grant Grundler <grundler@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> [krzk: Rebase on v4.4]

Also added to 4.9.y, thanks.

greg k-h

^ permalink raw reply

* [PATCH] net: amd8111e: remove redundant duplicated if statement
From: Colin King @ 2018-04-25 10:31 UTC (permalink / raw)
  To: David S . Miller, Allen Pais, netdev; +Cc: kernel-janitors, linux-kernel

From: Colin Ian King <colin.king@canonical.com>

There are two identical nested if statements, the second is redundant
and can be removed. Also clean up white space formatting.

Cleans up cppcheck warning:
drivers/net/ethernet/amd/amd8111e.c:1080: (warning) Identical inner 'if'
condition is always true.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/net/ethernet/amd/amd8111e.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/amd/amd8111e.c b/drivers/net/ethernet/amd/amd8111e.c
index c99e3e845ac0..a90080f12e67 100644
--- a/drivers/net/ethernet/amd/amd8111e.c
+++ b/drivers/net/ethernet/amd/amd8111e.c
@@ -1074,16 +1074,12 @@ static int amd8111e_calc_coalesce(struct net_device *dev)
 				amd8111e_set_coalesce(dev,TX_INTR_COAL);
 				coal_conf->tx_coal_type = MEDIUM_COALESCE;
 			}
-
-		}
-		else if(tx_pkt_size >= 1024){
-			if (tx_pkt_size >= 1024){
-				if(coal_conf->tx_coal_type !=  HIGH_COALESCE){
-					coal_conf->tx_timeout = 4;
-					coal_conf->tx_event_count = 8;
-					amd8111e_set_coalesce(dev,TX_INTR_COAL);
-					coal_conf->tx_coal_type = HIGH_COALESCE;
-				}
+		} else if (tx_pkt_size >= 1024) {
+			if (coal_conf->tx_coal_type != HIGH_COALESCE) {
+				coal_conf->tx_timeout = 4;
+				coal_conf->tx_event_count = 8;
+				amd8111e_set_coalesce(dev, TX_INTR_COAL);
+				coal_conf->tx_coal_type = HIGH_COALESCE;
 			}
 		}
 	}
-- 
2.17.0

^ permalink raw reply related

* [PATCH] mkiss: remove redundant check for len > 0
From: Colin King @ 2018-04-25 10:43 UTC (permalink / raw)
  To: David S . Miller, Johannes Berg, Stephen Hemminger, netdev
  Cc: kernel-janitors, linux-kernel

From: Colin Ian King <colin.king@canonical.com>

The check for len > 0 is always true and hence is redundant as
this check is already being made to execute the code inside the
while-loop. Hence it is redundant and can be removed.

Cleans up cppcheck warning:
drivers/net/hamradio/mkiss.c:220: (warning) Identical inner 'if'
condition is always true.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/net/hamradio/mkiss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/hamradio/mkiss.c b/drivers/net/hamradio/mkiss.c
index c180b480f8ef..13e4c1eff353 100644
--- a/drivers/net/hamradio/mkiss.c
+++ b/drivers/net/hamradio/mkiss.c
@@ -217,7 +217,7 @@ static int kiss_esc_crc(unsigned char *s, unsigned char *d, unsigned short crc,
 			c = *s++;
 		else if (len > 1)
 			c = crc >> 8;
-		else if (len > 0)
+		else
 			c = crc & 0xff;
 
 		len--;
-- 
2.17.0

^ permalink raw reply related

* [PATCH net 1/1] net/smc: keep clcsock reference in smc_tcp_listen_work()
From: Ursula Braun @ 2018-04-25 10:48 UTC (permalink / raw)
  To: davem; +Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, ubraun

The internal CLC socket should exist till the SMC-socket is released.
Function tcp_listen_worker() releases the internal CLC socket of a
listen socket, if an smc_close_active() is called. This function
is called for the final release(), but it is called for shutdown
SHUT_RDWR as well. This opens a door for protection faults, if
socket calls using the internal CLC socket are called for a
shutdown listen socket.

With the changes of
commit 3d502067599f ("net/smc: simplify wait when closing listen socket")
there is no need anymore to release the internal CLC socket in
function tcp_listen_worker((). It is sufficient to release it in
smc_release().

Fixes: 127f49705823 ("net/smc: release clcsock from tcp_listen_worker")
Signed-off-by: Ursula Braun <ubraun@linux.ibm.com>
Reported-by: syzbot+9045fc589fcd196ef522@syzkaller.appspotmail.com
Reported-by: syzbot+28a2c86cf19c81d871fa@syzkaller.appspotmail.com
Reported-by: syzbot+9605e6cace1b5efd4a0a@syzkaller.appspotmail.com
Reported-by: syzbot+cf9012c597c8379d535c@syzkaller.appspotmail.com
---
 net/smc/af_smc.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index f5d4b69dbabc..4470501374bf 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -978,10 +978,6 @@ static void smc_tcp_listen_work(struct work_struct *work)
 	}
 
 out:
-	if (lsmc->clcsock) {
-		sock_release(lsmc->clcsock);
-		lsmc->clcsock = NULL;
-	}
 	release_sock(lsk);
 	sock_put(&lsmc->sk); /* sock_hold in smc_listen */
 }
-- 
2.13.5

^ permalink raw reply related

* Re: [PATCH bpf-next 14/15] xsk: statistics support
From: Magnus Karlsson @ 2018-04-25 10:50 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Björn Töpel, Karlsson, Magnus, Alexander Duyck,
	Alexander Duyck, John Fastabend, Alexei Starovoitov,
	Jesper Dangaard Brouer, Daniel Borkmann, Michael S. Tsirkin,
	Network Development, michael.lundkvist, Brandeburg, Jesse,
	Singhai, Anjali, Zhang, Qi Z
In-Reply-To: <CAF=yD-KSZnnA+ntcBnW5Ohrb--OedbSuhqWVhcrxze6zH0-mFw@mail.gmail.com>

On Tue, Apr 24, 2018 at 6:58 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Mon, Apr 23, 2018 at 9:56 AM, Björn Töpel <bjorn.topel@gmail.com> wrote:
>> From: Magnus Karlsson <magnus.karlsson@intel.com>
>>
>> In this commit, a new getsockopt is added: XDP_STATISTICS. This is
>> used to obtain stats from the sockets.
>>
>> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
>
>> +static int xsk_getsockopt(struct socket *sock, int level, int optname,
>> +                         char __user *optval, int __user *optlen)
>> +{
>> +       struct sock *sk = sock->sk;
>> +       struct xdp_sock *xs = xdp_sk(sk);
>> +       int len;
>> +
>> +       if (level != SOL_XDP)
>> +               return -ENOPROTOOPT;
>> +
>> +       if (get_user(len, optlen))
>> +               return -EFAULT;
>> +       if (len < 0)
>> +               return -EINVAL;
>> +
>> +       switch (optname) {
>> +       case XDP_STATISTICS:
>> +       {
>> +               struct xdp_statistics stats;
>> +
>> +               if (len != sizeof(stats))
>> +                       return -EINVAL;
>> +
>> +               mutex_lock(&xs->mutex);
>> +               stats.rx_dropped = xs->rx_dropped;
>> +               stats.rx_invalid_descs = xskq_nb_invalid_descs(xs->rx);
>> +               stats.tx_invalid_descs = xskq_nb_invalid_descs(xs->tx);
>> +               mutex_unlock(&xs->mutex);
>> +
>> +               if (copy_to_user(optval, &stats, sizeof(stats)))
>> +                       return -EFAULT;
>> +               return 0;
>
> For forward compatibility, could allow caller to pass a struct larger
> than stats and return the number of bytes filled in.

Yes definitely. Will fix right away.

> The lock can also be elided with something like gnet_stats, but it is probably
> taken rarely enough that that is not worth the effort, at least right now.

Will put this on the ever expanding todo list for future patches ;-).

Thanks: Magnus

^ permalink raw reply

* Re: [PATCH v2] bpf, x86_32: add eBPF JIT compiler for ia32
From: Wang YanQing @ 2018-04-25 11:00 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: ast, illusionist.neo, tglx, mingo, hpa, davem, x86, netdev,
	linux-kernel
In-Reply-To: <20e1eabd-e821-8240-cb4c-6da253c49585@iogearbox.net>

On Wed, Apr 25, 2018 at 02:11:16AM +0200, Daniel Borkmann wrote:
> On 04/19/2018 05:54 PM, Wang YanQing wrote:
> > The JIT compiler emits ia32 bit instructions. Currently, It supports
> > eBPF only. Classic BPF is supported because of the conversion by BPF core.
> > 
> > Almost all instructions from eBPF ISA supported except the following:
> > BPF_ALU64 | BPF_DIV | BPF_K
> > BPF_ALU64 | BPF_DIV | BPF_X
> > BPF_ALU64 | BPF_MOD | BPF_K
> > BPF_ALU64 | BPF_MOD | BPF_X
> > BPF_STX | BPF_XADD | BPF_W
> > BPF_STX | BPF_XADD | BPF_DW
> > 
> > It doesn't support BPF_JMP|BPF_CALL with BPF_PSEUDO_CALL too.
> > 
> > ia32 has few general purpose registers, EAX|EDX|ECX|EBX|ESI|EDI,
> > and in these six registers, we can't treat all of them as real
> > general purpose registers in jit:
> > MUL instructions need EAX:EDX, shift instructions need ECX.
> > 
> > So I decide to use stack to emulate all eBPF 64 registers, this will
> > simplify the implementation a lot, because we don't need to face the
> > flexible memory address modes on ia32, for example, we don't need to
> > write below code pattern for one BPF_ADD instruction:
> > 
> >     if (src is a register && dst is a register)
> >     {
> >        //one instruction encoding for ADD instruction
> >     } else if (only src is a register)
> >     {
> >        //another different instruction encoding for ADD instruction
> >     } else if (only dst is a register)
> >     {
> >        //another different instruction encoding for ADD instruction
> >     } else
> >     {
> >        //src and dst are all on stack.
> >        //move src or dst to temporary registers
> >     }
> > 
> > If the above example if-else-else-else isn't so painful, try to think
> > it for BPF_ALU64|BPF_*SHIFT* instruction which we need to use many
> > native instructions to emulate.
> > 
> > Tested on my PC (Intel(R) Core(TM) i5-5200U CPU) and virtualbox.
> 
> Just out of plain curiosity, do you have a specific use case on the
> JIT for x86_32? Are you targeting this to be used for e.g. Atom or
> the like (sorry, don't really have a good overview where x86_32 is
> still in use these days)?
Yes, you are right that x86_32 isn't the BIG DEAL anymore, but they willn't
disappear completely in near future, the same as 32-bit linux kernel on
64bit machine.

For me, because I use 32-bit linux on development/hacking notebook for many years,
I try to trace/perf the kernel, then I meet eBPF, it is strange that there isn't
a jit for it, so I try to fill the empty.
> 
> > Testing results on i5-5200U:
> > 
> > 1) test_bpf: Summary: 349 PASSED, 0 FAILED, [319/341 JIT'ed]
> > 2) test_progs: Summary: 81 PASSED, 2 FAILED.
> >    test_progs report "libbpf: incorrect bpf_call opcode" for
> >    test_l4lb_noinline and test_xdp_noinline, because there is
> >    no llvm-6.0 on my machine, and current implementation doesn't
> >    support BPF_PSEUDO_CALL, so I think we can ignore the two failed
> >    testcases.
> > 3) test_lpm: OK
> > 4) test_lru_map: OK
> > 5) test_verifier: Summary: 823 PASSED, 5 FAILED
> >    test_verifier report "invalid bpf_context access off=68 size=1/2/4/8"
> >    for all the 5 FAILED testcases with/without jit, we need to fix the
> >    failed testcases themself instead of this jit.
> 
> Can you elaborate further on these? Looks like this definitely needs
> fixing on 32 bit. Would be great to get a better understanding of the
> underlying bug(s) and properly fix them.
Ok! I will try to dig them out.
> 
> > Above tests are all done with following flags settings discretely:
> > 1:bpf_jit_enable=1 and bpf_jit_harden=0
> > 2:bpf_jit_enable=1 and bpf_jit_harden=2
> > 
> > Below are some numbers for this jit implementation:
> > Note:
> >   I run test_progs in kselftest 100 times continuously for every testcase,
> >   the numbers are in format: total/times=avg.
> >   The numbers that test_bpf reports show almost the same relation.
> > 
> > a:jit_enable=0 and jit_harden=0            b:jit_enable=1 and jit_harden=0
> >   test_pkt_access:PASS:ipv4:15622/100=156  test_pkt_access:PASS:ipv4:10057/100=100
> >   test_pkt_access:PASS:ipv6:9130/100=91    test_pkt_access:PASS:ipv6:5055/100=50
> >   test_xdp:PASS:ipv4:240198/100=2401       test_xdp:PASS:ipv4:145945/100=1459
> >   test_xdp:PASS:ipv6:137326/100=1373       test_xdp:PASS:ipv6:67337/100=673
> >   test_l4lb:PASS:ipv4:61100/100=611        test_l4lb:PASS:ipv4:38137/100=381
> >   test_l4lb:PASS:ipv6:101000/100=1010      test_l4lb:PASS:ipv6:57779/100=577
> > 
> > c:jit_enable=1 and jit_harden=2
> >   test_pkt_access:PASS:ipv4:12650/100=126
> >   test_pkt_access:PASS:ipv6:7074/100=70
> >   test_xdp:PASS:ipv4:147211/100=1472
> >   test_xdp:PASS:ipv6:85783/100=857
> >   test_l4lb:PASS:ipv4:53222/100=532
> >   test_l4lb:PASS:ipv6:76322/100=763
> > 
> > Yes, the numbers are pretty when turn off jit_harden, if we want to speedup
> > jit_harden, then we need to move BPF_REG_AX to *real* register instead of stack
> > emulation, but when we do it, we need to face all the pain I describe above. We
> > can do it in next step.
> > 
> > See Documentation/networking/filter.txt for more information.
> > 
> > Signed-off-by: Wang YanQing <udknight@gmail.com>
> [...]
> > +		/* call */
> > +		case BPF_JMP | BPF_CALL:
> > +		{
> > +			const u8 *r1 = bpf2ia32[BPF_REG_1];
> > +			const u8 *r2 = bpf2ia32[BPF_REG_2];
> > +			const u8 *r3 = bpf2ia32[BPF_REG_3];
> > +			const u8 *r4 = bpf2ia32[BPF_REG_4];
> > +			const u8 *r5 = bpf2ia32[BPF_REG_5];
> > +
> > +			if (insn->src_reg == BPF_PSEUDO_CALL)
> > +				goto notyet;
> 
> Here you bail out on BPF_PSEUDO_CALL, but in below bpf_int_jit_compile() you
> implement half of e.g. 1c2a088a6626 ("bpf: x64: add JIT support for multi-function
> programs"), which is rather confusing to leave it in this half complete state;
> either remove altogether or implement everything.
Done.
> 
> > +			func = (u8 *) __bpf_call_base + imm32;
> > +			jmp_offset = func - (image + addrs[i]);
> > +
> > +			if (!imm32 || !is_simm32(jmp_offset)) {
> > +				pr_err("unsupported bpf func %d addr %p image %p\n",
> > +				       imm32, func, image);
> > +				return -EINVAL;
> > +			}
> > +
> > +			/* mov eax,dword ptr [ebp+off] */
> > +			EMIT3(0x8B, add_2reg(0x40, IA32_EBP, tmp2[0]),
> > +			      STACK_VAR(r1[0]));
> > +			/* mov edx,dword ptr [ebp+off] */
> > +			EMIT3(0x8B, add_2reg(0x40, IA32_EBP, tmp2[1]),
> > +			      STACK_VAR(r1[1]));
> > +
> > +			emit_push_r64(r5, &prog);
> > +			emit_push_r64(r4, &prog);
> > +			emit_push_r64(r3, &prog);
> > +			emit_push_r64(r2, &prog);
> > +
> > +			EMIT1_off32(0xE8, jmp_offset + 9);
> > +
> > +			/* mov dword ptr [ebp+off],eax */
> > +			EMIT3(0x89, add_2reg(0x40, IA32_EBP, tmp2[0]),
> > +			      STACK_VAR(r0[0]));
> > +			/* mov dword ptr [ebp+off],edx */
> > +			EMIT3(0x89, add_2reg(0x40, IA32_EBP, tmp2[1]),
> > +			      STACK_VAR(r0[1]));
> > +
> > +			/* add esp,32 */
> > +			EMIT3(0x83, add_1reg(0xC0, IA32_ESP), 32);
> > +			break;
> > +		}
> > +		case BPF_JMP | BPF_TAIL_CALL:
> > +			emit_bpf_tail_call(&prog);
> > +			break;
> > +
> > +		/* cond jump */
> > +		case BPF_JMP | BPF_JEQ | BPF_X:
> > +		case BPF_JMP | BPF_JNE | BPF_X:
> > +		case BPF_JMP | BPF_JGT | BPF_X:
> > +		case BPF_JMP | BPF_JLT | BPF_X:
> > +		case BPF_JMP | BPF_JGE | BPF_X:
> > +		case BPF_JMP | BPF_JLE | BPF_X:
> > +		case BPF_JMP | BPF_JSGT | BPF_X:
> > +		case BPF_JMP | BPF_JSLE | BPF_X:
> > +		case BPF_JMP | BPF_JSLT | BPF_X:
> > +		case BPF_JMP | BPF_JSGE | BPF_X:
> > +			/* mov esi,dword ptr [ebp+off] */
> > +			EMIT3(0x8B, add_2reg(0x40, IA32_EBP, tmp[0]),
> > +			      STACK_VAR(src_hi));
> > +			/* cmp dword ptr [ebp+off], esi */
> > +			EMIT3(0x39, add_2reg(0x40, IA32_EBP, tmp[0]),
> > +			      STACK_VAR(dst_hi));
> > +
> > +			EMIT2(IA32_JNE, 6);
> > +			/* mov esi,dword ptr [ebp+off] */
> > +			EMIT3(0x8B, add_2reg(0x40, IA32_EBP, tmp[0]),
> > +			      STACK_VAR(src_lo));
> > +			/* cmp dword ptr [ebp+off], esi */
> > +			EMIT3(0x39, add_2reg(0x40, IA32_EBP, tmp[0]),
> > +			      STACK_VAR(dst_lo));
> > +			goto emit_cond_jmp;
> > +
> > +		case BPF_JMP | BPF_JSET | BPF_X:
> > +			/* mov esi,dword ptr [ebp+off] */
> > +			EMIT3(0x8B, add_2reg(0x40, IA32_EBP, tmp[0]),
> > +			      STACK_VAR(dst_lo));
> > +			/* and esi,dword ptr [ebp+off]*/
> > +			EMIT3(0x23, add_2reg(0x40, IA32_EBP, tmp[0]),
> > +			      STACK_VAR(src_lo));
> > +
> > +			/* mov edi,dword ptr [ebp+off] */
> > +			EMIT3(0x8B, add_2reg(0x40, IA32_EBP, tmp[1]),
> > +			      STACK_VAR(dst_hi));
> > +			/* and edi,dword ptr [ebp+off] */
> > +			EMIT3(0x23, add_2reg(0x40, IA32_EBP, tmp[1]),
> > +			      STACK_VAR(src_hi));
> > +			/* or esi,edi */
> > +			EMIT2(0x09, add_2reg(0xC0, tmp[0], tmp[1]));
> > +			goto emit_cond_jmp;
> > +
> > +		case BPF_JMP | BPF_JSET | BPF_K: {
> > +			u32 hi;
> > +
> > +			hi = imm32 & (1<<31) ? (u32)~0 : 0;
> > +			/* mov esi,imm32 */
> > +			EMIT2_off32(0xC7, add_1reg(0xC0, tmp[0]), imm32);
> > +			/* and esi,dword ptr [ebp+off]*/
> > +			EMIT3(0x23, add_2reg(0x40, IA32_EBP, tmp[0]),
> > +			      STACK_VAR(dst_lo));
> > +
> > +			/* mov esi,imm32 */
> > +			EMIT2_off32(0xC7, add_1reg(0xC0, tmp[1]), hi);
> > +			/* and esi,dword ptr [ebp+off] */
> > +			EMIT3(0x23, add_2reg(0x40, IA32_EBP, tmp[1]),
> > +			      STACK_VAR(dst_hi));
> > +			/* or esi,edi */
> > +			EMIT2(0x09, add_2reg(0xC0, tmp[0], tmp[1]));
> > +			goto emit_cond_jmp;
> > +		}
> > +		case BPF_JMP | BPF_JEQ | BPF_K:
> > +		case BPF_JMP | BPF_JNE | BPF_K:
> > +		case BPF_JMP | BPF_JGT | BPF_K:
> > +		case BPF_JMP | BPF_JLT | BPF_K:
> > +		case BPF_JMP | BPF_JGE | BPF_K:
> > +		case BPF_JMP | BPF_JLE | BPF_K:
> > +		case BPF_JMP | BPF_JSGT | BPF_K:
> > +		case BPF_JMP | BPF_JSLE | BPF_K:
> > +		case BPF_JMP | BPF_JSLT | BPF_K:
> > +		case BPF_JMP | BPF_JSGE | BPF_K: {
> > +			u32 hi;
> > +
> > +			hi = imm32 & (1<<31) ? (u32)~0 : 0;
> > +			/* mov esi,imm32 */
> > +			EMIT2_off32(0xC7, add_1reg(0xC0, tmp[0]), hi);
> > +			/* cmp dword ptr [ebp+off],esi */
> > +			EMIT3(0x39, add_2reg(0x40, IA32_EBP, tmp[0]),
> > +			      STACK_VAR(dst_hi));
> > +
> > +			EMIT2(IA32_JNE, 6);
> > +			/* mov esi,imm32 */
> > +			EMIT2_off32(0xC7, add_1reg(0xC0, tmp[0]), imm32);
> > +			/* cmp dword ptr [ebp+off],esi */
> > +			EMIT3(0x39, add_2reg(0x40, IA32_EBP, tmp[0]),
> > +			      STACK_VAR(dst_lo));
> > +
> > +emit_cond_jmp:		/* convert BPF opcode to x86 */
> > +			switch (BPF_OP(code)) {
> > +			case BPF_JEQ:
> > +				jmp_cond = IA32_JE;
> > +				break;
> > +			case BPF_JSET:
> > +			case BPF_JNE:
> > +				jmp_cond = IA32_JNE;
> > +				break;
> > +			case BPF_JGT:
> > +				/* GT is unsigned '>', JA in x86 */
> > +				jmp_cond = IA32_JA;
> > +				break;
> > +			case BPF_JLT:
> > +				/* LT is unsigned '<', JB in x86 */
> > +				jmp_cond = IA32_JB;
> > +				break;
> > +			case BPF_JGE:
> > +				/* GE is unsigned '>=', JAE in x86 */
> > +				jmp_cond = IA32_JAE;
> > +				break;
> > +			case BPF_JLE:
> > +				/* LE is unsigned '<=', JBE in x86 */
> > +				jmp_cond = IA32_JBE;
> > +				break;
> > +			case BPF_JSGT:
> > +				/* signed '>', GT in x86 */
> > +				jmp_cond = IA32_JG;
> > +				break;
> > +			case BPF_JSLT:
> > +				/* signed '<', LT in x86 */
> > +				jmp_cond = IA32_JL;
> > +				break;
> > +			case BPF_JSGE:
> > +				/* signed '>=', GE in x86 */
> > +				jmp_cond = IA32_JGE;
> > +				break;
> > +			case BPF_JSLE:
> > +				/* signed '<=', LE in x86 */
> > +				jmp_cond = IA32_JLE;
> > +				break;
> > +			default: /* to silence gcc warning */
> > +				return -EFAULT;
> > +			}
> > +			jmp_offset = addrs[i + insn->off] - addrs[i];
> > +			if (is_imm8(jmp_offset)) {
> > +				EMIT2(jmp_cond, jmp_offset);
> > +			} else if (is_simm32(jmp_offset)) {
> > +				EMIT2_off32(0x0F, jmp_cond + 0x10, jmp_offset);
> > +			} else {
> > +				pr_err("cond_jmp gen bug %llx\n", jmp_offset);
> > +				return -EFAULT;
> > +			}
> > +
> > +			break;
> > +		}
> > +		case BPF_JMP | BPF_JA:
> > +			jmp_offset = addrs[i + insn->off] - addrs[i];
> > +			if (!jmp_offset)
> > +				/* optimize out nop jumps */
> > +				break;
> > +emit_jmp:
> > +			if (is_imm8(jmp_offset)) {
> > +				EMIT2(0xEB, jmp_offset);
> > +			} else if (is_simm32(jmp_offset)) {
> > +				EMIT1_off32(0xE9, jmp_offset);
> > +			} else {
> > +				pr_err("jmp gen bug %llx\n", jmp_offset);
> > +				return -EFAULT;
> > +			}
> > +			break;
> > +
> > +		case BPF_LD | BPF_IND | BPF_W:
> > +			func = sk_load_word;
> > +			goto common_load;
> > +		case BPF_LD | BPF_ABS | BPF_W:
> > +			func = CHOOSE_LOAD_FUNC(imm32, sk_load_word);
> > +common_load:
> > +			jmp_offset = func - (image + addrs[i]);
> > +			if (!func || !is_simm32(jmp_offset)) {
> > +				pr_err("unsupported bpf func %d addr %p image %p\n",
> > +				       imm32, func, image);
> > +				return -EINVAL;
> > +			}
> > +			if (BPF_MODE(code) == BPF_ABS) {
> > +				/* mov %edx, imm32 */
> > +				EMIT1_off32(0xBA, imm32);
> > +			} else {
> > +				/* mov edx,dword ptr [ebp+off] */
> > +				EMIT3(0x8B, add_2reg(0x40, IA32_EBP, IA32_EDX),
> > +				      STACK_VAR(src_lo));
> > +				if (imm32) {
> > +					if (is_imm8(imm32))
> > +						/* add %edx, imm8 */
> > +						EMIT3(0x83, 0xC2, imm32);
> > +					else
> > +						/* add %edx, imm32 */
> > +						EMIT2_off32(0x81, 0xC2, imm32);
> > +				}
> > +			}
> > +			emit_load_skb_data_hlen(&prog);
> 
> The only place where you call the emit_load_skb_data_hlen() is here, so it
> effectively doesn't cache anything as with the other JITs. I would suggest
> to keep the complexity of the BPF_ABS/IND rather very low, remove the
> arch/x86/net/bpf_jit32.S handling altogether and just follow the model the
> way arm64 implemented this in the arch/arm64/net/bpf_jit_comp.c JIT. For
> eBPF these instructions are legacy and have been source of many subtle
> bugs in the various BPF JITs in the past, plus nobody complained about the
> current interpreter speed for LD_ABS/IND on x86_32 anyway so far. ;) We're
> also very close to have a rewrite of LD_ABS/IND out of native eBPF with
> similar performance to be able to finally remove them from the core.
Done.

Thanks.

^ permalink raw reply

* [PATCH net 1/3] net: mvpp2: Fix clk error path in mvpp2_probe
From: Maxime Chevallier @ 2018-04-25 11:07 UTC (permalink / raw)
  To: davem
  Cc: ymarkman, Antoine Tenart, netdev, gregory.clement, linux-kernel,
	Maxime Chevallier, nadavh, linux-arm-kernel, thomas.petazzoni,
	miquel.raynal, stefanc, mw, linux
In-Reply-To: <20180425110731.20153-1-maxime.chevallier@bootlin.com>

When clk_prepare_enable fails for the axi_clk, the mg_clk isn't properly
cleaned up. Add another jump label to handle that case, and make sure we
jump to it in the later error cases.

Fixes: 4792ea04bcd0 ("net: mvpp2: Fix clock resource by adding an optional bus clock")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/marvell/mvpp2.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
index 4202f9b5b966..0c2f04813d42 100644
--- a/drivers/net/ethernet/marvell/mvpp2.c
+++ b/drivers/net/ethernet/marvell/mvpp2.c
@@ -8774,12 +8774,12 @@ static int mvpp2_probe(struct platform_device *pdev)
 		if (IS_ERR(priv->axi_clk)) {
 			err = PTR_ERR(priv->axi_clk);
 			if (err == -EPROBE_DEFER)
-				goto err_gop_clk;
+				goto err_mg_clk;
 			priv->axi_clk = NULL;
 		} else {
 			err = clk_prepare_enable(priv->axi_clk);
 			if (err < 0)
-				goto err_gop_clk;
+				goto err_mg_clk;
 		}
 
 		/* Get system's tclk rate */
@@ -8793,7 +8793,7 @@ static int mvpp2_probe(struct platform_device *pdev)
 	if (priv->hw_version == MVPP22) {
 		err = dma_set_mask(&pdev->dev, MVPP2_DESC_DMA_MASK);
 		if (err)
-			goto err_mg_clk;
+			goto err_axi_clk;
 		/* Sadly, the BM pools all share the same register to
 		 * store the high 32 bits of their address. So they
 		 * must all have the same high 32 bits, which forces
@@ -8801,14 +8801,14 @@ static int mvpp2_probe(struct platform_device *pdev)
 		 */
 		err = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
 		if (err)
-			goto err_mg_clk;
+			goto err_axi_clk;
 	}
 
 	/* Initialize network controller */
 	err = mvpp2_init(pdev, priv);
 	if (err < 0) {
 		dev_err(&pdev->dev, "failed to initialize controller\n");
-		goto err_mg_clk;
+		goto err_axi_clk;
 	}
 
 	/* Initialize ports */
@@ -8821,7 +8821,7 @@ static int mvpp2_probe(struct platform_device *pdev)
 	if (priv->port_count == 0) {
 		dev_err(&pdev->dev, "no ports enabled\n");
 		err = -ENODEV;
-		goto err_mg_clk;
+		goto err_axi_clk;
 	}
 
 	/* Statistics must be gathered regularly because some of them (like
@@ -8849,8 +8849,9 @@ static int mvpp2_probe(struct platform_device *pdev)
 			mvpp2_port_remove(priv->port_list[i]);
 		i++;
 	}
-err_mg_clk:
+err_axi_clk:
 	clk_disable_unprepare(priv->axi_clk);
+err_mg_clk:
 	if (priv->hw_version == MVPP22)
 		clk_disable_unprepare(priv->mg_clk);
 err_gop_clk:
-- 
2.11.0

^ permalink raw reply related

* [PATCH net 0/3] net: mvpp2: Fix hangs when starting some interfaces on 7k/8k
From: Maxime Chevallier @ 2018-04-25 11:07 UTC (permalink / raw)
  To: davem
  Cc: Maxime Chevallier, netdev, linux-kernel, Antoine Tenart,
	thomas.petazzoni, gregory.clement, miquel.raynal, nadavh, stefanc,
	ymarkman, mw, linux, linux-arm-kernel

Armada 7K / 8K clock management has recently been reworked, see :

commit c7e92def1ef4 ("clk: mvebu: cp110: Fix clock tree representation")

I have been experiencing overall system hangs on MacchiatoBin when starting
the eth1 interface since then. It turns out some clocks dependencies were
missing in the PPv2 and xmdio driver, the clock rework made this visible.

This series adds the missing clocks in the CP-110 DT bindings for these 2
controllers, updating the documentation accordingly. It also adds support
for the missing 'MG Core clock' in mvpp2.

Thanks to Gregory Clement for finding the root cause of this bug.

Maxime Chevallier (3):
  net: mvpp2: Fix clk error path in mvpp2_probe
  net: mvpp2: Fix clock resource by adding missing mg_core_clk
  ARM64: dts: marvell: armada-cp110: Add clocks for the xmdio node

 .../devicetree/bindings/net/marvell-pp2.txt        |  9 ++++---
 arch/arm64/boot/dts/marvell/armada-cp110.dtsi      |  7 +++--
 drivers/net/ethernet/marvell/mvpp2.c               | 31 +++++++++++++++++-----
 3 files changed, 34 insertions(+), 13 deletions(-)

-- 
2.11.0

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox