[PATCH v1] drivers: edac: Drop unnecessary error check for debugfs_create_dir

[PATCH v1] drivers: edac: Drop unnecessary error check for debugfs_create_dir

[Atul Kumar Pant]

This patch removes the error checking for debugfs_create_dir.
Even if we get an error from this function, other debugfs APIs will
handle the error value and doesn't crash in that case. Hence caller can
safely ignore the errors that occur during the creation of debugfs nodes.

Signed-off-by: Atul Kumar Pant <atulpant.linux@gmail.com>
---
 drivers/edac/zynqmp_edac.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/edac/zynqmp_edac.c b/drivers/edac/zynqmp_edac.c
index cefbbafb945e..a8449f0711d7 100644
--- a/drivers/edac/zynqmp_edac.c
+++ b/drivers/edac/zynqmp_edac.c
@@ -351,8 +351,6 @@ static void setup_debugfs(struct edac_device_ctl_info *edac_dev)
 	struct edac_priv *priv = edac_dev->pvt_info;
 
 	priv->debugfs_dir = edac_debugfs_create_dir("ocm");
-	if (IS_ERR(priv->debugfs_dir))
-		return;
 
 	edac_debugfs_create_x32("inject_fault_count", 0644, priv->debugfs_dir,
 				&priv->fault_injection_cnt);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] drivers: edac: Drop unnecessary error check for debugfs_create_dir

[Michal Simek]

On 8/15/23 22:38, Atul Kumar Pant wrote:
> This patch removes the error checking for debugfs_create_dir.

Avoid using "This patch".

> Even if we get an error from this function, other debugfs APIs will
> handle the error value and doesn't crash in that case. Hence caller can
> safely ignore the errors that occur during the creation of debugfs nodes.

First of all which issue do you have? Did you see that folder is not created?

I am not quite sure if this is the right behavior.
In the code there is
135         if (!parent)
136                 parent = edac_debugfs;

It means you are right that if creating ocm folder can fail and properties will 
be still created under edac_debugfs but is this the right behavior?

altera_edac/armada_xp_edac/i10nm/i5100/igen6/others are checking return value 
that's why I can't see any reason to remove this checking from one driver.

If you want to fix all please send patch for all but I don't think it will 
improve situation and it will just hide different issue if creating folder fails.
And debugfs will be disabled in production system anyway.

Thanks,
Michal





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] drivers: edac: Drop unnecessary error check for debugfs_create_dir

[Atul Kumar Pant]

On Fri, Aug 25, 2023 at 09:31:54AM +0200, Michal Simek wrote:
> 
> 
> On 8/15/23 22:38, Atul Kumar Pant wrote:
> > This patch removes the error checking for debugfs_create_dir.
> 
> Avoid using "This patch".

	Thanks for pointing this out. I'll remember this.

> 
> > Even if we get an error from this function, other debugfs APIs will
> > handle the error value and doesn't crash in that case. Hence caller can
> > safely ignore the errors that occur during the creation of debugfs nodes.
> 
> First of all which issue do you have? Did you see that folder is not created?

	I have not seen any issue as such. But going by the comments before
	the debugfs_create_dir API (https://elixir.bootlin.com/linux/latest/source/fs/debugfs/inode.c#L583),
	we can ignore safely ignore the return value from this API.

> 
> I am not quite sure if this is the right behavior.
> In the code there is
> 135         if (!parent)
> 136                 parent = edac_debugfs;
> 
> It means you are right that if creating ocm folder can fail and properties
> will be still created under edac_debugfs but is this the right behavior?
> 
> altera_edac/armada_xp_edac/i10nm/i5100/igen6/others are checking return
> value that's why I can't see any reason to remove this checking from one
> driver.
> 
> If you want to fix all please send patch for all but I don't think it will
> improve situation and it will just hide different issue if creating folder
> fails.

	Understood your point. Are you suggesting that we should keep these
	checks as it is, or should I fix for all the drivers and upload the
	patch ?

> And debugfs will be disabled in production system anyway.
> 
> Thanks,
> Michal
> 
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] drivers: edac: Drop unnecessary error check for debugfs_create_dir

[Michal Simek]

