Larry Finger [off-list ref] writes:

On 08/29/2015 04:18 PM, Jes.Sorensen@redhat.com wrote:

quoted

From: Jes Sorensen <redacted>

This is an alternate driver for a number of Realtek WiFi USB devices,
including RTL8723AU, RTL8188CU, RTL8188RU, RTL8191CU, and RTL8192CU.
It was written from scratch utilizing the Linux mac80211 stack.

After spending months cleaning up the vendor provided rtl8723au
driver, which comes with it's own 802.11 stack included, I decided to
rewrite this driver from the bottom up.

Many thanks to Johannes Berg for 802.11 insights and help and Larry
Finger for help with the vendor driver.

The full git log for the development of this driver can be found here:
git git://git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git
     branch rtl8723au-mac80211

This driver is still under development, but has proven to be very
stable for me. It currently supports station mode only. It has support
for OFDM and CCK rates, as well as AMPDU. It does lack certain
features found in the staging driver, such as power management and
40MHz channel support. In addition it does not support AD-HOC, AP, and
monitor mode support at this point.

The driver is known to work with the following devices:
Lenovo Yoga (rtl8723au)
TP-Link TL-WN823N (rtl8192cu)
Etekcity 6R (rtl8188cu)
Daffodil LAN03 (rtl8188cu)
Alfa AWUS036NHR (rtl8188ru)

Signed-off-by: Jes Sorensen <redacted>

I did not realize you were resubmitting this driver so soon. I have
been accumulating some patches that I was going to send to you in the
next couple of days. The most important of these are described inline
below.

Thanks - I already ran it through checkpatch, there is nothing from
checkpatch that needs to be resolved.

quoted

+
+static int rtl8xxxu_debug = 0;

Checkpatch reports:

ERROR: do not initialise statics to 0 or NULL
#106: FILE: drivers/net/wireless/rtl8xxxu.c:45:
+static int rtl8xxxu_debug = 0;

I really hate these pointless checkpatch warnings, but I guess I should
just post the 0 in comments to not have to waste time on people
submitting patches for this.

quoted

+	dev_info(&priv->udev->dev, "%s: dumping efuse (0x%02lx bytes):\n",
+		 __func__, sizeof(struct rtl8192cu_efuse));

On a 32-bit PowerPC, the above line outputs the following:

warning: format ‘%lx’ expects argument of type ‘long unsigned int’,
but argument 4 has type ‘unsigned int’ [-Wformat]

This issue does not affect the object code, but it should be
fixed. Setting the format specifier to %02x and adding a cast of (int)
before the "size_of" will fix it on both sets of systems.

Ewww, I didn't realize PowerPC 32 was this ugly :( Adding (long) as a
cast would have the same effect here, but we will end up with an ugly
cast in either case.

quoted

+static void rtl8723a_phy_lc_calibrate(struct rtl8xxxu_priv *priv)
+{
+	u32 val32;
+	u32 rf_amode, rf_bmode = 0, lstf;
+
+	/* Check continuous TX and Packet TX */
+	lstf = rtl8xxxu_read32(priv, REG_OFDM1_LSTF);
+
+	if (lstf & OFDM_LSTF_MASK) {
+		/* Disable all continuous TX */
+		val32 = lstf & ~OFDM_LSTF_MASK;
+		rtl8xxxu_write32(priv, REG_OFDM1_LSTF, val32);
+
+		/* Read original RF mode Path A */
+		rf_amode = rtl8xxxu_read_rfreg(priv, RF_A, RF6052_REG_AC);

The compiler on the PPC system (V4.6.3) is not as good at determining
if a variable has been set as is V4.8.3. It generates the warning
"warning: ‘rf_amode’ may be used uninitialized in this function
[-Wuninitialized]" for the above statement. As I hate to initialize
any variable that does not need it, this one should probably be left
alone; however, you may wish to add a comment.

A comment would suffice, but the question is really whether it adds
value to the code doing so - since 99.99% of users will never see this
compiler warning. I am not opposed to zero initializing it, as I had to
do the same with rf_bmode.

quoted

+static void rtl8xxxu_tx(struct ieee80211_hw *hw,
+			struct ieee80211_tx_control *control,
+			struct sk_buff *skb)
+{
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
+	struct ieee80211_rate *tx_rate = ieee80211_get_tx_rate(hw, tx_info);
+	struct rtl8xxxu_priv *priv = hw->priv;
+	struct rtl8xxxu_tx_desc *tx_desc;
+	struct ieee80211_sta *sta = NULL;
+	struct rtl8xxxu_sta_priv *sta_priv = NULL;
+	struct device *dev = &priv->udev->dev;
+	struct urb *urb;
+	u32 queue, rate;
+	u16 pktlen = skb->len;
+	u16 seq_number;
+	u16 rate_flag = tx_info->control.rates[0].flags;
+	int ret;
+
+	if (skb_headroom(skb) < sizeof(struct rtl8xxxu_tx_desc)) {
+		dev_warn(dev,
+			 "%s: Not enough headroom (%i) for tx descriptor\n",
+			 __func__, skb_headroom(skb));
+		goto error;
+	}
+
+	if (unlikely(skb->len > (65535 - sizeof(struct rtl8xxxu_tx_desc)))) {
+		dev_warn(dev, "%s: Trying to send over-sized skb (%i)\n",
+			 __func__, skb->len);
+		goto error;
+	}
+
+	urb = usb_alloc_urb(0, GFP_KERNEL);

The above statement generated a "scheduling while atomic" splat. The
gfp_t argument needs to be GFP_KERNEL.

You are seeing scheduling while atomic in the TX path? That just seems
wrong to me - Johannes is the mac80211 TX path not meant to allow
sleeping?

I have never seen this on x86 and I have been running the driver for a
long time. It is puzzling this causes a problem on PowerPC. It there
something special in the config for it? I am inclined to say there is
something wrong with the PPC32 setup rather than with my usage of
GFP_KERNEL in the TX path.

quoted

+{USB_DEVICE_AND_INTERFACE_INFO(USB_VENDOR_ID_REALTEK, 0x817e, 0xff,

0xff, 0xff),

quoted

+	.driver_info = (unsigned long)&rtl8192cu_fops},

I have tested a device with USB ID 0x0bda (USB_VENDOR_ID_REALTEK):0x817e.

Were you happy with the results, or did it cause problems? Ie. did you
try on x86 or on PPC32?

Some comments that are not part of the review, but may be of interest
to potential users:

1. The gain settings on the RTL81{88,92}CU parts greatly reduce its
usefullnes. My Edimax 7811 can only find 1 of my 5 local APs in a
scan. It can connect to that AP, but achieves less than 10 Mbps for
both RX and TX operations. I expect to be able to discover better
initialization variables, but that may take a while.

Interesting, I am trying to use the settings coming out of the efuse,
but it is not impossible I do something wrong with them. So far I got
decent results with 8192cu devices, But my testing with those has been
limited.

2. The driver fails to produce a wireless device on the PowerPC
because the firmware ready to run bit is never set. I have not seen
any reason for this to be a big-endian issue, and it may be another
pecularity of the USB subsystem on that laptop. For example, USB
devices that are plugged in at boot time fail, even though they work
if hot plugged into a running system.

For a new driver written from scratch, it looks pretty good.

Thanks for the comments, much appreciated!

I don't think any of this is showstopper material for inclusion right
now, albeit I do want to address them.

Cheers,
Jes