| Message ID | 20210323193617.3748164-1-pterjan@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtl8xxxu: Fix device info for RTL8192EU devices | expand |
Pascal Terjan <pterjan@google.com> 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> Can someone review this, please? Patch set to Deferred. -- https://patchwork.kernel.org/project/linux-wireless/patch/20210323193617.3748164-1-pterjan@google.com/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
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; >
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; > > >
=== 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(-) 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]; + + /* The whole section seems to be 80 characters so a record should not + * be able to be that large. + */ + if (l > 80) { + dev_warn(&priv->udev->dev, + "invalid record length %d while parsing \"%s\".\n", + l, record_name); + return; + } + + 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;