| 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: <OFDCE90357.0EDC5DC1-ONC1257586.003759A3-C1257586.00390AD3@transmode.se>
Date: Fri, 27 Mar 2009 11:23:04 +0100
From: Joakim Tjernlund <Joakim.Tjernlund@...nsmode.se>
To: Li Yang <leoli@...escale.com>
Cc: avorontsov@...mvista.com, linuxppc-dev@...abs.org,
netdev@...r.kernel.org, pku.leo@...il.com
Subject: Re: [PATCH] ucc_geth: Rework the TX logic.
pku.leo@...il.com wrote on 27/03/2009 10:45:12:
> On Fri, Mar 27, 2009 at 1:44 AM, Joakim Tjernlund
> <Joakim.Tjernlund@...nsmode.se> wrote:
> > The line:
> > if ((bd == ugeth->txBd[txQ]) && (netif_queue_stopped(dev) == 0))
> > break;
> > in ucc_geth_tx() didn not make sense to me. Rework & cleanup
> > this logic to something understandable.
> > ---
> >
> > Reworked the patch according to Antons comments.
> >
> > drivers/net/ucc_geth.c | 66
+++++++++++++++++++++++++++--------------------
> > 1 files changed, 38 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> > index 7fc91aa..465de3a 100644
> > --- a/drivers/net/ucc_geth.c
> > +++ b/drivers/net/ucc_geth.c
> > @@ -161,6 +161,17 @@ static struct ucc_geth_info ugeth_primary_info =
{
> >
> > static struct ucc_geth_info ugeth_info[8];
> >
> > +
> > +static inline u32 __iomem *bd2buf(u8 __iomem *bd)
> > +{
> > + return (u32 __iomem *)(bd+4);
> > +}
> > +
> > +static inline u32 __iomem *bd2status(u8 __iomem *bd)
> > +{
> > + return (u32 __iomem *)bd;
> > +}
>
>
> When the driver was reviewed for upstream merge, I was told to remove
> this kind of thing in favor of direct struct qe_bd access. So do we
> really have a guideline on this? Or it's just a matter of personal
> preference?
This is a bit better than the current type casts. Moving over to qe_bd
accesses is a bigger cleanup that will have to be a seperate patch.
I am not sure it is an all win as you probably loose the ability
to read both status and len in one access.
>
> > +
> > #ifdef DEBUG
> > static void mem_disp(u8 *addr, int size)
> > {
> > @@ -3048,6 +3059,7 @@ static int ucc_geth_start_xmit(struct sk_buff
*skb, struct net_device *dev)
> > u8 __iomem *bd; /* BD pointer */
> > u32 bd_status;
> > u8 txQ = 0;
> > + int tx_ind;
> >
> > ugeth_vdbg("%s: IN", __func__);
> >
> > @@ -3057,21 +3069,19 @@ static int ucc_geth_start_xmit(struct sk_buff
*skb, struct net_device *dev)
> >
> > /* Start from the next BD that should be filled */
> > bd = ugeth->txBd[txQ];
> > - bd_status = in_be32((u32 __iomem *)bd);
> > + bd_status = in_be32(bd2status(bd));
> > /* Save the skb pointer so we can free it later */
> > - ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] = skb;
> > + tx_ind = ugeth->skb_curtx[txQ];
> > + ugeth->tx_skbuff[txQ][tx_ind] = skb;
> >
> > /* Update the current skb pointer (wrapping if this was the
last) */
> > - ugeth->skb_curtx[txQ] =
> > - (ugeth->skb_curtx[txQ] +
> > - 1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
> > + tx_ind = (tx_ind + 1) &
TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
> > + ugeth->skb_curtx[txQ] = tx_ind;
> >
> > /* set up the buffer descriptor */
> > - out_be32(&((struct qe_bd __iomem *)bd)->buf,
> > - dma_map_single(&ugeth->dev->dev, skb->data,
> > - skb->len, DMA_TO_DEVICE));
> > -
> > - /* printk(KERN_DEBUG"skb->data is 0x%x\n",skb->data); */
> > + out_be32(bd2buf(bd),
> > + dma_map_single(&ugeth->dev->dev, skb->data,
> > + skb->len, DMA_TO_DEVICE));
> >
> > bd_status = (bd_status & T_W) | T_R | T_I | T_L | skb->len;
> >
> > @@ -3088,9 +3098,9 @@ static int ucc_geth_start_xmit(struct sk_buff
*skb, struct net_device *dev)
> >
> > /* If the next BD still needs to be cleaned up, then the bds
> > are full. We need to tell the kernel to stop sending us
stuff. */
> > - if (bd == ugeth->confBd[txQ]) {
> > - if (!netif_queue_stopped(dev))
> > - netif_stop_queue(dev);
> > +
> > + if (!in_be32(bd2buf(bd))) {
> > + netif_stop_queue(dev);
>
> Why does this make more sense? The BD with a NULL buffer means the
> ring is full? It's not the normal case.
>
> > }
> >
> > ugeth->txBd[txQ] = bd;
> > @@ -3198,41 +3208,41 @@ static int ucc_geth_tx(struct net_device *dev,
u8 txQ)
> > struct ucc_geth_private *ugeth = netdev_priv(dev);
> > u8 __iomem *bd; /* BD pointer */
> > u32 bd_status;
> > + int tx_ind, num_freed;
> >
> > bd = ugeth->confBd[txQ];
> > - bd_status = in_be32((u32 __iomem *)bd);
> > -
> > + bd_status = in_be32(bd2status(bd));
> > + tx_ind = ugeth->skb_dirtytx[txQ];
> > + num_freed = 0;
> > /* Normal processing. */
> > while ((bd_status & T_R) == 0) {
> > /* BD contains already transmitted buffer. */
> > /* Handle the transmitted buffer and release */
> > /* the BD to be used with the current frame */
> >
> > - if ((bd == ugeth->txBd[txQ]) &&
(netif_queue_stopped(dev) == 0))
> > - break;
> > + if (bd == ugeth->txBd[txQ])
> > + break; /* Queue is empty */
> >
> > dev->stats.tx_packets++;
> >
> > /* Free the sk buffer associated with this TxBD */
> > - dev_kfree_skb(ugeth->
> > -
tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]]);
> > - ugeth->tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]] = NULL;
> > - ugeth->skb_dirtytx[txQ] =
> > - (ugeth->skb_dirtytx[txQ] +
> > - 1) &
TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
> > -
> > - /* We freed a buffer, so now we can restart
transmission */
> > - if (netif_queue_stopped(dev))
> > - netif_wake_queue(dev);
> > + dev_kfree_skb(ugeth->tx_skbuff[txQ][tx_ind]);
> > + ugeth->tx_skbuff[txQ][tx_ind] = NULL;
> > + out_be32(bd2buf(bd), 0); /* Mark it free */
>
> Now I see why you depend on the buffer to judge if the BD ring is
> full. If you want to do this, make sure it's well documented in code,
> or others will be confused. And as Anton already commented, in/out
> access is slow. I think the original way can be fixed if you think
> it's broken, and will have better performance.
The code could use a better comment, but I really think this method
is superior and more robust. The problem is that in/out functions is
much slower than it has to be for QE IO memory.
The original way is broken and I tired to fix it but it always broke
as soon one applied some load.
Jocke
--
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