* Re: [PATCHv5] usbnet: Resubmit interrupt URB once if halted
From: Alan Stern @ 2011-04-22 15:47 UTC (permalink / raw)
To: Paul Stewart
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
oliver-GvhC2dPhHPQdnm+yROfE0A,
davemloft.net-hpIqsD4AKlfQT0dZR+AlfA,
bhutchings-s/n/eUQHGBpZroRs9YW3xA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20110422152451.3C5A9202ED-6A69KNNYBwgF248FYctl9mCaruZE5nAUZeezCHUQhQ4@public.gmane.org>
On Tue, 19 Apr 2011, Paul Stewart wrote:
> Resubmit interrupt URB if device is open. Use a flag set in
> usbnet_open() to determine this state. Also kill and free
> interrupt URB in usbnet_disconnect().
Generally good, but a couple of things are questionable...
> Signed-off-by: Paul Stewart <pstew-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> drivers/net/usb/usbnet.c | 14 ++++++++++++++
> include/linux/usb/usbnet.h | 1 +
> 2 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 02d25c7..c7cf4af 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -643,6 +643,7 @@ static int usbnet_open (struct net_device *net)
> }
> }
>
> + set_bit(EVENT_DEV_OPEN, &dev->flags);
> netif_start_queue (net);
> if (netif_msg_ifup (dev)) {
> char *framing;
You forgot to clear this flag in usbnet_stop().
By the way, there is FLAG_AVOID_UNLINK_URBS defined in usbnet.h and
used in usbnet_stop(). Is it meant to apply to the interrupt URB as
well as the others?
> @@ -1105,6 +1106,11 @@ void usbnet_disconnect (struct usb_interface *intf)
> if (dev->driver_info->unbind)
> dev->driver_info->unbind (dev, intf);
>
> + if (dev->interrupt) {
> + usb_kill_urb(dev->interrupt);
> + usb_free_urb(dev->interrupt);
> + }
> +
usb_kill_urb and usb_free_urb include their own tests for urb == NULL;
you don't need to test it here or below.
> free_netdev(net);
> usb_put_dev (xdev);
> }
> @@ -1285,6 +1291,10 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
> * wake the device
> */
> netif_device_attach (dev->net);
> +
> + /* Stop interrupt URBs */
> + if (dev->interrupt)
> + usb_kill_urb(dev->interrupt);
> }
> return 0;
> }
There is a subtle question here: When is the best time to kill the
interrupt URB? Without knowing any of the details, I'd guess that the
interrupt URB reports asynchronous events and the driver could run into
trouble if one of those events occurred while everything else was
turned off. This suggests that the interrupt URB should be killed as
soon as possible rather than as late as possible. But maybe it doesn't
matter; it all depends on the design of the driver.
> @@ -1297,6 +1307,10 @@ int usbnet_resume (struct usb_interface *intf)
> if (!--dev->suspend_count)
> tasklet_schedule (&dev->bh);
>
> + /* resume interrupt URBs */
> + if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags))
> + usb_submit_urb(dev->interrupt, GFP_NOIO);
> +
> return 0;
> }
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 v4 1/1] can: add pruss CAN driver.
From: Marc Kleine-Budde @ 2011-04-22 15:50 UTC (permalink / raw)
To: Subhasish Ghosh
Cc: sachi-EvXpCiN+lbve9wHmmfpqLFaTQe2KTcn/,
davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/,
Netdev-u79uwXL29TY76Z2rM5mHXA, nsekhar-l0cyMroinI0, open list,
CAN NETWORK DRIVERS, Wolfgang Grandegger, m-watkins-l0cyMroinI0,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1303474267-6344-2-git-send-email-subhasish-EvXpCiN+lbve9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 40596 bytes --]
On 04/22/2011 02:11 PM, Subhasish Ghosh wrote:
> This patch adds support for the CAN device emulated on PRUSS.
After commenting the code inline, some remarks:
- Your tx path looks broken, see commits inline
- Please setup a proper struct to describe your register layout, make
use of arrays for rx and tx
- don't use u32, s32 for not hardware related variables like return
codes and loop counter.
- the routines that load and save the can data bytes from/into your
mailbox look way to complicated. Please write down the layout so that
we can think of a elegant way to do it
- a lot of functions unconditionally return 0, make them void if no
error can happen
- think about using managed devices, that would simplify the probe and
release function
>
> Signed-off-by: Subhasish Ghosh <subhasish-EvXpCiN+lbve9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
> ---
> drivers/net/can/Kconfig | 7 +
> drivers/net/can/Makefile | 1 +
> drivers/net/can/pruss_can.c | 1074 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1082 insertions(+), 0 deletions(-)
> create mode 100644 drivers/net/can/pruss_can.c
>
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 5dec456..4682a4f 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -111,6 +111,13 @@ config PCH_CAN
> embedded processor.
> This driver can access CAN bus.
>
> +config CAN_TI_DA8XX_PRU
> + depends on CAN_DEV && ARCH_DAVINCI && ARCH_DAVINCI_DA850
> + tristate "PRU based CAN emulation for DA8XX"
> + ---help---
> + Enable this to emulate a CAN controller on the PRU of DA8XX.
> + If not sure, mark N
Please indent the help text with 1 tab and 2 spaces
> +
> source "drivers/net/can/mscan/Kconfig"
>
> source "drivers/net/can/sja1000/Kconfig"
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 53c82a7..d0b7cbd 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_CAN_SJA1000) += sja1000/
> obj-$(CONFIG_CAN_MSCAN) += mscan/
> obj-$(CONFIG_CAN_AT91) += at91_can.o
> obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
> +obj-$(CONFIG_CAN_TI_DA8XX_PRU) += pruss_can.o
> obj-$(CONFIG_CAN_MCP251X) += mcp251x.o
> obj-$(CONFIG_CAN_BFIN) += bfin_can.o
> obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o
> diff --git a/drivers/net/can/pruss_can.c b/drivers/net/can/pruss_can.c
> new file mode 100644
> index 0000000..7702509
> --- /dev/null
> +++ b/drivers/net/can/pruss_can.c
> @@ -0,0 +1,1074 @@
> +/*
> + * TI DA8XX PRU CAN Emulation device driver
> + * Author: subhasish-EvXpCiN+lbve9wHmmfpqLFaTQe2KTcn/@public.gmane.org
> + *
> + * This driver supports TI's PRU CAN Emulation and the
> + * specs for the same is available at <http://www.ti.com>
> + *
> + * Copyright (C) 2010, 2011 Texas Instruments Incorporated
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed as is WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/bitops.h>
> +#include <linux/interrupt.h>
> +#include <linux/errno.h>
> +#include <linux/netdevice.h>
> +#include <linux/skbuff.h>
> +#include <linux/platform_device.h>
> +#include <linux/firmware.h>
> +#include <linux/clk.h>
> +#include <linux/types.h>
> +#include <linux/sysfs.h>
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +#include <linux/mfd/pruss.h>
> +
> +#define PRUSS_CAN_RX_PRU_0 PRUSS_NUM0
> +#define PRUSS_CAN_TX_PRU_1 PRUSS_NUM1
> +#define PRUSS_CAN_TX_SYS_EVT 34
> +#define PRUSS_CAN_RX_SYS_EVT 33
> +#define PRUSS_CAN_ARM2DSP_SYS_EVT 32
> +#define PRUSS_CAN_DELAY_LOOP_LENGTH 2
> +#define PRUSS_CAN_TIMER_SETUP_DELAY 14
> +#define PRUSS_CAN_GPIO_SETUP_DELAY 150
> +#define PRUSS_CAN_TX_GBL_STAT_REG (PRUSS_PRU1_BASE_ADDRESS + 0x04)
> +#define PRUSS_CAN_TX_INTR_MASK_REG (PRUSS_PRU1_BASE_ADDRESS + 0x08)
> +#define PRUSS_CAN_TX_INTR_STAT_REG (PRUSS_PRU1_BASE_ADDRESS + 0x0C)
> +#define PRUSS_CAN_TX_MBOX0_STAT_REG (PRUSS_PRU1_BASE_ADDRESS + 0x10)
> +#define PRUSS_CAN_TX_ERR_CNTR_REG (PRUSS_PRU1_BASE_ADDRESS + 0x30)
I think Wolfgang and me have asked you to describe the register layout
with a struct.
> +#define PRUSS_CAN_TX_INT_STAT 0x2
> +#define PRUSS_CAN_MBOX_OFFSET 0x40
> +#define PRUSS_CAN_MBOX_SIZE 0x10
> +#define PRUSS_CAN_MBOX_STAT_OFFSET 0x10
> +#define PRUSS_CAN_MBOX_STAT_SIZE 0x04
> +#define PRUSS_CAN_GBL_STAT_REG_VAL 0x00000040
> +#define PRUSS_CAN_INTR_MASK_REG_VAL 0x00004000
> +#define PRUSS_CAN_TIMING_VAL_TX (PRUSS_PRU1_BASE_ADDRESS + 0xC0)
> +#define PRUSS_CAN_TIMING_SETUP (PRUSS_PRU1_BASE_ADDRESS + 0xC4)
> +#define PRUSS_CAN_RX_GBL_STAT_REG (PRUSS_PRU0_BASE_ADDRESS + 0x04)
> +#define PRUSS_CAN_RX_INTR_MASK_REG (PRUSS_PRU0_BASE_ADDRESS + 0x08)
> +#define PRUSS_CAN_RX_INTR_STAT_REG (PRUSS_PRU0_BASE_ADDRESS + 0x0C)
> +#define PRUSS_CAN_RX_ERR_CNTR_REG (PRUSS_PRU0_BASE_ADDRESS + 0x34)
> +#define PRUSS_CAN_TIMING_VAL_RX (PRUSS_PRU0_BASE_ADDRESS + 0xD0)
> +#define PRUSS_CAN_BIT_TIMING_VAL_RX (PRUSS_PRU0_BASE_ADDRESS + 0xD4)
> +#define PRUSS_CAN_ID_MAP (PRUSS_PRU0_BASE_ADDRESS + 0xF0)
> +#define PRUSS_CAN_ERROR_ACTIVE 128
> +#define PRUSS_CAN_GBL_STAT_REG_MASK 0x1F
> +#define PRUSS_CAN_GBL_STAT_REG_SET_TX 0x80
> +#define PRUSS_CAN_GBL_STAT_REG_SET_RX 0x40
> +#define PRUSS_CAN_GBL_STAT_REG_STOP_TX 0x7F
> +#define PRUSS_CAN_GBL_STAT_REG_STOP_RX 0xBF
> +#define PRUSS_CAN_SET_TX_REQ 0x00000080
> +#define PRUSS_CAN_STD_FRAME_MASK 18
> +#define PRUSS_CAN_START 1
> +#define PRUSS_CAN_MB_MIN 0
> +#define PRUSS_CAN_MB_MAX 7
> +#define PRUSS_CAN_MID_IDE BIT(29)
> +#define PRUSS_CAN_ISR_BIT_CCI BIT(15)
> +#define PRUSS_CAN_ISR_BIT_ESI BIT(14)
> +#define PRUSS_CAN_ISR_BIT_SRDI BIT(13)
> +#define PRUSS_CAN_ISR_BIT_RRI BIT(8)
> +#define PRUSS_CAN_GSR_BIT_EPM BIT(4)
> +#define PRUSS_CAN_GSR_BIT_BFM BIT(3)
> +#define PRUSS_CAN_RTR_BUFF_NUM 8
> +#define PRUSS_DEF_NAPI_WEIGHT 8
> +#define PRUSS_CAN_DRV_NAME "da8xx_pruss_can"
> +#define PRUSS_CAN_DRV_DESC "TI PRU CAN Controller Driver v1.0"
> +
> +enum can_tx_dir {
> + TRANSMIT = 1,
> + RECEIVE
please add a "," after RECEIVE
> +};
> +
> +struct can_mbox {
I suggest to add a namespace to these structs, e.g. pru_can_mbox. Same
comment applies to the other structs.
> + canid_t can_id;
You write this struct into the hardware, don't you? So you should not
use kernel internal types to describe your hardware layout. Think about
declaring this struct __packed__
> + u8 data[8];
> + u16 datalength;
> + u16 crc;
> +};
> +
> +struct can_emu_cntx {
> + enum can_tx_dir txdir;
> + struct can_mbox mbox;
> + u32 mboxno;
> + u8 pruno;
> + u32 gbl_stat;
> + u32 intr_stat;
> + u32 mbox_stat;
> +};
> +
> +struct can_emu_priv {
> + struct can_priv can;
> + struct napi_struct napi;
> + struct net_device *ndev;
> + struct device *dev;
> + struct clk *clk_timer;
> + struct can_emu_cntx can_tx_cntx;
> + struct can_emu_cntx can_rx_cntx;
I have not looked at the rest of the code, but it smells that you should
make this an array of two cntx.
> + unsigned int clk_freq_pru;
> + unsigned int trx_irq;
> + unsigned int tx_head;
> + unsigned int tx_tail;
> + unsigned int tx_next;
> + unsigned int mbx_id[8];
> +};
> +
> +static struct can_bittiming_const pru_can_bittiming_const = {
> + .name = PRUSS_CAN_DRV_NAME,
> + .tseg1_min = 1,
> + .tseg1_max = 16,
> + .tseg2_min = 1,
> + .tseg2_max = 8,
> + .sjw_max = 4,
> + .brp_min = 1,
> + .brp_max = 256,
> + .brp_inc = 1,
> +};
> +
> +static int pru_can_mbox_write(struct device *dev,
> + struct can_emu_cntx *emu_cntx)
> +{
> + u32 offset = 0;
^^^^^
not needed
> +
> + offset = PRUSS_PRU1_BASE_ADDRESS + PRUSS_CAN_MBOX_OFFSET
> + + (PRUSS_CAN_MBOX_SIZE * emu_cntx->mboxno);
> +
> + pruss_writel_multi(dev, offset, (u32 *) &(emu_cntx->mbox), 4);
> +
> + return 0;
> +}
> +
> +static int pru_can_mbox_read(struct device *dev,
> + struct can_emu_cntx *emu_cntx)
> +{
> + u32 offset = 0;
dito
> +
> + offset = PRUSS_PRU0_BASE_ADDRESS + PRUSS_CAN_MBOX_OFFSET
> + + (PRUSS_CAN_MBOX_SIZE * emu_cntx->mboxno);
> +
> + pruss_readl_multi(dev, offset, (u32 *) &emu_cntx->mbox, 4);
where does this "4" come from? consider using sizeof()
> +
> + return 0;
why do you return 0 here? pruss_readl_multi is not void, although it
always returns 0, too. consider make all void.
> +}
> +
> +static int pru_can_rx_id_set(struct device *dev, u32 nodeid, u32 mboxno)
> +{
> + pruss_writel(dev, (PRUSS_CAN_ID_MAP + (mboxno * 4)), nodeid);
> +
> + return 0;
consider making this a void function.
> +}
> +
> +static int pru_can_intr_stat_get(struct device *dev,
> + struct can_emu_cntx *emu_cntx)
> +{
> + if (emu_cntx->pruno == PRUCORE_1)
> + pruss_readl(dev, PRUSS_CAN_TX_INTR_STAT_REG,
> + &emu_cntx->intr_stat);
> + else if (emu_cntx->pruno == PRUCORE_0)
> + pruss_readl(dev, PRUSS_CAN_RX_INTR_STAT_REG,
> + &emu_cntx->intr_stat);
If you describe the register layout with a struct with an array
containing with rx and tx registers you can get rid of the if..else..
use emu_cntx->pruno as index to the array.
> + else
> + return -EINVAL;
It's an internally used function, if emu_cntx->pruno is bogous here
you've got really big problems. I think it's save to remove this. Then
this function would become a void function.
> +
> + return 0;
> +}
> +
> +static int pru_can_gbl_stat_get(struct device *dev,
> + struct can_emu_cntx *emu_cntx)
> +{
> + if (emu_cntx->pruno == PRUCORE_1)
> + pruss_readl(dev, PRUSS_CAN_TX_GBL_STAT_REG,
> + &emu_cntx->gbl_stat);
> + else if (emu_cntx->pruno == PRUCORE_0)
> + pruss_readl(dev, PRUSS_CAN_RX_GBL_STAT_REG,
> + &emu_cntx->gbl_stat);
> + else
> + return -EINVAL;
> + return 0;
Same comments apply here, too.
> +}
> +
> +static int pru_can_tx_mode_set(struct device *dev, bool transfer_flag,
> + enum can_tx_dir ecan_trx)
> +{
> + u32 value;
> +
> + if (ecan_trx == TRANSMIT) {
> + pruss_readl(dev, PRUSS_CAN_RX_GBL_STAT_REG, &value);
> + if (transfer_flag) {
> + value &= PRUSS_CAN_GBL_STAT_REG_MASK;
> + value |= PRUSS_CAN_GBL_STAT_REG_SET_TX;
> + } else
> + value &= PRUSS_CAN_GBL_STAT_REG_STOP_TX;
> +
> + pruss_writel(dev, PRUSS_CAN_RX_GBL_STAT_REG, value);
> + pruss_writel(dev, PRUSS_CAN_TX_GBL_STAT_REG, value);
> + } else if (ecan_trx == RECEIVE) {
> + pruss_readl(dev, PRUSS_CAN_RX_GBL_STAT_REG, &value);
> + if (transfer_flag) {
> + value &= PRUSS_CAN_GBL_STAT_REG_MASK;
> + value |= PRUSS_CAN_GBL_STAT_REG_SET_RX;
> + } else
> + value &= PRUSS_CAN_GBL_STAT_REG_STOP_RX;
> +
> + pruss_writel(dev, PRUSS_CAN_RX_GBL_STAT_REG, value);
> + pruss_writel(dev, PRUSS_CAN_TX_GBL_STAT_REG, value);
> + } else
Same comments apply here, too.
> + return -EINVAL;
> +
> + return 0;
> +}
> +
is this array const?
> +static u32 pruss_intc_init[19][3] = {
> + {PRUSS_INTC_POLARITY0, PRU_INTC_REGMAP_MASK, 0xFFFFFFFF},
> + {PRUSS_INTC_POLARITY1, PRU_INTC_REGMAP_MASK, 0xFFFFFFFF},
> + {PRUSS_INTC_TYPE0, PRU_INTC_REGMAP_MASK, 0x1C000000},
> + {PRUSS_INTC_TYPE1, PRU_INTC_REGMAP_MASK, 0},
> + {PRUSS_INTC_GLBLEN, 0, 1},
> + {PRUSS_INTC_HOSTMAP0, PRU_INTC_REGMAP_MASK, 0x03020100},
> + {PRUSS_INTC_HOSTMAP1, PRU_INTC_REGMAP_MASK, 0x07060504},
> + {PRUSS_INTC_HOSTMAP2, PRU_INTC_REGMAP_MASK, 0x0000908},
> + {PRUSS_INTC_CHANMAP0, PRU_INTC_REGMAP_MASK, 0},
> + {PRUSS_INTC_CHANMAP8, PRU_INTC_REGMAP_MASK, 0x00020200},
> + {PRUSS_INTC_STATIDXCLR, 0, 32},
> + {PRUSS_INTC_STATIDXCLR, 0, 19},
> + {PRUSS_INTC_ENIDXSET, 0, 19},
> + {PRUSS_INTC_STATIDXCLR, 0, 18},
> + {PRUSS_INTC_ENIDXSET, 0, 18},
> + {PRUSS_INTC_STATIDXCLR, 0, 34},
> + {PRUSS_INTC_ENIDXSET, 0, 34},
> + {PRUSS_INTC_ENIDXSET, 0, 32},
> + {PRUSS_INTC_HOSTINTEN, 0, 5}
please add ","
> +};
> +
> +static int pru_can_emu_init(struct device *dev, u32 pruclock)
> +{
> + u32 value[8] = {[0 ... 7] = 1};
> + u32 i;
we usually use plain ints as a for-loop variable.
> +
> + /* PRU Internal Registers Initializations */
> + pruss_writel_multi(dev, PRUSS_CAN_TX_MBOX0_STAT_REG, value, 8);
use sizeof(), or ARRAY_SIZE
> +
> + *value = PRUSS_CAN_GBL_STAT_REG_VAL;
> + pruss_writel(dev, PRUSS_CAN_TX_GBL_STAT_REG, value[0]);
> + pruss_writel(dev, PRUSS_CAN_RX_GBL_STAT_REG, value[0]);
why not:
pruss_writel(dev, PRUSS_CAN_TX_GBL_STAT_REG, PRUSS_CAN_GBL_STAT_REG_VAL);
> +
> + *value = PRUSS_CAN_INTR_MASK_REG_VAL;
> + pruss_writel(dev, PRUSS_CAN_TX_INTR_MASK_REG, value[0]);
> + pruss_writel(dev, PRUSS_CAN_RX_INTR_MASK_REG, value[0]);
> +
> + for (i = 0; i < 19; i++)
ARRAY_SIZE
> + (i < 12) ? pruss_rmwl(dev, pruss_intc_init[i][0],
> + pruss_intc_init[i][1],
> + pruss_intc_init[i][2]) :
> + pruss_idx_writel(dev, pruss_intc_init[i][0],
> + pruss_intc_init[i][2]);
if..else here, please
or put the stuff into two arrays
> + return 0;
> +}
> +
> +static void pru_can_emu_exit(struct device *dev)
> +{
> + pruss_disable(dev, PRUSS_CAN_RX_PRU_0);
> + pruss_disable(dev, PRUSS_CAN_TX_PRU_1);
> +}
> +
> +static int pru_can_tx(struct device *dev, u8 mboxnumber, u8 pruno)
> +{
> + u32 value = 0;
> +
> + if (PRUCORE_1 == pruno) {
we usually write it the other way round...:
if (pruno == PRUCORE_1)
> + value = PRUSS_CAN_SET_TX_REQ;
> + pruss_writel(dev, ((PRUSS_PRU1_BASE_ADDRESS +
> + (PRUSS_CAN_MBOX_STAT_OFFSET +
> + (PRUSS_CAN_MBOX_STAT_SIZE * mboxnumber)))
> + & 0xFFFF), value);
don't use value, use PRUSS_CAN_SET_TX_REQ directly
> + } else if (PRUCORE_0 == pruno) {
> + pruss_readl(dev, PRUSS_CAN_RX_INTR_STAT_REG, &value);
> + value = value & ~(1 << mboxnumber);
> + pruss_writel(dev, PRUSS_CAN_RX_INTR_STAT_REG, value);
> + value = 0;
> + pruss_writel(dev, ((PRUSS_PRU0_BASE_ADDRESS +
> + (PRUSS_CAN_MBOX_STAT_OFFSET +
> + (PRUSS_CAN_MBOX_STAT_SIZE * mboxnumber)))
> + & 0xFFFF), value);
same here
> + } else
> + return -EINVAL;
trust your own code, get rid of the -EINVAL, make this a void function.
> + return 0;
> +}
> +
> +static int pru_can_reset_tx(struct device *dev)
> +{
> + pruss_idx_writel(dev, PRUSS_INTC_STATIDXCLR, PRUSS_CAN_ARM2DSP_SYS_EVT);
> + pruss_idx_writel(dev, PRUSS_INTC_ENIDXSET, PRUSS_CAN_ARM2DSP_SYS_EVT);
> + pruss_idx_writel(dev, PRUSS_INTC_STATIDXSET, PRUSS_CAN_ARM2DSP_SYS_EVT);
> + return 0;
void function?
> +}
> +
> +static void pru_can_mask_ints(struct device *dev, u32 trx, bool enable)
> +{
> + u32 value = 0;
not needed
> +
> + value = (trx == PRUSS_CAN_TX_PRU_1) ?
> + PRUSS_CAN_TX_SYS_EVT : PRUSS_CAN_RX_SYS_EVT;
use a struct with arrays for the register description
> + enable ? pruss_idx_writel(dev, PRUSS_INTC_ENIDXSET, value) :
> + pruss_idx_writel(dev, PRUSS_INTC_ENIDXCLR, value);
if..else
> +}
> +
> +static unsigned int pru_can_get_error_cnt(struct device *dev, u8 pruno)
> +{
> + u32 value = 0;
not needed
> +
> + if (pruno == PRUSS_CAN_RX_PRU_0)
> + pruss_readl(dev, PRUSS_CAN_RX_ERR_CNTR_REG, &value);
> + else if (pruno == PRUSS_CAN_TX_PRU_1)
> + pruss_readl(dev, PRUSS_CAN_TX_ERR_CNTR_REG, &value);
> + else
> + return -EINVAL;
remove the -EINVAL
> +
> + return value;
> +}
> +
> +static unsigned int pru_can_get_intc_status(struct device *dev)
> +{
> + u32 value = 0;
not needed
> +
> + pruss_readl(dev, PRUSS_INTC_STATCLRINT1, &value);
> + return value;
> +}
> +
> +static void pru_can_clr_intc_status(struct device *dev, u32 trx)
> +{
> + u32 value = 0;
dito
> +
> + value = (trx == PRUSS_CAN_TX_PRU_1) ?
> + PRUSS_CAN_TX_SYS_EVT : PRUSS_CAN_RX_SYS_EVT;
use a struct + array for the resiter desc
> + pruss_idx_writel(dev, PRUSS_INTC_STATIDXCLR, value);
> +}
> +
> +static int pru_can_get_state(const struct net_device *ndev,
> + enum can_state *state)
> +{
> + struct can_emu_priv *priv = netdev_priv(ndev);
> + *state = priv->can.state;
we don't implemnt this function anymore..
> +
> + return 0;
> +}
> +
> +static int pru_can_set_bittiming(struct net_device *ndev)
> +{
> + struct can_emu_priv *priv = netdev_priv(ndev);
> + struct can_bittiming *bt = &priv->can.bittiming;
> + u32 value;
> +
> + value = priv->can.clock.freq / bt->bitrate;
> + pruss_writel(priv->dev, PRUSS_CAN_TIMING_VAL_TX, value);
> + pruss_writel(priv->dev, PRUSS_CAN_BIT_TIMING_VAL_RX, value);
> +
> + value = (bt->phase_seg2 + bt->phase_seg1 +
> + bt->prop_seg + 1) * bt->brp;
> + value = (value >> 1) - PRUSS_CAN_TIMER_SETUP_DELAY;
> + value = (value << 16) | value;
> + pruss_writel(priv->dev, PRUSS_CAN_TIMING_VAL_RX, value);
> +
> + value = (PRUSS_CAN_GPIO_SETUP_DELAY *
> + (priv->clk_freq_pru / 1000000) / 1000) /
> + PRUSS_CAN_DELAY_LOOP_LENGTH;
> +
> + pruss_writel(priv->dev, PRUSS_CAN_TIMING_SETUP, value);
> + return 0;
> +}
> +
> +static void pru_can_stop(struct net_device *ndev)
> +{
> + struct can_emu_priv *priv = netdev_priv(ndev);
> +
> + pru_can_mask_ints(priv->dev, PRUSS_CAN_TX_PRU_1, false);
> + pru_can_mask_ints(priv->dev, PRUSS_CAN_RX_PRU_0, false);
> + pru_can_reset_tx(priv->dev);
> + priv->can.state = CAN_STATE_STOPPED;
> +}
> +
> +/*
> + * This is to just set the can state to ERROR_ACTIVE
> + * ip link set canX up type can bitrate 125000
fix the comment
> + */
> +static void pru_can_start(struct net_device *ndev)
> +{
> + struct can_emu_priv *priv = netdev_priv(ndev);
> +
> + if (priv->can.state != CAN_STATE_STOPPED)
> + pru_can_stop(ndev);
> +
> + pru_can_mask_ints(priv->dev, PRUSS_CAN_TX_PRU_1, true);
> + pru_can_mask_ints(priv->dev, PRUSS_CAN_RX_PRU_0, true);
> +
> + pru_can_gbl_stat_get(priv->dev, &priv->can_tx_cntx);
> + pru_can_gbl_stat_get(priv->dev, &priv->can_rx_cntx);
> +
> + if (PRUSS_CAN_GSR_BIT_EPM & priv->can_tx_cntx.gbl_stat)
> + priv->can.state = CAN_STATE_ERROR_PASSIVE;
> + else if (PRUSS_CAN_GSR_BIT_BFM & priv->can_tx_cntx.gbl_stat)
> + priv->can.state = CAN_STATE_BUS_OFF;
> + else
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +}
> +
> +static int pru_can_set_mode(struct net_device *ndev, enum can_mode mode)
> +{
> + int ret = 0;
> +
> + switch (mode) {
> + case CAN_MODE_START:
> + pru_can_start(ndev);
> + netif_wake_queue(ndev);
> + break;
> + default:
> + ret = -EOPNOTSUPP;
> + break;
> + }
> + return ret;
> +}
> +
> +static netdev_tx_t pru_can_start_xmit(struct sk_buff *skb,
> + struct net_device *ndev)
> +{
> + struct can_emu_priv *priv = netdev_priv(ndev);
> + struct can_frame *cf = (struct can_frame *)skb->data;
> + int count;
> + u8 *data = cf->data;
> + u8 dlc = cf->can_dlc;
> + u8 *pdata = NULL;
> +
> + if (can_dropped_invalid_skb(ndev, skb))
> + return NETDEV_TX_OK;
> +
> + netif_stop_queue(ndev);
why do you stop the queue unconditionally here?
> + if (cf->can_id & CAN_EFF_FLAG) /* Extended frame format */
> + priv->can_tx_cntx.mbox.can_id =
> + (cf->can_id & CAN_EFF_MASK) | PRUSS_CAN_MID_IDE;
> + else /* Standard frame format */
> + priv->can_tx_cntx.mbox.can_id =
> + (cf->can_id & CAN_SFF_MASK) << PRUSS_CAN_STD_FRAME_MASK;
> +
> + if (cf->can_id & CAN_RTR_FLAG) /* Remote transmission request */
> + priv->can_tx_cntx.mbox.can_id |= CAN_RTR_FLAG;
> +
> + pdata = &priv->can_tx_cntx.mbox.data[0] + (dlc - 1);
> + for (count = 0; count < (u8) dlc; count++)
> + *pdata-- = *data++;
What does this loop do? endianess conversion? Please don't open code this.
> +
> + priv->can_tx_cntx.mbox.datalength = (u16) dlc;
no need to cast
> + priv->can_tx_cntx.mbox.crc = 0;
> +/*
> + * search for the next available mbx
> + * if the next mbx is busy, then try the next + 1
> + * do this until the head is reached.
> + * if still unable to tx, stop accepting any packets
> + * if able to tx and the head is reached, then reset next to tail, i.e mbx0
> + * if head is not reached, then just point to the next mbx
> + */
indention, please
Your tx algorithm looks fishy. You always use can_get_echo_skb(ndev, 0).
This means you can have only 1 can frame on the fly. But you say you
look for a free mailbox. You have to tx mailboxes in a defined order,
otherwise your hardware/firmware is broken. If your hardware transmits
frames in order, you always know which one will be the next free
mailbox. You have a power of 2 number of mailboxes, you can simply apply
a mask to get to the real mailbox number. No need for manual wrap
around. Have a look at the at91_can tx sheme.
Activate the tx_interrupt, putting a can frame into a mailbox, stop the
tx_queue if there are no free mailboxes, or in case of a wrap around.
Reenable the tx_queue in the tx_complete interrupt handler.
> + for (; priv->tx_next <= priv->tx_head; priv->tx_next++) {
> + priv->can_tx_cntx.mboxno = priv->tx_next;
> + if (-1 == pru_can_mbox_write(priv->dev,
> + &priv->can_tx_cntx)) {
this function will always return 0.
> + if (priv->tx_next == priv->tx_head) {
> + priv->tx_next = priv->tx_tail;
> + netif_stop_queue(ndev); /* IF stalled */
> + dev_err(priv->dev,
> + "%s: no tx mbx available", __func__);
> + return NETDEV_TX_BUSY;
> + } else
> + continue;
> + } else {
> + /* set transmit request */
> + pru_can_tx(priv->dev, priv->tx_next,
> + PRUSS_CAN_TX_PRU_1);
> + pru_can_tx_mode_set(priv->dev, false, RECEIVE);
> + pru_can_tx_mode_set(priv->dev, true, TRANSMIT);
> + pru_can_reset_tx(priv->dev);
> + priv->tx_next++;
> + can_put_echo_skb(skb, ndev, 0);
^^^
see comment above
> + break;
> + }
> + }
> + if (priv->tx_next > priv->tx_head)
> + priv->tx_next = priv->tx_tail;
> +
> + return NETDEV_TX_OK;
> +}
> +
> +static int pru_can_rx(struct net_device *ndev, u32 mbxno)
> +{
> + struct can_emu_priv *priv = netdev_priv(ndev);
> + struct net_device_stats *stats = &ndev->stats;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> + u8 *data = NULL;
> + u8 *pdata = NULL;
> + int count = 0;
> +
> + skb = alloc_can_skb(ndev, &cf);
> + if (!skb) {
> + if (printk_ratelimit())
> + dev_err(priv->dev,
> + "alloc_can_skb() failed\n");
> + return -ENOMEM;
> + }
> + data = cf->data;
> + /* get payload */
> + priv->can_rx_cntx.mboxno = mbxno;
> + if (pru_can_mbox_read(priv->dev, &priv->can_rx_cntx)) {
function always returns 0!
> + dev_err(priv->dev, "failed to get data from mailbox\n");
> + return -EAGAIN;
> + }
> + /* give ownweship to pru */
> + pru_can_tx(priv->dev, mbxno, PRUSS_CAN_RX_PRU_0);
> +
> + /* get data length code */
> + cf->can_dlc = get_can_dlc(priv->can_rx_cntx.mbox.datalength & 0xF);
This looks to complicated. Please state how the individual can bytes are
placed in the mailbox, so that we can think of a simpler way to do this.
> + if (cf->can_dlc <= 4) {
> + pdata = &priv->can_rx_cntx.mbox.data[4] + (4 - cf->can_dlc);
> + for (count = 0; count < cf->can_dlc; count++)
> + *data++ = *pdata++;
> + } else {
> + pdata = &priv->can_rx_cntx.mbox.data[4];
> + for (count = 0; count < 4; count++)
> + *data++ = *pdata++;
> + pdata = &priv->can_rx_cntx.mbox.data[3] - (cf->can_dlc - 5);
> + for (count = 0; count < cf->can_dlc - 4; count++)
> + *data++ = *pdata++;
> + }
> +
> + /* get id extended or std */
> + if (priv->can_rx_cntx.mbox.can_id & PRUSS_CAN_MID_IDE)
> + cf->can_id = (priv->can_rx_cntx.mbox.can_id & CAN_EFF_MASK)
> + | CAN_EFF_FLAG;
the usual way is to write the "|" at the end of the line.
> + else
> + cf->can_id = (priv->can_rx_cntx.mbox.can_id >> 18)
> + & CAN_SFF_MASK;
> +
> + if (priv->can_rx_cntx.mbox.can_id & CAN_RTR_FLAG)
> + cf->can_id |= CAN_RTR_FLAG;
please don't copy any data to the can frame in case if an RTR message.
> +
> + stats->rx_bytes += cf->can_dlc;
> + netif_receive_skb(skb);
> + stats->rx_packets++;
> + return 0;
> +}
> +
> +static int pru_can_err(struct net_device *ndev, int int_status,
> + int err_status)
> +{
> + struct can_emu_priv *priv = netdev_priv(ndev);
> + struct net_device_stats *stats = &ndev->stats;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> + u32 tx_err_cnt, rx_err_cnt;
> +
> + skb = alloc_can_err_skb(ndev, &cf);
> + if (!skb) {
> + if (printk_ratelimit())
> + dev_err(priv->dev,
> + "alloc_can_err_skb() failed\n");
> + return -ENOMEM;
> + }
> +
> + if (err_status & PRUSS_CAN_GSR_BIT_EPM) { /* error passive int */
> + priv->can.state = CAN_STATE_ERROR_PASSIVE;
> + ++priv->can.can_stats.error_passive;
> + cf->can_id |= CAN_ERR_CRTL;
> + tx_err_cnt = pru_can_get_error_cnt(priv->dev,
> + PRUSS_CAN_TX_PRU_1);
> + rx_err_cnt = pru_can_get_error_cnt(priv->dev,
> + PRUSS_CAN_RX_PRU_0);
> + if (tx_err_cnt > PRUSS_CAN_ERROR_ACTIVE - 1)
> + cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
> + if (rx_err_cnt > PRUSS_CAN_ERROR_ACTIVE - 1)
> + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
> +
> + dev_dbg(priv->ndev->dev.parent, "Error passive interrupt\n");
> + }
> +
> + if (err_status & PRUSS_CAN_GSR_BIT_BFM) {
> + priv->can.state = CAN_STATE_BUS_OFF;
> + cf->can_id |= CAN_ERR_BUSOFF;
> + /*
> + * Disable all interrupts in bus-off to avoid int hog
> + * this should be handled by the pru
> + */
> + pru_can_mask_ints(priv->dev, PRUSS_CAN_TX_PRU_1, false);
> + pru_can_mask_ints(priv->dev, PRUSS_CAN_RX_PRU_0, false);
> + can_bus_off(ndev);
> + dev_dbg(priv->ndev->dev.parent, "Bus off mode\n");
> + }
> +
> + netif_rx(skb);
> + stats->rx_packets++;
> + stats->rx_bytes += cf->can_dlc;
> + return 0;
> +}
> +
> +static int pru_can_rx_poll(struct napi_struct *napi, int quota)
> +{
> + struct net_device *ndev = napi->dev;
> + struct can_emu_priv *priv = netdev_priv(ndev);
> + u32 bit_set, mbxno = 0;
> + u32 num_pkts = 0;
> +
> + if (!netif_running(ndev))
> + return 0;
> +
> + do {
> + /* rx int sys_evt -> 33 */
> + pru_can_clr_intc_status(priv->dev, PRUSS_CAN_RX_PRU_0);
> + if (pru_can_intr_stat_get(priv->dev, &priv->can_rx_cntx))
> + return num_pkts;
> +
> + if (PRUSS_CAN_ISR_BIT_RRI & priv->can_rx_cntx.intr_stat) {
> + mbxno = PRUSS_CAN_RTR_BUFF_NUM;
> + pru_can_rx(ndev, mbxno);
> + num_pkts++;
> + } else {
> + /* Extract the mboxno from the status */
> + bit_set = fls(priv->can_rx_cntx.intr_stat & 0xFF);
> + if (bit_set) {
> + num_pkts++;
> + mbxno = bit_set - 1;
> + if (PRUSS_CAN_ISR_BIT_ESI & priv->can_rx_cntx.
> + intr_stat) {
> + pru_can_gbl_stat_get(priv->dev,
> + &priv->can_rx_cntx);
> + pru_can_err(ndev,
> + priv->can_rx_cntx.intr_stat,
> + priv->can_rx_cntx.gbl_stat);
> + } else
> + pru_can_rx(ndev, mbxno);
> + } else
> + break;
> + }
> + } while (((PRUSS_CAN_TX_INT_STAT & pru_can_get_intc_status(priv->dev))
> + && (num_pkts < quota)));
> +
> + /* Enable packet interrupt if all pkts are handled */
> + if (!(PRUSS_CAN_TX_INT_STAT & pru_can_get_intc_status(priv->dev))) {
> + napi_complete(napi);
> + /* Re-enable RX mailbox interrupts */
> + pru_can_mask_ints(priv->dev, PRUSS_CAN_RX_PRU_0, true);
> + }
> + return num_pkts;
> +}
> +
> +static irqreturn_t pru_tx_can_intr(int irq, void *dev_id)
> +{
> + struct net_device *ndev = dev_id;
> + struct can_emu_priv *priv = netdev_priv(ndev);
> + struct net_device_stats *stats = &ndev->stats;
> + u32 bit_set, mbxno;
> +
> + pru_can_intr_stat_get(priv->dev, &priv->can_tx_cntx);
> + if ((PRUSS_CAN_ISR_BIT_CCI & priv->can_tx_cntx.intr_stat)
> + || (PRUSS_CAN_ISR_BIT_SRDI & priv->can_tx_cntx.intr_stat)) {
> + dev_dbg(priv->ndev->dev.parent, "tx_int_status = 0x%X\n",
> + priv->can_tx_cntx.intr_stat);
> + can_free_echo_skb(ndev, 0);
^^^
make no sense if using multiple tx mailboxes
> + } else {
> + bit_set = fls(priv->can_tx_cntx.intr_stat & 0xFF);
> + if (!bit_set) {
> + dev_err(priv->dev, "%s: invalid mailbox number\n",
> + __func__);
> + can_free_echo_skb(ndev, 0);
^^^^
> + } else {
> + mbxno = bit_set - 1;
> + if (PRUSS_CAN_ISR_BIT_ESI & priv->can_tx_cntx.
> + intr_stat) {
> + /* read gsr and ack pru */
> + pru_can_gbl_stat_get(priv->dev,
> + &priv->can_tx_cntx);
> + pru_can_err(ndev, priv->can_tx_cntx.intr_stat,
> + priv->can_tx_cntx.gbl_stat);
> + } else {
> + stats->tx_packets++;
> + /* stats->tx_bytes += dlc; */
> + /*can_get_echo_skb(ndev, 0);*/
??
> + }
> + }
> + }
> + netif_wake_queue(ndev);
> + can_get_echo_skb(ndev, 0);
again?
> + pru_can_tx_mode_set(priv->dev, true, RECEIVE);
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t pru_rx_can_intr(int irq, void *dev_id)
why is this function calles rx_can_intr it's a generic interrupt function..
> +{
> + struct net_device *ndev = dev_id;
> + struct can_emu_priv *priv = netdev_priv(ndev);
> + u32 intc_status = 0;
> +
> + intc_status = pru_can_get_intc_status(priv->dev);
> +
> + /* tx int sys_evt -> 34 */
> + if (intc_status & 4) {
> + pru_can_clr_intc_status(priv->dev, PRUSS_CAN_TX_PRU_1);
> + return pru_tx_can_intr(irq, dev_id);
why are you returning here? is is possible the you have a can frame to
receivce?
> + }
> + /* Disable RX mailbox interrupts and let NAPI reenable them */
> + if (intc_status & 2) {
> + pru_can_mask_ints(priv->dev, PRUSS_CAN_RX_PRU_0, false);
> + napi_schedule(&priv->napi);
> + }
> +
> + return IRQ_HANDLED;
^^^^^^^^^^^^
that might not be true....
> +}
> +
> +static int pru_can_open(struct net_device *ndev)
> +{
> + struct can_emu_priv *priv = netdev_priv(ndev);
> + s32 err;
> +
> + /* register interrupt handler */
> + err = request_irq(priv->trx_irq, &pru_rx_can_intr, IRQF_SHARED,
> + "pru_can_irq", ndev);
use dev->name for interrupt name
> + if (err) {
> + dev_err(priv->dev, "error requesting rx interrupt\n");
> + goto exit_trx_irq;
> + }
> + err = open_candev(ndev);
> + if (err) {
> + dev_err(priv->dev, "open_candev() failed %d\n", err);
> + goto exit_open;
> + }
> +
> + pru_can_emu_init(priv->dev, priv->can.clock.freq);
> + priv->tx_tail = PRUSS_CAN_MB_MIN;
> + priv->tx_head = PRUSS_CAN_MB_MAX;
> + pru_can_start(ndev);
> + napi_enable(&priv->napi);
> + netif_start_queue(ndev);
> + return 0;
> +
> +exit_open:
> + free_irq(priv->trx_irq, ndev);
> +exit_trx_irq:
> + return err;
> +}
> +
> +static int pru_can_close(struct net_device *ndev)
> +{
> + struct can_emu_priv *priv = netdev_priv(ndev);
> +
> + netif_stop_queue(ndev);
> + napi_disable(&priv->napi);
> + close_candev(ndev);
> + free_irq(priv->trx_irq, ndev);
> + return 0;
> +}
> +
> +static const struct net_device_ops pru_can_netdev_ops = {
> + .ndo_open = pru_can_open,
> + .ndo_stop = pru_can_close,
> + .ndo_start_xmit = pru_can_start_xmit,
> +};
> +
> +/* Shows all the mailbox IDs */
> +static ssize_t pru_sysfs_mbx_id_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct can_emu_priv *priv = netdev_priv(to_net_dev(dev));
> +
> + return snprintf(buf, PAGE_SIZE, "<mbx_no:mbx_id>\n"
> + "0:0x%X 1:0x%X 2:0x%X 3:0x%X "
> + "4:0x%X 5:0x%X 6:0x%X 7:0x%X\n",
> + priv->mbx_id[0], priv->mbx_id[1],
> + priv->mbx_id[2], priv->mbx_id[3],
> + priv->mbx_id[4], priv->mbx_id[5],
> + priv->mbx_id[6], priv->mbx_id[7]);
> +}
> +
> +/*
> + * Sets Mailbox IDs
> + * This should be programmed as mbx_num:mbx_id (in hex)
> + * eg: $ echo 0:0x123 > /sys/class/net/can0/mbx_id
> + */
> +static ssize_t pru_sysfs_mbx_id_set(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + struct net_device *ndev = to_net_dev(dev);
> + struct can_emu_priv *priv = netdev_priv(ndev);
> + unsigned long can_id;
> + unsigned long mbx_num;
> + char mbx[2] = {*buf, '\0'}; /* mbx num */
> + ssize_t ret = count;
> + s32 err;
I think you have to lock here:
rtnl_lock();
> +
> + if (ndev->flags & IFF_UP) {
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + if (*(buf + 1) != ':') {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + err = strict_strtoul(mbx, 0, &mbx_num);
> + if (err) {
> + ret = err;
> + goto out;
> + }
> +
> + if (mbx_num > 7) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + err = strict_strtoul((buf + 2), 0, &can_id);
> + if (err) {
> + ret = err;
> + goto out;
> + }
> +
> + priv->mbx_id[mbx_num] = can_id;
> + pru_can_rx_id_set(priv->dev, priv->mbx_id[mbx_num], mbx_num);
> +
> + return ret;
> +out:
> + dev_err(priv->dev, "invalid buffer format\n");
> + return ret;
> +}
> +
> +static DEVICE_ATTR(mbx_id, S_IWUSR | S_IRUGO,
> + pru_sysfs_mbx_id_show, pru_sysfs_mbx_id_set);
> +
> +static struct attribute *pru_sysfs_attrs[] = {
> + &dev_attr_mbx_id.attr,
> + NULL,
> +};
> +
> +static struct attribute_group pru_sysfs_attr_group = {
> + .attrs = pru_sysfs_attrs,
> +};
> +
> +static int __devinit pru_can_probe(struct platform_device *pdev)
> +{
> + struct net_device *ndev = NULL;
> + const struct da850_evm_pruss_can_data *pdata;
> + struct can_emu_priv *priv = NULL;
> + struct device *dev = &pdev->dev;
> + struct clk *clk_pruss;
> + const struct firmware *fw_rx;
> + const struct firmware *fw_tx;
> + u32 err;
use int
> +
> + pdata = dev->platform_data;
> + if (!pdata) {
> + dev_err(&pdev->dev, "platform data not found\n");
> + return -EINVAL;
> + }
> + (pdata->setup)();
no need fot the ( )
> +
> + ndev = alloc_candev(sizeof(struct can_emu_priv), PRUSS_CAN_MB_MAX + 1);
> + if (!ndev) {
> + dev_err(&pdev->dev, "alloc_candev failed\n");
> + err = -ENOMEM;
> + goto probe_exit;
> + }
> +
> + ndev->sysfs_groups[0] = &pru_sysfs_attr_group;
> +
> + priv = netdev_priv(ndev);
> +
> + priv->trx_irq = platform_get_irq(to_platform_device(dev->parent), 0);
> + if (!priv->trx_irq) {
> + dev_err(&pdev->dev, "unable to get pru "
> + "interrupt resources!\n");
> + err = -ENODEV;
> + goto probe_exit;
> + }
> +
> + priv->ndev = ndev;
> + priv->dev = dev;
> +
> + priv->can.bittiming_const = &pru_can_bittiming_const;
> + priv->can.do_set_bittiming = pru_can_set_bittiming;
> + priv->can.do_set_mode = pru_can_set_mode;
> + priv->can.do_get_state = pru_can_get_state;
> + priv->can_tx_cntx.pruno = PRUSS_CAN_TX_PRU_1;
> + priv->can_rx_cntx.pruno = PRUSS_CAN_RX_PRU_0;
> +
> + /* we support local echo, no arp */
> + ndev->flags |= (IFF_ECHO | IFF_NOARP);
no need to se NOARP
> +
> + /* pdev->dev->device_private->driver_data = ndev */
> + platform_set_drvdata(pdev, ndev);
> + SET_NETDEV_DEV(ndev, &pdev->dev);
> + ndev->netdev_ops = &pru_can_netdev_ops;
> +
> + priv->clk_timer = clk_get(&pdev->dev, "pll1_sysclk2");
> + if (IS_ERR(priv->clk_timer)) {
> + dev_err(&pdev->dev, "no timer clock available\n");
> + err = PTR_ERR(priv->clk_timer);
> + priv->clk_timer = NULL;
> + goto probe_exit_candev;
> + }
> +
> + priv->can.clock.freq = clk_get_rate(priv->clk_timer);
> +
> + clk_pruss = clk_get(NULL, "pruss");
> + if (IS_ERR(clk_pruss)) {
> + dev_err(&pdev->dev, "no clock available: pruss\n");
> + err = -ENODEV;
> + goto probe_exit_clk;
> + }
> + priv->clk_freq_pru = clk_get_rate(clk_pruss);
> + clk_put(clk_pruss);
why do you put the clock here?
> +
> + err = register_candev(ndev);
> + if (err) {
> + dev_err(&pdev->dev, "register_candev() failed\n");
> + err = -ENODEV;
> + goto probe_exit_clk;
> + }
> +
> + err = request_firmware(&fw_tx, "PRU_CAN_Emulation_Tx.bin",
> + &pdev->dev);
> + if (err) {
> + dev_err(&pdev->dev, "can't load firmware\n");
> + err = -ENODEV;
> + goto probe_exit_clk;
> + }
> +
> + dev_info(&pdev->dev, "fw_tx size %d. downloading...\n", fw_tx->size);
> +
> + err = request_firmware(&fw_rx, "PRU_CAN_Emulation_Rx.bin",
> + &pdev->dev);
> + if (err) {
> + dev_err(&pdev->dev, "can't load firmware\n");
> + err = -ENODEV;
> + goto probe_release_fw;
> + }
> + dev_info(&pdev->dev, "fw_rx size %d. downloading...\n", fw_rx->size);
> +
> + /* init the pru */
> + pru_can_emu_init(priv->dev, priv->can.clock.freq);
> + udelay(200);
> +
> + netif_napi_add(ndev, &priv->napi, pru_can_rx_poll,
> + PRUSS_DEF_NAPI_WEIGHT);
personally I'd wait to add the interface to napi until the firmware is
loaded.
> +
> + pruss_enable(priv->dev, PRUSS_CAN_RX_PRU_0);
> + pruss_enable(priv->dev, PRUSS_CAN_TX_PRU_1);
> +
> + /* download firmware into pru */
> + err = pruss_load(priv->dev, PRUSS_CAN_RX_PRU_0,
> + (u32 *)fw_rx->data, (fw_rx->size / 4));
> + if (err) {
> + dev_err(&pdev->dev, "firmware download error\n");
> + err = -ENODEV;
> + goto probe_release_fw_1;
> + }
> + release_firmware(fw_rx);
> + err = pruss_load(priv->dev, PRUSS_CAN_TX_PRU_1,
> + (u32 *)fw_tx->data, (fw_tx->size / 4));
> + if (err) {
> + dev_err(&pdev->dev, "firmware download error\n");
> + err = -ENODEV;
> + goto probe_release_fw_1;
> + }
> + release_firmware(fw_tx);
> +
> + pruss_run(priv->dev, PRUSS_CAN_RX_PRU_0);
> + pruss_run(priv->dev, PRUSS_CAN_TX_PRU_1);
> +
> + dev_info(&pdev->dev,
> + "%s device registered (trx_irq = %d, clk = %d)\n",
> + PRUSS_CAN_DRV_NAME, priv->trx_irq, priv->can.clock.freq);
> +
> + return 0;
> +
> +probe_release_fw_1:
> + release_firmware(fw_rx);
> +probe_release_fw:
> + release_firmware(fw_tx);
> +probe_exit_clk:
> + clk_put(priv->clk_timer);
> +probe_exit_candev:
> + if (NULL != ndev)
> + free_candev(ndev);
> +probe_exit:
> + return err;
> +}
> +
> +static int __devexit pru_can_remove(struct platform_device *pdev)
> +{
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + struct can_emu_priv *priv = netdev_priv(ndev);
> +
> + pru_can_stop(ndev);
> + pru_can_emu_exit(priv->dev);
> + clk_put(priv->clk_timer);
> + unregister_candev(ndev);
> + free_candev(ndev);
> + platform_set_drvdata(pdev, NULL);
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int pru_can_suspend(struct platform_device *pdev,
> + pm_message_t mesg)
> +{
> + dev_info(&pdev->dev, "%s not yet implemented\n", __func__);
> + return 0;
> +}
> +
> +static int pru_can_resume(struct platform_device *pdev)
> +{
> + dev_info(&pdev->dev, "%s not yet implemented\n", __func__);
> + return 0;
> +}
> +#else
> +#define pru_can_suspend NULL
> +#define pru_can_resume NULL
> +#endif
> +
> +static struct platform_driver omapl_pru_can_driver = {
> + .probe = pru_can_probe,
> + .remove = __devexit_p(pru_can_remove),
> + .suspend = pru_can_suspend,
> + .resume = pru_can_resume,
> + .driver = {
> + .name = PRUSS_CAN_DRV_NAME,
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static int __init pru_can_init(void)
> +{
> + pr_debug(KERN_INFO PRUSS_CAN_DRV_DESC "\n");
> + return platform_driver_register(&omapl_pru_can_driver);
> +}
> +
> +module_init(pru_can_init);
> +
> +static void __exit pru_can_exit(void)
> +{
> + pr_debug(KERN_INFO PRUSS_CAN_DRV_DESC " unloaded\n");
> + platform_driver_unregister(&omapl_pru_can_driver);
> +}
> +
> +module_exit(pru_can_exit);
> +
> +MODULE_AUTHOR("Subhasish Ghosh <subhasish-EvXpCiN+lbve9wHmmfpqLFaTQe2KTcn/@public.gmane.org>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("omapl pru CAN netdevice driver");
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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
[-- Attachment #2: Type: text/plain, Size: 188 bytes --]
_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core
^ permalink raw reply
* Re: [PATCHv5] usbnet: Resubmit interrupt URB once if halted
From: Paul Stewart @ 2011-04-22 15:59 UTC (permalink / raw)
To: Alan Stern
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
oliver-GvhC2dPhHPQdnm+yROfE0A, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
bhutchings-s/n/eUQHGBpZroRs9YW3xA
In-Reply-To: <Pine.LNX.4.44L0.1104221129530.1877-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
On Fri, Apr 22, 2011 at 8:47 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Tue, 19 Apr 2011, Paul Stewart wrote:
>
>> Resubmit interrupt URB if device is open. Use a flag set in
>> usbnet_open() to determine this state. Also kill and free
>> interrupt URB in usbnet_disconnect().
>
> Generally good, but a couple of things are questionable...
>
>> Signed-off-by: Paul Stewart <pstew-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>> drivers/net/usb/usbnet.c | 14 ++++++++++++++
>> include/linux/usb/usbnet.h | 1 +
>> 2 files changed, 15 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index 02d25c7..c7cf4af 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -643,6 +643,7 @@ static int usbnet_open (struct net_device *net)
>> }
>> }
>>
>> + set_bit(EVENT_DEV_OPEN, &dev->flags);
>> netif_start_queue (net);
>> if (netif_msg_ifup (dev)) {
>> char *framing;
>
> You forgot to clear this flag in usbnet_stop().
Actually, I didn't. Note that flags are set to 0 in usbnet_stop().
>
> By the way, there is FLAG_AVOID_UNLINK_URBS defined in usbnet.h and
> used in usbnet_stop(). Is it meant to apply to the interrupt URB as
> well as the others?
I don't know who to ask about that. Whether or not the flag is set,
in practice the interrupt URB still seems to be killed by some other
facility, so regardless of that request, they may lose anyway.
>> @@ -1105,6 +1106,11 @@ void usbnet_disconnect (struct usb_interface *intf)
>> if (dev->driver_info->unbind)
>> dev->driver_info->unbind (dev, intf);
>>
>> + if (dev->interrupt) {
>> + usb_kill_urb(dev->interrupt);
>> + usb_free_urb(dev->interrupt);
>> + }
>> +
>
> usb_kill_urb and usb_free_urb include their own tests for urb == NULL;
> you don't need to test it here or below.
Okay.
>
>> free_netdev(net);
>> usb_put_dev (xdev);
>> }
>> @@ -1285,6 +1291,10 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
>> * wake the device
>> */
>> netif_device_attach (dev->net);
>> +
>> + /* Stop interrupt URBs */
>> + if (dev->interrupt)
>> + usb_kill_urb(dev->interrupt);
>> }
>> return 0;
>> }
>
> There is a subtle question here: When is the best time to kill the
> interrupt URB? Without knowing any of the details, I'd guess that the
> interrupt URB reports asynchronous events and the driver could run into
> trouble if one of those events occurred while everything else was
> turned off. This suggests that the interrupt URB should be killed as
> soon as possible rather than as late as possible. But maybe it doesn't
> matter; it all depends on the design of the driver.
I'm not sure I can answer this question either. As it stands, nobody
was killing them before. Just trying to make it better.
>
>> @@ -1297,6 +1307,10 @@ int usbnet_resume (struct usb_interface *intf)
>> if (!--dev->suspend_count)
>> tasklet_schedule (&dev->bh);
>>
>> + /* resume interrupt URBs */
>> + if (dev->interrupt && test_bit(EVENT_DEV_OPEN, &dev->flags))
>> + usb_submit_urb(dev->interrupt, GFP_NOIO);
>> +
>> return 0;
>> }
>
> Alan Stern
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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
* [PATCH] net: allow user to change NETIF_F_HIGHDMA
From: Michał Mirosław @ 2011-04-22 16:31 UTC (permalink / raw)
To: netdev
NETIF_F_HIGHDMA is like any other TX offloads, so allow user to toggle it.
This is needed later for bridge and bonding convertsion to hw_features.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
include/linux/netdevice.h | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index cb8178a..405ce21 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1080,7 +1080,7 @@ struct net_device {
/* Features valid for ethtool to change */
/* = all defined minus driver/device-class-related */
-#define NETIF_F_NEVER_CHANGE (NETIF_F_HIGHDMA | NETIF_F_VLAN_CHALLENGED | \
+#define NETIF_F_NEVER_CHANGE (NETIF_F_VLAN_CHALLENGED | \
NETIF_F_LLTX | NETIF_F_NETNS_LOCAL)
#define NETIF_F_ETHTOOL_BITS (0x7f3fffff & ~NETIF_F_NEVER_CHANGE)
@@ -1098,6 +1098,7 @@ struct net_device {
#define NETIF_F_ALL_TX_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_SG | \
NETIF_F_FRAGLIST | NETIF_F_ALL_TSO | \
+ NETIF_F_HIGHDMA | \
NETIF_F_SCTP_CSUM | NETIF_F_FCOE_CRC)
/*
--
1.7.2.5
^ permalink raw reply related
* [PATCH] net: fix netdev_increment_features()
From: Michał Mirosław @ 2011-04-22 16:31 UTC (permalink / raw)
To: netdev
Cc: Ben Hutchings, Jay Vosburgh, Andy Gospodarek, Stephen Hemminger,
bridge
Simplify and fix netdev_increment_features() to conform to what is
stated in netdevice.h comments about NETIF_F_ONE_FOR_ALL.
Include FCoE segmentation and VLAN-challedged flags in computation.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
I noticed this while converting bridge code to hw_features. After
adding devices to the bridge all features were always cleared regardless
of features active in those devices.
include/linux/netdevice.h | 7 ++++++-
net/core/dev.c | 37 ++++++++++++-------------------------
2 files changed, 18 insertions(+), 26 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 405ce21..86140e4 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1107,7 +1107,12 @@ struct net_device {
*/
#define NETIF_F_ONE_FOR_ALL (NETIF_F_GSO_SOFTWARE | NETIF_F_GSO_ROBUST | \
NETIF_F_SG | NETIF_F_HIGHDMA | \
- NETIF_F_FRAGLIST)
+ NETIF_F_FRAGLIST | NETIF_F_VLAN_CHALLENGED)
+ /*
+ * If one device doesn't support one of these features, then disable it
+ * for all in netdev_increment_features.
+ */
+#define NETIF_F_ALL_FOR_ALL (NETIF_F_NOCACHE_COPY | NETIF_F_FSO)
/* changeable features with no special hardware requirements */
#define NETIF_F_SOFT_FEATURES (NETIF_F_GSO | NETIF_F_GRO)
diff --git a/net/core/dev.c b/net/core/dev.c
index 3871bf6..1a2c91c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6184,33 +6184,20 @@ static int dev_cpu_callback(struct notifier_block *nfb,
*/
u32 netdev_increment_features(u32 all, u32 one, u32 mask)
{
+ if (mask & NETIF_F_GEN_CSUM)
+ mask |= NETIF_F_ALL_CSUM;
+ mask |= NETIF_F_VLAN_CHALLENGED;
+
+ all |= one & (NETIF_F_ONE_FOR_ALL|NETIF_F_ALL_CSUM) & mask;
+ all &= one | ~NETIF_F_ALL_FOR_ALL;
+
/* If device needs checksumming, downgrade to it. */
- if (all & NETIF_F_NO_CSUM && !(one & NETIF_F_NO_CSUM))
- all ^= NETIF_F_NO_CSUM | (one & NETIF_F_ALL_CSUM);
- else if (mask & NETIF_F_ALL_CSUM) {
- /* If one device supports v4/v6 checksumming, set for all. */
- if (one & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) &&
- !(all & NETIF_F_GEN_CSUM)) {
- all &= ~NETIF_F_ALL_CSUM;
- all |= one & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
- }
+ if (all & (NETIF_F_ALL_CSUM & ~NETIF_F_NO_CSUM))
+ all &= ~NETIF_F_NO_CSUM;
- /* If one device supports hw checksumming, set for all. */
- if (one & NETIF_F_GEN_CSUM && !(all & NETIF_F_GEN_CSUM)) {
- all &= ~NETIF_F_ALL_CSUM;
- all |= NETIF_F_HW_CSUM;
- }
- }
-
- /* If device can't no cache copy, don't do for all */
- if (!(one & NETIF_F_NOCACHE_COPY))
- all &= ~NETIF_F_NOCACHE_COPY;
-
- one |= NETIF_F_ALL_CSUM;
-
- one |= all & NETIF_F_ONE_FOR_ALL;
- all &= one | NETIF_F_LLTX | NETIF_F_GSO | NETIF_F_UFO;
- all |= one & mask & NETIF_F_ONE_FOR_ALL;
+ /* If one device supports hw checksumming, set for all. */
+ if (all & NETIF_F_GEN_CSUM)
+ all &= ~(NETIF_F_ALL_CSUM & ~NETIF_F_GEN_CSUM);
return all;
}
--
1.7.2.5
^ permalink raw reply related
* [PATCH] bridge: convert br_features_recompute() to ndo_fix_features
From: Michał Mirosław @ 2011-04-22 16:31 UTC (permalink / raw)
To: netdev; +Cc: Stephen Hemminger, bridge
Note: netdev_update_features() needs only rtnl_lock as br->port_list
is only changed while holding it.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
net/bridge/br_device.c | 61 ++++++----------------------------------------
net/bridge/br_if.c | 17 +++++++------
net/bridge/br_notify.c | 5 +---
net/bridge/br_private.h | 3 +-
4 files changed, 19 insertions(+), 67 deletions(-)
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 45cfd54..a6b2f86 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -90,8 +90,7 @@ static int br_dev_open(struct net_device *dev)
struct net_bridge *br = netdev_priv(dev);
netif_carrier_off(dev);
-
- br_features_recompute(br);
+ netdev_update_features(dev);
netif_start_queue(dev);
br_stp_enable_bridge(br);
br_multicast_open(br);
@@ -188,48 +187,11 @@ static void br_getinfo(struct net_device *dev, struct ethtool_drvinfo *info)
strcpy(info->bus_info, "N/A");
}
-static int br_set_sg(struct net_device *dev, u32 data)
+static u32 br_fix_features(struct net_device *dev, u32 features)
{
struct net_bridge *br = netdev_priv(dev);
- if (data)
- br->feature_mask |= NETIF_F_SG;
- else
- br->feature_mask &= ~NETIF_F_SG;
-
- br_features_recompute(br);
- return 0;
-}
-
-static int br_set_tso(struct net_device *dev, u32 data)
-{
- struct net_bridge *br = netdev_priv(dev);
-
- if (data)
- br->feature_mask |= NETIF_F_TSO;
- else
- br->feature_mask &= ~NETIF_F_TSO;
-
- br_features_recompute(br);
- return 0;
-}
-
-static int br_set_tx_csum(struct net_device *dev, u32 data)
-{
- struct net_bridge *br = netdev_priv(dev);
-
- if (data)
- br->feature_mask |= NETIF_F_NO_CSUM;
- else
- br->feature_mask &= ~NETIF_F_ALL_CSUM;
-
- br_features_recompute(br);
- return 0;
-}
-
-static int br_set_flags(struct net_device *netdev, u32 data)
-{
- return ethtool_op_set_flags(netdev, data, ETH_FLAG_TXVLAN);
+ return br_features_recompute(br, features);
}
#ifdef CONFIG_NET_POLL_CONTROLLER
@@ -330,16 +292,6 @@ static int br_del_slave(struct net_device *dev, struct net_device *slave_dev)
static const struct ethtool_ops br_ethtool_ops = {
.get_drvinfo = br_getinfo,
.get_link = ethtool_op_get_link,
- .get_tx_csum = ethtool_op_get_tx_csum,
- .set_tx_csum = br_set_tx_csum,
- .get_sg = ethtool_op_get_sg,
- .set_sg = br_set_sg,
- .get_tso = ethtool_op_get_tso,
- .set_tso = br_set_tso,
- .get_ufo = ethtool_op_get_ufo,
- .set_ufo = ethtool_op_set_ufo,
- .get_flags = ethtool_op_get_flags,
- .set_flags = br_set_flags,
};
static const struct net_device_ops br_netdev_ops = {
@@ -359,6 +311,7 @@ static const struct net_device_ops br_netdev_ops = {
#endif
.ndo_add_slave = br_add_slave,
.ndo_del_slave = br_del_slave,
+ .ndo_fix_features = br_fix_features,
};
static void br_dev_free(struct net_device *dev)
@@ -389,7 +342,10 @@ void br_dev_setup(struct net_device *dev)
dev->features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HIGHDMA |
NETIF_F_GSO_MASK | NETIF_F_NO_CSUM | NETIF_F_LLTX |
- NETIF_F_NETNS_LOCAL | NETIF_F_GSO | NETIF_F_HW_VLAN_TX;
+ NETIF_F_NETNS_LOCAL | NETIF_F_HW_VLAN_TX;
+ dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HIGHDMA |
+ NETIF_F_GSO_MASK | NETIF_F_NO_CSUM |
+ NETIF_F_HW_VLAN_TX;
br->dev = dev;
spin_lock_init(&br->lock);
@@ -401,7 +357,6 @@ void br_dev_setup(struct net_device *dev)
memcpy(br->group_addr, br_group_address, ETH_ALEN);
- br->feature_mask = dev->features;
br->stp_enabled = BR_NO_STP;
br->designated_root = br->bridge_id;
br->bridge_max_age = br->max_age = 20 * HZ;
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 7f5379c..1156460 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -291,15 +291,15 @@ int br_min_mtu(const struct net_bridge *br)
/*
* Recomputes features using slave's features
*/
-void br_features_recompute(struct net_bridge *br)
+u32 br_features_recompute(struct net_bridge *br, u32 features)
{
struct net_bridge_port *p;
- u32 features, mask;
+ u32 mask;
- features = mask = br->feature_mask;
if (list_empty(&br->port_list))
- goto done;
+ return features;
+ mask = features;
features &= ~NETIF_F_ONE_FOR_ALL;
list_for_each_entry(p, &br->port_list, list) {
@@ -307,8 +307,7 @@ void br_features_recompute(struct net_bridge *br)
p->dev->features, mask);
}
-done:
- br->dev->features = netdev_fix_features(br->dev, features);
+ return features;
}
/* called with RTNL */
@@ -373,9 +372,10 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
list_add_rcu(&p->list, &br->port_list);
+ netdev_update_features(br->dev);
+
spin_lock_bh(&br->lock);
changed_addr = br_stp_recalculate_bridge_id(br);
- br_features_recompute(br);
if ((dev->flags & IFF_UP) && netif_carrier_ok(dev) &&
(br->dev->flags & IFF_UP))
@@ -423,9 +423,10 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
spin_lock_bh(&br->lock);
br_stp_recalculate_bridge_id(br);
- br_features_recompute(br);
spin_unlock_bh(&br->lock);
+ netdev_update_features(br->dev);
+
return 0;
}
diff --git a/net/bridge/br_notify.c b/net/bridge/br_notify.c
index 606b323..6545ee9 100644
--- a/net/bridge/br_notify.c
+++ b/net/bridge/br_notify.c
@@ -66,10 +66,7 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
break;
case NETDEV_FEAT_CHANGE:
- spin_lock_bh(&br->lock);
- if (netif_running(br->dev))
- br_features_recompute(br);
- spin_unlock_bh(&br->lock);
+ netdev_update_features(br->dev);
break;
case NETDEV_DOWN:
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index e2a4034..54578f2 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -183,7 +183,6 @@ struct net_bridge
struct br_cpu_netstats __percpu *stats;
spinlock_t hash_lock;
struct hlist_head hash[BR_HASH_SIZE];
- u32 feature_mask;
#ifdef CONFIG_BRIDGE_NETFILTER
struct rtable fake_rtable;
bool nf_call_iptables;
@@ -379,7 +378,7 @@ extern int br_add_if(struct net_bridge *br,
extern int br_del_if(struct net_bridge *br,
struct net_device *dev);
extern int br_min_mtu(const struct net_bridge *br);
-extern void br_features_recompute(struct net_bridge *br);
+extern u32 br_features_recompute(struct net_bridge *br, u32 features);
/* br_input.c */
extern int br_handle_frame_finish(struct sk_buff *skb);
--
1.7.2.5
^ permalink raw reply related
* Re: [PATCH] net: allow user to change NETIF_F_HIGHDMA
From: Stephen Hemminger @ 2011-04-22 16:38 UTC (permalink / raw)
To: Michał Mirosław; +Cc: netdev
In-Reply-To: <20110422163116.37DE613909@rere.qmqm.pl>
On Fri, 22 Apr 2011 18:31:16 +0200 (CEST)
Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> NETIF_F_HIGHDMA is like any other TX offloads, so allow user to toggle it.
> This is needed later for bridge and bonding convertsion to hw_features.
>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
This was meant to be a hidden flag that that reflects capability of
hardware. Do yo really mean to allow user to disable it via ethtool?
^ permalink raw reply
* Re: [PATCH] net: allow user to change NETIF_F_HIGHDMA
From: Michał Mirosław @ 2011-04-22 16:48 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20110422093850.13df40ce@nehalam>
On Fri, Apr 22, 2011 at 09:38:50AM -0700, Stephen Hemminger wrote:
> On Fri, 22 Apr 2011 18:31:16 +0200 (CEST)
> Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> > NETIF_F_HIGHDMA is like any other TX offloads, so allow user to toggle it.
> > This is needed later for bridge and bonding convertsion to hw_features.
> This was meant to be a hidden flag that that reflects capability of
> hardware. Do yo really mean to allow user to disable it via ethtool?
Yes. BTW, you can say the same about e.g. NETIF_F_SG. There's no harm in
user being able to disable HIGHDMA (except obvious performance issue when
disabled -- as with other TX offloads).
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: Re: [v2 PATCH 0/6] IPVS: init and cleanup.
From: Julian Anastasov @ 2011-04-22 16:49 UTC (permalink / raw)
To: Hans Schillstrom
Cc: horms, ebiederm, lvs-devel, netdev, netfilter-devel,
hans.schillstrom
In-Reply-To: <1pukl91.dee882dc58d0fe53f4e800b09c9f9eab@obelix.schillstrom.com>
Hello,
On Fri, 22 Apr 2011, Hans Schillstrom wrote:
> Hello Julian
>
> >From what I can see there is nothing in the rs_table except for NAT:ed dest, or ?
> so the code fragment below will not work.
>
> I.e. my double list_for_each that goes throug the service table must be there...
> or do we need both?
Yes, I now see that dests are added to rs_table only for
NAT. So, we have no choice, we have to walk svc->destinations.
To remove usage of dest_trash in ip_vs_dst_event we can move
the ip_vs_dst_reset call in __ip_vs_del_dest() above the
atomic_dec_and_test check. But it is not a safe operation.
While with svc->usecnt we know if packet accesses the svc,
the same for dest is not possible. Packet can hit existing
connection (without searching for svc), conn has reference to
dest, so the packet can refresh dest->dst_cache at any point
of time, eg. while we move dest to trash. That is why
__ip_vs_del_dest calls ip_vs_dst_reset only when dest->refcnt
reaches 0, i.e. when no conn refers to dest. So, even if we
call ip_vs_dst_reset() before the atomic_dec_and_test check,
it is still possible some packet to attach new dst_cache
entry. As result, we can not assume that dests in trash have
no dst_cache entry. So, ip_vs_dst_event() needs to walk
both svc->destinations and ipvs->dest_trash under mutex lock.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [PATCH V3 0/8] macvtap/vhost TX zero copy support
From: Shirley Ma @ 2011-04-22 17:31 UTC (permalink / raw)
To: Jon Mason
Cc: David Miller, mst, Eric Dumazet, Avi Kivity, Arnd Bergmann,
netdev, kvm, linux-kernel
In-Reply-To: <BANLkTi=ckukmCdOOvQc29vtYgxeiSMGqBw@mail.gmail.com>
On Thu, 2011-04-21 at 10:29 -0400, Jon Mason wrote:
> Why are only these 3 drivers getting support? As far as I can tell,
> the only requirement is HIGHDMA. If this is the case, is there really
> a need for an additional flag to support this? If you can key off of
> HIGHDMA, all devices that support this would get the benefit.
Agreed. So far, I have only verified these three 10Gb NICs we have in
our lab. We can enable all devices which support HIGHDMA.
Thanks
Shirley
^ permalink raw reply
* Re: [PATCH V3 0/8] macvtap/vhost TX zero copy support
From: Shirley Ma @ 2011-04-22 17:52 UTC (permalink / raw)
To: David Miller
Cc: mst, Eric Dumazet, Avi Kivity, Arnd Bergmann, netdev, kvm,
linux-kernel
In-Reply-To: <1303328216.19336.18.camel@localhost.localdomain>
On Wed, 2011-04-20 at 12:36 -0700, Shirley Ma wrote:
> I am collecting more test results against 2.6.39-rc3 kernel and will
> provide the test matrix later.
Single TCP_STREAM 120 secs test results over ixgbe 10Gb NIC results:
Message BW(Gb/s)qemu-kvm (NumCPU)vhost-net(NumCPU) PerfTop irq/s
4K 7408.57 92.1% 22.6% 1229
4K(Orig)4913.17 118.1% 84.1% 2086
8K 9129.90 89.3% 23.3% 1141
8K(Orig)7094.55 115.9% 84.7% 2157
16K 9178.81 89.1% 23.3% 1139
16K(Orig)8927.1 118.7% 83.4% 2262
64K 9171.43 88.4% 24.9% 1253
64K(Orig)9085.85 115.9% 82.4% 2229
For message size less or equal than 2K, there is a known KVM guest TX
overrun issue. With this zerocopy patch, the issue becomes more severe,
guest io_exits has tripled than before, so the performance is not good.
Once the TX overrun problem has been addressed, I will retest the small
message size performance.
Thanks
Shirley
^ permalink raw reply
* Re: [PATCHv6] usbnet: Resubmit interrupt URB if device is open
From: Alan Stern @ 2011-04-22 17:57 UTC (permalink / raw)
To: Paul Stewart
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, Oliver Neukum,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, bhutchings-s/n/eUQHGBpZroRs9YW3xA,
USB list
In-Reply-To: <20110422161813.55A0D20334-6A69KNNYBwgF248FYctl9mCaruZE5nAUZeezCHUQhQ4@public.gmane.org>
On Tue, 19 Apr 2011, Paul Stewart wrote:
> Resubmit interrupt URB if device is open. Use a flag set in
> usbnet_open() to determine this state. Also kill and free
> interrupt URB in usbnet_disconnect().
>
> Signed-off-by: Paul Stewart <pstew-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> drivers/net/usb/usbnet.c | 11 +++++++++++
> include/linux/usb/usbnet.h | 1 +
> 2 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 02d25c7..b4572c3 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -643,6 +643,7 @@ static int usbnet_open (struct net_device *net)
> }
> }
>
> + set_bit(EVENT_DEV_OPEN, &dev->flags);
> netif_start_queue (net);
> if (netif_msg_ifup (dev)) {
> char *framing;
> @@ -1105,6 +1106,9 @@ void usbnet_disconnect (struct usb_interface *intf)
> if (dev->driver_info->unbind)
> dev->driver_info->unbind (dev, intf);
>
> + usb_kill_urb(dev->interrupt);
> + usb_free_urb(dev->interrupt);
> +
> free_netdev(net);
> usb_put_dev (xdev);
> }
> @@ -1285,6 +1289,9 @@ int usbnet_suspend (struct usb_interface *intf, pm_message_t message)
> * wake the device
> */
> netif_device_attach (dev->net);
> +
> + /* Stop interrupt URBs */
> + usb_kill_urb(dev->interrupt);
> }
> return 0;
> }
> @@ -1297,6 +1304,10 @@ int usbnet_resume (struct usb_interface *intf)
> if (!--dev->suspend_count)
> tasklet_schedule (&dev->bh);
>
> + /* resume interrupt URBs */
> + if (test_bit(EVENT_DEV_OPEN, &dev->flags))
> + usb_submit_urb(dev->interrupt, GFP_NOIO);
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(usbnet_resume);
> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> index ba09fe8..d148cca 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -64,6 +64,7 @@ struct usbnet {
> # define EVENT_RX_MEMORY 2
> # define EVENT_STS_SPLIT 3
> # define EVENT_LINK_RESET 4
> +# define EVENT_DEV_OPEN 5
> };
>
> static inline struct usb_driver *driver_of(struct usb_interface *intf)
I have no more objections, but this really should be reviewed by
somebody who has a better understanding of usbnet. Such as Oliver.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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-2.6] inet: constify ip headers and in6_addr
From: David Miller @ 2011-04-22 18:04 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1303483982.3134.131.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 22 Apr 2011 16:53:02 +0200
> Add const qualifiers to structs iphdr, ipv6hdr and in6_addr pointers
> where possible, to make code intention more obvious.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCHv6] usbnet: Resubmit interrupt URB if device is open
From: David Miller @ 2011-04-22 18:07 UTC (permalink / raw)
To: pstew-F7+t8E8rja9g9hUCZPvPmw
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz,
linux-usb-u79uwXL29TY76Z2rM5mHXA, oliver-GvhC2dPhHPQdnm+yROfE0A,
bhutchings-s/n/eUQHGBpZroRs9YW3xA
In-Reply-To: <20110422161813.55A0D20334-6A69KNNYBwgF248FYctl9mCaruZE5nAUZeezCHUQhQ4@public.gmane.org>
Please fix the date on your email postings.
Even in the context of the "date" of your commit, you're making changes
and updates even right now, so the date should be updating to be more
in line with "right now"
Yet all of your postings still are dated April 19th.
This screws up my patch queue at:
http://patchwork.ozlabs.org/project/netdev/list/
because instead of your new patches showing up at the top where they
belong, they sneak in near the end and that's a real pain.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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
* [PATCH] netconsole: fix deadlock when removing net driver that netconsole is using (v2)
From: Neil Horman @ 2011-04-22 18:10 UTC (permalink / raw)
To: netdev; +Cc: Neil Horman, David S. Miller
In-Reply-To: <1303481216-10348-1-git-send-email-nhorman@tuxdriver.com>
A deadlock was reported to me recently that occured when netconsole was being
used in a virtual guest. If the virtio_net driver was removed while netconsole
was setup to use an interface that was driven by that driver, the guest
deadlocked. No backtrace was provided because netconsole was the only console
configured, but it became clear pretty quickly what the problem was. In
netconsole_netdev_event, if we get an unregister event, we call
__netpoll_cleanup with the target_list_lock held and irqs disabled.
__netpoll_cleanup can, if pending netpoll packets are waiting call
cancel_delayed_work_sync, which is a sleeping path. the might_sleep call in
that path gets triggered, causing a console warning to be issued. The
netconsole write handler of course tries to take the target_list_lock again,
which we already hold, causing deadlock.
The fix is pretty striaghtforward. Simply drop the target_list_lock and
re-enable irqs prior to calling __netpoll_cleanup, the re-acquire the lock, and
restart the loop. Confirmed by myself to fix the problem reported.
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "David S. Miller" <davem@davemloft.net>
---
drivers/net/netconsole.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index dfb67eb..eb41e44 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -671,6 +671,7 @@ static int netconsole_netdev_event(struct notifier_block *this,
goto done;
spin_lock_irqsave(&target_list_lock, flags);
+restart:
list_for_each_entry(nt, &target_list, list) {
netconsole_target_get(nt);
if (nt->np.dev == dev) {
@@ -683,9 +684,16 @@ static int netconsole_netdev_event(struct notifier_block *this,
* rtnl_lock already held
*/
if (nt->np.dev) {
+ spin_unlock_irqrestore(
+ &target_list_lock,
+ flags);
__netpoll_cleanup(&nt->np);
+ spin_lock_irqsave(&target_list_lock,
+ flags);
dev_put(nt->np.dev);
nt->np.dev = NULL;
+ netconsole_target_put(nt);
+ goto restart;
}
/* Fall through */
case NETDEV_GOING_DOWN:
--
1.7.4.4
^ permalink raw reply related
* Re: [PATCH] netconsole: fix deadlock when removing net driver that netconsole is using (v2)
From: David Miller @ 2011-04-22 18:40 UTC (permalink / raw)
To: nhorman; +Cc: netdev
In-Reply-To: <1303495859-12279-1-git-send-email-nhorman@tuxdriver.com>
From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 22 Apr 2011 14:10:59 -0400
> @@ -683,9 +684,16 @@ static int netconsole_netdev_event(struct notifier_block *this,
> * rtnl_lock already held
> */
> if (nt->np.dev) {
> + spin_unlock_irqrestore(
> + &target_list_lock,
> + flags);
> __netpoll_cleanup(&nt->np);
> + spin_lock_irqsave(&target_list_lock,
> + flags);
> dev_put(nt->np.dev);
> nt->np.dev = NULL;
> + netconsole_target_put(nt);
> + goto restart;
If you drop the lock here, another cpu can put the device and set it
to NULL.
In which case you'll double release when you regrab the lock here.
Too bad you can't NULL out the device before dropping the lock,
because that way would be able to ensure you'd be the only releaser.
But that won't work because __netpoll_cleanup() wants the device
pointer to be set.
Hmmm...
^ permalink raw reply
* (unknown),
From: Dr. Clarke Harrison @ 2011-04-22 19:23 UTC (permalink / raw)
British National Lottery
United Kingdom.
P.O Box 789 Harrogate,HG1 2YR
International powerball Online Lotto Program.
WINNING NUMBER 04-23-39-49-50 (39)
NOTIFICATION OF WINNING
Attn:
You won the sum of £2.4 Million pounds! GBP in our monthly sweepstakes,
just concluded April 21th, you are hereby advice to get back to us, to claimed
your prize.
Requirements:
1.First Name
2 Last Name :
3.Home Address:
4.Age:
5.Sex:
6.Occupation:
7.Phone Number:
8.Country Of Residence.
Contact Agent: Dr. Clarke Harrison
E-mail: powerball_winonline@hotmail.com
Regards,
Mrs. Fisher schmitz
British Powerball Lottery
United Kingdom
^ permalink raw reply
* pull request: wireless-2.6 2011-04-22
From: John W. Linville @ 2011-04-22 20:11 UTC (permalink / raw)
To: davem; +Cc: linux-wireless, netdev, linux-kernel
Dave,
Here are some fixes intended for 2.6.39...
This includes a Bluetooth pull, described thusly by Gustavo:
Four small fixes for 2.6.39. An incorrect refcnt balance for
hci connection by Ville Tervo, a warning fix in the command
timer by Vinicius Gomes, an ERTM fix by Ruiyi Zhang, and SCO
synchronization fix by Luiz Augusto von Dentz.
On top of those, there are some use-after-free fixes from Stansilaw for
iwlegacy and iwlwifi, a mac80211 debugfs locking fix from Johannes, and
a small logic fix for the return code of ath9k's ah_stoprecv from Felix,
and an iwlwifi fix for frame injection from Daniel Halperin.
Please let me know if there are problems!
John
---
The following changes since commit f01cb5fbea1c1613621f9f32f385e12c1a29dde0:
Revert "bridge: Forward reserved group addresses if !STP" (2011-04-21 21:17:25 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-2.6.git master
Daniel Halperin (1):
iwlwifi: fix frame injection for HT channels
Felix Fietkau (1):
ath9k: fix the return value of ath_stoprecv
Johannes Berg (1):
mac80211: fix SMPS debugfs locking
John W. Linville (1):
Merge branch 'master' of git://git.kernel.org/.../padovan/bluetooth-2.6
Luiz Augusto von Dentz (1):
Bluetooth: fix shutdown on SCO sockets
Ruiyi Zhang (1):
Bluetooth: Only keeping SAR bits when retransmitting one frame.
Stanislaw Gruszka (2):
iwlwifi: fix skb usage after free
iwl4965: fix skb usage after free
Ville Tervo (1):
Bluetooth: Fix refcount balance for hci connection
Vinicius Costa Gomes (1):
Bluetooth: Fix keeping the command timer running
drivers/net/wireless/ath/ath9k/recv.c | 2 +-
drivers/net/wireless/iwlegacy/iwl-4965-tx.c | 10 +++++++---
drivers/net/wireless/iwlwifi/iwl-agn-rxon.c | 7 +------
drivers/net/wireless/iwlwifi/iwl-agn-tx.c | 10 +++++++---
net/bluetooth/hci_core.c | 5 ++---
net/bluetooth/hci_event.c | 2 --
net/bluetooth/l2cap_core.c | 1 +
net/bluetooth/sco.c | 9 +++++++++
net/mac80211/cfg.c | 2 ++
net/mac80211/debugfs_netdev.c | 4 ++--
10 files changed, 32 insertions(+), 20 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index dcd19bc..b29c80d 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -506,7 +506,7 @@ bool ath_stoprecv(struct ath_softc *sc)
"confusing the DMA engine when we start RX up\n");
ATH_DBG_WARN_ON_ONCE(!stopped);
}
- return stopped || reset;
+ return stopped && !reset;
}
void ath_flushrecv(struct ath_softc *sc)
diff --git a/drivers/net/wireless/iwlegacy/iwl-4965-tx.c b/drivers/net/wireless/iwlegacy/iwl-4965-tx.c
index 5c40502..fbec88d 100644
--- a/drivers/net/wireless/iwlegacy/iwl-4965-tx.c
+++ b/drivers/net/wireless/iwlegacy/iwl-4965-tx.c
@@ -1127,12 +1127,16 @@ int iwl4965_tx_queue_reclaim(struct iwl_priv *priv, int txq_id, int index)
q->read_ptr = iwl_legacy_queue_inc_wrap(q->read_ptr, q->n_bd)) {
tx_info = &txq->txb[txq->q.read_ptr];
- iwl4965_tx_status(priv, tx_info,
- txq_id >= IWL4965_FIRST_AMPDU_QUEUE);
+
+ if (WARN_ON_ONCE(tx_info->skb == NULL))
+ continue;
hdr = (struct ieee80211_hdr *)tx_info->skb->data;
- if (hdr && ieee80211_is_data_qos(hdr->frame_control))
+ if (ieee80211_is_data_qos(hdr->frame_control))
nfreed++;
+
+ iwl4965_tx_status(priv, tx_info,
+ txq_id >= IWL4965_FIRST_AMPDU_QUEUE);
tx_info->skb = NULL;
priv->cfg->ops->lib->txq_free_tfd(priv, txq);
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-rxon.c b/drivers/net/wireless/iwlwifi/iwl-agn-rxon.c
index dfdbea6..fbbde07 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn-rxon.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn-rxon.c
@@ -335,7 +335,6 @@ int iwlagn_mac_config(struct ieee80211_hw *hw, u32 changed)
struct ieee80211_channel *channel = conf->channel;
const struct iwl_channel_info *ch_info;
int ret = 0;
- bool ht_changed[NUM_IWL_RXON_CTX] = {};
IWL_DEBUG_MAC80211(priv, "changed %#x", changed);
@@ -383,10 +382,8 @@ int iwlagn_mac_config(struct ieee80211_hw *hw, u32 changed)
for_each_context(priv, ctx) {
/* Configure HT40 channels */
- if (ctx->ht.enabled != conf_is_ht(conf)) {
+ if (ctx->ht.enabled != conf_is_ht(conf))
ctx->ht.enabled = conf_is_ht(conf);
- ht_changed[ctx->ctxid] = true;
- }
if (ctx->ht.enabled) {
if (conf_is_ht40_minus(conf)) {
@@ -455,8 +452,6 @@ int iwlagn_mac_config(struct ieee80211_hw *hw, u32 changed)
if (!memcmp(&ctx->staging, &ctx->active, sizeof(ctx->staging)))
continue;
iwlagn_commit_rxon(priv, ctx);
- if (ht_changed[ctx->ctxid])
- iwlagn_update_qos(priv, ctx);
}
out:
mutex_unlock(&priv->mutex);
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-tx.c b/drivers/net/wireless/iwlwifi/iwl-agn-tx.c
index a709d05..2dd7d54 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn-tx.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn-tx.c
@@ -1224,12 +1224,16 @@ int iwlagn_tx_queue_reclaim(struct iwl_priv *priv, int txq_id, int index)
q->read_ptr = iwl_queue_inc_wrap(q->read_ptr, q->n_bd)) {
tx_info = &txq->txb[txq->q.read_ptr];
- iwlagn_tx_status(priv, tx_info,
- txq_id >= IWLAGN_FIRST_AMPDU_QUEUE);
+
+ if (WARN_ON_ONCE(tx_info->skb == NULL))
+ continue;
hdr = (struct ieee80211_hdr *)tx_info->skb->data;
- if (hdr && ieee80211_is_data_qos(hdr->frame_control))
+ if (ieee80211_is_data_qos(hdr->frame_control))
nfreed++;
+
+ iwlagn_tx_status(priv, tx_info,
+ txq_id >= IWLAGN_FIRST_AMPDU_QUEUE);
tx_info->skb = NULL;
if (priv->cfg->ops->lib->txq_inval_byte_cnt_tbl)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 2216620..e7dced9 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -587,10 +587,8 @@ static int hci_dev_do_close(struct hci_dev *hdev)
hci_req_cancel(hdev, ENODEV);
hci_req_lock(hdev);
- /* Stop timer, it might be running */
- del_timer_sync(&hdev->cmd_timer);
-
if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
+ del_timer_sync(&hdev->cmd_timer);
hci_req_unlock(hdev);
return 0;
}
@@ -629,6 +627,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
/* Drop last sent command */
if (hdev->sent_cmd) {
+ del_timer_sync(&hdev->cmd_timer);
kfree_skb(hdev->sent_cmd);
hdev->sent_cmd = NULL;
}
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index cebe7588..b257015 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2387,8 +2387,6 @@ static inline void hci_io_capa_reply_evt(struct hci_dev *hdev, struct sk_buff *s
if (!conn)
goto unlock;
- hci_conn_hold(conn);
-
conn->remote_cap = ev->capability;
conn->remote_oob = ev->oob_data;
conn->remote_auth = ev->authentication;
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index ca27f3a..2c8dd44 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1051,6 +1051,7 @@ static void l2cap_retransmit_one_frame(struct sock *sk, u8 tx_seq)
tx_skb = skb_clone(skb, GFP_ATOMIC);
bt_cb(skb)->retries++;
control = get_unaligned_le16(tx_skb->data + L2CAP_HDR_SIZE);
+ control &= L2CAP_CTRL_SAR;
if (pi->conn_state & L2CAP_CONN_SEND_FBIT) {
control |= L2CAP_CTRL_FINAL;
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 42fdffd..94954c7 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -369,6 +369,15 @@ static void __sco_sock_close(struct sock *sk)
case BT_CONNECTED:
case BT_CONFIG:
+ if (sco_pi(sk)->conn) {
+ sk->sk_state = BT_DISCONN;
+ sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
+ hci_conn_put(sco_pi(sk)->conn->hcon);
+ sco_pi(sk)->conn = NULL;
+ } else
+ sco_chan_del(sk, ECONNRESET);
+ break;
+
case BT_CONNECT:
case BT_DISCONN:
sco_chan_del(sk, ECONNRESET);
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 3342135..4404973 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1504,6 +1504,8 @@ int __ieee80211_request_smps(struct ieee80211_sub_if_data *sdata,
enum ieee80211_smps_mode old_req;
int err;
+ lockdep_assert_held(&sdata->u.mgd.mtx);
+
old_req = sdata->u.mgd.req_smps;
sdata->u.mgd.req_smps = smps_mode;
diff --git a/net/mac80211/debugfs_netdev.c b/net/mac80211/debugfs_netdev.c
index dacace6..9ea7c0d 100644
--- a/net/mac80211/debugfs_netdev.c
+++ b/net/mac80211/debugfs_netdev.c
@@ -177,9 +177,9 @@ static int ieee80211_set_smps(struct ieee80211_sub_if_data *sdata,
if (sdata->vif.type != NL80211_IFTYPE_STATION)
return -EOPNOTSUPP;
- mutex_lock(&local->iflist_mtx);
+ mutex_lock(&sdata->u.mgd.mtx);
err = __ieee80211_request_smps(sdata, smps_mode);
- mutex_unlock(&local->iflist_mtx);
+ mutex_unlock(&sdata->u.mgd.mtx);
return err;
}
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply related
* Congratulation!!!
From: Mr Scott Carson @ 2011-04-21 12:07 UTC (permalink / raw)
£500,000GBP has been awarded to you in the BBC Online Promo Held on the 12 April
2011, send Name/Tel/Country to our office now.
----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.
^ permalink raw reply
* Re: pull request: wireless-2.6 2011-04-22
From: David Miller @ 2011-04-22 20:21 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20110422201142.GA27119@tuxdriver.com>
From: "John W. Linville" <linville@tuxdriver.com>
Date: Fri, 22 Apr 2011 16:11:43 -0400
> Here are some fixes intended for 2.6.39...
>
> This includes a Bluetooth pull, described thusly by Gustavo:
>
> Four small fixes for 2.6.39. An incorrect refcnt balance for
> hci connection by Ville Tervo, a warning fix in the command
> timer by Vinicius Gomes, an ERTM fix by Ruiyi Zhang, and SCO
> synchronization fix by Luiz Augusto von Dentz.
>
> On top of those, there are some use-after-free fixes from Stansilaw for
> iwlegacy and iwlwifi, a mac80211 debugfs locking fix from Johannes, and
> a small logic fix for the return code of ath9k's ah_stoprecv from Felix,
> and an iwlwifi fix for frame injection from Daniel Halperin.
>
> Please let me know if there are problems!
Pulled, thanks John.
^ permalink raw reply
* Congratulation!!!
From: Mr Scott Carson @ 2011-04-21 12:07 UTC (permalink / raw)
£500,000GBP has been awarded to you in the BBC Online Promo Held on the 12 April
2011, send Name/Tel/Country to our office now.
----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.
^ permalink raw reply
* Re: [PATCH] netconsole: fix deadlock when removing net driver that netconsole is using (v2)
From: Neil Horman @ 2011-04-22 21:31 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20110422.114033.104072820.davem@davemloft.net>
On Fri, Apr 22, 2011 at 11:40:33AM -0700, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Fri, 22 Apr 2011 14:10:59 -0400
>
> > @@ -683,9 +684,16 @@ static int netconsole_netdev_event(struct notifier_block *this,
> > * rtnl_lock already held
> > */
> > if (nt->np.dev) {
> > + spin_unlock_irqrestore(
> > + &target_list_lock,
> > + flags);
> > __netpoll_cleanup(&nt->np);
> > + spin_lock_irqsave(&target_list_lock,
> > + flags);
> > dev_put(nt->np.dev);
> > nt->np.dev = NULL;
> > + netconsole_target_put(nt);
> > + goto restart;
>
> If you drop the lock here, another cpu can put the device and set it
> to NULL.
>
> In which case you'll double release when you regrab the lock here.
>
> Too bad you can't NULL out the device before dropping the lock,
> because that way would be able to ensure you'd be the only releaser.
>
> But that won't work because __netpoll_cleanup() wants the device
> pointer to be set.
>
> Hmmm...
>
I understand what you're saying here, but I think we're ok in this particular
case. I say that because all other callers of __netpoll_cleanup, call it via
netpoll_cleanup, which does the dev_put under protection of the rtnl_lock. This
call is also under the rtnl_lock protection, its just taken when the event
notification is made (thats why we call __netpoll_cleanup instead of
netpoll_cleanup). The target_list_lock just protects the integrity of the
netconsole_target list. If someone disables a netconsole via configfs, they'll
block on the rtnl_lock. Since no path through configfs takes the
target_list_lock and rtnl (via netpoll_cleanup) in any nested fashion, we're
safe from deadlock.
Best
Neil
^ permalink raw reply
* Re: [PATCH] netconsole: fix deadlock when removing net driver that netconsole is using (v2)
From: David Miller @ 2011-04-22 21:34 UTC (permalink / raw)
To: nhorman; +Cc: netdev
In-Reply-To: <20110422213145.GA2332@neilslaptop.think-freely.org>
From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 22 Apr 2011 17:31:45 -0400
> I understand what you're saying here, but I think we're ok in this particular
> case. I say that because all other callers of __netpoll_cleanup, call it via
> netpoll_cleanup, which does the dev_put under protection of the rtnl_lock. This
> call is also under the rtnl_lock protection, its just taken when the event
> notification is made (thats why we call __netpoll_cleanup instead of
> netpoll_cleanup). The target_list_lock just protects the integrity of the
> netconsole_target list. If someone disables a netconsole via configfs, they'll
> block on the rtnl_lock. Since no path through configfs takes the
> target_list_lock and rtnl (via netpoll_cleanup) in any nested fashion, we're
> safe from deadlock.
That's right, rtnl saves us.
Thanks for explaining, applied, thanks!
^ permalink raw reply
* RE: rtnetlink and many VFs
From: Rose, Gregory V @ 2011-04-22 22:29 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev, sf-linux-drivers
In-Reply-To: <1303409517.3165.39.camel@bwh-desktop>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Ben Hutchings
> Sent: Thursday, April 21, 2011 11:12 AM
> To: Rose, Gregory V
> Cc: David Miller; netdev; sf-linux-drivers
> Subject: RE: rtnetlink and many VFs
>
> > > I think that what 'ip link show' is doing now seems to be perfectly
> > > valid. It allocates a 16K buffer which would be enough if netlink
> > > didn't apply this PAGE_SIZE limit to single messages.
> >
If someone who knew (or knows) a bit more about netlink could point out where this limitation is set in the code I'd really appreciate it. I've been poking around for a few hours and still can't find it. I found the spot in the iproute2 code where the 16k is allocated and traced it down to the netlink socket call but from the kernel side I just can't find this PAGE_SIZE limit invocation.
- Greg
^ permalink raw reply
* Re: rtnetlink and many VFs
From: David Miller @ 2011-04-22 22:31 UTC (permalink / raw)
To: gregory.v.rose; +Cc: bhutchings, netdev, linux-net-drivers
In-Reply-To: <43F901BD926A4E43B106BF17856F0755014603C6BE@orsmsx508.amr.corp.intel.com>
From: "Rose, Gregory V" <gregory.v.rose@intel.com>
Date: Fri, 22 Apr 2011 15:29:41 -0700
> If someone who knew (or knows) a bit more about netlink could point
> out where this limitation is set in the code I'd really appreciate
> it. I've been poking around for a few hours and still can't find
> it.
NLMSG_GOODSIZE
^ 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