linux-kernel - Re: [REGRESION] Suspend hangs with 3.6-rc1 on Lenovo T60 notebook

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87obmals06.fsf@tucsk.pomaz.szeredi.hu>
Date:	Thu, 16 Aug 2012 18:29:45 +0200
From:	Miklos Szeredi <miklos@...redi.hu>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	Hans de Goede <hdegoede@...hat.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Tejun Heo <tj@...nel.org>, <linux-kernel@...r.kernel.org>,
	<linux-pm@...r.kernel.org>
Subject: Re: [REGRESION] Suspend hangs with 3.6-rc1 on Lenovo T60 notebook

Alan Stern <stern@...land.harvard.edu> writes:

> On Thu, 16 Aug 2012, Hans de Goede wrote:
>
>> Ah right, these are bus_driver operations. That explains some things, so I've
>> done some more research asking myself: "Why does generic_ide_suspend(), which
>> is a *bus* op, call dev_get_drvdata?", the answer to that seems to be that
>> the ide subsystem is abusing (IMHO) drvdata to store per device bus_driver
>> data. Which I believe is not how drvdata is intended to be used.
>> 
>> With that said, the above knowledge has allowed me to write an (ugly) fix for
>> the regression Miklos is seeing. Miklos can you give the attached patch a
>> try please?
>> 
>> > It clearly should check if drive is not NULL before using that pointer.
>> 
>> I assume you mean drive*r*, yes I agree that generic_ide_remove should
>> check for that. So who is going to write a patch for that?
>
> The existing code could certainly be improved.  Your patch does:
>
>> +               /*
>> +                * device_register() will have cleared drvdata on
>> +                * device_attach failure, but we use drvdata to store per
>> +                * device bus info, rather then for driver info, so restore it.
>> +                */
>> +               dev_set_drvdata(dev, drive);
>
> But at this point, dev is defined by:
>
> 		struct device *dev = &drive->gendev;
>
> So why bother setting anything?  It seems to me that 
> generic_ide_suspend() and generic_ide_resume() could easily replace
>
> 	ide_drive_t *drive = dev_get_drvdata(dev);
>
> with
>
> 	ide_drive_t *drive = dev_to_ide_drive(dev);
>
> where dev_to_ide_drive is defined as "container_of(dev, ide_drive_t,
> gendev)" (if this isn't defined already).

Yes, this appears to work.  Following patch fixes the suspend oops.

Thanks,
Miklos

---
 drivers/ide/ide-pm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ide/ide-pm.c b/drivers/ide/ide-pm.c
index 9240609..8d1e32d 100644
--- a/drivers/ide/ide-pm.c
+++ b/drivers/ide/ide-pm.c
@@ -4,7 +4,7 @@
 
 int generic_ide_suspend(struct device *dev, pm_message_t mesg)
 {
-	ide_drive_t *drive = dev_get_drvdata(dev);
+	ide_drive_t *drive = to_ide_device(dev);
 	ide_drive_t *pair = ide_get_pair_dev(drive);
 	ide_hwif_t *hwif = drive->hwif;
 	struct request *rq;
@@ -40,7 +40,7 @@ int generic_ide_suspend(struct device *dev, pm_message_t mesg)
 
 int generic_ide_resume(struct device *dev)
 {
-	ide_drive_t *drive = dev_get_drvdata(dev);
+	ide_drive_t *drive = to_ide_device(dev);
 	ide_drive_t *pair = ide_get_pair_dev(drive);
 	ide_hwif_t *hwif = drive->hwif;
 	struct request *rq;
-- 
1.7.7

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/