| Message ID | 20230726110750.3925-1-machel@vivo.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,v3] wifi: ath9k: Remove error checking for debugfs_create_dir() | expand |
Wang Ming <machel@vivo.com> writes: > It is expected that most callers should _ignore_ the errors > return by debugfs_create_dir() in ath9k_htc_init_debug(). > > Signed-off-by: Wang Ming <machel@vivo.com> ath9k patches go to ath-next, not net-next. So do *not* put net-next into Subject. No need to resend because of this.
Wang Ming <machel@vivo.com> writes: > It is expected that most callers should _ignore_ the errors > return by debugfs_create_dir() in ath9k_htc_init_debug(). > > Signed-off-by: Wang Ming <machel@vivo.com> > --- > drivers/net/wireless/ath/ath9k/htc_drv_debug.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_debug.c b/drivers/net/wireless/ath/ath9k/htc_drv_debug.c > index b3ed65e5c4da..85ad45771b44 100644 > --- a/drivers/net/wireless/ath/ath9k/htc_drv_debug.c > +++ b/drivers/net/wireless/ath/ath9k/htc_drv_debug.c > @@ -491,8 +491,6 @@ int ath9k_htc_init_debug(struct ath_hw *ah) > > priv->debug.debugfs_phy = debugfs_create_dir(KBUILD_MODNAME, > priv->hw->wiphy->debugfsdir); > - if (!priv->debug.debugfs_phy) > - return -ENOMEM; Hmm, so it's true that all the debugfs_create* functions deal correctly with the dir pointer being an error pointer, which means that it's possible to just ignore the return value of debugfs_create_dir() without anything breaking. However, it also seems kinda pointless to have all those calls if we know they're going to fail, so I prefer v1 of this patch that just fixed the IS_ERR check. No need to resend, we can just apply v1 instead... -Toke
Toke Høiland-Jørgensen <toke@toke.dk> writes: > Wang Ming <machel@vivo.com> writes: > >> It is expected that most callers should _ignore_ the errors >> return by debugfs_create_dir() in ath9k_htc_init_debug(). >> >> Signed-off-by: Wang Ming <machel@vivo.com> >> --- >> drivers/net/wireless/ath/ath9k/htc_drv_debug.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_debug.c b/drivers/net/wireless/ath/ath9k/htc_drv_debug.c >> index b3ed65e5c4da..85ad45771b44 100644 >> --- a/drivers/net/wireless/ath/ath9k/htc_drv_debug.c >> +++ b/drivers/net/wireless/ath/ath9k/htc_drv_debug.c >> @@ -491,8 +491,6 @@ int ath9k_htc_init_debug(struct ath_hw *ah) >> >> priv->debug.debugfs_phy = debugfs_create_dir(KBUILD_MODNAME, >> priv->hw->wiphy->debugfsdir); >> - if (!priv->debug.debugfs_phy) >> - return -ENOMEM; > > Hmm, so it's true that all the debugfs_create* functions deal correctly > with the dir pointer being an error pointer, which means that it's > possible to just ignore the return value of debugfs_create_dir() without > anything breaking. The comment in debugfs_create_dir() states: * NOTE: it's expected that most callers should _ignore_ the errors returned * by this function. Other debugfs functions handle the fact that the "dentry" * passed to them could be an error and they don't crash in that case. * Drivers should generally work fine even if debugfs fails to init anyway. > However, it also seems kinda pointless to have all those calls if we > know they're going to fail, so I prefer v1 of this patch that just > fixed the IS_ERR check. No need to resend, we can just apply v1 > instead... Because of the comment I'm leaning towards v3.
Kalle Valo <kvalo@kernel.org> writes: > Toke Høiland-Jørgensen <toke@toke.dk> writes: > >> Wang Ming <machel@vivo.com> writes: >> >>> It is expected that most callers should _ignore_ the errors >>> return by debugfs_create_dir() in ath9k_htc_init_debug(). >>> >>> Signed-off-by: Wang Ming <machel@vivo.com> >>> --- >>> drivers/net/wireless/ath/ath9k/htc_drv_debug.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_debug.c b/drivers/net/wireless/ath/ath9k/htc_drv_debug.c >>> index b3ed65e5c4da..85ad45771b44 100644 >>> --- a/drivers/net/wireless/ath/ath9k/htc_drv_debug.c >>> +++ b/drivers/net/wireless/ath/ath9k/htc_drv_debug.c >>> @@ -491,8 +491,6 @@ int ath9k_htc_init_debug(struct ath_hw *ah) >>> >>> priv->debug.debugfs_phy = debugfs_create_dir(KBUILD_MODNAME, >>> priv->hw->wiphy->debugfsdir); >>> - if (!priv->debug.debugfs_phy) >>> - return -ENOMEM; >> >> Hmm, so it's true that all the debugfs_create* functions deal correctly >> with the dir pointer being an error pointer, which means that it's >> possible to just ignore the return value of debugfs_create_dir() without >> anything breaking. > > The comment in debugfs_create_dir() states: > > * NOTE: it's expected that most callers should _ignore_ the errors returned > * by this function. Other debugfs functions handle the fact that the "dentry" > * passed to them could be an error and they don't crash in that case. > * Drivers should generally work fine even if debugfs fails to init anyway. > >> However, it also seems kinda pointless to have all those calls if we >> know they're going to fail, so I prefer v1 of this patch that just >> fixed the IS_ERR check. No need to resend, we can just apply v1 >> instead... > > Because of the comment I'm leaning towards v3. Well, the comment says "most callers" :) I think having an early return like this is perfectly valid optimisation, even if it doesn't really make any performance difference. I don't feel incredibly strongly about it (given that the current check is broken I guess the early return has never actually worked), so if you feel like overriding your submaintainer on this, feel free ;) -Toke
Toke Høiland-Jørgensen <toke@toke.dk> writes: > Kalle Valo <kvalo@kernel.org> writes: > >> Toke Høiland-Jørgensen <toke@toke.dk> writes: >> >>> Wang Ming <machel@vivo.com> writes: >>> >>>> It is expected that most callers should _ignore_ the errors >>>> return by debugfs_create_dir() in ath9k_htc_init_debug(). >>>> >>>> Signed-off-by: Wang Ming <machel@vivo.com> >>>> --- >>>> drivers/net/wireless/ath/ath9k/htc_drv_debug.c | 2 -- >>>> 1 file changed, 2 deletions(-) >>>> >>>> diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_debug.c b/drivers/net/wireless/ath/ath9k/htc_drv_debug.c >>>> index b3ed65e5c4da..85ad45771b44 100644 >>>> --- a/drivers/net/wireless/ath/ath9k/htc_drv_debug.c >>>> +++ b/drivers/net/wireless/ath/ath9k/htc_drv_debug.c >>>> @@ -491,8 +491,6 @@ int ath9k_htc_init_debug(struct ath_hw *ah) >>>> >>>> priv->debug.debugfs_phy = debugfs_create_dir(KBUILD_MODNAME, >>>> priv->hw->wiphy->debugfsdir); >>>> - if (!priv->debug.debugfs_phy) >>>> - return -ENOMEM; >>> >>> Hmm, so it's true that all the debugfs_create* functions deal correctly >>> with the dir pointer being an error pointer, which means that it's >>> possible to just ignore the return value of debugfs_create_dir() without >>> anything breaking. >> >> The comment in debugfs_create_dir() states: >> >> * NOTE: it's expected that most callers should _ignore_ the errors returned >> * by this function. Other debugfs functions handle the fact that the "dentry" >> * passed to them could be an error and they don't crash in that case. >> * Drivers should generally work fine even if debugfs fails to init anyway. >> >>> However, it also seems kinda pointless to have all those calls if we >>> know they're going to fail, so I prefer v1 of this patch that just >>> fixed the IS_ERR check. No need to resend, we can just apply v1 >>> instead... >> >> Because of the comment I'm leaning towards v3. > > Well, the comment says "most callers" :) > > I think having an early return like this is perfectly valid > optimisation, even if it doesn't really make any performance difference. > I don't feel incredibly strongly about it (given that the current check > is broken I guess the early return has never actually worked), so if you > feel like overriding your submaintainer on this, feel free ;) No no, I don't want to override anything :) Just making sure you were aware of the comment. v1 is in my pending branch right now.
Kalle Valo <kvalo@kernel.org> writes: > Toke Høiland-Jørgensen <toke@toke.dk> writes: > >> Kalle Valo <kvalo@kernel.org> writes: >> >>> Toke Høiland-Jørgensen <toke@toke.dk> writes: >>> >>>> Wang Ming <machel@vivo.com> writes: >>>> >>>>> It is expected that most callers should _ignore_ the errors >>>>> return by debugfs_create_dir() in ath9k_htc_init_debug(). >>>>> >>>>> Signed-off-by: Wang Ming <machel@vivo.com> >>>>> --- >>>>> drivers/net/wireless/ath/ath9k/htc_drv_debug.c | 2 -- >>>>> 1 file changed, 2 deletions(-) >>>>> >>>>> diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_debug.c b/drivers/net/wireless/ath/ath9k/htc_drv_debug.c >>>>> index b3ed65e5c4da..85ad45771b44 100644 >>>>> --- a/drivers/net/wireless/ath/ath9k/htc_drv_debug.c >>>>> +++ b/drivers/net/wireless/ath/ath9k/htc_drv_debug.c >>>>> @@ -491,8 +491,6 @@ int ath9k_htc_init_debug(struct ath_hw *ah) >>>>> >>>>> priv->debug.debugfs_phy = debugfs_create_dir(KBUILD_MODNAME, >>>>> priv->hw->wiphy->debugfsdir); >>>>> - if (!priv->debug.debugfs_phy) >>>>> - return -ENOMEM; >>>> >>>> Hmm, so it's true that all the debugfs_create* functions deal correctly >>>> with the dir pointer being an error pointer, which means that it's >>>> possible to just ignore the return value of debugfs_create_dir() without >>>> anything breaking. >>> >>> The comment in debugfs_create_dir() states: >>> >>> * NOTE: it's expected that most callers should _ignore_ the errors returned >>> * by this function. Other debugfs functions handle the fact that the "dentry" >>> * passed to them could be an error and they don't crash in that case. >>> * Drivers should generally work fine even if debugfs fails to init anyway. >>> >>>> However, it also seems kinda pointless to have all those calls if we >>>> know they're going to fail, so I prefer v1 of this patch that just >>>> fixed the IS_ERR check. No need to resend, we can just apply v1 >>>> instead... >>> >>> Because of the comment I'm leaning towards v3. >> >> Well, the comment says "most callers" :) >> >> I think having an early return like this is perfectly valid >> optimisation, even if it doesn't really make any performance difference. >> I don't feel incredibly strongly about it (given that the current check >> is broken I guess the early return has never actually worked), so if you >> feel like overriding your submaintainer on this, feel free ;) > > No no, I don't want to override anything :) Just making sure you were > aware of the comment. v1 is in my pending branch right now. Alright, cool :) -Toke
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_debug.c b/drivers/net/wireless/ath/ath9k/htc_drv_debug.c index b3ed65e5c4da..85ad45771b44 100644 --- a/drivers/net/wireless/ath/ath9k/htc_drv_debug.c +++ b/drivers/net/wireless/ath/ath9k/htc_drv_debug.c @@ -491,8 +491,6 @@ int ath9k_htc_init_debug(struct ath_hw *ah) priv->debug.debugfs_phy = debugfs_create_dir(KBUILD_MODNAME, priv->hw->wiphy->debugfsdir); - if (!priv->debug.debugfs_phy) - return -ENOMEM; ath9k_cmn_spectral_init_debug(&priv->spec_priv, priv->debug.debugfs_phy);
It is expected that most callers should _ignore_ the errors return by debugfs_create_dir() in ath9k_htc_init_debug(). Signed-off-by: Wang Ming <machel@vivo.com> --- drivers/net/wireless/ath/ath9k/htc_drv_debug.c | 2 -- 1 file changed, 2 deletions(-)