by Adham Abozaeid

[permalink] [raw]

Subject: Re: [PATCH v2 01/16] wilc1000: add wilc_hif.h

On 10/23/19 3:03 AM, Kalle Valo wrote:
> <[email protected]> wrote:
>
>> From: Ajay Singh <[email protected]>
>>
>> Moved '/driver/staging/wilc1000/wilc_hif.h' to
>> '/driver/net/wireless/mirochip/wilc1000/wilc_hif.h'.
>>
>> Signed-off-by: Ajay Singh <[email protected]>
> (My patchwork script doesn't support cover letters, yet, so I need to reply to
> the first patch)
>
> I was supposed to do a quick 15 minute review, but I got overboard and used
> over an hour for this :) But anyway, below are my comments. Mostly looks good
> but some work still to do.
Thanks a lot for the complete and detailed review of the driver.
I am glad to know that state of the driver is *good* and thanks for your acknowledgment. Defiantly, Johannes's review comments helped us to improve the driver.
I will continue to work on your review comments and try to address all of them. Most of the comments are clear but there are few comments which need some more clarification especially related to *wid_List*.
> +#ifndef HOST_INT_H
>
> Add WILC prefix?
Agree. Will add the WILC_ prefix
> +struct assoc_resp {
> + __le16 capab_info;
> + __le16 status_code;
> + __le16 aid;
> +} __packed;
>
> use struct ieee80211_mgmt?
the ieee8022_mgmt struct includes the mac header, but the frame received from the device doesn't have it, hence, we can't use the ieee802211_mgmt struct directly
> + //extract RSN capabilities
>
> No C++ style comments, please.
Agree. Will change to C style.
> + wid_list[1].val = (s8 *)&auth_type;
>
> A little bit too much of casting, like in this example, for my taste
> but I guess it's not that a bit of problem. Didn't investigate why
> this particular cast was need, but I think Johannes already commented
> how odd this wid_list looks like. And why sometimes it's (s8 *) and
> others (u8 *)?
You are right, different locations using u8* and other s8*. I will check and submit the required changes.

