Message ID | 20221017235732.10145-9-LinoSanfilippo@gmx.de |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp1698295wrs; Mon, 17 Oct 2022 17:04:53 -0700 (PDT) X-Google-Smtp-Source: AMsMyM68BmWCGqvxVPkeszQDBqyd+T7njj6t7SeHjUKh2qF36bqcrPxDTUjbttKtMaXf/jHmEJ9t X-Received: by 2002:a17:906:58d2:b0:78d:9d2f:3005 with SMTP id e18-20020a17090658d200b0078d9d2f3005mr162085ejs.697.1666051493187; Mon, 17 Oct 2022 17:04:53 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666051493; cv=none; d=google.com; s=arc-20160816; b=ygzCs5LdUECRsN+9z11BHeZ4SzhoclqU+KYX0vW/C8+lNO8A2CPXEQCxW4mIoG2Mro YG47/uuxgYhjZXoTbU6EywVnd2YKshmTDZxIioTzyzhBNKv+QtTsrAKct1OF8C3sGJUM +MgG0BAfAW7AqEmG3fJs09PjDdiBkBGK3C6LzMNaw5T3TBP6wWtBV6DJLnS5Oc/i3bn9 w8vbOagl6PuXdhSQlSd5MM3+E9bSLZTlAZxmj1ywtuvxI0d2OTfwJdZ6DKJ7UyJS4vwD MxpSwYMBYP0nq3PKEDSfYa9B5JimRiR6kIzXuj+bue53yEBRd6hCrYp29Ub/q0OdQgbv Dh7A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=juUo90/80vPHuf7Br7pPmfpOCEHFG9Rm2lnetqnzIHM=; b=tBWc9qa5nqy0p27+gVlUCsouGVyJ0nYP5ZeU+8AOMrGI+d8GG7dI3g3ZOE3SUdbs/l EruMyR/P/f4buopD7QPZ2mgSuCubrTEs9H/ABZJb3VeN+P7U8hgL96N3NzO/61u8E1fy D+8CBiGeChhSZM1u7AUuVn+qtMFS5kE0ox7zeYT5fcPIVOD2xsoX+vfSaGS+7c/wfZ3v awMHCAZ9slo6kRm4++gAm9iDmH8q81Ha68gmAKVcmXSwLoLsk8eJVYNBf1o5zeZnTr1+ oyWEZ49nXe15R1/zSVhgMpb0a0GmrQEU+GVU7g2snzK/HgA1tv3I7Fl9Ag+2Cr4wUU9C ug6Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmx.net header.s=badeba3b8450 header.b=fnPIkL8W; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=gmx.de Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id en16-20020a056402529000b00457166171c6si8843760edb.432.2022.10.17.17.04.28; Mon, 17 Oct 2022 17:04:53 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@gmx.net header.s=badeba3b8450 header.b=fnPIkL8W; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=gmx.de Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231381AbiJQX6y (ORCPT <rfc822;kernel.ruili@gmail.com> + 99 others); Mon, 17 Oct 2022 19:58:54 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38962 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230527AbiJQX6U (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 17 Oct 2022 19:58:20 -0400 Received: from mout.gmx.net (mout.gmx.net [212.227.17.21]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E48768680E; Mon, 17 Oct 2022 16:58:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=badeba3b8450; t=1666051073; bh=juUo90/80vPHuf7Br7pPmfpOCEHFG9Rm2lnetqnzIHM=; h=X-UI-Sender-Class:From:To:Cc:Subject:Date:In-Reply-To:References; b=fnPIkL8W4M5bNzWcyi9a75ql9//2R5Qy5TTlZIAT+iB8WFEVVuSSmN1bSc1RtN0uY LhMTzPVAdreBkT0DkwGUWoLcGL/JFoUSTjPSLgSh0djG4HqgFFJFvcppL3qhatMWaa uXuzZuPfEHACEPBWbUEYr8maZL5rmCW/s3yzS7iQ= X-UI-Sender-Class: 01bb95c1-4bf8-414a-932a-4f6e2808ef9c Received: from Venus.speedport.ip ([84.162.5.241]) by mail.gmx.net (mrgmx105 [212.227.17.168]) with ESMTPSA (Nemesis) id 1MvbG2-1p1Q9e4Ays-00shHM; Tue, 18 Oct 2022 01:57:53 +0200 From: Lino Sanfilippo <LinoSanfilippo@gmx.de> To: peterhuewe@gmx.de, jarkko@kernel.org, jgg@ziepe.ca Cc: stefanb@linux.vnet.ibm.com, linux@mniewoehner.de, linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org, jandryuk@gmail.com, pmenzel@molgen.mpg.de, l.sanfilippo@kunbus.com, LinoSanfilippo@gmx.de, lukas@wunner.de, p.rosenberger@kunbus.com Subject: [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality Date: Tue, 18 Oct 2022 01:57:29 +0200 Message-Id: <20221017235732.10145-9-LinoSanfilippo@gmx.de> X-Mailer: git-send-email 2.36.1 In-Reply-To: <20221017235732.10145-1-LinoSanfilippo@gmx.de> References: <20221017235732.10145-1-LinoSanfilippo@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: base64 X-Provags-ID: V03:K1:pfbvcY5/MnkRaWRBLgtPcwmK2mbOel1Z3UYYqdvt3JndJu+H5cD FAoaoS0SutEuP3LCLzCNhIOkJtF2DEO6oBIoSgP+tedMAPiM0R0a84B8sVXdmgiAByjQhYy soeoP6jRc04xmrcqLujVilMFrF+HDBq/Zi5Pgz1VLTN4xzRp7Fyrd/auP6q5p9R+06L8F5R sSRpMRdC1xzQtND7q8QqQ== X-UI-Out-Filterresults: notjunk:1;V03:K0:JnTBYZ/7PB4=:KZYDysI51Ry1uIXk85o4bG 58lGLrCGw5YUPuWOMFmXUNEAlYfAyGW+4y0wnOuLnyzROILtpRqQJGFLOfldiUzsoMBbwY+sc IshNotdBUUevOyscDEvq6fpa4gEC+I70ZZ4sfCMEmUAKlfcM0erXnL4t3nw/6yKTWOH+QuPRd kLJIyG9aUFdkhRqlR2yWuRgOh2OqU7JW+e0Ws82sqHXnTzgQmlBagvidatEr6xMNO7/JAVkQp vvufnGAN432tY9nmqGchlHUHG9yDJ2ExPVH59OL9lkHC1q8G9t9L2epnE9zkbOelZQlrm3yWM d/q/wJvkapck+9f8lSXkkyX36L29vfQAgnl5UzQGWEdxuqANy4yYKA6QGOtAZtIB1hZQy+v35 iJO+3s/NESqK8B1yXwe7tT95Zkv87YPZ1ak72yNgzQ2nUC+nSaIeNMXxXZ5P6ZAe3I9KGjtsT cQldjasaVHUF0ou73OiPorxH6T3iq/R1lwUPrI54RcEx+TJ9zuSTGLsVqMGxTroeKAyHxSUFi jpwaV2qPDGgew21RFvbsRkOdwd7+ERHKsP9GTrdrnUAKTmIVryq96IrICXHbKPpG17BGDPWCM 95qU8pBAdTaB7DNXKhXNnmv9wEvUUcgAemGhYZCuLj5yN8lyLjFb+dA1UGRoJakOixRajkwWK uGcqN7QuM6iwMxxG/vJ5z3lRNdy0yFsIheA7B8y99YBCg+YThvNZhsivQGM3hvK3vikUHmqM7 lBlZ/r040/qm38vyOp4638/WoFR72kxkHo4dWoYOdE2tKLMS/TCZYQZUyWgQ3fma+cbIWm5UU FY0l8/xw+RkyQt4zQHvYtLwp6nwDWyAk/pAhwOohx4gPw419UfjwSnYOele/xrUCgrEvZA0lZ s5esI/w4tCmBuW/rKWBMktrbHfM6IdPF3DM/ci3FXTTGkoKozhGOLAgZOEgEXnLPOeMmIGPZj rATlNmgTLuvyPe6jW0/X617a+vlkZOnw3QkuJz5wAkRc6vrERw6QYgE73wYRAfhR3nwLxuolx zAGXFm2o+ggOaohgTuvyqOTXNxv4PAY5ZmbES7gDeFxyBhSa8W0/apBKC2hXDd7MzFwU46E+E KynRTv9hu8D6XOoyXhH4Qv9JImL6k214AJFGjKKRnVmUM0ZUMFkWo9haeM6iiy8ACpkaiOMHf ny4FVS9kmW/C3gpdgkHopdmFn1/fZYq5vuktEHP1a2o3RiUlHWZ9lXnD8fwSfRsK1dxky9LPA lGctsENdBn/Hy5QYIUSWTKdDjmVINVcWJRXI5ptBbWg0I7aw5ga6e3Xgnn0+LX6WNVUWPL8jb Q9hl6KxGaficrqsN63yz9KWf2mgHf8m656+Mzl3IO6HPt2ktyjxgPQATMUBzzeADcHOkPZekd JtbO525f9QKDCSM/c8hnSlO0syEkBqU8tBDp93MlOcyT+JwgIy7Xy24Ww0lDtJEIeOeyYAc3c EL0bUJMT9bNzKPWrP7/s3ndh8a27fGgrGV72540cxK4kaQtcvrvNn4Cf7b1jsgD14gY8tIT5e zZ0XvaYKFL1TPRy6BHRa7mFasUbRLAlzMGem5sPI9tP7U X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H2, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1746981610363460627?= X-GMAIL-MSGID: =?utf-8?q?1746981610363460627?= |
Series |
TPM IRQ fixes
|
|
Commit Message
Lino Sanfilippo
Oct. 17, 2022, 11:57 p.m. UTC
From: Lino Sanfilippo <l.sanfilippo@kunbus.com> Implement a usage counter for the (default) locality used by the TPM TIS driver: Request the locality from the TPM if it has not been claimed yet, otherwise only increment the counter. Also release the locality if the counter is 0 otherwise only decrement the counter. Ensure thread-safety by protecting the counter with a mutex. This allows to request and release the locality from a thread and the interrupt handler at the same time without the danger to interfere with each other. By doing this refactor the names of the amended functions to use the proper prefix. Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> Tested-by: Michael Niewöhner <linux@mniewoehner.de> --- drivers/char/tpm/tpm_tis_core.c | 75 ++++++++++++++++++++++----------- drivers/char/tpm/tpm_tis_core.h | 2 + 2 files changed, 53 insertions(+), 24 deletions(-)
Comments
On Tue, Oct 18, 2022 at 01:57:29AM +0200, Lino Sanfilippo wrote: > Implement a usage counter for the (default) locality used by the TPM TIS > driver: > Request the locality from the TPM if it has not been claimed yet, otherwise > only increment the counter. Also release the locality if the counter is 0 > otherwise only decrement the counter. Ensure thread-safety by protecting > the counter with a mutex. > > This allows to request and release the locality from a thread and the > interrupt handler at the same time without the danger to interfere with > each other. [...] > +static int tpm_tis_release_locality(struct tpm_chip *chip, int l) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > - tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > + mutex_lock(&priv->locality_count_mutex); > + priv->locality_count--; > + if (priv->locality_count == 0) > + tpm_tis_release_locality_locked(priv, l); > + mutex_unlock(&priv->locality_count_mutex); > > return 0; > } Hm, any reason not to use struct kref for the locality counter? Provides correct memory ordering (no mutex needed) and allows for calling a release function too upon reaching 0. Thanks, Lukas
On 18.10.22 08:25, Lukas Wunner wrote: > Hm, any reason not to use struct kref for the locality counter? > Provides correct memory ordering (no mutex needed) and allows for > calling a release function too upon reaching 0. > I already tried this but krefs turned out to be not very usable in this case. See my post here: https://lore.kernel.org/lkml/09eefdab-f677-864a-99f7-869d7a8744c2@gmx.de/ Regards, Lino
On Tue, Oct 18, 2022 at 01:57:29AM +0200, Lino Sanfilippo wrote: > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > Implement a usage counter for the (default) locality used by the TPM TIS > driver: > Request the locality from the TPM if it has not been claimed yet, otherwise > only increment the counter. Also release the locality if the counter is 0 > otherwise only decrement the counter. Ensure thread-safety by protecting > the counter with a mutex. > > This allows to request and release the locality from a thread and the > interrupt handler at the same time without the danger to interfere with > each other. > > By doing this refactor the names of the amended functions to use the proper > prefix. > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > Tested-by: Michael Niewöhner <linux@mniewoehner.de> > --- > drivers/char/tpm/tpm_tis_core.c | 75 ++++++++++++++++++++++----------- > drivers/char/tpm/tpm_tis_core.h | 2 + > 2 files changed, 53 insertions(+), 24 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index 4336f7ea8c2b..79dfab65976f 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -165,16 +165,27 @@ static bool check_locality(struct tpm_chip *chip, int l) > return false; > } > > -static int release_locality(struct tpm_chip *chip, int l) > +static int tpm_tis_release_locality_locked(struct tpm_tis_data *priv, int l) Nit: usually you would actually use "unlocked" here, not locked. Probably best name would be __tpm_tis_release_locality(). > +{ > + tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > + > + return 0; > +} > + > +static int tpm_tis_release_locality(struct tpm_chip *chip, int l) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > - tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > + mutex_lock(&priv->locality_count_mutex); > + priv->locality_count--; > + if (priv->locality_count == 0) > + tpm_tis_release_locality_locked(priv, l); > + mutex_unlock(&priv->locality_count_mutex); > > return 0; > } Since the function pointer has the word "relinquish" and not "release", perhaps these should also use that word for consistency. > > -static int request_locality(struct tpm_chip *chip, int l) > +static int tpm_tis_request_locality_locked(struct tpm_chip *chip, int l) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > unsigned long stop, timeout; > @@ -215,6 +226,20 @@ static int request_locality(struct tpm_chip *chip, int l) > return -1; > } > > +static int tpm_tis_request_locality(struct tpm_chip *chip, int l) > +{ > + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > + int ret = 0; > + > + mutex_lock(&priv->locality_count_mutex); > + if (priv->locality_count == 0) > + ret = tpm_tis_request_locality_locked(chip, l); > + if (!ret) > + priv->locality_count++; > + mutex_unlock(&priv->locality_count_mutex); > + return ret; > +} > + > static u8 tpm_tis_status(struct tpm_chip *chip) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > @@ -682,7 +707,7 @@ static int probe_itpm(struct tpm_chip *chip) > if (vendor != TPM_VID_INTEL) > return 0; > > - if (request_locality(chip, 0) != 0) > + if (tpm_tis_request_locality(chip, 0) != 0) > return -EBUSY; > > rc = tpm_tis_send_data(chip, cmd_getticks, len); > @@ -703,7 +728,7 @@ static int probe_itpm(struct tpm_chip *chip) > > out: > tpm_tis_ready(chip); > - release_locality(chip, priv->locality); > + tpm_tis_release_locality(chip, priv->locality); > > return rc; > } > @@ -762,7 +787,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) > cap_t cap; > int ret; > > - ret = request_locality(chip, 0); > + ret = tpm_tis_request_locality(chip, 0); > if (ret < 0) > return ret; > > @@ -771,7 +796,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) > else > ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0); > > - release_locality(chip, 0); > + tpm_tis_release_locality(chip, 0); > > return ret; > } > @@ -796,33 +821,33 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, > } > priv->irq = irq; > > - rc = request_locality(chip, 0); > + rc = tpm_tis_request_locality(chip, 0); > if (rc < 0) > return rc; > > rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality), > &original_int_vec); > if (rc < 0) { > - release_locality(chip, priv->locality); > + tpm_tis_release_locality(chip, priv->locality); > return rc; > } > > rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq); > if (rc < 0) { > - release_locality(chip, priv->locality); > + tpm_tis_release_locality(chip, priv->locality); > return rc; > } > > rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status); > if (rc < 0) { > - release_locality(chip, priv->locality); > + tpm_tis_release_locality(chip, priv->locality); > return rc; > } > > /* Clear all existing */ > rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status); > if (rc < 0) { > - release_locality(chip, priv->locality); > + tpm_tis_release_locality(chip, priv->locality); > return rc; > } > > @@ -830,11 +855,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, > rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), > intmask | TPM_GLOBAL_INT_ENABLE); > if (rc < 0) { > - release_locality(chip, priv->locality); > + tpm_tis_release_locality(chip, priv->locality); > return rc; > } > > - release_locality(chip, priv->locality); > + tpm_tis_release_locality(chip, priv->locality); > clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags); > > /* Generate an interrupt by having the core call through to > @@ -970,8 +995,8 @@ static const struct tpm_class_ops tpm_tis = { > .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID, > .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID, > .req_canceled = tpm_tis_req_canceled, > - .request_locality = request_locality, > - .relinquish_locality = release_locality, > + .request_locality = tpm_tis_request_locality, > + .relinquish_locality = tpm_tis_release_locality, > .clk_enable = tpm_tis_clkrun_enable, > }; > > @@ -1005,6 +1030,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > priv->timeout_min = TPM_TIMEOUT_USECS_MIN; > priv->timeout_max = TPM_TIMEOUT_USECS_MAX; > priv->phy_ops = phy_ops; > + priv->locality_count = 0; > + mutex_init(&priv->locality_count_mutex); > > dev_set_drvdata(&chip->dev, priv); > > @@ -1083,14 +1110,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > > intmask &= ~TPM_GLOBAL_INT_ENABLE; > > - rc = request_locality(chip, 0); > + rc = tpm_tis_request_locality(chip, 0); > if (rc < 0) { > rc = -ENODEV; > goto out_err; > } > > tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); > - release_locality(chip, 0); > + tpm_tis_release_locality(chip, 0); > > rc = tpm_chip_start(chip); > if (rc) > @@ -1124,13 +1151,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > * proper timeouts for the driver. > */ > > - rc = request_locality(chip, 0); > + rc = tpm_tis_request_locality(chip, 0); > if (rc < 0) > goto out_err; > > rc = tpm_get_timeouts(chip); > > - release_locality(chip, 0); > + tpm_tis_release_locality(chip, 0); > > if (rc) { > dev_err(dev, "Could not get TPM timeouts and durations\n"); > @@ -1150,11 +1177,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > dev_err(&chip->dev, FW_BUG > "TPM interrupt not working, polling instead\n"); > > - rc = request_locality(chip, 0); > + rc = tpm_tis_request_locality(chip, 0); > if (rc < 0) > goto out_err; > disable_interrupts(chip); > - release_locality(chip, 0); > + tpm_tis_release_locality(chip, 0); > } > } > > @@ -1221,13 +1248,13 @@ int tpm_tis_resume(struct device *dev) > * an error code but for unknown reason it isn't handled. > */ > if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) { > - ret = request_locality(chip, 0); > + ret = tpm_tis_request_locality(chip, 0); > if (ret < 0) > return ret; > > tpm1_do_selftest(chip); > > - release_locality(chip, 0); > + tpm_tis_release_locality(chip, 0); > } > > return 0; > diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h > index 2deef11c88db..13bdcf38e56f 100644 > --- a/drivers/char/tpm/tpm_tis_core.h > +++ b/drivers/char/tpm/tpm_tis_core.h > @@ -91,6 +91,8 @@ enum tpm_tis_flags { > > struct tpm_tis_data { > u16 manufacturer_id; > + struct mutex locality_count_mutex; BTW, why mutex and not spinlock? Hmm.. also I think you might have given feedback already on this but could the lock cover the whole struct instead of a counter? I tried to dig lore for earlier response but could not find. I'm sorry if I'm asking the same question again. You could probably use rcu for that. > + unsigned int locality_count; > int locality; > int irq; > unsigned int int_mask; > -- > 2.36.1 > BR, Jarkko
On Tue, Oct 18, 2022 at 08:25:08AM +0200, Lukas Wunner wrote: > On Tue, Oct 18, 2022 at 01:57:29AM +0200, Lino Sanfilippo wrote: > > Implement a usage counter for the (default) locality used by the TPM TIS > > driver: > > Request the locality from the TPM if it has not been claimed yet, otherwise > > only increment the counter. Also release the locality if the counter is 0 > > otherwise only decrement the counter. Ensure thread-safety by protecting > > the counter with a mutex. > > > > This allows to request and release the locality from a thread and the > > interrupt handler at the same time without the danger to interfere with > > each other. > [...] > > +static int tpm_tis_release_locality(struct tpm_chip *chip, int l) > > { > > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > > > - tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > > + mutex_lock(&priv->locality_count_mutex); > > + priv->locality_count--; > > + if (priv->locality_count == 0) > > + tpm_tis_release_locality_locked(priv, l); > > + mutex_unlock(&priv->locality_count_mutex); > > > > return 0; > > } > > Hm, any reason not to use struct kref for the locality counter? > Provides correct memory ordering (no mutex needed) and allows for > calling a release function too upon reaching 0. I proposed for last version kref. I have no idea why this is still using mutex. And now I apparently have proposed rcu for the whole struct (forgot what I had put my feedback for earlier version). This keeps being confusing patch as the commit message does not really go to the bottom line why mutex is really the best possible choice here. BR, Jarkko
On 23.10.22 06:40, Jarkko Sakkinen wrote: > On Tue, Oct 18, 2022 at 01:57:29AM +0200, Lino Sanfilippo wrote: >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> >> Implement a usage counter for the (default) locality used by the TPM TIS >> driver: >> Request the locality from the TPM if it has not been claimed yet, otherwise >> only increment the counter. Also release the locality if the counter is 0 >> otherwise only decrement the counter. Ensure thread-safety by protecting >> the counter with a mutex. >> >> This allows to request and release the locality from a thread and the >> interrupt handler at the same time without the danger to interfere with >> each other. >> >> By doing this refactor the names of the amended functions to use the proper >> prefix. >> >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> Tested-by: Michael Niewöhner <linux@mniewoehner.de> >> --- >> drivers/char/tpm/tpm_tis_core.c | 75 ++++++++++++++++++++++----------- >> drivers/char/tpm/tpm_tis_core.h | 2 + >> 2 files changed, 53 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >> index 4336f7ea8c2b..79dfab65976f 100644 >> --- a/drivers/char/tpm/tpm_tis_core.c >> +++ b/drivers/char/tpm/tpm_tis_core.c >> @@ -165,16 +165,27 @@ static bool check_locality(struct tpm_chip *chip, int l) >> return false; >> } >> >> -static int release_locality(struct tpm_chip *chip, int l) >> +static int tpm_tis_release_locality_locked(struct tpm_tis_data *priv, int l) > > Nit: usually you would actually use "unlocked" here, not locked. > > Probably best name would be __tpm_tis_release_locality(). Agreed. This is also consistent with the naming scheme used for many other kernel functions. > >> +{ >> + tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); >> + >> + return 0; >> +} >> + >> +static int tpm_tis_release_locality(struct tpm_chip *chip, int l) >> { >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> >> - tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); >> + mutex_lock(&priv->locality_count_mutex); >> + priv->locality_count--; >> + if (priv->locality_count == 0) >> + tpm_tis_release_locality_locked(priv, l); >> + mutex_unlock(&priv->locality_count_mutex); >> >> return 0; >> } > > Since the function pointer has the word "relinquish" and not "release", > perhaps these should also use that word for consistency. > >> >> -static int request_locality(struct tpm_chip *chip, int l) >> +static int tpm_tis_request_locality_locked(struct tpm_chip *chip, int l) >> { >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> unsigned long stop, timeout; >> @@ -215,6 +226,20 @@ static int request_locality(struct tpm_chip *chip, int l) >> return -1; >> } >> >> +static int tpm_tis_request_locality(struct tpm_chip *chip, int l) >> +{ >> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> + int ret = 0; >> + >> + mutex_lock(&priv->locality_count_mutex); >> + if (priv->locality_count == 0) >> + ret = tpm_tis_request_locality_locked(chip, l); >> + if (!ret) >> + priv->locality_count++; >> + mutex_unlock(&priv->locality_count_mutex); >> + return ret; >> +} >> + >> static u8 tpm_tis_status(struct tpm_chip *chip) >> { >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> @@ -682,7 +707,7 @@ static int probe_itpm(struct tpm_chip *chip) >> if (vendor != TPM_VID_INTEL) >> return 0; >> >> - if (request_locality(chip, 0) != 0) >> + if (tpm_tis_request_locality(chip, 0) != 0) >> return -EBUSY; >> >> rc = tpm_tis_send_data(chip, cmd_getticks, len); >> @@ -703,7 +728,7 @@ static int probe_itpm(struct tpm_chip *chip) >> >> out: >> tpm_tis_ready(chip); >> - release_locality(chip, priv->locality); >> + tpm_tis_release_locality(chip, priv->locality); >> >> return rc; >> } >> @@ -762,7 +787,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) >> cap_t cap; >> int ret; >> >> - ret = request_locality(chip, 0); >> + ret = tpm_tis_request_locality(chip, 0); >> if (ret < 0) >> return ret; >> >> @@ -771,7 +796,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) >> else >> ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0); >> >> - release_locality(chip, 0); >> + tpm_tis_release_locality(chip, 0); >> >> return ret; >> } >> @@ -796,33 +821,33 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, >> } >> priv->irq = irq; >> >> - rc = request_locality(chip, 0); >> + rc = tpm_tis_request_locality(chip, 0); >> if (rc < 0) >> return rc; >> >> rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality), >> &original_int_vec); >> if (rc < 0) { >> - release_locality(chip, priv->locality); >> + tpm_tis_release_locality(chip, priv->locality); >> return rc; >> } >> >> rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq); >> if (rc < 0) { >> - release_locality(chip, priv->locality); >> + tpm_tis_release_locality(chip, priv->locality); >> return rc; >> } >> >> rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status); >> if (rc < 0) { >> - release_locality(chip, priv->locality); >> + tpm_tis_release_locality(chip, priv->locality); >> return rc; >> } >> >> /* Clear all existing */ >> rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status); >> if (rc < 0) { >> - release_locality(chip, priv->locality); >> + tpm_tis_release_locality(chip, priv->locality); >> return rc; >> } >> >> @@ -830,11 +855,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, >> rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), >> intmask | TPM_GLOBAL_INT_ENABLE); >> if (rc < 0) { >> - release_locality(chip, priv->locality); >> + tpm_tis_release_locality(chip, priv->locality); >> return rc; >> } >> >> - release_locality(chip, priv->locality); >> + tpm_tis_release_locality(chip, priv->locality); >> clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags); >> >> /* Generate an interrupt by having the core call through to >> @@ -970,8 +995,8 @@ static const struct tpm_class_ops tpm_tis = { >> .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID, >> .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID, >> .req_canceled = tpm_tis_req_canceled, >> - .request_locality = request_locality, >> - .relinquish_locality = release_locality, >> + .request_locality = tpm_tis_request_locality, >> + .relinquish_locality = tpm_tis_release_locality, >> .clk_enable = tpm_tis_clkrun_enable, >> }; >> >> @@ -1005,6 +1030,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >> priv->timeout_min = TPM_TIMEOUT_USECS_MIN; >> priv->timeout_max = TPM_TIMEOUT_USECS_MAX; >> priv->phy_ops = phy_ops; >> + priv->locality_count = 0; >> + mutex_init(&priv->locality_count_mutex); >> >> dev_set_drvdata(&chip->dev, priv); >> >> @@ -1083,14 +1110,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >> >> intmask &= ~TPM_GLOBAL_INT_ENABLE; >> >> - rc = request_locality(chip, 0); >> + rc = tpm_tis_request_locality(chip, 0); >> if (rc < 0) { >> rc = -ENODEV; >> goto out_err; >> } >> >> tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); >> - release_locality(chip, 0); >> + tpm_tis_release_locality(chip, 0); >> >> rc = tpm_chip_start(chip); >> if (rc) >> @@ -1124,13 +1151,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >> * proper timeouts for the driver. >> */ >> >> - rc = request_locality(chip, 0); >> + rc = tpm_tis_request_locality(chip, 0); >> if (rc < 0) >> goto out_err; >> >> rc = tpm_get_timeouts(chip); >> >> - release_locality(chip, 0); >> + tpm_tis_release_locality(chip, 0); >> >> if (rc) { >> dev_err(dev, "Could not get TPM timeouts and durations\n"); >> @@ -1150,11 +1177,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >> dev_err(&chip->dev, FW_BUG >> "TPM interrupt not working, polling instead\n"); >> >> - rc = request_locality(chip, 0); >> + rc = tpm_tis_request_locality(chip, 0); >> if (rc < 0) >> goto out_err; >> disable_interrupts(chip); >> - release_locality(chip, 0); >> + tpm_tis_release_locality(chip, 0); >> } >> } >> >> @@ -1221,13 +1248,13 @@ int tpm_tis_resume(struct device *dev) >> * an error code but for unknown reason it isn't handled. >> */ >> if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) { >> - ret = request_locality(chip, 0); >> + ret = tpm_tis_request_locality(chip, 0); >> if (ret < 0) >> return ret; >> >> tpm1_do_selftest(chip); >> >> - release_locality(chip, 0); >> + tpm_tis_release_locality(chip, 0); >> } >> >> return 0; >> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h >> index 2deef11c88db..13bdcf38e56f 100644 >> --- a/drivers/char/tpm/tpm_tis_core.h >> +++ b/drivers/char/tpm/tpm_tis_core.h >> @@ -91,6 +91,8 @@ enum tpm_tis_flags { >> >> struct tpm_tis_data { >> u16 manufacturer_id; >> + struct mutex locality_count_mutex; > > BTW, why mutex and not spinlock? > > Hmm.. also I think you might have given feedback already on this > but could the lock cover the whole struct instead of a counter? > I tried to dig lore for earlier response but could not find. I'm > sorry if I'm asking the same question again. > Actually thats on me, since it took me much too long to send the v8 after the v7 review. However the reason that we need a mutex here is that we not only increase or decrease the locality_counter under the mutex, but also do the locality request and release by writing to the ACCESS register. Since in the SPI case each communication over the spi bus is protected by the bus_lock_mutex of the SPI device we must not hold a spinlock when doing the register accesses. Concerning covering the whole tpm_tis_data struct: Most structure elements are set once at driver startup but never changed at driver runtime. So no locking needed for these. The only exception is "flags" and "locality_count" whereby "flags" is accessed by atomic bit manipulating functions and thus does not need extra locking. So "locality_count" is AFAICS the only element that needs to be protected by the mutex. > You could probably use rcu for that. > >> + unsigned int locality_count; Note that we do not only have to increase or decrease the counter atomically, but the first increment (when the counter goes from 1 to 0) AND requesting the locality by writing to the ACCESS register has to be atomic. Likewise the last decrement (when the counter goes from 1 to 0) AND releasing the locality has to be done atomically: mutex_lock(&priv->locality_count_mutex); priv->locality_count--; if (priv->locality_count == 0) tpm_tis_release_locality_locked(priv, l); mutex_unlock(&priv->locality_count_mutex); I dont know if this is possible with RCU, especially since AFAIK RCU works with pointers instead of integral data types. Regards, Lino >> int locality; >> int irq; >> unsigned int int_mask; >> -- >> 2.36.1 >> > > BR, Jarkko
On 23.10.22 07:26, Jarkko Sakkinen wrote: > On Tue, Oct 18, 2022 at 08:25:08AM +0200, Lukas Wunner wrote: >> On Tue, Oct 18, 2022 at 01:57:29AM +0200, Lino Sanfilippo wrote: >>> Implement a usage counter for the (default) locality used by the TPM TIS >>> driver: >>> Request the locality from the TPM if it has not been claimed yet, otherwise >>> only increment the counter. Also release the locality if the counter is 0 >>> otherwise only decrement the counter. Ensure thread-safety by protecting >>> the counter with a mutex. >>> >>> This allows to request and release the locality from a thread and the >>> interrupt handler at the same time without the danger to interfere with >>> each other. >> [...] >>> +static int tpm_tis_release_locality(struct tpm_chip *chip, int l) >>> { >>> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >>> >>> - tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); >>> + mutex_lock(&priv->locality_count_mutex); >>> + priv->locality_count--; >>> + if (priv->locality_count == 0) >>> + tpm_tis_release_locality_locked(priv, l); >>> + mutex_unlock(&priv->locality_count_mutex); >>> >>> return 0; >>> } >> >> Hm, any reason not to use struct kref for the locality counter? >> Provides correct memory ordering (no mutex needed) and allows for >> calling a release function too upon reaching 0. > > I proposed for last version kref. I have no idea why this is still > using mutex. And now I apparently have proposed rcu for the whole > struct (forgot what I had put my feedback for earlier version). > > This keeps being confusing patch as the commit message does not > really go to the bottom line why mutex is really the best possible > choice here. > I actually tried to implement this via kref but then came to the conclusion it is rather not a good choice for our case. Please see my response to your former request to implement this via kref: https://lore.kernel.org/all/09eefdab-f677-864a-99f7-869d7a8744c2@gmx.de/ Regards, Lino
On Tue, Oct 25, 2022 at 02:15:51AM +0200, Lino Sanfilippo wrote: > > On 23.10.22 06:40, Jarkko Sakkinen wrote: > > On Tue, Oct 18, 2022 at 01:57:29AM +0200, Lino Sanfilippo wrote: > >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > >> > >> Implement a usage counter for the (default) locality used by the TPM TIS > >> driver: > >> Request the locality from the TPM if it has not been claimed yet, otherwise > >> only increment the counter. Also release the locality if the counter is 0 > >> otherwise only decrement the counter. Ensure thread-safety by protecting > >> the counter with a mutex. > >> > >> This allows to request and release the locality from a thread and the > >> interrupt handler at the same time without the danger to interfere with > >> each other. > >> > >> By doing this refactor the names of the amended functions to use the proper > >> prefix. > >> > >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > >> Tested-by: Michael Niewöhner <linux@mniewoehner.de> > >> --- > >> drivers/char/tpm/tpm_tis_core.c | 75 ++++++++++++++++++++++----------- > >> drivers/char/tpm/tpm_tis_core.h | 2 + > >> 2 files changed, 53 insertions(+), 24 deletions(-) > >> > >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > >> index 4336f7ea8c2b..79dfab65976f 100644 > >> --- a/drivers/char/tpm/tpm_tis_core.c > >> +++ b/drivers/char/tpm/tpm_tis_core.c > >> @@ -165,16 +165,27 @@ static bool check_locality(struct tpm_chip *chip, int l) > >> return false; > >> } > >> > >> -static int release_locality(struct tpm_chip *chip, int l) > >> +static int tpm_tis_release_locality_locked(struct tpm_tis_data *priv, int l) > > > > Nit: usually you would actually use "unlocked" here, not locked. > > > > Probably best name would be __tpm_tis_release_locality(). > > > Agreed. This is also consistent with the naming scheme used for many other > kernel functions. > > > > >> +{ > >> + tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > >> + > >> + return 0; > >> +} > >> + > >> +static int tpm_tis_release_locality(struct tpm_chip *chip, int l) > >> { > >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > >> > >> - tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > >> + mutex_lock(&priv->locality_count_mutex); > >> + priv->locality_count--; > >> + if (priv->locality_count == 0) > >> + tpm_tis_release_locality_locked(priv, l); > >> + mutex_unlock(&priv->locality_count_mutex); > >> > >> return 0; > >> } > > > > Since the function pointer has the word "relinquish" and not "release", > > perhaps these should also use that word for consistency. > > > >> > >> -static int request_locality(struct tpm_chip *chip, int l) > >> +static int tpm_tis_request_locality_locked(struct tpm_chip *chip, int l) > >> { > >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > >> unsigned long stop, timeout; > >> @@ -215,6 +226,20 @@ static int request_locality(struct tpm_chip *chip, int l) > >> return -1; > >> } > >> > >> +static int tpm_tis_request_locality(struct tpm_chip *chip, int l) > >> +{ > >> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > >> + int ret = 0; > >> + > >> + mutex_lock(&priv->locality_count_mutex); > >> + if (priv->locality_count == 0) > >> + ret = tpm_tis_request_locality_locked(chip, l); > >> + if (!ret) > >> + priv->locality_count++; > >> + mutex_unlock(&priv->locality_count_mutex); > >> + return ret; > >> +} > >> + > >> static u8 tpm_tis_status(struct tpm_chip *chip) > >> { > >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > >> @@ -682,7 +707,7 @@ static int probe_itpm(struct tpm_chip *chip) > >> if (vendor != TPM_VID_INTEL) > >> return 0; > >> > >> - if (request_locality(chip, 0) != 0) > >> + if (tpm_tis_request_locality(chip, 0) != 0) > >> return -EBUSY; > >> > >> rc = tpm_tis_send_data(chip, cmd_getticks, len); > >> @@ -703,7 +728,7 @@ static int probe_itpm(struct tpm_chip *chip) > >> > >> out: > >> tpm_tis_ready(chip); > >> - release_locality(chip, priv->locality); > >> + tpm_tis_release_locality(chip, priv->locality); > >> > >> return rc; > >> } > >> @@ -762,7 +787,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) > >> cap_t cap; > >> int ret; > >> > >> - ret = request_locality(chip, 0); > >> + ret = tpm_tis_request_locality(chip, 0); > >> if (ret < 0) > >> return ret; > >> > >> @@ -771,7 +796,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) > >> else > >> ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0); > >> > >> - release_locality(chip, 0); > >> + tpm_tis_release_locality(chip, 0); > >> > >> return ret; > >> } > >> @@ -796,33 +821,33 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, > >> } > >> priv->irq = irq; > >> > >> - rc = request_locality(chip, 0); > >> + rc = tpm_tis_request_locality(chip, 0); > >> if (rc < 0) > >> return rc; > >> > >> rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality), > >> &original_int_vec); > >> if (rc < 0) { > >> - release_locality(chip, priv->locality); > >> + tpm_tis_release_locality(chip, priv->locality); > >> return rc; > >> } > >> > >> rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq); > >> if (rc < 0) { > >> - release_locality(chip, priv->locality); > >> + tpm_tis_release_locality(chip, priv->locality); > >> return rc; > >> } > >> > >> rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status); > >> if (rc < 0) { > >> - release_locality(chip, priv->locality); > >> + tpm_tis_release_locality(chip, priv->locality); > >> return rc; > >> } > >> > >> /* Clear all existing */ > >> rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status); > >> if (rc < 0) { > >> - release_locality(chip, priv->locality); > >> + tpm_tis_release_locality(chip, priv->locality); > >> return rc; > >> } > >> > >> @@ -830,11 +855,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, > >> rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), > >> intmask | TPM_GLOBAL_INT_ENABLE); > >> if (rc < 0) { > >> - release_locality(chip, priv->locality); > >> + tpm_tis_release_locality(chip, priv->locality); > >> return rc; > >> } > >> > >> - release_locality(chip, priv->locality); > >> + tpm_tis_release_locality(chip, priv->locality); > >> clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags); > >> > >> /* Generate an interrupt by having the core call through to > >> @@ -970,8 +995,8 @@ static const struct tpm_class_ops tpm_tis = { > >> .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID, > >> .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID, > >> .req_canceled = tpm_tis_req_canceled, > >> - .request_locality = request_locality, > >> - .relinquish_locality = release_locality, > >> + .request_locality = tpm_tis_request_locality, > >> + .relinquish_locality = tpm_tis_release_locality, > >> .clk_enable = tpm_tis_clkrun_enable, > >> }; > >> > >> @@ -1005,6 +1030,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > >> priv->timeout_min = TPM_TIMEOUT_USECS_MIN; > >> priv->timeout_max = TPM_TIMEOUT_USECS_MAX; > >> priv->phy_ops = phy_ops; > >> + priv->locality_count = 0; > >> + mutex_init(&priv->locality_count_mutex); > >> > >> dev_set_drvdata(&chip->dev, priv); > >> > >> @@ -1083,14 +1110,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > >> > >> intmask &= ~TPM_GLOBAL_INT_ENABLE; > >> > >> - rc = request_locality(chip, 0); > >> + rc = tpm_tis_request_locality(chip, 0); > >> if (rc < 0) { > >> rc = -ENODEV; > >> goto out_err; > >> } > >> > >> tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); > >> - release_locality(chip, 0); > >> + tpm_tis_release_locality(chip, 0); > >> > >> rc = tpm_chip_start(chip); > >> if (rc) > >> @@ -1124,13 +1151,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > >> * proper timeouts for the driver. > >> */ > >> > >> - rc = request_locality(chip, 0); > >> + rc = tpm_tis_request_locality(chip, 0); > >> if (rc < 0) > >> goto out_err; > >> > >> rc = tpm_get_timeouts(chip); > >> > >> - release_locality(chip, 0); > >> + tpm_tis_release_locality(chip, 0); > >> > >> if (rc) { > >> dev_err(dev, "Could not get TPM timeouts and durations\n"); > >> @@ -1150,11 +1177,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > >> dev_err(&chip->dev, FW_BUG > >> "TPM interrupt not working, polling instead\n"); > >> > >> - rc = request_locality(chip, 0); > >> + rc = tpm_tis_request_locality(chip, 0); > >> if (rc < 0) > >> goto out_err; > >> disable_interrupts(chip); > >> - release_locality(chip, 0); > >> + tpm_tis_release_locality(chip, 0); > >> } > >> } > >> > >> @@ -1221,13 +1248,13 @@ int tpm_tis_resume(struct device *dev) > >> * an error code but for unknown reason it isn't handled. > >> */ > >> if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) { > >> - ret = request_locality(chip, 0); > >> + ret = tpm_tis_request_locality(chip, 0); > >> if (ret < 0) > >> return ret; > >> > >> tpm1_do_selftest(chip); > >> > >> - release_locality(chip, 0); > >> + tpm_tis_release_locality(chip, 0); > >> } > >> > >> return 0; > >> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h > >> index 2deef11c88db..13bdcf38e56f 100644 > >> --- a/drivers/char/tpm/tpm_tis_core.h > >> +++ b/drivers/char/tpm/tpm_tis_core.h > >> @@ -91,6 +91,8 @@ enum tpm_tis_flags { > >> > >> struct tpm_tis_data { > >> u16 manufacturer_id; > >> + struct mutex locality_count_mutex; > > > > BTW, why mutex and not spinlock? > > > > Hmm.. also I think you might have given feedback already on this > > but could the lock cover the whole struct instead of a counter? > > I tried to dig lore for earlier response but could not find. I'm > > sorry if I'm asking the same question again. > > > > Actually thats on me, since it took me much too long to send the v8 after the v7 review. > > However the reason that we need a mutex here is that we not only increase or decrease > the locality_counter under the mutex, but also do the locality request and release by > writing to the ACCESS register. Since in the SPI case each communication over the spi bus > is protected by the bus_lock_mutex of the SPI device we must not hold a spinlock when doing > the register accesses. > > Concerning covering the whole tpm_tis_data struct: > Most structure elements are set once at driver startup but never changed at driver > runtime. So no locking needed for these. The only exception is "flags" and "locality_count" > whereby "flags" is accessed by atomic bit manipulating functions and thus > does not need extra locking. So "locality_count" is AFAICS the only element that needs to be > protected by the mutex. OK, but you should should still address this in commit message, e.g. by mentioning that in the case of SPI bus mutex is required because the bus itself needs to be locked in the mutex. I.e. this a claim, definitely not an argument: "Ensure thread-safety by protecting the counter with a mutex." BR, Jarkko
On Tue, Oct 25, 2022 at 02:25:39AM +0200, Lino Sanfilippo wrote: > > > On 23.10.22 07:26, Jarkko Sakkinen wrote: > > On Tue, Oct 18, 2022 at 08:25:08AM +0200, Lukas Wunner wrote: > >> On Tue, Oct 18, 2022 at 01:57:29AM +0200, Lino Sanfilippo wrote: > >>> Implement a usage counter for the (default) locality used by the TPM TIS > >>> driver: > >>> Request the locality from the TPM if it has not been claimed yet, otherwise > >>> only increment the counter. Also release the locality if the counter is 0 > >>> otherwise only decrement the counter. Ensure thread-safety by protecting > >>> the counter with a mutex. > >>> > >>> This allows to request and release the locality from a thread and the > >>> interrupt handler at the same time without the danger to interfere with > >>> each other. > >> [...] > >>> +static int tpm_tis_release_locality(struct tpm_chip *chip, int l) > >>> { > >>> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > >>> > >>> - tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > >>> + mutex_lock(&priv->locality_count_mutex); > >>> + priv->locality_count--; > >>> + if (priv->locality_count == 0) > >>> + tpm_tis_release_locality_locked(priv, l); > >>> + mutex_unlock(&priv->locality_count_mutex); > >>> > >>> return 0; > >>> } > >> > >> Hm, any reason not to use struct kref for the locality counter? > >> Provides correct memory ordering (no mutex needed) and allows for > >> calling a release function too upon reaching 0. > > > > I proposed for last version kref. I have no idea why this is still > > using mutex. And now I apparently have proposed rcu for the whole > > struct (forgot what I had put my feedback for earlier version). > > > > This keeps being confusing patch as the commit message does not > > really go to the bottom line why mutex is really the best possible > > choice here. > > > > > I actually tried to implement this via kref but then came to the > conclusion it is rather not a good choice for our case. Please > see my response to your former request to implement this via kref: > > https://lore.kernel.org/all/09eefdab-f677-864a-99f7-869d7a8744c2@gmx.de/ OK, my bad I missed this, sorry. BR, Jarkko
On 01.11.22 02:06, Jarkko Sakkinen wrote: > On Tue, Oct 25, 2022 at 02:15:51AM +0200, Lino Sanfilippo wrote: >> Actually thats on me, since it took me much too long to send the v8 after the v7 review. >> >> However the reason that we need a mutex here is that we not only increase or decrease >> the locality_counter under the mutex, but also do the locality request and release by >> writing to the ACCESS register. Since in the SPI case each communication over the spi bus >> is protected by the bus_lock_mutex of the SPI device we must not hold a spinlock when doing >> the register accesses. >> >> Concerning covering the whole tpm_tis_data struct: >> Most structure elements are set once at driver startup but never changed at driver >> runtime. So no locking needed for these. The only exception is "flags" and "locality_count" >> whereby "flags" is accessed by atomic bit manipulating functions and thus >> does not need extra locking. So "locality_count" is AFAICS the only element that needs to be >> protected by the mutex. > > OK, but you should should still address this in commit message, e.g. > by mentioning that in the case of SPI bus mutex is required because > the bus itself needs to be locked in the mutex. > > I.e. this a claim, definitely not an argument: "Ensure thread-safety by > protecting the counter with a mutex." > Ok, I will rephrase the commit message accordingly. Thanks for the review! Regards, Lino
On Fri, Nov 04, 2022 at 05:18:21PM +0100, Lino Sanfilippo wrote: > > > On 01.11.22 02:06, Jarkko Sakkinen wrote: > > On Tue, Oct 25, 2022 at 02:15:51AM +0200, Lino Sanfilippo wrote: > > >> Actually thats on me, since it took me much too long to send the v8 after the v7 review. > >> > >> However the reason that we need a mutex here is that we not only increase or decrease > >> the locality_counter under the mutex, but also do the locality request and release by > >> writing to the ACCESS register. Since in the SPI case each communication over the spi bus > >> is protected by the bus_lock_mutex of the SPI device we must not hold a spinlock when doing > >> the register accesses. > >> > >> Concerning covering the whole tpm_tis_data struct: > >> Most structure elements are set once at driver startup but never changed at driver > >> runtime. So no locking needed for these. The only exception is "flags" and "locality_count" > >> whereby "flags" is accessed by atomic bit manipulating functions and thus > >> does not need extra locking. So "locality_count" is AFAICS the only element that needs to be > >> protected by the mutex. > > > > OK, but you should should still address this in commit message, e.g. > > by mentioning that in the case of SPI bus mutex is required because > > the bus itself needs to be locked in the mutex. > > > > I.e. this a claim, definitely not an argument: "Ensure thread-safety by > > protecting the counter with a mutex." > > > > Ok, I will rephrase the commit message accordingly. > Thanks for the review! Yeah, np. I.e. I understand your reasoning but it is easy to intuitively think it as not the right solution. Thus, it deserves a remark, right? :-) BR, Jarkko
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index 4336f7ea8c2b..79dfab65976f 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -165,16 +165,27 @@ static bool check_locality(struct tpm_chip *chip, int l) return false; } -static int release_locality(struct tpm_chip *chip, int l) +static int tpm_tis_release_locality_locked(struct tpm_tis_data *priv, int l) +{ + tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); + + return 0; +} + +static int tpm_tis_release_locality(struct tpm_chip *chip, int l) { struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); - tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); + mutex_lock(&priv->locality_count_mutex); + priv->locality_count--; + if (priv->locality_count == 0) + tpm_tis_release_locality_locked(priv, l); + mutex_unlock(&priv->locality_count_mutex); return 0; } -static int request_locality(struct tpm_chip *chip, int l) +static int tpm_tis_request_locality_locked(struct tpm_chip *chip, int l) { struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); unsigned long stop, timeout; @@ -215,6 +226,20 @@ static int request_locality(struct tpm_chip *chip, int l) return -1; } +static int tpm_tis_request_locality(struct tpm_chip *chip, int l) +{ + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + int ret = 0; + + mutex_lock(&priv->locality_count_mutex); + if (priv->locality_count == 0) + ret = tpm_tis_request_locality_locked(chip, l); + if (!ret) + priv->locality_count++; + mutex_unlock(&priv->locality_count_mutex); + return ret; +} + static u8 tpm_tis_status(struct tpm_chip *chip) { struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); @@ -682,7 +707,7 @@ static int probe_itpm(struct tpm_chip *chip) if (vendor != TPM_VID_INTEL) return 0; - if (request_locality(chip, 0) != 0) + if (tpm_tis_request_locality(chip, 0) != 0) return -EBUSY; rc = tpm_tis_send_data(chip, cmd_getticks, len); @@ -703,7 +728,7 @@ static int probe_itpm(struct tpm_chip *chip) out: tpm_tis_ready(chip); - release_locality(chip, priv->locality); + tpm_tis_release_locality(chip, priv->locality); return rc; } @@ -762,7 +787,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) cap_t cap; int ret; - ret = request_locality(chip, 0); + ret = tpm_tis_request_locality(chip, 0); if (ret < 0) return ret; @@ -771,7 +796,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) else ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0); - release_locality(chip, 0); + tpm_tis_release_locality(chip, 0); return ret; } @@ -796,33 +821,33 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, } priv->irq = irq; - rc = request_locality(chip, 0); + rc = tpm_tis_request_locality(chip, 0); if (rc < 0) return rc; rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality), &original_int_vec); if (rc < 0) { - release_locality(chip, priv->locality); + tpm_tis_release_locality(chip, priv->locality); return rc; } rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq); if (rc < 0) { - release_locality(chip, priv->locality); + tpm_tis_release_locality(chip, priv->locality); return rc; } rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status); if (rc < 0) { - release_locality(chip, priv->locality); + tpm_tis_release_locality(chip, priv->locality); return rc; } /* Clear all existing */ rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status); if (rc < 0) { - release_locality(chip, priv->locality); + tpm_tis_release_locality(chip, priv->locality); return rc; } @@ -830,11 +855,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask | TPM_GLOBAL_INT_ENABLE); if (rc < 0) { - release_locality(chip, priv->locality); + tpm_tis_release_locality(chip, priv->locality); return rc; } - release_locality(chip, priv->locality); + tpm_tis_release_locality(chip, priv->locality); clear_bit(TPM_TIS_IRQ_TESTED, &priv->flags); /* Generate an interrupt by having the core call through to @@ -970,8 +995,8 @@ static const struct tpm_class_ops tpm_tis = { .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID, .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID, .req_canceled = tpm_tis_req_canceled, - .request_locality = request_locality, - .relinquish_locality = release_locality, + .request_locality = tpm_tis_request_locality, + .relinquish_locality = tpm_tis_release_locality, .clk_enable = tpm_tis_clkrun_enable, }; @@ -1005,6 +1030,8 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, priv->timeout_min = TPM_TIMEOUT_USECS_MIN; priv->timeout_max = TPM_TIMEOUT_USECS_MAX; priv->phy_ops = phy_ops; + priv->locality_count = 0; + mutex_init(&priv->locality_count_mutex); dev_set_drvdata(&chip->dev, priv); @@ -1083,14 +1110,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, intmask &= ~TPM_GLOBAL_INT_ENABLE; - rc = request_locality(chip, 0); + rc = tpm_tis_request_locality(chip, 0); if (rc < 0) { rc = -ENODEV; goto out_err; } tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); - release_locality(chip, 0); + tpm_tis_release_locality(chip, 0); rc = tpm_chip_start(chip); if (rc) @@ -1124,13 +1151,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, * proper timeouts for the driver. */ - rc = request_locality(chip, 0); + rc = tpm_tis_request_locality(chip, 0); if (rc < 0) goto out_err; rc = tpm_get_timeouts(chip); - release_locality(chip, 0); + tpm_tis_release_locality(chip, 0); if (rc) { dev_err(dev, "Could not get TPM timeouts and durations\n"); @@ -1150,11 +1177,11 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, dev_err(&chip->dev, FW_BUG "TPM interrupt not working, polling instead\n"); - rc = request_locality(chip, 0); + rc = tpm_tis_request_locality(chip, 0); if (rc < 0) goto out_err; disable_interrupts(chip); - release_locality(chip, 0); + tpm_tis_release_locality(chip, 0); } } @@ -1221,13 +1248,13 @@ int tpm_tis_resume(struct device *dev) * an error code but for unknown reason it isn't handled. */ if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) { - ret = request_locality(chip, 0); + ret = tpm_tis_request_locality(chip, 0); if (ret < 0) return ret; tpm1_do_selftest(chip); - release_locality(chip, 0); + tpm_tis_release_locality(chip, 0); } return 0; diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h index 2deef11c88db..13bdcf38e56f 100644 --- a/drivers/char/tpm/tpm_tis_core.h +++ b/drivers/char/tpm/tpm_tis_core.h @@ -91,6 +91,8 @@ enum tpm_tis_flags { struct tpm_tis_data { u16 manufacturer_id; + struct mutex locality_count_mutex; + unsigned int locality_count; int locality; int irq; unsigned int int_mask;