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

[Johannes Berg <johannes@sipsolutions.net>]

On Wed, 2016-11-09 at 17:00 +0300, igor.mitsyanko.os@quantenna.com
wrote:

> +static int
> +qtnf_change_virtual_intf(struct wiphy *wiphy,
> +			 struct net_device *dev,
> +			 enum nl80211_iftype type, u32 *flags,
> +			 struct vif_params *params)
> +{
> +	struct qtnf_vif *vif;
> +	u8 *mac_addr;
> +
> +	vif = qtnf_netdev_get_priv(dev);
> +
> +	if (params)
> +		mac_addr = params->macaddr;
> +	else
> +		mac_addr = NULL;
> +
> +	if (qtnf_cmd_send_change_intf_type(vif, type, mac_addr)) {
> +		pr_err("failed to change interface type\n");
> +		return -EFAULT;
> +	}
> +
> +	vif->wdev.iftype = type;
> +	return 0;
> +}

Do you really support arbitrary type changes?

You might even have to handle ongoing scans, etc.

> +	/* Clear the vif in mac */

"mac"? Maybe you mean cfg80211?

> +	vif->netdev->ieee80211_ptr = NULL;
> +	vif->netdev = NULL;
> +	vif->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;
> +	eth_zero_addr(vif->mac_addr);
> +
> +	return 0;
> +}

But I'm not sure this makes sense? You're not actually deleting the
interface here, so why sever all the links/clear all the data?


> +/* concatenate all the beacon IEs into one buffer
> + * Take IEs from head, tail and beacon_ies fields of
cfg80211_beacon_data
> + * and append it to provided buffer.
> + * Checks total IE buf length to be <= than IEEE80211_MAX_DATA_LEN.
> + * Checks IE buffers to be valid, so that resulting buffer
> + * should be a valid IE buffer with length <=
IEEE80211_MAX_DATA_LEN.
> + */

I'm not sure this is right - beacon_ies is head+tail already, I think?

> +static int
> +qtnf_dump_station(struct wiphy *wiphy, struct net_device *dev,
> +		  int idx, u8 *mac, struct station_info *sinfo)
> +{
> +	struct qtnf_vif *vif = qtnf_netdev_get_priv(dev);
> +	const struct qtnf_sta_node *sta_node;
> +	int ret;
> +
> +	sta_node = qtnf_sta_list_lookup_index(&vif->sta_list, idx);
> +
> +	if (unlikely(!sta_node))
> +		return -ENOENT;
> +
> +	ether_addr_copy(mac, sta_node->mac_addr);
> +
> +	ret = qtnf_cmd_get_sta_info(vif, sta_node->mac_addr, sinfo);
> +
> +	if (unlikely(ret == -ENOENT)) {
> +		sinfo->filled = 0;
> +		ret = 0;
> +	}

This case seems slightly odd - what does it mean that the station
existed, but getting the information returned -ENOENT? Is that because
it's racy, somehow? If so, wouldn't it be better to take this as an
indication that the station doesn't exist, and skip this entry entirely
or something?

> +	/* nofity cfg80211 */

typo :)

> +	while (payload_len >= sizeof(struct qlink_tlv_hdr)) {
> +		tlv_type = le16_to_cpu(tlv->type);
> +		tlv_value_len = le16_to_cpu(tlv->len);
> +		tlv_full_len = tlv_value_len + sizeof(struct
qlink_tlv_hdr);
> +
> +		if (tlv_full_len > payload_len) {
> +			pr_warn("malformed TLV 0x%.2X; LEN: %u\n",
> +				tlv_type, tlv_value_len);
> +			return -EINVAL;
> +		}
> +
> +		if (tlv_type == QTN_TLV_ID_IE_SET) {
> +			ies = tlv->val;
> +			ies_len = tlv_value_len;
> +		}
> +
> +		payload_len -= tlv_full_len;
> +		tlv = (struct qlink_tlv_hdr *)(tlv->val +
tlv_value_len);
> +	}
> +
> +	if (payload_len) {
> +		pr_warn("malformed IEs buf; bytes left: %zu\n",
payload_len);
> +		return -EINVAL;
> +	}

Don't you mean "malformed TLVs buf"? It's obviously similar, but you
refer to this encoding as TLV, not IE.

Maybe you should ignore it too, since it's a firmware bug?

