* 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