Re: [PATCH V3 2/4] net-next: mediatek: add support for MT7623 ethernet
On Thu, 2016-03-03 at 21:02 +0100, John Crispin wrote: > Add ethernet support for MediaTek SoCs from the MT7623 family. [] > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h > b/drivers/net/ethernet/mediatek/mtk_eth_soc.h [] > +/* ugly macro hack to make sure hw_stats and ethtool strings are consistent > */ I agree and think the _FE macros defines and uses should be removed.
Re: [PATCH net-next v3 2/3] nsh: logging module
On Tue, 2016-03-01 at 11:11 +, Brian Russell wrote:
> Module can register for Type 1 or specified classes of Type 2 metadata
> and will then log incoming matching packets.
This logging mechanism seems like a way to fill/DoS logs.
Maybe use pr_info_ratelimit?
Maybe use the trace_events mechanisms instead?
> Signed-off-by: Brian Russell
> ---
> net/ipv4/Kconfig | 8
> net/ipv4/Makefile | 1 +
> net/ipv4/nsh_log.c | 135
> +
> 3 files changed, 144 insertions(+)
> create mode 100644 net/ipv4/nsh_log.c
>
> diff --git a/net/ipv4/Kconfig b/net/ipv4/Kconfig
> index df14c59..87b6dde 100644
> --- a/net/ipv4/Kconfig
> +++ b/net/ipv4/Kconfig
> @@ -223,6 +223,14 @@ config NET_NSH
>
> To compile it as a module, choose M here. If unsure, say N.
>
> +config NET_NSH_LOG
> +tristate 'NSH Metadata Logger'
> + depends on NET_NSH
> + help
> + Log packets with incoming NSH metadata.
> +
> + To compile it as a module, choose M here. If unsure, say N.
> +
> config IP_MROUTE
> bool "IP: multicast routing"
> depends on IP_MULTICAST
> diff --git a/net/ipv4/Makefile b/net/ipv4/Makefile
> index 14b7995..69377fb 100644
> --- a/net/ipv4/Makefile
> +++ b/net/ipv4/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_NET_FOU) += fou.o
> obj-$(CONFIG_NET_IPGRE_DEMUX) += gre.o
> obj-$(CONFIG_NET_IPGRE) += ip_gre.o
> obj-$(CONFIG_NET_NSH) += nsh.o
> +obj-$(CONFIG_NET_NSH_LOG) += nsh_log.o
> obj-$(CONFIG_NET_UDP_TUNNEL) += udp_tunnel.o
> obj-$(CONFIG_NET_IPVTI) += ip_vti.o
> obj-$(CONFIG_SYN_COOKIES) += syncookies.o
> diff --git a/net/ipv4/nsh_log.c b/net/ipv4/nsh_log.c
> new file mode 100644
> index 000..3d774ed
> --- /dev/null
> +++ b/net/ipv4/nsh_log.c
> @@ -0,0 +1,135 @@
> +/*
> + * Network Service Header (NSH) logging module.
> + *
> + * Copyright (c) 2016 by Brocade Communications Systems, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include
> +#include
> +#include
> +
> +static bool t1_enabled = false;
> +module_param(t1_enabled, bool, 0444);
> +MODULE_PARM_DESC(t1_enabled, "Type 1 Metadata log enabled");
> +
> +#define MAX_T2_CLASSES 10
> +static unsigned int t2_classes[MAX_T2_CLASSES];
> +static int num_t2 = 0;
> +module_param_array(t2_classes, uint, &num_t2, 0444);
> +MODULE_PARM_DESC(t2_classes, "Type 2 Metadata classes log enabled");
> +
> +static const char *nsh_next_proto(u8 next_proto)
> +{
> + switch (next_proto) {
> + case NSH_NEXT_PROTO_IPv4:
> + return "IPv4";
> + case NSH_NEXT_PROTO_IPv6:
> + return "IPv6";
> + case NSH_NEXT_PROTO_ETH:
> + return "Eth";
> + default:
> + return "Unknown";
> + }
> +}
> +
> +/* Type 1 metadata has fixed length, 4 x 32-bit words */
> +static int nsh_log_t1(struct sk_buff *skb, u32 service_path_id,
> + u8 service_index, u8 next_proto,
> + struct nsh_metadata *ctx_hdr, unsigned int num_ctx_hdrs)
> +{
> + u32 *data;
> +
> + if ((ctx_hdr->class != NSH_MD_CLASS_TYPE_1) ||
> + (ctx_hdr->type != NSH_MD_TYPE_TYPE_1) ||
> + (ctx_hdr->len != NSH_MD_LEN_TYPE_1) ||
> + (num_ctx_hdrs != 1))
> + return -EINVAL;
> +
> + data = ctx_hdr->data;
> + pr_info("NSH T1 Rx(%s): SPI=%u SI=%u Next=%s"
> + " MD 0x%08x 0x%08x 0x%08x 0x%08x\n", skb->dev->name,
> + service_path_id, service_index, nsh_next_proto(next_proto),
> + data[0], data[1], data[2], data[3]);
> +
> + return 0;
> +}
> +
> +/* Type 2 metadata consists of a variable number of TLVs */
> +#define T2_BUFSIZE 512
> +static int nsh_log_t2(struct sk_buff *skb, u32 service_path_id,
> + u8 service_index, u8 next_proto,
> + struct nsh_metadata *ctx_hdrs, unsigned int num_ctx_hdrs)
> +{
> + char t2_buf[T2_BUFSIZE];
> + int wrlen;
> + u32 *data;
> + int i,j;
> +
> + wrlen = snprintf(t2_buf, T2_BUFSIZE,
> + "NSH T2 Class %u Rx(%s): SPI=%u SI=%u Next=%s MD",
> + ctx_hdrs[0].class, skb->dev->name, service_path_id,
> + service_index, nsh_next_proto(next_proto));
> +
> + for (i = 0; i < num_ctx_hdrs; i++) {
> + wrlen += snprintf(t2_buf+wrlen, T2_BUFSIZE-wrlen,
> + " TLV%d Type=%u Len=%u", i+1,
> + ctx_hdrs[i].type, ctx_hdrs[i].len);
> + data = ctx_hdrs[i].data;
> + for (j = 0; j < ctx_hdrs[i].len; j++)
> + wrlen += snprintf(t2_buf+wrlen, T2_BUFSIZE-wrlen,
> + " 0x%08x", data[j]);
> + }
> + pr_info("%s\n", t2_buf);
> + return 0;
> +}
> +
> +static struct nsh_listener nsh_log_t1_entry
Re: [net-next 13/15] i40e/i40evf: use logical operators, not bitwise
On Wed, 2016-02-17 at 19:38 -0800, Jeff Kirsher wrote:
> From: Mitch Williams
>
> Mr. Spock would certainly raise an eyebrow to see us using bitwise
> operators, when we should clearly be relying on logic. Fascinating.
I think it read better before this change.
Spock might have looked at the type of the
variable before raising that eyebrow.
clean_complete is bool so it's not actually a
bitwise operation,
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
[]
> @@ -1996,7 +1996,8 @@ int i40e_napi_poll(struct napi_struct *napi, int budget)
> * budget and be more aggressive about cleaning up the Tx descriptors.
> */
> i40e_for_each_ring(ring, q_vector->tx) {
> - clean_complete &= i40e_clean_tx_irq(ring, vsi->work_limit);
> + clean_complete = clean_complete &&
> + i40e_clean_tx_irq(ring, vsi->work_limit);
etc...
Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types
On Sat, 2016-01-30 at 09:51 -0800, Eric Dumazet wrote: > On Sat, 2016-01-30 at 12:05 -0200, Lucas Tanure wrote: > > On Sat, Jan 30, 2016 at 11:45 AM, Patrick McHardy wrote: > > > On 30.01, Lucas Tanure wrote: > > > > As suggested by checkpatch.pl: > > > > CHECK: Prefer kernel type 'uX' over 'uintX_t' > > > > > > You might have noticed we have literally hundreds of them spread over 100 > > > files in the netfilter code. We'll gradually change them when the code is > > > touched anyways. > > > > > > > net/ipv4/netfilter/ip_tables.c | 5 ++--- > > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > Yes, I checked that. But would be better to change that now? > > Because: > > - could take years to anyone to touch the code, as the code already > > works very well > > - be more standardized could facilitate reading the code > > - It's a good way to encourage new people to contribute to the code > > > > Thanks! > > These changes are a pain for people having to constantly backport fixes > into stable kernels, or rebase their patches before upstream > submissions. > > Things like 'git cherry-pick' , 'git rebase' no longer work. > This is a huge pain, and manual editing to resolve conflicts often > add bugs. > > Really, do you believe the 'uX' over 'uintX_t' stuff really matters for > people working on adding new features and fixing bugs ? > > I am certain that if you had to work like us, you would quickly see the > utility of such changes is negative. > > Sure, new submissions should be clean, but 'fixing' old code is not > worth it. That might depend on whether or not the linux kernel is a "long-life project" and whether or no any old branch of it is also important and sufficiently long-life. The active life of a backport branch for the linux kernel seems to be 3 or 4 years. The linux kernel will likely be useful for a few more decades beyond that. Complex and long-life projects like the linux kernel might benefit more in code complexity reduction patches like these rather than code stasis for backward porting ease. In general, arguing for stasis leads to ossification, slow decline. Change for change's sake is poor, but changes to reduce complexity, improve maintainability (for some measure of it) and especially improve performance should be welcomed where feasible.
Re: [PATCH 3/4] netfilter: ipv4: use preferred kernel types
On Sat, 2016-01-30 at 12:05 -0200, Lucas Tanure wrote: > On Sat, Jan 30, 2016 at 11:45 AM, Patrick McHardy wrote: > > On 30.01, Lucas Tanure wrote: > > > As suggested by checkpatch.pl: > > > CHECK: Prefer kernel type 'uX' over 'uintX_t' > > > > You might have noticed we have literally hundreds of them spread over 100 > > files in the netfilter code. We'll gradually change them when the code is > > touched anyways. > > > > > net/ipv4/netfilter/ip_tables.c | 5 ++--- > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > Yes, I checked that. But would be better to change that now? > Because: > - could take years to anyone to touch the code, as the code already > works very well > - be more standardized could facilitate reading the code > - It's a good way to encourage new people to contribute to the code The last one bullet point is what staging is for. It might be better to do them all at once: $ git grep --name-only "\bu_int" net/netfilter/ | \ xargs perl -p -i -e 's/\bu_int(\d+)_t\b/u\1/g'
Re: [PATCH] lib: fix callers of strtobool to use char array
On Wed, 2016-01-27 at 16:45 -0800, Kees Cook wrote: > Some callers of strtobool were passing a pointer to unterminated strings. > This fixes the issue and consolidates some logic in cifs. This may be incomplete as it duplicates the behavior for the old number of characters, but this is not a solution for the entry of a bool that is "on" or "off". > diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c [] > @@ -290,7 +305,8 @@ static ssize_t cifs_stats_proc_write(struct file *file, > } > } > spin_unlock(&cifs_tcp_ses_lock); > - } > + } else > + return rc; Likely better to reverse the test and unindent the preceding block. Otherwise, please make sure to use the general brace form of when one branch needs braces, the other branch should have them too.
Re: [PATCH v2] net: Add eth_platform_get_mac_address() helper.
On Wed, 2016-01-06 at 19:01 -0500, David Miller wrote: > The caller checks for NULL, and this is the default implementation > doing precisely what it is meant to do. Yeah, I noticed that later. Nevermind... cheers, Joe -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] net: Add eth_platform_get_mac_address() helper.
On Thu, 2016-01-07 at 00:26 +0100, Bjørn Mork wrote:
> Joe Perches writes:
> > On Wed, 2016-01-06 at 16:33 -0500, David Miller wrote:
> > > A repeating pattern in drivers has become to use OF node information
> > > and, if not found, platform specific host information to extract the
> > > ethernet address for a given device.
> > []
> > > diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> > []
> > > @@ -485,3 +487,32 @@ static int __init eth_offload_init(void)
> > > }
> > >
> > > fs_initcall(eth_offload_init);
> > > +
> > > +unsigned char * __weak arch_get_platform_mac_address(void)
> > > +{
> > > + return NULL;
> >
> > WARN_ON_ONCE ?
>
> That would prevent a driver from using this with additional fallback
> methods. For what reason?
It's declared __weak and should be overridden by
an arch specific function.
A NULL address would cause a fault when using
a function like copy_ether_addr.
> I don't have a specific usecase, but I can imagine drivers falling back
> to e.g a random address without wanting to be noisy about it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] net: Add eth_platform_get_mac_address() helper.
On Wed, 2016-01-06 at 16:33 -0500, David Miller wrote:
> A repeating pattern in drivers has become to use OF node information
> and, if not found, platform specific host information to extract the
> ethernet address for a given device.
[]
> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
[]
> @@ -485,3 +487,32 @@ static int __init eth_offload_init(void)
> }
>
> fs_initcall(eth_offload_init);
> +
> +unsigned char * __weak arch_get_platform_mac_address(void)
> +{
> + return NULL;
WARN_ON_ONCE ?
> +}
> +
> +int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
> +{
> + const unsigned char *addr;
> + struct device_node *dp;
> +
> + if (dev_is_pci(dev))
> + dp = pci_device_to_OF_node(to_pci_dev(dev));
> + else
> + dp = dev->of_node;
> +
> + addr = NULL;
> + if (dp)
> + addr = of_get_mac_address(dp);
> + if (!addr)
> + addr = arch_get_platform_mac_address();
> +
> + if (!addr)
> + return -ENODEV;
> +
> + ether_addr_copy(mac_addr, addr);
Couldn't some oddball arch have an unaligned addr?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next 1/2] Support outside netns for tunnels.
On Mon, 2016-01-04 at 11:47 -0800, Tom Herbert wrote:
> On Mon, Jan 4, 2016 at 10:45 AM, Saurabh Mohan
> wrote:
> >
> > This patch enchances a tunnel interface, like gre, to have the tunnel
> > encap/decap be in the context of a network namespace that is different from
> > the namespace of the tunnel interface.
> >
> > From userspace this feature may be configured using the new 'onetns'
> > keyword:
> > ip netns exec custa ip link add dev tun1 type gre local 10.0.0.1 \
> > remote 10.0.0.2 onetns outside
> >
> > In the above example the tunnel would be in the 'custa' namespace and the
> > tunnel endpoints would be in the 'outside' namespace.
> >
> > Also, proposing the use of netns name 'global' to specify the global
> > namespace.
> >
> > If this patch set is accepted then I will add support for other tunnels as
> > well.
> >
> This might be interesting. Can you please ad a 0/n patch that
> describes the motivation for this, in particular I would like to know
> if this has a positive impact on ns performance if somehow we are
> eliminating indirection.
[]
> > diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
[]
> > @@ -3,6 +3,7 @@
> >
> > #include
> > #include
> > +#include
> >
> >
> > #define SIOCGETTUNNEL (SIOCDEVPRIVATE + 0)
> > @@ -27,6 +28,14 @@
> > #define GRE_FLAGS __cpu_to_be16(0x00F8)
> > #define GRE_VERSION__cpu_to_be16(0x0007)
> >
> > +struct o_netns_parm {
> > + __u8o_netns_flag;
> > + __u32 o_netns_fd;
> > + charnetns[NAME_MAX];
> > +};
Trivia:
It could eliminate a few padding bytes if the
o_netns_fd and o_netns_flag fields were reversed.
and netns[NAME_MAX] is normally netns[NAME_MAX + 1]
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH net-next 05/24] phy: add phydev_name() macro
On Mon, 2016-01-04 at 18:36 +0100, Andrew Lunn wrote:
> Add a phydev_name() macro, to help with moving some structure members
> from phy_device.
[]
> diff --git a/include/linux/phy.h b/include/linux/phy.h
[]
> @@ -783,6 +783,8 @@static inline int phy_read_status(struct phy_device
> *phydev)
> #define phydev_dbg(_phydev, format, args...) \
> dev_dbg(&_phydev->dev, format, ##args)
>
> +#define phydev_name(_phydev) dev_name(&_phydev->dev)
> +
This should use parentheses around phydev
#define phydev_name(phydev) dev_name(&(phydev)->dev)
or likely even better be a static inline
static inline const char * phydev_name(const struct phy_device *phydev)
{
return dev_name(&phydev->dev);
}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/12] net-next: mediatek: add switch driver for mt7621
On Sun, 2016-01-03 at 16:26 +0100, John Crispin wrote:
> This driver is very basic and only provides basic init and irq support.
> The SoC and switch core both have support for a special tag making DSA
> support possible.
trivia:
> diff --git a/drivers/net/ethernet/mediatek/gsw_mt7621.c
> b/drivers/net/ethernet/mediatek/gsw_mt7621.c
[]
> +static irqreturn_t gsw_interrupt_mt7621(int irq, void *_priv)
> +{
> + struct fe_priv *priv = (struct fe_priv *)_priv;
> + struct mt7620_gsw *gsw = (struct mt7620_gsw *)priv->soc->swpriv;
> + u32 reg, i;
> +
> + reg = mt7530_mdio_r32(gsw, 0x700c);
> +
> + for (i = 0; i < 5; i++)
> + if (reg & BIT(i)) {
> + unsigned int link;
> +
> + link = mt7530_mdio_r32(gsw,
> + 0x3008 + (i * 0x100)) & 0x1;
> +
> + if (link != priv->link[i]) {
> + priv->link[i] = link;
> + if (link)
> + netdev_info(priv->netdev,
> + "port %d link up\n", i);
> + else
> + netdev_info(priv->netdev,
> + "port %d link down\n", i);
> + }
> + }
It's more common to use braces after the for loop.
Perhaps this could be written with a continue; to reduce
indentation and also use a single netdev_info().
for (i = 0; i < 5; i++) {
unsigned int link;
if (!(reg & BIT(i)))
continue;
link = mt7530_mdio_r32(gsw, 0x3008 + (i * 0x100)) & 0x1;
if (link == priv->link[i])
continue;
priv->link[i] = link;
netdev_info(priv->netdev, "port %d link %s\n",
i, link ? "up" : "down");
}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] xen-netback: Fine-tuning for three function implementations
On Sat, 2016-01-02 at 18:50 +0100, SF Markus Elfring wrote:
> A few update suggestions were taken into account
> from static source code analysis.
While static analysis can be useful, I don't think these
specific conversions are generally useful.
Perhaps it would be more useful to convert the string
duplication or snprintf logic to kstrdup/kasprintf
This:
if (num_queues == 1) {
xspath = kzalloc(strlen(dev->otherend) + 1, GFP_KERNEL);
if (!xspath) {
xenbus_dev_fatal(dev, -ENOMEM,
"reading ring references");
return -ENOMEM;
}
strcpy(xspath, dev->otherend);
} else {
xspathsize = strlen(dev->otherend) + xenstore_path_ext_size;
xspath = kzalloc(xspathsize, GFP_KERNEL);
if (!xspath) {
xenbus_dev_fatal(dev, -ENOMEM,
"reading ring references");
return -ENOMEM;
}
snprintf(xspath, xspathsize, "%s/queue-%u", dev->otherend,
queue->id);
}
could be simplified to something like:
if (num_queues == 1)
xspath = kstrdup(dev->otherend, GFP_KERNEL);
else
xspath = kasprintf(GFP_KERNEL, "%s/queue-%u",
dev->otherend, queue->id);
if (!xspath)
etc...
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH next] net/core/dev: Warn on an impossibly short offload frame
On Sat, 2016-01-02 at 19:25 -0500, Aaron Conole wrote:
> When signaling that a GRO frame is ready to be processed, the network stack
> correctly checks length and aborts processing when a frame is less than 14
> bytes. However, such a condition is really indicative of a broken driver,
> and should be loudly signaled, rather than silently dropped as the case is
> today.
>
> Convert the condition to use WARN_ON() to ensure that the stack loudly
> complains about such broken drivers.
[]
> diff --git a/net/core/dev.c b/net/core/dev.c
[]
> @@ -4579,7 +4579,7 @@ static struct sk_buff *napi_frags_skb(struct
> napi_struct *napi)
> eth = skb_gro_header_fast(skb, 0);
> if (unlikely(skb_gro_header_hard(skb, hlen))) {
> eth = skb_gro_header_slow(skb, hlen, 0);
> - if (unlikely(!eth)) {
> + if (WARN_ON(!eth)) {
> napi_reuse_skb(napi, skb);
> return NULL;
> }
It's generally a good idea to use
WARN_ON_RATELIMIT or WARN_ON_ONCE.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] /drivers/net/wireless/ath/ath9k remove unnecessary ?: operator
On Tue, 2015-12-29 at 01:38 +0700, Ivan Safonov wrote:
> On 12/29/2015 12:56 AM, Joe Perches wrote:
> > On Mon, 2015-12-28 at 20:48 +0700, Ivan Safonov wrote:
> > > ((thermometer < 0) ? 0 : (thermometer == X)) and (thermometer ==
> > > X) are equal for X >= 0.
> > X is not guaranteed to be >= 0 here
>
> X is fixed constant. In this case X is {0, 1, 2}.
Looks like it can be -1 to me (range: -1, 0, 1, 2)
static int ar9003_hw_get_thermometer(struct ath_hw *ah)
{
struct ar9300_eeprom *eep = &ah->eeprom.ar9300_eep;
struct ar9300_base_eep_hdr *pBase = &eep->baseEepHeader;
int thermometer = (pBase->miscConfiguration >> 1) & 0x3;
return --thermometer;
}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] /drivers/net/wireless/ath/ath9k remove unnecessary ?: operator
On Mon, 2015-12-28 at 20:48 +0700, Ivan Safonov wrote:
> ((thermometer < 0) ? 0 : (thermometer == X)) and (thermometer == X) are equal
> for X >= 0.
X is not guaranteed to be >= 0 here
> Signed-off-by: Ivan Safonov
> ---
> drivers/net/wireless/ath/ath9k/ar9003_eeprom.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
> b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
[]
> @@ -4097,16 +4097,16 @@ static void ar9003_hw_thermometer_apply(struct ath_hw
> *ah)
> REG_RMW_FIELD(ah, AR_PHY_65NM_CH2_RXTX4,
> AR_PHY_65NM_CH0_RXTX4_THERM_ON_OVR, therm_on);
>
> - therm_on = (thermometer < 0) ? 0 : (thermometer == 0);
> + therm_on = thermometer == 0;
This code is not equivalent.
Check what happens when thermometer is -1.
> REG_RMW_FIELD(ah, AR_PHY_65NM_CH0_RXTX4,
> AR_PHY_65NM_CH0_RXTX4_THERM_ON, therm_on);
> if (pCap->chip_chainmask & BIT(1)) {
> - therm_on = (thermometer < 0) ? 0 : (thermometer == 1);
> + therm_on = thermometer == 1;
> REG_RMW_FIELD(ah, AR_PHY_65NM_CH1_RXTX4,
> AR_PHY_65NM_CH0_RXTX4_THERM_ON, therm_on);
> }
> if (pCap->chip_chainmask & BIT(2)) {
> - therm_on = (thermometer < 0) ? 0 : (thermometer == 2);
> + therm_on = thermometer == 2;
> REG_RMW_FIELD(ah, AR_PHY_65NM_CH2_RXTX4,
> AR_PHY_65NM_CH0_RXTX4_THERM_ON, therm_on);
> }
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: pull-request: mac80211 2015-12-15
On Thu, 2015-12-17 at 13:44 +0100, Johannes Berg wrote: > On Wed, 2015-12-16 at 18:34 -0500, David Miller wrote: > > > > Something about your text encoding kept this from ending up > > in patchwork for some reason. > > > > Hm. I don't see anything special with this, seems to just be plain > text > 8bit transfer encoding. > > Do you want me to watch out for things getting into patchwork in the > future? > Hey Johannes. You seem to be using: X-Mailer: Evolution 3.18.1-1 which, to be overly technical, _sucks_. The new composer for Evolution has way too many defects to list. It adds non-breaking spaces (NBSP) characters instead of a standard ASCII space in various places. Every version of Evolution since 3.14 is terrible at sending text only messages. 3.12 works -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/15] add Intel(R) X722 iWARP driver
On Wed, 2015-12-16 at 13:58 -0600, Faisal Latif wrote: > This series contains the addition of the i40iw.ko driver. This series should probably be respun against -next instead of linus' tree. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/15] i40iw: add main, hdr, status
On Wed, 2015-12-16 at 13:58 -0600, Faisal Latif wrote:
> i40iw_main.c contains routines for i40e <=> i40iw interface and setup.
> i40iw.h is header file for main device data structures.
> i40iw_status.h is for return status codes.
[]
> diff --git a/drivers/infiniband/hw/i40iw/i40iw.h
> b/drivers/infiniband/hw/i40iw/i40iw.h
[]
> +#define i40iw_pr_err(fmt, args ...) pr_err("%s: error " fmt, __func__, ##
> args)
> +
> +#define i40iw_pr_info(fmt, args ...) pr_info("%s: " fmt, __func__, ## args)
> +
> +#define i40iw_pr_warn(fmt, args ...) pr_warn("%s: " fmt, __func__, ## args)
Using "error " in the output doesn't really add much
as there's already a KERN_ERR with the output.
Using __func__ hardly adds anything.
Using netdev_ is generally preferred
> +
> +struct i40iw_cqp_request {
> + struct cqp_commands_info info;
> + wait_queue_head_t waitq;
> + struct list_head list;
> + atomic_t refcount;
> + void (*callback_fcn)(struct i40iw_cqp_request*, u32);
> + void *param;
> + struct i40iw_cqp_compl_info compl_info;
> + u8 waiting:1;
> + u8 request_done:1;
> + u8 dynamic:1;
> + u8 polling:1;
These would bitfields might be better as bool
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] i40e: Add support for client interface for IWARP driver
On Wed, 2015-12-16 at 13:58 -0600, Faisal Latif wrote: > From: Anjali Singhai Jain > > This patch adds a Client interface for i40iw driver > support. Also expands the Virtchannel to support messages > from i40evf driver on behalf of i40iwvf driver. [] > diff --git a/drivers/net/ethernet/intel/i40e/i40e_client.c > b/drivers/net/ethernet/intel/i40e/i40e_client.c [] > + * Contact Information: > + * e1000-devel Mailing List trivia: This should probably be: intel-wired-...@lists.osuosl.org -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] ipv6: add IPV6_HDRINCL option for raw sockets
On Wed, 2015-12-16 at 17:22 +0100, Hannes Frederic Sowa wrote:
> Same as in Windows, we miss IPV6_HDRINCL for SOL_IPV6 and SOL_RAW.
> The SOL_IP/IP_HDRINCL is not available for IPv6 sockets.
[]
> diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
[]
> @@ -972,6 +972,11 @@ static int do_rawv6_setsockopt(struct sock *sk,
> int level, int optname,
> return -EFAULT;
>
> switch (optname) {
> + case IPV6_HDRINCL:
> + if (sk->sk_type != SOCK_RAW)
> + return -EINVAL;
> + inet_sk(sk)->hdrincl = !!val;
trivia:
ipv4/sockglue.c uses the ternary '? 1 : 0' convention for this.
It might be nicer to be consistent.
Then again sockglue.c is inconsistent about that too.
net/ipv4/ip_sockglue.c:736: inet->hdrincl = val ? 1 : 0;
net/ipv4/ip_sockglue.c:743: inet->nodefrag = val ? 1 : 0;
net/ipv4/ip_sockglue.c:746: inet->bind_address_no_port = val ? 1 :
0;
net/ipv4/ip_sockglue.c:754: inet->recverr = !!val;
net/ipv4/ip_sockglue.c:772: inet->mc_loop = !!val;
net/ipv4/ip_sockglue.c:1123:inet->freebind = !!val;
There's no change to object code either way.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 8/8] treewide: Remove newlines inside DEFINE_PER_CPU() macros
On Mon, 2015-12-07 at 17:53 +0100, Michal Marek wrote: > On 2015-12-07 17:33, David Laight wrote: > > From: Michal Marek > > > Sent: 04 December 2015 15:26 > > > Otherwise make tags can't parse them: > > > > > > ctags: Warning: arch/ia64/kernel/smp.c:60: null expansion of name pattern > > > "\1" > > ... > > > > Seems to me you need to fix ctags. > > I'm sure the maintainers of ctags and etags would accept patches to > describe a custom context-free grammar via commandline options, but > until then, let's continue using the regular expressions in tags.sh and > remove newlines in macros that tags.sh is trying to expand. > Do you have a list of the most common macros? Perhaps it'd be good to add exceptions to checkpatch 80 column line rules for them. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] net: hns: optimize XGE capability by reducing cpu usage
On Mon, 2015-12-07 at 16:58 +0800, Yankejian (Hackim Yim) wrote:
> On 2015/12/7 11:32, Joe Perches wrote:
> > On Sun, 2015-12-06 at 22:29 -0500, David Miller wrote:
> > > > From: yankejian
> > > > Date: Sat, 5 Dec 2015 15:32:29 +0800
> > > >
> > > > > > +#if (PAGE_SIZE < 8192)
> > > > > > + if (hnae_buf_size(ring) == HNS_BUFFER_SIZE_2048) {
> > > > > > + truesize = hnae_buf_size(ring);
> > > > > > + } else {
> > > > > > + truesize = ALIGN(size, L1_CACHE_BYTES);
> > > > > > + last_offset = hnae_page_size(ring) -
> > > > > > hnae_buf_size(ring);
> > > > > > + }
> > > > > > +
> > > > > > +#else
> > > > > > + truesize = ALIGN(size, L1_CACHE_BYTES);
> > > > > > + last_offset = hnae_page_size(ring) -
> > > > > > hnae_buf_size(ring);
> > > > > > +#endif
> > > >
> > > > This is not indented properly, and it looks terrible.
> > And it makes one curious as to why last_offset isn't set
> > in the first block.
>
> Hi Joe,
Hello.
> if hnae_buf_size que equal to HNS_BUFFER_SIZE, last_offset is useless in the
> routines of this function.
> so it is ignored in the first block. thanks for your suggestion.
More to the point, last_offset is initialized to 0.
It'd be clearer not to initialize it at all and
set it to 0 in the first block and not overwrite
the initialization in each subsequent block.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] net: hns: optimize XGE capability by reducing cpu usage
On Sun, 2015-12-06 at 22:29 -0500, David Miller wrote:
> From: yankejian
> Date: Sat, 5 Dec 2015 15:32:29 +0800
>
> > +#if (PAGE_SIZE < 8192)
> > + if (hnae_buf_size(ring) == HNS_BUFFER_SIZE_2048) {
> > + truesize = hnae_buf_size(ring);
> > + } else {
> > + truesize = ALIGN(size, L1_CACHE_BYTES);
> > + last_offset = hnae_page_size(ring) - hnae_buf_size(ring);
> > + }
> > +
> > +#else
> > + truesize = ALIGN(size, L1_CACHE_BYTES);
> > + last_offset = hnae_page_size(ring) - hnae_buf_size(ring);
> > +#endif
>
> This is not indented properly, and it looks terrible.
And it makes one curious as to why last_offset isn't set
in the first block.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mwifiex: fix semicolon.cocci warnings
On Sun, 2015-12-06 at 22:56 +0100, Julia Lawall wrote:
> Remove unneeded semicolon.
>
> Generated by: scripts/coccinelle/misc/semicolon.cocci
There are a lot more of these
arch/arm/mach-ixp4xx/goramo_mlr.c:78:static u32 hw_bits = 0xFFFD;/*
assume all hardware present */;
crypto/tgr192.c:563:tgr192_update(desc, NULL, 0); /* flush */ ;
crypto/tgr192.c:591:tgr192_update(desc, NULL, 0); /* flush */ ;
drivers/block/drbd/drbd_int.h:982: struct list_head list; /* on
device->pending_bitmap_io */;
drivers/block/drbd/drbd_state.c:2202: enum drbd_state_rv err, rv =
SS_UNKNOWN_ERROR; /* continue waiting */;
drivers/block/drbd/drbd_main.c:1453:return drop_it; /* && (device->state ==
R_PRIMARY) */;
drivers/block/ataflop.c:779:dma_wd.fdc_speed = 0; /* always seek
with 8 Mhz */;
drivers/scsi/bnx2i/bnx2i.h:540: u16 itt[BNX2X_MAX_CQS];
/* Cstorm CQ sequence to notify array, updated by driver */;
drivers/net/ethernet/chelsio/cxgb4vf/sge.c:1802:for
(frag = 0, fp = gl.frags; /**/; frag++, fp++) {
drivers/net/wireless/marvell/mwifiex/init.c:98: priv->beacon_period = 100; /*
beacon interval */ ;
drivers/net/wireless/intel/iwlwifi/mvm/fw-api-power.h:313:}; /*
TX_POWER_REDUCED_FLAGS_TYPE_API_E_VER_2 */;
drivers/net/wireless/intersil/prism54/islpci_dev.c:112: return
-EILSEQ; /* Illegal byte sequence */;
drivers/spi/spi-bcm2835.c:239: bs->dma_pending = 0;
/* and mark as completed */;
drivers/spi/spi-topcliff-pch.c:879: param->chan_id = data->ch * 2; /* Tx =
0, 2 */;
drivers/spi/spi-topcliff-pch.c:894: param->chan_id = data->ch * 2 + 1; /*
Rx = Tx + 1 */;
drivers/usb/serial/keyspan.c:1966: dr->bRequest = 0xB0;/* 49wg
control message */;
drivers/usb/serial/oti6858.c:112: u8 rx_bytes_avail; /*
number of bytes in rx buffer */;
drivers/clk/tegra/clk-dfll.c:723: int coef = 128; /* FIXME: td->cg_scale?
*/;
drivers/media/pci/ivtv/ivtv-driver.h:532: u8 even[2]; /* two-byte
payload of even field */;
fs/xfs/libxfs/xfs_btree.h:98: case XFS_BTNUM_MAX: ASSERT(0); /* fucking gcc
*/ ; break; \
fs/xfs/libxfs/xfs_btree.h:118: case XFS_BTNUM_MAX: ASSERT(0); /* fucking gcc
*/ ; break; \
fs/xfs/xfs_itable.c:375:ac.ac_ubleft = ubcount * statstruct_size; /*
bytes */;
include/uapi/linux/nfc.h:278: char service_name[NFC_LLCP_MAX_SERVICE_NAME];
/* Service name URI */;
kernel/trace/ring_buffer.c:4896:set_current_state(TASK_INTERRUPTIBLE);
/* Just run for 10 seconds */;
sound/soc/intel/atom/sst/sst.c:71: sst_shim_write64(drv->shim,
drv->ipc_reg.ipcx, header.full);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] MAINTAINERS: add myself as Renesas Ethernet drivers reviewer
On Sat, 2015-12-05 at 04:13 +0300, Sergei Shtylyov wrote: > On 12/05/2015 04:02 AM, Joe Perches wrote: > > > Since you seem to be get_maintainers.pl maintainer, I (and not only I) > > > would like to propose a switch to suppress the committers/authors/etc. > > > It's > > > generally annoying and unhelpful to have the bunch of people listed, none > > > of > > > which usually are a good addressees for the patches... > > > > Done! > > > Add "--nogit" and "--nogit-fallback" > > Em, --nogit is the dafault, according to --help. --nogit-fallback is > enough but it's undocumented. :-/ umm $ ./scripts/get_maintainer.pl --help [...] --git-fallback => use git when no exact MAINTAINERS pattern (default: 1) [...] Most options have both positive and negative forms. The negative forms for -- are --no and --no-. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] MAINTAINERS: add myself as Renesas Ethernet drivers reviewer
On Sat, 2015-12-05 at 03:57 +0300, Sergei Shtylyov wrote: > Since you seem to be get_maintainers.pl maintainer, I (and not only I) > would like to propose a switch to suppress the committers/authors/etc. It's > generally annoying and unhelpful to have the bunch of people listed, none of > which usually are a good addressees for the patches... Done! Add "--nogit" and "--nogit-fallback" -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] MAINTAINERS: add myself as Renesas Ethernet drivers reviewer
On Sat, 2015-12-05 at 03:03 +0300, Sergei Shtylyov wrote: > liAdd myself as a reviewer for the Renesas Ethernet drivers -- > hopefully I > won't miss the buggy patches anymore. :-) > > Signed-off-by: Sergei Shtylyov > > --- > The patch is against DaveM's 'net.git' repo. > > MAINTAINERS |8 > 1 file changed, 8 insertions(+) > > Index: net/MAINTAINERS > === > --- net.orig/MAINTAINERS > +++ net/MAINTAINERS > @@ -8929,6 +8929,14 @@ F: drivers/rpmsg/ > F: Documentation/rpmsg.txt > F: include/linux/rpmsg.h > > +RENESAS ETHERNET DRIVERS > +R: Sergei Shtylyov > +L: net...@vger.kernel.orgc > +L: linux...@vger.kernel.org > +S: Maintained > +F: drivers/net/ethernet/renesas/ > +F: include/linux/sh_eth.h Hello Sergei Typically a reviewer isn't a maintainer. Does anyone actually maintain this? I suggest you mark this "S: Odd fixes" or remove the "S:" line altogether unless you really want to become the maintainer. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next v5 2/8] dpaa_eth: add support for DPAA Ethernet
On Fri, 2015-12-04 at 14:55 -0500, David Miller wrote: > From: Madalin Bucur > Date: Thu, 3 Dec 2015 15:49:43 +0200 > > > @@ -0,0 +1,22 @@ > > +menuconfig FSL_DPAA_ETH > > + tristate "DPAA Ethernet" > > + depends on FSL_SOC && FSL_BMAN && FSL_QMAN && FSL_FMAN > > + select PHYLIB > > + select FSL_FMAN_MAC > > I do not see the FSL_FMAN_MAC Kconfig symbol defined anywhere in the > tree. I believe this patch series is dependent on two other patch series mentioned in the cover letter. --- The latest FMan driver patches were submitted by Igal Liberman: https://patchwork.ozlabs.org/project/netdev/list/?submitter=64715&state=* The latest Q/BMan drivers were submitted by Roy Pledge: https://patchwork.ozlabs.org/project/linuxppc-dev/list/?submitter=66331&state=* -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: use-after-free in sctp_do_sm
On Fri, 2015-12-04 at 11:47 -0500, Jason Baron wrote: > When DYNAMIC_DEBUG is enabled we have this wrapper from > include/linux/dynamic_debug.h: > > if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT)) > > > So the compiler is not emitting the side-effects in this > case. Huh? Do I misunderstand what you are writing? You are testing a variable that is not generally set so the call is not being performed in the general case, but the compiler can not elide the code. If the variable was enabled via the control file, the __dynamic_pr_debug would be performed with the use-after-free. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next 01/17] fm10k: Fix error handling in the function fm10k_setup_tc for certain function calls
On Thu, 2015-12-03 at 16:29 -0800, Jeff Kirsher wrote: > From: Nick I guess this is an OK patch, but the From line isn't correct (missing full name) and the changelog is nigh on unreadable. > This fixes the function fm10k_setup_tc to properly check if the > calls to either the function fm10k_init_queueing_scheme or the > function fm10k_mbx_request_irq fail by returning a error code to > signal that the call to either function has failed. Furthermore > if this arises exit immediately from the function fm10k_setup_tc > by returning the returned error code from the failed function call > to signal to the caller that setting up the tc on the device has > failed and the caller needs to handle this failed setup. Maybe something like: Add error handling in fm10k_setup_tc of fm10k_init_queueing_scheme and fm10k_mbx_request_irq. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: use-after-free in sctp_do_sm
(adding lkml as this is likely better discussed there) On Thu, 2015-12-03 at 15:42 -0500, Jason Baron wrote: > On 12/03/2015 03:24 PM, Joe Perches wrote: > > On Thu, 2015-12-03 at 15:10 -0500, Jason Baron wrote: > > > On 12/03/2015 03:03 PM, Joe Perches wrote: > > > > On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote: > > > > > On 12/03/2015 01:52 PM, Aaron Conole wrote: > > > > > > I think that as a minimum, the following patch should be evaluted, > > > > > > but am unsure to whom I should submit it (after I test): > > > > [] > > > > > Agreed - the intention here is certainly to have no side effects. It > > > > > looks like 'no_printk()' is used in quite a few other places that > > > > > would > > > > > benefit from this change. So we probably want a generic > > > > > 'really_no_printk()' macro. > > > > > > > > https://lkml.org/lkml/2012/6/17/231 > > > > > > I don't see this in the tree. > > > > It never got applied. > > > > > Also maybe we should just convert > > > no_printk() to do what your 'eliminated_printk()'. > > > > Some of them at least. > > > > > So we can convert all users with this change? > > > > I don't think so, I think there are some > > function evaluation/side effects that are > > required. I believe some do hardware I/O. > > > > It'd be good to at least isolate them. > > > > I'm not sure how to find them via some > > automated tool/mechanism though. > > > > I asked Julia Lawall about it once in this > > thread: https://lkml.org/lkml/2014/12/3/696 > > > > Seems rather fragile to have side effects that we rely > upon hidden in a printk(). Yup. > Just convert them and see what breaks :) I appreciate your optimism. It's very 1995. Try it and see what happens. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: use-after-free in sctp_do_sm
On Thu, 2015-12-03 at 15:10 -0500, Jason Baron wrote: > On 12/03/2015 03:03 PM, Joe Perches wrote: > > On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote: > > > On 12/03/2015 01:52 PM, Aaron Conole wrote: > > > > I think that as a minimum, the following patch should be evaluted, > > > > but am unsure to whom I should submit it (after I test): > > [] > > > Agreed - the intention here is certainly to have no side effects. It > > > looks like 'no_printk()' is used in quite a few other places that would > > > benefit from this change. So we probably want a generic > > > 'really_no_printk()' macro. > > > > https://lkml.org/lkml/2012/6/17/231 > > I don't see this in the tree. It never got applied. > Also maybe we should just convert > no_printk() to do what your 'eliminated_printk()'. Some of them at least. > So we can convert all users with this change? I don't think so, I think there are some function evaluation/side effects that are required. I believe some do hardware I/O. It'd be good to at least isolate them. I'm not sure how to find them via some automated tool/mechanism though. I asked Julia Lawall about it once in this thread: https://lkml.org/lkml/2014/12/3/696 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: use-after-free in sctp_do_sm
On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote: > On 12/03/2015 01:52 PM, Aaron Conole wrote: > > I think that as a minimum, the following patch should be evaluted, > > but am unsure to whom I should submit it (after I test): [] > Agreed - the intention here is certainly to have no side effects. It > looks like 'no_printk()' is used in quite a few other places that would > benefit from this change. So we probably want a generic > 'really_no_printk()' macro. https://lkml.org/lkml/2012/6/17/231 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: use-after-free in sctp_do_sm
On Thu, 2015-12-03 at 13:52 -0500, Aaron Conole wrote:
> Dmitry Vyukov writes:
> > On Thu, Dec 3, 2015 at 6:02 PM, Eric Dumazet wrote:
> > > On Thu, Dec 3, 2015 at 7:55 AM, Dmitry Vyukov wrote:
> > > > On Thu, Dec 3, 2015 at 3:48 PM, Eric Dumazet wrote:
> > > > > >
> > > > > > No, I don't. But pr_debug always computes its arguments. See
> > > > > > no_printk
> > > > > > in printk.h. So this use-after-free happens for all users.
> > > > >
> > > > > Hmm.
> > > > >
> > > > > pr_debug() should be a nop unless either DEBUG or
> > > > > CONFIG_DYNAMIC_DEBUG are set
> > > > >
> > > > > On our production kernels, pr_debug() is a nop.
> > > > >
> > > > > Can you double check ? Thanks !
> > > >
> > > >
> > > > Why should it be nop? no_printk thing in printk.h pretty much
> > > > explicitly makes it not a nop...
>
> Because it was until commit 5264f2f75d8. It also violates my reading of
> the following from printk.h:
>
> * All of these will print unconditionally, although note that pr_debug()
> * and other debug macros are compiled out unless either DEBUG is defined
> * or CONFIG_DYNAMIC_DEBUG is set.
>
> > > >
> > > > Double-checked: debug_post_sfx leads to some generated code:
> > > >
> > > > debug_post_sfx();
> > > > 8229f256: 48 8b 85 58 fe ff ffmov-0x1a8(%rbp),%rax
> > > > 8229f25d: 48 85 c0test %rax,%rax
> > > > 8229f260: 74 24 je
> > > > 8229f286
> > > > 8229f262: 8b b0 a8 00 00 00 mov0xa8(%rax),%esi
> > > > 8229f268: 48 8b 85 60 fe ff ffmov-0x1a0(%rbp),%rax
> > > > 8229f26f: 44 89 85 74 fe ff ffmov%r8d,-0x18c(%rbp)
> > > > 8229f276: 48 8b 78 20 mov0x20(%rax),%rdi
> > > > 8229f27a: e8 71 28 01 00 callq
> > > > 822b1af0
> > > > 8229f27f: 44 8b 85 74 fe ff ffmov-0x18c(%rbp),%r8d
> > > >
> > > > return error;
> > > > }
> > > > 8229f286: 48 81 c4 a0 01 00 00add$0x1a0,%rsp
> > > > 8229f28d: 44 89 c0mov%r8d,%eax
> > > > 8229f290: 5b pop%rbx
> > > > 8229f291: 41 5c pop%r12
> > > > 8229f293: 41 5d pop%r13
> > > > 8229f295: 41 5e pop%r14
> > > > 8229f297: 41 5f pop%r15
> > > > 8229f299: 5d pop%rbp
> > > > 8229f29a: c3 retq
> > >
> > > This is a serious concern, because we let in the past lot of patches
> > > converting traditional
>
> +1
>
> > > #ifdef DEBUG
> > > # define some_hand_coded_ugly_debug() printk( _
> > > #else
> > > # define some_hand_coded_ugly_debug()
> > > #endif
> > >
> > > On the premise pr_debug() would be a nop.
> > >
> > > It seems it is not always the case. This is a very serious problem.
>
> +1
>
> > > We probably have hundred of potential bugs, because few people
> > > actually make sure all debugging stuff is correct,
> > > like comments can be wrong because they are not updated properly as
> > > time flies.
> > >
> > > It is definitely a nop for many cases.
> > >
> > > +void eric_test_pr_debug(struct sock *sk)
> > > +{
> > > + if (atomic_read(&sk->sk_omem_alloc))
> > > + pr_debug("%s: optmem leakage for sock %p\n",
> > > +__func__, sk);
> > > +}
> > >
> > > ->
> > >
> > > 4740 :
> > > 4740: e8 00 00 00 00 callq 4745
> > > 4741: R_X86_64_PC32 __fentry__-0x4
> > > 4745: 55 push %rbp
> > > 4746: 8b 87 24 01 00 00 mov0x124(%rdi),%eax //
> > > atomic_read() but nothing follows
> > > 474c: 48 89 e5 mov%rsp,%rbp
> > > 474f: 5d pop%rbp
> > > 4750: c3 retq
> >
> >
> >
> > I would expect that it is nop when argument evaluation does not have
> > side-effects. For example, for a load of a variable compiler will most
> > likely elide it (though, it does not have to elide it, because the
> > load is spelled in the code, so it can also legally emit the load and
> > doesn't use the result).
> > But if argument computation has side-effect (or compiler can't prove
> > otherwise), it must emit code. It must emit code for function calls
> > when the function is defined in a different translation unit, and for
> > volatile accesses (most likely including atomic accesses), etc
>
> This isn't 100% true. As you state, in order to reach the return 0, all
> side effects must be evaluated. Load generally does not have side
> effects, so it can be safely elided, but function() must be emitted.
>
> However, that is _not_ required to get the desired warning emission on a
> printf argument function, see http://pastebin.com/UHuaydkj for an
> exa
Re: [PATCH] i40e: Fix i40e_print_features() VEB mode output
On Wed, 2015-12-02 at 15:48 -0500, David Miller wrote: > From: Joe Perches 02 Dec 2015 02:12:29 -0800 > > On Wed, 2015-12-02 at 01:56 -0800, Jeff Kirsher wrote: > >> On Wed, 2015-12-02 at 00:38 -0800, Joe Perches wrote: > >> > Noticed-by: Sergei Shtylyov > >> Don't you mean Reported-by? I am not aware of Noticed-by as being a > >> recognized signature. > > At least for the get_maintainer script, "-by:" is a signature > Is patchwork using the same regexp? If not, for the time being don't > user non-standard tags, and furthermore please ask the patchwork folks > to use something similar to getmaintainer.pl It doesn't seem so. response_re = re.compile( r'^(Tested|Reviewed|Acked|Signed-off|Nacked|Reported)-by: .*$', re.M | re.I) patchwork also doesn't seem very forgiving of misformatted signatures. (btw: Junio Hamano seems to prefer calling them trailers) And patchwork doesn't seem to handle "cc: " markers either. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i40e: Fix i40e_print_features() VEB mode output
On Wed, 2015-12-02 at 01:56 -0800, Jeff Kirsher wrote:
> On Wed, 2015-12-02 at 00:38 -0800, Joe Perches wrote:
> > Commit 7fd89545f337 ("i40e: remove BUG_ON from feature string
> > building")
> > added defective output when I40E_FLAG_VEB_MODE_ENABLED was set in
> > function i40e_print_features.
> >
> > Fix it.
> >
> > Miscellanea:
> >
> > o Remove unnecessary string variable
> > o Add space before not after fixed strings
> > o Use kmalloc not kzalloc
> > o Don't initialize i to 0, use result of first snprintf
> >
> > Noticed-by: Sergei Shtylyov
>
> Don't you mean Reported-by? I am not aware of Noticed-by as being a
> recognized signature.
At least for the get_maintainer script, "-by:" is a signature
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i40e: Fix i40e_print_features() VEB mode output
Commit 7fd89545f337 ("i40e: remove BUG_ON from feature string building")
added defective output when I40E_FLAG_VEB_MODE_ENABLED was set in
function i40e_print_features.
Fix it.
Miscellanea:
o Remove unnecessary string variable
o Add space before not after fixed strings
o Use kmalloc not kzalloc
o Don't initialize i to 0, use result of first snprintf
Noticed-by: Sergei Shtylyov
Signed-off-by: Joe Perches
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 42 +
1 file changed, 19 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 4b7d874..145eeb5 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -10240,52 +10240,48 @@ static int i40e_setup_pf_filter_control(struct
i40e_pf *pf)
static void i40e_print_features(struct i40e_pf *pf)
{
struct i40e_hw *hw = &pf->hw;
- char *buf, *string;
- int i = 0;
+ char *buf;
+ int i;
- string = kzalloc(INFO_STRING_LEN, GFP_KERNEL);
- if (!string) {
- dev_err(&pf->pdev->dev, "Features string allocation failed\n");
+ buf = kmalloc(INFO_STRING_LEN, GFP_KERNEL);
+ if (!buf)
return;
- }
-
- buf = string;
- i += snprintf(&buf[i], REMAIN(i), "Features: PF-id[%d] ", hw->pf_id);
+ i = snprintf(buf, INFO_STRING_LEN, "Features: PF-id[%d]", hw->pf_id);
#ifdef CONFIG_PCI_IOV
- i += snprintf(&buf[i], REMAIN(i), "VFs: %d ", pf->num_req_vfs);
+ i += snprintf(&buf[i], REMAIN(i), " VFs: %d", pf->num_req_vfs);
#endif
- i += snprintf(&buf[i], REMAIN(i), "VSIs: %d QP: %d RX: %s ",
+ i += snprintf(&buf[i], REMAIN(i), " VSIs: %d QP: %d RX: %s",
pf->hw.func_caps.num_vsis,
pf->vsi[pf->lan_vsi]->num_queue_pairs,
pf->flags & I40E_FLAG_RX_PS_ENABLED ? "PS" : "1BUF");
if (pf->flags & I40E_FLAG_RSS_ENABLED)
- i += snprintf(&buf[i], REMAIN(i), "RSS ");
+ i += snprintf(&buf[i], REMAIN(i), " RSS");
if (pf->flags & I40E_FLAG_FD_ATR_ENABLED)
- i += snprintf(&buf[i], REMAIN(i), "FD_ATR ");
+ i += snprintf(&buf[i], REMAIN(i), " FD_ATR");
if (pf->flags & I40E_FLAG_FD_SB_ENABLED) {
- i += snprintf(&buf[i], REMAIN(i), "FD_SB ");
- i += snprintf(&buf[i], REMAIN(i), "NTUPLE ");
+ i += snprintf(&buf[i], REMAIN(i), " FD_SB");
+ i += snprintf(&buf[i], REMAIN(i), " NTUPLE");
}
if (pf->flags & I40E_FLAG_DCB_CAPABLE)
- i += snprintf(&buf[i], REMAIN(i), "DCB ");
+ i += snprintf(&buf[i], REMAIN(i), " DCB");
#if IS_ENABLED(CONFIG_VXLAN)
- i += snprintf(&buf[i], REMAIN(i), "VxLAN ");
+ i += snprintf(&buf[i], REMAIN(i), " VxLAN");
#endif
if (pf->flags & I40E_FLAG_PTP)
- i += snprintf(&buf[i], REMAIN(i), "PTP ");
+ i += snprintf(&buf[i], REMAIN(i), " PTP");
#ifdef I40E_FCOE
if (pf->flags & I40E_FLAG_FCOE_ENABLED)
- i += snprintf(&buf[i], REMAIN(i), "FCOE ");
+ i += snprintf(&buf[i], REMAIN(i), " FCOE");
#endif
if (pf->flags & I40E_FLAG_VEB_MODE_ENABLED)
- i += snprintf(&buf[i], REMAIN(i), "VEPA ");
+ i += snprintf(&buf[i], REMAIN(i), " VEB");
else
- buf += sprintf(buf, "VEPA ");
+ i += snprintf(&buf[i], REMAIN(i), " VEPA");
- dev_info(&pf->pdev->dev, "%s\n", string);
- kfree(string);
+ dev_info(&pf->pdev->dev, "%s\n", buf);
+ kfree(buf);
WARN_ON(i > INFO_STRING_LEN);
}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next v2 04/15] i40e: remove BUG_ON from feature string building
On Wed, 2015-11-25 at 11:36 -0800, Joe Perches wrote:
> On Wed, 2015-11-25 at 10:35 -0800, Jeff Kirsher wrote:
> > On Wed, 2015-11-25 at 21:26 +0300, Sergei Shtylyov wrote:
> > > On 11/25/2015 09:21 PM, Jeff Kirsher wrote:
> > >
> > > > From: Shannon Nelson
> > > >
> > > > There's really no reason to kill the kernel thread just because
> > > > of a
> > > > little info string. This reworks the code to use snprintf's
> > > > limiting to
> > > > assure that the string is never too long, and WARN_ON to still
> > > > put out
> > > > a warning that we might want to look at the feature list
> > > > length.
> > > >
> > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > > > b/drivers/net/ethernet/intel/i40e/i40e_main.c
> []
> > > >if (pf->flags & I40E_FLAG_VEB_MODE_ENABLED)
> > > > - buf += sprintf(buf, "VEB ");
> > > > + i += snprintf(&buf[i], REMAIN(i), "VEPA ");
> > >
> > > Not "VEB "?
> >
> > Nice catch Sergei, I will wait a till this afternoon to respin the
> > patch series, just in case there are other changes needed that our
> > validation did not catch. :-)
>
> trivia:
>
> If you redo these, it'd be nicer not to use " " after each
> fixed string, but use " " before each fixed string.
>
> The final output string would be 1 byte shorter overall and
> not have an excess " " before the newline.
>
> The declaration of i doesn't need initialization to 0:
>
> i = snprintf(buf, INFO_STRING_LEN, "Features: PF-id[%d]", ...
>
> would work.
Maybe something like this patch (net-next)
Fix I40E_FLAG_VEB_MODE_ENABLED output of VEB
Miscellanea:
o Remove unnecessary string variable
o Add space before not after fixed strings
o Use kmalloc not kzalloc
o Don't initialize i to 0, use result of first snprintf
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 42 +
1 file changed, 19 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 4b7d874..145eeb5 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -10240,52 +10240,48 @@ static int i40e_setup_pf_filter_control(struct
i40e_pf *pf)
static void i40e_print_features(struct i40e_pf *pf)
{
struct i40e_hw *hw = &pf->hw;
- char *buf, *string;
- int i = 0;
+ char *buf;
+ int i;
- string = kzalloc(INFO_STRING_LEN, GFP_KERNEL);
- if (!string) {
- dev_err(&pf->pdev->dev, "Features string allocation failed\n");
+ buf = kmalloc(INFO_STRING_LEN, GFP_KERNEL);
+ if (!buf)
return;
- }
-
- buf = string;
- i += snprintf(&buf[i], REMAIN(i), "Features: PF-id[%d] ", hw->pf_id);
+ i = snprintf(buf, INFO_STRING_LEN, "Features: PF-id[%d]", hw->pf_id);
#ifdef CONFIG_PCI_IOV
- i += snprintf(&buf[i], REMAIN(i), "VFs: %d ", pf->num_req_vfs);
+ i += snprintf(&buf[i], REMAIN(i), " VFs: %d", pf->num_req_vfs);
#endif
- i += snprintf(&buf[i], REMAIN(i), "VSIs: %d QP: %d RX: %s ",
+ i += snprintf(&buf[i], REMAIN(i), " VSIs: %d QP: %d RX: %s",
pf->hw.func_caps.num_vsis,
pf->vsi[pf->lan_vsi]->num_queue_pairs,
pf->flags & I40E_FLAG_RX_PS_ENABLED ? "PS" : "1BUF");
if (pf->flags & I40E_FLAG_RSS_ENABLED)
- i += snprintf(&buf[i], REMAIN(i), "RSS ");
+ i += snprintf(&buf[i], REMAIN(i), " RSS");
if (pf->flags & I40E_FLAG_FD_ATR_ENABLED)
- i += snprintf(&buf[i], REMAIN(i), "FD_ATR ");
+ i += snprintf(&buf[i], REMAIN(i), " FD_ATR");
if (pf->flags & I40E_FLAG_FD_SB_ENABLED) {
- i += snprintf(&buf[i], REMAIN(i), "FD_SB ");
- i += snprintf(&buf[i], REMAIN(i), "NTUPLE ");
+ i += snprintf(&buf[i], REMAIN(i), " FD_SB");
+ i += snprintf(&buf[i], REMAIN(i), " NTUPLE");
}
if (pf->flags & I40E_FLAG_DCB_CAPABLE)
- i += snprintf(&buf[i], REMAIN(i), "DCB ");
+ i += snprintf(&buf[i], REMAIN(i), " DCB");
#if IS_ENABLED(CONFIG_VXLAN)
- i += snprintf(&buf[i], REMAIN(i), &quo
Re: [PATCH] vhost: replace % with & on data path
On Mon, 2015-11-30 at 10:34 +0200, Michael S. Tsirkin wrote: > We know vring num is a power of 2, so use & > to mask the high bits. [] > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c [] > @@ -1366,10 +1366,12 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq, > /* Only get avail ring entries after they have been exposed by guest. */ > smp_rmb(); > > + } ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] staging: net: core: skbuff.c: Added do-while pair for complex macros
On Thu, 2015-11-26 at 22:06 +0530, Jitendra Kumar Khasdev wrote:
> This patch is to file skbuff.c that fixes up following error,
> reported by checkpatch.pl tool,
Your subject title is not correct.
This is not a staging patch.
> 1. ERROR: Macros with multiple statements should be enclosed
>in a do - while loop.
checkpatch is brainless.
Not every message it emits needs fixing.
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
[]
> @@ -748,11 +748,13 @@ void consume_skb(struct sk_buff *skb)
> EXPORT_SYMBOL(consume_skb);
>
> /* Make sure a field is enclosed inside headers_start/headers_end section */
> -#define CHECK_SKB_FIELD(field) \
> - BUILD_BUG_ON(offsetof(struct sk_buff, field) < \
> - offsetof(struct sk_buff, headers_start)); \
> - BUILD_BUG_ON(offsetof(struct sk_buff, field) > \
> - offsetof(struct sk_buff, headers_end));\
> +#define CHECK_SKB_FIELD(field)
> \
> + do {\
> + BUILD_BUG_ON(offsetof(struct sk_buff, field) < \
> + offsetof(struct sk_buff, headers_start)); \
> + BUILD_BUG_ON(offsetof(struct sk_buff, field) > \
> + offsetof(struct sk_buff, headers_end)); \
> + } while (0) \
Perhaps the last check should add a sizeof(field)
BUILD_BUG_ON((offsetof(struct sk_buff, field) +
FIELD_SIZEOF(struct sk_buff, field)) >
offsetof(struct sk_buff, headers_end));
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next v2 04/15] i40e: remove BUG_ON from feature string building
On Wed, 2015-11-25 at 10:35 -0800, Jeff Kirsher wrote: > On Wed, 2015-11-25 at 21:26 +0300, Sergei Shtylyov wrote: > > On 11/25/2015 09:21 PM, Jeff Kirsher wrote: > > > > > From: Shannon Nelson > > > > > > There's really no reason to kill the kernel thread just because of a > > > little info string. This reworks the code to use snprintf's limiting to > > > assure that the string is never too long, and WARN_ON to still put out > > > a warning that we might want to look at the feature list length. > > > > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c > > > b/drivers/net/ethernet/intel/i40e/i40e_main.c [] > > >if (pf->flags & I40E_FLAG_VEB_MODE_ENABLED) > > > - buf += sprintf(buf, "VEB "); > > > + i += snprintf(&buf[i], REMAIN(i), "VEPA "); > > > > Not "VEB "? > > Nice catch Sergei, I will wait a till this afternoon to respin the > patch series, just in case there are other changes needed that our > validation did not catch. :-) trivia: If you redo these, it'd be nicer not to use " " after each fixed string, but use " " before each fixed string. The final output string would be 1 byte shorter overall and not have an excess " " before the newline. The declaration of i doesn't need initialization to 0: i = snprintf(buf, INFO_STRING_LEN, "Features: PF-id[%d]", ... would work. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next 06/16] i40e: Properly cast type for arithmetic
On Wed, 2015-11-25 at 02:46 -0800, Jeff Kirsher wrote: > On Tue, 2015-11-24 at 16:43 -0800, Joe Perches wrote: > > On Tue, 2015-11-24 at 16:04 -0800, Jeff Kirsher wrote: > > > From: Helin Zhang > > > > > > Pointer of type void * shouldn't be used in arithmetic, which may > > > result in compilation error. Casting of (u8 *) can be added to fix > > > that. > > > > > > > void * arithmetic is used quite frequently in the kernel. > > > > What compiler emits an error? > > When you use the gcc -Wpointer-arith, it generates the warning. make W=3 does that $ make help [...] make W=n [targets] Enable extra gcc checks, n=1,2,3 where 1: warnings which may be relevant and do not occur too often 2: warnings which occur quite often but may still be relevant 3: more obscure warnings, can most likely be ignored but I think it should be ignored. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] MAINTAINERS: PHY: Change maintainer to reviewer
On Tue, 2015-11-24 at 15:29 -0800, Florian Fainelli wrote: > Now that there is a reviewer role, add myself as reviewer since the PHY > library code is maintained via the networking tree. [] > diff --git a/MAINTAINERS b/MAINTAINERS [] > @@ -4195,7 +4195,7 @@ F: include/linux/netfilter_bridge/ > F:> > net/bridge/ > > ETHERNET PHY LIBRARY > -M: Florian Fainelli > +R: Florian Fainelli > L: netdev@vger.kernel.org > S: Maintained > F: include/linux/phy.h Just because the upstream path is not a tree you manage doesn't mean you aren't a maintainer. I think the status should be something other than "Maintained" if you are not the maintainer. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next 11/16] i40e/i40evf: clean up error messages
On Tue, 2015-11-24 at 16:04 -0800, Jeff Kirsher wrote:
> Clean up and enhance error messages related to VF MAC/VLAN filters.
> Indicate which VF is having issues, and if possible indicate the MAC
> address or VLAN involved.
trivia:
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
[]
> @@ -1623,7 +1623,8 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf,
> u8 *msg, u16 msglen)
>
> if (!f) {
> dev_err(&pf->pdev->dev,
> - "Unable to add VF MAC filter\n");
> + "Unable to add MAC filter %pM for VF %d\n",
> + al->list[i].addr, vf->vf_id);
Maybe use %hu for %d?
(here and elsewhere)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next 06/16] i40e: Properly cast type for arithmetic
On Tue, 2015-11-24 at 16:04 -0800, Jeff Kirsher wrote: > From: Helin Zhang > > Pointer of type void * shouldn't be used in arithmetic, which may > result in compilation error. Casting of (u8 *) can be added to fix > that. > void * arithmetic is used quite frequently in the kernel. What compiler emits an error? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next 04/15] ixgbe: Delete redundant include file
On Mon, 2015-11-23 at 11:36 -0800, Jeff Kirsher wrote: > Delete a redundant include of net/vxlan.h. unrelated trivia: > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c [] > char ixgbe_driver_name[] = "ixgbe"; This could be const or a #define -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/ipv4/ipconfig: Rejoin broken lines in console output
On Tue, 2015-11-24 at 14:08 +0100, Geert Uytterhoeven wrote:
> Commit 09605cc12c078306 ("net ipv4: use preferred log methods") replaced
> a few calls of pr_cont() after a console print without a trailing
> newline by pr_info(), causing lines to be split during IP
> autoconfiguration
Thanks Geert.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] wireless: change cfg80211 regulatory domain info as debug messages
On Sun, 2015-11-15 at 15:31 +0800, Dave Young wrote:
> cfg80211 module prints a lot of messages like below. Actually printing
> once is acceptable but sometimes it will print again and again, it looks
> very annoying. It is better to change these detail messages to debugging
> only.
So maybe add some wrapper that does a pr_info then
a pr_debug for the second and subsequent uses like:
#define pr_info_once_then_debug(fmt, ...) \
({ \
static bool __print_once __read_mostly; \
\
if (!__print_once) {\
__print_once = true;\
pr_info(fmt, ##__VA_ARGS__);\
} else {\
pr_debug(fmt, ##__VA_ARGS__); \
} \
})
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] stmmac: avoid ipq806x constant overflow warning
On Fri, 2015-11-13 at 08:37 +0100, Geert Uytterhoeven wrote:
> On Thu, Nov 12, 2015 at 10:03 PM, Arnd Bergmann
> wrote:
> > Building dwmac-ipq806x on a 64-bit architecture produces a harmless
> > warning from gcc:
> >
> > stmmac/dwmac-ipq806x.c: In function 'ipq806x_gmac_probe':
> > include/linux/bitops.h:6:19: warning: overflow in implicit constant
> > conversion [-Woverflow]
> > val = QSGMII_PHY_CDR_EN |
> > stmmac/dwmac-ipq806x.c:333:8: note: in expansion of macro
> > 'QSGMII_PHY_CDR_EN'
> > #define QSGMII_PHY_CDR_EN BIT(0)
> > #define BIT(nr) (1UL << (nr))
> >
> > This is a result of the type conversion rules in C, when we take
> > the
> > logical OR of multiple different types. In particular, we have
> > and unsigned long
> >
> > QSGMII_PHY_CDR_EN == BIT(0) == (1ul << 0) ==
> > 0x0001ul
> >
> > and a signed int
> >
> > 0xC << QSGMII_PHY_TX_DRV_AMP_OFFSET == 0xc000
> >
> > which together gives a signed long value
> >
> > 0xc001l
> >
> > and when this is passed into a function that takes an unsigned int
> > type,
> > gcc warns about the signed overflow and the loss of the upper 32
> > -bits that
> > are all ones.
> >
> > This patch adds 'ul' type modifiers to the literal numbers passed
> > in
> > here, so now the expression remains an 'unsigned long' with the
> > upper
> > bits all zero, and that avoids the signed overflow and the warning.
>
> FWIW, the 64-bitness of BIT() on 64-bit platforms is also causing
> subtle
> warnings in other places, e.g. when inverting them to create bit
> mask, cfr.
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commi
> t/?id=a9efeca613a8fe5281d7c91f5c8c9ea46f2312f6
>
> Gr{oetje,eeting}s,
I still think specific length BIT macros
can be useful.
https://lkml.org/lkml/2015/10/16/852
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] decnet: remove macro-local declarations
On Thu, 2015-11-05 at 20:38 +0100, Julia Lawall wrote: > On Thu, 5 Nov 2015, David Miller wrote: > > From: Julia Lawall > > Date: Thu, 5 Nov 2015 11:18:16 +0100> > > > Move the variable declarations from the for_nexthops macro to the > > > surrounding context, so that it is clear where these variables are > > > declared. This also makes it possible to remove the endfor_nexthops > > > macro. > > > > > > This change adds new arguments to the macro for_nexthops. They are > > > ordered > > > such that a pointer to the referenced object comes first, the index in the > > > list comes next, and the list itself comes last, roughly in analogy with > > > the list_for_each macros. [] > > > This patch takes care of a single file, where the macros are defined > > > locally. If the basic transformation looks OK, I will change the other > > > files that either likewise define their own macros or use the macros in > > > net/mpls/internal.h. The potentially affected files are: > > ... > > > > This looks fine to me. > > > > Please resubmit this when net-next opens back up, which should be > > shortly after -rc1. > > OK, I'll do the others then too. If you do can you please parenthesize the macro arguments? #define for_nexthops(nh, nhsel, fi) \ for (nhsel = 0, nh = (fi)->fib_nh; nhsel < (fi)->fib_nhs; (nh)++, (nhsel)++) instead of for(nhsel = 0, nh = (fi)->fib_nh; nhsel < (fi)->fib_nhs; nh++, nhsel++) And perhaps a renaming might be better s/for_nexthops/for_each_nexthop/ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next v4 3/8] dpaa_eth: add support for S/G frames
On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> Add support for Scater/Gather (S/G) frames. The FMan can place
> the frame content into multiple buffers and provide a S/G Table
> (SGT) into one first buffer with references to the others.
trivia:
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
[]
> @@ -347,7 +347,7 @@ static inline void clear_fd(struct qm_fd *fd)
> }
>
> static inline int _dpa_tx_fq_to_id(const struct dpa_priv_s *priv,
> -struct qman_fq *tx_fq)
> + struct qman_fq *tx_fq)
superfluous change?
> +void dpa_release_sgt(struct qm_sg_entry *sgt)
> +{
> + struct dpa_bp *dpa_bp;
> + struct bm_buffer bmb[DPA_BUFF_RELEASE_MAX];
> + u8 i = 0, j;
Using int may be better than u8 for indexing
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next v4 6/8] dpaa_eth: add ethtool statistics
On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> Add a series of counters to be exported through ethtool:
> - add detailed counters for reception errors;
> - add detailed counters for QMan enqueue reject events;
> - count the number of fragmented skbs received from the stack;
> - count all frames received on the Tx confirmation path;
> - add congestion group statistics;
> - count the number of interrupts for each CPU.
[]
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
[]
> +static void dpa_get_strings(struct net_device *net_dev, u32 stringset, u8
> *data)
> +{
> + unsigned int i, j, num_cpus, size;
> + char string_cpu[ETH_GSTRING_LEN];
> + u8 *strings;
> +
> + strings = data;
> + num_cpus = num_online_cpus();
> + size = DPA_STATS_GLOBAL_LEN * ETH_GSTRING_LEN;
> +
> + for (i = 0; i < DPA_STATS_PERCPU_LEN; i++) {
> + for (j = 0; j < num_cpus; j++) {
> + snprintf(string_cpu, ETH_GSTRING_LEN, "%s [CPU %d]",
> + dpa_stats_percpu[i], j);
> + memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> + strings += ETH_GSTRING_LEN;
> + }
> + snprintf(string_cpu, ETH_GSTRING_LEN, "%s [TOTAL]",
> + dpa_stats_percpu[i]);
> + memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> + strings += ETH_GSTRING_LEN;
> + }
> + memcpy(strings, dpa_stats_global, size);
> +}
This leaks uninitialized stack via a memcpy of uninitialized
string_cpu bytes into user-space.
Using
char string_cpu[ETH_GSTRING_LEN] = {};
or a memset before each snprintf would fix it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next v4 3/8] dpaa_eth: add support for S/G frames
On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> Add support for Scater/Gather (S/G) frames. The FMan can place
> the frame content into multiple buffers and provide a S/G Table
> (SGT) into one first buffer with references to the others.
trivia: scatter
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth_common.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth_common.c
[]
> @@ -1177,10 +1177,42 @@ void dpaa_eth_init_ports(struct mac_device *mac_dev,
> port_fqs->rx_defq, &buf_layout[RX]);
> }
>
> +void dpa_release_sgt(struct qm_sg_entry *sgt)
> +{
> + struct dpa_bp *dpa_bp;
> + struct bm_buffer bmb[DPA_BUFF_RELEASE_MAX];
Where is "struct bm_buffer" defined?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next v4 2/8] dpaa_eth: add support for DPAA Ethernet
On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote:
> This introduces the Freescale Data Path Acceleration Architecture
> (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan,
> BMan, PAMU and FMan drivers to deliver Ethernet connectivity on
> the Freescale DPAA QorIQ platforms.
[]
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
[]
> +static void _dpa_rx_error(struct net_device *net_dev,
> + const struct dpa_priv_s *priv,
> + struct dpa_percpu_priv_s *percpu_priv,
> + const struct qm_fd *fd,
> + u32 fqid)
> +{
> + /* limit common, possibly innocuous Rx FIFO Overflow errors'
> + * interference with zero-loss convergence benchmark results.
> + */
> + if (likely(fd->status & FM_FD_ERR_PHYSICAL))
> + pr_warn_once("non-zero error counters in fman statistics
> (sysfs)\n");
> + else
> + if (net_ratelimit())
> + netif_err(priv, hw, net_dev, "Err FD status = 0x%08x\n",
> + fd->status & FM_FD_STAT_RX_ERRORS);
It's a bit of a pity the logging message code is
a mix of pr_, dev_, netdev_
and netif_
Perhaps netif__ratelimited macros should be added.
Something like:
---
include/linux/netdevice.h | 54 +++
1 file changed, 54 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 210d11a..555471d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4025,6 +4025,60 @@ do {
\
})
#endif
+#define netif_level_ratelimited(level, priv, type, dev, fmt, args...) \
+do { \
+ if (netif_msg_##type(priv) && net_ratelimit()) \
+ netdev_##level(dev, fmt, ##args); \
+} while (0)
+
+#define netif_emerg_ratelimited(priv, type, dev, fmt, args...) \
+ netif_level_ratelimited(emerg, priv, type, dev, fmt, ##args)
+#define netif_alert_ratelimited(priv, type, dev, fmt, args...) \
+ netif_level_ratelimited(alert, priv, type, dev, fmt, ##args)
+#define netif_crit_ratelimited(priv, type, dev, fmt, args...) \
+ netif_level_ratelimited(crit, priv, type, dev, fmt, ##args)
+#define netif_err_ratelimited(priv, type, dev, fmt, args...) \
+ netif_level_ratelimited(err, priv, type, dev, fmt, ##args)
+#define netif_warn_ratelimited(priv, type, dev, fmt, args...) \
+ netif_level_ratelimited(warn, priv, type, dev, fmt, ##args)
+#define netif_notice_ratelimited(priv, type, dev, fmt, args...)
\
+ netif_level_ratelimited(notice, priv, type, dev, fmt, ##args)
+#define netif_info_ratelimited(priv, type, dev, fmt, args...) \
+ netif_level_ratelimited(info, priv, type, dev, fmt, ##args)
+
+#if defined(CONFIG_DYNAMIC_DEBUG)
+/* descriptor check is first to prevent flooding with "callbacks suppressed" */
+#define netif_dbg_ratelimited(priv, type, dev, fmt, args...) \
+do { \
+ DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
+ if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT) &&\
+ netif_msg_##type(priv) && net_ratelimit()) \
+ __dynamic_netdev_dbg(&descriptor, dev, fmt, ##args);\
+} while (0)
+#elif defined(DEBUG)
+#define netif_dbg_ratelimited(priv, type, dev, fmt, args...) \
+do { \
+ if (netif_msg_##type(priv) && net_ratelimit()) \
+ netif_printk(priv, type, KERN_DEBUG, dev, fmt, ##args); \
+} while (0)
+#else
+#define netif_dbg_ratelimited(priv, type, dev, fmt, args...) \
+do { \
+ if (0) \
+ netif_printk(priv, type, KERN_DEBUG, dev, fmt, ##args); \
+} while (0)
+#endif
+
+#if defined(VERBOSE_DEBUG)
+#define netif_vdbg_ratelimited netif_dbg_ratelimited
+#else
+#define netif_vdbg(priv, type, dev, fmt, args...) \
+do { \
+ if (0) \
+ netif_printk(priv, type, KERN_DEBUG, dev, fmt, ##args); \
+} while (0)
+#endif
+
/*
* The list of packet types we will receive (as opposed to discard)
* and the routines to invoke.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next v4 1/8] devres: add devm_alloc_percpu()
On Mon, 2015-11-02 at 19:31 +0200, Madalin Bucur wrote: > Introduce managed counterparts for alloc_percpu() and free_percpu(). > Add devm_alloc_percpu() and devm_free_percpu() into the managed > interfaces list. trivia, could be fixed later > +/** > + * __devm_alloc_percpu - Resource-managed alloc_percpu > + * @dev: Device to allocate per-cpu memory for > + * @size: Size of per-cpu memory to allocate > + * @align: Alignement of per-cpu memory to allocate French spelling? alignment -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: hix5hd2_gmac: avoid integer overload warning
On Fri, 2015-10-16 at 21:50 +0300, Sergei Shtylyov wrote:
> On 10/16/2015 09:04 PM, Joe Perches wrote:
>
> >>>> BITS_RX_EN is an 'unsigned long' constant, so the ones complement of that
> >>>> has bits set that do not fit into a 32-bit variable on 64-bit
> >>>> architectures,
> >>>> which causes a harmless gcc warning:
> >>> ...
> >>>> static void hix5hd2_port_disable(struct hix5hd2_priv *priv)
> >>>> {
> >>>> - writel_relaxed(~(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);
> >>>> + writel_relaxed(~(u32)(BITS_RX_EN | BITS_TX_EN), priv->base +
> >>>> PORT_EN);
> >>>>writel_relaxed(0, priv->base + DESC_WR_RD_ENA);
> >>>
> >>> ISTM that just means that the constants shouldn't be 'long'.
> >>
> >> Right, but that would probably mean changing the BIT() macro or not using
> >> it
> >> here. In the past I've argued against using that macro, but I've given
> >> up that fight.
> >
> > Fight on... (Somebody must have gone to USC here)
> >
> > There might be value in aefin BIT_U32 macro.
> > Maybe BIT_U64 too.
>
> There's BIT_ULL() already.
I know, but symmetry is good.
I think there'd be no harm in adding it.
Perhaps adding all the sized variants would be useful.
Something like:
#define BIT_OF_TYPE(type, nr) \
({ \
typeof(type) rtn; \
BUILD_BUG_ON(__builtin_constant_p(nr) &&\
((nr) < 0 || \
(nr) >= sizeof(type) * BITS_PER_BYTE)); \
rtn = ((type)1) << (nr);\
rtn;\
})
#define BIT_U8(nr) BIT_OF_TYPE(u8, nr)
#define BIT_U16(nr) BIT_OF_TYPE(u16, nr)
#define BIT_U32(nr) BIT_OF_TYPE(u32, nr)
#define BIT_U64(nr) BIT_OF_TYPE(u64, nr)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: hisilicon: add OF dependency
On Sat, 2015-10-17 at 02:16 +0800, kbuild test robot wrote: > All errors (new ones prefixed by >>): >drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c: In function > 'hns_dsaf_get_cfg': > >> drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c:151:3: error: implicit > >> declaration of function 'iounmap' [-Werror=implicit-function-declaration] > iounmap(dsaf_dev->io_base); > ^ >cc1: some warnings being treated as errors > > vim +/iounmap +151 drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c [] > 511e6bc0 huangdaode 2015-09-17 142 if > (!dma_set_mask_and_coherent(dsaf_dev->dev, DMA_BIT_MASK(64ULL))) btw: this should be DMA_BIT_MASK(64) without the ULL -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: hix5hd2_gmac: avoid integer overload warning
On Fri, 2015-10-16 at 13:28 +0200, Arnd Bergmann wrote:
> On Friday 16 October 2015 11:14:44 David Laight wrote:
> > From: Arnd Bergmann
> > > Sent: 16 October 2015 11:01
> > > BITS_RX_EN is an 'unsigned long' constant, so the ones complement of that
> > > has bits set that do not fit into a 32-bit variable on 64-bit
> > > architectures,
> > > which causes a harmless gcc warning:
> > ...
> > > static void hix5hd2_port_disable(struct hix5hd2_priv *priv)
> > > {
> > > - writel_relaxed(~(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);
> > > + writel_relaxed(~(u32)(BITS_RX_EN | BITS_TX_EN), priv->base +
> > > PORT_EN);
> > > writel_relaxed(0, priv->base + DESC_WR_RD_ENA);
> >
> > ISTM that just means that the constants shouldn't be 'long'.
>
> Right, but that would probably mean changing the BIT() macro or not using it
> here. In the past I've argued against using that macro, but I've given
> up that fight.
Fight on... (Somebody must have gone to USC here)
There might be value in a BIT_U32 macro.
Maybe BIT_U64 too.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next 05/17] i40e: Change some messages from info to debug only
On Thu, 2015-10-15 at 02:39 -0700, Jeff Kirsher wrote: > From: Neerav Parikh > > There are several error messages that have been printing when there is > no functional issue. These messages should be available at debug message > level only. [] > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c > b/drivers/net/ethernet/intel/i40e/i40e_main.c [] > @@ -6593,9 +6593,9 @@ static void i40e_reset_and_rebuild(struct i40e_pf *pf, > bool reinit) > /* make sure our flow control settings are restored */ > ret = i40e_set_fc(&pf->hw, &set_fc_aq_fail, true); > if (ret) > - dev_info(&pf->pdev->dev, "set fc fail, err %s aq_err %s\n", > - i40e_stat_str(&pf->hw, ret), > - i40e_aq_str(&pf->hw, pf->hw.aq.asq_last_status)); > + dev_dbg(&pf->pdev->dev, "setting flow control: ret = %s > last_status = %s\n", > + i40e_stat_str(&pf->hw, ret), > + i40e_aq_str(&pf->hw, pf->hw.aq.asq_last_status)); [] > @@ -10333,10 +10339,9 @@ static int i40e_probe(struct pci_dev *pdev, const > struct pci_device_id *ent) > /* get the requested speeds from the fw */ > err = i40e_aq_get_phy_capabilities(hw, false, false, &abilities, NULL); > if (err) > - dev_info(&pf->pdev->dev, > - "get phy capabilities failed, err %s aq_err %s, > advertised speed settings may not be correct\n", > - i40e_stat_str(&pf->hw, err), > - i40e_aq_str(&pf->hw, pf->hw.aq.asq_last_status)); > + dev_dbg(&pf->pdev->dev, "get requested speeds ret = %s > last_status = %s\n", > + i40e_stat_str(&pf->hw, err), > + i40e_aq_str(&pf->hw, pf->hw.aq.asq_last_status)); > pf->hw.phy.link_info.requested_speeds = abilities.link_speed; > > /* get the supported phy types from the fw */ Perhaps these 2 are functional issues and should remain dev_info or maybe upgraded to notice or err -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ethtool: Use kcalloc instead of kmalloc for ethtool_get_strings
It seems that kernel memory can leak into userspace by a
kmalloc, ethtool_get_strings, then copy_to_user sequence.
Avoid this by using kcalloc to zero fill the copied buffer.
Signed-off-by: Joe Perches
---
stable too...
On Tue, 2015-10-13 at 23:59 -0700, Jeff Kirsher wrote:
> From: Jacob Keller
[]
> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
> b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
[]
> @@ -206,13 +206,13 @@ static void fm10k_get_stat_strings(struct net_device
> *dev, u8 *data)
> }
>
> for (i = 0; i < interface->hw.mac.max_queues; i++) {
> - sprintf(p, "tx_queue_%u_packets", i);
> + snprintf(p, ETH_GSTRING_LEN, "tx_queue_%u_packets", i);
It seems these need a memset after the snprintf to zero fill
bytes after the string terminating \0 to avoid leaking
contents of any unset bytes.
It'd probably be better to allocate a zeroed buffer instead.
> p += ETH_GSTRING_LEN;
> - sprintf(p, "tx_queue_%u_bytes", i);
> + snprintf(p, ETH_GSTRING_LEN, "tx_queue_%u_bytes", i);
so...
net/core/ethtool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index b495ab1..29edf74 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1284,7 +1284,7 @@ static int ethtool_get_strings(struct net_device *dev,
void __user *useraddr)
gstrings.len = ret;
- data = kmalloc(gstrings.len * ETH_GSTRING_LEN, GFP_USER);
+ data = kcalloc(gstrings.len, ETH_GSTRING_LEN, GFP_USER);
if (!data)
return -ENOMEM;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] e1000 driver remove checkpatch errors, warnings and checks.
On Tue, 2015-10-13 at 15:23 -0700, Alexander Duyck wrote: > Please don't just blindly > follow checkpatch as it can give out erroneous information. > > Looking over most of this patch series it seems like it is taking > readability in the wrong direction and reducing the ability to maintain > the driver since this code has been "maintenance only" for some time > now. If somebody comes up with a legitimate fix for an issue at some > point in the future they will need to work around these patches in order > to back-port it into a stable release and that just hurts maintainability. > > I'd say this whole series should be rejected on the grounds that this > driver is mostly stable and should only really be modified for bug fixes > at this point. If we really need to go through and do a checkpatch > sweep we should probably just focus on serious errors only instead of > going astray and chasing down things that are false hits or minor issues > that are mostly a matter of preference. Excellent advice. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro
On Mon, 2015-10-12 at 13:59 +0200, Arnd Bergmann wrote: > On Monday 12 October 2015 11:23:44 huangdaode wrote: > > + s += sprintf(s, > > + "\t\ttx_ring on > > %p:%u,%u,%u,%u,%u,%llu,%llu\n", > > + h->qs[i]->tx_ring.io_base, [] > There is actually a more significant problem with this code, which I > failed to notice when doing the original bugfix: > > You have a sysfs interface here that exports internal data of the > device that should not be visible like this. One problem is that > the io_base is a kernel pointer that must not be visible to non-root > users (so we don't easily create an attack surface for exploits). Using %pK might have been appropriate. > It would probably be better to completely remove that sysfs interface, and > to use the ethtool netlink interface to export them. But this would be better. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2 1/2] hisilicon net: removes the once HANDEL_TX_MSG macro
Hello Huang. On Mon, 2015-10-12 at 11:23 +0800, huangdaode wrote: > This patch changes the code style to make the code more simple. > also removes the once used HNADEL_TX_MSG macro, according to the HANDEL_TX_MSG typo > review comments from Joe Perches. > > Signed-off-by: huangdaode > Reviewed-by: Joe Perches I didn't review this. Please do not add signatures for another person when not specifically added by that person. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: nhs: fix 32-bit build warning
On Tue, 2015-10-06 at 23:53 +0200, Arnd Bergmann wrote:
> diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.c
> b/drivers/net/ethernet/hisilicon/hns/hnae.c
[]
> @@ -448,12 +448,12 @@ static ssize_t handles_show(struct device *dev,
> s += sprintf(buf + s, "handle %d (eport_id=%u from %s):\n",
> i++, h->eport_id, h->dev->name);
> for (j = 0; j < h->q_num; j++) {
> - s += sprintf(buf + s, "\tqueue[%d] on 0x%llx\n",
> - j, (u64)h->qs[i]->io_base);
> -#define HANDEL_TX_MSG "\t\ttx_ring on 0x%llx:%u,%u,%u,%u,%u,%llu,%llu\n"
> + s += sprintf(buf + s, "\tqueue[%d] on %p\n",
> + j, h->qs[i]->io_base);
> +#define HANDEL_TX_MSG "\t\ttx_ring on %p:%u,%u,%u,%u,%u,%llu,%llu\n"
> s += sprintf(buf + s,
>HANDEL_TX_MSG,
Maybe one day remove the misspelled and
used once HANDEL_TX_MSG macro too.
Another possibility is to use a generally
smaller object code style like:
char *s = buf;
...
s += sprintf(s, ...)
...
return s - buf;
instead of
ssize_t s = 0;
...
s += sprintf(buf + s, ...)
...
return s;
---
drivers/net/ethernet/hisilicon/hns/hnae.c | 35 +++
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.c
b/drivers/net/ethernet/hisilicon/hns/hnae.c
index f52e99a..d2af46f 100644
--- a/drivers/net/ethernet/hisilicon/hns/hnae.c
+++ b/drivers/net/ethernet/hisilicon/hns/hnae.c
@@ -439,40 +439,39 @@ EXPORT_SYMBOL(hnae_ae_unregister);
static ssize_t handles_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- ssize_t s = 0;
+ char *s = buf;
struct hnae_ae_dev *hdev = cls_to_ae_dev(dev);
struct hnae_handle *h;
int i = 0, j;
list_for_each_entry_rcu(h, &hdev->handle_list, node) {
- s += sprintf(buf + s, "handle %d (eport_id=%u from %s):\n",
- i++, h->eport_id, h->dev->name);
+ s += sprintf(s, "handle %d (eport_id=%u from %s):\n",
+i++, h->eport_id, h->dev->name);
for (j = 0; j < h->q_num; j++) {
- s += sprintf(buf + s, "\tqueue[%d] on %p\n",
+ s += sprintf(s, "\tqueue[%d] on %p\n",
j, h->qs[i]->io_base);
-#define HANDEL_TX_MSG "\t\ttx_ring on %p:%u,%u,%u,%u,%u,%llu,%llu\n"
- s += sprintf(buf + s,
-HANDEL_TX_MSG,
+ s += sprintf(s,
+"\t\ttx_ring on
%p:%u,%u,%u,%u,%u,%llu,%llu\n",
h->qs[i]->tx_ring.io_base,
h->qs[i]->tx_ring.buf_size,
h->qs[i]->tx_ring.desc_num,
h->qs[i]->tx_ring.max_desc_num_per_pkt,
h->qs[i]->tx_ring.max_raw_data_sz_per_desc,
h->qs[i]->tx_ring.max_pkt_size,
-h->qs[i]->tx_ring.stats.sw_err_cnt,
-h->qs[i]->tx_ring.stats.io_err_cnt);
- s += sprintf(buf + s,
- "\t\trx_ring on %p:%u,%u,%llu,%llu,%llu\n",
- h->qs[i]->rx_ring.io_base,
- h->qs[i]->rx_ring.buf_size,
- h->qs[i]->rx_ring.desc_num,
- h->qs[i]->rx_ring.stats.sw_err_cnt,
- h->qs[i]->rx_ring.stats.io_err_cnt,
- h->qs[i]->rx_ring.stats.seg_pkt_cnt);
+h->qs[i]->tx_ring.stats.sw_err_cnt,
+h->qs[i]->tx_ring.stats.io_err_cnt);
+ s += sprintf(s,
+"\t\trx_ring on %p:%u,%u,%llu,%llu,%llu\n",
+h->qs[i]->rx_ring.io_base,
+h->qs[i]->rx_ring.buf_size,
+h->qs[i]->rx_ring.desc_num,
+h->qs[i]->rx_ring.stats.sw_err_cnt,
+h->qs[i]->rx_ring.stats.io_err_cnt,
+h->qs[i]->rx_ring.stats.seg_pkt_cnt);
}
}
- return s;
+ return s - buf;
}
static DEVICE_ATTR_RO(handles);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mISDN: use kstrdup() in dsp_pipeline_build
On Sat, 2015-10-10 at 02:32 -0700, Geliang Tang wrote:
> Use kstrdup instead of strlen-kmalloc-strcpy.
Not the same code.
Instead of returning early, a 0 length string will
now set pipeline->inuse to 0.
Maybe that's OK, but you should state why in the
commit log.
> diff --git a/drivers/isdn/mISDN/dsp_pipeline.c
> b/drivers/isdn/mISDN/dsp_pipeline.c
[]
> @@ -250,14 +250,9 @@ int dsp_pipeline_build(struct dsp_pipeline *pipeline,
> const char *cfg)
> if (!cfg)
> return 0;
>
> - len = strlen(cfg);
> - if (!len)
> - return 0;
> -
> - dup = kmalloc(len + 1, GFP_ATOMIC);
> + dup = kstrdup(cfg, GFP_ATOMIC);
> if (!dup)
> return 0;
> - strcpy(dup, cfg);
> while ((tok = strsep(&dup, "|"))) {
> if (!strlen(tok))
> continue;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] bnxt_en: New Broadcom ethernet driver.
On Sat, 2015-10-10 at 07:39 -0400, Michael Chan wrote: > Broadcom ethernet driver for the new family of NetXtreme-C/E > ethernet devices. Thanks. In addition to the kbuild test robot suggestions, you might try running scripts/checkpatch.pl on this and fix up some style trivia. Maybe try "scripts/checkpatch.pl --fix-inplace" for the mechanical stuff. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mlxsw: fix warnings for big-endian 32-bit dma_addr_t
On Tue, 2015-10-06 at 23:47 +0200, Arnd Bergmann wrote: > The recently added mlxsw driver produces warnings in ARM > allmodconfig: > > drivers/net/ethernet/mellanox/mlxsw/pci.c: In function 'mlxsw_pci_cmd_exec': > drivers/net/ethernet/mellanox/mlxsw/pci.c:1585:59: warning: right shift count > >= width of type [-Wshift-count-overflow] > linux/byteorder/big_endian.h:38:51: note: in definition of macro > '__cpu_to_be32' > drivers/net/ethernet/mellanox/mlxsw/pci.c:76:2: note: in expansion of macro > 'iowrite32be' > > This changes the type of the local variable to u64, which gets rid of the > warning and seems nicer than adding #ifdefs. Using upper_32_bits instead of the shift might be nicer than changing the type. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] stmmac: replace if (netif_msg_type) by their netif_xxx counterpart
On Thu, 2015-09-10 at 14:37 +0200, LABBE Corentin wrote: > replace all > if (netif_msg_type(priv)) dev_xxx(priv->devices, ...) > by the simplier macro netif_xxx(priv, hw, priv->dev, ...) Thanks. Trivia: > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c [] > @@ -733,10 +735,9 @@ static void stmmac_adjust_link(struct net_device *dev) > stmmac_hw_fix_mac_speed(priv); > break; > default: > - if (netif_msg_link(priv)) > - netdev_warn(priv->dev, > - "%s: Speed (%d) not > 10/100\n", > - dev->name, phydev->speed); > + netif_warn(priv, link, priv->dev, > +"%s: Speed (%d) not 10/100\n", > +dev->name, phydev->speed); Maybe add another patch removing dev->name where used in logging as it's likely redundant. > @@ -1038,18 +1039,15 @@ static int init_dma_desc_rings(struct net_device > *dev, gfp_t flags) > > priv->dma_buf_sz = bfsize; > > - if (netif_msg_probe(priv)) > - netdev_dbg(priv->dev, "%s: txsize %d, rxsize %d, bfsize %d\n", > -__func__, txsize, rxsize, bfsize); > + netif_dbg(priv, probe, priv->dev, "%s: txsize %d, rxsize %d, bfsize > %d\n", > + __func__, txsize, rxsize, bfsize); And another removing __func__ from the _dbg uses as it's relatively low value and dynamic debug can add it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] stmmac: replace all pr_xxx by their netdev_xxx counterpart
On Thu, 2015-09-10 at 14:37 +0200, LABBE Corentin wrote: > So this patch replace all pr_xxx by their dev_xxx counterpart. Minor mismatch between subject and commit message. Maybe not worth resending. Thanks -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mv643xx_eth: Neaten mv643xx_eth_program_multicast_filter
The code around the allocation and loops are a bit obfuscated.
Neaten it by using:
o kcalloc with decimal count and sizeof(u32)
o Decimal loop indexing and i++ not i += 4
o A promiscuous block using a similar style
to the multicast block
o Remove unnecessary variables
Signed-off-by: Joe Perches
---
Here's a neatening patch respun against Rasmus' patch
Might be useful.
drivers/net/ethernet/marvell/mv643xx_eth.c | 43 +++---
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c
b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 960169e..c78ae18 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -1845,29 +1845,19 @@ static void mv643xx_eth_program_multicast_filter(struct
net_device *dev)
struct netdev_hw_addr *ha;
int i;
- if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI)) {
- int port_num;
- u32 accept;
+ if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI))
+ goto promiscuous;
-oom:
- port_num = mp->port_num;
- accept = 0x01010101;
- for (i = 0; i < 0x100; i += 4) {
- wrl(mp, SPECIAL_MCAST_TABLE(port_num) + i, accept);
- wrl(mp, OTHER_MCAST_TABLE(port_num) + i, accept);
- }
- return;
- }
-
- mc_spec = kzalloc(0x200, GFP_ATOMIC);
- if (mc_spec == NULL)
- goto oom;
- mc_other = mc_spec + (0x100 >> 2);
+ /* Allocate both mc_spec and mc_other tables */
+ mc_spec = kcalloc(128, sizeof(u32), GFP_ATOMIC);
+ if (!mc_spec)
+ goto promiscuous;
+ mc_other = &mc_spec[64];
netdev_for_each_mc_addr(ha, dev) {
u8 *a = ha->addr;
u32 *table;
- int entry;
+ u8 entry;
if (memcmp(a, "\x01\x00\x5e\x00\x00", 5) == 0) {
table = mc_spec;
@@ -1880,12 +1870,23 @@ oom:
table[entry >> 2] |= 1 << (8 * (entry & 3));
}
- for (i = 0; i < 0x100; i += 4) {
- wrl(mp, SPECIAL_MCAST_TABLE(mp->port_num) + i, mc_spec[i >> 2]);
- wrl(mp, OTHER_MCAST_TABLE(mp->port_num) + i, mc_other[i >> 2]);
+ for (i = 0; i < 64; i++) {
+ wrl(mp, SPECIAL_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+ mc_spec[i]);
+ wrl(mp, OTHER_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+ mc_other[i]);
}
kfree(mc_spec);
+ return;
+
+promiscuous:
+ for (i = 0; i < 64; i++) {
+ wrl(mp, SPECIAL_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+ 0x01010101u);
+ wrl(mp, OTHER_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+ 0x01010101u);
+ }
}
static void mv643xx_eth_set_rx_mode(struct net_device *dev)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] net: mv643xx_eth: use kzalloc
On Wed, 2015-09-09 at 11:45 -0700, Joe Perches wrote:
> On Wed, 2015-09-09 at 20:34 +0200, Rasmus Villemoes wrote:
> > mc_spec and mc_other are u32*, we allocate 0x200 = 512 bytes = 128
> > u32s, and pointer arithmetic makes mc_other point to the latter 64. Then
> > the memory is cleared 256 bytes at a time.
> >
> > It's unusual and slightly obfuscated code, but I don't think it's
> > wrong.
Perhaps this would make the code a bit clearer:
Use kcalloc, decimal sizes, decimal indexing,
and a promiscuous exit block using the same
style as the non-promiscuous multicast block.
---
drivers/net/ethernet/marvell/mv643xx_eth.c | 46 ++
1 file changed, 22 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c
b/drivers/net/ethernet/marvell/mv643xx_eth.c
index d52639b..9230ed5 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -1845,32 +1845,19 @@ static void mv643xx_eth_program_multicast_filter(struct
net_device *dev)
struct netdev_hw_addr *ha;
int i;
- if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI)) {
- int port_num;
- u32 accept;
+ if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI))
+ goto promiscuous;
-oom:
- port_num = mp->port_num;
- accept = 0x01010101;
- for (i = 0; i < 0x100; i += 4) {
- wrl(mp, SPECIAL_MCAST_TABLE(port_num) + i, accept);
- wrl(mp, OTHER_MCAST_TABLE(port_num) + i, accept);
- }
- return;
- }
-
- mc_spec = kmalloc(0x200, GFP_ATOMIC);
- if (mc_spec == NULL)
- goto oom;
- mc_other = mc_spec + (0x100 >> 2);
-
- memset(mc_spec, 0, 0x100);
- memset(mc_other, 0, 0x100);
+ /* Allocate both mc_spec and mc_other tables */
+ mc_spec = kcalloc(128, sizeof(u32), GFP_ATOMIC);
+ if (!mc_spec)
+ goto promiscuous;
+ mc_other = &mc_spec[64];
netdev_for_each_mc_addr(ha, dev) {
u8 *a = ha->addr;
u32 *table;
- int entry;
+ u8 entry;
if (memcmp(a, "\x01\x00\x5e\x00\x00", 5) == 0) {
table = mc_spec;
@@ -1883,12 +1870,23 @@ oom:
table[entry >> 2] |= 1 << (8 * (entry & 3));
}
- for (i = 0; i < 0x100; i += 4) {
- wrl(mp, SPECIAL_MCAST_TABLE(mp->port_num) + i, mc_spec[i >> 2]);
- wrl(mp, OTHER_MCAST_TABLE(mp->port_num) + i, mc_other[i >> 2]);
+ for (i = 0; i < 64; i++) {
+ wrl(mp, SPECIAL_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+ mc_spec[i]);
+ wrl(mp, OTHER_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+ mc_other[i]);
}
kfree(mc_spec);
+ return;
+
+promiscuous:
+ for (i = 0; i < 64; i++) {
+ wrl(mp, SPECIAL_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+ 0x01010101u);
+ wrl(mp, OTHER_MCAST_TABLE(mp->port_num) + i * sizeof(u32),
+ 0x01010101u);
+ }
}
static void mv643xx_eth_set_rx_mode(struct net_device *dev)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] net: mv643xx_eth: use kzalloc
On Wed, 2015-09-09 at 20:34 +0200, Rasmus Villemoes wrote: > mc_spec and mc_other are u32*, we allocate 0x200 = 512 bytes = 128 > u32s, and pointer arithmetic makes mc_other point to the latter 64. Then > the memory is cleared 256 bytes at a time. > > It's unusual and slightly obfuscated code, but I don't think it's > wrong. Right, misread. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] stmmac: replace all pr_xxx by their dev_xxx counterpart
On Wed, 2015-09-09 at 15:14 +0200, LABBE Corentin wrote:
> The stmmac driver use lots of pr_xxx functions to print information.
> This is bad since we cannot know which device logs the information.
> (moreover if two stmmac device are present)
[]
> So this patch replace all pr_xxx by their dev_xxx counterpart.
Using
netdev_(priv->dev, ...
instead of
dev_(priv->device,
would be more consistent with other ethernet devices.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[]
> @@ -298,7 +298,7 @@ bool stmmac_eee_init(struct stmmac_priv *priv)
>*/
> spin_lock_irqsave(&priv->lock, flags);
> if (priv->eee_active) {
> - pr_debug("stmmac: disable EEE\n");
> + dev_dbg(priv->device, "disable EEE\n");
netdev_dbg(priv->dev, ...)
> @@ -657,10 +657,10 @@ static int stmmac_init_ptp(struct stmmac_priv *priv)
> priv->adv_ts = 1;
>
> if (netif_msg_hw(priv) && priv->dma_cap.time_stamp)
> - pr_debug("IEEE 1588-2002 Time Stamp supported\n");
> + dev_dbg(priv->device, "IEEE 1588-2002 Time Stamp supported\n");
And these netif_msg_ could be
if (priv->dma_cap.timestamp)
netif_dbg(priv, hw, priv->dev, ...);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] net: mv643xx_eth: use kzalloc
On Wed, 2015-09-09 at 10:38 +0200, Rasmus Villemoes wrote:
> The double memset is a little ugly; using kzalloc avoids it altogether.
[]
> diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c
> b/drivers/net/ethernet/marvell/mv643xx_eth.c
[]
> @@ -1859,14 +1859,11 @@ oom:
> return;
> }
>
> - mc_spec = kmalloc(0x200, GFP_ATOMIC);
> + mc_spec = kzalloc(0x200, GFP_ATOMIC);
> if (mc_spec == NULL)
> goto oom;
> mc_other = mc_spec + (0x100 >> 2);
This sure looks wrong as it sets a pointer
to unallocated memory.
> - memset(mc_spec, 0, 0x100);
> - memset(mc_other, 0, 0x100);
So this does a memset of random memory.
for (i = 0; i < 0x100; i += 4) {
wrl(mp, SPECIAL_MCAST_TABLE(mp->port_num) + i, mc_spec[i >> 2]);
wrl(mp, OTHER_MCAST_TABLE(mp->port_num) + i, mc_other[i >> 2]);
}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/core/sysctl_net_core.c unused variable
On Sun, 2015-09-06 at 16:36 +0100, Nick Warne wrote:
> On 06/09/15 16:28, Joe Perches wrote:
> > On Sun, 2015-09-06 at 16:16 +0100, Nick Warne wrote:
> >> On 06/09/15 15:52, Joe Perches wrote:
> >> > On Sun, 2015-09-06 at 15:13 +0100, Nick Warne wrote:
> >> >> gcc version 4.8.2 (GCC) warns that 'static int one = 1;' is declared but
> >> >> not used in file net/core/sysctl_net_core.c.
> >> >
> >> > Only when CONFIG_NET isn't set.
> >>
> >> CONFIG_NET=y
> >>
> >> Peculiar indeed.
> >>
> >> >> Reading the file, that is
> >> >> the case. Attached is a patch to remove it.
> >> >
> >> > $ git grep -w -n one net/core/sysctl_net_core.c
> >> > net/core/sysctl_net_core.c:26:static int one = 1;
> >> > net/core/sysctl_net_core.c:332: .extra2 = &one
> >> >
> >> >> Signed-off-by: Nick Warne
> >> >
> >> > Please use grep to augment reading.
> >>
> >> grep -w -n one net/core/sysctl_net_core.c
> >> 26:static int one = 1;
> >>
> >> ?
> >>
> >> I just don't have the &one.
> >>
> >> I am confused now.
> >
> > What source tree are you using?
>
> Latest longterm 3.18.21
OK, it's important to mention that otherwise the
assumption would be a git tree like net or net-next.
> > What changes in what branch exist?
>
> I am not using git (if that is what you mean by 'branches') - just
> tarballs from kernel.org
(OK, using git and linux-stable)
$ git grep -w -n one v3.18.21 -- net/core/sysctl_net_core.c
v3.18.21:net/core/sysctl_net_core.c:26:static int one = 1;
And the responsible commit:
commit c48cf4f27d4555a455c3fef71137bd0fc44d1656
("net: sysctl_net_core: check SNDBUF and RCVBUF for min length")
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT] Networking
On Thu, 2015-09-03 at 11:22 -0700, Linus Torvalds wrote:
> On Thu, Sep 3, 2015 at 10:40 AM, David Miller wrote:
> >
> > Linus, what GCC version are you using and what does the warning look
> > like?
>
> I'm on whatever is in F22. gcc -v says
>
>gcc version 5.1.1 20150618 (Red Hat 5.1.1-4) (GCC)
>
> and the warning looks like so:
>
> net/mac80211/rate.c: In function ‘rate_control_cap_mask’:
> net/mac80211/rate.c:719:25: warning: ‘sizeof’ on array function
> parameter ‘mcs_mask’ will return size of ‘u8 * {aka unsigned char *}’
> [-Wsizeof-array-argument]
> for (i = 0; i < sizeof(mcs_mask); i++)
>^
>
> (note the lack of warning about the use of an array in the function
> definition parameter list - I tried to find if there's any way to
> enable such a warning, but couldn't find anything. Maybe my google-fu
> is weak, but more probably that just doesn't exist).
Coccinelle might be a better tool for this but
a possible checkpatch patch is below:
It produces output like:
$ ./scripts/checkpatch.pl -f net/iucv/iucv.c --types=sized_array_argument
WARNING: Avoid sized array arguments
#716: FILE: net/iucv/iucv.c:716:
+static int iucv_sever_pathid(u16 pathid, u8 userdata[16])
+{
WARNING: Avoid sized array arguments
#878: FILE: net/iucv/iucv.c:878:
+int iucv_path_accept(struct iucv_path *path, struct iucv_handler *handler,
+u8 userdata[16], void *private)
+{
WARNING: Avoid sized array arguments
#925: FILE: net/iucv/iucv.c:925:
+int iucv_path_connect(struct iucv_path *path, struct iucv_handler *handler,
+ u8 userid[8], u8 system[8], u8 userdata[16],
+ void *private)
+{
WARNING: Avoid sized array arguments
#988: FILE: net/iucv/iucv.c:988:
+int iucv_path_quiesce(struct iucv_path *path, u8 userdata[16])
+{
WARNING: Avoid sized array arguments
#1020: FILE: net/iucv/iucv.c:1020:
+int iucv_path_resume(struct iucv_path *path, u8 userdata[16])
+{
WARNING: Avoid sized array arguments
#1050: FILE: net/iucv/iucv.c:1050:
+int iucv_path_sever(struct iucv_path *path, u8 userdata[16])
+{
total: 0 errors, 6 warnings, 0 checks, 2119 lines checked
---
scripts/checkpatch.pl | 18 ++
1 file changed, 18 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e14dcdb..747b164 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5422,6 +5422,24 @@ sub process {
"externs should be avoided in .c files\n" .
$herecurr);
}
+# check for function arguments using arg[SIZE]
+ if ($^V && $^V ge 5.10.0 &&
+ defined $stat &&
+ $stat =~
/^.\s*(?:$Declare|$DeclareMisordered)\s*$Ident\s*($balanced_parens)\s*\{/s) {
+ my $func_args = $1;
+ if ($func_args =~
/(.*)\[\s*(?:$Constant|[A-Z0-9_]+)\s*\]/ && (!defined($1) || $1 !~
/\[\s*\]\s*$/)) {
+ my $ctx = '';
+ my $herectx = $here . "\n";
+ my $cnt = statement_rawlines($stat);
+ for (my $n = 0; $n < $cnt; $n++) {
+ $herectx .= raw_line($linenr, $n) .
"\n";
+ $n = $cnt if ($herectx =~ /{/);
+ }
+ WARN("SIZED_ARRAY_ARGUMENT",
+"Avoid sized array arguments\n" .
$herectx);
+ }
+ }
+
# checks for new __setup's
if ($rawline =~ /\b__setup\("([^"]*)"/) {
my $name = $1;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] flow_dissector: Use 'const' where possible.
On Tue, 2015-09-01 at 21:19 -0700, David Miller wrote: > Signed-off-by: David S. Miller [] > net/core/flow_dissector.c | 79 > --- Thanks David. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] flow_dissector: Fix function argument ordering dependency
On Tue, 2015-09-01 at 18:11 -0700, Tom Herbert wrote:
> Commit c6cc1ca7f4d70c ("flowi: Abstract out functions to get flow hash
> based on flowi") introduced a bug in __skb_set_sw_hash where we
> require a dependency on evaluating arguments in a function in order.
> There is no such ordering enforced in C, so this incorrect. This
> patch fixes that by splitting out the arguments. This bug was
> found via a compiler warning that keys may be uninitialized.
To help the reader know a bit more about how these
functions operate, perhaps it'd also be useful/better
to mark arguments as const where appropriate.
I didn't look to see if any other functions could
have the flow related args as const.
The __get_hash_from_flowi6 and flow_keys_have_l4
functions could be:
---
include/net/flow.h | 2 +-
include/net/flow_dissector.h | 2 +-
net/core/flow_dissector.c| 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/net/flow.h b/include/net/flow.h
index dafe97c..96fef26 100644
--- a/include/net/flow.h
+++ b/include/net/flow.h
@@ -244,7 +244,7 @@ void flow_cache_flush(struct net *net);
void flow_cache_flush_deferred(struct net *net);
extern atomic_t flow_cache_genid;
-__u32 __get_hash_from_flowi6(struct flowi6 *fl6, struct flow_keys *keys);
+__u32 __get_hash_from_flowi6(const struct flowi6 *fl6, struct flow_keys *keys);
static inline __u32 get_hash_from_flowi6(struct flowi6 *fl6)
{
diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 8c8548c..9fdcc3f 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -177,7 +177,7 @@ struct flow_keys_digest {
void make_flow_keys_digest(struct flow_keys_digest *digest,
const struct flow_keys *flow);
-static inline bool flow_keys_have_l4(struct flow_keys *keys)
+static inline bool flow_keys_have_l4(const struct flow_keys *keys)
{
return (keys->ports.ports || keys->tags.flow_label);
}
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 345a040..ccece96 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -787,7 +787,7 @@ u32 skb_get_poff(const struct sk_buff *skb)
return __skb_get_poff(skb, skb->data, &keys, skb_headlen(skb));
}
-__u32 __get_hash_from_flowi6(struct flowi6 *fl6, struct flow_keys *keys)
+__u32 __get_hash_from_flowi6(const struct flowi6 *fl6, struct flow_keys *keys)
{
memset(keys, 0, sizeof(*keys));
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mwifiex: Make mwifiex_dbg a function, reduce object size
On Tue, 2015-09-01 at 15:09 +, David Laight wrote: > From: Joe Perches > > Sent: 31 August 2015 18:47 > > > > The mwifiex_dbg macro has two tests that could be consolidated > > into a function reducing overall object size ~10KB (~4%). > > > > So convert the macro into a function. > > This looks like it will slow things down somewhat. I doubt in any meaningful way. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mwifiex: Make mwifiex_dbg a function, reduce object size
The mwifiex_dbg macro has two tests that could be consolidated
into a function reducing overall object size ~10KB (~4%).
So convert the macro into a function.
$ size drivers/net/wireless/mwifiex/built-in.o* (x86-64 defconfig)
textdata bss dec hex filename
23310286284809 246539 3c30b
drivers/net/wireless/mwifiex/built-in.o.new
24394986284809 257386 3ed6a
drivers/net/wireless/mwifiex/built-in.o.old
Signed-off-by: Joe Perches
---
drivers/net/wireless/mwifiex/main.c | 20
drivers/net/wireless/mwifiex/main.h | 17 -
2 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/mwifiex/main.c
b/drivers/net/wireless/mwifiex/main.c
index 278dc94..4fa8ca3 100644
--- a/drivers/net/wireless/mwifiex/main.c
+++ b/drivers/net/wireless/mwifiex/main.c
@@ -1447,6 +1447,26 @@ exit_sem_err:
}
EXPORT_SYMBOL_GPL(mwifiex_remove_card);
+void _mwifiex_dbg(const struct mwifiex_adapter *adapter, int mask,
+ const char *fmt, ...)
+{
+ struct va_format vaf;
+ va_list args;
+
+ if (!adapter->dev || !(adapter->debug_mask & mask))
+ return;
+
+ va_start(args, fmt);
+
+ vaf.fmt = fmt;
+ vaf.va = &args;
+
+ dev_info(adapter->dev, "%pV", &vaf);
+
+ va_end(args);
+}
+EXPORT_SYMBOL_GPL(_mwifiex_dbg);
+
/*
* This function initializes the module.
*
diff --git a/drivers/net/wireless/mwifiex/main.h
b/drivers/net/wireless/mwifiex/main.h
index 6b95121..96663214 100644
--- a/drivers/net/wireless/mwifiex/main.h
+++ b/drivers/net/wireless/mwifiex/main.h
@@ -48,6 +48,9 @@
extern const char driver_version[];
+struct mwifiex_adapter;
+struct mwifiex_private;
+
enum {
MWIFIEX_ASYNC_CMD,
MWIFIEX_SYNC_CMD
@@ -180,12 +183,11 @@ enum MWIFIEX_DEBUG_LEVEL {
MWIFIEX_DBG_FATAL | \
MWIFIEX_DBG_ERROR)
-#define mwifiex_dbg(adapter, dbg_mask, fmt, args...) \
-do { \
- if ((adapter)->debug_mask & MWIFIEX_DBG_##dbg_mask) \
- if ((adapter)->dev) \
- dev_info((adapter)->dev, fmt, ## args); \
-} while (0)
+__printf(3, 4)
+void _mwifiex_dbg(const struct mwifiex_adapter *adapter, int mask,
+ const char *fmt, ...);
+#define mwifiex_dbg(adapter, mask, fmt, ...) \
+ _mwifiex_dbg(adapter, MWIFIEX_DBG_##mask, fmt, ##__VA_ARGS__)
#define DEBUG_DUMP_DATA_MAX_LEN128
#define mwifiex_dbg_dump(adapter, dbg_mask, str, buf, len) \
@@ -506,9 +508,6 @@ enum mwifiex_iface_work_flags {
MWIFIEX_IFACE_WORK_CARD_RESET,
};
-struct mwifiex_adapter;
-struct mwifiex_private;
-
struct mwifiex_private {
struct mwifiex_adapter *adapter;
u8 bss_type;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC V3 2/2] net: Optimize snmp stat aggregation by walking all the percpu data at once
On Sat, 2015-08-29 at 07:32 -0700, Eric Dumazet wrote:
> On Sat, 2015-08-29 at 14:37 +0530, Raghavendra K T wrote:
>
> >
> > static inline void __snmp6_fill_stats64(u64 *stats, void __percpu *mib,
> > - int items, int bytes, size_t syncpoff)
> > + int items, int bytes, size_t syncpoff)
> > {
> > - int i;
> > + int i, c;
> > int pad = bytes - sizeof(u64) * items;
> > + u64 buff[items];
> > +
>
> One last comment : using variable length arrays is confusing for the
> reader, and sparse as well.
>
> $ make C=2 net/ipv6/addrconf.o
> ...
> CHECK net/ipv6/addrconf.c
> net/ipv6/addrconf.c:4733:18: warning: Variable length array is used.
> net/ipv6/addrconf.c:4737:25: error: cannot size expression
>
>
> I suggest you remove 'items' parameter and replace it by
> IPSTATS_MIB_MAX, as __snmp6_fill_stats64() is called exactly once.
If you respin, I suggest:
o remove "items" from the __snmp6_fill_stats64 arguments
and use IPSTATS_MIB_MAX in the function instead
o add braces around the for_each_possible_cpu loop
for_each_possible_cpu(c) {
for (i = 1; i < items; i++)
buff[i] += snmp_get_cpu_field64(mib, c, i, syncpoff);
}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC V2 2/2] net: Optimize snmp stat aggregation by walking all the percpu data at once
On Fri, 2015-08-28 at 17:06 -0700, Eric Dumazet wrote: > On Fri, 2015-08-28 at 16:12 -0700, Joe Perches wrote: > > Generally true. It's always difficult to know how much > > stack has been consumed though and smaller stack frames > > are generally better. [] > So for a _leaf_ function, it is better to declare an automatic variable, > as you in fact reduce max stack depth. That of course depends on what a "leaf" is and whether or not any other function call in the "leaf" consumes stack. inet6_fill_ifla6_attrs does call other functions (none of which has the stack frame size of k.alloc) > Not only it uses less kernel stack, it is also way faster, as you avoid > kmalloc()/kfree() overhead and reuse probably already hot cache lines in > kernel stack. yup. You'll also never neglect to free stack like the original RFC patch neglected to free the alloc. cheers, Joe -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC V2 2/2] net: Optimize snmp stat aggregation by walking all the percpu data at once
On Fri, 2015-08-28 at 15:29 -0700, Eric Dumazet wrote: > On Fri, 2015-08-28 at 14:26 -0700, Joe Perches wrote: > 1) u64 array[XX] on stack is naturally aligned, Of course it is. > kzalloc() wont improve this at all. Not sure what you believe. An alloc would only reduce stack use. Copying into the buffer, then copying the buffer into the skb may be desirable on some arches though. > 2) put_unaligned() is basically a normal memory write on x86. > memcpy(dst,src,...) will have a problem anyway on arches that care, > because src & dst wont have same alignment. OK, so all the world's an x86? On arm32, copying 288 bytes using nearly all aligned word transfers is generally faster than using only unsigned short transfers. > 288 bytes on stack in a leaf function in this path is totally fine, it > is not like we're calling ext4/xfs/nfs code after this point. Generally true. It's always difficult to know how much stack has been consumed though and smaller stack frames are generally better. Anyway, the block copy from either the alloc'd or stack buffer amounts only to a slight performance improvement for arm32. It doesn't really have much other utility. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC V2 2/2] net: Optimize snmp stat aggregation by walking all the percpu data at once
On Fri, 2015-08-28 at 14:14 -0700, Eric Dumazet wrote: > On Fri, 2015-08-28 at 14:09 -0700, Joe Perches wrote: > > On Fri, 2015-08-28 at 13:55 -0700, Eric Dumazet wrote: > > > On Fri, 2015-08-28 at 13:53 -0700, Joe Perches wrote: > > > > It's 288 bytes on stack, maybe a kzalloc would be clearer too. > > > > > > Could you read patch history and check why this has been rejected ? > > > > I don't see a rejection, just that the initial > > submission didn't check the allocation or add > > an allocation buffer via kcalloc/kzalloc to the > > inet6_fill_ifla6_attrs caller and change the > > snmp6_fill_stats arguments. > > > > It could also eliminate the put_unaligned calls. > > Not really. You do not properly read this code. Always a possibility, but I don't think so. > put_unaligned is happening on a space allocated from rtnetlink skb, not > the temp space needed to perform the per cpu folding. That's why I suggested changing the snmp_fill_stats arguments. If the naturally aligned allocated u64 array is used and then copied as a block to the rtnetlink skb, I believe there's no alignment issue that would require put_unaligned. Do I still miss something? > https://lkml.org/lkml/2015/8/25/476 I read that. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC V2 2/2] net: Optimize snmp stat aggregation by walking all the percpu data at once
On Fri, 2015-08-28 at 13:55 -0700, Eric Dumazet wrote: > On Fri, 2015-08-28 at 13:53 -0700, Joe Perches wrote: > > On Fri, 2015-08-28 at 13:33 -0700, Eric Dumazet wrote: > > > We do not bother for small struct. > > > > > > Here, the array is big enough that David prefers having an explicit > > > memset() so that it clearly shows that author of this code was aware of > > > this. > > > > It's 288 bytes on stack, maybe a kzalloc would be clearer too. > > Could you read patch history and check why this has been rejected ? I don't see a rejection, just that the initial submission didn't check the allocation or add an allocation buffer via kcalloc/kzalloc to the inet6_fill_ifla6_attrs caller and change the snmp6_fill_stats arguments. It could also eliminate the put_unaligned calls. https://lkml.org/lkml/2015/8/25/114 Was there some other thread? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC V2 2/2] net: Optimize snmp stat aggregation by walking all the percpu data at once
On Fri, 2015-08-28 at 13:33 -0700, Eric Dumazet wrote: > We do not bother for small struct. > > Here, the array is big enough that David prefers having an explicit > memset() so that it clearly shows that author of this code was aware of > this. It's 288 bytes on stack, maybe a kzalloc would be clearer too. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] net/mlx4_en: Fix IPv6 csum calculation
On Fri, 2015-08-28 at 14:28 -0500, cls...@linux.vnet.ibm.com wrote:
> From: Carol L Soto
>
> Seeing this message with mlx4_eni with IPv6: hw csum failure
>
> Changing IPv6 csum calculation to be based on OFED 2.4 code.
> When calculate IPv6 csum based also on source and destination
> addresses.
[]
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
> b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
[]
> +static __wsum csum_ipv6_magic_nofold(const struct in6_addr *saddr,
> + const struct in6_addr *daddr,
> + __u32 len, unsigned short proto)
> +{
> + __wsum res = 0;
> +
> + res = csum_add(res, saddr->in6_u.u6_addr32[0]);
> + res = csum_add(res, saddr->in6_u.u6_addr32[1]);
> + res = csum_add(res, saddr->in6_u.u6_addr32[2]);
> + res = csum_add(res, saddr->in6_u.u6_addr32[3]);
> + res = csum_add(res, daddr->in6_u.u6_addr32[0]);
> + res = csum_add(res, daddr->in6_u.u6_addr32[1]);
> + res = csum_add(res, daddr->in6_u.u6_addr32[2]);
> + res = csum_add(res, daddr->in6_u.u6_addr32[3]);
> + res = csum_add(res, len);
> + res = csum_add(res, htonl(proto));
You should try running sparse on this code.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC V2 2/2] net: Optimize snmp stat aggregation by walking all the percpu data at once
On Fri, 2015-08-28 at 11:24 -0700, David Miller wrote:
> From: Raghavendra K T
> Date: Fri, 28 Aug 2015 12:09:52 +0530
>
> > On 08/28/2015 12:08 AM, David Miller wrote:
> >> From: Raghavendra K T
> >> Date: Wed, 26 Aug 2015 23:07:33 +0530
> >>
> >>> @@ -4641,10 +4647,12 @@ static inline void __snmp6_fill_stats64(u64
> >>> *stats, void __percpu *mib,
> >>> static void snmp6_fill_stats(u64 *stats, struct inet6_dev *idev, int
> >>> attrtype,
> >>>int bytes)
> >>> {
> >>> + u64 buff[IPSTATS_MIB_MAX] = {0,};
> >>> +
> ...
> > hope you wanted to know the overhead than to change the current
> > patch. please let me know..
>
> I want you to change that variable initializer to an explicit memset().
>
> The compiler is emitting a memset() or similar _anyways_.
>
> Not because it will have any impact at all upon performance, but because
> of how it looks to people trying to read and understand the code.
I don't read it as particularly different.
There are > 100 uses of the not quite a memset initialization
style using "= { <0,> }" in net/
$ git grep -E "=\s*\{\s*0?\s*,?\s*\}" net | wc -l
138
There is a difference though if a struct is copied to
user-space as a {} initialization only guarantees that
struct members are initialized to 0 where memset also
zeros any alignment padding.
Maybe a checkpatch rule like this?
---
scripts/checkpatch.pl | 7 +++
1 file changed, 7 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e14dcdb..f79e5c9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3237,6 +3237,13 @@ sub process {
next;
}
+# check for non-global initializations that could be memset
+ if ($realfile =~ m@^(drivers/net/|net/)@ &&
+ $sline =~ /^.\s+$Declare\s*$Ident\s*=\s*\{\s*0?\s*,?\s*\}/)
{
+ CHK("BRACE_INITIALIZATION",
+ "Prefer an explicit memset to a declaration
initialization\n" . $herecurr);
+ }
+
# check for initialisation to aggregates open brace on the next line
if ($line =~ /^.\s*{/ &&
$prevline =~ /(?:^|[^=])=\s*$/) {
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch net-next v2 2/3] mlxsw: adjust transmit fail log message level in __mlxsw_emad_transmit
On Thu, 2015-08-27 at 17:59 +0200, Jiri Pirko wrote:
> When transmit fails, it is an error, not a warning.
[]
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c
> b/drivers/net/ethernet/mellanox/mlxsw/core.c
[]
> @@ -376,8 +376,8 @@ static int __mlxsw_emad_transmit(struct mlxsw_core
> *mlxsw_core,
>
> err = mlxsw_core_skb_transmit(mlxsw_core->driver_priv, skb, tx_info);
> if (err) {
> - dev_warn(mlxsw_core->bus_info->dev, "Failed to transmit EMAD
> (tid=%llx)\n",
> - mlxsw_core->emad.tid);
> + dev_err(mlxsw_core->bus_info->dev, "Failed to transmit EMAD
> (tid=%llx)\n",
> + mlxsw_core->emad.tid);
dev_err_ratelimited?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next v2 01/14] i40e: don't degrade __le16
On Wed, 2015-08-26 at 15:49 -0700, Jeff Kirsher wrote: > diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c [] > @@ -2106,11 +2106,12 @@ int i40e_ndo_set_vf_port_vlan(struct net_device > *netdev, [] > dev_err(&pf->pdev->dev, > "VF %d has already configured VLAN filters and the > administrator is requesting a port VLAN override.\nPlease unload and reload > the VF driver for this change to take effect.\n", > vf_id); Unrelated trivia: This might be better with separate dev_err calls so there is a consistent prefix in the logging output. dev_err(&pf->pdev->dev, "VF %d has already configured VLAN filters and the administrator is requesting a port VLAN override.\n", vf_id); dev_err(&pf->pdev->dev, "Please unload and reload the VF driver for this change to take effect.\n"); -- 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
[TRIVIAL PATCH V2] smsc9194: Remove uncompilable #if 0'd use of pr_dbg
No pr_dbg method exists.
While this code is #if 0'd, it'd be nicer to
use the generic hex_dump, so use it instead.
Signed-off-by: Joe Perches
---
no print_hex_dump_dbg method exists either.
V2: Change the uncompilable print_hex_dump_dbg call to
print_hex_dump_debug that actually could be compiled...
Or maybe just delete the driver altogether.
It'd probably be nice to one day create something
like drivers/net/ethernet/obsolete and eventually
kill off code for the stuff that hasn't been
supported or sold in the last 20 years.
drivers/net/ethernet/smsc/smc9194.c | 32 ++--
1 file changed, 2 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smc9194.c
b/drivers/net/ethernet/smsc/smc9194.c
index 67d9fde..94857c1 100644
--- a/drivers/net/ethernet/smsc/smc9194.c
+++ b/drivers/net/ethernet/smsc/smc9194.c
@@ -1031,36 +1031,8 @@ err_out:
static void print_packet( byte * buf, int length )
{
#if 0
- int i;
- int remainder;
- int lines;
-
- pr_dbg("Packet of length %d\n", length);
- lines = length / 16;
- remainder = length % 16;
-
- for ( i = 0; i < lines ; i ++ ) {
- int cur;
-
- printk(KERN_DEBUG);
- for ( cur = 0; cur < 8; cur ++ ) {
- byte a, b;
-
- a = *(buf ++ );
- b = *(buf ++ );
- pr_cont("%02x%02x ", a, b);
- }
- pr_cont("\n");
- }
- printk(KERN_DEBUG);
- for ( i = 0; i < remainder/2 ; i++ ) {
- byte a, b;
-
- a = *(buf ++ );
- b = *(buf ++ );
- pr_cont("%02x%02x ", a, b);
- }
- pr_cont("\n");
+ print_hex_dump_debug(DRV_NAME, DUMP_PREFIX_OFFSET, 16, 1,
+buf, length, true);
#endif
}
#endif
--
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/
--
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
[TRIVIAL PATCH] smsc9194: Remove uncompilable #if 0'd use of pr_dbg
No pr_dbg method exists.
While this code is #if 0'd, it'd be nicer to
use the generic hex_dump, so use it instead.
Signed-off-by: Joe Perches
---
Or maybe just delete the driver altogether.
It'd probably be nice to one day create something
like drivers/net/ethernet/obsolete and eventually
kill off code for the stuff that hasn't been
supported or sold in the last 20 years.
drivers/net/ethernet/smsc/smc9194.c | 32 ++--
1 file changed, 2 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smc9194.c
b/drivers/net/ethernet/smsc/smc9194.c
index 67d9fde..94857c1 100644
--- a/drivers/net/ethernet/smsc/smc9194.c
+++ b/drivers/net/ethernet/smsc/smc9194.c
@@ -1031,36 +1031,8 @@ err_out:
static void print_packet( byte * buf, int length )
{
#if 0
- int i;
- int remainder;
- int lines;
-
- pr_dbg("Packet of length %d\n", length);
- lines = length / 16;
- remainder = length % 16;
-
- for ( i = 0; i < lines ; i ++ ) {
- int cur;
-
- printk(KERN_DEBUG);
- for ( cur = 0; cur < 8; cur ++ ) {
- byte a, b;
-
- a = *(buf ++ );
- b = *(buf ++ );
- pr_cont("%02x%02x ", a, b);
- }
- pr_cont("\n");
- }
- printk(KERN_DEBUG);
- for ( i = 0; i < remainder/2 ; i++ ) {
- byte a, b;
-
- a = *(buf ++ );
- b = *(buf ++ );
- pr_cont("%02x%02x ", a, b);
- }
- pr_cont("\n");
+ print_hex_dump_dbg(DRV_NAME, DUMP_PREFIX_OFFSET, 16, 1,
+ buf, length, true);
#endif
}
#endif
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: phy: workaround for buggy cable detection by LAN8700 after cable plugging
On Thu, 2015-08-13 at 20:11 +0300, Igor Plyatov wrote:
> > On Thu, 2015-08-13 at 16:12 +0300, Igor Plyatov wrote:
> >> * Due to HW bug, LAN8700 sometimes does not detect presence of energy in
> >> the
> >>Ethernet cable in Energy Detect Power-Down mode (e.g while EDPWRDOWN
> >> bit is
> >>set, the ENERGYON bit does not asserted sometimes). This is a common
> >> bug of
> >>LAN87xx family of PHY chips.
> >> * The lan87xx_read_status() was improved to acquire ENERGYON bit. Its
> >> previous
> >>algorythm still not reliable on 100 % and sometimes skip cable plugging.
> > []
> >> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> > []
> >> @@ -104,10 +104,12 @@ static int lan911x_config_init(struct phy_device
> >> *phydev)
> >> static int lan87xx_read_status(struct phy_device *phydev)
> >> {
> >>int err = genphy_read_status(phydev);
> >> + int rc;
> > Is there a reason to move this declaration?
>
> There is no strict requirement to move declaration of the rc.
> It was made just to have all declarations easily visible.
Generally it's better to have declarations
in the minimal/narrowest scope possible.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: phy: workaround for buggy cable detection by LAN8700 after cable plugging
On Thu, 2015-08-13 at 16:12 +0300, Igor Plyatov wrote:
> * Due to HW bug, LAN8700 sometimes does not detect presence of energy in the
> Ethernet cable in Energy Detect Power-Down mode (e.g while EDPWRDOWN bit is
> set, the ENERGYON bit does not asserted sometimes). This is a common bug of
> LAN87xx family of PHY chips.
> * The lan87xx_read_status() was improved to acquire ENERGYON bit. Its previous
> algorythm still not reliable on 100 % and sometimes skip cable plugging.
[]
> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
[]
> @@ -104,10 +104,12 @@ static int lan911x_config_init(struct phy_device
> *phydev)
> static int lan87xx_read_status(struct phy_device *phydev)
> {
> int err = genphy_read_status(phydev);
> + int rc;
Is there a reason to move this declaration?
> + int i;
>
> if (!phydev->link) {
> /* Disable EDPD to wake up PHY */
> - int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> + rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> if (rc < 0)
> return rc;
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/fddi:change HWM_REVERSE() macro
On Tue, 2015-08-11 at 00:14 +0800, yalin wang wrote: > HWM_REVERSE Is unused and it would be better if removed. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net_dbg_ratelimited: turn into no-op when !DEBUG
On Tue, 2015-08-04 at 17:07 +0200, Jason A. Donenfeld wrote:
> The pr_debug family of functions turns into a no-op when -DDEBUG is not
> specified, opting instead to call "no_printk", which gets compiled to a
> no-op (but retains gcc's nice warnings about printf-style arguments).
[]
> diff --git a/include/linux/net.h b/include/linux/net.h
[]
> @@ -239,8 +239,14 @@ do {
> \
> net_ratelimited_function(pr_warn, fmt, ##__VA_ARGS__)
> #define net_info_ratelimited(fmt, ...) \
> net_ratelimited_function(pr_info, fmt, ##__VA_ARGS__)
> +#if defined(DEBUG)
> #define net_dbg_ratelimited(fmt, ...)\
> net_ratelimited_function(pr_debug, fmt, ##__VA_ARGS__)
> +#else
> +#define net_dbg_ratelimited(fmt, ...)\
> + if (0) \
> + no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> +#endif
This should be:
#define net_dbg_ratelimited(fmt, ...) \
do {\
if (0) \
no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
} while (0)
to be safe in if/else uses
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net_dbg_ratelimited: turn into no-op when !DEBUG
On Mon, 2015-08-03 at 20:57 -0700, Joe Perches wrote: > On Tue, 2015-08-04 at 05:26 +0200, Jason A. Donenfeld wrote: > > This patch replaces calls to net_dbg_ratelimited when !DEBUG with > > no_printk, keeping with the idiom of all the other debug print helpers. > > Makes sense, thanks Jason. Perhaps better still would be to use if (0) no_printk so that the call and whatever argument calls the net_dbg_ratelimited makes are completely eliminated. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net_dbg_ratelimited: turn into no-op when !DEBUG
On Tue, 2015-08-04 at 05:26 +0200, Jason A. Donenfeld wrote: > The pr_debug family of functions turns into a no-op when -DDEBUG is not > specified, opting instead to call "no_printk", which gets compiled to a > no-op (but retains gcc's nice warnings about printf-style arguments). > > The problem with net_dbg_ratelimited is that it is defined to be a > variant of net_ratelimited_function, which expands to essentially: > > if (net_ratelimit()) > pr_debug(fmt, ...); > > When DEBUG is not defined, then this becomes, > > if (net_ratelimit()) > ; > > This seems benign, except it isn't. Firstly, there's the obvious > overhead of calling net_ratelimit needlessly, which does quite some book > keeping for the rate limiting. Given that the pr_debug and > net_dbg_ratelimited family of functions are sprinkled liberally through > performance critical code, with developers assuming they'll be compiled > out to a no-op most of the time, we certainly do not want this needless > book keeping. Secondly, and most visibly, even though no debug message > is printed when DEBUG is not defined, if there is a flood of > invocations, dmesg winds up peppered with messages such as > "net_ratelimit: 320 callbacks suppressed". This is because our > aforementioned net_ratelimit() function actually prints this text in > some circumstances. It's especially odd to see this when there isn't any > other accompanying debug message. > > So, in sum, it doesn't make sense to have this function's current > behavior, and instead it should match what every other debug family of > functions in the kernel does with !DEBUG -- nothing. > > This patch replaces calls to net_dbg_ratelimited when !DEBUG with > no_printk, keeping with the idiom of all the other debug print helpers. Makes sense, thanks Jason. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fddi: Use a more more typical logging style
On Mon, 2015-08-03 at 16:05 -0700, David Miller wrote: > From: Joe Perches > Date: Sun, 02 Aug 2015 21:27:45 -0700 > > > Use macros that don't require fixed argument counts so > > format and arguments can be verified by the compiler. > > > > Miscellanea: > > > > o Remove a few #if uses to allow dynamic debug to always work > > o whitespace neatening > > > > Signed-off-by: Joe Perches > > This doesn't apply cleanly to net-next, please respin. Apologies for that. I used a newer version of the Evolution email client (3.16.0) which corrupts tabs. 3.16.3 doesn't seem to do that. I'll probably downgrade back to the old 3.12 version though. It doesn't send some attachments properly, but the editor at least works well. -- 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
