Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

[off-list ref] writes:

From: Avinash Patil <redacted>

This patch adds support for new FullMAC WiFi driver for Quantenna
QSR10G chipsets.

QSR10G is Quantenna's 8x8, 160M, 11ac offering.
QSR10G supports 2 simultaneous WMACs- one 5G and one 2G. 5G WMAC
supports 160M, 8x8 configuration.
FW supports 8 concurrent virtual interfaces on each WMAC.

Patch introduces 2 new drivers- qtnfmac.ko for interfacing with
kernel/cfg80211 and qtnfmac_pcie.ko for PCIe bus interface.

Signed-off-by: Dmitrii Lebed <redacted>
Signed-off-by: Sergei Maksimenko <redacted>
Signed-off-by: Sergey Matyukevich <redacted>
Signed-off-by: Bindu Therthala <redacted>
Signed-off-by: Huizhao Wang <redacted>
Signed-off-by: Kamlesh Rath <redacted>
Signed-off-by: Avinash Patil <redacted>
Signed-off-by: Igor Mitsyanko <redacted>

I read this through and looking good. Below are some comments.

I also saw few warnings but I suspect they are because of cfg80211 API
changes (didn't bother to check that now):

drivers/net/wireless/quantenna/qtnfmac/event.c: In function `qtnf_event_handle_scan_complete':
drivers/net/wireless/quantenna/qtnfmac/event.c:342:2: warning: passing argument 2 of `cfg80211_scan_done' makes pointer from integer without a cast [enabled by default]
./include/net/cfg80211.h:4104:6: note: expected `struct cfg80211_scan_info *' but argument is of type `u32'
drivers/net/wireless/quantenna/qtnfmac/cfg80211.c: In function `qtnf_virtual_intf_cleanup':
drivers/net/wireless/quantenna/qtnfmac/cfg80211.c:1093:4: warning: passing argument 2 of `cfg80211_scan_done' makes pointer from integer without a cast [enabled by default]
./include/net/cfg80211.h:4104:6: note: expected `struct cfg80211_scan_info *' but argument is of type `int'

Resend to correct email kvalo@codeaurora.org.

BTW, no need to send me directly. I take patches directly from patchwork
so sending them to linux-wireless is enough. Doesn't do any harm to send
them to, I just immediately delete them from my INBOX :)

Changelist V1->V2:
1. Get rid of confidentiality footer.
2. Rewrite qlink.h header to make it easier to use, add
documentation to most of qlink.h definitions.

 MAINTAINERS                                        |    8 +
 drivers/net/wireless/Kconfig                       |    1 +
 drivers/net/wireless/Makefile                      |    1 +
 drivers/net/wireless/quantenna/Kconfig             |   16 +
 drivers/net/wireless/quantenna/Makefile            |    6 +
 drivers/net/wireless/quantenna/include/bus.h       |  195 ++
 .../wireless/quantenna/include/pcie_regs_pearl.h   |  353 ++++
 drivers/net/wireless/quantenna/include/qlink.h     |  939 ++++++++++
 .../net/wireless/quantenna/include/qtn_hw_ids.h    |   34 +
 .../net/wireless/quantenna/include/shm_ipc_defs.h  |   46 +

Is there a particular reason why you have a separate include directory?
There's just one quantenna driver for now so the extra include directory
feels unnecessary. I would prefer to have that only once there are two
quantenna drivers and we know exactly what is shared between the two.

The include directory is not listed.

+ccflags-y += -D__CHECK_ENDIAN

Very good.

+/* Supported rates to be advertised to the cfg80211 */
+static struct ieee80211_rate qtnf_rates[] = {
+	{.bitrate = 10, .hw_value = 2, },
+	{.bitrate = 20, .hw_value = 4, },
+	{.bitrate = 55, .hw_value = 11, },
+	{.bitrate = 110, .hw_value = 22, },
+	{.bitrate = 60, .hw_value = 12, },
+	{.bitrate = 90, .hw_value = 18, },
+	{.bitrate = 120, .hw_value = 24, },
+	{.bitrate = 180, .hw_value = 36, },
+	{.bitrate = 240, .hw_value = 48, },
+	{.bitrate = 360, .hw_value = 72, },
+	{.bitrate = 480, .hw_value = 96, },
+	{.bitrate = 540, .hw_value = 108, },
+};
+
+/* Channel definitions to be advertised to cfg80211 */
+static struct ieee80211_channel qtnf_channels_2ghz[] = {
+	{.center_freq = 2412, .hw_value = 1, },
+	{.center_freq = 2417, .hw_value = 2, },
+	{.center_freq = 2422, .hw_value = 3, },
+	{.center_freq = 2427, .hw_value = 4, },
+	{.center_freq = 2432, .hw_value = 5, },
+	{.center_freq = 2437, .hw_value = 6, },
+	{.center_freq = 2442, .hw_value = 7, },
+	{.center_freq = 2447, .hw_value = 8, },
+	{.center_freq = 2452, .hw_value = 9, },
+	{.center_freq = 2457, .hw_value = 10, },
+	{.center_freq = 2462, .hw_value = 11, },
+	{.center_freq = 2467, .hw_value = 12, },
+	{.center_freq = 2472, .hw_value = 13, },
+	{.center_freq = 2484, .hw_value = 14, },
+};

I guess some of these static variables could be also const, but didn't
check.

+MODULE_AUTHOR("Quantenna Communications");
+MODULE_DESCRIPTION("Quantenna 802.11 wireless LAN FullMAC driver.");
+MODULE_LICENSE("Dual BSD/GPL");

[...]

MODULE_LICENSE() doesn't match license in headers, is that correct?

+static int
+qtnf_event_handle_sta_assoc(struct qtnf_wmac *mac, struct qtnf_vif *vif,
+			    const struct qlink_event_sta_assoc *sta_assoc,
+			    u16 len)
+{
+	const u8 *sta_addr;
+	u16 frame_control;
+	struct station_info sinfo = { 0 };
+	size_t payload_len;
+	u16 tlv_type;
+	u16 tlv_value_len;
+	size_t tlv_full_len;
+	const struct qlink_tlv_hdr *tlv;
+
+	if (unlikely(len < sizeof(*sta_assoc))) {
+		pr_err("%s: payload is too short (%u < %zu)\n", __func__,
+		       len, sizeof(*sta_assoc));
+		return -EINVAL;
+	}

I see unlikely() used a lot, I counted 145 times. Not a big issue but I
don't see the point. In hot path I understand using it, but not
everywhere.

+/* sysfs knobs: stats and other diagnistics */

Johannes also commented about this. Please use debugfs or some generic
interface.

-- 
Kalle Valo