On Thu, Jun 21, 2018 at 04:14:46PM -0400, Stefan Berger wrote: > On 06/21/2018 03:06 PM, Jason Gunthorpe wrote: > >On Thu, Jun 21, 2018 at 02:19:44PM -0400, Stefan Berger wrote: > >>On 06/21/2018 01:56 PM, Jason Gunthorpe wrote: > >>>On Thu, Jun 21, 2018 at 01:45:03PM -0400, Stefan Berger wrote: > >>>>On 06/21/2018 01:15 PM, Jarkko Sakkinen wrote: > >>>>>On Wed, Jun 20, 2018 at 04:42:33PM -0400, Stefan Berger wrote: > >>>>>>Implement tpm_chip_find() for other subsystems to find a TPM chip and > >>>>>>get a reference to that chip. Once done with using the chip, the reference > >>>>>>is released using tpm_chip_put(). > >>>>>> > >>>>>>Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx> > >>>>>You should sort this out in a way that we don't end up with duplicate > >>>>>functions. > >>>>Do you want me to create a function *like* tpm_chip_find_get() that takes an > >>>>additional parameter whether to get the ops semaphore and have that function > >>>>called by the existing tpm_chip_find_get() and the new tpm_chip_find(). The > >>>>latter would then not get the ops semphore. I didn't want to do this since > >>>>one time the function returns with a lock held and the other time not. > >>>Another option, and I haven't looked, is to revise the callers of > >>>tpm_chip_find_get to not require it to hold the ops semaphore for > >>>them. > >>We have tpm_chip_unregister calling tpm_del_char_device to set the ops to > >>NULL once a chip is unregistered. All existing callers, if they pass in a > >>tpm_chip != NULL, currently fail if the ops are NULL. (If they pass in > >>tpm_chip = NULL, they shouldn't find a chip once ops are null and it has > >>been removed from the IDR). I wouldn't change that since IMA will call in > >>with a tpm_chip != NULL and we want to protect the ops. All existing code > >>within the tpm subsystem does seem to call tpm_chip_find_get() with a NULL > >>pointer, though. Also trusted keys seems to pass in a NULL pointer every > >>time. > >> > >>>Either by giving them an API to do it, or revising the TPM entry > >>>points to do it. > >>> > >>>I didn't look, but how did the ops semaphore get grabbed in your > >>>revised patches? They do grab it, right? > >>The revised patches do not touch the existing code much but will call > >>tpm_chip_find_get() and get that semaphore every time before the ops are > >>used. IMA is the only caller of tpm_chip_find() that now gets an additional > >>reference to the tpm_chip and these APIs get called like this from IMA: > >> > >>ima init: chip = tpm_chip_find() > >> > >>ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip) > >> > >>ima::tpm: tpm_chip_find_get(chip) ... tpm_put_ops(chip) > >> > >>[repeat] > >> > >>ima shutdown: tpm_chip_put(chip) > >Maybe just change tpm_chip_find_get() into tpm_get_ops(chip) and > >convert all callers? > > And then re-introduce tpm_chip_find_get() for IMA to call ? You could keep it as 'tpm_chip_find', that seems like a fine name to me Jason
