SATA open bugs

SATA open bugs

[Natalie Protasevich]

Hi Tejun, Alan, Jeff,

This is open bug list for SATA subsystem. It has more items than other
subsystems, but mainly because this is such a hot technology now and
so much work is being done in this area. Needless to say It is very
well maintained and exemplary for some other areas that are not that
lucky...
Andrew and myself did some initial cleaning and updates but we really
need your participation and help with the rest.
Some of open bugs are not actually on this list, only the ones that
has been quiet for a while or never been looked at.
Older bugs are probably not really bugs anymore, since SATA has
changed so much but you can probably clean up this list with good
guesses about things being fixed or fixed already, or simply too cold
to keep open.

Thanks,
--Natalie

http://bugzilla.kernel.org/show_bug.cgi?id=7693
http://bugzilla.kernel.org/show_bug.cgi?id=8738 adaptec and sata bad interaction
http://bugzilla.kernel.org/show_bug.cgi?id=8563 - waiting on alan
http://bugzilla.kernel.org/show_bug.cgi?id=8441 - patch review
http://bugzilla.kernel.org/show_bug.cgi?id=8649
http://bugzilla.kernel.org/show_bug.cgi?id=8649
http://bugzilla.kernel.org/show_bug.cgi?id=8424 - patch review
http://bugzilla.kernel.org/show_bug.cgi?id=8410 - patch status from tejun
http://bugzilla.kernel.org/show_bug.cgi?id=8331 - waiting on reporter
http://bugzilla.kernel.org/show_bug.cgi?id=8281
http://bugzilla.kernel.org/show_bug.cgi?id=8223 small fixup, low
priority tejun is out of time
http://bugzilla.kernel.org/show_bug.cgi?id=7997
http://bugzilla.kernel.org/show_bug.cgi?id=7869
http://bugzilla.kernel.org/show_bug.cgi?id=7811 - regression
http://bugzilla.kernel.org/show_bug.cgi?id=7805 tejun is pinging
pktcdvd maintainer (?)
http://bugzilla.kernel.org/show_bug.cgi?id=7599
http://bugzilla.kernel.org/show_bug.cgi?id=7547 tejun is asking " and Jens
http://bugzilla.kernel.org/show_bug.cgi?id=7545 cold
http://bugzilla.kernel.org/show_bug.cgi?id=7507
http://bugzilla.kernel.org/show_bug.cgi?id=7501
http://bugzilla.kernel.org/show_bug.cgi?id=7500
http://bugzilla.kernel.org/show_bug.cgi?id=7489 status?
http://bugzilla.kernel.org/show_bug.cgi?id=7411 status?
http://bugzilla.kernel.org/show_bug.cgi?id=7241 waiting on reporter
http://bugzilla.kernel.org/show_bug.cgi?id=7119 cold fixed?
http://bugzilla.kernel.org/show_bug.cgi?id=7029 reporter is trying to get the hw
http://bugzilla.kernel.org/show_bug.cgi?id=6891 - old sata report
http://bugzilla.kernel.org/show_bug.cgi?id=6890 - old kernel, needs retesting
http://bugzilla.kernel.org/show_bug.cgi?id=6665 - old kernel
http://bugzilla.kernel.org/show_bug.cgi?id=6594 - cold
http://bugzilla.kernel.org/show_bug.cgi?id=6592 - pinged
http://bugzilla.kernel.org/show_bug.cgi?id=6587
http://bugzilla.kernel.org/show_bug.cgi?id=6516 - waiting on reporter
http://bugzilla.kernel.org/show_bug.cgi?id=6253 - cold? waiting on reporter
http://bugzilla.kernel.org/show_bug.cgi?id=6207 - cold?
http://bugzilla.kernel.org/show_bug.cgi?id=5863 - cold
http://bugzilla.kernel.org/show_bug.cgi?id=5798 - cold
http://bugzilla.kernel.org/show_bug.cgi?id=4968 - cold
http://bugzilla.kernel.org/show_bug.cgi?id=4238 pinged
http://bugzilla.kernel.org/show_bug.cgi?id=3993 pinged
http://bugzilla.kernel.org/show_bug.cgi?id=3725 pinged
http://bugzilla.kernel.org/show_bug.cgi?id=3352 cold? waiting on reporter
http://bugzilla.kernel.org/show_bug.cgi?id=2893 pinged

Re: SATA open bugs

[Mark Lord]

