Proposal for a new PCI function call

Proposal for a new PCI function call

[Steve Modica]

Hi All,

We found recently that the acenic driver for the 3com gigabit ethernet card does
not enable 64 bit DMAs.  (this is done by setting the appropriate mask in
pci_dev->dma_mask).

Jes suggested that the appropriate way to fix this would be to create a function
like pci_enable_dma64 and then have the driver call that, rather than directly
setting this value (a small handful of drivers do this now).

I think the function idea would let us do some sanity checking to make sure
drivers weren't setting this to 64bit on non-64 bit busses and stuff.

Anyhow, so long as no one has any heartburn with this, we'll try to put
something together and submit it.  I'm not subscribed to the list *yet* (need to
get procmail setup), so please leave me in the CC line.

Thanks!
Steve

-- 
Steve Modica
Manager - Networking Drivers Group
"Give a man a fish, and he will eat for a day, hit him with a fish and
he leaves you alone" - me

Re: Proposal for a new PCI function call

[Jeff Garzik]

Steve Modica wrote:
> 
> Hi All,
> 
> We found recently that the acenic driver for the 3com gigabit ethernet card does
> not enable 64 bit DMAs.  (this is done by setting the appropriate mask in
> pci_dev->dma_mask).
> 
> Jes suggested that the appropriate way to fix this would be to create a function
> like pci_enable_dma64 and then have the driver call that, rather than directly
> setting this value (a small handful of drivers do this now).
> 
> I think the function idea would let us do some sanity checking to make sure
> drivers weren't setting this to 64bit on non-64 bit busses and stuff.

pci_set_dma_mask.  Modify that to do the additional checks you need.

Nobody should be setting dma_mask directly anymore, it should be done
through this function.

	Jeff


-- 
Jeff Garzik       | Sam: "Mind if I drive?"
Building 1024     | Max: "Not if you don't mind me clawing at the dash
MandrakeSoft      |       and shrieking like a cheerleader."

Re: Proposal for a new PCI function call

[Jes Sorensen]

>>>>> "Jeff" == Jeff Garzik <jgarzik@mandrakesoft.com> writes:

>> I think the function idea would let us do some sanity checking to
>> make sure drivers weren't setting this to 64bit on non-64 bit
>> busses and stuff.

Jeff> pci_set_dma_mask.  Modify that to do the additional checks you
Jeff> need.

Jeff> Nobody should be setting dma_mask directly anymore, it should be
Jeff> done through this function.

Hmmm, I was wondering if could come up with a pretty way to do this on
32 bit boxes that wants to enable highmem DMA. Right now
pci_set_dma_mask() wants a dma_addr_t which means you have to do
#ifdef CONFIG_HIGHMEM <blah> #else <bleh> #endif.

Introducing a new function that takes bit flags as arguments might be
better?

Jes

Re: Proposal for a new PCI function call

[Alan Cox]

> Hmmm, I was wondering if could come up with a pretty way to do this on
> 32 bit boxes that wants to enable highmem DMA. Right now
> pci_set_dma_mask() wants a dma_addr_t which means you have to do
> #ifdef CONFIG_HIGHMEM <blah> #else <bleh> #endif.
> 
> Introducing a new function that takes bit flags as arguments might be
> better?

pci_set_dma_mask_bits() ? So you could do

pci_set_dma_mask_bits(pdev, 64);

We want everything to go through pci_set_dma_mask... type functions either way
so that we can and the mask with upstream bridges when we hit address range 
limits in some peoples hardware

Alan

Re: Proposal for a new PCI function call

[Jes Sorensen]

>>>>> "Alan" == Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

>> Introducing a new function that takes bit flags as arguments might
>> be better?

Alan> pci_set_dma_mask_bits() ? So you could do

Alan> pci_set_dma_mask_bits(pdev, 64);

Alan> We want everything to go through pci_set_dma_mask... type
Alan> functions either way so that we can and the mask with upstream
Alan> bridges when we hit address range limits in some peoples
Alan> hardware

Looks good to me

Jes

Re: Proposal for a new PCI function call

[Jeff Garzik]

Jes Sorensen wrote:
> >>>>> "Jeff" == Jeff Garzik <jgarzik@mandrakesoft.com> writes:
> >> I think the function idea would let us do some sanity checking to
> >> make sure drivers weren't setting this to 64bit on non-64 bit
> >> busses and stuff.

> Jeff> pci_set_dma_mask.  Modify that to do the additional checks you
> Jeff> need.

> Jeff> Nobody should be setting dma_mask directly anymore, it should be
> Jeff> done through this function.

> Hmmm, I was wondering if could come up with a pretty way to do this on
> 32 bit boxes that wants to enable highmem DMA. Right now
> pci_set_dma_mask() wants a dma_addr_t which means you have to do
> #ifdef CONFIG_HIGHMEM <blah> #else <bleh> #endif.

It seems to me that not doing #ifdef CONFIG_HIGHMEM right now is a
bug...  I think it's the megaraid driver that wants to set dma_addr_t to
a 64-bit mask.


Alan Cox wrote:
> pci_set_dma_mask_bits() ? So you could do
> 
> pci_set_dma_mask_bits(pdev, 64);

As they say, "six of one, 'half-dozen of the other."  I don't have a
preference...

	Jeff


-- 
Jeff Garzik       | Sam: "Mind if I drive?"
Building 1024     | Max: "Not if you don't mind me clawing at the dash
MandrakeSoft      |       and shrieking like a cheerleader."

Re: Proposal for a new PCI function call

[Venkatesh Ramamurthy]

> It seems to me that not doing #ifdef CONFIG_HIGHMEM right now is a
> bug...  I think it's the megaraid driver that wants to set dma_addr_t to
> a 64-bit mask.

MegaRAID driver:
Only if the flag __LP64__ is defined, a 64 bit mask is set , otherwise only
a 32 bit mask is used instead. However check for CONFIG_HIGHMEM needs to be
done.

Re: Proposal for a new PCI function call

[Jes Sorensen]

>>>>> "Jeff" == Jeff Garzik <jgarzik@mandrakesoft.com> writes:

Jeff> Jes Sorensen wrote:
>> Hmmm, I was wondering if could come up with a pretty way to do this
>> on 32 bit boxes that wants to enable highmem DMA. Right now
>> pci_set_dma_mask() wants a dma_addr_t which means you have to do
>> #ifdef CONFIG_HIGHMEM <blah> #else <bleh> #endif.

Jeff> It seems to me that not doing #ifdef CONFIG_HIGHMEM right now is
Jeff> a bug...  I think it's the megaraid driver that wants to set
Jeff> dma_addr_t to a 64-bit mask.

There's a bunch of them doing it - it's butt-ugly though.

Jes