[v2,07/35] brcmfmac: pcie: Read Apple OTP information

On Apple platforms, the One Time Programmable ROM in the Broadcom chips
contains information about the specific board design (module, vendor,
version) that is required to select the correct NVRAM file. Parse this
OTP ROM and extract the required strings.

Note that the user OTP offset/size is per-chip. This patch does not add
any chips yet.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 .../broadcom/brcm80211/brcmfmac/pcie.c        | 219 ++++++++++++++++++
 include/linux/bcma/bcma_driver_chipcommon.h   |   1 +
 2 files changed, 220 insertions(+)

On 1/4/2022 8:26 AM, Hector Martin wrote:
> On Apple platforms, the One Time Programmable ROM in the Broadcom chips
> contains information about the specific board design (module, vendor,
> version) that is required to select the correct NVRAM file. Parse this
> OTP ROM and extract the required strings.
> 
> Note that the user OTP offset/size is per-chip. This patch does not add
> any chips yet.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>   .../broadcom/brcm80211/brcmfmac/pcie.c        | 219 ++++++++++++++++++
>   include/linux/bcma/bcma_driver_chipcommon.h   |   1 +
>   2 files changed, 220 insertions(+)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index a52a6f8081eb..74c9a4f74813 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c

[...]

> +static int brcmf_pcie_read_otp(struct brcmf_pciedev_info *devinfo)
> +{
> +	const struct pci_dev *pdev = devinfo->pdev;
> +	struct brcmf_bus *bus = dev_get_drvdata(&pdev->dev);
> +	u32 coreid, base, words, idx, sromctl;
> +	u16 *otp;
> +	struct brcmf_core *core;
> +	int ret;
> +
> +	switch (devinfo->ci->chip) {
> +	default:
> +		/* OTP not supported on this chip */
> +		return 0;
> +	}

Does not seem this code is put to work yet. Will dive into it later on.

> +	core = brcmf_chip_get_core(devinfo->ci, coreid);
> +	if (!core) {
> +		brcmf_err(bus, "No OTP core\n");
> +		return -ENODEV;
> +	}
> +
> +	if (coreid == BCMA_CORE_CHIPCOMMON) {
> +		/* Chips with OTP accessed via ChipCommon need additional
> +		 * handling to access the OTP
> +		 */
> +		brcmf_pcie_select_core(devinfo, coreid);
> +		sromctl = READCC32(devinfo, sromcontrol);
> +
> +		if (!(sromctl & BCMA_CC_SROM_CONTROL_OTP_PRESENT)) {
> +			/* Chip lacks OTP, try without it... */
> +			brcmf_err(bus,
> +				  "OTP unavailable, using default firmware\n");
> +			return 0;
> +		}
> +
> +		/* Map OTP to shadow area */
> +		WRITECC32(devinfo, sromcontrol,
> +			  sromctl | BCMA_CC_SROM_CONTROL_OTPSEL);
> +	}
> +
> +	otp = kzalloc(sizeof(u16) * words, GFP_KERNEL);
> +
> +	/* Map bus window to SROM/OTP shadow area in core */
> +	base = brcmf_pcie_buscore_prep_addr(devinfo->pdev, base + core->base);

I guess this changes the bar window...

> +	brcmf_dbg(PCIE, "OTP data:\n");
> +	for (idx = 0; idx < words; idx++) {
> +		otp[idx] = brcmf_pcie_read_reg16(devinfo, base + 2 * idx);
> +		brcmf_dbg(PCIE, "[%8x] 0x%04x\n", base + 2 * idx, otp[idx]);
> +	}
> +
> +	if (coreid == BCMA_CORE_CHIPCOMMON) {
> +		brcmf_pcie_select_core(devinfo, coreid);

... which is why you need to reselect the core. Otherwise it makes no 
sense to me.

> +		WRITECC32(devinfo, sromcontrol, sromctl);
> +	}
> +
> +	ret = brcmf_pcie_parse_otp(devinfo, (u8 *)otp, 2 * words);
> +	kfree(otp);
> +
> +	return ret;
> +}

