Re: [PATCH] via-velocity fixes

[Francois Romieu]

Tejun Heo <tj@xxxxxxxxxxx> :
[...]
>  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@xxxxxxxxxxx with Cc: to jgarzik@xxxxxxxxx and
alan@xxxxxxxxxx ?

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/