> +	qdev_vif = netdev_priv(dev);
> +	*((unsigned long *)qdev_vif) = (unsigned long)vif;

This seems very strange - why unsigned long, rather than void? I mean

 *(void **)qdev_vif = vif;

> +static int qtnf_pcie_init_shm_ipc(struct qtnf_pcie_bus_priv *priv)
> +{
> +	struct qtnf_shm_ipc_region __iomem *ipc_tx_reg;
> +	struct qtnf_shm_ipc_region __iomem *ipc_rx_reg;
> +	const struct qtnf_shm_ipc_int ipc_int = {
qtnf_ipc_gen_ep_int, priv };
> +	const struct qtnf_shm_ipc_rx_callback rx_callback = {
> +					qtnf_pcie_control_rx_callbac
k, priv };

If those are const, why not also static? In fact, it seems they really
should be, since they're registered below?

> +static int alloc_bd_table(struct qtnf_pcie_bus_priv *priv)
> +{
> +	unsigned long vaddr;
> +	dma_addr_t paddr;
> +	int len;
> +
> +	len = priv->tx_bd_num * sizeof(struct qtnf_tx_bd) +
> +		priv->rx_bd_num * sizeof(struct qtnf_rx_bd);
> +
> +	vaddr = (unsigned long)dmam_alloc_coherent(&priv->pdev->dev,
> +						   len, &paddr,
GFP_KERNEL);
> +	if (!vaddr)
> +		return -ENOMEM;
> +
> +	/* tx bd */
> +
> +	memset((void *)vaddr, 0, len);

Those unsigned long/void * casts look strange too. Why not use a "void
*vaddr" to start with?

> +	priv->bd_table_vaddr = vaddr;

Maybe need a cast here, if that variable is needed at all (identical to
tx_bd_vbase), or make that struct member also void *?

> +	priv->bd_table_paddr = paddr;
> +	priv->bd_table_len = len;
> +
> +	priv->tx_bd_vbase = (struct qtnf_tx_bd *)vaddr;

Don't even need that cast then.

> +	priv->tx_bd_pbase = paddr;
> +
> +	pr_debug("TX descriptor table: vaddr=0x%p paddr=%pad\n",
> +		 (void *)vaddr, &paddr);
> +
> +	priv->tx_bd_reclaim_start = 0;
> +	priv->tx_bd_index = 0;
> +	priv->tx_queue_len = 0;
> +
> +	/* rx bd */
> +
> +	vaddr += priv->tx_bd_num * sizeof(struct qtnf_tx_bd);

Here you can do something like

 vaddr = ((struct qtnf_tx_bd)vaddr) + priv->tx_bd_num;

> +	paddr += priv->tx_bd_num * sizeof(struct qtnf_tx_bd);
> +
> +	priv->rx_bd_vbase = (struct qtnf_rx_bd *)vaddr;

no need for the cast here then.

> +	priv->rx_bd_pbase = paddr;
> +
> +	writel(QTN_HOST_LO32(paddr),
> +	       PCIE_HDP_TX_HOST_Q_BASE_L(priv->pcie_reg_base));
> +	writel(QTN_HOST_HI32(paddr),
> +	       PCIE_HDP_TX_HOST_Q_BASE_H(priv->pcie_reg_base));
> +	writel(priv->rx_bd_num | (sizeof(struct qtnf_rx_bd)) << 16,
> +	       PCIE_HDP_TX_HOST_Q_SZ_CTRL(priv->pcie_reg_base));
> +
> +	priv->hw_txproc_wr_ptr = priv->rx_bd_num -
rx_bd_reserved_param;
> +
> +	writel(priv->hw_txproc_wr_ptr,
> +	       PCIE_HDP_TX_HOST_Q_WR_PTR(priv->pcie_reg_base));
> +
> +	pr_debug("RX descriptor table: vaddr=0x%p paddr=%pad\n",
> +		 (void *)vaddr, &paddr);

Nor here.

On the whole, it probably doesn't really matter (I'd let Kalle decide I
guess). Just looks odd to me.

> +	/* sync up all descriptor updates before passing them to EP
*/
> +	wmb();

I think you need dma_wmb()?


So I mostly looked at the cfg80211 bits, obviously - the other comments
were just in passing.

I also didn't review the flows - some of these things are tricky (e.g.
are there races between userspace asking to disconnect, and disconnect
notification, and similar). Maybe it helps anyway :)

johannes