Re: [PATCH v5 04/10] PCI/TSM: Authenticate devices via platform TSM

[<dan.j.williams@intel.com>]

Alexey Kardashevskiy wrote:
> 
> 
> On 3/9/25 01:13, Aneesh Kumar K.V wrote:
> > Dan Williams <dan.j.williams@intel.com> writes:
> > 
> > ....
> >> +
> >> +/**
> >> + * struct pci_tsm - Core TSM context for a given PCIe endpoint
> >> + * @pdev: Back ref to device function, distinguishes type of pci_tsm context
> >> + * @dsm: PCI Device Security Manager for link operations on @pdev
> >> + * @ops: Link Confidentiality or Device Function Security operations
> >> + *
> >> + * This structure is wrapped by low level TSM driver data and returned by
> >> + * probe()/lock(), it is freed by the corresponding remove()/unlock().
> >> + *
> >> + * For link operations it serves to cache the association between a Device
> >> + * Security Manager (DSM) and the functions that manager can assign to a TVM.
> >> + * That can be "self", for assigning function0 of a TEE I/O device, a
> >> + * sub-function (SR-IOV virtual function, or non-function0
> >> + * multifunction-device), or a downstream endpoint (PCIe upstream switch-port as
> >> + * DSM).
> >> + */
> >> +struct pci_tsm {
> >> +	struct pci_dev *pdev;
> >> +	struct pci_dev *dsm;
> >>
> > 
> > struct pci_dev *dsm_dev?
> 
> Unless we start calling pci_tsm_pf0 instances "dsm", I'd keep it "dsm".

The per device data for the physical function 0 responsibilities is
called pci_tsm_pf0, the actual device that plays the DSM role in the
TDISP specification is dsm_dev.

> >> +	const struct pci_tsm_ops *ops;
> >> +};
> >> +
> >> +/**
> >> + * struct pci_tsm_pf0 - Physical Function 0 TDISP link context
> >> + * @base: generic core "tsm" context
> >> + * @lock: mutual exclustion for pci_tsm_ops invocation
> >> + * @doe_mb: PCIe Data Object Exchange mailbox
> >> + */
> >> +struct pci_tsm_pf0 {
> >> +	struct pci_tsm base;
> >>
> > 
> > struct pci_tsm base_tsm ?
> 
> It is usually pdev->tsm->... so it has "tsm" in the value, having another "tsm" is hardly useful imho.

It only shows up in a few places. I think it is ok.

> >> +	struct mutex lock;
> >> +	struct pci_doe_mb *doe_mb;
> >> +};
> >> +
> > 
> > 
> > Both the above will help when we have names likes
> > 
> > dsm->pci.base.pdev; vs dsm->pci.base_tsm.pdev;
> 
> What type this "dsm" of in this example? Currently it is pci_dev which has no "pci" member. Thanks,

True, not sure what Aneesh was thinking here, but that does not really
change my view of the above renames.

I do want to stop spinning this patch based on trivial naming issues. I
think at this point everyone has had a chance to weigh in with their
preferences. I know I have picked up some from you, Aneesh, and Yilun
against my first choice.

Let's please stop quibbling with the names post v6 unless Bjorn raises a
specific concern.