Re: [PATCH] rtl8xxxu: Fix device info for RTL8192EU devices

From: Jes Sorensen <jes.sorensen@gmail.com>
To: Pascal Terjan <pterjan@google.com>,
	linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rtl8xxxu: Fix device info for RTL8192EU devices
Date: Mon, 19 Apr 2021 07:53:57 -0400	[thread overview]
Message-ID: <a69a3d60-71a0-3153-b248-dacc8b95bea8@gmail.com> (raw)
In-Reply-To: <20210323193617.3748164-1-pterjan@google.com>

On 3/23/21 3:36 PM, Pascal Terjan wrote:
> Based on 2001:3319 and 2357:0109 which I used to test the fix and
> 0bda:818b and 2357:0108 for which I found efuse dumps online.
> 
> == 2357:0109 ==
> === Before ===
> Vendor: Realtek
> Product: \x03802.11n NI
> Serial:
> === After ===
> Vendor: Realtek
> Product: 802.11n NIC
> Serial not available.
> 
> == 2001:3319 ==
> === Before ===
> Vendor: Realtek
> Product: Wireless N
> Serial: no USB Adap
> === After ===
> Vendor: Realtek
> Product: Wireless N Nano USB Adapter
> Serial not available.
> 
> Signed-off-by: Pascal Terjan <pterjan@google.com>
> ---
>  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  | 11 ++--
>  .../realtek/rtl8xxxu/rtl8xxxu_8192e.c         | 53 ++++++++++++++++---
>  2 files changed, 50 insertions(+), 14 deletions(-)

This makes sense, you may want to account for the total length of the
record though, see below.

Some cosmetic nits too.

> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> index d6d1be4169e5..acb6b0cd3667 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> @@ -853,15 +853,10 @@ struct rtl8192eu_efuse {
>  	u8 usb_optional_function;
>  	u8 res9[2];
>  	u8 mac_addr[ETH_ALEN];		/* 0xd7 */
> -	u8 res10[2];
> -	u8 vendor_name[7];
> -	u8 res11[2];
> -	u8 device_name[0x0b];		/* 0xe8 */
> -	u8 res12[2];
> -	u8 serial[0x0b];		/* 0xf5 */
> -	u8 res13[0x30];
> +	u8 device_info[80];
> +	u8 res11[3];
>  	u8 unknown[0x0d];		/* 0x130 */
> -	u8 res14[0xc3];
> +	u8 res12[0xc3];
>  };
>  
>  struct rtl8xxxu_reg8val {
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> index cfe2dfdae928..9c5fad49ed2a 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> @@ -554,9 +554,39 @@ rtl8192e_set_tx_power(struct rtl8xxxu_priv *priv, int channel, bool ht40)
>  	}
>  }
>  
> +static void rtl8192eu_log_device_info(struct rtl8xxxu_priv *priv,
> +				      char *record_name,
> +				      char **record)
> +{
> +	/* A record is [ total length | 0x03 | value ] */
> +	unsigned char l = (*record)[0];

These parenthesis make no sense.

> +
> +	/* The whole section seems to be 80 characters so a record should not
> +	 * be able to be that large.
> +	 */

Please respect the comment formatting of the driver, ie
/*
 * Foo
 */

> +	if (l > 80) {
> +		dev_warn(&priv->udev->dev,
> +			 "invalid record length %d while parsing \"%s\".\n",
> +			 l, record_name);
> +		return;
> +	}

The 80 check is only valid for the first entry, consecutive entries are
already advanced. Maybe switch it over to use an index to address into
the record keep an index and just pass in efuse->device_info instead.

> +
> +	if (l >= 2) {
> +		char value[80];
> +
> +		memcpy(value, &(*record)[2], l - 2);
> +		value[l - 2] = '\0';
> +		dev_info(&priv->udev->dev, "%s: %s\n", record_name, value);
> +		*record = *record + l;
> +	} else {
> +		dev_info(&priv->udev->dev, "%s not available.\n", record_name);
> +	}
> +}
> +
>  static int rtl8192eu_parse_efuse(struct rtl8xxxu_priv *priv)
>  {
>  	struct rtl8192eu_efuse *efuse = &priv->efuse_wifi.efuse8192eu;
> +	char *record = efuse->device_info;
>  	int i;
>  
>  	if (efuse->rtl_id != cpu_to_le16(0x8129))
> @@ -604,12 +634,23 @@ static int rtl8192eu_parse_efuse(struct rtl8xxxu_priv *priv)
>  	priv->has_xtalk = 1;
>  	priv->xtalk = priv->efuse_wifi.efuse8192eu.xtal_k & 0x3f;
>  
> -	dev_info(&priv->udev->dev, "Vendor: %.7s\n", efuse->vendor_name);
> -	dev_info(&priv->udev->dev, "Product: %.11s\n", efuse->device_name);
> -	if (memchr_inv(efuse->serial, 0xff, 11))
> -		dev_info(&priv->udev->dev, "Serial: %.11s\n", efuse->serial);
> -	else
> -		dev_info(&priv->udev->dev, "Serial not available.\n");
> +	/* device_info section seems to be laid out as records
> +	 * [ total length | 0x03 | value ] so:
> +	 * - vendor length + 2
> +	 * - 0x03
> +	 * - vendor string (not null terminated)
> +	 * - product length + 2
> +	 * - 0x03
> +	 * - product string (not null terminated)
> +	 * Then there is one or 2 0x00 on all the 4 devices I own or found
> +	 * dumped online.
> +	 * As previous version of the code handled an optional serial
> +	 * string, I now assume there may be a third record if the
> +	 * length is not 0.
> +	 */
> +	rtl8192eu_log_device_info(priv, "Vendor", &record);
> +	rtl8192eu_log_device_info(priv, "Product", &record);
> +	rtl8192eu_log_device_info(priv, "Serial", &record);
>  
>  	if (rtl8xxxu_debug & RTL8XXXU_DEBUG_EFUSE) {
>  		unsigned char *raw = priv->efuse_wifi.raw;
> 