Re: [PATCH net-drivers-2.6 5/6] e100: Performance optimizations to e100 Tx Path

[Ganesh Venkatesan <ganesh.venkatesan@gmail.com>]

On 4/20/05, Randy.Dunlap <rddunlap@osdl.org> wrote:
> On Tue, 19 Apr 2005 22:50:31 -0700 (PDT) Malli Chilakala wrote:

> 
> |
> | -#define E100_WAIT_SCB_TIMEOUT 40
> | +#define E100_WAIT_SCB_TIMEOUT 20000 /* we might have to wait 100ms!!! */
> 
> What correlation is there between 20000 and 100 ms ?
> 

This needs some review and fixing on our side. 

> |  static inline int e100_exec_cmd(struct nic *nic, u8 cmd, dma_addr_t dma_addr)
> |  {
> |       unsigned long flags;
> | @@ -847,6 +847,10 @@ static inline int e100_exec_cb(struct ni
> |                        * because the controller is too busy, so
> |                        * let's just queue the command and try again
> |                        * when another command is scheduled. */
> | +                     if(err == -ENOSPC) {
>                         if (err == -ENOSPC) {
> is preferred (with space after if).
> (same comment for below)

Is there a clear directive on 'if(' versus 'if ('? I see both styles
being used. We are trying to stay consistent with 'if('.


> 
> | +                             //request a reset
> Kernel comment style is /* ... */, not //.
> (same comment for below)
> 

Agreed. Will fix this.

> | +                             schedule_work(&nic->tx_timeout_task);
> | +                     }
> |                       break;
> |               } else {
> |                       nic->cuc_cmd = cuc_resume;
> | @@ -1289,12 +1294,15 @@ static inline void e100_xmit_prepare(str
> |       struct sk_buff *skb)
> |  {
> |       cb->command = nic->tx_command;
> | +     /* interrupt every 16 packets regardless of delay */
> | +     if((nic->cbs_avail & ~15) == nic->cbs_avail) cb->command |= cb_i;
> Don't put if() and statement on one line, please.
> It tends to hide code unintentionally.

Will fix this.

> 
> |       cb->u.tcb.tbd_array = cb->dma_addr + offsetof(struct cb, u.tcb.tbd);
> |       cb->u.tcb.tcb_byte_count = 0;
> |       cb->u.tcb.threshold = nic->tx_threshold;
> |       cb->u.tcb.tbd_count = 1;
> |       cb->u.tcb.tbd.buf_addr = cpu_to_le32(pci_map_single(nic->pdev,
> |               skb->data, skb->len, PCI_DMA_TODEVICE));
> | +     // check for mapping failure?
> |       cb->u.tcb.tbd.size = cpu_to_le16(skb->len);
> |  }
> 
> ---
> ~Randy
> 
>