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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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?

> +       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() ?

> +       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()

> +
> +               if (*p++ != '=') /* implicit NUL check */
> +                       return -EINVAL;

Have you checked the next_arg() implementation?

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

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

too many parentheses

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

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

> +       }
> +
> +       return ret;
> +}

...

> +               /* Map OTP to shadow area */
> +               WRITECC32(devinfo, sromcontrol,
> +                         sromctl | BCMA_CC_SROM_CONTROL_OTPSEL);

One line?

...

> +       otp = kzalloc(sizeof(u16) * words, GFP_KERNEL);

No check, why? I see in many places you forgot to check for NULL from
allocator functions.
Moreover here you should use kcalloc() which does overflow protection.

-- 
With Best Regards,
Andy Shevchenko