Natalie Protasevich wrote:
> Hi Tejun, Alan, Jeff,
> 
> This is open bug list for SATA subsystem. It has more items than other
...
> http://bugzilla.kernel.org/show_bug.cgi?id=7693
> http://bugzilla.kernel.org/show_bug.cgi?id=8738 adaptec and sata bad interaction
> http://bugzilla.kernel.org/show_bug.cgi?id=8563 - waiting on alan
> http://bugzilla.kernel.org/show_bug.cgi?id=8441 - patch review
> http://bugzilla.kernel.org/show_bug.cgi?id=8649
> http://bugzilla.kernel.org/show_bug.cgi?id=8649
> http://bugzilla.kernel.org/show_bug.cgi?id=8424 - patch review
...

Err.. this would be far more useful if the posting also included
one-liner descriptions of the bugs.

Clicking on one link at a time over and over just doesn't encourage
the same level of participation.

Cheers

Re: SATA open bugs

[Tejun Heo]

Hello, Natalie.

Natalie Protasevich wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=7693

This bug isn't related to libata in anyway.  It's "Can't use radeonfb
with dvi port on a Samsung SyncMaster 204B (1600x1200)".

> http://bugzilla.kernel.org/show_bug.cgi?id=8738 adaptec and sata bad interaction

This is SATA support in SAS controller (SCSI) and probably solved by now.

> http://bugzilla.kernel.org/show_bug.cgi?id=8563 - waiting on alan
> http://bugzilla.kernel.org/show_bug.cgi?id=8441 - patch review

This is being worked on.  It requires some redesign in IRQ masking and
polling code.

> http://bugzilla.kernel.org/show_bug.cgi?id=8649

This one is chrp specific.  The native mode fix didn't work.  I'll
follow up later.

> http://bugzilla.kernel.org/show_bug.cgi?id=8424 - patch review

This one is on Alan.

> http://bugzilla.kernel.org/show_bug.cgi?id=8410 - patch status from tejun

This is done.  Closing.

> http://bugzilla.kernel.org/show_bug.cgi?id=8331 - waiting on reporter
> http://bugzilla.kernel.org/show_bug.cgi?id=8281

Will check with SIMG and see whether we need to worry about flash.

> http://bugzilla.kernel.org/show_bug.cgi?id=8223 small fixup, low
> priority tejun is out of time

On my todo list.  I think I'll defer this till we do EH per-queue.

> http://bugzilla.kernel.org/show_bug.cgi?id=7997
> http://bugzilla.kernel.org/show_bug.cgi?id=7869

Closed.

> http://bugzilla.kernel.org/show_bug.cgi?id=7811 - regression

Seems to be yet another IRQ routing problem on via chipset.  I don't
think this has much to do with libata drivers.

> http://bugzilla.kernel.org/show_bug.cgi?id=7805 tejun is pinging
> pktcdvd maintainer (?)

Not a libata bug.  This is caused by pktcdvd holding onto the device
thus preventing it from being revalidated.  Didn't get pong for my ping
yet.  Jens, any ideas?

> http://bugzilla.kernel.org/show_bug.cgi?id=7599

Closed.

> http://bugzilla.kernel.org/show_bug.cgi?id=7547 tejun is asking " and Jens

This one is pktcdvd too.  Jens?

> http://bugzilla.kernel.org/show_bug.cgi?id=7545 cold

Marked as duplicate of bug 8421.

> http://bugzilla.kernel.org/show_bug.cgi?id=7507

Asked for testing on newer kernels.

> http://bugzilla.kernel.org/show_bug.cgi?id=7501

Working on it now.

> http://bugzilla.kernel.org/show_bug.cgi?id=7500

Patch proposed.

> http://bugzilla.kernel.org/show_bug.cgi?id=7489 status?

Closed.

> http://bugzilla.kernel.org/show_bug.cgi?id=7411 status?

Asked Alan to close it.

> http://bugzilla.kernel.org/show_bug.cgi?id=7241 waiting on reporter

Ditto.

> http://bugzilla.kernel.org/show_bug.cgi?id=7119 cold fixed?

Pinged.

> http://bugzilla.kernel.org/show_bug.cgi?id=7029 reporter is trying to get the hw

Pinged.

> http://bugzilla.kernel.org/show_bug.cgi?id=6891 - old sata report

Closed as INSUFFICIENT_DATA.

> http://bugzilla.kernel.org/show_bug.cgi?id=6890 - old kernel, needs retesting

Eeeek.  Yet another via IRQ problem.  Pinged.

> http://bugzilla.kernel.org/show_bug.cgi?id=6665 - old kernel

Marked as duplicate of bug 8421.

