Re: [RFC PATCH v1 03/38] tsm: Move dsm_dev from pci_tdi to pci_tsm

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Bjorn Helgaas <helgaas@kernel.org> writes:

> On Mon, Jul 28, 2025 at 07:21:40PM +0530, Aneesh Kumar K.V (Arm) wrote:
>
> Subject line should include "PCI" prefix.
>
> Needs a commit log, even if it repeats what's in the subject.  Would
> also be good to know *why* this is desirable.
>
>> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
>> ---
>>  drivers/pci/tsm.c       | 72 ++++++++++++++++++++++++-----------------
>>  include/linux/pci-tsm.h |  4 +--
>>  2 files changed, 45 insertions(+), 31 deletions(-)
>> 
>> diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
>> index 794de2f258c3..e4a3b5b37939 100644
>> --- a/drivers/pci/tsm.c
>> +++ b/drivers/pci/tsm.c
>> @@ -415,15 +415,55 @@ static enum pci_tsm_type pci_tsm_type(struct pci_dev *pdev)
>>  	return PCI_TSM_INVALID;
>>  }
>>  
>> +/* lookup the Device Security Manager (DSM) pf0 for @pdev */
>
> s/lookup/Look up/ (it's a verb here)
>  
>> +static struct pci_dev *dsm_dev_get(struct pci_dev *pdev)
>> +{
>> +	struct pci_dev *uport_pf0;
>> +
>
> Remove this blank line ...
>
>> +	struct pci_dev *pf0 __free(pci_dev_put) = pf0_dev_get(pdev);
>
> and add one here.
>
>> +	if (!pf0)
>> +		return NULL;
>> +
>> +	if (pf0 == pdev)
>> +		return no_free_ptr(pf0);
>> +
>> +	/* Check that @pf0 was not initialized as PCI_TSM_DOWNSTREAM */
>> +	if (pf0->tsm && pf0->tsm->type == PCI_TSM_PF0)
>> +		return no_free_ptr(pf0);
>> +
>> +	/*
>> +	 * For cases where a switch may be hosting TDISP services on
>> +	 * behalf of downstream devices, check the first usptream port
>> +	 * relative to this endpoint.
>
> s/usptream/upstream/
>
>> +	 */
>> +	if (!pdev->dev.parent || !pdev->dev.parent->parent)
>> +		return NULL;
>> +
>> +	uport_pf0 = to_pci_dev(pdev->dev.parent->parent);
>> +	if (!uport_pf0->tsm)
>> +		return NULL;
>> +	return pci_dev_get(uport_pf0);
>> +}
>> +
>>  /**
>>   * pci_tsm_initialize() - base 'struct pci_tsm' initialization
>>   * @pdev: The PCI device
>>   * @tsm: context to initialize
>>   */
>> -void pci_tsm_initialize(struct pci_dev *pdev, struct pci_tsm *tsm)
>> +int pci_tsm_initialize(struct pci_dev *pdev, struct pci_tsm *tsm)
>>  {
>> +	struct pci_dev *dsm_dev __free(pci_dev_put) = dsm_dev_get(pdev);
>> +	if (!dsm_dev)
>> +		return -EINVAL;
>> +
>>  	tsm->type = pci_tsm_type(pdev);
>>  	tsm->pdev = pdev;
>
> Add blank line before comment.
>
>> +	/*
>> +	 * No reference needed because when we destroy
>> +	 * dsm_dev all the tdis get destroyed before that.
>
> "tdi" looks like an initialism, which would normally be capitalized.
>
>> +	 */
>> +	tsm->dsm_dev = dsm_dev;
>> +	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(pci_tsm_initialize);
>>  
>> @@ -447,7 +487,8 @@ int pci_tsm_pf0_initialize(struct pci_dev *pdev, struct pci_tsm_pf0 *tsm)
>>  	}
>>  
>>  	tsm->state = PCI_TSM_INIT;
>> -	pci_tsm_initialize(pdev, &tsm->tsm);
>> +	if (pci_tsm_initialize(pdev, &tsm->tsm))
>> +		return -ENODEV;
>>  
>>  	return 0;
>>  }
>> @@ -612,32 +653,6 @@ int pci_tsm_doe_transfer(struct pci_dev *pdev, enum pci_doe_proto type,
>>  }
>>  EXPORT_SYMBOL_GPL(pci_tsm_doe_transfer);
>>  
>> -/* lookup the Device Security Manager (DSM) pf0 for @pdev */
>> -static struct pci_dev *dsm_dev_get(struct pci_dev *pdev)
>> -{
>> -	struct pci_dev *uport_pf0;
>> -
>> -	struct pci_dev *pf0 __free(pci_dev_put) = pf0_dev_get(pdev);
>> -	if (!pf0)
>> -		return NULL;
>> -
>> -	/* Check that @pf0 was not initialized as PCI_TSM_DOWNSTREAM */
>> -	if (pf0->tsm && pf0->tsm->type == PCI_TSM_PF0)
>> -		return no_free_ptr(pf0);
>> -
>> -	/*
>> -	 * For cases where a switch may be hosting TDISP services on
>> -	 * behalf of downstream devices, check the first usptream port
>> -	 * relative to this endpoint.
>> -	 */
>> -	if (!pdev->dev.parent || !pdev->dev.parent->parent)
>> -		return NULL;
>> -
>> -	uport_pf0 = to_pci_dev(pdev->dev.parent->parent);
>> -	if (!uport_pf0->tsm)
>> -		return NULL;
>> -	return pci_dev_get(uport_pf0);
>> -}
>
> This code move looks like it could be a separate patch that only moves
> (and fixes the typos I mentioned).
>
> Then a second patch could do what the subject claims (moving dsm_dev
> from pci_tdi to pci_tsm) so it's not buried in the simple move.
>
>>  /* Only implement non-interruptible lock for now */
>>  static struct mutex *tdi_ops_lock(struct pci_dev *pf0_dev)
>> @@ -695,7 +710,6 @@ int pci_tsm_bind(struct pci_dev *pdev, struct kvm *kvm, u64 tdi_id)
>>  		return -ENXIO;
>>  
>>  	tdi->pdev = pdev;
>> -	tdi->dsm_dev = dsm_dev;
>>  	tdi->kvm = kvm;
>>  	pdev->tsm->tdi = tdi;
>>  
>> diff --git a/include/linux/pci-tsm.h b/include/linux/pci-tsm.h
>> index 1920ca591a42..0d4303726b25 100644
>> --- a/include/linux/pci-tsm.h
>> +++ b/include/linux/pci-tsm.h
>> @@ -38,7 +38,6 @@ enum pci_tsm_type {
>>   */
>>  struct pci_tdi {
>>  	struct pci_dev *pdev;
>> -	struct pci_dev *dsm_dev;
>>  	struct kvm *kvm;
>>  };
>>  
>> @@ -56,6 +55,7 @@ struct pci_tdi {
>>   */
>>  struct pci_tsm {
>>  	struct pci_dev *pdev;
>> +	struct pci_dev *dsm_dev;
>>  	enum pci_tsm_type type;
>>  	struct pci_tdi *tdi;
>>  };
>> @@ -173,7 +173,7 @@ void pci_tsm_core_unregister(const struct pci_tsm_ops *ops);
>>  int pci_tsm_doe_transfer(struct pci_dev *pdev, enum pci_doe_proto type,
>>  			 const void *req, size_t req_sz, void *resp,
>>  			 size_t resp_sz);
>> -void pci_tsm_initialize(struct pci_dev *pdev, struct pci_tsm *tsm);
>> +int pci_tsm_initialize(struct pci_dev *pdev, struct pci_tsm *tsm);
>>  int pci_tsm_pf0_initialize(struct pci_dev *pdev, struct pci_tsm_pf0 *tsm);
>>  int pci_tsm_bind(struct pci_dev *pdev, struct kvm *kvm, u64 tdi_id);
>>  int pci_tsm_unbind(struct pci_dev *pdev);
>> -- 
>> 2.43.0
>> 

Thanks for the review comments. I'll update the patch with the suggested changes.

-aneesh