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? Regards, Lino
