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;
Date:   Mon, 7 Nov 2022 18:03:49 +0200
From:   Jarkko Sakkinen <jarkko@kernel.org>
To:     Lino Sanfilippo <l.sanfilippo@kunbus.com>
Cc:     Lino Sanfilippo <LinoSanfilippo@gmx.de>, peterhuewe@gmx.de,
        jgg@ziepe.ca, 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, lukas@wunner.de,
        p.rosenberger@kunbus.com
Subject: Re: [PATCH v8 08/11] tpm, tpm: Implement usage counter for locality
Message-ID: <Y2ksZXP1dunRyul8@kernel.org>
References: <20221017235732.10145-1-LinoSanfilippo@gmx.de>
 <20221017235732.10145-9-LinoSanfilippo@gmx.de>
 <Y1TFzO1CBY8d4rfa@kernel.org>
 <fcf53b02-a505-46ac-006e-0e317f00bbe3@gmx.de>
 <Y2Bw/yhTs258Iiew@kernel.org>
 <57b193a6-8340-c883-04bc-6cfbc3c638cd@kunbus.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <57b193a6-8340-c883-04bc-6cfbc3c638cd@kunbus.com>
Precedence: bulk

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