netdev - Re: [PATCH v5 net-next 14/18] ionic: Add Tx and Rx handling

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: <c34c828d-c511-079f-ffcc-bf6c6bb9a5d7@pensando.io>
Date:   Tue, 27 Aug 2019 11:52:03 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Yunsheng Lin <linyunsheng@...wei.com>, netdev@...r.kernel.org,
        davem@...emloft.net
Subject: Re: [PATCH v5 net-next 14/18] ionic: Add Tx and Rx handling

On 8/26/19 7:32 PM, Yunsheng Lin wrote:
> On 2019/8/27 5:33, Shannon Nelson wrote:
>> Add both the Tx and Rx queue setup and handling.  The related
>> stats display comes later.  Instead of using the generic napi
>> routines used by the slow-path commands, the Tx and Rx paths
>> are simplified and inlined in one file in order to get better
>> compiler optimizations.
>>
>> Signed-off-by: Shannon Nelson <snelson@...sando.io>
>> ---
[...]
>> +static int ionic_txrx_init(struct ionic_lif *lif)
>> +{
>> +	unsigned int i;
>> +	int err;
>> +
>> +	for (i = 0; i < lif->nxqs; i++) {
>> +		err = ionic_lif_txq_init(lif, lif->txqcqs[i].qcq);
>> +		if (err)
>> +			goto err_out;
>> +
>> +		err = ionic_lif_rxq_init(lif, lif->rxqcqs[i].qcq);
>> +		if (err) {
>> +			ionic_lif_qcq_deinit(lif, lif->txqcqs[i-1].qcq);
>> +			goto err_out;
>> +		}
>> +	}
>> +
>> +	ionic_set_rx_mode(lif->netdev);
>> +
>> +	return 0;
>> +
>> +err_out:
>> +	for (i--; i > 0; i--) {
>> +		ionic_lif_qcq_deinit(lif, lif->txqcqs[i-1].qcq);
>> +		ionic_lif_qcq_deinit(lif, lif->rxqcqs[i-1].qcq);
>> +	}
> The "i--" has been done in for initialization, and
> ionic_lif_qcq_deinit is called with lif->rxqcqs[i-1], which may
> cause the last lif->txqcqs or lif->rxqcqs not initialized problem.
>
> It may be more common to do the below:
> while (i--) {
> 	ionic_lif_qcq_deinit(lif, lif->txqcqs[i].qcq);
> 	ionic_lif_qcq_deinit(lif, lif->rxqcqs[i].qcq);
> }

Sure.

>> +
>> +	return err;
>> +}
>> +
>> +static int ionic_txrx_enable(struct ionic_lif *lif)
>> +{
>> +	int i, err;
>> +
>> +	for (i = 0; i < lif->nxqs; i++) {
>> +		err = ionic_qcq_enable(lif->txqcqs[i].qcq);
>> +		if (err)
>> +			goto err_out;
>> +
>> +		ionic_rx_fill(&lif->rxqcqs[i].qcq->q);
>> +		err = ionic_qcq_enable(lif->rxqcqs[i].qcq);
>> +		if (err) {
>> +			ionic_qcq_disable(lif->txqcqs[i].qcq);
>> +			goto err_out;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +
>> +err_out:
>> +	for (i--; i >= 0 ; i--) {
>> +		ionic_qcq_disable(lif->rxqcqs[i].qcq);
>> +		ionic_qcq_disable(lif->txqcqs[i].qcq);
>> +	}
> It may be better to use the above pattern too.

Okay


>> +static dma_addr_t ionic_tx_map_single(struct ionic_queue *q, void *data, size_t len)
>> +{
>> +	struct ionic_tx_stats *stats = q_to_tx_stats(q);
>> +	struct device *dev = q->lif->ionic->dev;
>> +	dma_addr_t dma_addr;
>> +
>> +	dma_addr = dma_map_single(dev, data, len, DMA_TO_DEVICE);
>> +	if (dma_mapping_error(dev, dma_addr)) {
>> +		net_warn_ratelimited("%s: DMA single map failed on %s!\n",
>> +				     q->lif->netdev->name, q->name);
>> +		stats->dma_map_err++;
>> +		return 0;
> zero may be a valid dma address, maybe check the dma_mapping_error in
> ionic_tx_tso instead.

Hmmm, hadn't thought of 0 as a valid address...
I'll need to make a similar adjustment to ionic_tx_map_frag() uses.

>
>
> +
> +static void ionic_tx_tcp_inner_pseudo_csum(struct sk_buff *skb)
> +{
> +	skb_cow_head(skb, 0);
> May need to check for return error of skb_cow_head.

Sure, and in both places.

Thanks,
sln