[RFC net-next 2/2] chelsio: Move original cxgb driver into ancient subdirectory

[Joe Perches]

This hardware is no longer sold or supported by Chelsio.
The hardware is relatively rare, so move it to an ancient subdirectory.

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/net/ethernet/chelsio/Kconfig| 2 +-
 drivers/net/ethernet/chelsio/Makefile   | 2 +-
 drivers/net/ethernet/chelsio/ancient/Makefile   | 1 +
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/Makefile| 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/common.h| 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/cphy.h  | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/cpl5_cmd.h  | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/cxgb2.c | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/elmer0.h| 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/espi.c  | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/espi.h  | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/fpga_defs.h | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/gmac.h  | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/mv88e1xxx.c | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/mv88e1xxx.h | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/mv88x201x.c | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/my3126.c| 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/pm3393.c| 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/regs.h  | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/sge.c   | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/sge.h   | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/subr.c  | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/suni1x10gexp_regs.h | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/tp.c| 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/tp.h| 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/vsc7326.c   | 0
 drivers/net/ethernet/chelsio/{ => ancient}/cxgb/vsc7326_reg.h   | 0
 27 files changed, 3 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/ethernet/chelsio/ancient/Makefile
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/Makefile (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/common.h (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/cphy.h (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/cpl5_cmd.h (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/cxgb2.c (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/elmer0.h (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/espi.c (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/espi.h (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/fpga_defs.h (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/gmac.h (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/mv88e1xxx.c (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/mv88e1xxx.h (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/mv88x201x.c (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/my3126.c (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/pm3393.c (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/regs.h (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/sge.c (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/sge.h (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/subr.c (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/suni1x10gexp_regs.h 
(100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/tp.c (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/tp.h (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/vsc7326.c (100%)
 rename drivers/net/ethernet/chelsio/{ => ancient}/cxgb/vsc7326_reg.h (100%)

diff --git a/drivers/net/ethernet/chelsio/Kconfig 
b/drivers/net/ethernet/chelsio/Kconfig
index 4d187f2..25f4931 100644
--- a/drivers/net/ethernet/chelsio/Kconfig
+++ b/drivers/net/ethernet/chelsio/Kconfig
@@ -18,7 +18,7 @@ if NET_VENDOR_CHELSIO
 
 config CHELSIO_T1
tristate "Chelsio 10Gb Ethernet support"
-   depends on PCI
+   depends on PCI && ANCIENT_NETDEVICES
select CRC32
select MDIO
---help---
diff --git a/drivers/net/ethernet/chelsio/Makefile 
b/drivers/net/ethernet/chelsio/Makefile
index 390510b..10a8f92 100644
--- a/drivers/net/ethernet/chelsio/Makefile
+++ b/drivers/net/ethernet/chelsio/Makefile
@@ -2,7 +2,7 @@
 # Makefile for the Chelsio network device drivers.
 #
 
-obj-$(CONFIG_CHELSIO_T1) += cxgb/
+obj-y += ancient/
 obj-$(CONFIG_CHELSIO_T3) += cxgb3

[RFC net-next 1/2] drivers/net: Create an ANCIENT_NETDEVICES symbol

[Joe Perches]

Many drivers are ancient and written for hardware that is no longer
available.  These drivers are effectively untested under current
kernels.  Add a symbol that could guard inclusions of these drivers
into modern kernels unless specifically requested.

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/net/Kconfig | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 2a1ba62b..0878363 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -21,6 +21,25 @@ menuconfig NETDEVICES
 
  If unsure, say Y.
 
+menuconfig ANCIENT_NETDEVICES
+  default n
+  depends on NET && NETDEVICES
+  bool "Network ancient device support"
+  ---help---
+  Many old devices are no longer manufactured.
+
+  The drivers for these devices likely worked with the original
+  kernel version when the devices were manufactured, but do not
+  necessarily function well today.
+
+  If these devices still exist and are still functional, the drivers
+  for these devices get scant testing.
+
+  The drivers for these devices are unmaintained and not supported by
+  the vendor.
+
+  If you are unsure that you need drivers for old hardware, say N.
+
 # All the following symbols are dependent on NETDEVICES - do not repeat
 # that for each of the symbols.
 if NETDEVICES
-- 
2.6.3.368.gf34be46

Re: [PATCH] bus: mvebu-mbus: use %pad to print phys_addr_t

[Joe Perches]

On Tue, 2016-03-15 at 11:03 +0100, Arnd Bergmann wrote:
> A recent change to the mbus driver added a warning printk that
> prints a phys_addr_t using the %x format string, which fails in
> case we build with 64-bit phys_addr_t:

Hey Arnd.

This is a bad patch subject, %pad is for a dma_addr_t.
The patch subject made me assume the patch was incorrect.

> > This uses the special %pa format string instead, so we always
> print the correct type.
[]
> - pr_err("invalid dram address 0x%x\n", phyaddr);
> + pr_err("invalid dram address %pa\n", );

The patch is good though.

Re: [PATCH 2/2] isdn: hisax: isac: fixed code style issues.

[Joe Perches]

On Sun, 2016-03-13 at 21:21 +0200, Cosmin-Gabriel Samoila wrote:
> Fixed errors and warnings reported by checkpatch.pl.

Generally it's better to send multiple patches that each
change a specific type of style defect.

As is, this patch changes object code.
Fixing style inconsistency should not do that.

> diff --git a/drivers/isdn/hisax/isac.c b/drivers/isdn/hisax/isac.c
[]
> @@ -24,9 +24,8 @@
>  #define DBUSY_TIMER_VALUE 80
>  #define ARCOFI_USE 1
>  
> -static char *ISACVer[] =
> -{"2086/2186 V1.1", "2085 B1", "2085 B2",
> - "2085 V2.3"};
> +static char *ISACVer[] = {"2086/2186 V1.1", "2085 B1", "2085 B2",
> +"2085 V2.3"};

Most common kernel style would use:

static const char *ISACVer[] = {
"2086/2186 V1.1", "2085 B1", "2085 B2", "2085 V2.3"
};

> @@ -251,11 +252,12 @@ isac_interrupt(struct IsdnCardState *cs, u_char val)
>   cs->tx_skb = NULL;
>   }
>   }
> - if ((cs->tx_skb = skb_dequeue(>sq))) {
> + cs->tx_skb = skb_dequeue(>sq);
> + if (cs->tx_skb) {
>   cs->tx_cnt = 0;
>   isac_fill_fifo(cs);
> - } else
> - schedule_event(cs, D_XMTBUFREADY);
> + }
> + schedule_event(cs, D_XMTBUFREADY);

This changes object code.

Re: [net-next PATCH] csum: Update csum_block_add to use rotate instead of byteswap

[Joe Perches]

On Wed, 2016-03-09 at 08:08 -0800, Alexander Duyck wrote:
> On Tue, Mar 8, 2016 at 10:31 PM, Tom Herbert  wrote:
> > I took a look inlining these.
> > 
> > #define rol32(V, X) ({  \
> > int word = V;   \
> > if (__builtin_constant_p(X))\
> > asm("roll $" #X ",%[word]\n\t"  \
> > : [word] "=r" (word));  \
> > else\
> > asm("roll %%cl,%[word]\n\t" \
> > : [word] "=r" (word)\
> > : "c" (X)); \
> > word;   \
> > })
> > 
> > With this I'm seeing a nice speedup in jhash which uses a lot of rol32s...
> Is gcc really not converting the rol32 calls into rotates?

No, it is.

The difference in the object code with the asm for instance is:

(old, compiled with gcc 5.3.1)

:
 84e:   81 ee 09 41 52 21   sub$0x21524109,%esi
 854:   81 ef 09 41 52 21   sub$0x21524109,%edi
 85a:   55  push   %rbp
 85b:   89 f0   mov%esi,%eax
 85d:   89 f2   mov%esi,%edx
 85f:   48 ff 05 00 00 00 00incq   0x0(%rip)# 866 

 866:   c1 c2 0erol$0xe,%edx
 869:   35 f7 be ad de  xor$0xdeadbef7,%eax
 86e:   48 89 e5mov%rsp,%rbp
 871:   29 d0   sub%edx,%eax
 873:   48 ff 05 00 00 00 00incq   0x0(%rip)# 87a 

 87a:   48 ff 05 00 00 00 00incq   0x0(%rip)# 881 

 881:   89 c2   mov%eax,%edx
 883:   31 c7   xor%eax,%edi
 885:   c1 c2 0brol$0xb,%edx
 888:   29 d7   sub%edx,%edi
 88a:   89 fa   mov%edi,%edx
 88c:   31 fe   xor%edi,%esi
 88e:   c1 ca 07ror$0x7,%edx
 891:   29 d6   sub%edx,%esi
 893:   89 f2   mov%esi,%edx
 895:   31 f0   xor%esi,%eax
 897:   c1 c2 10rol$0x10,%edx
 89a:   29 d0   sub%edx,%eax
 89c:   89 c2   mov%eax,%edx
 89e:   31 c7   xor%eax,%edi
 8a0:   c1 c2 04rol$0x4,%edx
 8a3:   29 d7   sub%edx,%edi
 8a5:   31 fe   xor%edi,%esi
 8a7:   c1 c7 0erol$0xe,%edi
 8aa:   29 fe   sub%edi,%esi
 8ac:   31 f0   xor%esi,%eax
 8ae:   c1 ce 08ror$0x8,%esi
 8b1:   29 f0   sub%esi,%eax
 8b3:   5d  pop%rbp
 8b4:   c3  retq   

vs Tom's asm

084e :
 84e:   81 ee 09 41 52 21   sub$0x21524109,%esi
 854:   8d 87 f7 be ad de   lea-0x21524109(%rdi),%eax
 85a:   55  push   %rbp
 85b:   89 f2   mov%esi,%edx
 85d:   48 ff 05 00 00 00 00incq   0x0(%rip)# 864 

 864:   48 ff 05 00 00 00 00incq   0x0(%rip)# 86b 

 86b:   81 f2 f7 be ad de   xor$0xdeadbef7,%edx
 871:   48 89 e5mov%rsp,%rbp
 874:   c1 c1 0erol$0xe,%ecx
 877:   29 ca   sub%ecx,%edx
 879:   31 d0   xor%edx,%eax
 87b:   c1 c7 0brol$0xb,%edi
 87e:   29 f8   sub%edi,%eax
 880:   48 ff 05 00 00 00 00incq   0x0(%rip)# 887 

 887:   31 c6   xor%eax,%esi
 889:   c1 c7 19rol$0x19,%edi
 88c:   29 fe   sub%edi,%esi
 88e:   31 f2   xor%esi,%edx
 890:   c1 c7 10rol$0x10,%edi
 893:   29 fa   sub%edi,%edx
 895:   31 d0   xor%edx,%eax
 897:   c1 c7 04rol$0x4,%edi
 89a:   29 f8   sub%edi,%eax
 89c:   31 f0   xor%esi,%eax
 89e:   29 c8   sub%ecx,%eax
 8a0:   31 d0   xor%edx,%eax
 8a2:   5d  pop%rbp
 8a3:   c1 c2 18rol$0x18,%edx
 8a6:   29 d0   

Re: [net-next PATCH] csum: Update csum_block_add to use rotate instead of byteswap

[Joe Perches]

On Tue, 2016-03-08 at 21:23 -0800, Alexander Duyck wrote:
> On Tue, Mar 8, 2016 at 3:25 PM, Joe Perches <j...@perches.com> wrote:
> > On Tue, 2016-03-08 at 14:42 -0800, Alexander Duyck wrote:
> > > The code for csum_block_add was doing a funky byteswap to swap the even 
> > > and
> > > odd bytes of the checksum if the offset was odd.  Instead of doing this we
> > > can save ourselves some trouble and just shift by 8 as this should have 
> > > the
> > > same effect in terms of the final checksum value and only requires one
> > > instruction.
> > 3 instructions?
> I was talking about just the one ror vs mov, shl, shr, and ,and, add.
> 
> I assume when you say 3 you are including the test and either some
> form of conditional move or jump?

Yeah, instruction count also depends on architecture (arm/x86/ppc...)

> > > diff --git a/include/net/checksum.h b/include/net/checksum.h
[]
> > > @@ -88,8 +88,10 @@ static inline __wsum
> > >  csum_block_add(__wsum csum, __wsum csum2, int offset)
> > >  {
> > >   u32 sum = (__force u32)csum2;
> > > - if (offset&1)
> > > - sum = ((sum&0xFF00FF)<<8)+((sum>>8)&0xFF00FF);
> > > +
> > > + if (offset & 1)
> > > + sum = (sum << 24) + (sum >> 8);
> > Maybe use ror32(sum, 8);
> I was actually thinking I could use something like this.  I didn't
> realize it was even available.

Now you know: bitops.h

> > or maybe something like:
> > 
> > {
> > u32 sum;
> > 
> > /* rotated csum2 of odd offset will be the right checksum */
> > if (offset & 1)
> > sum = ror32((__force u32)csum2, 8);
> > else
> > sum = (__force u32)csum2;
> > 
> Any specific reason for breaking it up like this?  It seems like it
> was easier to just have sum be assigned first and then rotating it if
> needed.  What is gained by splitting the assignment up over two
> different calls?

It's only for reader clarity where a comment could be useful.
The compiler output shouldn't change.

Re: [net-next PATCH] csum: Update csum_block_add to use rotate instead of byteswap

[Joe Perches]

On Tue, 2016-03-08 at 14:42 -0800, Alexander Duyck wrote:
> The code for csum_block_add was doing a funky byteswap to swap the even and
> odd bytes of the checksum if the offset was odd.  Instead of doing this we
> can save ourselves some trouble and just shift by 8 as this should have the
> same effect in terms of the final checksum value and only requires one
> instruction.

3 instructions?

> In addition we can update csum_block_sub to just use csum_block_add with a
> inverse value for csum2.  This way we follow the same code path as
> csum_block_add without having to duplicate it.
> 
> Signed-off-by: Alexander Duyck 
> ---
>  include/net/checksum.h |   11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/checksum.h b/include/net/checksum.h
> index 10a16b5bd1c7..f9fac66c0e66 100644
> --- a/include/net/checksum.h
> +++ b/include/net/checksum.h
> @@ -88,8 +88,10 @@ static inline __wsum
>  csum_block_add(__wsum csum, __wsum csum2, int offset)
>  {
>   u32 sum = (__force u32)csum2;
> - if (offset&1)
> - sum = ((sum&0xFF00FF)<<8)+((sum>>8)&0xFF00FF);
> +
> + if (offset & 1)
> + sum = (sum << 24) + (sum >> 8);

Maybe use ror32(sum, 8);

or maybe something like:

{
u32 sum;

/* rotated csum2 of odd offset will be the right checksum */
if (offset & 1)
sum = ror32((__force u32)csum2, 8);
else
sum = (__force u32)csum2;

[PATCH] cisco: enic: Update logging macros and uses

[Joe Perches]

Don't hide varibles used by the logging macros.

Miscellanea:

o Use the more common ##__VA_ARGS__ extension
o Add missing newlines to formats
o Realign arguments

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/net/ethernet/cisco/enic/enic.h  | 22 --
 drivers/net/ethernet/cisco/enic/vnic_cq.c   |  2 +-
 drivers/net/ethernet/cisco/enic/vnic_dev.c  | 45 +++--
 drivers/net/ethernet/cisco/enic/vnic_intr.c |  3 +-
 drivers/net/ethernet/cisco/enic/vnic_rq.c   |  4 +--
 drivers/net/ethernet/cisco/enic/vnic_wq.c   |  4 +--
 6 files changed, 43 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic.h 
b/drivers/net/ethernet/cisco/enic/enic.h
index 7ba6d53..130f910 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -201,16 +201,20 @@ static inline struct net_device *vnic_get_netdev(struct 
vnic_dev *vdev)
 }
 
 /* wrappers function for kernel log
- * Make sure variable vdev of struct vnic_dev is available in the block where
- * these macros are used
  */
-#define vdev_info(args...) dev_info(>pdev->dev, args)
-#define vdev_warn(args...) dev_warn(>pdev->dev, args)
-#define vdev_err(args...)  dev_err(>pdev->dev, args)
-
-#define vdev_netinfo(args...)  netdev_info(vnic_get_netdev(vdev), args)
-#define vdev_netwarn(args...)  netdev_warn(vnic_get_netdev(vdev), args)
-#define vdev_neterr(args...)   netdev_err(vnic_get_netdev(vdev), args)
+#define vdev_err(vdev, fmt, ...)   \
+   dev_err(&(vdev)->pdev->dev, fmt, ##__VA_ARGS__)
+#define vdev_warn(vdev, fmt, ...)  \
+   dev_warn(&(vdev)->pdev->dev, fmt, ##__VA_ARGS__)
+#define vdev_info(vdev, fmt, ...)  \
+   dev_info(&(vdev)->pdev->dev, fmt, ##__VA_ARGS__)
+
+#define vdev_neterr(vdev, fmt, ...)\
+   netdev_err(vnic_get_netdev(vdev), fmt, ##__VA_ARGS__)
+#define vdev_netwarn(vdev, fmt, ...)   \
+   netdev_warn(vnic_get_netdev(vdev), fmt, ##__VA_ARGS__)
+#define vdev_netinfo(vdev, fmt, ...)   \
+   netdev_info(vnic_get_netdev(vdev), fmt, ##__VA_ARGS__)
 
 static inline struct device *enic_get_dev(struct enic *enic)
 {
diff --git a/drivers/net/ethernet/cisco/enic/vnic_cq.c 
b/drivers/net/ethernet/cisco/enic/vnic_cq.c
index abeda2a..9c682af 100644
--- a/drivers/net/ethernet/cisco/enic/vnic_cq.c
+++ b/drivers/net/ethernet/cisco/enic/vnic_cq.c
@@ -43,7 +43,7 @@ int vnic_cq_alloc(struct vnic_dev *vdev, struct vnic_cq *cq, 
unsigned int index,
 
cq->ctrl = vnic_dev_get_res(vdev, RES_TYPE_CQ, index);
if (!cq->ctrl) {
-   vdev_err("Failed to hook CQ[%d] resource\n", index);
+   vdev_err(vdev, "Failed to hook CQ[%d] resource\n", index);
return -EINVAL;
}
 
diff --git a/drivers/net/ethernet/cisco/enic/vnic_dev.c 
b/drivers/net/ethernet/cisco/enic/vnic_dev.c
index 1fdf5fe..8f27df3 100644
--- a/drivers/net/ethernet/cisco/enic/vnic_dev.c
+++ b/drivers/net/ethernet/cisco/enic/vnic_dev.c
@@ -53,14 +53,14 @@ static int vnic_dev_discover_res(struct vnic_dev *vdev,
return -EINVAL;
 
if (bar->len < VNIC_MAX_RES_HDR_SIZE) {
-   vdev_err("vNIC BAR0 res hdr length error\n");
+   vdev_err(vdev, "vNIC BAR0 res hdr length error\n");
return -EINVAL;
}
 
rh  = bar->vaddr;
mrh = bar->vaddr;
if (!rh) {
-   vdev_err("vNIC BAR0 res hdr not mem-mapped\n");
+   vdev_err(vdev, "vNIC BAR0 res hdr not mem-mapped\n");
return -EINVAL;
}
 
@@ -69,7 +69,7 @@ static int vnic_dev_discover_res(struct vnic_dev *vdev,
(ioread32(>version) != VNIC_RES_VERSION)) {
if ((ioread32(>magic) != MGMTVNIC_MAGIC) ||
(ioread32(>version) != MGMTVNIC_VERSION)) {
-   vdev_err("vNIC BAR0 res magic/version error exp 
(%lx/%lx) or (%lx/%lx), curr (%x/%x)\n",
+   vdev_err(vdev, "vNIC BAR0 res magic/version error exp 
(%lx/%lx) or (%lx/%lx), curr (%x/%x)\n",
 VNIC_RES_MAGIC, VNIC_RES_VERSION,
 MGMTVNIC_MAGIC, MGMTVNIC_VERSION,
 ioread32(>magic), ioread32(>version));
@@ -106,7 +106,7 @@ static int vnic_dev_discover_res(struct vnic_dev *vdev,
/* each count is stride bytes long */
len = count * VNIC_RES_STRIDE;
if (len + bar_offset > bar[bar_num].len) {
-   vdev_err("vNIC BAR0 resource %d out-of-bound

[PATCH 4/5] ti: wl1251: Convert wl1251_notice to wiphy_info/pr_info

[Joe Perches]

Use the more common logging mechanisms.

Convert to wiphy_info as these wl1251_notice uses were actually
emitted at KERN_INFO.

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/net/wireless/ti/wl1251/main.c   | 4 ++--
 drivers/net/wireless/ti/wl1251/sdio.c   | 2 +-
 drivers/net/wireless/ti/wl1251/wl1251.h | 3 ---
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/main.c 
b/drivers/net/wireless/ti/wl1251/main.c
index a0576e7..593bed9 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -1469,7 +1469,7 @@ static int wl1251_register_hw(struct wl1251 *wl)
 
wl->mac80211_registered = true;
 
-   wl1251_notice("loaded");
+   wiphy_info(wl->hw->wiphy, "loaded\n");
 
return 0;
 }
@@ -1503,7 +1503,7 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
goto out;
 
wl1251_debugfs_init(wl);
-   wl1251_notice("initialized");
+   wiphy_info(wl->hw->wiphy, "initialized\n");
 
ret = 0;
 
diff --git a/drivers/net/wireless/ti/wl1251/sdio.c 
b/drivers/net/wireless/ti/wl1251/sdio.c
index f48e985..cc52a97 100644
--- a/drivers/net/wireless/ti/wl1251/sdio.c
+++ b/drivers/net/wireless/ti/wl1251/sdio.c
@@ -382,7 +382,7 @@ static int __init wl1251_sdio_init(void)
 static void __exit wl1251_sdio_exit(void)
 {
sdio_unregister_driver(_sdio_driver);
-   wl1251_notice("unloaded");
+   pr_info("unloaded\n");
 }
 
 module_init(wl1251_sdio_init);
diff --git a/drivers/net/wireless/ti/wl1251/wl1251.h 
b/drivers/net/wireless/ti/wl1251/wl1251.h
index 5d520d2..62a40cc 100644
--- a/drivers/net/wireless/ti/wl1251/wl1251.h
+++ b/drivers/net/wireless/ti/wl1251/wl1251.h
@@ -54,9 +54,6 @@ enum {
 
 #define DEBUG_DUMP_LIMIT 1024
 
-#define wl1251_notice(fmt, arg...) \
-   printk(KERN_INFO DRIVER_PREFIX fmt "\n", ##arg)
-
 #define wl1251_info(fmt, arg...) \
printk(KERN_DEBUG DRIVER_PREFIX fmt "\n", ##arg)
 
-- 
2.6.3.368.gf34be46

[PATCH 2/5] ti: wl1251: Convert wl1251_error to wiphy_err/pr_err

[Joe Perches]

Use the more common logging mechanisms.

Miscellanea:

o Coalesce formats
o Realign arguments
o Reflow to fit 80 columns
o Add #define pr_fmt when pr_ is used

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/net/wireless/ti/wl1251/acx.c|  6 ++---
 drivers/net/wireless/ti/wl1251/boot.c   | 18 +++---
 drivers/net/wireless/ti/wl1251/cmd.c| 42 +
 drivers/net/wireless/ti/wl1251/event.c  |  2 +-
 drivers/net/wireless/ti/wl1251/init.c   | 13 +-
 drivers/net/wireless/ti/wl1251/io.c |  3 ++-
 drivers/net/wireless/ti/wl1251/main.c   | 41 ++--
 drivers/net/wireless/ti/wl1251/ps.c |  2 +-
 drivers/net/wireless/ti/wl1251/rx.c |  2 +-
 drivers/net/wireless/ti/wl1251/sdio.c   | 19 +--
 drivers/net/wireless/ti/wl1251/spi.c| 19 ---
 drivers/net/wireless/ti/wl1251/tx.c |  5 ++--
 drivers/net/wireless/ti/wl1251/wl1251.h |  3 ---
 13 files changed, 93 insertions(+), 82 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/acx.c 
b/drivers/net/wireless/ti/wl1251/acx.c
index d6fbdda..23b4882 100644
--- a/drivers/net/wireless/ti/wl1251/acx.c
+++ b/drivers/net/wireless/ti/wl1251/acx.c
@@ -28,7 +28,7 @@ int wl1251_acx_frame_rates(struct wl1251 *wl, u8 ctrl_rate, 
u8 ctrl_mod,
ret = wl1251_cmd_configure(wl, ACX_FW_GEN_FRAME_RATES,
   rates, sizeof(*rates));
if (ret < 0) {
-   wl1251_error("Failed to set FW rates and modulation");
+   wiphy_err(wl->hw->wiphy, "Failed to set FW rates and 
modulation\n");
goto out;
}
 
@@ -74,7 +74,7 @@ int wl1251_acx_default_key(struct wl1251 *wl, u8 key_id)
ret = wl1251_cmd_configure(wl, DOT11_DEFAULT_KEY,
   default_key, sizeof(*default_key));
if (ret < 0) {
-   wl1251_error("Couldn't set default key");
+   wiphy_err(wl->hw->wiphy, "Couldn't set default key\n");
goto out;
}
 
@@ -208,7 +208,7 @@ int wl1251_acx_feature_cfg(struct wl1251 *wl, u32 
data_flow_options)
ret = wl1251_cmd_configure(wl, ACX_FEATURE_CFG,
   feature, sizeof(*feature));
if (ret < 0) {
-   wl1251_error("Couldn't set HW encryption");
+   wiphy_err(wl->hw->wiphy, "Couldn't set HW encryption\n");
goto out;
}
 
diff --git a/drivers/net/wireless/ti/wl1251/boot.c 
b/drivers/net/wireless/ti/wl1251/boot.c
index 2000cd5..456629a 100644
--- a/drivers/net/wireless/ti/wl1251/boot.c
+++ b/drivers/net/wireless/ti/wl1251/boot.c
@@ -53,7 +53,7 @@ int wl1251_boot_soft_reset(struct wl1251 *wl)
if (time_after(jiffies, timeout)) {
/* 1.2 check pWhalBus->uSelfClearTime if the
 * timeout was reached */
-   wl1251_error("soft reset timeout");
+   wiphy_err(wl->hw->wiphy, "soft reset timeout\n");
return -1;
}
 
@@ -231,7 +231,7 @@ int wl1251_boot_run_firmware(struct wl1251 *wl)
wl1251_debug(DEBUG_BOOT, "chip id after firmware boot: 0x%x", chip_id);
 
if (chip_id != wl->chip_id) {
-   wl1251_error("chip id doesn't match after firmware boot");
+   wiphy_err(wl->hw->wiphy, "chip id doesn't match after firmware 
boot\n");
return -EIO;
}
 
@@ -242,8 +242,7 @@ int wl1251_boot_run_firmware(struct wl1251 *wl)
acx_intr = wl1251_reg_read32(wl, ACX_REG_INTERRUPT_NO_CLEAR);
 
if (acx_intr == 0x) {
-   wl1251_error("error reading hardware complete "
-"init indication");
+   wiphy_err(wl->hw->wiphy, "error reading hardware 
complete init indication\n");
return -EIO;
}
/* check that ACX_INTR_INIT_COMPLETE is enabled */
@@ -255,8 +254,7 @@ int wl1251_boot_run_firmware(struct wl1251 *wl)
}
 
if (loop > INIT_LOOP) {
-   wl1251_error("timeout waiting for the hardware to "
-"complete initialization");
+   wiphy_err(wl->hw->wiphy, "timeout waiting for the hardware to 
complete initialization\n");
return -EIO;
}
 
@@ -304,7 +302,7 @@ int wl1251_boot_run_firmware(struct wl1251 *wl)
 
ret = wl1251_event_unmask(wl);
if (ret < 0) {
-   wl1251_error("EVENT mask setting failed");
+   wiphy_err(wl->hw->wiphy, "EVENT mask setting failed\n");
return ret;
}
 
@@ -333,13 

[PATCH 5/5] ti: wl1251: Convert wl1251_info to wl1251_debug

[Joe Perches]

These logging messages are always emitted at KERN_DEBUG.

Add a DEBUG_ALWAYS enum to the debug type enum and convert the
macro and uses from wl1251_info( to wl1251_debug(DEBUG_ALWAYS,

Miscellanea:

o Remove the now unused wl1251_info macro

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/net/wireless/ti/wl1251/init.c   | 10 +-
 drivers/net/wireless/ti/wl1251/main.c   |  4 ++--
 drivers/net/wireless/ti/wl1251/sdio.c   |  4 ++--
 drivers/net/wireless/ti/wl1251/wl1251.h |  6 ++
 4 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/init.c 
b/drivers/net/wireless/ti/wl1251/init.c
index 796ccf4..29a990d 100644
--- a/drivers/net/wireless/ti/wl1251/init.c
+++ b/drivers/net/wireless/ti/wl1251/init.c
@@ -411,11 +411,11 @@ int wl1251_hw_init(struct wl1251 *wl)
goto out_free_data_path;
 
wl_mem_map = wl->target_mem_map;
-   wl1251_info("%d tx blocks at 0x%x, %d rx blocks at 0x%x",
-   wl_mem_map->num_tx_mem_blocks,
-   wl->data_path->tx_control_addr,
-   wl_mem_map->num_rx_mem_blocks,
-   wl->data_path->rx_control_addr);
+   wl1251_debug(DEBUG_ALWAYS, "%d tx blocks at 0x%x, %d rx blocks at 0x%x",
+wl_mem_map->num_tx_mem_blocks,
+wl->data_path->tx_control_addr,
+wl_mem_map->num_rx_mem_blocks,
+wl->data_path->rx_control_addr);
 
return 0;
 
diff --git a/drivers/net/wireless/ti/wl1251/main.c 
b/drivers/net/wireless/ti/wl1251/main.c
index 593bed9..32b8c98 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -423,7 +423,7 @@ static int wl1251_op_start(struct ieee80211_hw *hw)
 
wl->state = WL1251_STATE_ON;
 
-   wl1251_info("firmware booted (%s)", wl->fw_ver);
+   wl1251_debug(DEBUG_ALWAYS, "firmware booted (%s)", wl->fw_ver);
 
/* update hw/fw version info in wiphy struct */
wiphy->hw_version = wl->chip_id;
@@ -442,7 +442,7 @@ static void wl1251_op_stop(struct ieee80211_hw *hw)
 {
struct wl1251 *wl = hw->priv;
 
-   wl1251_info("down");
+   wl1251_debug(DEBUG_ALWAYS, "down");
 
wl1251_debug(DEBUG_MAC80211, "mac80211 stop");
 
diff --git a/drivers/net/wireless/ti/wl1251/sdio.c 
b/drivers/net/wireless/ti/wl1251/sdio.c
index cc52a97..d2fb7d1 100644
--- a/drivers/net/wireless/ti/wl1251/sdio.c
+++ b/drivers/net/wireless/ti/wl1251/sdio.c
@@ -290,12 +290,12 @@ static int wl1251_sdio_probe(struct sdio_func *func,
wl1251_sdio_ops.enable_irq = wl1251_enable_line_irq;
wl1251_sdio_ops.disable_irq = wl1251_disable_line_irq;
 
-   wl1251_info("using dedicated interrupt line");
+   wl1251_debug(DEBUG_ALWAYS, "using dedicated interrupt line");
} else {
wl1251_sdio_ops.enable_irq = wl1251_sdio_enable_irq;
wl1251_sdio_ops.disable_irq = wl1251_sdio_disable_irq;
 
-   wl1251_info("using SDIO interrupt");
+   wl1251_debug(DEBUG_ALWAYS, "using SDIO interrupt");
}
 
ret = wl1251_init_ieee80211(wl);
diff --git a/drivers/net/wireless/ti/wl1251/wl1251.h 
b/drivers/net/wireless/ti/wl1251/wl1251.h
index 62a40cc..705573f 100644
--- a/drivers/net/wireless/ti/wl1251/wl1251.h
+++ b/drivers/net/wireless/ti/wl1251/wl1251.h
@@ -47,6 +47,7 @@ enum {
DEBUG_MAC80211  = BIT(11),
DEBUG_CMD   = BIT(12),
DEBUG_ACX   = BIT(13),
+   DEBUG_ALWAYS= BIT(31),
DEBUG_ALL   = ~0,
 };
 
@@ -54,12 +55,9 @@ enum {
 
 #define DEBUG_DUMP_LIMIT 1024
 
-#define wl1251_info(fmt, arg...) \
-   printk(KERN_DEBUG DRIVER_PREFIX fmt "\n", ##arg)
-
 #define wl1251_debug(level, fmt, arg...) \
do { \
-   if (level & DEBUG_LEVEL) \
+   if (level == DEBUG_ALWAYS || (level & DEBUG_LEVEL)) \
printk(KERN_DEBUG DRIVER_PREFIX fmt "\n", ##arg); \
} while (0)
 
-- 
2.6.3.368.gf34be46

[PATCH 3/5] ti: wl1251: Convert wl1251_warning to wiphy_warn

[Joe Perches]

Use the more common logging mechanism.

Miscellanea:

o Coalesce formats
o Realign arguments
o Reflow to fit 80 columns

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/net/wireless/ti/wl1251/acx.c| 91 -
 drivers/net/wireless/ti/wl1251/cmd.c| 10 ++--
 drivers/net/wireless/ti/wl1251/init.c   |  4 +-
 drivers/net/wireless/ti/wl1251/main.c   | 26 ++
 drivers/net/wireless/ti/wl1251/rx.c |  4 +-
 drivers/net/wireless/ti/wl1251/tx.c |  4 +-
 drivers/net/wireless/ti/wl1251/wl1251.h |  3 --
 7 files changed, 82 insertions(+), 60 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/acx.c 
b/drivers/net/wireless/ti/wl1251/acx.c
index 23b4882..22d3daa 100644
--- a/drivers/net/wireless/ti/wl1251/acx.c
+++ b/drivers/net/wireless/ti/wl1251/acx.c
@@ -103,7 +103,8 @@ int wl1251_acx_wake_up_conditions(struct wl1251 *wl, u8 
wake_up_event,
ret = wl1251_cmd_configure(wl, ACX_WAKE_UP_CONDITIONS,
   wake_up, sizeof(*wake_up));
if (ret < 0) {
-   wl1251_warning("could not set wake up conditions: %d", ret);
+   wiphy_warn(wl->hw->wiphy, "could not set wake up conditions: 
%d\n",
+  ret);
goto out;
}
 
@@ -144,7 +145,7 @@ int wl1251_acx_fw_version(struct wl1251 *wl, char *buf, 
size_t len)
 
ret = wl1251_cmd_interrogate(wl, ACX_FW_REV, rev, sizeof(*rev));
if (ret < 0) {
-   wl1251_warning("ACX_FW_REV interrogate failed");
+   wiphy_warn(wl->hw->wiphy, "ACX_FW_REV interrogate failed\n");
goto out;
}
 
@@ -181,7 +182,8 @@ int wl1251_acx_tx_power(struct wl1251 *wl, int power)
 
ret = wl1251_cmd_configure(wl, DOT11_CUR_TX_PWR, acx, sizeof(*acx));
if (ret < 0) {
-   wl1251_warning("configure of tx power failed: %d", ret);
+   wiphy_warn(wl->hw->wiphy, "configure of tx power failed: %d\n",
+  ret);
goto out;
}
 
@@ -265,10 +267,11 @@ int wl1251_acx_data_path_params(struct wl1251 *wl,
 resp, sizeof(*resp));
 
if (ret < 0) {
-   wl1251_warning("failed to read data path parameters: %d", ret);
+   wiphy_warn(wl->hw->wiphy, "failed to read data path parameters: 
%d\n",
+  ret);
goto out;
} else if (resp->header.cmd.status != CMD_STATUS_SUCCESS) {
-   wl1251_warning("data path parameter acx status failed");
+   wiphy_warn(wl->hw->wiphy, "data path parameter acx status 
failed\n");
ret = -EIO;
goto out;
}
@@ -293,7 +296,8 @@ int wl1251_acx_rx_msdu_life_time(struct wl1251 *wl, u32 
life_time)
ret = wl1251_cmd_configure(wl, DOT11_RX_MSDU_LIFE_TIME,
   acx, sizeof(*acx));
if (ret < 0) {
-   wl1251_warning("failed to set rx msdu life time: %d", ret);
+   wiphy_warn(wl->hw->wiphy, "failed to set rx msdu life time: 
%d\n",
+  ret);
goto out;
}
 
@@ -319,7 +323,7 @@ int wl1251_acx_rx_config(struct wl1251 *wl, u32 config, u32 
filter)
ret = wl1251_cmd_configure(wl, ACX_RX_CFG,
   rx_config, sizeof(*rx_config));
if (ret < 0) {
-   wl1251_warning("failed to set rx config: %d", ret);
+   wiphy_warn(wl->hw->wiphy, "failed to set rx config: %d\n", ret);
goto out;
}
 
@@ -343,7 +347,8 @@ int wl1251_acx_pd_threshold(struct wl1251 *wl)
 
ret = wl1251_cmd_configure(wl, ACX_PD_THRESHOLD, pd, sizeof(*pd));
if (ret < 0) {
-   wl1251_warning("failed to set pd threshold: %d", ret);
+   wiphy_warn(wl->hw->wiphy, "failed to set pd threshold: %d\n",
+  ret);
goto out;
}
 
@@ -368,7 +373,8 @@ int wl1251_acx_slot(struct wl1251 *wl, enum acx_slot_type 
slot_time)
 
ret = wl1251_cmd_configure(wl, ACX_SLOT, slot, sizeof(*slot));
if (ret < 0) {
-   wl1251_warning("failed to set slot time: %d", ret);
+   wiphy_warn(wl->hw->wiphy, "failed to set slot time: %d\n",
+  ret);
goto out;
}
 
@@ -397,7 +403,8 @@ int wl1251_acx_group_address_tbl(struct wl1251 *wl, bool 
enable,
ret = wl1251_cmd_configure(wl, DOT11_GROUP_ADDRESS_TBL,
   acx, sizeof(*acx));
if (ret < 0) {
-   wl1251_warning("failed to set group addr table: %d", ret);
+   

[PATCH 1/5] ti: Convert wl1271_ logging macros to dev_ or pr_

[Joe Perches]

Use the more common logging mechanism passing wl->dev where
appropriate.  Remove the macros.  Add argument struct wl1271 *wl to
some functions to make these logging mechanisms work.

Miscellanea:

o Coalesce formats, add required trailing \n to formats
  Some formats already had previously incorrect \n uses
o Realign arguments
o Correct a couple typos and grammar defects
o Split a multiple line error message to multiple calls of dev_err
o Add #define pr_fmt when pr_ is used
o Remove unnecessary/duplicate pr_fmt use from wl1271_debug macro

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/net/wireless/ti/wl12xx/acx.c  |   2 +-
 drivers/net/wireless/ti/wl12xx/cmd.c  |  20 +--
 drivers/net/wireless/ti/wl12xx/main.c |  34 ++--
 drivers/net/wireless/ti/wl12xx/scan.c |  24 +--
 drivers/net/wireless/ti/wl18xx/acx.c  |  25 +--
 drivers/net/wireless/ti/wl18xx/cmd.c  |  20 +--
 drivers/net/wireless/ti/wl18xx/debugfs.c  |   2 +-
 drivers/net/wireless/ti/wl18xx/event.c|   8 +-
 drivers/net/wireless/ti/wl18xx/main.c |  50 +++---
 drivers/net/wireless/ti/wl18xx/scan.c |  16 +-
 drivers/net/wireless/ti/wl18xx/tx.c   |   8 +-
 drivers/net/wireless/ti/wlcore/acx.c  | 132 
 drivers/net/wireless/ti/wlcore/boot.c |  45 +++---
 drivers/net/wireless/ti/wlcore/cmd.c  | 103 +++--
 drivers/net/wireless/ti/wlcore/debug.h|  14 +-
 drivers/net/wireless/ti/wlcore/debugfs.c  |  54 +++
 drivers/net/wireless/ti/wlcore/event.c|  14 +-
 drivers/net/wireless/ti/wlcore/main.c | 248 --
 drivers/net/wireless/ti/wlcore/ps.c   |  15 +-
 drivers/net/wireless/ti/wlcore/rx.c   |  26 ++--
 drivers/net/wireless/ti/wlcore/scan.c |   4 +-
 drivers/net/wireless/ti/wlcore/sysfs.c|   8 +-
 drivers/net/wireless/ti/wlcore/testmode.c |  14 +-
 drivers/net/wireless/ti/wlcore/tx.c   |  14 +-
 drivers/net/wireless/ti/wlcore/wlcore_i.h |   3 -
 25 files changed, 464 insertions(+), 439 deletions(-)

diff --git a/drivers/net/wireless/ti/wl12xx/acx.c 
b/drivers/net/wireless/ti/wl12xx/acx.c
index bea06b2..4a11158 100644
--- a/drivers/net/wireless/ti/wl12xx/acx.c
+++ b/drivers/net/wireless/ti/wl12xx/acx.c
@@ -42,7 +42,7 @@ int wl1271_acx_host_if_cfg_bitmap(struct wl1271 *wl, u32 
host_cfg_bitmap)
ret = wl1271_cmd_configure(wl, ACX_HOST_IF_CFG_BITMAP,
   bitmap_conf, sizeof(*bitmap_conf));
if (ret < 0) {
-   wl1271_warning("wl1271 bitmap config opt failed: %d", ret);
+   dev_warn(wl->dev, "wl1271 bitmap config opt failed: %d\n", ret);
goto out;
}
 
diff --git a/drivers/net/wireless/ti/wl12xx/cmd.c 
b/drivers/net/wireless/ti/wl12xx/cmd.c
index 7485dba..8f358d3 100644
--- a/drivers/net/wireless/ti/wl12xx/cmd.c
+++ b/drivers/net/wireless/ti/wl12xx/cmd.c
@@ -54,7 +54,7 @@ int wl1271_cmd_ext_radio_parms(struct wl1271 *wl)
 
ret = wl1271_cmd_test(wl, ext_radio_parms, sizeof(*ext_radio_parms), 0);
if (ret < 0)
-   wl1271_warning("TEST_CMD_INI_FILE_RF_EXTENDED_PARAM failed");
+   dev_warn(wl->dev, "TEST_CMD_INI_FILE_RF_EXTENDED_PARAM 
failed\n");
 
kfree(ext_radio_parms);
return ret;
@@ -73,7 +73,7 @@ int wl1271_cmd_general_parms(struct wl1271 *wl)
return -ENODEV;
 
if (gp->tx_bip_fem_manufacturer >= WL1271_INI_FEM_MODULE_COUNT) {
-   wl1271_warning("FEM index from INI out of bounds");
+   dev_warn(wl->dev, "FEM index from INI out of bounds\n");
return -EINVAL;
}
 
@@ -97,7 +97,7 @@ int wl1271_cmd_general_parms(struct wl1271 *wl)
 
ret = wl1271_cmd_test(wl, gen_parms, sizeof(*gen_parms), answer);
if (ret < 0) {
-   wl1271_warning("CMD_INI_FILE_GENERAL_PARAM failed");
+   dev_warn(wl->dev, "CMD_INI_FILE_GENERAL_PARAM failed\n");
goto out;
}
 
@@ -105,7 +105,7 @@ int wl1271_cmd_general_parms(struct wl1271 *wl)
gen_parms->general_params.tx_bip_fem_manufacturer;
 
if (gp->tx_bip_fem_manufacturer >= WL1271_INI_FEM_MODULE_COUNT) {
-   wl1271_warning("FEM index from FW out of bounds");
+   dev_warn(wl->dev, "FEM index from FW out of bounds\n");
ret = -EINVAL;
goto out;
}
@@ -140,7 +140,7 @@ int wl128x_cmd_general_parms(struct wl1271 *wl)
return -ENODEV;
 
if (gp->tx_bip_fem_manufacturer >= WL1271_INI_FEM_MODULE_COUNT) {
-   wl1271_warning("FEM index from ini out of bounds");
+   dev_warn(wl->dev, "FEM index from ini out of bounds\n");
return -EINVAL;
}
 
@@ -165,7 +165,7 @@ int wl128x_cmd_general_parms(struct

[PATCH 0/5] wireless: ti: Convert specialized logging macros to kernel style

[Joe Perches]

Using the normal kernel logging mechanisms makes this code
a bit more like other wireless drivers.

Joe Perches (5):
  ti: Convert wl1271_ logging macros to dev_ or pr_
  ti: wl1251: Convert wl1251_error to wiphy_err/pr_err
  ti: wl1251: Convert wl1251_warning to wiphy_warn
  ti: wl1251: Convert wl1251_notice to wiphy_info/pr_info
  ti: wl1251: Convert wl1251_info to wl1251_debug

 drivers/net/wireless/ti/wl1251/acx.c  |  97 +++-
 drivers/net/wireless/ti/wl1251/boot.c |  18 +--
 drivers/net/wireless/ti/wl1251/cmd.c  |  52 ---
 drivers/net/wireless/ti/wl1251/event.c|   2 +-
 drivers/net/wireless/ti/wl1251/init.c |  27 ++--
 drivers/net/wireless/ti/wl1251/io.c   |   3 +-
 drivers/net/wireless/ti/wl1251/main.c |  75 +
 drivers/net/wireless/ti/wl1251/ps.c   |   2 +-
 drivers/net/wireless/ti/wl1251/rx.c   |   6 +-
 drivers/net/wireless/ti/wl1251/sdio.c |  25 +--
 drivers/net/wireless/ti/wl1251/spi.c  |  19 +--
 drivers/net/wireless/ti/wl1251/tx.c   |   9 +-
 drivers/net/wireless/ti/wl1251/wl1251.h   |  15 +-
 drivers/net/wireless/ti/wl12xx/acx.c  |   2 +-
 drivers/net/wireless/ti/wl12xx/cmd.c  |  20 +--
 drivers/net/wireless/ti/wl12xx/main.c |  34 ++--
 drivers/net/wireless/ti/wl12xx/scan.c |  24 +--
 drivers/net/wireless/ti/wl18xx/acx.c  |  25 +--
 drivers/net/wireless/ti/wl18xx/cmd.c  |  20 +--
 drivers/net/wireless/ti/wl18xx/debugfs.c  |   2 +-
 drivers/net/wireless/ti/wl18xx/event.c|   8 +-
 drivers/net/wireless/ti/wl18xx/main.c |  50 +++---
 drivers/net/wireless/ti/wl18xx/scan.c |  16 +-
 drivers/net/wireless/ti/wl18xx/tx.c   |   8 +-
 drivers/net/wireless/ti/wlcore/acx.c  | 132 
 drivers/net/wireless/ti/wlcore/boot.c |  45 +++---
 drivers/net/wireless/ti/wlcore/cmd.c  | 103 +++--
 drivers/net/wireless/ti/wlcore/debug.h|  14 +-
 drivers/net/wireless/ti/wlcore/debugfs.c  |  54 +++
 drivers/net/wireless/ti/wlcore/event.c|  14 +-
 drivers/net/wireless/ti/wlcore/main.c | 248 --
 drivers/net/wireless/ti/wlcore/ps.c   |  15 +-
 drivers/net/wireless/ti/wlcore/rx.c   |  26 ++--
 drivers/net/wireless/ti/wlcore/scan.c |   4 +-
 drivers/net/wireless/ti/wlcore/sysfs.c|   8 +-
 drivers/net/wireless/ti/wlcore/testmode.c |  14 +-
 drivers/net/wireless/ti/wlcore/tx.c   |  14 +-
 drivers/net/wireless/ti/wlcore/wlcore_i.h |   3 -
 38 files changed, 653 insertions(+), 600 deletions(-)

-- 
2.6.3.368.gf34be46

Re: [PATCH 2/3] net: macb: Fix more coding style issues

[Joe Perches]

On Mon, 2016-03-07 at 08:17 -0800, Moritz Fischer wrote:
> This commit takes care of the coding style warnings
> that are mostly due to a different comment style and
> lines over 80 chars.
[]
> diff --git a/drivers/net/ethernet/cadence/macb.c 
> b/drivers/net/ethernet/cadence/macb.c
[]
> @@ -127,8 +126,7 @@ static void hw_writel(struct macb *bp, int offset, u32 
> value)
>   writel_relaxed(value, bp->regs + offset);
>  }
>  
> -/*
> - * Find the CPU endianness by using the loopback bit of NCR register. When 
> the
> +/* Find the CPU endianness by using the loopback bit of NCR register. When 
> the
>   * CPU is in big endian we need to program swaped mode for management

swaped/swapped typo
@@ -945,6 +943,7 @@ static int macb_rx_frame(struct macb *bp, unsigned int 
first_frag,
>  static int macb_rx(struct macb *bp, int budget)
>  {
>   int received = 0;
> + int dropped;

This is an unnecessary and unmentioned change.

>   unsigned int tail;
>   int first_frag = -1;
>  
> @@ -968,7 +967,6 @@ static int macb_rx(struct macb *bp, int budget)
>   }
>  
>   if (ctrl & MACB_BIT(RX_EOF)) {
> - int dropped;
>   BUG_ON(first_frag == -1);
>  
>   dropped = macb_rx_frame(bp, first_frag, tail);
> 

Re: [PATCH net-next V2 13/16] net: fec: print more debug info in fec_timeout

[Joe Perches]

On Fri, 2016-03-04 at 09:05 -0700, Troy Kisky wrote:
> On 3/4/2016 3:06 AM, Fugang Duan wrote:
> > From: Troy Kisky  Sent: Thursday, February 
> > 25, 2016 8:37 AM
[]
> > > Print the current interrupt flags and mask and the interrupt state during 
> > > the last
> > > interrupt in fec_timeout.
[]
> > > diff --git a/drivers/net/ethernet/freescale/fec_main.c
[]
> > > @@ -1107,6 +1107,9 @@ fec_timeout(struct net_device *ndev)
> > >   int i;
> > >   uint events = 0;
> > > 
> > > + pr_err("%s: last=%x %x, mask %x\n", __func__, fep->last_ievents,
> > > +    readl(fep->hwp + FEC_IEVENT), readl(fep->hwp + FEC_IMASK));
> > > +
> > pr_err() -> netdev_err()
> Sounds good

This seems like debugging information rather than
an error a user can do anything with and if there's
a timeout, how likely is it that the hardware is
hosed and this would  repetitively and unnecessarily
fill up logs?

So maybe netdev_dbg and net_ratelimit() too.

if (net_ratelimit()
netdev_(etc...)

Re: [ethtool PATCH v2 10/12] internal.h: TRUE/FALSE macros

[Joe Perches]

On Thu, 2016-03-03 at 20:23 -0800, David Decotigny wrote:
> From: David Decotigny 
[]
> diff --git a/internal.h b/internal.h
[]
> @@ -42,6 +42,14 @@ typedef int32_t s32;
>  #include "ethtool-copy.h"
>  #include "net_tstamp-copy.h"
>  
> +#ifndef TRUE
> +#  define TRUE (!0)
> +#endif
> +
> +#ifndef FALSE
> +#  define FALSE (!!0)

#define TRUE 1
and
#define FALSE 0
are obvious and typical.

The ! and !! uses don't add anything other than obscurity.

Re: [PATCH V3 2/4] net-next: mediatek: add support for MT7623 ethernet

[Joe Perches]

On Thu, 2016-03-03 at 21:02 +0100, John Crispin wrote:
> Add ethernet support for MediaTek SoCs from the MT7623 family.
[]
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h 
> b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
[]
> +/* ugly macro hack to make sure hw_stats and ethtool strings are consistent 
> */

I agree and think the _FE macros defines and uses should be removed.

Re: [PATCH net-next v3 2/3] nsh: logging module

[Joe Perches]

On Tue, 2016-03-01 at 11:11 +, Brian Russell wrote:
> Module can register for Type 1 or specified classes of Type 2 metadata
> and will then log incoming matching packets.

This logging mechanism seems like a way to fill/DoS logs.

Maybe use pr_info_ratelimit?
Maybe use the trace_events mechanisms instead?

> Signed-off-by: Brian Russell 
> ---
>  net/ipv4/Kconfig   |   8 
>  net/ipv4/Makefile  |   1 +
>  net/ipv4/nsh_log.c | 135 
> +
>  3 files changed, 144 insertions(+)
>  create mode 100644 net/ipv4/nsh_log.c
> 
> diff --git a/net/ipv4/Kconfig b/net/ipv4/Kconfig
> index df14c59..87b6dde 100644
> --- a/net/ipv4/Kconfig
> +++ b/net/ipv4/Kconfig
> @@ -223,6 +223,14 @@ config NET_NSH
>  
>    To compile it as a module, choose M here.  If unsure, say N.
>  
> +config NET_NSH_LOG
> +tristate 'NSH Metadata Logger'
> + depends on NET_NSH
> + help
> +  Log packets with incoming NSH metadata.
> +
> +  To compile it as a module, choose M here.  If unsure, say N.
> +
>  config IP_MROUTE
>   bool "IP: multicast routing"
>   depends on IP_MULTICAST
> diff --git a/net/ipv4/Makefile b/net/ipv4/Makefile
> index 14b7995..69377fb 100644
> --- a/net/ipv4/Makefile
> +++ b/net/ipv4/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_NET_FOU) += fou.o
>  obj-$(CONFIG_NET_IPGRE_DEMUX) += gre.o
>  obj-$(CONFIG_NET_IPGRE) += ip_gre.o
>  obj-$(CONFIG_NET_NSH) += nsh.o
> +obj-$(CONFIG_NET_NSH_LOG) += nsh_log.o
>  obj-$(CONFIG_NET_UDP_TUNNEL) += udp_tunnel.o
>  obj-$(CONFIG_NET_IPVTI) += ip_vti.o
>  obj-$(CONFIG_SYN_COOKIES) += syncookies.o
> diff --git a/net/ipv4/nsh_log.c b/net/ipv4/nsh_log.c
> new file mode 100644
> index 000..3d774ed
> --- /dev/null
> +++ b/net/ipv4/nsh_log.c
> @@ -0,0 +1,135 @@
> +/*
> + * Network Service Header (NSH) logging module.
> + *
> + * Copyright (c) 2016 by Brocade Communications Systems, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include 
> +#include 
> +#include 
> +
> +static bool t1_enabled = false;
> +module_param(t1_enabled, bool, 0444);
> +MODULE_PARM_DESC(t1_enabled, "Type 1 Metadata log enabled");
> +
> +#define MAX_T2_CLASSES 10
> +static unsigned int t2_classes[MAX_T2_CLASSES];
> +static int num_t2 = 0;
> +module_param_array(t2_classes, uint, _t2, 0444);
> +MODULE_PARM_DESC(t2_classes, "Type 2 Metadata classes log enabled");
> +
> +static const char *nsh_next_proto(u8 next_proto)
> +{
> + switch (next_proto) {
> + case NSH_NEXT_PROTO_IPv4:
> + return "IPv4";
> + case NSH_NEXT_PROTO_IPv6:
> + return "IPv6";
> + case NSH_NEXT_PROTO_ETH:
> + return "Eth";
> + default:
> + return "Unknown";
> + }
> +}
> +
> +/* Type 1 metadata has fixed length, 4 x 32-bit words */
> +static int nsh_log_t1(struct sk_buff *skb, u32 service_path_id,
> +   u8 service_index, u8 next_proto,
> +   struct nsh_metadata *ctx_hdr, unsigned int num_ctx_hdrs)
> +{
> + u32 *data;
> +
> + if ((ctx_hdr->class != NSH_MD_CLASS_TYPE_1) ||
> + (ctx_hdr->type != NSH_MD_TYPE_TYPE_1) ||
> + (ctx_hdr->len != NSH_MD_LEN_TYPE_1) ||
> + (num_ctx_hdrs != 1))
> + return -EINVAL;
> +
> + data = ctx_hdr->data;
> + pr_info("NSH T1 Rx(%s): SPI=%u SI=%u Next=%s"
> + " MD 0x%08x 0x%08x 0x%08x 0x%08x\n", skb->dev->name,
> + service_path_id, service_index, nsh_next_proto(next_proto),
> + data[0], data[1], data[2], data[3]);
> +
> + return 0;
> +}
> +
> +/* Type 2 metadata consists of a variable number of TLVs */
> +#define T2_BUFSIZE 512
> +static int nsh_log_t2(struct sk_buff *skb, u32 service_path_id,
> +   u8 service_index, u8 next_proto,
> +   struct nsh_metadata *ctx_hdrs, unsigned int num_ctx_hdrs)
> +{
> + char t2_buf[T2_BUFSIZE];
> + int wrlen;
> + u32 *data;
> + int i,j;
> +
> + wrlen = snprintf(t2_buf, T2_BUFSIZE,
> +  "NSH T2 Class %u Rx(%s): SPI=%u SI=%u Next=%s MD",
> +  ctx_hdrs[0].class, skb->dev->name, service_path_id,
> +  service_index, nsh_next_proto(next_proto));
> +
> + for (i = 0; i < num_ctx_hdrs; i++) {
> + wrlen += snprintf(t2_buf+wrlen, T2_BUFSIZE-wrlen,
> +   " TLV%d Type=%u Len=%u", i+1,
> +   ctx_hdrs[i].type, ctx_hdrs[i].len);
> + data = ctx_hdrs[i].data;
> + for (j = 0; j < ctx_hdrs[i].len; j++)
> + wrlen += snprintf(t2_buf+wrlen, T2_BUFSIZE-wrlen,
> +   " 0x%08x", data[j]);
> + }
> + pr_info("%s\n", t2_buf);
> + return 0;
> +}
> +
> +static struct 

Re: [net-next 13/15] i40e/i40evf: use logical operators, not bitwise

[Joe Perches]

On Wed, 2016-02-17 at 19:38 -0800, Jeff Kirsher wrote:
> From: Mitch Williams 
> 
> Mr. Spock would certainly raise an eyebrow to see us using bitwise
> operators, when we should clearly be relying on logic. Fascinating.

I think it read better before this change.

Spock might have looked at the type of the
variable before raising that eyebrow.

clean_complete is bool so it's not actually a
bitwise operation,

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c 
> b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
[]
> @@ -1996,7 +1996,8 @@ int i40e_napi_poll(struct napi_struct *napi, int budget)
>    * budget and be more aggressive about cleaning up the Tx descriptors.
>    */
>   i40e_for_each_ring(ring, q_vector->tx) {
> - clean_complete &= i40e_clean_tx_irq(ring, vsi->work_limit);
> + clean_complete = clean_complete &&
> +  i40e_clean_tx_irq(ring, vsi->work_limit);

etc...

Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types

[Joe Perches]

On Sat, 2016-01-30 at 09:51 -0800, Eric Dumazet wrote:
> On Sat, 2016-01-30 at 12:05 -0200, Lucas Tanure wrote:
> > On Sat, Jan 30, 2016 at 11:45 AM, Patrick McHardy  wrote:
> > > On 30.01, Lucas Tanure wrote:
> > > > As suggested by checkpatch.pl:
> > > > CHECK: Prefer kernel type 'uX' over 'uintX_t'
> > > 
> > > You might have noticed we have literally hundreds of them spread over 100
> > > files in the netfilter code. We'll gradually change them when the code is
> > > touched anyways.
> > > 
> > > >  net/ipv4/netfilter/ip_tables.c | 5 ++---
> > > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > Yes, I checked that. But would be better to change that now?
> > Because:
> > - could take years to anyone to touch the code, as the code already
> > works very well
> > - be more standardized could facilitate reading the code
> > - It's a good way to encourage new people to contribute to the code
> > 
> > Thanks!
> 
> These changes are a pain for people having to constantly backport fixes
> into stable kernels, or rebase their patches before upstream
> submissions.
> 
> Things like 'git cherry-pick' , 'git rebase' no longer work.
> This is a huge pain, and manual editing to resolve conflicts often
> add bugs.
> 
> Really, do you believe the 'uX' over 'uintX_t' stuff really matters for
> people working on adding new features and fixing bugs ?
> 
> I am certain that if you had to work like us, you would quickly see the
> utility of such changes is negative.
> 
> Sure, new submissions should be clean, but 'fixing' old code is not
> worth it.

That might depend on whether or not the linux kernel is
a "long-life project" and whether or no any old branch
of it is also important and sufficiently long-life.

The active life of a backport branch for the linux kernel
seems to be 3 or 4 years.  The linux kernel will likely
be useful for a few more decades beyond that.

Complex and long-life projects like the linux kernel
might benefit more in code complexity reduction patches
like these rather than code stasis for backward porting
ease.

In general, arguing for stasis leads to ossification,
slow decline.

Change for change's sake is poor, but changes to reduce
complexity, improve maintainability (for some measure of
it) and especially improve performance should be
welcomed where feasible.

Re: [PATCH] lib: fix callers of strtobool to use char array

[Joe Perches]

On Wed, 2016-01-27 at 16:45 -0800, Kees Cook wrote:
> Some callers of strtobool were passing a pointer to unterminated strings.
> This fixes the issue and consolidates some logic in cifs.

This may be incomplete as it duplicates the behavior for
the old number of characters, but this is not a solution
for the entry of a bool that is "on" or "off".

> diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
[]
> @@ -290,7 +305,8 @@ static ssize_t cifs_stats_proc_write(struct file *file,
>   }
>   }
>   spin_unlock(_tcp_ses_lock);
> - }
> + } else
> + return rc;

Likely better to reverse the test and unindent the
preceding block.

Otherwise, please make sure to use the general brace
form of when one branch needs braces, the other branch
should have them too.

Re: [PATCH v2] net: Add eth_platform_get_mac_address() helper.

[Joe Perches]

On Wed, 2016-01-06 at 19:01 -0500, David Miller wrote:
> The caller checks for NULL, and this is the default implementation
> doing precisely what it is meant to do.

Yeah, I noticed that later. Nevermind...

cheers, Joe
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] net: Add eth_platform_get_mac_address() helper.

[Joe Perches]

On Wed, 2016-01-06 at 16:33 -0500, David Miller wrote:
> A repeating pattern in drivers has become to use OF node information
> and, if not found, platform specific host information to extract the
> ethernet address for a given device.
[]
> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
[]
> @@ -485,3 +487,32 @@ static int __init eth_offload_init(void)
>  }
>  
>  fs_initcall(eth_offload_init);
> +
> +unsigned char * __weak arch_get_platform_mac_address(void)
> +{
> +   return NULL;

WARN_ON_ONCE ?

> +}
> +
> +int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
> +{
> +   const unsigned char *addr;
> +   struct device_node *dp;
> +
> +   if (dev_is_pci(dev))
> +   dp = pci_device_to_OF_node(to_pci_dev(dev));
> +   else
> +   dp = dev->of_node;
> +
> +   addr = NULL;
> +   if (dp)
> +   addr = of_get_mac_address(dp);
> +   if (!addr)
> +   addr = arch_get_platform_mac_address();
> +
> +   if (!addr)
> +   return -ENODEV;
> +
> +   ether_addr_copy(mac_addr, addr);

Couldn't some oddball arch have an unaligned addr?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] net: Add eth_platform_get_mac_address() helper.

[Joe Perches]

On Thu, 2016-01-07 at 00:26 +0100, Bjørn Mork wrote:
> Joe Perches <j...@perches.com> writes:
> > On Wed, 2016-01-06 at 16:33 -0500, David Miller wrote:
> > > A repeating pattern in drivers has become to use OF node information
> > > and, if not found, platform specific host information to extract the
> > > ethernet address for a given device.
> > []
> > > diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> > []
> > > @@ -485,3 +487,32 @@ static int __init eth_offload_init(void)
> > >  }
> > >  
> > >  fs_initcall(eth_offload_init);
> > > +
> > > +unsigned char * __weak arch_get_platform_mac_address(void)
> > > +{
> > > +   return NULL;
> > 
> > WARN_ON_ONCE ?
> 
> That would prevent a driver from using this with additional fallback
> methods.  For what reason?

It's declared __weak and should be overridden by
an arch specific function.

A NULL address would cause a fault when using
a function like copy_ether_addr.


> I don't have a specific usecase, but I can imagine drivers falling back
> to e.g a random address without wanting to be noisy about it.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 1/2] Support outside netns for tunnels.

[Joe Perches]

On Mon, 2016-01-04 at 11:47 -0800, Tom Herbert wrote:
> On Mon, Jan 4, 2016 at 10:45 AM, Saurabh Mohan
>  wrote:
> > 
> > This patch enchances a tunnel interface, like gre, to have the tunnel
> > encap/decap be in the context of a network namespace that is different from
> > the namespace of the tunnel interface.
> > 
> > From userspace this feature may be configured using the new 'onetns' 
> > keyword:
> > ip netns exec custa ip link add dev tun1 type gre local 10.0.0.1 \
> >  remote 10.0.0.2 onetns outside
> > 
> > In the above example the tunnel would be in the 'custa' namespace and the
> > tunnel endpoints would be in the 'outside' namespace.
> > 
> > Also, proposing the use of netns name 'global' to specify the global 
> > namespace.
> > 
> > If this patch set is accepted then I will add support for other tunnels as
> > well.
> > 
> This might be interesting. Can you please ad a 0/n patch that
> describes the motivation for this, in particular I would like to know
> if this has a positive impact on ns performance if somehow we are
> eliminating indirection.
[]
> > diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
[]
> > @@ -3,6 +3,7 @@
> > 
> >  #include 
> >  #include 
> > +#include 
> > 
> > 
> >  #define SIOCGETTUNNEL   (SIOCDEVPRIVATE + 0)
> > @@ -27,6 +28,14 @@
> >  #define GRE_FLAGS  __cpu_to_be16(0x00F8)
> >  #define GRE_VERSION__cpu_to_be16(0x0007)
> > 
> > +struct o_netns_parm {
> > +   __u8o_netns_flag;
> > +   __u32   o_netns_fd;
> > +   charnetns[NAME_MAX];
> > +};

Trivia:

It could eliminate a few padding bytes if the
o_netns_fd and o_netns_flag fields were reversed.

and netns[NAME_MAX] is normally netns[NAME_MAX + 1]

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH net-next 05/24] phy: add phydev_name() macro

[Joe Perches]

On Mon, 2016-01-04 at 18:36 +0100, Andrew Lunn wrote:
> Add a phydev_name() macro, to help with moving some structure members
> from phy_device.
[]
> diff --git a/include/linux/phy.h b/include/linux/phy.h
[]
> @@ -783,6 +783,8 @@static inline int phy_read_status(struct phy_device 
> *phydev)
>  #define phydev_dbg(_phydev, format, args...) \
>   dev_dbg(&_phydev->dev, format, ##args)
>  
> +#define phydev_name(_phydev) dev_name(&_phydev->dev)
> +

This should use parentheses around phydev

#define phydev_name(phydev) dev_name(&(phydev)->dev)

or likely even better be a static inline

static inline const char * phydev_name(const struct phy_device *phydev)
{
return dev_name(>dev);
}

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/12] net-next: mediatek: add switch driver for mt7621

[Joe Perches]

On Sun, 2016-01-03 at 16:26 +0100, John Crispin wrote:
> This driver is very basic and only provides basic init and irq support.
> The SoC and switch core both have support for a special tag making DSA
> support possible.

trivia:

> diff --git a/drivers/net/ethernet/mediatek/gsw_mt7621.c 
> b/drivers/net/ethernet/mediatek/gsw_mt7621.c
[]
> +static irqreturn_t gsw_interrupt_mt7621(int irq, void *_priv)
> +{
> + struct fe_priv *priv = (struct fe_priv *)_priv;
> + struct mt7620_gsw *gsw = (struct mt7620_gsw *)priv->soc->swpriv;
> + u32 reg, i;
> +
> + reg = mt7530_mdio_r32(gsw, 0x700c);
> +
> + for (i = 0; i < 5; i++)
> + if (reg & BIT(i)) {
> + unsigned int link;
> +
> + link = mt7530_mdio_r32(gsw,
> +    0x3008 + (i * 0x100)) & 0x1;
> +
> + if (link != priv->link[i]) {
> + priv->link[i] = link;
> + if (link)
> + netdev_info(priv->netdev,
> + "port %d link up\n", i);
> + else
> + netdev_info(priv->netdev,
> + "port %d link down\n", i);
> + }
> + }

It's more common to use braces after the for loop.
Perhaps this could be written with a continue; to reduce
indentation and also use a single netdev_info().

for (i = 0; i < 5; i++) {
unsigned int link;

if (!(reg & BIT(i)))
continue;

link = mt7530_mdio_r32(gsw, 0x3008 + (i * 0x100)) & 0x1;

if (link == priv->link[i])
continue;

priv->link[i] = link;
netdev_info(priv->netdev, "port %d link %s\n",
i, link ? "up" : "down");
}


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH next] net/core/dev: Warn on an impossibly short offload frame

[Joe Perches]

On Sat, 2016-01-02 at 19:25 -0500, Aaron Conole wrote:
> When signaling that a GRO frame is ready to be processed, the network stack
> correctly checks length and aborts processing when a frame is less than 14
> bytes. However, such a condition is really indicative of a broken driver,
> and should be loudly signaled, rather than silently dropped as the case is
> today.
> 
> Convert the condition to use WARN_ON() to ensure that the stack loudly
> complains about such broken drivers.
[]
> diff --git a/net/core/dev.c b/net/core/dev.c
[]
> @@ -4579,7 +4579,7 @@ static struct sk_buff *napi_frags_skb(struct 
> napi_struct *napi)
>   eth = skb_gro_header_fast(skb, 0);
>   if (unlikely(skb_gro_header_hard(skb, hlen))) {
>   eth = skb_gro_header_slow(skb, hlen, 0);
> - if (unlikely(!eth)) {
> + if (WARN_ON(!eth)) {
>   napi_reuse_skb(napi, skb);
>   return NULL;
>   }

It's generally a good idea to use
WARN_ON_RATELIMIT or WARN_ON_ONCE.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] xen-netback: Fine-tuning for three function implementations

[Joe Perches]

On Sat, 2016-01-02 at 18:50 +0100, SF Markus Elfring wrote:
> A few update suggestions were taken into account
> from static source code analysis.

While static analysis can be useful, I don't think these
specific conversions are generally useful.

Perhaps it would be more useful to convert the string
duplication or snprintf logic to kstrdup/kasprintf

This:

if (num_queues == 1) {
xspath = kzalloc(strlen(dev->otherend) + 1, GFP_KERNEL);
if (!xspath) {
xenbus_dev_fatal(dev, -ENOMEM,
 "reading ring references");
return -ENOMEM;
}
strcpy(xspath, dev->otherend);
} else {
xspathsize = strlen(dev->otherend) + xenstore_path_ext_size;
xspath = kzalloc(xspathsize, GFP_KERNEL);
if (!xspath) {
xenbus_dev_fatal(dev, -ENOMEM,
 "reading ring references");
return -ENOMEM;
}
snprintf(xspath, xspathsize, "%s/queue-%u", dev->otherend,
 queue->id);
}

could be simplified to something like:

if (num_queues == 1)
xspath = kstrdup(dev->otherend, GFP_KERNEL);
else
xspath = kasprintf(GFP_KERNEL, "%s/queue-%u",
   dev->otherend, queue->id);
if (!xspath)
etc...

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] /drivers/net/wireless/ath/ath9k remove unnecessary ?: operator

[Joe Perches]

On Mon, 2015-12-28 at 20:48 +0700, Ivan Safonov wrote:
> ((thermometer < 0) ? 0 : (thermometer == X)) and (thermometer == X) are equal 
> for X >= 0.

X is not guaranteed to be >= 0 here

> Signed-off-by: Ivan Safonov 
> ---
>  drivers/net/wireless/ath/ath9k/ar9003_eeprom.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c 
> b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
[]
> @@ -4097,16 +4097,16 @@ static void ar9003_hw_thermometer_apply(struct ath_hw 
> *ah)
>   REG_RMW_FIELD(ah, AR_PHY_65NM_CH2_RXTX4,
>     AR_PHY_65NM_CH0_RXTX4_THERM_ON_OVR, therm_on);
>  
> - therm_on = (thermometer < 0) ? 0 : (thermometer == 0);
> + therm_on = thermometer == 0;

This code is not equivalent.

Check what happens when thermometer is -1.

>   REG_RMW_FIELD(ah, AR_PHY_65NM_CH0_RXTX4,
>     AR_PHY_65NM_CH0_RXTX4_THERM_ON, therm_on);
>   if (pCap->chip_chainmask & BIT(1)) {
> - therm_on = (thermometer < 0) ? 0 : (thermometer == 1);
> + therm_on = thermometer == 1;
>   REG_RMW_FIELD(ah, AR_PHY_65NM_CH1_RXTX4,
>     AR_PHY_65NM_CH0_RXTX4_THERM_ON, therm_on);
>   }
>   if (pCap->chip_chainmask & BIT(2)) {
> - therm_on = (thermometer < 0) ? 0 : (thermometer == 2);
> + therm_on = thermometer == 2;
>   REG_RMW_FIELD(ah, AR_PHY_65NM_CH2_RXTX4,
>     AR_PHY_65NM_CH0_RXTX4_THERM_ON, therm_on);
>   }

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] /drivers/net/wireless/ath/ath9k remove unnecessary ?: operator

[Joe Perches]

On Tue, 2015-12-29 at 01:38 +0700, Ivan Safonov wrote:
> On 12/29/2015 12:56 AM, Joe Perches wrote:
> > On Mon, 2015-12-28 at 20:48 +0700, Ivan Safonov wrote:
> > > ((thermometer < 0) ? 0 : (thermometer == X)) and (thermometer ==
> > > X) are equal for X >= 0.
> > X is not guaranteed to be >= 0 here
> 
> X is fixed constant. In this case X is {0, 1, 2}.

Looks like it can be -1 to me (range: -1, 0, 1, 2)

static int ar9003_hw_get_thermometer(struct ath_hw *ah)
{
struct ar9300_eeprom *eep = >eeprom.ar9300_eep;
struct ar9300_base_eep_hdr *pBase = >baseEepHeader;
int thermometer =  (pBase->miscConfiguration >> 1) & 0x3;

return --thermometer;
}

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: pull-request: mac80211 2015-12-15

[Joe Perches]

On Thu, 2015-12-17 at 13:44 +0100, Johannes Berg wrote:
> On Wed, 2015-12-16 at 18:34 -0500, David Miller wrote:
> >  
> > Something about your text encoding kept this from ending up
> > in patchwork for some reason.
> > 
> 
> Hm. I don't see anything special with this, seems to just be plain
> text
> 8bit transfer encoding.
> 
> Do you want me to watch out for things getting into patchwork in the
> future?
> 

Hey Johannes.

You seem to be using:

X-Mailer: Evolution 3.18.1-1 

which, to be overly technical, _sucks_.

The new composer for Evolution has way too many defects
to list.

It adds non-breaking spaces (NBSP) characters instead of
a standard ASCII space in various places.

Every version of Evolution since 3.14 is terrible at
sending text only messages.

3.12 works
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/15] i40e: Add support for client interface for IWARP driver

[Joe Perches]

On Wed, 2015-12-16 at 13:58 -0600, Faisal Latif wrote:
> From: Anjali Singhai Jain 
> 
> This patch adds a Client interface for i40iw driver
> support. Also expands the Virtchannel to support messages
> from i40evf driver on behalf of i40iwvf driver.
[]
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_client.c 
> b/drivers/net/ethernet/intel/i40e/i40e_client.c
[]
> + * Contact Information:
> + * e1000-devel Mailing List 

trivia:

This should probably be: intel-wired-...@lists.osuosl.org

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/15] i40iw: add main, hdr, status

[Joe Perches]

On Wed, 2015-12-16 at 13:58 -0600, Faisal Latif wrote:
> i40iw_main.c contains routines for i40e <=> i40iw interface and setup.
> i40iw.h is header file for main device data structures.
> i40iw_status.h is for return status codes.
[]
> diff --git a/drivers/infiniband/hw/i40iw/i40iw.h 
> b/drivers/infiniband/hw/i40iw/i40iw.h
[]
> +#define i40iw_pr_err(fmt, args ...) pr_err("%s: error " fmt, __func__, ## 
> args)
> +
> +#define i40iw_pr_info(fmt, args ...) pr_info("%s: " fmt, __func__, ## args)
> +
> +#define i40iw_pr_warn(fmt, args ...) pr_warn("%s: " fmt, __func__, ## args)

Using "error " in the output doesn't really add much
as there's already a KERN_ERR with the output.

Using __func__ hardly adds anything.

Using netdev_ is generally preferred

> +
> +struct i40iw_cqp_request {
> + struct cqp_commands_info info;
> + wait_queue_head_t waitq;
> + struct list_head list;
> + atomic_t refcount;
> + void (*callback_fcn)(struct i40iw_cqp_request*, u32);
> + void *param;
> + struct i40iw_cqp_compl_info compl_info;
> + u8 waiting:1;
> + u8 request_done:1;
> + u8 dynamic:1;
> + u8 polling:1;

These would bitfields might be better as bool

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] ipv6: add IPV6_HDRINCL option for raw sockets

[Joe Perches]

On Wed, 2015-12-16 at 17:22 +0100, Hannes Frederic Sowa wrote:
> Same as in Windows, we miss IPV6_HDRINCL for SOL_IPV6 and SOL_RAW.
> The SOL_IP/IP_HDRINCL is not available for IPv6 sockets.
[]
> diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
[]
> @@ -972,6 +972,11 @@ static int do_rawv6_setsockopt(struct sock *sk,
> int level, int optname,
>   return -EFAULT;
>  
>   switch (optname) {
> + case IPV6_HDRINCL:
> + if (sk->sk_type != SOCK_RAW)
> + return -EINVAL;
> + inet_sk(sk)->hdrincl = !!val;

trivia:

ipv4/sockglue.c uses the ternary '? 1 : 0' convention for this.
It might be nicer to be consistent.

Then again sockglue.c is inconsistent about that too.

net/ipv4/ip_sockglue.c:736: inet->hdrincl = val ? 1 : 0;
net/ipv4/ip_sockglue.c:743: inet->nodefrag = val ? 1 : 0;
net/ipv4/ip_sockglue.c:746: inet->bind_address_no_port = val ? 1 : 
0;
net/ipv4/ip_sockglue.c:754: inet->recverr = !!val;
net/ipv4/ip_sockglue.c:772: inet->mc_loop = !!val;
net/ipv4/ip_sockglue.c:1123:inet->freebind = !!val;

There's no change to object code either way.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/15] add Intel(R) X722 iWARP driver

[Joe Perches]

On Wed, 2015-12-16 at 13:58 -0600, Faisal Latif wrote:
> This series contains the addition of the i40iw.ko driver.

This series should probably be respun against -next
instead of linus' tree.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 8/8] treewide: Remove newlines inside DEFINE_PER_CPU() macros

[Joe Perches]

On Mon, 2015-12-07 at 17:53 +0100, Michal Marek wrote:
> On 2015-12-07 17:33, David Laight wrote:
> > From: Michal Marek
> > > Sent: 04 December 2015 15:26
> > > Otherwise make tags can't parse them:
> > > 
> > > ctags: Warning: arch/ia64/kernel/smp.c:60: null expansion of name pattern 
> > > "\1"
> > ...
> > 
> > Seems to me you need to fix ctags.
> 
> I'm sure the maintainers of ctags and etags would accept patches to
> describe a custom context-free grammar via commandline options, but
> until then, let's continue using the regular expressions in tags.sh and
> remove newlines in macros that tags.sh is trying to expand.
> 

Do you have a list of the most common macros?

Perhaps it'd be good to add exceptions to checkpatch
80 column line rules for them.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] net: hns: optimize XGE capability by reducing cpu usage

[Joe Perches]

On Mon, 2015-12-07 at 16:58 +0800, Yankejian (Hackim Yim) wrote:
> On 2015/12/7 11:32, Joe Perches wrote:
> > On Sun, 2015-12-06 at 22:29 -0500, David Miller wrote:
> > > > From: yankejian <yankej...@huawei.com>
> > > > Date: Sat, 5 Dec 2015 15:32:29 +0800
> > > > 
> > > > > > +#if (PAGE_SIZE < 8192)
> > > > > > + if (hnae_buf_size(ring) == HNS_BUFFER_SIZE_2048) {
> > > > > > + truesize = hnae_buf_size(ring);
> > > > > > + } else {
> > > > > > + truesize = ALIGN(size, L1_CACHE_BYTES);
> > > > > > + last_offset = hnae_page_size(ring) - 
> > > > > > hnae_buf_size(ring);
> > > > > > + }
> > > > > > +
> > > > > > +#else
> > > > > > + truesize = ALIGN(size, L1_CACHE_BYTES);
> > > > > > + last_offset = hnae_page_size(ring) - 
> > > > > > hnae_buf_size(ring);
> > > > > > +#endif
> > > > 
> > > > This is not indented properly, and it looks terrible.
> > And it makes one curious as to why last_offset isn't set
> > in the first block.
> 
> Hi Joe,

Hello.

> if hnae_buf_size que equal to HNS_BUFFER_SIZE, last_offset is useless in the 
> routines of this function.
> so it is ignored in the first block. thanks for your suggestion.

More to the point, last_offset is initialized to 0.

It'd be clearer not to initialize it at all and
set it to 0 in the first block and not overwrite
the initialization in each subsequent block.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] net: hns: optimize XGE capability by reducing cpu usage

[Joe Perches]

On Sun, 2015-12-06 at 22:29 -0500, David Miller wrote:
> From: yankejian 
> Date: Sat, 5 Dec 2015 15:32:29 +0800
> 
> > +#if (PAGE_SIZE < 8192)
> > + if (hnae_buf_size(ring) == HNS_BUFFER_SIZE_2048) {
> > + truesize = hnae_buf_size(ring);
> > + } else {
> > + truesize = ALIGN(size, L1_CACHE_BYTES);
> > + last_offset = hnae_page_size(ring) - hnae_buf_size(ring);
> > + }
> > +
> > +#else
> > + truesize = ALIGN(size, L1_CACHE_BYTES);
> > + last_offset = hnae_page_size(ring) - hnae_buf_size(ring);
> > +#endif
> 
> This is not indented properly, and it looks terrible.

And it makes one curious as to why last_offset isn't set
in the first block.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mwifiex: fix semicolon.cocci warnings

[Joe Perches]

On Sun, 2015-12-06 at 22:56 +0100, Julia Lawall wrote:
> Remove unneeded semicolon.
> 
> Generated by: scripts/coccinelle/misc/semicolon.cocci

There are a lot more of these

arch/arm/mach-ixp4xx/goramo_mlr.c:78:static u32 hw_bits = 0xFFFD;/* 
assume all hardware present */;
crypto/tgr192.c:563:tgr192_update(desc, NULL, 0); /* flush */ ;
crypto/tgr192.c:591:tgr192_update(desc, NULL, 0); /* flush */ ;
drivers/block/drbd/drbd_int.h:982:  struct list_head list; /* on 
device->pending_bitmap_io */;
drivers/block/drbd/drbd_state.c:2202:   enum drbd_state_rv err, rv = 
SS_UNKNOWN_ERROR; /* continue waiting */;
drivers/block/drbd/drbd_main.c:1453:return drop_it; /* && (device->state == 
R_PRIMARY) */;
drivers/block/ataflop.c:779:dma_wd.fdc_speed = 0;   /* always seek 
with 8 Mhz */;
drivers/scsi/bnx2i/bnx2i.h:540: u16 itt[BNX2X_MAX_CQS];
/* Cstorm CQ sequence to notify array, updated by driver */;
drivers/net/ethernet/chelsio/cxgb4vf/sge.c:1802:for 
(frag = 0, fp = gl.frags; /**/; frag++, fp++) {
drivers/net/wireless/marvell/mwifiex/init.c:98: priv->beacon_period = 100; /* 
beacon interval */ ;
drivers/net/wireless/intel/iwlwifi/mvm/fw-api-power.h:313:}; /* 
TX_POWER_REDUCED_FLAGS_TYPE_API_E_VER_2 */;
drivers/net/wireless/intersil/prism54/islpci_dev.c:112: return 
-EILSEQ; /* Illegal byte sequence  */;
drivers/spi/spi-bcm2835.c:239:  bs->dma_pending = 0;

/* and mark as completed */;
drivers/spi/spi-topcliff-pch.c:879: param->chan_id = data->ch * 2; /* Tx = 
0, 2 */;
drivers/spi/spi-topcliff-pch.c:894: param->chan_id = data->ch * 2 + 1; /* 
Rx = Tx + 1 */;
drivers/usb/serial/keyspan.c:1966:  dr->bRequest = 0xB0;/* 49wg 
control message */;
drivers/usb/serial/oti6858.c:112:   u8  rx_bytes_avail; /* 
number of bytes in rx buffer */;
drivers/clk/tegra/clk-dfll.c:723:   int coef = 128; /* FIXME: td->cg_scale? 
*/;
drivers/media/pci/ivtv/ivtv-driver.h:532:   u8 even[2]; /* two-byte 
payload of even field */;
fs/xfs/libxfs/xfs_btree.h:98:   case XFS_BTNUM_MAX: ASSERT(0); /* fucking gcc 
*/ ; break;   \
fs/xfs/libxfs/xfs_btree.h:118:  case XFS_BTNUM_MAX: ASSERT(0); /* fucking gcc 
*/ ; break; \
fs/xfs/xfs_itable.c:375:ac.ac_ubleft = ubcount * statstruct_size; /* 
bytes */;
include/uapi/linux/nfc.h:278:   char service_name[NFC_LLCP_MAX_SERVICE_NAME]; 
/* Service name URI */;
kernel/trace/ring_buffer.c:4896:set_current_state(TASK_INTERRUPTIBLE);
/* Just run for 10 seconds */;
sound/soc/intel/atom/sst/sst.c:71:  sst_shim_write64(drv->shim, 
drv->ipc_reg.ipcx, header.full);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: use-after-free in sctp_do_sm

[Joe Perches]

On Fri, 2015-12-04 at 11:47 -0500, Jason Baron wrote:
> When DYNAMIC_DEBUG is enabled we have this wrapper from
> include/linux/dynamic_debug.h:
> 
> if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT))
>   
> 
> So the compiler is not emitting the side-effects in this
> case.

Huh?  Do I misunderstand what you are writing?

You are testing a variable that is not generally set
so the call is not being performed in the general case,
but the compiler can not elide the code.

If the variable was enabled via the control file, the
__dynamic_pr_debug would be performed with the
use-after-free.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next v5 2/8] dpaa_eth: add support for DPAA Ethernet

[Joe Perches]

On Fri, 2015-12-04 at 14:55 -0500, David Miller wrote:
> From: Madalin Bucur 
> Date: Thu, 3 Dec 2015 15:49:43 +0200
> 
> > @@ -0,0 +1,22 @@
> > +menuconfig FSL_DPAA_ETH
> > + tristate "DPAA Ethernet"
> > + depends on FSL_SOC && FSL_BMAN && FSL_QMAN && FSL_FMAN
> > + select PHYLIB
> > + select FSL_FMAN_MAC
> 
> I do not see the FSL_FMAN_MAC Kconfig symbol defined anywhere in the
> tree.

I believe this patch series is dependent on two
other patch series mentioned in the cover letter.
---
The latest FMan driver patches were submitted by Igal Liberman:
https://patchwork.ozlabs.org/project/netdev/list/?submitter=64715=*

The latest Q/BMan drivers were submitted by Roy Pledge:
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?submitter=66331=*
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] MAINTAINERS: add myself as Renesas Ethernet drivers reviewer

[Joe Perches]

On Sat, 2015-12-05 at 04:13 +0300, Sergei Shtylyov wrote:
> On 12/05/2015 04:02 AM, Joe Perches wrote:
> > > Since you seem to be get_maintainers.pl maintainer, I (and not only I)
> > > would like to propose a switch to suppress the committers/authors/etc. 
> > > It's
> > > generally annoying and unhelpful to have the bunch of people listed, none 
> > > of
> > > which usually are a good addressees for the patches...
> > 
> > Done!
> 
> > Add "--nogit" and "--nogit-fallback"
> 
> Em, --nogit is the dafault, according to --help. --nogit-fallback is 
> enough but it's undocumented. :-/

umm

$ ./scripts/get_maintainer.pl --help
[...]
    --git-fallback => use git when no exact MAINTAINERS pattern (default: 1)
[...]
  Most options have both positive and negative forms.
  The negative forms for -- are --no and --no-.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] MAINTAINERS: add myself as Renesas Ethernet drivers reviewer

[Joe Perches]

On Sat, 2015-12-05 at 03:57 +0300, Sergei Shtylyov wrote:
> Since you seem to be get_maintainers.pl maintainer, I (and not only I)
> would like to propose a switch to suppress the committers/authors/etc. It's 
> generally annoying and unhelpful to have the bunch of people listed, none of 
> which usually are a good addressees for the patches...

Done!

Add "--nogit" and "--nogit-fallback"

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] MAINTAINERS: add myself as Renesas Ethernet drivers reviewer

[Joe Perches]

On Sat, 2015-12-05 at 03:03 +0300, Sergei Shtylyov wrote:
> liAdd myself as  a reviewer for the Renesas Ethernet drivers --
> hopefully I
> won't miss the buggy  patches anymore. :-)
> 
> Signed-off-by: Sergei Shtylyov 
> 
> ---
> The patch is against DaveM's 'net.git' repo.
> 
>  MAINTAINERS |8 
>  1 file changed, 8 insertions(+)
> 
> Index: net/MAINTAINERS
> ===
> --- net.orig/MAINTAINERS
> +++ net/MAINTAINERS
> @@ -8929,6 +8929,14 @@ F: drivers/rpmsg/
>  F:   Documentation/rpmsg.txt
>  F:   include/linux/rpmsg.h
>  
> +RENESAS ETHERNET DRIVERS
> +R:   Sergei Shtylyov 
> +L:   net...@vger.kernel.orgc
> +L:   linux...@vger.kernel.org
> +S:   Maintained
> +F:   drivers/net/ethernet/renesas/
> +F:   include/linux/sh_eth.h

Hello Sergei

Typically a reviewer isn't a maintainer.
Does anyone actually maintain this?

I suggest you mark this "S: Odd fixes" or
remove the "S:" line altogether unless you
really want to become the maintainer.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: use-after-free in sctp_do_sm

[Joe Perches]

On Thu, 2015-12-03 at 15:10 -0500, Jason Baron wrote:
> On 12/03/2015 03:03 PM, Joe Perches wrote:
> > On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote:
> > > On 12/03/2015 01:52 PM, Aaron Conole wrote:
> > > > I think that as a minimum, the following patch should be evaluted,
> > > > but am unsure to whom I should submit it (after I test):
> > []
> > > Agreed - the intention here is certainly to have no side effects. It
> > > looks like 'no_printk()' is used in quite a few other places that would
> > > benefit from this change. So we probably want a generic
> > > 'really_no_printk()' macro.
> > 
> > https://lkml.org/lkml/2012/6/17/231
> 
> I don't see this in the tree.

It never got applied.

> Also maybe we should just convert
> no_printk() to do what your 'eliminated_printk()'.

Some of them at least.

> So we can convert all users with this change?

I don't think so, I think there are some
function evaluation/side effects that are
required.  I believe some do hardware I/O.

It'd be good to at least isolate them.

I'm not sure how to find them via some
automated tool/mechanism though.

I asked Julia Lawall about it once in this
thread:  https://lkml.org/lkml/2014/12/3/696

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: use-after-free in sctp_do_sm

[Joe Perches]

On Thu, 2015-12-03 at 13:52 -0500, Aaron Conole wrote:
> Dmitry Vyukov  writes:
> > On Thu, Dec 3, 2015 at 6:02 PM, Eric Dumazet  wrote:
> > > On Thu, Dec 3, 2015 at 7:55 AM, Dmitry Vyukov  wrote:
> > > > On Thu, Dec 3, 2015 at 3:48 PM, Eric Dumazet  wrote:
> > > > > > 
> > > > > > No, I don't. But pr_debug always computes its arguments. See 
> > > > > > no_printk
> > > > > > in printk.h. So this use-after-free happens for all users.
> > > > > 
> > > > > Hmm.
> > > > > 
> > > > > pr_debug() should be a nop unless either DEBUG or
> > > > > CONFIG_DYNAMIC_DEBUG are set
> > > > > 
> > > > > On our production kernels, pr_debug() is a nop.
> > > > > 
> > > > > Can you double check ? Thanks !
> > > > 
> > > > 
> > > > Why should it be nop? no_printk thing in printk.h pretty much
> > > > explicitly makes it not a nop...
> 
> Because it was until commit 5264f2f75d8. It also violates my reading of
> the following from printk.h:
> 
>  * All of these will print unconditionally, although note that pr_debug()
>  * and other debug macros are compiled out unless either DEBUG is defined
>  * or CONFIG_DYNAMIC_DEBUG is set.
> 
> > > > 
> > > > Double-checked: debug_post_sfx leads to some generated code:
> > > > 
> > > > debug_post_sfx();
> > > > 8229f256:   48 8b 85 58 fe ff ffmov-0x1a8(%rbp),%rax
> > > > 8229f25d:   48 85 c0test   %rax,%rax
> > > > 8229f260:   74 24   je
> > > > 8229f286 
> > > > 8229f262:   8b b0 a8 00 00 00   mov0xa8(%rax),%esi
> > > > 8229f268:   48 8b 85 60 fe ff ffmov-0x1a0(%rbp),%rax
> > > > 8229f26f:   44 89 85 74 fe ff ffmov%r8d,-0x18c(%rbp)
> > > > 8229f276:   48 8b 78 20 mov0x20(%rax),%rdi
> > > > 8229f27a:   e8 71 28 01 00  callq
> > > > 822b1af0 
> > > > 8229f27f:   44 8b 85 74 fe ff ffmov-0x18c(%rbp),%r8d
> > > > 
> > > > return error;
> > > > }
> > > > 8229f286:   48 81 c4 a0 01 00 00add$0x1a0,%rsp
> > > > 8229f28d:   44 89 c0mov%r8d,%eax
> > > > 8229f290:   5b  pop%rbx
> > > > 8229f291:   41 5c   pop%r12
> > > > 8229f293:   41 5d   pop%r13
> > > > 8229f295:   41 5e   pop%r14
> > > > 8229f297:   41 5f   pop%r15
> > > > 8229f299:   5d  pop%rbp
> > > > 8229f29a:   c3  retq
> > > 
> > > This is a serious concern, because we let in the past lot of patches
> > > converting traditional
> 
> +1
> 
> > > #ifdef DEBUG
> > > # define some_hand_coded_ugly_debug()  printk( _
> > > #else
> > > # define some_hand_coded_ugly_debug()
> > > #endif
> > > 
> > > On the premise pr_debug() would be a nop.
> > > 
> > > It seems it is not always the case. This is a very serious problem.
> 
> +1
> 
> > > We probably have hundred of potential bugs, because few people
> > > actually make sure all debugging stuff is correct,
> > > like comments can be wrong because they are not updated properly as
> > > time flies.
> > > 
> > > It is definitely a nop for many cases.
> > > 
> > > +void eric_test_pr_debug(struct sock *sk)
> > > +{
> > > +   if (atomic_read(>sk_omem_alloc))
> > > +   pr_debug("%s: optmem leakage for sock %p\n",
> > > +__func__, sk);
> > > +}
> > > 
> > > ->
> > > 
> > > 4740 :
> > > 4740: e8 00 00 00 00   callq  4745 
> > > 4741: R_X86_64_PC32 __fentry__-0x4
> > > 4745: 55   push   %rbp
> > > 4746: 8b 87 24 01 00 00 mov0x124(%rdi),%eax //
> > > atomic_read()  but nothing follows
> > > 474c: 48 89 e5 mov%rsp,%rbp
> > > 474f: 5d   pop%rbp
> > > 4750: c3   retq
> > 
> > 
> > 
> > I would expect that it is nop when argument evaluation does not have
> > side-effects. For example, for a load of a variable compiler will most
> > likely elide it (though, it does not have to elide it, because the
> > load is spelled in the code, so it can also legally emit the load and
> > doesn't use the result).
> > But if argument computation has side-effect (or compiler can't prove
> > otherwise), it must emit code. It must emit code for function calls
> > when the function is defined in a different translation unit, and for
> > volatile accesses (most likely including atomic accesses), etc
> 
> This isn't 100% true. As you state, in order to reach the return 0, all
> side effects must be evaluated. Load generally does not have side
> effects, so it can be safely elided, but function() must be emitted.
> 
> However, that is _not_ required to get the desired warning emission on a
> printf 

Re: use-after-free in sctp_do_sm

[Joe Perches]

On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote:
> On 12/03/2015 01:52 PM, Aaron Conole wrote:
> > I think that as a minimum, the following patch should be evaluted,
> > but am unsure to whom I should submit it (after I test):
[]
> Agreed - the intention here is certainly to have no side effects. It
> looks like 'no_printk()' is used in quite a few other places that would
> benefit from this change. So we probably want a generic
> 'really_no_printk()' macro.

https://lkml.org/lkml/2012/6/17/231

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: use-after-free in sctp_do_sm

[Joe Perches]

(adding lkml as this is likely better discussed there)

On Thu, 2015-12-03 at 15:42 -0500, Jason Baron wrote:
> On 12/03/2015 03:24 PM, Joe Perches wrote:
> > On Thu, 2015-12-03 at 15:10 -0500, Jason Baron wrote:
> > > On 12/03/2015 03:03 PM, Joe Perches wrote:
> > > > On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote:
> > > > > On 12/03/2015 01:52 PM, Aaron Conole wrote:
> > > > > > I think that as a minimum, the following patch should be evaluted,
> > > > > > but am unsure to whom I should submit it (after I test):
> > > > []
> > > > > Agreed - the intention here is certainly to have no side effects. It
> > > > > looks like 'no_printk()' is used in quite a few other places that 
> > > > > would
> > > > > benefit from this change. So we probably want a generic
> > > > > 'really_no_printk()' macro.
> > > > 
> > > > https://lkml.org/lkml/2012/6/17/231
> > > 
> > > I don't see this in the tree.
> > 
> > It never got applied.
> > 
> > > Also maybe we should just convert
> > > no_printk() to do what your 'eliminated_printk()'.
> > 
> > Some of them at least.
> > 
> > > So we can convert all users with this change?
> > 
> > I don't think so, I think there are some
> > function evaluation/side effects that are
> > required.  I believe some do hardware I/O.
> > 
> > It'd be good to at least isolate them.
> > 
> > I'm not sure how to find them via some
> > automated tool/mechanism though.
> > 
> > I asked Julia Lawall about it once in this
> > thread:  https://lkml.org/lkml/2014/12/3/696
> > 
> 
> Seems rather fragile to have side effects that we rely
> upon hidden in a printk().

Yup.

> Just convert them and see what breaks :)

I appreciate your optimism.  It's very 1995.
Try it and see what happens.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next 01/17] fm10k: Fix error handling in the function fm10k_setup_tc for certain function calls

[Joe Perches]

On Thu, 2015-12-03 at 16:29 -0800, Jeff Kirsher wrote:
> From: Nick 

I guess this is an OK patch, but the From line isn't correct
(missing full name) and the changelog is nigh on unreadable.

> This fixes the function fm10k_setup_tc to properly check if the
> calls to either the function fm10k_init_queueing_scheme or the
> function fm10k_mbx_request_irq fail by returning a error code to
> signal that the call to either function has failed. Furthermore
> if this arises exit immediately from the function fm10k_setup_tc
> by returning the returned error code from the failed function call
> to signal to the caller that setting up the tc on the device has
> failed and the caller needs to handle this failed setup.

Maybe something like:

Add error handling in fm10k_setup_tc of fm10k_init_queueing_scheme
and fm10k_mbx_request_irq.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] i40e: Fix i40e_print_features() VEB mode output

[Joe Perches]

Commit 7fd89545f337 ("i40e: remove BUG_ON from feature string building")
added defective output when I40E_FLAG_VEB_MODE_ENABLED was set in
function i40e_print_features.

Fix it.

Miscellanea:

o Remove unnecessary string variable
o Add space before not after fixed strings
o Use kmalloc not kzalloc
o Don't initialize i to 0, use result of first snprintf

Noticed-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 42 +
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c 
b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 4b7d874..145eeb5 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -10240,52 +10240,48 @@ static int i40e_setup_pf_filter_control(struct 
i40e_pf *pf)
 static void i40e_print_features(struct i40e_pf *pf)
 {
    struct i40e_hw *hw = >hw;
-   char *buf, *string;
-   int i = 0;
+   char *buf;
+   int i;
 
-   string = kzalloc(INFO_STRING_LEN, GFP_KERNEL);
-   if (!string) {
-   dev_err(>pdev->dev, "Features string allocation failed\n");
+   buf = kmalloc(INFO_STRING_LEN, GFP_KERNEL);
+   if (!buf)
    return;
-   }
-
-   buf = string;
 
-   i += snprintf([i], REMAIN(i), "Features: PF-id[%d] ", hw->pf_id);
+   i = snprintf(buf, INFO_STRING_LEN, "Features: PF-id[%d]", hw->pf_id);
 #ifdef CONFIG_PCI_IOV
-   i += snprintf([i], REMAIN(i), "VFs: %d ", pf->num_req_vfs);
+   i += snprintf([i], REMAIN(i), " VFs: %d", pf->num_req_vfs);
 #endif
-   i += snprintf([i], REMAIN(i), "VSIs: %d QP: %d RX: %s ",
+   i += snprintf([i], REMAIN(i), " VSIs: %d QP: %d RX: %s",
      pf->hw.func_caps.num_vsis,
      pf->vsi[pf->lan_vsi]->num_queue_pairs,
      pf->flags & I40E_FLAG_RX_PS_ENABLED ? "PS" : "1BUF");
 
    if (pf->flags & I40E_FLAG_RSS_ENABLED)
-   i += snprintf([i], REMAIN(i), "RSS ");
+   i += snprintf([i], REMAIN(i), " RSS");
    if (pf->flags & I40E_FLAG_FD_ATR_ENABLED)
-   i += snprintf([i], REMAIN(i), "FD_ATR ");
+   i += snprintf([i], REMAIN(i), " FD_ATR");
    if (pf->flags & I40E_FLAG_FD_SB_ENABLED) {
-   i += snprintf([i], REMAIN(i), "FD_SB ");
-   i += snprintf([i], REMAIN(i), "NTUPLE ");
+   i += snprintf([i], REMAIN(i), " FD_SB");
+   i += snprintf([i], REMAIN(i), " NTUPLE");
    }
    if (pf->flags & I40E_FLAG_DCB_CAPABLE)
-   i += snprintf([i], REMAIN(i), "DCB ");
+   i += snprintf([i], REMAIN(i), " DCB");
 #if IS_ENABLED(CONFIG_VXLAN)
-   i += snprintf([i], REMAIN(i), "VxLAN ");
+   i += snprintf([i], REMAIN(i), " VxLAN");
 #endif
    if (pf->flags & I40E_FLAG_PTP)
-   i += snprintf([i], REMAIN(i), "PTP ");
+   i += snprintf([i], REMAIN(i), " PTP");
 #ifdef I40E_FCOE
    if (pf->flags & I40E_FLAG_FCOE_ENABLED)
-   i += snprintf([i], REMAIN(i), "FCOE ");
+   i += snprintf([i], REMAIN(i), " FCOE");
 #endif
    if (pf->flags & I40E_FLAG_VEB_MODE_ENABLED)
-   i += snprintf([i], REMAIN(i), "VEPA ");
+   i += snprintf([i], REMAIN(i), " VEB");
    else
-   buf += sprintf(buf, "VEPA ");
+   i += snprintf([i], REMAIN(i), " VEPA");
 
-   dev_info(>pdev->dev, "%s\n", string);
-   kfree(string);
+   dev_info(>pdev->dev, "%s\n", buf);
+   kfree(buf);
    WARN_ON(i > INFO_STRING_LEN);
 }
 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i40e: Fix i40e_print_features() VEB mode output

[Joe Perches]

On Wed, 2015-12-02 at 15:48 -0500, David Miller wrote:
> From: Joe Perches <j...@perches.com> 02 Dec 2015 02:12:29 -0800
> > On Wed, 2015-12-02 at 01:56 -0800, Jeff Kirsher wrote:
> >> On Wed, 2015-12-02 at 00:38 -0800, Joe Perches wrote:
> >> > Noticed-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com> 
> >> Don't you mean Reported-by?  I am not aware of Noticed-by as being a
> >> recognized signature. 
> > At least for the get_maintainer script, "-by:" is a signature
> Is patchwork using the same regexp?  If not, for the time being don't
> user non-standard tags, and furthermore please ask the patchwork folks
> to use something similar to getmaintainer.pl

It doesn't seem so.

response_re = re.compile(
r'^(Tested|Reviewed|Acked|Signed-off|Nacked|Reported)-by: .*$',
re.M | re.I)

patchwork also doesn't seem very forgiving of misformatted signatures.
(btw: Junio Hamano seems to prefer calling them trailers)

And patchwork doesn't seem to handle "cc: " markers either.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i40e: Fix i40e_print_features() VEB mode output

[Joe Perches]

On Wed, 2015-12-02 at 01:56 -0800, Jeff Kirsher wrote:
> On Wed, 2015-12-02 at 00:38 -0800, Joe Perches wrote:
> > Commit 7fd89545f337 ("i40e: remove BUG_ON from feature string
> > building")
> > added defective output when I40E_FLAG_VEB_MODE_ENABLED was set in
> > function i40e_print_features.
> > 
> > Fix it.
> > 
> > Miscellanea:
> > 
> > o Remove unnecessary string variable
> > o Add space before not after fixed strings
> > o Use kmalloc not kzalloc
> > o Don't initialize i to 0, use result of first snprintf
> > 
> > Noticed-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
> 
> Don't you mean Reported-by?  I am not aware of Noticed-by as being a
> recognized signature.

At least for the get_maintainer script, "-by:" is a signature


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next v2 04/15] i40e: remove BUG_ON from feature string building

[Joe Perches]

On Wed, 2015-11-25 at 11:36 -0800, Joe Perches wrote:
> On Wed, 2015-11-25 at 10:35 -0800, Jeff Kirsher wrote:
> > On Wed, 2015-11-25 at 21:26 +0300, Sergei Shtylyov wrote:
> > > On 11/25/2015 09:21 PM, Jeff Kirsher wrote:
> > > 
> > > > From: Shannon Nelson <shannon.nel...@intel.com>
> > > > 
> > > > There's really no reason to kill the kernel thread just because
> > > > of a
> > > > little info string. This reworks the code to use snprintf's
> > > > limiting to
> > > > assure that the string is never too long, and WARN_ON to still
> > > > put out
> > > > a warning that we might want to look at the feature list
> > > > length.
> > > > 
> > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > > > b/drivers/net/ethernet/intel/i40e/i40e_main.c
> []
> > > >if (pf->flags & I40E_FLAG_VEB_MODE_ENABLED)
> > > > - buf += sprintf(buf, "VEB ");
> > > > + i += snprintf([i], REMAIN(i), "VEPA ");
> > > 
> > > Not "VEB "?
> > 
> > Nice catch Sergei, I will wait a till this afternoon to respin the
> > patch series, just in case there are other changes needed that our
> > validation did not catch. :-)
> 
> trivia:
> 
> If you redo these, it'd be nicer not to use " " after each
> fixed string, but use " " before each fixed string.
> 
> The final output string would be 1 byte shorter overall and
> not have an excess " " before the newline.
> 
> The declaration of i doesn't need initialization to 0:
> 
>   i = snprintf(buf, INFO_STRING_LEN, "Features: PF-id[%d]", ...
> 
> would work.

Maybe something like this patch (net-next)

Fix I40E_FLAG_VEB_MODE_ENABLED output of VEB

Miscellanea:
o Remove unnecessary string variable
o Add space before not after fixed strings
o Use kmalloc not kzalloc
o Don't initialize i to 0, use result of first snprintf

---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 42 +
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c 
b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 4b7d874..145eeb5 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -10240,52 +10240,48 @@ static int i40e_setup_pf_filter_control(struct 
i40e_pf *pf)
 static void i40e_print_features(struct i40e_pf *pf)
 {
struct i40e_hw *hw = >hw;
-   char *buf, *string;
-   int i = 0;
+   char *buf;
+   int i;
 
-   string = kzalloc(INFO_STRING_LEN, GFP_KERNEL);
-   if (!string) {
-   dev_err(>pdev->dev, "Features string allocation failed\n");
+   buf = kmalloc(INFO_STRING_LEN, GFP_KERNEL);
+   if (!buf)
return;
-   }
-
-   buf = string;
 
-   i += snprintf([i], REMAIN(i), "Features: PF-id[%d] ", hw->pf_id);
+   i = snprintf(buf, INFO_STRING_LEN, "Features: PF-id[%d]", hw->pf_id);
 #ifdef CONFIG_PCI_IOV
-   i += snprintf([i], REMAIN(i), "VFs: %d ", pf->num_req_vfs);
+   i += snprintf([i], REMAIN(i), " VFs: %d", pf->num_req_vfs);
 #endif
-   i += snprintf([i], REMAIN(i), "VSIs: %d QP: %d RX: %s ",
+   i += snprintf([i], REMAIN(i), " VSIs: %d QP: %d RX: %s",
  pf->hw.func_caps.num_vsis,
  pf->vsi[pf->lan_vsi]->num_queue_pairs,
  pf->flags & I40E_FLAG_RX_PS_ENABLED ? "PS" : "1BUF");
 
if (pf->flags & I40E_FLAG_RSS_ENABLED)
-   i += snprintf([i], REMAIN(i), "RSS ");
+   i += snprintf([i], REMAIN(i), " RSS");
if (pf->flags & I40E_FLAG_FD_ATR_ENABLED)
-   i += snprintf([i], REMAIN(i), "FD_ATR ");
+   i += snprintf([i], REMAIN(i), " FD_ATR");
if (pf->flags & I40E_FLAG_FD_SB_ENABLED) {
-   i += snprintf([i], REMAIN(i), "FD_SB ");
-   i += snprintf([i], REMAIN(i), "NTUPLE ");
+   i += snprintf([i], REMAIN(i), " FD_SB");
+   i += snprintf([i], REMAIN(i), " NTUPLE");
}
if (pf->flags & I40E_FLAG_DCB_CAPABLE)
-   i += snprintf([i], REMAIN(i), "DCB ");
+   i += snprintf([i], REMAIN(i), " DCB");
 #if IS_ENABLED(CONFIG_VXLAN)
-   i += snprintf([i], REMAIN(i), "VxLAN ");
+   i += snprintf([i], REMAIN(i), " VxLAN");
 #endif
if (pf->flags & I40E_FLAG_PTP)
-  

Re: [PATCH] vhost: replace % with & on data path

[Joe Perches]

On Mon, 2015-11-30 at 10:34 +0200, Michael S. Tsirkin wrote:
> We know vring num is a power of 2, so use &
> to mask the high bits.
[]
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
[]
> @@ -1366,10 +1366,12 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
>   /* Only get avail ring entries after they have been exposed by guest. */
>   smp_rmb();
>  
> + }

?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] staging: net: core: skbuff.c: Added do-while pair for complex macros

[Joe Perches]

On Thu, 2015-11-26 at 22:06 +0530, Jitendra Kumar Khasdev wrote:
> This patch is to file skbuff.c that fixes up following error,
> reported by checkpatch.pl tool,

Your subject title is not correct.
This is not a staging patch.

> 1. ERROR: Macros with multiple statements should be enclosed
>in a do - while loop.

checkpatch is brainless.
Not every message it emits needs fixing.


> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
[]
> @@ -748,11 +748,13 @@ void consume_skb(struct sk_buff *skb)
>  EXPORT_SYMBOL(consume_skb);
>  
>  /* Make sure a field is enclosed inside headers_start/headers_end section */
> -#define CHECK_SKB_FIELD(field) \
> - BUILD_BUG_ON(offsetof(struct sk_buff, field) <  \
> -  offsetof(struct sk_buff, headers_start));  \
> - BUILD_BUG_ON(offsetof(struct sk_buff, field) >  \
> -  offsetof(struct sk_buff, headers_end));\
> +#define CHECK_SKB_FIELD(field)   
> \
> + do {\
> + BUILD_BUG_ON(offsetof(struct sk_buff, field) <  \
> + offsetof(struct sk_buff, headers_start));   \
> + BUILD_BUG_ON(offsetof(struct sk_buff, field) >  \
> + offsetof(struct sk_buff, headers_end)); \
> + } while (0) \

Perhaps the last check should add a sizeof(field)

BUILD_BUG_ON((offsetof(struct sk_buff, field) +
  FIELD_SIZEOF(struct sk_buff, field)) >
 offsetof(struct sk_buff, headers_end));


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next v2 04/15] i40e: remove BUG_ON from feature string building

[Joe Perches]

On Wed, 2015-11-25 at 10:35 -0800, Jeff Kirsher wrote:
> On Wed, 2015-11-25 at 21:26 +0300, Sergei Shtylyov wrote:
> > On 11/25/2015 09:21 PM, Jeff Kirsher wrote:
> > 
> > > From: Shannon Nelson 
> > > 
> > > There's really no reason to kill the kernel thread just because of a
> > > little info string. This reworks the code to use snprintf's limiting to
> > > assure that the string is never too long, and WARN_ON to still put out
> > > a warning that we might want to look at the feature list length.
> > > 
> > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c 
> > > b/drivers/net/ethernet/intel/i40e/i40e_main.c
[]
> > >if (pf->flags & I40E_FLAG_VEB_MODE_ENABLED)
> > > - buf += sprintf(buf, "VEB ");
> > > + i += snprintf([i], REMAIN(i), "VEPA ");
> > 
> > Not "VEB "?
> 
> Nice catch Sergei, I will wait a till this afternoon to respin the
> patch series, just in case there are other changes needed that our
> validation did not catch. :-)

trivia:

If you redo these, it'd be nicer not to use " " after each
fixed string, but use " " before each fixed string.

The final output string would be 1 byte shorter overall and
not have an excess " " before the newline.

The declaration of i doesn't need initialization to 0:

i = snprintf(buf, INFO_STRING_LEN, "Features: PF-id[%d]", ...

would work.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next 06/16] i40e: Properly cast type for arithmetic

[Joe Perches]

On Wed, 2015-11-25 at 02:46 -0800, Jeff Kirsher wrote:
> On Tue, 2015-11-24 at 16:43 -0800, Joe Perches wrote:
> > On Tue, 2015-11-24 at 16:04 -0800, Jeff Kirsher wrote:
> > > From: Helin Zhang <helin.zh...@intel.com>
> > > 
> > > Pointer of type void * shouldn't be used in arithmetic, which may
> > > result in compilation error. Casting of (u8 *) can be added to fix
> > > that.
> > > 
> > 
> > void * arithmetic is used quite frequently in the kernel.
> > 
> > What compiler emits an error?
> 
> When you use the gcc -Wpointer-arith, it generates the warning.

make W=3 does that

$ make help
[...]
make W=n   [targets] Enable extra gcc checks, n=1,2,3 where
1: warnings which may be relevant and do not occur too often
2: warnings which occur quite often but may still be relevant
3: more obscure warnings, can most likely be ignored

but I think it should be ignored.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next 06/16] i40e: Properly cast type for arithmetic

[Joe Perches]

On Tue, 2015-11-24 at 16:04 -0800, Jeff Kirsher wrote:
> From: Helin Zhang 
> 
> Pointer of type void * shouldn't be used in arithmetic, which may
> result in compilation error. Casting of (u8 *) can be added to fix
> that.
> 

void * arithmetic is used quite frequently in the kernel.

What compiler emits an error?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next 11/16] i40e/i40evf: clean up error messages

[Joe Perches]

On Tue, 2015-11-24 at 16:04 -0800, Jeff Kirsher wrote:
> Clean up and enhance error messages related to VF MAC/VLAN filters.
> Indicate which VF is having issues, and if possible indicate the MAC
> address or VLAN involved.

trivia:


> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c 
> b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
[]
> @@ -1623,7 +1623,8 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, 
> u8 *msg, u16 msglen)
>  
>   if (!f) {
>   dev_err(>pdev->dev,
> - "Unable to add VF MAC filter\n");
> + "Unable to add MAC filter %pM for VF %d\n",
> +  al->list[i].addr, vf->vf_id);

Maybe use %hu for %d?
(here and elsewhere)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] MAINTAINERS: PHY: Change maintainer to reviewer

[Joe Perches]

On Tue, 2015-11-24 at 15:29 -0800, Florian Fainelli wrote:
> Now that there is a reviewer role, add myself as reviewer since the PHY
> library code is maintained via the networking tree.

[]

> diff --git a/MAINTAINERS b/MAINTAINERS
[]
> @@ -4195,7 +4195,7 @@ F:  include/linux/netfilter_bridge/
>  F:>  > net/bridge/
>  
>  ETHERNET PHY LIBRARY
> -M:   Florian Fainelli 
> +R:   Florian Fainelli 
>  L:   netdev@vger.kernel.org
>  S:   Maintained
>  F:   include/linux/phy.h

Just because the upstream path is not a tree you
manage doesn't mean you aren't a maintainer.

I think the status should be something other than
"Maintained" if you are not the maintainer.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net/ipv4/ipconfig: Rejoin broken lines in console output

[Joe Perches]

On Tue, 2015-11-24 at 14:08 +0100, Geert Uytterhoeven wrote:
> Commit 09605cc12c078306 ("net ipv4: use preferred log methods") replaced
> a few calls of pr_cont() after a console print without a trailing
> newline by pr_info(), causing lines to be split during IP
> autoconfiguration


Thanks Geert.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next 04/15] ixgbe: Delete redundant include file

[Joe Perches]

On Mon, 2015-11-23 at 11:36 -0800, Jeff Kirsher wrote:
> Delete a redundant include of net/vxlan.h.

unrelated trivia:

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
[]
>  char ixgbe_driver_name[] = "ixgbe";

This could be const or a #define

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] wireless: change cfg80211 regulatory domain info as debug messages

[Joe Perches]

On Sun, 2015-11-15 at 15:31 +0800, Dave Young wrote:
> cfg80211 module prints a lot of messages like below. Actually printing
> once is acceptable but sometimes it will print again and again, it looks
> very annoying. It is better to change these detail messages to debugging
> only.

So maybe add some wrapper that does a pr_info then
a pr_debug for the second and subsequent uses like:

#define pr_info_once_then_debug(fmt, ...)   \
({  \
static bool __print_once __read_mostly; \
\
if (!__print_once) {\
__print_once = true;\
pr_info(fmt, ##__VA_ARGS__);\
} else {\
pr_debug(fmt, ##__VA_ARGS__);   \
}   \
})

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] stmmac: avoid ipq806x constant overflow warning

[Joe Perches]

On Fri, 2015-11-13 at 08:37 +0100, Geert Uytterhoeven wrote:
> On Thu, Nov 12, 2015 at 10:03 PM, Arnd Bergmann 
> wrote:
> > Building dwmac-ipq806x on a 64-bit architecture produces a harmless
> > warning from gcc:
> > 
> > stmmac/dwmac-ipq806x.c: In function 'ipq806x_gmac_probe':
> > include/linux/bitops.h:6:19: warning: overflow in implicit constant
> > conversion [-Woverflow]
> >   val = QSGMII_PHY_CDR_EN |
> > stmmac/dwmac-ipq806x.c:333:8: note: in expansion of macro
> > 'QSGMII_PHY_CDR_EN'
> >  #define QSGMII_PHY_CDR_EN   BIT(0)
> >  #define BIT(nr)   (1UL << (nr))
> > 
> > This is a result of the type conversion rules in C, when we take
> > the
> > logical OR of multiple different types. In particular, we have
> > and unsigned long
> > 
> > QSGMII_PHY_CDR_EN == BIT(0) == (1ul << 0) ==
> > 0x0001ul
> > 
> > and a signed int
> > 
> > 0xC << QSGMII_PHY_TX_DRV_AMP_OFFSET == 0xc000
> > 
> > which together gives a signed long value
> > 
> > 0xc001l
> > 
> > and when this is passed into a function that takes an unsigned int
> > type,
> > gcc warns about the signed overflow and the loss of the upper 32
> > -bits that
> > are all ones.
> > 
> > This patch adds 'ul' type modifiers to the literal numbers passed
> > in
> > here, so now the expression remains an 'unsigned long' with the
> > upper
> > bits all zero, and that avoids the signed overflow and the warning.
> 
> FWIW, the 64-bitness of BIT() on 64-bit platforms is also causing
> subtle
> warnings in other places, e.g. when inverting them to create bit
> mask, cfr.
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commi
> t/?id=a9efeca613a8fe5281d7c91f5c8c9ea46f2312f6
> 
> Gr{oetje,eeting}s,

I still think specific length BIT macros
can be useful.

https://lkml.org/lkml/2015/10/16/852

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] decnet: remove macro-local declarations

[Joe Perches]

On Thu, 2015-11-05 at 20:38 +0100, Julia Lawall wrote:
> On Thu, 5 Nov 2015, David Miller wrote:
> > From: Julia Lawall 
> > Date: Thu,  5 Nov 2015 11:18:16 +0100> 
> > > Move the variable declarations from the for_nexthops macro to the
> > > surrounding context, so that it is clear where these variables are
> > > declared.  This also makes it possible to remove the endfor_nexthops 
> > > macro.
> > > 
> > > This change adds new arguments to the macro for_nexthops.  They are 
> > > ordered
> > > such that a pointer to the referenced object comes first, the index in the
> > > list comes next, and the list itself comes last, roughly in analogy with
> > > the list_for_each macros.
[]
> > > This patch takes care of a single file, where the macros are defined
> > > locally.  If the basic transformation looks OK, I will change the other
> > > files that either likewise define their own macros or use the macros in
> > > net/mpls/internal.h.  The potentially affected files are:
> >  ...
> > 
> > This looks fine to me.
> > 
> > Please resubmit this when net-next opens back up, which should be
> > shortly after -rc1.
> 
> OK, I'll do the others then too.

If you do can you please parenthesize the macro arguments?

#define for_nexthops(nh, nhsel, fi) \
for (nhsel = 0, nh = (fi)->fib_nh; nhsel < (fi)->fib_nhs; (nh)++, 
(nhsel)++)
instead of
for(nhsel = 0, nh = (fi)->fib_nh; nhsel < (fi)->fib_nhs; nh++, nhsel++)

And perhaps a renaming might be better

s/for_nexthops/for_each_nexthop/
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next v4 2/8] dpaa_eth: add support for DPAA Ethernet

[Joe Perches]

On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> This introduces the Freescale Data Path Acceleration Architecture
> (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan,
> BMan, PAMU and FMan drivers to deliver Ethernet connectivity on
> the Freescale DPAA QorIQ platforms.
[]
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c 
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
[]
> +static void _dpa_rx_error(struct net_device *net_dev,
> +   const struct dpa_priv_s *priv,
> +   struct dpa_percpu_priv_s *percpu_priv,
> +   const struct qm_fd *fd,
> +   u32 fqid)
> +{
> + /* limit common, possibly innocuous Rx FIFO Overflow errors'
> +  * interference with zero-loss convergence benchmark results.
> +  */
> + if (likely(fd->status & FM_FD_ERR_PHYSICAL))
> + pr_warn_once("non-zero error counters in fman statistics 
> (sysfs)\n");
> + else
> + if (net_ratelimit())
> + netif_err(priv, hw, net_dev, "Err FD status = 0x%08x\n",
> +   fd->status & FM_FD_STAT_RX_ERRORS);

It's a bit of a pity the logging message code is
a mix of pr_, dev_, netdev_
and netif_

Perhaps netif__ratelimited macros should be added.

Something like:

---
 include/linux/netdevice.h | 54 +++
 1 file changed, 54 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 210d11a..555471d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4025,6 +4025,60 @@ do { 
\
 })
 #endif
 
+#define netif_level_ratelimited(level, priv, type, dev, fmt, args...)  \
+do {   \
+   if (netif_msg_##type(priv) && net_ratelimit())  \
+   netdev_##level(dev, fmt, ##args);   \
+} while (0)
+
+#define netif_emerg_ratelimited(priv, type, dev, fmt, args...) \
+   netif_level_ratelimited(emerg, priv, type, dev, fmt, ##args)
+#define netif_alert_ratelimited(priv, type, dev, fmt, args...) \
+   netif_level_ratelimited(alert, priv, type, dev, fmt, ##args)
+#define netif_crit_ratelimited(priv, type, dev, fmt, args...)  \
+   netif_level_ratelimited(crit, priv, type, dev, fmt, ##args)
+#define netif_err_ratelimited(priv, type, dev, fmt, args...)   \
+   netif_level_ratelimited(err, priv, type, dev, fmt, ##args)
+#define netif_warn_ratelimited(priv, type, dev, fmt, args...)  \
+   netif_level_ratelimited(warn, priv, type, dev, fmt, ##args)
+#define netif_notice_ratelimited(priv, type, dev, fmt, args...)
\
+   netif_level_ratelimited(notice, priv, type, dev, fmt, ##args)
+#define netif_info_ratelimited(priv, type, dev, fmt, args...)  \
+   netif_level_ratelimited(info, priv, type, dev, fmt, ##args)
+
+#if defined(CONFIG_DYNAMIC_DEBUG)
+/* descriptor check is first to prevent flooding with "callbacks suppressed" */
+#define netif_dbg_ratelimited(priv, type, dev, fmt, args...)   \
+do {   \
+   DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
+   if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT) &&\
+   netif_msg_##type(priv) && net_ratelimit())  \
+   __dynamic_netdev_dbg(, dev, fmt, ##args);\
+} while (0)
+#elif defined(DEBUG)
+#define netif_dbg_ratelimited(priv, type, dev, fmt, args...)   \
+do {   \
+   if (netif_msg_##type(priv) && net_ratelimit())  \
+   netif_printk(priv, type, KERN_DEBUG, dev, fmt, ##args); \
+} while (0)
+#else
+#define netif_dbg_ratelimited(priv, type, dev, fmt, args...)   \
+do {   \
+   if (0)  \
+   netif_printk(priv, type, KERN_DEBUG, dev, fmt, ##args); \
+} while (0)
+#endif
+
+#if defined(VERBOSE_DEBUG)
+#define netif_vdbg_ratelimited netif_dbg_ratelimited
+#else
+#define netif_vdbg(priv, type, dev, fmt, args...)  \
+do {   \
+   if (0)  \
+   netif_printk(priv, type, KERN_DEBUG, dev, fmt, ##args); \
+} while (0)
+#endif
+
 /*
  * The list of packet types we will receive (as opposed to discard)
  * and the routines to invoke.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next v4 1/8] devres: add devm_alloc_percpu()

[Joe Perches]

On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> Introduce managed counterparts for alloc_percpu() and free_percpu().
> Add devm_alloc_percpu() and devm_free_percpu() into the managed
> interfaces list.

trivia, could be fixed later

> +/**
> + * __devm_alloc_percpu - Resource-managed alloc_percpu
> + * @dev: Device to allocate per-cpu memory for
> + * @size: Size of per-cpu memory to allocate
> + * @align: Alignement of per-cpu memory to allocate

French spelling?  alignment


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next v4 3/8] dpaa_eth: add support for S/G frames

[Joe Perches]

On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> Add support for Scater/Gather (S/G) frames. The FMan can place
> the frame content into multiple buffers and provide a S/G Table
> (SGT) into one first buffer with references to the others.

trivia: scatter

> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth_common.c 
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth_common.c
[]
> @@ -1177,10 +1177,42 @@ void dpaa_eth_init_ports(struct mac_device *mac_dev,
> port_fqs->rx_defq, _layout[RX]);
>  }
>  
> +void dpa_release_sgt(struct qm_sg_entry *sgt)
> +{
> + struct dpa_bp *dpa_bp;
> + struct bm_buffer bmb[DPA_BUFF_RELEASE_MAX];

Where is "struct bm_buffer" defined?


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next v4 6/8] dpaa_eth: add ethtool statistics

[Joe Perches]

On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> Add a series of counters to be exported through ethtool:
> - add detailed counters for reception errors;
> - add detailed counters for QMan enqueue reject events;
> - count the number of fragmented skbs received from the stack;
> - count all frames received on the Tx confirmation path;
> - add congestion group statistics;
> - count the number of interrupts for each CPU.
[]
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c 
> b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
[]
> +static void dpa_get_strings(struct net_device *net_dev, u32 stringset, u8 
> *data)
> +{
> + unsigned int i, j, num_cpus, size;
> + char string_cpu[ETH_GSTRING_LEN];
> + u8 *strings;
> +
> + strings   = data;
> + num_cpus  = num_online_cpus();
> + size  = DPA_STATS_GLOBAL_LEN * ETH_GSTRING_LEN;
> +
> + for (i = 0; i < DPA_STATS_PERCPU_LEN; i++) {
> + for (j = 0; j < num_cpus; j++) {
> + snprintf(string_cpu, ETH_GSTRING_LEN, "%s [CPU %d]",
> +  dpa_stats_percpu[i], j);
> + memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> + strings += ETH_GSTRING_LEN;
> + }
> + snprintf(string_cpu, ETH_GSTRING_LEN, "%s [TOTAL]",
> +  dpa_stats_percpu[i]);
> + memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> + strings += ETH_GSTRING_LEN;
> + }
> + memcpy(strings, dpa_stats_global, size);
> +}

This leaks uninitialized stack via a memcpy of uninitialized
string_cpu bytes into user-space.

Using
char string_cpu[ETH_GSTRING_LEN] = {};
or a memset before each snprintf would fix it.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next v4 3/8] dpaa_eth: add support for S/G frames

[Joe Perches]

On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> Add support for Scater/Gather (S/G) frames. The FMan can place
> the frame content into multiple buffers and provide a S/G Table
> (SGT) into one first buffer with references to the others.

trivia:

> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h 
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
[]
> @@ -347,7 +347,7 @@ static inline void clear_fd(struct qm_fd *fd)
>  }
>  
>  static inline int _dpa_tx_fq_to_id(const struct dpa_priv_s *priv,
> -struct qman_fq *tx_fq)
> + struct qman_fq *tx_fq)

superfluous change?

> +void dpa_release_sgt(struct qm_sg_entry *sgt)
> +{
> + struct dpa_bp *dpa_bp;
> + struct bm_buffer bmb[DPA_BUFF_RELEASE_MAX];
> + u8 i = 0, j;

Using int may be better than u8 for indexing


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: hix5hd2_gmac: avoid integer overload warning

[Joe Perches]

On Fri, 2015-10-16 at 13:28 +0200, Arnd Bergmann wrote:
> On Friday 16 October 2015 11:14:44 David Laight wrote:
> > From: Arnd Bergmann
> > > Sent: 16 October 2015 11:01
> > > BITS_RX_EN is an 'unsigned long' constant, so the ones complement of that
> > > has bits set that do not fit into a 32-bit variable on 64-bit 
> > > architectures,
> > > which causes a harmless gcc warning:
> > ...
> > >  static void hix5hd2_port_disable(struct hix5hd2_priv *priv)
> > >  {
> > > - writel_relaxed(~(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);
> > > + writel_relaxed(~(u32)(BITS_RX_EN | BITS_TX_EN), priv->base + 
> > > PORT_EN);
> > >   writel_relaxed(0, priv->base + DESC_WR_RD_ENA);
> > 
> > ISTM that just means that the constants shouldn't be 'long'.
> 
> Right, but that would probably mean changing the BIT() macro or not using it
> here. In the past I've argued against using that macro, but I've given
> up that fight.

Fight on... (Somebody must have gone to USC here)

There might be value in a BIT_U32 macro.
Maybe BIT_U64 too.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: hisilicon: add OF dependency

[Joe Perches]

On Sat, 2015-10-17 at 02:16 +0800, kbuild test robot wrote:
> All errors (new ones prefixed by >>):
>drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c: In function 
> 'hns_dsaf_get_cfg':
> >> drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c:151:3: error: implicit 
> >> declaration of function 'iounmap' [-Werror=implicit-function-declaration]
>   iounmap(dsaf_dev->io_base);
>   ^
>cc1: some warnings being treated as errors
> 
> vim +/iounmap +151 drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
[]
> 511e6bc0 huangdaode 2015-09-17  142   if 
> (!dma_set_mask_and_coherent(dsaf_dev->dev, DMA_BIT_MASK(64ULL)))

btw: this should be DMA_BIT_MASK(64) without the ULL



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: hix5hd2_gmac: avoid integer overload warning

[Joe Perches]

On Fri, 2015-10-16 at 21:50 +0300, Sergei Shtylyov wrote:
> On 10/16/2015 09:04 PM, Joe Perches wrote:
> 
> >>>> BITS_RX_EN is an 'unsigned long' constant, so the ones complement of that
> >>>> has bits set that do not fit into a 32-bit variable on 64-bit 
> >>>> architectures,
> >>>> which causes a harmless gcc warning:
> >>> ...
> >>>>   static void hix5hd2_port_disable(struct hix5hd2_priv *priv)
> >>>>   {
> >>>> - writel_relaxed(~(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);
> >>>> + writel_relaxed(~(u32)(BITS_RX_EN | BITS_TX_EN), priv->base + 
> >>>> PORT_EN);
> >>>>writel_relaxed(0, priv->base + DESC_WR_RD_ENA);
> >>>
> >>> ISTM that just means that the constants shouldn't be 'long'.
> >>
> >> Right, but that would probably mean changing the BIT() macro or not using 
> >> it
> >> here. In the past I've argued against using that macro, but I've given
> >> up that fight.
> >
> > Fight on... (Somebody must have gone to USC here)
> >
> > There might be value in aefin BIT_U32 macro.
> > Maybe BIT_U64 too.
> 
> There's BIT_ULL() already.

I know, but symmetry is good.
I think there'd be no harm in adding it.
Perhaps adding all the sized variants would be useful.

Something like:

#define BIT_OF_TYPE(type, nr)   \
({  \
typeof(type) rtn;   \
BUILD_BUG_ON(__builtin_constant_p(nr) &&\
 ((nr) < 0 ||   \
  (nr) >= sizeof(type) * BITS_PER_BYTE));   \
rtn = ((type)1) << (nr);\
rtn;\
})

#define BIT_U8(nr)  BIT_OF_TYPE(u8, nr)
#define BIT_U16(nr) BIT_OF_TYPE(u16, nr)
#define BIT_U32(nr) BIT_OF_TYPE(u32, nr)
#define BIT_U64(nr) BIT_OF_TYPE(u64, nr)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next 05/17] i40e: Change some messages from info to debug only

[Joe Perches]

On Thu, 2015-10-15 at 02:39 -0700, Jeff Kirsher wrote:
> From: Neerav Parikh 
> 
> There are several error messages that have been printing when there is
> no functional issue. These messages should be available at debug message
> level only.
[]
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c 
> b/drivers/net/ethernet/intel/i40e/i40e_main.c
[]
> @@ -6593,9 +6593,9 @@ static void i40e_reset_and_rebuild(struct i40e_pf *pf, 
> bool reinit)
>   /* make sure our flow control settings are restored */
>   ret = i40e_set_fc(>hw, _fc_aq_fail, true);
>   if (ret)
> - dev_info(>pdev->dev, "set fc fail, err %s aq_err %s\n",
> -  i40e_stat_str(>hw, ret),
> -  i40e_aq_str(>hw, pf->hw.aq.asq_last_status));
> + dev_dbg(>pdev->dev, "setting flow control: ret = %s 
> last_status = %s\n",
> + i40e_stat_str(>hw, ret),
> + i40e_aq_str(>hw, pf->hw.aq.asq_last_status));
[]
> @@ -10333,10 +10339,9 @@ static int i40e_probe(struct pci_dev *pdev, const 
> struct pci_device_id *ent)
>   /* get the requested speeds from the fw */
>   err = i40e_aq_get_phy_capabilities(hw, false, false, , NULL);
>   if (err)
> - dev_info(>pdev->dev,
> -  "get phy capabilities failed, err %s aq_err %s, 
> advertised speed settings may not be correct\n",
> -  i40e_stat_str(>hw, err),
> -  i40e_aq_str(>hw, pf->hw.aq.asq_last_status));
> + dev_dbg(>pdev->dev, "get requested speeds ret =  %s 
> last_status =  %s\n",
> + i40e_stat_str(>hw, err),
> + i40e_aq_str(>hw, pf->hw.aq.asq_last_status));
>   pf->hw.phy.link_info.requested_speeds = abilities.link_speed;
>  
>   /* get the supported phy types from the fw */


Perhaps these 2 are functional issues and should remain
dev_info or maybe upgraded to notice or err



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ethtool: Use kcalloc instead of kmalloc for ethtool_get_strings

[Joe Perches]

It seems that kernel memory can leak into userspace by a
kmalloc, ethtool_get_strings, then copy_to_user sequence.

Avoid this by using kcalloc to zero fill the copied buffer.

Signed-off-by: Joe Perches <j...@perches.com>
---

stable too...

On Tue, 2015-10-13 at 23:59 -0700, Jeff Kirsher wrote:
> From: Jacob Keller <jacob.e.kel...@intel.com>
[]
> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c 
> b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
[]
> @@ -206,13 +206,13 @@ static void fm10k_get_stat_strings(struct net_device 
> *dev, u8 *data)
>   }
>  
>   for (i = 0; i < interface->hw.mac.max_queues; i++) {
> - sprintf(p, "tx_queue_%u_packets", i);
> + snprintf(p, ETH_GSTRING_LEN, "tx_queue_%u_packets", i);

It seems these need a memset after the snprintf to zero fill
bytes after the string terminating \0 to avoid leaking
contents of any unset bytes.

It'd probably be better to allocate a zeroed buffer instead.

>   p += ETH_GSTRING_LEN;
> - sprintf(p, "tx_queue_%u_bytes", i);
> + snprintf(p, ETH_GSTRING_LEN, "tx_queue_%u_bytes", i);

so...

 net/core/ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index b495ab1..29edf74 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1284,7 +1284,7 @@ static int ethtool_get_strings(struct net_device *dev, 
void __user *useraddr)
 
gstrings.len = ret;
 
-   data = kmalloc(gstrings.len * ETH_GSTRING_LEN, GFP_USER);
+   data = kcalloc(gstrings.len, ETH_GSTRING_LEN, GFP_USER);
if (!data)
return -ENOMEM;
 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] e1000 driver remove checkpatch errors, warnings and checks.

[Joe Perches]

On Tue, 2015-10-13 at 15:23 -0700, Alexander Duyck wrote:
> Please don't just blindly 
> follow checkpatch as it can give out erroneous information.
> 
> Looking over most of this patch series it seems like it is taking 
> readability in the wrong direction and reducing the ability to maintain 
> the driver since this code has been "maintenance only" for some time 
> now.  If somebody comes up with a legitimate fix for an issue at some 
> point in the future they will need to work around these patches in order 
> to back-port it into a stable release and that just hurts maintainability.
> 
> I'd say this whole series should be rejected on the grounds that this 
> driver is mostly stable and should only really be modified for bug fixes 
> at this point.  If we really need to go through and do a checkpatch 
> sweep we should probably just focus on serious errors only instead of 
> going astray and chasing down things that are false hits or minor issues 
> that are mostly a matter of preference.

Excellent advice.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro

[Joe Perches]

On Mon, 2015-10-12 at 13:59 +0200, Arnd Bergmann wrote:
> On Monday 12 October 2015 11:23:44 huangdaode wrote:
> > +   s += sprintf(s,
> > +   "\t\ttx_ring on 
> > %p:%u,%u,%u,%u,%u,%llu,%llu\n",
> > +   h->qs[i]->tx_ring.io_base,
[]
> There is actually a more significant problem with this code, which I
> failed to notice when doing the original bugfix: 
> 
> You have a sysfs interface here that exports internal data of the
> device that should not be visible like this. One problem is that
> the io_base is a kernel pointer that must not be visible to non-root
> users (so we don't easily create an attack surface for exploits).

Using %pK might have been appropriate.

> It would probably be better to completely remove that sysfs interface, and
> to use the ethtool netlink interface to export them.

But this would be better.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: nhs: fix 32-bit build warning

[Joe Perches]

On Tue, 2015-10-06 at 23:53 +0200, Arnd Bergmann wrote:
> diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.c 
> b/drivers/net/ethernet/hisilicon/hns/hnae.c
[]
> @@ -448,12 +448,12 @@ static ssize_t handles_show(struct device *dev,
>   s += sprintf(buf + s, "handle %d (eport_id=%u from %s):\n",
>   i++, h->eport_id, h->dev->name);
>   for (j = 0; j < h->q_num; j++) {
> - s += sprintf(buf + s, "\tqueue[%d] on 0x%llx\n",
> -  j, (u64)h->qs[i]->io_base);
> -#define HANDEL_TX_MSG "\t\ttx_ring on 0x%llx:%u,%u,%u,%u,%u,%llu,%llu\n"
> + s += sprintf(buf + s, "\tqueue[%d] on %p\n",
> +  j, h->qs[i]->io_base);
> +#define HANDEL_TX_MSG "\t\ttx_ring on %p:%u,%u,%u,%u,%u,%llu,%llu\n"
>   s += sprintf(buf + s,
>HANDEL_TX_MSG,

Maybe one day remove the misspelled and
used once HANDEL_TX_MSG macro too.

Another possibility is to use a generally
smaller object code style like:

char *s = buf;
...
s += sprintf(s, ...)
...
return s - buf;

instead of

ssize_t s = 0;
...
s += sprintf(buf + s, ...)
...
return s;
---
 drivers/net/ethernet/hisilicon/hns/hnae.c | 35 +++
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.c 
b/drivers/net/ethernet/hisilicon/hns/hnae.c
index f52e99a..d2af46f 100644
--- a/drivers/net/ethernet/hisilicon/hns/hnae.c
+++ b/drivers/net/ethernet/hisilicon/hns/hnae.c
@@ -439,40 +439,39 @@ EXPORT_SYMBOL(hnae_ae_unregister);
 static ssize_t handles_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
-   ssize_t s = 0;
+   char *s = buf;
struct hnae_ae_dev *hdev = cls_to_ae_dev(dev);
struct hnae_handle *h;
int i = 0, j;
 
list_for_each_entry_rcu(h, >handle_list, node) {
-   s += sprintf(buf + s, "handle %d (eport_id=%u from %s):\n",
-   i++, h->eport_id, h->dev->name);
+   s += sprintf(s, "handle %d (eport_id=%u from %s):\n",
+i++, h->eport_id, h->dev->name);
for (j = 0; j < h->q_num; j++) {
-   s += sprintf(buf + s, "\tqueue[%d] on %p\n",
+   s += sprintf(s, "\tqueue[%d] on %p\n",
 j, h->qs[i]->io_base);
-#define HANDEL_TX_MSG "\t\ttx_ring on %p:%u,%u,%u,%u,%u,%llu,%llu\n"
-   s += sprintf(buf + s,
-HANDEL_TX_MSG,
+   s += sprintf(s,
+"\t\ttx_ring on 
%p:%u,%u,%u,%u,%u,%llu,%llu\n",
 h->qs[i]->tx_ring.io_base,
 h->qs[i]->tx_ring.buf_size,
 h->qs[i]->tx_ring.desc_num,
 h->qs[i]->tx_ring.max_desc_num_per_pkt,
 h->qs[i]->tx_ring.max_raw_data_sz_per_desc,
 h->qs[i]->tx_ring.max_pkt_size,
-h->qs[i]->tx_ring.stats.sw_err_cnt,
-h->qs[i]->tx_ring.stats.io_err_cnt);
-   s += sprintf(buf + s,
-   "\t\trx_ring on %p:%u,%u,%llu,%llu,%llu\n",
-   h->qs[i]->rx_ring.io_base,
-   h->qs[i]->rx_ring.buf_size,
-   h->qs[i]->rx_ring.desc_num,
-   h->qs[i]->rx_ring.stats.sw_err_cnt,
-   h->qs[i]->rx_ring.stats.io_err_cnt,
-   h->qs[i]->rx_ring.stats.seg_pkt_cnt);
+h->qs[i]->tx_ring.stats.sw_err_cnt,
+h->qs[i]->tx_ring.stats.io_err_cnt);
+   s += sprintf(s,
+"\t\trx_ring on %p:%u,%u,%llu,%llu,%llu\n",
+h->qs[i]->rx_ring.io_base,
+h->qs[i]->rx_ring.buf_size,
+h->qs[i]->rx_ring.desc_num,
+h->qs[i]->rx_ring.stats.sw_err_cnt,
+h->qs[i]->rx_ring.stats.io_err_cnt,
+h->qs[i]->rx_ring.stats.seg_pkt_cnt);
}
}
 
-   return s;
+   return s - buf;
 }
 
 static DEVICE_ATTR_RO(handles);


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro

[Joe Perches]

Hello Huang.

On Mon, 2015-10-12 at 11:23 +0800, huangdaode wrote:
> This patch changes the code style to make the code more simple.
> also removes the once used HNADEL_TX_MSG macro, according to the

HANDEL_TX_MSG typo

> review comments from Joe Perches.
> 
> Signed-off-by: huangdaode <huangda...@hisilicon.com>
> Reviewed-by: Joe Perches <j...@perches.com>

I didn't review this.

Please do not add signatures for another person when
not specifically added by that person.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] bnxt_en: New Broadcom ethernet driver.

[Joe Perches]

On Sat, 2015-10-10 at 07:39 -0400, Michael Chan wrote:
> Broadcom ethernet driver for the new family of NetXtreme-C/E
> ethernet devices.

Thanks.

In addition to the kbuild test robot suggestions,
you might try running scripts/checkpatch.pl on this
and fix up some style trivia.

Maybe try "scripts/checkpatch.pl --fix-inplace" for
the mechanical stuff.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mISDN: use kstrdup() in dsp_pipeline_build

[Joe Perches]

On Sat, 2015-10-10 at 02:32 -0700, Geliang Tang wrote:
> Use kstrdup instead of strlen-kmalloc-strcpy.

Not the same code.

Instead of returning early, a 0 length string will
now set pipeline->inuse to 0.

Maybe that's OK, but you should state why in the
commit log.

> diff --git a/drivers/isdn/mISDN/dsp_pipeline.c 
> b/drivers/isdn/mISDN/dsp_pipeline.c
[]
> @@ -250,14 +250,9 @@ int dsp_pipeline_build(struct dsp_pipeline *pipeline, 
> const char *cfg)
>   if (!cfg)
>   return 0;
>  
> - len = strlen(cfg);
> - if (!len)
> - return 0;
> -
> - dup = kmalloc(len + 1, GFP_ATOMIC);
> + dup = kstrdup(cfg, GFP_ATOMIC);
>   if (!dup)
>   return 0;
> - strcpy(dup, cfg);
>   while ((tok = strsep(, "|"))) {
>   if (!strlen(tok))
>   continue;



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mlxsw: fix warnings for big-endian 32-bit dma_addr_t

[Joe Perches]

On Tue, 2015-10-06 at 23:47 +0200, Arnd Bergmann wrote:
> The recently added mlxsw driver produces warnings in ARM
> allmodconfig:
> 
> drivers/net/ethernet/mellanox/mlxsw/pci.c: In function 'mlxsw_pci_cmd_exec':
> drivers/net/ethernet/mellanox/mlxsw/pci.c:1585:59: warning: right shift count 
> >= width of type [-Wshift-count-overflow]
> linux/byteorder/big_endian.h:38:51: note: in definition of macro 
> '__cpu_to_be32'
> drivers/net/ethernet/mellanox/mlxsw/pci.c:76:2: note: in expansion of macro 
> 'iowrite32be'
> 
> This changes the type of the local variable to u64, which gets rid of the
> warning and seems nicer than adding #ifdefs.

Using upper_32_bits instead of the shift might be
nicer than changing the type.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] stmmac: replace all pr_xxx by their netdev_xxx counterpart

[Joe Perches]

On Thu, 2015-09-10 at 14:37 +0200, LABBE Corentin wrote:
> So this patch replace all pr_xxx by their dev_xxx counterpart.

Minor mismatch between subject and commit message.
Maybe not worth resending.

Thanks

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] stmmac: replace if (netif_msg_type) by their netif_xxx counterpart

[Joe Perches]

On Thu, 2015-09-10 at 14:37 +0200, LABBE Corentin wrote:
> replace all
> if (netif_msg_type(priv)) dev_xxx(priv->devices, ...)
> by the simplier macro netif_xxx(priv, hw, priv->dev, ...)

Thanks.  Trivia:

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c 
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[]
> @@ -733,10 +735,9 @@ static void stmmac_adjust_link(struct net_device *dev)
>   stmmac_hw_fix_mac_speed(priv);
>   break;
>   default:
> - if (netif_msg_link(priv))
> - netdev_warn(priv->dev,
> - "%s: Speed (%d) not 
> 10/100\n",
> - dev->name, phydev->speed);
> + netif_warn(priv, link, priv->dev,
> +"%s: Speed (%d) not 10/100\n",
> +dev->name, phydev->speed);

Maybe add another patch removing dev->name where used
in logging as it's likely redundant.

> @@ -1038,18 +1039,15 @@ static int init_dma_desc_rings(struct net_device 
> *dev, gfp_t flags)
>  
>   priv->dma_buf_sz = bfsize;
>  
> - if (netif_msg_probe(priv))
> - netdev_dbg(priv->dev, "%s: txsize %d, rxsize %d, bfsize %d\n",
> -__func__, txsize, rxsize, bfsize);
> + netif_dbg(priv, probe, priv->dev, "%s: txsize %d, rxsize %d, bfsize 
> %d\n",
> +   __func__, txsize, rxsize, bfsize);

And another removing __func__ from the _dbg uses as
it's relatively low value and dynamic debug can add it.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] net: mv643xx_eth: use kzalloc

[Joe Perches]

On Wed, 2015-09-09 at 10:38 +0200, Rasmus Villemoes wrote:
> The double memset is a little ugly; using kzalloc avoids it altogether.
[]
> diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c 
> b/drivers/net/ethernet/marvell/mv643xx_eth.c
[]
> @@ -1859,14 +1859,11 @@ oom:
>   return;
>   }
>  
> - mc_spec = kmalloc(0x200, GFP_ATOMIC);
> + mc_spec = kzalloc(0x200, GFP_ATOMIC);
>   if (mc_spec == NULL)
>   goto oom;
>   mc_other = mc_spec + (0x100 >> 2);

This sure looks wrong as it sets a pointer
to unallocated memory.

> - memset(mc_spec, 0, 0x100);
> - memset(mc_other, 0, 0x100);

So this does a memset of random memory.

for (i = 0; i < 0x100; i += 4) {
wrl(mp, SPECIAL_MCAST_TABLE(mp->port_num) + i, mc_spec[i >> 2]);
wrl(mp, OTHER_MCAST_TABLE(mp->port_num) + i, mc_other[i >> 2]);
}


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] stmmac: replace all pr_xxx by their dev_xxx counterpart

[Joe Perches]

On Wed, 2015-09-09 at 15:14 +0200, LABBE Corentin wrote:
> The stmmac driver use lots of pr_xxx functions to print information.
> This is bad since we cannot know which device logs the information.
> (moreover if two stmmac device are present)
[]
> So this patch replace all pr_xxx by their dev_xxx counterpart.

Using
netdev_(priv->dev, ...
instead of
dev_(priv->device,

would be more consistent with other ethernet devices.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c 
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[]
> @@ -298,7 +298,7 @@ bool stmmac_eee_init(struct stmmac_priv *priv)
>*/
>   spin_lock_irqsave(>lock, flags);
>   if (priv->eee_active) {
> - pr_debug("stmmac: disable EEE\n");
> + dev_dbg(priv->device, "disable EEE\n");

netdev_dbg(priv->dev, ...)

> @@ -657,10 +657,10 @@ static int stmmac_init_ptp(struct stmmac_priv *priv)
>   priv->adv_ts = 1;
>  
>   if (netif_msg_hw(priv) && priv->dma_cap.time_stamp)
> - pr_debug("IEEE 1588-2002 Time Stamp supported\n");
> + dev_dbg(priv->device, "IEEE 1588-2002 Time Stamp supported\n");

And these netif_msg_ could be

if (priv->dma_cap.timestamp)
netif_dbg(priv, hw, priv->dev, ...);


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] net: mv643xx_eth: use kzalloc

[Joe Perches]

On Wed, 2015-09-09 at 11:45 -0700, Joe Perches wrote:
> On Wed, 2015-09-09 at 20:34 +0200, Rasmus Villemoes wrote:
> > mc_spec and mc_other are u32*, we allocate 0x200 = 512 bytes = 128
> > u32s, and pointer arithmetic makes mc_other point to the latter 64. Then
> > the memory is cleared 256 bytes at a time.
> > 
> > It's unusual and slightly obfuscated code, but I don't think it's
> > wrong. 

Perhaps this would make the code a bit clearer:

Use kcalloc, decimal sizes, decimal indexing,
and a promiscuous exit block using the same
style as the non-promiscuous multicast block.
---
  drivers/net/ethernet/marvell/mv643xx_eth.c | 46 ++
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c 
b/drivers/net/ethernet/marvell/mv643xx_eth.c
index d52639b..9230ed5 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -1845,32 +1845,19 @@ static void mv643xx_eth_program_multicast_filter(struct 
net_device *dev)
struct netdev_hw_addr *ha;
int i;
 
-   if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI)) {
-   int port_num;
-   u32 accept;
+   if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI))
+   goto promiscuous;
 
-oom:
-   port_num = mp->port_num;
-   accept = 0x01010101;
-   for (i = 0; i < 0x100; i += 4) {
-   wrl(mp, SPECIAL_MCAST_TABLE(port_num) + i, accept);
-   wrl(mp, OTHER_MCAST_TABLE(port_num) + i, accept);
-   }
-   return;
-   }
-
-   mc_spec = kmalloc(0x200, GFP_ATOMIC);
-   if (mc_spec == NULL)
-   goto oom;
-   mc_other = mc_spec + (0x100 >> 2);
-
-   memset(mc_spec, 0, 0x100);
-   memset(mc_other, 0, 0x100);
+   /* Allocate both mc_spec and mc_other tables */
+   mc_spec = kcalloc(128, sizeof(u32), GFP_ATOMIC);
+   if (!mc_spec)
+   goto promiscuous;
+   mc_other = _spec[64];
 
netdev_for_each_mc_addr(ha, dev) {
u8 *a = ha->addr;
u32 *table;
-   int entry;
+   u8 entry;
 
if (memcmp(a, "\x01\x00\x5e\x00\x00", 5) == 0) {
table = mc_spec;
@@ -1883,12 +1870,23 @@ oom:
table[entry >> 2] |= 1 << (8 * (entry & 3));
}
 
-   for (i = 0; i < 0x100; i += 4) {
-   wrl(mp, SPECIAL_MCAST_TABLE(mp->port_num) + i, mc_spec[i >> 2]);
-   wrl(mp, OTHER_MCAST_TABLE(mp->port_num) + i, mc_other[i >> 2]);
+   for (i = 0; i < 64; i++) {
+   wrl(mp, SPECIAL_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+   mc_spec[i]);
+   wrl(mp, OTHER_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+   mc_other[i]);
}
 
kfree(mc_spec);
+   return;
+
+promiscuous:
+   for (i = 0; i < 64; i++) {
+   wrl(mp, SPECIAL_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+   0x01010101u);
+   wrl(mp, OTHER_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+   0x01010101u);
+   }
 }
 
 static void mv643xx_eth_set_rx_mode(struct net_device *dev)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mv643xx_eth: Neaten mv643xx_eth_program_multicast_filter

[Joe Perches]

The code around the allocation and loops are a bit obfuscated.

Neaten it by using:

o kcalloc with decimal count and sizeof(u32)
o Decimal loop indexing and i++ not i += 4
o A promiscuous block using a similar style
  to the multicast block
o Remove unnecessary variables

Signed-off-by: Joe Perches <j...@perches.com>
---
Here's a neatening patch respun against Rasmus' patch
Might be useful.

 drivers/net/ethernet/marvell/mv643xx_eth.c | 43 +++---
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c 
b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 960169e..c78ae18 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -1845,29 +1845,19 @@ static void mv643xx_eth_program_multicast_filter(struct 
net_device *dev)
struct netdev_hw_addr *ha;
int i;
 
-   if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI)) {
-   int port_num;
-   u32 accept;
+   if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI))
+   goto promiscuous;
 
-oom:
-   port_num = mp->port_num;
-   accept = 0x01010101;
-   for (i = 0; i < 0x100; i += 4) {
-   wrl(mp, SPECIAL_MCAST_TABLE(port_num) + i, accept);
-   wrl(mp, OTHER_MCAST_TABLE(port_num) + i, accept);
-   }
-   return;
-   }
-
-   mc_spec = kzalloc(0x200, GFP_ATOMIC);
-   if (mc_spec == NULL)
-   goto oom;
-   mc_other = mc_spec + (0x100 >> 2);
+   /* Allocate both mc_spec and mc_other tables */
+   mc_spec = kcalloc(128, sizeof(u32), GFP_ATOMIC);
+   if (!mc_spec)
+   goto promiscuous;
+   mc_other = _spec[64];
 
netdev_for_each_mc_addr(ha, dev) {
u8 *a = ha->addr;
u32 *table;
-   int entry;
+   u8 entry;
 
if (memcmp(a, "\x01\x00\x5e\x00\x00", 5) == 0) {
table = mc_spec;
@@ -1880,12 +1870,23 @@ oom:
table[entry >> 2] |= 1 << (8 * (entry & 3));
}
 
-   for (i = 0; i < 0x100; i += 4) {
-   wrl(mp, SPECIAL_MCAST_TABLE(mp->port_num) + i, mc_spec[i >> 2]);
-   wrl(mp, OTHER_MCAST_TABLE(mp->port_num) + i, mc_other[i >> 2]);
+   for (i = 0; i < 64; i++) {
+   wrl(mp, SPECIAL_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+   mc_spec[i]);
+   wrl(mp, OTHER_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+   mc_other[i]);
}
 
kfree(mc_spec);
+   return;
+
+promiscuous:
+   for (i = 0; i < 64; i++) {
+   wrl(mp, SPECIAL_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+   0x01010101u);
+   wrl(mp, OTHER_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+   0x01010101u);
+   }
 }
 
 static void mv643xx_eth_set_rx_mode(struct net_device *dev)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] net: mv643xx_eth: use kzalloc

[Joe Perches]

On Wed, 2015-09-09 at 20:34 +0200, Rasmus Villemoes wrote:
> mc_spec and mc_other are u32*, we allocate 0x200 = 512 bytes = 128
> u32s, and pointer arithmetic makes mc_other point to the latter 64. Then
> the memory is cleared 256 bytes at a time.
> 
> It's unusual and slightly obfuscated code, but I don't think it's
> wrong. 

Right, misread.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net/core/sysctl_net_core.c unused variable

[Joe Perches]

On Sun, 2015-09-06 at 16:36 +0100, Nick Warne wrote:
> On 06/09/15 16:28, Joe Perches wrote:
> > On Sun, 2015-09-06 at 16:16 +0100, Nick Warne wrote:
> >> On 06/09/15 15:52, Joe Perches wrote:
> >> > On Sun, 2015-09-06 at 15:13 +0100, Nick Warne wrote:
> >> >> gcc version 4.8.2 (GCC) warns that 'static int one = 1;' is declared but
> >> >> not used in file net/core/sysctl_net_core.c.
> >> >
> >> > Only when CONFIG_NET isn't set.
> >>
> >> CONFIG_NET=y
> >>
> >> Peculiar indeed.
> >>
> >> >> Reading the file, that is
> >> >> the case.  Attached is a patch to remove it.
> >> >
> >> > $ git grep -w -n one net/core/sysctl_net_core.c
> >> > net/core/sysctl_net_core.c:26:static int one = 1;
> >> > net/core/sysctl_net_core.c:332: .extra2 = 
> >> >
> >> >> Signed-off-by:  Nick Warne <n...@linicks.net>
> >> >
> >> > Please use grep to augment reading.
> >>
> >> grep -w -n one net/core/sysctl_net_core.c
> >> 26:static int one = 1;
> >>
> >> ?
> >>
> >> I just don't have the 
> >>
> >> I am confused now.
> >
> > What source tree are you using?
> 
> Latest longterm 3.18.21

OK, it's important to mention that otherwise the
assumption would be a git tree like net or net-next.

> > What changes in what branch exist?
> 
> I am not using git (if that is what you mean by 'branches') - just 
> tarballs from kernel.org

(OK, using git and linux-stable)

$ git grep -w -n one v3.18.21 -- net/core/sysctl_net_core.c
v3.18.21:net/core/sysctl_net_core.c:26:static int one = 1;

And the responsible commit:

commit c48cf4f27d4555a455c3fef71137bd0fc44d1656
("net: sysctl_net_core: check SNDBUF and RCVBUF for min length")



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT] Networking

[Joe Perches]

On Thu, 2015-09-03 at 11:22 -0700, Linus Torvalds wrote:
> On Thu, Sep 3, 2015 at 10:40 AM, David Miller  wrote:
> >
> > Linus, what GCC version are you using and what does the warning look
> > like?
> 
> I'm on whatever is in F22. gcc -v says
> 
>gcc version 5.1.1 20150618 (Red Hat 5.1.1-4) (GCC)
> 
> and the warning looks like so:
> 
>   net/mac80211/rate.c: In function ‘rate_control_cap_mask’:
>   net/mac80211/rate.c:719:25: warning: ‘sizeof’ on array function
> parameter ‘mcs_mask’ will return size of ‘u8 * {aka unsigned char *}’
> [-Wsizeof-array-argument]
>  for (i = 0; i < sizeof(mcs_mask); i++)
>^
> 
> (note the lack of warning about the use of an array in the function
> definition parameter list - I tried to find if there's any way to
> enable such a warning, but couldn't find anything. Maybe my google-fu
> is weak, but more probably that just doesn't exist).

Coccinelle might be a better tool for this but
a possible checkpatch patch is below:

It produces output like:

$ ./scripts/checkpatch.pl -f net/iucv/iucv.c --types=sized_array_argument
WARNING: Avoid sized array arguments
#716: FILE: net/iucv/iucv.c:716:
+static int iucv_sever_pathid(u16 pathid, u8 userdata[16])
+{

WARNING: Avoid sized array arguments
#878: FILE: net/iucv/iucv.c:878:
+int iucv_path_accept(struct iucv_path *path, struct iucv_handler *handler,
+u8 userdata[16], void *private)
+{

WARNING: Avoid sized array arguments
#925: FILE: net/iucv/iucv.c:925:
+int iucv_path_connect(struct iucv_path *path, struct iucv_handler *handler,
+ u8 userid[8], u8 system[8], u8 userdata[16],
+ void *private)
+{

WARNING: Avoid sized array arguments
#988: FILE: net/iucv/iucv.c:988:
+int iucv_path_quiesce(struct iucv_path *path, u8 userdata[16])
+{

WARNING: Avoid sized array arguments
#1020: FILE: net/iucv/iucv.c:1020:
+int iucv_path_resume(struct iucv_path *path, u8 userdata[16])
+{

WARNING: Avoid sized array arguments
#1050: FILE: net/iucv/iucv.c:1050:
+int iucv_path_sever(struct iucv_path *path, u8 userdata[16])
+{

total: 0 errors, 6 warnings, 0 checks, 2119 lines checked
---
 scripts/checkpatch.pl | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e14dcdb..747b164 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5422,6 +5422,24 @@ sub process {
 "externs should be avoided in .c files\n" .  
$herecurr);
}
 
+# check for function arguments using arg[SIZE]
+   if ($^V && $^V ge 5.10.0 &&
+   defined $stat &&
+   $stat =~ 
/^.\s*(?:$Declare|$DeclareMisordered)\s*$Ident\s*($balanced_parens)\s*\{/s) {
+   my $func_args = $1;
+   if ($func_args =~ 
/(.*)\[\s*(?:$Constant|[A-Z0-9_]+)\s*\]/ && (!defined($1) || $1 !~ 
/\[\s*\]\s*$/)) {
+   my $ctx = '';
+   my $herectx = $here . "\n";
+   my $cnt = statement_rawlines($stat);
+   for (my $n = 0; $n < $cnt; $n++) {
+   $herectx .= raw_line($linenr, $n) . 
"\n";
+   $n = $cnt if ($herectx =~ /{/);
+   }
+   WARN("SIZED_ARRAY_ARGUMENT",
+"Avoid sized array arguments\n" . 
$herectx);
+   }
+   }
+
 # checks for new __setup's
if ($rawline =~ /\b__setup\("([^"]*)"/) {
my $name = $1;


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next] flow_dissector: Fix function argument ordering dependency

[Joe Perches]

On Tue, 2015-09-01 at 18:11 -0700, Tom Herbert wrote:
> Commit c6cc1ca7f4d70c ("flowi: Abstract out functions to get flow hash
> based on flowi") introduced a bug in __skb_set_sw_hash where we
> require a dependency on evaluating arguments in a function in order.
> There is no such ordering enforced in C, so this incorrect. This
> patch fixes that by splitting out the arguments. This bug was
> found via a compiler warning that keys may be uninitialized.

To help the reader know a bit more about how these
functions operate, perhaps it'd also be useful/better
to mark arguments as const where appropriate.

I didn't look to see if any other functions could
have the flow related args as const.

The __get_hash_from_flowi6 and flow_keys_have_l4
functions could be:
---
 include/net/flow.h   | 2 +-
 include/net/flow_dissector.h | 2 +-
 net/core/flow_dissector.c| 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/net/flow.h b/include/net/flow.h
index dafe97c..96fef26 100644
--- a/include/net/flow.h
+++ b/include/net/flow.h
@@ -244,7 +244,7 @@ void flow_cache_flush(struct net *net);
 void flow_cache_flush_deferred(struct net *net);
 extern atomic_t flow_cache_genid;
 
-__u32 __get_hash_from_flowi6(struct flowi6 *fl6, struct flow_keys *keys);
+__u32 __get_hash_from_flowi6(const struct flowi6 *fl6, struct flow_keys *keys);
 
 static inline __u32 get_hash_from_flowi6(struct flowi6 *fl6)
 {
diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 8c8548c..9fdcc3f 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -177,7 +177,7 @@ struct flow_keys_digest {
 void make_flow_keys_digest(struct flow_keys_digest *digest,
   const struct flow_keys *flow);
 
-static inline bool flow_keys_have_l4(struct flow_keys *keys)
+static inline bool flow_keys_have_l4(const struct flow_keys *keys)
 {
return (keys->ports.ports || keys->tags.flow_label);
 }
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 345a040..ccece96 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -787,7 +787,7 @@ u32 skb_get_poff(const struct sk_buff *skb)
return __skb_get_poff(skb, skb->data, , skb_headlen(skb));
 }
 
-__u32 __get_hash_from_flowi6(struct flowi6 *fl6, struct flow_keys *keys)
+__u32 __get_hash_from_flowi6(const struct flowi6 *fl6, struct flow_keys *keys)
 {
memset(keys, 0, sizeof(*keys));
 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] flow_dissector: Use 'const' where possible.

[Joe Perches]

On Tue, 2015-09-01 at 21:19 -0700, David Miller wrote:
> Signed-off-by: David S. Miller 
[]
>  net/core/flow_dissector.c | 79 
> ---

Thanks David.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mwifiex: Make mwifiex_dbg a function, reduce object size

[Joe Perches]

On Tue, 2015-09-01 at 15:09 +, David Laight wrote:
> From: Joe Perches
> > Sent: 31 August 2015 18:47
> > 
> > The mwifiex_dbg macro has two tests that could be consolidated
> > into a function reducing overall object size ~10KB (~4%).
> > 
> > So convert the macro into a function.
> 
> This looks like it will slow things down somewhat.

I doubt in any meaningful way.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mwifiex: Make mwifiex_dbg a function, reduce object size

[Joe Perches]

The mwifiex_dbg macro has two tests that could be consolidated
into a function reducing overall object size ~10KB (~4%).

So convert the macro into a function.

$ size drivers/net/wireless/mwifiex/built-in.o* (x86-64 defconfig)
   textdata bss dec hex filename
 23310286284809  246539   3c30b 
drivers/net/wireless/mwifiex/built-in.o.new
 24394986284809  257386   3ed6a 
drivers/net/wireless/mwifiex/built-in.o.old

Signed-off-by: Joe Perches <j...@perches.com>
---
 drivers/net/wireless/mwifiex/main.c | 20 
 drivers/net/wireless/mwifiex/main.h | 17 -
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/main.c 
b/drivers/net/wireless/mwifiex/main.c
index 278dc94..4fa8ca3 100644
--- a/drivers/net/wireless/mwifiex/main.c
+++ b/drivers/net/wireless/mwifiex/main.c
@@ -1447,6 +1447,26 @@ exit_sem_err:
 }
 EXPORT_SYMBOL_GPL(mwifiex_remove_card);
 
+void _mwifiex_dbg(const struct mwifiex_adapter *adapter, int mask,
+ const char *fmt, ...)
+{
+   struct va_format vaf;
+   va_list args;
+
+   if (!adapter->dev || !(adapter->debug_mask & mask))
+   return;
+
+   va_start(args, fmt);
+
+   vaf.fmt = fmt;
+   vaf.va = 
+
+   dev_info(adapter->dev, "%pV", );
+
+   va_end(args);
+}
+EXPORT_SYMBOL_GPL(_mwifiex_dbg);
+
 /*
  * This function initializes the module.
  *
diff --git a/drivers/net/wireless/mwifiex/main.h 
b/drivers/net/wireless/mwifiex/main.h
index 6b95121..96663214 100644
--- a/drivers/net/wireless/mwifiex/main.h
+++ b/drivers/net/wireless/mwifiex/main.h
@@ -48,6 +48,9 @@
 
 extern const char driver_version[];
 
+struct mwifiex_adapter;
+struct mwifiex_private;
+
 enum {
MWIFIEX_ASYNC_CMD,
MWIFIEX_SYNC_CMD
@@ -180,12 +183,11 @@ enum MWIFIEX_DEBUG_LEVEL {
MWIFIEX_DBG_FATAL | \
MWIFIEX_DBG_ERROR)
 
-#define mwifiex_dbg(adapter, dbg_mask, fmt, args...)   \
-do {   \
-   if ((adapter)->debug_mask & MWIFIEX_DBG_##dbg_mask) \
-   if ((adapter)->dev) \
-   dev_info((adapter)->dev, fmt, ## args); \
-} while (0)
+__printf(3, 4)
+void _mwifiex_dbg(const struct mwifiex_adapter *adapter, int mask,
+ const char *fmt, ...);
+#define mwifiex_dbg(adapter, mask, fmt, ...)   \
+   _mwifiex_dbg(adapter, MWIFIEX_DBG_##mask, fmt, ##__VA_ARGS__)
 
 #define DEBUG_DUMP_DATA_MAX_LEN128
 #define mwifiex_dbg_dump(adapter, dbg_mask, str, buf, len) \
@@ -506,9 +508,6 @@ enum mwifiex_iface_work_flags {
MWIFIEX_IFACE_WORK_CARD_RESET,
 };
 
-struct mwifiex_adapter;
-struct mwifiex_private;
-
 struct mwifiex_private {
struct mwifiex_adapter *adapter;
u8 bss_type;


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC V3 2/2] net: Optimize snmp stat aggregation by walking all the percpu data at once

[Joe Perches]

On Sat, 2015-08-29 at 07:32 -0700, Eric Dumazet wrote:
 On Sat, 2015-08-29 at 14:37 +0530, Raghavendra K T wrote:
 
   
   static inline void __snmp6_fill_stats64(u64 *stats, void __percpu *mib,
  - int items, int bytes, size_t syncpoff)
  +   int items, int bytes, size_t syncpoff)
   {
  -   int i;
  +   int i, c;
  int pad = bytes - sizeof(u64) * items;
  +   u64 buff[items];
  +
 
 One last comment : using variable length arrays is confusing for the
 reader, and sparse as well.
 
 $ make C=2 net/ipv6/addrconf.o
 ...
   CHECK   net/ipv6/addrconf.c
 net/ipv6/addrconf.c:4733:18: warning: Variable length array is used.
 net/ipv6/addrconf.c:4737:25: error: cannot size expression
 
 
 I suggest you remove 'items' parameter and replace it by
 IPSTATS_MIB_MAX, as __snmp6_fill_stats64() is called exactly once.

If you respin, I suggest:

o remove items from the __snmp6_fill_stats64 arguments
  and use IPSTATS_MIB_MAX in the function instead

o add braces around the for_each_possible_cpu loop

for_each_possible_cpu(c) {
for (i = 1; i  items; i++)
buff[i] += snmp_get_cpu_field64(mib, c, i, syncpoff);
}


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC V2 2/2] net: Optimize snmp stat aggregation by walking all the percpu data at once

[Joe Perches]

On Fri, 2015-08-28 at 15:29 -0700, Eric Dumazet wrote:
 On Fri, 2015-08-28 at 14:26 -0700, Joe Perches wrote:
 1) u64 array[XX] on stack is naturally aligned,

Of course it is.

 kzalloc() wont improve this at all. Not sure what you believe.

An alloc would only reduce stack use.

Copying into the buffer, then copying the buffer into the
skb may be desirable on some arches though.

 2) put_unaligned() is basically a normal memory write on x86.
  memcpy(dst,src,...) will have a problem anyway on arches that care,
 because src  dst wont have same alignment.

OK, so all the world's an x86?

On arm32, copying 288 bytes using nearly all aligned word
transfers is generally faster than using only unsigned
short transfers.

 288 bytes on stack in a leaf function in this path is totally fine, it
 is not like we're calling ext4/xfs/nfs code after this point.

Generally true.  It's always difficult to know how much
stack has been consumed though and smaller stack frames
are generally better.

Anyway, the block copy from either the alloc'd or stack
buffer amounts only to a slight performance improvement
for arm32.  It doesn't really have much other utility.


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC V2 2/2] net: Optimize snmp stat aggregation by walking all the percpu data at once

[Joe Perches]

On Fri, 2015-08-28 at 13:33 -0700, Eric Dumazet wrote:
 We do not bother for small struct.
 
 Here, the array is big enough that David prefers having an explicit
 memset() so that it clearly shows that author of this code was aware of
 this.

It's 288 bytes on stack, maybe a kzalloc would be clearer too.


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html