Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933008Ab2HPQ2r (ORCPT ); Thu, 16 Aug 2012 12:28:47 -0400 Received: from mail-wi0-f172.google.com ([209.85.212.172]:61577 "EHLO mail-wi0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932846Ab2HPQ2o (ORCPT ); Thu, 16 Aug 2012 12:28:44 -0400 From: Miklos Szeredi To: Alan Stern Cc: Hans de Goede , "Rafael J. Wysocki" , Greg Kroah-Hartman , Tejun Heo , , Subject: Re: [REGRESION] Suspend hangs with 3.6-rc1 on Lenovo T60 notebook References: Date: Thu, 16 Aug 2012 18:29:45 +0200 In-Reply-To: (Alan Stern's message of "Thu, 16 Aug 2012 11:13:33 -0400 (EDT)") Message-ID: <87obmals06.fsf@tucsk.pomaz.szeredi.hu> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2876 Lines: 83 Alan Stern 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/