| Message ID | 20240909193035.69823-1-marex@denx.de |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v5,1/9] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
Hello Marek, On 9/9/24 21:29, Marek Vasut wrote: > Export wilc_get_chipid() again and call it in driver probe to get > wilc->chipid assigned early on. This is necessary to discern WILC > 1000 from 3000 to disable WPA3/SAE on the later. I like the general change (reading chipid early enough so we can enable/disable NL80211_FEATURE_SAE accordingly before wiphy_register), but then there is no point in making wilc_get_chipid private in wlan.c in patch 2, to make it public to the module again in patch 7. Thanks, Alexis
On 9/9/24 21:29, Marek Vasut wrote: > Add error handling to chip_wakeup() and propagate the errors throughout > the entire driver. Add error handling to acquire_bus()/release_bus() and > host_sleep_notify()/host_wakeup_notify() functions as a result as well. > Fill the error handling to all call sites. Out of curiosity, what tree/branch are you using as a base for this series ? I wanted to pull it locally to also test it on wilc1000, but it fails to apply this patch, and the failure points to a conflict with one of my patch which has been merged quite some time ago in wireless-next: https://lore.kernel.org/all/20240613-wilc_suspend-v1-4-c2f766d0988c@bootlin.com/ Thanks, Alexis
On 9/9/24 21:29, Marek Vasut wrote: > The wilc_create_wiphy() is not used outside of cfg80211.c . > Make the function static and remove its entry from cfg80211.h > > Signed-off-by: Marek Vasut <marex@denx.deReviewed-by: Alexis Lothoré <alexis.lothore@bootlin.com>
On 9/10/24 11:13 AM, Alexis Lothoré wrote: > On 9/9/24 21:29, Marek Vasut wrote: >> Add error handling to chip_wakeup() and propagate the errors throughout >> the entire driver. Add error handling to acquire_bus()/release_bus() and >> host_sleep_notify()/host_wakeup_notify() functions as a result as well. >> Fill the error handling to all call sites. > > Out of curiosity, what tree/branch are you using as a base for this series ? I > wanted to pull it locally to also test it on wilc1000, but it fails to apply > this patch, and the failure points to a conflict with one of my patch which has > been merged quite some time ago in wireless-next: > https://lore.kernel.org/all/20240613-wilc_suspend-v1-4-c2f766d0988c@bootlin.com/ next-20240909 with this extra patch: wifi: wilc1000: Keep slot powered on during suspend/resume
On 9/9/24 21:29, Marek Vasut wrote:
> The cmd53_buf is a 4 byte buffer, embed it into the struct wilc_sdio.
Is this change really desirable ? I mean, looking at git log, it looks like this
kzalloc is voluntary, for usage with DMA (see [1] as initial fix and [2] as the
final patch which has landed in the driver)
[1]
https://patchwork.kernel.org/project/linux-wireless/patch/20220728152037.386543-1-michael@walle.cc/
[2] https://lore.kernel.org/all/20220809075749.62752-1-ajay.kathat@microchip.com/
Alexis
On 9/10/24 11:01 AM, Alexis Lothoré wrote: > Hello Marek, Hi, > On 9/9/24 21:29, Marek Vasut wrote: >> Export wilc_get_chipid() again and call it in driver probe to get >> wilc->chipid assigned early on. This is necessary to discern WILC >> 1000 from 3000 to disable WPA3/SAE on the later. > > I like the general change (reading chipid early enough so we can enable/disable > NL80211_FEATURE_SAE accordingly before wiphy_register), but then there is no > point in making wilc_get_chipid private in wlan.c in patch 2, to make it public > to the module again in patch 7. I was worried squashing it into 2/9 would be messy, but apparently not, so squashed into 2/9 for V6.
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 11:47 AM, Alexis Lothoré wrote: > On 9/9/24 21:29, Marek Vasut wrote: >> The cmd53_buf is a 4 byte buffer, embed it into the struct wilc_sdio. > > Is this change really desirable ? I mean, looking at git log, it looks like this > kzalloc is voluntary, for usage with DMA (see [1] as initial fix and [2] as the > final patch which has landed in the driver) It is unrelated to the series, so I'll just drop it for now.
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/9/24 21:29, Marek Vasut wrote: > The wilc_create_wiphy() is not used outside of cfg80211.c . > Make the function static and remove its entry from cfg80211.h > > Signed-off-by: Marek Vasut <marex@denx.de> It looks like I messed up my r-b on previous email, so in case it could have confused some scripts or automation: Reviewed-by: Alexis Lothoré <alexis.lothore@bootlin.com>
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/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml index 2460ccc082371..5d40f22765bb6 100644 --- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml +++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml @@ -16,7 +16,11 @@ description: properties: compatible: - const: microchip,wilc1000 + oneOf: + - items: + - const: microchip,wilc3000 + - const: microchip,wilc1000 + - const: microchip,wilc1000 reg: true