Re: [Bug #13663] suspend to ram regression (IDE related)

Re: [Bug #13663] suspend to ram regression (IDE related)

[Wu Zhangjin]

On Wed, 2009-07-01 at 22:31 +0800, Jeff Chua wrote:
> On Tue, Jun 30, 2009 at 12:21 AM, Jeff Chua<jeff.chua.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > I just tried, and it "seems" to work. Will try a few more cycles.
> 
> STD/STR survived quite a few cycles now. Patch seems to be doing the
> right thing.
> 
> On Mon, Jun 29, 2009 at 11:51 PM, Etienne
> Basset<etienne.basset-Bf/eaXMDFuuXqB7oj33eUg@public.gmane.org> wrote:
> 
> > To have STR/resume work with current git, I have to :
> 
> > 1) apply Bart's patch
> 
> This is not yet in Linus's tree. And much needed to really fix the problem.
> 
> > 2) revert this commit : a1317f714af7aed60ddc182d0122477cbe36ee9b
> 

Yes, This commit must be reverted, otherwise, STD/Hibernation will not
work either. I have tested it on two different loongson-based machines:
fuloong2e box and yeeloong2f netbook.(loongson is mips compatiable)

Here is what i have traced:

        hibernate(kernel/power/hibernate.c)
        --> hibernation_snapshot
        --> dpm_resume_end
        --> dpm_resume
        --> device_resume
        --> dev->bus->resume(generic_ide_resume), dev_name(dev) = 0.0
        --> blk_execute_rq
        {
                DECLARE_COMPLETION_ONSTACK(wait);
                ...
                wait_for_completion(&wait);     // stop here
                ...
        }

and I have tried to revert this part of the above patch:

-
-               WARN_ON_ONCE(hwif->rq);
 repeat:
                prev_port = hwif->host->cur_port;
+
+               if (drive->dev_flags & IDE_DFLAG_BLOCKED)
+                       rq = hwif->rq;
+               else
+                       WARN_ON_ONCE(hwif->rq);
+

it works! need more time to test!

thanks!
Wu Zhangjin

Re: [Bug #13663] suspend to ram regression (IDE related)

[Bartlomiej Zolnierkiewicz]

On Wednesday 01 July 2009 16:47:41 Wu Zhangjin wrote:
> On Wed, 2009-07-01 at 22:31 +0800, Jeff Chua wrote:
> > On Tue, Jun 30, 2009 at 12:21 AM, Jeff Chua<jeff.chua.linux@gmail.com> wrote:
> > 
> > > I just tried, and it "seems" to work. Will try a few more cycles.
> > 
> > STD/STR survived quite a few cycles now. Patch seems to be doing the
> > right thing.
> > 
> > On Mon, Jun 29, 2009 at 11:51 PM, Etienne
> > Basset<etienne.basset@numericable.fr> wrote:
> > 
> > > To have STR/resume work with current git, I have to :
> > 
> > > 1) apply Bart's patch
> > 
> > This is not yet in Linus's tree. And much needed to really fix the problem.
> > 
> > > 2) revert this commit : a1317f714af7aed60ddc182d0122477cbe36ee9b
> > 
> 
> Yes, This commit must be reverted, otherwise, STD/Hibernation will not
> work either. I have tested it on two different loongson-based machines:
> fuloong2e box and yeeloong2f netbook.(loongson is mips compatiable)

Since it seems like Dave is taking his sweet time with doing the revert
I stared at the code a bit more and I think that I finally found the bug
(thanks to your debugging work for giving me the right hint!).

The patch needs to take into the account a new code introduced by the recent
block layer changes (commit 8f6205cd572fece673da0255d74843680f67f879):

@@ -555,8 +560,11 @@ repeat:
                startstop = start_request(drive, rq);
                spin_lock_irq(&hwif->lock);
 
-               if (startstop == ide_stopped)
+               if (startstop == ide_stopped) {
+                       rq = hwif->rq;
+                       hwif->rq = NULL;
                        goto repeat;
+               }
        } else
                goto plug_device;
 out:

and not zero hwif->rq if the device is blocked. 

Could you try the attached patch and see if it fixes the issue?

[ Dave: while I appreciate fast handling of my patches I had strongly
  suggested giving this particular one some extra testing (because there
  were a lot of changes in between the time that it has been tested
  against other kernel subsystems).  Yet, it seems that its linux-next
  exposure was minimal at best..  :( ]

---
 drivers/ide/ide-io.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -532,7 +532,8 @@ repeat:
 
 		if (startstop == ide_stopped) {
 			rq = hwif->rq;
-			hwif->rq = NULL;
+			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0)
+				hwif->rq = NULL;
 			goto repeat;
 		}
 	} else

Re: [Bug #13663] suspend to ram regression (IDE related)

[Bartlomiej Zolnierkiewicz]

On Wednesday 01 July 2009 18:21:25 Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 01 July 2009 16:47:41 Wu Zhangjin wrote:
> > On Wed, 2009-07-01 at 22:31 +0800, Jeff Chua wrote:
> > > On Tue, Jun 30, 2009 at 12:21 AM, Jeff Chua<jeff.chua.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > 
> > > > I just tried, and it "seems" to work. Will try a few more cycles.
> > > 
> > > STD/STR survived quite a few cycles now. Patch seems to be doing the
> > > right thing.
> > > 
> > > On Mon, Jun 29, 2009 at 11:51 PM, Etienne
> > > Basset<etienne.basset-Bf/eaXMDFuuXqB7oj33eUg@public.gmane.org> wrote:
> > > 
> > > > To have STR/resume work with current git, I have to :
> > > 
> > > > 1) apply Bart's patch
> > > 
> > > This is not yet in Linus's tree. And much needed to really fix the problem.
> > > 
> > > > 2) revert this commit : a1317f714af7aed60ddc182d0122477cbe36ee9b
> > > 
> > 
> > Yes, This commit must be reverted, otherwise, STD/Hibernation will not
> > work either. I have tested it on two different loongson-based machines:
> > fuloong2e box and yeeloong2f netbook.(loongson is mips compatiable)
> 
> Since it seems like Dave is taking his sweet time with doing the revert
> I stared at the code a bit more and I think that I finally found the bug
> (thanks to your debugging work for giving me the right hint!).
> 
> The patch needs to take into the account a new code introduced by the recent
> block layer changes (commit 8f6205cd572fece673da0255d74843680f67f879):
> 
> @@ -555,8 +560,11 @@ repeat:
>                 startstop = start_request(drive, rq);
>                 spin_lock_irq(&hwif->lock);
>  
> -               if (startstop == ide_stopped)
> +               if (startstop == ide_stopped) {
> +                       rq = hwif->rq;
> +                       hwif->rq = NULL;
>                         goto repeat;
> +               }
>         } else
>                 goto plug_device;
>  out:
> 
> and not zero hwif->rq if the device is blocked. 
> 
> Could you try the attached patch and see if it fixes the issue?

Here is the more complete version, also taking into the account changes
in ide_intr() and ide_timer_expiry():

