Skip to contentSkip to conversation

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

DISCUSSION Roel Kluin 3 participants 16 messages Mar 2, 2009 – Mar 5, 2009 libata

lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

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

#1 lore raw
Quoted 7 lines · 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;

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

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

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.

Quoted 1 line

Roel

MBR, Sergei

#12 lore raw
In reply to Roel Kluin
Quoted 13 lines · 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 */
Quoted 6 lines

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

#2 lore raw
In reply to Sergei Shtylyov
Quoted 18 lines · Sergei Shtylyov
Quoted 7 lines · 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;

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

Quoted 5 lines
		...
		if (expiry) {
			...
			wait = expiry(drive);
			if (wait > 0) { /* continue */
Quoted 8 lines
Quoted 6 lines

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. ]

Quoted 2 lines

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

author
#3 lore raw
Quoted 27 lines · Bartlomiej Zolnierkiewicz
Quoted 19 lines · Sergei Shtylyov
Quoted 6 lines · 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;

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

Quoted 9 lines
		...
		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?

Quoted 1 line

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. ]

Quoted 7 lines
Quoted 2 lines

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 <[email protected]>

---

drivers/ide/ide-io.c +1 -1
index cc35d6d..0715692 100644
@@ -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);
#4 lore raw
Quoted 33 lines · Roel Kluin
Quoted 32 lines · Bartlomiej Zolnierkiewicz
Quoted 19 lines · Sergei Shtylyov
Quoted 6 lines · 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;

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

Quoted 9 lines
		...
		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?

Quoted 1 line

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. ]

Quoted 2 lines

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.

Quoted 7 lines
Quoted 2 lines

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. ;)

Quoted 4 lines

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).

Quoted 17 lines

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

Signed-off-by Roel Kluin <[email protected]>

---

drivers/ide/ide-io.c +1 -1
index cc35d6d..0715692 100644
@@ -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);
#7 lore raw
In reply to Bartlomiej Zolnierkiewicz

Hello.

Quoted 18 lines · Bartlomiej Zolnierkiewicz
Quoted 17 lines
Quoted 7 lines

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*.

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

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

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

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...

Quoted 7 lines

[ 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. ]

Quoted 2 lines

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?

Quoted 2 lines

Thanks,
Bart

MBR, Sergei

#13 lore raw
In reply to Bartlomiej Zolnierkiewicz
Quoted 20 lines · Bartlomiej Zolnierkiewicz
Quoted 13 lines

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 */
Quoted 6 lines

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?

Quoted 8 lines

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().

Quoted 2 lines

Thanks,
Bart

MBR, Sergei

#9 lore raw
In reply to Sergei Shtylyov
Quoted 16 lines · Hello, I wrote:
Quoted 7 lines

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*.

Quoted 5 lines
        ...
        if (expiry) {
            ...
            wait = expiry(drive);
            if (wait > 0) { /* continue */
Quoted 8 lines
Quoted 6 lines

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.

Quoted 1 line
Quoted 1 line

Roel

MBR, Sergei

#10 lore raw
Quoted 20 lines · Sergei Shtylyov
Quoted 17 lines · Hello, I wrote:
Quoted 7 lines

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*.

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

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

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

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.

#14 lore raw
In reply to Sergei Shtylyov
Quoted 26 lines · Sergei Shtylyov
Quoted 23 lines · Bartlomiej Zolnierkiewicz
Quoted 13 lines

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 */
Quoted 6 lines

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()

Quoted 7 lines
Quoted 4 lines

* 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...).

#8 lore raw
In reply to Sergei Shtylyov
Quoted 36 lines · Sergei Shtylyov

Hello.

Quoted 18 lines · Bartlomiej Zolnierkiewicz
Quoted 17 lines
Quoted 7 lines

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*.

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

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

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

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...

Quoted 2 lines

[ 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. ]

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

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.

#5 lore raw
In reply to Bartlomiej Zolnierkiewicz
Quoted 35 lines · Bartlomiej Zolnierkiewicz
Quoted 34 lines
Quoted 34 lines · Sergei Shtylyov
Quoted 22 lines
Quoted 6 lines · 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;

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

Quoted 9 lines
		...
		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?

Quoted 1 line

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. ]

Quoted 2 lines

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.

Quoted 7 lines
Quoted 4 lines

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.

Quoted 8 lines
Quoted 4 lines

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?

Quoted 2 lines
Quoted 2 lines

------------------------------>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

#15 lore raw
In reply to Bartlomiej Zolnierkiewicz
Quoted 29 lines · Bartlomiej Zolnierkiewicz
Quoted 28 lines
Quoted 28 lines
Quoted 13 lines

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 */
Quoted 6 lines

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?

Quoted 7 lines
Quoted 2 lines

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

#11 lore raw
In reply to Bartlomiej Zolnierkiewicz
Quoted 23 lines · Bartlomiej Zolnierkiewicz
Quoted 22 lines · Sergei Shtylyov
Quoted 19 lines · Hello, I wrote:
Quoted 7 lines

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*.

Quoted 9 lines
        ...
        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 *);

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

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

#6 lore raw
In reply to Bartlomiej Zolnierkiewicz
Quoted 69 lines · Bartlomiej Zolnierkiewicz
Quoted 33 lines · Roel Kluin
Quoted 32 lines · Bartlomiej Zolnierkiewicz
Quoted 19 lines · Sergei Shtylyov
Quoted 6 lines · 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;

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

Quoted 9 lines
		...
		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?

Quoted 1 line

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. ]

Quoted 2 lines

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.

Quoted 7 lines
Quoted 2 lines

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. ;)

Quoted 4 lines

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).

Quoted 16 lines

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

Signed-off-by Roel Kluin <[email protected]>

---

drivers/ide/ide-io.c +1 -1
index cc35d6d..0715692 100644
@@ -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;
Quoted 1 line

drivers/ide/ide-io.c void ide_timer_expiry (unsigned long data) · line 1195

Quoted 1 line
drivers/ide/ide-io.c +0 -0
 	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 <[email protected]>
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 <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Andrew Morton <[email protected]>
[bart: port it to the current tree]
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-io.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ide/ide-io.c +1 -1
Index: 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);