Re: [PATCH] rtnetlink: Call nlmsg_parse() with correct header length

[Rustad, Mark D]

On Apr 8, 2013, at 8:45 AM, Michael Riesch michael.rie...@omicron.at wrote:

 
 Signed-off-by: Michael Riesch michael.rie...@omicron.at
 Cc: David S. Miller da...@davemloft.net
 Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
 Cc: Jiri Benc jb...@redhat.com
 Cc: Theodore Ts'o ty...@mit.edu
 Cc: linux-kernel@vger.kernel.org
 ---
 Habidere,
 
 I encountered a netlink kernel warning when running avahi 0.6.31 on my system
 with kernel v3.4.35 (it appears several times):
 
   netlink: 12 bytes leftover after parsing attributes.
 
 Searching the web showed that commit 
 115c9b81928360d769a76c632bae62d15206a94a 
 rtnetlink: Fix problem with buffer allocation introduced this behaviour[1].
 
 Now I - knowing nothing about netlink whatsoever - assume that the nlmsg_parse
 function is called with the wrong header length. In user space the request
 message consists out of the message header (struct nlmsghdr, 16 bytes) and an
 ifinfomsg (struct ifinfomsg, 16 bytes). After that, request attributes could
 follow. nlmsg_parse checks for this attributes after a given header length. In
 rtnl_get_link() this header length is sizeof(struct ifinfomsg), but in
 rtnl_calcit() as well as in rntl_dump_ifinfo() the header length is
 sizeof(struct rtgenmsg), which is 1 byte.
 
 With this patch I got rid of these warnings. However, I do not know whether
 this is the correct solution, so I am looking forward to your comments.
 Regards, Michael
 
 [1] http://lists.infradead.org/pipermail/libnl/2012-April/000515.html
 
 net/core/rtnetlink.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
 index 900fc61..ebf6ace 100644
 --- a/net/core/rtnetlink.c
 +++ b/net/core/rtnetlink.c
 @@ -1065,7 +1065,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct 
 netlink_callback *cb)
   rcu_read_lock();
   cb-seq = net-dev_base_seq;
 
 - if (nlmsg_parse(cb-nlh, sizeof(struct rtgenmsg), tb, IFLA_MAX,
 + if (nlmsg_parse(cb-nlh, sizeof(struct ifinfomsg), tb, IFLA_MAX,
   ifla_policy) = 0) {
 
   if (tb[IFLA_EXT_MASK])
 @@ -1909,7 +1909,7 @@ static u16 rtnl_calcit(struct sk_buff *skb, struct 
 nlmsghdr *nlh)
   u32 ext_filter_mask = 0;
   u16 min_ifinfo_dump_size = 0;
 
 - if (nlmsg_parse(nlh, sizeof(struct rtgenmsg), tb, IFLA_MAX,
 + if (nlmsg_parse(nlh, sizeof(struct ifinfomsg), tb, IFLA_MAX,
   ifla_policy) = 0) {
   if (tb[IFLA_EXT_MASK])
   ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]);
 -- 
 1.7.9.5


I found that fcoemon has also been triggering these messages for some time. I 
found the same problem and arrived at exactly the same solution. I would have 
already sent it, but it is still in validation. As far as I am concerned:

Acked-by: Mark Rustad mark.d.rus...@intel.com

-- 
Mark Rustad, Networking Division, Intel Corporation--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] tcm_fc: rcu_deref outside rcu lock/unlock section

[Rustad, Mark D]

On Aug 18, 2012, at 3:35 PM, Nicholas A. Bellinger wrote:

 On Sat, 2012-08-18 at 16:10 +0400, Denis Efremov wrote:
 Use rcu_dereference_protected in order to prevent lockdep
 complaint. Sequel of the patch 863555be
 
 Found by Linux Driver Verification project (linuxtesting.org).
 
 Signed-off-by: Denis Efremov yefremov.de...@gmail.com
 ---
 drivers/target/tcm_fc/tfc_sess.c |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
 
 diff --git a/drivers/target/tcm_fc/tfc_sess.c 
 b/drivers/target/tcm_fc/tfc_sess.c
 index 87901fa..3c9e5b5 100644
 --- a/drivers/target/tcm_fc/tfc_sess.c
 +++ b/drivers/target/tcm_fc/tfc_sess.c
 @@ -456,7 +456,9 @@ static void ft_prlo(struct fc_rport_priv *rdata)
  struct ft_tport *tport;
 
  mutex_lock(ft_lport_lock);
 -tport = rcu_dereference(rdata-local_port-prov[FC_TYPE_FCP]);
 +tport = rcu_dereference_protected(rdata-local_port-prov[FC_TYPE_FCP],
 +  lockdep_is_held(ft_lport_lock));
 +
  if (!tport) {
  mutex_unlock(ft_lport_lock);
  return;
 
 This looks OK to silence lockdep.  CC'ing MDR + Kiran for good measure
 here, and will move from target-pending queue - master with their ACK.
 
 Thanks Dennis!
 
 --nab


Ack. In fact I wonder why I didn't encounter it myself.

-- 
Mark Rustad, LAN Access Division, Intel Corporation

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/8] ixgbe: Out of line ixgbe_read/write_reg

[Rustad, Mark D]

On May 16, 2014, at 2:43 PM, Andi Kleen a...@firstfloor.org wrote:

 From: Andi Kleen a...@linux.intel.com
 
 ixgbe_read_reg and ixgbe_write_reg are frequently called and are very big
 because they have complex error handling code.

Actually, this patch doesn't do anything to ixgbe_write_reg, which would almost 
certainly be very bad for performance, but instead changes ixgbe_write_reg64. 
The latter is not in a performance-sensitive path, but is only called from one 
site, so there is little reason to take it out-of-line.

I already have a patch in queue to make ixgbe_read_reg out-of-line, because it 
does have a very costly memory footprint inline, as you have found.

 Moving them out of line saves ~27k text in the ixgbe driver.
 
   text   data bss dec hex filename
 14220873  2008072 1507328 1773627310ea251 vmlinux-before-ixgbe
 14193673  2003976 1507328 1770497710e2811 vmlinux-ixgbe
 
 Cc: net...@vger.kernel.org
 Cc: Jeff Kirsher jeffrey.t.kirs...@intel.com
 Signed-off-by: Andi Kleen a...@linux.intel.com
 ---
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.h | 22 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c   | 22 ++
 2 files changed, 24 insertions(+), 20 deletions(-)
 
 diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h 
 b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
 index f12c40f..05f094d 100644
 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
 +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
 @@ -162,28 +162,10 @@ static inline void writeq(u64 val, void __iomem *addr)
 }
 #endif
 
 -static inline void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value)
 -{
 - u8 __iomem *reg_addr = ACCESS_ONCE(hw-hw_addr);
 +void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value);
 +u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg);
 
 - if (ixgbe_removed(reg_addr))
 - return;
 - writeq(value, reg_addr + reg);
 -}
 #define IXGBE_WRITE_REG64(a, reg, value) ixgbe_write_reg64((a), (reg), 
 (value))
 -
 -static inline u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg)
 -{
 - u8 __iomem *reg_addr = ACCESS_ONCE(hw-hw_addr);
 - u32 value;
 -
 - if (ixgbe_removed(reg_addr))
 - return IXGBE_FAILED_READ_REG;
 - value = readl(reg_addr + reg);
 - if (unlikely(value == IXGBE_FAILED_READ_REG))
 - ixgbe_check_remove(hw, reg);
 - return value;
 -}
 #define IXGBE_READ_REG(a, reg) ixgbe_read_reg((a), (reg))
 
 #define IXGBE_WRITE_REG_ARRAY(a, reg, offset, value) \
 diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 
 b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
 index d62e7a2..5f81f62 100644
 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
 +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
 @@ -371,6 +371,28 @@ void ixgbe_write_pci_cfg_word(struct ixgbe_hw *hw, u32 
 reg, u16 value)
   pci_write_config_word(adapter-pdev, reg, value);
 }
 
 +void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value)
 +{
 + u8 __iomem *reg_addr = ACCESS_ONCE(hw-hw_addr);
 +
 + if (ixgbe_removed(reg_addr))
 + return;
 + writeq(value, reg_addr + reg);
 +}
 +
 +u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg)
 +{
 + u8 __iomem *reg_addr = ACCESS_ONCE(hw-hw_addr);
 + u32 value;
 +
 + if (ixgbe_removed(reg_addr))
 + return IXGBE_FAILED_READ_REG;
 + value = readl(reg_addr + reg);
 + if (unlikely(value == IXGBE_FAILED_READ_REG))
 + ixgbe_check_remove(hw, reg);
 + return value;
 +}
 +
 static void ixgbe_service_event_complete(struct ixgbe_adapter *adapter)
 {
   BUG_ON(!test_bit(__IXGBE_SERVICE_SCHED, adapter-state));
 -- 
 1.9.0
 
 --
 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

-- 
Mark Rustad, Networking Division, Intel Corporation

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/8] ixgbe: Out of line ixgbe_read/write_reg

[Rustad, Mark D]

On May 19, 2014, at 4:25 PM, Andi Kleen a...@linux.intel.com wrote:

 On Mon, May 19, 2014 at 10:00:52PM +, Rustad, Mark D wrote:
 On May 16, 2014, at 2:43 PM, Andi Kleen a...@firstfloor.org wrote:
 
 From: Andi Kleen a...@linux.intel.com
 
 ixgbe_read_reg and ixgbe_write_reg are frequently called and are very big
 because they have complex error handling code.
 
 Actually, this patch doesn't do anything to ixgbe_write_reg, which would 
 almost certainly be very bad for performance, but instead changes 
 ixgbe_write_reg64.
 
 I doubt a few cycles around the write make a lot of difference for MMIO. MMIO 
 is dominated
 by other things.
 
 The latter is not in a performance-sensitive path, but is only called from 
 one site, so there is little reason to take it out-of-line.
 
 True I moved the wrong one.
 
 ixgbe_write_reg3305  (0.00%)  8 409  
 
 
 I already have a patch in queue to make ixgbe_read_reg out-of-line, because 
 it does have a very costly memory footprint inline, as you have found.
 
 Please move write_reg too.

I will take a look at moving most of them out-of-line. There are just a few in 
very hot paths that should remain inline.

-- 
Mark Rustad, Networking Division, Intel Corporation

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [E1000-devel] [PATCH] net: ethernet: intel: ixgbe: ixgbe_main.c: Cleaning up missing null-terminate after strncpy call

[Rustad, Mark D]

On Jun 4, 2014, at 2:55 PM, Joe Perches j...@perches.com wrote:

 On Wed, 2014-06-04 at 23:29 +0200, Rickard Strandqvist wrote:
 Added a guaranteed null-terminate after call to strncpy.
 
 Perhaps all of these should be strlcpy

The code that is there seems fine. The length of the array exceeds the length 
of the literal, and the strncpy ensures that the entire buffer is initialized 
so no information can possibly leak from the kernel.

I think this is fine as it is without any patch.

-- 
Mark Rustad, Networking Division, Intel Corporation

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] scsi: Resolve some missing-field-initializers warnings

[Rustad, Mark D]

The warning appears in W=2 builds. I had another way to silence it by using 
diagnostic control macros, but those macros were not accepted. Using a single 
designated initialization also silences it.

Sent from my iPhone

 On Oct 17, 2014, at 8:26 AM, Christoph Hellwig h...@infradead.org wrote:
 
 On Tue, Oct 14, 2014 at 06:38:53AM -0700, Jeff Kirsher wrote:
 From: Mark Rustad mark.d.rus...@intel.com
 
 Resolve some missing-field-initializers warnings by using
 designated initialization.
 
 What tool is warning about these?  This construct is perfectly valid C.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] kernel/sysctl: Resolve missing-field-initializers warnings

[Rustad, Mark D]

On Oct 21, 2014, at 12:07 PM, Andrew Morton a...@linux-foundation.org wrote:

 On Sat, 18 Oct 2014 17:39:10 -0700 ebied...@xmission.com (Eric W. Biederman) 
 wrote:
 
 Jeff Kirsher jeffrey.t.kirs...@intel.com writes:
 
 From: Mark Rustad mark.d.rus...@intel.com
 
 Resolve missing-field-initializers warnings in W=2 builds by
 using designated initialization.
 
 ick.  No.
 
 That gcc warning makes no sense.  In this case heeding it makes the code
 significantly uglier and significantly more confusing.
 
 
 Yeah, it's not pretty.
 
 --- a/kernel/sysctl.c
 +++ b/kernel/sysctl.c
 @@ -257,7 +257,7 @@ static struct ctl_table sysctl_base_table[] = {
 .mode   = 0555,
 .child  = dev_table,
 },
 -   { }
 +   { .procname = NULL }
 };
 
 We use { } to mean all zero in 12 squillion places.  Do they all warn
 or is there something special about this site?

Well, about 6 squillion of them are { }, a GCC extension, and the other 6 
squillion are { 0 }. Both forms generate the warning. There is nothing special 
about this site. I just was resolving warnings in order to find some that had 
some significance. A flood of 125,000 warnings is too awful to look at. I got 
it down to around 1,500 and did find a few hazards and sent patches to address 
them, which have been accepted in one form or another.

I had sent patches to add diagnostic control macros to allow a warning to be 
turned off for a range of code. I would have liked to use them to provide 
something like a ZERO_ENTRY macro that would have looked something like this:

#define ZERO_ENTRY DIAG_PUSH DIAG_IGNORE(missing-field-initializers) { 0 } 
DIAG_POP

which would have provided a standard way to get a zero entry that would have 
avoided the warnings. Borislav was quite opposed to the notion of diagnostic 
control macros. I rather like the notion as long as their use is tightly 
controlled.

I'm sure that we both feel that there should be a form that the compiler does 
not generate this warning for as a preferred solution. The designated 
initialization is at best a 3rd-best solution, though naming the field used to 
identify the end of the table is not a bad thing either.

I do like enabling lots of additional warnings to find problems in code, but 
when it results in such a flood of messages it is not a very useful approach, 
hence my tendency to want to address them somehow, so that meaningful ones can 
be noticed.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: kernel: Resolve a shadow warning

[Rustad, Mark D]

On Sep 4, 2014, at 2:38 PM, Andrew Morton a...@linux-foundation.org wrote:

 From: Mark Rustad mark.d.rus...@intel.com
 
 Resolve a shadow warning in W=2 builds arising from min/max macro
 references in a parameter to a min3/max3 macro. There is no
 functional issue - the warning is benign - but simply changing
 some local variable names will eliminate it.
 
 ...
 
 --- a/include/linux/kernel.h
 +++ b/include/linux/kernel.h
 @@ -716,22 +716,22 @@ static inline void ftrace_dump(enum ftrace_dump_mode 
 oops_dump_mode) { }
  _max1  _max2 ? _max1 : _max2; })
 
 #define min3(x, y, z) ({ \
 -typeof(x) _min1 = (x);  \
 -typeof(y) _min2 = (y);  \
 -typeof(z) _min3 = (z);  \
 -(void) (_min1 == _min2);  \
 -(void) (_min1 == _min3);  \
 -_min1  _min2 ? (_min1  _min3 ? _min1 : _min3) : \
 -(_min2  _min3 ? _min2 : _min3); })
 +typeof(x) _min31 = (x); \
 +typeof(y) _min32 = (y); \
 +typeof(z) _min33 = (z); \
 +(void) (_min31 == _min32);\
 +(void) (_min31 == _min33);\
 +_min31  _min32 ? (_min31  _min33 ? _min31 : _min33) : \
 +(_min32  _min33 ? _min32 : _min33); })
 
 #define max3(x, y, z) ({ \
 -typeof(x) _max1 = (x);  \
 -typeof(y) _max2 = (y);  \
 -typeof(z) _max3 = (z);  \
 -(void) (_max1 == _max2);  \
 -(void) (_max1 == _max3);  \
 -_max1  _max2 ? (_max1  _max3 ? _max1 : _max3) : \
 -(_max2  _max3 ? _max2 : _max3); })
 +typeof(x) _max31 = (x); \
 +typeof(y) _max32 = (y); \
 +typeof(z) _max33 = (z); \
 +(void) (_max31 == _max32);\
 +(void) (_max31 == _max33);\
 +_max31  _max32 ? (_max31  _max33 ? _max31 : _max33) : \
 +(_max32  _max33 ? _max32 : _max33); })
 
 /**
  * min_not_zero - return the minimum that is _not_ zero, unless both are zero
 
 I'm still sitting on the below patch.  It's stalled because I have a
 note that David potentially found issues with it.  But on rechecking,
 that appears to be stale, or not serious enough to prevent inclusion.
 
 I can't (be bothered to) check whether this patch fixes the warnings
 because your changelog didn't tell me how to trigger the warnings (bad
 changelog).  But it might fix them!  Can you please test it?

Actually it does. W=2 builds get warnings when a min/max macro is used as a 
parameter to min3/max3. If you move to the min3/max3 that makes nested calls to 
min/max, every reference to min3/max3 will generate a warning in W=2 builds no 
matter what. It is interesting that the compiler optimizes that better - I 
hadn't looked at that.

Not wanting to argue for poorer code, lets forget about the warnings for now. 
These particular shadow warnings are pretty trivial. I'll consider revisiting 
it later. And next time I'll check the code generation.

 From: Michal Nazarewicz min...@mina86.com
 Subject: include/linux/kernel.h: rewrite min3, max3 and clamp using min and 
 max
 
 It appears that gcc is better at optimising a double call to min and max
 rather than open coded min3 and max3.  This can be observed here:
 
$ cat min-max.c
#define min(x, y) ({   \
   typeof(x) _min1 = (x);  \
   typeof(y) _min2 = (y);  \
   (void) (_min1 == _min2);  \
   _min1  _min2 ? _min1 : _min2; })
#define min3(x, y, z) ({   \
   typeof(x) _min1 = (x);  \
   typeof(y) _min2 = (y);  \
   typeof(z) _min3 = (z);  \
   (void) (_min1 == _min2);  \
   (void) (_min1 == _min3);  \
   _min1  _min2 ? (_min1  _min3 ? _min1 : _min3) : \
   (_min2  _min3 ? _min2 : _min3); })
 
int fmin3(int x, int y, int z) { return min3(x, y, z); }
int fmin2(int x, int y, int z) { return min(min(x, y), z); }
 
$ gcc -O2 -o min-max.s -S min-max.c; cat min-max.s
   .file   min-max.c
   .text
   .p2align 4,,15
   .globl  fmin3
   .type   fmin3, @function
fmin3:
.LFB0:
   .cfi_startproc
   cmpl%esi, %edi
   jl  .L5
   cmpl%esi, %edx
   movl%esi, %eax
   cmovle  %edx, %eax
   ret
   .p2align 4,,10
   .p2align 3
.L5:
   cmpl%edi, %edx
   movl%edi, %eax
   cmovle  %edx, %eax
   ret
   .cfi_endproc
.LFE0:
   .size   fmin3, .-fmin3
   .p2align 4,,15
   .globl  fmin2
   .type   fmin2, @function
fmin2:
.LFB1:
   .cfi_startproc
   cmpl%edi, %esi
   movl%edx, %eax
   cmovle  %esi, %edi
   cmpl%edx, %edi
   cmovle  %edi, %eax
   ret
   .cfi_endproc

Re: [PATCH 2/2] kernel.h: use __COUNTER__ in min and max macros to avoid -Wshadow warnings

[Rustad, Mark D]

On Sep 12, 2014, at 3:40 PM, Andrew Morton a...@linux-foundation.org wrote:

 On Thu, 11 Sep 2014 23:39:36 +0200 Michal Nazarewicz min...@mina86.com 
 wrote:
 
 Because min and max macros use the same variable names no matter
 how many times they are called (or how deep the nesting of their
 calls), each time min or max calls are nested, the same variables
 are declared.  This is especially noisy after min3 and max3 have
 been changed to nest min/max calls.
 
 Using __COUNTER__ solves the problem since each variable will get
 a unique number aadded to it.  The code will still work even if
 the compiler does not support __COUNTER__, but then the protection
 from shadow warning won't work.
 
 The same applies to min_t and max_t macros.
 
 ...
 
 --- a/include/linux/kernel.h
 +++ b/include/linux/kernel.h
 @@ -695,15 +695,27 @@ static inline void ftrace_dump(enum ftrace_dump_mode 
 oops_dump_mode) { }
 #endif /* CONFIG_TRACING */
 
 /*
 + * Preprocessor magic generating unique identifiers to avoid -Wshadow 
 warnings
 + * used by min, max, min_t and max_t macros.  cnt is __COUNTER__, op is the
 + * comparison operator; tx (ty) is type of the first (second) argument,
 + * xx (yy) is name of a temporary variable to hold the first (second) 
 argument,
 + * and x (y) is the first (second) argument.
 + */
 +#define _min_max_var(cnt, base) _mm_ ## cnt ## base
 +#define _min_max__(op, tx, xx, x, ty, yy, y) ({ \
 +tx xx = (x);\
 +ty yy = (y);\
 +(void) (xx == yy);\
 +xx op yy ? xx : yy; })
 +#define _min_max_(cnt, op, tx, x, ty, y)\
 +_min_max__(op, tx, _min_max_var(cnt, a), x, ty, _min_max_var(cnt, b), y)
 +#define _min_max(...) _min_max_(__COUNTER__, __VA_ARGS__)
 
 The fact that __COUNTER__ is used in compiler-gcc4.h but not in
 compiler-gcc3.h makes me suspicious about its availability?

AFAICT not having __COUNTER__ simply means that the shadow warnings will appear 
in W=2 builds,
so it is no worse off than before. That is, __COUNTER__ will simply expand as 
__COUNTER__. No
error will result from this kind of use.

 I do think that [1/2] made the code significantly worse-looking and
 this one is getting crazy.

It is a little crazy, but I find that I would feel differently about it if I 
had come up with
the idea. Actually, I am really impressed with the discoveries that arose from 
my original patch,
including recognizing that the min3/max3 generated worse code than nested macro 
calls. I had
never considered that possibility.

 How useful is W=2 anyway?  Has anyone found
 a bug using it?