---
 drivers/ide/ide-io.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -532,7 +532,8 @@ repeat:
 
 		if (startstop == ide_stopped) {
 			rq = hwif->rq;
-			hwif->rq = NULL;
+			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0)
+				hwif->rq = NULL;
 			goto repeat;
 		}
 	} else
@@ -679,8 +680,10 @@ void ide_timer_expiry (unsigned long dat
 		spin_lock_irq(&hwif->lock);
 		enable_irq(hwif->irq);
 		if (startstop == ide_stopped && hwif->polling == 0) {
-			rq_in_flight = hwif->rq;
-			hwif->rq = NULL;
+			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
+				rq_in_flight = hwif->rq;
+				hwif->rq = NULL;
+			}
 			ide_unlock_port(hwif);
 			plug_device = 1;
 		}
@@ -856,8 +859,10 @@ irqreturn_t ide_intr (int irq, void *dev
 	 */
 	if (startstop == ide_stopped && hwif->polling == 0) {
 		BUG_ON(hwif->handler);
-		rq_in_flight = hwif->rq;
-		hwif->rq = NULL;
+		if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
+			rq_in_flight = hwif->rq;
+			hwif->rq = NULL;
+		}
 		ide_unlock_port(hwif);
 		plug_device = 1;
 	}

Re: [Bug #13663] suspend to ram regression (IDE related)

[Jeff Chua]

On Thu, Jul 2, 2009 at 12:29 AM, Bartlomiej
Zolnierkiewicz<bzolnier@gmail.com> wrote:
> Here is the more complete version, also taking into the account changes
> in ide_intr() and ide_timer_expiry():

This works great for. Survived STR, STD. I just applied on top vanilla
latest Linus's git pull. Nothing else to revert.

Thanks,
Jeff.


> ---
>  drivers/ide/ide-io.c |   15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> Index: b/drivers/ide/ide-io.c
> ===================================================================
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -532,7 +532,8 @@ repeat:
>
>                if (startstop == ide_stopped) {
>                        rq = hwif->rq;
> -                       hwif->rq = NULL;
> +                       if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0)
> +                               hwif->rq = NULL;
>                        goto repeat;
>                }
>        } else
> @@ -679,8 +680,10 @@ void ide_timer_expiry (unsigned long dat
>                spin_lock_irq(&hwif->lock);
>                enable_irq(hwif->irq);
>                if (startstop == ide_stopped && hwif->polling == 0) {
> -                       rq_in_flight = hwif->rq;
> -                       hwif->rq = NULL;
> +                       if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
> +                               rq_in_flight = hwif->rq;
> +                               hwif->rq = NULL;
> +                       }
>                        ide_unlock_port(hwif);
>                        plug_device = 1;
>                }
> @@ -856,8 +859,10 @@ irqreturn_t ide_intr (int irq, void *dev
>         */
>        if (startstop == ide_stopped && hwif->polling == 0) {
>                BUG_ON(hwif->handler);
> -               rq_in_flight = hwif->rq;
> -               hwif->rq = NULL;
> +               if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
> +                       rq_in_flight = hwif->rq;
> +                       hwif->rq = NULL;
> +               }
>                ide_unlock_port(hwif);
>                plug_device = 1;
>        }
>

Re: [Bug #13663] suspend to ram regression (IDE related)

[Etienne Basset]

Jeff Chua wrote:
> On Thu, Jul 2, 2009 at 12:29 AM, Bartlomiej
> Zolnierkiewicz<bzolnier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Here is the more complete version, also taking into the account changes
>> in ide_intr() and ide_timer_expiry():
> 
> This works great for. Survived STR, STD. I just applied on top vanilla
> latest Linus's git pull. Nothing else to revert.
> 
> Thanks,
> Jeff.
> 
> 
i confirm, this  works for me too :)
thanks,
Etienne


>> ---
>>  drivers/ide/ide-io.c |   15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> Index: b/drivers/ide/ide-io.c
>> ===================================================================
>> --- a/drivers/ide/ide-io.c
>> +++ b/drivers/ide/ide-io.c
>> @@ -532,7 +532,8 @@ repeat:
>>
>>                if (startstop == ide_stopped) {
>>                        rq = hwif->rq;
>> -                       hwif->rq = NULL;
>> +                       if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0)
>> +                               hwif->rq = NULL;
>>                        goto repeat;
>>                }
>>        } else
>> @@ -679,8 +680,10 @@ void ide_timer_expiry (unsigned long dat
>>                spin_lock_irq(&hwif->lock);
>>                enable_irq(hwif->irq);
>>                if (startstop == ide_stopped && hwif->polling == 0) {
>> -                       rq_in_flight = hwif->rq;
>> -                       hwif->rq = NULL;
>> +                       if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
>> +                               rq_in_flight = hwif->rq;
>> +                               hwif->rq = NULL;
>> +                       }
>>                        ide_unlock_port(hwif);
>>                        plug_device = 1;
>>                }
>> @@ -856,8 +859,10 @@ irqreturn_t ide_intr (int irq, void *dev
>>         */
>>        if (startstop == ide_stopped && hwif->polling == 0) {
>>                BUG_ON(hwif->handler);
>> -               rq_in_flight = hwif->rq;
>> -               hwif->rq = NULL;
>> +               if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
>> +                       rq_in_flight = hwif->rq;
>> +                       hwif->rq = NULL;
>> +               }
>>                ide_unlock_port(hwif);
>>                plug_device = 1;
>>        }
>>

Re: [Bug #13663] suspend to ram regression (IDE related)

[Wu Zhangjin]

On Wed, 2009-07-01 at 18:29 +0200, Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 01 July 2009 18:21:25 Bartlomiej Zolnierkiewicz wrote:
> > On Wednesday 01 July 2009 16:47:41 Wu Zhangjin wrote:
> > > On Wed, 2009-07-01 at 22:31 +0800, Jeff Chua wrote:
> > > > On Tue, Jun 30, 2009 at 12:21 AM, Jeff Chua<jeff.chua.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > > 
> > > > > I just tried, and it "seems" to work. Will try a few more cycles.
> > > > 
> > > > STD/STR survived quite a few cycles now. Patch seems to be doing the
> > > > right thing.
> > > > 
> > > > On Mon, Jun 29, 2009 at 11:51 PM, Etienne
> > > > Basset<etienne.basset-Bf/eaXMDFuuXqB7oj33eUg@public.gmane.org> wrote:
> > > > 
> > > > > To have STR/resume work with current git, I have to :
> > > > 
> > > > > 1) apply Bart's patch
> > > > 
> > > > This is not yet in Linus's tree. And much needed to really fix the problem.
> > > > 
> > > > > 2) revert this commit : a1317f714af7aed60ddc182d0122477cbe36ee9b
> > > > 
> > > 
> > > Yes, This commit must be reverted, otherwise, STD/Hibernation will not
> > > work either. I have tested it on two different loongson-based machines:
> > > fuloong2e box and yeeloong2f netbook.(loongson is mips compatiable)
> > 
> > Since it seems like Dave is taking his sweet time with doing the revert
> > I stared at the code a bit more and I think that I finally found the bug
> > (thanks to your debugging work for giving me the right hint!).
> > 
> > The patch needs to take into the account a new code introduced by the recent
> > block layer changes (commit 8f6205cd572fece673da0255d74843680f67f879):
> > 
> > @@ -555,8 +560,11 @@ repeat:
> >                 startstop = start_request(drive, rq);
> >                 spin_lock_irq(&hwif->lock);
> >  
> > -               if (startstop == ide_stopped)
> > +               if (startstop == ide_stopped) {
> > +                       rq = hwif->rq;
> > +                       hwif->rq = NULL;
> >                         goto repeat;
> > +               }
> >         } else
> >                 goto plug_device;
> >  out:
> > 
> > and not zero hwif->rq if the device is blocked. 
> > 
> > Could you try the attached patch and see if it fixes the issue?
> 
> Here is the more complete version, also taking into the account changes
> in ide_intr() and ide_timer_expiry():
> 

