Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753936AbcJICCF (ORCPT ); Sat, 8 Oct 2016 22:02:05 -0400 Received: from quartz.orcorp.ca ([184.70.90.242]:55418 "EHLO quartz.orcorp.ca" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752903AbcJICCE (ORCPT ); Sat, 8 Oct 2016 22:02:04 -0400 Date: Sat, 8 Oct 2016 19:42:56 -0600 From: Jason Gunthorpe To: Jarkko Sakkinen Cc: Peter Huewe , Marcel Selhorst , "moderated list:TPM DEVICE DRIVER" , open list Subject: Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs Message-ID: <20161009014256.GA8210@obsidianresearch.com> References: <1475972112-2819-1-git-send-email-jarkko.sakkinen@linux.intel.com> <1475972112-2819-2-git-send-email-jarkko.sakkinen@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1475972112-2819-2-git-send-email-jarkko.sakkinen@linux.intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Broken-Reverse-DNS: no host name found for IP address 10.0.0.151 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1333 Lines: 35 On Sun, Oct 09, 2016 at 03:15:09AM +0300, Jarkko Sakkinen wrote: > + ctrl = crb_map_res(dev, priv, &io_res, buf->control_address, > + sizeof(struct crb_regs) - > + offsetof(struct crb_regs, ctrl_req)); > + if (IS_ERR(ctrl)) > + return PTR_ERR(ctrl); > + > + /* The control area always overrlaps IO memory mapped from the ACPI > + * object with CRB start only devices. Thus, this is perfectly safe. > + */ > + priv->regs = (void *)((unsigned long)ctrl - > + offsetof(struct crb_regs, ctrl_req)); Hum. No, this makes bad assumptions about the structure of iomapping. The map itself needs to be done with the adjustment: ctrl = crb_map_res(dev, priv, &io_res, buf->control_address - offsetof(struct crb_regs, ctrl_req), sizeof(struct crb_regs)); .. and nothing actually proves that control_address follows anything in the driver, so this seems like a terrifying blind assumption, but everything about the iomap in this ACPI binding seems totally bonkers so that is in good company I guess. .. and the comment says this only holds for 'crb start only' devices, but the code doesn't actually act differently based on what sort of device we have.. Your commit message also seems to imply the new registers are only on newer hardware, but nothing seems to check for that before acessing them? Confusing. Jason