Re: [PATCH v4 07/10] PCI/IDE: Add IDE establishment helpers

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

Jonathan Cameron wrote:
[..]
> A few minor things inline.
[..]
> > +/**
> > + * pci_ide_stream_enable() - try to enable a Selective IDE Stream
> > + * @pdev: PCIe device object for either a Root Port or Endpoint Partner Port
> > + * @ide: registered and setup IDE settings descriptor
> > + *
> > + * Activate the stream by writing to the Selective IDE Stream Control
> > + * Register, report whether the state successfully transitioned to
> > + * secure mode.
> and report

ack.

[..]
> > diff --git a/include/linux/pci-ide.h b/include/linux/pci-ide.h
> > new file mode 100644
> > index 000000000000..89c1ef0de841
> > --- /dev/null
> > +++ b/include/linux/pci-ide.h
> > @@ -0,0 +1,70 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
> ...
> 
> > +/**
> > + * struct pci_ide_partner - Per port pair Selective IDE Stream settings
> > + * @rid_start: Partner Port Requester ID range start
> > + * @rid_start: Partner Port Requester ID range end
> > + * @stream_index: Selective IDE Stream Register Block selection
> > + * @setup: flag to track whether to run pci_ide_stream_teardown for this parnter slot
> 
> partner.
> 
> > + * @enable: flag whether to run pci_ide_stream_disable for this parnter slot
> 
> same again.

yes.

> > +/**
> > + * struct pci_ide - PCIe Selective IDE Stream descriptor
> > + * @pdev: PCIe Endpoint in the pci_ide_partner pair
> > + * @partner: Per-partner settings
> per-partner maybe?  Capitalization seems a little random
> as mostly you have used them for spec terms, but Per-partner probably
> isn't one?

true.

> > + * @host_bridge_stream: track platform Stream ID
> > + * @stream_id: unique Stream ID (within Partner Port pairing)
> > + * @name: name of the established Selective IDE Stream in sysfs
> > + *
> > + * Negative @stream_id values indicate "uninitialized" on the
> > + * expectation that with TSM established IDE the TSM owns the stream_id
> > + * allocation.
> > + */
> > +struct pci_ide {
> > +	struct pci_dev *pdev;
> > +	struct pci_ide_partner partner[PCI_IDE_PARTNER_MAX];
> > +	u8 host_bridge_stream;
> > +	int stream_id;
> > +	const char *name;
> > +};
> 
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index a7353df51fea..cc83ae274601 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -538,6 +538,8 @@ struct pci_dev {
> >  	u16		ide_cap;	/* Link Integrity & Data Encryption */
> >  	u8		nr_ide_mem;	/* Address association resources for streams */
> >  	u8		nr_link_ide;	/* Link Stream count (Selective Stream offset) */
> > +	u8		nr_sel_ide;	/* Selective Stream count (register block allocator) */
> > +	DECLARE_BITMAP(ide_stream_map, CONFIG_PCI_IDE_STREAM_MAX);
> >  	unsigned int	ide_cfg:1;	/* Config cycles over IDE */
> >  	unsigned int	ide_tee_limit:1; /* Disallow T=0 traffic over IDE */
> >  #endif
> > @@ -607,6 +609,10 @@ struct pci_host_bridge {
> >  	int		domain_nr;
> >  	struct list_head windows;	/* resource_entry */
> >  	struct list_head dma_ranges;	/* dma ranges resource list */
> > +#ifdef CONFIG_PCI_IDE
> > +	u8 nr_ide_streams;		/* Track available vs in-use streams */
> 
> Which does it do?  Confusing comment.

Oh, true, I was going for a combo comment for nr_ide_streams and
ide_stream_map, but missed on the clarity. Make that relationship
clearer:

-       u8 nr_ide_streams;              /* Track available vs in-use streams */
+       u8 nr_ide_streams; /* Max streams possibly active in @ide_stream_map */