[PATCH 1/6] amdgpu: add flags to track sysfs initialization status

[Russell, Kent]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Chen, Xiaogang <Xiaogang.Chen at amd.com>
> Sent: Thursday, January 2, 2025 6:08 PM
> To: Jiang Liu <gerry at linux.alibaba.com>; amd-gfx at lists.freedesktop.org; Russell,
> Kent <Kent.Russell at amd.com>; shuox.liu at linux.alibaba.com
> Subject: Re: [PATCH 1/6] amdgpu: add flags to track sysfs initialization status
>
>
> On 1/1/2025 11:36 PM, Jiang Liu wrote:
> > Add flags to track sysfs initialization status, so we can correctly
> > clean them up on error recover paths.
> >
> > Signed-off-by: Jiang Liu <gerry at linux.alibaba.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  3 ++
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 34 +++++++++++++++++-----
> >   2 files changed, 30 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index 22c7e9669162..e4b13e729770 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -1157,6 +1157,9 @@ struct amdgpu_device {
> >     bool                            in_runpm;
> >     bool                            has_pr3;
> >
> > +   bool                            sysfs_en;
> > +   bool                            fru_sysfs_en;
> > +   bool                            reg_state_sysfs_en;
> >     bool                            ucode_sysfs_en;
> >
> >     struct amdgpu_fru_info          *fru_info;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index d1bb9e85b6d7..3244966b0c39 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4533,8 +4533,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> >             adev->ucode_sysfs_en = true;
> >
> >     r = sysfs_create_files(&adev->dev->kobj, amdgpu_dev_attributes);
> > -   if (r)
> > +   if (r) {
> >             dev_err(adev->dev, "Could not create amdgpu device attr\n");
> > +           adev->sysfs_en = false;
> > +   } else {
> > +           adev->sysfs_en = true;
> > +   }
> > +
> not need use {}

The kernel style guide says that if one of the if blocks requires {}, that all other blocks require it too, even if the else is a single line. It's the last block before 3.1 (Spaces) at https://www.kernel.org/doc/html/v4.10/process/coding-style.html

Though with Lijo's feedback, it may get dropped altogether In v2 anyways.

 Kent

> >   #ifdef HAVE_PCI_DRIVER_DEV_GROUPS
> >     r = devm_device_add_group(adev->dev, &amdgpu_board_attrs_group);
> >     if (r)
> > @@ -4542,8 +4547,21 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> >                     "Could not create amdgpu board attributes\n");
> >   #endif
> >
> > -   amdgpu_fru_sysfs_init(adev);
> > -   amdgpu_reg_state_sysfs_init(adev);
> > +   r = amdgpu_fru_sysfs_init(adev);
> > +   if (r) {
> > +           dev_err(adev->dev, "Could not create amdgpu fru attr\n");
> > +           adev->fru_sysfs_en = false;
> > +   } else {
> > +           adev->fru_sysfs_en = true;
> > +   }
> not need use {}
> > +
> > +   r = amdgpu_reg_state_sysfs_init(adev);
> > +   if (r) {
> > +           dev_err(adev->dev, "Could not create amdgpu reg state attr\n");
> > +           adev->reg_state_sysfs_en = false;
> > +   } else {
> > +           adev->reg_state_sysfs_en = true;
> > +   }
> same as above
> >
> >     if (IS_ENABLED(CONFIG_PERF_EVENTS))
> >             r = amdgpu_pmu_init(adev);
> > @@ -4666,10 +4684,12 @@ void amdgpu_device_fini_hw(struct amdgpu_device
> *adev)
> >             amdgpu_pm_sysfs_fini(adev);
> >     if (adev->ucode_sysfs_en)
> >             amdgpu_ucode_sysfs_fini(adev);
> > -   sysfs_remove_files(&adev->dev->kobj, amdgpu_dev_attributes);
> > -   amdgpu_fru_sysfs_fini(adev);
> > -
> > -   amdgpu_reg_state_sysfs_fini(adev);
> > +   if (adev->sysfs_en)
> > +           sysfs_remove_files(&adev->dev->kobj, amdgpu_dev_attributes);
> > +   if (adev->fru_sysfs_en)
> > +           amdgpu_fru_sysfs_fini(adev);
> > +   if (adev->reg_state_sysfs_en)
> > +           amdgpu_reg_state_sysfs_fini(adev);
>
> If creations of these sys entries fail does kernel send warnings or just
> ignore that when delete these entries? Seems kernel keeps silence for
> this case.
>
> Regards
>
> Xiaogang
>
>
> >
> >     /* disable ras feature must before hw fini */
> >     amdgpu_ras_pre_fini(adev);