Messages in this thread | | | Date | Sun, 12 Sep 2004 14:10:20 +0200 | From | Francois Romieu <> | Subject | Re: [PATCH] via-velocity fixes |
| |
Tejun Heo <tj@home-tj.org> : [...] > First of all, many thanks for the driver. I've encountered problems > with recent updates to the driver and fixed them and some others. I'm > also interested in improving this driver, and having the programming > manual would be very helpful. Does anybody know how to obtain the > programming manual for this chip? Is it available online somewhere?
See www.viaarena.com for a .exe driver which turns out to be a .zip.
[...] > Recent receive ring related updates to via-velocity broke the driver. > My box lost packets, recevied duplicate truncated packets and soon > locked into infinite error interrupt handling. This patch fixes > receive ring handling and some other parts of the driver. List of > fixes follow. > > Using cpu_to_le32 on OWNED_BY_NIC is wrong. It will produce > 0x10000000 on big endian machines while owner bitfield would still > evaluate to 0 or 1.
The rx_desc structure is used in the network device, not in the host computer. I see nowhere in the code where it is said to the network adapter that it should change the ordering of the bytes in its registers.
Was this change tested on a big endian machine ?
> In velocity_give_rx_desc(), there should be a wmb() between resetting > the first four bytes of rdesc0 and setting owner. As resetting the > first four bytes isn't necessary, I just removed the function and > directly set owner.
- The function keeps code factored. Feel free to modify the function but please keep the function in place. - having the lower bytes cleaned can help when something goes wrong. I would favor a removal of the bitfield and use dumb accesses to registers (as u32) like can be found in others drivers (just mho).
Though simple, the changes to velocity_init_td_ring could be commented (it takes a few seconds to realize that the settings in velocity_info_tbl() allowed the former buggy code to work as well).
Can you be convinced to post the whole thing as a serie of incremental patches on netdev@oss.sgi.com with Cc: to jgarzik@pobox.com and alan@redhat.com ?
The patch looks quite good and it clearly fixes several points but I guess that most people prefer to review small incremental changes.
-- Ueimor - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |