linux-kernel - Re: [PATCH 1/2] net: mv643xx_eth: use {readl|writel}

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 13 May 2016 20:19:55 +0800
From:	Jisheng Zhang <jszhang@...vell.com>
To:	Arnd Bergmann <arnd@...db.de>
CC:	<linux-arm-kernel@...ts.infradead.org>,
	<sebastian.hesselbarth@...il.com>, <davem@...emloft.net>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] net: mv643xx_eth: use {readl|writel}_relaxed
 instead of readl/writel

Dear Arnd,

On Fri, 13 May 2016 14:09:43 +0200 Arnd Bergmann wrote:

> On Friday 13 May 2016 19:59:19 Jisheng Zhang wrote:
> >  /* port register accessors **************************************************/
> >  static inline u32 rdl(struct mv643xx_eth_private *mp, int offset)
> >  {
> > -	return readl(mp->shared->base + offset);
> > +	return readl_relaxed(mp->shared->base + offset);
> >  }
> >  
> >  static inline u32 rdlp(struct mv643xx_eth_private *mp, int offset)
> >  {
> > -	return readl(mp->base + offset);
> > +	return readl_relaxed(mp->base + offset);
> >  }  
> 
> I'd recommend not changing these in general, but introducing new 'rdl_relaxed()'
> and 'rdlp_relaxed()' etc variants that you use in the code that actually
> is performance sensitive, but use the normal non-relaxed versions by
> default.
> 
> Then add a comment to each use of the relaxed accessors on how you know
> that it's safe for that caller. This usually is just needed for the xmit()
> function and for the interrupt handler.

Got your points and I do think it makes sense. But could we always use the
relaxed version to save some LoCs, although I have no mv643xx_eth platform
but I can confirm similar relaxed version changes in pxa168_eth is safe and
this is what we do in product's kernel.

Above is just my humble opinion, comments are welcome.

Thanks,
Jisheng

>   
> > @@ -2642,10 +2642,10 @@ mv643xx_eth_conf_mbus_windows(struct mv643xx_eth_shared_private *msp,
> >  	int i;
> >  
> >  	for (i = 0; i < 6; i++) {
> > -		writel(0, base + WINDOW_BASE(i));
> > -		writel(0, base + WINDOW_SIZE(i));
> > +		writel_relaxed(0, base + WINDOW_BASE(i));
> > +		writel_relaxed(0, base + WINDOW_SIZE(i));
> >  		if (i < 4)
> > -			writel(0, base + WINDOW_REMAP_HIGH(i));
> > +			writel_relaxed(0, base + WINDOW_REMAP_HIGH(i));
> >  	}
> >  
> >  	win_enable = 0x3f;  
> 
> Configuring the mbus for instance is clearly not an operation in which
> performance matters at all, so better not touch that.
> 
> > @@ -2674,8 +2675,8 @@ static void infer_hw_params(struct mv643xx_eth_shared_private *msp)
> >  	 * [21:8], or a 16-bit coal limit in bits [25,21:7] of the
> >  	 * SDMA config register.
> >  	 */
> > -	writel(0x02000000, msp->base + 0x0400 + SDMA_CONFIG);
> > -	if (readl(msp->base + 0x0400 + SDMA_CONFIG) & 0x02000000)
> > +	writel_relaxed(0x02000000, msp->base + 0x0400 + SDMA_CONFIG);
> > +	if (readl_relaxed(msp->base + 0x0400 + SDMA_CONFIG) & 0x02000000)
> >  		msp->extended_rx_coal_limit = 1;
> >  	else
> >  		msp->extended_rx_coal_limit = 0;  
> 
> 
> This also seems to be configuration, rather than in the packet rx/tx hotpath.
> 
> 	Arnd