Danilo Krummrich
2024-Apr-25 13:22 UTC
[PATCH] [v7] nouveau: add command-line GSP-RM registry support
On 4/17/24 23:53, Timur Tabi wrote: <snip>> + > +/** > + * r535_gsp_rpc_set_registry - build registry RPC and call GSP-RM > + * @gsp: gsp pointer > + * > + * The GSP-RM registry is a set of key/value pairs that configure some aspects > + * of GSP-RM. The keys are strings, and the values are 32-bit integers. > + * > + * The registry is built from a combination of a static hard-coded list (see > + * above) and entries passed on the driver's command line. > + */ > static int > r535_gsp_rpc_set_registry(struct nvkm_gsp *gsp) > { > PACKED_REGISTRY_TABLE *rpc; > - char *strings; > - int str_offset; > - int i; > - size_t rpc_size = struct_size(rpc, entries, NV_GSP_REG_NUM_ENTRIES); > + unsigned int i; > + int ret; > > - /* add strings + null terminator */ > - for (i = 0; i < NV_GSP_REG_NUM_ENTRIES; i++) > - rpc_size += strlen(r535_registry_entries[i].name) + 1; > + INIT_LIST_HEAD(&gsp->registry_list); > + gsp->registry_rpc_size = sizeof(PACKED_REGISTRY_TABLE); > > - rpc = nvkm_gsp_rpc_get(gsp, NV_VGPU_MSG_FUNCTION_SET_REGISTRY, rpc_size); > - if (IS_ERR(rpc)) > - return PTR_ERR(rpc); > + for (i = 0; i < NV_GSP_REG_NUM_ENTRIES; i++) { > + ret = add_registry_num(gsp, r535_registry_entries[i].name, > + r535_registry_entries[i].value); > + if (ret) { > + clean_registry(gsp); > + return ret; > + } > + } > > - rpc->numEntries = NV_GSP_REG_NUM_ENTRIES; > + /* > + * The NVreg_RegistryDwords parameter is a string of key=value > + * pairs separated by semicolons. We need to extract and trim each > + * substring, and then parse the substring to extract the key and > + * value. > + */ > + if (NVreg_RegistryDwords) { > + char *p = kstrdup(NVreg_RegistryDwords, GFP_KERNEL); > + char *start, *next = p, *equal; > + size_t length; > + > + /* Remove any whitespace from the parameter string */ > + length = strip(p, " \t\n");With that, I see the following warning compiling this patch. warning: variable ?length? set but not used [-Wunused-but-set-variable] Did you intend to use the length for anything? Also, looking at the warning made me aware of 'p' potentially being NULL. If you agree, I can fix the warning and add the corresponding NULL check when applying the patch. - Danilo> + > + while ((start = strsep(&next, ";"))) { > + long value; > + > + equal = strchr(start, '='); > + if (!equal || equal == start || equal[1] == 0) { > + nvkm_error(&gsp->subdev, > + "ignoring invalid registry string '%s'\n", > + start); > + continue; > + } > > - str_offset = offsetof(typeof(*rpc), entries[NV_GSP_REG_NUM_ENTRIES]); > - strings = (char *)rpc + str_offset; > - 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; > + > + ret = kstrtol(equal + 1, 0, &value); > + if (!ret) { > + ret = add_registry_num(gsp, start, value); > + } else { > + /* Not a number, so treat it as a string */ > + ret = add_registry_string(gsp, start, equal + 1); > + } > + > + if (ret) { > + nvkm_error(&gsp->subdev, > + "ignoring invalid registry key/value '%s=%s'\n", > + start, equal + 1); > + continue; > + } > + } > + > + kfree(p); > } > - rpc->size = str_offset; > + > + rpc = nvkm_gsp_rpc_get(gsp, NV_VGPU_MSG_FUNCTION_SET_REGISTRY, gsp->registry_rpc_size); > + if (IS_ERR(rpc)) { > + clean_registry(gsp); > + return PTR_ERR(rpc); > + } > + > + build_registry(gsp, rpc); > > return nvkm_gsp_rpc_wr(gsp, rpc, false); > } > > base-commit: f7ad2ce5fd89ab5d146da8f486a310746df5dc9e > prerequisite-patch-id: 9bb653b6a53dcba4171d653e24a242461374f9fe > prerequisite-patch-id: 7093a9db84053e43f6f278bf1d159a25d14ceebf
Timur Tabi
2024-Apr-25 16:38 UTC
[PATCH] [v7] nouveau: add command-line GSP-RM registry support
On Thu, 2024-04-25 at 15:22 +0200, Danilo Krummrich wrote:> > + size_t length; > > + > > + /* Remove any whitespace from the parameter string */ > > + length = strip(p, " \t\n"); > > With that, I see the following warning compiling this patch. > > warning: variable ?length? set but not used [-Wunused-but-set-variable] > > Did you intend to use the length for anything?No, and I could have sworn I fixed that before sending out v7. I think I originally intended 'length' to determine when I finished parsing the string.> Also, looking at the warning made me aware of 'p' potentially being NULL. > > If you agree, I can fix the warning and add the corresponding NULL check > when > applying the patch.Yes, that would be great. You can just delete 'length'. The NULL check for 'p' should call clean_registry() before returning -ENOMEM. Thanks for catching this.