* Re: [PATCH] RDSRDMA: Fix cleanup of rds_iw_mr_pool
From: Jonathan Lallinger @ 2011-09-27 17:26 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20110927.013247.336618301904552114.davem@davemloft.net>
Hello David,
I am ashamed I made the same mistake twice. This happened because I had
two git trees (I made a second one when kernel.org went down based off
the github remote). I fixed, built, and ran several tests on the patch,
and then sent the wrong patch from an old git tree (which was never
build tested).
I can assure you I have a working patch, and it has been tested by the
QA group at Chelsio and it builds/runs but there are still additional
bugs in rds. So once I resolve those I will resend the correct patch
with some additional fixes.
I am sorry about this and it won't happen again.
Thanks,
Jonathan
David Miller wrote:
> From: Jonathan Lallinger <jonathan@ogc.us>
> Date: Tue, 13 Sep 2011 14:41:01 -0500
>
>
>> @@ -548,6 +550,7 @@ static int rds_iw_flush_mr_pool(struct rds_iw_mr_pool *pool, int free_all)
>> spin_unlock_irqrestore(&pool->list_lock, flags);
>> }
>>
>> + atomic_sub(unpinned, &poll->free_pinned);
>> atomic_sub(ncleaned, &pool->dirty_count);
>> atomic_sub(nfreed, &pool->item_count);
>>
>>
>
> net/rds/iw_rdma.c: In function ‘rds_iw_flush_mr_pool’:
> net/rds/iw_rdma.c:553:24: error: ‘poll’ undeclared (first use in this function)
> net/rds/iw_rdma.c:553:24: note: each undeclared identifier is reported only once for each function it appears in
>
> If you didn't even build test it, I know you didn't test it's
> functionality either.
>
> This is crazy.
>
> Well if it's not important enough to even build test this change
> before you post it, then it obviously doesn't matter if the RDMA
> module crashes the kernel when it's unloaded.
>
^ permalink raw reply
* Re: [PATCH net-next 1/2] be2net: fix multicast filter programming Multicast programming has been broken since commit 5b8821b7. Setting the MULTICAST flag while sending the cmd to the FW was missing. Fixed this.
From: David Miller @ 2011-09-27 17:28 UTC (permalink / raw)
To: sathya.perla; +Cc: netdev
In-Reply-To: <1317122035-5533-1-git-send-email-sathya.perla@emulex.com>
From: Sathya Perla <sathya.perla@emulex.com>
Date: Tue, 27 Sep 2011 16:43:54 +0530
> Also fixed-up some indentation in the adjacent lines.
>
> Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
Please don't create multi-line subjects, condense the subject into something that fits on
one 80-character line then put the details in the body of the commit message..
^ permalink raw reply
* Re: [PATCH net-next 1/2] be2net: fix multicast filter programming
From: David Miller @ 2011-09-27 17:29 UTC (permalink / raw)
To: sathya.perla; +Cc: netdev
In-Reply-To: <1317122713-5640-1-git-send-email-sathya.perla@emulex.com>
From: Sathya Perla <sathya.perla@emulex.com>
Date: Tue, 27 Sep 2011 16:55:13 +0530
> Re-posting with subject fixed!
>
> Multicast programming has been broken since commit 5b8821b7. Setting the
> MULTICAST flag while sending the cmd to the FW was missing. Fixed this.
>
> Also fixed-up some indentation in the adjacent lines.
>
> Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 2/2] be2net: Show newly flashed FW ver in ethtool
From: David Miller @ 2011-09-27 17:30 UTC (permalink / raw)
To: sathya.perla; +Cc: netdev, Suresh.Reddy
In-Reply-To: <747547b9-60c7-41b7-9584-23d0374b4503@exht2.ad.emulex.com>
From: Sathya Perla <sathya.perla@emulex.com>
Date: Tue, 27 Sep 2011 16:43:55 +0530
> This fix provides a newly flashed FW version (appended, in braces) along with
> the currently running FW version via ethtool. The newly flashed version runs only after a system reset.
>
> Signed-off-by: Suresh Reddy <Suresh.Reddy@emulex.com>
> Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next v2] candev: allow SJW user setting for bittiming calculation
From: Oliver Hartkopp @ 2011-09-27 17:32 UTC (permalink / raw)
To: Wolfgang Grandegger; +Cc: SocketCAN Core Mailing List, Linux Netdev List
In-Reply-To: <4E7D8537.8010302-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
On 09/24/11 09:22, Wolfgang Grandegger wrote:
> On 09/23/2011 11:32 AM, Pavel Pisa wrote:
>> On base of above analysis, I think that blindly set SJW
>> on maximum is not good idea. It should be at least limited
>> to 5% of bit time.
(..)
>> SJW is more problematic, but may it be use of 2 or 5%
>> of bittime by default with assurance that zero is
>> replaced by one, would serve to most people pleasure.
(..)
>> But there is still unknown parameter
>> capacity/length of connected wires so there is still
>> something left to user consideration.
>
> Thanks for your detailed explanation. It clearly shows that adjusting
> SJW is non-trivial and nothing a normal CAN user should deal with. When
> adjusting SJW, do you also need to tweek other bit-timing parameters,
> e.g. tq? I mean, would "ip link set can0 type can bitrate x
> sampling-point y sjw z" work for your setup or do you need to use the
> expert mode setting via "ip link set can0 type can tq ..." anyway?
Hello Wolfgang,
i double checked a specification where i got my requirement to influence the
SJW value from. It says (non literally):
"Put the SJW to the highest possible value only reduced by tseg2."
The fact that this requirement might no fit to any CAN setup or may not be in
best academical shape is not my problem. My requirement is to provide an easy
way to move the SJW away from it's default value, which is currently
hard-coded to 1 in the can-dev framework when using it's bittiming calculation
function.
As almost everything is already done (only the provided SJW is not evaluated
in dev.c) this patch is IMO an valid option to support it. If Joe user can
tune the sampling point, why should he not be able to influence the SJW if he
thinks, he knows what he's doing?
Especially as the good working bittiming calculation is not touched in any way
and the default SJW remains 1 (even if '0' is provided as user input).
Regards,
Oliver
^ permalink raw reply
* Re: [PATCH net-next] skge: handle irq better on single port card
From: David Miller @ 2011-09-27 17:41 UTC (permalink / raw)
To: shemminger; +Cc: netdev
In-Reply-To: <20110927102523.5f8b7f2a@nehalam.linuxnetplumber.net>
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue, 27 Sep 2011 10:25:23 -0700
> Most boards with SysKonnect/Marvell Ethernet have only a single port.
> For the single port case, use the standard Ethernet driver convention
> of allocating IRQ when device is brought up rather than at probe time.
>
> This patch also adds some additional read after writes to avoid any
> PCI posting problems when setting the IRQ mask.
>
> The error handling of dual port cards is also changed. If second port
> can not be brought up, then just fail. No point in continuing, since
> the failure is most certainly because of out of memory.
>
> It is worth noting that the dual port skge device has a single irq but two
> seperate status rings and therefore has two NAPI objects, one for
> each port.
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
Applied.
^ permalink raw reply
* Re: [PATCH] w5300: add WIZnet W5300 Ethernet driver
From: Francois Romieu @ 2011-09-27 17:45 UTC (permalink / raw)
To: Taehun Kim; +Cc: netdev, linux-kernel
In-Reply-To: <CAOkfWjcHxh60D9KvxLfgL0tw9XdPyDN9xr7q+9V+nM6B6g8tDA@mail.gmail.com>
Taehun Kim <kth3321@gmail.com> :
[...]
> Please check this patch. Not only it had been finished testing, but
> also confirmed by WIZnet(http://wiznet.co.kr).
(please don't include 1080 lines of continuous quote)
> If any problems, please let me know.
> WIZnet W5300 is a network chip into which 10/100 Ethernet controller, MAC,
> and TCP/IP are integrated.
> This driver supports just Ethernet function in W5300.
>
> Signed-off-by: Taehun Kim <kth3321@gmail.com>
> ---
> drivers/net/Kconfig | 5 +
> drivers/net/Makefile | 1 +
> drivers/net/w5300.c | 669 ++++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/net/w5300.h | 350 ++++++++++++++++++++++++++
The layout has changed. See David Miller's -next tree at :
git://github.com/davem330/net-next.git
It should probably go into drivers/net/ethernet/wiznet.
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -2016,6 +2016,11 @@ config LANTIQ_ETOP
> help
> Support for the MII0 inside the Lantiq SoC
>
> +config W5300
> + tristate "WIZnet W5300 ethernet driver"
> + depends on ARM
> + help
(nit)
---help---
[...]
> diff --git a/drivers/net/w5300.c b/drivers/net/w5300.c
> new file mode 100644
> index 0000000..fcc2579
> --- /dev/null
> +++ b/drivers/net/w5300.c
[...]
> +#define DRV_VERSION "1.0"
> +#define DRV_RELDATE "Sept 17, 2011"
> +
> +static const char driver_info[] =
> + KERN_INFO DEV_NAME ": Ethernet driver v" DRV_VERSION "("
> + DRV_RELDATE ")\n";
> +
> +MODULE_AUTHOR("Taehun Kim <kth3321@gmail.com>");
> +MODULE_DESCRIPTION("WIZnet W5300 Ethernet driver");
> +MODULE_VERSION(DRV_VERSION);
> +MODULE_LICENSE("GPL");
> +
> +/* Transmit timeout, default 5 seconds. */
> +static int watchdog = 5000;
> +module_param(watchdog, int, 0400);
> +MODULE_PARM_DESC(watchdog, "transmit timeout in milliseconds");
> +
> +/*
> + * This is W5300 information structure.
> + * Additional information is included in struct net_device.
> + */
> +struct wiz_private {
> + void __iomem *base;
> + struct net_device *dev;
> + u8 rxbuf_conf[MAX_SOCK_NUM];
> + u8 txbuf_conf[MAX_SOCK_NUM];
> + struct net_device_stats stats;
> + struct napi_struct napi;
> + spinlock_t lock;
> +};
> +
> +/* Default MAC address. */
> +static const u8 w5300_defmac[6] = {0x00, 0x08, 0xDC, 0xA0, 0x00, 0x01};
__initdata ?
> +
> +/* Default RX/TX buffer size(KByte). */
> +static const u8 w5300_rxbuf_conf[MAX_SOCK_NUM] = { 64, 0, 0, 0, 0, 0, 0, 0 };
> +static const u8 w5300_txbuf_conf[MAX_SOCK_NUM] = { 64, 0, 0, 0, 0, 0, 0, 0 };
__initdata ?
The size is known: you can save some 0.
> +/* Notifying packet size in the RX FIFO */
> +static int
> +w5300_get_rxsize(struct wiz_private *wp, int s)
> +{
> + u32 val;
> +
> + val = w5300_read(wp, Sn_RX_RSR(s));
> + val = (val << 16) + w5300_read(wp, Sn_RX_RSR2(s));
> + return val;
> +}
> +
> +/* Packet Receive Function. It reads received packet from the Rx FIFO. */
> +static int
> +w5300_recv_data(struct wiz_private *wp, int s,
> + u8 *buf, ssize_t len, int swap_enable)
The status code that this method returns is never checked.
> +{
> + int i;
> + u16 recv_data;
> +
> + if (unlikely(len <= 0))
> + return 0;
> +
> + if (swap_enable) {
> + /* read from RX FIFO */
> + for (i = 0; i < len; i += 2) {
> + recv_data = w5300_read(wp, Sn_RX_FIFO(s));
> + buf[i] = (u8) ((recv_data & 0xFF00) >> 8);
> + buf[i + 1] = (u8) (recv_data & 0x00FF);
> + }
> + } else {
> + for (i = 0; i < len; i += 2) {
> + recv_data = w5300_read(wp, Sn_RX_FIFO(s));
> + buf[i] = (u8) (recv_data & 0x00FF);
> + buf[i + 1] = (u8) ((recv_data & 0xFF00) >> 8);
> + }
> + }
You should use le{16/32}_to_cpu, swab16/32 and friends in place of this
'swap_enable' thing.
> + return len;
> +}
> +
> +/* Setting MAC address of W5300 */
> +static void
> +w5300_set_macaddr(struct wiz_private *wp, u8 * addr)
> +{
> + w5300_write(wp, SHAR, (u16) (addr[0] << 8) | (u16) addr[1]);
> + w5300_write(wp, SHAR2, (u16) (addr[2] << 8) | (u16) addr[3]);
> + w5300_write(wp, SHAR4, (u16) (addr[4] << 8) | (u16) addr[5]);
> +}
> +
> +/* Opening channels of W5300 */
> +static int
> +w5300_open(struct wiz_private *wp, u32 type)
Name it w5300_channel_open or more adequately w5300_channel_0_open
as it is hardwired to channel 0 ?
The status code that this method returns is never checked.
> +{
> + /* Which type will be used for open? */
> + switch (type) {
> + case Sn_MR_MACRAW:
> + case Sn_MR_MACRAW_MF:
> + w5300_write(wp, Sn_MR(0), type);
> + break;
> + default:
> + printk(KERN_ERR "%s: Unknown socket type (%d)\n",
> + DEV_NAME, type);
> + return -EFAULT;
> + }
> + w5300_write(wp, Sn_PORTR(0), 5300);
> +
> + w5300_write(wp, Sn_CR(0), Sn_CR_OPEN);
> + while (w5300_read(wp, Sn_CR(0)))
> + udelay(1);
> +
> + return 0;
> +}
> +
> +/* Activating the interrupt of related channel */
> +static void
> +w5300_interrupt_enable(struct wiz_private *wp, int s)
> +{
> + u16 mask;
> + mask = w5300_read(wp, IMR);
Please insert an empty line after the declaration.
> + mask |= (0x01 << s);
> + w5300_write(wp, IMR, mask);
> +}
It could imho fit in a single line.
> +
> +/* De-activating the interrupt of related channel */
> +static void
> +w5300_interrupt_disable(struct wiz_private *wp, int s)
> +{
> + u16 mask;
> + mask = w5300_read(wp, IMR);
> + mask &= ~(0x01 << s);
> + w5300_write(wp, IMR, mask);
> +}
> +
> +/* W5300 initialization function */
> +static int
> +w5300_reset(struct net_device *dev)
The status code that this method returns is never checked.
> +{
> + struct wiz_private *wp = netdev_priv(dev);
> + u32 txbuf_total = 0, i;
> + u16 mem_cfg = 0;
> +
> + DPRINTK("%s: w5300 chip reset\n", __func__);
> +
> + /* W5300 is initialized by sending RESET command. */
> + w5300_write(wp, MR, MR_RST);
> + mdelay(5);
> +
> + /* Mode Register Setting
> + * Ping uses S/W stack of the Linux kernel. Set the Ping Block.*/
> + w5300_write(wp, MR, MR_WDF(1) | MR_PB);
> +
> + /* Setting MAC address */
> + w5300_set_macaddr(wp, dev->dev_addr);
> +
> + /* Setting the size of Rx/Tx FIFO */
> + for (i = 0; i < MAX_SOCK_NUM; ++i) {
> + if (wp->rxbuf_conf[i] > 64) {
> + printk(KERN_ERR "%s: Illegal Channel(%d) RX memory size.\n",
> + DEV_NAME, i);
You can use nice netif_{err/info/...} helpers.
> + return -EINVAL;
> + }
> + if (wp->txbuf_conf[i] > 64) {
> + printk(KERN_ERR "%s: Illegal Channel(%d) TX memory size.\n",
> + DEV_NAME, i);
> + return -EINVAL;
> + }
> + txbuf_total += wp->txbuf_conf[i];
> + }
> +
> + if (txbuf_total % 8) {
> + printk(KERN_ERR "%s: Illegal memory size register setting.\n",
> + DEV_NAME);
> + return -EINVAL;
> + }
> + w5300_write(wp, RMSR0,
> + (u16) (wp->rxbuf_conf[0] << 8) | (u16) wp->rxbuf_conf[1]);
> + w5300_write(wp, RMSR2,
> + (u16) (wp->rxbuf_conf[2] << 8) | (u16) wp->rxbuf_conf[3]);
> + w5300_write(wp, RMSR4,
> + (u16) (wp->rxbuf_conf[4] << 8) | (u16) wp->rxbuf_conf[5]);
> + w5300_write(wp, RMSR6,
> + (u16) (wp->rxbuf_conf[6] << 8) | (u16) wp->rxbuf_conf[7]);
> + w5300_write(wp, TMSR0,
> + (u16) (wp->txbuf_conf[0] << 8) | (u16) wp->txbuf_conf[1]);
> + w5300_write(wp, TMSR2,
> + (u16) (wp->txbuf_conf[2] << 8) | (u16) wp->txbuf_conf[3]);
> + w5300_write(wp, TMSR4,
> + (u16) (wp->txbuf_conf[4] << 8) | (u16) wp->txbuf_conf[5]);
> + w5300_write(wp, TMSR6,
> + (u16) (wp->txbuf_conf[6] << 8) | (u16) wp->txbuf_conf[7]);
w5300_write expects an u16: the u8 will be correctly expanded before the shift
even if you remove the casts.
Btw this is nothing more than
for (i = 0; i < 4; i++) {
u16 size = (wp->rxbuf_conf[2*i] << 8) | wp->rxbuf_conf[2*i + 1];
w5300_write(wp, RMSR0 + 2*i, size);
}
It may even save a few bytes if you factor it out with a method accepting
{ wp->rxbuf_conf, RMSR0 } (resp. { wp->txbuf_conf, TMSR0 }) as parameters.
> +
> + /* Setting FIFO Memory Type (TX&RX) */
> + for (i = 0; i < txbuf_total / 8; ++i) {
> + mem_cfg <<= 1;
> + mem_cfg |= 1;
> + }
> + w5300_write(wp, MTYPER, mem_cfg);
> +
> + /* Masking all interrupts */
> + w5300_write(wp, IMR, 0x0000);
> +
> + return 0;
> +}
> +
> +/* Interrupt Handler(ISR) */
> +static irqreturn_t
> +wiz_interrupt(int irq, void *dev_instance)
> +{
> + struct net_device *dev = dev_instance;
> + struct wiz_private *wp = netdev_priv(dev);
> + unsigned long isr, ssr;
s/unsigned long/u16/
> + int s;
> +
> + isr = w5300_read(wp, IR);
> +
> + /* Completing all interrupts at a time. */
> + while (isr) {
> + w5300_write(wp, IR, isr);
> +
> + /* Finding the channel to create the interrupt */
> + s = find_first_bit(&isr, sizeof(u16));
> + ssr = w5300_read(wp, Sn_IR(s));
> + /* socket interrupt is cleared. */
> + w5300_write(wp, Sn_IR(s), ssr);
> + DPRINTK("%s: ISR = %X, SSR = %X, s = %X\n",
> + __func__, isr, ssr, s);
> + if (likely(!s)) {
> + if (ssr & Sn_IR_RECV) {
> + /* De-activation of interrupt */
> + w5300_interrupt_disable(wp, 0);
> + /* Receiving by polling method */
> + napi_schedule(&wp->napi);
> + }
> + }
> +
> + /* Is there any interrupt to be processed? */
> + isr = w5300_read(wp, IR);
> + }
> +
> + return IRQ_HANDLED;
The hardware implementation is supposed to enforce that the 'while' is
always entered, right ?
> +}
> +
> +static int
> +wiz_open(struct net_device *dev)
> +{
> + struct wiz_private *wp = netdev_priv(dev);
> + int ret;
> +
> + napi_enable(&wp->napi);
> +
> + ret = request_irq(dev->irq, wiz_interrupt,
> + IRQF_IRQPOLL, dev->name, dev);
Why do you use IRQF_IRQPOLL ?
> + if (ret < 0) {
> + printk(KERN_ERR "%s: request_irq() error!\n", DEV_NAME);
> + return ret;
> + }
> +
> + /* Activating the interrupt of channel 0 that is used for MACRAW. */
> + w5300_interrupt_enable(wp, 0);
> +
> + /* Sending OPEN command to use channel 0 as MACRAW mode. */
> + w5300_open(wp, Sn_MR_MACRAW_MF);
> +
> + netif_start_queue(dev);
> +
> + return 0;
> +}
> +
> +static int
> +wiz_close(struct net_device *dev)
> +{
> + struct wiz_private *wp = netdev_priv(dev);
> +
> + DPRINTK("%s\n", __func__);
> +
> + napi_disable(&wp->napi);
> +
> + /* Interrupt masking of all channels */
> + w5300_write(wp, IMR, 0x0000);
> + netif_stop_queue(dev);
The upperlayer takes care of netif_stop_queue. It is not needed.
> + w5300_write(wp, Sn_CR(0), Sn_CR_CLOSE);
> + free_irq(dev->irq, dev);
> +
> + return 0;
> +}
> +
> +static int
> +w5300_send_data(struct wiz_private *wp, u8 * buf, ssize_t len)
> +{
> + int i;
> + u16 send_data;
> +
> + /* Writing packets in to Tx FIFO */
> + for (i = 0; i < len; i += 2) {
> + send_data = (buf[i] << 8) | buf[i+1];
> + w5300_write(wp, Sn_TX_FIFO(0), send_data);
> + }
> +
> + w5300_write(wp, Sn_TX_WRSR(0), (u16)(len >> 16));
> + w5300_write(wp, Sn_TX_WRSR2(0), (u16)len);
> + w5300_write(wp, Sn_CR(0), Sn_CR_SEND);
> + while (w5300_read(wp, Sn_CR(0)))
> + udelay(1);
> +
> + return len;
> +}
> +
> +/* Function to transmit data at the MACRAW mode */
> +static int
> +wiz_start_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> + struct wiz_private *wp = netdev_priv(dev);
> + int ret;
> +
> + DPRINTK("%s: MAC_RAW SEND packet size = %d\n", __func__, skb->len);
> +
> + ret = w5300_send_data(wp, skb->data, skb->len);
> +
> + /* Statistical Process */
> + if (ret < 0) {
Something is pretty wrong if ret (= skb->len) is < 0.
> + wp->stats.tx_dropped++;
> + } else {
> + wp->stats.tx_bytes += skb->len;
> + wp->stats.tx_packets++;
> + dev->trans_start = jiffies;
> + }
> + dev_kfree_skb(skb);
> +
> + return 0;
s/0/NETDEV_TX_OK/
> +}
> +
> +static struct net_device_stats *
> +wiz_get_stats(struct net_device *dev)
> +{
> + struct wiz_private *wp = netdev_priv(dev);
> + DPRINTK("%s: get status\n", __func__);
> + return &wp->stats;
> +}
> +
> +/* It is called when multi-cast list or flag is changed. */
> +static void
> +wiz_set_multicast(struct net_device *dev)
> +{
> + struct wiz_private *wp = netdev_priv(dev);
> +
> + DPRINTK("%s: multicast\n", __func__);
> + if (dev->flags & IFF_PROMISC)
> + w5300_open(wp, Sn_MR_MACRAW);
> + else
> + w5300_open(wp, Sn_MR_MACRAW_MF);
Use a ternary operator ?
> +}
> +
> +static int
> +wiz_set_mac_address(struct net_device *dev, void *addr)
> +{
> + struct wiz_private *wp = netdev_priv(dev);
> + struct sockaddr *sock_addr = addr;
> +
> + DPRINTK("%s: set mac address\n", __func__);
> +
> + spin_lock(&wp->lock);
> + /* Changing MAC address of W5300 */
> + w5300_set_macaddr(wp, sock_addr->sa_data);
> + /* Changing MAC address of material structure to manage W5300 */
> + memcpy(dev->dev_addr, sock_addr->sa_data, dev->addr_len);
> + spin_unlock(&wp->lock);
> +
> + return 0;
> +}
> +
> +static void
> +wiz_tx_timeout(struct net_device *dev)
> +{
> + struct wiz_private *wp = netdev_priv(dev);
> + unsigned long flags;
> +
> + printk(KERN_WARNING "%s: Transmit timeout\n", dev->name);
> +
> + spin_lock_irqsave(&wp->lock, flags);
> + /* Initializing W5300 chip. */
> + w5300_reset(dev);
> + /* Waking up network interface */
> + netif_wake_queue(dev);
> + spin_unlock_irqrestore(&wp->lock, flags);
> +}
> +
> +/*
> + * Polling Function to process only receiving at the MACRAW mode.
> + * De-activating the interrupt when recv interrupt occurs,
> + * and processing the RECEIVE with this Function
> + * Activating the interrupt after completing RECEIVE process
> + * As recv interrupt often occurs at short intervals,
> + * there will system load in case that interrupt handler process the RECEIVE.
> + */
> +static int
> +wiz_rx_poll(struct napi_struct *napi, int budget)
> +{
> + struct sk_buff *skb;
It should be declared in the 'while' loop below.
> + struct wiz_private *wp = container_of(napi, struct wiz_private, napi);
> + struct net_device *dev = wp->dev;
> + u16 rxbuf_len, pktlen;
These should be declared in the 'while' loop below.
rxbuf_len can go away.
> + u32 crc;
It should be declared in the 'while' loop below.
> + int npackets = 0;
> +
> + /* Processing the RECEIVE during Rx FIFO is containing any packet */
> + while (w5300_get_rxsize(wp, 0) > 0) {
> +
> + /* The first 2byte is the information about packet lenth. */
> + w5300_recv_data(wp, 0, (u8 *)&pktlen, 2, 0);
> + DPRINTK("%s: pktlen = %d\n", __func__, pktlen);
> +
> + /*
> + * Allotting the socket buffer in which packet will be contained
> + * Ethernet packet is of 14byte.
> + * In order to make it multiplied by 2, the buffer allocation
> + * should be 2bytes bigger than the packet.
> + */
> + skb = dev_alloc_skb(pktlen + 2);
netdev_alloc_skb_ip_align
> + if (!skb) {
> + u8 *temp;
> + printk(KERN_ERR "%s: Memory squeeze, dropping packet.\n",
> + dev->name);
> + temp = kmalloc(pktlen + 4, GFP_KERNEL);
- "Memory squeeze ? Let's allocate more..."
- unchecked kmalloc
- GFP_KERNEL in poll
Either you use an already allocated buffer (it could even be static) or
you loop with a small on-stack buffer.
> + wp->stats.rx_dropped++;
> + w5300_recv_data(wp, 0, temp, pktlen + 4, 1);
> + kfree(temp);
> + return -ENOMEM;
poll is supposed to return the work actually done, not -Exyz.
> + }
> +
> + /* Initializing the socket buffer */
> + skb->dev = dev;
> + skb_reserve(skb, 2);
> + skb_put(skb, pktlen);
> +
> + /* Reading packets from W5300 Rx FIFO into socket buffer. */
> + w5300_recv_data(wp, 0, (u8 *)skb->data, pktlen, 1);
> +
> + /* Reading and discarding 4byte CRC. */
> + w5300_recv_data(wp, 0, (u8 *)&crc, 4, 0);
> +
> + /* The packet type is Ethernet. */
> + skb->protocol = eth_type_trans(skb, dev);
> +
> + /* Passing packets to uppder stack (kernel). */
> + netif_receive_skb(skb);
> +
> + /* Processing statistical information */
> + wp->stats.rx_packets++;
> + wp->stats.rx_bytes += pktlen;
> + wp->dev->last_rx = jiffies;
> + rxbuf_len -= pktlen;
> + npackets++;
> +
> + if (npackets >= budget)
> + break;
> + }
> +
> + /* If packet number is smaller than budget when getting out of loopback,
> + * the RECEIVE process is completed. */
> + if (npackets < budget) {
> + unsigned long flags;
> + spin_lock_irqsave(&wp->lock, flags);
> + w5300_interrupt_enable(wp, 0);
> + __napi_complete(napi);
> + spin_unlock_irqrestore(&wp->lock, flags);
> + }
> + return npackets;
> +}
> +
> +static const struct net_device_ops wiz_netdev_ops = {
> + .ndo_open = wiz_open,
> + .ndo_stop = wiz_close,
> + .ndo_validate_addr = eth_validate_addr,
> + .ndo_set_mac_address = wiz_set_mac_address,
> + .ndo_set_multicast_list = wiz_set_multicast,
> + .ndo_get_stats = wiz_get_stats,
> + .ndo_start_xmit = wiz_start_xmit,
> + .ndo_tx_timeout = wiz_tx_timeout,
Please add more tabs and align everything.
> +};
> +
> +/* Initialize W5300 driver. */
> +static int __devinit
> +w5300_drv_probe(struct platform_device *pdev)
> +{
> + struct net_device *dev;
> + struct wiz_private *wp;
> + struct resource *res;
> + void __iomem *addr;
> + int ret;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + /* Request the chip register regions. */
> + if (!request_mem_region(res->start, SZ_1M, DEV_NAME)) {
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + /* Allocatting struct net_device structure which is managing W5300 */
> + dev = alloc_etherdev(sizeof(struct wiz_private));
> + if (!dev) {
> + ret = -ENOMEM;
> + goto release_region;
> + }
> +
> + dev->dma = (unsigned char)-1;
> + dev->irq = platform_get_irq(pdev, 0);
> + wp = netdev_priv(dev);
> + wp->dev = dev;
> + addr = ioremap(res->start, SZ_1M);
> + if (!addr) {
> + ret = -ENOMEM;
> + goto release_both;
> + }
> +
> + platform_set_drvdata(pdev, dev);
> + wp->base = addr;
> +
> + spin_lock_init(&wp->lock);
> +
> + /* Initialization of Rx/Tx FIFO size */
> + memcpy(wp->rxbuf_conf, w5300_rxbuf_conf, MAX_SOCK_NUM * sizeof(u8));
> + memcpy(wp->txbuf_conf, w5300_txbuf_conf, MAX_SOCK_NUM * sizeof(u8));
You can spare the sizeof(u8).
> +
> + dev->base_addr = res->start;
Please leave base_addr rest in peace.
> + dev->addr_len = 6; /* MAC address length. */
Useless, it's already done through alloc_etherdev->ether_setup.
> + memcpy(dev->dev_addr, w5300_defmac, dev->addr_len);
> +
> + ether_setup(dev);
Useless, see alloc_etherdev.
> + dev->netdev_ops = &wiz_netdev_ops;
> + /* Setting napi. Enabling to process max 16 packets at a time. */
> + netif_napi_add(dev, &wp->napi, wiz_rx_poll, 16);
> +
> + dev->watchdog_timeo = msecs_to_jiffies(watchdog);
> +
> + strcpy(dev->name, "eth%d");
Useless.
> +
> + w5300_reset(dev);
> +
> + ret = register_netdev(dev);
> + if (ret != 0) {
> + platform_set_drvdata(pdev, NULL);
> + iounmap(addr);
> +release_both:
> + free_netdev(dev);
> +release_region:
> + release_resource(res);
> + }
> +out:
> + return ret;
> +}
> +
> +static int __devexit
> +w5300_drv_remove(struct platform_device *pdev)
> +{
> + struct net_device *dev = platform_get_drvdata(pdev);
> + struct wiz_private *wp = netdev_priv(dev);
> + struct resource *res;
> +
> + platform_set_drvdata(pdev, NULL);
> + unregister_netdev(dev);
> +
> + if (wp->base != NULL)
> + iounmap(wp->base);
wp->base can not be NULL here.
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res != NULL)
> + release_resource(res);
release_resource is supposed to balance request_resource, not
request_mem_resource.
> +
> + free_netdev(dev);
> +
> + return 0;
> +}
> +
> +static int
> +w5300_drv_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> + struct net_device *dev = platform_get_drvdata(pdev);
> +
> + if (dev) {
Useless.
> + struct wiz_private *wp = netdev_priv(dev);
> +
> + if (netif_running(dev)) {
> + netif_device_detach(dev);
> + w5300_write(wp, IMR, 0x0000);
> + w5300_write(wp, Sn_CR(0), Sn_CR_CLOSE);
> + }
> + }
> + return 0;
> +}
> +
> +static int
> +w5300_drv_resume(struct platform_device *pdev)
> +{
> + struct net_device *dev = platform_get_drvdata(pdev);
> +
> + if (dev) {
Useless.
> + struct wiz_private *wp = netdev_priv(dev);
> +
> + if (netif_running(dev)) {
> + w5300_reset(dev);
> + w5300_interrupt_enable(wp, 0);
> + w5300_open(wp, Sn_MR_MACRAW_MF);
> + netif_device_attach(dev);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver w5300_driver = {
> + .probe = w5300_drv_probe,
> + .remove = __devexit_p(w5300_drv_remove),
> + .suspend = w5300_drv_suspend,
> + .resume = w5300_drv_resume,
> + .driver = {
> + .name = DEV_NAME,
> + .owner = THIS_MODULE,
> + },
Please fix the alignment and don't mix tabs and space.
> +};
> +
> +static int __init
> +wiz_module_init(void)
I don't get why you do not use the whole line length.
> +{
> + return platform_driver_register(&w5300_driver);
> +}
> +
> +static void __exit
> +wiz_module_exit(void)
> +{
> + platform_driver_unregister(&w5300_driver);
> +}
> +
> +module_init(wiz_module_init);
> +module_exit(wiz_module_exit);
> diff --git a/drivers/net/w5300.h b/drivers/net/w5300.h
> new file mode 100644
> index 0000000..d8583be
> --- /dev/null
> +++ b/drivers/net/w5300.h
[...]
> +#define Sn_IR_DISCON 0x02 /**< Disconnect bit of Sn_IR */
> +#define Sn_IR_CON 0x01 /**< Connect bit of Sn_IR */
So far, so good...
> +
> +/* Sn_SSR values */
> +#define SOCK_CLOSED 0x00 /**< SOCKETn is released */
> +#define SOCK_ARP 0x01 /**< ARP-request is transmitted in order to acquire destination hardware address. */
> +#define SOCK_INIT 0x13 /**< SOCKETn is open as TCP mode. */
[...]
> +/* IP PROTOCOL */
> +#define IPPROTO_IP 0 /* Dummy for IP */
> +#define IPPROTO_ICMP 1 /* Control message protocol */
> +#define IPPROTO_IGMP 2 /* Internet group management protocol */
> +#define IPPROTO_GGP 3 /* Gateway^2 (deprecated) */
> +#define IPPROTO_TCP 6 /* TCP */
> +#define IPPROTO_PUP 12 /* PUP */
> +#define IPPROTO_UDP 17 /* UDP */
> +#define IPPROTO_IDP 22 /* XNS idp */
> +#define IPPROTO_ND 77 /* UNOFFICIAL net disk protocol */
> +#define IPPROTO_RAW 255 /* Raw IP packet */
... but you can trim most of those.
No netif_carrier_{on/off} trough the whole driver ?
--
Ueimor
^ permalink raw reply
* Re: [RFC] bridge: handle bridge group address per 802.1 standards
From: Ben Hutchings @ 2011-09-27 18:23 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20110926153630.3959b0ea@nehalam.linuxnetplumber.net>
On Mon, 2011-09-26 at 15:36 -0700, Stephen Hemminger wrote:
> The Linux bridge code would process all packets addressed to
> the multicast address 01:80:C2:00:00:0X as local and
> and never forward. This may have been correct in the ancient past, but
> reading the relevant standards, the correct behavior is to handle only
> the bridge group address as a special case and leave all other link
> local multicast packets alone.
I disagree.
According to my reading, we must filter at least the addresses ending in
4-D or F, while forwarding of the others should be configurable.
> Recently there has been some complaints about forwarding (or not) of
> 802.1X EAPOL frames by the bridge. Thanks to Tony Jeffree of the
> 802.1 Bridging Working Group for point me in the correct direction.
> The 802.1X-2010 standard Table 11-1 details how different
> addresses are assigned based on connectivity associations.
>
> Bridge group address: 01-80-C2-00-00-00
> PAE group address: 01-80-C2-00-00-03
> Link Layer Discovery 01-80-C2-00-00-0E
[...]
This table is informative, non normative. The text below refers to
802.1D table 7-9 (apparently should be 7-10) and 802.1Q table 8-1 as the
sources.
802.1D-2004 section 7.12.6, Reserved addresses, says:
Frames containing any of the group MAC Addresses specified in
Table 7-10 in their destination address field shall not be
relayed by the Bridge. They are configured in the Permanent
Database. Management shall not provide the capability to modify
or remove these entries from the Permanent or the Filtering
Databases.
In table 7-10 the reserved addresses are those with last digit in the
range 4-F.
802.1Q-2005 section 8.6.3, Frame filtering, says:
Each of the Reserved MAC Addresses specified in Table 8-1 shall
be permanently configured in the Filtering Database in
VLAN-aware Bridges. The Filtering Database Entries for Reserved
MAC Addresses shall specify filtering for all Bridge Ports and
all VLANs. Management shall not provide the capability to modify
or remove entries for Reserved MAC Addresses.
In table 8-1 the reserved addresses are those with last digit in the
range 4-D or F.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [RFC PATCH] net: Always fire at least one linkwatch event
From: David Miller @ 2011-09-27 18:59 UTC (permalink / raw)
To: nhorman; +Cc: netdev, jfeeney
In-Reply-To: <1316634689-15083-1-git-send-email-nhorman@tuxdriver.com>
From: Neil Horman <nhorman@tuxdriver.com>
Date: Wed, 21 Sep 2011 15:51:29 -0400
> It was recently noted that the tg3 driver had a problem in that after boot a
> kernel and if-upping the tg3 interface the sysfs operstate attribute continued
> to read 'unkown'. This was happening because tg3 assumes the default carrier
> state (which is to say the __LINK_STATE_NOCARRIER bit is clear) is correct.
> That said, when the device is if-upped, and the open path, calls
> netif_carrier_on, the test_and_set_bit call in that function returns false
> (since the bit was previously zero from its initial state). This means that
> netif_carrier_on call never generates a linkwatch event, and as a result
> dev->operstate never gets recomputed. This could be fixed by unconditionally
> calling netif_carrier_off in the probe routine, to simply force a state change
> on that bit, but that seems like a sub-par solution, given that many drivers may
> have this error. Instead it seems like it might be better to burn an extra bit
> in the state field to indicate that the CARRIER bit is still in the initial
> state and our first call to netif_carrier_[off|on] should always fire a
> linkwatch event.
I'm finding this analysis hard to follow.
tg3_open() does netif_carrier_off(), and this will set the
__LINK_STATE_NOCARRIER bit.
And since the registration state of the device is not
NETREG_UNINITIALIZED it will fire off a linkwatch event too.
So whenever the netif_carrier_on() happens later, the bit will be set
when the test_and_clear_bit() happens there. So the
test_and_clear_bit() will not return false.
The registration state is not NETREG_UNINITIALIZED, so (again) that
will not block the linkwatch event from firing.
^ permalink raw reply
* Re: [PATCH net 1/3] bnx2x: fix hw attention handling
From: David Miller @ 2011-09-27 19:04 UTC (permalink / raw)
To: dmitry; +Cc: netdev, eilong
In-Reply-To: <1316694813-25274-1-git-send-email-dmitry@broadcom.com>
From: "Dmitry Kravkov" <dmitry@broadcom.com>
Date: Thu, 22 Sep 2011 15:33:31 +0300
> Use register name to initialize attention mask
>
> Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
Applied.
^ permalink raw reply
* Re: [PATCH net 2/3] bnx2x: fix WOL by enablement PME in config space
From: David Miller @ 2011-09-27 19:04 UTC (permalink / raw)
To: dmitry; +Cc: netdev, eilong
In-Reply-To: <1316694813-25274-2-git-send-email-dmitry@broadcom.com>
From: "Dmitry Kravkov" <dmitry@broadcom.com>
Date: Thu, 22 Sep 2011 15:33:32 +0300
>
> Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
Applied.
^ permalink raw reply
* Re: [PATCH net 3/3] bnx2x: add missing break in bnx2x_dcbnl_get_cap
From: David Miller @ 2011-09-27 19:04 UTC (permalink / raw)
To: dmitry; +Cc: netdev, shmulikr, eilong
In-Reply-To: <1316694813-25274-3-git-send-email-dmitry@broadcom.com>
From: "Dmitry Kravkov" <dmitry@broadcom.com>
Date: Thu, 22 Sep 2011 15:33:33 +0300
> From: Shmulik Ravid <shmulikr@broadcom.com>
>
> Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
Applied.
^ permalink raw reply
* Re: pull request: batman-adv 2011-09-22 (regression fix)
From: David Miller @ 2011-09-27 19:06 UTC (permalink / raw)
To: lindner_marek-LWAfsSFWpa4
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
b.a.t.m.a.n-ZwoEplunGu2X36UT3dwllkB+6BGkLq7r
In-Reply-To: <1316717836-19374-1-git-send-email-lindner_marek-LWAfsSFWpa4@public.gmane.org>
From: Marek Lindner <lindner_marek-LWAfsSFWpa4@public.gmane.org>
Date: Thu, 22 Sep 2011 20:57:15 +0200
> The following changes since commit 322a8b034003c0d46d39af85bf24fee27b902f48:
>
> Linux 3.1-rc1 (2011-08-07 18:23:30 -0700)
>
> are available in the git repository at:
> git://git.open-mesh.org/linux-merge.git batman-adv/maint
Pulled, thanks.
^ permalink raw reply
* [PATCH] ipv6-multicast: Fix memory leak in input path.
From: greearb @ 2011-09-27 18:58 UTC (permalink / raw)
To: netdev; +Cc: Ben Greear
From: Ben Greear <greearb@candelatech.com>
Have to free the skb before returning if we fail
the fib lookup.
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 e9a8df9... 86e3cc1... M net/ipv6/ip6mr.c
net/ipv6/ip6mr.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index e9a8df9..86e3cc1 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -2053,8 +2053,10 @@ int ip6_mr_input(struct sk_buff *skb)
int err;
err = ip6mr_fib_lookup(net, &fl6, &mrt);
- if (err < 0)
+ if (err < 0) {
+ kfree_skb(skb);
return err;
+ }
read_lock(&mrt_lock);
cache = ip6mr_cache_find(mrt,
--
1.7.3.4
^ permalink raw reply related
* Re: [PATCH] ipv6-multicast: Fix memory leak in input path.
From: David Miller @ 2011-09-27 19:16 UTC (permalink / raw)
To: greearb; +Cc: netdev
In-Reply-To: <1317149897-14932-1-git-send-email-greearb@candelatech.com>
From: greearb@candelatech.com
Date: Tue, 27 Sep 2011 11:58:17 -0700
> From: Ben Greear <greearb@candelatech.com>
>
> Have to free the skb before returning if we fail
> the fib lookup.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
Applied, thanks.
^ permalink raw reply
* macvlan/macvtap patch in patchwork
From: David Miller @ 2011-09-27 19:14 UTC (permalink / raw)
To: kaber; +Cc: netdev, herbert, krkumar2, david.ward
Could you guys please review:
http://patchwork.ozlabs.org/patch/115273/
My gut instinct is that the current behavior is intentional, but since
the patch submitter didn't describe exactly what the undesirable
behavior is it's hard to tell what the patch is actually fixing.
Thanks.
^ permalink raw reply
* ICMP redirect issue
From: Flavio Leitner @ 2011-09-27 19:21 UTC (permalink / raw)
To: netdev
Hi,
While investigating an issue on Red Hat Enterprise Linux, I found that
upstream commit below removed the old_gw check.
commit f39925dbde7788cfb96419c0f092b086aa325c0f
Author: David S. Miller <davem@davemloft.net>
Date: Wed Feb 9 22:00:16 2011 -0800
ipv4: Cache learned redirect information in inetpeer.
The issue is about the gateway being a LVS, so the servers behind use
the IP alias address as the default gateway. However, when the gateway
sends an ICMP redirect, it comes from the primary IP address which is
ignored on older kernels because of the old_gw check:
- if (rth->rt_dst != daddr ||
- rth->rt_src != saddr ||
- rth->dst.error ||
- rth->rt_gateway != old_gw ||
- rth->dst.dev != dev)
- break;
Well, the consequence is that the issue doesn't happen in newer kernels
because it happily accepts the ICMP redirect.
The admin can still control using shared_media and secure_redirects if
the host should accept only the ICMP redirects for gateways listed in
default gateway list or not.
In terms of a security, if someone manages to send ICMP redirect, then
I think it possible to fake the saddr to appear as coming from the
correct gateway.
So, I'm not seeing a problem, but I was told to bring this up to netdev.
Thoughts?
thanks,
fbl
^ permalink raw reply
* Re: [PATCH] net/flow: remove sleeping and deferral mechanism from flow_cache_flush
From: David Miller @ 2011-09-27 19:28 UTC (permalink / raw)
To: madalin.bucur; +Cc: eric.dumazet, netdev, timo.teras
In-Reply-To: <1317056956-23644-1-git-send-email-madalin.bucur@freescale.com>
From: Madalin Bucur <madalin.bucur@freescale.com>
Date: Mon, 26 Sep 2011 20:09:16 +0300
> flow_cache_flush must not sleep as it can be called in atomic context;
> removed the schedule_work as the deferred processing lead to the flow
> cache gc never being actually run under heavy network load
>
> Signed-off-by: Madalin Bucur <madalin.bucur@freescale.com>
How is this called in an atomic context? The only caller of
flow_cache_flush() is __xfrm_garbage_collect() which is only invoked
during a NETDEV_DOWN event which ought to be non-atomic.
afinfo->garbage_collect is the only other place __xfrm_garbage_collect
is referenced, and that is completely unused and should thus be deleted
(I'll take care of that in net-next).
If NETDEV_DOWN notifier is in an atomic context, we need to accomodate
or fix that somehow.
^ permalink raw reply
* Re: [PATCH 1/2] net: check return value for dst_alloc
From: David Miller @ 2011-09-27 19:32 UTC (permalink / raw)
To: madalin.bucur; +Cc: eric.dumazet, netdev, timo.teras
In-Reply-To: <1317056676-23584-1-git-send-email-madalin.bucur@freescale.com>
From: Madalin Bucur <madalin.bucur@freescale.com>
Date: Mon, 26 Sep 2011 20:04:36 +0300
> return value of dst_alloc must be checked before use
>
> Signed-off-by: Madalin Bucur <madalin.bucur@freescale.com>
Applied.
^ permalink raw reply
* Re: [PATCH 2/2] net: check return value for dst_alloc
From: David Miller @ 2011-09-27 19:32 UTC (permalink / raw)
To: madalin.bucur; +Cc: eric.dumazet, netdev, timo.teras
In-Reply-To: <1317056696-23611-1-git-send-email-madalin.bucur@freescale.com>
From: Madalin Bucur <madalin.bucur@freescale.com>
Date: Mon, 26 Sep 2011 20:04:56 +0300
> return value of dst_alloc must be checked before use
>
> Signed-off-by: Madalin Bucur <madalin.bucur@freescale.com>
Applied.
^ permalink raw reply
* Re: [PATCH] net/flow: remove sleeping and deferral mechanism from flow_cache_flush
From: David Miller @ 2011-09-27 19:31 UTC (permalink / raw)
To: madalin.bucur; +Cc: eric.dumazet, netdev, timo.teras
In-Reply-To: <20110927.152836.1747700807304689813.davem@davemloft.net>
From: David Miller <davem@davemloft.net>
Date: Tue, 27 Sep 2011 15:28:36 -0400 (EDT)
> afinfo->garbage_collect is the only other place __xfrm_garbage_collect
> is referenced, and that is completely unused and should thus be deleted
> (I'll take care of that in net-next).
Nevermind I see how these are referenced directly via xfrm4_policy.c
and xfrm6_policy.c, sigh...
^ permalink raw reply
* Re: [PATCH] ipv6-multicast: Fix memory leak in IPv6 multicast.
From: David Miller @ 2011-09-27 19:34 UTC (permalink / raw)
To: greearb; +Cc: netdev
In-Reply-To: <1316819461-3192-1-git-send-email-greearb@candelatech.com>
From: greearb@candelatech.com
Date: Fri, 23 Sep 2011 16:11:01 -0700
> From: Ben Greear <greearb@candelatech.com>
>
> If reg_vif_xmit cannot find a routing entry, be sure to
> free the skb before returning the error.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
Applied.
^ permalink raw reply
* Re: [RFC PATCH] net: Always fire at least one linkwatch event
From: Neil Horman @ 2011-09-27 19:34 UTC (permalink / raw)
To: David Miller; +Cc: netdev, jfeeney
In-Reply-To: <20110927.145943.1365764295978178226.davem@redhat.com>
On Tue, Sep 27, 2011 at 02:59:43PM -0400, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Wed, 21 Sep 2011 15:51:29 -0400
>
> > It was recently noted that the tg3 driver had a problem in that after boot a
> > kernel and if-upping the tg3 interface the sysfs operstate attribute continued
> > to read 'unkown'. This was happening because tg3 assumes the default carrier
> > state (which is to say the __LINK_STATE_NOCARRIER bit is clear) is correct.
> > That said, when the device is if-upped, and the open path, calls
> > netif_carrier_on, the test_and_set_bit call in that function returns false
> > (since the bit was previously zero from its initial state). This means that
> > netif_carrier_on call never generates a linkwatch event, and as a result
> > dev->operstate never gets recomputed. This could be fixed by unconditionally
> > calling netif_carrier_off in the probe routine, to simply force a state change
> > on that bit, but that seems like a sub-par solution, given that many drivers may
> > have this error. Instead it seems like it might be better to burn an extra bit
> > in the state field to indicate that the CARRIER bit is still in the initial
> > state and our first call to netif_carrier_[off|on] should always fire a
> > linkwatch event.
>
> I'm finding this analysis hard to follow.
>
> tg3_open() does netif_carrier_off(), and this will set the
> __LINK_STATE_NOCARRIER bit.
>
Sorry, I should have explained further. In the interests of full disclosure,
this was initially reported on a RHEL 2.6.32 kernel, where netif_carrier_off was
not called from tg3_open. As a result, when tg3_carrier_on was called later in
the open path, the test_and_clear would return 0, since NOCARRIER was
initialized to 0, and we wouldn't fire a linkwatch event, which in turn meant
that operstate was never updated until a full ifup/down/up cycle was completed.
So tg3 actually works properly upstream, but the larger issue remains - Drivers
individually must set and clear the NOCARRIER flag in order to effectively prime
the linkwatch state machine, which seems to me haphazard and prone to recurring
bugs. What I'm proposing here is a driver independent method of ensuring that
the first call to netif_carrier_off/on gets called regardless of initial state.
This prevents drivers from having to individually remember to call
netif_carrier_off at the start of an open routine, which visually makes more
sense to me, especially when they almost immediately call netif_carrier_on right
afterwards.
Hope that clarifies things somewhat.
Neil
^ permalink raw reply
* Re: [PATCH] ipv6-multicast: Fix memory leak in input path.
From: Ben Greear @ 2011-09-27 19:34 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20110927.151616.1706187903528893128.davem@davemloft.net>
On 09/27/2011 12:16 PM, David Miller wrote:
> From: greearb@candelatech.com
> Date: Tue, 27 Sep 2011 11:58:17 -0700
>
>> From: Ben Greear<greearb@candelatech.com>
>>
>> Have to free the skb before returning if we fail
>> the fib lookup.
>>
>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>
> Applied, thanks.
Thanks.
This bug was introduced in 2.6.35, I believe, so should probably send
this to stable as well.
Thanks,
Ben
> --
> 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
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [RFC PATCH] net: Always fire at least one linkwatch event
From: David Miller @ 2011-09-27 19:49 UTC (permalink / raw)
To: nhorman; +Cc: netdev, jfeeney
In-Reply-To: <20110927193413.GA30020@hmsreliant.think-freely.org>
From: Neil Horman <nhorman@tuxdriver.com>
Date: Tue, 27 Sep 2011 15:34:13 -0400
> So tg3 actually works properly upstream, but the larger issue remains - Drivers
> individually must set and clear the NOCARRIER flag in order to effectively prime
> the linkwatch state machine, which seems to me haphazard and prone to recurring
> bugs.
Driver controls when the PHY is reset, auto-negotiation is started, etc. so it is
the only entity which is in the position to set the correct state.
So we kind of depend upon drivers managing the state correctly and accurately.
If I follow what tg3 is currently doing, just to show an example, it first
sets carrier off then resets then entire chip atomically. This reset will
restart auto-neg, etc. and trigger a subsequent link-up event which will
netif_carrier_on() and get the proper transition.
^ 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