Yes. But it is really hard to find anything useful when my normal kernel build 
throws 125000
warnings - for completely expected things - in W=2 builds. We do routinely 
build our drivers
with W=12, but we have a special hack to ignore the large number of warnings 
that the kernel
include files generate, while still getting warnings from our driver's code and 
include files.
They do sometimes catch problems.

 The number of warnings in default builds is already way
 too high :(

Do you mean the number of warnings enabled, or the number of warning messages 
being generated?
I am a fan of enabling lots of warnings, but it is not effective if doing so 
emits thousands of
messages. Back in the 2.4 kernel era, there was a time when I maintained a 
MIPS-based kernel
that built completely cleanly with way more warnings enabled than W=12 uses 
today. It took work
to get to that state, but we were able to maintain that for several kernel 
versions for our particular environment. These days, the kernel as a whole is 
in a much better state than we
started with for that old 2.4 kernel.

I'll say that there aren't really that many warnings that are the result of 
nested min/max
macros. There are *much* heavier hitters out there. For example nested 
externs account for
tens of thousands of warnings in W=2 builds. Such noise makes using W=2 kind of 
ridiculous
on the whole kernel. I'd like it to not be so ridiculous eventually. Some 60 
patches got my
build down under 1400 W=2 warnings. Then I realized that I had pushed that rock 
too far up the
hill without getting feedback on what I was doing.

I have been working on patches that add macros to allow disabling certain 
warnings in select
pieces of code. In that way the warning is silenced, but the macro remains as a 
sign to the
reader that there is something special going on. Unfortunately, those macros 
fail miserably
in the expansion of any macro called in the parameter of other macros, as the 
compile time
asserts sometimes are. And the compile time asserts make use of a nested extern 
by design.

It is enough to make me want to suggest dropping the nested externs warning 
from W=2, but
recognize that every nested extern that actually calls a function is a 
redundant prototype
declaration that could get to be inconsistent with the function it 

Re: [PATCH 2/2] kernel.h: use __COUNTER__ in min and max macros to avoid -Wshadow warnings

[Rustad, Mark D]

Michal,

On Sep 12, 2014, at 4:37 PM, Michal Nazarewicz min...@mina86.com wrote:

 On Fri, Sep 12 2014, Andrew Morton a...@linux-foundation.org wrote:
 On Thu, 11 Sep 2014 23:39:36 +0200 Michal Nazarewicz min...@mina86.com 
 wrote:
 
 Because min and max macros use the same variable names no matter
 how many times they are called (or how deep the nesting of their
 calls), each time min or max calls are nested, the same variables
 are declared.  This is especially noisy after min3 and max3 have
 been changed to nest min/max calls.
 
 Using __COUNTER__ solves the problem since each variable will get
 a unique number aadded to it.  The code will still work even if
 the compiler does not support __COUNTER__, but then the protection
 from shadow warning won't work.
 
 The same applies to min_t and max_t macros.
 
 ...
 
 --- a/include/linux/kernel.h
 +++ b/include/linux/kernel.h
 @@ -695,15 +695,27 @@ static inline void ftrace_dump(enum ftrace_dump_mode 
 oops_dump_mode) { }
 #endif /* CONFIG_TRACING */
 
 /*
 + * Preprocessor magic generating unique identifiers to avoid -Wshadow 
 warnings
 + * used by min, max, min_t and max_t macros.  cnt is __COUNTER__, op is the
 + * comparison operator; tx (ty) is type of the first (second) argument,
 + * xx (yy) is name of a temporary variable to hold the first (second) 
 argument,
 + * and x (y) is the first (second) argument.
 + */
 +#define _min_max_var(cnt, base) _mm_ ## cnt ## base
 +#define _min_max__(op, tx, xx, x, ty, yy, y) ({\
 +   tx xx = (x);\
 +   ty yy = (y);\
 +   (void) (xx == yy);\
 +   xx op yy ? xx : yy; })
 +#define _min_max_(cnt, op, tx, x, ty, y)   \
 +   _min_max__(op, tx, _min_max_var(cnt, a), x, ty, _min_max_var(cnt, b), y)
 +#define _min_max(...) _min_max_(__COUNTER__, __VA_ARGS__)
 
 The fact that __COUNTER__ is used in compiler-gcc4.h but not in
 compiler-gcc3.h makes me suspicious about its availability?
 
 http://gcc.gnu.org/ml/gcc-patches/2007-05/msg01579.html so looks like it
 has 7 years.  But as the commit message says, the code will still work,
 even w/o working __COUNTER__.
 
 I do think that [1/2] made the code significantly worse-looking
 
 Oh?  I actually thought [1/2] makes it nicer by having a single place
 where the min/max logic is implemented.

It does have that going for it.

 and this one is getting crazy.  How useful is W=2 anyway?
 
 I actually do agree with that.  I didn't have high hopes about getting
 this patch accepted, but wanted to send it out to show that it can be
 done, if it's really deemed useful.

Well, I learned something from it. Thank you for teaching this old dog a new 
trick.

 Has anyone found a bug using it?  The number of warnings in default
 builds is already way too high :(

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 7/7] sched: Silence nested-externs warnings

[Rustad, Mark D]

On Sep 19, 2014, at 12:34 PM, Richard Weinberger richard.weinber...@gmail.com 
wrote:

 On Fri, Sep 19, 2014 at 5:29 PM, Jeff Kirsher
 jeffrey.t.kirs...@intel.com wrote:
 From: Mark Rustad mark.d.rus...@intel.com
 
 Use diagnostic control macros to ignore nested-externs warnings
 in this case.
 
 CC: Ingo Molnar mi...@redhat.com
 CC: Peter Zijlstra pet...@infradead.org
 CC: Brian Norris computersforpe...@gmail.com
 Signed-off-by: Mark Rustad mark.d.rus...@intel.com
 Signed-off-by: Jeff Kirsher jeffrey.t.kirs...@intel.com
 ---
 include/linux/sched.h | 2 ++
 1 file changed, 2 insertions(+)
 
 diff --git a/include/linux/sched.h b/include/linux/sched.h
 index 5c2c885..ed52c76 100644
 --- a/include/linux/sched.h
 +++ b/include/linux/sched.h
 @@ -832,7 +832,9 @@ static inline int sched_info_on(void)
 #ifdef CONFIG_SCHEDSTATS
return 1;
 #elif defined(CONFIG_TASK_DELAY_ACCT)
 +   DIAG_PUSH DIAG_IGNORE(nested-externs)
extern int delayacct_on;
 +   DIAG_POP
 
 This ridiculous, please try to move this extern into the appropriate header 
 file
 instead of surrounding it with these macros.

Excellent. I'll try adding an include of delayacct.h instead. My patch was 
based on the assumption that the existing code was wanted in that form for some 
reason, so the patch served a purpose in recognizing that it isn't. So the 
macros have served a purpose before even being in place. :-)

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 7/7] sched: Silence nested-externs warnings

[Rustad, Mark D]

On Sep 19, 2014, at 1:41 PM, Richard Weinberger rich...@nod.at wrote:

 Am 19.09.2014 22:34, schrieb Rustad, Mark D:
 Excellent. I'll try adding an include of delayacct.h instead. My patch was 
 based on the assumption that the existing code was wanted in that form for 
 some reason, so the patch served a purpose in recognizing that it isn't. So 
 the macros have served a purpose before even being in place. :-)
 
 I bet sched.h does not include delayacct.h for a good reason.
 sched.h is included in a lot of files, maybe delayacct.h introduces nasty 
 dependency issues.
 You'll find out. ;-)

Right you are, but when I'm done, there will only be one extern declaration for 
that global, which is better than more than one. I'll think a little before 
settling on the best resolution.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 5/7] signal: Silence nested-externs warnings

[Rustad, Mark D]

On Sep 19, 2014, at 2:21 PM, Josh Triplett j...@joshtriplett.org wrote:

 On Fri, Sep 19, 2014 at 07:20:30PM +0200, Oleg Nesterov wrote:
 On 09/19, Richard Weinberger wrote:
 
 Am 19.09.2014 17:37, schrieb Jeff Kirsher:
 
 See patch 1 of the series.
 
 I was not CC'ed...
 
 Me too, and thus I don't understand this patch.
 
 But I have to admit it looks a bit ugly to me anyway.
 Can't we simply kill _NSIG_WORDS_is_unsupported_size ?
 
 This looks quite preferable.  Can you post that with a commit message
 and signoff?  Also, the indentation on the second of the three BUILD_BUG
 calls has some spaces in it, which it shouldn't.  With those fixed:
 Reviewed-by: Josh Triplett j...@joshtriplett.org

I haven't tried this patch myself yet, but assuming that it works, it is a far 
better way to go.

 diff --git a/include/linux/signal.h b/include/linux/signal.h
 index 750196f..679c9b4 100644
 --- a/include/linux/signal.h
 +++ b/include/linux/signal.h
 @@ -67,7 +67,6 @@ static inline int sigismember(sigset_t *set, int _sig)
 
 static inline int sigisemptyset(sigset_t *set)
 {
 -extern void _NSIG_WORDS_is_unsupported_size(void);
  switch (_NSIG_WORDS) {
  case 4:
  return (set-sig[3] | set-sig[2] |
 @@ -77,7 +76,7 @@ static inline int sigisemptyset(sigset_t *set)
  case 1:
  return set-sig[0] == 0;
  default:
 -_NSIG_WORDS_is_unsupported_size();
 +BUILD_BUG();
  return 0;
  }
 }
 @@ -90,7 +89,6 @@ static inline int sigisemptyset(sigset_t *set)
 #define _SIG_SET_BINOP(name, op) \
 static inline void name(sigset_t *r, const sigset_t *a, const sigset_t *b) \
 {\
 -extern void _NSIG_WORDS_is_unsupported_size(void);  \
  unsigned long a0, a1, a2, a3, b0, b1, b2, b3;   \
  \
  switch (_NSIG_WORDS) {  \
 @@ -107,7 +105,7 @@ static inline void name(sigset_t *r, const sigset_t *a, 
 const sigset_t *b) \
  r-sig[0] = op(a0, b0); \
  break;  \
  default:\
 -_NSIG_WORDS_is_unsupported_size();  \
 +BUILD_BUG();\
  }   \
 }
 
 @@ -128,8 +126,6 @@ _SIG_SET_BINOP(sigandnsets, _sig_andn)
 #define _SIG_SET_OP(name, op)
 \
 static inline void name(sigset_t *set)   
 \
 {\
 -extern void _NSIG_WORDS_is_unsupported_size(void);  \
 -\
  switch (_NSIG_WORDS) {  \
  case 4: set-sig[3] = op(set-sig[3]);  \
  set-sig[2] = op(set-sig[2]);  \
 @@ -137,7 +133,7 @@ static inline void name(sigset_t *set)   
 \
  case 1: set-sig[0] = op(set-sig[0]);  \
  break;  \
  default:\
 -_NSIG_WORDS_is_unsupported_size();  \
 +BUILD_BUG();\
  }   \
 }

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 7/7] sched: Silence nested-externs warnings

[Rustad, Mark D]

On Sep 19, 2014, at 3:54 PM, Peter Zijlstra pet...@infradead.org wrote:

 On Fri, Sep 19, 2014 at 08:29:40AM -0700, Jeff Kirsher wrote:
 From: Mark Rustad mark.d.rus...@intel.com
 
 Use diagnostic control macros to ignore nested-externs warnings
 in this case.
 
 CC: Ingo Molnar mi...@redhat.com
 CC: Peter Zijlstra pet...@infradead.org
 CC: Brian Norris computersforpe...@gmail.com
 Signed-off-by: Mark Rustad mark.d.rus...@intel.com
 Signed-off-by: Jeff Kirsher jeffrey.t.kirs...@intel.com
 ---
 include/linux/sched.h | 2 ++
 1 file changed, 2 insertions(+)
 
 diff --git a/include/linux/sched.h b/include/linux/sched.h
 index 5c2c885..ed52c76 100644
 --- a/include/linux/sched.h
 +++ b/include/linux/sched.h
 @@ -832,7 +832,9 @@ static inline int sched_info_on(void)
 #ifdef CONFIG_SCHEDSTATS
  return 1;
 #elif defined(CONFIG_TASK_DELAY_ACCT)
 +DIAG_PUSH DIAG_IGNORE(nested-externs)
  extern int delayacct_on;
 +DIAG_POP
  return delayacct_on;
 
 Who has this nested extern warn on in anycase?

They appear in W=2 builds, so you do have to ask for them.

 I've never seen it
 generate a warning. Also WTF is DIAG_PUSH/POP, its not a GCC thing
 afaict.

The first patch in the series adds macros to use the gcc/clang pragmas to 
control these things. Both compilers have the capability. The macros generate 
nothing for compilers that lack it.

In any case, this particular one will be resolved instead of silenced.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 22, 2014, at 8:33 AM, Borislav Petkov b...@alien8.de wrote:

 On Fri, Sep 19, 2014 at 08:29:33AM -0700, Jeff Kirsher wrote:
 The following patches silence over 100,000 warnings in a W=2
 kernel build. This series does most of it by using the compilers
 diagnostic controls. The first patch in the series adds macros to
 invoke the pragmas for those controls. Macros are provided for GCC
 and clang. Although they are highly compatible in this area, macros
 are provided for compiler-specific controls, and there is one
 example that uses a clang-specific control (look for DIAG_CLANG_IGNORE).
 
 Some missing-field-initializers warnings were resolved using
 the diagnostic control macros simply because so many lines
 would have had to have been changed. At this stage Mark thought 
 about avoiding possible merge issues. If the maintainer would 
 rather resolve them by using designated initialization, just 
 say so.
 
 The combined effect of this patch series and his other patches
 that did not use these diagnostic control macros was to reduce 
 the number of W=2 warnings from 127,164 to 1,345!
 
 Sorry but I don't see the point of actively adding macros to the code
 just so that gcc is happy. There's a reason why a bunch of warnings are
 disabled in the normal build and only enabled with the W= switch.
 
 The W= things are supposed to be used when developing code and have the
 compiler tell you about *possible* issues. That doesn't mean though that
 we have to actively fix otherwise perfectly fine code.

The problem is that the kernel include files throw so many warnings that it 
really discourages anyone from ever going through them, even for a single 
driver. The warnings are far more valuable and usable when known acceptable 
usages are silenced.

 Having the need to actively go in and add code so that gcc doesn't issue
 obscure warnings is going too far, IMO.

Well, the whole series of patches that I made definitely went too far - only 
the first 5 out of about 30 have been posted, but if we can make some progress 
on generating fewer warnings out of the include files, I think it would be 
helpful. Already the patches that use them have triggered some activity that 
has resulted in resolving warnings without use of the macros, and I see that as 
much better than simply using the macros.

The macros can serve a useful purpose, but they should not be widely used. When 
to use them is definitely a judgement call. If the macros are accepted, it may 
be worth adding a checkpatch.pl warning for adding a DIAG_*IGNORE macro.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 22, 2014, at 11:40 AM, Borislav Petkov b...@alien8.de wrote:

 On Mon, Sep 22, 2014 at 05:06:27PM +, Rustad, Mark D wrote:
 Well, the whole series of patches that I made definitely went too far
 - only the first 5 out of about 30 have been posted, but if we can
 make some progress on generating fewer warnings out of the include
 files, I think it would be helpful.
 
 Helpful for what? Those are W=2 warnings which are disabled in the
 default build.

It is helpful for using the warnings to look for problems or even just risks.

 The macros can serve a useful purpose, but they should not be widely
 used. When to use them is definitely a judgement call. If the macros
 are accepted, it may be worth adding a checkpatch.pl warning for
 adding a DIAG_*IGNORE macro.
 
 Right, so add the macros and tell people *not* to use them. That won't
 fly.

Right now the number of warnings generated when using W=2 simply tells people 
to never use W=2. That severely limits the value of a useful tool. A checkpatch 
warning doesn't mean to never do that, just that it needs a critical look and 
justification. That is certainly true of every patch I made that uses those 
macros.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] sched: Remove nested extern

[Rustad, Mark D]

On Sep 22, 2014, at 12:01 PM, Peter Zijlstra pet...@infradead.org wrote:

 On Mon, Sep 22, 2014 at 10:55:11AM -0700, Mark D Rustad wrote:
 Avoid W=2 nested-externs warning by moving the nested extern to
 a normal extern. This eliminates that warning which is generated
 for every inclusion of sched.h in a kernel build when W=2 is used.
 This also removes a point of maintenance if the definition of
 delayacct_on were ever to change.
 
 Yeah, so why is that warning worth using? Just disable the warn if
 you're bothered by it.

I assume that nested-externs is included in W=2 because many uses of
them, especially with function prototypes, creates a risk of inconsistent
declarations. The kernel has a fair number of them that seem to have
been added to disentangle include files. If they are judged to be
worthwhile, then it would be good to silence the warnings so that
attention can be given to other instances of the warnings. If those
nested externs are not worth the risk, well that is a different conversation.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] sched: Remove nested extern

[Rustad, Mark D]

On Sep 22, 2014, at 1:05 PM, Peter Zijlstra pet...@infradead.org wrote:

 On Mon, Sep 22, 2014 at 07:32:04PM +, Rustad, Mark D wrote:
 I assume that nested-externs is included in W=2 because many uses of
 them, especially with function prototypes, creates a risk of inconsistent
 declarations. The kernel has a fair number of them that seem to have
 been added to disentangle include files. If they are judged to be
 worthwhile, then it would be good to silence the warnings so that
 attention can be given to other instances of the warnings. If those
 nested externs are not worth the risk, well that is a different conversation.
 
 So would using something like LTO not be way better at actually finding
 the problems, seeing how it can actually see the inconsistency in
 declarations.
 
 It seems to me that banning the use of nested extern is misguided as
 they are a useful tool; they provide some means of keeping symbols from
 being globally visible even though they actually are. Its a really poor
 man's 'private', but its the best C provides.

By using the macros the use of nested externs can be discouraged without
being banned. Presence of a DIAG_IGNORE macro should mean that the exception
has been noted and accepted. And the macro remains as an indicator of that.
Or to be taken as a warning label: readers choice perhaps.

 Also, why do you care about W=2 nonsense anyhow? They're default
 disabled for a reason.

Because I have found that enabling many warnings helps identify problems
in code and it has been my standard practice since about 1999 to do so.
The compiler warnings are really just another form of static analysis,
and I use it routinely on every compile. Here is how routinely: I have
W=1 in my environment, W=12 is just too painful. I would change that
default to W=12 if it wasn't insane to do so.

In my own code, I use way more warnings than even W=12 enables and in
that code I just resolve them. I had never resorted to using the attributes
in my own code, but they are an available tool.

I just finally thought it might be time to consider opening up a new
method to manage them for the kernel.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 22, 2014, at 1:33 PM, Borislav Petkov b...@alien8.de wrote:

 Btw, out of curiosity, what is your use case for staring at those W=2
 warnings?

I know no one cares about out-of-tree drivers, but I have a hack that
allows building out-of-tree drivers without getting warnings from the
kernel includes. We do an automated compile of every patch with W=12
and expect clean compiles.

It would be nice to compile drivers in-tree and have a similar expectation.
I guess a similar hack could be developed, but since we are contributing
upstream, I would rather uncover any potential issues that may exist, even
if they aren't in the driver. The hack would tend to cover up such issues.
This is definitely NOT about covering up things that could be problems!

 In thinking about it, what we could also do is simply move the noisiest
 ones to W=3 or so, or even add another W= level. It'll be interesting to
 hear your use case though. AFAICT, this is the first time I hear of a
 more, let's say, serious use case of W= since we added the W= things a
 couple of years ago. :-)

Well, I have W=1 in my environment, so I don't even have to ask for it, I
just get it. W=12 is just insane, or I would use that all the time. I think
it would be nice for new code, or at least new drivers, to compile clean with
W=12, but that isn't possible when the kernel includes throw so many warnings.

Nested-externs, for example, can catch people gratuitously providing a
function prototype that could become a hazard, but some use of that may
be justified. The macros provide a way to specifically allow certain
instances while generally discouraging it. Of course if you never use
W=2 you may never catch those gratuitous declarations.

 Thanks.

Hopefully the discussion is somewhat useful.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] sched: Remove nested extern

[Rustad, Mark D]

On Sep 22, 2014, at 2:21 PM, Peter Zijlstra pet...@infradead.org wrote:

 On Mon, Sep 22, 2014 at 08:59:32PM +, Rustad, Mark D wrote:
 Because I have found that enabling many warnings helps identify problems
 in code and it has been my standard practice since about 1999 to do so.
 The compiler warnings are really just another form of static analysis,
 and I use it routinely on every compile. Here is how routinely: I have
 W=1 in my environment, W=12 is just too painful. I would change that
 default to W=12 if it wasn't insane to do so.
 
 Many warnings are just plain insane and stupid. They're not helping
 anybody. There's a very good reason many are disabled. I'm sure you can
 find some entertaining discussions on the topic if you search the LKML
 archives.

That is what I used to think. -Wshadow for example. What's the problem? It
is perfectly valid C. Well, I recently sent a patch that changed some
function parameters that used the name jiffies, which of course shadowed
the well-known global. If a macro were ever called in those functions
whose expansion ever tried to access jiffies, well it wouldn't do what
was expected. Not a bug now, but a trap for the future. I only found that
because I either resolved or silenced enough warnings to see something 
interesting.

Over the years I think I have resolved real bugs revealed by probably nearly
every one of the additional warning messages. They do have value, but that
value is typically lost if the norm is a flood of messages. Apple would not
have had their SSL problem if they had had -Wunreachable-code enabled and
noticed the message.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] moduleparam: Resolve missing-field-initializer warning

[Rustad, Mark D]

On Aug 31, 2014, at 5:52 PM, Rusty Russell ru...@rustcorp.com.au wrote:

 Jeff Kirsher jeffrey.t.kirs...@intel.com writes:
 From: Mark Rustad mark.d.rus...@intel.com
 
 Resolve a missing-field-initializer warning, that is produced
 by every reference to module_param_call, by using designated
 initialization for the first field. That is enough to silence
 the complaint.
 
 Signed-off-by: Mark Rustad mark.d.rus...@intel.com
 Signed-off-by: Jeff Kirsher jeffrey.t.kirs...@intel.com
 ---
 include/linux/moduleparam.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 
 Strange, I haven't seen this warning.  Compiler version?  And it's good
 to quote the error message, so people can google it.

