| Message ID | 20240127004331.1334804-1-davidm@egauge.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v4] wifi: wilc1000: validate chip id during bus probe | expand |
On 1/27/24 01:43, 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. As already mentioned multiple times, I am skeptical about the validity of keeping netdev registration before chip presence check, but I am not the maintainer, so I let Ajay and Kalle decide for this. Aside from that, and from the kasan warning which is not especially related to the series (and not observed in nominal case), I still have a few minor comments below > Signed-off-by: David Mosberger-Tang <davidm@egauge.net> > --- > changelog: > V1: original version > V2: Add missing forward declarations > V3: Add missing forward declarations, actually :-( > V4: - rearranged function order to improve readability > - now relative to wireless-next repository > - avoid change error return value and have lower-level functions > directly return -ENODEV instead > - removed any mention of WILC3000 > - export and use existing is_wilc1000() for chipid validation > - replaced strbool() function with open code > > drivers/net/wireless/microchip/wilc1000/spi.c | 74 ++++++++++++++----- > .../net/wireless/microchip/wilc1000/wlan.c | 3 +- > .../net/wireless/microchip/wilc1000/wlan.h | 1 + > 3 files changed, 59 insertions(+), 19 deletions(-) [...] > @@ -1142,7 +1170,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) > } > if (ret) { > dev_err(&spi->dev, "Failed with CRC7 on and off.\n"); > - return ret; > + return -ENODEV; You are still rewriting error codes here. At a lower level, sure, but still... When I suggested setting -ENODEV at lower level, I was thinking about places where no explicit error code was already in use, but spi_internal_read/spi_internal_write already generate proper error codes. Or am I missing a constraint, like the probe chain really needing -ENODEV ? > } > > /* set up the desired CRC configuration: */ > @@ -1165,7 +1193,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) > dev_err(&spi->dev, > "[wilc spi %d]: Failed internal write reg\n", > __LINE__); > - return ret; > + return -ENODEV; > } > /* update our state to match new protocol settings: */ > spi_priv->crc7_enabled = enable_crc7; > @@ -1176,17 +1204,27 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) > > spi_priv->probing_crc = false; > > + return 0; > +} > + > +static int wilc_validate_chipid(struct wilc *wilc) > +{ > + struct spi_device *spi = to_spi_device(wilc->dev); > + u32 chipid; > + int ret; > + > /* > * 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; > + return -ENODEV; > + } > + if (!is_wilc1000(chipid)) { > + dev_err(&spi->dev, "Unknown chip id 0x%x\n", chipid); > + return -ENODEV; > } > - > - spi_priv->isinit = true; > - > return 0; > } > > diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c > index 6b2f2269ddf8..3130a3ea8d71 100644 > --- a/drivers/net/wireless/microchip/wilc1000/wlan.c > +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c > @@ -12,10 +12,11 @@ > > #define WAKE_UP_TRIAL_RETRY 10000 > > -static inline bool is_wilc1000(u32 id) > +bool is_wilc1000(u32 id) > { > return (id & (~WILC_CHIP_REV_FIELD)) == WILC_1000_BASE_ID; > } > +EXPORT_SYMBOL_GPL(is_wilc1000); nit: Since the function is not static anymore, it would have been nice to move it with the other exported functions to maintain the existing functions ordering > static inline void acquire_bus(struct wilc *wilc, enum bus_acquire acquire) > { > diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h > index f02775f7e41f..ebdfb0afaf71 100644 > --- a/drivers/net/wireless/microchip/wilc1000/wlan.h > +++ b/drivers/net/wireless/microchip/wilc1000/wlan.h > @@ -409,6 +409,7 @@ struct wilc_cfg_rsp { > > struct wilc_vif; > > +bool is_wilc1000(u32 id); > int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer, > u32 buffer_size); > int wilc_wlan_start(struct wilc *wilc);
On 1/30/24 02:06, Alexis Lothoré wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 1/27/24 01:43, 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. > > As already mentioned multiple times, I am skeptical about the validity of > keeping netdev registration before chip presence check, but I am not the > maintainer, so I let Ajay and Kalle decide for this. Aside from that, and from > the kasan warning which is not especially related to the series (and not > observed in nominal case), I still have a few minor comments below > IMO the order of netdev registration before chip validity should be fine. Since wilc_bus_probe() is already doing netdev_cleanup that takes care of errors path after netdev registration. For this patch, it is okay to follow the same approach like the previous implementation. However, in the patch, please take care of disabling 'wilc->rtc_clk' in wilc_bus_probe() for the failure condition. static int wilc_bus_probe(struct spi_device *spi) { ... +power_down: + clk_disable_unprepare(wilc->rtc_clk); + wilc_wlan_power(wilc, false); netdev_cleanup: wilc_netdev_cleanup(wilc); I hope this patch is tested for failure scenario(when WILC1000 SPI device is not connected) as well. >> Signed-off-by: David Mosberger-Tang <davidm@egauge.net> >> --- >> changelog: >> V1: original version >> V2: Add missing forward declarations >> V3: Add missing forward declarations, actually :-( >> V4: - rearranged function order to improve readability >> - now relative to wireless-next repository >> - avoid change error return value and have lower-level functions >> directly return -ENODEV instead >> - removed any mention of WILC3000 >> - export and use existing is_wilc1000() for chipid validation >> - replaced strbool() function with open code >> >> drivers/net/wireless/microchip/wilc1000/spi.c | 74 ++++++++++++++----- >> .../net/wireless/microchip/wilc1000/wlan.c | 3 +- >> .../net/wireless/microchip/wilc1000/wlan.h | 1 + >> 3 files changed, 59 insertions(+), 19 deletions(-) > > [...] > >> @@ -1142,7 +1170,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) >> } >> if (ret) { >> dev_err(&spi->dev, "Failed with CRC7 on and off.\n"); >> - return ret; >> + return -ENODEV; > > You are still rewriting error codes here. At a lower level, sure, but still... > When I suggested setting -ENODEV at lower level, I was thinking about places > where no explicit error code was already in use, but > spi_internal_read/spi_internal_write already generate proper error codes. Or am > I missing a constraint, like the probe chain really needing -ENODEV ? > >> } >> >> /* set up the desired CRC configuration: */ >> @@ -1165,7 +1193,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) >> dev_err(&spi->dev, >> "[wilc spi %d]: Failed internal write reg\n", >> __LINE__); >> - return ret; >> + return -ENODEV; >> } >> /* update our state to match new protocol settings: */ >> spi_priv->crc7_enabled = enable_crc7; >> @@ -1176,17 +1204,27 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) >> >> spi_priv->probing_crc = false; >> >> + return 0; >> +} >> + >> +static int wilc_validate_chipid(struct wilc *wilc) >> +{ >> + struct spi_device *spi = to_spi_device(wilc->dev); >> + u32 chipid; >> + int ret; >> + >> /* >> * 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; >> + return -ENODEV; >> + } >> + if (!is_wilc1000(chipid)) { >> + dev_err(&spi->dev, "Unknown chip id 0x%x\n", chipid); >> + return -ENODEV; >> } >> - >> - spi_priv->isinit = true; >> - >> return 0; >> } >> >> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c >> index 6b2f2269ddf8..3130a3ea8d71 100644 >> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c >> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c >> @@ -12,10 +12,11 @@ >> >> #define WAKE_UP_TRIAL_RETRY 10000 >> >> -static inline bool is_wilc1000(u32 id) >> +bool is_wilc1000(u32 id) >> { >> return (id & (~WILC_CHIP_REV_FIELD)) == WILC_1000_BASE_ID; >> } >> +EXPORT_SYMBOL_GPL(is_wilc1000); > > nit: Since the function is not static anymore, it would have been nice to move > it with the other exported functions to maintain the existing functions ordering > >> static inline void acquire_bus(struct wilc *wilc, enum bus_acquire acquire) >> { >> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h >> index f02775f7e41f..ebdfb0afaf71 100644 >> --- a/drivers/net/wireless/microchip/wilc1000/wlan.h >> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.h >> @@ -409,6 +409,7 @@ struct wilc_cfg_rsp { >> >> struct wilc_vif; >> >> +bool is_wilc1000(u32 id); >> int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer, >> u32 buffer_size); >> int wilc_wlan_start(struct wilc *wilc); > > -- > Alexis Lothoré, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com >
Alexis Lothoré <alexis.lothore@bootlin.com> writes: > On 1/27/24 01:43, 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. > > As already mentioned multiple times, I am skeptical about the validity of > keeping netdev registration before chip presence check, but I am not the > maintainer, so I let Ajay and Kalle decide for this. I haven't checked the code but as a general comment I agree with Alexis, registering netdev before the hardware is ready sounds odd to me.
On Tue, 2024-01-30 at 10:06 +0100, Alexis Lothoré wrote: > On 1/27/24 01:43, David Mosberger-Tang wrote: > > > > @@ -1142,7 +1170,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) > > } > > if (ret) { > > dev_err(&spi->dev, "Failed with CRC7 on and off.\n"); > > - return ret; > > + return -ENODEV; > > You are still rewriting error codes here. At a lower level, sure, but still... > When I suggested setting -ENODEV at lower level, I was thinking about places > where no explicit error code was already in use, but > spi_internal_read/spi_internal_write already generate proper error codes. Or am > I missing a constraint, like the probe chain really needing -ENODEV ? Lower-level errors are often not meaningful at the higher level. For example, attempting to read a register over SPI may cause a CRC error if the device doesn't exist. That would result in -EINVAL, even though there was nothing invalid about the read, it's just that the device wasn't there. But I don't feel strongly about it. v5 of the patch does what you want. --david
On Thu, 2024-02-01 at 02:55 +0000, Ajay.Kathat@microchip.com wrote: > > However, in the patch, please take care of disabling 'wilc->rtc_clk' in > wilc_bus_probe() for the failure condition. > > static int wilc_bus_probe(struct spi_device *spi) { > ... > +power_down: > + clk_disable_unprepare(wilc->rtc_clk); > + wilc_wlan_power(wilc, false); > netdev_cleanup: > wilc_netdev_cleanup(wilc); Good catch - I fixed that in v5. > I hope this patch is tested for failure scenario(when WILC1000 SPI > device is not connected) as well. Yep. --david
On Thu, 2024-02-01 at 12:08 +0200, Kalle Valo wrote: > Alexis Lothoré <alexis.lothore@bootlin.com> writes: > > > On 1/27/24 01:43, 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. > > > > As already mentioned multiple times, I am skeptical about the validity of > > keeping netdev registration before chip presence check, but I am not the > > maintainer, so I let Ajay and Kalle decide for this. > > I haven't checked the code but as a general comment I agree with Alexis, > registering netdev before the hardware is ready sounds odd to me. I agree, but it's orthogonal to what my patch does. I did a quick scan and it looks like the cleanest thing to do would be to change all the code below "Spi Internal Read/Write Function" and "Spi interfaces" to take a spi_device pointer instead of the wilc pointer. The probe code could then safely call these lower-level functions without having to worry that they might inadvertently access a part of the wilc structure that hasn't been initialized yet. From the looks of it, that would probably also shorten the code, since many calls to to_spi_device(wilc->dev) would go away. However, it'd be a major rewrite of spi.c so it better had the buy in of the maintainer(s). --david
David Mosberger-Tang <davidm@egauge.net> writes: > On Tue, 2024-01-30 at 10:06 +0100, Alexis Lothoré wrote: >> On 1/27/24 01:43, David Mosberger-Tang wrote: >> >> >> > @@ -1142,7 +1170,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) >> > } >> > if (ret) { >> > dev_err(&spi->dev, "Failed with CRC7 on and off.\n"); >> > - return ret; >> > + return -ENODEV; >> >> You are still rewriting error codes here. At a lower level, sure, but still... >> When I suggested setting -ENODEV at lower level, I was thinking about places >> where no explicit error code was already in use, but >> spi_internal_read/spi_internal_write already generate proper error codes. Or am >> I missing a constraint, like the probe chain really needing -ENODEV ? > > Lower-level errors are often not meaningful at the higher level. For > example, attempting to read a register over SPI may cause a CRC error > if the device doesn't exist. That would result in -EINVAL, even though > there was nothing invalid about the read, it's just that the device > wasn't there. Changing the error values makes debugging harder so please avoid doing it unless absolutely necessary (and then explain the reason in a code comment).
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c index 77b4cdff73c3..6496a19a337e 100644 --- a/drivers/net/wireless/microchip/wilc1000/spi.c +++ b/drivers/net/wireless/microchip/wilc1000/spi.c @@ -42,7 +42,7 @@ MODULE_PARM_DESC(enable_crc16, #define WILC_SPI_RSP_HDR_EXTRA_DATA 8 struct wilc_spi { - bool isinit; /* true if SPI protocol has been configured */ + bool isinit; /* true if wilc_spi_init was successful */ bool probing_crc; /* true if we're probing chip's CRC config */ bool crc7_enabled; /* true if crc7 is currently enabled */ bool crc16_enabled; /* true if crc16 is currently enabled */ @@ -55,6 +55,8 @@ struct wilc_spi { static const struct wilc_hif_func wilc_hif_spi; static int wilc_spi_reset(struct wilc *wilc); +static int wilc_spi_configure_bus_protocol(struct wilc *wilc); +static int wilc_validate_chipid(struct wilc *wilc); /******************************************** * @@ -232,8 +234,26 @@ static int wilc_bus_probe(struct spi_device *spi) } clk_prepare_enable(wilc->rtc_clk); + dev_info(&spi->dev, "Selected CRC config: crc7=%s, crc16=%s\n", + enable_crc7 ? "on" : "off", enable_crc16 ? "on" : "off"); + + /* we need power to configure the bus protocol and to read the chip id: */ + + wilc_wlan_power(wilc, true); + + ret = wilc_spi_configure_bus_protocol(wilc); + if (ret) + goto power_down; + + ret = wilc_validate_chipid(wilc); + if (ret) + goto power_down; + + wilc_wlan_power(wilc, false); return 0; +power_down: + wilc_wlan_power(wilc, false); netdev_cleanup: wilc_netdev_cleanup(wilc); free: @@ -1105,26 +1125,34 @@ static int wilc_spi_deinit(struct wilc *wilc) static int wilc_spi_init(struct wilc *wilc, bool resume) { - struct spi_device *spi = to_spi_device(wilc->dev); struct wilc_spi *spi_priv = wilc->bus_data; - u32 reg; - u32 chipid; - int ret, i; + int ret; if (spi_priv->isinit) { /* Confirm we can read chipid register without error: */ - ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid); - if (ret == 0) + if (wilc_validate_chipid(wilc) == 0) return 0; - - dev_err(&spi->dev, "Fail cmd read chip id...\n"); } wilc_wlan_power(wilc, true); - /* - * configure protocol - */ + ret = wilc_spi_configure_bus_protocol(wilc); + if (ret) { + wilc_wlan_power(wilc, false); + return ret; + } + + spi_priv->isinit = true; + + return 0; +} + +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; + int ret, i; /* * Infer the CRC settings that are currently in effect. This @@ -1142,7 +1170,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) } if (ret) { dev_err(&spi->dev, "Failed with CRC7 on and off.\n"); - return ret; + return -ENODEV; } /* set up the desired CRC configuration: */ @@ -1165,7 +1193,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) dev_err(&spi->dev, "[wilc spi %d]: Failed internal write reg\n", __LINE__); - return ret; + return -ENODEV; } /* update our state to match new protocol settings: */ spi_priv->crc7_enabled = enable_crc7; @@ -1176,17 +1204,27 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) spi_priv->probing_crc = false; + return 0; +} + +static int wilc_validate_chipid(struct wilc *wilc) +{ + struct spi_device *spi = to_spi_device(wilc->dev); + u32 chipid; + int ret; + /* * 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; + return -ENODEV; + } + if (!is_wilc1000(chipid)) { + dev_err(&spi->dev, "Unknown chip id 0x%x\n", chipid); + return -ENODEV; } - - spi_priv->isinit = true; - return 0; } diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c index 6b2f2269ddf8..3130a3ea8d71 100644 --- a/drivers/net/wireless/microchip/wilc1000/wlan.c +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c @@ -12,10 +12,11 @@ #define WAKE_UP_TRIAL_RETRY 10000 -static inline bool is_wilc1000(u32 id) +bool is_wilc1000(u32 id) { return (id & (~WILC_CHIP_REV_FIELD)) == WILC_1000_BASE_ID; } +EXPORT_SYMBOL_GPL(is_wilc1000); static inline void acquire_bus(struct wilc *wilc, enum bus_acquire acquire) { diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h index f02775f7e41f..ebdfb0afaf71 100644 --- a/drivers/net/wireless/microchip/wilc1000/wlan.h +++ b/drivers/net/wireless/microchip/wilc1000/wlan.h @@ -409,6 +409,7 @@ struct wilc_cfg_rsp { struct wilc_vif; +bool is_wilc1000(u32 id); int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer, u32 buffer_size); int wilc_wlan_start(struct wilc *wilc);
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. Signed-off-by: David Mosberger-Tang <davidm@egauge.net> --- changelog: V1: original version V2: Add missing forward declarations V3: Add missing forward declarations, actually :-( V4: - rearranged function order to improve readability - now relative to wireless-next repository - avoid change error return value and have lower-level functions directly return -ENODEV instead - removed any mention of WILC3000 - export and use existing is_wilc1000() for chipid validation - replaced strbool() function with open code drivers/net/wireless/microchip/wilc1000/spi.c | 74 ++++++++++++++----- .../net/wireless/microchip/wilc1000/wlan.c | 3 +- .../net/wireless/microchip/wilc1000/wlan.h | 1 + 3 files changed, 59 insertions(+), 19 deletions(-)