[PATCH 2/5] drm/amd: Use attribute groups for PSP flashing attributes
Lazar, Lijo
lijo.lazar at amd.com
Tue Jun 27 09:06:29 UTC 2023
On 6/26/2023 8:34 PM, Mario Limonciello wrote:
> Individually creating attributes can be racy, instead make attributes
> using attribute groups and control their visibility with an is_visible
> callback to only show when using appropriate products.
>
> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 -
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 -----
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 49 +++++++++++-----------
> drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 1 -
> 5 files changed, 27 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 02b827785e399..a7ef43e25c758 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1004,7 +1004,6 @@ struct amdgpu_device {
> bool has_pr3;
>
> bool ucode_sysfs_en;
> - bool psp_sysfs_en;
>
> /* Chip product information */
> char product_number[20];
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 5c7d40873ee20..65fe0f3488679 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3907,14 +3907,6 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> } else
> adev->ucode_sysfs_en = true;
>
> - r = amdgpu_psp_sysfs_init(adev);
> - if (r) {
> - adev->psp_sysfs_en = false;
> - if (!amdgpu_sriov_vf(adev))
> - DRM_ERROR("Creating psp sysfs failed\n");
> - } else
> - adev->psp_sysfs_en = true;
> -
> /*
> * Register gpu instance before amdgpu_device_enable_mgpu_fan_boost.
> * Otherwise the mgpu fan boost feature will be skipped due to the
> @@ -4064,8 +4056,6 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
> amdgpu_pm_sysfs_fini(adev);
> if (adev->ucode_sysfs_en)
> amdgpu_ucode_sysfs_fini(adev);
> - if (adev->psp_sysfs_en)
> - amdgpu_psp_sysfs_fini(adev);
> sysfs_remove_files(&adev->dev->kobj, amdgpu_dev_attributes);
>
> /* disable ras feature must before hw fini */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 393b6fb7a71d3..99b8d3113d6af 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2827,11 +2827,13 @@ static struct pci_error_handlers amdgpu_pci_err_handler = {
> extern const struct attribute_group amdgpu_vram_mgr_attr_group;
> extern const struct attribute_group amdgpu_gtt_mgr_attr_group;
> extern const struct attribute_group amdgpu_vbios_version_attr_group;
> +extern const struct attribute_group amdgpu_flash_attr_group;
>
> static const struct attribute_group *amdgpu_sysfs_groups[] = {
> &amdgpu_vram_mgr_attr_group,
> &amdgpu_gtt_mgr_attr_group,
> &amdgpu_vbios_version_attr_group,
> + &amdgpu_flash_attr_group,
> NULL,
> };
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index eb687a338a1bd..4286c0b4beb90 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -3584,6 +3584,13 @@ static ssize_t amdgpu_psp_vbflash_read(struct file *filp, struct kobject *kobj,
> return 0;
> }
>
> +static struct bin_attribute psp_vbflash_bin_attr = {
> + .attr = {.name = "psp_vbflash", .mode = 0660},
> + .size = 0,
> + .write = amdgpu_psp_vbflash_write,
> + .read = amdgpu_psp_vbflash_read,
> +};
> +
> static ssize_t amdgpu_psp_vbflash_status(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -3600,39 +3607,39 @@ static ssize_t amdgpu_psp_vbflash_status(struct device *dev,
>
> return sysfs_emit(buf, "0x%x\n", vbflash_status);
> }
> +static DEVICE_ATTR(psp_vbflash_status, 0440, amdgpu_psp_vbflash_status, NULL);
>
> -static const struct bin_attribute psp_vbflash_bin_attr = {
> - .attr = {.name = "psp_vbflash", .mode = 0660},
> - .size = 0,
> - .write = amdgpu_psp_vbflash_write,
> - .read = amdgpu_psp_vbflash_read,
> +static struct attribute *flash_attrs[] = {
> + &dev_attr_psp_vbflash_status.attr,
> + &psp_vbflash_bin_attr.attr,
There is a separate bin_attrs/is_bin_visible in attribute group. Better
to make use of that instead of mixing bin and regular attributes.
> + NULL
> };
>
> -static DEVICE_ATTR(psp_vbflash_status, 0440, amdgpu_psp_vbflash_status, NULL);
> -
> -int amdgpu_psp_sysfs_init(struct amdgpu_device *adev)
> +static umode_t amdgpu_flash_attr_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
> {
> - int ret = 0;
> - struct psp_context *psp = &adev->psp;
> + struct device *dev = kobj_to_dev(kobj);
> + struct drm_device *ddev = dev_get_drvdata(dev);
> + struct amdgpu_device *adev = drm_to_adev(ddev);
>
> if (amdgpu_sriov_vf(adev))
> - return -EINVAL;
> + return 0;
>
> switch (adev->ip_versions[MP0_HWIP][0]) {
> case IP_VERSION(13, 0, 0):
> case IP_VERSION(13, 0, 7):
If we can init some flags during early init of each IP, then this type
of check can be removed here. Instead check the flags. May need more
work for such flags when some IPs support only read vs some supporting
both read/write.
Thanks,
Lijo
> - ret = sysfs_create_bin_file(&adev->dev->kobj, &psp_vbflash_bin_attr);
> - if (ret)
> - dev_err(adev->dev, "Failed to create device file psp_vbflash");
> - ret = device_create_file(adev->dev, &dev_attr_psp_vbflash_status);
> - if (ret)
> - dev_err(adev->dev, "Failed to create device file psp_vbflash_status");
> - return ret;
> + if (attr == &psp_vbflash_bin_attr.attr)
> + return 0660;
> + return 0440;
> default:
> return 0;
> }
> }
>
> +const struct attribute_group amdgpu_flash_attr_group = {
> + .attrs = flash_attrs,
> + .is_visible = amdgpu_flash_attr_is_visible,
> +};
> +
> const struct amd_ip_funcs psp_ip_funcs = {
> .name = "psp",
> .early_init = psp_early_init,
> @@ -3661,12 +3668,6 @@ static int psp_sysfs_init(struct amdgpu_device *adev)
> return ret;
> }
>
> -void amdgpu_psp_sysfs_fini(struct amdgpu_device *adev)
> -{
> - sysfs_remove_bin_file(&adev->dev->kobj, &psp_vbflash_bin_attr);
> - device_remove_file(adev->dev, &dev_attr_psp_vbflash_status);
> -}
> -
> static void psp_sysfs_fini(struct amdgpu_device *adev)
> {
> device_remove_file(adev->dev, &dev_attr_usbc_pd_fw);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> index cf4f60c661223..b441c07e5a16f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
> @@ -522,5 +522,4 @@ void psp_copy_fw(struct psp_context *psp, uint8_t *start_addr, uint32_t bin_size
> int is_psp_fw_valid(struct psp_bin_desc bin);
>
> int amdgpu_psp_sysfs_init(struct amdgpu_device *adev);
> -void amdgpu_psp_sysfs_fini(struct amdgpu_device *adev);
> #endif
More information about the amd-gfx
mailing list