* [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
* Re: [PATCH net-next 1/2] Revert "ARM: imx: add FEC sleep mode callback function"
From: Shawn Guo @ 2015-01-08 12:53 UTC (permalink / raw)
To: Fabio Estevam; +Cc: davem, fugang.duan, netdev, Fabio Estevam
In-Reply-To: <1420634393-30027-1-git-send-email-festevam@gmail.com>
On Wed, Jan 07, 2015 at 10:39:52AM -0200, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> i.MX platform maintainer Shawn Guo is not happy with the such commit as
> explained below [1]:
>
> "The GPR difference between SoCs can be encoded in device tree as well.
> It's pointless to repeat the same code pattern for every single
> platform, that need to set up GPR bits for enabling magic packet wake
> up, while the only difference is the register and bit offset.
>
> The platform code will become quite messy and unmaintainable if every
> device driver dump their GPR register setup code into platform.
>
> Sorry, but it's NACK from me."
>
> This reverts commit 456062b3ec6f5b9 ("ARM: imx: add FEC sleep mode callback
> function").
>
> [1] http://www.spinics.net/lists/netdev/msg310922.html
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
Thanks, Fabio. For both patches,
Acked-by: Shawn Guo <shawn.guo@linaro.org>
^ permalink raw reply
* Re: [PATCH net-next] mac80211: silent build warnings
From: Sergei Shtylyov @ 2015-01-08 12:59 UTC (permalink / raw)
To: Ying Xue, johannes-cdvu00un1VgdHxzADdlk8Q
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1420700655-9427-1-git-send-email-ying.xue-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
Hello.
On 1/8/2015 10:04 AM, Ying Xue wrote:
> Silent the following build warnings:
> net/mac80211/mlme.c: In function ‘ieee80211_rx_mgmt_beacon’:
> net/mac80211/mlme.c:1348:3: warning: ‘pwr_level_cisco’ may be used uninitialized in this function [-Wuninitialized]
> net/mac80211/mlme.c:1315:6: note: ‘pwr_level_cisco’ was declared here
> Signed-off-by: Ying Xue <ying.xue-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
> ---
> net/mac80211/mlme.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index 2c36c47..13b5506 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -1312,7 +1312,7 @@ static u32 ieee80211_handle_pwr_constr(struct ieee80211_sub_if_data *sdata,
> {
> bool has_80211h_pwr = false, has_cisco_pwr = false;
> int chan_pwr = 0, pwr_reduction_80211h = 0;
> - int pwr_level_cisco, pwr_level_80211h;
> + int pwr_level_cisco = 0, pwr_level_80211h = 0;
OK, but why are you also initializing the second variable?
WBR, Sergei
--
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
* Re: [PATCH net-next 1/3] net: add IPv4 routing FIB support for swdev
From: Hannes Frederic Sowa @ 2015-01-08 13:03 UTC (permalink / raw)
To: Shrijeet Mukherjee
Cc: Scott Feldman, Netdev, Jiří Pírko, john fastabend,
Thomas Graf, Jamal Hadi Salim, Andy Gospodarek, Roopa Prabhu
In-Reply-To: <1b5a5a02b187fde4c2491ce757cb0045@mail.gmail.com>
Hi,
On Mi, 2015-01-07 at 09:54 -0800, Shrijeet Mukherjee wrote:
> >
> >I could come up with several ways how to model hardware. Depending on that
> >the integration with rules is easy or nearly impossible:
> >
> >1) it simply cannot deal with ip rules, so there is no way an ACL can
> >influence the
> >outcome of a routing table lookup - if the feature should be used, it has
> >to use
> >the slow-path in the kernel.
>
> As Scott was saying, most hardware has table id's and the ability to
> identify and prioritize that way.
I saw Scott only talking about Rocker - maybe I missed it?
> >2) ACLs can influence which routing table will get queried - this sounds
> >very much
> >like the ip rule model and it seems not too hard to model that.
>
> This clearly can be made to work .. the problem is really the space of
> policy routing (i.e jump across VRF's incase of a lookup failure) when
> combined with the space of ip rule flexibility.
This very much depends on the hardware, I guess. The complexity is
increased by the routing offloading knowing about the ACL datastructures
and vice versa.
> >3) Routing implementations in the hardware have a single routing table and
> >the
> >leafs carry different actions with priorities: making this kind of model
> >working
> >with the ip rule concept will become very difficult and it might require
> >lots of
> >algorithmic code by every driver to adapt to a single API provided by
> >Linux. It
> >might be possible, if the hardware provides actions like backtrack and
> >retrack and
> >can keep state of priorities during walking the tree, I really doubt that.
>
>
> In the short term .. this maybe a good way to go but with a simplication.
> Some tables are offloaded and the rest at the full table level is in
> software. Finally then you can put a "default route" in the hardware table
> to punt to cpu and then keep the software model clever and the hardware
> model fast ?
Yes, the algorithm I described in my prior mail implicitly does that, we
can extend it bit by bit as new hardware supports more filter features.
Especially the default configuration with only the RT_TABLE_LOCAL and
RT_TABLE_MAIN allows complete offloading, which should be desirable.
To deal with the RT_TABLE_LOCAL, we might walk the whole routing table
and verify that all routes have full prefix length (32 ipv4 or 128
ipv6).
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH] net: Prevent multiple NAPI instances co-existing in the list
From: Sergei Shtylyov @ 2015-01-08 13:04 UTC (permalink / raw)
To: Dennis Chen, netdev, Herbert Xu, Miller, Eric Dumazet
In-Reply-To: <CA+U0gVh+TyEUJ+qmg+FE=UhvvQywfNxcouCyv1sutZ3fav5FAw@mail.gmail.com>
Hello.
On 1/8/2015 11:22 AM, Dennis Chen 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>
> ---
> 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
s/exhaust/exhausting/.
> + * 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",
Likewise. And s/exit/exiting/.
WBR, Sergei
^ permalink raw reply
* Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments
From: Hannes Frederic Sowa @ 2015-01-08 13:11 UTC (permalink / raw)
To: Rahul Sharma; +Cc: Pablo Neira Ayuso, netdev, linux-kernel, netfilter-devel
In-Reply-To: <CAFB3abz_-AbJsrBtwRX5t=TCCfzYsw82WDYH=A7jzA2N8UbAng@mail.gmail.com>
On Do, 2015-01-08 at 02:18 +0530, Rahul Sharma wrote:
> Hi Hannes,
>
> On Wed, Jan 7, 2015 at 4:13 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > Hi,
> >
> > On Mi, 2015-01-07 at 11:11 +0530, Rahul Sharma wrote:
> >> On Wed, Jan 7, 2015 at 4:17 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> > On Wed, Jan 07, 2015 at 03:03:20AM +0530, Rahul Sharma wrote:
> >> >> ipv6_find_hdr() currently assumes that the next-header field in the
> >> >> fragment header of the non-first fragment is the "protocol number of
> >> >> the last header" (here last header excludes any extension header
> >> >> protocol numbers ) which is incorrect as per RFC2460. The next-header
> >> >> value is the first header of the fragmentable part of the original
> >> >> packet (which can be extension header as well).
> >> >> This can create reassembly problems. For example: Fragmented
> >> >> authenticated OSPFv3 packets (where AH header is inserted before the
> >> >> protocol header). For the second fragment, the next header value in
> >> >> the fragment header will be NEXTHDR_AUTH which is correct but
> >> >> ipv6_find_hdr will return ENOENT since AH is an extension header
> >> >> resulting in second fragment getting dropped. This check for the
> >> >> presence of non-extension header needs to be removed.
> >> >>
> >> >> Signed-off-by: Rahul Sharma <rsharma@arista.com>
> >> >> ---
> >> >> --- linux-3.18.1/net/ipv6/exthdrs_core.c.orig 2015-01-06
> >> >> 10:25:36.411419863 -0800
> >> >> +++ linux-3.18.1/net/ipv6/exthdrs_core.c 2015-01-06
> >> >> 10:51:45.819364986 -0800
> >> >> @@ -171,10 +171,11 @@ EXPORT_SYMBOL_GPL(ipv6_find_tlv);
> >> >> * If the first fragment doesn't contain the final protocol header or
> >> >> * NEXTHDR_NONE it is considered invalid.
> >> >> *
> >> >> - * Note that non-1st fragment is special case that "the protocol number
> >> >> - * of last header" is "next header" field in Fragment header. In this case,
> >> >> - * *offset is meaningless and fragment offset is stored in *fragoff if fragoff
> >> >> - * isn't NULL.
> >> >> + * Note that non-1st fragment is special case that "the protocol number of the
> >> >> + * first header of the fragmentable part of the original packet" is
> >> >> + * "next header" field in the Fragment header. In this case, *offset is
> >> >> + * meaningless and fragment offset is stored in *fragoff if fragoff isn't
> >> >> + * NULL.
> >> >> *
> >> >> * if flags is not NULL and it's a fragment, then the frag flag
> >> >> * IP6_FH_F_FRAG will be set. If it's an AH header, the
> >> >> @@ -250,9 +251,7 @@ int ipv6_find_hdr(const struct sk_buff *
> >> >>
> >> >> _frag_off = ntohs(*fp) & ~0x7;
> >> >> if (_frag_off) {
> >> >> - if (target < 0 &&
> >> >> - ((!ipv6_ext_hdr(hp->nexthdr)) ||
> >> >
> >> > This check assumes that the following headers cannot show up in the
> >> > fragmented part of the IPv6 packet:
> >> >
> >> > 12 bool ipv6_ext_hdr(u8 nexthdr)
> >> > 13 {
> >> > 14 /*
> >> > 15 * find out if nexthdr is an extension header or a protocol
> >> > 16 */
> >> > 17 return (nexthdr == NEXTHDR_HOP) ||
> >> > 18 (nexthdr == NEXTHDR_ROUTING) ||
> >> > 19 (nexthdr == NEXTHDR_FRAGMENT) ||
> >> > 20 (nexthdr == NEXTHDR_AUTH) ||
> >> > 21 (nexthdr == NEXTHDR_NONE) ||
> >> > 22 (nexthdr == NEXTHDR_DEST);
> >> >
> >> >> - hp->nexthdr == NEXTHDR_NONE)) {
> >> >> + if (target < 0) {
> >> >> if (fragoff)
> >> >> *fragoff = _frag_off;
> >> >> return hp->nexthdr;
> >> >> --
> >> >> To unsubscribe from this list: send the line "unsubscribe netdev" in
> >> >> the body of a message to majordomo@vger.kernel.org
> >> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>
> >> I think this is incorrect. Authentication header shows up in the
> >> fragmentable part of the original IPv6 packet. So, for the non-first
> >> fragments the next-header field value can be NEXTHDR_AUTH.
> >
> > Pablo's mail got me thinking again.
> >
> > In general, IPv6 extension headers can appear in any order and stacks
> > must be process them. Fragmentation adds a limitation, that some
> > extension headers do not make sense and don't have any effect if they
> > appear after a fragmentation header (HbH and ROUTING).
> >
> > Looking at the rest of the function we don't check for HBHHDR or RTHDR
> > following a fragmentation header either if we process the first fragment
> > (core stack only processes HBH if directly following the ipv6 header
> > anyway).
> >
> > So, in my opinion, it is safe to completely remove this check and it
> > would align if the rest of the extension processing logic. The callers
> > all seem fine with that.
> >
> > Pablo, what do you think?
> >
> > Anyway, the patch does not apply cleanly, the patch header is mangled.
> > Could you check and send again?
> >
> > Thanks,
> > Hannes
> >
> >
> I am not sure if replying on the thread with a patch is a good idea
> (or should I send a new email). Anyway, let me know if this is works.
>
The patch was identified correctly but the commit message now is
scrambled, see:
http://patchwork.ozlabs.org/patch/426404/
Maybe just resend it as "[PATCH net v2]"?
Thanks,
Hannes
^ permalink raw reply
* Re: [PATCH net 1/2] gen_stats.c: Duplicate xstats buffer for later use
From: Sergei Shtylyov @ 2015-01-08 13:07 UTC (permalink / raw)
To: Ignacy Gawędzki, netdev
In-Reply-To: <20150108103518.GA7214@zenon.in.qult.net>
Hello.
On 1/8/2015 1:35 PM, Ignacy Gawędzki wrote:
> 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);
Please use kmemdup() instead of kmalloc()/memcpy().
[...]
WBR, Sergei
^ permalink raw reply
* Re: [bisected] no traffic on ssl vpn with 3.19rc1 - 3.19rc3
From: Marcelo Ricardo Leitner @ 2015-01-08 13:22 UTC (permalink / raw)
To: Billy Shuman, netdev; +Cc: herbert
In-Reply-To: <CAHQNsofShzPsOgtEZw1HHpOvJW5fo+MQtO8Ys6Uqsc+SN_LaLA@mail.gmail.com>
On 07-01-2015 17:10, Billy Shuman wrote:
> Hi,
>
> Since 3.19rc1 I get 100% packet loss through SSL vpn. I bisected with
> the following result:
>
> 0b46d0ee9c240c7430a47e9b0365674d4a04522 is the first bad commit
> commit e0b46d0ee9c240c7430a47e9b0365674d4a04522
> Author: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Fri Nov 7 21:22:23 2014 +0800
>
> tun: Use iovec iterators
>
> This patch removes the use of skb_copy_datagram_const_iovec in
> favour of the iovec iterator-based skb_copy_datagram_iter.
>
>
> https://bugzilla.kernel.org/show_bug.cgi?id=90901
Interesting. Similar effect that I had but your version has the fix already.
http://article.gmane.org/gmane.linux.network/340700
Marcelo
^ permalink raw reply
* [PATCH net v3] gen_stats.c: Duplicate xstats buffer for later use
From: Ignacy Gawędzki @ 2015-01-08 13:30 UTC (permalink / raw)
To: netdev
In-Reply-To: <20150108123343.GA8541@zenon.in.qult.net>
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 | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index 0c08062..c9f1fa8 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,10 @@ 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 = kmemdup(st, len, GFP_ATOMIC);
+ if (!d->xstats)
+ goto kmalloc_failure;
d->xstats_len = len;
}
@@ -313,6 +319,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 +355,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;
}
--
2.1.0
^ permalink raw reply related
* Re: Kernel Panic ip6_xmit (screenshot)
From: Hannes Frederic Sowa @ 2015-01-08 13:51 UTC (permalink / raw)
To: Jérôme Poulin; +Cc: netdev
In-Reply-To: <CALJXSJp22yVKZtM8UerAzBdpaZE1CQuavQ=cSJ+Xf7SM75ZJxw@mail.gmail.com>
On Mi, 2015-01-07 at 12:31 -0500, Jérôme Poulin wrote:
> I'm submitting this screenshot in case it would affect something
> important. I have no more details, I was using my desktop computer and
> suddenly a kernel panic occured.
>
> Here is the screenshot: http://postimg.org/image/9jhzcfqfd/
This is a tainted an ancient kernel, I am sorry, it is unlikely someone
will look into this if you cannot give more background information on
how you think this can be reproduced.
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH] net: Prevent multiple NAPI instances co-existing in the list
From: Eric Dumazet @ 2015-01-08 14:51 UTC (permalink / raw)
To: Dennis Chen; +Cc: netdev, Herbert Xu, Miller
In-Reply-To: <CA+U0gVh+TyEUJ+qmg+FE=UhvvQywfNxcouCyv1sutZ3fav5FAw@mail.gmail.com>
On Thu, 2015-01-08 at 16:22 +0800, Dennis Chen 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>
> ---
Well no.
I am removing some atomic ops in the stack, please do not add new ones,
especially if no driver is that buggy.
The unlikely() wont help the expensive stuff being done here.
^ permalink raw reply
* Re: [patch net-next] tc: add BPF based action
From: Hannes Frederic Sowa @ 2015-01-08 14:55 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, jhs, stephen
In-Reply-To: <1420649035-9522-1-git-send-email-jiri@resnulli.us>
On Mi, 2015-01-07 at 17:43 +0100, Jiri Pirko wrote:
> +static int tcf_bpf(struct sk_buff *skb, const struct tc_action *a,
> + struct tcf_result *res)
> +{
> + struct tcf_bpf *b = a->priv;
> + int action;
> + int filter_res;
> +
> + spin_lock(&b->tcf_lock);
> + b->tcf_tm.lastuse = jiffies;
> + bstats_update(&b->tcf_bstats, skb);
> + action = b->tcf_action;
> +
> + filter_res = BPF_PROG_RUN(b->filter, skb);
> + if (filter_res == -1)
> + goto drop;
> +
> + goto unlock;
> +
> +drop:
> + action = TC_ACT_SHOT;
> + b->tcf_qstats.drops++;
> +unlock:
> + spin_unlock(&b->tcf_lock);
> + return action;
> +}
In theory this could be like:
filter_res = BPF_PROG_RUN(b->filter, skb);
spin_lock(&b->tcf_lock);
<update stats...>
if (filter_res == -1)
goto drop;
action = b->tcf_action;
...
to keep BPF_PROG_RUN out of the spin_lock?
Bye,
Hannes
^ permalink raw reply
* Re: [patch net-next] tc: add BPF based action
From: Jiri Pirko @ 2015-01-08 15:01 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: netdev, davem, jhs, stephen
In-Reply-To: <1420728937.5928.8.camel@redhat.com>
Thu, Jan 08, 2015 at 03:55:37PM CET, hannes@redhat.com wrote:
>On Mi, 2015-01-07 at 17:43 +0100, Jiri Pirko wrote:
>> +static int tcf_bpf(struct sk_buff *skb, const struct tc_action *a,
>> + struct tcf_result *res)
>> +{
>> + struct tcf_bpf *b = a->priv;
>> + int action;
>> + int filter_res;
>> +
>> + spin_lock(&b->tcf_lock);
>> + b->tcf_tm.lastuse = jiffies;
>> + bstats_update(&b->tcf_bstats, skb);
>> + action = b->tcf_action;
>> +
>> + filter_res = BPF_PROG_RUN(b->filter, skb);
>> + if (filter_res == -1)
>> + goto drop;
>> +
>> + goto unlock;
>> +
>> +drop:
>> + action = TC_ACT_SHOT;
>> + b->tcf_qstats.drops++;
>> +unlock:
>> + spin_unlock(&b->tcf_lock);
>> + return action;
>> +}
>
>In theory this could be like:
>
>filter_res = BPF_PROG_RUN(b->filter, skb);
>
>spin_lock(&b->tcf_lock);
><update stats...>
>
>if (filter_res == -1)
> goto drop;
>
>action = b->tcf_action;
>...
>
>to keep BPF_PROG_RUN out of the spin_lock?
Okay. That makes sense. Will include this change into v2.
Thanks.
>
>Bye,
>Hannes
>
>
^ 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