| 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: <20100809111505.1cdc3ae5@s6510>
Date: Mon, 9 Aug 2010 11:15:05 -0400
From: Stephen Hemminger <shemminger@...tta.com>
To: Debashis Dutt <ddutt@...cade.COM>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/6] bna: Brocade 10Gb Ethernet device driver
On Fri, 6 Aug 2010 20:23:21 -0700
Debashis Dutt <ddutt@...cade.COM> wrote:
>
> 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.
The problem is that if device returns TX_BUSY, the net transmit scheduler
will end up re-calling the transmit routine. This looks ok in your driver
because it will set netif_stop_queue
--
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