On 8/28/23 15:35, Atul Kumar Pant wrote:
> On Fri, Aug 25, 2023 at 09:31:54AM +0200, Michal Simek wrote:
>>
>>
>> On 8/15/23 22:38, Atul Kumar Pant wrote:
>>> This patch removes the error checking for debugfs_create_dir.
>>
>> Avoid using "This patch".
> 
> 	Thanks for pointing this out. I'll remember this.
> 
>>
>>> Even if we get an error from this function, other debugfs APIs will
>>> handle the error value and doesn't crash in that case. Hence caller can
>>> safely ignore the errors that occur during the creation of debugfs nodes.
>>
>> First of all which issue do you have? Did you see that folder is not created?
> 
> 	I have not seen any issue as such. But going by the comments before
> 	the debugfs_create_dir API (https://elixir.bootlin.com/linux/latest/source/fs/debugfs/inode.c#L583),
> 	we can ignore safely ignore the return value from this API.
> 
>>
>> I am not quite sure if this is the right behavior.
>> In the code there is
>> 135         if (!parent)
>> 136                 parent = edac_debugfs;
>>
>> It means you are right that if creating ocm folder can fail and properties
>> will be still created under edac_debugfs but is this the right behavior?
>>
>> altera_edac/armada_xp_edac/i10nm/i5100/igen6/others are checking return
>> value that's why I can't see any reason to remove this checking from one
>> driver.
>>
>> If you want to fix all please send patch for all but I don't think it will
>> improve situation and it will just hide different issue if creating folder
>> fails.
> 
> 	Understood your point. Are you suggesting that we should keep these
> 	checks as it is, or should I fix for all the drivers and upload the
> 	patch ?

Up to Boris to decide but I would say keep it as is. Even debugfs is not stable 
interface I would like to be informed if something fails. But just 2c.

Thanks,
Michal




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] drivers: edac: Drop unnecessary error check for debugfs_create_dir

[Atul Kumar Pant]

On Mon, Aug 28, 2023 at 04:00:41PM +0200, Michal Simek wrote:
> 
> 
> On 8/28/23 15:35, Atul Kumar Pant wrote:
> > On Fri, Aug 25, 2023 at 09:31:54AM +0200, Michal Simek wrote:
> > > 
> > > 
> > > On 8/15/23 22:38, Atul Kumar Pant wrote:
> > > > This patch removes the error checking for debugfs_create_dir.
> > > 
> > > Avoid using "This patch".
> > 
> > 	Thanks for pointing this out. I'll remember this.
> > 
> > > 
> > > > Even if we get an error from this function, other debugfs APIs will
> > > > handle the error value and doesn't crash in that case. Hence caller can
> > > > safely ignore the errors that occur during the creation of debugfs nodes.
> > > 
> > > First of all which issue do you have? Did you see that folder is not created?
> > 
> > 	I have not seen any issue as such. But going by the comments before
> > 	the debugfs_create_dir API (https://elixir.bootlin.com/linux/latest/source/fs/debugfs/inode.c#L583),
> > 	we can ignore safely ignore the return value from this API.
> > 
> > > 
> > > I am not quite sure if this is the right behavior.
> > > In the code there is
> > > 135         if (!parent)
> > > 136                 parent = edac_debugfs;
> > > 
> > > It means you are right that if creating ocm folder can fail and properties
> > > will be still created under edac_debugfs but is this the right behavior?
> > > 
> > > altera_edac/armada_xp_edac/i10nm/i5100/igen6/others are checking return
> > > value that's why I can't see any reason to remove this checking from one
> > > driver.
> > > 
> > > If you want to fix all please send patch for all but I don't think it will
> > > improve situation and it will just hide different issue if creating folder
> > > fails.
> > 
> > 	Understood your point. Are you suggesting that we should keep these
> > 	checks as it is, or should I fix for all the drivers and upload the
> > 	patch ?
> 
> Up to Boris to decide but I would say keep it as is. Even debugfs is not
> stable interface I would like to be informed if something fails. But just
> 2c.

	Thanks you Michal. I'll wait for the reply from Boris.

> 
> Thanks,
> Michal
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel