| Message ID | 20180511101526.10734-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On 5/11/2018 12:15 PM, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > This allows reading all capabilities as reported by a firmware. They are > printed using native (raw) names, just like developers like it the most. > It's how firmware reports support for various features, e.g. supported > modes, supported standards, power saving details, max BSS-es. > > Access to all that info is useful for trying new firmwares, comparing > them and debugging features AKA bugs. What are you implying :-p ? Some comments below. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 36 ++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c > index 876731c57bf5..782121cb9399 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c > @@ -165,6 +165,41 @@ static void brcmf_feat_firmware_capabilities(struct brcmf_if *ifp) > } > } > > +/** > + * brcmf_cap_read() - expose firmware capabilities to debugfs. Please stick to naming convention brcmf_<module>_foo(), ie. brcmf_feat_cap_read(). > + * > + * @seq: sequence for debugfs entry. > + * @data: raw data pointer. > + */ > +static int brcmf_cap_read(struct seq_file *seq, void *data) > +{ > + struct brcmf_bus *bus_if = dev_get_drvdata(seq->private); > + struct brcmf_if *ifp = brcmf_get_ifp(bus_if->drvr, 0); > + char caps[MAX_CAPS_BUFFER_SIZE + 1] = { }; > + char *tmp; > + int err; > + > + err = brcmf_fil_iovar_data_get(ifp, "cap", caps, sizeof(caps)); > + if (err) { > + brcmf_err("could not get firmware cap (%d)\n", err); > + return err; > + } > + > + /* Put every capability in a new line */ > + for (tmp = caps; *tmp; tmp++) { > + if (*tmp == ' ') > + *tmp = '\n'; > + } > + > + /* Usually there is a space at the end of capabilities string */ > + seq_printf(seq, "%s", caps); > + /* So make sure we don't print two line breaks */ > + if (tmp > caps && *(tmp - 1) != '\n') > + seq_printf(seq, "\n"); > + > + return 0; > +} > + > void brcmf_feat_attach(struct brcmf_pub *drvr) > { > struct brcmf_if *ifp = brcmf_get_ifp(drvr, 0); > @@ -233,6 +268,7 @@ void brcmf_feat_attach(struct brcmf_pub *drvr) > void brcmf_feat_debugfs_create(struct brcmf_pub *drvr) > { > brcmf_debugfs_add_entry(drvr, "features", brcmf_feat_debugfs_read); > + brcmf_debugfs_add_entry(drvr, "cap", brcmf_cap_read); Prefer to name the debugfs entry "fwcap".
On 2018-05-13 20:59, Arend van Spriel wrote: > On 5/11/2018 12:15 PM, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> This allows reading all capabilities as reported by a firmware. They >> are >> printed using native (raw) names, just like developers like it the >> most. >> It's how firmware reports support for various features, e.g. supported >> modes, supported standards, power saving details, max BSS-es. >> >> Access to all that info is useful for trying new firmwares, comparing >> them and debugging features AKA bugs. > > What are you implying :-p ? Some comments below. ;) For real I was trying to figure out supported modes. It appears e.g. many/most firmwares support up to 8 MBSS ("mbss8") while brcmfmac allows up to 4 only. > Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 36 >> ++++++++++++++++++++++ >> 1 file changed, 36 insertions(+) >> >> diff --git >> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c >> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c >> index 876731c57bf5..782121cb9399 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c >> @@ -165,6 +165,41 @@ static void >> brcmf_feat_firmware_capabilities(struct brcmf_if *ifp) >> } >> } >> >> +/** >> + * brcmf_cap_read() - expose firmware capabilities to debugfs. > > Please stick to naming convention brcmf_<module>_foo(), ie. > brcmf_feat_cap_read(). Will do, thanks! I missed that naming convention because of brcmf_debugfs_fws_stats_read() / brcmf_debugfs_sdio_count_read() which slightly confused me. I see your point now! >> + * >> + * @seq: sequence for debugfs entry. >> + * @data: raw data pointer. >> + */ >> +static int brcmf_cap_read(struct seq_file *seq, void *data) >> +{ >> + struct brcmf_bus *bus_if = dev_get_drvdata(seq->private); >> + struct brcmf_if *ifp = brcmf_get_ifp(bus_if->drvr, 0); >> + char caps[MAX_CAPS_BUFFER_SIZE + 1] = { }; >> + char *tmp; >> + int err; >> + >> + err = brcmf_fil_iovar_data_get(ifp, "cap", caps, sizeof(caps)); >> + if (err) { >> + brcmf_err("could not get firmware cap (%d)\n", err); >> + return err; >> + } >> + >> + /* Put every capability in a new line */ >> + for (tmp = caps; *tmp; tmp++) { >> + if (*tmp == ' ') >> + *tmp = '\n'; >> + } >> + >> + /* Usually there is a space at the end of capabilities string */ >> + seq_printf(seq, "%s", caps); >> + /* So make sure we don't print two line breaks */ >> + if (tmp > caps && *(tmp - 1) != '\n') >> + seq_printf(seq, "\n"); >> + >> + return 0; >> +} >> + >> void brcmf_feat_attach(struct brcmf_pub *drvr) >> { >> struct brcmf_if *ifp = brcmf_get_ifp(drvr, 0); >> @@ -233,6 +268,7 @@ void brcmf_feat_attach(struct brcmf_pub *drvr) >> void brcmf_feat_debugfs_create(struct brcmf_pub *drvr) >> { >> brcmf_debugfs_add_entry(drvr, "features", brcmf_feat_debugfs_read); >> + brcmf_debugfs_add_entry(drvr, "cap", brcmf_cap_read); > > Prefer to name the debugfs entry "fwcap". Sure.
On 5/14/2018 7:11 AM, Rafał Miłecki wrote: >>> +/** >>> + * brcmf_cap_read() - expose firmware capabilities to debugfs. >> >> Please stick to naming convention brcmf_<module>_foo(), ie. >> brcmf_feat_cap_read(). > > Will do, thanks! I missed that naming convention because of > brcmf_debugfs_fws_stats_read() / brcmf_debugfs_sdio_count_read() which > slightly confused me. I see your point now! Yeah. Sorry for the confusion. There is always room to improve. Gr. AvS
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c index 876731c57bf5..782121cb9399 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c @@ -165,6 +165,41 @@ static void brcmf_feat_firmware_capabilities(struct brcmf_if *ifp) } } +/** + * brcmf_cap_read() - expose firmware capabilities to debugfs. + * + * @seq: sequence for debugfs entry. + * @data: raw data pointer. + */ +static int brcmf_cap_read(struct seq_file *seq, void *data) +{ + struct brcmf_bus *bus_if = dev_get_drvdata(seq->private); + struct brcmf_if *ifp = brcmf_get_ifp(bus_if->drvr, 0); + char caps[MAX_CAPS_BUFFER_SIZE + 1] = { }; + char *tmp; + int err; + + err = brcmf_fil_iovar_data_get(ifp, "cap", caps, sizeof(caps)); + if (err) { + brcmf_err("could not get firmware cap (%d)\n", err); + return err; + } + + /* Put every capability in a new line */ + for (tmp = caps; *tmp; tmp++) { + if (*tmp == ' ') + *tmp = '\n'; + } + + /* Usually there is a space at the end of capabilities string */ + seq_printf(seq, "%s", caps); + /* So make sure we don't print two line breaks */ + if (tmp > caps && *(tmp - 1) != '\n') + seq_printf(seq, "\n"); + + return 0; +} + void brcmf_feat_attach(struct brcmf_pub *drvr) { struct brcmf_if *ifp = brcmf_get_ifp(drvr, 0); @@ -233,6 +268,7 @@ void brcmf_feat_attach(struct brcmf_pub *drvr) void brcmf_feat_debugfs_create(struct brcmf_pub *drvr) { brcmf_debugfs_add_entry(drvr, "features", brcmf_feat_debugfs_read); + brcmf_debugfs_add_entry(drvr, "cap", brcmf_cap_read); } bool brcmf_feat_is_enabled(struct brcmf_if *ifp, enum brcmf_feat_id id)