> http://bugzilla.kernel.org/show_bug.cgi?id=6594 - cold

Should probably be closed.  It's too old now.  Jeff?

> http://bugzilla.kernel.org/show_bug.cgi?id=6592 - pinged

Closed.

> http://bugzilla.kernel.org/show_bug.cgi?id=6587

Pinged.

> http://bugzilla.kernel.org/show_bug.cgi?id=6516 - waiting on reporter

Definitely not a libata bug.  Pinged.

> http://bugzilla.kernel.org/show_bug.cgi?id=6253 - cold? waiting on reporter

Rejected as INSUFFICIENT_DATA.

> http://bugzilla.kernel.org/show_bug.cgi?id=6207 - cold?

Ditto.

> http://bugzilla.kernel.org/show_bug.cgi?id=5863 - cold

Closed.

> http://bugzilla.kernel.org/show_bug.cgi?id=5798 - cold

Closed.

> http://bugzilla.kernel.org/show_bug.cgi?id=4968 - cold

INSUFFICIENT_DATA

> http://bugzilla.kernel.org/show_bug.cgi?id=4238 pinged

FIXED

> http://bugzilla.kernel.org/show_bug.cgi?id=3993 pinged

I think it's about time to close this one.  It's way too cold.  Jeff?

> http://bugzilla.kernel.org/show_bug.cgi?id=3725 pinged

Closed.

> http://bugzilla.kernel.org/show_bug.cgi?id=3352 cold? waiting on reporter

Way too cold.  Somebody please close this.

> http://bugzilla.kernel.org/show_bug.cgi?id=2893 pinged

Jeff, please close this as INSUFFICIENT_DATA.

Thanks.

-- 
tejun

Re: SATA open bugs

[James Bottomley]

On Wed, 2007-08-08 at 15:31 -0700, Natalie Protasevich wrote:
> Hi Tejun, Alan, Jeff,
> 
> This is open bug list for SATA subsystem. It has more items than other
> subsystems, but mainly because this is such a hot technology now and
> so much work is being done in this area. Needless to say It is very
> well maintained and exemplary for some other areas that are not that
> lucky...
> Andrew and myself did some initial cleaning and updates but we really
> need your participation and help with the rest.
> Some of open bugs are not actually on this list, only the ones that
> has been quiet for a while or never been looked at.
> Older bugs are probably not really bugs anymore, since SATA has
> changed so much but you can probably clean up this list with good
> guesses about things being fixed or fixed already, or simply too cold
> to keep open.
> 
> Thanks,
> --Natalie
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=8738 adaptec and sata bad interaction

This one's not a bug.  the aic94xx didn't attach to sata devices before
post 2.6.22 by design.  The 2.6.23 rc's should work.

James

Re: SATA open bugs

[Jens Axboe]

On Thu, Aug 09 2007, Tejun Heo wrote:
> > http://bugzilla.kernel.org/show_bug.cgi?id=7805 tejun is pinging
> > pktcdvd maintainer (?)
> 
> Not a libata bug.  This is caused by pktcdvd holding onto the device
> thus preventing it from being revalidated.  Didn't get pong for my ping
> yet.  Jens, any ideas?
> 
> > http://bugzilla.kernel.org/show_bug.cgi?id=7547 tejun is asking " and Jens
> 
> This one is pktcdvd too.  Jens?

Ask Peter, I haven't worked on pktcdvd in ~5 years or so :-). CC'ed.

-- 
Jens Axboe

Re: SATA open bugs

[Matt Sealey]

Tejun Heo wrote:
> Hello, Natalie.
> 
>> http://bugzilla.kernel.org/show_bug.cgi?id=8649
> 
> This one is chrp specific.  The native mode fix didn't work.  I'll
> follow up later.

Following up in private with original porter now.

I really think this is a combination of a chip bug and firmware
configuration being futzed. We might have to leave IRQ steering
at 14/15 even in PCI mode, because the ISA bridge isn't so smart
as the docs imply it is.

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[Alan Cox]

> > http://bugzilla.kernel.org/show_bug.cgi?id=8424 - patch review
> This one is on Alan.

I think not - something horrible is happening in dma_alloc_coherent when
called from dmam_* with a non PCI device


Seems to be some kind of AMD64 specific DMA mapping bug ?

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[Andi Kleen]

On Thu, Aug 09, 2007 at 02:53:36PM +0100, Alan Cox wrote:
> > > http://bugzilla.kernel.org/show_bug.cgi?id=8424 - patch review
> > This one is on Alan.
> 
> I think not - something horrible is happening in dma_alloc_coherent when
> called from dmam_* with a non PCI device
> 
> 
> Seems to be some kind of AMD64 specific DMA mapping bug ?

I think it's dev->dma_mask == NULL. Clearly you're passing a non DMA
able device to dma_alloc_coherent(). Which seems like a caller bug.

-Andi

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[Alan Cox]

> > Seems to be some kind of AMD64 specific DMA mapping bug ?
> 
> I think it's dev->dma_mask == NULL. Clearly you're passing a non DMA
> able device to dma_alloc_coherent(). Which seems like a caller bug.

Ok - other archs seem to just return NULL in this case. If its your view
that dma_alloc_coherent(dev) should explode not return NULL then we can
wrap dma_alloc_coherent but I'm not sure the caller library has any
business knowing whether a specific class of device is DMA capable on a
specific platform ?

Alan

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[Andi Kleen]

On Thu, Aug 09, 2007 at 06:21:01PM +0100, Alan Cox wrote:
> > > Seems to be some kind of AMD64 specific DMA mapping bug ?
> > 
> > I think it's dev->dma_mask == NULL. Clearly you're passing a non DMA
> > able device to dma_alloc_coherent(). Which seems like a caller bug.
> 
> Ok - other archs seem to just return NULL in this case. If its your view

Ok then the initialization would fail.

I could do that, but clearly there must be some other bug
in the process that tries to do DMA on such random devices
in the first place.

> that dma_alloc_coherent(dev) should explode not return NULL then we can
> wrap dma_alloc_coherent but I'm not sure the caller library has any
> business knowing whether a specific class of device is DMA capable on a
> specific platform ?

Someone must ask that caller library to DMA to/from that device
in the first place. Whoever it is it is wrong. 

Or perhaps you got the wrong device here? For ISA devices we 
traditionally used NULL. Or if you set up your own ISA devices
(which I can't see a reason for but there might be one I'm missing) 
at least give them a dma mask. Then it should probably work on x86-64.

-Andi

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[Alan Cox]

> Someone must ask that caller library to DMA to/from that device
> in the first place. Whoever it is it is wrong. 

No I disagree.

> Or perhaps you got the wrong device here? For ISA devices we 
> traditionally used NULL. Or if you set up your own ISA devices
> (which I can't see a reason for but there might be one I'm missing) 
> at least give them a dma mask. Then it should probably work on x86-64.

The libata code currently (and this seems to work for all but x86-64)
does the following if it is setting up a potentially DMA capable device

	- Allocate a dma_coherent buffer
	- If it is refused then turn off DMA and use PIO

It has no idea whether a pcmcia, isa, platform or even PCI device happens
to be DMA capable, and there are platforms with PCI but very limited DMA
for example in the embedded space. In fact it has no idea this level
whether it is working a PCI, ISA, PCMCIA, SBUS or some other bus device.
It's supposed to be generic code.

Obviously doing anything other than dma_alloc_coherent if the allocation
fails is stupid but trying to allocate dma memory to find out if the
device can be used with DMA on a given platform is quite logical and
sensible in some cases.

Thus I think dma_alloc_coherent() for an ISA, PCMCIA or other class
(platform particularly) shouldn't explode on AMD64 but simply return
NULL. Its a sane request to make when you don't in your library know what
dev is.

Alan

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[Andi Kleen]

On Thu, Aug 09, 2007 at 06:53:10PM +0100, Alan Cox wrote:
> > Or perhaps you got the wrong device here? For ISA devices we 
> > traditionally used NULL. Or if you set up your own ISA devices
> > (which I can't see a reason for but there might be one I'm missing) 
> > at least give them a dma mask. Then it should probably work on x86-64.
> 
> The libata code currently (and this seems to work for all but x86-64)
> does the following if it is setting up a potentially DMA capable device

Where does the device come from? What device is it?
Is that known already?

> Thus I think dma_alloc_coherent() for an ISA, PCMCIA or other class
> (platform particularly) shouldn't explode on AMD64 but simply return
> NULL. Its a sane request to make when you don't in your library know what
> dev is.

But at least on x86-64 the device is likely DMA capable. At least
PCMCIA usually is.  Near all devices are DMA capable, except perhaps
SMBus and some real oddballs. So the device should not have a NULL dma_mask
in the first place. Or it is the wrong device. e.g. we have platform
devices for things like timers etc. which are really not DMA capable
because it doesn't make much sense to DMA your timer; but there
should be really no reason to use such a device in a libata driver. 

