| Message ID | 20230815203826.51792-1-atulpant.linux@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] drivers: edac: Drop unnecessary error check for debugfs_create_dir | expand |
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
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 > > > >
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
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 > > >
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);
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(-)