Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [GIT] Networking
@ 2018-08-05  7:47 David Miller
  2018-08-05 15:52 ` Linus Torvalds
  0 siblings, 1 reply; 4+ messages in thread
From: David Miller @ 2018-08-05  7:47 UTC (permalink / raw)
  To: torvalds; +Cc: akpm, netdev, linux-kernel


1) Handle frames in error situations properly in AF_XDP, from Jakub
   Kicinski.

2) tcp_mmap test case only tests ipv6 due to a thinko, fix from
   Maninder Singh.

3) Session refcnt fix in l2tp_ppp, from Guillaume Nault.

4) Fix regression in netlink bind handling of multicast
   gruops, from Dmitry Safonov.

Please pull, thanks a lot!

The following changes since commit e30cb13c5a09ff5f043a6570c32e49b063bea6a1:

  Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2018-08-02 10:12:02 -0700)

are available in the Git repository at:

  gitolite@ra.kernel.org:/pub/scm/linux/kernel/git/davem/net.git 

for you to fetch changes up to 91874ecf32e41b5d86a4cb9d60e0bee50d828058:

  netlink: Don't shift on 64 for ngroups (2018-08-04 17:52:51 -0700)

----------------------------------------------------------------
Colin Ian King (1):
      drivers: net: lmc: fix case value for target abort error

David S. Miller (2):
      Merge branch 'mlxsw-Fix-ACL-actions-error-condition-handling'
      Merge git://git.kernel.org/.../bpf/bpf

Dmitry Safonov (1):
      netlink: Don't shift on 64 for ngroups

Guillaume Nault (1):
      l2tp: fix missing refcount drop in pppol2tp_tunnel_ioctl()

Jakub Kicinski (1):
      net: xsk: don't return frames via the allocator on error

Maninder Singh (1):
      selftest/net: fix protocol family to work for IPv4.

Mathieu Xhonneux (1):
      selftests/bpf: update test_lwt_seg6local.sh according to iproute2

Nir Dotan (4):
      mlxsw: core_acl_flex_actions: Return error for conflicting actions
      mlxsw: core_acl_flex_actions: Remove redundant resource destruction
      mlxsw: core_acl_flex_actions: Remove redundant counter destruction
      mlxsw: core_acl_flex_actions: Remove redundant mirror resource destruction

Ursula Braun (1):
      net/smc: no cursor update send in state SMC_INIT

Yonghong Song (1):
      tools/bpftool: fix a percpu_array map dump problem

 drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c | 51 +++++++++++++++++++++++++++++----------------------
 drivers/net/wan/lmc/lmc_main.c                              |  2 +-
 net/l2tp/l2tp_ppp.c                                         | 13 +++++++++----
 net/netlink/af_netlink.c                                    |  4 ++--
 net/smc/smc_cdc.c                                           |  3 ++-
 net/xdp/xsk.c                                               |  4 +---
 tools/bpf/bpftool/map.c                                     | 14 +++++++++-----
 tools/testing/selftests/bpf/test_lwt_seg6local.sh           |  6 +++---
 tools/testing/selftests/net/tcp_mmap.c                      |  2 +-
 9 files changed, 57 insertions(+), 42 deletions(-)

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [GIT] Networking
  2018-08-05  7:47 [GIT] Networking David Miller
@ 2018-08-05 15:52 ` Linus Torvalds
  2018-08-06  2:19   ` [PATCH] checkpatch: warn on unnecessary int declarations Joe Perches
  2018-08-07 17:56   ` [GIT] Networking Dmitry Safonov
  0 siblings, 2 replies; 4+ messages in thread
From: Linus Torvalds @ 2018-08-05 15:52 UTC (permalink / raw)
  To: David Miller
  Cc: Andrew Morton, Network Development, Linux Kernel Mailing List

On Sun, Aug 5, 2018 at 12:47 AM David Miller <davem@davemloft.net> wrote:
>
> 4) Fix regression in netlink bind handling of multicast
>    gruops, from Dmitry Safonov.

This one is written kind of stupidly.

The code went from the original

        groups &= (1UL << nlk->ngroups) - 1;

(which is incorrect for large values of nlk->ngroups) to the fixed

        if (nlk->ngroups == 0)
                groups = 0;
        else if (nlk->ngroups < 8*sizeof(groups))
                groups &= (1UL << nlk->ngroups) - 1;

which isn't technically incorrect...

But it is stupid.

Why stupid? Because the test for 0 is pointless.

Just doing

        if (nlk->ngroups < 8*sizeof(groups))
                groups &= (1UL << nlk->ngroups) - 1;

would have been fine and more understandable, since the "mask by shift
count" already does the right thing for a ngroups value of 0. Now that
test for zero makes me go "what's special about zero?". It turns out
that the answer to that is "nothing".

I certainly didn't care enough to fix it up, and maybe the compiler is
even smart enough to remove the unnecessary test for zero, but it's
kind of sad to see this kind of "people didn't think the code through"
patch this late in the rc.

I'll be making an rc8 today anyway, but I did want to just point to it
and say "hey guys, the code is unnecessarily stupid and overly
complicated".

The type of "groups" is kind of silly too.

Yeah, "long unsigned int" isn't _technically_ wrong. But we normally
call that type "unsigned long".

And comparing against "8*sizeof(groups)" is misleading too, when the
actual masking expression works and is correct in "unsigned long"
because that's the type of the actual mask we're computing (due to the
"1UL").

So _regardless_ of the type of "groups" itself, the mask is actually
correct in unsigned long. I personally think it would have been more
legible as just

        unsigned long groups;
        ...
        if (nlk->ngroups < BITS_PER_LONG)
                groups &= (1UL << nlk->ngroups) - 1;

