Christoph Hellwig wrote:

On Sun, Apr 29, 2007 at 10:29:02AM -0700, Jeremy Fitzhardinge wrote:
  

quoted

+#include <linux/version.h>
    

not needed.
  

Yup.

quoted

+#include <xen/xenbus.h>
+#include <xen/interface/io/netif.h>
+#include <xen/interface/memory.h>
+#ifdef CONFIG_XEN_BALLOON
+#include <xen/balloon.h>
+#endif
+#include <xen/interface/grant_table.h>
    

Please don't try to put such a fucked up include hierachy in.
Just move everything under include/xen or you will soon get
problems with the 80 line length limit for your includes..
  

I'm trying to draw the distinction between kernel-internal Xen-related
headers, and the Xen hypervisor ABI itself.  The stuff under
xen/interface is more or less copied in from the main Xen tree, so it
would be nice to keep it distinct.

Also please make sure that <xen/balloon.h> can be included unconditionally,
as we really don't like ifdefs around includes.
  

The balloon driver isn't in this patch set, so I'll drop all references
to it.

quoted

+	grant_ref_t gref_tx_head;
    

What's a grant_ref_t?  Should this really be a typedef or better
a struct type?

Structurally it's just a u32.  I could wrap it up in a structure for
typedef reasons, I suppose.

  Also it really wants a xen_ prefix instead of someting
so generic.
  

OK.

quoted

+ * Implement our own carrier flag: the network stack's version causes delays
+ * when the carrier is re-enabled (in particular, dev_activate() may not
+ * immediately be called, which can cause packet loss).
+ */
    

Did you talk to the networking folks about these problems?
  

Not sure what the deliberations were leading to those changes.

Herbert, Keir?

quoted

+#define netfront_carrier_on(netif)	((netif)->carrier = 1)
+#define netfront_carrier_off(netif)	((netif)->carrier = 0)
+#define netfront_carrier_ok(netif)	((netif)->carrier)
    

Please use proper symbolic names for the ctal states and kill these
wrappers.
  

OK.

Any chance you could avoid these forward-prototypes by reordering
the functions a little?
  

I'll give it a go.

Also a lot of these names are horribly generic.  A proper xennet_
prefix would probably help.
  

Yes.

quoted

+static int __devexit netfront_remove(struct xenbus_device *dev)
+{
+	struct netfront_info *info = dev->dev.driver_data;
+
+	dev_dbg(&dev->dev, "%s\n", dev->nodename);
+
+	netif_disconnect_backend(info);
+
+	del_timer_sync(&info->rx_refill_timer);
+
+	xennet_sysfs_delif(info->netdev);
+
+	unregister_netdev(info->netdev);
+
+	free_netdev(info->netdev);
    

This looks like very wrong ordering to me.  unregister_netdev should
be the first thing in the remove function.
  

I know this ordering was the result of a relatively recent bugfix
replacing something that was completely broken.

quoted

+	SHARED_RING_INIT(rxs);
+	FRONT_RING_INIT(&info->rx, rxs, PAGE_SIZE);
    

Can you replace these shouting macros with proper named functions?
  

Will do.

quoted

+	 * receive ring. This creates a less bursty demand on the memory
+	 * allocator, so should reduce the chance of failed allocation requests
+	 * both for ourself and for other kernel subsystems.
+	 */
+	batch_target = np->rx_target - (req_prod - np->rx.rsp_cons);
+	for (i = skb_queue_len(&np->rx_batch); i < batch_target; i++) {
+		/*
+		 * Allocate an skb and a page. Do not use __dev_alloc_skb as
+		 * that will allocate page-sized buffers which is not
+		 * necessary here.
+		 * 16 bytes added as necessary headroom for netif_receive_skb.
+		 */
+		skb = alloc_skb(RX_COPY_THRESHOLD + 16 + NET_IP_ALIGN,
+				GFP_ATOMIC | __GFP_NOWARN);
    

This comment doesn't make any sense, __dev_alloc_skb is:
  

Yes, it has probably dated.  I'll give it another look.

quoted

+		skb->nh.raw = (void *)skb_shinfo(skb)->frags[0].page;
+		skb->h.raw = skb->nh.raw + rx->offset;
    

Stuff like this won't compile anymore in the current tree.
  

Fixed later in the series.

Thanks,
    J