Sorry, I can not apply this patch directly, which original version did
you use? I used the one in the master branch of linux-mips development
git repository.

commit 5a4f13fad1ab5bd08dea78fc55321e429d83cddf
Merge: ec9c45d e18ed14
Author: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Date:   Mon Jun 29 20:07:43 2009 -0700

    Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide-2.6
    
    * git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide-2.6:
      ide: memory overrun in ide_get_identity_ioctl() on big endian
machines using ioctl HDIO_OBSOLETE_IDENTITY
      ide: fix resume for CONFIG_BLK_DEV_IDEACPI=y
      ide-cd: handle fragmented packet commands gracefully
      ide: always kill the whole request on error
      ide: fix ide_kill_rq() for special ide-{floppy,tape} driver
requests

it this too old? should i merge another git repository?

I have tried to apply it manually, but unfortunately, also not work. any
other patch needed?

Thanks!
Wu Zhangjin
> ---
>  drivers/ide/ide-io.c |   15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> Index: b/drivers/ide/ide-io.c
> ===================================================================
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -532,7 +532,8 @@ repeat:
>  
>  		if (startstop == ide_stopped) {
>  			rq = hwif->rq;
> -			hwif->rq = NULL;
> +			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0)
> +				hwif->rq = NULL;
>  			goto repeat;
>  		}
>  	} else
> @@ -679,8 +680,10 @@ void ide_timer_expiry (unsigned long dat
>  		spin_lock_irq(&hwif->lock);
>  		enable_irq(hwif->irq);
>  		if (startstop == ide_stopped && hwif->polling == 0) {
> -			rq_in_flight = hwif->rq;
> -			hwif->rq = NULL;
> +			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
> +				rq_in_flight = hwif->rq;
> +				hwif->rq = NULL;
> +			}
>  			ide_unlock_port(hwif);
>  			plug_device = 1;
>  		}
> @@ -856,8 +859,10 @@ irqreturn_t ide_intr (int irq, void *dev
>  	 */
>  	if (startstop == ide_stopped && hwif->polling == 0) {
>  		BUG_ON(hwif->handler);
> -		rq_in_flight = hwif->rq;
> -		hwif->rq = NULL;
> +		if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
> +			rq_in_flight = hwif->rq;
> +			hwif->rq = NULL;
> +		}
>  		ide_unlock_port(hwif);
>  		plug_device = 1;
>  	}

Re: [Bug #13663] suspend to ram regression (IDE related)

[Jeff Chua]

On Thu, Jul 2, 2009 at 9:46 AM, Wu Zhangjin<wuzhangjin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> it this too old? should i merge another git repository?
> I have tried to apply it manually, but unfortunately, also not work. any
> other patch needed?

You need to be undo those two patches below ...

> On Mon, Jun 29, 2009 at 11:51 PM, Etienne Basset<etienne.basset-Bf/eaXMDFuuXqB7oj33eUg@public.gmane.org>
> To have STR/resume work with current git, I have to :
> 1) apply Bart's patch
> 2) revert this commit : a1317f714af7aed60ddc182d0122477cbe36ee9b

or try to pull from Linus's tree and try again. Latest is now ...

commit d960eea974f5e500c0dcb95a934239cc1f481cfd
Author: Randy Dunlap <randy.dunlap-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Date:   Mon Jun 29 14:54:11 2009 -0700

    kernel-doc: move ignoring kmemcheck



Jeff.

Re: [Bug #13663] suspend to ram regression (IDE related)

[Ralf Baechle]

On Thu, Jul 02, 2009 at 09:46:43AM +0800, Wu Zhangjin wrote:

> Sorry, I can not apply this patch directly, which original version did
> you use? I used the one in the master branch of linux-mips development
> git repository.

The master branch of linux-mips.org has no IDE changes over Linus' tree.

  Ralf

Re: [Bug #13663] suspend to ram regression (IDE related)

[Bartlomiej Zolnierkiewicz]

On Thursday 02 July 2009 03:46:43 Wu Zhangjin wrote:
> On Wed, 2009-07-01 at 18:29 +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Wednesday 01 July 2009 18:21:25 Bartlomiej Zolnierkiewicz wrote:
> > > On Wednesday 01 July 2009 16:47:41 Wu Zhangjin wrote:
> > > > On Wed, 2009-07-01 at 22:31 +0800, Jeff Chua wrote:
> > > > > On Tue, Jun 30, 2009 at 12:21 AM, Jeff Chua<jeff.chua.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > > > 
> > > > > > I just tried, and it "seems" to work. Will try a few more cycles.
> > > > > 
> > > > > STD/STR survived quite a few cycles now. Patch seems to be doing the
> > > > > right thing.
> > > > > 
> > > > > On Mon, Jun 29, 2009 at 11:51 PM, Etienne
> > > > > Basset<etienne.basset-Bf/eaXMDFuuXqB7oj33eUg@public.gmane.org> wrote:
> > > > > 
> > > > > > To have STR/resume work with current git, I have to :
> > > > > 
> > > > > > 1) apply Bart's patch
> > > > > 
> > > > > This is not yet in Linus's tree. And much needed to really fix the problem.
> > > > > 
> > > > > > 2) revert this commit : a1317f714af7aed60ddc182d0122477cbe36ee9b
> > > > > 
> > > > 
> > > > Yes, This commit must be reverted, otherwise, STD/Hibernation will not
> > > > work either. I have tested it on two different loongson-based machines:
> > > > fuloong2e box and yeeloong2f netbook.(loongson is mips compatiable)
> > > 
> > > Since it seems like Dave is taking his sweet time with doing the revert
> > > I stared at the code a bit more and I think that I finally found the bug
> > > (thanks to your debugging work for giving me the right hint!).
> > > 
> > > The patch needs to take into the account a new code introduced by the recent
> > > block layer changes (commit 8f6205cd572fece673da0255d74843680f67f879):
> > > 
> > > @@ -555,8 +560,11 @@ repeat:
> > >                 startstop = start_request(drive, rq);
> > >                 spin_lock_irq(&hwif->lock);
> > >  
> > > -               if (startstop == ide_stopped)
> > > +               if (startstop == ide_stopped) {
> > > +                       rq = hwif->rq;
> > > +                       hwif->rq = NULL;
> > >                         goto repeat;
> > > +               }
> > >         } else
> > >                 goto plug_device;
> > >  out:
> > > 
> > > and not zero hwif->rq if the device is blocked. 
> > > 
> > > Could you try the attached patch and see if it fixes the issue?
> > 
> > Here is the more complete version, also taking into the account changes
> > in ide_intr() and ide_timer_expiry():
> > 
> 
> Sorry, I can not apply this patch directly, which original version did
> you use? I used the one in the master branch of linux-mips development
> git repository.
> 
> commit 5a4f13fad1ab5bd08dea78fc55321e429d83cddf
> Merge: ec9c45d e18ed14
> Author: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Date:   Mon Jun 29 20:07:43 2009 -0700
> 
>     Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide-2.6
>     
>     * git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide-2.6:
>       ide: memory overrun in ide_get_identity_ioctl() on big endian
> machines using ioctl HDIO_OBSOLETE_IDENTITY
>       ide: fix resume for CONFIG_BLK_DEV_IDEACPI=y
>       ide-cd: handle fragmented packet commands gracefully
>       ide: always kill the whole request on error
>       ide: fix ide_kill_rq() for special ide-{floppy,tape} driver
> requests
> 
> it this too old? should i merge another git repository?

