* Re: [PATCH net] packet: bail out of packet_snd() if L2 header creation fails
From: Joe Perches @ 2015-01-11 18:52 UTC (permalink / raw)
To: Christoph Jaeger, Alan
Cc: davem, willemb, edumazet, dborkman, netdev, linux-kernel
In-Reply-To: <1420999276-28225-1-git-send-email-cj@linux.com>
On Sun, 2015-01-11 at 13:01 -0500, Christoph Jaeger wrote:
> Due to a misplaced parenthesis, the expression
>
> (unlikely(offset) < 0),
>
> which expands to
>
> (__builtin_expect(!!(offset), 0) < 0),
Here's another one:
drivers/platform/goldfish/goldfish_pipe.c:285: if (unlikely(bufflen) == 0)
^ permalink raw reply
* [PATCH] net: dnet: fix dnet_poll()
From: Eric Dumazet @ 2015-01-11 19:02 UTC (permalink / raw)
To: David Miller; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
A NAPI poll() handler is supposed to return exactly the budget when/if
napi_complete() has not been called.
It is also supposed to return number of frames that were received, so
that netdev_budget can have a meaning.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
Note : Untested patch, but this driver seems pretty buggy, not sure if
anyone uses it.
drivers/net/ethernet/dnet.c | 18 +++++-------------
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/dnet.c b/drivers/net/ethernet/dnet.c
index a379c3e4b57f..13d00a38a5bd 100644
--- a/drivers/net/ethernet/dnet.c
+++ b/drivers/net/ethernet/dnet.c
@@ -398,13 +398,8 @@ static int dnet_poll(struct napi_struct *napi, int budget)
* break out of while loop if there are no more
* packets waiting
*/
- if (!(dnet_readl(bp, RX_FIFO_WCNT) >> 16)) {
- napi_complete(napi);
- int_enable = dnet_readl(bp, INTR_ENB);
- int_enable |= DNET_INTR_SRC_RX_CMDFIFOAF;
- dnet_writel(bp, int_enable, INTR_ENB);
- return 0;
- }
+ if (!(dnet_readl(bp, RX_FIFO_WCNT) >> 16))
+ break;
cmd_word = dnet_readl(bp, RX_LEN_FIFO);
pkt_len = cmd_word & 0xFFFF;
@@ -433,20 +428,17 @@ static int dnet_poll(struct napi_struct *napi, int budget)
"size %u.\n", dev->name, pkt_len);
}
- budget -= npackets;
-
if (npackets < budget) {
/* We processed all packets available. Tell NAPI it can
- * stop polling then re-enable rx interrupts */
+ * stop polling then re-enable rx interrupts.
+ */
napi_complete(napi);
int_enable = dnet_readl(bp, INTR_ENB);
int_enable |= DNET_INTR_SRC_RX_CMDFIFOAF;
dnet_writel(bp, int_enable, INTR_ENB);
- return 0;
}
- /* There are still packets waiting */
- return 1;
+ return npackets;
}
static irqreturn_t dnet_interrupt(int irq, void *dev_id)
^ permalink raw reply related
* Re: [PATCH net] packet: bail out of packet_snd() if L2 header creation fails
From: Christoph Jaeger @ 2015-01-11 19:34 UTC (permalink / raw)
To: Joe Perches
Cc: Alan, davem, willemb, edumazet, dborkman, netdev, linux-kernel
In-Reply-To: <1421002345.9233.1.camel@perches.com>
On Sun, Jan 11, 2015 at 10:52:25AM -0800, Joe Perches wrote:
> On Sun, 2015-01-11 at 13:01 -0500, Christoph Jaeger wrote:
> > Due to a misplaced parenthesis, the expression
> >
> > (unlikely(offset) < 0),
> >
> > which expands to
> >
> > (__builtin_expect(!!(offset), 0) < 0),
>
> Here's another one:
>
> drivers/platform/goldfish/goldfish_pipe.c:285: if (unlikely(bufflen) == 0)
Well, the conditional statement works as intended. Of course, the branch
prediction doesn't.
Coccinelle should be able to check for this kind of likely()/unlikely() usage,
shouldn't it?
~Christoph
^ permalink raw reply
* [PATCH net-next v1] net: bnx2x: avoid macro redefinition
From: David Decotigny @ 2015-01-11 19:42 UTC (permalink / raw)
To: Ariel Elior, netdev, linux-kernel; +Cc: David Decotigny
From: David Decotigny <decot@googlers.com>
Signed-off-by: David Decotigny <decot@googlers.com>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index 792ba72..756053c 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -1138,12 +1138,8 @@ struct bnx2x_port {
u32 link_config[LINK_CONFIG_SIZE];
u32 supported[LINK_CONFIG_SIZE];
-/* link settings - missing defines */
-#define SUPPORTED_2500baseX_Full (1 << 15)
u32 advertising[LINK_CONFIG_SIZE];
-/* link settings - missing defines */
-#define ADVERTISED_2500baseX_Full (1 << 15)
u32 phy_addr;
--
2.2.0.rc0.207.ga3a616c
^ permalink raw reply related
* [PATCH] checkpatch: Add likely/unlikely comparison misuse test
From: Joe Perches @ 2015-01-11 19:49 UTC (permalink / raw)
To: Christoph Jaeger, Andrew Morton, Julia Lawall
Cc: Alan, davem, willemb, edumazet, dborkman, netdev, linux-kernel
In-Reply-To: <20150111193404.GN1513@betelgeuse.hsd1.ma.comcast.net>
Add a test for probably likely/unlikely misuses where
the comparison is likely misplaced
if (likely(foo) > 0)
vs
if (likely(foo > 0))
Signed-off-by: Joe Perches <joe@perches.com>
---
On Sun, 2015-01-11 at 14:34 -0500, Christoph Jaeger wrote:
> > drivers/platform/goldfish/goldfish_pipe.c:285: if (unlikely(bufflen) == 0)
>
> Well, the conditional statement works as intended. Of course, the branch
> prediction doesn't.
>
> Coccinelle should be able to check for this kind of likely()/unlikely() usage,
> shouldn't it?
Most likely, checkpatch could too, but not as well.
This misuse isn't very common. (2 in current source?)
scripts/checkpatch.pl | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6afc24b..b8d47dc 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5219,6 +5219,13 @@ sub process {
"#define of '$1' is wrong - use Kconfig variables or standard guards instead\n" . $herecurr);
}
+# likely/unlikely comparisons similar to "(likely(foo) > 0)"
+ if ($^V && $^V ge 5.10.0 &&
+ $line =~ /\b((?:un)?likely)\s*\(\s*$FuncArg\s*\)\s*$Compare/) {
+ WARN("LIKELY_MISUSE",
+ "Using $1 should generally have parentheses around the comparison\n" . $herecurr);
+ }
+
# whine mightly about in_atomic
if ($line =~ /\bin_atomic\s*\(/) {
if ($realfile =~ m@^drivers/@) {
^ permalink raw reply related
* [PATCH v4 00/04] can: Introduce support for Kvaser USBCAN-II devices
From: Ahmed S. Darwish @ 2015-01-11 20:05 UTC (permalink / raw)
To: Olivier Sobrie, Oliver Hartkopp, Wolfgang Grandegger,
Marc Kleine-Budde
Cc: David S. Miller, Paul Gortmaker, Linux-CAN, netdev, LKML
In-Reply-To: <20141223154654.GB6460@vivalin-002>
Hi,
Now since earlier v3 submission patches #1-3 got merged, this
is a new patch series expanding on patch v3 #4: support for
the USBCAN-II family.
A new series is introduced due to the extra additions suggested
by code review, which required being added in their own
self-contained patches.
Thanks,
Darwish
^ permalink raw reply
* Re: [PATCH v4 01/04] can: kvaser_usb: Don't dereference skb after a netif_rx() devices
From: Ahmed S. Darwish @ 2015-01-11 20:11 UTC (permalink / raw)
To: Olivier Sobrie, Oliver Hartkopp, Wolfgang Grandegger,
Marc Kleine-Budde
Cc: David S. Miller, Paul Gortmaker, Linux-CAN, netdev, LKML
In-Reply-To: <20150111200544.GA8855@linux>
From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
We should not touch the packet after a netif_rx: it might
get freed behind our back.
Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
drivers/net/can/usb/kvaser_usb.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index cc7bfc0..c32cd61 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -520,10 +520,10 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
skb = alloc_can_err_skb(priv->netdev, &cf);
if (skb) {
cf->can_id |= CAN_ERR_RESTARTED;
- netif_rx(skb);
stats->rx_packets++;
stats->rx_bytes += cf->can_dlc;
+ netif_rx(skb);
} else {
netdev_err(priv->netdev,
"No memory left for err_skb\n");
@@ -770,10 +770,9 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
priv->can.state = new_state;
- netif_rx(skb);
-
stats->rx_packets++;
stats->rx_bytes += cf->can_dlc;
+ netif_rx(skb);
}
static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
@@ -805,10 +804,9 @@ static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
stats->rx_over_errors++;
stats->rx_errors++;
- netif_rx(skb);
-
stats->rx_packets++;
stats->rx_bytes += cf->can_dlc;
+ netif_rx(skb);
}
}
@@ -887,10 +885,9 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
cf->can_dlc);
}
- netif_rx(skb);
-
stats->rx_packets++;
stats->rx_bytes += cf->can_dlc;
+ netif_rx(skb);
}
static void kvaser_usb_start_chip_reply(const struct kvaser_usb *dev,
--
1.9.1
^ permalink raw reply related
* [PATCH v4 2/4] can: kvaser_usb: Update error counters before exiting on OOM
From: Ahmed S. Darwish @ 2015-01-11 20:15 UTC (permalink / raw)
To: Olivier Sobrie, Oliver Hartkopp, Wolfgang Grandegger,
Marc Kleine-Budde
Cc: David S. Miller, Paul Gortmaker, Linux-CAN, netdev, LKML
In-Reply-To: <20150111201116.GB8855@linux>
From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
Let the error counters be more accurate in case of Out of
Memory conditions.
Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
drivers/net/can/usb/kvaser_usb.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index c32cd61..0eb870b 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -792,6 +792,9 @@ static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
}
if (msg->u.rx_can.flag & MSG_FLAG_OVERRUN) {
+ stats->rx_over_errors++;
+ stats->rx_errors++;
+
skb = alloc_can_err_skb(priv->netdev, &cf);
if (!skb) {
stats->rx_dropped++;
@@ -801,9 +804,6 @@ static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
cf->can_id |= CAN_ERR_CRTL;
cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
- stats->rx_over_errors++;
- stats->rx_errors++;
-
stats->rx_packets++;
stats->rx_bytes += cf->can_dlc;
netif_rx(skb);
--
1.9.1
^ permalink raw reply related
* Re: [PATCH net-next RFC 1/5] net-timestamp: no-payload option
From: Richard Cochran @ 2015-01-11 20:26 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: netdev, davem, eric.dumazet, luto
In-Reply-To: <1420824719-28848-2-git-send-email-willemb@google.com>
On Fri, Jan 09, 2015 at 12:31:55PM -0500, Willem de Bruijn wrote:
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index a317797..d81ef70 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -440,7 +440,7 @@ static bool ipv4_pktinfo_prepare_errqueue(const struct sock *sk,
>
> if ((ee_origin != SO_EE_ORIGIN_TIMESTAMPING) ||
> (!(sk->sk_tsflags & SOF_TIMESTAMPING_OPT_CMSG)) ||
> - (!skb->dev))
> + (!skb->dev) || (!skb->len))
> return false;
Nit: You have already tested for the condition (!skb->len) ...
>
> info->ipi_spec_dst.s_addr = ip_hdr(skb)->saddr;
> @@ -483,7 +483,7 @@ int ip_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
>
> serr = SKB_EXT_ERR(skb);
>
> - if (sin) {
> + if (sin && skb->len) {
> sin->sin_family = AF_INET;
> sin->sin_addr.s_addr = *(__be32 *)(skb_network_header(skb) +
> serr->addr_offset);
> @@ -496,8 +496,9 @@ int ip_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
> sin = &errhdr.offender;
> sin->sin_family = AF_UNSPEC;
>
> - if (serr->ee.ee_origin == SO_EE_ORIGIN_ICMP ||
> - ipv4_pktinfo_prepare_errqueue(sk, skb, serr->ee.ee_origin)) {
> + if (skb->len &&
> + (serr->ee.ee_origin == SO_EE_ORIGIN_ICMP ||
> + ipv4_pktinfo_prepare_errqueue(sk, skb, serr->ee.ee_origin))) {
... here.
> struct inet_sock *inet = inet_sk(sk);
>
> sin->sin_family = AF_INET;
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH RESEND 2/2] wlcore: align member-assigns in a structure-copy block
From: Giel van Schijndel @ 2015-01-11 20:32 UTC (permalink / raw)
To: Eliad Peller
Cc: Kalle Valo, LKML, John W. Linville, Arik Nemtsov,
open list:TI WILINK WIRELES..., open list:NETWORKING DRIVERS
In-Reply-To: <CAB3XZEcm4+=dYC-8_m_7YRdET_wPkWUYHcDuy9mCKdxkCFEnQA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3868 bytes --]
On Sun, Jan 11, 2015 at 12:22:49 +0200, Eliad Peller wrote:
> On Fri, Jan 9, 2015 at 7:03 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>> Giel van Schijndel <me@mortis.eu> writes:
>>> This highlights the differences (e.g. the bug fixed in the previous
>>> commit).
>>>
>>> Signed-off-by: Giel van Schijndel <me@mortis.eu>
>>> ---
>>> drivers/net/wireless/ti/wlcore/acx.c | 22 +++++++++++-----------
>>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ti/wlcore/acx.c b/drivers/net/wireless/ti/wlcore/acx.c
>>> index f28fa3b..93a2fa8 100644
>>> --- a/drivers/net/wireless/ti/wlcore/acx.c
>>> +++ b/drivers/net/wireless/ti/wlcore/acx.c
>>> @@ -1715,17 +1715,17 @@ int wl12xx_acx_config_hangover(struct wl1271 *wl)
>>> goto out;
>>> }
>>>
>>> - acx->recover_time = cpu_to_le32(conf->recover_time);
>>> - acx->hangover_period = conf->hangover_period;
>>> - acx->dynamic_mode = conf->dynamic_mode;
>>> - acx->early_termination_mode = conf->early_termination_mode;
>>> - acx->max_period = conf->max_period;
>>> - acx->min_period = conf->min_period;
>>> - acx->increase_delta = conf->increase_delta;
>>> - acx->decrease_delta = conf->decrease_delta;
>>> - acx->quiet_time = conf->quiet_time;
>>> - acx->increase_time = conf->increase_time;
>>> - acx->window_size = conf->window_size;
>>> + acx->recover_time = cpu_to_le32(conf->recover_time);
>>> + acx->hangover_period = conf->hangover_period;
>>> + acx->dynamic_mode = conf->dynamic_mode;
>>> + acx->early_termination_mode = conf->early_termination_mode;
>>> + acx->max_period = conf->max_period;
>>> + acx->min_period = conf->min_period;
>>> + acx->increase_delta = conf->increase_delta;
>>> + acx->decrease_delta = conf->decrease_delta;
>>> + acx->quiet_time = conf->quiet_time;
>>> + acx->increase_time = conf->increase_time;
>>> + acx->window_size = conf->window_size;
>>
>> I would like to get an ACK from one of the wlcore developers if I should
>> apply this (or not).
>>
> I don't have a strong opinion here.
> However, it looks pretty much redundant to take a random blob (which
> was just fixed by a correct patch) and re-indent it.
> The rest of the file doesn't follow this style, so i don't see a good
> reason to apply it here.
>
> I agree such indentation have some benefit, but it won't help with the
> more common use case (of copy-paste error) of copying the wrong field
> (i.e. D->a = S->b instead of D->a = S->a).
> For these cases the macros suggested by Arend and Johannes will do the
> trick. However i usually dislike such macros, as they tend to break
> some IDE features (e.g. auto completion).
> Maybe we can come up with some nice spatch to catch these cases.
What I dislike about those macros is just that they're not as familiar
to any C programmer as the assignment operator, so they make the code
less readable (even if just a little bit).
As for the IDE thing: I try not to use them, but have been told (by
colleagues) that Eclipse is reasonably smart about macros in C. I use
VIM with the clang_complete plugin and that does do proper completion
with expressions containing macros, but not inside macros based on what
the macro expansion would be, like the one above.
That's why I believe this kind of alignment is at least *an* improvement
even if it doesn't solve all possible problems.
--
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"Question: what do you call your programming methodology?
Answer: Faith based development. You code and then pray that it works."
-- John Spelner
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH net-next RFC 5/5] net-timestamp: tx timestamping default mode flag
From: Richard Cochran @ 2015-01-11 20:32 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: netdev, davem, eric.dumazet, luto
In-Reply-To: <1420824719-28848-6-git-send-email-willemb@google.com>
On Fri, Jan 09, 2015 at 12:31:59PM -0500, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> The number of timestamping points along the transmit path has grown,
> as have the options. Preferred behavior is to request timestamps with
> ID, without data (which enables batching) and for all supported
> timestamp points. Define a short option that enables all these
> defaults.
This "preferred behavior" is subjective, and it depends on the
application. I am sure it reflects your own interest, but for people
doing PTP over UDP or raw, it is a bit misleading.
I would drop this default and just let applications define their own.
Thanks,
Richard
^ permalink raw reply
* [PATCH v4 3/4] can: kvaser_usb: Add support for the Usbcan-II family
From: Ahmed S. Darwish @ 2015-01-11 20:36 UTC (permalink / raw)
To: Olivier Sobrie, Oliver Hartkopp, Wolfgang Grandegger,
Marc Kleine-Budde
Cc: David S. Miller, Paul Gortmaker, Linux-CAN, netdev, LKML
In-Reply-To: <20150111201519.GC8855@linux>
From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
divided into two major families: 'Leaf', and 'UsbcanII'. From an
Operating System perspective, the firmware of both families behave
in a not too drastically different fashion.
This patch adds support for the UsbcanII family of devices to the
current Kvaser Leaf-only driver.
CAN frames sending, receiving, and error handling paths has been
tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
should also work nicely with other products in the same category.
List of new devices supported by this driver update:
- Kvaser USBcan II HS/HS
- Kvaser USBcan II HS/LS
- Kvaser USBcan Rugged ("USBcan Rev B")
- Kvaser Memorator HS/HS
- Kvaser Memorator HS/LS
- Scania VCI2 (if you have the Kvaser logo on top)
Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
drivers/net/can/usb/Kconfig | 8 +-
drivers/net/can/usb/kvaser_usb.c | 612 ++++++++++++++++++++++++++++++---------
2 files changed, 487 insertions(+), 133 deletions(-)
** V4 Changelog:
- Use type-safe C methods instead of cpp macros
- Further clarify the code and comments on error events channel arbitration
- Remove defensive checks against non-existing families
- Re-order methods to remove forward declarations
- Smaller stuff spotted by earlier review (function prefexes, etc.)
** V3 Changelog:
- Fix padding for the usbcan_msg_tx_acknowledge command
- Remove kvaser_usb->max_channels and the MAX_NET_DEVICES macro
- Rename commands to CMD_LEAF_xxx and CMD_USBCAN_xxx
- Apply checkpatch.pl suggestions ('net/' comments, multi-line strings, etc.)
** V2 Changelog:
- Update Kconfig entries
- Use actual number of CAN channels (instead of max) where appropriate
- Rebase over a new set of UsbcanII-independent driver fixes
diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
index a77db919..f6f5500 100644
--- a/drivers/net/can/usb/Kconfig
+++ b/drivers/net/can/usb/Kconfig
@@ -25,7 +25,7 @@ config CAN_KVASER_USB
tristate "Kvaser CAN/USB interface"
---help---
This driver adds support for Kvaser CAN/USB devices like Kvaser
- Leaf Light.
+ Leaf Light and Kvaser USBcan II.
The driver provides support for the following devices:
- Kvaser Leaf Light
@@ -46,6 +46,12 @@ config CAN_KVASER_USB
- Kvaser USBcan R
- Kvaser Leaf Light v2
- Kvaser Mini PCI Express HS
+ - Kvaser USBcan II HS/HS
+ - Kvaser USBcan II HS/LS
+ - Kvaser USBcan Rugged ("USBcan Rev B")
+ - Kvaser Memorator HS/HS
+ - Kvaser Memorator HS/LS
+ - Scania VCI2 (if you have the Kvaser logo on top)
If unsure, say N.
diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 0eb870b..da47d17 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -6,10 +6,12 @@
* Parts of this driver are based on the following:
* - Kvaser linux leaf driver (version 4.78)
* - CAN driver for esd CAN-USB/2
+ * - Kvaser linux usbcanII driver (version 5.3)
*
* Copyright (C) 2002-2006 KVASER AB, Sweden. All rights reserved.
* Copyright (C) 2010 Matthias Fuchs <matthias.fuchs@esd.eu>, esd gmbh
* Copyright (C) 2012 Olivier Sobrie <olivier@sobrie.be>
+ * Copyright (C) 2015 Valeo Corporation
*/
#include <linux/completion.h>
@@ -21,6 +23,15 @@
#include <linux/can/dev.h>
#include <linux/can/error.h>
+/* Kvaser USB CAN dongles are divided into two major families:
+ * - Leaf: Based on Renesas M32C, running firmware labeled as 'filo'
+ * - UsbcanII: Based on Renesas M16C, running firmware labeled as 'helios'
+ */
+enum kvaser_usb_family {
+ KVASER_LEAF,
+ KVASER_USBCAN,
+};
+
#define MAX_TX_URBS 16
#define MAX_RX_URBS 4
#define START_TIMEOUT 1000 /* msecs */
@@ -30,8 +41,9 @@
#define RX_BUFFER_SIZE 3072
#define CAN_USB_CLOCK 8000000
#define MAX_NET_DEVICES 3
+#define MAX_USBCAN_NET_DEVICES 2
-/* Kvaser USB devices */
+/* Leaf USB devices */
#define KVASER_VENDOR_ID 0x0bfd
#define USB_LEAF_DEVEL_PRODUCT_ID 10
#define USB_LEAF_LITE_PRODUCT_ID 11
@@ -56,6 +68,24 @@
#define USB_LEAF_LITE_V2_PRODUCT_ID 288
#define USB_MINI_PCIE_HS_PRODUCT_ID 289
+static inline bool kvaser_is_leaf(const struct usb_device_id *id)
+{
+ return id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
+ id->idProduct <= USB_MINI_PCIE_HS_PRODUCT_ID;
+}
+
+/* USBCANII devices */
+#define USB_USBCAN_REVB_PRODUCT_ID 2
+#define USB_VCI2_PRODUCT_ID 3
+#define USB_USBCAN2_PRODUCT_ID 4
+#define USB_MEMORATOR_PRODUCT_ID 5
+
+static inline bool kvaser_is_usbcan(const struct usb_device_id *id)
+{
+ return id->idProduct >= USB_USBCAN_REVB_PRODUCT_ID &&
+ id->idProduct <= USB_MEMORATOR_PRODUCT_ID;
+}
+
/* USB devices features */
#define KVASER_HAS_SILENT_MODE BIT(0)
#define KVASER_HAS_TXRX_ERRORS BIT(1)
@@ -73,7 +103,7 @@
#define MSG_FLAG_TX_ACK BIT(6)
#define MSG_FLAG_TX_REQUEST BIT(7)
-/* Can states */
+/* Can states (M16C CxSTRH register) */
#define M16C_STATE_BUS_RESET BIT(0)
#define M16C_STATE_BUS_ERROR BIT(4)
#define M16C_STATE_BUS_PASSIVE BIT(5)
@@ -98,7 +128,13 @@
#define CMD_START_CHIP_REPLY 27
#define CMD_STOP_CHIP 28
#define CMD_STOP_CHIP_REPLY 29
-#define CMD_GET_CARD_INFO2 32
+#define CMD_READ_CLOCK 30
+#define CMD_READ_CLOCK_REPLY 31
+
+#define CMD_LEAF_GET_CARD_INFO2 32
+#define CMD_USBCAN_RESET_CLOCK 32
+#define CMD_USBCAN_CLOCK_OVERFLOW_EVENT 33
+
#define CMD_GET_CARD_INFO 34
#define CMD_GET_CARD_INFO_REPLY 35
#define CMD_GET_SOFTWARE_INFO 38
@@ -108,8 +144,9 @@
#define CMD_RESET_ERROR_COUNTER 49
#define CMD_TX_ACKNOWLEDGE 50
#define CMD_CAN_ERROR_EVENT 51
-#define CMD_USB_THROTTLE 77
-#define CMD_LOG_MESSAGE 106
+
+#define CMD_LEAF_USB_THROTTLE 77
+#define CMD_LEAF_LOG_MESSAGE 106
/* error factors */
#define M16C_EF_ACKE BIT(0)
@@ -121,6 +158,14 @@
#define M16C_EF_RCVE BIT(6)
#define M16C_EF_TRE BIT(7)
+/* Only Leaf-based devices can report M16C error factors,
+ * thus define our own error status flags for USBCANII
+ */
+#define USBCAN_ERROR_STATE_NONE 0
+#define USBCAN_ERROR_STATE_TX_ERROR BIT(0)
+#define USBCAN_ERROR_STATE_RX_ERROR BIT(1)
+#define USBCAN_ERROR_STATE_BUSERROR BIT(2)
+
/* bittiming parameters */
#define KVASER_USB_TSEG1_MIN 1
#define KVASER_USB_TSEG1_MAX 16
@@ -137,7 +182,7 @@
#define KVASER_CTRL_MODE_SELFRECEPTION 3
#define KVASER_CTRL_MODE_OFF 4
-/* log message */
+/* Extended CAN identifier flag */
#define KVASER_EXTENDED_FRAME BIT(31)
struct kvaser_msg_simple {
@@ -148,30 +193,55 @@ struct kvaser_msg_simple {
struct kvaser_msg_cardinfo {
u8 tid;
u8 nchannels;
- __le32 serial_number;
- __le32 padding;
+ union {
+ struct {
+ __le32 serial_number;
+ __le32 padding;
+ } __packed leaf0;
+ struct {
+ __le32 serial_number_low;
+ __le32 serial_number_high;
+ } __packed usbcan0;
+ } __packed;
__le32 clock_resolution;
__le32 mfgdate;
u8 ean[8];
u8 hw_revision;
- u8 usb_hs_mode;
- __le16 padding2;
+ union {
+ struct {
+ u8 usb_hs_mode;
+ } __packed leaf1;
+ struct {
+ u8 padding;
+ } __packed usbcan1;
+ } __packed;
+ __le16 padding;
} __packed;
struct kvaser_msg_cardinfo2 {
u8 tid;
- u8 channel;
+ u8 reserved;
u8 pcb_id[24];
__le32 oem_unlock_code;
} __packed;
-struct kvaser_msg_softinfo {
+struct leaf_msg_softinfo {
u8 tid;
- u8 channel;
+ u8 padding0;
__le32 sw_options;
__le32 fw_version;
__le16 max_outstanding_tx;
- __le16 padding[9];
+ __le16 padding1[9];
+} __packed;
+
+struct usbcan_msg_softinfo {
+ u8 tid;
+ u8 fw_name[5];
+ __le16 max_outstanding_tx;
+ u8 padding[6];
+ __le32 fw_version;
+ __le16 checksum;
+ __le16 sw_options;
} __packed;
struct kvaser_msg_busparams {
@@ -188,36 +258,86 @@ struct kvaser_msg_tx_can {
u8 channel;
u8 tid;
u8 msg[14];
- u8 padding;
- u8 flags;
+ union {
+ struct {
+ u8 padding;
+ u8 flags;
+ } __packed leaf;
+ struct {
+ u8 flags;
+ u8 padding;
+ } __packed usbcan;
+ } __packed;
+} __packed;
+
+struct kvaser_msg_rx_can_header {
+ u8 channel;
+ u8 flag;
} __packed;
-struct kvaser_msg_rx_can {
+struct leaf_msg_rx_can {
u8 channel;
u8 flag;
+
__le16 time[3];
u8 msg[14];
} __packed;
-struct kvaser_msg_chip_state_event {
+struct usbcan_msg_rx_can {
+ u8 channel;
+ u8 flag;
+
+ u8 msg[14];
+ __le16 time;
+} __packed;
+
+struct leaf_msg_chip_state_event {
u8 tid;
u8 channel;
+
__le16 time[3];
u8 tx_errors_count;
u8 rx_errors_count;
+
u8 status;
u8 padding[3];
} __packed;
-struct kvaser_msg_tx_acknowledge {
+struct usbcan_msg_chip_state_event {
+ u8 tid;
+ u8 channel;
+
+ u8 tx_errors_count;
+ u8 rx_errors_count;
+ __le16 time;
+
+ u8 status;
+ u8 padding[3];
+} __packed;
+
+struct kvaser_msg_tx_acknowledge_header {
+ u8 channel;
+ u8 tid;
+};
+
+struct leaf_msg_tx_acknowledge {
u8 channel;
u8 tid;
+
__le16 time[3];
u8 flags;
u8 time_offset;
} __packed;
-struct kvaser_msg_error_event {
+struct usbcan_msg_tx_acknowledge {
+ u8 channel;
+ u8 tid;
+
+ __le16 time;
+ __le16 padding;
+} __packed;
+
+struct leaf_msg_error_event {
u8 tid;
u8 flags;
__le16 time[3];
@@ -229,6 +349,18 @@ struct kvaser_msg_error_event {
u8 error_factor;
} __packed;
+struct usbcan_msg_error_event {
+ u8 tid;
+ u8 padding;
+ u8 tx_errors_count_ch0;
+ u8 rx_errors_count_ch0;
+ u8 tx_errors_count_ch1;
+ u8 rx_errors_count_ch1;
+ u8 status_ch0;
+ u8 status_ch1;
+ __le16 time;
+} __packed;
+
struct kvaser_msg_ctrl_mode {
u8 tid;
u8 channel;
@@ -243,7 +375,7 @@ struct kvaser_msg_flush_queue {
u8 padding[3];
} __packed;
-struct kvaser_msg_log_message {
+struct leaf_msg_log_message {
u8 channel;
u8 flags;
__le16 time[3];
@@ -260,19 +392,57 @@ struct kvaser_msg {
struct kvaser_msg_simple simple;
struct kvaser_msg_cardinfo cardinfo;
struct kvaser_msg_cardinfo2 cardinfo2;
- struct kvaser_msg_softinfo softinfo;
struct kvaser_msg_busparams busparams;
+
+ struct kvaser_msg_rx_can_header rx_can_header;
+ struct kvaser_msg_tx_acknowledge_header tx_acknowledge_header;
+
+ union {
+ struct leaf_msg_softinfo softinfo;
+ struct leaf_msg_rx_can rx_can;
+ struct leaf_msg_chip_state_event chip_state_event;
+ struct leaf_msg_tx_acknowledge tx_acknowledge;
+ struct leaf_msg_error_event error_event;
+ struct leaf_msg_log_message log_message;
+ } __packed leaf;
+
+ union {
+ struct usbcan_msg_softinfo softinfo;
+ struct usbcan_msg_rx_can rx_can;
+ struct usbcan_msg_chip_state_event chip_state_event;
+ struct usbcan_msg_tx_acknowledge tx_acknowledge;
+ struct usbcan_msg_error_event error_event;
+ } __packed usbcan;
+
struct kvaser_msg_tx_can tx_can;
- struct kvaser_msg_rx_can rx_can;
- struct kvaser_msg_chip_state_event chip_state_event;
- struct kvaser_msg_tx_acknowledge tx_acknowledge;
- struct kvaser_msg_error_event error_event;
struct kvaser_msg_ctrl_mode ctrl_mode;
struct kvaser_msg_flush_queue flush_queue;
- struct kvaser_msg_log_message log_message;
} u;
} __packed;
+/* Summary of a kvaser error event, for a unified Leaf/Usbcan error
+ * handling. Some discrepancies between the two families exist:
+ *
+ * - USBCAN firmware does not report M16C "error factors"
+ * - USBCAN controllers has difficulties reporting if the raised error
+ * event is for ch0 or ch1. They leave such arbitration to the OS
+ * driver by letting it compare error counters with previous values
+ * and decide the error event's channel. Thus for USBCAN, the channel
+ * field is only advisory.
+ */
+struct kvaser_error_summary {
+ u8 channel, status, txerr, rxerr;
+ union {
+ struct {
+ u8 error_factor;
+ } leaf;
+ struct {
+ u8 other_ch_status;
+ u8 error_state;
+ } usbcan;
+ };
+};
+
struct kvaser_usb_tx_urb_context {
struct kvaser_usb_net_priv *priv;
u32 echo_index;
@@ -288,6 +458,7 @@ struct kvaser_usb {
u32 fw_version;
unsigned int nchannels;
+ enum kvaser_usb_family family;
bool rxinitdone;
void *rxbuf[MAX_RX_URBS];
@@ -311,6 +482,7 @@ struct kvaser_usb_net_priv {
};
static const struct usb_device_id kvaser_usb_table[] = {
+ /* Leaf family IDs */
{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_DEVEL_PRODUCT_ID) },
{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_PRODUCT_ID) },
{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_PRODUCT_ID),
@@ -360,6 +532,17 @@ static const struct usb_device_id kvaser_usb_table[] = {
.driver_info = KVASER_HAS_TXRX_ERRORS },
{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_V2_PRODUCT_ID) },
{ USB_DEVICE(KVASER_VENDOR_ID, USB_MINI_PCIE_HS_PRODUCT_ID) },
+
+ /* USBCANII family IDs */
+ { USB_DEVICE(KVASER_VENDOR_ID, USB_USBCAN2_PRODUCT_ID),
+ .driver_info = KVASER_HAS_TXRX_ERRORS },
+ { USB_DEVICE(KVASER_VENDOR_ID, USB_USBCAN_REVB_PRODUCT_ID),
+ .driver_info = KVASER_HAS_TXRX_ERRORS },
+ { USB_DEVICE(KVASER_VENDOR_ID, USB_MEMORATOR_PRODUCT_ID),
+ .driver_info = KVASER_HAS_TXRX_ERRORS },
+ { USB_DEVICE(KVASER_VENDOR_ID, USB_VCI2_PRODUCT_ID),
+ .driver_info = KVASER_HAS_TXRX_ERRORS },
+
{ }
};
MODULE_DEVICE_TABLE(usb, kvaser_usb_table);
@@ -463,7 +646,14 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
if (err)
return err;
- dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);
+ switch (dev->family) {
+ case KVASER_LEAF:
+ dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);
+ break;
+ case KVASER_USBCAN:
+ dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);
+ break;
+ }
return 0;
}
@@ -482,7 +672,9 @@ static int kvaser_usb_get_card_info(struct kvaser_usb *dev)
return err;
dev->nchannels = msg.u.cardinfo.nchannels;
- if (dev->nchannels > MAX_NET_DEVICES)
+ if ((dev->nchannels > MAX_NET_DEVICES) ||
+ (dev->family == KVASER_USBCAN &&
+ dev->nchannels > MAX_USBCAN_NET_DEVICES))
return -EINVAL;
return 0;
@@ -496,8 +688,10 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
struct kvaser_usb_net_priv *priv;
struct sk_buff *skb;
struct can_frame *cf;
- u8 channel = msg->u.tx_acknowledge.channel;
- u8 tid = msg->u.tx_acknowledge.tid;
+ u8 channel, tid;
+
+ channel = msg->u.tx_acknowledge_header.channel;
+ tid = msg->u.tx_acknowledge_header.tid;
if (channel >= dev->nchannels) {
dev_err(dev->udev->dev.parent,
@@ -616,53 +810,24 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
}
static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
- const struct kvaser_msg *msg)
+ struct kvaser_error_summary *es)
{
struct can_frame *cf;
struct sk_buff *skb;
struct net_device_stats *stats;
struct kvaser_usb_net_priv *priv;
unsigned int new_state;
- u8 channel, status, txerr, rxerr, error_factor;
-
- switch (msg->id) {
- case CMD_CAN_ERROR_EVENT:
- channel = msg->u.error_event.channel;
- status = msg->u.error_event.status;
- txerr = msg->u.error_event.tx_errors_count;
- rxerr = msg->u.error_event.rx_errors_count;
- error_factor = msg->u.error_event.error_factor;
- break;
- case CMD_LOG_MESSAGE:
- channel = msg->u.log_message.channel;
- status = msg->u.log_message.data[0];
- txerr = msg->u.log_message.data[2];
- rxerr = msg->u.log_message.data[3];
- error_factor = msg->u.log_message.data[1];
- break;
- case CMD_CHIP_STATE_EVENT:
- channel = msg->u.chip_state_event.channel;
- status = msg->u.chip_state_event.status;
- txerr = msg->u.chip_state_event.tx_errors_count;
- rxerr = msg->u.chip_state_event.rx_errors_count;
- error_factor = 0;
- break;
- default:
- dev_err(dev->udev->dev.parent, "Invalid msg id (%d)\n",
- msg->id);
- return;
- }
- if (channel >= dev->nchannels) {
+ if (es->channel >= dev->nchannels) {
dev_err(dev->udev->dev.parent,
- "Invalid channel number (%d)\n", channel);
+ "Invalid channel number (%d)\n", es->channel);
return;
}
- priv = dev->nets[channel];
+ priv = dev->nets[es->channel];
stats = &priv->netdev->stats;
- if (status & M16C_STATE_BUS_RESET) {
+ if (es->status & M16C_STATE_BUS_RESET) {
kvaser_usb_unlink_tx_urbs(priv);
return;
}
@@ -675,9 +840,9 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
new_state = priv->can.state;
- netdev_dbg(priv->netdev, "Error status: 0x%02x\n", status);
+ netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status);
- if (status & M16C_STATE_BUS_OFF) {
+ if (es->status & M16C_STATE_BUS_OFF) {
cf->can_id |= CAN_ERR_BUSOFF;
priv->can.can_stats.bus_off++;
@@ -687,12 +852,12 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
netif_carrier_off(priv->netdev);
new_state = CAN_STATE_BUS_OFF;
- } else if (status & M16C_STATE_BUS_PASSIVE) {
+ } else if (es->status & M16C_STATE_BUS_PASSIVE) {
if (priv->can.state != CAN_STATE_ERROR_PASSIVE) {
cf->can_id |= CAN_ERR_CRTL;
- if (txerr || rxerr)
- cf->data[1] = (txerr > rxerr)
+ if (es->txerr || es->rxerr)
+ cf->data[1] = (es->txerr > es->rxerr)
? CAN_ERR_CRTL_TX_PASSIVE
: CAN_ERR_CRTL_RX_PASSIVE;
else
@@ -703,13 +868,11 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
}
new_state = CAN_STATE_ERROR_PASSIVE;
- }
-
- if (status == M16C_STATE_BUS_ERROR) {
+ } else if (es->status & M16C_STATE_BUS_ERROR) {
if ((priv->can.state < CAN_STATE_ERROR_WARNING) &&
- ((txerr >= 96) || (rxerr >= 96))) {
+ ((es->txerr >= 96) || (es->rxerr >= 96))) {
cf->can_id |= CAN_ERR_CRTL;
- cf->data[1] = (txerr > rxerr)
+ cf->data[1] = (es->txerr > es->rxerr)
? CAN_ERR_CRTL_TX_WARNING
: CAN_ERR_CRTL_RX_WARNING;
@@ -723,7 +886,7 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
}
}
- if (!status) {
+ if (!es->status) {
cf->can_id |= CAN_ERR_PROT;
cf->data[2] = CAN_ERR_PROT_ACTIVE;
@@ -739,34 +902,48 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
priv->can.can_stats.restarts++;
}
- if (error_factor) {
- priv->can.can_stats.bus_error++;
- stats->rx_errors++;
-
- cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
-
- if (error_factor & M16C_EF_ACKE)
- cf->data[3] |= (CAN_ERR_PROT_LOC_ACK);
- if (error_factor & M16C_EF_CRCE)
- cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
- CAN_ERR_PROT_LOC_CRC_DEL);
- if (error_factor & M16C_EF_FORME)
- cf->data[2] |= CAN_ERR_PROT_FORM;
- if (error_factor & M16C_EF_STFE)
- cf->data[2] |= CAN_ERR_PROT_STUFF;
- if (error_factor & M16C_EF_BITE0)
- cf->data[2] |= CAN_ERR_PROT_BIT0;
- if (error_factor & M16C_EF_BITE1)
- cf->data[2] |= CAN_ERR_PROT_BIT1;
- if (error_factor & M16C_EF_TRE)
- cf->data[2] |= CAN_ERR_PROT_TX;
+ switch (dev->family) {
+ case KVASER_LEAF:
+ if (es->leaf.error_factor) {
+ priv->can.can_stats.bus_error++;
+ stats->rx_errors++;
+
+ cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
+
+ if (es->leaf.error_factor & M16C_EF_ACKE)
+ cf->data[3] |= (CAN_ERR_PROT_LOC_ACK);
+ if (es->leaf.error_factor & M16C_EF_CRCE)
+ cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
+ CAN_ERR_PROT_LOC_CRC_DEL);
+ if (es->leaf.error_factor & M16C_EF_FORME)
+ cf->data[2] |= CAN_ERR_PROT_FORM;
+ if (es->leaf.error_factor & M16C_EF_STFE)
+ cf->data[2] |= CAN_ERR_PROT_STUFF;
+ if (es->leaf.error_factor & M16C_EF_BITE0)
+ cf->data[2] |= CAN_ERR_PROT_BIT0;
+ if (es->leaf.error_factor & M16C_EF_BITE1)
+ cf->data[2] |= CAN_ERR_PROT_BIT1;
+ if (es->leaf.error_factor & M16C_EF_TRE)
+ cf->data[2] |= CAN_ERR_PROT_TX;
+ }
+ break;
+ case KVASER_USBCAN:
+ if (es->usbcan.error_state & USBCAN_ERROR_STATE_TX_ERROR)
+ stats->tx_errors++;
+ if (es->usbcan.error_state & USBCAN_ERROR_STATE_RX_ERROR)
+ stats->rx_errors++;
+ if (es->usbcan.error_state & USBCAN_ERROR_STATE_BUSERROR) {
+ priv->can.can_stats.bus_error++;
+ cf->can_id |= CAN_ERR_BUSERROR;
+ }
+ break;
}
- cf->data[6] = txerr;
- cf->data[7] = rxerr;
+ cf->data[6] = es->txerr;
+ cf->data[7] = es->rxerr;
- priv->bec.txerr = txerr;
- priv->bec.rxerr = rxerr;
+ priv->bec.txerr = es->txerr;
+ priv->bec.rxerr = es->rxerr;
priv->can.state = new_state;
@@ -775,6 +952,124 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
netif_rx(skb);
}
+/* For USBCAN, report error to userspace iff the channels's errors counter
+ * has increased, or we're the only channel seeing a bus error state.
+ */
+static void kvaser_usbcan_conditionally_rx_error(const struct kvaser_usb *dev,
+ struct kvaser_error_summary *es)
+{
+ struct kvaser_usb_net_priv *priv;
+ int channel;
+ bool report_error;
+
+ channel = es->channel;
+ if (channel >= dev->nchannels) {
+ dev_err(dev->udev->dev.parent,
+ "Invalid channel number (%d)\n", channel);
+ return;
+ }
+
+ priv = dev->nets[channel];
+ report_error = false;
+
+ if (es->txerr > priv->bec.txerr) {
+ es->usbcan.error_state |= USBCAN_ERROR_STATE_TX_ERROR;
+ report_error = true;
+ }
+ if (es->rxerr > priv->bec.rxerr) {
+ es->usbcan.error_state |= USBCAN_ERROR_STATE_RX_ERROR;
+ report_error = true;
+ }
+ if ((es->status & M16C_STATE_BUS_ERROR) &&
+ !(es->usbcan.other_ch_status & M16C_STATE_BUS_ERROR)) {
+ es->usbcan.error_state |= USBCAN_ERROR_STATE_BUSERROR;
+ report_error = true;
+ }
+
+ if (report_error)
+ kvaser_usb_rx_error(dev, es);
+}
+
+static void kvaser_usbcan_rx_error(const struct kvaser_usb *dev,
+ const struct kvaser_msg *msg)
+{
+ struct kvaser_error_summary es = { };
+
+ switch (msg->id) {
+ /* Sometimes errors are sent as unsolicited chip state events */
+ case CMD_CHIP_STATE_EVENT:
+ es.channel = msg->u.usbcan.chip_state_event.channel;
+ es.status = msg->u.usbcan.chip_state_event.status;
+ es.txerr = msg->u.usbcan.chip_state_event.tx_errors_count;
+ es.rxerr = msg->u.usbcan.chip_state_event.rx_errors_count;
+ kvaser_usbcan_conditionally_rx_error(dev, &es);
+ break;
+
+ case CMD_CAN_ERROR_EVENT:
+ es.channel = 0;
+ es.status = msg->u.usbcan.error_event.status_ch0;
+ es.txerr = msg->u.usbcan.error_event.tx_errors_count_ch0;
+ es.rxerr = msg->u.usbcan.error_event.rx_errors_count_ch0;
+ es.usbcan.other_ch_status =
+ msg->u.usbcan.error_event.status_ch1;
+ kvaser_usbcan_conditionally_rx_error(dev, &es);
+
+ /* The USBCAN firmware does not support more than 2 channels.
+ * Now that ch0 was checked, check if ch1 has any errors.
+ */
+ if (dev->nchannels == MAX_USBCAN_NET_DEVICES) {
+ es.channel = 1;
+ es.status = msg->u.usbcan.error_event.status_ch1;
+ es.txerr = msg->u.usbcan.error_event.tx_errors_count_ch1;
+ es.rxerr = msg->u.usbcan.error_event.rx_errors_count_ch1;
+ es.usbcan.other_ch_status =
+ msg->u.usbcan.error_event.status_ch0;
+ kvaser_usbcan_conditionally_rx_error(dev, &es);
+ }
+ break;
+
+ default:
+ dev_err(dev->udev->dev.parent, "Invalid msg id (%d)\n",
+ msg->id);
+ }
+}
+
+static void kvaser_leaf_rx_error(const struct kvaser_usb *dev,
+ const struct kvaser_msg *msg)
+{
+ struct kvaser_error_summary es = { };
+
+ switch (msg->id) {
+ case CMD_CAN_ERROR_EVENT:
+ es.channel = msg->u.leaf.error_event.channel;
+ es.status = msg->u.leaf.error_event.status;
+ es.txerr = msg->u.leaf.error_event.tx_errors_count;
+ es.rxerr = msg->u.leaf.error_event.rx_errors_count;
+ es.leaf.error_factor = msg->u.leaf.error_event.error_factor;
+ break;
+ case CMD_LEAF_LOG_MESSAGE:
+ es.channel = msg->u.leaf.log_message.channel;
+ es.status = msg->u.leaf.log_message.data[0];
+ es.txerr = msg->u.leaf.log_message.data[2];
+ es.rxerr = msg->u.leaf.log_message.data[3];
+ es.leaf.error_factor = msg->u.leaf.log_message.data[1];
+ break;
+ case CMD_CHIP_STATE_EVENT:
+ es.channel = msg->u.leaf.chip_state_event.channel;
+ es.status = msg->u.leaf.chip_state_event.status;
+ es.txerr = msg->u.leaf.chip_state_event.tx_errors_count;
+ es.rxerr = msg->u.leaf.chip_state_event.rx_errors_count;
+ es.leaf.error_factor = 0;
+ break;
+ default:
+ dev_err(dev->udev->dev.parent, "Invalid msg id (%d)\n",
+ msg->id);
+ return;
+ }
+
+ kvaser_usb_rx_error(dev, &es);
+}
+
static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
const struct kvaser_msg *msg)
{
@@ -782,16 +1077,16 @@ static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
struct sk_buff *skb;
struct net_device_stats *stats = &priv->netdev->stats;
- if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
+ if (msg->u.rx_can_header.flag & (MSG_FLAG_ERROR_FRAME |
MSG_FLAG_NERR)) {
netdev_err(priv->netdev, "Unknow error (flags: 0x%02x)\n",
- msg->u.rx_can.flag);
+ msg->u.rx_can_header.flag);
stats->rx_errors++;
return;
}
- if (msg->u.rx_can.flag & MSG_FLAG_OVERRUN) {
+ if (msg->u.rx_can_header.flag & MSG_FLAG_OVERRUN) {
stats->rx_over_errors++;
stats->rx_errors++;
@@ -817,7 +1112,8 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
struct can_frame *cf;
struct sk_buff *skb;
struct net_device_stats *stats;
- u8 channel = msg->u.rx_can.channel;
+ u8 channel = msg->u.rx_can_header.channel;
+ const u8 *rx_msg;
if (channel >= dev->nchannels) {
dev_err(dev->udev->dev.parent,
@@ -828,19 +1124,32 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
priv = dev->nets[channel];
stats = &priv->netdev->stats;
- if ((msg->u.rx_can.flag & MSG_FLAG_ERROR_FRAME) &&
- (msg->id == CMD_LOG_MESSAGE)) {
- kvaser_usb_rx_error(dev, msg);
+ if ((msg->u.rx_can_header.flag & MSG_FLAG_ERROR_FRAME) &&
+ (dev->family == KVASER_LEAF && msg->id == CMD_LEAF_LOG_MESSAGE)) {
+ kvaser_leaf_rx_error(dev, msg);
return;
- } else if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
- MSG_FLAG_NERR |
- MSG_FLAG_OVERRUN)) {
+ } else if (msg->u.rx_can_header.flag & (MSG_FLAG_ERROR_FRAME |
+ MSG_FLAG_NERR |
+ MSG_FLAG_OVERRUN)) {
kvaser_usb_rx_can_err(priv, msg);
return;
- } else if (msg->u.rx_can.flag & ~MSG_FLAG_REMOTE_FRAME) {
+ } else if (msg->u.rx_can_header.flag & ~MSG_FLAG_REMOTE_FRAME) {
netdev_warn(priv->netdev,
"Unhandled frame (flags: 0x%02x)",
- msg->u.rx_can.flag);
+ msg->u.rx_can_header.flag);
+ return;
+ }
+
+ switch (dev->family) {
+ case KVASER_LEAF:
+ rx_msg = msg->u.leaf.rx_can.msg;
+ break;
+ case KVASER_USBCAN:
+ rx_msg = msg->u.usbcan.rx_can.msg;
+ break;
+ default:
+ dev_err(dev->udev->dev.parent,
+ "Invalid device family (%d)\n", dev->family);
return;
}
@@ -850,38 +1159,37 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
return;
}
- if (msg->id == CMD_LOG_MESSAGE) {
- cf->can_id = le32_to_cpu(msg->u.log_message.id);
+ if (dev->family == KVASER_LEAF && msg->id == CMD_LEAF_LOG_MESSAGE) {
+ cf->can_id = le32_to_cpu(msg->u.leaf.log_message.id);
if (cf->can_id & KVASER_EXTENDED_FRAME)
cf->can_id &= CAN_EFF_MASK | CAN_EFF_FLAG;
else
cf->can_id &= CAN_SFF_MASK;
- cf->can_dlc = get_can_dlc(msg->u.log_message.dlc);
+ cf->can_dlc = get_can_dlc(msg->u.leaf.log_message.dlc);
- if (msg->u.log_message.flags & MSG_FLAG_REMOTE_FRAME)
+ if (msg->u.leaf.log_message.flags & MSG_FLAG_REMOTE_FRAME)
cf->can_id |= CAN_RTR_FLAG;
else
- memcpy(cf->data, &msg->u.log_message.data,
+ memcpy(cf->data, &msg->u.leaf.log_message.data,
cf->can_dlc);
} else {
- cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) |
- (msg->u.rx_can.msg[1] & 0x3f);
+ cf->can_id = ((rx_msg[0] & 0x1f) << 6) | (rx_msg[1] & 0x3f);
if (msg->id == CMD_RX_EXT_MESSAGE) {
cf->can_id <<= 18;
- cf->can_id |= ((msg->u.rx_can.msg[2] & 0x0f) << 14) |
- ((msg->u.rx_can.msg[3] & 0xff) << 6) |
- (msg->u.rx_can.msg[4] & 0x3f);
+ cf->can_id |= ((rx_msg[2] & 0x0f) << 14) |
+ ((rx_msg[3] & 0xff) << 6) |
+ (rx_msg[4] & 0x3f);
cf->can_id |= CAN_EFF_FLAG;
}
- cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]);
+ cf->can_dlc = get_can_dlc(rx_msg[5]);
- if (msg->u.rx_can.flag & MSG_FLAG_REMOTE_FRAME)
+ if (msg->u.rx_can_header.flag & MSG_FLAG_REMOTE_FRAME)
cf->can_id |= CAN_RTR_FLAG;
else
- memcpy(cf->data, &msg->u.rx_can.msg[6],
+ memcpy(cf->data, &rx_msg[6],
cf->can_dlc);
}
@@ -944,21 +1252,35 @@ static void kvaser_usb_handle_message(const struct kvaser_usb *dev,
case CMD_RX_STD_MESSAGE:
case CMD_RX_EXT_MESSAGE:
- case CMD_LOG_MESSAGE:
+ kvaser_usb_rx_can_msg(dev, msg);
+ break;
+
+ case CMD_LEAF_LOG_MESSAGE:
+ if (dev->family != KVASER_LEAF)
+ goto warn;
kvaser_usb_rx_can_msg(dev, msg);
break;
case CMD_CHIP_STATE_EVENT:
case CMD_CAN_ERROR_EVENT:
- kvaser_usb_rx_error(dev, msg);
+ if (dev->family == KVASER_LEAF)
+ kvaser_leaf_rx_error(dev, msg);
+ else
+ kvaser_usbcan_rx_error(dev, msg);
break;
case CMD_TX_ACKNOWLEDGE:
kvaser_usb_tx_acknowledge(dev, msg);
break;
+ /* Ignored messages */
+ case CMD_USBCAN_CLOCK_OVERFLOW_EVENT:
+ if (dev->family != KVASER_USBCAN)
+ goto warn;
+ break;
+
default:
- dev_warn(dev->udev->dev.parent,
+warn: dev_warn(dev->udev->dev.parent,
"Unhandled message (%d)\n", msg->id);
break;
}
@@ -1178,7 +1500,7 @@ static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev)
dev->rxbuf[i],
dev->rxbuf_dma[i]);
- for (i = 0; i < MAX_NET_DEVICES; i++) {
+ for (i = 0; i < dev->nchannels; i++) {
struct kvaser_usb_net_priv *priv = dev->nets[i];
if (priv)
@@ -1286,6 +1608,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
struct kvaser_msg *msg;
int i, err;
int ret = NETDEV_TX_OK;
+ uint8_t *msg_tx_can_flags;
if (can_dropped_invalid_skb(netdev, skb))
return NETDEV_TX_OK;
@@ -1307,9 +1630,23 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
msg = buf;
msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_tx_can);
- msg->u.tx_can.flags = 0;
msg->u.tx_can.channel = priv->channel;
+ switch (dev->family) {
+ case KVASER_LEAF:
+ msg_tx_can_flags = &msg->u.tx_can.leaf.flags;
+ break;
+ case KVASER_USBCAN:
+ msg_tx_can_flags = &msg->u.tx_can.usbcan.flags;
+ break;
+ default:
+ dev_err(dev->udev->dev.parent,
+ "Invalid device family (%d)\n", dev->family);
+ goto releasebuf;
+ }
+
+ *msg_tx_can_flags = 0;
+
if (cf->can_id & CAN_EFF_FLAG) {
msg->id = CMD_TX_EXT_MESSAGE;
msg->u.tx_can.msg[0] = (cf->can_id >> 24) & 0x1f;
@@ -1327,7 +1664,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc);
if (cf->can_id & CAN_RTR_FLAG)
- msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;
+ *msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) {
if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
@@ -1596,6 +1933,17 @@ static int kvaser_usb_probe(struct usb_interface *intf,
if (!dev)
return -ENOMEM;
+ if (kvaser_is_leaf(id)) {
+ dev->family = KVASER_LEAF;
+ } else if (kvaser_is_usbcan(id)) {
+ dev->family = KVASER_USBCAN;
+ } else {
+ dev_err(&intf->dev,
+ "Product ID (%d) does not belong to any known Kvaser USB family",
+ id->idProduct);
+ return -ENODEV;
+ }
+
err = kvaser_usb_get_endpoints(intf, &dev->bulk_in, &dev->bulk_out);
if (err) {
dev_err(&intf->dev, "Cannot get usb endpoint(s)");
--
1.9.1
^ permalink raw reply related
* [PATCH v4 4/4] can: kvaser_usb: Retry first bulk transfer on -ETIMEDOUT
From: Ahmed S. Darwish @ 2015-01-11 20:45 UTC (permalink / raw)
To: Olivier Sobrie, Oliver Hartkopp, Wolfgang Grandegger,
Marc Kleine-Budde
Cc: Greg Kroah-Hartman, Linux-USB, Linux-CAN, netdev, LKML
In-Reply-To: <20150111203612.GA8999@linux>
From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
(This is a draft patch, I'm not sure if this fixes the USB
bug or only its psymptom. Feedback from the linux-usb folks
is really appreciated.)
When plugging the Kvaser USB/CAN dongle the first time, everything
works as expected and all of the transfers from and to the USB
device succeeds.
Meanwhile, after unplugging the device and plugging it again, the
first bulk transfer _always_ returns an -ETIMEDOUT. The following
behaviour was observied:
- Setting higher timeout values for the first bulk transfer never
solved the issue.
- Unloading, then loading, our kvaser_usb module in question
__always__ solved the issue.
- Checking first bulk transfer status, and retry the transfer
again in case of an -ETIMEDOUT also __always__ solved the issue.
This is what the patch below does.
- In the testing done so far, this issue appears only on laptops
but never on PCs (possibly power related?)
Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
drivers/net/can/usb/kvaser_usb.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index da47d17..5925637 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -1927,7 +1927,7 @@ static int kvaser_usb_probe(struct usb_interface *intf,
{
struct kvaser_usb *dev;
int err = -ENOMEM;
- int i;
+ int i, retry = 3;
dev = devm_kzalloc(&intf->dev, sizeof(*dev), GFP_KERNEL);
if (!dev)
@@ -1956,7 +1956,16 @@ static int kvaser_usb_probe(struct usb_interface *intf,
usb_set_intfdata(intf, dev);
- err = kvaser_usb_get_software_info(dev);
+ /* On some x86 laptops, plugging a USBCAN device again after
+ * an unplug makes the firmware always ignore the very first
+ * command. For such a case, provide some room for retries
+ * instead of completly exiting the driver.
+ */
+ while (retry--) {
+ err = kvaser_usb_get_software_info(dev);
+ if (err != -ETIMEDOUT)
+ break;
+ }
if (err) {
dev_err(&intf->dev,
"Cannot get software infos, error %d\n", err);
--
1.9.1
^ permalink raw reply related
* [PATCH v4 01/04] can: kvaser_usb: Don't dereference skb after a netif_rx()
From: Ahmed S. Darwish @ 2015-01-11 20:49 UTC (permalink / raw)
To: Olivier Sobrie, Oliver Hartkopp, Wolfgang Grandegger,
Marc Kleine-Budde
Cc: David S. Miller, Paul Gortmaker, Linux-CAN, netdev, LKML
In-Reply-To: <20150111201116.GB8855@linux>
From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
We should not touch the packet after a netif_rx: it might
get freed behind our back.
Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
drivers/net/can/usb/kvaser_usb.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
(Resend, fix the garbled subject line. Sorry)
diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index cc7bfc0..c32cd61 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -520,10 +520,10 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
skb = alloc_can_err_skb(priv->netdev, &cf);
if (skb) {
cf->can_id |= CAN_ERR_RESTARTED;
- netif_rx(skb);
stats->rx_packets++;
stats->rx_bytes += cf->can_dlc;
+ netif_rx(skb);
} else {
netdev_err(priv->netdev,
"No memory left for err_skb\n");
@@ -770,10 +770,9 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
priv->can.state = new_state;
- netif_rx(skb);
-
stats->rx_packets++;
stats->rx_bytes += cf->can_dlc;
+ netif_rx(skb);
}
static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
@@ -805,10 +804,9 @@ static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
stats->rx_over_errors++;
stats->rx_errors++;
- netif_rx(skb);
-
stats->rx_packets++;
stats->rx_bytes += cf->can_dlc;
+ netif_rx(skb);
}
}
@@ -887,10 +885,9 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
cf->can_dlc);
}
- netif_rx(skb);
-
stats->rx_packets++;
stats->rx_bytes += cf->can_dlc;
+ netif_rx(skb);
}
static void kvaser_usb_start_chip_reply(const struct kvaser_usb *dev,
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v4 4/4] can: kvaser_usb: Retry first bulk transfer on -ETIMEDOUT
From: Marc Kleine-Budde @ 2015-01-11 20:51 UTC (permalink / raw)
To: Ahmed S. Darwish, Olivier Sobrie, Oliver Hartkopp,
Wolfgang Grandegger
Cc: Greg Kroah-Hartman, Linux-USB, Linux-CAN, netdev, LKML
In-Reply-To: <20150111204508.GB8999@linux>
[-- Attachment #1: Type: text/plain, Size: 1616 bytes --]
On 01/11/2015 09:45 PM, Ahmed S. Darwish wrote:
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>
> (This is a draft patch, I'm not sure if this fixes the USB
> bug or only its psymptom. Feedback from the linux-usb folks
> is really appreciated.)
>
> When plugging the Kvaser USB/CAN dongle the first time, everything
> works as expected and all of the transfers from and to the USB
> device succeeds.
>
> Meanwhile, after unplugging the device and plugging it again, the
> first bulk transfer _always_ returns an -ETIMEDOUT. The following
> behaviour was observied:
>
> - Setting higher timeout values for the first bulk transfer never
> solved the issue.
>
> - Unloading, then loading, our kvaser_usb module in question
> __always__ solved the issue.
>
> - Checking first bulk transfer status, and retry the transfer
> again in case of an -ETIMEDOUT also __always__ solved the issue.
> This is what the patch below does.
>
> - In the testing done so far, this issue appears only on laptops
> but never on PCs (possibly power related?)
>
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
Does this patch apply apply between 3 and 4? If not, please re-arrange
the series. As this is a bug fix, patches 1, 2 and 4 will go via
net/master, 3 will go via net-next/master.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [RFC] Make predictable/persistent network interface names more handy
From: Richard Weinberger @ 2015-01-11 20:52 UTC (permalink / raw)
To: davem
Cc: coreteam, netfilter-devel, linux-kernel, netdev, bhutchings,
john.fastabend, herbert, vyasevic, jiri, vfalico, therbert,
edumazet, yoshfuji, jmorris, kuznet, kadlec, kaber, pablo, kay,
stephen
Since the Linux distribution of my choice makes use of
predictable network interface names[0] my USB gadgets
are no longer usb0 but enp0s29u1u2. Same for all other network devices.
While I can fully understand that this new naming scheme makes sense
for a lot of people and makes their work easier it does not really work for me.
My brain is not able to remember that my Beaglebone's USB-Ethernet
is now enp0s29u1u2. Even after looking at the output of ifconfig
I have to copy&paste the interface name.
Instead of just disabling the feature I thought about
a generic solution which satisfies both needs.
For block devices we also have predictable device names,
udev creates a symlink to the kernel device.
This works very good and reliable.
My idea is to use the network device alias as symlink.
Such that we can have both the easy to use kernel name and
the predictable/persistent name from udev.
systemd/udev could store the original kernel interface name
as alias after renaming the interface.
Existing users would not notice but one can still use the kernel name.
So I can use tcpdump -i usb0 _and_ tcpdump -i enp0s29u1u2.
This patch series implements my idea.
I'd love to get some feedback!
Patch 1/3 exposes the interfaces alias for general userspace usage, i.e. that ifconfig <alias> works.
Of course you can only use the first 15 chars of an alias.
In-kernel users of interface names need also an update, patch 2/3 updates x_tables.
I'm sure there is more todo, i.e. nftables.
We could also define that netfilter will never use the alias but this needs documented cleary.
Patch 3/3 is a cleanup in continuation of 2/3.
[0] http://www.freedesktop.org/wiki/Software/systemd/PredictableNetworkInterfaceNames/
Thanks,
//richard
git://git.kernel.org/pub/scm/linux/kernel/git/rw/misc.git netalias
[PATCH 1/3] net: Make interface aliases available for general usage
[PATCH 2/3] x_tables: Use also dev->ifalias for interface matching
[PATCH 3/3] x_tables: Factor out 16bit aligment ifname_compare()
include/linux/netdevice.h | 1 +
include/linux/netfilter/x_tables.h | 41 +++++++++++++++++++++++++++---
include/net/net_namespace.h | 1 +
net/core/dev.c | 52 ++++++++++++++++++++++++++++++++++++++
net/ipv4/netfilter/arp_tables.c | 37 ++++-----------------------
net/ipv4/netfilter/ip_tables.c | 15 ++++-------
net/ipv6/netfilter/ip6_tables.c | 18 +++++--------
net/netfilter/xt_physdev.c | 9 ++-----
8 files changed, 110 insertions(+), 64 deletions(-)
^ permalink raw reply
* [PATCH 1/3] net: Make interface aliases available for general usage
From: Richard Weinberger @ 2015-01-11 20:52 UTC (permalink / raw)
To: davem
Cc: coreteam, netfilter-devel, linux-kernel, netdev, bhutchings,
john.fastabend, herbert, vyasevic, jiri, vfalico, therbert,
edumazet, yoshfuji, jmorris, kuznet, kadlec, kaber, pablo, kay,
stephen, Richard Weinberger
In-Reply-To: <1421009571-5279-1-git-send-email-richard@nod.at>
Allow interface aliases to be used as regular interfaces.
Such that a command sequence like this one works:
$ ip l set eth0 alias internet
$ ip a s internet
$ tcpdump -n -i internet
Signed-off-by: Richard Weinberger <richard@nod.at>
---
include/linux/netdevice.h | 1 +
include/net/net_namespace.h | 1 +
net/core/dev.c | 52 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 54 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 679e6e9..e00b4e2 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1493,6 +1493,7 @@ struct net_device {
char name[IFNAMSIZ];
struct hlist_node name_hlist;
char *ifalias;
+ struct hlist_node ifalias_hlist;
/*
* I/O specific fields
* FIXME: Merge these and struct ifmap into one
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 2e8756b8..9fa0939 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -76,6 +76,7 @@ struct net {
struct list_head dev_base_head;
struct hlist_head *dev_name_head;
struct hlist_head *dev_index_head;
+ struct hlist_head *dev_ifalias_head;
unsigned int dev_base_seq; /* protected by rtnl_mutex */
int ifindex;
unsigned int dev_unreg_count;
diff --git a/net/core/dev.c b/net/core/dev.c
index 683d493..2551b03 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -202,6 +202,14 @@ static inline struct hlist_head *dev_index_hash(struct net *net, int ifindex)
return &net->dev_index_head[ifindex & (NETDEV_HASHENTRIES - 1)];
}
+static inline struct hlist_head *dev_ifalias_hash(struct net *net,
+ const char *ifalias)
+{
+ unsigned int hash = full_name_hash(ifalias, strnlen(ifalias, IFALIASZ));
+
+ return &net->dev_ifalias_head[hash_32(hash, NETDEV_HASHBITS)];
+}
+
static inline void rps_lock(struct softnet_data *sd)
{
#ifdef CONFIG_RPS
@@ -228,6 +236,9 @@ static void list_netdevice(struct net_device *dev)
hlist_add_head_rcu(&dev->name_hlist, dev_name_hash(net, dev->name));
hlist_add_head_rcu(&dev->index_hlist,
dev_index_hash(net, dev->ifindex));
+ if (dev->ifalias)
+ hlist_add_head_rcu(&dev->ifalias_hlist,
+ dev_ifalias_hash(net, dev->ifalias));
write_unlock_bh(&dev_base_lock);
dev_base_seq_inc(net);
@@ -245,6 +256,8 @@ static void unlist_netdevice(struct net_device *dev)
list_del_rcu(&dev->dev_list);
hlist_del_rcu(&dev->name_hlist);
hlist_del_rcu(&dev->index_hlist);
+ if (dev->ifalias)
+ hlist_del_rcu(&dev->ifalias_hlist);
write_unlock_bh(&dev_base_lock);
dev_base_seq_inc(dev_net(dev));
@@ -679,6 +692,11 @@ struct net_device *__dev_get_by_name(struct net *net, const char *name)
if (!strncmp(dev->name, name, IFNAMSIZ))
return dev;
+ head = dev_ifalias_hash(net, name);
+ hlist_for_each_entry(dev, head, ifalias_hlist)
+ if (dev->ifalias && !strncmp(dev->ifalias, name, IFALIASZ))
+ return dev;
+
return NULL;
}
EXPORT_SYMBOL(__dev_get_by_name);
@@ -704,6 +722,11 @@ struct net_device *dev_get_by_name_rcu(struct net *net, const char *name)
if (!strncmp(dev->name, name, IFNAMSIZ))
return dev;
+ head = dev_ifalias_hash(net, name);
+ hlist_for_each_entry_rcu(dev, head, ifalias_hlist)
+ if (dev->ifalias && !strncmp(dev->ifalias, name, IFALIASZ))
+ return dev;
+
return NULL;
}
EXPORT_SYMBOL(dev_get_by_name_rcu);
@@ -1169,6 +1192,20 @@ rollback:
return err;
}
+static void __hlist_del_alias(struct net_device *dev)
+{
+ write_lock_bh(&dev_base_lock);
+ hlist_del_rcu(&dev->ifalias_hlist);
+ write_unlock_bh(&dev_base_lock);
+}
+
+static void __hlist_add_alias(struct net_device *dev)
+{
+ write_lock_bh(&dev_base_lock);
+ hlist_add_head_rcu(&dev->ifalias_hlist, dev_ifalias_hash(dev_net(dev), dev->ifalias));
+ write_unlock_bh(&dev_base_lock);
+}
+
/**
* dev_set_alias - change ifalias of a device
* @dev: device
@@ -1189,15 +1226,24 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
if (!len) {
kfree(dev->ifalias);
dev->ifalias = NULL;
+ __hlist_del_alias(dev);
return 0;
}
new_ifalias = krealloc(dev->ifalias, len + 1, GFP_KERNEL);
if (!new_ifalias)
return -ENOMEM;
+
+ if (dev->ifalias) {
+ __hlist_del_alias(dev);
+ synchronize_rcu();
+ }
+
dev->ifalias = new_ifalias;
strlcpy(dev->ifalias, alias, len+1);
+ __hlist_add_alias(dev);
+
return len;
}
@@ -7150,8 +7196,14 @@ static int __net_init netdev_init(struct net *net)
if (net->dev_index_head == NULL)
goto err_idx;
+ net->dev_ifalias_head = netdev_create_hash();
+ if (net->dev_ifalias_head == NULL)
+ goto err_alias;
+
return 0;
+err_alias:
+ kfree(net->dev_index_head);
err_idx:
kfree(net->dev_name_head);
err_name:
--
1.8.4.5
^ permalink raw reply related
* [PATCH 2/3] x_tables: Use also dev->ifalias for interface matching
From: Richard Weinberger @ 2015-01-11 20:52 UTC (permalink / raw)
To: davem
Cc: coreteam, netfilter-devel, linux-kernel, netdev, bhutchings,
john.fastabend, herbert, vyasevic, jiri, vfalico, therbert,
edumazet, yoshfuji, jmorris, kuznet, kadlec, kaber, pablo, kay,
stephen, Richard Weinberger
In-Reply-To: <1421009571-5279-1-git-send-email-richard@nod.at>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
include/linux/netfilter/x_tables.h | 22 ++++++++++++++++++++++
net/ipv4/netfilter/arp_tables.c | 28 +++++++++++++++++-----------
net/ipv4/netfilter/ip_tables.c | 15 +++++----------
net/ipv6/netfilter/ip6_tables.c | 18 +++++++-----------
net/netfilter/xt_physdev.c | 9 ++-------
5 files changed, 53 insertions(+), 39 deletions(-)
diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
index a3e215b..15bda23 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -351,6 +351,28 @@ static inline unsigned long ifname_compare_aligned(const char *_a,
return ret;
}
+/*
+ * A wrapper around ifname_compare_aligned() to match against dev->name and
+ * dev->ifalias.
+ */
+static inline unsigned long ifname_compare_all(const struct net_device *dev,
+ const char *name,
+ const char *mask)
+{
+ unsigned long res = 0;
+
+ if (!dev)
+ goto out;
+
+ res = ifname_compare_aligned(dev->name, name, mask);
+ if (unlikely(dev->ifalias && res))
+ res = ifname_compare_aligned(dev->ifalias, name, mask);
+
+out:
+ return res;
+}
+
+
struct nf_hook_ops *xt_hook_link(const struct xt_table *, nf_hookfn *);
void xt_hook_unlink(const struct xt_table *, struct nf_hook_ops *);
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index f95b6f9..457d4ed 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -81,19 +81,30 @@ static inline int arp_devaddr_compare(const struct arpt_devaddr_info *ap,
* Some arches dont care, unrolling the loop is a win on them.
* For other arches, we only have a 16bit alignement.
*/
-static unsigned long ifname_compare(const char *_a, const char *_b, const char *_mask)
+static unsigned long ifname_compare(const struct net_device *dev,
+ const char *_b, const char *_mask)
{
#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
- unsigned long ret = ifname_compare_aligned(_a, _b, _mask);
+ unsigned long ret = ifname_compare_all(dev, _b, _mask);
#else
unsigned long ret = 0;
- const u16 *a = (const u16 *)_a;
+ const u16 *a = (const u16 *)dev->name;
const u16 *b = (const u16 *)_b;
const u16 *mask = (const u16 *)_mask;
int i;
for (i = 0; i < IFNAMSIZ/sizeof(u16); i++)
ret |= (a[i] ^ b[i]) & mask[i];
+
+ if (likely(!(dev->ifalias && ret)))
+ goto out;
+
+ ret = 0;
+ a = (const u16 *)dev->ifalias;
+ for (i = 0; i < IFNAMSIZ/sizeof(u16); i++)
+ ret |= (a[i] ^ b[i]) & mask[i];
+
+out:
#endif
return ret;
}
@@ -101,8 +112,8 @@ static unsigned long ifname_compare(const char *_a, const char *_b, const char *
/* Returns whether packet matches rule or not. */
static inline int arp_packet_match(const struct arphdr *arphdr,
struct net_device *dev,
- const char *indev,
- const char *outdev,
+ const struct net_device *indev,
+ const struct net_device *outdev,
const struct arpt_arp *arpinfo)
{
const char *arpptr = (char *)(arphdr + 1);
@@ -252,11 +263,9 @@ unsigned int arpt_do_table(struct sk_buff *skb,
const struct net_device *out,
struct xt_table *table)
{
- static const char nulldevname[IFNAMSIZ] __attribute__((aligned(sizeof(long))));
unsigned int verdict = NF_DROP;
const struct arphdr *arp;
struct arpt_entry *e, *back;
- const char *indev, *outdev;
void *table_base;
const struct xt_table_info *private;
struct xt_action_param acpar;
@@ -265,9 +274,6 @@ unsigned int arpt_do_table(struct sk_buff *skb,
if (!pskb_may_pull(skb, arp_hdr_len(skb->dev)))
return NF_DROP;
- indev = in ? in->name : nulldevname;
- outdev = out ? out->name : nulldevname;
-
local_bh_disable();
addend = xt_write_recseq_begin();
private = table->private;
@@ -291,7 +297,7 @@ unsigned int arpt_do_table(struct sk_buff *skb,
do {
const struct xt_entry_target *t;
- if (!arp_packet_match(arp, skb->dev, indev, outdev, &e->arp)) {
+ if (!arp_packet_match(arp, skb->dev, in, out, &e->arp)) {
e = arpt_next_entry(e);
continue;
}
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 99e810f..87df9ef 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -73,8 +73,8 @@ EXPORT_SYMBOL_GPL(ipt_alloc_initial_table);
/* Performance critical - called for every packet */
static inline bool
ip_packet_match(const struct iphdr *ip,
- const char *indev,
- const char *outdev,
+ const struct net_device *indev,
+ const struct net_device *outdev,
const struct ipt_ip *ipinfo,
int isfrag)
{
@@ -97,7 +97,7 @@ ip_packet_match(const struct iphdr *ip,
return false;
}
- ret = ifname_compare_aligned(indev, ipinfo->iniface, ipinfo->iniface_mask);
+ ret = ifname_compare_all(indev, ipinfo->iniface, ipinfo->iniface_mask);
if (FWINV(ret != 0, IPT_INV_VIA_IN)) {
dprintf("VIA in mismatch (%s vs %s).%s\n",
@@ -106,7 +106,7 @@ ip_packet_match(const struct iphdr *ip,
return false;
}
- ret = ifname_compare_aligned(outdev, ipinfo->outiface, ipinfo->outiface_mask);
+ ret = ifname_compare_all(outdev, ipinfo->outiface, ipinfo->outiface_mask);
if (FWINV(ret != 0, IPT_INV_VIA_OUT)) {
dprintf("VIA out mismatch (%s vs %s).%s\n",
@@ -292,11 +292,9 @@ ipt_do_table(struct sk_buff *skb,
const struct net_device *out,
struct xt_table *table)
{
- static const char nulldevname[IFNAMSIZ] __attribute__((aligned(sizeof(long))));
const struct iphdr *ip;
/* Initializing verdict to NF_DROP keeps gcc happy. */
unsigned int verdict = NF_DROP;
- const char *indev, *outdev;
const void *table_base;
struct ipt_entry *e, **jumpstack;
unsigned int *stackptr, origptr, cpu;
@@ -306,8 +304,6 @@ ipt_do_table(struct sk_buff *skb,
/* Initialization */
ip = ip_hdr(skb);
- indev = in ? in->name : nulldevname;
- outdev = out ? out->name : nulldevname;
/* We handle fragments by dealing with the first fragment as
* if it was a normal packet. All other fragments are treated
* normally, except that they will NEVER match rules that ask
@@ -348,8 +344,7 @@ ipt_do_table(struct sk_buff *skb,
const struct xt_entry_match *ematch;
IP_NF_ASSERT(e);
- if (!ip_packet_match(ip, indev, outdev,
- &e->ip, acpar.fragoff)) {
+ if (!ip_packet_match(ip, in, out, &e->ip, acpar.fragoff)) {
no_match:
e = ipt_next_entry(e);
continue;
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index e080fbb..9ed5d70 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -83,8 +83,8 @@ EXPORT_SYMBOL_GPL(ip6t_alloc_initial_table);
/* Performance critical - called for every packet */
static inline bool
ip6_packet_match(const struct sk_buff *skb,
- const char *indev,
- const char *outdev,
+ const struct net_device *indev,
+ const struct net_device *outdev,
const struct ip6t_ip6 *ip6info,
unsigned int *protoff,
int *fragoff, bool *hotdrop)
@@ -109,7 +109,7 @@ ip6_packet_match(const struct sk_buff *skb,
return false;
}
- ret = ifname_compare_aligned(indev, ip6info->iniface, ip6info->iniface_mask);
+ ret = ifname_compare_all(indev, ip6info->iniface, ip6info->iniface_mask);
if (FWINV(ret != 0, IP6T_INV_VIA_IN)) {
dprintf("VIA in mismatch (%s vs %s).%s\n",
@@ -118,7 +118,7 @@ ip6_packet_match(const struct sk_buff *skb,
return false;
}
- ret = ifname_compare_aligned(outdev, ip6info->outiface, ip6info->outiface_mask);
+ ret = ifname_compare_all(outdev, ip6info->outiface, ip6info->outiface_mask);
if (FWINV(ret != 0, IP6T_INV_VIA_OUT)) {
dprintf("VIA out mismatch (%s vs %s).%s\n",
@@ -318,10 +318,8 @@ ip6t_do_table(struct sk_buff *skb,
const struct net_device *out,
struct xt_table *table)
{
- static const char nulldevname[IFNAMSIZ] __attribute__((aligned(sizeof(long))));
/* Initializing verdict to NF_DROP keeps gcc happy. */
unsigned int verdict = NF_DROP;
- const char *indev, *outdev;
const void *table_base;
struct ip6t_entry *e, **jumpstack;
unsigned int *stackptr, origptr, cpu;
@@ -329,10 +327,8 @@ ip6t_do_table(struct sk_buff *skb,
struct xt_action_param acpar;
unsigned int addend;
- /* Initialization */
- indev = in ? in->name : nulldevname;
- outdev = out ? out->name : nulldevname;
- /* We handle fragments by dealing with the first fragment as
+ /* Initialization:
+ * We handle fragments by dealing with the first fragment as
* if it was a normal packet. All other fragments are treated
* normally, except that they will NEVER match rules that ask
* things we don't know, ie. tcp syn flag or ports). If the
@@ -368,7 +364,7 @@ ip6t_do_table(struct sk_buff *skb,
IP_NF_ASSERT(e);
acpar.thoff = 0;
- if (!ip6_packet_match(skb, indev, outdev, &e->ipv6,
+ if (!ip6_packet_match(skb, in, out, &e->ipv6,
&acpar.thoff, &acpar.fragoff, &acpar.hotdrop)) {
no_match:
e = ip6t_next_entry(e);
diff --git a/net/netfilter/xt_physdev.c b/net/netfilter/xt_physdev.c
index f440f57..8d2ee7d 100644
--- a/net/netfilter/xt_physdev.c
+++ b/net/netfilter/xt_physdev.c
@@ -25,10 +25,8 @@ MODULE_ALIAS("ip6t_physdev");
static bool
physdev_mt(const struct sk_buff *skb, struct xt_action_param *par)
{
- static const char nulldevname[IFNAMSIZ] __attribute__((aligned(sizeof(long))));
const struct xt_physdev_info *info = par->matchinfo;
unsigned long ret;
- const char *indev, *outdev;
const struct nf_bridge_info *nf_bridge;
/* Not a bridged IP packet or no info available yet:
@@ -68,8 +66,7 @@ physdev_mt(const struct sk_buff *skb, struct xt_action_param *par)
if (!(info->bitmask & XT_PHYSDEV_OP_IN))
goto match_outdev;
- indev = nf_bridge->physindev ? nf_bridge->physindev->name : nulldevname;
- ret = ifname_compare_aligned(indev, info->physindev, info->in_mask);
+ ret = ifname_compare_all(nf_bridge->physindev, info->physindev, info->in_mask);
if (!ret ^ !(info->invert & XT_PHYSDEV_OP_IN))
return false;
@@ -77,9 +74,7 @@ physdev_mt(const struct sk_buff *skb, struct xt_action_param *par)
match_outdev:
if (!(info->bitmask & XT_PHYSDEV_OP_OUT))
return true;
- outdev = nf_bridge->physoutdev ?
- nf_bridge->physoutdev->name : nulldevname;
- ret = ifname_compare_aligned(outdev, info->physoutdev, info->out_mask);
+ ret = ifname_compare_all(nf_bridge->physoutdev, info->physoutdev, info->out_mask);
return (!!ret ^ !(info->invert & XT_PHYSDEV_OP_OUT));
}
--
1.8.4.5
^ permalink raw reply related
* [PATCH 3/3] x_tables: Factor out 16bit aligment ifname_compare()
From: Richard Weinberger @ 2015-01-11 20:52 UTC (permalink / raw)
To: davem
Cc: coreteam, netfilter-devel, linux-kernel, netdev, bhutchings,
john.fastabend, herbert, vyasevic, jiri, vfalico, therbert,
edumazet, yoshfuji, jmorris, kuznet, kadlec, kaber, pablo, kay,
stephen, Richard Weinberger
In-Reply-To: <1421009571-5279-1-git-send-email-richard@nod.at>
arp_tables.c has a 16bit aligment ifname_compare(), factor
it out to use it for all tables.
Signed-off-by: Richard Weinberger <richard@nod.at>
---
include/linux/netfilter/x_tables.h | 25 ++++++++++++++++++-------
net/ipv4/netfilter/arp_tables.c | 37 ++-----------------------------------
2 files changed, 20 insertions(+), 42 deletions(-)
diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
index 15bda23..26dddc1 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -331,14 +331,15 @@ static inline void xt_write_recseq_end(unsigned int addend)
/*
* This helper is performance critical and must be inlined
*/
-static inline unsigned long ifname_compare_aligned(const char *_a,
- const char *_b,
- const char *_mask)
+static inline unsigned long ifname_compare(const char *_a,
+ const char *_b,
+ const char *_mask)
{
+ unsigned long ret;
+#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
const unsigned long *a = (const unsigned long *)_a;
const unsigned long *b = (const unsigned long *)_b;
const unsigned long *mask = (const unsigned long *)_mask;
- unsigned long ret;
ret = (a[0] ^ b[0]) & mask[0];
if (IFNAMSIZ > sizeof(unsigned long))
@@ -348,11 +349,21 @@ static inline unsigned long ifname_compare_aligned(const char *_a,
if (IFNAMSIZ > 3 * sizeof(unsigned long))
ret |= (a[3] ^ b[3]) & mask[3];
BUILD_BUG_ON(IFNAMSIZ > 4 * sizeof(unsigned long));
+#else
+ const u16 *a = (const u16 *)_a;
+ const u16 *b = (const u16 *)_b;
+ const u16 *mask = (const u16 *)_mask;
+ int i;
+
+ ret = 0;
+ for (i = 0; i < IFNAMSIZ/sizeof(u16); i++)
+ ret |= (a[i] ^ b[i]) & mask[i];
+#endif
return ret;
}
/*
- * A wrapper around ifname_compare_aligned() to match against dev->name and
+ * A wrapper around ifname_compare() to match against dev->name and
* dev->ifalias.
*/
static inline unsigned long ifname_compare_all(const struct net_device *dev,
@@ -364,9 +375,9 @@ static inline unsigned long ifname_compare_all(const struct net_device *dev,
if (!dev)
goto out;
- res = ifname_compare_aligned(dev->name, name, mask);
+ res = ifname_compare(dev->name, name, mask);
if (unlikely(dev->ifalias && res))
- res = ifname_compare_aligned(dev->ifalias, name, mask);
+ res = ifname_compare(dev->ifalias, name, mask);
out:
return res;
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 457d4ed..c978691 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -76,39 +76,6 @@ static inline int arp_devaddr_compare(const struct arpt_devaddr_info *ap,
return ret != 0;
}
-/*
- * Unfortunately, _b and _mask are not aligned to an int (or long int)
- * Some arches dont care, unrolling the loop is a win on them.
- * For other arches, we only have a 16bit alignement.
- */
-static unsigned long ifname_compare(const struct net_device *dev,
- const char *_b, const char *_mask)
-{
-#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
- unsigned long ret = ifname_compare_all(dev, _b, _mask);
-#else
- unsigned long ret = 0;
- const u16 *a = (const u16 *)dev->name;
- const u16 *b = (const u16 *)_b;
- const u16 *mask = (const u16 *)_mask;
- int i;
-
- for (i = 0; i < IFNAMSIZ/sizeof(u16); i++)
- ret |= (a[i] ^ b[i]) & mask[i];
-
- if (likely(!(dev->ifalias && ret)))
- goto out;
-
- ret = 0;
- a = (const u16 *)dev->ifalias;
- for (i = 0; i < IFNAMSIZ/sizeof(u16); i++)
- ret |= (a[i] ^ b[i]) & mask[i];
-
-out:
-#endif
- return ret;
-}
-
/* Returns whether packet matches rule or not. */
static inline int arp_packet_match(const struct arphdr *arphdr,
struct net_device *dev,
@@ -192,7 +159,7 @@ static inline int arp_packet_match(const struct arphdr *arphdr,
}
/* Look for ifname matches. */
- ret = ifname_compare(indev, arpinfo->iniface, arpinfo->iniface_mask);
+ ret = ifname_compare_all(indev, arpinfo->iniface, arpinfo->iniface_mask);
if (FWINV(ret != 0, ARPT_INV_VIA_IN)) {
dprintf("VIA in mismatch (%s vs %s).%s\n",
@@ -201,7 +168,7 @@ static inline int arp_packet_match(const struct arphdr *arphdr,
return 0;
}
- ret = ifname_compare(outdev, arpinfo->outiface, arpinfo->outiface_mask);
+ ret = ifname_compare_all(outdev, arpinfo->outiface, arpinfo->outiface_mask);
if (FWINV(ret != 0, ARPT_INV_VIA_OUT)) {
dprintf("VIA out mismatch (%s vs %s).%s\n",
--
1.8.4.5
^ permalink raw reply related
* Re: [PATCH 3/3] x_tables: Factor out 16bit aligment ifname_compare()
From: Joe Perches @ 2015-01-11 20:59 UTC (permalink / raw)
To: Richard Weinberger
Cc: davem, coreteam, netfilter-devel, linux-kernel, netdev,
bhutchings, john.fastabend, herbert, vyasevic, jiri, vfalico,
therbert, edumazet, yoshfuji, jmorris, kuznet, kadlec, kaber,
pablo, kay, stephen
In-Reply-To: <1421009571-5279-4-git-send-email-richard@nod.at>
On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote:
> arp_tables.c has a 16bit aligment ifname_compare(), factor
> it out to use it for all tables.
[]
> diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
[]
> @@ -331,14 +331,15 @@ static inline void xt_write_recseq_end(unsigned int addend)
> /*
> * This helper is performance critical and must be inlined
> */
> -static inline unsigned long ifname_compare_aligned(const char *_a,
> - const char *_b,
> - const char *_mask)
> +static inline unsigned long ifname_compare(const char *_a,
> + const char *_b,
> + const char *_mask)
Perhaps this would be better as bool ifname_compare
^ permalink raw reply
* Re: [PATCH 3/3] x_tables: Factor out 16bit aligment ifname_compare()
From: Richard Weinberger @ 2015-01-11 21:02 UTC (permalink / raw)
To: Joe Perches
Cc: davem, coreteam, netfilter-devel, linux-kernel, netdev,
bhutchings, john.fastabend, herbert, vyasevic, jiri, vfalico,
therbert, edumazet, yoshfuji, jmorris, kuznet, kadlec, kaber,
pablo, kay, stephen
In-Reply-To: <1421009969.9233.5.camel@perches.com>
Am 11.01.2015 um 21:59 schrieb Joe Perches:
> On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote:
>> arp_tables.c has a 16bit aligment ifname_compare(), factor
>> it out to use it for all tables.
> []
>> diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
> []
>> @@ -331,14 +331,15 @@ static inline void xt_write_recseq_end(unsigned int addend)
>> /*
>> * This helper is performance critical and must be inlined
>> */
>> -static inline unsigned long ifname_compare_aligned(const char *_a,
>> - const char *_b,
>> - const char *_mask)
>> +static inline unsigned long ifname_compare(const char *_a,
>> + const char *_b,
>> + const char *_mask)
>
> Perhaps this would be better as bool ifname_compare
Let's discuss the whole concept first, then we can go to bikeshedding mode.
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH 3/3] x_tables: Factor out 16bit aligment ifname_compare()
From: Joe Perches @ 2015-01-11 21:14 UTC (permalink / raw)
To: Richard Weinberger
Cc: davem, coreteam, netfilter-devel, linux-kernel, netdev,
bhutchings, john.fastabend, herbert, vyasevic, jiri, vfalico,
therbert, edumazet, yoshfuji, jmorris, kuznet, kadlec, kaber,
pablo, kay, stephen
In-Reply-To: <54B2E4DD.90200@nod.at>
On Sun, 2015-01-11 at 22:02 +0100, Richard Weinberger wrote:
> Am 11.01.2015 um 21:59 schrieb Joe Perches:
> > On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote:
> >> arp_tables.c has a 16bit aligment ifname_compare(), factor
> >> it out to use it for all tables.
[]
> > Perhaps this would be better as bool ifname_compare
> Let's discuss the whole concept first
The concept seems obvious enough
^ permalink raw reply
* Re: [PATCH 3/3] x_tables: Factor out 16bit aligment ifname_compare()
From: Richard Weinberger @ 2015-01-11 21:30 UTC (permalink / raw)
To: Joe Perches
Cc: davem, coreteam, netfilter-devel, linux-kernel, netdev,
bhutchings, john.fastabend, herbert, vyasevic, jiri, vfalico,
therbert, edumazet, yoshfuji, jmorris, kuznet, kadlec, kaber,
pablo, kay, stephen
In-Reply-To: <1421010853.9233.7.camel@perches.com>
Am 11.01.2015 um 22:14 schrieb Joe Perches:
> On Sun, 2015-01-11 at 22:02 +0100, Richard Weinberger wrote:
>> Am 11.01.2015 um 21:59 schrieb Joe Perches:
>>> On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote:
>>>> arp_tables.c has a 16bit aligment ifname_compare(), factor
>>>> it out to use it for all tables.
> []
>>> Perhaps this would be better as bool ifname_compare
>> Let's discuss the whole concept first
>
> The concept seems obvious enough
Anyway, I agree with Linus wrt. bool.
https://lkml.org/lkml/2013/8/31/138
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH net] packet: bail out of packet_snd() if L2 header creation fails
From: Daniel Borkmann @ 2015-01-11 21:38 UTC (permalink / raw)
To: Christoph Jaeger; +Cc: davem, willemb, edumazet, netdev, linux-kernel
In-Reply-To: <1420999276-28225-1-git-send-email-cj@linux.com>
On 01/11/2015 07:01 PM, Christoph Jaeger wrote:
> Due to a misplaced parenthesis, the expression
>
> (unlikely(offset) < 0),
>
> which expands to
>
> (__builtin_expect(!!(offset), 0) < 0),
>
> never evaluates to true. Therefore, when sending packets with
> PF_PACKET/SOCK_DGRAM, packet_snd() does not abort as intended
> if the creation of the layer 2 header fails.
>
> Spotted by Coverity - CID 1259975 ("Operands don't affect result").
>
> Fixes: 9c7077622dd9 ("packet: make packet_snd fail on len smaller than l2 header")
> Signed-off-by: Christoph Jaeger <cj@linux.com>
Thanks, Christoph!
Acked-by: Daniel Borkmann <dborkman@redhat.com>
^ permalink raw reply
* Re: [PATCH 3/3] x_tables: Factor out 16bit aligment ifname_compare()
From: Joe Perches @ 2015-01-11 21:39 UTC (permalink / raw)
To: Richard Weinberger
Cc: davem, coreteam, netfilter-devel, linux-kernel, netdev,
bhutchings, john.fastabend, herbert, vyasevic, jiri, vfalico,
therbert, edumazet, yoshfuji, jmorris, kuznet, kadlec, kaber,
pablo, kay, stephen
In-Reply-To: <54B2EB81.5050604@nod.at>
On Sun, 2015-01-11 at 22:30 +0100, Richard Weinberger wrote:
> Am 11.01.2015 um 22:14 schrieb Joe Perches:
> > On Sun, 2015-01-11 at 22:02 +0100, Richard Weinberger wrote:
> >> Am 11.01.2015 um 21:59 schrieb Joe Perches:
> >>> On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote:
> >>>> arp_tables.c has a 16bit aligment ifname_compare(), factor
> >>>> it out to use it for all tables.
> > []
> >>> Perhaps this would be better as bool ifname_compare
> >> Let's discuss the whole concept first
> >
> > The concept seems obvious enough
>
> Anyway, I agree with Linus wrt. bool.
> https://lkml.org/lkml/2013/8/31/138
I don't. He was right when he wrote:
https://lkml.org/lkml/2014/3/10/760
Linus Torvalds <>
I guess I haven't gotten over my hatred of people playing games with
them because support wasn't universal enough. But maybe it's
approaching being irrational these days.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox