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

Very brief review:

+/* */

That seems slightly odd.

+	/* bus private data */
+	char bus_priv[0];

You might want to - for future proofing - add some aligned() attribute.
Otherwise, if struct mutex doesn't have a size that's a multiple of the
pointer size, fields in here will not be aligned right.

+static inline void *get_bus_priv(struct qtnf_bus *bus)
+{
+	if (WARN_ON(!bus)) {
+		pr_err("qtnfmac: invalid bus pointer!\n");
+		return NULL;

Better to just use "WARN(!bus, "qtnfmac: invalid bus pointer!\n");"

Also, for pr_* the "qtnfmac: " prefix should be done with pr_fmt, not
manually.

+#define QLINK_HT_MCS_MASK_LEN	10
+#define QLINK_ETH_ALEN		6
+#define QLINK_MAX_SSID_LEN	32

These seem a bit strange? Why bother? They are standard values.
(not entirely sure what the MCS_MASK_LEN is used for though)

+/*
+ * struct qlink_ht_mcs_info - MCS information
+ *
+ * See &struct ieee80211_mcs_info.
+ */
+struct qlink_ht_mcs_info {
+	u8 rx_mask[QLINK_HT_MCS_MASK_LEN];
+	__le16 rx_highest;
+	u8 tx_params;
+	u8 reserved[3];
+} __packed;
+
+/*
+ * struct qlink_ht_cap - HT capabilities
+ *
+ * "HT capabilities element", see &struct ieee80211_ht_cap.
+ */
+struct qlink_ht_cap {
+	struct qlink_ht_mcs_info mcs;
+	__le32 tx_BF_cap_info;
+	__le16 cap_info;
+	__le16 extended_ht_cap_info;
+	u8 ampdu_params_info;
+	u8 antenna_selection_info;
+} __packed;
+
+/*
+ * struct qlink_vht_mcs_info - VHT MCS information
+ *
+ * See &struct ieee80211_vht_mcs_info.
+ */
+struct qlink_vht_mcs_info {
+	__le16 rx_mcs_map;
+	__le16 rx_highest;
+	__le16 tx_mcs_map;
+	__le16 tx_highest;
+} __packed;
+
+/*
+ * struct qlink_vht_cap - VHT capabilities
+ *
+ * "VHT capabilities element", see &struct ieee80211_vht_cap.
+ */
+struct qlink_vht_cap {
+	__le32 vht_cap_info;
+	struct qlink_vht_mcs_info supp_mcs;
+} __packed;

I think you shouldn't duplicate these, there's no sane way they can
ever be changed in ieee80211.h

+enum qlink_iface_type {
+	QLINK_IFTYPE_AP		= BIT(0),
+	QLINK_IFTYPE_STATION	= BIT(1),
+	QLINK_IFTYPE_ADHOC	= BIT(2),
+	QLINK_IFTYPE_MONITOR	= BIT(3),
+	QLINK_IFTYPE_WDS	= BIT(4),
+};

Not sure how you use these, but BIT() doesn't make a lot of sense for
something that's mutually exclusive?

+/**
+ * Copyright (c) 2015-2016 Quantenna Communications, Inc.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ **/

You should probably not have those double asterisks since they're
reserved for kernel-doc.

+	if (qtnf_cmd_send_start_ap(vif)) {
+		pr_err("failed to issue start AP command\n");
+		return -EFAULT;
+	}
+
+	if (!(vif->bss_status & QTNF_STATE_AP_START)) {
+		pr_err("failed to start AP operations in FW\n");
+		return -EFAULT;
+	}

This is strange - I'd expect send_start_ap() to not actually just send
it, but also wait for a response, and then it can return an error if
the flag didn't get set. If it doesn't, then it's racy, no?

+static int
+qtnf_connect(struct wiphy *wiphy, struct net_device *dev,
+	     struct cfg80211_connect_params *sme)
+{
+	struct qtnf_vif *vif;
+	struct qtnf_bss_config *bss_cfg;
+
+	vif = qtnf_netdev_get_priv(dev);
+	if (!vif) {
+		pr_err("core_attach: could not get valid vif
pointer\n");
+		return -EFAULT;
+	}

It seems that you're overdoing the error checks a bit - I don't see how
this could possibly fail?

+	memcpy(&bss_cfg->crypto, &sme->crypto, sizeof(bss_cfg-

quoted

crypto));

This makes no sense at all - you have to convert the format of this
somehow to make it work - at least endianness has to be fixed, even if
you copied all of the cfg80211 struct.

[snip - lots of stuff I didn't really look at]

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

I think you should not have these - maybe add those with separate
patches later that really can't be done otherwise, and then give very
good rationale for it. Having driver-specific sysfs is not a good idea
in general.

+static inline u64 qtnf_get_unaligned_le64(const __le64 *ptr)
+{
+	return le64_to_cpu(get_unaligned(ptr));
+}
+
+static inline u32 qtnf_get_unaligned_le32(const __le32 *ptr)
+{
+	return le32_to_cpu(get_unaligned(ptr));
+}
+
+static inline u16 qtnf_get_unaligned_le16(const __le16 *ptr)
+{
+	return le16_to_cpu(get_unaligned(ptr));
+}

Huh, what? These exist, or should exist, already.

[snip more]

johannes