| Message ID | 20200914031634.190721-3-wright.feng@cypress.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | brcmfmac: SoftAP creation and dcmd buffer size changes | expand |
Wright Feng <wright.feng@cypress.com> writes: > From: Kurt Lee <kurt.lee@cypress.com> > > In manufacturing line, test tool may be used to enable SoftAP. Such > SoftAP can't pass traffic because netif carrier is off by default. To > allow such use case, let brcmfmac parse ioctl cmd, and then set iftype > to ap mode and report netif_carrier_on to upper layer. nl80211 does not use ioctl(), so the commit log is misleading. > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c > @@ -64,6 +64,15 @@ static int brcmf_cfg80211_vndr_cmds_dcmd_handler(struct wiphy *wiphy, > *(char *)(dcmd_buf + len) = '\0'; > } > > + if (cmdhdr->cmd == BRCMF_C_SET_AP) { > + if (*(int *)(dcmd_buf) == 1) { > + ifp->vif->wdev.iftype = NL80211_IFTYPE_AP; > + brcmf_net_setcarrier(ifp, true); > + } else { > + ifp->vif->wdev.iftype = NL80211_IFTYPE_STATION; > + } > + } We now have rules for the vendor API: https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api As the BRCMF_VNDR_CMDS_DCMD just seems to be a simple pipe between the user space and the firmware I'm leaning towards that we should just remove support for that command from brcmfmac. And besides, for factory tests you should be using NL80211_CMD_TESTMODE. The vendor API is meant for normal mode usage.
On 9/14/2020 10:20 AM, Kalle Valo wrote: > Wright Feng <wright.feng@cypress.com> writes: > >> From: Kurt Lee <kurt.lee@cypress.com> >> >> In manufacturing line, test tool may be used to enable SoftAP. Such >> SoftAP can't pass traffic because netif carrier is off by default. To >> allow such use case, let brcmfmac parse ioctl cmd, and then set iftype >> to ap mode and report netif_carrier_on to upper layer. > > nl80211 does not use ioctl(), so the commit log is misleading. > >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c >> @@ -64,6 +64,15 @@ static int brcmf_cfg80211_vndr_cmds_dcmd_handler(struct wiphy *wiphy, >> *(char *)(dcmd_buf + len) = '\0'; >> } >> >> + if (cmdhdr->cmd == BRCMF_C_SET_AP) { >> + if (*(int *)(dcmd_buf) == 1) { >> + ifp->vif->wdev.iftype = NL80211_IFTYPE_AP; >> + brcmf_net_setcarrier(ifp, true); >> + } else { >> + ifp->vif->wdev.iftype = NL80211_IFTYPE_STATION; >> + } >> + } > > We now have rules for the vendor API: > > https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api > > As the BRCMF_VNDR_CMDS_DCMD just seems to be a simple pipe between the > user space and the firmware I'm leaning towards that we should just > remove support for that command from brcmfmac. > > And besides, for factory tests you should be using NL80211_CMD_TESTMODE. > The vendor API is meant for normal mode usage. We talked about this API before. This command used to be a TESTMODE command, but with the introduction of vendor commands in cfg80211 I changed that. My reasons being that 1) it helps me in cfg80211 callback development, and 2) (some of) our customers were not accepting the need for a separate driver (build) for manufacturing. My intent has always been to keep this opaque for the driver so I am opposed to patches like this that are interpreting the opaque blob. The firmware could simply generate a CARRIER event and the driver can act on that. Regards, Arend
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c index d07e7c7355d9..5edf5ac1167a 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/vendor.c @@ -64,6 +64,15 @@ static int brcmf_cfg80211_vndr_cmds_dcmd_handler(struct wiphy *wiphy, *(char *)(dcmd_buf + len) = '\0'; } + if (cmdhdr->cmd == BRCMF_C_SET_AP) { + if (*(int *)(dcmd_buf) == 1) { + ifp->vif->wdev.iftype = NL80211_IFTYPE_AP; + brcmf_net_setcarrier(ifp, true); + } else { + ifp->vif->wdev.iftype = NL80211_IFTYPE_STATION; + } + } + if (cmdhdr->set) ret = brcmf_fil_cmd_data_set(ifp, cmdhdr->cmd, dcmd_buf, ret_len);