linux-kernel - RE: [PATCH net-next 1/9] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BY2PR0701MB2005075560E4EA9F455049988DCF0@BY2PR0701MB2005.namprd07.prod.outlook.com>
Date:   Sat, 10 Jun 2017 12:43:21 +0000
From:   "Mintz, Yuval" <Yuval.Mintz@...ium.com>
To:     Salil Mehta <salil.mehta@...wei.com>,
        "davem@...emloft.net" <davem@...emloft.net>
CC:     "yisen.zhuang@...wei.com" <yisen.zhuang@...wei.com>,
        "huangdaode@...ilicon.com" <huangdaode@...ilicon.com>,
        "lipeng321@...wei.com" <lipeng321@...wei.com>,
        "mehta.salil.lnk@...il.com" <mehta.salil.lnk@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linuxarm@...wei.com" <linuxarm@...wei.com>
Subject: RE: [PATCH net-next 1/9] net: hns3: Add support of HNS3 Ethernet
 Driver for hip08 SoC

> +static void hns3_nic_net_down(struct net_device *ndev) {
> +	struct hns3_nic_priv *priv = netdev_priv(ndev);
> +	struct hnae3_ae_ops *ops;
> +	int i;
> +
> +	netif_tx_stop_all_queues(ndev);
> +	netif_carrier_off(ndev);
> +	netif_tx_disable(ndev);
> +
> +	ops = priv->ae_handle->ae_algo->ops;
> +
> +	if (ops->stop)
> +		ops->stop(priv->ae_handle);
> +
> +	netif_tx_stop_all_queues(ndev);

Looks a bit excessive. Why do you need all these netif_tx_stop_all_queues()?

> +int hns3_nic_net_xmit_hw(struct net_device *ndev,
...
> +out_map_frag_fail:
> +
> +	while (ring->next_to_use != next_to_use) {
> +		if (ring->next_to_use != next_to_use)
> +			dma_unmap_page(dev,
> +				       ring->desc_cb[ring->next_to_use].dma,
> +				       ring->desc_cb[ring->next_to_use].length,
> +				       DMA_TO_DEVICE);
> +		else
> +			dma_unmap_single(dev,
> +					 ring->desc_cb[next_to_use].dma,
> +					 ring->desc_cb[next_to_use].length,
> +					 DMA_TO_DEVICE);
> +	}

Something looks completely broken in this error-handling 'loop'.

> +static int hns3_setup_tc(struct net_device *ndev, u8 tc) {
...
> +	/* Assign UP2TC map for the VSI */
> +	for (i = 0; i < HNAE3_MAX_TC; i++) {
> +		netdev_set_prio_tc_map(ndev,
> +				       kinfo->tc_info[i].up,
> +				       kinfo->tc_info[i].tc);
> +	}
...
> +static int hns3_nic_setup_tc(struct net_device *dev, u32 handle,
> +			     u32 chain_index, __be16 protocol,
> +			     struct tc_to_netdev *tc)
> +{
> +	if (handle != TC_H_ROOT || tc->type != TC_SETUP_MQPRIO)
> +		return -EINVAL;
> +
> +	return hns3_setup_tc(dev, tc->mqprio->num_tc); }

Isn't mqprio going to override your priority2tc mapping with the one provided
by user?

> +
> +static int hns3_handle_rx_bd(struct hns3_enet_ring *ring,
> +			     struct sk_buff **out_skb, int *out_bnum) {
...
> +	/* Prefetch first cache line of first page */
> +	prefetch(va);
> +#if L1_CACHE_BYTES < 128
> +	prefetch(va + L1_CACHE_BYTES);
> +#endif

Might be better to comment what you're actually fetching