If it's some kind of PCMCIA device like implied by pata_pcmcia
then it's likely DMA capable.

So if I return NULL then either a really DMA capable device will
not work. Or you got a real oddball device that you shouldn't have gotten
in the first place. 

Perhaps the PCMCIA code sets up some of its devices wrong? 

-Andi

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[James Bottomley]

On Thu, 2007-08-09 at 18:53 +0100, Alan Cox wrote:
> > Someone must ask that caller library to DMA to/from that device
> > in the first place. Whoever it is it is wrong. 
> 
> No I disagree.

I'm with Andi here ... you're fortunate that parisc has no working
IDE/SATA interface (or rather we have so few running boxes with PCMCIA),
but if it did this call sequence would explode on that platform for a
different reason.

> > Or perhaps you got the wrong device here? For ISA devices we 
> > traditionally used NULL. Or if you set up your own ISA devices
> > (which I can't see a reason for but there might be one I'm missing) 
> > at least give them a dma mask. Then it should probably work on x86-64.
> 
> The libata code currently (and this seems to work for all but x86-64)
> does the following if it is setting up a potentially DMA capable device
> 
> 	- Allocate a dma_coherent buffer

You cannot allocate a dma_coherent buffer without passing in the correct
underlying device ... on parisc we'd explode trying to find the iommu to
map through (some of our hw has more than one).

> 	- If it is refused then turn off DMA and use PIO
> 
> It has no idea whether a pcmcia, isa, platform or even PCI device happens
> to be DMA capable, and there are platforms with PCI but very limited DMA
> for example in the embedded space. In fact it has no idea this level
> whether it is working a PCI, ISA, PCMCIA, SBUS or some other bus device.
> It's supposed to be generic code.
> 
> Obviously doing anything other than dma_alloc_coherent if the allocation
> fails is stupid but trying to allocate dma memory to find out if the
> device can be used with DMA on a given platform is quite logical and
> sensible in some cases.
> 
> Thus I think dma_alloc_coherent() for an ISA, PCMCIA or other class
> (platform particularly) shouldn't explode on AMD64 but simply return
> NULL. Its a sane request to make when you don't in your library know what
> dev is.

PCMCIA should be correctly plumbed into the generic model.  It's really
just a bridge ... how that bridge is wired critically affects how
coherent memory is allocated.

James

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[Alan Cox]

> Where does the device come from? What device is it?

At the higher level someone passed us a device and some mappings and
function methods and said "this is an IDE controller"

> Is that known already?

The core code has no idea or interest in where it came from.

> But at least on x86-64 the device is likely DMA capable. At least
> PCMCIA usually is.  Near all devices are DMA capable, except perhaps

PCMCIA usally isn't.

> So if I return NULL then either a really DMA capable device will
> not work. Or you got a real oddball device that you shouldn't have gotten
> in the first place. 

What about other platforms ? I don't want libata to have

#ifdef CONFIG_X86_64
	/* this platform is different)
	if (hack-around-in-dev-struct())
		return NULL;
#endif


All I want is

	This is a device, the caller has said it may be DMA capable
	dma_alloc_coherent
		NULL. ok in this situation it isn't

I don't want to know about arch internals. I don't want to know about
platform specific DMA rules. I just want to be able to ask for DMA and
get told yes or no. Oops is not a useful error return.

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[Alan Cox]

> You cannot allocate a dma_coherent buffer without passing in the correct
> underlying device ... on parisc we'd explode trying to find the iommu to
> map through (some of our hw has more than one).

We do pass the correct underlying device. Then dma_alloc_coherent oopses
on x86_64 if you pass some valid devices (eg pcmcia). 

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[Andi Kleen]

On Thu, Aug 09, 2007 at 11:34:58PM +0100, Alan Cox wrote:
> > Where does the device come from? What device is it?
> 
> At the higher level someone passed us a device and some mappings and
> function methods and said "this is an IDE controller"

Ok you're stabbing in the dark. I guess more debugging is needed.

> > But at least on x86-64 the device is likely DMA capable. At least
> > PCMCIA usually is.  Near all devices are DMA capable, except perhaps
> 
> PCMCIA usally isn't.

Hmm?

> I don't want to know about arch internals. I don't want to know about
> platform specific DMA rules. I just want to be able to ask for DMA and
> get told yes or no. Oops is not a useful error return.

I'm not blaming libata here, just whoever gave that bogus device to it.

-Andi

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[James Bottomley]

On Thu, 2007-08-09 at 23:37 +0100, Alan Cox wrote:
> > You cannot allocate a dma_coherent buffer without passing in the correct
> > underlying device ... on parisc we'd explode trying to find the iommu to
> > map through (some of our hw has more than one).
> 
> We do pass the correct underlying device. Then dma_alloc_coherent oopses
> on x86_64 if you pass some valid devices (eg pcmcia). 

If the device you're passing has a NULL dma_mask pointer that means the
platform hasn't set it up correctly for dma ... and that's the
underlying problem (although it's not necessarily a libata problem): you
can't call dma_ operations on a device that hasn't been set up in the
platform for it.

James

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[Andi Kleen]

I'll submit a patch to check this in my next batch.

But as James pointed out you'll likely need similar patches on other
architectures.

-Andi

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs) II

[Andi Kleen]

BTW unless I'm misreading the i386 code it'll not fail here, but
allocate memory. Surely that will cause failures later if you
rely on it failing? If you don't rely on it then changing x86-64
will also not help you.

-Andi

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[Alan Cox]

> > At the higher level someone passed us a device and some mappings and
> > function methods and said "this is an IDE controller"
> 
> Ok you're stabbing in the dark. I guess more debugging is needed.

Huh ?

> > > But at least on x86-64 the device is likely DMA capable. At least
> > > PCMCIA usually is.  Near all devices are DMA capable, except perhaps
> > 
> > PCMCIA usally isn't.
> 
> Hmm?

PCMCIA is not DMA capable, and indeed passing a pcmcia device to AMD64
boxes dma_alloc_ functions causes an Oops.

> I'm not blaming libata here, just whoever gave that bogus device to it.

For the simple example the PCMCIA layer.

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[Alan Cox]

> If the device you're passing has a NULL dma_mask pointer that means the
> platform hasn't set it up correctly for dma ... and that's the

Well it may not do DMA. 

> underlying problem (although it's not necessarily a libata problem): you
> can't call dma_ operations on a device that hasn't been set up in the
> platform for it.

Right.

Ok so what is the right way given a random *correctly* set up device that
may or may not be DMA capable to determine this. Its an arbitary device
pointer for an arbitary device class on an arbitary platform which may or
may not support DMA for this device class ?

Can I called dma_alloc_coherent and get a NULL back or do we need a
device_can_dma() check in each platform ?

Alan

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs) II

[Alan Cox]

> BTW unless I'm misreading the i386 code it'll not fail here, but
> allocate memory. Surely that will cause failures later if you
> rely on it failing? If you don't rely on it then changing x86-64
> will also not help you.

Eww that'll do strange things.

Ok so we do in fact need some kind of proper way to ask if a device is
DMA capable ?

Alan

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs)

[James Bottomley]

On Fri, 2007-08-10 at 00:17 +0100, Alan Cox wrote:
> > If the device you're passing has a NULL dma_mask pointer that means the
> > platform hasn't set it up correctly for dma ... and that's the
> 
> Well it may not do DMA. 

Unfortunately, I don't think we've considered that possibility in the
integration of the dma API and generic devices.

> > underlying problem (although it's not necessarily a libata problem): you
> > can't call dma_ operations on a device that hasn't been set up in the
> > platform for it.
> 
> Right.
> 
> Ok so what is the right way given a random *correctly* set up device that
> may or may not be DMA capable to determine this. Its an arbitary device
> pointer for an arbitary device class on an arbitary platform which may or
> may not support DMA for this device class ?
> 
> Can I called dma_alloc_coherent and get a NULL back or do we need a
> device_can_dma() check in each platform ?

Fundamentally it's a different question.  The dma_alloc is just saying
can you set up a coherent memory area that this device can reach.  Often
the platform cannot necessarily discover this.  I'd say the best signal
is that the driver has called dma_set_mask(dev, DMA_MASK_NONE) (which
isn't defined yet). So the platform has a signal that the device isn't
DMA capable.

James

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs) II

[Andi Kleen]

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

> > BTW unless I'm misreading the i386 code it'll not fail here, but
> > allocate memory. Surely that will cause failures later if you
> > rely on it failing? If you don't rely on it then changing x86-64
> > will also not help you.
> 
> Eww that'll do strange things.
> 

In theory I could change i386 too to return NULL in this case (dma_mask == NULL).
But I wonder how how many existing PCMCIA drivers I would break this way? 

Probably needs more testing at least.

My current patch is in 
ftp://ftp.firstfloor.org/pub/ak/x86_64/late-merge/patches/dma-alloc-mask

> Ok so we do in fact need some kind of proper way to ask if a device is
> DMA capable ?

Right now it seems to be (dev->dma_mask != NULL) 

-Andi

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs) II

[Alan Cox]

> > Ok so we do in fact need some kind of proper way to ask if a device is
> > DMA capable ?
> 
> Right now it seems to be (dev->dma_mask != NULL) 

I'll follow that path for now then

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs) II

[James Bottomley]

On Fri, 2007-08-10 at 14:54 +0100, Alan Cox wrote:
> > > Ok so we do in fact need some kind of proper way to ask if a device is
> > > DMA capable ?
> > 
> > Right now it seems to be (dev->dma_mask != NULL) 
> 
> I'll follow that path for now then

Not in non platform code, please ... somewhere on the Janitor's list is
moving the dma_mask from the bus specific devices into the generic
device ... when that happens this quantity will become u64 and they
won't know what to do with the NULL check.

Even today, it's wrong for this to be NULL.  It's a bug somewhere in the
pcmcia layer.  What should happen is that there should be a u64 dma_mask
somewhere in struct pcmcia_device where this should be pointing.

James

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs) II

[Alan Cox]

> Not in non platform code, please ... somewhere on the Janitor's list is
> moving the dma_mask from the bus specific devices into the generic
> device ... when that happens this quantity will become u64 and they
> won't know what to do with the NULL check.

Ok filed for "kernel summit" 

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs) II

[James Bottomley]

On Fri, 2007-08-10 at 17:58 +0100, Alan Cox wrote:
> > Not in non platform code, please ... somewhere on the Janitor's list is
> > moving the dma_mask from the bus specific devices into the generic
> > device ... when that happens this quantity will become u64 and they
> > won't know what to do with the NULL check.
> 
> Ok filed for "kernel summit" 

Surely we don't need to wait until then?  This is the correct fix, isn't
it?  (Obviously I'll split it into a generic and a pcmcia specific piece
if it looks OK to everyone).

It sets the PCMCIA dma_mask up correctly and introduces a DMA_MASK_NONE
(I prefer that to DMA_0BIT_MASK but I can add that too if people want)
and gives Alan his is_device_dma_capable() API.

James

diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c
index a996071..b3837d3 100644
--- a/drivers/pcmcia/ds.c
+++ b/drivers/pcmcia/ds.c
@@ -23,6 +23,7 @@
 #include <linux/crc32.h>
 #include <linux/firmware.h>
 #include <linux/kref.h>
+#include <linux/dma-mapping.h>
 
 #define IN_CARD_SERVICES
 #include <pcmcia/cs_types.h>
@@ -670,6 +671,9 @@ struct pcmcia_device * pcmcia_device_add(struct pcmcia_socket *s, unsigned int f
 	p_dev->dev.bus = &pcmcia_bus_type;
 	p_dev->dev.parent = s->dev.parent;
 	p_dev->dev.release = pcmcia_release_dev;
+	/* by default don't allow DMA */
+	p_dev->dma_mask = DMA_MASK_NONE;
+	p_dev->dev.dma_mask = &p_dev->dma_mask;
 	bus_id_len = sprintf (p_dev->dev.bus_id, "%d.%d", p_dev->socket->sock, p_dev->device_no);
 
 	p_dev->devname = kmalloc(6 + bus_id_len + 1, GFP_KERNEL);
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 2dc21cb..0ebfafb 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -24,6 +24,8 @@ enum dma_data_direction {
 #define DMA_28BIT_MASK	0x000000000fffffffULL
 #define DMA_24BIT_MASK	0x0000000000ffffffULL
 
+#define DMA_MASK_NONE	0x0ULL
+
 static inline int valid_dma_direction(int dma_direction)
 {
 	return ((dma_direction == DMA_BIDIRECTIONAL) ||
@@ -31,6 +33,11 @@ static inline int valid_dma_direction(int dma_direction)
 		(dma_direction == DMA_FROM_DEVICE));
 }
 
+static inline int is_device_dma_capable(struct device *dev)
+{
+	return dev->dma_mask != NULL && *dev->dma_mask != DMA_MASK_NONE;
+}
+
 #ifdef CONFIG_HAS_DMA
 #include <asm/dma-mapping.h>
 #else
diff --git a/include/pcmcia/ds.h b/include/pcmcia/ds.h
index 90ef552..f047a1f 100644
--- a/include/pcmcia/ds.h
+++ b/include/pcmcia/ds.h
@@ -184,6 +184,7 @@ struct pcmcia_device {
 
 	char *			prod_id[4];
 
+	u64			dma_mask;
 	struct device		dev;
 
 #ifdef CONFIG_PCMCIA_IOCTL

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs) II

[Andi Kleen]

> Surely we don't need to wait until then?  This is the correct fix, isn't
> it?  (Obviously I'll split it into a generic and a pcmcia specific piece
> if it looks OK to everyone).
> 
> It sets the PCMCIA dma_mask up correctly and introduces a DMA_MASK_NONE
> (I prefer that to DMA_0BIT_MASK but I can add that too if people want)
> and gives Alan his is_device_dma_capable() API.

Patch looks good to me.

-Andi

Re: AMD64 dma_alloc_coherent crashes on non PCI device (was SATA open bugs) II

[James Bottomley]

On Fri, 2007-08-10 at 22:27 +0200, Andi Kleen wrote:
> > Surely we don't need to wait until then?  This is the correct fix, isn't
> > it?  (Obviously I'll split it into a generic and a pcmcia specific piece
> > if it looks OK to everyone).
> > 
> > It sets the PCMCIA dma_mask up correctly and introduces a DMA_MASK_NONE
> > (I prefer that to DMA_0BIT_MASK but I can add that too if people want)
> > and gives Alan his is_device_dma_capable() API.
> 
> Patch looks good to me.

No one else has commented ... shall I just submit the following two for
the merge window?

James

[PATCH 1/1] introduce DMA_MASK_NONE as a signal for unable to do DMA

[James Bottomley]

Some devices are incapable of DMA and need to be recognised as such.
Introduce a NONE dma mask to facilitate this plus an inline function:
is_device_dma_capable() to check this.

Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 2dc21cb..0ebfafb 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -24,6 +24,8 @@ enum dma_data_direction {
 #define DMA_28BIT_MASK	0x000000000fffffffULL
 #define DMA_24BIT_MASK	0x0000000000ffffffULL
 
+#define DMA_MASK_NONE	0x0ULL
+
 static inline int valid_dma_direction(int dma_direction)
 {
 	return ((dma_direction == DMA_BIDIRECTIONAL) ||
@@ -31,6 +33,11 @@ static inline int valid_dma_direction(int dma_direction)
 		(dma_direction == DMA_FROM_DEVICE));
 }
 
+static inline int is_device_dma_capable(struct device *dev)
+{
+	return dev->dma_mask != NULL && *dev->dma_mask != DMA_MASK_NONE;
+}
+
 #ifdef CONFIG_HAS_DMA
 #include <asm/dma-mapping.h>
 #else
diff --git a/include/pcmcia/ds.h b/include/pcmcia/ds.h
index 90ef552..f047a1f 100644

[Patch 2/2] pcmcia: use DMA_MASK_NONE for the default for all pcmcia devices

[James Bottomley]

Most non cardbus devices can't do dma, so flag them as such in the
device creation routine.

Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>

diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c
index a996071..b3837d3 100644
--- a/drivers/pcmcia/ds.c
+++ b/drivers/pcmcia/ds.c
@@ -23,6 +23,7 @@
 #include <linux/crc32.h>
 #include <linux/firmware.h>
 #include <linux/kref.h>
+#include <linux/dma-mapping.h>
 
 #define IN_CARD_SERVICES
 #include <pcmcia/cs_types.h>
@@ -670,6 +671,9 @@ struct pcmcia_device * pcmcia_device_add(struct pcmcia_socket *s, unsigned int f
 	p_dev->dev.bus = &pcmcia_bus_type;
 	p_dev->dev.parent = s->dev.parent;
 	p_dev->dev.release = pcmcia_release_dev;
+	/* by default don't allow DMA */
+	p_dev->dma_mask = DMA_MASK_NONE;
+	p_dev->dev.dma_mask = &p_dev->dma_mask;
 	bus_id_len = sprintf (p_dev->dev.bus_id, "%d.%d", p_dev->socket->sock, p_dev->device_no);
 
 	p_dev->devname = kmalloc(6 + bus_id_len + 1, GFP_KERNEL);
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
diff --git a/include/pcmcia/ds.h b/include/pcmcia/ds.h
index 90ef552..f047a1f 100644
--- a/include/pcmcia/ds.h
+++ b/include/pcmcia/ds.h
@@ -184,6 +184,7 @@ struct pcmcia_device {
 
 	char *			prod_id[4];
 
+	u64			dma_mask;
 	struct device		dev;
 
 #ifdef CONFIG_PCMCIA_IOCTL