netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stephen Hemminger <stephen.hemminger@vyatta.com>
To: Inaky Perez-Gonzalez <inaky@linux.intel.com>
Cc: wimax@linuxwimax.org, linux-wireless@vger.kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [ANN] WiMAX stack and drivers for Intel WiMAX Link 5050
Date: Tue, 8 Apr 2008 13:56:01 -0500	[thread overview]
Message-ID: <20080408135601.5dad3566@speedy> (raw)
In-Reply-To: <200804011107.38563.inaky@linux.intel.com>

This is the short (not in depth) code review of kernel component of Wimax.

Generic stack: drivers/net/wimax

1. Why spread those over 8 files of 100 lines each. Better to have a single
   file with 1000 lines and get reduced namespace and better compiler inlining
   etc.

2. The debug infrastructure is a mess of ugly macros that are unlikely
   to accepted in the current form, rework or delete it.

3. Use of sysfs for family and version ok, but why bother?
   Please don't build sysfs version checks into the API.

4. __wimax_flush_queue: is a nop, just remove

5. Stack is using generic netlink instead use newer netlink interface
   for management of devices: newlink/dellink instead see macvlan

i2400m hardware driver

1. sysfs
  the inflight file is being used in a /proc style. Either change to
  one value per file or move to /proc/net/i2400m/ethX or better yet use debugfs
  /debugfs/i2400m/ethX/xxx

2. Use internal dev->stats for network stats (may not need get_stats at all)

3. No ioctl stub if not implemented

4. Use netdev_alloc_skb rather than alloc_skb for new code.

5. Use skb_copy_to_linear_data in i2400m_net_rx

6. Since hardware has to copy every received frame. Don't bother with
   checksum offload, the copy does the checksum for you and is safer.

7. Fine grain file organization in i2400m is bogus, put in one file and use
   proper name scope. Having 100 line files is unneeded

8. Fix the FIXME's?

9. Anyway to reuse existing usbnet infrastructure?

10. Since device is USB, Rx is in workqueue, so no need to go through
    netif_rx() softirq, should be able to go through netif_receive_skb

11. net_device and private data are zero on allocation, so no need
   for memset.

12. Since this is new code elminate all legacy ifdef's


  parent reply	other threads:[~2008-04-08 18:56 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-04-01 18:07 [ANN] WiMAX stack and drivers for Intel WiMAX Link 5050 Inaky Perez-Gonzalez
     [not found] ` <200804011107.38563.inaky-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2008-04-01 18:18   ` Stephen Hemminger
2008-04-01 18:33     ` Inaky Perez-Gonzalez
2008-04-08 15:13   ` Stephen Hemminger
2008-04-08 19:04   ` Stephen Hemminger
2008-04-08 18:56 ` Stephen Hemminger [this message]
2008-04-08 19:44   ` Marcel Holtmann
2008-04-08 20:59   ` Inaky Perez-Gonzalez
2008-04-09  6:10     ` Andi Kleen
     [not found]     ` <871w5fpnhs.fsf@basil.nowhere.org>
     [not found]       ` <200804091109.25265.inaky@linux.intel.com>
2008-04-09 20:31         ` Andi Kleen
  -- strict thread matches above, loose matches on Subject: below --
2008-04-08 18:57 jayant

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080408135601.5dad3566@speedy \
    --to=stephen.hemminger@vyatta.com \
    --cc=inaky@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=wimax@linuxwimax.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).