Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 1/1] debugfs: use IS_ERR to check for error
To:     Greg KH <gregkh@linuxfoundation.org>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
Cc:     rafael@kernel.org, linux-kernel@vger.kernel.org,
        Nirmoy Das <nirmoy.aiemd@gmail.com>
References: <20210902102917.2233-1-nirmoy.das@amd.com>
 <YTCpuo00wM7jGSIc@kroah.com> <5ab7739d-15a4-2e84-0320-fee8d5df1e83@amd.com>
 <YTDBfO/byKzGWV8G@kroah.com> <c918f1a5-7c88-bf47-14b6-b6e892695951@amd.com>
 <YTD8+tLP7KeGRXEc@kroah.com>
From:   "Das, Nirmoy" <nirmoy.das@amd.com>
Message-ID: <a6817cbd-b43d-5334-3e1e-62c1cd99e8fc@amd.com>
Date:   Thu, 2 Sep 2021 19:01:49 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
In-Reply-To: <YTD8+tLP7KeGRXEc@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?U0l5MjRTTXZyczE4WSszMlJqTTJWM0tiVU54ejVJL2lzSGRsS3JZRzdOL216?=
 =?utf-8?B?WS9OeUl2QXJUbCtpOENEYUtXVThuVHdxSnl0RmVhcTV0OXN6RDJ1d2FJV2Qy?=
 =?utf-8?B?WlFBOUVtOUN0bkNST2N6STltOTRYbWlJcWtqSFBxdXZaV1dXYjROVXh1OEh0?=
 =?utf-8?B?Z3A5L2tzNDV5dEtWOFdIQkY1TVc4dzB3VklHMzIrWHdxcFkzWmkrQUNac0Jp?=
 =?utf-8?B?dDg1dDBuSTlMRGZSbkdyenBQUnNpTVpYRU9yUFlnM2xOd1lEb25Sd3JMQ1V2?=
 =?utf-8?B?MzRmQ1NmeGZtUEdGMjlrUTlYM09VRGljOFF0a1UwcTJ4NkNqbmNBMTJGQXVJ?=
 =?utf-8?B?ZHVTd2tkS3B6RkRNZ1VuZ2NRMnRKY2NDa1E0L0xYK0RZTWZxaGVSYUExY051?=
 =?utf-8?B?aVg5cmVrazUwVzN3dnUvYWxJM3NaSjdVb09rY1M4U1JIcXpFb0RRdEhpU0ll?=
 =?utf-8?B?TmhaRXlGL1RKOHFLVGlQZkRwV2pUejZ4cmlZS2VnNU9Gck1SY0p0Z0hydWdK?=
 =?utf-8?B?SXEvR28yS3Y1Q0Z6Z3BhSm0vaHFKRmRHRDhCZDZsVWttVVhEaTlybTgyMmdC?=
 =?utf-8?B?dnpwTUd5WW5rc1hKSndWNXlMRHBjMXEwN3FmT1JtcDZkOWlzYmQzeHFZQVZj?=
 =?utf-8?B?OUtQQTZQaXdQUGw3NDJOeTBYM3k3UnNoMzd1QWFZTzg2OTBjZWZaMysxUkN0?=
 =?utf-8?B?L05RanNXREl4bWdaZWo2eU51Rkh6dUdnREJ4MlVTck4rVXhrUDdocEMyVy92?=
 =?utf-8?B?ZHU3YTlpNUQyMGw2RUNUUVVxVWZvbVBzSURHZm5XTFRUUm1KQm1WSjhsNmVw?=
 =?utf-8?B?QjFacTNxTzZPNDVoamhRbFVhMmREdE5JSFBmNUM0SmtTQm9hZi8wOFBseVRK?=
 =?utf-8?B?TzZFT3pEaHN6Mjk3dzNLa0tCdUxGZUtRSys5ZzFhU0VRRmFNVEZsWUZSZUhn?=
 =?utf-8?B?Qzh1NmthcEJUL2VlclZrZjlONmNyb3UzLzhxeXZoQVlHQU1xbFoxMFJ0M01k?=
 =?utf-8?B?S09GWEMvOE45SXdQakUzYkhTd1dtd0szNy9nTHBoZndTbk1adThrTXhIREVJ?=
 =?utf-8?B?VHp4UHpKNUV6aXdqdzNaaHAwZnpwOXUzTmRmaEF0dHBTREdpVGJlc1EzVjVI?=
 =?utf-8?B?WGwwelBxdVNYUHJlTHB5VTdBWHNHZWN3VGpQMExNZmtkazVyOW50WGhpeTIz?=
 =?utf-8?B?THVCSU0ySUxqSzNkd0ZSa0RYRjBOQXRkdWkxS084Tk5lUFhYam5wVlJTYjBG?=
 =?utf-8?B?ZTltWEllT1JobkkxejR4SXVRZzJpMXNKOHJZU0lFK05Xd25GVDdPRHhBVjJy?=
 =?utf-8?B?ZFE0bkdtU2dnNjZqUmJjRlFIdGxHb2l3MlBxaUZ0bk5nNjhBV3ByTWNpdHpw?=
 =?utf-8?B?dnN6N3hUb1k2UHlERGpLWXRPdzF5QlA0Vmw3aEpNREdwNEQ2aFBJVC9pZlc3?=
 =?utf-8?B?dXVxTVNQeklJcndlRW4wVGZiUTFyb2ZaekRWWWxTakJYOFRlcHZLUUtGd0lP?=
 =?utf-8?B?K2JXMFRwZCtrbFRBWllqTUJqNzB5NzZpOUJoalJvNUVXRElucWJJTDlaQ0Ru?=
 =?utf-8?B?M2t1OEVucE1OOWw5SXZESDhaY3RndGt1cG1zU09MTC9kMzA0NU4xSVl4Z1ky?=
 =?utf-8?B?WEhVNlV6Q0ZyY0JTMVlVdFBUQVg0aFlWSEhjWnJyNlVjbS9TMDJ0YTJUZ1Bz?=
 =?utf-8?B?Rnd5UmlIdEtVOEdhQmpadVNIWmVWSkp2aWNHZnJNU0xaRG9uNGZkYm55eUZY?=
 =?utf-8?B?VWtKa25xWjlmU3FUSS9rNjBOb210aWlPRWx5TGVaNEFmYUdrdVJGODZncGg1?=
 =?utf-8?B?UDRhb2laRktOSVJNMUFkMUYrb0g3TEhFdE9IMkpqcFZHRmY2MldpQVBoWEtV?=
 =?utf-8?Q?7cMN/aoUiZOEa?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 18062e31-656f-4ef6-89c1-08d96e335dad
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5136.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Sep 2021 17:01:54.4981
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: iXZZlTFAajEeXaqZpQ2zCfHxwXS2LKBoMT58UDkxVjN1hGXjnPparvWAKNuZhAeWhYhek0OfGJGbDVoKXwzlZA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR12MB5493
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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&amp;data=04%7C01%7Cnirmoy.das%40amd.com%7C7a1f1095c0d64416576c08d96e2f7b38%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637661973378236086%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=g9BQRJG8gvjGFq6oj5vk9PCemQ39U19CLmkMNHVUafg%3D&amp;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