* [PATCH 1/1] debugfs: use IS_ERR to check for error
@ 2021-09-02 10:29 Nirmoy Das
2021-09-02 10:31 ` Christian König
2021-09-02 10:38 ` Greg KH
0 siblings, 2 replies; 12+ messages in thread
From: Nirmoy Das @ 2021-09-02 10:29 UTC (permalink / raw)
To: rafael; +Cc: linux-kernel, gregkh, Christian.Koenig, Nirmoy Das
debugfs_create_file() 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>
---
fs/debugfs/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 8129a430d789..2f117c57160d 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -528,7 +528,7 @@ void debugfs_create_file_size(const char *name, umode_t mode,
{
struct dentry *de = debugfs_create_file(name, mode, parent, data, fops);
- if (de)
+ if (!IS_ERR(de))
d_inode(de)->i_size = file_size;
}
EXPORT_SYMBOL_GPL(debugfs_create_file_size);
--
2.32.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] debugfs: use IS_ERR to check for error
2021-09-02 10:29 [PATCH 1/1] debugfs: use IS_ERR to check for error Nirmoy Das
@ 2021-09-02 10:31 ` Christian König
2021-09-02 10:38 ` Greg KH
1 sibling, 0 replies; 12+ messages in thread
From: Christian König @ 2021-09-02 10:31 UTC (permalink / raw)
To: Nirmoy Das, rafael; +Cc: linux-kernel, gregkh
Am 02.09.21 um 12:29 schrieb Nirmoy Das:
> debugfs_create_file() 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>
> ---
> fs/debugfs/inode.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 8129a430d789..2f117c57160d 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -528,7 +528,7 @@ void debugfs_create_file_size(const char *name, umode_t mode,
> {
> struct dentry *de = debugfs_create_file(name, mode, parent, data, fops);
>
> - if (de)
> + if (!IS_ERR(de))
> d_inode(de)->i_size = file_size;
> }
> EXPORT_SYMBOL_GPL(debugfs_create_file_size);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] debugfs: use IS_ERR to check for error
2021-09-02 10:29 [PATCH 1/1] debugfs: use IS_ERR to check for error Nirmoy Das
2021-09-02 10:31 ` Christian König
@ 2021-09-02 10:38 ` Greg KH
2021-09-02 12:03 ` Christian König
1 sibling, 1 reply; 12+ messages in thread
From: Greg KH @ 2021-09-02 10:38 UTC (permalink / raw)
To: Nirmoy Das; +Cc: rafael, linux-kernel, Christian.Koenig
On Thu, Sep 02, 2021 at 12:29:17PM +0200, Nirmoy Das wrote:
> debugfs_create_file() 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>
> ---
> fs/debugfs/inode.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 8129a430d789..2f117c57160d 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -528,7 +528,7 @@ void debugfs_create_file_size(const char *name, umode_t mode,
> {
> struct dentry *de = debugfs_create_file(name, mode, parent, data, fops);
>
> - if (de)
> + if (!IS_ERR(de))
> d_inode(de)->i_size = file_size;
> }
> EXPORT_SYMBOL_GPL(debugfs_create_file_size);
> --
> 2.32.0
>
Ah, good catch, I'll queue this up after 5.15-rc1 is out, thanks!
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] debugfs: use IS_ERR to check for error
2021-09-02 10:38 ` Greg KH
@ 2021-09-02 12:03 ` Christian König
2021-09-02 12:20 ` Greg KH
0 siblings, 1 reply; 12+ messages in thread
From: Christian König @ 2021-09-02 12:03 UTC (permalink / raw)
To: Greg KH, Nirmoy Das; +Cc: rafael, linux-kernel
Am 02.09.21 um 12:38 schrieb Greg KH:
> On Thu, Sep 02, 2021 at 12:29:17PM +0200, Nirmoy Das wrote:
>> debugfs_create_file() returns encoded error so
>> use IS_ERR for checking return value.
>>
>> References: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1686&data=04%7C01%7CChristian.Koenig%40amd.com%7C82691bea64d3491fe86008d96dfddc60%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637661759378883940%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Xs4xnihnMzNvl%2BSEEpCcWkdvMaDw1Ofvekn%2Fnvz1mM8%3D&reserved=0
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>> fs/debugfs/inode.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>> index 8129a430d789..2f117c57160d 100644
>> --- a/fs/debugfs/inode.c
>> +++ b/fs/debugfs/inode.c
>> @@ -528,7 +528,7 @@ void debugfs_create_file_size(const char *name, umode_t mode,
>> {
>> struct dentry *de = debugfs_create_file(name, mode, parent, data, fops);
>>
>> - if (de)
>> + if (!IS_ERR(de))
>> d_inode(de)->i_size = file_size;
>> }
>> EXPORT_SYMBOL_GPL(debugfs_create_file_size);
>> --
>> 2.32.0
>>
> Ah, good catch, I'll queue this up after 5.15-rc1 is out, thanks!
Thinking more about this if I'm not completely mistaken
debugfs_create_file() returns -ENODEV when debugfs is disabled and NULL
on any other error.
So this needs to be IS_ERR_OR_NULL().
Regards,
Christian.
>
> greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] debugfs: use IS_ERR to check for error
2021-09-02 12:03 ` Christian König
@ 2021-09-02 12:20 ` Greg KH
2021-09-02 15:10 ` Christian König
0 siblings, 1 reply; 12+ messages in thread
From: Greg KH @ 2021-09-02 12:20 UTC (permalink / raw)
To: Christian König; +Cc: Nirmoy Das, rafael, linux-kernel
On Thu, Sep 02, 2021 at 02:03:12PM +0200, Christian König wrote:
>
>
> Am 02.09.21 um 12:38 schrieb Greg KH:
> > On Thu, Sep 02, 2021 at 12:29:17PM +0200, Nirmoy Das wrote:
> > > debugfs_create_file() returns encoded error so
> > > use IS_ERR for checking return value.
> > >
> > > References: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1686&data=04%7C01%7CChristian.Koenig%40amd.com%7C82691bea64d3491fe86008d96dfddc60%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637661759378883940%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Xs4xnihnMzNvl%2BSEEpCcWkdvMaDw1Ofvekn%2Fnvz1mM8%3D&reserved=0
> > > Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> > > ---
> > > fs/debugfs/inode.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > > index 8129a430d789..2f117c57160d 100644
> > > --- a/fs/debugfs/inode.c
> > > +++ b/fs/debugfs/inode.c
> > > @@ -528,7 +528,7 @@ void debugfs_create_file_size(const char *name, umode_t mode,
> > > {
> > > struct dentry *de = debugfs_create_file(name, mode, parent, data, fops);
> > > - if (de)
> > > + if (!IS_ERR(de))
> > > d_inode(de)->i_size = file_size;
> > > }
> > > EXPORT_SYMBOL_GPL(debugfs_create_file_size);
> > > --
> > > 2.32.0
> > >
> > Ah, good catch, I'll queue this up after 5.15-rc1 is out, thanks!
>
> Thinking more about this if I'm not completely mistaken
> debugfs_create_file() returns -ENODEV when debugfs is disabled and NULL on
> any other error.
How can this function be called if debugfs is not enabled in the system
configuration? This _is_ the debugfs core code.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] debugfs: use IS_ERR to check for error
2021-09-02 12:20 ` Greg KH
@ 2021-09-02 15:10 ` Christian König
2021-09-02 16:34 ` Greg KH
0 siblings, 1 reply; 12+ messages in thread
From: Christian König @ 2021-09-02 15:10 UTC (permalink / raw)
To: Greg KH; +Cc: Nirmoy Das, rafael, linux-kernel
Am 02.09.21 um 14:20 schrieb Greg KH:
> On Thu, Sep 02, 2021 at 02:03:12PM +0200, Christian König wrote:
>>
>> Am 02.09.21 um 12:38 schrieb Greg KH:
>>> On Thu, Sep 02, 2021 at 12:29:17PM +0200, Nirmoy Das wrote:
>>>> debugfs_create_file() returns encoded error so
>>>> use IS_ERR for checking return value.
>>>>
>>>> References: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1686&data=04%7C01%7Cchristian.koenig%40amd.com%7Cffc1109aeb744082181b08d96e0c06db%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637661820207117289%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=6NDHX9sEhGF2jakVfUJ7Qurql6UAdNpFQZ6XvCjwz0E%3D&reserved=0
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>> ---
>>>> fs/debugfs/inode.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>>>> index 8129a430d789..2f117c57160d 100644
>>>> --- a/fs/debugfs/inode.c
>>>> +++ b/fs/debugfs/inode.c
>>>> @@ -528,7 +528,7 @@ void debugfs_create_file_size(const char *name, umode_t mode,
>>>> {
>>>> struct dentry *de = debugfs_create_file(name, mode, parent, data, fops);
>>>> - if (de)
>>>> + if (!IS_ERR(de))
>>>> d_inode(de)->i_size = file_size;
>>>> }
>>>> EXPORT_SYMBOL_GPL(debugfs_create_file_size);
>>>> --
>>>> 2.32.0
>>>>
>>> Ah, good catch, I'll queue this up after 5.15-rc1 is out, thanks!
>> Thinking more about this if I'm not completely mistaken
>> debugfs_create_file() returns -ENODEV when debugfs is disabled and NULL on
>> any other error.
> How can this function be called if debugfs is not enabled in the system
> configuration? This _is_ the debugfs core code.
Well, that's what I meant. The original code is correct and Nirmoy's
patch here is breaking it.
Nirmoys other patch is for a driver and there the function can indeed
return both error code and NULL.
Thanks,
Christian.
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] debugfs: use IS_ERR to check for error
2021-09-02 15:10 ` Christian König
@ 2021-09-02 16:34 ` Greg KH
2021-09-02 16:44 ` Das, Nirmoy
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Greg KH @ 2021-09-02 16:34 UTC (permalink / raw)
To: Christian König; +Cc: Nirmoy Das, rafael, linux-kernel
On Thu, Sep 02, 2021 at 05:10:24PM +0200, Christian König wrote:
> Am 02.09.21 um 14:20 schrieb Greg KH:
> > On Thu, Sep 02, 2021 at 02:03:12PM +0200, Christian König wrote:
> > >
> > > Am 02.09.21 um 12:38 schrieb Greg KH:
> > > > On Thu, Sep 02, 2021 at 12:29:17PM +0200, Nirmoy Das wrote:
> > > > > debugfs_create_file() returns encoded error so
> > > > > use IS_ERR for checking return value.
> > > > >
> > > > > References: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1686&data=04%7C01%7Cchristian.koenig%40amd.com%7Cffc1109aeb744082181b08d96e0c06db%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637661820207117289%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=6NDHX9sEhGF2jakVfUJ7Qurql6UAdNpFQZ6XvCjwz0E%3D&reserved=0
> > > > > Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> > > > > ---
> > > > > fs/debugfs/inode.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > > > > index 8129a430d789..2f117c57160d 100644
> > > > > --- a/fs/debugfs/inode.c
> > > > > +++ b/fs/debugfs/inode.c
> > > > > @@ -528,7 +528,7 @@ void debugfs_create_file_size(const char *name, umode_t mode,
> > > > > {
> > > > > struct dentry *de = debugfs_create_file(name, mode, parent, data, fops);
> > > > > - if (de)
> > > > > + if (!IS_ERR(de))
> > > > > d_inode(de)->i_size = file_size;
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(debugfs_create_file_size);
> > > > > --
> > > > > 2.32.0
> > > > >
> > > > Ah, good catch, I'll queue this up after 5.15-rc1 is out, thanks!
> > > Thinking more about this if I'm not completely mistaken
> > > debugfs_create_file() returns -ENODEV when debugfs is disabled and NULL on
> > > any other error.
> > How can this function be called if debugfs is not enabled in the system
> > configuration? This _is_ the debugfs core code.
>
> Well, that's what I meant. The original code is correct and Nirmoy's patch
> here is breaking it.
Ah, yes, sorry, you are right. This function can not return an error
value, if something went wrong, the result will always be NULL.
> Nirmoys other patch is for a driver and there the function can indeed return
> both error code and NULL.
You should never be checking this stuff in a caller anyway, so no, don't
do it there either.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] debugfs: use IS_ERR to check for error
2021-09-02 16:34 ` Greg KH
@ 2021-09-02 16:44 ` Das, Nirmoy
2021-09-02 16:48 ` Das, Nirmoy
2021-09-02 17:01 ` Das, Nirmoy
2 siblings, 0 replies; 12+ messages in thread
From: Das, Nirmoy @ 2021-09-02 16:44 UTC (permalink / raw)
To: Greg KH, Christian König; +Cc: rafael, linux-kernel
On 9/2/2021 6:34 PM, Greg KH wrote:
> On Thu, Sep 02, 2021 at 05:10:24PM +0200, Christian König wrote:
>> Am 02.09.21 um 14:20 schrieb Greg KH:
>>> On Thu, Sep 02, 2021 at 02:03:12PM +0200, Christian König wrote:
>>>> Am 02.09.21 um 12:38 schrieb Greg KH:
>>>>> On Thu, Sep 02, 2021 at 12:29:17PM +0200, Nirmoy Das wrote:
>>>>>> debugfs_create_file() returns encoded error so
>>>>>> use IS_ERR for checking return value.
>>>>>>
>>>>>> References: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1686&data=04%7C01%7Cnirmoy.das%40amd.com%7C7a1f1095c0d64416576c08d96e2f7b38%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637661973378236086%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=g9BQRJG8gvjGFq6oj5vk9PCemQ39U19CLmkMNHVUafg%3D&reserved=0
>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>> ---
>>>>>> fs/debugfs/inode.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>>>>>> index 8129a430d789..2f117c57160d 100644
>>>>>> --- a/fs/debugfs/inode.c
>>>>>> +++ b/fs/debugfs/inode.c
>>>>>> @@ -528,7 +528,7 @@ void debugfs_create_file_size(const char *name, umode_t mode,
>>>>>> {
>>>>>> struct dentry *de = debugfs_create_file(name, mode, parent, data, fops);
>>>>>> - if (de)
>>>>>> + if (!IS_ERR(de))
>>>>>> d_inode(de)->i_size = file_size;
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(debugfs_create_file_size);
>>>>>> --
>>>>>> 2.32.0
>>>>>>
>>>>> Ah, good catch, I'll queue this up after 5.15-rc1 is out, thanks!
>>>> Thinking more about this if I'm not completely mistaken
>>>> debugfs_create_file() returns -ENODEV when debugfs is disabled and NULL on
>>>> any other error.
>>> How can this function be called if debugfs is not enabled in the system
>>> configuration? This _is_ the debugfs core code.
>> Well, that's what I meant. The original code is correct and Nirmoy's patch
>> here is breaking it.
> Ah, yes, sorry, you are right. This function can not return an error
> value, if something went wrong, the result will always be NULL.
The issue occurs when CONFIG_DEBUG_FS=y and CONFIG_DEBUG_FS_ALLOW_NONE=y
config options are set, so a call to
debugfs_create_file() will return ERR_PTR(-EPERM) instead of NULL.
>
>> Nirmoys other patch is for a driver and there the function can indeed return
>> both error code and NULL.
> You should never be checking this stuff in a caller anyway, so no, don't
> do it there either.
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] debugfs: use IS_ERR to check for error
2021-09-02 16:34 ` Greg KH
2021-09-02 16:44 ` Das, Nirmoy
@ 2021-09-02 16:48 ` Das, Nirmoy
2021-09-02 17:01 ` Das, Nirmoy
2 siblings, 0 replies; 12+ messages in thread
From: Das, Nirmoy @ 2021-09-02 16:48 UTC (permalink / raw)
To: Greg KH, Christian König; +Cc: rafael, linux-kernel
On 9/2/2021 6:34 PM, Greg KH wrote:
> On Thu, Sep 02, 2021 at 05:10:24PM +0200, Christian König wrote:
>> Am 02.09.21 um 14:20 schrieb Greg KH:
>>> On Thu, Sep 02, 2021 at 02:03:12PM +0200, Christian König wrote:
>>>> Am 02.09.21 um 12:38 schrieb Greg KH:
>>>>> On Thu, Sep 02, 2021 at 12:29:17PM +0200, Nirmoy Das wrote:
>>>>>> debugfs_create_file() returns encoded error so
>>>>>> use IS_ERR for checking return value.
>>>>>>
>>>>>> References: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1686&data=04%7C01%7Cnirmoy.das%40amd.com%7C7a1f1095c0d64416576c08d96e2f7b38%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637661973378236086%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=g9BQRJG8gvjGFq6oj5vk9PCemQ39U19CLmkMNHVUafg%3D&reserved=0
>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>> ---
>>>>>> fs/debugfs/inode.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>>>>>> index 8129a430d789..2f117c57160d 100644
>>>>>> --- a/fs/debugfs/inode.c
>>>>>> +++ b/fs/debugfs/inode.c
>>>>>> @@ -528,7 +528,7 @@ void debugfs_create_file_size(const char *name, umode_t mode,
>>>>>> {
>>>>>> struct dentry *de = debugfs_create_file(name, mode, parent, data, fops);
>>>>>> - if (de)
>>>>>> + if (!IS_ERR(de))
>>>>>> d_inode(de)->i_size = file_size;
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(debugfs_create_file_size);
>>>>>> --
>>>>>> 2.32.0
>>>>>>
>>>>> Ah, good catch, I'll queue this up after 5.15-rc1 is out, thanks!
>>>> Thinking more about this if I'm not completely mistaken
>>>> debugfs_create_file() returns -ENODEV when debugfs is disabled and NULL on
>>>> any other error.
>>> How can this function be called if debugfs is not enabled in the system
>>> configuration? This _is_ the debugfs core code.
>> Well, that's what I meant. The original code is correct and Nirmoy's patch
>> here is breaking it.
> Ah, yes, sorry, you are right. This function can not return an error
> value, if something went wrong, the result will always be NULL.
[I pressed send too early in my previous email :/ ]
The issue occurs when CONFIG_DEBUG_FS=y and CONFIG_DEBUG_FS_ALLOW_NONE=y
config options are set, so a call to
debugfs_create_file() will return ERR_PTR(-EPERM) instead of NULL. So I
think it still makes sense to keep the check with IS_ERR_OR_NULL()
Regards,
Nirmoy
>
>> Nirmoys other patch is for a driver and there the function can indeed return
>> both error code and NULL.
> You should never be checking this stuff in a caller anyway, so no, don't
> do it there either.
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] debugfs: use IS_ERR to check for error
2021-09-02 16:34 ` Greg KH
2021-09-02 16:44 ` Das, Nirmoy
2021-09-02 16:48 ` Das, Nirmoy
@ 2021-09-02 17:01 ` Das, Nirmoy
2021-09-03 6:27 ` Christian König
2 siblings, 1 reply; 12+ messages in thread
From: Das, Nirmoy @ 2021-09-02 17:01 UTC (permalink / raw)
To: Greg KH, Christian König; +Cc: rafael, linux-kernel, Nirmoy Das
On 9/2/2021 6:34 PM, Greg KH wrote:
> On Thu, Sep 02, 2021 at 05:10:24PM +0200, Christian König wrote:
>> Am 02.09.21 um 14:20 schrieb Greg KH:
>>> On Thu, Sep 02, 2021 at 02:03:12PM +0200, Christian König wrote:
>>>> Am 02.09.21 um 12:38 schrieb Greg KH:
>>>>> On Thu, Sep 02, 2021 at 12:29:17PM +0200, Nirmoy Das wrote:
>>>>>> debugfs_create_file() returns encoded error so
>>>>>> use IS_ERR for checking return value.
>>>>>>
>>>>>> References: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1686&data=04%7C01%7Cnirmoy.das%40amd.com%7C7a1f1095c0d64416576c08d96e2f7b38%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637661973378236086%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=g9BQRJG8gvjGFq6oj5vk9PCemQ39U19CLmkMNHVUafg%3D&reserved=0
>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>> ---
>>>>>> fs/debugfs/inode.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>>>>>> index 8129a430d789..2f117c57160d 100644
>>>>>> --- a/fs/debugfs/inode.c
>>>>>> +++ b/fs/debugfs/inode.c
>>>>>> @@ -528,7 +528,7 @@ void debugfs_create_file_size(const char *name, umode_t mode,
>>>>>> {
>>>>>> struct dentry *de = debugfs_create_file(name, mode, parent, data, fops);
>>>>>> - if (de)
>>>>>> + if (!IS_ERR(de))
>>>>>> d_inode(de)->i_size = file_size;
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(debugfs_create_file_size);
>>>>>> --
>>>>>> 2.32.0
>>>>>>
>>>>> Ah, good catch, I'll queue this up after 5.15-rc1 is out, thanks!
>>>> Thinking more about this if I'm not completely mistaken
>>>> debugfs_create_file() returns -ENODEV when debugfs is disabled and NULL on
>>>> any other error.
>>> How can this function be called if debugfs is not enabled in the system
>>> configuration? This _is_ the debugfs core code.
>> Well, that's what I meant. The original code is correct and Nirmoy's patch
>> here is breaking it.
> Ah, yes, sorry, you are right. This function can not return an error
> value, if something went wrong, the result will always be NULL.
I just realized that we don't return NULL on error anymore:
commit ff9fb72bc07705c00795ca48631f7fffe24d2c6b
Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Wed Jan 23 11:28:14 2019 +0100
debugfs: return error values, not NULL
and the current doc also says "If an error occurs, ERR_PTR(-ERROR) will
be returned."
If I am not missing anything, this patch should be fine.
Regards,
Nirmoy
>
>> Nirmoys other patch is for a driver and there the function can indeed return
>> both error code and NULL.
> You should never be checking this stuff in a caller anyway, so no, don't
> do it there either.
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] debugfs: use IS_ERR to check for error
2021-09-02 17:01 ` Das, Nirmoy
@ 2021-09-03 6:27 ` Christian König
2021-09-21 7:03 ` Greg KH
0 siblings, 1 reply; 12+ messages in thread
From: Christian König @ 2021-09-03 6:27 UTC (permalink / raw)
To: Das, Nirmoy, Greg KH; +Cc: rafael, linux-kernel, Nirmoy Das
Am 02.09.21 um 19:01 schrieb Das, Nirmoy:
>
> On 9/2/2021 6:34 PM, Greg KH wrote:
>> On Thu, Sep 02, 2021 at 05:10:24PM +0200, Christian König wrote:
>>> Am 02.09.21 um 14:20 schrieb Greg KH:
>>>> On Thu, Sep 02, 2021 at 02:03:12PM +0200, Christian König wrote:
>>>>> Am 02.09.21 um 12:38 schrieb Greg KH:
>>>>>> On Thu, Sep 02, 2021 at 12:29:17PM +0200, Nirmoy Das wrote:
>>>>>>> debugfs_create_file() returns encoded error so
>>>>>>> use IS_ERR for checking return value.
>>>>>>>
>>>>>>> References:
>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1686&data=04%7C01%7Cnirmoy.das%40amd.com%7C7a1f1095c0d64416576c08d96e2f7b38%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637661973378236086%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=g9BQRJG8gvjGFq6oj5vk9PCemQ39U19CLmkMNHVUafg%3D&reserved=0
>>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>>> ---
>>>>>>> fs/debugfs/inode.c | 2 +-
>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>>>>>>> index 8129a430d789..2f117c57160d 100644
>>>>>>> --- a/fs/debugfs/inode.c
>>>>>>> +++ b/fs/debugfs/inode.c
>>>>>>> @@ -528,7 +528,7 @@ void debugfs_create_file_size(const char
>>>>>>> *name, umode_t mode,
>>>>>>> {
>>>>>>> struct dentry *de = debugfs_create_file(name, mode,
>>>>>>> parent, data, fops);
>>>>>>> - if (de)
>>>>>>> + if (!IS_ERR(de))
>>>>>>> d_inode(de)->i_size = file_size;
>>>>>>> }
>>>>>>> EXPORT_SYMBOL_GPL(debugfs_create_file_size);
>>>>>>> --
>>>>>>> 2.32.0
>>>>>>>
>>>>>> Ah, good catch, I'll queue this up after 5.15-rc1 is out, thanks!
>>>>> Thinking more about this if I'm not completely mistaken
>>>>> debugfs_create_file() returns -ENODEV when debugfs is disabled and
>>>>> NULL on
>>>>> any other error.
>>>> How can this function be called if debugfs is not enabled in the
>>>> system
>>>> configuration? This _is_ the debugfs core code.
>>> Well, that's what I meant. The original code is correct and Nirmoy's
>>> patch
>>> here is breaking it.
>> Ah, yes, sorry, you are right. This function can not return an error
>> value, if something went wrong, the result will always be NULL.
>
>
> I just realized that we don't return NULL on error anymore:
>
> commit ff9fb72bc07705c00795ca48631f7fffe24d2c6b
> Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Date: Wed Jan 23 11:28:14 2019 +0100
>
> debugfs: return error values, not NULL
>
>
> and the current doc also says "If an error occurs, ERR_PTR(-ERROR)
> will be returned."
>
> If I am not missing anything, this patch should be fine.
Ah! Yes, now that makes sense.
Looks like that my memory and the documentation under
https://www.kernel.org/doc/htmldocs/filesystems/API-debugfs-create-file.html
is outdated.
I can update my memory, but I have no idea where this documentation
comes from and how to fix it.
Regards,
Christian.
>
>
> Regards,
>
> Nirmoy
>
>>
>>> Nirmoys other patch is for a driver and there the function can
>>> indeed return
>>> both error code and NULL.
>> You should never be checking this stuff in a caller anyway, so no, don't
>> do it there either.
>>
>> thanks,
>>
>> greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] debugfs: use IS_ERR to check for error
2021-09-03 6:27 ` Christian König
@ 2021-09-21 7:03 ` Greg KH
0 siblings, 0 replies; 12+ messages in thread
From: Greg KH @ 2021-09-21 7:03 UTC (permalink / raw)
To: Christian König; +Cc: Das, Nirmoy, rafael, linux-kernel, Nirmoy Das
On Fri, Sep 03, 2021 at 08:27:34AM +0200, Christian König wrote:
> Am 02.09.21 um 19:01 schrieb Das, Nirmoy:
> >
> > On 9/2/2021 6:34 PM, Greg KH wrote:
> > > On Thu, Sep 02, 2021 at 05:10:24PM +0200, Christian König wrote:
> > > > Am 02.09.21 um 14:20 schrieb Greg KH:
> > > > > On Thu, Sep 02, 2021 at 02:03:12PM +0200, Christian König wrote:
> > > > > > Am 02.09.21 um 12:38 schrieb Greg KH:
> > > > > > > On Thu, Sep 02, 2021 at 12:29:17PM +0200, Nirmoy Das wrote:
> > > > > > > > debugfs_create_file() returns encoded error so
> > > > > > > > use IS_ERR for checking return value.
> > > > > > > >
> > > > > > > > References: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1686&data=04%7C01%7Cnirmoy.das%40amd.com%7C7a1f1095c0d64416576c08d96e2f7b38%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637661973378236086%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=g9BQRJG8gvjGFq6oj5vk9PCemQ39U19CLmkMNHVUafg%3D&reserved=0
> > > > > > > > Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> > > > > > > > ---
> > > > > > > > fs/debugfs/inode.c | 2 +-
> > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > > > > > > > index 8129a430d789..2f117c57160d 100644
> > > > > > > > --- a/fs/debugfs/inode.c
> > > > > > > > +++ b/fs/debugfs/inode.c
> > > > > > > > @@ -528,7 +528,7 @@ void
> > > > > > > > debugfs_create_file_size(const char *name,
> > > > > > > > umode_t mode,
> > > > > > > > {
> > > > > > > > struct dentry *de =
> > > > > > > > debugfs_create_file(name, mode, parent, data,
> > > > > > > > fops);
> > > > > > > > - if (de)
> > > > > > > > + if (!IS_ERR(de))
> > > > > > > > d_inode(de)->i_size = file_size;
> > > > > > > > }
> > > > > > > > EXPORT_SYMBOL_GPL(debugfs_create_file_size);
> > > > > > > > --
> > > > > > > > 2.32.0
> > > > > > > >
> > > > > > > Ah, good catch, I'll queue this up after 5.15-rc1 is out, thanks!
> > > > > > Thinking more about this if I'm not completely mistaken
> > > > > > debugfs_create_file() returns -ENODEV when debugfs is
> > > > > > disabled and NULL on
> > > > > > any other error.
> > > > > How can this function be called if debugfs is not enabled in
> > > > > the system
> > > > > configuration? This _is_ the debugfs core code.
> > > > Well, that's what I meant. The original code is correct and
> > > > Nirmoy's patch
> > > > here is breaking it.
> > > Ah, yes, sorry, you are right. This function can not return an error
> > > value, if something went wrong, the result will always be NULL.
> >
> >
> > I just realized that we don't return NULL on error anymore:
> >
> > commit ff9fb72bc07705c00795ca48631f7fffe24d2c6b
> > Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Date: Wed Jan 23 11:28:14 2019 +0100
> >
> > debugfs: return error values, not NULL
> >
> >
> > and the current doc also says "If an error occurs, ERR_PTR(-ERROR) will
> > be returned."
> >
> > If I am not missing anything, this patch should be fine.
>
> Ah! Yes, now that makes sense.
>
> Looks like that my memory and the documentation under
> https://www.kernel.org/doc/htmldocs/filesystems/API-debugfs-create-file.html
> is outdated.
>
> I can update my memory, but I have no idea where this documentation comes
> from and how to fix it.
I do not know how that is created either, it looks very old. This link
should always be used instead:
https://www.kernel.org/doc/html/latest/index.html
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-09-21 7:03 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-02 10:29 [PATCH 1/1] debugfs: use IS_ERR to check for error Nirmoy Das
2021-09-02 10:31 ` Christian König
2021-09-02 10:38 ` Greg KH
2021-09-02 12:03 ` Christian König
2021-09-02 12:20 ` Greg KH
2021-09-02 15:10 ` Christian König
2021-09-02 16:34 ` Greg KH
2021-09-02 16:44 ` Das, Nirmoy
2021-09-02 16:48 ` Das, Nirmoy
2021-09-02 17:01 ` Das, Nirmoy
2021-09-03 6:27 ` Christian König
2021-09-21 7:03 ` Greg KH
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.