On Thu, Jul 28, 2022 at 07:36:19PM +0200, Lino Sanfilippo wrote: > > > On 04.07.22 19:45, Lino Sanfilippo wrote: > > > > > > On 01.07.22 01:29, Jarkko Sakkinen wrote: > > > >> > >> I'm kind of thinking that should tpm_tis_data have a lock for its > >> contents? > > > > Most of the tpm_tis_data structure elements are set once during init and > > then never changed but only read. So no need for locking for these. The > > exceptions I see are > > > > - flags > > - locality_count > > - locality > > > > > > whereby "flags" is accessed by atomic bit manipulating functions and thus > > does not need extra locking. "locality_count" is protected by the locality_count_mutex. > > "locality" is only set in check_locality() which is called from tpm_tis_request_locality_locked() > > which holds the locality_count_mutex. So check_locality() is also protected by the locality_count_mutex > > (which for this reason should probably rather be called locality_mutex since it protects both the "locality_count" > > and the "locality" variable). > > > > There is one other place check_locality() is called from, namely the interrupt handler. This is also the only > > place in which "locality" could be assigned another value than 0 (aka the default). In this case there > > is no lock, so this could indeed by racy. > > > > The solution I see for this is: > > 1. remove the entire loop that checks for the current locality, i.e. this code: > > > > if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT) > > for (i = 0; i < 5; i++) > > if (check_locality(chip, i)) > > break; > > > > So we avoid "locality" from being changed to something that is not the default. > > > > > > I wonder if we need tpm_tis_data->locality at all: the claimed locality is already tracked in > chip->locality and in TPM TIS we never use anything else than locality 0 so it never changes. > > Is there any good reason not to remove it? I think it would be a great idea to unify them. BR, Jarkko