Weird, I used linux-next but Linus' tree should also be fine
(as it matches linux-next w.r.t. ide currently).

Anyway since the patch was confirmed to fix the problem by
Jeff and Etienne here is the final version for Dave.

From: Bartlomiej Zolnierkiewicz <bzolnier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Subject: [PATCH] ide: make resume work again

It turns out that commit a1317f714af7aed60ddc182d0122477cbe36ee9b
("ide: improve handling of Power Management requests") needs to take
into the account a new code added by the recent block layer changes
in commit 8f6205cd572fece673da0255d74843680f67f879 ("ide: dequeue
in-flight request") and prevent clearing of hwif->rq if the device
is blocked.

Thanks to Etienne, Wu and Jeff for help in fixing the issue.

Reported-and-tested-by: Jeff Chua <jeff.chua.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reported-and-tested-by: Etienne Basset <etienne.basset-Bf/eaXMDFuuXqB7oj33eUg@public.gmane.org>
Reported-by: Wu Zhangjin <wuzhangjin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
Added patch description, no other changes.

 drivers/ide/ide-io.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -532,7 +532,8 @@ repeat:
 
 		if (startstop == ide_stopped) {
 			rq = hwif->rq;
-			hwif->rq = NULL;
+			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0)
+				hwif->rq = NULL;
 			goto repeat;
 		}
 	} else
@@ -679,8 +680,10 @@ void ide_timer_expiry (unsigned long dat
 		spin_lock_irq(&hwif->lock);
 		enable_irq(hwif->irq);
 		if (startstop == ide_stopped && hwif->polling == 0) {
-			rq_in_flight = hwif->rq;
-			hwif->rq = NULL;
+			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
+				rq_in_flight = hwif->rq;
+				hwif->rq = NULL;
+			}
 			ide_unlock_port(hwif);
 			plug_device = 1;
 		}
@@ -856,8 +859,10 @@ irqreturn_t ide_intr (int irq, void *dev
 	 */
 	if (startstop == ide_stopped && hwif->polling == 0) {
 		BUG_ON(hwif->handler);
-		rq_in_flight = hwif->rq;
-		hwif->rq = NULL;
+		if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
+			rq_in_flight = hwif->rq;
+			hwif->rq = NULL;
+		}
 		ide_unlock_port(hwif);
 		plug_device = 1;
 	}

Re: [Bug #13663] suspend to ram regression (IDE related)

[Wu Zhangjin]

On Thu, 2009-07-02 at 18:13 +0200, Bartlomiej Zolnierkiewicz wrote:
> On Thursday 02 July 2009 03:46:43 Wu Zhangjin wrote:
> > On Wed, 2009-07-01 at 18:29 +0200, Bartlomiej Zolnierkiewicz wrote:
> > > On Wednesday 01 July 2009 18:21:25 Bartlomiej Zolnierkiewicz wrote:
> > > > On Wednesday 01 July 2009 16:47:41 Wu Zhangjin wrote:
> > > > > On Wed, 2009-07-01 at 22:31 +0800, Jeff Chua wrote:
> > > > > > On Tue, Jun 30, 2009 at 12:21 AM, Jeff Chua<jeff.chua.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > > > > 
> > > > > > > I just tried, and it "seems" to work. Will try a few more cycles.
> > > > > > 
> > > > > > STD/STR survived quite a few cycles now. Patch seems to be doing the
> > > > > > right thing.
> > > > > > 
> > > > > > On Mon, Jun 29, 2009 at 11:51 PM, Etienne
> > > > > > Basset<etienne.basset-Bf/eaXMDFuuXqB7oj33eUg@public.gmane.org> wrote:
> > > > > > 
> > > > > > > To have STR/resume work with current git, I have to :
> > > > > > 
> > > > > > > 1) apply Bart's patch
> > > > > > 
> > > > > > This is not yet in Linus's tree. And much needed to really fix the problem.
> > > > > > 
> > > > > > > 2) revert this commit : a1317f714af7aed60ddc182d0122477cbe36ee9b
> > > > > > 
> > > > > 
> > > > > Yes, This commit must be reverted, otherwise, STD/Hibernation will not
> > > > > work either. I have tested it on two different loongson-based machines:
> > > > > fuloong2e box and yeeloong2f netbook.(loongson is mips compatiable)
> > > > 
> > > > Since it seems like Dave is taking his sweet time with doing the revert
> > > > I stared at the code a bit more and I think that I finally found the bug
> > > > (thanks to your debugging work for giving me the right hint!).
> > > > 
> > > > The patch needs to take into the account a new code introduced by the recent
> > > > block layer changes (commit 8f6205cd572fece673da0255d74843680f67f879):
> > > > 
> > > > @@ -555,8 +560,11 @@ repeat:
> > > >                 startstop = start_request(drive, rq);
> > > >                 spin_lock_irq(&hwif->lock);
> > > >  
> > > > -               if (startstop == ide_stopped)
> > > > +               if (startstop == ide_stopped) {
> > > > +                       rq = hwif->rq;
> > > > +                       hwif->rq = NULL;
> > > >                         goto repeat;
> > > > +               }
> > > >         } else
> > > >                 goto plug_device;
> > > >  out:
> > > > 
> > > > and not zero hwif->rq if the device is blocked. 
> > > > 
> > > > Could you try the attached patch and see if it fixes the issue?
> > > 
> > > Here is the more complete version, also taking into the account changes
> > > in ide_intr() and ide_timer_expiry():
> > > 
> > 
> > Sorry, I can not apply this patch directly, which original version did
> > you use? I used the one in the master branch of linux-mips development
> > git repository.
> > 
> > commit 5a4f13fad1ab5bd08dea78fc55321e429d83cddf
> > Merge: ec9c45d e18ed14
> > Author: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> > Date:   Mon Jun 29 20:07:43 2009 -0700
> > 
> >     Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide-2.6
> >     
> >     * git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide-2.6:
> >       ide: memory overrun in ide_get_identity_ioctl() on big endian
> > machines using ioctl HDIO_OBSOLETE_IDENTITY
> >       ide: fix resume for CONFIG_BLK_DEV_IDEACPI=y
> >       ide-cd: handle fragmented packet commands gracefully
> >       ide: always kill the whole request on error
> >       ide: fix ide_kill_rq() for special ide-{floppy,tape} driver
> > requests
> > 
> > it this too old? should i merge another git repository?
> 
> Weird, I used linux-next but Linus' tree should also be fine
> (as it matches linux-next w.r.t. ide currently).

