Re: [PATCH 3/4] myri10ge - Driver core

[Brice Goglin]

Arnd Bergmann wrote:
>> +static int myri10ge_open(struct net_device *dev)
>>     
>
> This function is too long to read easily.
>   

Ok we have split it a little bit.

>> +	/* allocate the host shadow rings */
>> +
>> +	bytes = 8 + (MYRI10GE_MCP_ETHER_MAX_SEND_DESC_TSO + 4)
>> +	    * sizeof(*mgp->tx.req_list);
>> +	mgp->tx.req_bytes = kmalloc(bytes, GFP_KERNEL);
>> +	if (mgp->tx.req_bytes == NULL)
>> +		goto abort_with_nothing;
>> +	memset(mgp->tx.req_bytes, 0, bytes);
>> +
>> +	/* ensure req_list entries are aligned to 8 bytes */
>> +	mgp->tx.req_list = (struct mcp_kreq_ether_send *)
>> +	    ALIGN((unsigned long)mgp->tx.req_bytes, 8);
>> +
>> +	bytes = rx_ring_entries * sizeof(*mgp->rx_small.shadow);
>> +	mgp->rx_small.shadow = kmalloc(bytes, GFP_KERNEL);
>> +	if (mgp->rx_small.shadow == NULL)
>> +		goto abort_with_tx_req_bytes;
>> +	memset(mgp->rx_small.shadow, 0, bytes);
>> +
>> +	bytes = rx_ring_entries * sizeof(*mgp->rx_big.shadow);
>> +	mgp->rx_big.shadow = kmalloc(bytes, GFP_KERNEL);
>> +	if (mgp->rx_big.shadow == NULL)
>> +		goto abort_with_rx_small_shadow;
>> +	memset(mgp->rx_big.shadow, 0, bytes);
>> +
>> +	/* allocate the host info rings */
>> +
>> +	bytes = tx_ring_entries * sizeof(*mgp->tx.info);
>> +	mgp->tx.info = kmalloc(bytes, GFP_KERNEL);
>> +	if (mgp->tx.info == NULL)
>> +		goto abort_with_rx_big_shadow;
>> +	memset(mgp->tx.info, 0, bytes);
>> +
>> +	bytes = rx_ring_entries * sizeof(*mgp->rx_small.info);
>> +	mgp->rx_small.info = kmalloc(bytes, GFP_KERNEL);
>> +	if (mgp->rx_small.info == NULL)
>> +		goto abort_with_tx_info;
>> +	memset(mgp->rx_small.info, 0, bytes);
>> +
>> +	bytes = rx_ring_entries * sizeof(*mgp->rx_big.info);
>> +	mgp->rx_big.info = kmalloc(bytes, GFP_KERNEL);
>> +	if (mgp->rx_big.info == NULL)
>> +		goto abort_with_rx_small_info;
>> +	memset(mgp->rx_big.info, 0, bytes);
>> +
>>     
>
> Can you do all these allocations at once? Maybe you can even
> move them into the size passed to alloc_etherdev.
>   

They are different things conceptually, so we prefer to allocate
them separately.

> If you need separate allocations, using kzalloc simplifies
> your code.
>   

Right, will do.

>> +		/* Break the SKB or Fragment up into pieces which
>> +		 * do not cross mgp->tx.boundary */
>> +		low = MYRI10GE_LOWPART_TO_U32(bus);
>> +		high_swapped = htonl(MYRI10GE_HIGHPART_TO_U32(bus));
>> +		while (len) {
>> +			u8 flags_next;
>> +			int cum_len_next;
>> +
>> +			if (unlikely(count == max_segments))
>> +				goto abort_linearize;
>> +
>> +			boundary = (low + tx->boundary) & ~(tx->boundary - 1);
>> +			seglen = boundary - low;
>> +			if (seglen > len)
>> +				seglen = len;
>> +			flags_next = flags & ~MYRI10GE_MCP_ETHER_FLAGS_FIRST;
>> +			cum_len_next = cum_len + seglen;
>> +#ifdef NETIF_F_TSO
>> +			if (mss) {	/* TSO */
>> +				(req - rdma_count)->rdma_count = rdma_count + 1;
>> +
>> +				if (likely(cum_len >= 0)) {	/* payload */
>> +					int next_is_first, chop;
>> +
>> +					chop = (cum_len_next > mss);
>> +					cum_len_next = cum_len_next % mss;
>> +					next_is_first = (cum_len_next == 0);
>> +					flags |= chop *
>> +					    MYRI10GE_MCP_ETHER_FLAGS_TSO_CHOP;
>> +					flags_next |= next_is_first *
>> +					    MYRI10GE_MCP_ETHER_FLAGS_FIRST;
>> +					rdma_count |= -(chop | next_is_first);
>> +					rdma_count += chop & !next_is_first;
>> +				} else if (likely(cum_len_next >= 0)) {	/* header ends */
>> +					int small;
>> +
>> +					rdma_count = -1;
>> +					cum_len_next = 0;
>> +					seglen = -cum_len;
>> +					small =
>> +					    (mss <=
>> +					     MYRI10GE_MCP_ETHER_SEND_SMALL_SIZE);
>> +					flags_next =
>> +					    MYRI10GE_MCP_ETHER_FLAGS_TSO_PLD |
>> +					    MYRI10GE_MCP_ETHER_FLAGS_FIRST |
>> +					    (small *
>> +					     MYRI10GE_MCP_ETHER_FLAGS_SMALL);
>> +				}
>> +			}
>>     
>
> 100 characters per line are too much, as are six levels of intentation,
> or the number of lines in this function.
> You should try to split into into smaller ones.
>   

We have tried :) But there is no nice way to split this code. So I guess
we will have to keep it like this.

Thanks,
Brice

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/