ide_timer_expiry() - shouldn't 'wait' be int?

ide_timer_expiry() - shouldn't 'wait' be int?

[Roel Kluin]

vi drivers/ide/ide-io.c +906 and note:

void ide_timer_expiry (unsigned long data)
{
	ide_expiry_t *expiry = hwif->expiry;
	...
	unsigned long   wait = -1;
		...
		if (expiry) {
			...
			wait = expiry(drive);
			if (wait > 0) { /* continue */

also note that in include/linux/ide.h:883:

typedef int (ide_expiry_t)(ide_drive_t *);

doesn't this mean that expiry returns int, and wait therefore should
be int as well?

Roel

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Sergei Shtylyov]

Roel Kluin wrote:

> vi drivers/ide/ide-io.c +906 and note:

> void ide_timer_expiry (unsigned long data)
> {
> 	ide_expiry_t *expiry = hwif->expiry;
> 	...
> 	unsigned long   wait = -1;

    Hm, haven't nothiced that this is *unsigned*.

> 		...
> 		if (expiry) {
> 			...
> 			wait = expiry(drive);
> 			if (wait > 0) { /* continue */

> also note that in include/linux/ide.h:883:

> typedef int (ide_expiry_t)(ide_drive_t *);

> doesn't this mean that expiry returns int, and wait therefore should
> be int as well?

    It rather means that ide_expiry_t() should return unsigned.
    However, you're right as ide_dma_timeout_retry() takes *int* as a 2nd 
argument.

> Roel

MBR, Sergei

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Bartlomiej Zolnierkiewicz]

On Monday 02 March 2009, Sergei Shtylyov wrote:
> Roel Kluin wrote:
> 
> > vi drivers/ide/ide-io.c +906 and note:
> 
> > void ide_timer_expiry (unsigned long data)
> > {
> > 	ide_expiry_t *expiry = hwif->expiry;
> > 	...
> > 	unsigned long   wait = -1;
> 
>     Hm, haven't nothiced that this is *unsigned*.
> 
> > 		...
> > 		if (expiry) {
> > 			...
> > 			wait = expiry(drive);
> > 			if (wait > 0) { /* continue */
> 
> > also note that in include/linux/ide.h:883:
> 
> > typedef int (ide_expiry_t)(ide_drive_t *);
> 
> > doesn't this mean that expiry returns int, and wait therefore should
> > be int as well?
> 
>     It rather means that ide_expiry_t() should return unsigned.

Seconded.  Roel, could you also handle it?

[ However since this is 2.6.30 stuff and there has been much work in
  this area recently please base in top of linux-next or pata-2.6 tree. ]

>     However, you're right as ide_dma_timeout_retry() takes *int* as a 2nd 
> argument.

Though it works fine (by a luck :) we should also fix it while we're at it.

Thanks,
Bart

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Roel Kluin]

Bartlomiej Zolnierkiewicz wrote:
> On Monday 02 March 2009, Sergei Shtylyov wrote:
>> Roel Kluin wrote:
>>
>>> vi drivers/ide/ide-io.c +906 and note:
>>> void ide_timer_expiry (unsigned long data)
>>> {
>>> 	ide_expiry_t *expiry = hwif->expiry;
>>> 	...
>>> 	unsigned long   wait = -1;
>>     Hm, haven't nothiced that this is *unsigned*.
>>
>>> 		...
>>> 		if (expiry) {
>>> 			...
>>> 			wait = expiry(drive);
>>> 			if (wait > 0) { /* continue */
>>> also note that in include/linux/ide.h:883:
>>> typedef int (ide_expiry_t)(ide_drive_t *);
>>> doesn't this mean that expiry returns int, and wait therefore should
>>> be int as well?
>>     It rather means that ide_expiry_t() should return unsigned.
> 
> Seconded.  Roel, could you also handle it?
> 
> [ However since this is 2.6.30 stuff and there has been much work in
>   this area recently please base in top of linux-next or pata-2.6 tree. ]
> 
>>     However, you're right as ide_dma_timeout_retry() takes *int* as a 2nd 
>> argument.
> 
> Though it works fine (by a luck :) we should also fix it while we're at it.
> 
> Thanks,
> Bart

I'm a little confused, do you want wait to be int, as my patch does below,
or do you want the typedef to be:

typedef unsigned long (ide_expiry_t)(ide_drive_t *);

If the latter, I think the functions that expiry points to have to be adapted as
well, right?

this is against tip-latest, feel free to modify the changelog.
------------------------------>8-------------8<---------------------------------
expiry() returns int, negative expiry() return values won't be noticed.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index cc35d6d..0715692 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -1190,7 +1190,7 @@ void ide_timer_expiry (unsigned long data)
 	ide_handler_t	*handler;
 	ide_expiry_t	*expiry;
 	unsigned long	flags;
-	unsigned long	wait = -1;
+	int		wait = -1;
 
 	spin_lock_irqsave(&ide_lock, flags);
 

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Bartlomiej Zolnierkiewicz]

On Monday 02 March 2009, Roel Kluin wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > On Monday 02 March 2009, Sergei Shtylyov wrote:
> >> Roel Kluin wrote:
> >>
> >>> vi drivers/ide/ide-io.c +906 and note:
> >>> void ide_timer_expiry (unsigned long data)
> >>> {
> >>> 	ide_expiry_t *expiry = hwif->expiry;
> >>> 	...
> >>> 	unsigned long   wait = -1;
> >>     Hm, haven't nothiced that this is *unsigned*.
> >>
> >>> 		...
> >>> 		if (expiry) {
> >>> 			...
> >>> 			wait = expiry(drive);
> >>> 			if (wait > 0) { /* continue */
> >>> also note that in include/linux/ide.h:883:
> >>> typedef int (ide_expiry_t)(ide_drive_t *);
> >>> doesn't this mean that expiry returns int, and wait therefore should
> >>> be int as well?
> >>     It rather means that ide_expiry_t() should return unsigned.
> > 
> > Seconded.  Roel, could you also handle it?
> > 
> > [ However since this is 2.6.30 stuff and there has been much work in
> >   this area recently please base in top of linux-next or pata-2.6 tree. ]
> > 
> >>     However, you're right as ide_dma_timeout_retry() takes *int* as a 2nd 
> >> argument.
> > 
> > Though it works fine (by a luck :) we should also fix it while we're at it.
> > 
> > Thanks,
> > Bart
> 
> I'm a little confused, do you want wait to be int, as my patch does below,
> or do you want the typedef to be:
> 
> typedef unsigned long (ide_expiry_t)(ide_drive_t *);

Both. ;)

> If the latter, I think the functions that expiry points to have to be adapted as
> well, right?
> 
> this is against tip-latest, feel free to modify the changelog.

Thanks!  This is exactly what I meant for 2.6.29 (one-line bugfix),
for 2.6.30 we should also do s/int/unsigned long/ cleanup (on top of
the current linux-next/pata-2.6 tree).

> ------------------------------>8-------------8<---------------------------------
> expiry() returns int, negative expiry() return values won't be noticed.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> index cc35d6d..0715692 100644
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -1190,7 +1190,7 @@ void ide_timer_expiry (unsigned long data)
>  	ide_handler_t	*handler;
>  	ide_expiry_t	*expiry;
>  	unsigned long	flags;
> -	unsigned long	wait = -1;
> +	int		wait = -1;
>  
>  	spin_lock_irqsave(&ide_lock, flags);

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>>On Monday 02 March 2009, Sergei Shtylyov wrote:

>>>>Roel Kluin wrote:

>>>>>vi drivers/ide/ide-io.c +906 and note:
>>>>>void ide_timer_expiry (unsigned long data)
>>>>>{
>>>>>	ide_expiry_t *expiry = hwif->expiry;
>>>>>	...
>>>>>	unsigned long   wait = -1;

>>>>    Hm, haven't nothiced that this is *unsigned*.

>>>>>		...
>>>>>		if (expiry) {
>>>>>			...
>>>>>			wait = expiry(drive);
>>>>>			if (wait > 0) { /* continue */
>>>>>also note that in include/linux/ide.h:883:
>>>>>typedef int (ide_expiry_t)(ide_drive_t *);
>>>>>doesn't this mean that expiry returns int, and wait therefore should
>>>>>be int as well?

>>>>    It rather means that ide_expiry_t() should return unsigned.

>>>Seconded.  Roel, could you also handle it?

>>>[ However since this is 2.6.30 stuff and there has been much work in
>>>  this area recently please base in top of linux-next or pata-2.6 tree. ]

>>>>    However, you're right as ide_dma_timeout_retry() takes *int* as a 2nd 
>>>>argument.

>>>Though it works fine (by a luck :) we should also fix it while we're at it.

>>I'm a little confused, do you want wait to be int, as my patch does below,
>>or do you want the typedef to be:
>>
>>typedef unsigned long (ide_expiry_t)(ide_drive_t *);
> 
> 
> Both. ;)

    No, not both really. Sorry for getting everyone confused.

>>If the latter, I think the functions that expiry points to have to be adapted as
>>well, right?

>>this is against tip-latest, feel free to modify the changelog.

> Thanks!  This is exactly what I meant for 2.6.29 (one-line bugfix),
> for 2.6.30 we should also do s/int/unsigned long/ cleanup (on top of
> the current linux-next/pata-2.6 tree).

    Er, what cleanup?

>>------------------------------>8-------------8<---------------------------------
>>expiry() returns int, negative expiry() return values won't be noticed.

    In fact, -1 will still get noticed by its only user, 
ide_dma_timeout_retry() because 'wait' will be cast back to int.

MBR, Sergei

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Bartlomiej Zolnierkiewicz]

On Monday 02 March 2009, Bartlomiej Zolnierkiewicz wrote:
> On Monday 02 March 2009, Roel Kluin wrote:
> > Bartlomiej Zolnierkiewicz wrote:
> > > On Monday 02 March 2009, Sergei Shtylyov wrote:
> > >> Roel Kluin wrote:
> > >>
> > >>> vi drivers/ide/ide-io.c +906 and note:
> > >>> void ide_timer_expiry (unsigned long data)
> > >>> {
> > >>> 	ide_expiry_t *expiry = hwif->expiry;
> > >>> 	...
> > >>> 	unsigned long   wait = -1;
> > >>     Hm, haven't nothiced that this is *unsigned*.
> > >>
> > >>> 		...
> > >>> 		if (expiry) {
> > >>> 			...
> > >>> 			wait = expiry(drive);
> > >>> 			if (wait > 0) { /* continue */
> > >>> also note that in include/linux/ide.h:883:
> > >>> typedef int (ide_expiry_t)(ide_drive_t *);
> > >>> doesn't this mean that expiry returns int, and wait therefore should
> > >>> be int as well?
> > >>     It rather means that ide_expiry_t() should return unsigned.
> > > 
> > > Seconded.  Roel, could you also handle it?
> > > 
> > > [ However since this is 2.6.30 stuff and there has been much work in
> > >   this area recently please base in top of linux-next or pata-2.6 tree. ]
> > > 
> > >>     However, you're right as ide_dma_timeout_retry() takes *int* as a 2nd 
> > >> argument.
> > > 
> > > Though it works fine (by a luck :) we should also fix it while we're at it.
> > > 
> > > Thanks,
> > > Bart
> > 
> > I'm a little confused, do you want wait to be int, as my patch does below,
> > or do you want the typedef to be:
> > 
> > typedef unsigned long (ide_expiry_t)(ide_drive_t *);
> 
> Both. ;)
> 
> > If the latter, I think the functions that expiry points to have to be adapted as
> > well, right?
> > 
> > this is against tip-latest, feel free to modify the changelog.
> 
> Thanks!  This is exactly what I meant for 2.6.29 (one-line bugfix),
> for 2.6.30 we should also do s/int/unsigned long/ cleanup (on top of
> the current linux-next/pata-2.6 tree).
> 
> > ------------------------------>8-------------8<---------------------------------
> > expiry() returns int, negative expiry() return values won't be noticed.
> > 
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > ---
> > diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> > index cc35d6d..0715692 100644
> > --- a/drivers/ide/ide-io.c
> > +++ b/drivers/ide/ide-io.c
> > @@ -1190,7 +1190,7 @@ void ide_timer_expiry (unsigned long data)
> >  	ide_handler_t	*handler;
> >  	ide_expiry_t	*expiry;
> >  	unsigned long	flags;
> > -	unsigned long	wait = -1;
> > +	int		wait = -1;
> >  
> >  	spin_lock_irqsave(&ide_lock, flags);

I failed to notice it before but the patch was against some old kernel
(we don't have ide_lock anymore) so it won't apply to Linus' tree...

Anyway I ended up with the following version:

From: Roel Kluin <roel.kluin@gmail.com>
Subject: ide: expiry() returns int, negative expiry() return values won't be noticed

bart:
It seems like the bug could cause insanely long timeouts for:
- ATA_DMA_ERR error in dma_timer_expiry()
- commands without ->expiry in tc86c001_timer_expiry()
  (TC86C001 IDE controller only)

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
[bart: port it to the current tree]
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-io.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -908,7 +908,7 @@ void ide_timer_expiry (unsigned long dat
 	ide_drive_t	*uninitialized_var(drive);
 	ide_handler_t	*handler;
 	unsigned long	flags;
-	unsigned long	wait = -1;
+	int		wait = -1;
 	int		plug_device = 0;
 
 	spin_lock_irqsave(&hwif->lock, flags);

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>vi drivers/ide/ide-io.c +906 and note:

>>>void ide_timer_expiry (unsigned long data)
>>>{
>>>	ide_expiry_t *expiry = hwif->expiry;
>>>	...
>>>	unsigned long   wait = -1;

>>    Hm, haven't nothiced that this is *unsigned*.

>>>		...
>>>		if (expiry) {
>>>			...
>>>			wait = expiry(drive);
>>>			if (wait > 0) { /* continue */
>>
>>>also note that in include/linux/ide.h:883:

>>>typedef int (ide_expiry_t)(ide_drive_t *);

>>>doesn't this mean that expiry returns int, and wait therefore should
>>>be int as well?

>>    It rather means that ide_expiry_t() should return unsigned.

> Seconded.  Roel, could you also handle it?

    Not worth it, IMO...

> [ However since this is 2.6.30 stuff and there has been much work in
>   this area recently please base in top of linux-next or pata-2.6 tree. ]

>>    However, you're right as ide_dma_timeout_retry() takes *int* as a 2nd 
>>argument.

> Though it works fine (by a luck :) we should also fix it while we're at it.

    Fix what?

> Thanks,
> Bart

MBR, Sergei

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Bartlomiej Zolnierkiewicz]

On Monday 02 March 2009, Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>>vi drivers/ide/ide-io.c +906 and note:
> 
> >>>void ide_timer_expiry (unsigned long data)
> >>>{
> >>>	ide_expiry_t *expiry = hwif->expiry;
> >>>	...
> >>>	unsigned long   wait = -1;
> 
> >>    Hm, haven't nothiced that this is *unsigned*.
> 
> >>>		...
> >>>		if (expiry) {
> >>>			...
> >>>			wait = expiry(drive);
> >>>			if (wait > 0) { /* continue */
> >>
> >>>also note that in include/linux/ide.h:883:
> 
> >>>typedef int (ide_expiry_t)(ide_drive_t *);
> 
> >>>doesn't this mean that expiry returns int, and wait therefore should
> >>>be int as well?
> 
> >>    It rather means that ide_expiry_t() should return unsigned.
> 
> > Seconded.  Roel, could you also handle it?
> 
>     Not worth it, IMO...
> 
> > [ However since this is 2.6.30 stuff and there has been much work in
> >   this area recently please base in top of linux-next or pata-2.6 tree. ]
> 
> >>    However, you're right as ide_dma_timeout_retry() takes *int* as a 2nd 
> >>argument.
> 
> > Though it works fine (by a luck :) we should also fix it while we're at it.
> 
>     Fix what?

ide_dma_timeout_retry's argument -- it is not the error value that we want to
pass but the timeout value.

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Sergei Shtylyov]

Hello, I wrote:

>> vi drivers/ide/ide-io.c +906 and note:

>> void ide_timer_expiry (unsigned long data)
>> {
>>     ide_expiry_t *expiry = hwif->expiry;
>>     ...
>>     unsigned long   wait = -1;

>    Hm, haven't nothiced that this is *unsigned*.

>>         ...
>>         if (expiry) {
>>             ...
>>             wait = expiry(drive);
>>             if (wait > 0) { /* continue */

>> also note that in include/linux/ide.h:883:

>> typedef int (ide_expiry_t)(ide_drive_t *);

>> doesn't this mean that expiry returns int, and wait therefore should
>> be int as well?

>    It rather means that ide_expiry_t() should return unsigned.

    Er, not really, as it can return -1 too.

>> Roel

MBR, Sergei

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Bartlomiej Zolnierkiewicz]

On Monday 02 March 2009, Sergei Shtylyov wrote:
> Hello, I wrote:
> 
> >> vi drivers/ide/ide-io.c +906 and note:
> 
> >> void ide_timer_expiry (unsigned long data)
> >> {
> >>     ide_expiry_t *expiry = hwif->expiry;
> >>     ...
> >>     unsigned long   wait = -1;
> 
> >    Hm, haven't nothiced that this is *unsigned*.
> 
> >>         ...
> >>         if (expiry) {
> >>             ...
> >>             wait = expiry(drive);
> >>             if (wait > 0) { /* continue */
> 
> >> also note that in include/linux/ide.h:883:
> 
> >> typedef int (ide_expiry_t)(ide_drive_t *);
> 
> >> doesn't this mean that expiry returns int, and wait therefore should
> >> be int as well?
> 
> >    It rather means that ide_expiry_t() should return unsigned.
> 
>     Er, not really, as it can return -1 too.

It can just return 0 instead.

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Bartlomiej Zolnierkiewicz]

On Monday 02 March 2009, Bartlomiej Zolnierkiewicz wrote:
> On Monday 02 March 2009, Sergei Shtylyov wrote:
> > Hello, I wrote:
> > 
> > >> vi drivers/ide/ide-io.c +906 and note:
> > 
> > >> void ide_timer_expiry (unsigned long data)
> > >> {
> > >>     ide_expiry_t *expiry = hwif->expiry;
> > >>     ...
> > >>     unsigned long   wait = -1;
> > 
> > >    Hm, haven't nothiced that this is *unsigned*.
> > 
> > >>         ...
> > >>         if (expiry) {
> > >>             ...
> > >>             wait = expiry(drive);
> > >>             if (wait > 0) { /* continue */
> > 
> > >> also note that in include/linux/ide.h:883:
> > 
> > >> typedef int (ide_expiry_t)(ide_drive_t *);
> > 
> > >> doesn't this mean that expiry returns int, and wait therefore should
> > >> be int as well?
> > 
> > >    It rather means that ide_expiry_t() should return unsigned.
> > 
> >     Er, not really, as it can return -1 too.
> 
> It can just return 0 instead.

This time I got confused.  ide_dma_timeout_retry() takes different actions
depending on error == -1 and error == 0.

[ BTW I worked on unifying those cases so ->dma_test_irq call can be moved
  out from ide_dma_timeout_retry() to ide_timer_expiry() (=> it can be later
  merged with ->expiry tests)... ]

Thanks,
Bart

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Bartlomiej Zolnierkiewicz]

On Monday 02 March 2009, Roel Kluin wrote:
> vi drivers/ide/ide-io.c +906 and note:
> 
> void ide_timer_expiry (unsigned long data)
> {
> 	ide_expiry_t *expiry = hwif->expiry;
> 	...
> 	unsigned long   wait = -1;
> 		...
> 		if (expiry) {
> 			...
> 			wait = expiry(drive);
> 			if (wait > 0) { /* continue */
> 
> also note that in include/linux/ide.h:883:
> 
> typedef int (ide_expiry_t)(ide_drive_t *);
> 
> doesn't this mean that expiry returns int, and wait therefore should
> be int as well?

It does... and it seems like it could cause insanely long timeouts for:

* ATA_DMA_ERR error in dma_timer_expiry()

* commands without ->expiry in tc86c001_timer_expiry()
  (TC86C001 IDE controller only)

This is 2.6.29 material, care to make a patch?

Thanks,
Bart

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>vi drivers/ide/ide-io.c +906 and note:

>>void ide_timer_expiry (unsigned long data)
>>{
>>	ide_expiry_t *expiry = hwif->expiry;
>>	...
>>	unsigned long   wait = -1;
>>		...
>>		if (expiry) {
>>			...
>>			wait = expiry(drive);
>>			if (wait > 0) { /* continue */

>>also note that in include/linux/ide.h:883:

>>typedef int (ide_expiry_t)(ide_drive_t *);

>>doesn't this mean that expiry returns int, and wait therefore should
>>be int as well?

> It does... and it seems like it could cause insanely long timeouts for:

> * ATA_DMA_ERR error in dma_timer_expiry()

> * commands without ->expiry in tc86c001_timer_expiry()
>   (TC86C001 IDE controller only)

> This is 2.6.29 material, care to make a patch?

    Er, it's not that bad as it gets cast back to *int* when calling 
ide_dma_timer_expiry().

> Thanks,
> Bart

MBR, Sergei

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Bartlomiej Zolnierkiewicz]

On Monday 02 March 2009, Sergei Shtylyov wrote:
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>vi drivers/ide/ide-io.c +906 and note:
> 
> >>void ide_timer_expiry (unsigned long data)
> >>{
> >>	ide_expiry_t *expiry = hwif->expiry;
> >>	...
> >>	unsigned long   wait = -1;
> >>		...
> >>		if (expiry) {
> >>			...
> >>			wait = expiry(drive);
> >>			if (wait > 0) { /* continue */
> 
> >>also note that in include/linux/ide.h:883:
> 
> >>typedef int (ide_expiry_t)(ide_drive_t *);
> 
> >>doesn't this mean that expiry returns int, and wait therefore should
> >>be int as well?
> 
> > It does... and it seems like it could cause insanely long timeouts for:
> 
> > * ATA_DMA_ERR error in dma_timer_expiry()
> 
> > * commands without ->expiry in tc86c001_timer_expiry()
> >   (TC86C001 IDE controller only)
> 
> > This is 2.6.29 material, care to make a patch?
> 
>     Er, it's not that bad as it gets cast back to *int* when calling 
> ide_dma_timer_expiry().

This case yes, however look at "wait > 0" one:

On -1 returned from ->expiry code sets "rather" long timeouts
(4294967295/HZ on 32-bit...).

Re: ide_timer_expiry() - shouldn't 'wait' be int?

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>>>vi drivers/ide/ide-io.c +906 and note:

>>>>void ide_timer_expiry (unsigned long data)
>>>>{
>>>>	ide_expiry_t *expiry = hwif->expiry;
>>>>	...
>>>>	unsigned long   wait = -1;
>>>>		...
>>>>		if (expiry) {
>>>>			...
>>>>			wait = expiry(drive);
>>>>			if (wait > 0) { /* continue */

>>>>also note that in include/linux/ide.h:883:

>>>>typedef int (ide_expiry_t)(ide_drive_t *);

>>>>doesn't this mean that expiry returns int, and wait therefore should
>>>>be int as well?

>>>It does... and it seems like it could cause insanely long timeouts for:

>>>* ATA_DMA_ERR error in dma_timer_expiry()

>>>* commands without ->expiry in tc86c001_timer_expiry()
>>>  (TC86C001 IDE controller only)

>>>This is 2.6.29 material, care to make a patch?

>>    Er, it's not that bad as it gets cast back to *int* when calling 
>>ide_dma_timer_expiry().

> This case yes, however look at "wait > 0" one:

> On -1 returned from ->expiry code sets "rather" long timeouts
> (4294967295/HZ on 32-bit...).

    Ah... ignore me then. :-<

MBR, Sergei