Re: [RFC v1 000/256] wireless: cl8k driver for Celeno IEEE 802.11ax devices

[viktor.barna@celeno.com]

Hi Johannes,

we are very grateful for the initial review and a big sorry for such a
prolonged delayed response from our side - we were working on minimizing driver
and RFCv2 is almost ready.

> * without command line interfaces with string parsing built into the
>   kernel
>   (OK, that's probably something that will never go upstream anyway)

RFCv2 will contain removed CLI, some of the stuff moved to the debugfs
(because in most of the cases that's only debugging commands), but we will
publish that patchset even without that - to have as minimum solution as
possible (as it is also required in the guide). Some time later we can
re-introduce the debugfs solution in the future after aproval of base driver.

> * Kconfig vs. code inconsistencies cleaned up, you have a TON of
>   CONFIG_* ifdefs that don't even exist and can never be set, so remove
>   the code, at least for now?

ACK. Unused defines were removed, and others were refactored in a way, that
allows to tune them via Kconfig.

> * remove wrappers like cl_snprintf(), though that seems part of all
>   those command line interfaces built into the driver

ACK. RFCv2 will be without them at all.

> * consider joining some of the many header files into bigger chunks,
>   header files that are 50% boilerplate aren't really all that useful

ACK. RFCv2 consists of ~98 files now (functionality was merged by some similar
area - like all the TX in tx.c, all the RX stuff in the rx.c etc). If that is
still huge - please, feel free to comment. Maybe, there is some acceptance
criteria? It looks much closer to existing drivers. If the number is still high,
just for the sake of review we can merge header files into bigger pieces.

> * namespace your things better - e.g. "is_ipv4_packet" and friends?
>   (why do you even care?)

ACK. That case was related to the lookup of time-sensitive traffic - to avoid
putting it in the slowpath.

> * remove all vendor commands for now, and read the vendor commands
>   upstreaming documentation before re-introducing them
>   https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api

ACK. All the netlink will be removed in RFCv2.

> * remove utils/string.c, obviously, use kernel stuff directly or
>   improve where needed I guess - but likely all part of the weird CLI-
>   in-kernel stuff?

Yep, you are correct, in most the cases, custom abstraction was part of the CLI
(and configuration parsing). It will be removed in RFCv2.

> * remove abstraction layers like cl_timer

ACK.

> * what's this VNS stuff? doesn't look like it belongs into a driver
>   (a mac80211 one at that!)

Very Near STA - extra functionality, that allows to tune TX power regarding STA
position. We can move it out temporary since it has bindings to the FW and
reimplement in the mac80211 if there is necessity.

> * bitfields in endian-safe code are generally frowned upon, so you
>   shouldn't need cl_are_host_bits_le(). And also cl_are_host_bits_be()?
>   What?

cl8k driver is using dev_coredump subsystem for dump preparation, and these
functions were part of dump information about the host - it could be useful for
postmortem analysis of the dump themselves outside of environment where they
were collected.

> * Use ERR_PTR() and friends, instead of out pointer parameters.

ACK. We will take it into account during RFCv3.

Again, thanks for the initial review!

PS: Mr. Kalle confirmed, that disclaimer issue is resolved, that is good,
thanks!

Best regards,
Viktor Barna