netdev - RE: [PATCH] wireless: adf702x: new driver for ADF7020/21 parts

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: <1261497729.2845.8.camel@achroite.uk.solarflarecom.com>
Date:	Tue, 22 Dec 2009 16:02:09 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	"Hennerich, Michael" <Michael.Hennerich@...log.com>
Cc:	Mike Frysinger <vapier@...too.org>, netdev@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	uclinux-dist-devel@...ckfin.uclinux.org
Subject: RE: [PATCH] wireless: adf702x: new driver for ADF7020/21 parts

On Tue, 2009-12-22 at 11:46 +0000, Hennerich, Michael wrote:
[...]
> >
> >> +	unsigned rx:1;
> >> +	unsigned rx_size;
> >> +	u32 *rx_buf;
> >> +	u32 *tx_buf;
> >> +
> >> +	/* Base reg base of SPORT controller */
> >> +	volatile struct sport_register *sport;
> >
> >MMIO pointers should normally be declared like:
> >
> >	struct sport_register __iomem *sport;
> >
> >and used with readl, writel etc.
> 
> Well SPORT is a SoC peripheral it's mapped into Blackfin System MMR space.
> Our versions of readX writeX are implemented for off-chip peripherals.
> If you take a look at Blackfin io.h you will notice that we disable global interrupts, 
> to avoid killed and reissued reads. This is not necessary for the System MMR space.   
> 
> Preferably I like to keep this as is, but I could also change it to use the 
> Blackfin MMR accessor functions bfin_read{32,16} bfin_write{32,16}  

I don't know any specifics of the Blackfin architecture, so you should
probably do what you think best!

[...]
> >> +static irqreturn_t adf702x_rx_interrupt(int irq, void *dev_id)
> >> +{
> >[...]
> >> +			lp->rx_size = adf702x_getrxsize(lp, offset);
> >> +			if (offset == 1) {
> >> +				set_dma_x_count(lp->dma_ch_rx, lp->rx_size);
> >> +				set_dma_start_addr(lp->dma_ch_rx,
> >> +					(unsigned long)lp->rx_buf);
> >> +			} else {
> >> +				lp->rx_buf[0] = lp->rx_buf[3];
> >> +				set_dma_x_count(lp->dma_ch_rx, lp->rx_size - 1);
> >> +				set_dma_start_addr(lp->dma_ch_rx,
> >> +					(unsigned long)&lp->rx_buf[1]);
> >> +			}
> >> +			enable_dma(lp->dma_ch_rx);
> >> +			SSYNC();
> >
> >Is this some odd kind of memory barrier?
> 
> Well - it's an instruction that ensures that the write buffers are flushed and all instructions executed. 

I suspect you only need a write memory barrier here (wmb()) not a
pipeline flush.

[...]
> >> +	ndev->mtu = 576;
> >
> >Even though you use Ethernet frames?
> 
> We could stick with the default MTU - but it would worsen the overall response time.
[...]

Do what you think is best; I was just checking that you have a good
reason for this.

Ben,

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
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