| Message ID | 20240909193035.69823-8-marex@denx.de (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v5,1/9] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string | expand |
On 9/9/24 21:29, Marek Vasut wrote: > Register wiphy after reading out chipid, so the chipid can be > used to determine chip features and not advertise WPA3/SAE > support to userspace on WILC3000. Note that wilc_netdev_cleanup() > will deregister the wiphy in fail path. > > Signed-off-by: Marek Vasut <marex@denx.de> [...] > @@ -1804,14 +1803,8 @@ static struct wilc *wilc_create_wiphy(struct device *dev) > BIT(NL80211_IFTYPE_P2P_GO) | > BIT(NL80211_IFTYPE_P2P_CLIENT); > wiphy->flags |= WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL; > - wiphy->features |= NL80211_FEATURE_SAE; > set_wiphy_dev(wiphy, dev); > wl->wiphy = wiphy; > - ret = wiphy_register(wiphy); > - if (ret) { > - wiphy_free(wiphy); > - return NULL; > - } If I am reading the patch correctly, there are still some failure paths in wilc_cfg80211_init which try to call wiphy_unregister on the (not registered anymore in there) wphy. > return wl; > } > > @@ -1861,6 +1854,14 @@ int wilc_cfg80211_init(struct wilc **wilc, struct device *dev, int io_type, > } > EXPORT_SYMBOL_GPL(wilc_cfg80211_init); > > +int wilc_cfg80211_register(struct wilc *wilc) > +{ > + wilc->wiphy->features |= NL80211_FEATURE_SAE; Even if I get the general need, it feels weird to have parts of the wphy init performed in wilc_create_wiphy, and some parts (the features field) here. Wouldn't it work to just move wilc_create_wiphy content here, since wphy will not be usable anyway before eventually registering it ? Alexis
On 9/10/24 12:08 PM, Alexis Lothoré wrote: > On 9/9/24 21:29, Marek Vasut wrote: >> Register wiphy after reading out chipid, so the chipid can be >> used to determine chip features and not advertise WPA3/SAE >> support to userspace on WILC3000. Note that wilc_netdev_cleanup() >> will deregister the wiphy in fail path. >> >> Signed-off-by: Marek Vasut <marex@denx.de> > > [...] > >> @@ -1804,14 +1803,8 @@ static struct wilc *wilc_create_wiphy(struct device *dev) >> BIT(NL80211_IFTYPE_P2P_GO) | >> BIT(NL80211_IFTYPE_P2P_CLIENT); >> wiphy->flags |= WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL; >> - wiphy->features |= NL80211_FEATURE_SAE; >> set_wiphy_dev(wiphy, dev); >> wl->wiphy = wiphy; >> - ret = wiphy_register(wiphy); >> - if (ret) { >> - wiphy_free(wiphy); >> - return NULL; >> - } > > If I am reading the patch correctly, there are still some failure paths in > wilc_cfg80211_init which try to call wiphy_unregister on the (not registered > anymore in there) wphy. >> return wl; >> } >> >> @@ -1861,6 +1854,14 @@ int wilc_cfg80211_init(struct wilc **wilc, struct device *dev, int io_type, >> } >> EXPORT_SYMBOL_GPL(wilc_cfg80211_init); >> >> +int wilc_cfg80211_register(struct wilc *wilc) >> +{ >> + wilc->wiphy->features |= NL80211_FEATURE_SAE; > > Even if I get the general need, it feels weird to have parts of the wphy init > performed in wilc_create_wiphy, and some parts (the features field) here. > Wouldn't it work to just move wilc_create_wiphy content here, since wphy will > not be usable anyway before eventually registering it ? That's what I thought initially too, but look closely at wilc_create_wiphy(): struct wilc *wilc_create_wiphy(struct device *dev) { ... struct wiphy *wiphy; struct wilc *wl; ... wiphy = wiphy_new(&wilc_cfg80211_ops, sizeof(*wl)); ... wl = wiphy_priv(wiphy); // <----------- HERE , *wl is struct wilc ... return wl; } That 'struct wilc' is allocated as part of wiphy_new() and used all around the place before we reach wiphy_register() much later on.
On 9/10/24 12:53, Marek Vasut wrote: > On 9/10/24 12:08 PM, Alexis Lothoré wrote: >> On 9/9/24 21:29, Marek Vasut wrote: [...] >>> EXPORT_SYMBOL_GPL(wilc_cfg80211_init); >>> +int wilc_cfg80211_register(struct wilc *wilc) >>> +{ >>> + wilc->wiphy->features |= NL80211_FEATURE_SAE; >> >> Even if I get the general need, it feels weird to have parts of the wphy init >> performed in wilc_create_wiphy, and some parts (the features field) here. >> Wouldn't it work to just move wilc_create_wiphy content here, since wphy will >> not be usable anyway before eventually registering it ? > That's what I thought initially too, but look closely at wilc_create_wiphy(): > > struct wilc *wilc_create_wiphy(struct device *dev) > { > ... > struct wiphy *wiphy; > struct wilc *wl; > ... > wiphy = wiphy_new(&wilc_cfg80211_ops, sizeof(*wl)); > ... > wl = wiphy_priv(wiphy); // <----------- HERE , *wl is struct wilc > ... > return wl; > } > > That 'struct wilc' is allocated as part of wiphy_new() and used all around the > place before we reach wiphy_register() much later on. Meh, true. We could still let any part affecting the struct wilc in wilc_create_wiphy, and move any wphy configuration in wilc_cfg80211_register, but then I am not sure anymore if it makes things better.
diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c index 11e0f8a473467..30015c5920467 100644 --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c @@ -1761,7 +1761,6 @@ static struct wilc *wilc_create_wiphy(struct device *dev) { struct wiphy *wiphy; struct wilc *wl; - int ret; wiphy = wiphy_new(&wilc_cfg80211_ops, sizeof(*wl)); if (!wiphy) @@ -1804,14 +1803,8 @@ static struct wilc *wilc_create_wiphy(struct device *dev) BIT(NL80211_IFTYPE_P2P_GO) | BIT(NL80211_IFTYPE_P2P_CLIENT); wiphy->flags |= WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL; - wiphy->features |= NL80211_FEATURE_SAE; set_wiphy_dev(wiphy, dev); wl->wiphy = wiphy; - ret = wiphy_register(wiphy); - if (ret) { - wiphy_free(wiphy); - return NULL; - } return wl; } @@ -1861,6 +1854,14 @@ int wilc_cfg80211_init(struct wilc **wilc, struct device *dev, int io_type, } EXPORT_SYMBOL_GPL(wilc_cfg80211_init); +int wilc_cfg80211_register(struct wilc *wilc) +{ + wilc->wiphy->features |= NL80211_FEATURE_SAE; + + return wiphy_register(wilc->wiphy); +} +EXPORT_SYMBOL_GPL(wilc_cfg80211_register); + int wilc_init_host_int(struct net_device *net) { int ret; diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.h b/drivers/net/wireless/microchip/wilc1000/cfg80211.h index fc04cc6615c1f..2dc9c1c42d609 100644 --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.h +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.h @@ -10,6 +10,7 @@ int wilc_cfg80211_init(struct wilc **wilc, struct device *dev, int io_type, const struct wilc_hif_func *ops); +int wilc_cfg80211_register(struct wilc *wilc); void wilc_deinit_host_int(struct net_device *net); int wilc_init_host_int(struct net_device *net); void wilc_wfi_monitor_rx(struct net_device *mon_dev, u8 *buff, u32 size); diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c index 01a96d0f562a6..0240f66e70bc1 100644 --- a/drivers/net/wireless/microchip/wilc1000/sdio.c +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c @@ -181,6 +181,10 @@ static int wilc_sdio_probe(struct sdio_func *func, if (ret) goto clk_disable_unprepare; + ret = wilc_cfg80211_register(wilc); + if (ret) + goto clk_disable_unprepare; + ret = wilc_load_mac_from_nv(wilc); if (ret) { pr_err("Can not retrieve MAC address from chip\n"); diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c index 1b60a13df6cfa..edc518c3b0253 100644 --- a/drivers/net/wireless/microchip/wilc1000/spi.c +++ b/drivers/net/wireless/microchip/wilc1000/spi.c @@ -250,6 +250,10 @@ static int wilc_bus_probe(struct spi_device *spi) if (ret) goto power_down; + ret = wilc_cfg80211_register(wilc); + if (ret) + goto power_down; + ret = wilc_load_mac_from_nv(wilc); if (ret) { pr_err("Can not retrieve MAC address from chip\n");
Register wiphy after reading out chipid, so the chipid can be used to determine chip features and not advertise WPA3/SAE support to userspace on WILC3000. Note that wilc_netdev_cleanup() will deregister the wiphy in fail path. Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: "David S. Miller" <davem@davemloft.net> Cc: Adham Abozaeid <adham.abozaeid@microchip.com> Cc: Ajay Singh <ajay.kathat@microchip.com> Cc: Alexis Lothoré <alexis.lothore@bootlin.com> Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev> Cc: Conor Dooley <conor+dt@kernel.org> Cc: Eric Dumazet <edumazet@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Kalle Valo <kvalo@kernel.org> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Rob Herring <robh@kernel.org> Cc: devicetree@vger.kernel.org Cc: linux-wireless@vger.kernel.org Cc: netdev@vger.kernel.org --- V5: New patch --- .../net/wireless/microchip/wilc1000/cfg80211.c | 15 ++++++++------- .../net/wireless/microchip/wilc1000/cfg80211.h | 1 + drivers/net/wireless/microchip/wilc1000/sdio.c | 4 ++++ drivers/net/wireless/microchip/wilc1000/spi.c | 4 ++++ 4 files changed, 17 insertions(+), 7 deletions(-)