* Re: [PATCH] vhost/net: length miscalculation
From: Michael S. Tsirkin @ 2015-01-08 8:08 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: kvm, netdev, linux-kernel, virtualization
In-Reply-To: <54AD9DD8.2080008@cogentembedded.com>
On Wed, Jan 07, 2015 at 11:58:00PM +0300, Sergei Shtylyov wrote:
> Hello.
>
> On 01/07/2015 11:55 AM, Michael S. Tsirkin wrote:
>
> >commit 8b38694a2dc8b18374310df50174f1e4376d6824
> > vhost/net: virtio 1.0 byte swap
> >had this chunk:
> >- heads[headcount - 1].len += datalen;
> >+ heads[headcount - 1].len = cpu_to_vhost32(vq, len - datalen);
>
> >This adds datalen with the wrong sign, causing guest panics.
>
> >Fixes: 8b38694a2dc8b18374310df50174f1e4376d6824
>
> The format of this tag assumes 12-digit SHA1 hash and the commit
> description enclosed in parens and double quotes. See
> Documentation/SubmittingPatches.
>
> >Reported-by: Alex Williamson <alex.williamson@redhat.com>
> >Suggested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> WBR, Sergei
I pushed the patches to Linus unfortunately - there's
some urgency since many people are hitting the bug.
Will do my best to do it right next time.
--
MST
^ permalink raw reply
* Re: [PATCH] net/at91_ether: prepare and unprepare clock
From: Boris Brezillon @ 2015-01-08 8:13 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Nicolas Ferre, David S. Miller, linux-arm-kernel, linux-kernel,
netdev
In-Reply-To: <1420671566-30784-1-git-send-email-alexandre.belloni@free-electrons.com>
On Wed, 7 Jan 2015 23:59:26 +0100
Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:
> The clock is enabled without being prepared, this leads to:
>
> WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:889 __clk_enable+0x24/0xa8()
>
> and a non working ethernet interface.
>
> Use clk_prepare_enable() and clk_disable_unprepare() to handle the clock.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/net/ethernet/cadence/at91_ether.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/at91_ether.c b/drivers/net/ethernet/cadence/at91_ether.c
> index 55eb7f2af2b4..7ef55f5fa664 100644
> --- a/drivers/net/ethernet/cadence/at91_ether.c
> +++ b/drivers/net/ethernet/cadence/at91_ether.c
> @@ -340,7 +340,7 @@ static int __init at91ether_probe(struct platform_device *pdev)
> res = PTR_ERR(lp->pclk);
> goto err_free_dev;
> }
> - clk_enable(lp->pclk);
> + clk_prepare_enable(lp->pclk);
>
> lp->hclk = ERR_PTR(-ENOENT);
> lp->tx_clk = ERR_PTR(-ENOENT);
> @@ -406,7 +406,7 @@ static int __init at91ether_probe(struct platform_device *pdev)
> err_out_unregister_netdev:
> unregister_netdev(dev);
> err_disable_clock:
> - clk_disable(lp->pclk);
> + clk_disable_unprepare(lp->pclk);
> err_free_dev:
> free_netdev(dev);
> return res;
> @@ -424,7 +424,7 @@ static int at91ether_remove(struct platform_device *pdev)
> kfree(lp->mii_bus->irq);
> mdiobus_free(lp->mii_bus);
> unregister_netdev(dev);
> - clk_disable(lp->pclk);
> + clk_disable_unprepare(lp->pclk);
> free_netdev(dev);
>
> return 0;
> @@ -440,7 +440,7 @@ static int at91ether_suspend(struct platform_device *pdev, pm_message_t mesg)
> netif_stop_queue(net_dev);
> netif_device_detach(net_dev);
>
> - clk_disable(lp->pclk);
> + clk_disable_unprepare(lp->pclk);
> }
> return 0;
> }
> @@ -451,7 +451,7 @@ static int at91ether_resume(struct platform_device *pdev)
> struct macb *lp = netdev_priv(net_dev);
>
> if (netif_running(net_dev)) {
> - clk_enable(lp->pclk);
> + clk_prepare_enable(lp->pclk);
>
> netif_device_attach(net_dev);
> netif_start_queue(net_dev);
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH] net: Prevent multiple NAPI instances co-existing in the list
From: Dennis Chen @ 2015-01-08 8:22 UTC (permalink / raw)
To: netdev, Herbert Xu, Miller, Eric Dumazet; +Cc: Dennis Chen
Some drivers may clear the NAPI_STATE_SCHED bit upon the state of the
NAPI instance after exhaust the budget in the poll function, which
will open a window for next device interrupt handler to insert a same
instance to the list after calling list_add_tail(&n->poll_list,
repoll) if we don't set this bit.
Signed-off-by: Dennis Chen <kernel.org.gnu@gmail.com>
---
net/core/dev.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/net/core/dev.c b/net/core/dev.c
index 683d493..b3107ac 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4619,6 +4619,14 @@ static int napi_poll(struct napi_struct *n,
struct list_head *repoll)
n->dev ? n->dev->name : "backlog");
goto out_unlock;
}
+
+ /* Some drivers may exit the polling mode when exhaust the
+ * budget. Set the NAPI_STATE_SCHED bit to prevent multiple NAPI
+ * instances in the list in case of next device interrupt raised.
+ */
+ if (unlikely(!test_and_set_bit(NAPI_STATE_SCHED, &n->state)))
+ pr_warn_once("%s: exit polling mode after exhaust the budget\n",
+ n->dev ? n->dev->name : "backlog");
list_add_tail(&n->poll_list, repoll);
--
1.7.9.5
--
Den
^ permalink raw reply related
* Re: [PATCH net-next v2 0/8] net: extend ethtool link mode bitmaps to 48 bits
From: Amir Vadai @ 2015-01-08 8:40 UTC (permalink / raw)
To: David Decotigny
Cc: Florian Fainelli, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Saeed Mahameed,
David S. Miller, Jason Wang, Michael S. Tsirkin, Herbert Xu,
Al Viro, Ben Hutchings, Masatake YAMATO, Xi Wang, Neil Horman,
WANG Cong, Flavio Leitner, Tom Gundersen, Jiri Pirko,
Vlad Yasevich, Eric W. Biederman, Venkata Duvvuru,
Govindarajulu Varadarajan <_govind
In-Reply-To: <CAG88wWYPDpwkWkL+Pj2VKrX5WVp=at8v0=gcFAVAA8nntv+-nw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 1/6/2015 7:36 PM, David Decotigny wrote:
> Interesting. It seems that the band-aid I was proposing is already
> obsolete. We could still use the remaining reserved 16 bits to encode
> 5 more bits per mask (that is: 53 bits / mask total). But if I
> understand you, it would allow us to survive only a few months longer,
> as opposed to a few weeks.
>
> One short-term alternative solution I can imagine is the following:
> /* For example bitmap-based for variable length: */
> struct ethtool_link_mode {
> __u32 cmd;
> __u8 autoneg :1;
> __u8 duplex :2;
> __u16 supported_nbits;
> __u16 advertising_nbits;
> __u16 lp_advertising_nbits;
> __u32 reserved[4];
> __u8 masks[0];
> };
> /* Or simpler, statically limited to 64b / mask, but easier to migrate
> to for driver authors: */
I think the first options is better. A driver will have to do changes in
order to support >32 link modes, so better change it once now, without
having to change it again for >64 link modes.
> struct ethtool_link_mode {
> __u32 cmd;
> __u8 autoneg :1;
> __u8 duplex :2;
> __u64 supported;
> __u64 advertising;
> __u64 lp_advertising;
> __u32 reserved[4];
> };
> #define ETHTOOL_GLINK_MODE 0x0000004a
> #define ETHTOOL_SLINK_MODE 0x0000004b
> struct ethtool_ops {
> ...
> int (*get_link_mode)(struct net_device *, struct ethtool_link_mode *);
> int (*set_link_mode)(struct net_device *, struct ethtool_link_mode *);
> };
>
> The same thing required for EEE.
Yeh :(
>
> I am not sure about moving the autoneg and duplex fields into the new
> struct. Especially the "duplex" field.
I think so too. ethtool user space will call ETHTOOL_[GS]SET and after
that ETHTOOL_[GS]LINK_MODE (if supported). No need to get the
duplex/autoneg fields again.
>
> Then the idea would be to update the ethtool user-space tool to try
> get/set_link mode when reporting/changing the autoneg/advertising
> settings.
>
> Both will require significant effort from the driver authors.
> Especially if the variable-length bitmap approach is preferred:
> - most drivers currently use simple bitwise arithmetic in their code,
> and that goes far beyond get/set_settings, it is sometimes part of the
> core driver logic. They will have to migrate to the bitmap API if they
> want to use the larger bitmaps (note: no change needed if they are
> happy with <= 32b / mask)
As I said above, it will save as doing this work again in the future,
and more problematic, save another version to backport in the future. In
addition, not all drivers will have to do it, only if >32 link speeds is
needed - this work will be required.
> - we would have to progressively deprecate the use of #define
> ADVERTISED_1000baseT_Full in favor of an enum of the bit indices.
Maybe we could use some macro juggling to define the legacy macro's
using enum for the first 32 bits, and fail the compilation if used on
>32. For example, calling this:
DEFINE_LINK_MODE(ADVERTISED_1000baseT_Full, 5)
Will add the following:
ADVERTISED_1000baseT_Full_SHIFT = 5
ADVERTISED_1000baseT_Full = (1<<5)
DEFINE_LINK_MODE(ADVERTISED_100000baseKR5_Full, 50) will add:
ADVERTISED_100000baseKR5_Full_SHIFT = 50
ADVERTISED_100000baseKR5_Full = #error new link speeds must be defined
using [gs]et_link_speed
This will break compilation if ADVERTISED_100000baseKR5_Full is used in
[gs]et_settings (I know the '#error' will not print something very
pretty - I used it only to explain what I meant)
>
> Any feedback welcome. In the meantime, I am going to propose a v3 of
> current option with 53 bits / mask. I can also propose a prototype of
> the scheme described above, please let me know.
I think that it is better to do it once, and skip the 53 bits / mask
version.
I'll be happy to assist.
Amir
^ permalink raw reply
* Re: [PATCH net-next v5 0/7]: ixgbevf: Allow querying VFs RSS indirection table and key
From: Vlad Zolotarov @ 2015-01-08 8:42 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: netdev, gleb, avi, Greg Rose, Greg Rose
In-Reply-To: <1420661025.2491.73.camel@jtkirshe-mobl>
On 01/07/15 22:03, Jeff Kirsher wrote:
> On Wed, 2015-01-07 at 21:26 +0200, Vlad Zolotarov wrote:
>> Add the ethtool ops to VF driver to allow querying the RSS indirection
>> table
>> and RSS Random Key.
>>
>> On some devices VFs share the RSS Redirection Table and Hash Key with
>> a PF and letting
>> the VF query this information may introduce some security risks.
>> Therefore we disable this
>> feature by default for such devices (e.g. 82599) and allow it for
>> those where there isn't any
>> possible risk (e.g. on x550). The new netdev op is going to allow a
>> system administrator to
>> change the default behaviour with "ip link set" command.
>>
>> - netdev: Add a new netdev op to allow/block VF from querying RSS
>> Indirection Table and
>> RSS Hash Key.
>> - PF driver: Add new VF-PF channel commands.
>> - VF driver: Utilize these new commands and add the corresponding
>> ethtool callbacks.
>>
>> New in v5:
>> - Added a new netdev op to allow/block VF from querying RSS
>> Indirection Table and
>> RSS Hash Key.
>> - Let VF query the RSS info only if VF is allowed to.
>>
>> New in v4:
>> - Forgot to run checkpatch on v3 and there were a few styling
>> things to fix. ;)
>>
>> New in v3:
>> - Added a missing support for x550 devices.
>> - Mask the indirection table values according to PSRTYPE[n].RQPL.
>> - Minimized the number of added VF-PF commands.
>>
>> New in v2:
>> - Added a detailed description to patches 4 and 5.
>>
>> New in v1 (compared to RFC):
>> - Use "if-else" statement instead of a "switch-case" for a single
>> option case.
>> More specifically: in cases where the newly added API version is
>> the only one
>> allowed. We may consider using a "switch-case" back again when
>> the list of
>> allowed API versions in these specific places grows up.
>>
>> Vlad Zolotarov (7):
>> if_link: Add an additional parameter to ifla_vf_info for RSS
>> querying
>> ixgbe: Add a new netdev op to allow/prevent a VF from querying an
>> RSS
>> info
>> ixgbe: Add a RETA query command to VF-PF channel API
>> ixgbevf: Add a RETA query code
>> ixgbe: Add GET_RSS_KEY command to VF-PF channel commands set
>> ixgbevf: Add RSS Key query code
>> ixgbevf: Add the appropriate ethtool ops to query RSS indirection
>> table and key
>>
>> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 +
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 7 ++
>> drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.h | 10 ++
>> drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 119
>> +++++++++++++++++++
>> drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h | 2 +
>> drivers/net/ethernet/intel/ixgbevf/ethtool.c | 42 +++++++
>> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 +-
>> drivers/net/ethernet/intel/ixgbevf/mbx.h | 10 ++
>> drivers/net/ethernet/intel/ixgbevf/vf.c | 132
>> ++++++++++++++++++++++
>> drivers/net/ethernet/intel/ixgbevf/vf.h | 2 +
>> include/linux/if_link.h | 1 +
>> include/linux/netdevice.h | 8 ++
>> include/uapi/linux/if_link.h | 8 ++
>> net/core/rtnetlink.c | 33 +++++-
>> 14 files changed, 372 insertions(+), 7 deletions(-)
> Thanks Vlad, I will add your patches to my queue.
Thanks, guys (Greg and Jeff).
^ permalink raw reply
* [patch iproute2 v2] iplink: print out addrgenmode attribute
From: Jiri Pirko @ 2015-01-08 8:49 UTC (permalink / raw)
To: netdev; +Cc: stephen, thaller, vadim4j
addrgenmode is currently write only by ip. So display this information
if provided by kernel as well.
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
v1->v2:
-rebased on current master (instead of net-next branch)
---
ip/ipaddress.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 28dfe8c..d8bda30 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -255,6 +255,29 @@ static void print_linktype(FILE *fp, struct rtattr *tb)
}
}
+static void print_af_spec(FILE *fp, struct rtattr *af_spec_attr)
+{
+ struct rtattr *inet6_attr;
+ struct rtattr *tb[IFLA_INET6_MAX + 1];
+
+ inet6_attr = parse_rtattr_one_nested(AF_INET6, af_spec_attr);
+ if (!inet6_attr)
+ return;
+
+ parse_rtattr_nested(tb, IFLA_INET6_MAX, inet6_attr);
+
+ if (tb[IFLA_INET6_ADDR_GEN_MODE]) {
+ switch (rta_getattr_u8(tb[IFLA_INET6_ADDR_GEN_MODE])) {
+ case IN6_ADDR_GEN_MODE_EUI64:
+ fprintf(fp, "addrgenmode eui64 ");
+ break;
+ case IN6_ADDR_GEN_MODE_NONE:
+ fprintf(fp, "addrgenmode none ");
+ break;
+ }
+ }
+}
+
static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
{
struct ifla_vf_mac *vf_mac;
@@ -658,6 +681,9 @@ int print_linkinfo(const struct sockaddr_nl *who,
if (tb[IFLA_LINKINFO] && show_details)
print_linktype(fp, tb[IFLA_LINKINFO]);
+ if (do_link && tb[IFLA_AF_SPEC] && show_details)
+ print_af_spec(fp, tb[IFLA_AF_SPEC]);
+
if ((do_link || show_details) && tb[IFLA_IFALIAS]) {
fprintf(fp, "%s alias %s", _SL_,
rta_getattr_str(tb[IFLA_IFALIAS]));
--
1.9.3
^ permalink raw reply related
* Re: [PATCH net] gso: do GSO for local skb with size bigger than MTU
From: Fan Du @ 2015-01-08 9:39 UTC (permalink / raw)
To: Jesse Gross
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org, Michael S. Tsirkin,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jason Wang,
Du, Fan, fw-HFFVJYpyMKqzQB+pC5nmwQ@public.gmane.org,
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <CAEP_g=8EBeQUFkRRsG3sznYryd+LE9qJKWQXfS==HG2HDO=UKA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
于 2015年01月08日 04:52, Jesse Gross 写道:
>> My understanding is:
>> >controller sets the forwarding rules into kernel datapath, any flow not
>> >matching
>> >with the rules are threw to controller by upcall. Once the rule decision is
>> >made
>> >by controller, then, this flow packet is pushed down to datapath to be
>> >forwarded
>> >again according to the new rule.
>> >
>> >So I'm not sure whether pushing the over-MTU-sized packet or pushing the
>> >forged ICMP
>> >without encapsulation to controller is required by current ovs
>> >implementation. By doing
>> >so, such over-MTU-sized packet is treated as a event for the controller to
>> >be take
>> >care of.
> If flows are implementing routing (again, they are doing things like
> decrementing the TTL) then it is necessary for them to also handle
> this situation using some potentially new primitives (like a size
> check). Otherwise you end up with issues like the ones that I
> mentioned above like needing to forge addresses because you don't know
> what the correct ones are.
Thanks for explaining, Jesse!
btw, I don't get it about "to forge addresses", building ICMP message
with Guest packet doesn't require to forge address when not encapsulating
ICMP message with outer headers.
If the flows aren't doing things to
> implement routing, then you really have a flat L2 network and you
> shouldn't be doing this type of behavior at all as I described in the
> original plan.
For flows implementing routing scenario:
First of all, over-MTU-sized packet could only be detected once the flow
as been consulted(each port could implement a 'check' hook to do this),
and just before send to the actual port.
Then pushing the over-MTU-sized packet back to controller, it's the controller
who will will decide whether to build ICMP message, or whatever routing behaviour
it may take. And sent it back with the port information. This ICMP message will
travel back to Guest.
Why does the flow has to use primitive like a "check size"? "check size"
will only take effect after do_output. I'm not very clear with this approach.
And not all scenario involving flow with routing behaviour, just set up a
vxlan tunnel, and attach KVM guest or Docker onto it for playing or developing.
This wouldn't necessarily require user to set additional specific flows to make
over-MTU-sized packet pass through the tunnel correctly. In such scenario, I
think the original patch in this thread to fragment tunnel packet is still needed
OR workout a generic component to build ICMP for all type tunnel in L2 level.
Both of those will act as a backup plan as there is no such specific flow as
default.
If I missed something obviously, please let me know.
--
No zuo no die but I have to try.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
^ permalink raw reply
* Re: [PATCH v3 1/4] can: kvaser_usb: Don't free packets when tight on URBs
From: Marc Kleine-Budde @ 2015-01-08 9:59 UTC (permalink / raw)
To: Ahmed S. Darwish, Olivier Sobrie, Oliver Hartkopp,
Wolfgang Grandegger
Cc: David S. Miller, Paul Gortmaker, Linux-CAN, netdev, LKML
In-Reply-To: <20150105174910.GA6304@linux>
[-- Attachment #1: Type: text/plain, Size: 1240 bytes --]
On 01/05/2015 06:49 PM, Ahmed S. Darwish wrote:
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>
> Flooding the Kvaser CAN to USB dongle with multiple reads and
> writes in high frequency caused seemingly-random panics in the
> kernel.
>
> On further inspection, it seems the driver erroneously freed the
> to-be-transmitted packet upon getting tight on URBs and returning
> NETDEV_TX_BUSY, leading to invalid memory writes and double frees
> at a later point in time.
>
> Note:
>
> Finding no more URBs/transmit-contexts and returning NETDEV_TX_BUSY
> is a driver bug in and out of itself: it means that our start/stop
> queue flow control is broken.
>
> This patch only fixes the (buggy) error handling code; the root
> cause shall be fixed in a later commit.
>
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> Acked-by: Olivier Sobrie <olivier@sobrie.be>
Applied 1-3 to can/master + added stable on Cc.
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
* Re: [PATCH 6/6] openvswitch: Support VXLAN Group Policy extension
From: Thomas Graf @ 2015-01-08 10:22 UTC (permalink / raw)
To: Jesse Gross
Cc: David Miller, Stephen Hemminger, Pravin Shelar, netdev,
dev@openvswitch.org
In-Reply-To: <CAEP_g=_t_KMUdit7WUSg+OORh=FoQtzLyygaVMftyDV-tAxJvA@mail.gmail.com>
On 01/07/15 at 05:18pm, Jesse Gross wrote:
> On Wed, Jan 7, 2015 at 3:01 PM, Thomas Graf <tgraf@suug.ch> wrote:
> > The encoding will be based on struct ovs_vxlan_opts which is extended
> > as needed by appending new members to the end of the struct. Parsers
> > will look at the provided length to see which fields are provided.
>
> But this means that if there are two extensions that are conflicting
> or if one is retired you still need to carry the earlier members of
> the struct. Why not make them real netlink attributes?
I figured that due to the limited space available in the VXLAN header
the structure would never grow big. I have no problem converting this
to use Netlink attributes internally though. Will address this in v2.
^ permalink raw reply
* Re: REGRESSION in nfnetlink on 3.18.x (bisected)
From: Pablo Neira Ayuso @ 2015-01-08 10:47 UTC (permalink / raw)
To: Andre Tomt; +Cc: netfilter-devel, netdev
In-Reply-To: <54ADAD1E.3060101@tomt.net>
On Wed, Jan 07, 2015 at 11:03:10PM +0100, Andre Tomt wrote:
> On 23. des. 2014 00:23, Andre Tomt wrote:
> >On 22. des. 2014 12:56, Pablo Neira Ayuso wrote:
> >>Could you give a test to this patch? Thanks.
> >>
> >
> >Initial testing looks good with this patch applied on top of 3.18.1
> >I will give it a spin on some more systems tomorrow.
>
> No news is good news :-)
>
> ~10 3.18.x systems in various roles have had this fix for two weeks
> with no issues.
Thanks a lot for testing. I already sent the patch to David:
http://patchwork.ozlabs.org/patch/426205/
^ permalink raw reply
* [PATCH net 2/2] gen_stats.c: Fix comment above gnet_stats_start_copy()
From: Ignacy Gawędzki @ 2015-01-08 10:35 UTC (permalink / raw)
To: netdev
The original comment was obviously a failed copy/paste.
Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
---
net/core/gen_stats.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index 5770a0e..1b25d80 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -77,7 +77,7 @@ gnet_stats_start_copy_compat(struct sk_buff *skb, int type, int tc_stats_type,
EXPORT_SYMBOL(gnet_stats_start_copy_compat);
/**
- * gnet_stats_start_copy_compat - start dumping procedure in compatibility mode
+ * gnet_stats_start_copy - start dumping procedure
* @skb: socket buffer to put statistics TLVs into
* @type: TLV type for top level statistic TLV
* @lock: statistics lock
--
1.9.1
^ permalink raw reply related
* [PATCH net 1/2] gen_stats.c: Duplicate xstats buffer for later use
From: Ignacy Gawędzki @ 2015-01-08 10:35 UTC (permalink / raw)
To: netdev
The gnet_stats_copy_app() function gets called, more often than not, with its
second argument a pointer to an automatic variable in the caller's stack.
Therefore, to avoid copying garbage afterwards when calling
gnet_stats_finish_copy(), this data is better copied to a dynamically allocated
memory that gets freed after use.
Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
---
net/core/gen_stats.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index 0c08062..5770a0e 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -305,7 +305,10 @@ int
gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
{
if (d->compat_xstats) {
- d->xstats = st;
+ d->xstats = kmalloc(len, GFP_KERNEL);
+ if (!d->xstats)
+ goto kmalloc_failure;
+ memcpy(d->xstats, st, len);
d->xstats_len = len;
}
@@ -313,6 +316,9 @@ gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
return gnet_stats_copy(d, TCA_STATS_APP, st, len);
return 0;
+kmalloc_failure:
+ spin_unlock_bh(d->lock);
+ return -1;
}
EXPORT_SYMBOL(gnet_stats_copy_app);
@@ -340,8 +346,13 @@ gnet_stats_finish_copy(struct gnet_dump *d)
return -1;
if (d->compat_xstats && d->xstats) {
- if (gnet_stats_copy(d, d->compat_xstats, d->xstats,
- d->xstats_len) < 0)
+ int result = gnet_stats_copy(d, d->compat_xstats,
+ d->xstats, d->xstats_len);
+ kfree(d->xstats);
+ d->xstats = NULL;
+ d->xstats_len = 0;
+
+ if (result < 0)
return -1;
}
--
1.9.1
^ permalink raw reply related
* [patch -next] net: eth: xgene: devm_ioremap() returns NULL on error
From: Dan Carpenter @ 2015-01-08 10:52 UTC (permalink / raw)
To: Iyappan Subramanian, Feng Kan
Cc: Keyur Chudgar, Grant Likely, Rob Herring, netdev, kernel-janitors
devm_ioremap() returns NULL on failure, it doesn't return an ERR_PTR.
Fixes: de7b5b3d790a ('net: eth: xgene: change APM X-Gene SoC platform ethernet to support ACPI')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
index 1e56bf3..02add38 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
@@ -804,9 +804,9 @@ static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata)
return -ENODEV;
}
pdata->base_addr = devm_ioremap(dev, res->start, resource_size(res));
- if (IS_ERR(pdata->base_addr)) {
+ if (!pdata->base_addr) {
dev_err(dev, "Unable to retrieve ENET Port CSR region\n");
- return PTR_ERR(pdata->base_addr);
+ return -ENOMEM;
}
res = platform_get_resource(pdev, IORESOURCE_MEM, RES_RING_CSR);
@@ -816,9 +816,9 @@ static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata)
}
pdata->ring_csr_addr = devm_ioremap(dev, res->start,
resource_size(res));
- if (IS_ERR(pdata->ring_csr_addr)) {
+ if (!pdata->ring_csr_addr) {
dev_err(dev, "Unable to retrieve ENET Ring CSR region\n");
- return PTR_ERR(pdata->ring_csr_addr);
+ return -ENOMEM;
}
res = platform_get_resource(pdev, IORESOURCE_MEM, RES_RING_CMD);
@@ -828,9 +828,9 @@ static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata)
}
pdata->ring_cmd_addr = devm_ioremap(dev, res->start,
resource_size(res));
- if (IS_ERR(pdata->ring_cmd_addr)) {
+ if (!pdata->ring_cmd_addr) {
dev_err(dev, "Unable to retrieve ENET Ring command region\n");
- return PTR_ERR(pdata->ring_cmd_addr);
+ return -ENOMEM;
}
ret = platform_get_irq(pdev, 0);
^ permalink raw reply related
* Re: [PATCH] net/at91_ether: prepare and unprepare clock
From: Nicolas Ferre @ 2015-01-08 10:55 UTC (permalink / raw)
To: Alexandre Belloni, David S. Miller, Cyrille Pitchen
Cc: Boris Brezillon, linux-arm-kernel, linux-kernel, netdev
In-Reply-To: <1420671566-30784-1-git-send-email-alexandre.belloni@free-electrons.com>
Le 07/01/2015 23:59, Alexandre Belloni a écrit :
> The clock is enabled without being prepared, this leads to:
>
> WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:889 __clk_enable+0x24/0xa8()
>
> and a non working ethernet interface.
>
> Use clk_prepare_enable() and clk_disable_unprepare() to handle the clock.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> drivers/net/ethernet/cadence/at91_ether.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/at91_ether.c b/drivers/net/ethernet/cadence/at91_ether.c
> index 55eb7f2af2b4..7ef55f5fa664 100644
> --- a/drivers/net/ethernet/cadence/at91_ether.c
> +++ b/drivers/net/ethernet/cadence/at91_ether.c
> @@ -340,7 +340,7 @@ static int __init at91ether_probe(struct platform_device *pdev)
> res = PTR_ERR(lp->pclk);
> goto err_free_dev;
> }
> - clk_enable(lp->pclk);
> + clk_prepare_enable(lp->pclk);
>
> lp->hclk = ERR_PTR(-ENOENT);
> lp->tx_clk = ERR_PTR(-ENOENT);
> @@ -406,7 +406,7 @@ static int __init at91ether_probe(struct platform_device *pdev)
> err_out_unregister_netdev:
> unregister_netdev(dev);
> err_disable_clock:
> - clk_disable(lp->pclk);
> + clk_disable_unprepare(lp->pclk);
> err_free_dev:
> free_netdev(dev);
> return res;
> @@ -424,7 +424,7 @@ static int at91ether_remove(struct platform_device *pdev)
> kfree(lp->mii_bus->irq);
> mdiobus_free(lp->mii_bus);
> unregister_netdev(dev);
> - clk_disable(lp->pclk);
> + clk_disable_unprepare(lp->pclk);
> free_netdev(dev);
>
> return 0;
> @@ -440,7 +440,7 @@ static int at91ether_suspend(struct platform_device *pdev, pm_message_t mesg)
> netif_stop_queue(net_dev);
> netif_device_detach(net_dev);
>
> - clk_disable(lp->pclk);
> + clk_disable_unprepare(lp->pclk);
> }
> return 0;
> }
> @@ -451,7 +451,7 @@ static int at91ether_resume(struct platform_device *pdev)
> struct macb *lp = netdev_priv(net_dev);
>
> if (netif_running(net_dev)) {
> - clk_enable(lp->pclk);
> + clk_prepare_enable(lp->pclk);
>
> netif_device_attach(net_dev);
> netif_start_queue(net_dev);
>
--
Nicolas Ferre
^ permalink raw reply
* RE: [PATCH net 1/2] gen_stats.c: Duplicate xstats buffer for later use
From: David Laight @ 2015-01-08 11:05 UTC (permalink / raw)
To: 'Ignacy Gawedzki', netdev@vger.kernel.org
In-Reply-To: <20150108103518.GA7214@zenon.in.qult.net>
From: Ignacy Gawedzki
> The gnet_stats_copy_app() function gets called, more often than not, with its
> second argument a pointer to an automatic variable in the caller's stack.
> Therefore, to avoid copying garbage afterwards when calling
> gnet_stats_finish_copy(), this data is better copied to a dynamically allocated
> memory that gets freed after use.
>
> Signed-off-by: Ignacy Gawedzki <ignacy.gawedzki@green-communications.fr>
> ---
> net/core/gen_stats.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
> index 0c08062..5770a0e 100644
> --- a/net/core/gen_stats.c
> +++ b/net/core/gen_stats.c
> @@ -305,7 +305,10 @@ int
> gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
> {
> if (d->compat_xstats) {
> - d->xstats = st;
> + d->xstats = kmalloc(len, GFP_KERNEL);
> + if (!d->xstats)
> + goto kmalloc_failure;
> + memcpy(d->xstats, st, len);
> d->xstats_len = len;
> }
>
> @@ -313,6 +316,9 @@ gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
> return gnet_stats_copy(d, TCA_STATS_APP, st, len);
>
> return 0;
> +kmalloc_failure:
> + spin_unlock_bh(d->lock);
> + return -1;
> }
This rather implies that you are calling kmalloc() with a spin lock help.
If this is valid at all then you should probably specify GPF_ATOMIC.
OTOH it is better to call kmalloc() before acquiring the lock.
The locking itself looks odd - since the corresponding spin_lock_bh()
isn't in the same function.
David
^ permalink raw reply
* Re: [PATCH] net: Prevent multiple NAPI instances co-existing in the list
From: Herbert Xu @ 2015-01-08 11:15 UTC (permalink / raw)
To: Dennis Chen; +Cc: netdev, davem, eric.dumazet, kernel.org.gnu
In-Reply-To: <CA+U0gVh+TyEUJ+qmg+FE=UhvvQywfNxcouCyv1sutZ3fav5FAw@mail.gmail.com>
Dennis Chen <kernel.org.gnu@gmail.com> wrote:
> Some drivers may clear the NAPI_STATE_SCHED bit upon the state of the
> NAPI instance after exhaust the budget in the poll function, which
> will open a window for next device interrupt handler to insert a same
> instance to the list after calling list_add_tail(&n->poll_list,
> repoll) if we don't set this bit.
>
> Signed-off-by: Dennis Chen <kernel.org.gnu@gmail.com>
Which driver is doing this?
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH v3 4/4] can: kvaser_usb: Add support for the Usbcan-II family
From: Marc Kleine-Budde @ 2015-01-08 11:53 UTC (permalink / raw)
To: Ahmed S. Darwish, Olivier Sobrie, Oliver Hartkopp,
Wolfgang Grandegger
Cc: David S. Miller, Paul Gortmaker, Linux-CAN, netdev, LKML
In-Reply-To: <20150105183131.GD6304@linux>
[-- Attachment #1: Type: text/plain, Size: 36539 bytes --]
On 01/05/2015 07:31 PM, Ahmed S. Darwish wrote:
> 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 | 618 +++++++++++++++++++++++++++++++--------
> 2 files changed, 503 insertions(+), 123 deletions(-)
>
> ** 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
>
> ** 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.)
>
> 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 cc7bfc0..43b3928 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
> @@ -55,6 +67,16 @@
> #define USB_CAN_R_PRODUCT_ID 39
> #define USB_LEAF_LITE_V2_PRODUCT_ID 288
> #define USB_MINI_PCIE_HS_PRODUCT_ID 289
> +#define LEAF_PRODUCT_ID(id) (id >= USB_LEAF_DEVEL_PRODUCT_ID && \
> + id <= USB_MINI_PCIE_HS_PRODUCT_ID)
Can you please convert both *_PRODUCT_ID() macros into static inline
bool functions.
> +
> +/* 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
> +#define USBCAN_PRODUCT_ID(id) (id >= USB_USBCAN_REVB_PRODUCT_ID && \
> + id <= USB_MEMORATOR_PRODUCT_ID)
>
> /* USB devices features */
> #define KVASER_HAS_SILENT_MODE BIT(0)
> @@ -73,7 +95,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 +120,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 +136,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 +150,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 +174,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 +185,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 +250,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 {
> +struct kvaser_msg_rx_can_header {
> u8 channel;
> u8 flag;
> +} __packed;
> +
> +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 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 {
> +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 +341,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 +367,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 +384,50 @@ 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;
>
> +/* Leaf/USBCAN-agnostic summary of an error event.
> + * No M16C error factors for USBCAN-based devices.
> + */
> +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 +443,7 @@ struct kvaser_usb {
>
> u32 fw_version;
> unsigned int nchannels;
> + enum kvaser_usb_family family;
>
> bool rxinitdone;
> void *rxbuf[MAX_RX_URBS];
> @@ -311,6 +467,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 +517,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 +631,18 @@ 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;
> + default:
> + dev_err(dev->udev->dev.parent,
> + "Invalid device family (%d)\n", dev->family);
> + return -EINVAL;
The default case should not happen. I think you can remove it.
> + }
>
> return 0;
> }
> @@ -484,6 +663,9 @@ static int kvaser_usb_get_card_info(struct kvaser_usb *dev)
> dev->nchannels = msg.u.cardinfo.nchannels;
> if (dev->nchannels > MAX_NET_DEVICES)
> return -EINVAL;
> + if (dev->family == KVASER_USBCAN &&
> + dev->nchannels > MAX_USBCAN_NET_DEVICES)
> + return -EINVAL;
Nitpick, as the new "if" also does a test on nchannels, why no extend
the existing "if" with an "||"?
>
> return 0;
> }
> @@ -496,8 +678,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,
> @@ -615,37 +799,80 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
> priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> }
>
> -static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> - const struct kvaser_msg *msg)
> +static void kvaser_report_error_event(const struct kvaser_usb *dev,
> + struct kvaser_error_summary *es);
Please rearange your code that forward declarations are not needed (if
possible - I haven't checked, though).
> +
> +/* Report error to userspace iff the controller's errors counter has
> + * increased, or we're the only channel seeing the bus error state.
> + *
> + * As reported by USBCAN sheets, "the CAN controller has difficulties
> + * to tell whether an error frame arrived on channel 1 or on channel 2."
> + * Thus, error counters are compared with their earlier values to
> + * determine which channel was responsible for the error event.
Your code doesn't match this comment. You compare the error counters
against the old values to tell if it's a rx or tx error, the channel
information from the struct kvaser_error_summary is used directly.
> + */
> +static void usbcan_report_error_if_applicable(const struct kvaser_usb *dev,
> + struct kvaser_error_summary *es)
Nitpick: can you please add a "kvaser_" prefix to the all usbcan_* and
leaf_* functions.
> {
> - 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;
> + int old_tx_err_count, old_rx_err_count, channel, report_error;
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];
> + old_tx_err_count = priv->bec.txerr;
> + old_rx_err_count = priv->bec.rxerr;
Why do you make a copy of priv->bec, AFAICS you can use
priv->bec.{r,t}xerr directly?
> +
> + report_error = 0;
> + if (es->txerr > old_tx_err_count) {
> + es->usbcan.error_state |= USBCAN_ERROR_STATE_TX_ERROR;
> + report_error = 1;
> + }
> + if (es->rxerr > old_rx_err_count) {
> + es->usbcan.error_state |= USBCAN_ERROR_STATE_RX_ERROR;
> + report_error = 1;
> + }
> + 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 = 1;
> + }
> +
> + if (report_error)
> + kvaser_report_error_event(dev, es);
> +}
> +
> +/* Extract error summary from a Leaf-based device error message */
> +static void leaf_extract_error_from_msg(const struct kvaser_usb *dev,
> + const struct kvaser_msg *msg)
> +{
> + struct kvaser_error_summary es = { 0, };
IIRC "es = { };" should be sufficient.
>
> 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;
> + 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_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];
> + 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:
> - 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;
> + 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",
> @@ -653,16 +880,92 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> return;
> }
>
> - if (channel >= dev->nchannels) {
> + kvaser_report_error_event(dev, &es);
> +}
> +
> +/* Extract error summary from a USBCANII-based device error message */
> +static void usbcan_extract_error_from_msg(const struct kvaser_usb *dev,
> + const struct kvaser_msg *msg)
> +{
> + struct kvaser_error_summary es = { 0, };
same here.
> +
> + 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;
> + usbcan_report_error_if_applicable(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;
> + usbcan_report_error_if_applicable(dev, &es);
> +
> + /* For error events, the USBCAN firmware does not support
> + * more than 2 channels: ch0, and ch1.
> + */
> + if (dev->nchannels > 1) {
> + es.channel = 1;
Why is channel == 1 if the device has more than 1 channel?
> + 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;
> + usbcan_report_error_if_applicable(dev, &es);
> + }
> + break;
> +
> + default:
> + dev_err(dev->udev->dev.parent, "Invalid msg id (%d)\n",
> + msg->id);
> + }
> +}
> +
> +static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> + const struct kvaser_msg *msg)
> +{
> + switch (dev->family) {
> + case KVASER_LEAF:
> + leaf_extract_error_from_msg(dev, msg);
> + break;
> + case KVASER_USBCAN:
> + usbcan_extract_error_from_msg(dev, msg);
> + break;
> + default:
should not happen.
> dev_err(dev->udev->dev.parent,
> - "Invalid channel number (%d)\n", channel);
> + "Invalid device family (%d)\n", dev->family);
> return;
> }
> +}
>
> - priv = dev->nets[channel];
> +static void kvaser_report_error_event(const struct kvaser_usb *dev,
> + 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;
> +
> + if (es->channel >= dev->nchannels) {
> + dev_err(dev->udev->dev.parent,
> + "Invalid channel number (%d)\n", es->channel);
> + return;
> + }
> +
> + 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 +978,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 +990,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 +1006,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 +1024,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 +1040,52 @@ 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;
> + default:
> + dev_err(dev->udev->dev.parent,
> + "Invalid device family (%d)\n", dev->family);
> + goto err;
should not happen.
> }
>
> - 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;
>
> @@ -774,6 +1093,11 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
>
> stats->rx_packets++;
> stats->rx_bytes += cf->can_dlc;
> +
> + return;
> +
> +err:
> + dev_kfree_skb(skb);
> }
>
> static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
> @@ -783,16 +1107,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) {
> skb = alloc_can_err_skb(priv->netdev, &cf);
> if (!skb) {
> stats->rx_dropped++;
> return;
> }
Can you prepare a (seperate) patch that does the stats, even in case of OOM here. Same for kvaser_report_error_event()
>
> cf->can_id |= CAN_ERR_CRTL;
> cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>
> stats->rx_over_errors++;
> stats->rx_errors++;
>
> netif_rx(skb);
>
> stats->rx_packets++;
> stats->rx_bytes += cf->can_dlc;
Another patch would be not to touch cf after netif_rx(), please move the stats handling before calling netif_rx(). Same applies to the kvaser_usb_rx_can_msg() function.
> }
> @@ -819,7 +1143,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,
> @@ -830,19 +1155,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)) {
> + if ((msg->u.rx_can_header.flag & MSG_FLAG_ERROR_FRAME) &&
> + (dev->family == KVASER_LEAF && msg->id == CMD_LEAF_LOG_MESSAGE)) {
> kvaser_usb_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;
> }
>
> @@ -852,38 +1190,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);
> }
>
> @@ -947,7 +1284,12 @@ 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;
>
> @@ -960,8 +1302,14 @@ static void kvaser_usb_handle_message(const struct kvaser_usb *dev,
> 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;
> }
> @@ -1181,7 +1529,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)
> @@ -1289,6 +1637,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;
> @@ -1310,9 +1659,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;
> @@ -1330,7 +1693,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) {
> @@ -1599,6 +1962,17 @@ static int kvaser_usb_probe(struct usb_interface *intf,
> if (!dev)
> return -ENOMEM;
>
> + if (LEAF_PRODUCT_ID(id->idProduct)) {
> + dev->family = KVASER_LEAF;
> + } else if (USBCAN_PRODUCT_ID(id->idProduct)) {
> + 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)");
regards,
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
* Re: [PATCH v2] sh_eth: Fix access to TRSCER register
From: Geert Uytterhoeven @ 2015-01-08 11:57 UTC (permalink / raw)
To: Nobuhiro Iwamatsu
Cc: netdev@vger.kernel.org, Yoshihiro Shimoda, Linux-sh list
On Thu, Jan 8, 2015 at 7:25 AM, Nobuhiro Iwamatsu
<nobuhiro.iwamatsu.yj@renesas.com> wrote:
> TRSCER register is configured differently by SoCs. TRSCER of R-Car Gen2 is
> RINT8 bit only valid, other bits are reserved bits. This removes access to
> TRSCER register reserve bit by adding variable trscer_err_mask to
> sh_eth_cpu_data structure, set the register information to each SoCs.
>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> (on r8a7791)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH net 1/2] gen_stats.c: Duplicate xstats buffer for later use
From: 'Ignacy Gawedzki' @ 2015-01-08 12:02 UTC (permalink / raw)
To: David Laight; +Cc: netdev@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1CAC353D@AcuExch.aculab.com>
On Thu, Jan 08, 2015 at 11:05:24AM +0000, thus spake David Laight:
> This rather implies that you are calling kmalloc() with a spin lock help.
> If this is valid at all then you should probably specify GPF_ATOMIC.
Yes, you're right, my mistake.
> OTOH it is better to call kmalloc() before acquiring the lock.
This would certainly be the best solution, but still, it looks pretty hard to
implement this way since the whole sequence of functions is called from
tc_fill_qdisc() and tc_fill_tclass() in net/sched/sch_api.c: first a call to
gnet_stats_start_copy_compat() acquires the lock, then a call to per-qdisc
dump_stats() is performed that itself calls gnet_stats_copy_app() with the
pointer to the automatic structure that needs to be duplicated and finally
gnet_stats_finish_copy() is called that eventually releases the lock. In the
originaly code, only gnet_stats_copy() can cause a failure and so the release
of the lock is performed there in such a case.
I don't see any easy way of knowing how much to allocate *before* the call to
gnet_stats_start_copy_compat().
> The locking itself looks odd - since the corresponding spin_lock_bh()
> isn't in the same function.
I agree that this doesn't look too good.
For the moment I'll post a corrected patch that uses GPF_ATOMIC. Then anyone
can come up with a better fix anytime.
Ignacy
--
Ignacy Gawędzki
R&D Engineer
Green Communications
^ permalink raw reply
* [PATCH net-next v2] tcp: avoid reducing cwnd when ACK+DSACK is received
From: Sébastien Barré @ 2015-01-08 12:20 UTC (permalink / raw)
To: David Miller
Cc: Sébastien Barré, netdev, Gregory Detal,
Nandita Dukkipati, Yuchung Cheng, Neal Cardwell
When the peer has delayed ack enabled, it may reply to a probe with an
ACK+D-SACK, with ack value set to tlp_high_seq. In the current code,
such ACK+DSACK will be missed and only at next, higher ack will the TLP
episode be considered done. Since the DSACK is not present anymore,
this will cost a cwnd reduction.
This patch ensures that this scenario does not cause a cwnd reduction, since
receiving an ACK+DSACK indicates that both the initial segment and the probe
have been received by the peer.
Cc: Gregory Detal <gregory.detal@uclouvain.be>
Cc: Nandita Dukkipati <nanditad@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Signed-off-by: Sébastien Barré <sebastien.barre@uclouvain.be>
---
Changes:
Applied Neal's comments:
-adapted commit first line
-moved logic to if condition, and removed is_tlp_dupack
Thanks Neal for those comments !
net/ipv4/tcp_input.c | 30 +++++++++++++-----------------
1 file changed, 13 insertions(+), 17 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 075ab4d..cf63a29 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3363,29 +3363,25 @@ static void tcp_replace_ts_recent(struct tcp_sock *tp, u32 seq)
static void tcp_process_tlp_ack(struct sock *sk, u32 ack, int flag)
{
struct tcp_sock *tp = tcp_sk(sk);
- bool is_tlp_dupack = (ack == tp->tlp_high_seq) &&
- !(flag & (FLAG_SND_UNA_ADVANCED |
- FLAG_NOT_DUP | FLAG_DATA_SACKED));
/* Mark the end of TLP episode on receiving TLP dupack or when
* ack is after tlp_high_seq.
+ * With delayed acks, we may also get a regular ACK+DSACK, in which
+ * case we don't want to reduce the cwnd either.
*/
- if (is_tlp_dupack) {
+ if (((ack == tp->tlp_high_seq) &&
+ !(flag & (FLAG_SND_UNA_ADVANCED |
+ FLAG_NOT_DUP | FLAG_DATA_SACKED))) ||
+ (!before(ack, tp->tlp_high_seq) && (flag & FLAG_DSACKING_ACK))) {
tp->tlp_high_seq = 0;
- return;
- }
-
- if (after(ack, tp->tlp_high_seq)) {
+ } else if (after(ack, tp->tlp_high_seq)) {
tp->tlp_high_seq = 0;
- /* Don't reduce cwnd if DSACK arrives for TLP retrans. */
- if (!(flag & FLAG_DSACKING_ACK)) {
- tcp_init_cwnd_reduction(sk);
- tcp_set_ca_state(sk, TCP_CA_CWR);
- tcp_end_cwnd_reduction(sk);
- tcp_try_keep_open(sk);
- NET_INC_STATS_BH(sock_net(sk),
- LINUX_MIB_TCPLOSSPROBERECOVERY);
- }
+ tcp_init_cwnd_reduction(sk);
+ tcp_set_ca_state(sk, TCP_CA_CWR);
+ tcp_end_cwnd_reduction(sk);
+ tcp_try_keep_open(sk);
+ NET_INC_STATS_BH(sock_net(sk),
+ LINUX_MIB_TCPLOSSPROBERECOVERY);
}
}
--
tg: (44d84d7..) net-next/tlp-dsack-handling (depends on: net-next/master)
^ permalink raw reply related
* RE: [PATCH net 1/2] gen_stats.c: Duplicate xstats buffer for later use
From: David Laight @ 2015-01-08 12:26 UTC (permalink / raw)
To: 'Ignacy Gawedzki', netdev@vger.kernel.org
In-Reply-To: <20150108103518.GA7214@zenon.in.qult.net>
From: Ignacy Gawedzki
> The gnet_stats_copy_app() function gets called, more often than not, with its
> second argument a pointer to an automatic variable in the caller's stack.
> Therefore, to avoid copying garbage afterwards when calling
> gnet_stats_finish_copy(), this data is better copied to a dynamically allocated
> memory that gets freed after use.
>
> Signed-off-by: Ignacy Gawedzki <ignacy.gawedzki@green-communications.fr>
> ---
> net/core/gen_stats.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
> index 0c08062..5770a0e 100644
> --- a/net/core/gen_stats.c
> +++ b/net/core/gen_stats.c
> @@ -305,7 +305,10 @@ int
> gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
> {
> if (d->compat_xstats) {
> - d->xstats = st;
> + d->xstats = kmalloc(len, GFP_KERNEL);
> + if (!d->xstats)
> + goto kmalloc_failure;
> + memcpy(d->xstats, st, len);
> d->xstats_len = len;
Looking at this again, isn't the purpose of the 'void *st' to pass
down a temporary buffer that is 'big enough' ?
David
^ permalink raw reply
* [PATCH v3] ath10k: fixup wait_for_completion_timeout return handling
From: Nicholas Mc Guire @ 2015-01-08 12:27 UTC (permalink / raw)
To: Kalle Valo
Cc: Michal Kazior, Ben Greear, Chun-Yeow Yeoh, Yanbo Li,
Sergei Shtylyov, ath10k-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Nicholas Mc Guire
wait_for_completion_timeout does not return negative values so the tests
for <= 0 are not needed and the case differentiation in the error handling
path unnecessary.
v2: all wait_for_completion_timeout changes in a single patch
v3: patch formatting cleanups - no change of actual patch
Signed-off-by: Nicholas Mc Guire <der.herr-kA1LtwSENNE@public.gmane.org>
---
patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
CONFIG_ATH10K=m
patch is against linux-next 3.19.0-rc1 -next-20141226
None of the proposed cleanups are critical.
All changes should only be removing unreachable cases.
drivers/net/wireless/ath/ath10k/debug.c | 2 +-
drivers/net/wireless/ath/ath10k/htc.c | 6 ++----
drivers/net/wireless/ath/ath10k/htt.c | 2 +-
drivers/net/wireless/ath/ath10k/mac.c | 2 +-
4 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index a716758..7e1fe93 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -373,7 +373,7 @@ static int ath10k_debug_fw_stats_request(struct ath10k *ar)
ret = wait_for_completion_timeout(&ar->debug.fw_stats_complete,
1*HZ);
- if (ret <= 0)
+ if (ret == 0)
return -ETIMEDOUT;
spin_lock_bh(&ar->data_lock);
diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index f1946a6..2fd9e18 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -703,11 +703,9 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
/* wait for response */
status = wait_for_completion_timeout(&htc->ctl_resp,
ATH10K_HTC_CONN_SVC_TIMEOUT_HZ);
- if (status <= 0) {
- if (status == 0)
- status = -ETIMEDOUT;
+ if (status == 0) {
ath10k_err(ar, "Service connect timeout: %d\n", status);
- return status;
+ return -ETIMEDOUT;
}
/* we controlled the buffer creation, it's aligned */
diff --git a/drivers/net/wireless/ath/ath10k/htt.c b/drivers/net/wireless/ath/ath10k/htt.c
index 56cb4ac..58b6fc1 100644
--- a/drivers/net/wireless/ath/ath10k/htt.c
+++ b/drivers/net/wireless/ath/ath10k/htt.c
@@ -102,7 +102,7 @@ int ath10k_htt_setup(struct ath10k_htt *htt)
status = wait_for_completion_timeout(&htt->target_version_received,
HTT_TARGET_VERSION_TIMEOUT_HZ);
- if (status <= 0) {
+ if (status == 0) {
ath10k_warn(ar, "htt version request timed out\n");
return -ETIMEDOUT;
}
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index c400567..f9d7dbb 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2151,7 +2151,7 @@ void ath10k_offchan_tx_work(struct work_struct *work)
ret = wait_for_completion_timeout(&ar->offchan_tx_completed,
3 * HZ);
- if (ret <= 0)
+ if (ret == 0)
ath10k_warn(ar, "timed out waiting for offchannel skb %p\n",
skb);
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH net 1/2] gen_stats.c: Duplicate xstats buffer for later use
From: 'Ignacy Gawedzki' @ 2015-01-08 12:32 UTC (permalink / raw)
To: David Laight; +Cc: netdev@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1CAC359A@AcuExch.aculab.com>
On Thu, Jan 08, 2015 at 12:26:17PM +0000, thus spake David Laight:
> Looking at this again, isn't the purpose of the 'void *st' to pass
> down a temporary buffer that is 'big enough' ?
The buffer is large enough at the time of the call, yes. But the thing is
that in most of the actual callers, it is an automatic variable in the
caller's frame, while the buffer is actually used at a later point when
gnet_stats_finish_copy() is called (when that frame doesn't exist anymore).
I'm about to send a revised version of the patch that also corrects a few
shortcomings in the management of the dynamically allocated buffer w.r.t. the
acquired lock.
Ignacy
--
Ignacy Gawędzki
R&D Engineer
Green Communications
^ permalink raw reply
* [PATCH net v2] gen_stats.c: Duplicate xstats buffer for later use
From: Ignacy Gawędzki @ 2015-01-08 12:33 UTC (permalink / raw)
To: netdev
The gnet_stats_copy_app() function gets called, more often than not, with its
second argument a pointer to an automatic variable in the caller's stack.
Therefore, to avoid copying garbage afterwards when calling
gnet_stats_finish_copy(), this data is better copied to a dynamically allocated
memory that gets freed after use.
Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
---
net/core/gen_stats.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index 0c08062..5ad2fe7 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -32,6 +32,9 @@ gnet_stats_copy(struct gnet_dump *d, int type, void *buf, int size)
return 0;
nla_put_failure:
+ kfree(d->xstats);
+ d->xstats = NULL;
+ d->xstats_len = 0;
spin_unlock_bh(d->lock);
return -1;
}
@@ -305,7 +308,11 @@ int
gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
{
if (d->compat_xstats) {
- d->xstats = st;
+ kfree(d->xstats);
+ d->xstats = kmalloc(len, GFP_ATOMIC);
+ if (!d->xstats)
+ goto kmalloc_failure;
+ memcpy(d->xstats, st, len);
d->xstats_len = len;
}
@@ -313,6 +320,10 @@ gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
return gnet_stats_copy(d, TCA_STATS_APP, st, len);
return 0;
+kmalloc_failure:
+ d->xstats_len = 0;
+ spin_unlock_bh(d->lock);
+ return -1;
}
EXPORT_SYMBOL(gnet_stats_copy_app);
@@ -345,6 +356,9 @@ gnet_stats_finish_copy(struct gnet_dump *d)
return -1;
}
+ kfree(d->xstats);
+ d->xstats = NULL;
+ d->xstats_len = 0;
spin_unlock_bh(d->lock);
return 0;
}
--
1.9.1
^ permalink raw reply related
* bridge-utils : bridge fdb replace undocumented
From: Mathieu Rohon @ 2015-01-08 12:52 UTC (permalink / raw)
To: netdev
Hi,
the command :
#bridge fdb replace
can be useful to replace a learned Mac address by a static one. We'd
like to use this command to solve an openstack bug :
https://bugs.launchpad.net/neutron/+bug/1367999
However it is not documented in the manpage of bridge-utilis version 1.5.9.
Also, I don't know what is the minimum version to have this features.
regards,
Mathieu
^ 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