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
>