Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver
On Sunday 15 July 2007 11:27:09 Bryan Wu wrote: +#if defined(CONFIG_BFIN_MAC_USE_L1) +# define bfin_mac_alloc(dma_handle, size) l1_data_sram_zalloc(size) +# define bfin_mac_free(dma_handle, ptr)l1_data_sram_free(ptr) +#else +# define bfin_mac_alloc(dma_handle, size) \ + dma_alloc_coherent(NULL, size, dma_handle, GFP_NORMAL) What is GFP_NORMAL? It's not defined in latest linus' tree. I think you should use GFP_KERNEL, if you can sleep, or GFP_ATOMIC, if you can't. The rest looks OK. Except the endianess issues. It might be no issue on the hardware this runs on, but in favour of clean code you might use leXX_to_cpu and friends anyway. :) This kind of bugs is done so often, even in places where it _does_ matter. So, at least for the human reader, the leXX_to_cpu stuff says that you really understood what you were doing when writing the code. The current code says (to me), that it works by accident, somehow, although it seems you knew what you were doing. :) -- Greetings Michael. - 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 try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver
On Sun, Jul 15, 2007 at 12:36:51PM +0200, Michael Buesch wrote: On Sunday 15 July 2007 11:27:09 Bryan Wu wrote: +#if defined(CONFIG_BFIN_MAC_USE_L1) +# define bfin_mac_alloc(dma_handle, size) l1_data_sram_zalloc(size) +# define bfin_mac_free(dma_handle, ptr)l1_data_sram_free(ptr) +#else +# define bfin_mac_alloc(dma_handle, size) \ +dma_alloc_coherent(NULL, size, dma_handle, GFP_NORMAL) What is GFP_NORMAL? It's not defined in latest linus' tree. I think you should use GFP_KERNEL, if you can sleep, or GFP_ATOMIC, if you can't. Actually this whole thing looks fishy. There should be a struct device for the dma allocation, through a platform_device. And the CONFIG_BFIN_MAC_USE_L1 should go away, the l1 sram should have a dma provider so this can be handled through the dma api. - 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 try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver
On Sun, 2007-07-15 at 12:36 +0200, Michael Buesch wrote:
On Sunday 15 July 2007 11:27:09 Bryan Wu wrote:
+#if defined(CONFIG_BFIN_MAC_USE_L1)
+# define bfin_mac_alloc(dma_handle, size) l1_data_sram_zalloc(size)
+# define bfin_mac_free(dma_handle, ptr)l1_data_sram_free(ptr)
+#else
+# define bfin_mac_alloc(dma_handle, size) \
+dma_alloc_coherent(NULL, size, dma_handle, GFP_NORMAL)
What is GFP_NORMAL? It's not defined in latest linus' tree.
I think you should use GFP_KERNEL, if you can sleep, or GFP_ATOMIC,
if you can't.
Yes, it should be GFP_KERNEL, i missed up with ZONE_NORMAL and ZONE_DMA.
The rest looks OK. Except the endianess issues. It might be no
issue on the hardware this runs on, but in favour of clean code
you might use leXX_to_cpu and friends anyway. :)
This kind of bugs is done so often, even in places where it _does_
matter. So, at least for the human reader, the leXX_to_cpu stuff
says that you really understood what you were doing when writing
the code. The current code says (to me), that it works by
accident, somehow, although it seems you knew what you were doing. :)
As you know, this driver is original developed by Luke Yang. Now I am
maintaining it.
I just don't understand in this driver where should use leXX_to_cpu()
and where should
use cpu_to_leXX(). please give me some comments about the following
change:
---
@@ -483,9 +487,12 @@
void setup_mac_addr(u8 * mac_addr)
{
+ u32 addr_low = le32_to_cpu(*(u32 *) mac_addr[0]);
+ u16 addr_hi = le16_to_cpu(*(u16 *) mac_addr[4]);
+
/* this depends on a little-endian machine */
- bfin_write_EMAC_ADDRLO(*(u32 *) mac_addr[0]);
- bfin_write_EMAC_ADDRHI(*(u16 *) mac_addr[4]);
+ bfin_write_EMAC_ADDRLO(addr_low);
+ bfin_write_EMAC_ADDRHI(addr_hi);
}
static void adjust_tx_list(void)
@@ -866,10 +873,10 @@
int retval;
/* Grab the MAC address in the MAC */
- *(u32 *) ((dev-dev_addr[0])) = bfin_read_EMAC_ADDRLO();
- *(u16 *) ((dev-dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
+ *(u32 *) ((dev-dev_addr[0])) = cpu_to_le32(bfin_read_EMAC_ADDRLO());
+ *(u16 *) ((dev-dev_addr[4])) = cpu_to_le16((u16)
bfin_read_EMAC_ADDRHI());
---
Thanks
- Bryan Wu
-
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 try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver
On Sun, 2007-07-15 at 11:53 +0100, Christoph Hellwig wrote: On Sun, Jul 15, 2007 at 12:36:51PM +0200, Michael Buesch wrote: On Sunday 15 July 2007 11:27:09 Bryan Wu wrote: +#if defined(CONFIG_BFIN_MAC_USE_L1) +# define bfin_mac_alloc(dma_handle, size) l1_data_sram_zalloc(size) +# define bfin_mac_free(dma_handle, ptr)l1_data_sram_free(ptr) +#else +# define bfin_mac_alloc(dma_handle, size) \ + dma_alloc_coherent(NULL, size, dma_handle, GFP_NORMAL) What is GFP_NORMAL? It's not defined in latest linus' tree. I think you should use GFP_KERNEL, if you can sleep, or GFP_ATOMIC, if you can't. Actually this whole thing looks fishy. There should be a struct device for the dma allocation, through a platform_device. And the CONFIG_BFIN_MAC_USE_L1 should go away, the l1 sram should have a dma provider so this can be handled through the dma api. We do have L1 memory DMA allocator and manager, it is depends on Blackfin arch. And this driver is Blackfin on-chip ethernet MAC controller. So it is OK, any idea? Thanks - Bryan Wu - 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 try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver
On Sunday 15 July 2007 14:07:44 Bryan Wu wrote:
@@ -483,9 +487,12 @@
void setup_mac_addr(u8 * mac_addr)
{
+ u32 addr_low = le32_to_cpu(*(u32 *) mac_addr[0]);
+ u16 addr_hi = le16_to_cpu(*(u16 *) mac_addr[4]);
+
/* this depends on a little-endian machine */
- bfin_write_EMAC_ADDRLO(*(u32 *) mac_addr[0]);
- bfin_write_EMAC_ADDRHI(*(u16 *) mac_addr[4]);
+ bfin_write_EMAC_ADDRLO(addr_low);
+ bfin_write_EMAC_ADDRHI(addr_hi);
}
static void adjust_tx_list(void)
@@ -866,10 +873,10 @@
int retval;
/* Grab the MAC address in the MAC */
- *(u32 *) ((dev-dev_addr[0])) = bfin_read_EMAC_ADDRLO();
- *(u16 *) ((dev-dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
+ *(u32 *) ((dev-dev_addr[0])) = cpu_to_le32(bfin_read_EMAC_ADDRLO());
+ *(u16 *) ((dev-dev_addr[4])) = cpu_to_le16((u16)
bfin_read_EMAC_ADDRHI());
Try something like this:
@@ -483,9 +487,12 @@
void setup_mac_addr(u8 * mac_addr)
{
+ u32 addr_low = le32_to_cpu(*(__le32 *) mac_addr[0]);
+ u16 addr_hi = le16_to_cpu(*(__le16 *) mac_addr[4]);
+
- /* this depends on a little-endian machine */
- bfin_write_EMAC_ADDRLO(*(u32 *) mac_addr[0]);
- bfin_write_EMAC_ADDRHI(*(u16 *) mac_addr[4]);
+ bfin_write_EMAC_ADDRLO(addr_low);
+ bfin_write_EMAC_ADDRHI(addr_hi);
}
static void adjust_tx_list(void)
@@ -866,10 +873,10 @@
int retval;
/* Grab the MAC address in the MAC */
- *(u32 *) ((dev-dev_addr[0])) = bfin_read_EMAC_ADDRLO();
- *(u16 *) ((dev-dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
+ *(__le32 *) ((dev-dev_addr[0])) =
cpu_to_le32(bfin_read_EMAC_ADDRLO());
+ *(__le16 *) ((dev-dev_addr[4])) = cpu_to_le16((u16)
bfin_read_EMAC_ADDRHI());
--
Greetings Michael.
-
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 try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver
Bryan Wu wrote:
On Sun, 2007-07-15 at 14:17 +0200, Michael Buesch wrote:
On Sunday 15 July 2007 14:07:44 Bryan Wu wrote:
@@ -483,9 +487,12 @@
void setup_mac_addr(u8 * mac_addr)
{
+ u32 addr_low = le32_to_cpu(*(u32 *) mac_addr[0]);
+ u16 addr_hi = le16_to_cpu(*(u16 *) mac_addr[4]);
+
/* this depends on a little-endian machine */
- bfin_write_EMAC_ADDRLO(*(u32 *) mac_addr[0]);
- bfin_write_EMAC_ADDRHI(*(u16 *) mac_addr[4]);
+ bfin_write_EMAC_ADDRLO(addr_low);
+ bfin_write_EMAC_ADDRHI(addr_hi);
}
static void adjust_tx_list(void)
@@ -866,10 +873,10 @@
int retval;
/* Grab the MAC address in the MAC */
- *(u32 *) ((dev-dev_addr[0])) = bfin_read_EMAC_ADDRLO();
- *(u16 *) ((dev-dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
+ *(u32 *) ((dev-dev_addr[0])) = cpu_to_le32(bfin_read_EMAC_ADDRLO());
+ *(u16 *) ((dev-dev_addr[4])) = cpu_to_le16((u16)
bfin_read_EMAC_ADDRHI());
Try something like this:
@@ -483,9 +487,12 @@
void setup_mac_addr(u8 * mac_addr)
{
+ u32 addr_low = le32_to_cpu(*(__le32 *) mac_addr[0]);
+ u16 addr_hi = le16_to_cpu(*(__le16 *) mac_addr[4]);
+
- /* this depends on a little-endian machine */
- bfin_write_EMAC_ADDRLO(*(u32 *) mac_addr[0]);
- bfin_write_EMAC_ADDRHI(*(u16 *) mac_addr[4]);
+ bfin_write_EMAC_ADDRLO(addr_low);
+ bfin_write_EMAC_ADDRHI(addr_hi);
}
static void adjust_tx_list(void)
@@ -866,10 +873,10 @@
int retval;
/* Grab the MAC address in the MAC */
- *(u32 *) ((dev-dev_addr[0])) = bfin_read_EMAC_ADDRLO();
- *(u16 *) ((dev-dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
+ *(__le32 *) ((dev-dev_addr[0])) =
cpu_to_le32(bfin_read_EMAC_ADDRLO());
+ *(__le16 *) ((dev-dev_addr[4])) = cpu_to_le16((u16)
bfin_read_EMAC_ADDRHI());
Thanks a lot, Michael.
I got a generic question about this endianess check. When should use it
in a driver or something else? I grep it in the driver/net/
---
drivers/net/e100.c: ns-tx_window_errors +=
le32_to_cpu(s-tx_late_collisions);
drivers/net/e100.c: ns-tx_carrier_errors +=
le32_to_cpu(s-tx_lost_crs);
drivers/net/e100.c: ns-tx_fifo_errors +=
le32_to_cpu(s-tx_underruns);
drivers/net/e100.c: ns-tx_errors +=
le32_to_cpu(s-tx_max_collisions) +
drivers/net/e100.c: le32_to_cpu(s-tx_lost_crs);
drivers/net/e100.c: ns-rx_length_errors +=
le32_to_cpu(s-rx_short_frame_errors) +
drivers/net/e100.c: ns-rx_crc_errors +=
le32_to_cpu(s-rx_crc_errors);
drivers/net/e100.c: ns-rx_frame_errors +=
le32_to_cpu(s-rx_alignment_errors);
drivers/net/e100.c: ns-rx_over_errors +=
le32_to_cpu(s-rx_overrun_errors);
drivers/net/e100.c: ns-rx_fifo_errors +=
le32_to_cpu(s-rx_overrun_errors);
drivers/net/e100.c: ns-rx_missed_errors +=
le32_to_cpu(s-rx_resource_errors);
drivers/net/e100.c: ns-rx_errors += le32_to_cpu(s-rx_crc_errors) +
drivers/net/e100.c: le32_to_cpu(s-rx_alignment_errors) +
drivers/net/e100.c: le32_to_cpu(s-rx_short_frame_errors) +
drivers/net/e100.c: le32_to_cpu(s-rx_cdt_errors);
drivers/net/e100.c: nic-tx_deferred += le32_to_cpu(s-tx_deferred);
drivers/net/e100.c: le32_to_cpu(s-tx_single_collisions);
drivers/net/e100.c: le32_to_cpu(s-tx_multiple_collisions);
drivers/net/e100.c: nic-tx_fc_pause +=
le32_to_cpu(s-fc_xmt_pause);
drivers/net/e100.c: nic-rx_fc_pause +=
le32_to_cpu(s-fc_rcv_pause);
drivers/net/e100.c:
le32_to_cpu(s-fc_rcv_unsupported);
drivers/net/e100.c:
le32_to_cpu(cb-u.tcb.tbd.buf_addr),
drivers/net/e100.c:
le32_to_cpu(cb-u.tcb.tbd.buf_addr),
---
Normally, it is used to protect some rx/tx status flags or dma buf addr.
Any guide line for this leXX_to_cpu usage?
It has to be used when accessing any data structure stored in RAM that
the device will access and where byte order is significant. cpu_to_le32
when writing to the RAM, le32_to_cpu when reading it. (or le16, etc. if
needed).
--
Robert Hancock Saskatoon, SK, Canada
To email, remove nospam from [EMAIL PROTECTED]
Home Page: http://www.roberthancock.com/
-
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 try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver
On Sunday 15 July 2007 16:01, Bryan Wu wrote: drivers/net/e100.c: ns-tx_window_errors += le32_to_cpu(s-tx_late_collisions); drivers/net/e100.c: ns-tx_carrier_errors += le32_to_cpu(s-tx_lost_crs); drivers/net/e100.c: ns-tx_fifo_errors += le32_to_cpu(s-tx_underruns); drivers/net/e100.c: ns-tx_errors += le32_to_cpu(s-tx_max_collisions) + drivers/net/e100.c: le32_to_cpu(s-tx_lost_crs); drivers/net/e100.c: ns-rx_length_errors += le32_to_cpu(s-rx_short_frame_errors) + drivers/net/e100.c: ns-rx_crc_errors += le32_to_cpu(s-rx_crc_errors); drivers/net/e100.c: ns-rx_frame_errors += le32_to_cpu(s-rx_alignment_errors); drivers/net/e100.c: ns-rx_over_errors += le32_to_cpu(s-rx_overrun_errors); drivers/net/e100.c: ns-rx_fifo_errors += le32_to_cpu(s-rx_overrun_errors); drivers/net/e100.c: ns-rx_missed_errors += le32_to_cpu(s-rx_resource_errors); drivers/net/e100.c: ns-rx_errors += le32_to_cpu(s-rx_crc_errors) + drivers/net/e100.c: le32_to_cpu(s-rx_alignment_errors) + drivers/net/e100.c: le32_to_cpu(s-rx_short_frame_errors) + drivers/net/e100.c: le32_to_cpu(s-rx_cdt_errors); drivers/net/e100.c: nic-tx_deferred += le32_to_cpu(s-tx_deferred); drivers/net/e100.c: le32_to_cpu(s-tx_single_collisions); drivers/net/e100.c: le32_to_cpu(s-tx_multiple_collisions); drivers/net/e100.c: nic-tx_fc_pause += le32_to_cpu(s-fc_xmt_pause); drivers/net/e100.c: nic-rx_fc_pause += le32_to_cpu(s-fc_rcv_pause); drivers/net/e100.c: le32_to_cpu(s-fc_rcv_unsupported); drivers/net/e100.c: le32_to_cpu(cb-u.tcb.tbd.buf_addr), drivers/net/e100.c: le32_to_cpu(cb-u.tcb.tbd.buf_addr), --- Normally, it is used to protect some rx/tx status flags or dma buf addr. Any guide line for this leXX_to_cpu usage? Easy. leXX_to_cpu converts a little endian value to CPU-endianess. So if your CPU is LE, this is a no-op. The cpu_to_leXX func converts from CPU-endianess to LE. Same goes for the bigendian variants of the funcs. In drivers you often have values with specific endianess (mostly LE, sometimes BE). For example when values are put by the device to DMA memory. So if the device works in LE, you must convert the value from LE to CPU endianess after reading it from DMA. I your case, however, the issue is different. You had a byte array and did pointer casting tricks on it. Pointer casting _is_ a common source for endianess issues. In general, if you try to cast a pointer, think twice about it. It's likely a bug. So you had a pointer to a byte array, which is u8*. You casted that to u16* and u32*. That's broken, because an u8 array is little endian. LE means the least significant part of the entity comes first in memory. Which is obviously always true for a byte array. So you must use leXX_to_cpu when doing this kind of pointer tricks. - 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