but by now I'm just nitpicking.

                  Linus

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH] checkpatch: warn on unnecessary int declarations
  2018-08-05 15:52 ` Linus Torvalds
@ 2018-08-06  2:19   ` Joe Perches
  2018-08-07 17:56   ` [GIT] Networking Dmitry Safonov
  1 sibling, 0 replies; 4+ messages in thread
From: Joe Perches @ 2018-08-06  2:19 UTC (permalink / raw)
  To: Linus Torvalds, David Miller, Andrew Morton, Andy Whitcroft
  Cc: Network Development, Linux Kernel Mailing List

On Sun, 2018-08-05 at 08:52 -0700, Linus Torvalds wrote:
> "long unsigned int" isn't _technically_ wrong. But we normally
> call that type "unsigned long".

So add a checkpatch test for it.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ce72cc4784e6..bc6dda34394e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3829,6 +3829,26 @@ sub process {
 			     "type '$tmp' should be specified in [[un]signed] [short|int|long|long long] order\n" . $herecurr);
 		}
 
+# check for unnecessary <signed> int declarations of short/long/long long
+		while ($sline =~ m{\b($TypeMisordered(\s*\*)*|$C90_int_types)\b}g) {
+			my $type = trim($1);
+			next if ($type !~ /\bint\b/);
+			next if ($type !~ /\b(?:short|long\s+long|long)\b/);
+			my $new_type = $type;
+			$new_type =~ s/\b\s*int\s*\b/ /;
+			$new_type =~ s/\b\s*(?:un)?signed\b\s*/ /;
+			$new_type =~ s/^const\s+//;
+			$new_type = "unsigned $new_type" if ($type =~ /\bunsigned\b/);
+			$new_type = "const $new_type" if ($type =~ /^const\b/);
+			$new_type =~ s/\s+/ /g;
+			$new_type = trim($new_type);
+			if (WARN("UNNECESSARY_INT",
+				 "Prefer '$new_type' over '$type' as the int is unnecessary\n" . $herecurr) &&
+			    $fix) {
+				$fixed[$fixlinenr] =~ s/\b\Q$type\E\b/$new_type/;
+			}
+		}
+
 # check for static const char * arrays.
 		if ($line =~ /\bstatic\s+const\s+char\s*\*\s*(\w+)\s*\[\s*\]\s*=\s*/) {
 			WARN("STATIC_CONST_CHAR_ARRAY",

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [GIT] Networking
  2018-08-05 15:52 ` Linus Torvalds
  2018-08-06  2:19   ` [PATCH] checkpatch: warn on unnecessary int declarations Joe Perches
@ 2018-08-07 17:56   ` Dmitry Safonov
  1 sibling, 0 replies; 4+ messages in thread
From: Dmitry Safonov @ 2018-08-07 17:56 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: David Miller, Andrew Morton, Network Development,
	Linux Kernel Mailing List

Hi Linus,

2018-08-05 16:52 GMT+01:00 Linus Torvalds <torvalds@linux-foundation.org>:
> On Sun, Aug 5, 2018 at 12:47 AM David Miller <davem@davemloft.net> wrote:
>>
>> 4) Fix regression in netlink bind handling of multicast
>>    gruops, from Dmitry Safonov.
>
> This one is written kind of stupidly.
>
> The code went from the original
>
>         groups &= (1UL << nlk->ngroups) - 1;
>
> (which is incorrect for large values of nlk->ngroups) to the fixed
>
>         if (nlk->ngroups == 0)
>                 groups = 0;
>         else if (nlk->ngroups < 8*sizeof(groups))
>                 groups &= (1UL << nlk->ngroups) - 1;
>
> which isn't technically incorrect...
>
> But it is stupid.
>
> Why stupid? Because the test for 0 is pointless.

Heh, I've been stupid enough at that moment to think that
(1 << 0 == 1) and forgetting that I'm subtracting 1 for mask.

> Just doing
>
>         if (nlk->ngroups < 8*sizeof(groups))
>                 groups &= (1UL << nlk->ngroups) - 1;
>
> would have been fine and more understandable, since the "mask by shift
> count" already does the right thing for a ngroups value of 0. Now that
> test for zero makes me go "what's special about zero?". It turns out
> that the answer to that is "nothing".
>
> I certainly didn't care enough to fix it up, and maybe the compiler is
> even smart enough to remove the unnecessary test for zero, but it's
> kind of sad to see this kind of "people didn't think the code through"
> patch this late in the rc.

Yes, sorry.

> I'll be making an rc8 today anyway, but I did want to just point to it
> and say "hey guys, the code is unnecessarily stupid and overly
> complicated".
>
> The type of "groups" is kind of silly too.
>
> Yeah, "long unsigned int" isn't _technically_ wrong. But we normally
> call that type "unsigned long".
>
> And comparing against "8*sizeof(groups)" is misleading too, when the
> actual masking expression works and is correct in "unsigned long"
> because that's the type of the actual mask we're computing (due to the
> "1UL").
>
> So _regardless_ of the type of "groups" itself, the mask is actually
> correct in unsigned long. I personally think it would have been more
> legible as just
>
>         unsigned long groups;
>         ...
>         if (nlk->ngroups < BITS_PER_LONG)
>                 groups &= (1UL << nlk->ngroups) - 1;
>
> but by now I'm just nitpicking.

I'll prepare the cleanup for linux-next.

Sorry about the stupid code,
             Dmitry

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-05  7:47 [GIT] Networking David Miller
2018-08-05 15:52 ` Linus Torvalds
2018-08-06  2:19   ` [PATCH] checkpatch: warn on unnecessary int declarations Joe Perches
2018-08-07 17:56   ` [GIT] Networking Dmitry Safonov

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git