| Message ID | 20230113053416.2175988-1-Jun.Ma2@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/ttm: Check ttm_debugfs_root before creating files under it | expand |
Am 13.01.23 um 06:34 schrieb Ma Jun: > Check the ttm_debugfs_root before creating files under it. > If the ttm_debugfs_root is NULL, all the files created for > ttm/ will be placed in the /sys/kerne/debug/ but not > /sys/kernel/debug/ttm/ Well NAK for upstreaming. Why should ttm_debugfs_root be NULL here? Regards, Christian. > > Signed-off-by: Ma Jun <Jun.Ma2@amd.com> > --- > drivers/gpu/drm/ttm/ttm_device.c | 3 ++- > drivers/gpu/drm/ttm/ttm_pool.c | 10 ++++++---- > drivers/gpu/drm/ttm/ttm_tt.c | 5 +++-- > 3 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c > index e7147e304637..967bc2244df3 100644 > --- a/drivers/gpu/drm/ttm/ttm_device.c > +++ b/drivers/gpu/drm/ttm/ttm_device.c > @@ -105,7 +105,8 @@ static int ttm_global_init(void) > INIT_LIST_HEAD(&glob->device_list); > atomic_set(&glob->bo_count, 0); > > - debugfs_create_atomic_t("buffer_objects", 0444, ttm_debugfs_root, > + if(ttm_debugfs_root) > + debugfs_create_atomic_t("buffer_objects", 0444, ttm_debugfs_root, > &glob->bo_count); > out: > if (ret && ttm_debugfs_root) > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > index 21b61631f73a..d95a65f759df 100644 > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c > @@ -713,10 +713,12 @@ int ttm_pool_mgr_init(unsigned long num_pages) > } > > #ifdef CONFIG_DEBUG_FS > - debugfs_create_file("page_pool", 0444, ttm_debugfs_root, NULL, > - &ttm_pool_debugfs_globals_fops); > - debugfs_create_file("page_pool_shrink", 0400, ttm_debugfs_root, NULL, > - &ttm_pool_debugfs_shrink_fops); > + if(ttm_debugfs_root) { > + debugfs_create_file("page_pool", 0444, ttm_debugfs_root, NULL, > + &ttm_pool_debugfs_globals_fops); > + debugfs_create_file("page_pool_shrink", 0400, ttm_debugfs_root, NULL, > + &ttm_pool_debugfs_shrink_fops); > + } > #endif > > mm_shrinker.count_objects = ttm_pool_shrinker_count; > diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c > index d505603930a7..fec443494ef0 100644 > --- a/drivers/gpu/drm/ttm/ttm_tt.c > +++ b/drivers/gpu/drm/ttm/ttm_tt.c > @@ -394,8 +394,9 @@ DEFINE_SHOW_ATTRIBUTE(ttm_tt_debugfs_shrink); > void ttm_tt_mgr_init(unsigned long num_pages, unsigned long num_dma32_pages) > { > #ifdef CONFIG_DEBUG_FS > - debugfs_create_file("tt_shrink", 0400, ttm_debugfs_root, NULL, > - &ttm_tt_debugfs_shrink_fops); > + if(ttm_debugfs_root) > + debugfs_create_file("tt_shrink", 0400, ttm_debugfs_root, NULL, > + &ttm_tt_debugfs_shrink_fops); > #endif > > if (!ttm_pages_limit)
On 1/13/2023 5:37 PM, Christian König wrote: > Am 13.01.23 um 06:34 schrieb Ma Jun: >> Check the ttm_debugfs_root before creating files under it. >> If the ttm_debugfs_root is NULL, all the files created for >> ttm/ will be placed in the /sys/kerne/debug/ but not >> /sys/kernel/debug/ttm/ > > Well NAK for upstreaming. Why should ttm_debugfs_root be NULL here? > In my case, when the ttm/ removal fails during amdgpu uninstall and then we try to modprobe the amdgpu again, the ttm_debugfs_root will be NULL because the ttm/ already exists. Regards, Ma Jun > Regards, > Christian. > >> >> Signed-off-by: Ma Jun <Jun.Ma2@amd.com> >> --- >> drivers/gpu/drm/ttm/ttm_device.c | 3 ++- >> drivers/gpu/drm/ttm/ttm_pool.c | 10 ++++++---- >> drivers/gpu/drm/ttm/ttm_tt.c | 5 +++-- >> 3 files changed, 11 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c >> index e7147e304637..967bc2244df3 100644 >> --- a/drivers/gpu/drm/ttm/ttm_device.c >> +++ b/drivers/gpu/drm/ttm/ttm_device.c >> @@ -105,7 +105,8 @@ static int ttm_global_init(void) >> INIT_LIST_HEAD(&glob->device_list); >> atomic_set(&glob->bo_count, 0); >> >> - debugfs_create_atomic_t("buffer_objects", 0444, ttm_debugfs_root, >> + if(ttm_debugfs_root) >> + debugfs_create_atomic_t("buffer_objects", 0444, ttm_debugfs_root, >> &glob->bo_count); >> out: >> if (ret && ttm_debugfs_root) >> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c >> index 21b61631f73a..d95a65f759df 100644 >> --- a/drivers/gpu/drm/ttm/ttm_pool.c >> +++ b/drivers/gpu/drm/ttm/ttm_pool.c >> @@ -713,10 +713,12 @@ int ttm_pool_mgr_init(unsigned long num_pages) >> } >> >> #ifdef CONFIG_DEBUG_FS >> - debugfs_create_file("page_pool", 0444, ttm_debugfs_root, NULL, >> - &ttm_pool_debugfs_globals_fops); >> - debugfs_create_file("page_pool_shrink", 0400, ttm_debugfs_root, NULL, >> - &ttm_pool_debugfs_shrink_fops); >> + if(ttm_debugfs_root) { >> + debugfs_create_file("page_pool", 0444, ttm_debugfs_root, NULL, >> + &ttm_pool_debugfs_globals_fops); >> + debugfs_create_file("page_pool_shrink", 0400, ttm_debugfs_root, NULL, >> + &ttm_pool_debugfs_shrink_fops); >> + } >> #endif >> >> mm_shrinker.count_objects = ttm_pool_shrinker_count; >> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c >> index d505603930a7..fec443494ef0 100644 >> --- a/drivers/gpu/drm/ttm/ttm_tt.c >> +++ b/drivers/gpu/drm/ttm/ttm_tt.c >> @@ -394,8 +394,9 @@ DEFINE_SHOW_ATTRIBUTE(ttm_tt_debugfs_shrink); >> void ttm_tt_mgr_init(unsigned long num_pages, unsigned long num_dma32_pages) >> { >> #ifdef CONFIG_DEBUG_FS >> - debugfs_create_file("tt_shrink", 0400, ttm_debugfs_root, NULL, >> - &ttm_tt_debugfs_shrink_fops); >> + if(ttm_debugfs_root) >> + debugfs_create_file("tt_shrink", 0400, ttm_debugfs_root, NULL, >> + &ttm_tt_debugfs_shrink_fops); >> #endif >> >> if (!ttm_pages_limit) >
Am 16.01.23 um 03:44 schrieb Ma, Jun: > On 1/13/2023 5:37 PM, Christian König wrote: >> Am 13.01.23 um 06:34 schrieb Ma Jun: >>> Check the ttm_debugfs_root before creating files under it. >>> If the ttm_debugfs_root is NULL, all the files created for >>> ttm/ will be placed in the /sys/kerne/debug/ but not >>> /sys/kernel/debug/ttm/ >> Well NAK for upstreaming. Why should ttm_debugfs_root be NULL here? >> > In my case, when the ttm/ removal fails during amdgpu uninstall and then > we try to modprobe the amdgpu again, the ttm_debugfs_root will be NULL > because the ttm/ already exists. Yeah, but this is just papering over a previous bug which in turn is not a valid argument for a code change. What should happen instead is that the original bug needs to get fixed. Regards, Christian. > > Regards, > Ma Jun > >> Regards, >> Christian. >> >>> Signed-off-by: Ma Jun <Jun.Ma2@amd.com> >>> --- >>> drivers/gpu/drm/ttm/ttm_device.c | 3 ++- >>> drivers/gpu/drm/ttm/ttm_pool.c | 10 ++++++---- >>> drivers/gpu/drm/ttm/ttm_tt.c | 5 +++-- >>> 3 files changed, 11 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c >>> index e7147e304637..967bc2244df3 100644 >>> --- a/drivers/gpu/drm/ttm/ttm_device.c >>> +++ b/drivers/gpu/drm/ttm/ttm_device.c >>> @@ -105,7 +105,8 @@ static int ttm_global_init(void) >>> INIT_LIST_HEAD(&glob->device_list); >>> atomic_set(&glob->bo_count, 0); >>> >>> - debugfs_create_atomic_t("buffer_objects", 0444, ttm_debugfs_root, >>> + if(ttm_debugfs_root) >>> + debugfs_create_atomic_t("buffer_objects", 0444, ttm_debugfs_root, >>> &glob->bo_count); >>> out: >>> if (ret && ttm_debugfs_root) >>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c >>> index 21b61631f73a..d95a65f759df 100644 >>> --- a/drivers/gpu/drm/ttm/ttm_pool.c >>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c >>> @@ -713,10 +713,12 @@ int ttm_pool_mgr_init(unsigned long num_pages) >>> } >>> >>> #ifdef CONFIG_DEBUG_FS >>> - debugfs_create_file("page_pool", 0444, ttm_debugfs_root, NULL, >>> - &ttm_pool_debugfs_globals_fops); >>> - debugfs_create_file("page_pool_shrink", 0400, ttm_debugfs_root, NULL, >>> - &ttm_pool_debugfs_shrink_fops); >>> + if(ttm_debugfs_root) { >>> + debugfs_create_file("page_pool", 0444, ttm_debugfs_root, NULL, >>> + &ttm_pool_debugfs_globals_fops); >>> + debugfs_create_file("page_pool_shrink", 0400, ttm_debugfs_root, NULL, >>> + &ttm_pool_debugfs_shrink_fops); >>> + } >>> #endif >>> >>> mm_shrinker.count_objects = ttm_pool_shrinker_count; >>> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c >>> index d505603930a7..fec443494ef0 100644 >>> --- a/drivers/gpu/drm/ttm/ttm_tt.c >>> +++ b/drivers/gpu/drm/ttm/ttm_tt.c >>> @@ -394,8 +394,9 @@ DEFINE_SHOW_ATTRIBUTE(ttm_tt_debugfs_shrink); >>> void ttm_tt_mgr_init(unsigned long num_pages, unsigned long num_dma32_pages) >>> { >>> #ifdef CONFIG_DEBUG_FS >>> - debugfs_create_file("tt_shrink", 0400, ttm_debugfs_root, NULL, >>> - &ttm_tt_debugfs_shrink_fops); >>> + if(ttm_debugfs_root) >>> + debugfs_create_file("tt_shrink", 0400, ttm_debugfs_root, NULL, >>> + &ttm_tt_debugfs_shrink_fops); >>> #endif >>> >>> if (!ttm_pages_limit)
diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c index e7147e304637..967bc2244df3 100644 --- a/drivers/gpu/drm/ttm/ttm_device.c +++ b/drivers/gpu/drm/ttm/ttm_device.c @@ -105,7 +105,8 @@ static int ttm_global_init(void) INIT_LIST_HEAD(&glob->device_list); atomic_set(&glob->bo_count, 0); - debugfs_create_atomic_t("buffer_objects", 0444, ttm_debugfs_root, + if(ttm_debugfs_root) + debugfs_create_atomic_t("buffer_objects", 0444, ttm_debugfs_root, &glob->bo_count); out: if (ret && ttm_debugfs_root) diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c index 21b61631f73a..d95a65f759df 100644 --- a/drivers/gpu/drm/ttm/ttm_pool.c +++ b/drivers/gpu/drm/ttm/ttm_pool.c @@ -713,10 +713,12 @@ int ttm_pool_mgr_init(unsigned long num_pages) } #ifdef CONFIG_DEBUG_FS - debugfs_create_file("page_pool", 0444, ttm_debugfs_root, NULL, - &ttm_pool_debugfs_globals_fops); - debugfs_create_file("page_pool_shrink", 0400, ttm_debugfs_root, NULL, - &ttm_pool_debugfs_shrink_fops); + if(ttm_debugfs_root) { + debugfs_create_file("page_pool", 0444, ttm_debugfs_root, NULL, + &ttm_pool_debugfs_globals_fops); + debugfs_create_file("page_pool_shrink", 0400, ttm_debugfs_root, NULL, + &ttm_pool_debugfs_shrink_fops); + } #endif mm_shrinker.count_objects = ttm_pool_shrinker_count; diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c index d505603930a7..fec443494ef0 100644 --- a/drivers/gpu/drm/ttm/ttm_tt.c +++ b/drivers/gpu/drm/ttm/ttm_tt.c @@ -394,8 +394,9 @@ DEFINE_SHOW_ATTRIBUTE(ttm_tt_debugfs_shrink); void ttm_tt_mgr_init(unsigned long num_pages, unsigned long num_dma32_pages) { #ifdef CONFIG_DEBUG_FS - debugfs_create_file("tt_shrink", 0400, ttm_debugfs_root, NULL, - &ttm_tt_debugfs_shrink_fops); + if(ttm_debugfs_root) + debugfs_create_file("tt_shrink", 0400, ttm_debugfs_root, NULL, + &ttm_tt_debugfs_shrink_fops); #endif if (!ttm_pages_limit)
Check the ttm_debugfs_root before creating files under it. If the ttm_debugfs_root is NULL, all the files created for ttm/ will be placed in the /sys/kerne/debug/ but not /sys/kernel/debug/ttm/ Signed-off-by: Ma Jun <Jun.Ma2@amd.com> --- drivers/gpu/drm/ttm/ttm_device.c | 3 ++- drivers/gpu/drm/ttm/ttm_pool.c | 10 ++++++---- drivers/gpu/drm/ttm/ttm_tt.c | 5 +++-- 3 files changed, 11 insertions(+), 7 deletions(-)