| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <F363E7AC84E1B646A0358B281A46F4AEA9A0F6565F@HQ1-EXCH03.corp.brocade.com>
Date: Fri, 6 Aug 2010 20:23:21 -0700
From: Debashis Dutt <ddutt@...cade.COM>
To: Stephen Hemminger <shemminger@...tta.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH 1/6] bna: Brocade 10Gb Ethernet device driver
Hi Stephen,
Thanks a lot for your comments.
> + if (likely
> + (wis > BNA_QE_FREE_CNT(tcb, tcb->q_depth) ||
> + vectors > BNA_QE_FREE_CNT(unmap_q, unmap_q->q_depth))) {
> + BNAD_UPDATE_CTR(bnad, netif_queue_stop);
> + return NETDEV_TX_BUSY;
>The transmit routine should check for available space after
> queueing to device, so you can avoid having to return
>TX_BUSY.
However your above comment is not very clear to me.
Our Tx routine already cleans up the Tx buffers at the end, through a tasklet.
Cleaning of Tx buffers happens
1) In the sending context at the end of the Tx routine.
2) In the IRQ context when we get a Tx completion interrupt.
Only thing that we could have done, is do this check again at the end of the Tx routine
and called netif_stop_queue() if required, so that the stack stops sending. Even then I am
not sure that we could avoid the current check and returning TX_BUSY.
It would be nice if you clarify, so that I understand this better.
Thanks
--Debashis
Linux LL Driver Team.
-----Original Message-----
From: Stephen Hemminger [mailto:shemminger@...tta.com]
Sent: Wednesday, August 04, 2010 10:09 AM
To: Rasesh Mody
Cc: netdev@...r.kernel.org; Debashis Dutt; Jing Huang
Subject: Re: [PATCH 1/6] bna: Brocade 10Gb Ethernet device driver
On Tue, 3 Aug 2010 22:15:36 -0700
Rasesh Mody <rmody@...cade.com> wrote:
> From: Rasesh Mody <rmody@...cade.com>
>
> This is patch 1/6 which contains linux driver source for
> Brocade's BR1010/BR1020 10Gb CEE capable ethernet adapter.
> Source is based against net-next-2.6.
>
> We wish this patch to be considered for inclusion in net-next-2.6
>
> Signed-off-by: Rasesh Mody <rmody@...cade.com>
> ---
> bnad.c | 3326 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> bnad.h | 474 ++++++++
> bnad_ethtool.c | 1269 +++++++++++++++++++++
> 3 files changed, 5069 insertions(+)
>
> diff -ruP net-next-2.6.35-rc1-orig/drivers/net/bna/bnad.c net-next-2.6.35-rc1-mod/drivers/net/bna/bnad.c
> --- net-next-2.6.35-rc1-orig/drivers/net/bna/bnad.c 1969-12-31 16:00:00.000000000 -0800
> +++ net-next-2.6.35-rc1-mod/drivers/net/bna/bnad.c 2010-08-02 17:19:19.447239000 -0700
> @@ -0,0 +1,3326 @@
> +/*
> + * Linux network driver for Brocade Converged Network Adapter.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License (GPL) Version 2 as
> + * published by the Free Software Foundation
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +/*
> + * Copyright (c) 2005-2009 Brocade Communications Systems, Inc.
> + * All rights reserved
> + * www.brocade.com
> + */
> +#include <linux/netdevice.h>
> +#include <linux/skbuff.h>
> +#include <linux/etherdevice.h>
> +#include <linux/in.h>
> +#include <linux/ethtool.h>
> +#include <linux/if_vlan.h>
> +#include <linux/if_ether.h>
> +#include <linux/ip.h>
> +
> +#include "bnad.h"
> +#include "bna.h"
> +#include "cna.h"
> +
> +DEFINE_MUTEX(bnad_fwimg_mutex);
> +
> +/*
> + * Module params
> + */
> +static uint bnad_msix_disable;
> +module_param(bnad_msix_disable, uint, 0444);
> +MODULE_PARM_DESC(bnad_msix_disable, "Disable MSIX mode");
> +
> +static uint bnad_ioc_auto_recover = 1;
> +module_param(bnad_ioc_auto_recover, uint, 0444);
> +MODULE_PARM_DESC(bnad_ioc_auto_recover, "Enable / Disable auto recovery");
> +
> +/*
> + * Global variables
> + */
> +u32 bna_id;
> +u32 bnad_rxqs_per_cq = 2;
> +
> +DECLARE_MUTEX(bnad_list_sem);
> +LIST_HEAD(bnad_list);
> +
> +const u8 bnad_bcast_addr[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
Surprised this isn't defined somewhere else.
> +
> +/*
> + * Local MACROS
> + */
> +#define BNAD_TX_UNMAPQ_DEPTH (bnad->txq_depth * 2)
> +
> +#define BNAD_RX_UNMAPQ_DEPTH (bnad->rxq_depth)
> +
> +#define BNAD_GET_MBOX_IRQ(_bnad) \
> + (((_bnad)->cfg_flags & BNAD_CF_MSIX) ? \
> + ((_bnad)->msix_table[(_bnad)->msix_num - 1].vector) : \
> + ((_bnad)->pcidev->irq))
> +
> +#define BNAD_FILL_UNMAPQ_MEM_REQ(_res_info, _num, _depth) \
> +do { \
> + (_res_info)->res_type = BNA_RES_T_MEM; \
> + (_res_info)->res_u.mem_info.mem_type = BNA_MEM_T_KVA; \
> + (_res_info)->res_u.mem_info.num = (_num); \
> + (_res_info)->res_u.mem_info.len = \
> + sizeof(struct bnad_unmap_q) + \
> + (sizeof(struct bnad_skb_unmap) * ((_depth) - 1)); \
> +} while (0)
> +
> +void
> +bnad_add_to_list(struct bnad *bnad)
> +{
> + down(&bnad_list_sem);
> + list_add_tail(&bnad->list_entry, &bnad_list);
> + bna_id++;
> + up(&bnad_list_sem);
> +}
Why do you need to list semaphore? Isn't RTNL mutex held
when this is done. If you have to have own exclusion use
a mutex for this.
> +void
> +bnad_remove_from_list(struct bnad *bnad)
> +{
> + down(&bnad_list_sem);
> + list_del(&bnad->list_entry);
> + up(&bnad_list_sem);
> +}
> +
> +const struct pci_device_id bnad_pci_id_table[] = {
> + {
> + PCI_DEVICE(PCI_VENDOR_ID_BROCADE,
> + PCI_DEVICE_ID_BROCADE_CT),
> + .class = PCI_CLASS_NETWORK_ETHERNET << 8,
> + .class_mask = 0xffff00
> + }, {0, }
> +};
Why is this not static?
> +/* TX */
> +/* bnad_start_xmit : Netdev entry point for Transmit */
> +/* Called under lock held by net_device */
> +
> +netdev_tx_t
> +bnad_start_xmit(struct sk_buff *skb, struct net_device *netdev)
Should also be static...
> +{
> + struct bnad *bnad = netdev_priv(netdev);
> +
> + u16 txq_prod, vlan_tag = 0;
> + u32 unmap_prod, wis, wis_used, wi_range;
> + u32 vectors, vect_id, i, acked;
> + u32 tx_id;
> + int err;
> +
> + struct bnad_tx_info *tx_info;
> + struct bna_tcb *tcb;
> + struct bnad_unmap_q *unmap_q;
> + dma_addr_t dma_addr;
> + struct bna_txq_entry *txqent;
> + bna_txq_wi_ctrl_flag_t flags;
> +
> + if (unlikely
> + (skb->len <= ETH_HLEN || skb->len > BFI_TX_MAX_DATA_PER_PKT)) {
> + dev_kfree_skb(skb);
> + return NETDEV_TX_OK;
> + }
> +
> + /*
> + * Takes care of the Tx that is scheduled between clearing the flag
> + * and the netif_stop_queue() call.
> + */
> + if (unlikely(!test_bit(BNAD_RF_TX_STARTED, &bnad->run_flags))) {
> + dev_kfree_skb(skb);
> + return NETDEV_TX_OK;
> + }
> +
> + tx_id = BNAD_GET_TX_ID(skb);
> +
> + tx_info = &bnad->tx_info[tx_id];
> + tcb = tx_info->tcb[tx_id];
> + unmap_q = tcb->unmap_q;
> +
> + vectors = 1 + skb_shinfo(skb)->nr_frags;
> + if (vectors > BFI_TX_MAX_VECTORS_PER_PKT) {
> + dev_kfree_skb(skb);
> + return NETDEV_TX_OK;
> + }
> + wis = BNA_TXQ_WI_NEEDED(vectors); /* 4 vectors per work item */
> + acked = 0;
> + if (unlikely
> + (wis > BNA_QE_FREE_CNT(tcb, tcb->q_depth) ||
> + vectors > BNA_QE_FREE_CNT(unmap_q, unmap_q->q_depth))) {
> + if ((u16) (*tcb->hw_consumer_index) !=
> + tcb->consumer_index &&
> + !test_and_set_bit(BNAD_TXQ_FREE_SENT, &tcb->flags)) {
> + acked = bnad_free_txbufs(bnad, tcb);
> + bna_ib_ack(tcb->i_dbell, acked);
> + smp_mb__before_clear_bit();
> + clear_bit(BNAD_TXQ_FREE_SENT, &tcb->flags);
> + } else {
> + netif_stop_queue(netdev);
> + BNAD_UPDATE_CTR(bnad, netif_queue_stop);
> + }
> +
> + smp_mb();
> + /*
> + * Check again to deal with race condition between
> + * netif_stop_queue here, and netif_wake_queue in
> + * interrupt handler which is not inside netif tx lock.
> + */
> + if (likely
> + (wis > BNA_QE_FREE_CNT(tcb, tcb->q_depth) ||
> + vectors > BNA_QE_FREE_CNT(unmap_q, unmap_q->q_depth))) {
> + BNAD_UPDATE_CTR(bnad, netif_queue_stop);
> + return NETDEV_TX_BUSY;
The transmit routine should check for available space after
queueing to device, so you can avoid having to return
TX_BUSY.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists