On Tue, 2024-01-23 at 11:18 +0100, Alexis Lothoré wrote: > On 1/22/24 23:03, David Mosberger-Tang wrote: > > Previously, the driver created a net device (typically wlan0) as soon > > as the module was loaded. This commit changes the driver to follow > > normal Linux convention of creating the net device only when bus > > probing detects a supported chip. > > I would gladly help review/test the patch, but please give us some time between > versions to take a look (even if you can mention if you found issues yourself). > Also, each version should be a separate thread, bearing the new version in the > "Subject" line. > Additionally (to answer your cover letter), the patches must target the wireless > branches (likely wireless-testing), not linux-next > (https://wireless.wiki.kernel.org/en/developers/documentation/git-guide) Yeah, sorry about that. > I have a working wilc-over-spi setup with which I can easily unplug the module, > so I gave a try to your series, and while the lack of chip detect indeed makes > the netdevice registration not executed, I've got a nasty kasan warning: > > driver_probe_device from __driver_attach+0x1a0/0x29c > > > > [141/1863] > __driver_attach from bus_for_each_dev+0xf0/0x14c > bus_for_each_dev from bus_add_driver+0x130/0x288 > bus_add_driver from driver_register+0xd4/0x1c0 > driver_register from do_one_initcall+0xfc/0x204 > do_one_initcall from kernel_init_freeable+0x240/0x2a0 > kernel_init_freeable from kernel_init+0x20/0x144 > kernel_init from ret_from_fork+0x14/0x28 > Exception stack(0xc3163fb0 to 0xc3163ff8) > 3fa0: 00000000 00000000 00000000 00000000 > 3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > Allocated by task 1: > kasan_set_track+0x3c/0x5c > __kasan_kmalloc+0x8c/0x94 > __kmalloc_node+0x64/0x184 > kvmalloc_node+0x48/0x114 > alloc_netdev_mqs+0x68/0x664 > alloc_etherdev_mqs+0x28/0x34 > wilc_netdev_ifc_init+0x34/0x37c > wilc_cfg80211_init+0x278/0x330 > wilc_bus_probe+0xb4/0x398 > spi_probe+0xb8/0xdc > really_probe+0x134/0x588 > __driver_probe_device+0xe0/0x288 > driver_probe_device+0x60/0x118 > __driver_attach+0x1a0/0x29c > bus_for_each_dev+0xf0/0x14c > bus_add_driver+0x130/0x288 > driver_register+0xd4/0x1c0 > do_one_initcall+0xfc/0x204 > kernel_init_freeable+0x240/0x2a0 > kernel_init+0x20/0x144 > ret_from_fork+0x14/0x28 > > Freed by task 1: > kasan_set_track+0x3c/0x5c > kasan_save_free_info+0x30/0x3c > __kasan_slab_free+0xe4/0x12c > __kmem_cache_free+0x94/0x1cc > device_release+0x54/0xf8 > kobject_put+0xf4/0x238 > netdev_run_todo+0x414/0x7dc > wilc_netdev_cleanup+0xe4/0x244 > wilc_bus_probe+0x2b8/0x398 > spi_probe+0xb8/0xdc > really_probe+0x134/0x588 > __driver_probe_device+0xe0/0x288 > driver_probe_device+0x60/0x118 > __driver_attach+0x1a0/0x29c > bus_for_each_dev+0xf0/0x14c > bus_add_driver+0x130/0x288 > driver_register+0xd4/0x1c0 > do_one_initcall+0xfc/0x204 > kernel_init_freeable+0x240/0x2a0 > kernel_init+0x20/0x144 > ret_from_fork+0x14/0x28 > > It looks like an already existing/dormant issue in the error-managing path of > spi probe of the driver (looks like we are trying to unregister a netdevice > which has never been registered ?), but since your series triggers it, it should > be handled too. I need help interpreting this. What does KASAN actually complain about? A double free or something else? register_netdev() does get called (through wilc_cfg80211_init()) and then when the chip detect fails, unregister_netdev() gets called (from wilc_netdev_cleanup()), so I don't see any obvious issues, but there is a lot of other stuff going on there that I'm not familiar with. > > > + } > > + > > return 0; > > > > netdev_cleanup: > > @@ -1074,58 +1092,43 @@ static int wilc_spi_write(struct wilc *wilc, u32 addr, u8 *buf, u32 size) > > * > > ********************************************/ > > > > -static int wilc_spi_reset(struct wilc *wilc) > > +static const char * > > +strbool(bool val) > > I am not convinced we need a dedicated helper just to print boolean values (and > why the super short line break ?) Sure, I can remove that. > Also, such change should likely be in a separate patch since it generate quite > some lines of diff but none of those being about the initial topic Sure, I can rearrange the order of the functions to mimize the size of the diff. > > { > > - struct spi_device *spi = to_spi_device(wilc->dev); > > - struct wilc_spi *spi_priv = wilc->bus_data; > > - int result; > > - > > - result = wilc_spi_special_cmd(wilc, CMD_RESET); > > - if (result && !spi_priv->probing_crc) > > - dev_err(&spi->dev, "Failed cmd reset\n"); > > - > > - return result; > > -} > > - > > -static bool wilc_spi_is_init(struct wilc *wilc) > > -{ > > - struct wilc_spi *spi_priv = wilc->bus_data; > > - > > - return spi_priv->isinit; > > + return val ? "on" : "off"; > > } > > > > -static int wilc_spi_deinit(struct wilc *wilc) > > +static int wilc_validate_chipid(struct wilc *wilc) > > { > > - struct wilc_spi *spi_priv = wilc->bus_data; > > + struct spi_device *spi = to_spi_device(wilc->dev); > > + u32 chipid, base_id; > > + int ret; > > > > - spi_priv->isinit = false; > > - wilc_wlan_power(wilc, false); > > + /* > > + * make sure can read chip id without protocol error > > + */ > > + ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid); > > + if (ret) { > > + dev_err(&spi->dev, "Fail cmd read chip id...\n"); > > + return ret; > > + } > > + base_id = chipid & ~WILC_CHIP_REV_FIELD; > > + if (base_id != WILC_1000_BASE_ID && base_id != WILC_3000_BASE_ID) { > > - WILC3000 is currently not supported (yet) by the upstream driver, so there is > no reason to validate its presence if we can not handle it later. Any mention of > it should then be removed from this series Oh, I didn't realize that. I was just going off of this web page: https://www.microchip.com/en-us/software-library/wilc1000_3000_linux_driver as I never played with WILC3000. > - I see that there is already a helper to handle masking and check chip id in > wlan.c (is_wilc1000). You should likely reuse this/avoid the duplication Sure, it'll need to be an exported symbol, but other than that, it's fine. > > > + dev_err(&spi->dev, "Unknown chip id 0x%x\n", chipid); > > + return ret; > > + } > > + dev_info(&spi->dev, "Detected chip id 0x%x (crc7=%s, crc16=%s)\n", > > + chipid, strbool(enable_crc7), strbool(enable_crc16)); > > return 0; > > } > > > > -static int wilc_spi_init(struct wilc *wilc, bool resume) > > +static int wilc_spi_configure_bus_protocol(struct wilc *wilc) > > { > > struct spi_device *spi = to_spi_device(wilc->dev); > > struct wilc_spi *spi_priv = wilc->bus_data; > > u32 reg; > > - u32 chipid; > > int ret, i; > > > > - if (spi_priv->isinit) { > > - /* Confirm we can read chipid register without error: */ > > - ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid); > > - if (ret == 0) > > - return 0; > > - > > - dev_err(&spi->dev, "Fail cmd read chip id...\n"); > > - } > > - > > - wilc_wlan_power(wilc, true); > > - > > - /* > > - * configure protocol > > - */ > > - > > /* > > * Infer the CRC settings that are currently in effect. This > > * is necessary because we can't be sure that the chip has > > @@ -1176,12 +1179,54 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) > > > > spi_priv->probing_crc = false; > > > > - /* > > - * make sure can read chip id without protocol error > > - */ > > - ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid); > > + return 0; > > +} > > + > > +static int wilc_spi_reset(struct wilc *wilc) > > +{ > > + struct spi_device *spi = to_spi_device(wilc->dev); > > + struct wilc_spi *spi_priv = wilc->bus_data; > > + int result; > > + > > + result = wilc_spi_special_cmd(wilc, CMD_RESET); > > + if (result && !spi_priv->probing_crc) > > + dev_err(&spi->dev, "Failed cmd reset\n"); > > + > > + return result; > > +} > > + > > +static bool wilc_spi_is_init(struct wilc *wilc) > > +{ > > + struct wilc_spi *spi_priv = wilc->bus_data; > > + > > + return spi_priv->isinit; > > +} > > + > > +static int wilc_spi_deinit(struct wilc *wilc) > > +{ > > + struct wilc_spi *spi_priv = wilc->bus_data; > > + > > + spi_priv->isinit = false; > > + wilc_wlan_power(wilc, false); > > + return 0; > > +} > > + > > +static int wilc_spi_init(struct wilc *wilc, bool resume) > > +{ > > + struct wilc_spi *spi_priv = wilc->bus_data; > > + int ret; > > + > > + if (spi_priv->isinit) { > > Ok, so indeed in this new version of the patch, the flag still makes sense for > upper layers. > > > + /* Confirm we can read chipid register without error: */ > > + if (wilc_validate_chipid(wilc) == 0) > > + return 0; > > + } > > + > > + wilc_wlan_power(wilc, true); > > + > > + ret = wilc_spi_configure_bus_protocol(wilc); > > if (ret) { > > - dev_err(&spi->dev, "Fail cmd read chip id...\n"); > > + wilc_wlan_power(wilc, false); > > return ret; > > } > > > > diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h > > index a72cd5cac81d..43dda9f0d9ca 100644 > > --- a/drivers/net/wireless/microchip/wilc1000/wlan.h > > +++ b/drivers/net/wireless/microchip/wilc1000/wlan.h > > @@ -182,6 +182,7 @@ > > #define WILC_CORTUS_BOOT_FROM_IRAM 0x71 > > > > #define WILC_1000_BASE_ID 0x100000 > > +#define WILC_3000_BASE_ID 0x300000 > > See comment above for WILC3000 > > > > > #define WILC_1000_BASE_ID_2A 0x1002A0 > > #define WILC_1000_BASE_ID_2A_REV1 (WILC_1000_BASE_ID_2A + 1) >
