| Message ID | 20220719143302.2071223-5-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | wcn36xx: Add in debugfs export of firmware feature bits | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, 19 Jul 2022 at 16:33, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > Add in the ability to easily find the firmware feature bits reported in the > get feature exchange without having to compile-in debug prints. > > root@linaro-alip:~# cat /sys/kernel/debug/ieee80211/phy0/wcn36xx/firmware_feat_caps > MCC > P2P > DOT11AC > SLM_SESSIONIZATION > DOT11AC_OPMODE > SAP32STA > TDLS > P2P_GO_NOA_DECOUPLE_INIT_SCAN > WLANACTIVE_OFFLOAD > BEACON_OFFLOAD > SCAN_OFFLOAD > BCN_MISS_OFFLOAD > STA_POWERSAVE > STA_ADVANCED_PWRSAVE > BCN_FILTER > RTT > RATECTRL > WOW > WLAN_ROAM_SCAN_OFFLOAD > SPECULATIVE_PS_POLL > IBSS_HEARTBEAT_OFFLOAD > WLAN_SCAN_OFFLOAD > WLAN_PERIODIC_TX_PTRN > ADVANCE_TDLS > BATCH_SCAN > FW_IN_TX_PATH > EXTENDED_NSOFFLOAD_SLOT > CH_SWITCH_V1 > HT40_OBSS_SCAN > UPDATE_CHANNEL_LIST > WLAN_MCADDR_FLT > WLAN_CH144 > TDLS_SCAN_COEXISTENCE > LINK_LAYER_STATS_MEAS > MU_MIMO > EXTENDED_SCAN > DYNAMIC_WMM_PS > MAC_SPOOFED_SCAN > FW_STATS > WPS_PRBRSP_TMPL > BCN_IE_FLT_DELTA > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Reviewed-by: Loic Poulain <loic.poulain@linaro.org> > --- > drivers/net/wireless/ath/wcn36xx/debug.c | 37 ++++++++++++++++++++++++ > drivers/net/wireless/ath/wcn36xx/debug.h | 1 + > 2 files changed, 38 insertions(+) > > diff --git a/drivers/net/wireless/ath/wcn36xx/debug.c b/drivers/net/wireless/ath/wcn36xx/debug.c > index 6af306ae41ad..220f338045bd 100644 > --- a/drivers/net/wireless/ath/wcn36xx/debug.c > +++ b/drivers/net/wireless/ath/wcn36xx/debug.c > @@ -21,6 +21,7 @@ > #include "wcn36xx.h" > #include "debug.h" > #include "pmc.h" > +#include "firmware.h" > > #ifdef CONFIG_WCN36XX_DEBUGFS > > @@ -136,6 +137,40 @@ static const struct file_operations fops_wcn36xx_dump = { > .write = write_file_dump, > }; > > +static ssize_t read_file_firmware_feature_caps(struct file *file, > + char __user *user_buf, > + size_t count, loff_t *ppos) > +{ > + struct wcn36xx *wcn = file->private_data; > + unsigned long page = get_zeroed_page(GFP_KERNEL); > + char *p = (char *)page; > + int i; > + int ret; > + > + if (!p) > + return -ENOMEM; > + > + mutex_lock(&wcn->hal_mutex); > + for (i = 0; i < MAX_FEATURE_SUPPORTED; i++) { > + if (wcn36xx_firmware_get_feat_caps(wcn->fw_feat_caps, i)) { > + p += sprintf(p, "%s\n", > + wcn36xx_firmware_get_cap_name(i)); > + } > + } > + mutex_unlock(&wcn->hal_mutex); > + > + ret = simple_read_from_buffer(user_buf, count, ppos, (char *)page, > + (unsigned long)p - page); > + > + free_page(page); > + return ret; > +} > + > +static const struct file_operations fops_wcn36xx_firmware_feat_caps = { > + .open = simple_open, > + .read = read_file_firmware_feature_caps, > +}; > + > #define ADD_FILE(name, mode, fop, priv_data) \ > do { \ > struct dentry *d; \ > @@ -163,6 +198,8 @@ void wcn36xx_debugfs_init(struct wcn36xx *wcn) > > ADD_FILE(bmps_switcher, 0600, &fops_wcn36xx_bmps, wcn); > ADD_FILE(dump, 0200, &fops_wcn36xx_dump, wcn); > + ADD_FILE(firmware_feat_caps, 0200, > + &fops_wcn36xx_firmware_feat_caps, wcn); > } > > void wcn36xx_debugfs_exit(struct wcn36xx *wcn) > diff --git a/drivers/net/wireless/ath/wcn36xx/debug.h b/drivers/net/wireless/ath/wcn36xx/debug.h > index 46307aa562d3..7116d96e0543 100644 > --- a/drivers/net/wireless/ath/wcn36xx/debug.h > +++ b/drivers/net/wireless/ath/wcn36xx/debug.h > @@ -31,6 +31,7 @@ struct wcn36xx_dfs_entry { > struct dentry *rootdir; > struct wcn36xx_dfs_file file_bmps_switcher; > struct wcn36xx_dfs_file file_dump; > + struct wcn36xx_dfs_file file_firmware_feat_caps; > }; > > void wcn36xx_debugfs_init(struct wcn36xx *wcn); > -- > 2.36.1 >
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > Add in the ability to easily find the firmware feature bits reported in the > get feature exchange without having to compile-in debug prints. > > root@linaro-alip:~# cat /sys/kernel/debug/ieee80211/phy0/wcn36xx/firmware_feat_caps > MCC > P2P > DOT11AC > SLM_SESSIONIZATION > DOT11AC_OPMODE > SAP32STA > TDLS > P2P_GO_NOA_DECOUPLE_INIT_SCAN > WLANACTIVE_OFFLOAD > BEACON_OFFLOAD > SCAN_OFFLOAD > BCN_MISS_OFFLOAD > STA_POWERSAVE > STA_ADVANCED_PWRSAVE > BCN_FILTER > RTT > RATECTRL > WOW > WLAN_ROAM_SCAN_OFFLOAD > SPECULATIVE_PS_POLL > IBSS_HEARTBEAT_OFFLOAD > WLAN_SCAN_OFFLOAD > WLAN_PERIODIC_TX_PTRN > ADVANCE_TDLS > BATCH_SCAN > FW_IN_TX_PATH > EXTENDED_NSOFFLOAD_SLOT > CH_SWITCH_V1 > HT40_OBSS_SCAN > UPDATE_CHANNEL_LIST > WLAN_MCADDR_FLT > WLAN_CH144 > TDLS_SCAN_COEXISTENCE > LINK_LAYER_STATS_MEAS > MU_MIMO > EXTENDED_SCAN > DYNAMIC_WMM_PS > MAC_SPOOFED_SCAN > FW_STATS > WPS_PRBRSP_TMPL > BCN_IE_FLT_DELTA > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> [...] > +static ssize_t read_file_firmware_feature_caps(struct file *file, > + char __user *user_buf, > + size_t count, loff_t *ppos) > +{ > + struct wcn36xx *wcn = file->private_data; > + unsigned long page = get_zeroed_page(GFP_KERNEL); > + char *p = (char *)page; > + int i; > + int ret; > + > + if (!p) > + return -ENOMEM; > + > + mutex_lock(&wcn->hal_mutex); > + for (i = 0; i < MAX_FEATURE_SUPPORTED; i++) { > + if (wcn36xx_firmware_get_feat_caps(wcn->fw_feat_caps, i)) { > + p += sprintf(p, "%s\n", > + wcn36xx_firmware_get_cap_name(i)); > + } > + } > + mutex_unlock(&wcn->hal_mutex); > + > + ret = simple_read_from_buffer(user_buf, count, ppos, (char *)page, > + (unsigned long)p - page); > + > + free_page(page); > + return ret; > +} Why not use the normal use kzalloc() and kfree()? That way you would not need a separate page variable. What's the benefit from get_zeroed_page()? Also I don't see any checks for a memory allocation failure.
On 27/07/2022 11:31, Kalle Valo wrote: > Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: >> +static ssize_t read_file_firmware_feature_caps(struct file *file, >> + char __user *user_buf, >> + size_t count, loff_t *ppos) >> +{ >> + struct wcn36xx *wcn = file->private_data; >> + unsigned long page = get_zeroed_page(GFP_KERNEL); >> + char *p = (char *)page; >> + int i; >> + int ret; >> + >> + if (!p) >> + return -ENOMEM; >> + >> + mutex_lock(&wcn->hal_mutex); >> + for (i = 0; i < MAX_FEATURE_SUPPORTED; i++) { >> + if (wcn36xx_firmware_get_feat_caps(wcn->fw_feat_caps, i)) { >> + p += sprintf(p, "%s\n", >> + wcn36xx_firmware_get_cap_name(i)); >> + } >> + } >> + mutex_unlock(&wcn->hal_mutex); >> + >> + ret = simple_read_from_buffer(user_buf, count, ppos, (char *)page, >> + (unsigned long)p - page); >> + >> + free_page(page); >> + return ret; >> +} > > Why not use the normal use kzalloc() and kfree()? That way you would not > need a separate page variable. What's the benefit from > get_zeroed_page()? TBH I did a copy/paste here from another driver... I forget which > > Also I don't see any checks for a memory allocation failure. > its there char *p = (char*) page; if (!p) return -ENOMEM; I can V2 this for kzalloc and kfree if you prefer though --- bod
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > On 27/07/2022 11:31, Kalle Valo wrote: >> Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > >>> +static ssize_t read_file_firmware_feature_caps(struct file *file, >>> + char __user *user_buf, >>> + size_t count, loff_t *ppos) >>> +{ >>> + struct wcn36xx *wcn = file->private_data; >>> + unsigned long page = get_zeroed_page(GFP_KERNEL); >>> + char *p = (char *)page; >>> + int i; >>> + int ret; >>> + >>> + if (!p) >>> + return -ENOMEM; >>> + >>> + mutex_lock(&wcn->hal_mutex); >>> + for (i = 0; i < MAX_FEATURE_SUPPORTED; i++) { >>> + if (wcn36xx_firmware_get_feat_caps(wcn->fw_feat_caps, i)) { >>> + p += sprintf(p, "%s\n", >>> + wcn36xx_firmware_get_cap_name(i)); >>> + } >>> + } >>> + mutex_unlock(&wcn->hal_mutex); >>> + >>> + ret = simple_read_from_buffer(user_buf, count, ppos, (char *)page, >>> + (unsigned long)p - page); >>> + >>> + free_page(page); >>> + return ret; >>> +} >> >> Why not use the normal use kzalloc() and kfree()? That way you would not >> need a separate page variable. What's the benefit from >> get_zeroed_page()? > > > TBH I did a copy/paste here from another driver... I forget which >> >> Also I don't see any checks for a memory allocation failure. >> > > its there > > char *p = (char*) page; > > if (!p) > return -ENOMEM; Ah, it's pretty evil to have the error handling so far away from the actual call :) > I can V2 this for kzalloc and kfree if you prefer though Yes, please do that. We should use standard infrastructure as much as possible.
diff --git a/drivers/net/wireless/ath/wcn36xx/debug.c b/drivers/net/wireless/ath/wcn36xx/debug.c index 6af306ae41ad..220f338045bd 100644 --- a/drivers/net/wireless/ath/wcn36xx/debug.c +++ b/drivers/net/wireless/ath/wcn36xx/debug.c @@ -21,6 +21,7 @@ #include "wcn36xx.h" #include "debug.h" #include "pmc.h" +#include "firmware.h" #ifdef CONFIG_WCN36XX_DEBUGFS @@ -136,6 +137,40 @@ static const struct file_operations fops_wcn36xx_dump = { .write = write_file_dump, }; +static ssize_t read_file_firmware_feature_caps(struct file *file, + char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct wcn36xx *wcn = file->private_data; + unsigned long page = get_zeroed_page(GFP_KERNEL); + char *p = (char *)page; + int i; + int ret; + + if (!p) + return -ENOMEM; + + mutex_lock(&wcn->hal_mutex); + for (i = 0; i < MAX_FEATURE_SUPPORTED; i++) { + if (wcn36xx_firmware_get_feat_caps(wcn->fw_feat_caps, i)) { + p += sprintf(p, "%s\n", + wcn36xx_firmware_get_cap_name(i)); + } + } + mutex_unlock(&wcn->hal_mutex); + + ret = simple_read_from_buffer(user_buf, count, ppos, (char *)page, + (unsigned long)p - page); + + free_page(page); + return ret; +} + +static const struct file_operations fops_wcn36xx_firmware_feat_caps = { + .open = simple_open, + .read = read_file_firmware_feature_caps, +}; + #define ADD_FILE(name, mode, fop, priv_data) \ do { \ struct dentry *d; \ @@ -163,6 +198,8 @@ void wcn36xx_debugfs_init(struct wcn36xx *wcn) ADD_FILE(bmps_switcher, 0600, &fops_wcn36xx_bmps, wcn); ADD_FILE(dump, 0200, &fops_wcn36xx_dump, wcn); + ADD_FILE(firmware_feat_caps, 0200, + &fops_wcn36xx_firmware_feat_caps, wcn); } void wcn36xx_debugfs_exit(struct wcn36xx *wcn) diff --git a/drivers/net/wireless/ath/wcn36xx/debug.h b/drivers/net/wireless/ath/wcn36xx/debug.h index 46307aa562d3..7116d96e0543 100644 --- a/drivers/net/wireless/ath/wcn36xx/debug.h +++ b/drivers/net/wireless/ath/wcn36xx/debug.h @@ -31,6 +31,7 @@ struct wcn36xx_dfs_entry { struct dentry *rootdir; struct wcn36xx_dfs_file file_bmps_switcher; struct wcn36xx_dfs_file file_dump; + struct wcn36xx_dfs_file file_firmware_feat_caps; }; void wcn36xx_debugfs_init(struct wcn36xx *wcn);
Add in the ability to easily find the firmware feature bits reported in the get feature exchange without having to compile-in debug prints. root@linaro-alip:~# cat /sys/kernel/debug/ieee80211/phy0/wcn36xx/firmware_feat_caps MCC P2P DOT11AC SLM_SESSIONIZATION DOT11AC_OPMODE SAP32STA TDLS P2P_GO_NOA_DECOUPLE_INIT_SCAN WLANACTIVE_OFFLOAD BEACON_OFFLOAD SCAN_OFFLOAD BCN_MISS_OFFLOAD STA_POWERSAVE STA_ADVANCED_PWRSAVE BCN_FILTER RTT RATECTRL WOW WLAN_ROAM_SCAN_OFFLOAD SPECULATIVE_PS_POLL IBSS_HEARTBEAT_OFFLOAD WLAN_SCAN_OFFLOAD WLAN_PERIODIC_TX_PTRN ADVANCE_TDLS BATCH_SCAN FW_IN_TX_PATH EXTENDED_NSOFFLOAD_SLOT CH_SWITCH_V1 HT40_OBSS_SCAN UPDATE_CHANNEL_LIST WLAN_MCADDR_FLT WLAN_CH144 TDLS_SCAN_COEXISTENCE LINK_LAYER_STATS_MEAS MU_MIMO EXTENDED_SCAN DYNAMIC_WMM_PS MAC_SPOOFED_SCAN FW_STATS WPS_PRBRSP_TMPL BCN_IE_FLT_DELTA Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/net/wireless/ath/wcn36xx/debug.c | 37 ++++++++++++++++++++++++ drivers/net/wireless/ath/wcn36xx/debug.h | 1 + 2 files changed, 38 insertions(+)