I'm not very clear though on why the current implementation of wid_list is odd, and what's the recommendation to fix it, so I'd appreciate more clarification on that.
The wid_list is an array of struct that holds the wid parameters, which is the configuration data to be sent to the device.
This array of structs is passed to cfg_send_config_pkt, which takes care of formatting a config packet that the device understands, from the wid struct, then sends it to the device. It also takes care of waiting for the response from the device.
> + case 'S':
>
> Isn't a proper define much more readable and as a bonus it would
> document the meaning of this value? For example, it could
> WILC_RESPONSE_TYPE_SCAN or whatever it means. Oh, and the same for
> 'R', 'I' and others.
Agree. Will change to meaningful macros.
> do {
> ....
> } while (1);
>
> I see quite a lot of these unconditonal loops in the driver:
>
> wilc_netdev.c:157: while (1) {
> wilc_wlan.c:532: } while (1);
> wilc_wlan.c:602: } while (1);
> wilc_wlan.c:730: } while (1);
> wilc_wlan.c:753: } while (1);
> wilc_wlan.c:1002: } while (1);
> wilc_wlan.c:1009: } while (1);
> wilc_wlan_cfg.c:151: } while (1);
> wilc_wlan_cfg.c:167: } while (1);
> wilc_wlan_cfg.c:183: } while (1);
> wilc_wlan_cfg.c:198: } while (1);
> wilc_wlan_cfg.c:230: } while (1);
> wilc_wlan_cfg.c:303: } while (1);
> wilc_wlan_cfg.c:315: } while
> (1);
> wilc_wlan_cfg.c:327: } while (1);
> wilc_wlan_cfg.c:346: } while (1);
>
> This is not recommended in kernel code because a small bug can cause a
> never ending loop in kernel. Put some kind of limit to the loop,
> either counter or time based, for example:
>
> count = 0;
>
> do {
> ....
> } while (count++ < 100);
>
> Or even some of the while loops could be replaced with for loop, like
> the one in wilc_wlan_parse_info_frame().
Agree. Will go through the infinite loops and try to limit them.
> + u8 type = (id >> 12) & 0xf;
>
> No magic values, please. My recommendation is to use GENMASK() and
> friends, maybe u16_get_bits()?
>
> I also see identical magic values elsewhere, which should be a strong
> indication that this needs to be implemented with a proper define.
Agree. Will use macros to define magic values, and GENMASk/u16_get_bits() to parse bit fields
> +int wilc_wlan_cfg_get_wid(u8 *frame, u32 offset, u16 id)
> +{
> + u8 *buf;
> +
> + if ((offset + 2) >= WILC_MAX_CFG_FRAME_SIZE)
> + return 0;
> +
> + buf = &frame[offset];
> +
> + buf[0] = (u8)id;
> + buf[1] = (u8)(id >> 8);
>
> In general a struct is MUCH better than manually playing with bytes
> using 'u8 buf[]', but I think Johannes told you already that. Here you
> could just have a simple '__le16 id' and you could assign to it with
> cpu_to_le16(id), a lot cleaner than what's above.
Agree. These changes were already submitted to staging after we received Johannes' feedback to use put_unaligned_le16(), so will be included in v3 of the patch
> + /*call host interface info parse as well*/
>
> A space after '/*' and before '*/'. Have you run checkpatch? It should
> catch these simple style issues? And you can run with --file directly
> on the source tree. Not of course all checkpatch warnings need to be
> fixed, but obvious ones like this for sure.
Agree. Will add spaces as recommended.
Actually we ran checkpatch, but it didn't catch this warning.
Currently the driver has only one warning in wilc_wfi_cfgoperations.c for a line being over 80 chars. not sure why this wasn't caught by the checkpatch script.
I'm using the parameters below to run the script, so please let me know if we should be using something else.
./scripts/checkpatch.pl --no-tree --fix-inplace -f drivers/staging/wilc1000/*.c
./scripts/checkpatch.pl --no-tree --fix-inplace -f drivers/staging/wilc1000/*.h
> +#define GET_PKT_OFFSET(a) (((a) >> 22) & 0x1ff)
>
> GENMASK() & co
>
> +/*Parameters needed for host interface for remaining on channel*/
>
> checkpatch
>
> + struct wilc_vif *vif[WILC_NUM_CONCURRENT_IFC];
> + /*protect vif list*/
> + struct mutex vif_mutex;
>
> I would add a newline before the comment, that would make wilc_wfi_netdevice.h
> a lot more readable. But that's a style issue and up to you.
>
> + if (ch_list_attr_idx) {
> + u8 limit = ch_list_attr_idx + 3 + buf[ch_list_attr_idx + 1];
> +
> + for (i = ch_list_attr_idx + 3; i < limit; i++) {
> + if (buf[i] == 0x51) {
> + for (j = i + 2; j < ((i + 2) +
> buf[i + 1]); j++)
> + buf[j] = sta_ch;
> + break;
> + }
> + }
> + }
>
> No magic values like 0x51, please. And I think this loop needs a
> comment what's happening. But I suspect that if you had proper structs
> (and not this ugly buf[] stuff) the code would be self-explanatory and
> there would be no need for comments.
>
> + if (op_ch_attr_idx) {
> + buf[op_ch_attr_idx + 6] = 0x51;
> + buf[op_ch_attr_idx + 7] = sta_ch;
> + }
>
> Ditto. And even more of that in wilc_wfi_cfgoperations.c.
>
> +static struct net_device *get_if_handler(struct wilc *wilc, u8
> *mac_header)
> +{
> + u8 *bssid, *bssid1;
> + int i = 0;
> + struct net_device *ndev = NULL;
> +
> + bssid = mac_header + 10;
> + bssid1 = mac_header + 4;
>
> And here a proper struct for mac_header would be so much cleaner.
ok
> +static u8 crc7(u8 crc, const u8 *buffer, u32 len)
> +{
> + while (len--)
> + crc = crc7_byte(crc, *buffer++);
> + return crc;
> +}
>
> What's wrong with <linux/crc7.h>? Why reinvent the wheel?
The new implementation of crc7 after commit 1836eea209546b870dd83f3f4ef234d6598a560d uses a different syndrome table than what the WILC SPI uses, and we can't change that since crc7 calculation is in done in the hardware IP.
> I see so much this u8 buf[] stuff that I'll stop commenting about it
> now. But, for example, spi_cmd_complete() would be a lot cleaner with
> proper structs and some refactoring (one function per command or
> something like that).
Sure, will check and refactor spi_cmd_complete().
> + if (addr < 0x30) {
>
> Proper defines for magic values, please. This was even used multiple
> times.
>
> + wilc->hif_func->hif_read_reg(wilc, 0xf0, &reg);
> +
> + wilc->hif_func->hif_write_reg(wilc, 0xf0, reg & ~BIT(0));
> + wilc->hif_func->hif_write_reg(wilc, 0xfa, 0);
>
> Magic values. I'll also stop commenting about magic values, I think you got
> the point already :)
Ok.
> +#ifdef WILC_DISABLE_PMU
> +#else
> + reg |= WILC_HAVE_USE_PMU;
> +#endif
> +
> +#ifdef WILC_SLEEP_CLK_SRC_XO
> + reg |= WILC_HAVE_SLEEP_CLK_SRC_XO;
> +#elif defined WILC_SLEEP_CLK_SRC_RTC
> + reg |= WILC_HAVE_SLEEP_CLK_SRC_RTC;
> +#endif
> +
> +#ifdef WILC_EXT_PA_INV_TX_RX
> + reg |= WILC_HAVE_EXT_PA_INV_TX_RX;
> +#endif
> + reg |= WILC_HAVE_USE_IRQ_AS_HOST_WAKE;
> + reg |= WILC_HAVE_LEGACY_RF_SETTINGS;
> +#ifdef XTAL_24
> + reg |= WILC_HAVE_XTAL_24;
> +#endif
> +#ifdef DISABLE_WILC_UART
> + reg |= WILC_HAVE_DISABLE_WILC_UART;
> +#endif
>
> This kind of configuration should happen on runtime, not compile time.
> In other words, the same kernel module should work on _all_ hardware
> without recompilation.
>
> + reg = (BIT(0) | BIT(1) | BIT(2) | BIT(3) | BIT(8) | BIT(9) | BIT(26) |
> + BIT(29) | BIT(30) | BIT(31));
>
> I said I would stop commenting about magic values, but here I really
> have to comment about it :)
>
> Device tree bindings were not visible. And CC
> [email protected] as we need an ack from the DT maintainers. Also
> I have understood that they require the bindings in YAML format now,
> at least that was the comment I got with ath11k.
I see, the device tree binding file is not shown properly in the patch because patch indicate the changes as file movement from "staging" to "Documentation/devicetree/bindings/net/wireless" folder.
We don't have binding in YAML format yet so I will make the changes and sent them to review to [email protected] also.

> Please remove the 'wilc_' prefix from the filenames, the directory
> name is already wilc1000 so no need to replicate that. For example, rename
> wilc_hif.c to just hif.c.
ok.
> +config WILC1000_HW_OOB_INTR
> + bool "WILC1000 out of band interrupt"
> + depends on WILC1000_SDIO
> + help
> + This option enables out-of-band interrupt support for the WILC1000
> + chipset. This OOB interrupt is intended to provide a faster interrupt
> + mechanism for SDIO host controllers that don't support SDIO interrupt.
> + Select this option If the SDIO host controller in your platform
> + doesn't support SDIO time devision interrupt.
>
> I think this should be a runtime setting (see my comment about other
> compile time settings), for example a module parameter. Would that
> work?
Okay. Will change this to runtime setting using module paramter.