[7/8] ath10k-ct: Support qcom,ath10k-calibration-data-base64

See the patch notes about the stock firmware for TP-Link Onhub and
https://chromium-review.googlesource.com/243115.

As noted there, the production firmware for Google OnHub devices only
provide the *-base64 Device Tree property, and so either the kernel or
some user space mechanism needs to know how to parse/convert this
property.

I haven't submitted this patch upstream. However, it applies relatively
cleanly to the tree even after almost 8 years, so it doesn't seem too
hard to maintain.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---

 .../970-ath10k-calibration-base64.patch       | 249 ++++++++++++++++++
 1 file changed, 249 insertions(+)
 create mode 100644 package/kernel/ath10k-ct/patches/970-ath10k-calibration-base64.patch

On Mon, Jan 02, 2023 at 03:25:33PM -0800, Brian Norris wrote:
> See the patch notes about the stock firmware for TP-Link Onhub and
> https://chromium-review.googlesource.com/243115.
> 
> As noted there, the production firmware for Google OnHub devices only
> provide the *-base64 Device Tree property, and so either the kernel or
> some user space mechanism needs to know how to parse/convert this
> property.
> 
> I haven't submitted this patch upstream. However, it applies relatively
> cleanly to the tree even after almost 8 years, so it doesn't seem too
> hard to maintain.

It's an additional patch to refresh. Also this is only for ct but should
also be put for the normal ath10k driver.

Aside from this, I notice this is actually NOT used in the 2 device you
are proposing. I'm totally missing something or this is not needed at
all?

Also on top of that the besto solution for these special case is to
parse the base64 data in userspace a produce a calibration bin to pass
with sysfs. A patch and some code to decode base64 seems overkill to me.

> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> 
>  .../970-ath10k-calibration-base64.patch       | 249 ++++++++++++++++++
>  1 file changed, 249 insertions(+)
>  create mode 100644 package/kernel/ath10k-ct/patches/970-ath10k-calibration-base64.patch
> 
> diff --git a/package/kernel/ath10k-ct/patches/970-ath10k-calibration-base64.patch b/package/kernel/ath10k-ct/patches/970-ath10k-calibration-base64.patch
> new file mode 100644
> index 000000000000..739bdee9ccf4
> --- /dev/null
> +++ b/package/kernel/ath10k-ct/patches/970-ath10k-calibration-base64.patch
> @@ -0,0 +1,249 @@
> +Adapted from:
> +https://chromium-review.googlesource.com/307062
> +
> +This "hack" remained in the production kernel, and so the production firmware
> +for Google OnHub still only knows how to patch the *-base64 Device Tree
> +property.
> +
> +CHROMIUM: HACK: ath10k: read calibration data in base64 format from DT
> +
> + Chrome OS firmware doesn't support binary format in VPD currently.
> + As a workaround, the firmware stores the calibration data in base64
> + format in the same node with the different name:
> +
> +   qcom,ath10k-calibration-data-base64 = [ 01 02 03 ... ];
> +
> +   Since the original property "qcom,ath10k-calibration-data" is always
> +   looked for first, it should have an invalid size (e.g. 1).
> +
> +   BUG=chrome-os-partner:35262
> +   TEST=build/boot on storm suceeded. Setup Storm board as AP using
> +   hostapd and
> +   connected to the board using another device. Device was able to
> +   connect to
> +   the internet and load multiple websites.
> +
> +   Change-Id: I95675a803fad3b94977ecd0977bd9980779ad7e9
> +   Signed-off-by: Toshi Kikuchi <toshik@chromium.org>
> +   Reviewed-on: https://chromium-review.googlesource.com/243115
> +   Reviewed-by: Grant Grundler <grundler@chromium.org>
> +
> +Change-Id: I17874f0ed03e28d279b08fe70aca70af57c90bda
> +Signed-off-by: Anilkumar Kolli <akolli@codeaurora.org>
> +Reviewed-on: https://chromium-review.googlesource.com/307062
> +Commit-Ready: Grant Grundler <grundler@chromium.org>
> +Tested-by: Grant Grundler <grundler@chromium.org>
> +Reviewed-by: Srinivasa duvvuri <sduvvuri@chromium.org>
> +Reviewed-by: Grant Grundler <grundler@chromium.org>
> +--- a/ath10k-5.15/Makefile
> ++++ b/ath10k-5.15/Makefile
> +@@ -4,6 +4,7 @@ ath10k_core-y += mac.o \
> + 		 debug.o \
> + 		 core.o \
> + 		 coredump.o \
> ++		 decode64.o \
> + 		 htc.o \
> + 		 htt.o \
> + 		 htt_rx.o \
> +--- a/ath10k-5.15/core.c
> ++++ b/ath10k-5.15/core.c
> +@@ -18,6 +18,7 @@
> + #include <linux/ctype.h>
> + 
> + #include "core.h"
> ++#include "decode64.h"
> + #include "mac.h"
> + #include "htc.h"
> + #include "hif.h"
> +@@ -2167,6 +2168,73 @@ static int ath10k_download_cal_file(stru
> + 	return 0;
> + }
> + 
> ++static int ath10k_download_cal_dt_base64(struct ath10k *ar)
> ++{
> ++	struct device_node *node;
> ++	int data_len;
> ++	void *data;
> ++	int ret;
> ++
> ++	node = ar->dev->of_node;
> ++	if (!node)
> ++		/* Device Tree is optional, don't print any warnings if
> ++		 * there's no node for ath10k.
> ++		 */
> ++		return -ENOENT;
> ++
> ++	if (!of_get_property(node, "qcom,ath10k-calibration-data-base64",
> ++			     &data_len)) {
> ++		/* The calibration data node is optional */
> ++		return -ENOENT;
> ++	}
> ++
> ++	data = kmalloc(data_len, GFP_KERNEL);
> ++	if (!data) {
> ++		ret = -ENOMEM;
> ++		goto out;
> ++	}
> ++
> ++	ret = of_property_read_u8_array(node,
> ++					"qcom,ath10k-calibration-data-base64",
> ++					data, data_len);
> ++	if (ret) {
> ++		ath10k_warn(ar,
> ++		    "failed to read calibration data (base64) from DT: %d\n",
> ++			    ret);
> ++		goto out_free;
> ++	}
> ++
> ++	data_len = strip_nl(data, data + data_len, data);
> ++	data_len = decode64(data, data + data_len, data);
> ++	if (data_len < 0) {
> ++		ath10k_warn(ar,
> ++			    "base64 decoder found invalid input\n");
> ++		ret = -EINVAL;
> ++		goto out_free;
> ++	}
> ++
> ++	if (data_len != ar->hw_params.cal_data_len) {
> ++		ath10k_warn(ar, "invalid calibration data length in DT: %d\n",
> ++			    data_len);
> ++		ret = -EMSGSIZE;
> ++		goto out_free;
> ++	}
> ++
> ++	ret = ath10k_download_board_data(ar, data, data_len);
> ++	if (ret) {
> ++		ath10k_warn(ar, "failed to download base64 calibration data from Device Tree: %d\n",
> ++			    ret);
> ++		goto out_free;
> ++	}
> ++
> ++	ret = 0;
> ++out_free:
> ++	kfree(data);
> ++
> ++out:
> ++	return ret;
> ++}
> ++
> + static int ath10k_download_cal_dt(struct ath10k *ar, const char *dt_name)
> + {
> + 	struct device_node *node;
> +@@ -2787,6 +2855,12 @@ static int ath10k_download_cal_data(stru
> + 		   "boot did not find target EEPROM entry, try OTP next: %d\n",
> + 		   ret);
> + 
> ++	ret = ath10k_download_cal_dt_base64(ar);
> ++	if (ret == 0) {
> ++		ar->cal_mode = ATH10K_CAL_MODE_DT;
> ++		goto done;
> ++	}
> ++
> + 	ret = ath10k_download_and_run_otp(ar);
> + 	if (ret) {
> + 		ath10k_err(ar, "failed to run otp: %d (download-cal-data)\n", ret);
> +--- /dev/null
> ++++ b/ath10k-5.15/decode64.c
> +@@ -0,0 +1,89 @@
> ++/*
> ++ * Copyright (c) 2013 The Linux Foundation. All rights reserved.
> ++ * Copyright (c) 2014, The Chromium OS Authors
> ++ *
> ++ * This program is free software; you can redistribute it and/or
> ++ * modify it under the terms of the GNU General Public License as
> ++ * published by the Free Software Foundation; either version 2 of
> ++ * the License, or (at your option) any later version.
> ++ *
> ++ * This program is distributed in the hope that it will be useful,
> ++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> ++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> ++ * GNU General Public License for more details.
> ++ *
> ++ * You should have received a copy of the GNU General Public License
> ++ * along with this program; if not, write to the Free Software
> ++ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> ++ * MA 02111-1307 USA
> ++ */
> ++
> ++#include "decode64.h"
> ++
> ++static char revkey[128] = {
> ++	-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
> ++	-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
> ++	-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 62, -1, -1, -1, 63,
> ++	52, 53, 54, 55, 56, 57, 58, 59, 60, 61, -1, -1, -1, -1, -1, -1,
> ++	-1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14,
> ++	15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, -1, -1, -1, -1, -1,
> ++	-1, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40,
> ++	41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, -1, -1, -1, -1, -1,
> ++};
> ++
> ++int
> ++decode64(unsigned char *src, unsigned char *src_end, unsigned char *dst)
> ++{
> ++	unsigned char *dst_end = dst;
> ++
> ++	while (&src[3] < src_end) {
> ++		int x;
> ++		int t[4];
> ++		int i;
> ++
> ++		if (src[0] == '=' || src[1] == '=' ||
> ++		    (src[2] == '=' && src[3] != '=')) {
> ++			return -1;
> ++		}
> ++
> ++		for (i = 0; i < 4; i++) {
> ++			if (src[i] == '=') {
> ++				t[i] = 0;
> ++			} else {
> ++				if (src[i] >= 128 ||
> ++				    ((t[i] = revkey[src[i]]) < 0)) {
> ++					return -1;
> ++				}
> ++			}
> ++		}
> ++
> ++		x = (t[0] << 18) + (t[1] << 12) + (t[2] << 6) + t[3];
> ++
> ++		*dst_end++ = (x >> 16) & 0xff;
> ++		if (src[2] != '=')
> ++			*dst_end++ = (x >> 8) & 0xff;
> ++		if (src[3] != '=')
> ++			*dst_end++ = x & 0xff;
> ++
> ++		src += 4;
> ++	}
> ++
> ++	if (src != src_end)
> ++		return -1;
> ++
> ++	return dst_end - dst;
> ++}
> ++
> ++int
> ++strip_nl(unsigned char *src, unsigned char *src_end, unsigned char *dst)
> ++{
> ++	unsigned char *dst_end = dst;
> ++
> ++	while (src < src_end) {
> ++		if (*src != '\n')
> ++			*dst_end++ = *src;
> ++		src++;
> ++	}
> ++
> ++	return dst_end - dst;
> ++}
> +--- /dev/null
> ++++ b/ath10k-5.15/decode64.h
> +@@ -0,0 +1,10 @@
> ++#ifndef _DECODE64_H_
> ++#define _DECODE64_H_
> ++
> ++int decode64(unsigned char *src, unsigned char *src_end,
> ++	     unsigned char *dst);
> ++
> ++int strip_nl(unsigned char *src, unsigned char *src_end,
> ++	     unsigned char *dst);
> ++
> ++#endif /* _DECODE64_H_ */
> -- 
> 2.39.0
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

On Tue, 3 Jan 2023 at 00:30, Brian Norris <computersforpeace@gmail.com> wrote:
>
> See the patch notes about the stock firmware for TP-Link Onhub and
> https://chromium-review.googlesource.com/243115.
>
> As noted there, the production firmware for Google OnHub devices only
> provide the *-base64 Device Tree property, and so either the kernel or
> some user space mechanism needs to know how to parse/convert this
> property.
>
> I haven't submitted this patch upstream. However, it applies relatively
> cleanly to the tree even after almost 8 years, so it doesn't seem too
> hard to maintain.

This should be sent upstream, otherwise its gonna be left downstream forever.

Regards,
Robert
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>
>  .../970-ath10k-calibration-base64.patch       | 249 ++++++++++++++++++
>  1 file changed, 249 insertions(+)
>  create mode 100644 package/kernel/ath10k-ct/patches/970-ath10k-calibration-base64.patch
>
> diff --git a/package/kernel/ath10k-ct/patches/970-ath10k-calibration-base64.patch b/package/kernel/ath10k-ct/patches/970-ath10k-calibration-base64.patch
> new file mode 100644
> index 000000000000..739bdee9ccf4
> --- /dev/null
> +++ b/package/kernel/ath10k-ct/patches/970-ath10k-calibration-base64.patch
> @@ -0,0 +1,249 @@
> +Adapted from:
> +https://chromium-review.googlesource.com/307062
> +
> +This "hack" remained in the production kernel, and so the production firmware
> +for Google OnHub still only knows how to patch the *-base64 Device Tree
> +property.
> +
> +CHROMIUM: HACK: ath10k: read calibration data in base64 format from DT
> +
> + Chrome OS firmware doesn't support binary format in VPD currently.
> + As a workaround, the firmware stores the calibration data in base64
> + format in the same node with the different name:
> +
> +   qcom,ath10k-calibration-data-base64 = [ 01 02 03 ... ];
> +
> +   Since the original property "qcom,ath10k-calibration-data" is always
> +   looked for first, it should have an invalid size (e.g. 1).
> +
> +   BUG=chrome-os-partner:35262
> +   TEST=build/boot on storm suceeded. Setup Storm board as AP using
> +   hostapd and
> +   connected to the board using another device. Device was able to
> +   connect to
> +   the internet and load multiple websites.
> +
> +   Change-Id: I95675a803fad3b94977ecd0977bd9980779ad7e9
> +   Signed-off-by: Toshi Kikuchi <toshik@chromium.org>
> +   Reviewed-on: https://chromium-review.googlesource.com/243115
> +   Reviewed-by: Grant Grundler <grundler@chromium.org>
> +
> +Change-Id: I17874f0ed03e28d279b08fe70aca70af57c90bda
> +Signed-off-by: Anilkumar Kolli <akolli@codeaurora.org>
> +Reviewed-on: https://chromium-review.googlesource.com/307062
> +Commit-Ready: Grant Grundler <grundler@chromium.org>
> +Tested-by: Grant Grundler <grundler@chromium.org>
> +Reviewed-by: Srinivasa duvvuri <sduvvuri@chromium.org>
> +Reviewed-by: Grant Grundler <grundler@chromium.org>
> +--- a/ath10k-5.15/Makefile
> ++++ b/ath10k-5.15/Makefile
> +@@ -4,6 +4,7 @@ ath10k_core-y += mac.o \
> +                debug.o \
> +                core.o \
> +                coredump.o \
> ++               decode64.o \
> +                htc.o \
> +                htt.o \
> +                htt_rx.o \
> +--- a/ath10k-5.15/core.c
> ++++ b/ath10k-5.15/core.c
> +@@ -18,6 +18,7 @@
> + #include <linux/ctype.h>
> +
> + #include "core.h"
> ++#include "decode64.h"
> + #include "mac.h"
> + #include "htc.h"
> + #include "hif.h"
> +@@ -2167,6 +2168,73 @@ static int ath10k_download_cal_file(stru
> +       return 0;
> + }
> +
> ++static int ath10k_download_cal_dt_base64(struct ath10k *ar)
> ++{
> ++      struct device_node *node;
> ++      int data_len;
> ++      void *data;
> ++      int ret;
> ++
> ++      node = ar->dev->of_node;
> ++      if (!node)
> ++              /* Device Tree is optional, don't print any warnings if
> ++               * there's no node for ath10k.
> ++               */
> ++              return -ENOENT;
> ++
> ++      if (!of_get_property(node, "qcom,ath10k-calibration-data-base64",
> ++                           &data_len)) {
> ++              /* The calibration data node is optional */
> ++              return -ENOENT;
> ++      }
> ++
> ++      data = kmalloc(data_len, GFP_KERNEL);
> ++      if (!data) {
> ++              ret = -ENOMEM;
> ++              goto out;
> ++      }
> ++
> ++      ret = of_property_read_u8_array(node,
> ++                                      "qcom,ath10k-calibration-data-base64",
> ++                                      data, data_len);
> ++      if (ret) {
> ++              ath10k_warn(ar,
> ++                  "failed to read calibration data (base64) from DT: %d\n",
> ++                          ret);
> ++              goto out_free;
> ++      }
> ++
> ++      data_len = strip_nl(data, data + data_len, data);
> ++      data_len = decode64(data, data + data_len, data);
> ++      if (data_len < 0) {
> ++              ath10k_warn(ar,
> ++                          "base64 decoder found invalid input\n");
> ++              ret = -EINVAL;
> ++              goto out_free;
> ++      }
> ++
> ++      if (data_len != ar->hw_params.cal_data_len) {
> ++              ath10k_warn(ar, "invalid calibration data length in DT: %d\n",
> ++                          data_len);
> ++              ret = -EMSGSIZE;
> ++              goto out_free;
> ++      }
> ++
> ++      ret = ath10k_download_board_data(ar, data, data_len);
> ++      if (ret) {
> ++              ath10k_warn(ar, "failed to download base64 calibration data from Device Tree: %d\n",
> ++                          ret);
> ++              goto out_free;
> ++      }
> ++
> ++      ret = 0;
> ++out_free:
> ++      kfree(data);
> ++
> ++out:
> ++      return ret;
> ++}
> ++
> + static int ath10k_download_cal_dt(struct ath10k *ar, const char *dt_name)
> + {
> +       struct device_node *node;
> +@@ -2787,6 +2855,12 @@ static int ath10k_download_cal_data(stru
> +                  "boot did not find target EEPROM entry, try OTP next: %d\n",
> +                  ret);
> +
> ++      ret = ath10k_download_cal_dt_base64(ar);
> ++      if (ret == 0) {
> ++              ar->cal_mode = ATH10K_CAL_MODE_DT;
> ++              goto done;
> ++      }
> ++
> +       ret = ath10k_download_and_run_otp(ar);
> +       if (ret) {
> +               ath10k_err(ar, "failed to run otp: %d (download-cal-data)\n", ret);
> +--- /dev/null
> ++++ b/ath10k-5.15/decode64.c
> +@@ -0,0 +1,89 @@
> ++/*
> ++ * Copyright (c) 2013 The Linux Foundation. All rights reserved.
> ++ * Copyright (c) 2014, The Chromium OS Authors
> ++ *
> ++ * This program is free software; you can redistribute it and/or
> ++ * modify it under the terms of the GNU General Public License as
> ++ * published by the Free Software Foundation; either version 2 of
> ++ * the License, or (at your option) any later version.
> ++ *
> ++ * This program is distributed in the hope that it will be useful,
> ++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> ++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> ++ * GNU General Public License for more details.
> ++ *
> ++ * You should have received a copy of the GNU General Public License
> ++ * along with this program; if not, write to the Free Software
> ++ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> ++ * MA 02111-1307 USA
> ++ */
> ++
> ++#include "decode64.h"
> ++
> ++static char revkey[128] = {
> ++      -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
> ++      -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
> ++      -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 62, -1, -1, -1, 63,
> ++      52, 53, 54, 55, 56, 57, 58, 59, 60, 61, -1, -1, -1, -1, -1, -1,
> ++      -1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14,
> ++      15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, -1, -1, -1, -1, -1,
> ++      -1, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40,
> ++      41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, -1, -1, -1, -1, -1,
> ++};
> ++
> ++int
> ++decode64(unsigned char *src, unsigned char *src_end, unsigned char *dst)
> ++{
> ++      unsigned char *dst_end = dst;
> ++
> ++      while (&src[3] < src_end) {
> ++              int x;
> ++              int t[4];
> ++              int i;
> ++
> ++              if (src[0] == '=' || src[1] == '=' ||
> ++                  (src[2] == '=' && src[3] != '=')) {
> ++                      return -1;
> ++              }
> ++
> ++              for (i = 0; i < 4; i++) {
> ++                      if (src[i] == '=') {
> ++                              t[i] = 0;
> ++                      } else {
> ++                              if (src[i] >= 128 ||
> ++                                  ((t[i] = revkey[src[i]]) < 0)) {
> ++                                      return -1;
> ++                              }
> ++                      }
> ++              }
> ++
> ++              x = (t[0] << 18) + (t[1] << 12) + (t[2] << 6) + t[3];
> ++
> ++              *dst_end++ = (x >> 16) & 0xff;
> ++              if (src[2] != '=')
> ++                      *dst_end++ = (x >> 8) & 0xff;
> ++              if (src[3] != '=')
> ++                      *dst_end++ = x & 0xff;
> ++
> ++              src += 4;
> ++      }
> ++
> ++      if (src != src_end)
> ++              return -1;
> ++
> ++      return dst_end - dst;
> ++}
> ++
> ++int
> ++strip_nl(unsigned char *src, unsigned char *src_end, unsigned char *dst)
> ++{
> ++      unsigned char *dst_end = dst;
> ++
> ++      while (src < src_end) {
> ++              if (*src != '\n')
> ++                      *dst_end++ = *src;
> ++              src++;
> ++      }
> ++
> ++      return dst_end - dst;
> ++}
> +--- /dev/null
> ++++ b/ath10k-5.15/decode64.h
> +@@ -0,0 +1,10 @@
> ++#ifndef _DECODE64_H_
> ++#define _DECODE64_H_
> ++
> ++int decode64(unsigned char *src, unsigned char *src_end,
> ++           unsigned char *dst);
> ++
> ++int strip_nl(unsigned char *src, unsigned char *src_end,
> ++           unsigned char *dst);
> ++
> ++#endif /* _DECODE64_H_ */
> --
> 2.39.0
>
>
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Hi Christian,

On Wed, Jan 04, 2023 at 01:58:01PM +0100, Christian Marangi wrote:
> On Mon, Jan 02, 2023 at 03:25:33PM -0800, Brian Norris wrote:
> > See the patch notes about the stock firmware for TP-Link Onhub and
> > https://chromium-review.googlesource.com/243115.
> > 
> > As noted there, the production firmware for Google OnHub devices only
> > provide the *-base64 Device Tree property, and so either the kernel or
> > some user space mechanism needs to know how to parse/convert this
> > property.
...
>
> Aside from this, I notice this is actually NOT used in the 2 device you
> are proposing. I'm totally missing something or this is not needed at
> all?

It's provided by the production firmware, which patches it into the
device tree on its own. So you're not going to find it in the kernel or
DTS sources.

If you want to look at its source though, it's here:
https://chromium.googlesource.com/chromiumos/platform/depthcharge/+/refs/heads/firmware-storm-6315.B/src/board/storm/wifi_calibration.c

And unfortunately, systems I'm looking at were only programmed with the
base64 VPD:

    # ls -1 /sys/firmware/vpd/ro/wifi*
    /sys/firmware/vpd/ro/wifi_base64_calibration0
    /sys/firmware/vpd/ro/wifi_base64_calibration1
    /sys/firmware/vpd/ro/wifi_base64_calibration2

> Also on top of that the besto solution for these special case is to
> parse the base64 data in userspace a produce a calibration bin to pass
> with sysfs. A patch and some code to decode base64 seems overkill to me.

I'd love something that's more pleasant than in-kernel base64. Is there
some sysfs method for this that I'm not aware of? The closest I find is:

    /sys/kernel/debug/ieee80211/*/ath10k/cal_data

But that's read-only, not read-write. And it's not completely obvious to
me that this data can be (re)written to the target radio arbitrarily, so
I suppose the API would be a bit fiddly -- that one has to know to write
this file before ever bringing up the interface (but after loading the
driver/module).

Without a user space API, this is the best I came up with.

Brian

On Thu, 5 Jan 2023 at 19:47, Brian Norris <computersforpeace@gmail.com> wrote:
>
> Hi Christian,
>
> On Wed, Jan 04, 2023 at 01:58:01PM +0100, Christian Marangi wrote:
> > On Mon, Jan 02, 2023 at 03:25:33PM -0800, Brian Norris wrote:
> > > See the patch notes about the stock firmware for TP-Link Onhub and
> > > https://chromium-review.googlesource.com/243115.
> > >
> > > As noted there, the production firmware for Google OnHub devices only
> > > provide the *-base64 Device Tree property, and so either the kernel or
> > > some user space mechanism needs to know how to parse/convert this
> > > property.
> ...
> >
> > Aside from this, I notice this is actually NOT used in the 2 device you
> > are proposing. I'm totally missing something or this is not needed at
> > all?
>
> It's provided by the production firmware, which patches it into the
> device tree on its own. So you're not going to find it in the kernel or
> DTS sources.
>
> If you want to look at its source though, it's here:
> https://chromium.googlesource.com/chromiumos/platform/depthcharge/+/refs/heads/firmware-storm-6315.B/src/board/storm/wifi_calibration.c
>
> And unfortunately, systems I'm looking at were only programmed with the
> base64 VPD:
>
>     # ls -1 /sys/firmware/vpd/ro/wifi*
>     /sys/firmware/vpd/ro/wifi_base64_calibration0
>     /sys/firmware/vpd/ro/wifi_base64_calibration1
>     /sys/firmware/vpd/ro/wifi_base64_calibration2
>
> > Also on top of that the besto solution for these special case is to
> > parse the base64 data in userspace a produce a calibration bin to pass
> > with sysfs. A patch and some code to decode base64 seems overkill to me.
>
> I'd love something that's more pleasant than in-kernel base64. Is there
> some sysfs method for this that I'm not aware of? The closest I find is:
>
>     /sys/kernel/debug/ieee80211/*/ath10k/cal_data
>
> But that's read-only, not read-write. And it's not completely obvious to
> me that this data can be (re)written to the target radio arbitrarily, so
> I suppose the API would be a bit fiddly -- that one has to know to write
> this file before ever bringing up the interface (but after loading the
> driver/module).
>
> Without a user space API, this is the best I came up with.

You can extract and provide the caldata from userspace by acting on the hotplug
event that kernel files after request_firmware() fails, look at what
Fritzbox devices are doing in:
https://github.com/openwrt/openwrt/blob/master/target/linux/ipq40xx/base-files/etc/hotplug.d/firmware/11-ath10k-caldata

Basically, you trigger on the requested file and device and then you
can just use a userspace
script or binary to actually provide that file.
That would probably work best here as I dont know if upstream will
accept adding a base64
method just for one or 2 devices.

Regards,
Robert
>
> Brian
>
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

On Thu, Jan 5, 2023 at 11:02 AM Robert Marko <robimarko@gmail.com> wrote:
> On Thu, 5 Jan 2023 at 19:47, Brian Norris <computersforpeace@gmail.com> wrote:
> > On Wed, Jan 04, 2023 at 01:58:01PM +0100, Christian Marangi wrote:
> > > Also on top of that the besto solution for these special case is to
> > > parse the base64 data in userspace a produce a calibration bin to pass
> > > with sysfs. A patch and some code to decode base64 seems overkill to me.
> >
> > I'd love something that's more pleasant than in-kernel base64. Is there
> > some sysfs method for this that I'm not aware of? The closest I find is:
> >
> >     /sys/kernel/debug/ieee80211/*/ath10k/cal_data
> >
> > But that's read-only, not read-write. And it's not completely obvious to
> > me that this data can be (re)written to the target radio arbitrarily, so
> > I suppose the API would be a bit fiddly -- that one has to know to write
> > this file before ever bringing up the interface (but after loading the
> > driver/module).
> >
> > Without a user space API, this is the best I came up with.
>
> You can extract and provide the caldata from userspace by acting on the hotplug
> event that kernel files after request_firmware() fails, look at what
> Fritzbox devices are doing in:
> https://github.com/openwrt/openwrt/blob/master/target/linux/ipq40xx/base-files/etc/hotplug.d/firmware/11-ath10k-caldata

Awesome, I had an inkling that other devices would probably have some
similar problems, but I didn't find exactly how they're handling it.

And wow, I didn't realize there was a uevent/sysfs method for filling
in the gaps on missing /lib/firmware files. TIL.

> Basically, you trigger on the requested file and device and then you
> can just use a userspace
> script or binary to actually provide that file.

Yep, this totally looks like it'll work for me. I'll just need to
force a 'base64' utility into these images, but otherwise, looks
pretty easy.

> That would probably work best here as I dont know if upstream will
> accept adding a base64
> method just for one or 2 devices.

Yeah, I figured this is arcane enough (and not really a typical job
for DT bindings) that we'd need to patch up something ourselves. I'm
glad to not have to approach the "convince upstream" question on this
ugly patch, and even more glad to find a way do this without modifying
the kernel/driver.

Thanks a lot,
Brian

In case you are open to giving more helpful tips to a relative
newcomer to openwrt development:

On Thu, Jan 5, 2023 at 11:59 AM Brian Norris
<computersforpeace@gmail.com> wrote:
> I'll just need to
> force a 'base64' utility into these images

This is turning out to be nontrivial. The only in-tree base64 tool is
a busybox tool, and it's not enabled in the default busybox
configuration. I don't see an easy way for a target to change this
default. I *do* see package dependencies that do this (like
DEPENDS:=+@BUSYBOX_CONFIG_<feature>), but I don't think that works in
a target (e.g., adding to DEVICE_PACKAGES).

Do I need to create some intermediate/stub package just to express the
dependency, or is there some better way?

Brian

On Thu, Jan 05, 2023 at 02:03:24PM -0800, Brian Norris wrote:
> In case you are open to giving more helpful tips to a relative
> newcomer to openwrt development:
> 
> On Thu, Jan 5, 2023 at 11:59 AM Brian Norris
> <computersforpeace@gmail.com> wrote:
> > I'll just need to
> > force a 'base64' utility into these images
> 
> This is turning out to be nontrivial. The only in-tree base64 tool is
> a busybox tool, and it's not enabled in the default busybox
> configuration. I don't see an easy way for a target to change this
> default. I *do* see package dependencies that do this (like
> DEPENDS:=+@BUSYBOX_CONFIG_<feature>), but I don't think that works in
> a target (e.g., adding to DEVICE_PACKAGES).
> 
> Do I need to create some intermediate/stub package just to express the
> dependency, or is there some better way?
> 

In theory there is [1] coreutils-base64 as a separate package but I
think we have to move that to core packages as it's part of the feeds
package.

Just for testing can you test if that package suits your need for a
userspace solution?

[1] https://openwrt.org/packages/pkgdata/coreutils-base64

Hi

On 2023-01-06, Christian Marangi wrote:
> On Thu, Jan 05, 2023 at 02:03:24PM -0800, Brian Norris wrote:
> > On Thu, Jan 5, 2023 at 11:59 AM Brian Norris
[...]
> > Do I need to create some intermediate/stub package just to express the
> > dependency, or is there some better way?
> >
>
> In theory there is [1] coreutils-base64 as a separate package but I
> think we have to move that to core packages as it's part of the feeds
> package.

There would be another alternative for base64 functionality, openssl...
While not the lightest package, it's already available in main and
with DEVICE_PACKAGES a relatively low-maintenance alternative.

openssl enc -base64 -in foo -out foo.base64
openssl enc -d -base64 -in foo.base64 -out foo

With root/ overlay on eMMC, this shouldn't be too heavy for this
kind of hardware.

Disclaimer: I did not check wolfssl/ mbedtls for potential
alternatives.

Regards
	Stefan Lippers-Hollmann

On Thu, Jan 5, 2023 at 7:12 PM Stefan Lippers-Hollmann <s.l-h@gmx.de> wrote:
> On 2023-01-06, Christian Marangi wrote:
> > On Thu, Jan 05, 2023 at 02:03:24PM -0800, Brian Norris wrote:
> > > On Thu, Jan 5, 2023 at 11:59 AM Brian Norris
> [...]
> > > Do I need to create some intermediate/stub package just to express the
> > > dependency, or is there some better way?
> > >
> >
> > In theory there is [1] coreutils-base64 as a separate package but I
> > think we have to move that to core packages as it's part of the feeds
> > package.

coreutils definitely works. (busybox would too, except I just can't
get the dependency mechanics right.)

> There would be another alternative for base64 functionality, openssl...
> While not the lightest package, it's already available in main and
> with DEVICE_PACKAGES a relatively low-maintenance alternative.
>
> openssl enc -base64 -in foo -out foo.base64
> openssl enc -d -base64 -in foo.base64 -out foo

I have to throw "-A" in there apparently, since openssl is apparently
too strict on (lack of) line wrapping in the data source, but that
works too.

> With root/ overlay on eMMC, this shouldn't be too heavy for this
> kind of hardware.

Yeah, should be no big deal on space. These devices have ~4GB storage
[1]. Does anybody care on non-disk-space grounds? Like "lack of
choice"? (I guess one can install multiple SSL libraries.)

So what do you all think? Pick my favorite? I suppose I kinda like the
'base64' (either coreutils or busybox) option, since it's a bit of a
simpler tool, and has multiple options (in case somebody is trying to
slim things for some reason). But openssl is easiest from a friction
point of view, since it's just a DEVICE_PACKAGES dependency away,
instead of porting over something from feeds.

Brian

[1] Although the default partitioning sets
CONFIG_TARGET_ROOTFS_PARTSIZE=200, which limits the default build
unnecessarily to ~128MiB. I still need to figure out the best way to
fix that:
https://openwrt.org/toh/google/wifi#expanding_storage_optional

> Le 6 janv. 2023 à 04:48, Brian Norris <computersforpeace@gmail.com> a écrit :
> 
> On Thu, Jan 5, 2023 at 7:12 PM Stefan Lippers-Hollmann <s.l-h@gmx.de> wrote:
>> On 2023-01-06, Christian Marangi wrote:
>>> On Thu, Jan 05, 2023 at 02:03:24PM -0800, Brian Norris wrote:
>>>> On Thu, Jan 5, 2023 at 11:59 AM Brian Norris
>> [...]
>>>> Do I need to create some intermediate/stub package just to express the
>>>> dependency, or is there some better way?
>>>> 
>>> 
>>> In theory there is [1] coreutils-base64 as a separate package but I
>>> think we have to move that to core packages as it's part of the feeds
>>> package.
> 
> coreutils definitely works. (busybox would too, except I just can't
> get the dependency mechanics right.)
> 
>> There would be another alternative for base64 functionality, openssl...
>> While not the lightest package, it's already available in main and
>> with DEVICE_PACKAGES a relatively low-maintenance alternative.
>> 
>> openssl enc -base64 -in foo -out foo.base64
>> openssl enc -d -base64 -in foo.base64 -out foo
> 
> I have to throw "-A" in there apparently, since openssl is apparently
> too strict on (lack of) line wrapping in the data source, but that
> works too.
> 
>> With root/ overlay on eMMC, this shouldn't be too heavy for this
>> kind of hardware.
> 
> Yeah, should be no big deal on space. These devices have ~4GB storage
> [1]. Does anybody care on non-disk-space grounds? Like "lack of
> choice"? (I guess one can install multiple SSL libraries.)

Indeed. Coreutils seems like a better choice here.

As an aside (and documentation for similar cases with devices that have smaller storage), note that if you don’t want to store the extracted firmware to permanent storage, caldata_sysfsload_from_file() offers an alternative to this.

> [1] Although the default partitioning sets
> CONFIG_TARGET_ROOTFS_PARTSIZE=200, which limits the default build
> unnecessarily to ~128MiB. I still need to figure out the best way to
> fix that:

You can probably adjust this on your target-specific config, but the reason for keeping it small is to keep sysupgrade times reasonable, IIRC.

HTH,
T

On Fri, Jan 6, 2023 at 12:39 AM Thibaut <hacks@slashdirt.org> wrote:
> > Le 6 janv. 2023 à 04:48, Brian Norris <computersforpeace@gmail.com> a écrit :

> Indeed. Coreutils seems like a better choice here.

OK. So I guess I just copy/paste it back into the openwrt tree from
the packages feed, and then later clean it out of packages.git?

> As an aside (and documentation for similar cases with devices that have smaller storage), note that if you don’t want to store the extracted firmware to permanent storage, caldata_sysfsload_from_file() offers an alternative to this.

Thanks. I was trying to grok why there were at least two variants in
caldata.sh; I liked (and imitated) the caldata_sysfsload_from_file()
approach better.

> > [1] Although the default partitioning sets
> > CONFIG_TARGET_ROOTFS_PARTSIZE=200, which limits the default build
> > unnecessarily to ~128MiB. I still need to figure out the best way to
> > fix that:
>
> You can probably adjust this on your target-specific config,

I suppose, although that's still in the same problem space of "how to
set a CONFIG_* default from a target" -- that also can't be done in
DEVICE_PACKAGES. (And again, I'm a little new at this, so bear with
me.)

I could also just hardcode something better I guess. I see others do this:
https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/ipq40xx/image/generic.mk;h=f92e11c797127344c6e14d6292956eeca8f048eb;hb=d3e89e69c52115d1c02f5c16a4aa6b721e003578#l103
(Build/qsdk-ipq-app-gpt)

> but the reason for keeping it small is to keep sysupgrade times reasonable, IIRC.

Ah, makes sense.

BTW, speaking of sysupgrade (and maybe this deserves a change of
Subject if this discussion goes very far): I've found that the emmc.sh
upgrade helpers still don't seem very reliable when the rootfs size
changes (and so the "f2fs loop-mounted rootfs_data appended to the
rootfs" has to move). sysupgrade can be a bit hard to debug, since you
lose access to many normal services along the way, so I'm not 100%
sure, but it seems like the two filesystems (rootfs and data/overlay)
collide, and that a separate rootfs_data partition (such as the one in
the glinet_gl-b2200 target above) would work better. Am I missing
something here? It seems that sysupgrade is very ad-hoc in openwrt,
and that eMMC/block support is still somewhat underdeveloped, so maybe
there aren't really any conventions here, and one just does whatever
works for them? I'm thinking of just adding the rootfs_data partition
to suck up the remaining GBs of disk (and maybe adding to the Google
Wifi target too), but I'm still not sure of the Best solution.

As a bonus, with a rootfs_data partition, the sysupgrade could
potentially be a lot simpler (and does not need to scale with the size
of the rootfs_data contents), because the filesystem could mostly just
stay in-place.

Brian