I just cloned the linux-next git repo, and tested your patch with
STD/Hibernation, unfortunately, it also not work :-(

here is the Call Trace:

blk_delete_timer+0x0/0x20
blk_requeue_request+0x24/0xd0
ide_requeue_and_plug+0x38/0xb0
ide_intr+0x120/0x300             --->  ide_intr....
handle_IRQ_event+0x94/0x230
handle_level_irq+0x7c/0x120
mach_irq_dispatch+0xc8/0x158
ret_from_irq+0x0/0x4
cpu_idle+0x30/0x60
start_kernel+0x330/0x34c

If _NOT_ apply your patch and comment this part, it works:

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index d5f3c77..a45de2b 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -468,12 +468,12 @@ void do_ide_request(struct request_queue *q)
                ide_hwif_t *prev_port;
 repeat:
                prev_port = hwif->host->cur_port;
-
+/*
                if (drive->dev_flags & IDE_DFLAG_BLOCKED)
                        rq = hwif->rq;
                else
                        WARN_ON_ONCE(hwif->rq);
-
+*/
                if (drive->dev_flags & IDE_DFLAG_SLEEPING &&
                    time_after(drive->sleep, jiffies)) {
                        ide_unlock_port(hwif);
 

Regards,
Wu Zhangjin
> 
> Anyway since the patch was confirmed to fix the problem by
> Jeff and Etienne here is the final version for Dave.
> 
> From: Bartlomiej Zolnierkiewicz <bzolnier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Subject: [PATCH] ide: make resume work again
> 
> It turns out that commit a1317f714af7aed60ddc182d0122477cbe36ee9b
> ("ide: improve handling of Power Management requests") needs to take
> into the account a new code added by the recent block layer changes
> in commit 8f6205cd572fece673da0255d74843680f67f879 ("ide: dequeue
> in-flight request") and prevent clearing of hwif->rq if the device
> is blocked.
> 
> Thanks to Etienne, Wu and Jeff for help in fixing the issue.
> 
> Reported-and-tested-by: Jeff Chua <jeff.chua.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reported-and-tested-by: Etienne Basset <etienne.basset-Bf/eaXMDFuuXqB7oj33eUg@public.gmane.org>
> Reported-by: Wu Zhangjin <wuzhangjin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> Added patch description, no other changes.
> 
>  drivers/ide/ide-io.c |   15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> Index: b/drivers/ide/ide-io.c
> ===================================================================
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -532,7 +532,8 @@ repeat:
>  
>  		if (startstop == ide_stopped) {
>  			rq = hwif->rq;
> -			hwif->rq = NULL;
> +			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0)
> +				hwif->rq = NULL;
>  			goto repeat;
>  		}
>  	} else
> @@ -679,8 +680,10 @@ void ide_timer_expiry (unsigned long dat
>  		spin_lock_irq(&hwif->lock);
>  		enable_irq(hwif->irq);
>  		if (startstop == ide_stopped && hwif->polling == 0) {
> -			rq_in_flight = hwif->rq;
> -			hwif->rq = NULL;
> +			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
> +				rq_in_flight = hwif->rq;
> +				hwif->rq = NULL;
> +			}
>  			ide_unlock_port(hwif);
>  			plug_device = 1;
>  		}
> @@ -856,8 +859,10 @@ irqreturn_t ide_intr (int irq, void *dev
>  	 */
>  	if (startstop == ide_stopped && hwif->polling == 0) {
>  		BUG_ON(hwif->handler);
> -		rq_in_flight = hwif->rq;
> -		hwif->rq = NULL;
> +		if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
> +			rq_in_flight = hwif->rq;
> +			hwif->rq = NULL;
> +		}
>  		ide_unlock_port(hwif);
>  		plug_device = 1;
>  	}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug #13663] suspend to ram regression (IDE related)

[Wu Zhangjin]

On Fri, 2009-07-03 at 11:58 +0800, Wu Zhangjin wrote:
> On Thu, 2009-07-02 at 18:13 +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Thursday 02 July 2009 03:46:43 Wu Zhangjin wrote:
> > > On Wed, 2009-07-01 at 18:29 +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > On Wednesday 01 July 2009 18:21:25 Bartlomiej Zolnierkiewicz wrote:
> > > > > On Wednesday 01 July 2009 16:47:41 Wu Zhangjin wrote:
> > > > > > On Wed, 2009-07-01 at 22:31 +0800, Jeff Chua wrote:
> > > > > > > On Tue, Jun 30, 2009 at 12:21 AM, Jeff Chua<jeff.chua.linux@gmail.com> wrote:
> > > > > > > 
> > > > > > > > I just tried, and it "seems" to work. Will try a few more cycles.
> > > > > > > 
> > > > > > > STD/STR survived quite a few cycles now. Patch seems to be doing the
> > > > > > > right thing.
> > > > > > > 
> > > > > > > On Mon, Jun 29, 2009 at 11:51 PM, Etienne
> > > > > > > Basset<etienne.basset@numericable.fr> wrote:
> > > > > > > 
> > > > > > > > To have STR/resume work with current git, I have to :
> > > > > > > 
> > > > > > > > 1) apply Bart's patch
> > > > > > > 
> > > > > > > This is not yet in Linus's tree. And much needed to really fix the problem.
> > > > > > > 
> > > > > > > > 2) revert this commit : a1317f714af7aed60ddc182d0122477cbe36ee9b
> > > > > > > 
> > > > > > 
> > > > > > Yes, This commit must be reverted, otherwise, STD/Hibernation will not
> > > > > > work either. I have tested it on two different loongson-based machines:
> > > > > > fuloong2e box and yeeloong2f netbook.(loongson is mips compatiable)
> > > > > 
> > > > > Since it seems like Dave is taking his sweet time with doing the revert
> > > > > I stared at the code a bit more and I think that I finally found the bug
> > > > > (thanks to your debugging work for giving me the right hint!).
> > > > > 
> > > > > The patch needs to take into the account a new code introduced by the recent
> > > > > block layer changes (commit 8f6205cd572fece673da0255d74843680f67f879):
> > > > > 
> > > > > @@ -555,8 +560,11 @@ repeat:
> > > > >                 startstop = start_request(drive, rq);
> > > > >                 spin_lock_irq(&hwif->lock);
> > > > >  
> > > > > -               if (startstop == ide_stopped)
> > > > > +               if (startstop == ide_stopped) {
> > > > > +                       rq = hwif->rq;
> > > > > +                       hwif->rq = NULL;
> > > > >                         goto repeat;
> > > > > +               }
> > > > >         } else
> > > > >                 goto plug_device;
> > > > >  out:
> > > > > 
> > > > > and not zero hwif->rq if the device is blocked. 
> > > > > 
> > > > > Could you try the attached patch and see if it fixes the issue?
> > > > 
> > > > Here is the more complete version, also taking into the account changes
> > > > in ide_intr() and ide_timer_expiry():
> > > > 
> > > 
> > > Sorry, I can not apply this patch directly, which original version did
> > > you use? I used the one in the master branch of linux-mips development
> > > git repository.
> > > 
> > > commit 5a4f13fad1ab5bd08dea78fc55321e429d83cddf
> > > Merge: ec9c45d e18ed14
> > > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > > Date:   Mon Jun 29 20:07:43 2009 -0700
> > > 
> > >     Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide-2.6
> > >     
> > >     * git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide-2.6:
> > >       ide: memory overrun in ide_get_identity_ioctl() on big endian
> > > machines using ioctl HDIO_OBSOLETE_IDENTITY
> > >       ide: fix resume for CONFIG_BLK_DEV_IDEACPI=y
> > >       ide-cd: handle fragmented packet commands gracefully
> > >       ide: always kill the whole request on error
> > >       ide: fix ide_kill_rq() for special ide-{floppy,tape} driver
> > > requests
> > > 
> > > it this too old? should i merge another git repository?
> > 
> > Weird, I used linux-next but Linus' tree should also be fine
> > (as it matches linux-next w.r.t. ide currently).
> 
> I just cloned the linux-next git repo, and tested your patch with
> STD/Hibernation, unfortunately, it also not work :-(
> 
> here is the Call Trace:
> 
> blk_delete_timer+0x0/0x20
> blk_requeue_request+0x24/0xd0
> ide_requeue_and_plug+0x38/0xb0
> ide_intr+0x120/0x300             --->  ide_intr....
> handle_IRQ_event+0x94/0x230
> handle_level_irq+0x7c/0x120
> mach_irq_dispatch+0xc8/0x158
> ret_from_irq+0x0/0x4
> cpu_idle+0x30/0x60
> start_kernel+0x330/0x34c
> 
There are two more lines after the Call Trace:

Disabling lock debugging due to kernel taint
Kernel panic - not syncing: Fatal exception in interrupt.

> If _NOT_ apply your patch and comment this part, it works:
> 
> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> index d5f3c77..a45de2b 100644
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -468,12 +468,12 @@ void do_ide_request(struct request_queue *q)
>                 ide_hwif_t *prev_port;
>  repeat:
>                 prev_port = hwif->host->cur_port;
> -
> +/*
>                 if (drive->dev_flags & IDE_DFLAG_BLOCKED)
>                         rq = hwif->rq;
>                 else
>                         WARN_ON_ONCE(hwif->rq);
> -
> +*/
>                 if (drive->dev_flags & IDE_DFLAG_SLEEPING &&
>                     time_after(drive->sleep, jiffies)) {
>                         ide_unlock_port(hwif);
>  
> 
> Regards,
> Wu Zhangjin
> > 
> > Anyway since the patch was confirmed to fix the problem by
> > Jeff and Etienne here is the final version for Dave.
> > 
> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Subject: [PATCH] ide: make resume work again
> > 
> > It turns out that commit a1317f714af7aed60ddc182d0122477cbe36ee9b
> > ("ide: improve handling of Power Management requests") needs to take
> > into the account a new code added by the recent block layer changes
> > in commit 8f6205cd572fece673da0255d74843680f67f879 ("ide: dequeue
> > in-flight request") and prevent clearing of hwif->rq if the device
> > is blocked.
> > 
> > Thanks to Etienne, Wu and Jeff for help in fixing the issue.
> > 
> > Reported-and-tested-by: Jeff Chua <jeff.chua.linux@gmail.com>
> > Reported-and-tested-by: Etienne Basset <etienne.basset@numericable.fr>
> > Reported-by: Wu Zhangjin <wuzhangjin@gmail.com>
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > ---
> > Added patch description, no other changes.
> > 
> >  drivers/ide/ide-io.c |   15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> > 
> > Index: b/drivers/ide/ide-io.c
> > ===================================================================
> > --- a/drivers/ide/ide-io.c
> > +++ b/drivers/ide/ide-io.c
> > @@ -532,7 +532,8 @@ repeat:
> >  
> >  		if (startstop == ide_stopped) {
> >  			rq = hwif->rq;
> > -			hwif->rq = NULL;
> > +			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0)
> > +				hwif->rq = NULL;
> >  			goto repeat;
> >  		}
> >  	} else
> > @@ -679,8 +680,10 @@ void ide_timer_expiry (unsigned long dat
> >  		spin_lock_irq(&hwif->lock);
> >  		enable_irq(hwif->irq);
> >  		if (startstop == ide_stopped && hwif->polling == 0) {
> > -			rq_in_flight = hwif->rq;
> > -			hwif->rq = NULL;
> > +			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
> > +				rq_in_flight = hwif->rq;
> > +				hwif->rq = NULL;
> > +			}
> >  			ide_unlock_port(hwif);
> >  			plug_device = 1;
> >  		}
> > @@ -856,8 +859,10 @@ irqreturn_t ide_intr (int irq, void *dev
> >  	 */
> >  	if (startstop == ide_stopped && hwif->polling == 0) {
> >  		BUG_ON(hwif->handler);
> > -		rq_in_flight = hwif->rq;
> > -		hwif->rq = NULL;
> > +		if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
> > +			rq_in_flight = hwif->rq;
> > +			hwif->rq = NULL;
> > +		}
> >  		ide_unlock_port(hwif);
> >  		plug_device = 1;
> >  	}
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug #13663] suspend to ram regression (IDE related)

[Bartlomiej Zolnierkiewicz]

On Friday 03 July 2009 05:58:25 Wu Zhangjin wrote:
> On Thu, 2009-07-02 at 18:13 +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Thursday 02 July 2009 03:46:43 Wu Zhangjin wrote:
> > > On Wed, 2009-07-01 at 18:29 +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > On Wednesday 01 July 2009 18:21:25 Bartlomiej Zolnierkiewicz wrote:
> > > > > On Wednesday 01 July 2009 16:47:41 Wu Zhangjin wrote:
> > > > > > On Wed, 2009-07-01 at 22:31 +0800, Jeff Chua wrote:
> > > > > > > On Tue, Jun 30, 2009 at 12:21 AM, Jeff Chua<jeff.chua.linux@gmail.com> wrote:
> > > > > > > 
> > > > > > > > I just tried, and it "seems" to work. Will try a few more cycles.
> > > > > > > 
> > > > > > > STD/STR survived quite a few cycles now. Patch seems to be doing the
> > > > > > > right thing.
> > > > > > > 
> > > > > > > On Mon, Jun 29, 2009 at 11:51 PM, Etienne
> > > > > > > Basset<etienne.basset@numericable.fr> wrote:
> > > > > > > 
> > > > > > > > To have STR/resume work with current git, I have to :
> > > > > > > 
> > > > > > > > 1) apply Bart's patch
> > > > > > > 
> > > > > > > This is not yet in Linus's tree. And much needed to really fix the problem.
> > > > > > > 
> > > > > > > > 2) revert this commit : a1317f714af7aed60ddc182d0122477cbe36ee9b
> > > > > > > 
> > > > > > 
> > > > > > Yes, This commit must be reverted, otherwise, STD/Hibernation will not
> > > > > > work either. I have tested it on two different loongson-based machines:
> > > > > > fuloong2e box and yeeloong2f netbook.(loongson is mips compatiable)
> > > > > 
> > > > > Since it seems like Dave is taking his sweet time with doing the revert
> > > > > I stared at the code a bit more and I think that I finally found the bug
> > > > > (thanks to your debugging work for giving me the right hint!).
> > > > > 
> > > > > The patch needs to take into the account a new code introduced by the recent
> > > > > block layer changes (commit 8f6205cd572fece673da0255d74843680f67f879):
> > > > > 
> > > > > @@ -555,8 +560,11 @@ repeat:
> > > > >                 startstop = start_request(drive, rq);
> > > > >                 spin_lock_irq(&hwif->lock);
> > > > >  
> > > > > -               if (startstop == ide_stopped)
> > > > > +               if (startstop == ide_stopped) {
> > > > > +                       rq = hwif->rq;
> > > > > +                       hwif->rq = NULL;
> > > > >                         goto repeat;
> > > > > +               }
> > > > >         } else
> > > > >                 goto plug_device;
> > > > >  out:
> > > > > 
> > > > > and not zero hwif->rq if the device is blocked. 
> > > > > 
> > > > > Could you try the attached patch and see if it fixes the issue?
> > > > 
> > > > Here is the more complete version, also taking into the account changes
> > > > in ide_intr() and ide_timer_expiry():
> > > > 
> > > 
> > > Sorry, I can not apply this patch directly, which original version did
> > > you use? I used the one in the master branch of linux-mips development
> > > git repository.
> > > 
> > > commit 5a4f13fad1ab5bd08dea78fc55321e429d83cddf
> > > Merge: ec9c45d e18ed14
> > > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > > Date:   Mon Jun 29 20:07:43 2009 -0700
> > > 
> > >     Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide-2.6
> > >     
> > >     * git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide-2.6:
> > >       ide: memory overrun in ide_get_identity_ioctl() on big endian
> > > machines using ioctl HDIO_OBSOLETE_IDENTITY
> > >       ide: fix resume for CONFIG_BLK_DEV_IDEACPI=y
> > >       ide-cd: handle fragmented packet commands gracefully
> > >       ide: always kill the whole request on error
> > >       ide: fix ide_kill_rq() for special ide-{floppy,tape} driver
> > > requests
> > > 
> > > it this too old? should i merge another git repository?
> > 
> > Weird, I used linux-next but Linus' tree should also be fine
> > (as it matches linux-next w.r.t. ide currently).
> 
> I just cloned the linux-next git repo, and tested your patch with
> STD/Hibernation, unfortunately, it also not work :-(
> 
> here is the Call Trace:
> 
> blk_delete_timer+0x0/0x20
> blk_requeue_request+0x24/0xd0
> ide_requeue_and_plug+0x38/0xb0
> ide_intr+0x120/0x300             --->  ide_intr....
> handle_IRQ_event+0x94/0x230
> handle_level_irq+0x7c/0x120
> mach_irq_dispatch+0xc8/0x158
> ret_from_irq+0x0/0x4
> cpu_idle+0x30/0x60
> start_kernel+0x330/0x34c
> 
> If _NOT_ apply your patch and comment this part, it works:

OK, I see another gotcha added by recent changes, we need to explicitly
initialize rq_in_flight variables now.  Revised patch below..

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ide: make resume work again (for real)

It turns out that commit a1317f714af7aed60ddc182d0122477cbe36ee9b
("ide: improve handling of Power Management requests") needs to take
into the account a new code added by the recent block layer changes
in commit 8f6205cd572fece673da0255d74843680f67f879 ("ide: dequeue
in-flight request") and prevent clearing of hwif->rq if the device
is blocked.

Thanks to Etienne, Wu and Jeff for help in fixing the issue.

Reported-and-tested-by: Jeff Chua <jeff.chua.linux@gmail.com>
Reported-and-tested-by: Etienne Basset <etienne.basset@numericable.fr>
Reported-by: Wu Zhangjin <wuzhangjin@gmail.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
Added patch description, no other changes.

 drivers/ide/ide-io.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -532,7 +532,8 @@ repeat:
 
 		if (startstop == ide_stopped) {
 			rq = hwif->rq;
-			hwif->rq = NULL;
+			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0)
+				hwif->rq = NULL;
 			goto repeat;
 		}
 	} else
