From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751296AbdAQT1m (ORCPT ); Tue, 17 Jan 2017 14:27:42 -0500 Received: from quartz.orcorp.ca ([184.70.90.242]:39782 "EHLO quartz.orcorp.ca" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751166AbdAQT1l (ORCPT ); Tue, 17 Jan 2017 14:27:41 -0500 Date: Tue, 17 Jan 2017 12:27:28 -0700 From: Jason Gunthorpe To: Andrey Pronin Cc: Peter Huewe , Marcel Selhorst , Jarkko Sakkinen , tpmdd-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, semenzato@chromium.org, groeck@chromium.org Subject: Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown Message-ID: <20170117192728.GF27528@obsidianresearch.com> References: <20170114000954.17728-1-apronin@chromium.org> <20170114002857.GA5851@obsidianresearch.com> <20170114004230.GA21035@apronin> <20170116161919.GA20238@obsidianresearch.com> <20170117175827.GA124090@apronin> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170117175827.GA124090@apronin> User-Agent: Mutt/1.5.24 (2015-08-30) X-Broken-Reverse-DNS: no host name found for IP address 10.0.0.156 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 17, 2017 at 09:58:27AM -0800, Andrey Pronin wrote: > > Yes, sorry, I should have mentioned that.. Maybe that is too much to > > fix.. > > If we fix sysfs to go through tpm_try_get_ops, then all we can do for > shutdown is indeed something like Maybe yes, I also had at one point a thought to push the read side of the ops_sem all the way down to the transmit_cmd level... But that complicates calling shutdown. > down_write(&chip->ops_sem); > if (chip->ops && chip->flags & TPM_CHIP_FLAG_TPM2) > tpm2_shutdown(chip, TPM2_SU_CLEAR); > chip->ops = NULL; > up_write(&chip->ops_sem); > > Does that sound like a good plan? > If we don't want sysfs to increment/decrement the reference count for > the device, we can still make it go through Grabbing the extra kref is harmless.. > > I'm confused - doesn't your system reset the TPM when it reboots? > > Isn't that required so the firmware starts with known PCRs? Doesn't > > reset trump unorderly shutdown? > > > > That's right, the TPM is reset when the system reboots. However, for > TPM 2.0, if it resets w/o Shutdown(CLEAR) first, it will detect it > during Startup, and mark as unorderly shutdown. Shutdown(CLEAR) is > the signal to the TPM to save its state to nvram and prepare to reset. > If it was not done, it is possible that something was not saved (e.g. > the DA counter), and the chip correctly marks it as a potential DA > problem. Okay, that makes sense, and needs to go in a comment someplace! > > > All these things are handled by tpm_chip_unregister(). I thought about > > > creating a tpm_chip_shutdown routine that could be called from shutdown > > > handlers of the drivers that need it (and I'd do it for every driver, > > > especially in 2.0 case). But decided that it's better to reuse the > > > existing tpm_chip_unregister() that already does what's needed. > > > > If for some reason we need this for every driver then this is probably > > a better approach - but that seems very, very strange to me. > > As described above, we can do a smaller tpm_chip_shutdown() that the > drivers that need it (2.0 or susceptible to issues if reset in the > middle of command) can call. > I'll do it, if it sounds like the right plan to you. Yes please.. Is there some way we can have the TPM core do this without requiring the driver to add a shutdown the struct driver? Maybe we could put something in chip->dev->driver? Not sure.. Jason