Re: [PATCH] Add Realtek 8187B support

[Pavel Roskin <proski@gnu.org>]

On Tue, 2008-04-08 at 19:31 -0300, Herton Ronaldo Krzesinski wrote:

P.S. More stuff as I'm going through the patch.  Sorry, I had to post
the first reply quickly to disavow my Signed-off-by.

> Also I fixed a bug of a missing "priv->vif = vif" in rtl8187_config_interface
> that makes kernel oops in transmit code in ieee80211_generic_frame_duration
> function.

This should be a separate patch.  In the kernel, it's very important to
be able to pinpoint which change broke things.  And git is well suited
for working with multiple patches (my personal preference is StGIT,
which is not just suited for that - it's the main thing StGIT does).

> diff --git a/drivers/net/wireless/rtl8180_rtl8225.c b/drivers/net/wireless/rtl8180_rtl8225.c
> index cd22781..b4c6ae0 100644
> --- a/drivers/net/wireless/rtl8180_rtl8225.c
> +++ b/drivers/net/wireless/rtl8180_rtl8225.c
> @@ -718,10 +718,7 @@ static void rtl8225_rf_set_channel(struct ieee80211_hw *dev,
>  	struct rtl8180_priv *priv = dev->priv;
>  	int chan = ieee80211_frequency_to_channel(conf->channel->center_freq);
>  
> -	if (priv->rf->init == rtl8225_rf_init)
> -		rtl8225_rf_set_tx_power(dev, chan);
> -	else
> -		rtl8225z2_rf_set_tx_power(dev, chan);
> +	priv->rf->set_txpwr(dev, chan);

It seems to me it's unrelated to rtl8187b support.  You are doing some
unrelated refactoring here, which could be done in a separate patch.

> -	.set_chan	= rtl8225_rf_set_channel
> +	.set_chan	= rtl8225_rf_set_channel,
> +	.set_txpwr	= rtl8225_rf_set_tx_power

Same thing here.  If you need extra infrastructure, add it in one patch
and leave hardware support for another.

> +struct rtl8187b_rx_hdr {
> +	__le32 flags;
> +	__le64 mac_time;
> +	u8 noise;
> +	u8 signal;
> +	u8 agc;
> +	u8 reserved;
> +	__le32 unused;
> +} __attribute__((packed));

I know, you are copying parts from the rtl8187 header.  But looking at
the vendor driver, "reserved" and "unused" are better described.  Nobody
would guess it by looking at this header.  It would be great if you at
least give a hint that the vendor driver describes the structure of
those fields.  That should be a separate patch, of course.

> +struct rtl8187b_tx_hdr {
> +	__le32 flags;
> +	__le16 rts_duration;
> +	__le16 len;
> +	__le32 unused_1;
> +	__le16 unused_2;
> +	__le16 tx_duration;
> +	__le32 unused_3;
> +	__le32 retry;
> +	__le32 unused_4[2];
> +} __attribute__((packed));

Those are indeed "unused in" the vendor driver.

> +#define DEVICE_RTL8187	0
> +#define DEVICE_RTL8187B	1

I prefer enums here, but I don't feel strongly about it.

>  struct rtl8187_priv {
>  	/* common between rtl818x drivers */
>  	struct rtl818x_csr *map;
> @@ -76,70 +103,100 @@ struct rtl8187_priv {
>  	u32 rx_conf;
>  	u16 txpwr_base;
>  	u8 asic_rev;
> +	u8 is_rtl8187b;
> +	enum {
> +		RTL8187BvB,
> +		RTL8187BvD,
> +		RTL8187BvE
> +	} hw_rev;

Or maybe we could have one enum here, and use (hw_rev > DEVICE_RTL8187)
instead of is_rtl8187b.  Maybe there will be future revisions that
change something else.  The standard approaches are flags that are ORed
or enums that are compared.  is_something doesn't scale well.
 
> -static inline u8 rtl818x_ioread8(struct rtl8187_priv *priv, u8 *addr)
> +static inline u8 rtl818x_ioread8_idx(struct rtl8187_priv *priv,
> +				     u8 *addr, u8 idx)
>  {
>  	u8 val;
>  
>  	usb_control_msg(priv->udev, usb_rcvctrlpipe(priv->udev, 0),
>  			RTL8187_REQ_GET_REG, RTL8187_REQT_READ,
> -			(unsigned long)addr, 0, &val, sizeof(val), HZ / 2);
> +			(unsigned long)addr, idx & 0x03, &val,
> +			sizeof(val), HZ / 2);

As I said, "addr" should not be a pointer.  In fact, the fifth argument
is __u16.  There is no way a valid pointer can even be passed to
usb_control_msg() without truncating it.  Make it __u16 and remove all
casts unless they are needed and you know why.

> -static inline u16 rtl818x_ioread16(struct rtl8187_priv *priv, __le16 *addr)
> +#define rtl818x_ioread8(priv, addr) rtl818x_ioread8_idx(priv, addr, 0)

Inline functions are preferred for that.

> +static inline u16 rtl818x_ioread16_idx(struct rtl8187_priv *priv,
> +				       __le16 *addr, u8 idx)
>  {
>  	__le16 val;
>  
>  	usb_control_msg(priv->udev, usb_rcvctrlpipe(priv->udev, 0),
>  			RTL8187_REQ_GET_REG, RTL8187_REQT_READ,
> -			(unsigned long)addr, 0, &val, sizeof(val), HZ / 2);
> +			(unsigned long)addr, idx & 0x03, &val,
> +			sizeof(val), HZ / 2);

More of the same stuff.
 
>  static struct usb_device_id rtl8187_table[] __devinitdata = {
> -	/* Realtek */
> -	{USB_DEVICE(0x0bda, 0x8187)},
> +	/* Realtek 8187 */
> +	{USB_DEVICE(0x0bda, 0x8187), .driver_info = DEVICE_RTL8187},

You are mixing to things in the comment.  DEVICE_RTL8187 already
indicates that it's 8187.  Better keep Realtek devices together, and
leave the details to the code.

> -	hdr = (struct rtl8187_tx_hdr *)skb_push(skb, sizeof(*hdr));
> -	hdr->flags = cpu_to_le32(flags);
> -	hdr->len = 0;
> -	hdr->rts_duration = rts_dur;
> -	hdr->retry = cpu_to_le32(control->retry_limit << 8);
> +	if (!priv->is_rtl8187b) {

I would explore the possibility of splitting the hardware specific parts
of rtl8187_tx() into two separate functions.

> @@ -240,11 +264,25 @@ static void rtl8187_rx_cb(struct urb *urb)

And the same on the rx side.  gcc is good at inlining static functions
that are used once.

> +		rx_status.mactime = le64_to_cpu(hdr_b->mac_time);

Careful here!  mac_time is not size-aligned in rtl8187b_rx_hdr.  I would
ask our alignment gurus it it's OK.

> -static int rtl8187_init_hw(struct ieee80211_hw *dev)
> +static int rtl8187_cmd_reset(struct ieee80211_hw *dev)
>  {
>  	struct rtl8187_priv *priv = dev->priv;
>  	u8 reg;
>  	int i;

It seems to me that there is too much in common between those functions.
Please consider refactoring.  I would also suggest that you submit it
separately.

Skipping lots of stuff for a later review, as it's getting late, and the
end it far away.  I hope you got the idea.

>  	u8	reserved_19[3];
> +	__le16	INT_MIG;
> +#define RTL818X_R8187B_B	0
> +#define RTL818X_R8187B_D	1
> +#define RTL818X_R8187B_E	2

You are inconsistent here.  You were using an enum in another structure
essentially for the same thing.

-- 
Regards,
Pavel Roskin