The message is only seen when doing a W=2 build. I happened to be using gcc 
4.8.3, but I think most versions would produce the warning when it is enabled. 
It can either be silenced by using even a single designated initializer as I 
did here, or providing values for all of the fields. Because of the number of 
references to the macro, this change silences many warnings in W=2 builds.

One instance of the full warning message looks like this:

/home/share/git/nn-mdr/include/linux/moduleparam.h:198:16: warning: missing 
initializer for field ‘free’ of ‘struct kernel_param_ops’ 
[-Wmissing-field-initializers]
  static struct kernel_param_ops __param_ops_##name =  \
^
/home/share/git/nn-mdr/fs/fuse/inode.c:35:1: note: in expansion of macro 
‘module_param_call’
 module_param_call(max_user_bgreq, set_global_limit, param_get_uint,
 ^
/home/share/git/nn-mdr/include/linux/moduleparam.h:56:9: note: ‘free’ declared 
here
  void (*free)(void *arg);


 Cheers,
 Rusty.
 
 diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
 index 494f99e..d99a9e9 100644
 --- a/include/linux/moduleparam.h
 +++ b/include/linux/moduleparam.h
 @@ -196,7 +196,7 @@ struct kparam_array
 /* Obsolete - use module_param_cb() */
 #define module_param_call(name, set, get, arg, perm) \
  static struct kernel_param_ops __param_ops_##name = \
 -{ 0, (void *)set, (void *)get };\
 +{ .flags = 0, (void *)set, (void *)get };   \

This could also be resolved by adding a , NULL to the initializer above 
instead of the designated initializer. The designated initializer means that if 
additional optional fields were to be added in the future, this would not 
have to be touched to avoid generating the warning. However you prefer it. If 
instead you would prefer to designate all fields, the formal parameter names 
would have to change, since get and set would get substituted for the field 
designators .get and .set.

  __module_param_call(MODULE_PARAM_PREFIX,\
  name, __param_ops_##name, arg, \
  (perm) + sizeof(__check_old_set_param(set))*0, -1)
 -- 
 1.9.3

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] semaphore: Resolve some shadow warnings

[Rustad, Mark D]

On Sep 1, 2014, at 4:41 PM, Jeff Kirsher jeffrey.t.kirs...@intel.com wrote:

 On Mon, 2014-09-01 at 14:02 +0200, Peter Zijlstra wrote:
 On Thu, Aug 28, 2014 at 05:19:26AM -0700, Jeff Kirsher wrote:
 From: Mark Rustad mark.d.rus...@intel.com
 
 Resolve some shadow warnings resulting from using the name
 jiffies, which is a well-known global. This is not a problem
 of course, but it could be a trap for someone copying and
 pasting code, and it just makes W=2 a little cleaner.
 
 Signed-off-by: Mark Rustad mark.d.rus...@intel.com
 Signed-off-by: Jeff Kirsher jeffrey.t.kirs...@intel.com
 
 Why isn't Mark sending this email?
 
 Mark sent me several patches like this, for me to push upstream.  So, I
 am making sure the appropriate owner is the receives the patch versus
 blindly sending to LKML.
 
 
 ---
 kernel/locking/semaphore.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)
 
 diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
 index 6815171..7782dbc 100644
 --- a/kernel/locking/semaphore.c
 +++ b/kernel/locking/semaphore.c
 @@ -36,7 +36,7 @@
 static noinline void __down(struct semaphore *sem);
 static noinline int __down_interruptible(struct semaphore *sem);
 static noinline int __down_killable(struct semaphore *sem);
 -static noinline int __down_timeout(struct semaphore *sem, long jiffies);
 +static noinline int __down_timeout(struct semaphore *sem, long njiffies);
 static noinline void __up(struct semaphore *sem);
 
 So what's wrong with calling it timeout instead? That's what most
 other sites do.
 
 Timeout would work as well to resolve the shadow warnings.

It would, but then it would be unclear as to what units the timeout was in. I 
have no other objection to timeout, I was just trying to preserve the meaning 
in the existing overloaded name. The n to me suggests a number and, if 
anything, number of jiffies conveys a more precise meaning than simply jiffies 
did.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 25, 2014, at 12:45 AM, Geert Uytterhoeven ge...@linux-m68k.org wrote:

 Instead of grepping, you can feed the build log to linux-log-summary.
 Or when changing a driver, feed the before and after build logs to
 linux-log-diff. That way you won't miss the single new warning you've
 just introduced.
 
 https://github.com/geertu/linux-scripts

Thanks for making me aware of these tools. linux-log-summary does reduce the 
totally insane 125,000 warnings to the merely unreasonable 10,000 or so. I'm 
not being sarcastic, they *are* very nice tools that I will use. Thank you for 
pointing me to them.

I still observe that thousands of those unique warnings are emitted by the 
syscall table and its override initializations. I still think they should be 
silenced because they serve no purpose in that particular case. That is, the 
warning is complaining about what it is designed to complain about, but in this 
particular instance, the code is written as it needs to be, so why not 
acknowledge and capture that exception in the source?

A couple thousand more come from every use of compiletime_assert, for which I 
do not have a solution yet. Even my macros don't help with that one.

Most of the others come from null-entry table initializations, i.e. { 0 }, 
which give missing field initializer warnings. I'd like to define a macro 
something like:

#define ZERO_ENTRY DIAG_PUSH DIAG_IGNORE(missing-field-initializers) \
{ 0 } DIAG_POP

Then simply using ZERO_ENTRY, a zero entry could be provided without a 
complaint from the compiler. But of course that can't be done if the DIAG_* 
macros aren't there. It also would keep the DIAG_* macros out of the .c files 
and just in the header where ZERO_ENTRY is defined.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 26, 2014, at 12:58 PM, j...@joshtriplett.org j...@joshtriplett.org 
wrote:

 On Fri, Sep 26, 2014 at 07:37:19PM +, Rustad, Mark D wrote:
 Most of the others come from null-entry table initializations, i.e. {
 0 }, which give missing field initializer warnings.
 
 I'd suggest that such initializers should just be {}, not { 0 }, and we
 should teach compilers to specifically *not* complain about empty
 initializers even when otherwise complaining about missing fields.
 Initializing a structure to 0 is completely sensible.

I agree completely! But of course that isn't how it is now. I guess I have 
spent too many years stuck on a single version of gcc that I tend not to think 
of changing the compiler readily enough. At least now I can upgrade the 
compiler freely.

Made me go check to be sure. Indeed even { } still throws the 
missing-initializers warning with gcc 4.8.3.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 23, 2014, at 7:49 AM, Josh Triplett j...@joshtriplett.org wrote:

 On Tue, Sep 23, 2014 at 10:01:20AM +0200, Borislav Petkov wrote:
 ./arch/x86/include/asm/io_apic.h: In function ‘io_apic_modify’:
 ./arch/x86/include/asm/io_apic.h:223:48: warning: declaration of ‘apic’ 
 shadows a global declaration [-Wshadow]
 static inline void io_apic_modify(unsigned int apic, unsigned int reg, 
 unsigned int value)
^
 In file included from ./arch/x86/include/asm/smp.h:12:0,
 from include/linux/smp.h:59,
 from include/linux/topology.h:33,
 from include/linux/gfp.h:8,
 from include/linux/kmod.h:22,
 from include/linux/module.h:13,
 from drivers/edac/amd64_edac.h:65,
 from drivers/edac/amd64_edac.c:1:
 ./arch/x86/include/asm/apic.h:366:21: warning: shadowed declaration is here 
 [-Wshadow]
 extern struct apic *apic;
 ^
 
 So gcc complains that an unsigned int shadows a struct apic pointer.
 
 Here, I think the right fix involves picking a more descriptive name
 than apic for the global varible.

I agree, but I don't know enough about the area to necessarily know what it 
should be called instead. I do have a patch that changes the local variables 
instead, but even as I made it, I didn't really think it was right. But it 
silenced a ton of warnings and let me see other things.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 23, 2014, at 1:22 AM, Borislav Petkov b...@alien8.de wrote:

 On Mon, Sep 22, 2014 at 09:50:54PM +, Rustad, Mark D wrote:
 * Fixing those is a good idea if the fixes are clean - I think we all
 agree by now that adding code just to shut up gcc is not nice.

Yes, but I think there are a few cases where it could be helpful. When
there is something exceptional that will throw a warning. In one of the
patches that Jeff sent, I used the DIAG_CLANG_IGNORE macro to suppress
the warning that is thrown for every entry of the syscall table when
compiled with clang. The code is right and doing exactly what is wanted,
so note the exception and make it shut up.

 * Then, even if all those warnings were fixed one fine day, the people
 who fix them would be fighting windmills because every new patch which
 adds new places causing those warnings would simply go in because the
 warnings are not visible in default builds.
 
 So the question IMO turns into: are there some warnings which we should
 promote to default builds so that they get taken care of eventually...

I would generally be in favor of that over time, but I recognize that with
the range of architectures and toolchains that need to be supported, that
may be difficult.

 Well, I have W=1 in my environment, so I don't even have to ask for it, I
 just get it.
 
 I think this was the initial use case we had in mind for W= - use it
 during development in order to have the compiler do extra checks to your
 code. And it has caught a couple of issues, FWIW.

Yes, not everyone building the kernel is a developer. I certainly get that.
Right now W=2 is kind of useless for developers because so much noise is
generated. With the number of people working on Linux, it may be enough if
a handful are taking a look at W=2 messages, but right now it is a pretty
awful task to even try to look.

 Nested-externs, for example, can catch people gratuitously providing a
 function prototype that could become a hazard, but some use of that may
 be justified. The macros provide a way to specifically allow certain
 instances while generally discouraging it. Of course if you never use
 W=2 you may never catch those gratuitous declarations.
 
 Sure, but the cost for fixing that is what bothers me. For that
 particular case, it probably would even be cleaner to add a
 nested-extern check to checkpatch instead of cluttering the code with
 those macros.

Generally there should be relatively few exceptions that need to be tagged.
If there were 1000, that would be way too many. The full series of my
patches had 90 instances. Some of the few that have been sent have been
resolved in better ways, which we all agree is better. Suppose that 40
can't be reasonably resolved in direct ways. Is that really costly? I'm
trying to understand what your perception of the cost is.

Perhaps checkpatch would be a better gatekeeper for new code. OTOH, some of
those nested externs have already been eliminated, so at least for now the
warning is serving a purpose since it is scrubbing existing code.

 Hopefully the discussion is somewhat useful.
 
 Well, it has become already, as you can see. :-D

I take it as a given that the kernel sometimes has special needs and needs
to do special things. The macros make it possible to mark those special
usages. I prefer adding the macros in a few places to eliminating a warning
altogether unless the warning is always useless. I'm sure we agree that we
don't want to ever turn on -pedantic! :-)

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 23, 2014, at 11:44 AM, Borislav Petkov b...@alien8.de wrote:

 On Tue, Sep 23, 2014 at 05:24:22PM +, Rustad, Mark D wrote:
 Yes, but I think there are a few cases where it could be helpful. When
 there is something exceptional that will throw a warning. In one of the
 patches that Jeff sent, I used the DIAG_CLANG_IGNORE macro to suppress
 the warning that is thrown for every entry of the syscall table when
 compiled with clang. The code is right and doing exactly what is wanted,
 so note the exception and make it shut up.
 
 So we're doing some dancing around solely to shut up the compiler? Even
 if the code is correct?!? Sorry, this is just ass backwards.

Well, please consider the specifics. The entire syscall table is initialized
with a constant pattern to be sure that every item is initialized. Then each
syscall is initialized into its proper place. The compiler is complaining that
entries are being initialized twice.

Most of the time, that is not done, and so it may catch a patch foulup or
something. In this particular case, it is normal and intended. There is
nothing wrong, so there is no reason to throw a warning for every single
entry in the table. Which is what happens with clang today.

So the code is correct, but in general the warning can reveal certain issues.
Just not in this particular usage. This happens to be a warning specific to
clang at the moment.

 If it were me, I'd say even one is too much. Because the very thing
 of adding code just to shut up the compiler which generates otherwise
 correct code is simply very very wrong in my book.
 
 Bear in mind that even if initially you have a low number of macro
 invocations, that number will grow because people will start using it in
 other places too. And all of a sudden, the thing has spread like weed
 and there's no removing it anymore. So we better not start in the first
 place.

That is why it would be more than reasonable for checkpatch to warn on the
macro introductions. It is certainly a more significant thing than a
line  80 characters.

 Again, we should take compiler warnings with a grain of salt and judge
 them only by the quality of the generated code. IMO.

I think more than the nature of the executable code matters. The namespace
issues revealed by -Wshadow can certainly create nasty surprises over time.
But we can't get that value from them when they are lost in an ocean of
warnings that are always there and are not the source of any trouble.

The problem is that when so many warnings are generated, particularly by
include files, even useful warnings will not be seen. Specifically
silencing ones that are deemed to be ok will help in seeing ones that
are not. The silencing has the greatest effect in the include files,
since there is such a multiplier effect.

Warnings that no one looks at, or bother to generate at all, have
absolutely no value. Even a silenced warning continues to wear its
shame attribute (macro). Hmm. Maybe instead of DIAG_* they should be
named SHAME_*.

Most of the time, it is new instances of warnings that are most likely to
reveal a problem. Hiding them in a flood of normal warnings prevents
them from ever being seen. And that is a shame.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 23, 2014, at 11:44 AM, Borislav Petkov b...@alien8.de wrote:

 Again, we should take compiler warnings with a grain of salt and judge
 them only by the quality of the generated code. IMO.

The more I thought about this, the more I think it argues for having some
diagnostic control macros. Tools such as the compiler can be very thorough -
that is their strength. The warnings that the compiler emits are things
that humans should consider. There are times when the human can judge
that nothing need be done about it, so why not use the macros to indicate
that that judgement has been made and get it out of the way so more
potentially interesting issues can be found? How many people need to
make that judgement over and over? That is clearly a waste of time and
is why these higher warning levels simply don't get looked at. So lets
capture that judgement in the code. That judgement would continue to be
open to reevaluation in any case.

The syscall issue is not arising from an include file, but I think
demonstrates the use nicely and I think is a fine example of a legitimate
use in a C file.

For me, it is about getting more value out of the evaluation that the compiler
can do. When the output of W=2 is unusable because it is so voluminous - filled
with things known to not cause problems - why not silence some of major
sources so that more interesting issues might be seen? By not doing so, W=2
has close to 0 value because no one looks at it.

I guess I also trust the maintainers to not accept lots of patches that add
uses of such macros. They have certainly demonstrated that up to this point.

:-)

-- 
Mark Rustad, Networking Division, Intel Corporation

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC][PATCH 01/12 v3] x86/kvm/tracing: Use helper function trace_seq_buffer_ptr()

[Rustad, Mark D]

On Nov 4, 2014, at 7:52 AM, Steven Rostedt rost...@goodmis.org wrote:

 From: Steven Rostedt (Red Hat) rost...@goodmis.org
 
 To allow for the restructiong of the trace_seq code, we need users
 of it to use the helper functions instead of accessing the internals
 of the trace_seq structure itself.
 
 Cc: Mark Rustad mark.d.rus...@intel.com
 Cc: Jeff Kirsher jeffrey.t.kirs...@intel.com
 Cc: Paolo Bonzini pbonz...@redhat.com
 Signed-off-by: Steven Rostedt rost...@goodmis.org
 ---
 arch/x86/kvm/mmutrace.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
 index 5aaf35641768..ce463a9cc8fb 100644
 --- a/arch/x86/kvm/mmutrace.h
 +++ b/arch/x86/kvm/mmutrace.h
 @@ -22,7 +22,7 @@
   __entry-unsync = sp-unsync;
 
 #define KVM_MMU_PAGE_PRINTK() ({  \
 - const u32 saved_len = p-len;   \
 + const char *saved_ptr = trace_seq_buffer_ptr(p);\

I think the above should not be a const char *, because the location pointed to 
is surely being changed. It should either be a char * or a char * const.

   static const char *access_str[] = { \
   ---, --x, w--, w-x, -u-, -ux, wu-, wux  \
   };  \
 @@ -41,7 +41,7 @@
role.nxe ?  : !,   \
__entry-root_count,   \
__entry-unsync ? unsync : sync, 0);   \
 - p-buffer + saved_len;  \
 + saved_ptr;  \
   })
 
 #define kvm_mmu_trace_pferr_flags   \
 -- 
 2.1.1

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [RFC][PATCH 01/12 v3] x86/kvm/tracing: Use helper function trace_seq_buffer_ptr()

[Rustad, Mark D]

On Nov 4, 2014, at 11:35 AM, Steven Rostedt rost...@goodmis.org wrote:

 On Tue, 4 Nov 2014 14:09:54 -0500
 Steven Rostedt rost...@goodmis.org wrote:
 
 On Tue, 4 Nov 2014 17:17:08 +
 Rustad, Mark D mark.d.rus...@intel.com wrote:
 
 On Nov 4, 2014, at 7:52 AM, Steven Rostedt rost...@goodmis.org wrote:
 
 From: Steven Rostedt (Red Hat) rost...@goodmis.org
 
 To allow for the restructiong of the trace_seq code, we need users
 of it to use the helper functions instead of accessing the internals
 of the trace_seq structure itself.
 
 Cc: Mark Rustad mark.d.rus...@intel.com
 Cc: Jeff Kirsher jeffrey.t.kirs...@intel.com
 Cc: Paolo Bonzini pbonz...@redhat.com
 Signed-off-by: Steven Rostedt rost...@goodmis.org
 ---
 arch/x86/kvm/mmutrace.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
 index 5aaf35641768..ce463a9cc8fb 100644
 --- a/arch/x86/kvm/mmutrace.h
 +++ b/arch/x86/kvm/mmutrace.h
 @@ -22,7 +22,7 @@
__entry-unsync = sp-unsync;
 
 #define KVM_MMU_PAGE_PRINTK() ({   \
 -  const u32 saved_len = p-len;   \
 +  const char *saved_ptr = trace_seq_buffer_ptr(p);\
 
 I think the above should not be a const char *, because the location 
 pointed to is surely being changed. It should either be a char * or a char 
 * const.
 
 Ah right. It should be 'char * const'.
 
 
 Actually, I take that back. The contents of saved_ptr should not be
 modified.

At least not by the caller of the macro. The subsequent call to 
trace_seq_printf will be changing the contents at that address, but not through 
use of that pointer.

 It may seem strange, but the update is done via the trace_seq_printf().
 Then that content is return back to the user. The user should
 definitely *not* modify the contents of saved_ptr.

Agreed.

 This patch is good as is. It should not be a char *, or char * const.

Yes. I did further checking and agree. Although that memory will be written, it 
isn't written through that pointer and it is the best type as a return value.

 -- Steve
 
 
 
 
static const char *access_str[] = { \
---, --x, w--, w-x, -u-, -ux, wu-, wux  \
};  \
 @@ -41,7 +41,7 @@
 role.nxe ?  : !,   \
 __entry-root_count,   \
 __entry-unsync ? unsync : sync, 0);   \
 -  p-buffer + saved_len;  \
 +  saved_ptr;  \
})
 
 #define kvm_mmu_trace_pferr_flags   \
 -- 
 2.1.1
 
 
 

Acked-by: Mark Rustad mark.d.rus...@intel.com

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] irda: Convert function pointer arrays and uses to const

[Rustad, Mark D]

