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