Re: [PATCH] Add Realtek 8187B support

[Pavel Roskin <proski@gnu.org>]

Hello!

On Tue, 2008-04-08 at 19:31 -0300, Herton Ronaldo Krzesinski wrote:
> Hi, this patch (made against wireless-testing repository) adds support for
> 8187B to the rtl8187 module. It is based on code made by Realtek in their
> open source driver, plus contains code by initial patch made by John W.
> Linville and feedback/fixes to his initial patch by Pavel Roskin (thus I'm
> adding them to Signed-offs).

I appreciate your efforts, but I have to point out some mistakes.

I have never tested your patch.  Therefore, you cannot just put my name
on it.  Also, Signed-off-by has a certain legal meaning, and should be
generally used only by authors of the code and those forwarding it:
http://kerneltrap.org/taxonomy/term/245

Patches should start with the driver name followed by the semicolon.  At
this stage you should probably be posting an RFT (request for testing).
http://linuxwireless.org/en/developers/Documentation/SubmittingPatches

The patch produces a warning on x64_64:

/home/proski/src/linux-2.6/drivers/net/wireless/rtl8187_dev.c: In
function 'rtl8187b_init_hw':
/home/proski/src/linux-2.6/drivers/net/wireless/rtl8187_dev.c:586:
warning: cast to pointer from integer of different size

I see you introduce function rtl818x_ioread8_idx(), which takes pointer
to u8 as the second argument, but you cast u16 values to (u8*) in so
many places, that I wonder if that function should take u16 instead.

checkpatch.pl complains about "line over 80 characters" in many places.
I know that many developers don't like this limitation, but it's trivial
to fix in your case.  Thus, checkpatch.pl is only showing that you
didn't run it :-)

Then there are 2 sparse warnings introduced by your patch:

/home/proski/src/linux-2.6/drivers/net/wireless/rtl8187_rtl8225.c:575:6:
warning: symbol 'rtl8225z2_b_rf_set_tx_power' was not declared. Should
it be static?
/home/proski/src/linux-2.6/drivers/net/wireless/rtl8187_rtl8225.c:827:6:
warning: symbol 'rtl8225z2_b_rf_init' was not declared. Should it be
static?

Finally, I tried your patch on my hardware (Trendnet TEW-424UB, USB ID
0bda:8189).  The driver loads and works:

phy2: Selected rate control algorithm 'pid'
phy2: hwaddr 00:14:d1:45:a9:0b, RTL8187BvE V0 + rtl8225z2
usbcore: registered new interface driver rtl8187

But bringing the interface up is very slow - it takes whole 28 seconds:

# time ifconfig wlan1 up

real    0m28.354s
user    0m0.000s
sys     0m0.140s

And the connection is rather bad.  I'm connecting to an AP in the next
room (one wall and 5 meters distance at most).  It starts rather fine at
1Mbps, but then it goes quickly to 54Mbsp, and I get 41% packet loss.
The rate never goes down.  The rate control algorithm is pid.

-- 
Regards,
Pavel Roskin