* [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs
@ 2021-09-02 11:44 Nirmoy Das
2021-09-02 11:44 ` [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings Nirmoy Das
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Nirmoy Das @ 2021-09-02 11:44 UTC (permalink / raw)
To: amd-gfx; +Cc: Christian.Koenig, Nirmoy Das
debugfs APIs returns encoded error so use
IS_ERR for checking return value.
References: https://gitlab.freedesktop.org/drm/amd/-/issues/1686
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 ++----
drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 2 +-
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index d256215ab2c7..077f9baf74fe 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1696,18 +1696,16 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
struct dentry *ent;
int r, i;
-
-
ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
&fops_ib_preempt);
- if (!ent) {
+ if (IS_ERR(ent)) {
DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs file\n");
return -EIO;
}
ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, adev,
&fops_sclk_set);
- if (!ent) {
+ if (IS_ERR(ent)) {
DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n");
return -EIO;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index 7b634a1517f9..f40753e1a60d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -428,7 +428,7 @@ int amdgpu_debugfs_ring_init(struct amdgpu_device *adev,
ent = debugfs_create_file(name,
S_IFREG | S_IRUGO, root,
ring, &amdgpu_debugfs_ring_fops);
- if (!ent)
+ if (IS_ERR(ent))
return -ENOMEM;
i_size_write(ent->d_inode, ring->ring_size + 12);
--
2.32.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings 2021-09-02 11:44 [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs Nirmoy Das @ 2021-09-02 11:44 ` Nirmoy Das 2021-09-03 6:36 ` Sharma, Shashank 2021-09-02 15:23 ` [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs Sharma, Shashank 2021-09-03 7:53 ` Christian König 2 siblings, 1 reply; 16+ messages in thread From: Nirmoy Das @ 2021-09-02 11:44 UTC (permalink / raw) To: amd-gfx; +Cc: Christian.Koenig, Nirmoy Das Use debugfs_create_file_size API for creating ring debugfs file, also cleanup surrounding code. Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 4 +--- drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 16 +++++----------- drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 8 +------- 3 files changed, 7 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c index 077f9baf74fe..dee56ab19a8f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev) if (!ring) continue; - if (amdgpu_debugfs_ring_init(adev, ring)) { - DRM_ERROR("Failed to register debugfs file for rings !\n"); - } + amdgpu_debugfs_ring_init(adev, ring); } amdgpu_ras_debugfs_create_all(adev); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c index f40753e1a60d..968521d80514 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c @@ -415,26 +415,20 @@ static const struct file_operations amdgpu_debugfs_ring_fops = { #endif -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring) { #if defined(CONFIG_DEBUG_FS) struct drm_minor *minor = adev_to_drm(adev)->primary; - struct dentry *ent, *root = minor->debugfs_root; + struct dentry *root = minor->debugfs_root; char name[32]; sprintf(name, "amdgpu_ring_%s", ring->name); - ent = debugfs_create_file(name, - S_IFREG | S_IRUGO, root, - ring, &amdgpu_debugfs_ring_fops); - if (IS_ERR(ent)) - return -ENOMEM; - - i_size_write(ent->d_inode, ring->ring_size + 12); - ring->ent = ent; + debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring, + &amdgpu_debugfs_ring_fops, + ring->ring_size + 12); #endif - return 0; } /** diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h index 88d80eb3fea1..c29fbce0a5b4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h @@ -253,10 +253,6 @@ struct amdgpu_ring { bool has_compute_vm_bug; bool no_scheduler; int hw_prio; - -#if defined(CONFIG_DEBUG_FS) - struct dentry *ent; -#endif }; #define amdgpu_ring_parse_cs(r, p, ib) ((r)->funcs->parse_cs((p), (ib))) @@ -356,8 +352,6 @@ static inline void amdgpu_ring_write_multiple(struct amdgpu_ring *ring, int amdgpu_ring_test_helper(struct amdgpu_ring *ring); -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring); -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring); - #endif -- 2.32.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings 2021-09-02 11:44 ` [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings Nirmoy Das @ 2021-09-03 6:36 ` Sharma, Shashank 2021-09-03 8:09 ` Das, Nirmoy 0 siblings, 1 reply; 16+ messages in thread From: Sharma, Shashank @ 2021-09-03 6:36 UTC (permalink / raw) To: Nirmoy Das, amd-gfx; +Cc: Christian.Koenig On 9/2/2021 5:14 PM, Nirmoy Das wrote: > Use debugfs_create_file_size API for creating ring debugfs > file, also cleanup surrounding code. > > Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 4 +--- > drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 16 +++++----------- > drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 8 +------- > 3 files changed, 7 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > index 077f9baf74fe..dee56ab19a8f 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev) > if (!ring) > continue; > > - if (amdgpu_debugfs_ring_init(adev, ring)) { > - DRM_ERROR("Failed to register debugfs file for rings !\n"); > - } > + amdgpu_debugfs_ring_init(adev, ring); > } > > amdgpu_ras_debugfs_create_all(adev); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c > index f40753e1a60d..968521d80514 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c > @@ -415,26 +415,20 @@ static const struct file_operations amdgpu_debugfs_ring_fops = { > > #endif > > -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, > +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, > struct amdgpu_ring *ring) > { > #if defined(CONFIG_DEBUG_FS) > struct drm_minor *minor = adev_to_drm(adev)->primary; > - struct dentry *ent, *root = minor->debugfs_root; > + struct dentry *root = minor->debugfs_root; > char name[32]; > > sprintf(name, "amdgpu_ring_%s", ring->name); > > - ent = debugfs_create_file(name, > - S_IFREG | S_IRUGO, root, > - ring, &amdgpu_debugfs_ring_fops); > - if (IS_ERR(ent)) > - return -ENOMEM; Why are we doing this ? Why to make it void from int ? - Shashank > - > - i_size_write(ent->d_inode, ring->ring_size + 12); > - ring->ent = ent; > + debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring, > + &amdgpu_debugfs_ring_fops, > + ring->ring_size + 12); > #endif > - return 0; > } > > /** > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h > index 88d80eb3fea1..c29fbce0a5b4 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h > @@ -253,10 +253,6 @@ struct amdgpu_ring { > bool has_compute_vm_bug; > bool no_scheduler; > int hw_prio; > - > -#if defined(CONFIG_DEBUG_FS) > - struct dentry *ent; > -#endif > }; > > #define amdgpu_ring_parse_cs(r, p, ib) ((r)->funcs->parse_cs((p), (ib))) > @@ -356,8 +352,6 @@ static inline void amdgpu_ring_write_multiple(struct amdgpu_ring *ring, > > int amdgpu_ring_test_helper(struct amdgpu_ring *ring); > > -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, > +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, > struct amdgpu_ring *ring); > -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring); > - > #endif > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings 2021-09-03 6:36 ` Sharma, Shashank @ 2021-09-03 8:09 ` Das, Nirmoy 2021-09-03 15:26 ` Sharma, Shashank 0 siblings, 1 reply; 16+ messages in thread From: Das, Nirmoy @ 2021-09-03 8:09 UTC (permalink / raw) To: Sharma, Shashank, amd-gfx; +Cc: Christian.Koenig On 9/3/2021 8:36 AM, Sharma, Shashank wrote: > > > On 9/2/2021 5:14 PM, Nirmoy Das wrote: >> Use debugfs_create_file_size API for creating ring debugfs >> file, also cleanup surrounding code. >> >> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 4 +--- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 16 +++++----------- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 8 +------- >> 3 files changed, 7 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >> index 077f9baf74fe..dee56ab19a8f 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device >> *adev) >> if (!ring) >> continue; >> - if (amdgpu_debugfs_ring_init(adev, ring)) { >> - DRM_ERROR("Failed to register debugfs file for rings !\n"); >> - } >> + amdgpu_debugfs_ring_init(adev, ring); >> } >> amdgpu_ras_debugfs_create_all(adev); >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >> index f40753e1a60d..968521d80514 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >> @@ -415,26 +415,20 @@ static const struct file_operations >> amdgpu_debugfs_ring_fops = { >> #endif >> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >> struct amdgpu_ring *ring) >> { >> #if defined(CONFIG_DEBUG_FS) >> struct drm_minor *minor = adev_to_drm(adev)->primary; >> - struct dentry *ent, *root = minor->debugfs_root; >> + struct dentry *root = minor->debugfs_root; >> char name[32]; >> sprintf(name, "amdgpu_ring_%s", ring->name); >> - ent = debugfs_create_file(name, >> - S_IFREG | S_IRUGO, root, >> - ring, &amdgpu_debugfs_ring_fops); >> - if (IS_ERR(ent)) >> - return -ENOMEM; > > Why are we doing this ? Why to make it void from int ? We tend to ignore debugfs return values as those are not serious errors. This to sync with rest of our debugfs calls. Regards, Nirmoy > > - Shashank > > >> - >> - i_size_write(ent->d_inode, ring->ring_size + 12); >> - ring->ent = ent; >> + debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring, >> + &amdgpu_debugfs_ring_fops, >> + ring->ring_size + 12); >> #endif >> - return 0; >> } >> /** >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >> index 88d80eb3fea1..c29fbce0a5b4 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >> @@ -253,10 +253,6 @@ struct amdgpu_ring { >> bool has_compute_vm_bug; >> bool no_scheduler; >> int hw_prio; >> - >> -#if defined(CONFIG_DEBUG_FS) >> - struct dentry *ent; >> -#endif >> }; >> #define amdgpu_ring_parse_cs(r, p, ib) ((r)->funcs->parse_cs((p), >> (ib))) >> @@ -356,8 +352,6 @@ static inline void >> amdgpu_ring_write_multiple(struct amdgpu_ring *ring, >> int amdgpu_ring_test_helper(struct amdgpu_ring *ring); >> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >> struct amdgpu_ring *ring); >> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring); >> - >> #endif >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings 2021-09-03 8:09 ` Das, Nirmoy @ 2021-09-03 15:26 ` Sharma, Shashank 2021-09-03 15:51 ` Das, Nirmoy 0 siblings, 1 reply; 16+ messages in thread From: Sharma, Shashank @ 2021-09-03 15:26 UTC (permalink / raw) To: Das, Nirmoy, amd-gfx; +Cc: Christian.Koenig On 9/3/2021 1:39 PM, Das, Nirmoy wrote: > > On 9/3/2021 8:36 AM, Sharma, Shashank wrote: >> >> >> On 9/2/2021 5:14 PM, Nirmoy Das wrote: >>> Use debugfs_create_file_size API for creating ring debugfs >>> file, also cleanup surrounding code. >>> >>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 4 +--- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 16 +++++----------- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 8 +------- >>> 3 files changed, 7 insertions(+), 21 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>> index 077f9baf74fe..dee56ab19a8f 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device >>> *adev) >>> if (!ring) >>> continue; >>> - if (amdgpu_debugfs_ring_init(adev, ring)) { >>> - DRM_ERROR("Failed to register debugfs file for rings !\n"); >>> - } >>> + amdgpu_debugfs_ring_init(adev, ring); >>> } >>> amdgpu_ras_debugfs_create_all(adev); >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>> index f40753e1a60d..968521d80514 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>> @@ -415,26 +415,20 @@ static const struct file_operations >>> amdgpu_debugfs_ring_fops = { >>> #endif >>> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>> struct amdgpu_ring *ring) >>> { >>> #if defined(CONFIG_DEBUG_FS) >>> struct drm_minor *minor = adev_to_drm(adev)->primary; >>> - struct dentry *ent, *root = minor->debugfs_root; >>> + struct dentry *root = minor->debugfs_root; >>> char name[32]; >>> sprintf(name, "amdgpu_ring_%s", ring->name); >>> - ent = debugfs_create_file(name, >>> - S_IFREG | S_IRUGO, root, >>> - ring, &amdgpu_debugfs_ring_fops); >>> - if (IS_ERR(ent)) >>> - return -ENOMEM; >> >> Why are we doing this ? Why to make it void from int ? > > > We tend to ignore debugfs return values as those are not serious errors. > This to sync with rest of our > > debugfs calls. > > > Regards, > > Nirmoy > I am not suere if completely removing the provision of return value is a good way of doing it, we can always ignore it at the caller side, isn't it ? - Shashank >> >> - Shashank >> >> >>> - >>> - i_size_write(ent->d_inode, ring->ring_size + 12); >>> - ring->ent = ent; >>> + debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring, >>> + &amdgpu_debugfs_ring_fops, >>> + ring->ring_size + 12); >>> #endif >>> - return 0; >>> } >>> /** >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>> index 88d80eb3fea1..c29fbce0a5b4 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>> @@ -253,10 +253,6 @@ struct amdgpu_ring { >>> bool has_compute_vm_bug; >>> bool no_scheduler; >>> int hw_prio; >>> - >>> -#if defined(CONFIG_DEBUG_FS) >>> - struct dentry *ent; >>> -#endif >>> }; >>> #define amdgpu_ring_parse_cs(r, p, ib) ((r)->funcs->parse_cs((p), >>> (ib))) >>> @@ -356,8 +352,6 @@ static inline void >>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring, >>> int amdgpu_ring_test_helper(struct amdgpu_ring *ring); >>> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>> struct amdgpu_ring *ring); >>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring); >>> - >>> #endif >>> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings 2021-09-03 15:26 ` Sharma, Shashank @ 2021-09-03 15:51 ` Das, Nirmoy 2021-09-03 16:14 ` Das, Nirmoy 0 siblings, 1 reply; 16+ messages in thread From: Das, Nirmoy @ 2021-09-03 15:51 UTC (permalink / raw) To: Sharma, Shashank, amd-gfx; +Cc: Christian.Koenig On 9/3/2021 5:26 PM, Sharma, Shashank wrote: > > > On 9/3/2021 1:39 PM, Das, Nirmoy wrote: >> >> On 9/3/2021 8:36 AM, Sharma, Shashank wrote: >>> >>> >>> On 9/2/2021 5:14 PM, Nirmoy Das wrote: >>>> Use debugfs_create_file_size API for creating ring debugfs >>>> file, also cleanup surrounding code. >>>> >>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> >>>> --- >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 4 +--- >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 16 +++++----------- >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 8 +------- >>>> 3 files changed, 7 insertions(+), 21 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>> index 077f9baf74fe..dee56ab19a8f 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device >>>> *adev) >>>> if (!ring) >>>> continue; >>>> - if (amdgpu_debugfs_ring_init(adev, ring)) { >>>> - DRM_ERROR("Failed to register debugfs file for rings >>>> !\n"); >>>> - } >>>> + amdgpu_debugfs_ring_init(adev, ring); >>>> } >>>> amdgpu_ras_debugfs_create_all(adev); >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>> index f40753e1a60d..968521d80514 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>> @@ -415,26 +415,20 @@ static const struct file_operations >>>> amdgpu_debugfs_ring_fops = { >>>> #endif >>>> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>> struct amdgpu_ring *ring) >>>> { >>>> #if defined(CONFIG_DEBUG_FS) >>>> struct drm_minor *minor = adev_to_drm(adev)->primary; >>>> - struct dentry *ent, *root = minor->debugfs_root; >>>> + struct dentry *root = minor->debugfs_root; >>>> char name[32]; >>>> sprintf(name, "amdgpu_ring_%s", ring->name); >>>> - ent = debugfs_create_file(name, >>>> - S_IFREG | S_IRUGO, root, >>>> - ring, &amdgpu_debugfs_ring_fops); >>>> - if (IS_ERR(ent)) >>>> - return -ENOMEM; >>> >>> Why are we doing this ? Why to make it void from int ? >> >> >> We tend to ignore debugfs return values as those are not serious >> errors. This to sync with rest of our >> >> debugfs calls. >> >> >> Regards, >> >> Nirmoy >> > > > I am not suere if completely removing the provision of return value is > a good way of doing it, we can always ignore it at the caller side, > isn't it ? Yes, we are currently throwing an error msg and ignoring it. I don't have a strong opinion regarding this, I will send a v2 restoring previous behavior. Thanks, Nirmoy > > - Shashank > >>> >>> - Shashank >>> >>> >>>> - >>>> - i_size_write(ent->d_inode, ring->ring_size + 12); >>>> - ring->ent = ent; >>>> + debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring, >>>> + &amdgpu_debugfs_ring_fops, >>>> + ring->ring_size + 12); >>>> #endif >>>> - return 0; >>>> } >>>> /** >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>> index 88d80eb3fea1..c29fbce0a5b4 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>> @@ -253,10 +253,6 @@ struct amdgpu_ring { >>>> bool has_compute_vm_bug; >>>> bool no_scheduler; >>>> int hw_prio; >>>> - >>>> -#if defined(CONFIG_DEBUG_FS) >>>> - struct dentry *ent; >>>> -#endif >>>> }; >>>> #define amdgpu_ring_parse_cs(r, p, ib) >>>> ((r)->funcs->parse_cs((p), (ib))) >>>> @@ -356,8 +352,6 @@ static inline void >>>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring, >>>> int amdgpu_ring_test_helper(struct amdgpu_ring *ring); >>>> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>> struct amdgpu_ring *ring); >>>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring); >>>> - >>>> #endif >>>> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings 2021-09-03 15:51 ` Das, Nirmoy @ 2021-09-03 16:14 ` Das, Nirmoy 2021-09-05 8:03 ` Sharma, Shashank 0 siblings, 1 reply; 16+ messages in thread From: Das, Nirmoy @ 2021-09-03 16:14 UTC (permalink / raw) To: Sharma, Shashank, amd-gfx; +Cc: Christian.Koenig Hi Shashank, On 9/3/2021 5:51 PM, Das, Nirmoy wrote: > > On 9/3/2021 5:26 PM, Sharma, Shashank wrote: >> >> >> On 9/3/2021 1:39 PM, Das, Nirmoy wrote: >>> >>> On 9/3/2021 8:36 AM, Sharma, Shashank wrote: >>>> >>>> >>>> On 9/2/2021 5:14 PM, Nirmoy Das wrote: >>>>> Use debugfs_create_file_size API for creating ring debugfs >>>>> file, also cleanup surrounding code. >>>>> >>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> >>>>> --- >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 4 +--- >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 16 +++++----------- >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 8 +------- >>>>> 3 files changed, 7 insertions(+), 21 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>> index 077f9baf74fe..dee56ab19a8f 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device >>>>> *adev) >>>>> if (!ring) >>>>> continue; >>>>> - if (amdgpu_debugfs_ring_init(adev, ring)) { >>>>> - DRM_ERROR("Failed to register debugfs file for rings >>>>> !\n"); >>>>> - } >>>>> + amdgpu_debugfs_ring_init(adev, ring); >>>>> } >>>>> amdgpu_ras_debugfs_create_all(adev); >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>> index f40753e1a60d..968521d80514 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>> @@ -415,26 +415,20 @@ static const struct file_operations >>>>> amdgpu_debugfs_ring_fops = { >>>>> #endif >>>>> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>> struct amdgpu_ring *ring) >>>>> { >>>>> #if defined(CONFIG_DEBUG_FS) >>>>> struct drm_minor *minor = adev_to_drm(adev)->primary; >>>>> - struct dentry *ent, *root = minor->debugfs_root; >>>>> + struct dentry *root = minor->debugfs_root; >>>>> char name[32]; >>>>> sprintf(name, "amdgpu_ring_%s", ring->name); >>>>> - ent = debugfs_create_file(name, >>>>> - S_IFREG | S_IRUGO, root, >>>>> - ring, &amdgpu_debugfs_ring_fops); >>>>> - if (IS_ERR(ent)) >>>>> - return -ENOMEM; >>>> >>>> Why are we doing this ? Why to make it void from int ? >>> >>> >>> We tend to ignore debugfs return values as those are not serious >>> errors. This to sync with rest of our >>> >>> debugfs calls. >>> >>> >>> Regards, >>> >>> Nirmoy >>> >> >> >> I am not suere if completely removing the provision of return value >> is a good way of doing it, we can always ignore it at the caller >> side, isn't it ? I just realized while making the change debugfs_create_file_size() is void return, so we don't have anything useful to return in amdgpu_debugfs_ring_init() Regards, Nirmoy > > > Yes, we are currently throwing an error msg and ignoring it. I don't > have a strong opinion regarding this, I will send a v2 restoring > previous behavior. > > > Thanks, > > Nirmoy > > >> >> - Shashank >> >>>> >>>> - Shashank >>>> >>>> >>>>> - >>>>> - i_size_write(ent->d_inode, ring->ring_size + 12); >>>>> - ring->ent = ent; >>>>> + debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring, >>>>> + &amdgpu_debugfs_ring_fops, >>>>> + ring->ring_size + 12); >>>>> #endif >>>>> - return 0; >>>>> } >>>>> /** >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>> index 88d80eb3fea1..c29fbce0a5b4 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>> @@ -253,10 +253,6 @@ struct amdgpu_ring { >>>>> bool has_compute_vm_bug; >>>>> bool no_scheduler; >>>>> int hw_prio; >>>>> - >>>>> -#if defined(CONFIG_DEBUG_FS) >>>>> - struct dentry *ent; >>>>> -#endif >>>>> }; >>>>> #define amdgpu_ring_parse_cs(r, p, ib) >>>>> ((r)->funcs->parse_cs((p), (ib))) >>>>> @@ -356,8 +352,6 @@ static inline void >>>>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring, >>>>> int amdgpu_ring_test_helper(struct amdgpu_ring *ring); >>>>> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>> struct amdgpu_ring *ring); >>>>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring); >>>>> - >>>>> #endif >>>>> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings 2021-09-03 16:14 ` Das, Nirmoy @ 2021-09-05 8:03 ` Sharma, Shashank 2021-09-05 11:31 ` Das, Nirmoy 0 siblings, 1 reply; 16+ messages in thread From: Sharma, Shashank @ 2021-09-05 8:03 UTC (permalink / raw) To: Das, Nirmoy, amd-gfx; +Cc: Christian.Koenig On 9/3/2021 9:44 PM, Das, Nirmoy wrote: > Hi Shashank, > > On 9/3/2021 5:51 PM, Das, Nirmoy wrote: >> >> On 9/3/2021 5:26 PM, Sharma, Shashank wrote: >>> >>> >>> On 9/3/2021 1:39 PM, Das, Nirmoy wrote: >>>> >>>> On 9/3/2021 8:36 AM, Sharma, Shashank wrote: >>>>> >>>>> >>>>> On 9/2/2021 5:14 PM, Nirmoy Das wrote: >>>>>> Use debugfs_create_file_size API for creating ring debugfs >>>>>> file, also cleanup surrounding code. >>>>>> >>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> >>>>>> --- >>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 4 +--- >>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 16 +++++----------- >>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 8 +------- >>>>>> 3 files changed, 7 insertions(+), 21 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>> index 077f9baf74fe..dee56ab19a8f 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct amdgpu_device >>>>>> *adev) >>>>>> if (!ring) >>>>>> continue; >>>>>> - if (amdgpu_debugfs_ring_init(adev, ring)) { >>>>>> - DRM_ERROR("Failed to register debugfs file for rings >>>>>> !\n"); >>>>>> - } >>>>>> + amdgpu_debugfs_ring_init(adev, ring); >>>>>> } >>>>>> amdgpu_ras_debugfs_create_all(adev); >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>> index f40753e1a60d..968521d80514 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>> @@ -415,26 +415,20 @@ static const struct file_operations >>>>>> amdgpu_debugfs_ring_fops = { >>>>>> #endif >>>>>> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>> struct amdgpu_ring *ring) >>>>>> { >>>>>> #if defined(CONFIG_DEBUG_FS) >>>>>> struct drm_minor *minor = adev_to_drm(adev)->primary; >>>>>> - struct dentry *ent, *root = minor->debugfs_root; >>>>>> + struct dentry *root = minor->debugfs_root; >>>>>> char name[32]; >>>>>> sprintf(name, "amdgpu_ring_%s", ring->name); >>>>>> - ent = debugfs_create_file(name, >>>>>> - S_IFREG | S_IRUGO, root, >>>>>> - ring, &amdgpu_debugfs_ring_fops); >>>>>> - if (IS_ERR(ent)) >>>>>> - return -ENOMEM; >>>>> >>>>> Why are we doing this ? Why to make it void from int ? >>>> >>>> >>>> We tend to ignore debugfs return values as those are not serious >>>> errors. This to sync with rest of our >>>> >>>> debugfs calls. >>>> >>>> >>>> Regards, >>>> >>>> Nirmoy >>>> >>> >>> >>> I am not suere if completely removing the provision of return value >>> is a good way of doing it, we can always ignore it at the caller >>> side, isn't it ? > > > > I just realized while making the change debugfs_create_file_size() is > void return, so we don't have anything useful to return in > amdgpu_debugfs_ring_init() > > Ah, it makes better sense now. Probably just a mention in the body of the message that we are moving from debugfs_create_file() to debugfs_create_file_size(), will make this change of return type more logical. - Shashank > Regards, > > Nirmoy > > >> >> >> Yes, we are currently throwing an error msg and ignoring it. I don't >> have a strong opinion regarding this, I will send a v2 restoring >> previous behavior. >> >> >> Thanks, >> >> Nirmoy >> >> >>> >>> - Shashank >>> >>>>> >>>>> - Shashank >>>>> >>>>> >>>>>> - >>>>>> - i_size_write(ent->d_inode, ring->ring_size + 12); >>>>>> - ring->ent = ent; >>>>>> + debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring, >>>>>> + &amdgpu_debugfs_ring_fops, >>>>>> + ring->ring_size + 12); >>>>>> #endif >>>>>> - return 0; >>>>>> } >>>>>> /** >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>> index 88d80eb3fea1..c29fbce0a5b4 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>> @@ -253,10 +253,6 @@ struct amdgpu_ring { >>>>>> bool has_compute_vm_bug; >>>>>> bool no_scheduler; >>>>>> int hw_prio; >>>>>> - >>>>>> -#if defined(CONFIG_DEBUG_FS) >>>>>> - struct dentry *ent; >>>>>> -#endif >>>>>> }; >>>>>> #define amdgpu_ring_parse_cs(r, p, ib) >>>>>> ((r)->funcs->parse_cs((p), (ib))) >>>>>> @@ -356,8 +352,6 @@ static inline void >>>>>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring, >>>>>> int amdgpu_ring_test_helper(struct amdgpu_ring *ring); >>>>>> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>> struct amdgpu_ring *ring); >>>>>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring); >>>>>> - >>>>>> #endif >>>>>> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings 2021-09-05 8:03 ` Sharma, Shashank @ 2021-09-05 11:31 ` Das, Nirmoy 2021-09-06 16:45 ` Sharma, Shashank 0 siblings, 1 reply; 16+ messages in thread From: Das, Nirmoy @ 2021-09-05 11:31 UTC (permalink / raw) To: Sharma, Shashank, amd-gfx; +Cc: Christian.Koenig On 9/5/2021 10:03 AM, Sharma, Shashank wrote: > > > On 9/3/2021 9:44 PM, Das, Nirmoy wrote: >> Hi Shashank, >> >> On 9/3/2021 5:51 PM, Das, Nirmoy wrote: >>> >>> On 9/3/2021 5:26 PM, Sharma, Shashank wrote: >>>> >>>> >>>> On 9/3/2021 1:39 PM, Das, Nirmoy wrote: >>>>> >>>>> On 9/3/2021 8:36 AM, Sharma, Shashank wrote: >>>>>> >>>>>> >>>>>> On 9/2/2021 5:14 PM, Nirmoy Das wrote: >>>>>>> Use debugfs_create_file_size API for creating ring debugfs >>>>>>> file, also cleanup surrounding code. >>>>>>> >>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 4 +--- >>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 16 +++++----------- >>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 8 +------- >>>>>>> 3 files changed, 7 insertions(+), 21 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>>> index 077f9baf74fe..dee56ab19a8f 100644 >>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct >>>>>>> amdgpu_device *adev) >>>>>>> if (!ring) >>>>>>> continue; >>>>>>> - if (amdgpu_debugfs_ring_init(adev, ring)) { >>>>>>> - DRM_ERROR("Failed to register debugfs file for >>>>>>> rings !\n"); >>>>>>> - } >>>>>>> + amdgpu_debugfs_ring_init(adev, ring); >>>>>>> } >>>>>>> amdgpu_ras_debugfs_create_all(adev); >>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>>> index f40753e1a60d..968521d80514 100644 >>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>>> @@ -415,26 +415,20 @@ static const struct file_operations >>>>>>> amdgpu_debugfs_ring_fops = { >>>>>>> #endif >>>>>>> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>>> struct amdgpu_ring *ring) >>>>>>> { >>>>>>> #if defined(CONFIG_DEBUG_FS) >>>>>>> struct drm_minor *minor = adev_to_drm(adev)->primary; >>>>>>> - struct dentry *ent, *root = minor->debugfs_root; >>>>>>> + struct dentry *root = minor->debugfs_root; >>>>>>> char name[32]; >>>>>>> sprintf(name, "amdgpu_ring_%s", ring->name); >>>>>>> - ent = debugfs_create_file(name, >>>>>>> - S_IFREG | S_IRUGO, root, >>>>>>> - ring, &amdgpu_debugfs_ring_fops); >>>>>>> - if (IS_ERR(ent)) >>>>>>> - return -ENOMEM; >>>>>> >>>>>> Why are we doing this ? Why to make it void from int ? >>>>> >>>>> >>>>> We tend to ignore debugfs return values as those are not serious >>>>> errors. This to sync with rest of our >>>>> >>>>> debugfs calls. >>>>> >>>>> >>>>> Regards, >>>>> >>>>> Nirmoy >>>>> >>>> >>>> >>>> I am not suere if completely removing the provision of return value >>>> is a good way of doing it, we can always ignore it at the caller >>>> side, isn't it ? >> >> >> >> I just realized while making the change debugfs_create_file_size() is >> void return, so we don't have anything useful to return in >> amdgpu_debugfs_ring_init() >> >> > > Ah, it makes better sense now. Probably just a mention in the body of > the message that we are moving from debugfs_create_file() to > debugfs_create_file_size(), will make this change of return type more > logical. Yes, I have that "Use debugfs_create_file_size API for creating ring debugfs file,..." Nirmoy > > - Shashank > >> Regards, >> >> Nirmoy >> >> >>> >>> >>> Yes, we are currently throwing an error msg and ignoring it. I don't >>> have a strong opinion regarding this, I will send a v2 restoring >>> previous behavior. >>> >>> >>> Thanks, >>> >>> Nirmoy >>> >>> >>>> >>>> - Shashank >>>> >>>>>> >>>>>> - Shashank >>>>>> >>>>>> >>>>>>> - >>>>>>> - i_size_write(ent->d_inode, ring->ring_size + 12); >>>>>>> - ring->ent = ent; >>>>>>> + debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring, >>>>>>> + &amdgpu_debugfs_ring_fops, >>>>>>> + ring->ring_size + 12); >>>>>>> #endif >>>>>>> - return 0; >>>>>>> } >>>>>>> /** >>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>>> index 88d80eb3fea1..c29fbce0a5b4 100644 >>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>>> @@ -253,10 +253,6 @@ struct amdgpu_ring { >>>>>>> bool has_compute_vm_bug; >>>>>>> bool no_scheduler; >>>>>>> int hw_prio; >>>>>>> - >>>>>>> -#if defined(CONFIG_DEBUG_FS) >>>>>>> - struct dentry *ent; >>>>>>> -#endif >>>>>>> }; >>>>>>> #define amdgpu_ring_parse_cs(r, p, ib) >>>>>>> ((r)->funcs->parse_cs((p), (ib))) >>>>>>> @@ -356,8 +352,6 @@ static inline void >>>>>>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring, >>>>>>> int amdgpu_ring_test_helper(struct amdgpu_ring *ring); >>>>>>> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>>> struct amdgpu_ring *ring); >>>>>>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring); >>>>>>> - >>>>>>> #endif >>>>>>> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings 2021-09-05 11:31 ` Das, Nirmoy @ 2021-09-06 16:45 ` Sharma, Shashank 2021-09-08 8:54 ` Das, Nirmoy 0 siblings, 1 reply; 16+ messages in thread From: Sharma, Shashank @ 2021-09-06 16:45 UTC (permalink / raw) To: Das, Nirmoy, amd-gfx; +Cc: Christian.Koenig On 9/5/2021 5:01 PM, Das, Nirmoy wrote: > > On 9/5/2021 10:03 AM, Sharma, Shashank wrote: >> >> >> On 9/3/2021 9:44 PM, Das, Nirmoy wrote: >>> Hi Shashank, >>> >>> On 9/3/2021 5:51 PM, Das, Nirmoy wrote: >>>> >>>> On 9/3/2021 5:26 PM, Sharma, Shashank wrote: >>>>> >>>>> >>>>> On 9/3/2021 1:39 PM, Das, Nirmoy wrote: >>>>>> >>>>>> On 9/3/2021 8:36 AM, Sharma, Shashank wrote: >>>>>>> >>>>>>> >>>>>>> On 9/2/2021 5:14 PM, Nirmoy Das wrote: >>>>>>>> Use debugfs_create_file_size API for creating ring debugfs >>>>>>>> file, also cleanup surrounding code. >>>>>>>> >>>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 4 +--- >>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 16 +++++----------- >>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 8 +------- >>>>>>>> 3 files changed, 7 insertions(+), 21 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>>>> index 077f9baf74fe..dee56ab19a8f 100644 >>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>>>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct >>>>>>>> amdgpu_device *adev) >>>>>>>> if (!ring) >>>>>>>> continue; >>>>>>>> - if (amdgpu_debugfs_ring_init(adev, ring)) { >>>>>>>> - DRM_ERROR("Failed to register debugfs file for >>>>>>>> rings !\n"); >>>>>>>> - } >>>>>>>> + amdgpu_debugfs_ring_init(adev, ring); >>>>>>>> } >>>>>>>> amdgpu_ras_debugfs_create_all(adev); >>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>>>> index f40753e1a60d..968521d80514 100644 >>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>>>> @@ -415,26 +415,20 @@ static const struct file_operations >>>>>>>> amdgpu_debugfs_ring_fops = { >>>>>>>> #endif >>>>>>>> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>>>> struct amdgpu_ring *ring) >>>>>>>> { >>>>>>>> #if defined(CONFIG_DEBUG_FS) >>>>>>>> struct drm_minor *minor = adev_to_drm(adev)->primary; >>>>>>>> - struct dentry *ent, *root = minor->debugfs_root; >>>>>>>> + struct dentry *root = minor->debugfs_root; >>>>>>>> char name[32]; >>>>>>>> sprintf(name, "amdgpu_ring_%s", ring->name); >>>>>>>> - ent = debugfs_create_file(name, >>>>>>>> - S_IFREG | S_IRUGO, root, >>>>>>>> - ring, &amdgpu_debugfs_ring_fops); >>>>>>>> - if (IS_ERR(ent)) >>>>>>>> - return -ENOMEM; >>>>>>> >>>>>>> Why are we doing this ? Why to make it void from int ? >>>>>> >>>>>> >>>>>> We tend to ignore debugfs return values as those are not serious >>>>>> errors. This to sync with rest of our >>>>>> >>>>>> debugfs calls. >>>>>> >>>>>> >>>>>> Regards, >>>>>> >>>>>> Nirmoy >>>>>> >>>>> >>>>> >>>>> I am not suere if completely removing the provision of return value >>>>> is a good way of doing it, we can always ignore it at the caller >>>>> side, isn't it ? >>> >>> >>> >>> I just realized while making the change debugfs_create_file_size() is >>> void return, so we don't have anything useful to return in >>> amdgpu_debugfs_ring_init() >>> >>> >> >> Ah, it makes better sense now. Probably just a mention in the body of >> the message that we are moving from debugfs_create_file() to >> debugfs_create_file_size(), will make this change of return type more >> logical. > > > Yes, I have that "Use debugfs_create_file_size API for creating ring > debugfs file,..." > > My bad, I was too focused (and a bit confused due to uasge of clean-up) around the code change. Suggestion for message: Use debugfs_create_file_size API for creating ring debugfs, and as its a NULL returning API, change the return type for amdgpu_debugfs_ring_init API as well. With (or even without) this change, please feel free to use: Reviewed-by: Shashank Sharma <shashank.sharma@amd.com> - Shashank > Nirmoy > >> >> - Shashank >> >>> Regards, >>> >>> Nirmoy >>> >>> >>>> >>>> >>>> Yes, we are currently throwing an error msg and ignoring it. I don't >>>> have a strong opinion regarding this, I will send a v2 restoring >>>> previous behavior. >>>> >>>> >>>> Thanks, >>>> >>>> Nirmoy >>>> >>>> >>>>> >>>>> - Shashank >>>>> >>>>>>> >>>>>>> - Shashank >>>>>>> >>>>>>> >>>>>>>> - >>>>>>>> - i_size_write(ent->d_inode, ring->ring_size + 12); >>>>>>>> - ring->ent = ent; >>>>>>>> + debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, ring, >>>>>>>> + &amdgpu_debugfs_ring_fops, >>>>>>>> + ring->ring_size + 12); >>>>>>>> #endif >>>>>>>> - return 0; >>>>>>>> } >>>>>>>> /** >>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>>>> index 88d80eb3fea1..c29fbce0a5b4 100644 >>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>>>> @@ -253,10 +253,6 @@ struct amdgpu_ring { >>>>>>>> bool has_compute_vm_bug; >>>>>>>> bool no_scheduler; >>>>>>>> int hw_prio; >>>>>>>> - >>>>>>>> -#if defined(CONFIG_DEBUG_FS) >>>>>>>> - struct dentry *ent; >>>>>>>> -#endif >>>>>>>> }; >>>>>>>> #define amdgpu_ring_parse_cs(r, p, ib) >>>>>>>> ((r)->funcs->parse_cs((p), (ib))) >>>>>>>> @@ -356,8 +352,6 @@ static inline void >>>>>>>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring, >>>>>>>> int amdgpu_ring_test_helper(struct amdgpu_ring *ring); >>>>>>>> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>>>> struct amdgpu_ring *ring); >>>>>>>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring); >>>>>>>> - >>>>>>>> #endif >>>>>>>> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings 2021-09-06 16:45 ` Sharma, Shashank @ 2021-09-08 8:54 ` Das, Nirmoy 0 siblings, 0 replies; 16+ messages in thread From: Das, Nirmoy @ 2021-09-08 8:54 UTC (permalink / raw) To: Sharma, Shashank, amd-gfx; +Cc: Christian.Koenig On 9/6/2021 6:45 PM, Sharma, Shashank wrote: > > > On 9/5/2021 5:01 PM, Das, Nirmoy wrote: >> >> On 9/5/2021 10:03 AM, Sharma, Shashank wrote: >>> >>> >>> On 9/3/2021 9:44 PM, Das, Nirmoy wrote: >>>> Hi Shashank, >>>> >>>> On 9/3/2021 5:51 PM, Das, Nirmoy wrote: >>>>> >>>>> On 9/3/2021 5:26 PM, Sharma, Shashank wrote: >>>>>> >>>>>> >>>>>> On 9/3/2021 1:39 PM, Das, Nirmoy wrote: >>>>>>> >>>>>>> On 9/3/2021 8:36 AM, Sharma, Shashank wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 9/2/2021 5:14 PM, Nirmoy Das wrote: >>>>>>>>> Use debugfs_create_file_size API for creating ring debugfs >>>>>>>>> file, also cleanup surrounding code. >>>>>>>>> >>>>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> >>>>>>>>> --- >>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 4 +--- >>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 16 >>>>>>>>> +++++----------- >>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 8 +------- >>>>>>>>> 3 files changed, 7 insertions(+), 21 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>>>>> index 077f9baf74fe..dee56ab19a8f 100644 >>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>>>>>> @@ -1734,9 +1734,7 @@ int amdgpu_debugfs_init(struct >>>>>>>>> amdgpu_device *adev) >>>>>>>>> if (!ring) >>>>>>>>> continue; >>>>>>>>> - if (amdgpu_debugfs_ring_init(adev, ring)) { >>>>>>>>> - DRM_ERROR("Failed to register debugfs file for >>>>>>>>> rings !\n"); >>>>>>>>> - } >>>>>>>>> + amdgpu_debugfs_ring_init(adev, ring); >>>>>>>>> } >>>>>>>>> amdgpu_ras_debugfs_create_all(adev); >>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>>>>> index f40753e1a60d..968521d80514 100644 >>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>>>>>>> @@ -415,26 +415,20 @@ static const struct file_operations >>>>>>>>> amdgpu_debugfs_ring_fops = { >>>>>>>>> #endif >>>>>>>>> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>>>>> struct amdgpu_ring *ring) >>>>>>>>> { >>>>>>>>> #if defined(CONFIG_DEBUG_FS) >>>>>>>>> struct drm_minor *minor = adev_to_drm(adev)->primary; >>>>>>>>> - struct dentry *ent, *root = minor->debugfs_root; >>>>>>>>> + struct dentry *root = minor->debugfs_root; >>>>>>>>> char name[32]; >>>>>>>>> sprintf(name, "amdgpu_ring_%s", ring->name); >>>>>>>>> - ent = debugfs_create_file(name, >>>>>>>>> - S_IFREG | S_IRUGO, root, >>>>>>>>> - ring, &amdgpu_debugfs_ring_fops); >>>>>>>>> - if (IS_ERR(ent)) >>>>>>>>> - return -ENOMEM; >>>>>>>> >>>>>>>> Why are we doing this ? Why to make it void from int ? >>>>>>> >>>>>>> >>>>>>> We tend to ignore debugfs return values as those are not serious >>>>>>> errors. This to sync with rest of our >>>>>>> >>>>>>> debugfs calls. >>>>>>> >>>>>>> >>>>>>> Regards, >>>>>>> >>>>>>> Nirmoy >>>>>>> >>>>>> >>>>>> >>>>>> I am not suere if completely removing the provision of return >>>>>> value is a good way of doing it, we can always ignore it at the >>>>>> caller side, isn't it ? >>>> >>>> >>>> >>>> I just realized while making the change debugfs_create_file_size() >>>> is void return, so we don't have anything useful to return in >>>> amdgpu_debugfs_ring_init() >>>> >>>> >>> >>> Ah, it makes better sense now. Probably just a mention in the body >>> of the message that we are moving from debugfs_create_file() to >>> debugfs_create_file_size(), will make this change of return type >>> more logical. >> >> >> Yes, I have that "Use debugfs_create_file_size API for creating ring >> debugfs file,..." >> >> > > My bad, I was too focused (and a bit confused due to uasge of > clean-up) around the code change. > > Suggestion for message: Use debugfs_create_file_size API for creating > ring debugfs, and as its a NULL returning API, change the return type > for amdgpu_debugfs_ring_init API as well. Thanks Shashank, I pushed the change with your suggested commit message. Nirmoy > > With (or even without) this change, please feel free to use: > > Reviewed-by: Shashank Sharma <shashank.sharma@amd.com> > > - Shashank > >> Nirmoy >> >>> >>> - Shashank >>> >>>> Regards, >>>> >>>> Nirmoy >>>> >>>> >>>>> >>>>> >>>>> Yes, we are currently throwing an error msg and ignoring it. I >>>>> don't have a strong opinion regarding this, I will send a v2 >>>>> restoring previous behavior. >>>>> >>>>> >>>>> Thanks, >>>>> >>>>> Nirmoy >>>>> >>>>> >>>>>> >>>>>> - Shashank >>>>>> >>>>>>>> >>>>>>>> - Shashank >>>>>>>> >>>>>>>> >>>>>>>>> - >>>>>>>>> - i_size_write(ent->d_inode, ring->ring_size + 12); >>>>>>>>> - ring->ent = ent; >>>>>>>>> + debugfs_create_file_size(name, S_IFREG | S_IRUGO, root, >>>>>>>>> ring, >>>>>>>>> + &amdgpu_debugfs_ring_fops, >>>>>>>>> + ring->ring_size + 12); >>>>>>>>> #endif >>>>>>>>> - return 0; >>>>>>>>> } >>>>>>>>> /** >>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>>>>> index 88d80eb3fea1..c29fbce0a5b4 100644 >>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h >>>>>>>>> @@ -253,10 +253,6 @@ struct amdgpu_ring { >>>>>>>>> bool has_compute_vm_bug; >>>>>>>>> bool no_scheduler; >>>>>>>>> int hw_prio; >>>>>>>>> - >>>>>>>>> -#if defined(CONFIG_DEBUG_FS) >>>>>>>>> - struct dentry *ent; >>>>>>>>> -#endif >>>>>>>>> }; >>>>>>>>> #define amdgpu_ring_parse_cs(r, p, ib) >>>>>>>>> ((r)->funcs->parse_cs((p), (ib))) >>>>>>>>> @@ -356,8 +352,6 @@ static inline void >>>>>>>>> amdgpu_ring_write_multiple(struct amdgpu_ring *ring, >>>>>>>>> int amdgpu_ring_test_helper(struct amdgpu_ring *ring); >>>>>>>>> -int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>>>>> +void amdgpu_debugfs_ring_init(struct amdgpu_device *adev, >>>>>>>>> struct amdgpu_ring *ring); >>>>>>>>> -void amdgpu_debugfs_ring_fini(struct amdgpu_ring *ring); >>>>>>>>> - >>>>>>>>> #endif >>>>>>>>> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs 2021-09-02 11:44 [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs Nirmoy Das 2021-09-02 11:44 ` [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings Nirmoy Das @ 2021-09-02 15:23 ` Sharma, Shashank 2021-09-03 7:53 ` Christian König 2 siblings, 0 replies; 16+ messages in thread From: Sharma, Shashank @ 2021-09-02 15:23 UTC (permalink / raw) To: Nirmoy Das, amd-gfx; +Cc: Christian.Koenig On 9/2/2021 5:14 PM, Nirmoy Das wrote: > debugfs APIs returns encoded error so use > IS_ERR for checking return value. > > References: https://gitlab.freedesktop.org/drm/amd/-/issues/1686 > Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 ++---- > drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 2 +- > 2 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > index d256215ab2c7..077f9baf74fe 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > @@ -1696,18 +1696,16 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev) > struct dentry *ent; > int r, i; > > - > - > ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev, > &fops_ib_preempt); > - if (!ent) { > + if (IS_ERR(ent)) { > DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs file\n"); > return -EIO; > } > > ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, adev, > &fops_sclk_set); > - if (!ent) { > + if (IS_ERR(ent)) { > DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n"); > return -EIO; > } > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c > index 7b634a1517f9..f40753e1a60d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c > @@ -428,7 +428,7 @@ int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, > ent = debugfs_create_file(name, > S_IFREG | S_IRUGO, root, > ring, &amdgpu_debugfs_ring_fops); > - if (!ent) > + if (IS_ERR(ent)) > return -ENOMEM; > > i_size_write(ent->d_inode, ring->ring_size + 12); > ACK: Shashank Sharma <shashank.sharma@amd.com> Regards Shashank ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs 2021-09-02 11:44 [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs Nirmoy Das 2021-09-02 11:44 ` [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings Nirmoy Das 2021-09-02 15:23 ` [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs Sharma, Shashank @ 2021-09-03 7:53 ` Christian König 2021-09-03 8:13 ` Das, Nirmoy 2 siblings, 1 reply; 16+ messages in thread From: Christian König @ 2021-09-03 7:53 UTC (permalink / raw) To: Nirmoy Das, amd-gfx Am 02.09.21 um 13:44 schrieb Nirmoy Das: > debugfs APIs returns encoded error so use > IS_ERR for checking return value. > > References: https://gitlab.freedesktop.org/drm/amd/-/issues/1686 > Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 ++---- > drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 2 +- > 2 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > index d256215ab2c7..077f9baf74fe 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > @@ -1696,18 +1696,16 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev) > struct dentry *ent; > int r, i; > > - > - > ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev, > &fops_ib_preempt); > - if (!ent) { > + if (IS_ERR(ent)) { > DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs file\n"); > return -EIO; > } > > ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, adev, > &fops_sclk_set); > - if (!ent) { > + if (IS_ERR(ent)) { > DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n"); > return -EIO; > } > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c > index 7b634a1517f9..f40753e1a60d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c > @@ -428,7 +428,7 @@ int amdgpu_debugfs_ring_init(struct amdgpu_device *adev, > ent = debugfs_create_file(name, > S_IFREG | S_IRUGO, root, > ring, &amdgpu_debugfs_ring_fops); > - if (!ent) > + if (IS_ERR(ent)) > return -ENOMEM; > > i_size_write(ent->d_inode, ring->ring_size + 12); ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs 2021-09-03 7:53 ` Christian König @ 2021-09-03 8:13 ` Das, Nirmoy 2021-09-03 8:17 ` Christian König 0 siblings, 1 reply; 16+ messages in thread From: Das, Nirmoy @ 2021-09-03 8:13 UTC (permalink / raw) To: Christian König, amd-gfx; +Cc: Sharma, Shashank Hi Christian and Shashank, Please review the v2 : https://patchwork.freedesktop.org/patch/452175/ In v2, I am returning "PTR_ERR(ent)" instead of -EIO which I think makes more sense. Regards, Nirmoy On 9/3/2021 9:53 AM, Christian König wrote: > Am 02.09.21 um 13:44 schrieb Nirmoy Das: >> debugfs APIs returns encoded error so use >> IS_ERR for checking return value. >> >> References: https://gitlab.freedesktop.org/drm/amd/-/issues/1686 >> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> > > Reviewed-by: Christian König <christian.koenig@amd.com> > >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 ++---- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 2 +- >> 2 files changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >> index d256215ab2c7..077f9baf74fe 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >> @@ -1696,18 +1696,16 @@ int amdgpu_debugfs_init(struct amdgpu_device >> *adev) >> struct dentry *ent; >> int r, i; >> - >> - >> ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev, >> &fops_ib_preempt); >> - if (!ent) { >> + if (IS_ERR(ent)) { >> DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs >> file\n"); >> return -EIO; >> } >> ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, adev, >> &fops_sclk_set); >> - if (!ent) { >> + if (IS_ERR(ent)) { >> DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n"); >> return -EIO; >> } >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >> index 7b634a1517f9..f40753e1a60d 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >> @@ -428,7 +428,7 @@ int amdgpu_debugfs_ring_init(struct amdgpu_device >> *adev, >> ent = debugfs_create_file(name, >> S_IFREG | S_IRUGO, root, >> ring, &amdgpu_debugfs_ring_fops); >> - if (!ent) >> + if (IS_ERR(ent)) >> return -ENOMEM; >> i_size_write(ent->d_inode, ring->ring_size + 12); > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs 2021-09-03 8:13 ` Das, Nirmoy @ 2021-09-03 8:17 ` Christian König 2021-09-03 15:28 ` Sharma, Shashank 0 siblings, 1 reply; 16+ messages in thread From: Christian König @ 2021-09-03 8:17 UTC (permalink / raw) To: Das, Nirmoy, amd-gfx; +Cc: Sharma, Shashank Yeah, sounds logical to me as well. Feel free to stick my rb on that as well. Christian. Am 03.09.21 um 10:13 schrieb Das, Nirmoy: > Hi Christian and Shashank, > > > Please review the v2 : https://patchwork.freedesktop.org/patch/452175/ > > In v2, I am returning "PTR_ERR(ent)" instead of -EIO which I think > makes more sense. > > Regards, > Nirmoy > > On 9/3/2021 9:53 AM, Christian König wrote: >> Am 02.09.21 um 13:44 schrieb Nirmoy Das: >>> debugfs APIs returns encoded error so use >>> IS_ERR for checking return value. >>> >>> References: https://gitlab.freedesktop.org/drm/amd/-/issues/1686 >>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> >> >> Reviewed-by: Christian König <christian.koenig@amd.com> >> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 ++---- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 2 +- >>> 2 files changed, 3 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>> index d256215ab2c7..077f9baf74fe 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>> @@ -1696,18 +1696,16 @@ int amdgpu_debugfs_init(struct amdgpu_device >>> *adev) >>> struct dentry *ent; >>> int r, i; >>> - >>> - >>> ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev, >>> &fops_ib_preempt); >>> - if (!ent) { >>> + if (IS_ERR(ent)) { >>> DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs >>> file\n"); >>> return -EIO; >>> } >>> ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, >>> adev, >>> &fops_sclk_set); >>> - if (!ent) { >>> + if (IS_ERR(ent)) { >>> DRM_ERROR("unable to create amdgpu_set_sclk debugsfs >>> file\n"); >>> return -EIO; >>> } >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>> index 7b634a1517f9..f40753e1a60d 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>> @@ -428,7 +428,7 @@ int amdgpu_debugfs_ring_init(struct >>> amdgpu_device *adev, >>> ent = debugfs_create_file(name, >>> S_IFREG | S_IRUGO, root, >>> ring, &amdgpu_debugfs_ring_fops); >>> - if (!ent) >>> + if (IS_ERR(ent)) >>> return -ENOMEM; >>> i_size_write(ent->d_inode, ring->ring_size + 12); >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs 2021-09-03 8:17 ` Christian König @ 2021-09-03 15:28 ` Sharma, Shashank 0 siblings, 0 replies; 16+ messages in thread From: Sharma, Shashank @ 2021-09-03 15:28 UTC (permalink / raw) To: Christian König, Das, Nirmoy, amd-gfx On 9/3/2021 1:47 PM, Christian König wrote: > Yeah, sounds logical to me as well. Feel free to stick my rb on that as > well. > > Christian. > > Am 03.09.21 um 10:13 schrieb Das, Nirmoy: >> Hi Christian and Shashank, >> >> >> Please review the v2 : https://patchwork.freedesktop.org/patch/452175/ >> >> In v2, I am returning "PTR_ERR(ent)" instead of -EIO which I think >> makes more sense. Agree, LGTM as well, Reviewed-By: Shashank Sharma <shashank.sharma@amd.com> Regards Shashank >> >> Regards, >> Nirmoy >> >> On 9/3/2021 9:53 AM, Christian König wrote: >>> Am 02.09.21 um 13:44 schrieb Nirmoy Das: >>>> debugfs APIs returns encoded error so use >>>> IS_ERR for checking return value. >>>> >>>> References: https://gitlab.freedesktop.org/drm/amd/-/issues/1686 >>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com> >>> >>> Reviewed-by: Christian König <christian.koenig@amd.com> >>> >>>> --- >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 ++---- >>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 2 +- >>>> 2 files changed, 3 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>> index d256215ab2c7..077f9baf74fe 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>> @@ -1696,18 +1696,16 @@ int amdgpu_debugfs_init(struct amdgpu_device >>>> *adev) >>>> struct dentry *ent; >>>> int r, i; >>>> - >>>> - >>>> ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev, >>>> &fops_ib_preempt); >>>> - if (!ent) { >>>> + if (IS_ERR(ent)) { >>>> DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs >>>> file\n"); >>>> return -EIO; >>>> } >>>> ent = debugfs_create_file("amdgpu_force_sclk", 0200, root, >>>> adev, >>>> &fops_sclk_set); >>>> - if (!ent) { >>>> + if (IS_ERR(ent)) { >>>> DRM_ERROR("unable to create amdgpu_set_sclk debugsfs >>>> file\n"); >>>> return -EIO; >>>> } >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>> index 7b634a1517f9..f40753e1a60d 100644 >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c >>>> @@ -428,7 +428,7 @@ int amdgpu_debugfs_ring_init(struct >>>> amdgpu_device *adev, >>>> ent = debugfs_create_file(name, >>>> S_IFREG | S_IRUGO, root, >>>> ring, &amdgpu_debugfs_ring_fops); >>>> - if (!ent) >>>> + if (IS_ERR(ent)) >>>> return -ENOMEM; >>>> i_size_write(ent->d_inode, ring->ring_size + 12); >>> > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-09-08 8:54 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-02 11:44 [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs Nirmoy Das 2021-09-02 11:44 ` [PATCH 2/2] drm/amdgpu: cleanup debugfs for amdgpu rings Nirmoy Das 2021-09-03 6:36 ` Sharma, Shashank 2021-09-03 8:09 ` Das, Nirmoy 2021-09-03 15:26 ` Sharma, Shashank 2021-09-03 15:51 ` Das, Nirmoy 2021-09-03 16:14 ` Das, Nirmoy 2021-09-05 8:03 ` Sharma, Shashank 2021-09-05 11:31 ` Das, Nirmoy 2021-09-06 16:45 ` Sharma, Shashank 2021-09-08 8:54 ` Das, Nirmoy 2021-09-02 15:23 ` [PATCH 1/2] drm/amdgpu: use IS_ERR for debugfs APIs Sharma, Shashank 2021-09-03 7:53 ` Christian König 2021-09-03 8:13 ` Das, Nirmoy 2021-09-03 8:17 ` Christian König 2021-09-03 15:28 ` Sharma, Shashank
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.