Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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
