Re: [RFC PATCH 03/13] drm/amdgpu: add a flag to track ras debugfs creation status

[Mario Limonciello <mario.limonciello@xxxxxxx>]

On 1/9/2025 21:19, Gerry Liu wrote:
> > 2025年1月9日 01:19，Mario Limonciello <mario.limonciello@xxxxxxx 
> > <mailto:mario.limonciello@xxxxxxx>> 写道：
> >
> > On 1/8/2025 07:59, Jiang Liu wrote:
> > > Add a flag to track ras debugfs creation status, to avoid possible
> > > incorrect reference count management for ras block object  in function
> > > amdgpu_ras_aca_is_supported().
> >
> > Rather than taking a marker position, why not just check for
> > obj->fs_data.debugfs_name to be non NULL in amdgpu_ras_fs_fini()?
> I plan to use marker as a common status track mechanism, so used marker 
> here:)
>
> > > Signed-off-by: Jiang Liu <gerry@xxxxxxxxxxxxxxxxx 
> > > <mailto:gerry@xxxxxxxxxxxxxxxxx>>
> > > ---
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu.h     | 2 +-
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 9 +++++++--
> > >  2 files changed, 8 insertions(+), 3 deletions(-)
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/ 
> > > amd/amdgpu/amdgpu.h
> > > index 32941f29507c..2ef7d3102be3 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > > @@ -378,7 +378,7 @@ enum amdgpu_marker {
> > > AMDGPU_MARKER_IRQ6= 6,
> > > AMDGPU_MARKER_IRQ7= 7,
> > > AMDGPU_MARKER_IRQ_MAX= 47,
> > > -AMDGPU_MARKER_DEBUGFS= 63,
> > > +AMDGPU_MARKER_RAS_DEBUGFS= 63,
> >
> > Any particular reason you jumped to 63 in this patch and then counted 
> > down in the next one?  IE why not throw it at 48 (and then 49 for next 
> > one)?
> I’m not sure how much markers are needed for IRQ, so I split the space 
> into two parts: one for irq and one for others.

I think it's up to Alex and Christian here but as it's all internal to 
amdgpu we can always reassign the enum in the future.

So I think we should just take 48 and 49.  If another IRQ is needed we 
can slot it in at 48 and bump these by 1.

Alex, Christian, thoughts?

> > >  };
> > >    #define AMDGPU_MARKER_INDEX_IRQ(idx)(AMDGPU_MARKER_INDEX_IRQ0 + (idx))
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/ 
> > > drm/amd/amdgpu/amdgpu_ras.c
> > > index 6d52e22691f7..efd72b07a185 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > > @@ -1996,7 +1996,8 @@ static void amdgpu_ras_debugfs_create(struct 
> > > amdgpu_device *adev,
> > >  {
> > > struct ras_manager *obj = amdgpu_ras_find_obj(adev, &head->head);
> > >  -if (!obj || !dir)
> > > +if (!obj || !dir ||
> > > +   amdgpu_ras_test_marker(adev, &head->head, AMDGPU_MARKER_RAS_DEBUGFS))
> > > return;
> > > get_obj(obj);
> > > @@ -2007,6 +2008,8 @@ static void amdgpu_ras_debugfs_create(struct 
> > > amdgpu_device *adev,
> > > debugfs_create_file(obj->fs_data.debugfs_name, S_IWUGO | S_IRUGO, dir,
> > >    obj, &amdgpu_ras_debugfs_ops);
> > > +
> > > +amdgpu_ras_set_marker(adev, &head->head, AMDGPU_MARKER_RAS_DEBUGFS);
> > >  }
> > >    static bool amdgpu_ras_aca_is_supported(struct amdgpu_device *adev)
> > > @@ -2134,7 +2137,9 @@ static int amdgpu_ras_fs_fini(struct 
> > > amdgpu_device *adev)
> > > if (IS_ENABLED(CONFIG_DEBUG_FS)) {
> > > list_for_each_entry_safe(con_obj, tmp, &con->head, node) {
> > > ip_obj = amdgpu_ras_find_obj(adev, &con_obj->head);
> > > -if (ip_obj)
> > > +if (ip_obj &&
> > > +   amdgpu_ras_test_and_clear_marker(adev, &ip_obj->head,
> > > +    AMDGPU_MARKER_RAS_DEBUGFS))
> > > put_obj(ip_obj);
> > > }
> > > }