Re: [PATCH net-next v2 03/15] idpf: add controlq init and reset checks

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 14 Jun 2023 10:14:16 -0700 Tony Nguyen wrote:
> +static void idpf_ctlq_init_rxq_bufs(struct idpf_ctlq_info *cq)
> +{
> +	int i = 0;
> +
> +	for (i = 0; i < cq->ring_size; i++) {

no need to init i twice

> +	if (!qinfo->len || !qinfo->buf_size ||
> +	    qinfo->len > IDPF_CTLQ_MAX_RING_SIZE ||
> +	    qinfo->buf_size > IDPF_CTLQ_MAX_BUF_LEN)
> +		return -EINVAL;

Looks like defensive programming, it's generally discouraged in 
the kernel.

> +init_free_q:
> +	kfree(cq);
> +	cq = NULL;

no need to clear local variables

> +	return err;
> +}

> +	int i = 0;
> +
> +	INIT_LIST_HEAD(&hw->cq_list_head);
> +
> +	for (i = 0; i < num_q; i++) {

init, again, please fix throughout

> +		struct idpf_ctlq_create_info *qinfo = q_info + i;
> +
> +		err = idpf_ctlq_add(hw, qinfo, &cq);
> +		if (err)
> +			goto init_destroy_qs;
> +	}
> +
> +	return err;

return 0 is more idiomatic, you can't reach it with an errno


> +void idpf_ctlq_deinit(struct idpf_hw *hw)
> +{
> +	struct idpf_ctlq_info *cq = NULL, *tmp = NULL;

You really like to init the stack :S

> +	list_for_each_entry_safe(cq, tmp, &hw->cq_list_head, cq_list)
> +		idpf_ctlq_remove(hw, cq);
> +}

> +	if (!cq || !cq->ring_size)
> +		return -ENOBUFS;

even worse defensive programming

> +	mutex_lock(&cq->cq_lock);
> +
> +	/* Ensure there are enough descriptors to send all messages */
> +	num_desc_avail = IDPF_CTLQ_DESC_UNUSED(cq);
> +	if (num_desc_avail == 0 || num_desc_avail < num_q_msg) {
> +		err = -ENOSPC;
> +		goto sq_send_command_out;

name labels after what they jump to, err_unlock

> +void *idpf_alloc_dma_mem(struct idpf_hw *hw, struct idpf_dma_mem *mem, u64 size)
> +{
> +	struct idpf_adapter *adapter = hw->back;
> +	size_t sz = ALIGN(size, 4096);
> +
> +	mem->va = dma_alloc_coherent(&adapter->pdev->dev, sz,
> +				     &mem->pa, GFP_KERNEL | __GFP_ZERO);

DMA API always zeros memory, I thought cocci warns about this
Did you run cocci checks?