Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933818AbcDSSsG (ORCPT ); Tue, 19 Apr 2016 14:48:06 -0400 Received: from mga04.intel.com ([192.55.52.120]:60777 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932168AbcDSSsE (ORCPT ); Tue, 19 Apr 2016 14:48:04 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,506,1455004800"; d="scan'208";a="688887254" Date: Tue, 19 Apr 2016 21:47:58 +0300 From: Jarkko Sakkinen To: Jason Gunthorpe Cc: Peter Huewe , linux-security-module@vger.kernel.org, stable@vger.kernel.org, Marcel Selhorst , "moderated list:TPM DEVICE DRIVER" , open list Subject: Re: [PATCH] tpm_crb: fix mapping of the buffers Message-ID: <20160419184758.GA13115@intel.com> References: <1461059658-8884-1-git-send-email-jarkko.sakkinen@linux.intel.com> <20160419170953.GB20844@obsidianresearch.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160419170953.GB20844@obsidianresearch.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1573 Lines: 54 On Tue, Apr 19, 2016 at 11:09:53AM -0600, Jason Gunthorpe wrote: > On Tue, Apr 19, 2016 at 12:54:18PM +0300, Jarkko Sakkinen wrote: > > Cc: stable@vger.kernel.org > > Fixes: 1bd047be37d9 ("tpm_crb: Use devm_ioremap_resource") > > Signed-off-by: Jarkko Sakkinen > > drivers/char/tpm/tpm_crb.c | 39 ++++++++++++++++++++++++++++----------- > > 1 file changed, 28 insertions(+), 11 deletions(-) > > This looks OK > > Reviewed-by: Jason Gunthorpe Thanks! > > + if (cmd_pa != rsp_pa) { > > + priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size); > > + return PTR_ERR_OR_ZERO(priv->rsp); > > + } > > I would use an else here, 'exit on success' is considered an > anti-pattern. > Eg: > > if (cmd_pa == rsp_pa) { > /* According to the PTP specification, overlapping command and response > * buffer sizes must be identical. > */ > if (cmd_size != rsp_size) { > dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical"); > return -EINVAL; > } > priv->rsp = priv->cmd; > } > else { > priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size); > if (IS_ERR(priv->rsp)) > return PTR_ERR(priv->rsp); > } > > return 0; I have to (in order to do right things right): 1. Update the patch. 2. Smoke test with the machines that I have. 3. Send a new version for review (because of the revamped control flow). It's not that I wouldn't be willing to do this. I just don't think this matters enough to be worth of the trouble. > Jason /Jarkko