Re: [PATCH][MIPS][7/7] AR7: ethernet
Overall, looks pretty clean, good job! Comments: 1) [major issue] Don't take and release a heavy lock on every single RX packet. 2) remove net_device_stats from private structure, and use net_device::stats 3) rx_ring_size should not be a module param, since that should be supported via ethtool 4) cpmac_tx_timeout() doesn't really do anything to alleviate the condition 5) don't set dev-mem_start and dev-mem_end, those are fields that are going away, and that's not their intended purpose. just store a pointer to the resource info. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH][MIPS][7/7] AR7: ethernet
Driver for the cpmac 100M ethernet driver. Jeff, here is the meat ;) Signed-off-by: Matteo Croce [EMAIL PROTECTED] Signed-off-by: Eugene Konev [EMAIL PROTECTED] diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index 6a0863e..28ba0dc 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -1822,6 +1822,15 @@ config SC92031 To compile this driver as a module, choose M here: the module will be called sc92031. This is recommended. +config CPMAC + tristate TI AR7 CPMAC Ethernet support (EXPERIMENTAL) + depends on NET_ETHERNET EXPERIMENTAL AR7 + select PHYLIB + select FIXED_PHY + select FIXED_MII_100_FDX + help + TI AR7 CPMAC Ethernet support + config NET_POCKET bool Pocket and portable adapters depends on PARPORT diff --git a/drivers/net/Makefile b/drivers/net/Makefile index 9501d64..b536934 100644 --- a/drivers/net/Makefile +++ b/drivers/net/Makefile @@ -157,6 +157,7 @@ obj-$(CONFIG_8139CP) += 8139cp.o obj-$(CONFIG_8139TOO) += 8139too.o obj-$(CONFIG_ZNET) += znet.o obj-$(CONFIG_LAN_SAA9730) += saa9730.o +obj-$(CONFIG_CPMAC) += cpmac.o obj-$(CONFIG_DEPCA) += depca.o obj-$(CONFIG_EWRK3) += ewrk3.o obj-$(CONFIG_ATP) += atp.o diff --git a/drivers/net/cpmac.c b/drivers/net/cpmac.c new file mode 100644 index 000..50aad94 --- /dev/null +++ b/drivers/net/cpmac.c @@ -0,0 +1,1166 @@ +/* + * Copyright (C) 2006, 2007 Eugene Konev + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * 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. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include linux/module.h +#include linux/init.h +#include linux/moduleparam.h + +#include linux/sched.h +#include linux/kernel.h +#include linux/slab.h +#include linux/errno.h +#include linux/types.h +#include linux/delay.h +#include linux/version.h + +#include linux/netdevice.h +#include linux/etherdevice.h +#include linux/ethtool.h +#include linux/skbuff.h +#include linux/mii.h +#include linux/phy.h +#include linux/platform_device.h +#include linux/dma-mapping.h +#include asm/gpio.h + +MODULE_AUTHOR(Eugene Konev); +MODULE_DESCRIPTION(TI AR7 ethernet driver (CPMAC)); +MODULE_LICENSE(GPL); + +static int rx_ring_size = 64; +static int disable_napi; +static int debug_level = 8; +static int dumb_switch; + +module_param(rx_ring_size, int, 0644); +module_param(disable_napi, int, 0644); +/* Next 2 are only used in cpmac_probe, so it's pointless to change them */ +module_param(debug_level, int, 0444); +module_param(dumb_switch, int, 0444); + +MODULE_PARM_DESC(rx_ring_size, Size of rx ring (in skbs)); +MODULE_PARM_DESC(disable_napi, Disable NAPI polling); +MODULE_PARM_DESC(debug_level, Number of NETIF_MSG bits to enable); +MODULE_PARM_DESC(dumb_switch, Assume switch is not connected to MDIO bus); + +/* frame size + 802.1q tag */ +#define CPMAC_SKB_SIZE (ETH_FRAME_LEN + 4) +#define CPMAC_TX_RING_SIZE 8 + +/* Ethernet registers */ +#define CPMAC_TX_CONTROL 0x0004 +#define CPMAC_TX_TEARDOWN 0x0008 +#define CPMAC_RX_CONTROL 0x0014 +#define CPMAC_RX_TEARDOWN 0x0018 +#define CPMAC_MBP 0x0100 +# define MBP_RXPASSCRC 0x4000 +# define MBP_RXQOS 0x2000 +# define MBP_RXNOCHAIN 0x1000 +# define MBP_RXCMF 0x0100 +# define MBP_RXSHORT 0x0080 +# define MBP_RXCEF 0x0040 +# define MBP_RXPROMISC 0x0020 +# define MBP_PROMISCCHAN(channel) (((channel) 0x7) 16) +# define MBP_RXBCAST 0x2000 +# define MBP_BCASTCHAN(channel)(((channel) 0x7) 8) +# define MBP_RXMCAST 0x0020 +# define MBP_MCASTCHAN(channel)((channel) 0x7) +#define CPMAC_UNICAST_ENABLE 0x0104 +#define CPMAC_UNICAST_CLEAR0x0108 +#define CPMAC_MAX_LENGTH 0x010c +#define CPMAC_BUFFER_OFFSET0x0110 +#define CPMAC_MAC_CONTROL 0x0160 +# define MAC_TXPTYPE 0x0200 +# define MAC_TXPACE0x0040 +# define MAC_MII 0x0020 +# define MAC_TXFLOW0x0010 +# define MAC_RXFLOW0x0008 +# define MAC_MTEST 0x0004 +# define MAC_LOOPBACK
Re: [PATCH][MIPS][7/7] AR7: ethernet
On Thu, Sep 13, 2007 at 02:42:46AM +0100, Thiemo Seufer wrote: All struct members here are sized such that there is no padding needed, so the packed attribute doesn't buy you anything - unless of course the entire structure is missaligned but I don't see how that would be possible in this driver so the __attribute__ ((packed)) should go - it result in somwhat larger and slower code. FWIW, a modern gcc will warn about such superfluous packed attributes, that's another reason to remove those. I doubt it will in this case; the packed structure is dereferenced by a pointer so no way for gcc to know the alignment. Ralf - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][MIPS][7/7] AR7: ethernet
On Sat, Sep 08, 2007 at 02:23:00AM +0200, Matteo Croce wrote:
Driver for the cpmac 100M ethernet driver.
It works fine disabling napi support, enabling it gives a kernel panic
when the first IPv6 packet has to be forwarded.
Other than that works fine.
Signed-off-by: Matteo Croce [EMAIL PROTECTED]
Signed-off-by: Eugene Konev [EMAIL PROTECTED]
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index d9b7d9c..6f38a84 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -1822,6 +1822,15 @@ config SC92031
To compile this driver as a module, choose M here: the module
will be called sc92031. This is recommended.
+config CPMAC
+ tristate TI AR7 CPMAC Ethernet support (EXPERIMENTAL)
+ depends on NET_ETHERNET EXPERIMENTAL AR7
The dependency on NET_ETHERNET is not needed because this config block is
enclosed in a
if NET_ETHERNET
...
endif # NET_ETHERNET
block.
+ select PHYLIB
+ select FIXED_PHY
+ select FIXED_MII_100_FDX
+ help
+ TI AR7 CPMAC Ethernet support
+
config NET_POCKET
bool Pocket and portable adapters
depends on PARPORT
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 535d2a0..bb22df9 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -156,6 +156,7 @@ obj-$(CONFIG_8139CP) += 8139cp.o
obj-$(CONFIG_8139TOO) += 8139too.o
obj-$(CONFIG_ZNET) += znet.o
obj-$(CONFIG_LAN_SAA9730) += saa9730.o
+obj-$(CONFIG_CPMAC) += cpmac.o
obj-$(CONFIG_DEPCA) += depca.o
obj-$(CONFIG_EWRK3) += ewrk3.o
obj-$(CONFIG_ATP) += atp.o
diff --git a/drivers/net/cpmac.c b/drivers/net/cpmac.c
new file mode 100644
index 000..c10ab08
--- /dev/null
+++ b/drivers/net/cpmac.c
@@ -0,0 +1,1194 @@
+/*
+ * Copyright (C) 2006, 2007 Eugene Konev
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include linux/module.h
+#include linux/init.h
+#include linux/moduleparam.h
+
+#include linux/sched.h
+#include linux/kernel.h
+#include linux/slab.h
+#include linux/errno.h
+#include linux/types.h
+#include linux/delay.h
+#include linux/version.h
+
+#include linux/netdevice.h
+#include linux/etherdevice.h
+#include linux/ethtool.h
+#include linux/skbuff.h
+#include linux/mii.h
+#include linux/phy.h
+#include linux/platform_device.h
+#include asm/ar7/ar7.h
+#include gpio.h
+
+MODULE_AUTHOR(Eugene Konev);
+MODULE_DESCRIPTION(TI AR7 ethernet driver (CPMAC));
+MODULE_LICENSE(GPL);
+
+static int rx_ring_size = 64;
+static int disable_napi;
+module_param(rx_ring_size, int, 64);
+module_param(disable_napi, int, 0);
+MODULE_PARM_DESC(rx_ring_size, Size of rx ring (in skbs));
+MODULE_PARM_DESC(disable_napi, Disable NAPI polling);
+
+/* Register definitions */
+struct cpmac_control_regs {
+ u32 revision;
+ u32 control;
+ u32 teardown;
+ u32 unused;
+} __attribute__ ((packed));
+
+struct cpmac_int_regs {
+ u32 stat_raw;
+ u32 stat_masked;
+ u32 enable;
+ u32 clear;
+} __attribute__ ((packed));
+
+struct cpmac_stats {
+ u32 good;
+ u32 bcast;
+ u32 mcast;
+ u32 pause;
+ u32 crc_error;
+ u32 align_error;
+ u32 oversized;
+ u32 jabber;
+ u32 undersized;
+ u32 fragment;
+ u32 filtered;
+ u32 qos_filtered;
+ u32 octets;
+} __attribute__ ((packed));
All struct members here are sized such that there is no padding needed, so
the packed attribute doesn't buy you anything - unless of course the
entire structure is missaligned but I don't see how that would be possible
in this driver so the __attribute__ ((packed)) should go - it result in
somwhat larger and slower code.
In any case, the __packed attribute is prefered over __attribute__ ((packed))
for readability sake.
+
+struct cpmac_regs {
+ struct cpmac_control_regs tx_ctrl;
+ struct cpmac_control_regs rx_ctrl;
+ u32 unused1[56];
+ u32 mbp;
+/* MBP bits */
+#define MBP_RXPASSCRC 0x4000
+#define MBP_RXQOS 0x2000
+#define MBP_RXNOCHAIN 0x1000
+#define MBP_RXCMF 0x0100
+#define MBP_RXSHORT 0x0080
+#define MBP_RXCEF 0x0040
+#define MBP_RXPROMISC 0x0020
+#define MBP_PROMISCCHAN(chan)
Re: [PATCH][MIPS][7/7] AR7: ethernet
Ralf Baechle wrote:
On Sat, Sep 08, 2007 at 02:23:00AM +0200, Matteo Croce wrote:
[snip]
+/* Register definitions */
+struct cpmac_control_regs {
+ u32 revision;
+ u32 control;
+ u32 teardown;
+ u32 unused;
+} __attribute__ ((packed));
+
+struct cpmac_int_regs {
+ u32 stat_raw;
+ u32 stat_masked;
+ u32 enable;
+ u32 clear;
+} __attribute__ ((packed));
+
+struct cpmac_stats {
+ u32 good;
+ u32 bcast;
+ u32 mcast;
+ u32 pause;
+ u32 crc_error;
+ u32 align_error;
+ u32 oversized;
+ u32 jabber;
+ u32 undersized;
+ u32 fragment;
+ u32 filtered;
+ u32 qos_filtered;
+ u32 octets;
+} __attribute__ ((packed));
All struct members here are sized such that there is no padding needed, so
the packed attribute doesn't buy you anything - unless of course the
entire structure is missaligned but I don't see how that would be possible
in this driver so the __attribute__ ((packed)) should go - it result in
somwhat larger and slower code.
FWIW, a modern gcc will warn about such superfluous packed attributes,
that's another reason to remove those.
Thiemo
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][MIPS][7/7] AR7: ethernet
On Thu, 6 Sep 2007, Andrew Morton wrote:
On Thu, 6 Sep 2007 17:34:10 +0200 Matteo Croce [EMAIL PROTECTED] wrote:
Driver for the cpmac 100M ethernet driver.
It works fine disabling napi support, enabling it gives a kernel panic
when the first IPv6 packet has to be forwarded.
Other than that works fine.
The driver does a lot of open-coded dma_cache_inv() calls (in a way which
assumes a 32-bit bus, too). I assume that dma_cache_inv() is some mips
No, even i386 has it ;-)
thing. I'd have thought that it would be better to use the dma mapping API
thoughout the driver, and its associated dma invalidation APIs.
However, Ralf just posted a patch to remove it on all architectures, and
driver writers should consider it gone.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [EMAIL PROTECTED]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][MIPS][7/7] AR7: ethernet
Matteo Croce wrote: Il Friday 07 September 2007 00:30:25 Andrew Morton ha scritto: On Thu, 6 Sep 2007 17:34:10 +0200 Matteo Croce [EMAIL PROTECTED] wrote: Driver for the cpmac 100M ethernet driver. It works fine disabling napi support, enabling it gives a kernel panic when the first IPv6 packet has to be forwarded. Other than that works fine. I'm not too sure why I got cc'ed on this (and not on patches 1-6?) but whatever. I mailed every maintainer in the respective section in the file MAINTAINERS and you were in the NETWORK DEVICE DRIVERS section This patch introduces quite a number of basic coding-style mistakes. Please run it through scripts/checkpatch.pl and review the output. Already done. I'm collecting other suggestions before committing cool, I'll wait for the resend before reviewing, then. As an author I understand that fixing up coding style / cosmetic stuff rather than meat is annoying. But it is important to emphasize that a clean driver is what makes a good, thorough, effective review possible. Jeff - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][MIPS][7/7] AR7: ethernet
On Thu, 6 Sep 2007 17:34:10 +0200 Matteo Croce [EMAIL PROTECTED] wrote:
Driver for the cpmac 100M ethernet driver.
It works fine disabling napi support, enabling it gives a kernel panic
when the first IPv6 packet has to be forwarded.
Other than that works fine.
I'm not too sure why I got cc'ed on this (and not on patches 1-6?) but
whatever.
This patch introduces quite a number of basic coding-style mistakes.
Please run it through scripts/checkpatch.pl and review the output.
The patch introduces vast number of volatile structure fields. Please see
Documentation/volatile-considered-harmful.txt.
The patch inroduces a modest number of unneeded (and undesirable) casts of
void*, such as
+ struct cpmac_mdio_regs *regs = (struct cpmac_mdio_regs *)bus-priv;
please check for those and fix them up.
The driver implements a driver-private skb pool. I don't know if this is
something which we like net drivers doing? If it is approved then surely
there should be a common implementation for it somewhere?
The driver does a lot of open-coded dma_cache_inv() calls (in a way which
assumes a 32-bit bus, too). I assume that dma_cache_inv() is some mips
thing. I'd have thought that it would be better to use the dma mapping API
thoughout the driver, and its associated dma invalidation APIs.
The driver has some LINUX_VERSION_CODE ifdefs. We usually prefer that such
code not be present in a merged-up driver.
+ priv-regs-mac_hash_low = 0x;
+ priv-regs-mac_hash_high = 0x;
+ } else {
+ for (i = 0, iter = dev-mc_list; i dev-mc_count;
+ i++, iter = iter-next) {
+ hash = 0;
+ tmp = iter-dmi_addr[0];
+ hash ^= (tmp 2) ^ (tmp 4);
+ tmp = iter-dmi_addr[1];
+ hash ^= (tmp 4) ^ (tmp 2);
+ tmp = iter-dmi_addr[2];
+ hash ^= (tmp 6) ^ tmp;
+ tmp = iter-dmi_addr[4];
+ hash ^= (tmp 2) ^ (tmp 4);
+ tmp = iter-dmi_addr[5];
+ hash ^= (tmp 4) ^ (tmp 2);
+ tmp = iter-dmi_addr[6];
+ hash ^= (tmp 6) ^ tmp;
+ hash = 0x3f;
+ if (hash 32) {
+ hashlo |= 1hash;
+ } else {
+ hashhi |= 1(hash - 32);
+ }
+ }
+
+ priv-regs-mac_hash_low = hashlo;
+ priv-regs-mac_hash_high = hashhi;
+ }
Do we not have a library function anywhere which will perform this little
multicasting hash?
+static inline struct sk_buff *cpmac_rx_one(struct net_device *dev,
+struct cpmac_priv *priv,
+struct cpmac_desc *desc)
+{
+ unsigned long flags;
+ char *data;
+ struct sk_buff *skb, *result = NULL;
+
+ priv-regs-rx_ack[0] = virt_to_phys(desc);
+ if (unlikely(!desc-datalen)) {
+ if (printk_ratelimit())
+ printk(KERN_WARNING %s: rx: spurious interrupt\n,
+dev-name);
+ priv-stats.rx_errors++;
+ return NULL;
+ }
+
+ spin_lock_irqsave(priv-lock, flags);
+ skb = cpmac_get_skb(dev);
+ if (likely(skb)) {
+ data = (char *)phys_to_virt(desc-hw_data);
+ dma_cache_inv((u32)data, desc-datalen);
+ skb_put(desc-skb, desc-datalen);
+ desc-skb-protocol = eth_type_trans(desc-skb, dev);
+ desc-skb-ip_summed = CHECKSUM_NONE;
+ priv-stats.rx_packets++;
+ priv-stats.rx_bytes += desc-datalen;
+ result = desc-skb;
+ desc-skb = skb;
+ } else {
+#ifdef CPMAC_DEBUG
+ if (printk_ratelimit())
+ printk(%s: low on skbs, dropping packet\n,
+dev-name);
+#endif
+ priv-stats.rx_dropped++;
+ }
+ spin_unlock_irqrestore(priv-lock, flags);
+
+ desc-hw_data = virt_to_phys(desc-skb-data);
+ desc-buflen = CPMAC_SKB_SIZE;
+ desc-dataflags = CPMAC_OWN;
+ dma_cache_wback((u32)desc, 16);
+
+ return result;
+}
This function is far too large to be inlined.
+static irqreturn_t cpmac_irq(int irq, void *dev_id)
+{
+ struct net_device *dev = (struct net_device *)dev_id;
unneeded cast
+static void cpmac_tx_timeout(struct net_device *dev)
+{
+ struct cpmac_priv *priv = netdev_priv(dev);
+ struct cpmac_desc *desc;
+
+ priv-stats.tx_errors++;
+ desc =
Re: [PATCH][MIPS][7/7] AR7: ethernet
Il Friday 07 September 2007 00:30:25 Andrew Morton ha scritto:
On Thu, 6 Sep 2007 17:34:10 +0200 Matteo Croce [EMAIL PROTECTED] wrote:
Driver for the cpmac 100M ethernet driver.
It works fine disabling napi support, enabling it gives a kernel panic
when the first IPv6 packet has to be forwarded.
Other than that works fine.
I'm not too sure why I got cc'ed on this (and not on patches 1-6?) but
whatever.
I mailed every maintainer in the respective section in the file MAINTAINERS
and you were in the NETWORK DEVICE DRIVERS section
This patch introduces quite a number of basic coding-style mistakes.
Please run it through scripts/checkpatch.pl and review the output.
Already done. I'm collecting other suggestions before committing
The patch introduces vast number of volatile structure fields. Please see
Documentation/volatile-considered-harmful.txt.
Removing them and the kernel hangs at module load
The patch inroduces a modest number of unneeded (and undesirable) casts of
void*, such as
+ struct cpmac_mdio_regs *regs = (struct cpmac_mdio_regs *)bus-priv;
please check for those and fix them up.
Done
The driver implements a driver-private skb pool. I don't know if this is
something which we like net drivers doing? If it is approved then surely
there should be a common implementation for it somewhere?
Are you referring at cpmac_poll?
The driver has some LINUX_VERSION_CODE ifdefs. We usually prefer that such
code not be present in a merged-up driver.
I will remove in the final release, now I need for testing: my running kernel
is older than current git
+ priv-regs-mac_hash_low = 0x;
+ priv-regs-mac_hash_high = 0x;
+ } else {
+ for (i = 0, iter = dev-mc_list; i dev-mc_count;
+ i++, iter = iter-next) {
+ hash = 0;
+ tmp = iter-dmi_addr[0];
+ hash ^= (tmp 2) ^ (tmp 4);
+ tmp = iter-dmi_addr[1];
+ hash ^= (tmp 4) ^ (tmp 2);
+ tmp = iter-dmi_addr[2];
+ hash ^= (tmp 6) ^ tmp;
+ tmp = iter-dmi_addr[4];
+ hash ^= (tmp 2) ^ (tmp 4);
+ tmp = iter-dmi_addr[5];
+ hash ^= (tmp 4) ^ (tmp 2);
+ tmp = iter-dmi_addr[6];
+ hash ^= (tmp 6) ^ tmp;
+ hash = 0x3f;
+ if (hash 32) {
+ hashlo |= 1hash;
+ } else {
+ hashhi |= 1(hash - 32);
+ }
+ }
+
+ priv-regs-mac_hash_low = hashlo;
+ priv-regs-mac_hash_high = hashhi;
+ }
Do we not have a library function anywhere which will perform this little
multicasting hash?
Can you tell me the function so i'll implement it?
+static inline struct sk_buff *cpmac_rx_one(struct net_device *dev,
+ struct cpmac_priv *priv,
+ struct cpmac_desc *desc)
+{
+ unsigned long flags;
+ char *data;
+ struct sk_buff *skb, *result = NULL;
+
+ priv-regs-rx_ack[0] = virt_to_phys(desc);
+ if (unlikely(!desc-datalen)) {
+ if (printk_ratelimit())
+ printk(KERN_WARNING %s: rx: spurious interrupt\n,
+ dev-name);
+ priv-stats.rx_errors++;
+ return NULL;
+ }
+
+ spin_lock_irqsave(priv-lock, flags);
+ skb = cpmac_get_skb(dev);
+ if (likely(skb)) {
+ data = (char *)phys_to_virt(desc-hw_data);
+ dma_cache_inv((u32)data, desc-datalen);
+ skb_put(desc-skb, desc-datalen);
+ desc-skb-protocol = eth_type_trans(desc-skb, dev);
+ desc-skb-ip_summed = CHECKSUM_NONE;
+ priv-stats.rx_packets++;
+ priv-stats.rx_bytes += desc-datalen;
+ result = desc-skb;
+ desc-skb = skb;
+ } else {
+#ifdef CPMAC_DEBUG
+ if (printk_ratelimit())
+ printk(%s: low on skbs, dropping packet\n,
+ dev-name);
+#endif
+ priv-stats.rx_dropped++;
+ }
+ spin_unlock_irqrestore(priv-lock, flags);
+
+ desc-hw_data = virt_to_phys(desc-skb-data);
+ desc-buflen = CPMAC_SKB_SIZE;
+ desc-dataflags = CPMAC_OWN;
+ dma_cache_wback((u32)desc, 16);
+
+ return result;
+}
This function is far too large to be inlined.
+static irqreturn_t cpmac_irq(int irq, void *dev_id)
+{
+ struct net_device *dev = (struct net_device *)dev_id;
unneeded cast
fixed
Re: [PATCH][MIPS][7/7] AR7: ethernet
On Thu, 6 Sep 2007 15:30:25 -0700 Andrew Morton wrote:
On Thu, 6 Sep 2007 17:34:10 +0200 Matteo Croce [EMAIL PROTECTED] wrote:
Driver for the cpmac 100M ethernet driver.
It works fine disabling napi support, enabling it gives a kernel panic
when the first IPv6 packet has to be forwarded.
Other than that works fine.
I'm not too sure why I got cc'ed on this (and not on patches 1-6?) but
whatever.
This patch introduces quite a number of basic coding-style mistakes.
Please run it through scripts/checkpatch.pl and review the output.
The patch introduces vast number of volatile structure fields. Please see
Documentation/volatile-considered-harmful.txt.
The patch inroduces a modest number of unneeded (and undesirable) casts of
void*, such as
+ struct cpmac_mdio_regs *regs = (struct cpmac_mdio_regs *)bus-priv;
please check for those and fix them up.
The driver implements a driver-private skb pool. I don't know if this is
something which we like net drivers doing? If it is approved then surely
there should be a common implementation for it somewhere?
The driver does a lot of open-coded dma_cache_inv() calls (in a way which
assumes a 32-bit bus, too). I assume that dma_cache_inv() is some mips
thing. I'd have thought that it would be better to use the dma mapping API
thoughout the driver, and its associated dma invalidation APIs.
The driver has some LINUX_VERSION_CODE ifdefs. We usually prefer that such
code not be present in a merged-up driver.
+ priv-regs-mac_hash_low = 0x;
+ priv-regs-mac_hash_high = 0x;
+ } else {
+ for (i = 0, iter = dev-mc_list; i dev-mc_count;
+ i++, iter = iter-next) {
+ hash = 0;
+ tmp = iter-dmi_addr[0];
+ hash ^= (tmp 2) ^ (tmp 4);
+ tmp = iter-dmi_addr[1];
+ hash ^= (tmp 4) ^ (tmp 2);
+ tmp = iter-dmi_addr[2];
+ hash ^= (tmp 6) ^ tmp;
+ tmp = iter-dmi_addr[4];
+ hash ^= (tmp 2) ^ (tmp 4);
+ tmp = iter-dmi_addr[5];
+ hash ^= (tmp 4) ^ (tmp 2);
+ tmp = iter-dmi_addr[6];
+ hash ^= (tmp 6) ^ tmp;
+ hash = 0x3f;
+ if (hash 32) {
+ hashlo |= 1hash;
+ } else {
+ hashhi |= 1(hash - 32);
+ }
+ }
+
+ priv-regs-mac_hash_low = hashlo;
+ priv-regs-mac_hash_high = hashhi;
+ }
Do we not have a library function anywhere which will perform this little
multicasting hash?
Depends on the ethernet controller, but the ones that I know about
just use a CRC (crc-16 IIRC) calculation for the multicast hash.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][MIPS][7/7] AR7: ethernet
On Fri, 7 Sep 2007 01:21:41 +0200 Matteo Croce [EMAIL PROTECTED] wrote: The patch introduces vast number of volatile structure fields. Please see Documentation/volatile-considered-harmful.txt. Removing them and the kernel hangs at module load They can't just be removed. Please see the document. There are I/O APIs which, if properly used, make volatile unneeded. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