On 06/01/2022 21.37, Arend van Spriel wrote:
> On 1/4/2022 8:26 AM, Hector Martin wrote:
>> +static int brcmf_pcie_read_otp(struct brcmf_pciedev_info *devinfo)
>> +{
>> +	const struct pci_dev *pdev = devinfo->pdev;
>> +	struct brcmf_bus *bus = dev_get_drvdata(&pdev->dev);
>> +	u32 coreid, base, words, idx, sromctl;
>> +	u16 *otp;
>> +	struct brcmf_core *core;
>> +	int ret;
>> +
>> +	switch (devinfo->ci->chip) {
>> +	default:
>> +		/* OTP not supported on this chip */
>> +		return 0;
>> +	}
> 
> Does not seem this code is put to work yet. Will dive into it later on.

The specific OTP ranges and cores are added by the subsequent patches
that add support for individual chips, once all the scaffolding is in place.

> 
>> +	core = brcmf_chip_get_core(devinfo->ci, coreid);
>> +	if (!core) {
>> +		brcmf_err(bus, "No OTP core\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (coreid == BCMA_CORE_CHIPCOMMON) {
>> +		/* Chips with OTP accessed via ChipCommon need additional
>> +		 * handling to access the OTP
>> +		 */
>> +		brcmf_pcie_select_core(devinfo, coreid);
>> +		sromctl = READCC32(devinfo, sromcontrol);
>> +
>> +		if (!(sromctl & BCMA_CC_SROM_CONTROL_OTP_PRESENT)) {
>> +			/* Chip lacks OTP, try without it... */
>> +			brcmf_err(bus,
>> +				  "OTP unavailable, using default firmware\n");
>> +			return 0;
>> +		}
>> +
>> +		/* Map OTP to shadow area */
>> +		WRITECC32(devinfo, sromcontrol,
>> +			  sromctl | BCMA_CC_SROM_CONTROL_OTPSEL);
>> +	}
>> +
>> +	otp = kzalloc(sizeof(u16) * words, GFP_KERNEL);
>> +
>> +	/* Map bus window to SROM/OTP shadow area in core */
>> +	base = brcmf_pcie_buscore_prep_addr(devinfo->pdev, base + core->base);
> 
> I guess this changes the bar window...
> 
>> +	brcmf_dbg(PCIE, "OTP data:\n");
>> +	for (idx = 0; idx < words; idx++) {
>> +		otp[idx] = brcmf_pcie_read_reg16(devinfo, base + 2 * idx);
>> +		brcmf_dbg(PCIE, "[%8x] 0x%04x\n", base + 2 * idx, otp[idx]);
>> +	}
>> +
>> +	if (coreid == BCMA_CORE_CHIPCOMMON) {
>> +		brcmf_pcie_select_core(devinfo, coreid);
> 
> ... which is why you need to reselect the core. Otherwise it makes no 
> sense to me.

Yes; *technically* with the BCMA_CORE_CHIPCOMMON core the OTP is always
within the first 0x1000 and so I wouldn't have to reselect it, since
it'd end up with the same window, but that is not the case with
BCMA_CORE_GCI used on other chips (where the OTP offset is >0x1000),
although those don't hit this code path. So while this line could be
removed without causing any issues, I find it more orthogonal and safer
to keep the pattern where I select the core before accessing
core-relative fixed registers, and treat brcmf_pcie_buscore_prep_addr as
invalidating the BAR window for all intents and purposes.

On 2022/01/04 20:26, Andy Shevchenko wrote:
> On Tue, Jan 4, 2022 at 9:28 AM Hector Martin <marcan@marcan.st> wrote:
>>
>> On Apple platforms, the One Time Programmable ROM in the Broadcom chips
>> contains information about the specific board design (module, vendor,
>> version) that is required to select the correct NVRAM file. Parse this
>> OTP ROM and extract the required strings.
>>
>> Note that the user OTP offset/size is per-chip. This patch does not add
>> any chips yet.
> 
> ...
> 
>> +static int
>> +brcmf_pcie_parse_otp_sys_vendor(struct brcmf_pciedev_info *devinfo,
>> +                               u8 *data, size_t size)
>> +{
>> +       int idx = 4;
> 
> Can you rather have a structure
> 
> struct my_cool_and_strange_blob {
> __le32 hdr;
> const char ...[];
> ...
> }
> 
> and then cast your data to this struct?

That would mean I need to copy, since the original data is not aligned.
Since it's just one u32 header (which we don't even truly know the
interpretation of), it seems get_unaligned_le32 is easier.

> 
>> +       const char *chip_params;
>> +       const char *board_params;
>> +       const char *p;
>> +
>> +       /* 4-byte header and two empty strings */
>> +       if (size < 6)
>> +               return -EINVAL;
>> +
>> +       if (get_unaligned_le32(data) != BRCMF_OTP_VENDOR_HDR)
>> +               return -EINVAL;
>> +
>> +       chip_params = &data[idx];
> 
>> +       /* Skip first string, including terminator */
>> +       idx += strnlen(chip_params, size - idx) + 1;
> 
> strsep() ?

We're splitting on \0 here, so that won't work.

> 
>> +       if (idx >= size)
>> +               return -EINVAL;
>> +
>> +       board_params = &data[idx];
>> +
>> +       /* Skip to terminator of second string */
>> +       idx += strnlen(board_params, size - idx);
>> +       if (idx >= size)
>> +               return -EINVAL;
>> +
>> +       /* At this point both strings are guaranteed NUL-terminated */
>> +       brcmf_dbg(PCIE, "OTP: chip_params='%s' board_params='%s'\n",
>> +                 chip_params, board_params);
>> +
>> +       p = board_params;
>> +       while (*p) {
>> +               char tag = *p++;
>> +               const char *end;
>> +               size_t len;
>> +
>> +               if (tag == ' ') /* Skip extra spaces */
>> +                       continue;
> 
> skip_spaces()

Sure.

> 
>> +
>> +               if (*p++ != '=') /* implicit NUL check */
>> +                       return -EINVAL;
> 
> Have you checked the next_arg() implementation?

That function has a lot more logic (handling quotes, etc) and no other
hardware drivers use it. I'm not sure I feel comfortable using it to
parse untrusted data from a potentially compromised device. The parsing
we need to do here is much simpler.

> 
>> +               /* *p might be NUL here, if so end == p and len == 0 */
>> +               end = strchrnul(p, ' ');
>> +               len = end - p;
>> +
>> +               /* leave 1 byte for NUL in destination string */
>> +               if (len > (BRCMF_OTP_MAX_PARAM_LEN - 1))
>> +                       return -EINVAL;
>> +
>> +               /* Copy len characters plus a NUL terminator */
>> +               switch (tag) {
>> +               case 'M':
>> +                       strscpy(devinfo->otp.module, p, len + 1);
>> +                       break;
>> +               case 'V':
>> +                       strscpy(devinfo->otp.vendor, p, len + 1);
>> +                       break;
>> +               case 'm':
>> +                       strscpy(devinfo->otp.version, p, len + 1);
>> +                       break;
>> +               }
>> +
>> +               /* Skip to space separator or NUL */
>> +               p = end;
>> +       }
>> +
>> +       brcmf_dbg(PCIE, "OTP: module=%s vendor=%s version=%s\n",
>> +                 devinfo->otp.module, devinfo->otp.vendor,
>> +                 devinfo->otp.version);
>> +
>> +       if (!devinfo->otp.module ||
>> +           !devinfo->otp.vendor ||
>> +           !devinfo->otp.version)
>> +               return -EINVAL;
>> +
>> +       devinfo->otp.valid = true;
>> +       return 0;
>> +}
>> +
>> +static int
>> +brcmf_pcie_parse_otp(struct brcmf_pciedev_info *devinfo, u8 *otp, size_t size)
>> +{
>> +       int p = 0;
> 
>> +       int ret = -1;
> 
> Use proper error codes.

Ack.

> 
>> +       brcmf_dbg(PCIE, "parse_otp size=%ld\n", size);
>> +
>> +       while (p < (size - 1)) {
> 
> too many parentheses

Really? I see this is all over kernel code. I know it's redundant, but I
find parentheses around expressions used for one side of a comparison to
be a lot more readable since you don't have to start doubting whether
that particular operator has higher precedence than the comparison (+
does but & does not).

> 
>> +               u8 type = otp[p];
>> +               u8 length = otp[p + 1];
>> +
>> +               if (type == 0)
>> +                       break;
>> +
>> +               if ((p + 2 + length) > size)
>> +                       break;
>> +
>> +               switch (type) {
>> +               case BRCMF_OTP_SYS_VENDOR:
>> +                       brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): SYS_VENDOR\n",
> 
> length as hex a bit harder to parse

Not so sure about that, especially if you're trying to mentally add it
to offsets... but sure, I can make it decimal.

> 
>> +                                 p, length);
>> +                       ret = brcmf_pcie_parse_otp_sys_vendor(devinfo,
>> +                                                             &otp[p + 2],
>> +                                                             length);
>> +                       break;
>> +               case BRCMF_OTP_BRCM_CIS:
>> +                       brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): BRCM_CIS\n",
>> +                                 p, length);
>> +                       break;
>> +               default:
>> +                       brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): Unknown type 0x%x\n",
>> +                                 p, length, type);
>> +                       break;
>> +               }
> 
>> +               p += 2 + length;
> 
> 
> length + 2 is easier to read.

I was following the data order here; 2 header bytes and then length
payload bytes. Same reason I used p + 2 + length above.

> 
>> +       }
>> +
>> +       return ret;
>> +}
> 
> ...
> 
>> +               /* Map OTP to shadow area */
>> +               WRITECC32(devinfo, sromcontrol,
>> +                         sromctl | BCMA_CC_SROM_CONTROL_OTPSEL);
> 
> One line?

That exceeds 80 chars, which seems to be the standard in this file which
I'm trying to stick to. If people are okay with pushing to 100 lines,
there are lots of other places I could unwrap lines in this series.

> 
> ...
> 
>> +       otp = kzalloc(sizeof(u16) * words, GFP_KERNEL);
> 
> No check, why? I see in many places you forgot to check for NULL from
> allocator functions.

I think at some point something convinced me that kzalloc and friends
don't fail with GFP_KERNEL... which they rarely do, but they do. I'll
fix it, and add a few missing checks to the existing code while I'm at it.

> Moreover here you should use kzalloc() which does overflow protection.
words is a constant from the switch statement so this could never
overflow anyway, but sure.