Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210323193617.3748164-1-pterjan@google.com> <a69a3d60-71a0-3153-b248-dacc8b95bea8@gmail.com>
In-Reply-To: <a69a3d60-71a0-3153-b248-dacc8b95bea8@gmail.com>
From:   Pascal Terjan <pterjan@google.com>
Date:   Mon, 19 Apr 2021 16:55:05 +0100
Message-ID: <CAANdO=Kn+VeC3ucyOZdctdVoVH5Oc3drLAaipzz4ShL24e3ckg@mail.gmail.com>
Subject: Re: [PATCH] rtl8xxxu: Fix device info for RTL8192EU devices
To:     Jes Sorensen <jes.sorensen@gmail.com>
Cc:     linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, 19 Apr 2021 at 12:53, Jes Sorensen <jes.sorensen@gmail.com> wrote:
>
> 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.

Thanks for the review, I'll send a v2

> > 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
>  */

I blame checkpatch telling me "WARNING: networking block comments
don't use an empty /* line, use /* Comment..." (and myself for not
checking the driver again when fixing it :) )

> > +     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;
> >
>