On Dec 10, 2014, at 10:28 AM, Joe Perches j...@perches.com wrote:

 Making things const is a good thing.
 
 (x86-64 defconfig with all irda)
 $ size net/irda/built-in.o*
   text   data bss dec hex filename
 109276   1868 244  111388   1b31c net/irda/built-in.o.new
 108828   2316 244  111388   1b31c net/irda/built-in.o.old
 
 Signed-off-by: Joe Perches j...@perches.com
 ---
 include/net/irda/parameters.h  | 6 +++---
 net/irda/ircomm/ircomm_param.c | 8 
 net/irda/irttp.c   | 6 --
 net/irda/parameters.c  | 8 
 net/irda/qos.c | 6 +++---
 5 files changed, 18 insertions(+), 16 deletions(-)
 
 diff --git a/include/net/irda/parameters.h b/include/net/irda/parameters.h
 index 42713c9..2d9cd00 100644
 --- a/include/net/irda/parameters.h
 +++ b/include/net/irda/parameters.h
 @@ -71,17 +71,17 @@ typedef int (*PV_HANDLER)(void *self, __u8 *buf, int len, 
 __u8 pi,
 PV_TYPE type, PI_HANDLER func);
 
 typedef struct {
 - PI_HANDLER func;  /* Handler for this parameter identifier */
 + const PI_HANDLER func;  /* Handler for this parameter identifier */
   PV_TYPEtype;  /* Data type for this parameter */
 } pi_minor_info_t;
 
 typedef struct {
 - pi_minor_info_t *pi_minor_call_table;
 + const pi_minor_info_t *pi_minor_call_table;

Might you want to go a little further and make it:
const pi_minor_into_t * const pi_minor_call_table;
so that the pointer itself is also constant? That could apply to some others 
below as well.

   int len;
 } pi_major_info_t;
 
 typedef struct {
 - pi_major_info_t *tables;
 + const pi_major_info_t *tables;
   int  len;
   __u8 pi_mask;
   int  pi_major_offset;
 diff --git a/net/irda/ircomm/ircomm_param.c b/net/irda/ircomm/ircomm_param.c
 index 27be782..3c4caa6 100644
 --- a/net/irda/ircomm/ircomm_param.c
 +++ b/net/irda/ircomm/ircomm_param.c
 @@ -61,12 +61,12 @@ static int ircomm_param_dte(void *instance, irda_param_t 
 *param, int get);
 static int ircomm_param_dce(void *instance, irda_param_t *param, int get);
 static int ircomm_param_poll(void *instance, irda_param_t *param, int get);
 
 -static pi_minor_info_t pi_minor_call_table_common[] = {
 +static const pi_minor_info_t pi_minor_call_table_common[] = {
   { ircomm_param_service_type, PV_INT_8_BITS },
   { ircomm_param_port_type,PV_INT_8_BITS },
   { ircomm_param_port_name,PV_STRING }
 };
 -static pi_minor_info_t pi_minor_call_table_non_raw[] = {
 +static const pi_minor_info_t pi_minor_call_table_non_raw[] = {
   { ircomm_param_data_rate,PV_INT_32_BITS | PV_BIG_ENDIAN },
   { ircomm_param_data_format,  PV_INT_8_BITS },
   { ircomm_param_flow_control, PV_INT_8_BITS },
 @@ -74,13 +74,13 @@ static pi_minor_info_t pi_minor_call_table_non_raw[] = {
   { ircomm_param_enq_ack,  PV_INT_16_BITS },
   { ircomm_param_line_status,  PV_INT_8_BITS }
 };
 -static pi_minor_info_t pi_minor_call_table_9_wire[] = {
 +static const pi_minor_info_t pi_minor_call_table_9_wire[] = {
   { ircomm_param_dte,  PV_INT_8_BITS },
   { ircomm_param_dce,  PV_INT_8_BITS },
   { ircomm_param_poll, PV_NO_VALUE },
 };
 
 -static pi_major_info_t pi_major_call_table[] = {
 +static const pi_major_info_t pi_major_call_table[] = {
   { pi_minor_call_table_common,  3 },
   { pi_minor_call_table_non_raw, 6 },
   { pi_minor_call_table_9_wire,  3 }
 diff --git a/net/irda/irttp.c b/net/irda/irttp.c
 index 3ef0b08..b6ab41d 100644
 --- a/net/irda/irttp.c
 +++ b/net/irda/irttp.c
 @@ -71,11 +71,13 @@ static void irttp_status_indication(void *instance,
   LINK_STATUS link, LOCK_STATUS lock);
 
 /* Information for parsing parameters in IrTTP */
 -static pi_minor_info_t pi_minor_call_table[] = {
 +static const pi_minor_info_t pi_minor_call_table[] = {
   { NULL, 0 }, /* 0x00 */
   { irttp_param_max_sdu_size, PV_INTEGER | PV_BIG_ENDIAN } /* 0x01 */
 };
 -static pi_major_info_t pi_major_call_table[] = { { pi_minor_call_table, 2 } 
 };
 +static const pi_major_info_t pi_major_call_table[] = {
 + { pi_minor_call_table, 2 }
 +};
 static pi_param_info_t param_info = { pi_major_call_table, 1, 0x0f, 4 };
 
 / GLOBAL PROCEDURES /
 diff --git a/net/irda/parameters.c b/net/irda/parameters.c
 index 006786b..16ce32f 100644
 --- a/net/irda/parameters.c
 +++ b/net/irda/parameters.c
 @@ -52,7 +52,7 @@ static int irda_insert_no_value(void *self, __u8 *buf, int 
 len, __u8 pi,
 static int irda_param_unpack(__u8 *buf, char *fmt, ...);
 
 /* Parameter value call table. Must match PV_TYPE */
 -static PV_HANDLER pv_extract_table[] = {
 +static const PV_HANDLER pv_extract_table[] = {
   irda_extract_integer, /* Handler for any length integers */
   

Re: [PATCH] iwl4965: Enable checking of format strings

[Rustad, Mark D]

On Feb 11, 2015, at 2:51 PM, Rasmus Villemoes li...@rasmusvillemoes.dk wrote:

 Since these fmt_* variables are just const char*, and not const
 char[], gcc (and smatch) doesn't to type checking of the arguments to
 the printf functions. Since the linker knows perfectly well to merge
 identical string constants, there's no point in having three static
 pointers waste memory and give an extra level of indirection.
 
 This removes over 100 non-constant format argument warnings from
 smatch, accounting for about 20% of all such warnings in an
 allmodconfig.
 
 Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk
 ---
 drivers/net/wireless/iwlegacy/4965-debug.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/net/wireless/iwlegacy/4965-debug.c 
 b/drivers/net/wireless/iwlegacy/4965-debug.c
 index e0597bfdddb8..18855325cc1c 100644
 --- a/drivers/net/wireless/iwlegacy/4965-debug.c
 +++ b/drivers/net/wireless/iwlegacy/4965-debug.c
 @@ -28,10 +28,9 @@
 #include common.h
 #include 4965.h
 
 -static const char *fmt_value =   %-30s %10u\n;
 -static const char *fmt_table =   %-30s %10u  %10u  %10u  %10u\n;
 -static const char *fmt_header =
 -%-32scurrent  cumulative   delta max\n;

Why not change these to:
static const char fmt_value[] =   %-30s %10u\n;
static const char fmt_table[] =   %-30s %10u  %10u  %10u  %10u\n;
static const char fmt_header[] =
%-32scurrent  cumulative   delta max\n;

I think that is better than the macros and avoids the extra pointers that I 
agree are useless.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [Bugfix] x86, irq: Fix a regression caused by commit b5dc8e6c21e7

[Rustad, Mark D]

Gerry,

 On Aug 9, 2015, at 1:15 AM, Jiang Liu jiang@linux.intel.com wrote:
 
 Alex Deucher, Mark Rustad and Alexander Holler reported a regression
 with the latest v4.2-rc4 kernel, which breaks some SATA controllers.
 With multi-MSI capable SATA controllers, only the first port works,
 all other ports times out when executing SATA commands. This regression
 bisects to 52f518a3a7c2 (x86/MSI: Use hierarchical irqdomains to manage
 MSI interrupts), but it's not the root cause, it just triggers a bug
 caused by b5dc8e6c21e7 (x86/irq: Use hierarchical irqdomain to manage
 CPU interrupt vectors).
 
 With this patch applied, the affected SATA controllers work as expected.

I see the same thing here as well.

 Signed-off-by: Jiang Liu jiang@linux.intel.com
 Reported-by: Alex Deucher alexdeuc...@gmail.com
 Reported-by: Mark Rustad mrus...@gmail.com
 Reported-by: Alexander Holler hol...@ahsoftware.de
 ---
 Hi Alex, Mark and Alexandler,
   Sorry for the long delay to root cause this regression, it's
 really annoying. Could you please help test this patch against the
 latest v4.2-rcx?

It works for me. Thanks.

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [Intel-wired-lan] [PATCH] fm10k:Fix error handling in the function fm10k_setup_tc

[Rustad, Mark D]

Nicholas Krause  wrote:

> This fixes error handling in the function fm10k_setup_tc to properly
> check if the call to the function fm10k_open has failed by returning
> a error and if so return immediately to the caller of the function
> fm10k_setup_tc to properly signal this non recoverable failure.
> 
> Signed-off-by: Nicholas Krause 
> ---
> drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c 
> b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> index 99228bf..5a4e702 100644
> --- a/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> @@ -1146,6 +1146,7 @@ static struct rtnl_link_stats64 
> *fm10k_get_stats64(struct net_device *netdev,
> int fm10k_setup_tc(struct net_device *dev, u8 tc)
> {
>   struct fm10k_intfc *interface = netdev_priv(dev);
> + int err;
> 
>   /* Currently only the PF supports priority classes */
>   if (tc && (interface->hw.mac.type != fm10k_mac_pf))
> @@ -1175,7 +1176,9 @@ int fm10k_setup_tc(struct net_device *dev, u8 tc)
>   fm10k_mbx_request_irq(interface);
> 
>   if (netif_running(dev))
> - fm10k_open(dev);
> + err = fm10k_open(dev);
> + if (err)
> + return err;
> 
>   /* flag to indicate SWPRI has yet to be updated */
>   interface->flags |= FM10K_FLAG_SWPRI_CONFIG;

NAK. This will reference an uninitialized err variable when netif_running 
returns false. The compiler should complain about this, suggesting that it 
wasn't compiled.

--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 3.2 035/107] PCI: Add dev_flags bit to access VPD through function 0

[Rustad, Mark D]

Ben Hutchings  wrote:

> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -176,6 +176,8 @@ enum pci_dev_flags {
>   PCI_DEV_FLAGS_NO_D3 = (__force pci_dev_flags_t) 2,
>   /* Provide indication device is assigned by a Virtual Machine Manager */
>   PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) 4,
> + /* Get VPD from function 0 VPD */
> + PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
> };
> 
> enum pci_irq_reroute_variant {

In this hunk I happened to notice the change in how these values are assigned. 
Should the new value remain (1 << 8) or should it fall in line with the older 
implementation and simply be 8? Or should it be 256? It depends on which kind 
of consistency you prefer for the backport.

--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 3.2 035/107] PCI: Add dev_flags bit to access VPD through function 0

[Rustad, Mark D]

Ben Hutchings  wrote:

> They're bit masks, not bit numbers, both in 3.2 and upstream.  In
> mainline, bits 3-7 have already been assigned to other flags.  I don't
> see the need to renumber or write the value differently when
> backporting.

No problem. I just saw the different representations and was wondering how you 
felt about it. Next time I'll know. Thanks.

--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [GIT] Networking

[Rustad, Mark D]

> On Sep 4, 2015, at 2:07 AM, David Laight  wrote:
> 
>> I find them useful as syntactic sugar. We have not used them a lot, but 
>> there are cases in our crypto
>> handling code where we have fixed size array inputs/outputs and there we 
>> opted to use them. They make
>> it easy to remember what the expected sizes of input and output are without 
>> having to read through the
>> implementation (of course we never even tried to use sizeof on these 
>> pointers).
>> 
>> static int smp_ah(struct crypto_blkcipher *tfm, const u8 irk[16],
>>  const u8 r[3], u8 res[3])
> 
> Expect that it looks like you are passing arrays by value,
> but instead you are passing by reference.
> 
> Explicitly pass by reference and sizeof works.

It depends on what you mean by works. It at least doesn't look so misleading 
when passing by reference and so works more as expected. The sizeof in either 
case will never return the size of the array. To have sizeof return the size of 
the array would require a typedef of the array to pass by reference. In some 
cases that could be the right thing to do.

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [GIT] Networking

[Rustad, Mark D]

> On Sep 7, 2015, at 4:02 AM, David Laight  wrote:
> 
> Feed:
> int bar(int (*f)[10]) { return sizeof *f; }
> into cc -O2 -S and look at the generated code - returns 40 not 4.

Yes, indeed it does. And with clang too. I guess I was too easily discouraged 
when looking for a workable syntax some years ago. At the time I stopped when 
the typedef worked, which really just encapsulates this. I should have 
recognized that then. Thanks for making it all so clear.

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] igb: don't unmap hw_addr if its NULL

[Rustad, Mark D]

> On Sep 9, 2015, at 9:07 PM, Jarod Wilson  wrote:
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c 
> b/drivers/net/ethernet/intel/igb/igb_main.c
> index e174fbb..a5e0022 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -2823,7 +2823,8 @@ static void igb_remove(struct pci_dev *pdev)
> 
>   igb_clear_interrupt_scheme(adapter);
> 
> - pci_iounmap(pdev, hw->hw_addr);
> + if (hw->hw_addr)
> + pci_iounmap(pdev, hw->hw_addr);
>   if (hw->flash_address)
>   iounmap(hw->flash_address);
>   pci_release_selected_regions(pdev,

I don't think that this is entirely the right solution. In ixgbe we have a 
separate pointer, io_addr, used to manage the resource, so that the space can 
be freed even after hw_addr is cleared. With the approach above, the 
pci_iounmap will not ever be called on the space. You can see how ixgbe is 
doing it.

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [RFC PATCH] vfio/pci: Use kernel VPD access functions

[Rustad, Mark D]

> On Sep 11, 2015, at 6:11 PM, Rustad, Mark D <mark.d.rus...@intel.com> wrote:
> 
> Superficially this looks pretty good. I need to think harder to be sure of 
> the details.

This is the first time I've looked at all at any of the vfio code, but this is 
still looking good to me. Thanks for taking this on and exposing the vfio code 
to me. I hope more devices will be able to take advantage of the quirk and get 
their VPD issues resolved.

I did run this on a host with a device with VPD assigned to a guest and did not 
see any trouble when accessing it concurrently from both the guest and the host 
on the same and different functions. I don't think my particular environment is 
ideal to fully reproduce the problem (no writable VPD area), but my initial 
testing looks good.

Acked-by: Mark Rustad <mark.d.rus...@intel.com>

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [RFC PATCH] vfio/pci: Use kernel VPD access functions

[Rustad, Mark D]

Alex,

> On Sep 11, 2015, at 11:16 AM, Alex Williamson  
> wrote:
> 
> RFC - Is this something we should do?

Superficially this looks pretty good. I need to think harder to be sure of the 
details.

> Should we consider providing
> similar emulation through PCI sysfs to allow lspci to also make use
> of the vpd interfaces?

It looks to me like lspci already uses the vpd attribute in sysfs to access 
VPD, so maybe nothing more than this is needed. No doubt lspci can be coerced 
into accessing VPD directly, but is that really worth going after? I'm not so 
sure.

An strace of lspci accessing a device with VPD shows me:

write(1, "\tCapabilities: [e0] Vital Produc"..., 39 Capabilities: [e0] 
Vital Product Data
) = 39
open("/sys/bus/pci/devices/:02:00.0/vpd", O_RDONLY) = 4
^^^ accesses to this should be safe, I 
think

pread(4, "\202", 1, 0)  = 1
pread(4, "\10\0", 2, 1) = 2
pread(4, "PVL Dell", 8, 3)  = 8
write(1, "\t\tProduct Name: PVL Dell\n", 25 Product Name: PVL Dell
) = 25

and so forth.

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH net 3/7] openvswitch: Fix skb leak in ovs_fragment()

[Rustad, Mark D]

> On Sep 29, 2015, at 3:39 PM, Joe Stringer  wrote:
> 
> @@ -728,8 +727,14 @@ static void ovs_fragment(struct vport *vport, struct 
> sk_buff *skb, u16 mru,
>   WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
> ovs_vport_name(vport), ntohs(ethertype), mru,
> vport->dev->mtu);
> - kfree_skb(skb);
> + goto out;
>   }
> +
> + skb = NULL;
> +
> +out:
> + if (skb)
> + kfree_skb(skb);
> }
> 
> static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,

Wouldn't that hunk be better as:

@@ -728,8 +727,13 @@ static void ovs_fragment(struct vport *vport, struct 
sk_buff *skb, u16 mru,
WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
  ovs_vport_name(vport), ntohs(ethertype), mru,
  vport->dev->mtu);
-   kfree_skb(skb);
+   goto out;
}
+
+   return;
+
+out:
+   kfree_skb(skb);
}

static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] PCI: Relax function 0 VPD test and relocate

[Rustad, Mark D]

> On Sep 15, 2015, at 9:24 PM, Alex Williamson  
> wrote:
> 
> When we quirk a device with PCI_DEV_FLAGS_VPD_REF_F0 we're expecting
> to find a device where all the functions are identical.  If we don't
> find that, we don't make VPD accessible through pci_vpd_ops.  That
> means that if we quirk devices we shouldn't, we filter them out by
> hiding VPD entirely rather than allowing default access.  Instead, we
> can flip this around to only quirk devices that match a slightly more
> rigorous test in the quirk, allowing regular access for anything else.
> 
> Tests for the multifunction flag are removed since a) function 0 and
> the function under test are clearly a multifunction device if we're
> scanning a non-zero function in the same slot and b) at this point the
> flag is only set in the device under test if the multifunction bit is
> set in the PCI HEADER, which is a point of interpretation for the PCI
> spec.
> 
> Signed-off-by: Alex Williamson 
> ---
> 
> This is potentially another stable candiate since we're continuing to
> iterate on 932c435caba8, but since we don't actually know of a device
> where VPD is blocked (we don't think my Skylake example actually
> supports VPD), I'm not including it.  I would support it if requested
> though.

This looks good to me. I can't really test the cases it addresses, but it seems 
reasonable.
Acked-by: Mark Rustad 

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] PCI: Fix devfn for VPD access through function 0

[Rustad, Mark D]

> On Sep 15, 2015, at 10:17 AM, Alex Williamson  
> wrote:
> 
> Commit 932c435caba8 ("PCI: Add dev_flags bit to access VPD through
> function 0") passes PCI_SLOT(devfn) for the devfn parameter of
> pci_get_slot().  Generally this works because we're fairly well
> guaranteed that a PCIe device is at slot address 0, but for the
> general case, including conventional PCI, it's incorrect.  We need
> to get the slot and then convert it back into a devfn.
> 
> Fixes: 932c435caba8 ("PCI: Add dev_flags bit to access VPD through function 
> 0")
> Signed-off-by: Alex Williamson 
> Cc: sta...@vger.kernel.org
> ---
> 
> Since the original patch and quirk was marked for stable and applies
> to all Intel NICs, regardless of the bus type, I assume this needs to
> chase it or VPD might disappear on e1000/e100 if it currently exists.

This looks good to me.
Acked-by: Mark Rustad 

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] PCI: Fix devfn for VPD access through function 0

[Rustad, Mark D]

> On Sep 24, 2015, at 11:27 AM, Bjorn Helgaas  wrote:
> 
> Applied to for-linus for v4.3 with acks from Myron & Mark, thanks!
> 
> I removed the stable tag because 932c435caba8 first appeared in
> v4.3-rc1, so it shouldn't appear in any stable kernels yet.  Right?

I have seen the patch in some stable code reviews already. I know it was 
deferred from one, but I see it in the changelogs for 4.1.8 and 4.2.1. I have 
lost track if it is in any others.

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 3.12 00/33] 3.12.48-stable review

[Rustad, Mark D]

> On Sep 18, 2015, at 1:11 AM, Jiri Slaby  wrote:
> 
> On 09/15/2015, 06:12 PM, Shuah Khan wrote:
>> 
>> 
>> Jiri,
>> 
>> I am seeing problems during PCI scans with this patch. I had
>> to boot it in recovery mode once and it is booting fine after
>> that, however, this is a concern
>> 
>> Could these be the reason?
>> 
>> Mark Rustad (2):
>>  PCI: Add dev_flags bit to access VPD through function 0
>>  PCI: Add VPD function 0 quirk for Intel Ethernet devices
> 
> Ok, so I released 3.12.48 without these two, but left them in the queue
> for the next release as they don't look as they caused the issue. If you
> can confirm whether they introduce the problem or not, it would be great.

I haven't been able to see a way for the patches to be involved, but I have no 
objection to deferring these. My only concern is what the cause actually was.

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] mwifiex: fix possible NULL dereference

[Rustad, Mark D]

Andy Shevchenko  wrote:


On Mon, Apr 11, 2016 at 6:27 PM, Sudip Mukherjee
 wrote:

From: Sudip Mukherjee 

We have a check for card just after dereferencing it. So if it is NULL
we have already dereferenced it before its check. Lets dereference it
after checking card for NULL.


IIUC the code does nothing with dereference.

I would have told NAK if I would have been maintainer.


Signed-off-by: Sudip Mukherjee 
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c  
b/drivers/net/wireless/marvell/mwifiex/pcie.c

index edf8b07..84562d0 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2884,10 +2884,11 @@ static void mwifiex_unregister_dev(struct  
mwifiex_adapter *adapter)

 {
struct pcie_service_card *card = adapter->card;


Let's say it's 0.


   const struct mwifiex_pcie_card_reg *reg;
-   struct pci_dev *pdev = card->dev;


This would be equal to offset of dev member in pcie_service_card struct.

Nothing wrong here.


Actually, that is not true. The dereference of card tells the compiler that  
card can't be NULL, so it is free to eliminate the check below.  
Unbelievably, this can even happen for a reference such as >thing  
where the pointer isn't even actually dereferenced at all!



+   struct pci_dev *pdev;
int i;

if (card) {
+   pdev = card->dev;
if (card->msix_enable) {
for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
synchronize_irq(card->msix_entries[i].vector);
--
1.9.1




--
With Best Regards,
Andy Shevchenko



--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero

[Rustad, Mark D]

Denys Vlasenko  wrote:


Users report that under VMWare, er32(TIMINCA) returns zero.
This causes division by zero at init time as follows:

 ==>incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
/* latch SYSTIMH on read of SYSTIML */
systim_next = (cycle_t)er32(SYSTIML);
systim_next |= (cycle_t)er32(SYSTIMH) << 32;

time_delta = systim_next - systim;
temp = time_delta;
 >  rem = do_div(temp, incvalue);

This change makes kernel survive this, and users report that
NIC does work after this change.

Since on real hardware incvalue is never zero, this should not affect
real hardware use case.

Signed-off-by: Denys Vlasenko 
CC: Jeff Kirsher 
CC: "Ruinskiy, Dima" 
CC: intel-wired-...@lists.osuosl.org
CC: net...@vger.kernel.org
CC: LKML 
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c  
b/drivers/net/ethernet/intel/e1000e/netdev.c

index 269087c..0626935 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4315,7 +4315,8 @@ static cycle_t e1000e_cyclecounter_read(const  
struct cyclecounter *cc)


time_delta = systim_next - systim;
temp = time_delta;
-   rem = do_div(temp, incvalue);
+   /* VMWare users have seen incvalue of zero, don't div / 
0 */
+   rem = incvalue ? do_div(temp, incvalue) : (time_delta 
!= 0);

systim = systim_next;



I seem to recall that this was rejected before because it really is  
VMWare's bug and, if they fix it, any existing VMs that use this will just  
work. Changing the driver will only fix it for vms that install a new  
driver. I don't object to doing it, it just seems like not the most  
effective place to address the issue.


--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH net-next 2/2] intel: ixgbevf: Support Windows hosts (Hyper-V)

[Rustad, Mark D]

Some comments below:

K. Y. Srinivasan  wrote:


On Hyper-V, the VF/PF communication is a via software mediated path
as opposed to the hardware mailbox. Make the necessary
adjustments to support Hyper-V.

Signed-off-by: K. Y. Srinivasan 
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h  |   11 ++
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |   56 ++---
 drivers/net/ethernet/intel/ixgbevf/mbx.c  |   12 ++
 drivers/net/ethernet/intel/ixgbevf/vf.c   |  138 +
 drivers/net/ethernet/intel/ixgbevf/vf.h   |2 +
 5 files changed, 201 insertions(+), 18 deletions(-)




diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c  
b/drivers/net/ethernet/intel/ixgbevf/vf.c

index 4d613a4..92397fd 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.c
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.c





@@ -126,6 +133,23 @@ static s32 ixgbevf_reset_hw_vf(struct ixgbe_hw *hw)
 }

 /**
+ * Hyper-V variant; the VF/PF communication is through the PCI
+ * config space.
+ */
+static s32 ixgbevf_hv_reset_hw_vf(struct ixgbe_hw *hw)
+{
+   int i;
+   struct ixgbevf_adapter *adapter = hw->back;


The two lines above should be swapped so that the longer one is first.


+
+   for (i = 0; i < 6; i++)
+   pci_read_config_byte(adapter->pdev,
+(i + IXGBE_HV_RESET_OFFSET),
+>mac.perm_addr[i]);
+
+   return 0;
+}
+
+/**
  *  ixgbevf_stop_hw_vf - Generic stop Tx/Rx units
  *  @hw: pointer to hardware structure
  *
@@ -656,6 +680,68 @@ out:
 }

 /**
+ * Hyper-V variant; there is no mailbox communication.
+ */
+static s32 ixgbevf_hv_check_mac_link_vf(struct ixgbe_hw *hw,
+   ixgbe_link_speed *speed,
+   bool *link_up,
+   bool autoneg_wait_to_complete)
+{
+   struct ixgbe_mbx_info *mbx = >mbx;
+   struct ixgbe_mac_info *mac = >mac;
+   s32 ret_val = 0;


ret_val here is redundant as this is the only assignment to it.
Please delete it and just return 0 at the end.


+   u32 links_reg;
+
+   /* If we were hit with a reset drop the link */
+   if (!mbx->ops.check_for_rst(hw) || !mbx->timeout)
+   mac->get_link_status = true;
+
+   if (!mac->get_link_status)
+   goto out;
+
+   /* if link status is down no point in checking to see if pf is up */
+   links_reg = IXGBE_READ_REG(hw, IXGBE_VFLINKS);
+   if (!(links_reg & IXGBE_LINKS_UP))
+   goto out;
+
+   /* for SFP+ modules and DA cables on 82599 it can take up to 500usecs
+* before the link status is correct
+*/
+   if (mac->type == ixgbe_mac_82599_vf) {
+   int i;
+
+   for (i = 0; i < 5; i++) {
+   udelay(100);
+   links_reg = IXGBE_READ_REG(hw, IXGBE_VFLINKS);
+
+   if (!(links_reg & IXGBE_LINKS_UP))
+   goto out;
+   }
+   }
+
+   switch (links_reg & IXGBE_LINKS_SPEED_82599) {
+   case IXGBE_LINKS_SPEED_10G_82599:
+   *speed = IXGBE_LINK_SPEED_10GB_FULL;
+   break;
+   case IXGBE_LINKS_SPEED_1G_82599:
+   *speed = IXGBE_LINK_SPEED_1GB_FULL;
+   break;
+   case IXGBE_LINKS_SPEED_100_82599:
+   *speed = IXGBE_LINK_SPEED_100_FULL;
+   break;
+   }
+
+   /* if we passed all the tests above then the link is up and we no
+* longer need to check for link
+*/
+   mac->get_link_status = false;
+
+out:
+   *link_up = !mac->get_link_status;
+   return ret_val;


Just return 0 above.


+}
+
+/**
  *  ixgbevf_rlpml_set_vf - Set the maximum receive packet length
  *  @hw: pointer to the HW structure
  *  @max_size: value to assign to max frame size





@@ -663,6 +749,19 @@ out:
 void ixgbevf_rlpml_set_vf(struct ixgbe_hw *hw, u16 max_size)
 {
u32 msgbuf[2];
+   u32 reg;
+
+   if (x86_hyper == _hyper_ms_hyperv) {
+   /*
+* If we are on Hyper-V, we implement


Please format the comment above netdev-style, /* If we are...
as you did elsewhere.


+* this functionality differently.
+*/
+   reg =  IXGBE_READ_REG(hw, IXGBE_VFRXDCTL(0));
+   /* CRC == 4 */
+   reg |= ((max_size + 4) | IXGBE_RXDCTL_RLPML_EN);
+   IXGBE_WRITE_REG(hw, IXGBE_VFRXDCTL(0), reg);
+   return;
+   }

msgbuf[0] = IXGBE_VF_SET_LPE;
msgbuf[1] = max_size;
@@ -679,6 +778,16 @@ int ixgbevf_negotiate_api_version(struct ixgbe_hw  
*hw, int api)

int err;
u32 msg[3];

+   if (x86_hyper == _hyper_ms_hyperv) {
+   /*
+* Hyper-V only supports api version ixgbe_mbox_api_10


Again, please use netdev-style 

Re: [PATCH net-next 2/2] intel: ixgbevf: Support Windows hosts (Hyper-V)

[Rustad, Mark D]

KY Srinivasan <k...@microsoft.com> wrote:





-Original Message-
From: Rustad, Mark D [mailto:mark.d.rus...@intel.com]
Sent: Thursday, April 14, 2016 4:01 PM
To: KY Srinivasan <k...@microsoft.com>
Cc: David Miller <da...@davemloft.net>; netdev
<net...@vger.kernel.org>; linux-kernel@vger.kernel.org;
de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
jasow...@redhat.com; e...@mellanox.com; ja...@mellanox.com;
yevge...@mellanox.com; Ronciak, John <john.ronc...@intel.com>; intel-
wired-...@linuxonhyperv.com
Subject: Re: [PATCH net-next 2/2] intel: ixgbevf: Support Windows hosts
(Hyper-V)

Some comments below:


Mark,

Thank you for the comments. I will address them and repost the patches.

Regards,

K. Y


Please look closely at Alex's comments. I think they are much more important.

--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH net-next 2/2] intel: ixgbevf: Support Windows hosts (Hyper-V)

[Rustad, Mark D]

KY Srinivasan <k...@microsoft.com> wrote:





-Original Message-
From: Rustad, Mark D [mailto:mark.d.rus...@intel.com]
Sent: Thursday, April 14, 2016 5:07 PM
To: KY Srinivasan <k...@microsoft.com>
Cc: David Miller <da...@davemloft.net>; netdev
<net...@vger.kernel.org>; linux-kernel@vger.kernel.org;
de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
jasow...@redhat.com; e...@mellanox.com; ja...@mellanox.com;
yevge...@mellanox.com; Ronciak, John <john.ronc...@intel.com>; intel-
wired-...@linuxonhyperv.com
Subject: Re: [PATCH net-next 2/2] intel: ixgbevf: Support Windows hosts
(Hyper-V)

KY Srinivasan <k...@microsoft.com> wrote:


-----Original Message-
From: Rustad, Mark D [mailto:mark.d.rus...@intel.com]
Sent: Thursday, April 14, 2016 4:01 PM
To: KY Srinivasan <k...@microsoft.com>
Cc: David Miller <da...@davemloft.net>; netdev
<net...@vger.kernel.org>; linux-kernel@vger.kernel.org;
de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
jasow...@redhat.com; e...@mellanox.com; ja...@mellanox.com;
yevge...@mellanox.com; Ronciak, John <john.ronc...@intel.com>;

intel-

wired-...@linuxonhyperv.com
Subject: Re: [PATCH net-next 2/2] intel: ixgbevf: Support Windows hosts
(Hyper-V)

Some comments below:


Mark,

Thank you for the comments. I will address them and repost the patches.

Regards,

K. Y


Please look closely at Alex's comments. I think they are much more
important.


I am looking at Alex's comments as I am writing this.

K. Y

--
Mark Rustad, Networking Division, Intel Corporation


Can you please stop putting that crazy intel-wired-...@linuxonhyperv.com in  
your messages? It doesn't exist.


--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [Intel-wired-lan] [PATCH net] e1000e: fix PTP on e1000_pch_lpt variants

[Rustad, Mark D]

Jarod Wilson  wrote:


I've got reports that the Intel I-218V NIC in Intel NUC5i5RYH systems used
as a PTP slave experiences random ~10 hour clock jumps, which are resolved
if the same workaround for the 82574 and 82583 is employed. Switching from
an if to a select, because the list of NIC types could well grow further
and we'd already have to wrap the conditionals.

CC: Jeff Kirsher 
CC: intel-wired-...@lists.osuosl.org
CC: net...@vger.kernel.org
Signed-off-by: Jarod Wilson 
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c  
b/drivers/net/ethernet/intel/e1000e/netdev.c

index 2b2e2f8..866fea0 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4335,7 +4335,10 @@ static cycle_t e1000e_cyclecounter_read(const  
struct cyclecounter *cc)

systim = (cycle_t)systimel;
systim |= (cycle_t)systimeh << 32;

-   if ((hw->mac.type == e1000_82574) || (hw->mac.type == e1000_82583)) {
+   switch (hw->mac.type) {
+   case e1000_82574:
+   case e1000_82583:
+   case e1000_pch_lpt:
u64 time_delta, rem, temp;
u32 incvalue;
int i;


I don't think that it is acceptable to declare local variables inside a  
switch statement quite like this. At a minimum, a new block needs to be  
opened to allow the declarations.


@@ -4360,6 +4363,9 @@ static cycle_t e1000e_cyclecounter_read(const  
struct cyclecounter *cc)

(rem == 0))
break;
}
+   break;
+   default:
+   break;
}
return systim;
 }


--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCHv2 3/4] pci: Determine actual VPD size on first access

[Rustad, Mark D]

Benjamin Herrenschmidt  wrote:


We may want some kind of "strict" vs. "relaxed" model here to
differenciate the desktop user wanting to give a function to his/her
windows partition and doesn't care about strict isolation vs. the cloud
data center.


I don't think desktop users appreciate hangs any more than anyone else, and  
that is one of the symptoms that can arise here without the vfio  
coordination.


--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCHv2 3/4] pci: Determine actual VPD size on first access

[Rustad, Mark D]

Benjamin Herrenschmidt  wrote:


Filtering things to work around bugs in existing guests to avoid crashes
is a different kettle of fish and could be justified but keep in mind that
in most cases a malicious guest will be able to exploit those HW flaws.


Bugs in existing guests is an interesting case, but I have been focused on  
getting acceptable behavior from a properly functioning guest, in the face  
of hardware issues that can only be resolved in a single place.


I agree that a malicious guest can cause all kinds of havoc with  
directly-assigned devices. Consider a 4-port PHY chip on a shared MDIO bus,  
for instance. There is really nothing to be done about the potential for  
mischief with that kind of thing.


The VPD problem that I had been concerned about arises from a bad design in  
the PCI spec together with implementations that share the registers across  
functions. The hardware isn't going to change and I really doubt that the  
spec will either, so we address it the only place we can.


I am certain that we agree that not everything can or should be addressed  
in vfio. I did not mean to suggest it should try to address everything, but  
I think it should make it possible for correctly behaving guests to work. I  
think that is not unreasonable.


Perhaps the VPD range check should really just have been implemented for  
the sysfs interface, and left the vfio case unchecked. I don't know because  
I was not involved in that issue. Perhaps someone more intimately involved  
can comment on that notion.



Assuming that a device coming back from a guest is functional and not
completely broken and can be re-used without a full PERST or power cycle
is a wrong assumption. It may or may not work, no amount of "filtering"
will fix the fundamental issue. If your HW won't give you access to PERST
well ... blame Intel for not specifying a standard way to generate it in
the first place :-)


Yeah, I worry about the state that a malicious guest could leave a device  
in, but I consider direct assignment always risky anyway. I would just like  
it to at least work in the non-malicious guest cases.


I guess my previous response was really just too terse, I was just focused  
on unavoidable hangs and data corruption, which even were happening without  
any guest involvement. For me, guests were just an additional exposure of  
the same underlying issue.


With hindsight, it is easy to see that a standard reset would now be a  
pretty useful thing. I am sure that even if it existed, we would now have  
lots and lots of quirks around it as well! :-)


--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH v3] net: ip, diag -- Add diag interface for raw sockets

[Rustad, Mark D]

Greg  wrote:


Someday Linux will be a modern OS that just includes IPV6 and forces a
config option to NOT have it.

That'll be great.  All the IS_ENABLED_(CONFIG_IPV6) scattered everywhere
is nuts.




Better wait until everyone at least *has* IPv6! I have yet to have IPv6  
deployed on any of my employer's networks or get IPv6 service from any ISP  
at my home. When I was at Apple in the 90's I was told that Apple needed  
IPv6 by next year or "we were dead". Well Apple nearly died, but IPv6 had  
nothing to do with that! And I still haven't experienced an IPv6  
deployment! Yeah, I have run it a bit point-to-point to resolve technical  
issues, but that isn't a "deployment" and not very interesting.


As much as we would like things to move faster, much of the world just  
doesn't. Witness the e1000 discussion today for example. Hardware doesn't  
vanish overnight, and I know that my ISP has a network full of CPE that  
doesn't do IPv6, so I'm not expecting their status to change any time soon.


It would be great though.


--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [Intel-wired-lan] [PATCH 1/1] ixgbe: fcoe: return value of skb_linearize should be handled

[Rustad, Mark D]

Zhouyi Zhou  wrote:

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  
b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

index fee1f29..4926d48 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -2173,8 +2173,7 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector  
*q_vector,

total_rx_bytes += ddp_bytes;
total_rx_packets += DIV_ROUND_UP(ddp_bytes,
 mss);
-   }
-   if (!ddp_bytes) {
+   } else {
dev_kfree_skb_any(skb);
continue;
}


This is changing the logic by treating a negative ddp_bytes value (an error  
return) the same as a 0 value. This is probably wrong and inappropriate for  
this patch in any case.


--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] x86-32: fix tlb flushing when lguest clears PGE

[Rustad, Mark D]

On Mar 12, 2017, at 7:02 PM, Kees Cook  wrote:

> Are there nominations for most comprehensive changelog of the year? :)
> This is awesome.

Especially for a one-liner! Truly comprehensive and completely relevant.

--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP

Re: [PATCH V4 net-next 7/8] net: hns3: Add Ethtool support to HNS3 driver

[Rustad, Mark D]

> On Jul 23, 2017, at 10:05 AM, Florian Fainelli  wrote:
>> +
>> +strncpy(drvinfo->version, HNAE_DRIVER_VERSION,
>> +sizeof(drvinfo->version));
>> +drvinfo->version[sizeof(drvinfo->version) - 1] = '\0';
> 
> strlcpy() would probably do that for you.

You need to be careful about strlcpy - it does not completely write the 
destination buffer as strncpy does, and so can result in a kernel memory leak 
if the destination is not known to already be cleared.

>> +
>> +strncpy(drvinfo->driver, HNAE_DRIVER_NAME, sizeof(drvinfo->driver));
>> +drvinfo->driver[sizeof(drvinfo->driver) - 1] = '\0';
> 
> Same here

Same here

>> +
>> +strncpy(drvinfo->bus_info, priv->dev->bus->name,
>> +sizeof(drvinfo->bus_info));> +  
>> drvinfo->bus_info[ETHTOOL_BUSINFO_LEN - 1] = '\0';
> 
> And here.

And here too. I haven't looked at this deeply enough to know whether a leak 
could be created by strlcpy here, but I wanted to raise it as something to be 
considered before switching to it. Blindly adopting strlcpy is hazardous as are 
tools that unconditionally recommend it.

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP

Re: [PATCH V2] r8152: add Linksys USB3GIGV1 id

[Rustad, Mark D]

> On Sep 27, 2017, at 9:39 AM, Grant Grundler  wrote:
> 
> On Wed, Sep 27, 2017 at 12:15 AM, Oliver Neukum  wrote:
>> Am Dienstag, den 26.09.2017, 08:19 -0700 schrieb Doug Anderson:
>>> 
>>> I know that for at least some of the adapters in the CDC Ethernet
>>> blacklist it was claimed that the CDC Ethernet support in the adapter
>>> was kinda broken anyway so the blacklist made sense.  ...but for the
>>> Linksys Gigabit adapter the CDC Ethernet driver seems to work OK, it's
>>> just not quite as full featured / efficient as the R8152 driver.
>>> 
>>> Is that not a concern?  I guess you could tell people in this
>>> situation that they simply need to enable the R8152 driver to get
>>> continued support for their Ethernet adapter?
>> 
>> Hi,
>> 
>> yes, it is a valid concern. An #ifdef will be needed.
> 
> Good idea - I will post V3 shortly.
> 
> I'm assuming you mean to add #ifdef CONFIG_USB_RTL8152 around the
> blacklist entry in cdc_ether driver.

Shouldn't that be an #if IS_ENABLED(...) test, since that seems to be the 
proper way to check configured drivers.

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP

Re: netlink backwards compatibility in userspace tools

[Rustad, Mark D]

> On Sep 29, 2017, at 3:22 AM, Jason A. Donenfeld  wrote:
> 
> Hi guys,
> 
> One handy aspect of Netlink is that it's backwards compatible. This
> means that you can run old userspace utilities on new kernels, even if
> the new kernel supports new features and netlink attributes. The wire
> format is stable enough that the data marshaled can be extended
> without breaking compat. Neat.
> 
> I was wondering, though, what you think the best stance is toward
> these old userspace utilities. What should they do if the kernel sends
> it netlink attributes that it does not recognize? At the moment, I'm
> doing something like this:
> 
> static void warn_unrecognized(void)
> {
>static bool once = false;
>if (once)
>return;
>once = true;
>fprintf(stderr,
>"Warning: this program received from your kernel one or more\n"
>"attributes that it did not recognize. It is possible that\n"
>"this version of wg(8) is older than your kernel. You may\n"
>"want to update this program.\n");
> }
> 
> This seems like a somewhat sensible warning, but then I wonder about
> distributions like Debian, which has a long stable life cycle, so it
> frequently has very old tools (ancient iproute2 for example). Then,
> VPS providers have these Debian images run on top of newer kernels.
> People in this situation would undoubtedly see the above warning a lot
> and not be able to do anything about it. Not horrible, but a bit
> annoying. Is this an okay annoyance? Or is it advised to just have no
> warning at all? One idea would be to put it behind an environment
> variable flag, but I don't like too many nobs.
> 
> I'm generally wondering about attitudes toward this kind of userspace
> program behavior in response to newer kernels.
> 
> Thanks,
> Jason

That seems like a bit much. Consider only emitting a message with the use of a 
verbose flag - or two. Even then the message should be shortened - the first 
sentence is entirely adequate even in verbose mode.

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP

Re: [RFC PATCH V2] virtio_pci: Add SR-IOV support

[Rustad, Mark D]

> On Feb 22, 2018, at 10:26 AM, Christoph Hellwig  wrote:
> 
> Can we move this into common code as a a generic_sriov_configure
> helper?  Nothing is really virtio specific, and it seems like
> some other drivers could also use it, e.g. ena or nvme.

That seems like a good idea to me, especially if PCI developers concur.

-- 
Mark Rustad, Networking Division, Intel Corporation

Re: [RFC PATCH V4] pci: virtio_pci: Add SR-IOV support for virtio_pci devices

[Rustad, Mark D]

Alex,

> On Feb 26, 2018, at 7:26 AM, Alexander Duyck  
> wrote:
> 
> Mark,
> 
> In the future please don't put my "Reviewed-by" on a patch that I
> haven't reviewed. I believe I reviewed one of the earlier patches, but
> I hadn't reviewed this version.

I'm very sorry. I completely spaced doing something about that. I think yours 
was the first Reviewed-by I ever had in this way. In the future I will remove 
such things from my changelog right after sending. Thanks for alerting me to 
what I had failed to do.

> Also, after thinking about it over the weekend we may want to look at
> just coming up with a truly "generic" solution that is applied to
> SR-IOV capable devices that don't have a SR-IOV capable driver loaded
> on them. That would allow us to handle the uio, vfio, pci-stub, and
> virtio cases all in one fell swoop. I think us going though and
> modifying one patch at a time to do this kind of thing isn't going to
> scale.

The notion of that kind of troubles me - at least pci-stub does. Having worked 
on ixgbe a bit, I have to wonder what kind of havoc would ensue if an ixgbe 
device were assigned to a guest, and an attempt was made to allocate VFs by the 
pci-stub. The guest could be running any version of the ixgbe driver, possibly 
even an old one that didn't support SR-IOV. Even if it did support SR-IOV, I 
don't know how it would respond to mailbox messages when it doesn't think it 
has VFs.

> I'll try to do some digging and find the VFIO approach we had been
> working on. I think with a couple tweaks we can probably make that
> truly generic and ready for submission.

I'd like to know more about you are thinking about.

-- 
Mark Rustad, Networking Division, Intel Corporation

Re: [pci PATCH v7 2/5] virtio_pci: Add support for unmanaged SR-IOV on virtio_pci devices

[Rustad, Mark D]

On Mar 15, 2018, at 11:42 AM, Alexander Duyck  wrote:

> From: Alexander Duyck 
> 
> Hardware-realized virtio_pci devices can implement SR-IOV, so this
> patch enables its use. The device in question is an upcoming Intel
> NIC that implements both a virtio_net PF and virtio_net VFs. These
> are hardware realizations of what has been up to now been a software
> interface.
> 
> The device in question has the following 4-part PCI IDs:
> 
> PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe
> VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe
> 
> The patch currently needs no check for device ID, because the callback
> will never be made for devices that do not assert the capability or
> when run on a platform incapable of SR-IOV.
> 
> One reason for this patch is because the hardware requires the
> vendor ID of a VF to be the same as the vendor ID of the PF that
> created it. So it seemed logical to simply have a fully-functioning
> virtio_net PF create the VFs. This patch makes that possible.
> 
> Reviewed-by: Christoph Hellwig 
> Signed-off-by: Mark Rustad 
> Signed-off-by: Alexander Duyck 
> ---
> 
> v4: Dropped call to pci_disable_sriov in virtio_pci_remove function
> v5: Replaced call to pci_sriov_configure_unmanaged with
> pci_sriov_configure_simple
> v6: Dropped "#ifdef" checks for IOV wrapping sriov_configure definition
> v7: No code change, added Reviewed-by
> 
>  drivers/virtio/virtio_pci_common.c |1 +
>  1 file changed, 1 insertion(+)

Tested with the identified device.

Tested-by: Mark Rustad 

--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP

Re: [virtio-dev] [pci PATCH v7 1/5] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources

[Rustad, Mark D]

On Mar 15, 2018, at 11:41 AM, Alexander Duyck  wrote:

> From: Alexander Duyck 
> 
> This patch adds a common configuration function called
> pci_sriov_configure_simple that will allow for managing VFs on devices
> where the PF is not capable of managing VF resources.
> 
> Signed-off-by: Alexander Duyck 
> ---
> 
> v5: New patch replacing pci_sriov_configure_unmanaged with
>   pci_sriov_configure_simple
> Dropped bits related to autoprobe changes
> v6: Defined pci_sriov_configure_simple as NULL if IOV is disabled
> v7: Updated pci_sriov_configure_simple to drop need for err value
> Fixed comment explaining why pci_sriov_configure_simple is NULL
> 
>  drivers/pci/iov.c   |   31 +++
>  include/linux/pci.h |3 +++
>  2 files changed, 34 insertions(+)

Tested with the device identified in patch #2.

Tested-by: Mark Rustad 

--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP

Re: [virtio-dev] [pci PATCH v7 2/5] virtio_pci: Add support for unmanaged SR-IOV on virtio_pci devices

[Rustad, Mark D]

On Apr 3, 2018, at 11:27 AM, Michael S. Tsirkin  wrote:


I'm not sure why you would need a feature bit. The capability is
controlled via PCI configuration space. If it is present the device
has the capability. If it is not then it does not.

Basically if the PCI configuration space is not present then the sysfs
entries will not be spawned and nothing will attempt to use this
function.

- ALex


It's about compability with older guests which ignore the
capability.

The feature is thus helpful so host knows whether guest supports VFs.


This is not about a guest creating its own VFs. This is about a host PF  
that happens to have a virtio interface to be able to create virtio VFs  
that can be assigned to guests. Nothing changes at all from a guest  
perspective. Or maybe I am not understanding what you mean by "whether  
guest supports VFs".


--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP

Re: [RFC PATCH V3] virtio_pci: Add SR-IOV support

[Rustad, Mark D]

> On Feb 27, 2018, at 7:35 AM, David Miller  wrote:
> 
> I don't like these helpers on many different levels.



> So kill off pci_sriov_enable() helper completely, it is unnecessary,
> and rename the disable helper so that it says something meaningful to
> the reader.

Yes. Once pointed out, I completely agree with your comments and wish that I 
had seen those things myself.

> Thanks.

V3 was junk, but your comments apply to V4 as well, so please ignore it.

Thank you for your valuable review.

-- 
Mark Rustad, Networking Division, Intel Corporation

Re: [PATCH] rtnetlink: Call nlmsg_parse() with correct header length

[Rustad, Mark D]

On Apr 8, 2013, at 8:45 AM, Michael Riesch  wrote:

> 
> Signed-off-by: Michael Riesch 
> Cc: "David S. Miller" 
> Cc: Greg Kroah-Hartman 
> Cc: Jiri Benc 
> Cc: "Theodore Ts'o" 
> Cc: linux-kernel@vger.kernel.org
> ---
> Habidere,
> 
> I encountered a netlink kernel warning when running avahi 0.6.31 on my system
> with kernel v3.4.35 (it appears several times):
> 
>   netlink: 12 bytes leftover after parsing attributes.
> 
> Searching the web showed that commit 
> "115c9b81928360d769a76c632bae62d15206a94a 
> rtnetlink: Fix problem with buffer allocation" introduced this behaviour[1].
> 
> Now I - knowing nothing about netlink whatsoever - assume that the nlmsg_parse
> function is called with the wrong header length. In user space the request
> message consists out of the message header (struct nlmsghdr, 16 bytes) and an
> ifinfomsg (struct ifinfomsg, 16 bytes). After that, request attributes could
> follow. nlmsg_parse checks for this attributes after a given header length. In
> rtnl_get_link() this header length is sizeof(struct ifinfomsg), but in
> rtnl_calcit() as well as in rntl_dump_ifinfo() the header length is
> sizeof(struct rtgenmsg), which is 1 byte.
> 
> With this patch I got rid of these warnings. However, I do not know whether
> this is the correct solution, so I am looking forward to your comments.
> Regards, Michael
> 
> [1] http://lists.infradead.org/pipermail/libnl/2012-April/000515.html
> 
> net/core/rtnetlink.c |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 900fc61..ebf6ace 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1065,7 +1065,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct 
> netlink_callback *cb)
>   rcu_read_lock();
>   cb->seq = net->dev_base_seq;
> 
> - if (nlmsg_parse(cb->nlh, sizeof(struct rtgenmsg), tb, IFLA_MAX,
> + if (nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb, IFLA_MAX,
>   ifla_policy) >= 0) {
> 
>   if (tb[IFLA_EXT_MASK])
> @@ -1909,7 +1909,7 @@ static u16 rtnl_calcit(struct sk_buff *skb, struct 
> nlmsghdr *nlh)
>   u32 ext_filter_mask = 0;
>   u16 min_ifinfo_dump_size = 0;
> 
> - if (nlmsg_parse(nlh, sizeof(struct rtgenmsg), tb, IFLA_MAX,
> + if (nlmsg_parse(nlh, sizeof(struct ifinfomsg), tb, IFLA_MAX,
>   ifla_policy) >= 0) {
>   if (tb[IFLA_EXT_MASK])
>   ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]);
> -- 
> 1.7.9.5


I found that fcoemon has also been triggering these messages for some time. I 
found the same problem and arrived at exactly the same solution. I would have 
already sent it, but it is still in validation. As far as I am concerned:

Acked-by: Mark Rustad 

-- 
Mark Rustad, Networking Division, Intel Corporation--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [E1000-devel] [PATCH] net: ethernet: intel: ixgbe: ixgbe_main.c: Cleaning up missing null-terminate after strncpy call

[Rustad, Mark D]

On Jun 4, 2014, at 2:55 PM, Joe Perches  wrote:

> On Wed, 2014-06-04 at 23:29 +0200, Rickard Strandqvist wrote:
>> Added a guaranteed null-terminate after call to strncpy.
> 
> Perhaps all of these should be strlcpy

The code that is there seems fine. The length of the array exceeds the length 
of the literal, and the strncpy ensures that the entire buffer is initialized 
so no information can possibly leak from the kernel.

I think this is fine as it is without any patch.

-- 
Mark Rustad, Networking Division, Intel Corporation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/8] ixgbe: Out of line ixgbe_read/write_reg

[Rustad, Mark D]

On May 16, 2014, at 2:43 PM, Andi Kleen  wrote:

> From: Andi Kleen 
> 
> ixgbe_read_reg and ixgbe_write_reg are frequently called and are very big
> because they have complex error handling code.

Actually, this patch doesn't do anything to ixgbe_write_reg, which would almost 
certainly be very bad for performance, but instead changes ixgbe_write_reg64. 
The latter is not in a performance-sensitive path, but is only called from one 
site, so there is little reason to take it out-of-line.

I already have a patch in queue to make ixgbe_read_reg out-of-line, because it 
does have a very costly memory footprint inline, as you have found.

> Moving them out of line saves ~27k text in the ixgbe driver.
> 
>   text   data bss dec hex filename
> 14220873  2008072 1507328 1773627310ea251 vmlinux-before-ixgbe
> 14193673  2003976 1507328 1770497710e2811 vmlinux-ixgbe
> 
> Cc: net...@vger.kernel.org
> Cc: Jeff Kirsher 
> Signed-off-by: Andi Kleen 
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe_common.h | 22 ++
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c   | 22 ++
> 2 files changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h 
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
> index f12c40f..05f094d 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
> @@ -162,28 +162,10 @@ static inline void writeq(u64 val, void __iomem *addr)
> }
> #endif
> 
> -static inline void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value)
> -{
> - u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
> +void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value);
> +u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg);
> 
> - if (ixgbe_removed(reg_addr))
> - return;
> - writeq(value, reg_addr + reg);
> -}
> #define IXGBE_WRITE_REG64(a, reg, value) ixgbe_write_reg64((a), (reg), 
> (value))
> -
> -static inline u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg)
> -{
> - u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
> - u32 value;
> -
> - if (ixgbe_removed(reg_addr))
> - return IXGBE_FAILED_READ_REG;
> - value = readl(reg_addr + reg);
> - if (unlikely(value == IXGBE_FAILED_READ_REG))
> - ixgbe_check_remove(hw, reg);
> - return value;
> -}
> #define IXGBE_READ_REG(a, reg) ixgbe_read_reg((a), (reg))
> 
> #define IXGBE_WRITE_REG_ARRAY(a, reg, offset, value) \
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index d62e7a2..5f81f62 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -371,6 +371,28 @@ void ixgbe_write_pci_cfg_word(struct ixgbe_hw *hw, u32 
> reg, u16 value)
>   pci_write_config_word(adapter->pdev, reg, value);
> }
> 
> +void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value)
> +{
> + u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
> +
> + if (ixgbe_removed(reg_addr))
> + return;
> + writeq(value, reg_addr + reg);
> +}
> +
> +u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg)
> +{
> + u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
> + u32 value;
> +
> + if (ixgbe_removed(reg_addr))
> + return IXGBE_FAILED_READ_REG;
> + value = readl(reg_addr + reg);
> + if (unlikely(value == IXGBE_FAILED_READ_REG))
> + ixgbe_check_remove(hw, reg);
> + return value;
> +}
> +
> static void ixgbe_service_event_complete(struct ixgbe_adapter *adapter)
> {
>   BUG_ON(!test_bit(__IXGBE_SERVICE_SCHED, >state));
> -- 
> 1.9.0
> 
> --
> 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

-- 
Mark Rustad, Networking Division, Intel Corporation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/8] ixgbe: Out of line ixgbe_read/write_reg

[Rustad, Mark D]

On May 19, 2014, at 4:25 PM, Andi Kleen  wrote:

> On Mon, May 19, 2014 at 10:00:52PM +0000, Rustad, Mark D wrote:
>> On May 16, 2014, at 2:43 PM, Andi Kleen  wrote:
>> 
>>> From: Andi Kleen 
>>> 
>>> ixgbe_read_reg and ixgbe_write_reg are frequently called and are very big
>>> because they have complex error handling code.
>> 
>> Actually, this patch doesn't do anything to ixgbe_write_reg, which would 
>> almost certainly be very bad for performance, but instead changes 
>> ixgbe_write_reg64.
> 
> I doubt a few cycles around the write make a lot of difference for MMIO. MMIO 
> is dominated
> by other things.
> 
>> The latter is not in a performance-sensitive path, but is only called from 
>> one site, so there is little reason to take it out-of-line.
> 
> True I moved the wrong one.
> 
> ixgbe_write_reg3305  (0.00%)  8 409  
> 
> 
>> I already have a patch in queue to make ixgbe_read_reg out-of-line, because 
>> it does have a very costly memory footprint inline, as you have found.
> 
> Please move write_reg too.

I will take a look at moving most of them out-of-line. There are just a few in 
very hot paths that should remain inline.

-- 
Mark Rustad, Networking Division, Intel Corporation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [virtio-dev] [pci PATCH v7 1/5] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources

[Rustad, Mark D]

On Mar 15, 2018, at 11:41 AM, Alexander Duyck  wrote:

> From: Alexander Duyck 
> 
> This patch adds a common configuration function called
> pci_sriov_configure_simple that will allow for managing VFs on devices
> where the PF is not capable of managing VF resources.
> 
> Signed-off-by: Alexander Duyck 
> ---
> 
> v5: New patch replacing pci_sriov_configure_unmanaged with
>   pci_sriov_configure_simple
> Dropped bits related to autoprobe changes
> v6: Defined pci_sriov_configure_simple as NULL if IOV is disabled
> v7: Updated pci_sriov_configure_simple to drop need for err value
> Fixed comment explaining why pci_sriov_configure_simple is NULL
> 
>  drivers/pci/iov.c   |   31 +++
>  include/linux/pci.h |3 +++
>  2 files changed, 34 insertions(+)

Tested with the device identified in patch #2.

Tested-by: Mark Rustad 

--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP

Re: [pci PATCH v7 2/5] virtio_pci: Add support for unmanaged SR-IOV on virtio_pci devices

[Rustad, Mark D]

On Mar 15, 2018, at 11:42 AM, Alexander Duyck  wrote:

> From: Alexander Duyck 
> 
> Hardware-realized virtio_pci devices can implement SR-IOV, so this
> patch enables its use. The device in question is an upcoming Intel
> NIC that implements both a virtio_net PF and virtio_net VFs. These
> are hardware realizations of what has been up to now been a software
> interface.
> 
> The device in question has the following 4-part PCI IDs:
> 
> PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe
> VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe
> 
> The patch currently needs no check for device ID, because the callback
> will never be made for devices that do not assert the capability or
> when run on a platform incapable of SR-IOV.
> 
> One reason for this patch is because the hardware requires the
> vendor ID of a VF to be the same as the vendor ID of the PF that
> created it. So it seemed logical to simply have a fully-functioning
> virtio_net PF create the VFs. This patch makes that possible.
> 
> Reviewed-by: Christoph Hellwig 
> Signed-off-by: Mark Rustad 
> Signed-off-by: Alexander Duyck 
> ---
> 
> v4: Dropped call to pci_disable_sriov in virtio_pci_remove function
> v5: Replaced call to pci_sriov_configure_unmanaged with
> pci_sriov_configure_simple
> v6: Dropped "#ifdef" checks for IOV wrapping sriov_configure definition
> v7: No code change, added Reviewed-by
> 
>  drivers/virtio/virtio_pci_common.c |1 +
>  1 file changed, 1 insertion(+)

Tested with the identified device.

Tested-by: Mark Rustad 

--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP

Re: [RFC PATCH V2] virtio_pci: Add SR-IOV support

[Rustad, Mark D]

> On Feb 22, 2018, at 10:26 AM, Christoph Hellwig  wrote:
> 
> Can we move this into common code as a a generic_sriov_configure
> helper?  Nothing is really virtio specific, and it seems like
> some other drivers could also use it, e.g. ena or nvme.

That seems like a good idea to me, especially if PCI developers concur.

-- 
Mark Rustad, Networking Division, Intel Corporation

Re: [PATCH V4 net-next 7/8] net: hns3: Add Ethtool support to HNS3 driver

[Rustad, Mark D]

> On Jul 23, 2017, at 10:05 AM, Florian Fainelli  wrote:
>> +
>> +strncpy(drvinfo->version, HNAE_DRIVER_VERSION,
>> +sizeof(drvinfo->version));
>> +drvinfo->version[sizeof(drvinfo->version) - 1] = '\0';
> 
> strlcpy() would probably do that for you.

You need to be careful about strlcpy - it does not completely write the 
destination buffer as strncpy does, and so can result in a kernel memory leak 
if the destination is not known to already be cleared.

>> +
>> +strncpy(drvinfo->driver, HNAE_DRIVER_NAME, sizeof(drvinfo->driver));
>> +drvinfo->driver[sizeof(drvinfo->driver) - 1] = '\0';
> 
> Same here

Same here

>> +
>> +strncpy(drvinfo->bus_info, priv->dev->bus->name,
>> +sizeof(drvinfo->bus_info));> +  
>> drvinfo->bus_info[ETHTOOL_BUSINFO_LEN - 1] = '\0';
> 
> And here.

And here too. I haven't looked at this deeply enough to know whether a leak 
could be created by strlcpy here, but I wanted to raise it as something to be 
considered before switching to it. Blindly adopting strlcpy is hazardous as are 
tools that unconditionally recommend it.

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP

Re: [PATCH V2] r8152: add Linksys USB3GIGV1 id

[Rustad, Mark D]

> On Sep 27, 2017, at 9:39 AM, Grant Grundler  wrote:
> 
> On Wed, Sep 27, 2017 at 12:15 AM, Oliver Neukum  wrote:
>> Am Dienstag, den 26.09.2017, 08:19 -0700 schrieb Doug Anderson:
>>> 
>>> I know that for at least some of the adapters in the CDC Ethernet
>>> blacklist it was claimed that the CDC Ethernet support in the adapter
>>> was kinda broken anyway so the blacklist made sense.  ...but for the
>>> Linksys Gigabit adapter the CDC Ethernet driver seems to work OK, it's
>>> just not quite as full featured / efficient as the R8152 driver.
>>> 
>>> Is that not a concern?  I guess you could tell people in this
>>> situation that they simply need to enable the R8152 driver to get
>>> continued support for their Ethernet adapter?
>> 
>> Hi,
>> 
>> yes, it is a valid concern. An #ifdef will be needed.
> 
> Good idea - I will post V3 shortly.
> 
> I'm assuming you mean to add #ifdef CONFIG_USB_RTL8152 around the
> blacklist entry in cdc_ether driver.

Shouldn't that be an #if IS_ENABLED(...) test, since that seems to be the 
proper way to check configured drivers.

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP

Re: netlink backwards compatibility in userspace tools

[Rustad, Mark D]

> On Sep 29, 2017, at 3:22 AM, Jason A. Donenfeld  wrote:
> 
> Hi guys,
> 
> One handy aspect of Netlink is that it's backwards compatible. This
> means that you can run old userspace utilities on new kernels, even if
> the new kernel supports new features and netlink attributes. The wire
> format is stable enough that the data marshaled can be extended
> without breaking compat. Neat.
> 
> I was wondering, though, what you think the best stance is toward
> these old userspace utilities. What should they do if the kernel sends
> it netlink attributes that it does not recognize? At the moment, I'm
> doing something like this:
> 
> static void warn_unrecognized(void)
> {
>static bool once = false;
>if (once)
>return;
>once = true;
>fprintf(stderr,
>"Warning: this program received from your kernel one or more\n"
>"attributes that it did not recognize. It is possible that\n"
>"this version of wg(8) is older than your kernel. You may\n"
>"want to update this program.\n");
> }
> 
> This seems like a somewhat sensible warning, but then I wonder about
> distributions like Debian, which has a long stable life cycle, so it
> frequently has very old tools (ancient iproute2 for example). Then,
> VPS providers have these Debian images run on top of newer kernels.
> People in this situation would undoubtedly see the above warning a lot
> and not be able to do anything about it. Not horrible, but a bit
> annoying. Is this an okay annoyance? Or is it advised to just have no
> warning at all? One idea would be to put it behind an environment
> variable flag, but I don't like too many nobs.
> 
> I'm generally wondering about attitudes toward this kind of userspace
> program behavior in response to newer kernels.
> 
> Thanks,
> Jason

That seems like a bit much. Consider only emitting a message with the use of a 
verbose flag - or two. Even then the message should be shortened - the first 
sentence is entirely adequate even in verbose mode.

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP

Re: [virtio-dev] [pci PATCH v7 2/5] virtio_pci: Add support for unmanaged SR-IOV on virtio_pci devices

[Rustad, Mark D]

On Apr 3, 2018, at 11:27 AM, Michael S. Tsirkin  wrote:


I'm not sure why you would need a feature bit. The capability is
controlled via PCI configuration space. If it is present the device
has the capability. If it is not then it does not.

Basically if the PCI configuration space is not present then the sysfs
entries will not be spawned and nothing will attempt to use this
function.

- ALex


It's about compability with older guests which ignore the
capability.

The feature is thus helpful so host knows whether guest supports VFs.


This is not about a guest creating its own VFs. This is about a host PF  
that happens to have a virtio interface to be able to create virtio VFs  
that can be assigned to guests. Nothing changes at all from a guest  
perspective. Or maybe I am not understanding what you mean by "whether  
guest supports VFs".


--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP

Re: [RFC PATCH V4] pci: virtio_pci: Add SR-IOV support for virtio_pci devices

[Rustad, Mark D]

Alex,

> On Feb 26, 2018, at 7:26 AM, Alexander Duyck  
> wrote:
> 
> Mark,
> 
> In the future please don't put my "Reviewed-by" on a patch that I
> haven't reviewed. I believe I reviewed one of the earlier patches, but
> I hadn't reviewed this version.

I'm very sorry. I completely spaced doing something about that. I think yours 
was the first Reviewed-by I ever had in this way. In the future I will remove 
such things from my changelog right after sending. Thanks for alerting me to 
what I had failed to do.

> Also, after thinking about it over the weekend we may want to look at
> just coming up with a truly "generic" solution that is applied to
> SR-IOV capable devices that don't have a SR-IOV capable driver loaded
> on them. That would allow us to handle the uio, vfio, pci-stub, and
> virtio cases all in one fell swoop. I think us going though and
> modifying one patch at a time to do this kind of thing isn't going to
> scale.

The notion of that kind of troubles me - at least pci-stub does. Having worked 
on ixgbe a bit, I have to wonder what kind of havoc would ensue if an ixgbe 
device were assigned to a guest, and an attempt was made to allocate VFs by the 
pci-stub. The guest could be running any version of the ixgbe driver, possibly 
even an old one that didn't support SR-IOV. Even if it did support SR-IOV, I 
don't know how it would respond to mailbox messages when it doesn't think it 
has VFs.

> I'll try to do some digging and find the VFIO approach we had been
> working on. I think with a couple tweaks we can probably make that
> truly generic and ready for submission.

I'd like to know more about you are thinking about.

-- 
Mark Rustad, Networking Division, Intel Corporation

Re: [RFC PATCH V3] virtio_pci: Add SR-IOV support

[Rustad, Mark D]

> On Feb 27, 2018, at 7:35 AM, David Miller  wrote:
> 
> I don't like these helpers on many different levels.



> So kill off pci_sriov_enable() helper completely, it is unnecessary,
> and rename the disable helper so that it says something meaningful to
> the reader.

Yes. Once pointed out, I completely agree with your comments and wish that I 
had seen those things myself.

> Thanks.

V3 was junk, but your comments apply to V4 as well, so please ignore it.

Thank you for your valuable review.

-- 
Mark Rustad, Networking Division, Intel Corporation

Re: [PATCH] tcm_fc: rcu_deref outside rcu lock/unlock section

[Rustad, Mark D]

On Aug 18, 2012, at 3:35 PM, Nicholas A. Bellinger wrote:

> On Sat, 2012-08-18 at 16:10 +0400, Denis Efremov wrote:
>> Use rcu_dereference_protected in order to prevent lockdep
>> complaint. Sequel of the patch 863555be
>> 
>> Found by Linux Driver Verification project (linuxtesting.org).
>> 
>> Signed-off-by: Denis Efremov 
>> ---
>> drivers/target/tcm_fc/tfc_sess.c |4 +++-
>> 1 files changed, 3 insertions(+), 1 deletions(-)
>> 
>> diff --git a/drivers/target/tcm_fc/tfc_sess.c 
>> b/drivers/target/tcm_fc/tfc_sess.c
>> index 87901fa..3c9e5b5 100644
>> --- a/drivers/target/tcm_fc/tfc_sess.c
>> +++ b/drivers/target/tcm_fc/tfc_sess.c
>> @@ -456,7 +456,9 @@ static void ft_prlo(struct fc_rport_priv *rdata)
>>  struct ft_tport *tport;
>> 
>>  mutex_lock(_lport_lock);
>> -tport = rcu_dereference(rdata->local_port->prov[FC_TYPE_FCP]);
>> +tport = rcu_dereference_protected(rdata->local_port->prov[FC_TYPE_FCP],
>> +  lockdep_is_held(_lport_lock));
>> +
>>  if (!tport) {
>>  mutex_unlock(_lport_lock);
>>  return;
> 
> This looks OK to silence lockdep.  CC'ing MDR + Kiran for good measure
> here, and will move from target-pending queue -> master with their ACK.
> 
> Thanks Dennis!
> 
> --nab


Ack. In fact I wonder why I didn't encounter it myself.

-- 
Mark Rustad, LAN Access Division, Intel Corporation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 25, 2014, at 12:45 AM, Geert Uytterhoeven  wrote:

> Instead of grepping, you can feed the build log to linux-log-summary.
> Or when changing a driver, feed the before and after build logs to
> linux-log-diff. That way you won't miss the single new warning you've
> just introduced.
> 
> https://github.com/geertu/linux-scripts

Thanks for making me aware of these tools. linux-log-summary does reduce the 
totally insane 125,000 warnings to the merely unreasonable 10,000 or so. I'm 
not being sarcastic, they *are* very nice tools that I will use. Thank you for 
pointing me to them.

I still observe that thousands of those unique warnings are emitted by the 
syscall table and its override initializations. I still think they should be 
silenced because they serve no purpose in that particular case. That is, the 
warning is complaining about what it is designed to complain about, but in this 
particular instance, the code is written as it needs to be, so why not 
acknowledge and capture that exception in the source?

A couple thousand more come from every use of compiletime_assert, for which I 
do not have a solution yet. Even my macros don't help with that one.

Most of the others come from null-entry table initializations, i.e. { 0 }, 
which give missing field initializer warnings. I'd like to define a macro 
something like:

#define ZERO_ENTRY DIAG_PUSH DIAG_IGNORE(missing-field-initializers) \
{ 0 } DIAG_POP

Then simply using ZERO_ENTRY, a zero entry could be provided without a 
complaint from the compiler. But of course that can't be done if the DIAG_* 
macros aren't there. It also would keep the DIAG_* macros out of the .c files 
and just in the header where ZERO_ENTRY is defined.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 26, 2014, at 12:58 PM,   
wrote:

> On Fri, Sep 26, 2014 at 07:37:19PM +0000, Rustad, Mark D wrote:
>> Most of the others come from null-entry table initializations, i.e. {
>> 0 }, which give missing field initializer warnings.
> 
> I'd suggest that such initializers should just be {}, not { 0 }, and we
> should teach compilers to specifically *not* complain about empty
> initializers even when otherwise complaining about missing fields.
> Initializing a structure to 0 is completely sensible.

I agree completely! But of course that isn't how it is now. I guess I have 
spent too many years stuck on a single version of gcc that I tend not to think 
of changing the compiler readily enough. At least now I can upgrade the 
compiler freely.

Made me go check to be sure. Indeed even { } still throws the 
missing-initializers warning with gcc 4.8.3.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] irda: Convert function pointer arrays and uses to const

[Rustad, Mark D]

On Dec 10, 2014, at 10:28 AM, Joe Perches  wrote:

> Making things const is a good thing.
> 
> (x86-64 defconfig with all irda)
> $ size net/irda/built-in.o*
>   text   data bss dec hex filename
> 109276   1868 244  111388   1b31c net/irda/built-in.o.new
> 108828   2316 244  111388   1b31c net/irda/built-in.o.old
> 
> Signed-off-by: Joe Perches 
> ---
> include/net/irda/parameters.h  | 6 +++---
> net/irda/ircomm/ircomm_param.c | 8 
> net/irda/irttp.c   | 6 --
> net/irda/parameters.c  | 8 
> net/irda/qos.c | 6 +++---
> 5 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/include/net/irda/parameters.h b/include/net/irda/parameters.h
> index 42713c9..2d9cd00 100644
> --- a/include/net/irda/parameters.h
> +++ b/include/net/irda/parameters.h
> @@ -71,17 +71,17 @@ typedef int (*PV_HANDLER)(void *self, __u8 *buf, int len, 
> __u8 pi,
> PV_TYPE type, PI_HANDLER func);
> 
> typedef struct {
> - PI_HANDLER func;  /* Handler for this parameter identifier */
> + const PI_HANDLER func;  /* Handler for this parameter identifier */
>   PV_TYPEtype;  /* Data type for this parameter */
> } pi_minor_info_t;
> 
> typedef struct {
> - pi_minor_info_t *pi_minor_call_table;
> + const pi_minor_info_t *pi_minor_call_table;

Might you want to go a little further and make it:
const pi_minor_into_t * const pi_minor_call_table;
so that the pointer itself is also constant? That could apply to some others 
below as well.

>   int len;
> } pi_major_info_t;
> 
> typedef struct {
> - pi_major_info_t *tables;
> + const pi_major_info_t *tables;
>   int  len;
>   __u8 pi_mask;
>   int  pi_major_offset;
> diff --git a/net/irda/ircomm/ircomm_param.c b/net/irda/ircomm/ircomm_param.c
> index 27be782..3c4caa6 100644
> --- a/net/irda/ircomm/ircomm_param.c
> +++ b/net/irda/ircomm/ircomm_param.c
> @@ -61,12 +61,12 @@ static int ircomm_param_dte(void *instance, irda_param_t 
> *param, int get);
> static int ircomm_param_dce(void *instance, irda_param_t *param, int get);
> static int ircomm_param_poll(void *instance, irda_param_t *param, int get);
> 
> -static pi_minor_info_t pi_minor_call_table_common[] = {
> +static const pi_minor_info_t pi_minor_call_table_common[] = {
>   { ircomm_param_service_type, PV_INT_8_BITS },
>   { ircomm_param_port_type,PV_INT_8_BITS },
>   { ircomm_param_port_name,PV_STRING }
> };
> -static pi_minor_info_t pi_minor_call_table_non_raw[] = {
> +static const pi_minor_info_t pi_minor_call_table_non_raw[] = {
>   { ircomm_param_data_rate,PV_INT_32_BITS | PV_BIG_ENDIAN },
>   { ircomm_param_data_format,  PV_INT_8_BITS },
>   { ircomm_param_flow_control, PV_INT_8_BITS },
> @@ -74,13 +74,13 @@ static pi_minor_info_t pi_minor_call_table_non_raw[] = {
>   { ircomm_param_enq_ack,  PV_INT_16_BITS },
>   { ircomm_param_line_status,  PV_INT_8_BITS }
> };
> -static pi_minor_info_t pi_minor_call_table_9_wire[] = {
> +static const pi_minor_info_t pi_minor_call_table_9_wire[] = {
>   { ircomm_param_dte,  PV_INT_8_BITS },
>   { ircomm_param_dce,  PV_INT_8_BITS },
>   { ircomm_param_poll, PV_NO_VALUE },
> };
> 
> -static pi_major_info_t pi_major_call_table[] = {
> +static const pi_major_info_t pi_major_call_table[] = {
>   { pi_minor_call_table_common,  3 },
>   { pi_minor_call_table_non_raw, 6 },
>   { pi_minor_call_table_9_wire,  3 }
> diff --git a/net/irda/irttp.c b/net/irda/irttp.c
> index 3ef0b08..b6ab41d 100644
> --- a/net/irda/irttp.c
> +++ b/net/irda/irttp.c
> @@ -71,11 +71,13 @@ static void irttp_status_indication(void *instance,
>   LINK_STATUS link, LOCK_STATUS lock);
> 
> /* Information for parsing parameters in IrTTP */
> -static pi_minor_info_t pi_minor_call_table[] = {
> +static const pi_minor_info_t pi_minor_call_table[] = {
>   { NULL, 0 }, /* 0x00 */
>   { irttp_param_max_sdu_size, PV_INTEGER | PV_BIG_ENDIAN } /* 0x01 */
> };
> -static pi_major_info_t pi_major_call_table[] = { { pi_minor_call_table, 2 } 
> };
> +static const pi_major_info_t pi_major_call_table[] = {
> + { pi_minor_call_table, 2 }
> +};
> static pi_param_info_t param_info = { pi_major_call_table, 1, 0x0f, 4 };
> 
> / GLOBAL PROCEDURES /
> diff --git a/net/irda/parameters.c b/net/irda/parameters.c
> index 006786b..16ce32f 100644
> --- a/net/irda/parameters.c
> +++ b/net/irda/parameters.c
> @@ -52,7 +52,7 @@ static int irda_insert_no_value(void *self, __u8 *buf, int 
> len, __u8 pi,
> static int irda_param_unpack(__u8 *buf, char *fmt, ...);
> 
> /* Parameter value call table. Must match PV_TYPE */
> -static PV_HANDLER pv_extract_table[] = {
> +static const PV_HANDLER pv_extract_table[] = {
>   

Re: [PATCH 7/7] sched: Silence nested-externs warnings

[Rustad, Mark D]

On Sep 19, 2014, at 12:34 PM, Richard Weinberger  
wrote:

> On Fri, Sep 19, 2014 at 5:29 PM, Jeff Kirsher
>  wrote:
>> From: Mark Rustad 
>> 
>> Use diagnostic control macros to ignore nested-externs warnings
>> in this case.
>> 
>> CC: Ingo Molnar 
>> CC: Peter Zijlstra 
>> CC: Brian Norris 
>> Signed-off-by: Mark Rustad 
>> Signed-off-by: Jeff Kirsher 
>> ---
>> include/linux/sched.h | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 5c2c885..ed52c76 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -832,7 +832,9 @@ static inline int sched_info_on(void)
>> #ifdef CONFIG_SCHEDSTATS
>>return 1;
>> #elif defined(CONFIG_TASK_DELAY_ACCT)
>> +   DIAG_PUSH DIAG_IGNORE(nested-externs)
>>extern int delayacct_on;
>> +   DIAG_POP
> 
> This ridiculous, please try to move this extern into the appropriate header 
> file
> instead of surrounding it with these macros.

Excellent. I'll try adding an include of delayacct.h instead. My patch was 
based on the assumption that the existing code was wanted in that form for some 
reason, so the patch served a purpose in recognizing that it isn't. So the 
macros have served a purpose before even being in place. :-)

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 7/7] sched: Silence nested-externs warnings

[Rustad, Mark D]

On Sep 19, 2014, at 1:41 PM, Richard Weinberger  wrote:

> Am 19.09.2014 22:34, schrieb Rustad, Mark D:
>> Excellent. I'll try adding an include of delayacct.h instead. My patch was 
>> based on the assumption that the existing code was wanted in that form for 
>> some reason, so the patch served a purpose in recognizing that it isn't. So 
>> the macros have served a purpose before even being in place. :-)
> 
> I bet sched.h does not include delayacct.h for a good reason.
> sched.h is included in a lot of files, maybe delayacct.h introduces nasty 
> dependency issues.
> You'll find out. ;-)

Right you are, but when I'm done, there will only be one extern declaration for 
that global, which is better than more than one. I'll think a little before 
settling on the best resolution.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 5/7] signal: Silence nested-externs warnings

[Rustad, Mark D]

On Sep 19, 2014, at 2:21 PM, Josh Triplett  wrote:

> On Fri, Sep 19, 2014 at 07:20:30PM +0200, Oleg Nesterov wrote:
>> On 09/19, Richard Weinberger wrote:
>>> 
>>> Am 19.09.2014 17:37, schrieb Jeff Kirsher:
 
 See patch 1 of the series.
>>> 
>>> I was not CC'ed...
>> 
>> Me too, and thus I don't understand this patch.
>> 
>> But I have to admit it looks a bit ugly to me anyway.
>> Can't we simply kill _NSIG_WORDS_is_unsupported_size ?
> 
> This looks quite preferable.  Can you post that with a commit message
> and signoff?  Also, the indentation on the second of the three BUILD_BUG
> calls has some spaces in it, which it shouldn't.  With those fixed:
> Reviewed-by: Josh Triplett 

I haven't tried this patch myself yet, but assuming that it works, it is a far 
better way to go.

>> diff --git a/include/linux/signal.h b/include/linux/signal.h
>> index 750196f..679c9b4 100644
>> --- a/include/linux/signal.h
>> +++ b/include/linux/signal.h
>> @@ -67,7 +67,6 @@ static inline int sigismember(sigset_t *set, int _sig)
>> 
>> static inline int sigisemptyset(sigset_t *set)
>> {
>> -extern void _NSIG_WORDS_is_unsupported_size(void);
>>  switch (_NSIG_WORDS) {
>>  case 4:
>>  return (set->sig[3] | set->sig[2] |
>> @@ -77,7 +76,7 @@ static inline int sigisemptyset(sigset_t *set)
>>  case 1:
>>  return set->sig[0] == 0;
>>  default:
>> -_NSIG_WORDS_is_unsupported_size();
>> +BUILD_BUG();
>>  return 0;
>>  }
>> }
>> @@ -90,7 +89,6 @@ static inline int sigisemptyset(sigset_t *set)
>> #define _SIG_SET_BINOP(name, op) \
>> static inline void name(sigset_t *r, const sigset_t *a, const sigset_t *b) \
>> {\
>> -extern void _NSIG_WORDS_is_unsupported_size(void);  \
>>  unsigned long a0, a1, a2, a3, b0, b1, b2, b3;   \
>>  \
>>  switch (_NSIG_WORDS) {  \
>> @@ -107,7 +105,7 @@ static inline void name(sigset_t *r, const sigset_t *a, 
>> const sigset_t *b) \
>>  r->sig[0] = op(a0, b0); \
>>  break;  \
>>  default:\
>> -_NSIG_WORDS_is_unsupported_size();  \
>> +BUILD_BUG();\
>>  }   \
>> }
>> 
>> @@ -128,8 +126,6 @@ _SIG_SET_BINOP(sigandnsets, _sig_andn)
>> #define _SIG_SET_OP(name, op)
>> \
>> static inline void name(sigset_t *set)   
>> \
>> {\
>> -extern void _NSIG_WORDS_is_unsupported_size(void);  \
>> -\
>>  switch (_NSIG_WORDS) {  \
>>  case 4: set->sig[3] = op(set->sig[3]);  \
>>  set->sig[2] = op(set->sig[2]);  \
>> @@ -137,7 +133,7 @@ static inline void name(sigset_t *set)   
>> \
>>  case 1: set->sig[0] = op(set->sig[0]);  \
>>  break;  \
>>  default:\
>> -_NSIG_WORDS_is_unsupported_size();  \
>> +BUILD_BUG();\
>>  }   \
>> }

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 7/7] sched: Silence nested-externs warnings

[Rustad, Mark D]

On Sep 19, 2014, at 3:54 PM, Peter Zijlstra  wrote:

> On Fri, Sep 19, 2014 at 08:29:40AM -0700, Jeff Kirsher wrote:
>> From: Mark Rustad 
>> 
>> Use diagnostic control macros to ignore nested-externs warnings
>> in this case.
>> 
>> CC: Ingo Molnar 
>> CC: Peter Zijlstra 
>> CC: Brian Norris 
>> Signed-off-by: Mark Rustad 
>> Signed-off-by: Jeff Kirsher 
>> ---
>> include/linux/sched.h | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 5c2c885..ed52c76 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -832,7 +832,9 @@ static inline int sched_info_on(void)
>> #ifdef CONFIG_SCHEDSTATS
>>  return 1;
>> #elif defined(CONFIG_TASK_DELAY_ACCT)
>> +DIAG_PUSH DIAG_IGNORE(nested-externs)
>>  extern int delayacct_on;
>> +DIAG_POP
>>  return delayacct_on;
> 
> Who has this nested extern warn on in anycase?

They appear in W=2 builds, so you do have to ask for them.

> I've never seen it
> generate a warning. Also WTF is DIAG_PUSH/POP, its not a GCC thing
> afaict.

The first patch in the series adds macros to use the gcc/clang pragmas to 
control these things. Both compilers have the capability. The macros generate 
nothing for compilers that lack it.

In any case, this particular one will be resolved instead of silenced.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 22, 2014, at 8:33 AM, Borislav Petkov  wrote:

> On Fri, Sep 19, 2014 at 08:29:33AM -0700, Jeff Kirsher wrote:
>> The following patches silence over 100,000 warnings in a W=2
>> kernel build. This series does most of it by using the compilers
>> diagnostic controls. The first patch in the series adds macros to
>> invoke the pragmas for those controls. Macros are provided for GCC
>> and clang. Although they are highly compatible in this area, macros
>> are provided for compiler-specific controls, and there is one
>> example that uses a clang-specific control (look for DIAG_CLANG_IGNORE).
>> 
>> Some missing-field-initializers warnings were resolved using
>> the diagnostic control macros simply because so many lines
>> would have had to have been changed. At this stage Mark thought 
>> about avoiding possible merge issues. If the maintainer would 
>> rather resolve them by using designated initialization, just 
>> say so.
>> 
>> The combined effect of this patch series and his other patches
>> that did not use these diagnostic control macros was to reduce 
>> the number of W=2 warnings from 127,164 to 1,345!
> 
> Sorry but I don't see the point of actively adding macros to the code
> just so that gcc is happy. There's a reason why a bunch of warnings are
> disabled in the normal build and only enabled with the W= switch.
> 
> The W= things are supposed to be used when developing code and have the
> compiler tell you about *possible* issues. That doesn't mean though that
> we have to actively "fix" otherwise perfectly fine code.

The problem is that the kernel include files throw so many warnings that it 
really discourages anyone from ever going through them, even for a single 
driver. The warnings are far more valuable and usable when known acceptable 
usages are silenced.

> Having the need to actively go in and add code so that gcc doesn't issue
> obscure warnings is going too far, IMO.

Well, the whole series of patches that I made definitely went too far - only 
the first 5 out of about 30 have been posted, but if we can make some progress 
on generating fewer warnings out of the include files, I think it would be 
helpful. Already the patches that use them have triggered some activity that 
has resulted in resolving warnings without use of the macros, and I see that as 
much better than simply using the macros.

The macros can serve a useful purpose, but they should not be widely used. When 
to use them is definitely a judgement call. If the macros are accepted, it may 
be worth adding a checkpatch.pl warning for adding a DIAG_*IGNORE macro.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 22, 2014, at 11:40 AM, Borislav Petkov  wrote:

> On Mon, Sep 22, 2014 at 05:06:27PM +0000, Rustad, Mark D wrote:
>> Well, the whole series of patches that I made definitely went too far
>> - only the first 5 out of about 30 have been posted, but if we can
>> make some progress on generating fewer warnings out of the include
>> files, I think it would be helpful.
> 
> Helpful for what? Those are W=2 warnings which are disabled in the
> default build.

It is helpful for using the warnings to look for problems or even just risks.

>> The macros can serve a useful purpose, but they should not be widely
>> used. When to use them is definitely a judgement call. If the macros
>> are accepted, it may be worth adding a checkpatch.pl warning for
>> adding a DIAG_*IGNORE macro.
> 
> Right, so add the macros and tell people *not* to use them. That won't
> fly.

Right now the number of warnings generated when using W=2 simply tells people 
to never use W=2. That severely limits the value of a useful tool. A checkpatch 
warning doesn't mean to never do that, just that it needs a critical look and 
justification. That is certainly true of every patch I made that uses those 
macros.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] sched: Remove nested extern

[Rustad, Mark D]

On Sep 22, 2014, at 12:01 PM, Peter Zijlstra  wrote:

> On Mon, Sep 22, 2014 at 10:55:11AM -0700, Mark D Rustad wrote:
>> Avoid W=2 nested-externs warning by moving the nested extern to
>> a normal extern. This eliminates that warning which is generated
>> for every inclusion of sched.h in a kernel build when W=2 is used.
>> This also removes a point of maintenance if the definition of
>> delayacct_on were ever to change.
> 
> Yeah, so why is that warning worth using? Just disable the warn if
> you're bothered by it.

I assume that nested-externs is included in W=2 because many uses of
them, especially with function prototypes, creates a risk of inconsistent
declarations. The kernel has a fair number of them that seem to have
been added to disentangle include files. If they are judged to be
worthwhile, then it would be good to silence the warnings so that
attention can be given to other instances of the warnings. If those
nested externs are not worth the risk, well that is a different conversation.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] sched: Remove nested extern

[Rustad, Mark D]

On Sep 22, 2014, at 1:05 PM, Peter Zijlstra  wrote:

> On Mon, Sep 22, 2014 at 07:32:04PM +0000, Rustad, Mark D wrote:
>> I assume that nested-externs is included in W=2 because many uses of
>> them, especially with function prototypes, creates a risk of inconsistent
>> declarations. The kernel has a fair number of them that seem to have
>> been added to disentangle include files. If they are judged to be
>> worthwhile, then it would be good to silence the warnings so that
>> attention can be given to other instances of the warnings. If those
>> nested externs are not worth the risk, well that is a different conversation.
> 
> So would using something like LTO not be way better at actually finding
> the problems, seeing how it can actually see the inconsistency in
> declarations.
> 
> It seems to me that banning the use of nested extern is misguided as
> they are a useful tool; they provide some means of keeping symbols from
> being globally visible even though they actually are. Its a really poor
> man's 'private', but its the best C provides.

By using the macros the use of nested externs can be discouraged without
being banned. Presence of a DIAG_IGNORE macro should mean that the exception
has been noted and accepted. And the macro remains as an indicator of that.
Or to be taken as a warning label: readers choice perhaps.

> Also, why do you care about W=2 nonsense anyhow? They're default
> disabled for a reason.

Because I have found that enabling many warnings helps identify problems
in code and it has been my standard practice since about 1999 to do so.
The compiler warnings are really just another form of static analysis,
and I use it routinely on every compile. Here is how routinely: I have
W=1 in my environment, W=12 is just too painful. I would change that
default to W=12 if it wasn't insane to do so.

In my own code, I use way more warnings than even W=12 enables and in
that code I just resolve them. I had never resorted to using the attributes
in my own code, but they are an available tool.

I just finally thought it might be time to consider opening up a new
method to manage them for the kernel.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 22, 2014, at 1:33 PM, Borislav Petkov  wrote:

> Btw, out of curiosity, what is your use case for staring at those W=2
> warnings?

I know no one cares about out-of-tree drivers, but I have a hack that
allows building out-of-tree drivers without getting warnings from the
kernel includes. We do an automated compile of every patch with W=12
and expect clean compiles.

It would be nice to compile drivers in-tree and have a similar expectation.
I guess a similar hack could be developed, but since we are contributing
upstream, I would rather uncover any potential issues that may exist, even
if they aren't in the driver. The hack would tend to cover up such issues.
This is definitely NOT about covering up things that could be problems!

> In thinking about it, what we could also do is simply move the noisiest
> ones to W=3 or so, or even add another W= level. It'll be interesting to
> hear your use case though. AFAICT, this is the first time I hear of a
> more, let's say, serious use case of W= since we added the W= things a
> couple of years ago. :-)

Well, I have W=1 in my environment, so I don't even have to ask for it, I
just get it. W=12 is just insane, or I would use that all the time. I think
it would be nice for new code, or at least new drivers, to compile clean with
W=12, but that isn't possible when the kernel includes throw so many warnings.

Nested-externs, for example, can catch people gratuitously providing a
function prototype that could become a hazard, but some use of that may
be justified. The macros provide a way to specifically allow certain
instances while generally discouraging it. Of course if you never use
W=2 you may never catch those gratuitous declarations.

> Thanks.

Hopefully the discussion is somewhat useful.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] sched: Remove nested extern

[Rustad, Mark D]

On Sep 22, 2014, at 2:21 PM, Peter Zijlstra  wrote:

> On Mon, Sep 22, 2014 at 08:59:32PM +0000, Rustad, Mark D wrote:
>> Because I have found that enabling many warnings helps identify problems
>> in code and it has been my standard practice since about 1999 to do so.
>> The compiler warnings are really just another form of static analysis,
>> and I use it routinely on every compile. Here is how routinely: I have
>> W=1 in my environment, W=12 is just too painful. I would change that
>> default to W=12 if it wasn't insane to do so.
> 
> Many warnings are just plain insane and stupid. They're not helping
> anybody. There's a very good reason many are disabled. I'm sure you can
> find some entertaining discussions on the topic if you search the LKML
> archives.

That is what I used to think. -Wshadow for example. What's the problem? It
is perfectly valid C. Well, I recently sent a patch that changed some
function parameters that used the name jiffies, which of course shadowed
the well-known global. If a macro were ever called in those functions
whose expansion ever tried to access jiffies, well it wouldn't do what
was expected. Not a bug now, but a trap for the future. I only found that
because I either resolved or silenced enough warnings to see something 
interesting.

Over the years I think I have resolved real bugs revealed by probably nearly
every one of the additional warning messages. They do have value, but that
value is typically lost if the norm is a flood of messages. Apple would not
have had their SSL problem if they had had -Wunreachable-code enabled and
noticed the message.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 23, 2014, at 7:49 AM, Josh Triplett  wrote:

> On Tue, Sep 23, 2014 at 10:01:20AM +0200, Borislav Petkov wrote:
>> ./arch/x86/include/asm/io_apic.h: In function ‘io_apic_modify’:
>> ./arch/x86/include/asm/io_apic.h:223:48: warning: declaration of ‘apic’ 
>> shadows a global declaration [-Wshadow]
>> static inline void io_apic_modify(unsigned int apic, unsigned int reg, 
>> unsigned int value)
>>^
>> In file included from ./arch/x86/include/asm/smp.h:12:0,
>> from include/linux/smp.h:59,
>> from include/linux/topology.h:33,
>> from include/linux/gfp.h:8,
>> from include/linux/kmod.h:22,
>> from include/linux/module.h:13,
>> from drivers/edac/amd64_edac.h:65,
>> from drivers/edac/amd64_edac.c:1:
>> ./arch/x86/include/asm/apic.h:366:21: warning: shadowed declaration is here 
>> [-Wshadow]
>> extern struct apic *apic;
>> ^
>> 
>> So gcc complains that an unsigned int shadows a struct apic pointer.
> 
> Here, I think the right fix involves picking a more descriptive name
> than "apic" for the global varible.

I agree, but I don't know enough about the area to necessarily know what it 
should be called instead. I do have a patch that changes the local variables 
instead, but even as I made it, I didn't really think it was right. But it 
silenced a ton of warnings and let me see other things.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 23, 2014, at 1:22 AM, Borislav Petkov  wrote:

> On Mon, Sep 22, 2014 at 09:50:54PM +0000, Rustad, Mark D wrote:
> * Fixing those is a good idea if the fixes are clean - I think we all
> agree by now that adding code just to shut up gcc is not nice.

Yes, but I think there are a few cases where it could be helpful. When
there is something exceptional that will throw a warning. In one of the
patches that Jeff sent, I used the DIAG_CLANG_IGNORE macro to suppress
the warning that is thrown for every entry of the syscall table when
compiled with clang. The code is right and doing exactly what is wanted,
so note the exception and make it shut up.

> * Then, even if all those warnings were fixed one fine day, the people
> who fix them would be fighting windmills because every new patch which
> adds new places causing those warnings would simply go in because the
> warnings are not visible in default builds.
> 
> So the question IMO turns into: are there some warnings which we should
> promote to default builds so that they get taken care of eventually...

I would generally be in favor of that over time, but I recognize that with
the range of architectures and toolchains that need to be supported, that
may be difficult.

>> Well, I have W=1 in my environment, so I don't even have to ask for it, I
>> just get it.
> 
> I think this was the initial use case we had in mind for W= - use it
> during development in order to have the compiler do extra checks to your
> code. And it has caught a couple of issues, FWIW.

Yes, not everyone building the kernel is a developer. I certainly get that.
Right now W=2 is kind of useless for developers because so much noise is
generated. With the number of people working on Linux, it may be enough if
a handful are taking a look at W=2 messages, but right now it is a pretty
awful task to even try to look.

>> Nested-externs, for example, can catch people gratuitously providing a
>> function prototype that could become a hazard, but some use of that may
>> be justified. The macros provide a way to specifically allow certain
>> instances while generally discouraging it. Of course if you never use
>> W=2 you may never catch those gratuitous declarations.
> 
> Sure, but the cost for fixing that is what bothers me. For that
> particular case, it probably would even be cleaner to add a
> nested-extern check to checkpatch instead of cluttering the code with
> those macros.

Generally there should be relatively few exceptions that need to be tagged.
If there were 1000, that would be way too many. The full series of my
patches had 90 instances. Some of the few that have been sent have been
resolved in better ways, which we all agree is better. Suppose that 40
can't be reasonably resolved in direct ways. Is that really costly? I'm
trying to understand what your perception of the cost is.

Perhaps checkpatch would be a better gatekeeper for new code. OTOH, some of
those nested externs have already been eliminated, so at least for now the
warning is serving a purpose since it is scrubbing existing code.

>> Hopefully the discussion is somewhat useful.
> 
> Well, it has become already, as you can see. :-D

I take it as a given that the kernel sometimes has special needs and needs
to do special things. The macros make it possible to mark those special
usages. I prefer adding the macros in a few places to eliminating a warning
altogether unless the warning is always useless. I'm sure we agree that we
don't want to ever turn on -pedantic! :-)

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 23, 2014, at 11:44 AM, Borislav Petkov  wrote:

> On Tue, Sep 23, 2014 at 05:24:22PM +0000, Rustad, Mark D wrote:
>> Yes, but I think there are a few cases where it could be helpful. When
>> there is something exceptional that will throw a warning. In one of the
>> patches that Jeff sent, I used the DIAG_CLANG_IGNORE macro to suppress
>> the warning that is thrown for every entry of the syscall table when
>> compiled with clang. The code is right and doing exactly what is wanted,
>> so note the exception and make it shut up.
> 
> So we're doing some dancing around solely to shut up the compiler? Even
> if the code is correct?!? Sorry, this is just ass backwards.

Well, please consider the specifics. The entire syscall table is initialized
with a constant pattern to be sure that every item is initialized. Then each
syscall is initialized into its proper place. The compiler is complaining that
entries are being initialized twice.

Most of the time, that is not done, and so it may catch a patch foulup or
something. In this particular case, it is normal and intended. There is
nothing wrong, so there is no reason to throw a warning for every single
entry in the table. Which is what happens with clang today.

So the code is correct, but in general the warning can reveal certain issues.
Just not in this particular usage. This happens to be a warning specific to
clang at the moment.

> If it were me, I'd say even one is too much. Because the very thing
> of adding code just to shut up the compiler which generates otherwise
> correct code is simply very very wrong in my book.
> 
> Bear in mind that even if initially you have a low number of macro
> invocations, that number will grow because people will start using it in
> other places too. And all of a sudden, the thing has spread like weed
> and there's no removing it anymore. So we better not start in the first
> place.

That is why it would be more than reasonable for checkpatch to warn on the
macro introductions. It is certainly a more significant thing than a
line > 80 characters.

> Again, we should take compiler warnings with a grain of salt and judge
> them only by the quality of the generated code. IMO.

I think more than the nature of the executable code matters. The namespace
issues revealed by -Wshadow can certainly create nasty surprises over time.
But we can't get that value from them when they are lost in an ocean of
warnings that are always there and are not the source of any trouble.

The problem is that when so many warnings are generated, particularly by
include files, even useful warnings will not be seen. Specifically
silencing ones that are deemed to be "ok" will help in seeing ones that
are not. The silencing has the greatest effect in the include files,
since there is such a multiplier effect.

Warnings that no one looks at, or bother to generate at all, have
absolutely no value. Even a silenced warning continues to wear its
shame attribute (macro). Hmm. Maybe instead of DIAG_* they should be
named SHAME_*.

Most of the time, it is new instances of warnings that are most likely to
reveal a problem. Hiding them in a flood of "normal" warnings prevents
them from ever being seen. And that is a shame.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] moduleparam: Resolve missing-field-initializer warning

[Rustad, Mark D]

On Aug 31, 2014, at 5:52 PM, Rusty Russell  wrote:

> Jeff Kirsher  writes:
>> From: Mark Rustad 
>> 
>> Resolve a missing-field-initializer warning, that is produced
>> by every reference to module_param_call, by using designated
>> initialization for the first field. That is enough to silence
>> the complaint.
>> 
>> Signed-off-by: Mark Rustad 
>> Signed-off-by: Jeff Kirsher 
>> ---
>> include/linux/moduleparam.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Strange, I haven't seen this warning.  Compiler version?  And it's good
> to quote the error message, so people can google it.

The message is only seen when doing a W=2 build. I happened to be using gcc 
4.8.3, but I think most versions would produce the warning when it is enabled. 
It can either be silenced by using even a single designated initializer as I 
did here, or providing values for all of the fields. Because of the number of 
references to the macro, this change silences many warnings in W=2 builds.

One instance of the full warning message looks like this:

/home/share/git/nn-mdr/include/linux/moduleparam.h:198:16: warning: missing 
initializer for field ‘free’ of ‘struct kernel_param_ops’ 
[-Wmissing-field-initializers]
  static struct kernel_param_ops __param_ops_##name =  \
^
/home/share/git/nn-mdr/fs/fuse/inode.c:35:1: note: in expansion of macro 
‘module_param_call’
 module_param_call(max_user_bgreq, set_global_limit, param_get_uint,
 ^
/home/share/git/nn-mdr/include/linux/moduleparam.h:56:9: note: ‘free’ declared 
here
  void (*free)(void *arg);


> Cheers,
> Rusty.
> 
>> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
>> index 494f99e..d99a9e9 100644
>> --- a/include/linux/moduleparam.h
>> +++ b/include/linux/moduleparam.h
>> @@ -196,7 +196,7 @@ struct kparam_array
>> /* Obsolete - use module_param_cb() */
>> #define module_param_call(name, set, get, arg, perm) \
>>  static struct kernel_param_ops __param_ops_##name = \
>> -{ 0, (void *)set, (void *)get };\
>> +{ .flags = 0, (void *)set, (void *)get };   \

This could also be resolved by adding a ", NULL" to the initializer above 
instead of the designated initializer. The designated initializer means that if 
additional "optional" fields were to be added in the future, this would not 
have to be touched to avoid generating the warning. However you prefer it. If 
instead you would prefer to designate all fields, the formal parameter names 
would have to change, since get and set would get substituted for the field 
designators .get and .set.

>>  __module_param_call(MODULE_PARAM_PREFIX,\
>>  name, &__param_ops_##name, arg, \
>>  (perm) + sizeof(__check_old_set_param(set))*0, -1)
>> -- 
>> 1.9.3

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] semaphore: Resolve some shadow warnings

[Rustad, Mark D]

On Sep 1, 2014, at 4:41 PM, Jeff Kirsher  wrote:

> On Mon, 2014-09-01 at 14:02 +0200, Peter Zijlstra wrote:
>> On Thu, Aug 28, 2014 at 05:19:26AM -0700, Jeff Kirsher wrote:
>>> From: Mark Rustad 
>>> 
>>> Resolve some shadow warnings resulting from using the name
>>> jiffies, which is a well-known global. This is not a problem
>>> of course, but it could be a trap for someone copying and
>>> pasting code, and it just makes W=2 a little cleaner.
>>> 
>>> Signed-off-by: Mark Rustad 
>>> Signed-off-by: Jeff Kirsher 
>> 
>> Why isn't Mark sending this email?
> 
> Mark sent me several patches like this, for me to push upstream.  So, I
> am making sure the appropriate owner is the receives the patch versus
> blindly sending to LKML.
> 
>> 
>>> ---
>>> kernel/locking/semaphore.c | 12 ++--
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
>>> index 6815171..7782dbc 100644
>>> --- a/kernel/locking/semaphore.c
>>> +++ b/kernel/locking/semaphore.c
>>> @@ -36,7 +36,7 @@
>>> static noinline void __down(struct semaphore *sem);
>>> static noinline int __down_interruptible(struct semaphore *sem);
>>> static noinline int __down_killable(struct semaphore *sem);
>>> -static noinline int __down_timeout(struct semaphore *sem, long jiffies);
>>> +static noinline int __down_timeout(struct semaphore *sem, long njiffies);
>>> static noinline void __up(struct semaphore *sem);
>> 
>> So what's wrong with calling it "timeout" instead? That's what most
>> other sites do.
> 
> Timeout would work as well to resolve the shadow warnings.

It would, but then it would be unclear as to what units the timeout was in. I 
have no other objection to timeout, I was just trying to preserve the meaning 
in the existing overloaded name. The "n" to me suggests a number and, if 
anything, number of jiffies conveys a more precise meaning than simply jiffies 
did.

-- 
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: kernel: Resolve a shadow warning

[Rustad, Mark D]

On Sep 4, 2014, at 2:38 PM, Andrew Morton  wrote:

>> From: Mark Rustad 
>> 
>> Resolve a shadow warning in W=2 builds arising from min/max macro
>> references in a parameter to a min3/max3 macro. There is no
>> functional issue - the warning is benign - but simply changing
>> some local variable names will eliminate it.
>> 
>> ...
>> 
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -716,22 +716,22 @@ static inline void ftrace_dump(enum ftrace_dump_mode 
>> oops_dump_mode) { }
>>  _max1 > _max2 ? _max1 : _max2; })
>> 
>> #define min3(x, y, z) ({ \
>> -typeof(x) _min1 = (x);  \
>> -typeof(y) _min2 = (y);  \
>> -typeof(z) _min3 = (z);  \
>> -(void) (&_min1 == &_min2);  \
>> -(void) (&_min1 == &_min3);  \
>> -_min1 < _min2 ? (_min1 < _min3 ? _min1 : _min3) : \
>> -(_min2 < _min3 ? _min2 : _min3); })
>> +typeof(x) _min31 = (x); \
>> +typeof(y) _min32 = (y); \
>> +typeof(z) _min33 = (z); \
>> +(void) (&_min31 == &_min32);\
>> +(void) (&_min31 == &_min33);\
>> +_min31 < _min32 ? (_min31 < _min33 ? _min31 : _min33) : \
>> +(_min32 < _min33 ? _min32 : _min33); })
>> 
>> #define max3(x, y, z) ({ \
>> -typeof(x) _max1 = (x);  \
>> -typeof(y) _max2 = (y);  \
>> -typeof(z) _max3 = (z);  \
>> -(void) (&_max1 == &_max2);  \
>> -(void) (&_max1 == &_max3);  \
>> -_max1 > _max2 ? (_max1 > _max3 ? _max1 : _max3) : \
>> -(_max2 > _max3 ? _max2 : _max3); })
>> +typeof(x) _max31 = (x); \
>> +typeof(y) _max32 = (y); \
>> +typeof(z) _max33 = (z); \
>> +(void) (&_max31 == &_max32);\
>> +(void) (&_max31 == &_max33);\
>> +_max31 > _max32 ? (_max31 > _max33 ? _max31 : _max33) : \
>> +(_max32 > _max33 ? _max32 : _max33); })
>> 
>> /**
>>  * min_not_zero - return the minimum that is _not_ zero, unless both are zero
> 
> I'm still sitting on the below patch.  It's stalled because I have a
> note that David potentially found issues with it.  But on rechecking,
> that appears to be stale, or not serious enough to prevent inclusion.
> 
> I can't (be bothered to) check whether this patch fixes the warnings
> because your changelog didn't tell me how to trigger the warnings (bad
> changelog).  But it might fix them!  Can you please test it?

Actually it does. W=2 builds get warnings when a min/max macro is used as a 
parameter to min3/max3. If you move to the min3/max3 that makes nested calls to 
min/max, every reference to min3/max3 will generate a warning in W=2 builds no 
matter what. It is interesting that the compiler optimizes that better - I 
hadn't looked at that.

Not wanting to argue for poorer code, lets forget about the warnings for now. 
These particular shadow warnings are pretty trivial. I'll consider revisiting 
it later. And next time I'll check the code generation.

> From: Michal Nazarewicz 
> Subject: include/linux/kernel.h: rewrite min3, max3 and clamp using min and 
> max
> 
> It appears that gcc is better at optimising a double call to min and max
> rather than open coded min3 and max3.  This can be observed here:
> 
>$ cat min-max.c
>#define min(x, y) ({   \
>   typeof(x) _min1 = (x);  \
>   typeof(y) _min2 = (y);  \
>   (void) (&_min1 == &_min2);  \
>   _min1 < _min2 ? _min1 : _min2; })
>#define min3(x, y, z) ({   \
>   typeof(x) _min1 = (x);  \
>   typeof(y) _min2 = (y);  \
>   typeof(z) _min3 = (z);  \
>   (void) (&_min1 == &_min2);  \
>   (void) (&_min1 == &_min3);  \
>   _min1 < _min2 ? (_min1 < _min3 ? _min1 : _min3) : \
>   (_min2 < _min3 ? _min2 : _min3); })
> 
>int fmin3(int x, int y, int z) { return min3(x, y, z); }
>int fmin2(int x, int y, int z) { return min(min(x, y), z); }
> 
>$ gcc -O2 -o min-max.s -S min-max.c; cat min-max.s
>   .file   "min-max.c"
>   .text
>   .p2align 4,,15
>   .globl  fmin3
>   .type   fmin3, @function
>fmin3:
>.LFB0:
>   .cfi_startproc
>   cmpl%esi, %edi
>   jl  .L5
>   cmpl%esi, %edx
>   movl%esi, %eax
>   cmovle  %edx, %eax
>   ret
>   .p2align 4,,10
>   .p2align 3
>.L5:
>   cmpl%edi, %edx
>   movl%edi, %eax
>   cmovle  %edx, %eax
>   ret
>   .cfi_endproc
>.LFE0:
>   .size   fmin3, .-fmin3
>   .p2align 4,,15
>   .globl  fmin2
>   .type   fmin2, @function
>fmin2:
>.LFB1:
>   .cfi_startproc
>   cmpl%edi, %esi

Re: [PATCH 0/7] Silence even more W=2 warnings

[Rustad, Mark D]

On Sep 23, 2014, at 11:44 AM, Borislav Petkov  wrote:

> Again, we should take compiler warnings with a grain of salt and judge
> them only by the quality of the generated code. IMO.

The more I thought about this, the more I think it argues for having some
diagnostic control macros. Tools such as the compiler can be very thorough -
that is their strength. The warnings that the compiler emits are things
that humans should consider. There are times when the human can judge
that nothing need be done about it, so why not use the macros to indicate
that that judgement has been made and get it out of the way so more
potentially interesting issues can be found? How many people need to
make that judgement over and over? That is clearly a waste of time and
is why these higher warning levels simply don't get looked at. So lets
capture that judgement in the code. That judgement would continue to be
open to reevaluation in any case.

The syscall issue is not arising from an include file, but I think
demonstrates the use nicely and I think is a fine example of a legitimate
use in a C file.

For me, it is about getting more value out of the evaluation that the compiler
can do. When the output of W=2 is unusable because it is so voluminous - filled
with things known to not cause problems - why not silence some of major
sources so that more interesting issues might be seen? By not doing so, W=2
has close to 0 value because no one looks at it.

I guess I also trust the maintainers to not accept lots of patches that add
uses of such macros. They have certainly demonstrated that up to this point.

:-)

-- 
Mark Rustad, Networking Division, Intel Corporation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/