On Tue, 2024-02-13 at 16:43 +0100, Danilo Krummrich wrote:
> > +struct registry_list_entry {
> > + struct list_head list;
>
> Nit: 'head' or 'entry' might be more suitable.
Will fix in v4.
>
> > + size_t name_len;
> > + u32 type;
>
> I prefer to represent type as enum or use a define for '1 = integer' instead.
> This also gets you rid of the need to explicitly explain it's meaning in the
> documentation of add_registry().
Will fix in v4.
>
> > + u32 data;
> > + u32 length;
> > + char name[];
> > +};
> > +
> > +/**
> > + * add_registry -- adds a registry entry
> > + * @gsp: gsp pointer
> > + * @name: name of the registry key
> > + * @type: type of data (1 = integer)
> > + * @data: value
> > + * @length: size of data, in bytes
> > + *
> > + * Adds a registry key/value pair to the registry database.
> > + *
> > + * Currently, only 32-bit integers (type == 1, length == 4) are supported.
>
> What if we pass something else? This function doesn't seem to ensure nothing
> else is accepted. Although I recognize that it's only used by add_registry_num()
> enforcing this limitation by it's function signature.
GSP-RM technically supports two other types: binary data and strings. For
example, apparently it's possible to override the VBIOS itself with a
registry key. However, I'm not familiar with any actual non-numeric
registry keys that the user would set.
I can add support for binary data and strings.
>
> > + *
> > + * This function collects the registry information in a linked list. After
> > + * all registry keys have been added, build_registry() is used to create the
> > + * RPC data structure.
> > + *
> > + * registry_rpc_size is a running total of the size of all registry keys.
> > + * It's used to avoid an O(n) calculation of the size when the RPC is built.
> > + *
> > + * Returns 0 on success, or negative error code on error.
> > + */
> > +static int add_registry(struct nvkm_gsp *gsp, const char *name, u32 type, u32 data, u32 length)
> > +{
> > + struct registry_list_entry *reg;
> > + size_t nlen = strlen(name) + 1;
> > +
> > + /* Set an arbitrary limit to avoid problems with broken command lines */
> > + if (strlen(name) > 64)
>
> Could re-use nlen for this check.
Will fix in v4.
>
> > + return -EFBIG;
>
> This error code doesn't seem to apply here, prefer EINVAL.
You don't like creative usage of error codes?
> > + while ((start = strsep(&next, ";"))) {
> > + long value;
> > +
> > + equal = strchr(start, '=');
> > + if (!equal || (equal == start) || !isdigit(equal[1])) {
> > + nvkm_error(&gsp->subdev,
> > + "ignoring invalid registry string '%s'\n", start);
> > + continue;
> > + }
> >
> > - rpc->numEntries = NV_GSP_REG_NUM_ENTRIES;
> > + ret = kstrtol(equal + 1, 0, &value);
> > + if (ret) {
> > + nvkm_error(&gsp->subdev,
> > + "ignoring invalid registry value in '%s'\n", start);
> > + continue;
> > + }
>
> At a first glance, the string parsing looks correct to me. Did you test with invalid
> strings as well?
Yes. It would be nice if there were a regex parser in the kernel, but I
think I did a good job testing and rejecting strings.
> > - str_offset = offsetof(typeof(*rpc), entries[NV_GSP_REG_NUM_ENTRIES]);
> > - strings = (char *)&rpc->entries[NV_GSP_REG_NUM_ENTRIES];
> > - for (i = 0; i < NV_GSP_REG_NUM_ENTRIES; i++) {
> > - int name_len = strlen(r535_registry_entries[i].name) + 1;
> > -
> > - rpc->entries[i].nameOffset = str_offset;
> > - rpc->entries[i].type = 1;
> > - rpc->entries[i].data = r535_registry_entries[i].value;
> > - rpc->entries[i].length = 4;
> > - memcpy(strings, r535_registry_entries[i].name, name_len);
> > - strings += name_len;
> > - str_offset += name_len;
> > + /* Truncate the key=value string to just key */
> > + *equal = 0;
>
> What's the purpose for that?
Take for example, this parameter passed to Nouveau:
NVreg_RegistryDwords="RM1457588=1;TEST=53"
The strsep() call points 'next' to "RM1457588=1", replacing the ; with \0.
'equal' then points to the '='. kstrtol() converts the "1" to 1. So all
that's left is extracting the "RM1457588" into its own string. That's what
"*equal = 0" does.
