Messages in this thread | | | Subject | Re: [PATCH] iwlwifi: mvm: no need to check return value of debugfs_create functions | From | Laura Abbott <> | Date | Tue, 26 Mar 2019 18:47:33 -0700 |
| |
On 3/26/19 6:26 PM, Greg Kroah-Hartman wrote: > On Tue, Mar 26, 2019 at 04:55:54PM -0700, Laura Abbott wrote: >> On 1/22/19 7:21 AM, Greg Kroah-Hartman wrote: >>> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c >>> index 33b0af24a537..c52cdc538678 100644 >>> --- a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c >>> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c >>> @@ -1446,9 +1446,8 @@ static ssize_t iwl_dbgfs_quota_min_read(struct file *file, >>> #define MVM_DEBUGFS_READ_WRITE_FILE_OPS(name, bufsz) \ >>> _MVM_DEBUGFS_READ_WRITE_FILE_OPS(name, bufsz, struct ieee80211_vif) >>> #define MVM_DEBUGFS_ADD_FILE_VIF(name, parent, mode) do { \ >>> - if (!debugfs_create_file(#name, mode, parent, vif, \ >>> - &iwl_dbgfs_##name##_ops)) \ >>> - goto err; \ >>> + debugfs_create_file(#name, mode, parent, vif, \ >>> + &iwl_dbgfs_##name##_ops); \ >>> } while (0) >>> MVM_DEBUGFS_READ_FILE_OPS(mac_params); >>> @@ -1483,12 +1482,6 @@ void iwl_mvm_vif_dbgfs_register(struct iwl_mvm *mvm, struct ieee80211_vif *vif) >>> mvmvif->dbgfs_dir = debugfs_create_dir("iwlmvm", dbgfs_dir); >>> - if (!mvmvif->dbgfs_dir) { >>> - IWL_ERR(mvm, "Failed to create debugfs directory under %pd\n", >>> - dbgfs_dir); >>> - return; >>> - } >>> - >>> if (iwlmvm_mod_params.power_scheme != IWL_POWER_SCHEME_CAM && >>> ((vif->type == NL80211_IFTYPE_STATION && !vif->p2p) || >>> (vif->type == NL80211_IFTYPE_STATION && vif->p2p))) >>> @@ -1537,12 +1530,6 @@ void iwl_mvm_vif_dbgfs_register(struct iwl_mvm *mvm, struct ieee80211_vif *vif) >>> mvmvif->dbgfs_slink = debugfs_create_symlink(dbgfs_dir->d_name.name, >>> mvm->debugfs_dir, buf); >>> - if (!mvmvif->dbgfs_slink) >>> - IWL_ERR(mvm, "Can't create debugfs symbolic link under %pd\n", >>> - dbgfs_dir); >>> - return; >>> -err: >>> - IWL_ERR(mvm, "Can't create debugfs entity\n"); >>> } >> >> Fedora got a bug report https://bugzilla.redhat.com/show_bug.cgi?id=1691034 >> of a crash with 5.0 and the user did a bisect which pointed to ff9fb72bc077 >> ("debugfs: return error values, not NULL") because the error checking is >> no longer correct in this driver. >> >> Based on https://patchwork.kernel.org/patch/10865839/, it looks like >> this is supposed to go in for 5.2 but this needs to go in now as >> the error checking is currently broken without it. Can this get queued >> for Linus so we can get it in 5.0 stable? > > That's odd, I can't see how the error checking is wrong here. If the > directory is not created, an error will be returned, which should be > able to be handled by debugfs_create_file(). > > So with this patch does the error go away? >
The full patch didn't apply cleanly and I didn't try to backport it for the reporter to test. I was going off of the theory that if the patch was there it would fix the problem.
What I _think_ is going wrong is dbgfs_dir is actually an errno value:
struct dentry *dbgfs_dir = vif->debugfs_dir; struct iwl_mvm_vif *mvmvif = iwl_mvm_vif_from_mac80211(vif); char buf[100];
/* * Check if debugfs directory already exist before creating it. * This may happen when, for example, resetting hw or suspend-resume */ if (!dbgfs_dir || mvmvif->dbgfs_dir) return;
so this blows up in the snprintf
snprintf(buf, 100, "../../../%pd3/%pd", dbgfs_dir, mvmvif->dbgfs_dir);
Unless I misunderstood what the debugfs error change did. I think this also means the if check needs to look for IS_ERR and not just !dbgfs_dir.
Thanks, Laura
> strange, > > greg k-h >
| |