Re: pci_set_mwi() and ath5k

[Matthew Wilcox <matthew@wil.cx>]

On Wed, Nov 04, 2009 at 05:14:26PM -0500, Luis R. Rodriguez wrote:
> On Wed, Nov 04, 2009 at 02:04:11PM -0800, Luis R. Rodriguez wrote:
> > On Wed, Nov 4, 2009 at 2:00 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> > > On Wed, Nov 04, 2009 at 01:52:30PM -0800, Luis R. Rodriguez wrote:
> > >> > Even better: I just confirmation from our systems team that our legacy
> > >> > devices and 11n PCI devices don't support MWR so I'll remove all that
> > >> > cruft crap.
> > >>
> > >> I meant MWI of course.
> > >
> > > Yes, but they don't necessarily just use cacheline size for MWI ... some
> > > devices use cacheline size for setting up data structures. ?Might be
> > > worth just checking explicitly that they don't use the cacheline size
> > > register for anything.
> > 
> > Oh right -- so the typical Atheros hack for this is to check the cache
> > line size, and if its 0 set it to L1_CACHE_BYTES. Then eventually read
> > from PCI_CACHE_LINE_SIZE pci config to align the skb data. So what I
> > was doing now is removing all this cruft and replacing it with a
> > generic allocator for atheros drivers that aligns simply to the
> > L1_CACHE_BYTES. Sound kosher?
> 
> Something like this:

Doesn't look kosher to me.  You're not programming the CLS register
now at all, which means you're relying on something else having set
it up for you.  If you could EXPORT_SYMBOL(pci_set_cacheline_size)
and include a call to it somewhere, that would be good.  You can rely
on pci_cache_line_size not changing after the system has booted.

>  	/*
> -	 * Cache line size is used to size and align various
> -	 * structures used to communicate with the hardware.
> -	 */
> -	pci_read_config_byte(pdev, PCI_CACHE_LINE_SIZE, &csz);
> -	if (csz == 0) {
> -		/*
> -		 * Linux 2.4.18 (at least) writes the cache line size
> -		 * register as a 16-bit wide register which is wrong.
> -		 * We must have this setup properly for rx buffer
> -		 * DMA to work so force a reasonable value here if it
> -		 * comes up zero.
> -		 */
> -		csz = L1_CACHE_BYTES >> 2;
> -		pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, csz);
> -	}

These comments are what give me pause.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."