From patchwork Thu Aug 16 16:29:45 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Miklos Szeredi X-Patchwork-Id: 1334421 Return-Path: X-Original-To: patchwork-linux-pm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id B4D5240210 for ; Thu, 16 Aug 2012 16:28:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932942Ab2HPQ2q (ORCPT ); Thu, 16 Aug 2012 12:28:46 -0400 Received: from mail-we0-f174.google.com ([74.125.82.174]:57854 "EHLO mail-we0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932897Ab2HPQ2o (ORCPT ); Thu, 16 Aug 2012 12:28:44 -0400 Received: by weyx8 with SMTP id x8so1858363wey.19 for ; Thu, 16 Aug 2012 09:28:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=szeredi.hu; s=google; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version:content-type; bh=oIsmAxMijvqW+MEvO/GTmV4pQoLQSPOrhXvEwafNyd4=; b=XnRP4WtFYiy9GsUBrI8UKtBwvL99TX919/mYa8NXSCBQRyWhO+YEuucFdtKh0SxcnT BCGlr2UidNJpO8Nmu7CyCJL2YgA6TayM5tKIz5Qgcm3BXttXa/b0L65tmByjBwsrGdkl 6eaagX4NQfMMNvmK7V6iiP4lW9vQgmZCA3adk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version:content-type:x-gm-message-state; bh=oIsmAxMijvqW+MEvO/GTmV4pQoLQSPOrhXvEwafNyd4=; b=AfjAXgttJwo10gmC0uDQZdFbSF67Cdf+o3FCwi7+HsbsNXZQIh3YTHi9AhYuG8Zu72 PhoYJf6WReuei7vF9fO00Gqqkf67317iO0RXgFskr8Uy2L9f1BQUBQpqmgBA2PcYfzyS zN5MmD/4S0oRm0Dq6BwjVOMhJB519VGyHnROEJBD1YbUtP7Xa7Wgs2jVU3VxetjQP/Yy Auehuh5kqrJdPGiy9EY+GA+TQhQ6XaRwLdGi6WLqbgL+q42xitKJjZi38iOnU1k2CKui e7dcq8fYV4cpJyoy8q+BrdJFvZBnj3DAwuDlhxoTyk4xprbPUv7oH1hk5cVErSjqic7Y 1CFA== Received: by 10.180.98.200 with SMTP id ek8mr4476639wib.0.1345134502361; Thu, 16 Aug 2012 09:28:22 -0700 (PDT) Received: from tucsk.pomaz.szeredi.hu (92-249-155-223.pool.digikabel.hu. [92.249.155.223]) by mx.google.com with ESMTPS id bc2sm6934294wib.0.2012.08.16.09.28.17 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 16 Aug 2012 09:28:21 -0700 (PDT) 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 X-Gm-Message-State: ALoCoQm5erQ26IEd4PtG1bDhLP7W9Ga+Jh7A33eoGB2H1hfAe9M3rAJlFz7JtXuwhX90Hn91nruI Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org 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;