netdev - Re: [net-next 1/5] stmmac: add CHAINED descriptor mode support (V2)

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: <4E97E079.8030205@st.com>
Date:	Fri, 14 Oct 2011 09:10:49 +0200
From:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org, rayagond@...avyalabs.com
Subject: Re: [net-next 1/5] stmmac: add CHAINED descriptor mode support (V2)

On 10/13/2011 10:39 PM, David Miller wrote:
> From: Giuseppe CAVALLARO <peppe.cavallaro@...com>
> Date: Wed, 12 Oct 2011 15:38:04 +0200
> 
>> +#if defined(CONFIG_STMMAC_RING)
>> +
>> +static unsigned int stmmac_jumbo_frm(struct stmmac_priv *priv,
>> +				     struct sk_buff *skb, int csum_insertion)
>> +{
> 
> This is not exactly what I meant.
> 
> In your original patch, two or three line snippets of code were conditionalized.
> 
> That's what I wanted you to do here.  Keep as much common code around as possible
> in the driver *.c file, but the small 2 or 3 line conditional parts are implemented
> in very small well contained inline functions implemented in a header file.
> 
> These small, 2 or 3 line, inline functions are where the ifdefs go.
> 
> I didn't mean to replicate all of the functions, in their entirety, into some
> header file.

This is what I wanted to do indeed. :-(

I had added new small functions like where possible (used in the main):

static void stmmac_refill_desc3(int bfsize, struct dma_desc *p)
static void stmmac_init_desc3(int des3_as_data_buf, struct dma_desc *p)
static void stmmac_clean_desc3(struct dma_desc *p)

I guess this is what you actually wanted.

In other cases, I had put two implementation of the same function
specialized for ring and chained mode. This was the case of the enhanced
and normal descriptors. Instead of implementing new inline funtcs I
direcly moved the functions themselves into the header because small enough.

For example

inline void enh_desc_release_tx_desc(struct dma_desc *p)
{
	memset(p, 0, offsetof(struct dma_desc, des2));
	p->des01.etx.second_address_chained = 1;
}

and
inline void enh_desc_release_tx_desc(struct dma_desc *p)
{
	int ter = p->des01.etx.end_ring;

	memset(p, 0, offsetof(struct dma_desc, des2));
	p->des01.etx.end_ring = ter;
}


Unfortunately, jumbo frame function is big :-( and I agree with you that
it's not good to have this in the Header.

At any rate, I'll try to reduce the code in the header as much possible
although this makes more complex the driver's API.

Thanks for your feedback.

Let me know for other advice and comments

Regards
Peppe

> 
> You might was well put the entire driver into a header file, then you can add
> all the ifdefs you want :-)
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html