RE: [PATCH net-next 1/4] enetc: Introduce basic PF and VF ENETC ethernet drivers

[Claudiu Manoil]

>-----Original Message-----
>From: David Miller <davem@xxxxxxxxxxxxx>
>Sent: Saturday, November 17, 2018 10:08 PM
>To: Claudiu Manoil <claudiu.manoil@xxxxxxx>
>Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Alexandru
>Marginean <alexandru.marginean@xxxxxxx>; Catalin Horghidan
><catalin.horghidan@xxxxxxx>
>Subject: Re: [PATCH net-next 1/4] enetc: Introduce basic PF and VF ENETC
>ethernet drivers
>
>From: Claudiu Manoil <claudiu.manoil@xxxxxxx>
>Date: Thu, 15 Nov 2018 18:13:53 +0200
>
>> +	if (!nr_frags && !(flags & ENETC_TXBD_FLAGS_EX))
>> +		flags |= ENETC_TXBD_FLAGS_F;
>> +
>> +	txbd->flags = flags;
>> +
>> +	if (flags & ENETC_TXBD_FLAGS_EX) {
> ...
>> +		if (!nr_frags)
> ...
>> +	}
>> +
>> +	frag = &skb_shinfo(skb)->frags[0];
>> +	for (f = 0; f < nr_frags; f++, frag++) {
> ...
>> +	}
>> +
>> +	if (nr_frags)
>> +		/* last BD needs 'F' bit set */
>> +		txbd->flags = ENETC_TXBD_FLAGS_F;
>
>Isn't it so much simpler to just have an unconditional:
>
>	txbd->flags |= ENETC_TXBD_FLAGS_F;
>
>at the end of this code segment?
>
>Then all of this code conditional on nr_frags can go away.

True, but this code also avoids reading from the buffer descriptor memory
area (dma coherent), which is not cheap.  As it is now, the driver only writes
the txbd  (excepting the extension descriptor).  I'll look into how big this penalty
really is.  I'm also in favor of more readable code.

Thanks,
Claudiu