Patch is fine, if it does what you want. A few comments inline.
On 2022-01-24 13:07, Tom St Denis wrote:
> Newer hardware has a discovery table in hardware that the kernel will
> rely on instead of header files for things like IP offsets. This
> sysfs entry adds a simple to parse table of IP instances and segment
> offsets.
>
> Produces output that looks like:
>
> $ cat ip_discovery_text
> ATHUB{0} v2.0.0: 00000c00 02408c00
> CLKA{0} v11.0.0: 00016c00 02401800
> CLKA{1} v11.0.0: 00016e00 02401c00
> CLKA{2} v11.0.0: 00017000 02402000
> CLKA{3} v11.0.0: 00017200 02402400
> CLKA{4} v11.0.0: 0001b000 0242d800
> CLKB{0} v11.0.0: 00017e00 0240bc00
> DBGU_NBIO{0} v3.0.0: 000001c0 02409000
> DBGU0{0} v3.0.0: 00000180 02409800
> DBGU1{0} v3.0.0: 000001a0 02409c00
> DF{0} v3.0.0: 00007000 0240b800
> DFX{0} v4.1.0: 00000580 02409400
> DFX_DAP{0} v2.0.0: 000005a0 00b80000 0240c400
> DMU{0} v2.0.2: 00000012 000000c0 000034c0 00009000 02403c00
> FUSE{0} v11.0.0: 00017400 02401400
> GC{0} v10.1.10: 00001260 0000a000 02402c00
>
> Signed-off-by: Tom St Denis <tom.stdenis@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 77 ++++++++++++++++++-
> 2 files changed, 77 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 3bc76759c143..6920f73bbe73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1019,6 +1019,7 @@ struct amdgpu_device {
> struct amdgpu_ip_block ip_blocks[AMDGPU_MAX_IP_NUM];
> uint32_t harvest_ip_mask;
> int num_ip_blocks;
> + char *ip_discovery_text;
The above is not aligned. Don't use hard tabs to align. Just use spaces.
> struct mutex mn_lock;
> DECLARE_HASHTABLE(mn_hash, 7);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index 285811509d94..c64cab0ad18e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -267,6 +267,19 @@ static void amdgpu_discovery_harvest_config_quirk(struct amdgpu_device *adev)
> }
> }
>
> +static ssize_t ip_discovery_text_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct drm_device *ddev = dev_get_drvdata(dev);
> + struct amdgpu_device *adev = drm_to_adev(ddev);
> +
> + return sysfs_emit(buf, "%s", adev->ip_discovery_text);
> +}
> +
> +static DEVICE_ATTR(ip_discovery_text, S_IRUGO,
> + ip_discovery_text_show, NULL);
> +
> +
> static int amdgpu_discovery_init(struct amdgpu_device *adev)
> {
> struct table_info *info;
> @@ -351,6 +364,11 @@ static int amdgpu_discovery_init(struct amdgpu_device *adev)
> goto out;
> }
>
> + // init sysfs for ip_discovery
> + r = sysfs_create_file(&adev->dev->kobj, &dev_attr_ip_discovery_text.attr);
> + if (r)
> + dev_err(adev->dev, "Could not create amdgpu device attr\n");
> +
> return 0;
>
> out:
> @@ -363,7 +381,11 @@ static int amdgpu_discovery_init(struct amdgpu_device *adev)
> void amdgpu_discovery_fini(struct amdgpu_device *adev)
> {
> kfree(adev->mman.discovery_bin);
> + kfree(adev->ip_discovery_text);
> + sysfs_remove_file(&adev->dev->kobj, &dev_attr_ip_discovery_text.attr);
> +
> adev->mman.discovery_bin = NULL;
> + adev->ip_discovery_text = NULL;
> }
>
> static int amdgpu_discovery_validate_ip(const struct ip *ip)
> @@ -382,6 +404,20 @@ static int amdgpu_discovery_validate_ip(const struct ip *ip)
> return 0;
> }
>
> +static int add_string(char **dst, unsigned *size, char *src)
> +{
> + if (strlen(src) + strlen(*dst) >= *size) {
> + void *tmp = krealloc(*dst, *size + 4096, GFP_KERNEL);
> + if (!tmp) {
> + return -1;
> + }
> + *dst = tmp;
> + *size = *size + 4096;
> + }
> + strcat(*dst, src);
Heavy reliance in *dst and src ending in '\0'. See below.
> + return 0;
> +}
> +
> int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
> {
> struct binary_header *bhdr;
> @@ -396,6 +432,8 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
> int hw_ip;
> int i, j, k;
> int r;
> + unsigned txt_size = 4096;
Can you use a local macro here for this literal, and possibly define that local macro to PAGE_SIZE.
> + char *linebuf;
>
> r = amdgpu_discovery_init(adev);
> if (r) {
> @@ -410,6 +448,15 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
>
> DRM_DEBUG("number of dies: %d\n", num_dies);
>
> + adev->ip_discovery_text = kzalloc(txt_size, GFP_KERNEL);
> + linebuf = kzalloc(4096, GFP_KERNEL);
> + if (!adev->ip_discovery_text || !linebuf) {
> + DRM_ERROR("Out of memory\n");
> + kfree(linebuf);
> + kfree(adev->ip_discovery_text);
> + return -ENOMEM;
> + }
> +
> for (i = 0; i < num_dies; i++) {
> die_offset = le16_to_cpu(ihdr->die_info[i].die_offset);
> dhdr = (struct die_header *)(adev->mman.discovery_bin + die_offset);
> @@ -419,6 +466,8 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
> if (le16_to_cpu(dhdr->die_id) != i) {
> DRM_ERROR("invalid die id %d, expected %d\n",
> le16_to_cpu(dhdr->die_id), i);
> + kfree(linebuf);
> + kfree(adev->ip_discovery_text);
> return -EINVAL;
> }
>
> @@ -458,6 +507,19 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
> le16_to_cpu(ip->hw_id) == SDMA3_HWID)
> adev->sdma.num_instances++;
>
> + snprintf(linebuf, 4096, "%s{%d} v%d.%d.%d: ",
> + hw_id_names[le16_to_cpu(ip->hw_id)],
> + ip->number_instance,
> + ip->major, ip->minor,
> + ip->revision);
This and add_string() and sysfs_emit() all rely that the strings in "ip" all end with '\0'. snprintf() is somehow protected here, since it uses the 4096 limit. Is it possible that a string could be too long and overwrite the 4096 limit? Maybe make it 4096-1, so that snprintf has space to write the '\0', or make sure that we do end with '\0'? Unless we can guarantee that it'll always end with '\0'...
> + if (add_string(&adev->ip_discovery_text, &txt_size, linebuf)) {
> + DRM_ERROR("Out of memory\n");
> + kfree(linebuf);
> + kfree(adev->ip_discovery_text);
> + return -ENOMEM;
> + }
> +
> +
> for (k = 0; k < num_base_address; k++) {
> /*
> * convert the endianness of base addresses in place,
> @@ -465,6 +527,19 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
> */
> ip->base_address[k] = le32_to_cpu(ip->base_address[k]);
> DRM_DEBUG("\t0x%08x\n", ip->base_address[k]);
> + snprintf(linebuf, 4096, "%08lx ", (unsigned long)ip->base_address[k]);
> + if (add_string(&adev->ip_discovery_text, &txt_size, linebuf)) {
> + DRM_ERROR("Out of memory\n");
> + kfree(linebuf);
> + kfree(adev->ip_discovery_text);
> + return -ENOMEM;
> + }
> + }
> + if (add_string(&adev->ip_discovery_text, &txt_size, "\n")) {
> + DRM_ERROR("Out of memory\n");
> + kfree(linebuf);
> + kfree(adev->ip_discovery_text);
> + return -ENOMEM;
> }
>
> for (hw_ip = 0; hw_ip < MAX_HWIP; hw_ip++) {
> @@ -491,7 +566,7 @@ int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
> ip_offset += sizeof(*ip) + 4 * (ip->num_base_address - 1);
> }
> }
> -
> + kfree(linebuf);
> return 0;
> }
>
Regards,
--
Luben