@@ -616,7 +617,7 @@ void ide_timer_expiry (unsigned long dat
 	unsigned long	flags;
 	int		wait = -1;
 	int		plug_device = 0;
-	struct request	*uninitialized_var(rq_in_flight);
+	struct request	*rq_in_flight = NULL;
 
 	spin_lock_irqsave(&hwif->lock, flags);
 
@@ -679,8 +680,10 @@ void ide_timer_expiry (unsigned long dat
 		spin_lock_irq(&hwif->lock);
 		enable_irq(hwif->irq);
 		if (startstop == ide_stopped && hwif->polling == 0) {
-			rq_in_flight = hwif->rq;
-			hwif->rq = NULL;
+			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
+				rq_in_flight = hwif->rq;
+				hwif->rq = NULL;
+			}
 			ide_unlock_port(hwif);
 			plug_device = 1;
 		}
@@ -775,7 +778,7 @@ irqreturn_t ide_intr (int irq, void *dev
 	ide_startstop_t startstop;
 	irqreturn_t irq_ret = IRQ_NONE;
 	int plug_device = 0;
-	struct request *uninitialized_var(rq_in_flight);
+	struct request *rq_in_flight = NULL;
 
 	if (host->host_flags & IDE_HFLAG_SERIALIZE) {
 		if (hwif != host->cur_port)
@@ -856,8 +859,10 @@ irqreturn_t ide_intr (int irq, void *dev
 	 */
 	if (startstop == ide_stopped && hwif->polling == 0) {
 		BUG_ON(hwif->handler);
-		rq_in_flight = hwif->rq;
-		hwif->rq = NULL;
+		if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
+			rq_in_flight = hwif->rq;
+			hwif->rq = NULL;
+		}
 		ide_unlock_port(hwif);
 		plug_device = 1;
 	}

Re: [Bug #13663] suspend to ram regression (IDE related)

[Wu Zhangjin]

Hi,

> OK, I see another gotcha added by recent changes, we need to explicitly
> initialize rq_in_flight variables now.  Revised patch below..
> 

Sorry, STD also not work. if apply this patch, the same problem as not
apply it, it stopped at:

...
PM: Crete hibernation image:
PM: Need to copy ... pages
PM: Hibernation image created ...

I think it's better to revert this commit:
 a1317f714af7aed60ddc182d0122477cbe36ee9b ("ide: improve handling of
Power Management requests")

Regards,
Wu Zhangjin

> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH] ide: make resume work again (for real)
> 
> It turns out that commit a1317f714af7aed60ddc182d0122477cbe36ee9b
> ("ide: improve handling of Power Management requests") needs to take
> into the account a new code added by the recent block layer changes
> in commit 8f6205cd572fece673da0255d74843680f67f879 ("ide: dequeue
> in-flight request") and prevent clearing of hwif->rq if the device
> is blocked.
> 
> Thanks to Etienne, Wu and Jeff for help in fixing the issue.
> 
> Reported-and-tested-by: Jeff Chua <jeff.chua.linux@gmail.com>
> Reported-and-tested-by: Etienne Basset <etienne.basset@numericable.fr>
> Reported-by: Wu Zhangjin <wuzhangjin@gmail.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
> Added patch description, no other changes.
> 
>  drivers/ide/ide-io.c |   19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> Index: b/drivers/ide/ide-io.c
> ===================================================================
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -532,7 +532,8 @@ repeat:
>  
>  		if (startstop == ide_stopped) {
>  			rq = hwif->rq;
> -			hwif->rq = NULL;
> +			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0)
> +				hwif->rq = NULL;
>  			goto repeat;
>  		}
>  	} else
> @@ -616,7 +617,7 @@ void ide_timer_expiry (unsigned long dat
>  	unsigned long	flags;
>  	int		wait = -1;
>  	int		plug_device = 0;
> -	struct request	*uninitialized_var(rq_in_flight);
> +	struct request	*rq_in_flight = NULL;
>  
>  	spin_lock_irqsave(&hwif->lock, flags);
>  
> @@ -679,8 +680,10 @@ void ide_timer_expiry (unsigned long dat
>  		spin_lock_irq(&hwif->lock);
>  		enable_irq(hwif->irq);
>  		if (startstop == ide_stopped && hwif->polling == 0) {
> -			rq_in_flight = hwif->rq;
> -			hwif->rq = NULL;
> +			if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
> +				rq_in_flight = hwif->rq;
> +				hwif->rq = NULL;
> +			}
>  			ide_unlock_port(hwif);
>  			plug_device = 1;
>  		}
> @@ -775,7 +778,7 @@ irqreturn_t ide_intr (int irq, void *dev
>  	ide_startstop_t startstop;
>  	irqreturn_t irq_ret = IRQ_NONE;
>  	int plug_device = 0;
> -	struct request *uninitialized_var(rq_in_flight);
> +	struct request *rq_in_flight = NULL;
>  
>  	if (host->host_flags & IDE_HFLAG_SERIALIZE) {
>  		if (hwif != host->cur_port)
> @@ -856,8 +859,10 @@ irqreturn_t ide_intr (int irq, void *dev
>  	 */
>  	if (startstop == ide_stopped && hwif->polling == 0) {
>  		BUG_ON(hwif->handler);
> -		rq_in_flight = hwif->rq;
> -		hwif->rq = NULL;
> +		if ((drive->dev_flags & IDE_DFLAG_BLOCKED) == 0) {
> +			rq_in_flight = hwif->rq;
> +			hwif->rq = NULL;
> +		}
>  		ide_unlock_port(hwif);
>  		plug_device = 1;
>  	}

Re: [Bug #13663] suspend to ram regression (IDE related)

[Bartlomiej Zolnierkiewicz]

On Friday 03 July 2009 17:31:36 Wu Zhangjin wrote:
> Hi,
> 
> > OK, I see another gotcha added by recent changes, we need to explicitly
> > initialize rq_in_flight variables now.  Revised patch below..
> > 
> 
> Sorry, STD also not work. if apply this patch, the same problem as not
> apply it, it stopped at:
> 
> ...
> PM: Crete hibernation image:
> PM: Need to copy ... pages
> PM: Hibernation image created ...
> 
> I think it's better to revert this commit:
>  a1317f714af7aed60ddc182d0122477cbe36ee9b ("ide: improve handling of
> Power Management requests")
 
I completely agree and I've already requested this a week ago
(this commit was not meant for going straight to -rc tree anyway).

Re: [Bug #13663] suspend to ram regression (IDE related)

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Mon, 6 Jul 2009 16:57:59 +0200

>> I think it's better to revert this commit:
>>  a1317f714af7aed60ddc182d0122477cbe36ee9b ("ide: improve handling of
>> Power Management requests")
>  
> I completely agree and I've already requested this a week ago
> (this commit was not meant for going straight to -rc tree anyway).

I'll revert this today and push that to Linus.

[Bug 13663] New: suspend to ram regression (IDE related)

[bugzilla-daemon]

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

           Summary: suspend to ram regression (IDE related)
           Product: IO/Storage
           Version: 2.5
    Kernel Version: 2.6.30
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: IDE
        AssignedTo: io_ide@kernel-bugs.osdl.org
        ReportedBy: rjw@sisk.pl
            Blocks: 7216,13070
        Regression: Yes


Subject    : [REGRESSION] 2.6.29=>2.6.30 suspend to ram regression (IDE
related)
Submitter  : Etienne Basset <etienne.basset@numericable.fr>
Notify-Also : Jeff Chua <jeff.chua.linux@gmail.com>
Date       : 2009-06-26 17:40
References : http://lkml.org/lkml/2009/6/26/242
Handled-By : Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Patch      : http://patchwork.kernel.org/patch/32719/

This entry is being used for tracking a regression from 2.6.29.  Please don't
close it until the problem is fixed in the mainline.

-- 
Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

[Bug 13663] suspend to ram regression (IDE related)

[bugzilla-daemon]

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


Rafael J. Wysocki <rjw@sisk.pl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |PATCH_ALREADY_AVAILABLE




-- 
Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

[Bug 13663] suspend to ram regression (IDE related)

[bugzilla-daemon]

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





--- Comment #1 from Rafael J. Wysocki <rjw@sisk.pl>  2009-06-29 23:25:03 ---
References : http://lkml.org/lkml/2009/6/29/126

-- 
Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

[Bug 13663] suspend to ram regression (IDE related)

[bugzilla-daemon]

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


Rafael J. Wysocki <rjw@sisk.pl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED
         Resolution|PATCH_ALREADY_AVAILABLE     |CODE_FIX




--- Comment #2 from Rafael J. Wysocki <rjw@sisk.pl>  2009-07-07 20:32:02 ---
Fixed by commit 2bf427b25b79eb7cea27963a66c3d4684cae0e0c .

-- 
Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.