From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751152AbdAPJd3 (ORCPT ); Mon, 16 Jan 2017 04:33:29 -0500 Received: from mga03.intel.com ([134.134.136.65]:34062 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750914AbdAPJd1 (ORCPT ); Mon, 16 Jan 2017 04:33:27 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,238,1477983600"; d="scan'208";a="1094652757" Date: Mon, 16 Jan 2017 11:33:18 +0200 From: Jarkko Sakkinen To: Andrey Pronin Cc: Jason Gunthorpe , Peter Huewe , Marcel Selhorst , 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: <20170116093318.wmmr2z54pc6k7t2c@intel.com> References: <20170114000954.17728-1-apronin@chromium.org> <20170114002857.GA5851@obsidianresearch.com> <20170114004230.GA21035@apronin> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170114004230.GA21035@apronin> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.6.2-neo (2016-08-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 13, 2017 at 04:42:30PM -0800, Andrey Pronin wrote: > On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote: > > On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote: > > > Resetting TPM while processing a command may lead to issues > > > on the next boot. Ensure that we don't have any ongoing > > > commands, and that no further commands can be sent to the chip > > > by unregistering the device in the shutdown handler. > > > tpm_chip_unregister() waits for the completion of an ongoing > > > command, if any, and then clears out chip->ops and unregisters > > > sysfs entities. > > > > Unregistering in a shutdown handler seems very strange, it also waits > > for userspace things, so I wonder if it could be problematic? > > > > Maybe just use > > > > down_write(&chip->ops_sem); > > chip->ops = NULL; > > up_write(&chip->ops_sem); > > > > In the shutdown handler? > > down_write(&chip->ops_sem) would still wait for completing the initiated > writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops(). > Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be > unregistered first. Why don't you fix the tpm-sysfs issue but rather misusing tpm_chip_unregister? /Jarkko