Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756426AbbLARf6 (ORCPT ); Tue, 1 Dec 2015 12:35:58 -0500 Received: from quartz.orcorp.ca ([184.70.90.242]:59275 "EHLO quartz.orcorp.ca" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755471AbbLARf5 (ORCPT ); Tue, 1 Dec 2015 12:35:57 -0500 Date: Tue, 1 Dec 2015 10:35:49 -0700 From: Jason Gunthorpe To: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= Cc: Jarkko Sakkinen , tpmdd-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, Martin Wilck , Peter Huewe Subject: Re: [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter Message-ID: <20151201173549.GB691@obsidianresearch.com> References: <1448911632-20070-1-git-send-email-jgunthorpe@obsidianresearch.com> <1448911632-20070-3-git-send-email-jgunthorpe@obsidianresearch.com> <20151201072835.GQ10431@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20151201072835.GQ10431@pengutronix.de> User-Agent: Mutt/1.5.23 (2014-03-12) X-Broken-Reverse-DNS: no host name found for IP address 10.0.0.160 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2626 Lines: 76 On Tue, Dec 01, 2015 at 08:28:35AM +0100, Uwe Kleine-K?nig wrote: > On Mon, Nov 30, 2015 at 12:27:12PM -0700, Jason Gunthorpe wrote: > > The TPM core has long assumed that every device has a driver attached, > > however b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are > > called unconditionally") breaks that assumption. > > Maybe it's worth to point out that b8b2c7d845d5 didn't break it on > purpose and is fixed accordingly. Still the assumption isn't valid, but > works in practise. Purposeful or not, it is the source of the bug this patch is fixing.. I'm happy with any language, proposal? > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (res == NULL) { > > indention problems here. Woops, forgot to run check patch.. > > + if (res) > > + tpm_info.irq = res->start; > > + else { > > If one branch of an if/else has braces, all of them should. Is that a thing now? Surprised checkpatch doesn't complain. > > static bool force; > > +#ifdef CONFIG_X86 > > module_param(force, bool, 0444); > > MODULE_PARM_DESC(force, "Force device probe rather than using ACPI entry"); > > +#endif > > Is this added ifdef intended to be in this commit? Yes, upon auditing all this it is clear the values we have are hard-wired to x86, so this will never work on another platform. I'm happy to put that in another patch. > > -err_init: > > - platform_device_unregister(pdev); > > -err_dev: > > - platform_driver_unregister(&tis_drv); > > +out4: > > +#ifdef CONFIG_ACPI > > + acpi_bus_unregister_driver(&tis_acpi_driver); > > +out3: > > +#endif > > + pnp_unregister_driver(&tis_pnp_driver); > > +out2: > > + platform_device_unregister(force_pdev); > > +out1: > > Might be a matter of taste, but having nicer names for the error labels > makes review easier. For example I would have called "out3" > "err_register_acpi" instead. Then you can easily verify that it's placed > right in the error path being directly after > acpi_bus_unregister_driver. The downside is it is harder to review the goto sites because there is no longer much logic to their ordering, but sure, names seem a bit more common in tpm. > Also all kind of strange things happen if you later need to add a label > between out2 and out3. drivers/scsi/hpsa.c for example used "clean2_5" > in a similar situation. Yes, it isn't so bad to do that. Jason -- 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/