Re: [PATCH 1/2] drm/amdgpu: do not keep debugfs dentry

[Nirmoy <nirmodas@xxxxxxx>]

On 2/10/21 7:22 PM, Christian König wrote:
> Am 10.02.21 um 17:06 schrieb Nirmoy Das:
> > Cleanup unwanted  debugfs dentries.
>
> Maybe write unnecessary instead of unwanted.
>
> > Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu.h         |  4 ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 12 +++-----
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c     | 34 ++++++++++-----------
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h     |  4 ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c     |  4 +--
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h     |  4 ---
> >   drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h     |  3 --
> >   7 files changed, 23 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index e0c797a5f739..e3d4d2dcb3a0 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -797,10 +797,6 @@ struct amdgpu_device {
> >       struct amdgpu_i2c_chan *i2c_bus[AMDGPU_MAX_I2C_BUS];
> >       struct amdgpu_debugfs debugfs[AMDGPU_DEBUGFS_MAX_COMPONENTS];
> >       unsigned            debugfs_count;
> > -#if defined(CONFIG_DEBUG_FS)
> > -    struct dentry                   *debugfs_preempt;
> > -    struct dentry *debugfs_regs[AMDGPU_DEBUGFS_MAX_COMPONENTS];
> > -#endif
> >       struct amdgpu_atif        *atif;
> >       struct amdgpu_atcs        atcs;
> >       struct mutex            srbm_mutex;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > index 4c38c5771cbc..874522217b7c 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > @@ -1228,7 +1228,6 @@ int amdgpu_debugfs_regs_init(struct 
> > amdgpu_device *adev)
> >                         adev, debugfs_regs[i]);
> >           if (!i && !IS_ERR_OR_NULL(ent))
> >               i_size_write(ent->d_inode, adev->rmmio_size);
> > -        adev->debugfs_regs[i] = ent;
> >       }
> >         return 0;
> > @@ -1592,21 +1591,20 @@ DEFINE_SIMPLE_ATTRIBUTE(fops_sclk_set, NULL,
> >   int amdgpu_debugfs_init(struct amdgpu_device *adev)
> >   {
> >       int r, i;
> > +    struct dentry *ent = NULL;
>
> Please don't initialize the variables and sort them so that r and i 
> are last.
>
> >   -    adev->debugfs_preempt =
> > -        debugfs_create_file("amdgpu_preempt_ib", 0600,
> > +    ent = debugfs_create_file("amdgpu_preempt_ib", 0600,
> > adev_to_drm(adev)->primary->debugfs_root, adev,
> >                       &fops_ib_preempt);
> > -    if (!(adev->debugfs_preempt)) {
> > +    if (!ent) {
> >           DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs 
> > file\n");
> >           return -EIO;
> >       }
> >   -    adev->smu.debugfs_sclk =
> > -        debugfs_create_file("amdgpu_force_sclk", 0200,
> > +    ent = debugfs_create_file("amdgpu_force_sclk", 0200,
> > adev_to_drm(adev)->primary->debugfs_root, adev,
> >                       &fops_sclk_set);
> > -    if (!(adev->smu.debugfs_sclk)) {
> > +    if (!ent) {
> >           DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n");
> >           return -EIO;
> >       }
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > index 1fb2a91ad30a..1efdfb9b5506 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > @@ -1137,15 +1137,16 @@ static int amdgpu_ras_sysfs_remove_all(struct 
> > amdgpu_device *adev)
> >    *
> >    */
> >   /* debugfs begin */
> > -static void amdgpu_ras_debugfs_create_ctrl_node(struct amdgpu_device 
> > *adev)
> > +static void amdgpu_ras_debugfs_create_ctrl_node(struct amdgpu_device 
> > *adev,
> > +                        struct dentry *dir)
> >   {
> >       struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
> >       struct drm_minor *minor = adev_to_drm(adev)->primary;
> >   -    con->dir = debugfs_create_dir(RAS_FS_NAME, minor->debugfs_root);
> > -    debugfs_create_file("ras_ctrl", S_IWUGO | S_IRUGO, con->dir,
> > +    dir = debugfs_create_dir(RAS_FS_NAME, minor->debugfs_root);
> > +    debugfs_create_file("ras_ctrl", S_IWUGO | S_IRUGO, dir,
> >                   adev, &amdgpu_ras_debugfs_ctrl_ops);
> > -    debugfs_create_file("ras_eeprom_reset", S_IWUGO | S_IRUGO, 
> > con->dir,
> > +    debugfs_create_file("ras_eeprom_reset", S_IWUGO | S_IRUGO, dir,
> >                   adev, &amdgpu_ras_debugfs_eeprom_ops);
> >         /*
> > @@ -1156,7 +1157,7 @@ static void 
> > amdgpu_ras_debugfs_create_ctrl_node(struct amdgpu_device *adev)
> >        * ERREVENT_ATHUB_INTERRUPT generated. Normal GPU recovery routine
> >        * will never be called.
> >        */
> > -    debugfs_create_bool("auto_reboot", S_IWUGO | S_IRUGO, con->dir,
> > +    debugfs_create_bool("auto_reboot", S_IWUGO | S_IRUGO, dir,
> >                   &con->reboot);
> >         /*
> > @@ -1164,16 +1165,16 @@ static void 
> > amdgpu_ras_debugfs_create_ctrl_node(struct amdgpu_device *adev)
> >        * of RAS IPs during ras recovery.
> >        */
> >       debugfs_create_bool("disable_ras_err_cnt_harvest", 0644,
> > -            con->dir, &con->disable_ras_err_cnt_harvest);
> > +            dir, &con->disable_ras_err_cnt_harvest);
> >   }
> >     static void amdgpu_ras_debugfs_create(struct amdgpu_device *adev,
> > -        struct ras_fs_if *head)
> > +                      struct ras_fs_if *head,
> > +                      struct dentry *dir)
> >   {
> > -    struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
> >       struct ras_manager *obj = amdgpu_ras_find_obj(adev, &head->head);
> >   -    if (!obj || obj->ent)
> > +    if (!obj || !dir)
> >           return;
> >         get_obj(obj);
> > @@ -1182,9 +1183,8 @@ static void amdgpu_ras_debugfs_create(struct 
> > amdgpu_device *adev,
> >               head->debugfs_name,
> >               sizeof(obj->fs_data.debugfs_name));
> >   -    obj->ent = debugfs_create_file(obj->fs_data.debugfs_name,
> > -                       S_IWUGO | S_IRUGO, con->dir, obj,
> > -                       &amdgpu_ras_debugfs_ops);
> > +    debugfs_create_file(obj->fs_data.debugfs_name, S_IWUGO | 
> > S_IRUGO, dir,
> > +                obj, &amdgpu_ras_debugfs_ops);
> >   }
> >     void amdgpu_ras_debugfs_create_all(struct amdgpu_device *adev)
> > @@ -1192,6 +1192,7 @@ void amdgpu_ras_debugfs_create_all(struct 
> > amdgpu_device *adev)
> >       struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
> >       struct ras_manager *obj;
> >       struct ras_fs_if fs_info;
> > +    struct dentry *dir = NULL;
> >         /*
> >        * it won't be called in resume path, no need to check
> > @@ -1200,7 +1201,7 @@ void amdgpu_ras_debugfs_create_all(struct 
> > amdgpu_device *adev)
> >       if (!IS_ENABLED(CONFIG_DEBUG_FS) || !con)
> >           return;
> >   -    amdgpu_ras_debugfs_create_ctrl_node(adev);
> > +    amdgpu_ras_debugfs_create_ctrl_node(adev, dir);
>
> That won't work. You need to return the created directory here.
>
> And please stop initializing variables if it isn't explicitly needed, 
> the compiler would have pointed out that there is something wrong here 
> without that.


I keep repeating that.


> >         list_for_each_entry(obj, &con->head, node) {
> >           if (amdgpu_ras_is_supported(adev, obj->head.block) &&
> > @@ -1208,7 +1209,7 @@ void amdgpu_ras_debugfs_create_all(struct 
> > amdgpu_device *adev)
> >               sprintf(fs_info.debugfs_name, "%s_err_inject",
> >                       ras_block_str(obj->head.block));
> >               fs_info.head = obj->head;
> > -            amdgpu_ras_debugfs_create(adev, &fs_info);
> > +            amdgpu_ras_debugfs_create(adev, &fs_info, dir);
> >           }
> >       }
> >   }
> > @@ -1218,10 +1219,9 @@ static void amdgpu_ras_debugfs_remove(struct 
> > amdgpu_device *adev,
> >   {
> >       struct ras_manager *obj = amdgpu_ras_find_obj(adev, head);
> >   -    if (!obj || !obj->ent)
> > +    if (!obj)
> >           return;
> >   -    obj->ent = NULL;
> >       put_obj(obj);
> >   }
>
> Is that function really doing anything any more? I don't think so.


Yes, not really.

Thanks Christian, I will send v2 fixing everything above.


Nirmoy

> Regards,
> Christian.
>
> >   @@ -1233,8 +1233,6 @@ static void 
> > amdgpu_ras_debugfs_remove_all(struct amdgpu_device *adev)
> >       list_for_each_entry_safe(obj, tmp, &con->head, node) {
> >           amdgpu_ras_debugfs_remove(adev, &obj->head);
> >       }
> > -
> > -    con->dir = NULL;
> >   }
> >   /* debugfs end */
> >   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> > index 762f5e46c007..aed0716efa5a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> > @@ -318,8 +318,6 @@ struct amdgpu_ras {
> >       uint32_t supported;
> >       uint32_t features;
> >       struct list_head head;
> > -    /* debugfs */
> > -    struct dentry *dir;
> >       /* sysfs */
> >       struct device_attribute features_attr;
> >       struct bin_attribute badpages_attr;
> > @@ -395,8 +393,6 @@ struct ras_manager {
> >       struct list_head node;
> >       /* the device */
> >       struct amdgpu_device *adev;
> > -    /* debugfs */
> > -    struct dentry *ent;
> >       /* sysfs */
> >       struct device_attribute sysfs_attr;
> >       int attr_inuse;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > index aaad9e304ad9..f4d7cf60c262 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > @@ -1788,7 +1788,7 @@ static void 
> > amdgpu_ttm_training_data_block_init(struct amdgpu_device *adev)
> >           (adev->gmc.mc_vram_size - GDDR6_MEM_TRAINING_OFFSET);
> >       ctx->train_data_size =
> >           GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES;
> > -
> > +
> > DRM_DEBUG("train_data_size:%llx,p2c_train_data_offset:%llx,c2p_train_data_offset:%llx.\n",
> >               ctx->train_data_size,
> >               ctx->p2c_train_data_offset,
> > @@ -2560,11 +2560,11 @@ int amdgpu_ttm_debugfs_init(struct 
> > amdgpu_device *adev)
> >                   ttm_debugfs_entries[count].fops);
> >           if (IS_ERR(ent))
> >               return PTR_ERR(ent);
> > +
> >           if (ttm_debugfs_entries[count].domain == TTM_PL_VRAM)
> >               i_size_write(ent->d_inode, adev->gmc.mc_vram_size);
> >           else if (ttm_debugfs_entries[count].domain == TTM_PL_TT)
> >               i_size_write(ent->d_inode, adev->gmc.gart_size);
> > -        adev->mman.debugfs_entries[count] = ent;
> >       }
> >         count = ARRAY_SIZE(amdgpu_ttm_debugfs_list);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> > index 6c142455fc66..4df4cf2fd4dd 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> > @@ -68,10 +68,6 @@ struct amdgpu_mman {
> >       bool                initialized;
> >       void __iomem            *aper_base_kaddr;
> >   -#if defined(CONFIG_DEBUG_FS)
> > -    struct dentry            *debugfs_entries[8];
> > -#endif
> > -
> >       /* buffer handling */
> >       const struct amdgpu_buffer_funcs    *buffer_funcs;
> >       struct amdgpu_ring            *buffer_funcs_ring;
> > diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h 
> > b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > index 10b0624ade65..83147b7d836e 100644
> > --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > @@ -439,9 +439,6 @@ struct smu_context
> >       struct smu_baco_context        smu_baco;
> >       struct smu_temperature_range    thermal_range;
> >       void *od_settings;
> > -#if defined(CONFIG_DEBUG_FS)
> > -    struct dentry                   *debugfs_sclk;
> > -#endif
> >         struct smu_umd_pstate_table    pstate_table;
> >       uint32_t pstate_sclk;
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx