[PATCH 1/1] pata_it821x: fix lost interrupt with atapi devices

[PATCH 1/1] pata_it821x: fix lost interrupt with atapi devices

[Jeff Norden]

 From: Jeff Norden <jnorden@math.tntech.edu>

Fix "lost" interrupt problem when using dma with CD/DVD drives in some
configurations.  This problem can make installing linux from media
impossible for distro's that have switched to libata-only configurations.

The simple fix is to eliminate the use of dma for reading drive status, etc,
by checking the number of bytes to transferred.

This change will only affect the behavior of atapi devices, not disks.
There is more info at http://bugzilla.redhat.com/show_bug.cgi?id=242229
This patch is for 2.6.22.1

Signed-off-by: Jeff Norden <jnorden@math.tntech.edu>

---

--- pata_it821x.c.orig	2007-08-16 14:20:49.000000000 -0500
+++ pata_it821x.c	2007-08-31 16:09:22.000000000 -0500
@@ -533,6 +533,10 @@ static int it821x_check_atapi_dma(struct
 	struct ata_port *ap = qc->ap;
 	struct it821x_dev *itdev = ap->private_data;
 
+	/* Only use dma for transfers to/from the media. */
+	if (qc->nbytes < 2048)
+		return -EOPNOTSUPP;
+
 	/* No ATAPI DMA in smart mode */
 	if (itdev->smart)
 		return -EOPNOTSUPP;

Re: [PATCH 1/1] pata_it821x: fix lost interrupt with atapi devices

[Mikael Pettersson]

Jeff Norden writes:
 >  From: Jeff Norden <jnorden@math.tntech.edu>
 > 
 > Fix "lost" interrupt problem when using dma with CD/DVD drives in some
 > configurations.  This problem can make installing linux from media
 > impossible for distro's that have switched to libata-only configurations.
 > 
 > The simple fix is to eliminate the use of dma for reading drive status, etc,
 > by checking the number of bytes to transferred.
 > 
 > This change will only affect the behavior of atapi devices, not disks.
 > There is more info at http://bugzilla.redhat.com/show_bug.cgi?id=242229
 > This patch is for 2.6.22.1
 > 
 > Signed-off-by: Jeff Norden <jnorden@math.tntech.edu>
 > 
 > ---
 > 
 > --- pata_it821x.c.orig	2007-08-16 14:20:49.000000000 -0500
 > +++ pata_it821x.c	2007-08-31 16:09:22.000000000 -0500
 > @@ -533,6 +533,10 @@ static int it821x_check_atapi_dma(struct
 >  	struct ata_port *ap = qc->ap;
 >  	struct it821x_dev *itdev = ap->private_data;
 >  
 > +	/* Only use dma for transfers to/from the media. */
 > +	if (qc->nbytes < 2048)
 > +		return -EOPNOTSUPP;
 > +
 >  	/* No ATAPI DMA in smart mode */
 >  	if (itdev->smart)
 >  		return -EOPNOTSUPP;
 > 

This looks like a gross hack. Aren't you supposed to inspect
the command instead and whitelist the ones you know are OK,
like pata_pdc2027x.c and sata_promise.c do?

Re: [PATCH 1/1] pata_it821x: fix lost interrupt with atapi devices

[Alan Cox]

/from the media. */
>  > +	if (qc->nbytes < 2048)
>  > +		return -EOPNOTSUPP;
>  > +
>  >  	/* No ATAPI DMA in smart mode */
>  >  	if (itdev->smart)
>  >  		return -EOPNOTSUPP;
>  > 
> 
> This looks like a gross hack. Aren't you supposed to inspect
> the command instead and whitelist the ones you know are OK,
> like pata_pdc2027x.c and sata_promise.c do?

It does seem to be about transfer size in the IT821x case not commands.
It may be to do with how we issue ATAPI command transfer sizes from high
up (patch went to Jeff) but for now this is definitely the right approach

Reviewed-by: Alan Cox <alan@redhat.com>

Re: [PATCH 1/1] pata_it821x: fix lost interrupt with atapi devices

[Tejun Heo]

[cc'ing Albert and linux-ide]

Alan Cox wrote:
> /from the media. */
>>  > +	if (qc->nbytes < 2048)
>>  > +		return -EOPNOTSUPP;
>>  > +
>>  >  	/* No ATAPI DMA in smart mode */
>>  >  	if (itdev->smart)
>>  >  		return -EOPNOTSUPP;
>>  > 
>>
>> This looks like a gross hack. Aren't you supposed to inspect
>> the command instead and whitelist the ones you know are OK,
>> like pata_pdc2027x.c and sata_promise.c do?
> 
> It does seem to be about transfer size in the IT821x case not commands.
> It may be to do with how we issue ATAPI command transfer sizes from high
> up (patch went to Jeff) but for now this is definitely the right approach
> 
> Reviewed-by: Alan Cox <alan@redhat.com>

I wonder whether we should be using similar check in generic path too.
We have quite a few cases where MWDMA ATAPI devices choking on commands
with small transfer sizes.  I don't think we'll experience significant
performance regression with this applied and even if there is some, it's
far better to have slightly slower working device.

What do you guys think?

-- 
tejun

Re: [PATCH 1/1] pata_it821x: fix lost interrupt with atapi devices

[Jeff Garzik]

Tejun Heo wrote:
> [cc'ing Albert and linux-ide]
> 
> Alan Cox wrote:
>> /from the media. */
>>>  > +	if (qc->nbytes < 2048)
>>>  > +		return -EOPNOTSUPP;
>>>  > +
>>>  >  	/* No ATAPI DMA in smart mode */
>>>  >  	if (itdev->smart)
>>>  >  		return -EOPNOTSUPP;
>>>  > 
>>>
>>> This looks like a gross hack. Aren't you supposed to inspect
>>> the command instead and whitelist the ones you know are OK,
>>> like pata_pdc2027x.c and sata_promise.c do?
>> It does seem to be about transfer size in the IT821x case not commands.
>> It may be to do with how we issue ATAPI command transfer sizes from high
>> up (patch went to Jeff) but for now this is definitely the right approach
>>
>> Reviewed-by: Alan Cox <alan@redhat.com>
> 
> I wonder whether we should be using similar check in generic path too.
> We have quite a few cases where MWDMA ATAPI devices choking on commands
> with small transfer sizes.  I don't think we'll experience significant
> performance regression with this applied and even if there is some, it's
> far better to have slightly slower working device.
> 
> What do you guys think?

Need to look at, or know, a standard profile of submitted commands. 
It's quite possible some high performance commands want this, where 
possible.

	Jeff

Re: [PATCH 1/1] pata_it821x: fix lost interrupt with atapi devices

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> [cc'ing Albert and linux-ide]
>>
>> Alan Cox wrote:
>>> /from the media. */
>>>>  > +    if (qc->nbytes < 2048)
>>>>  > +        return -EOPNOTSUPP;
>>>>  > +
>>>>  >      /* No ATAPI DMA in smart mode */
>>>>  >      if (itdev->smart)
>>>>  >          return -EOPNOTSUPP;
>>>>  >
>>>> This looks like a gross hack. Aren't you supposed to inspect
>>>> the command instead and whitelist the ones you know are OK,
>>>> like pata_pdc2027x.c and sata_promise.c do?
>>> It does seem to be about transfer size in the IT821x case not commands.
>>> It may be to do with how we issue ATAPI command transfer sizes from high
>>> up (patch went to Jeff) but for now this is definitely the right
>>> approach
>>>
>>> Reviewed-by: Alan Cox <alan@redhat.com>
>>
>> I wonder whether we should be using similar check in generic path too.
>> We have quite a few cases where MWDMA ATAPI devices choking on commands
>> with small transfer sizes.  I don't think we'll experience significant
>> performance regression with this applied and even if there is some, it's
>> far better to have slightly slower working device.
>>
>> What do you guys think?
> 
> Need to look at, or know, a standard profile of submitted commands. It's
> quite possible some high performance commands want this, where possible.

Yeah, agreed.  I'm quite confused what's going on with all these MWDMA
failures and would really like to know what's broken.  The reason can be
one of the following two.

1. libata itself isn't doing anything wrong but using SCSI for mid and
high level drivers are causing problems as more types of commands are used.

2. Mode programming for MWDMA is broken.  This doesn't seem too likely
as we're seeing MWDMA failures across different low level drivers but is
still a possibility.

However, it's now like drivers/ide drivers didn't have MWDMA problems.
Some drives show similar behavior on IDE too.  It's less noticeable
because IDE is much quicker to downstep to PIO mode.  libata EH logic
definitely can use some improvements here.

I think the correct thing to do regarding command DMA filtering is to
follow what the other OS does.  Apparently, that's all ATAPI device
vendors test against.

Thanks.

-- 
tejun

Re: [PATCH 1/1] pata_it821x: fix lost interrupt with atapi devices

[Alan Cox]

On Mon, Sep 17, 2007 at 01:46:01PM +0200, Tejun Heo wrote:
> failures and would really like to know what's broken.  The reason can be
> one of the following two.
> 
> 1. libata itself isn't doing anything wrong but using SCSI for mid and
> high level drivers are causing problems as more types of commands are used.
> 
> 2. Mode programming for MWDMA is broken.  This doesn't seem too likely
> as we're seeing MWDMA failures across different low level drivers but is
> still a possibility.

3 and 4

3. Libata is wrong (it is in places)

4. Old IDE uses DMA much less for ATAPI especially

Re: [PATCH 1/1] pata_it821x: fix lost interrupt with atapi devices

[Alan Cox]

On Mon, Sep 17, 2007 at 07:00:03AM +0200, Tejun Heo wrote:
> I wonder whether we should be using similar check in generic path too.
> We have quite a few cases where MWDMA ATAPI devices choking on commands
> with small transfer sizes.  I don't think we'll experience significant
> performance regression with this applied and even if there is some, it's
> far better to have slightly slower working device.
> 
> What do you guys think?

I think to start with someone should fix the ATAPI state machine. Stick
a BUG() in the driver if dma start is called when the atapi command data 
size is zero

Alan

Re: [PATCH 1/1] pata_it821x: fix lost interrupt with atapi devices

[Jeff Garzik]

Jeff Norden wrote:
>  From: Jeff Norden <jnorden@math.tntech.edu>
> 
> Fix "lost" interrupt problem when using dma with CD/DVD drives in some
> configurations.  This problem can make installing linux from media
> impossible for distro's that have switched to libata-only configurations.
> 
> The simple fix is to eliminate the use of dma for reading drive status, etc,
> by checking the number of bytes to transferred.
> 
> This change will only affect the behavior of atapi devices, not disks.
> There is more info at http://bugzilla.redhat.com/show_bug.cgi?id=242229
> This patch is for 2.6.22.1
> 
> Signed-off-by: Jeff Norden <jnorden@math.tntech.edu>

applied