Re: [PATCH 3/6] PCI/IDE: Initialize an ID for all IDE streams

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Tue,  4 Nov 2025 20:00:52 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> The PCIe spec defines two types of streams - selective and link.  Each
> stream has an ID from the same bucket so a stream ID does not tell the
> type.  The spec defines an "enable" bit for every stream and required
> stream IDs to be unique among all enabled stream but there is no such
> requirement for disabled streams.
> 
> However, when IDE_KM is programming keys, an IDE-capable device needs
> to know the type of stream being programmed to write it directly to
> the hardware as keys are relatively large, possibly many of them and
> devices often struggle with keeping around rather big data not being
> used.
> 
> Walk through all streams on a device and initialise the IDs to some
> unique number, both link and selective.
> 
> The weakest part of this proposal is the host bridge ide_stream_ids_ida.
> Technically, a Stream ID only needs to be unique within a given partner
> pair. However, with "anonymous" / unassigned streams there is no convenient
> place to track the available ids. Proceed with an ida in the host bridge
> for now, but consider moving this tracking to be an ide_stream_ids_ida per
> device.
> 
> Co-developed-by: Alexey Kardashevskiy <aik@amd.com>
> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

A small side discussion on whether a new type of cleanup helper makes
sense here for allocations that need to stash some data which is never
used except in __free. Bit of an odd corner case but could see something
similar for pool allocators (Which is kind of what we have here).

> ---
>  drivers/pci/pci.h       |   2 +
>  include/linux/pci-ide.h |   6 ++
>  include/linux/pci.h     |   1 +
>  drivers/pci/ide.c       | 133 ++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/remove.c    |   1 +
>  5 files changed, 143 insertions(+)
> 
> diff --git a/drivers/pci/ide.c b/drivers/pci/ide.c
> index d7fc741f3a26..33b3c54c62a1 100644
> --- a/drivers/pci/ide.c
> +++ b/drivers/pci/ide.c
> @@ -35,8 +35,54 @@ static int sel_ide_offset(struct pci_dev *pdev,
>  				settings->stream_index, pdev->nr_ide_mem);
>  }
>  
> +static bool reserve_stream_index(struct pci_dev *pdev, u8 idx)
> +{
> +	int ret;
> +
> +	ret = ida_alloc_range(&pdev->ide_stream_ida, idx, idx, GFP_KERNEL);
> +	if (ret < 0)
> +		return false;
> +	return true;

	return ret >= 0; perhaps


> +}
> +
> +static bool reserve_stream_id(struct pci_host_bridge *hb, u8 id)
> +{
> +	int ret;
> +
> +	ret = ida_alloc_range(&hb->ide_stream_ids_ida, id, id, GFP_KERNEL);
> +	if (ret < 0)
> +		return false;
> +	return true;

	return ret >= 0;

> +}
> +
> +static bool claim_stream(struct pci_host_bridge *hb, u8 stream_id,
> +			 struct pci_dev *pdev, u8 stream_idx)
> +{
> +	dev_info(&hb->dev, "Stream ID %d active at init\n", stream_id);
> +	if (!reserve_stream_id(hb, stream_id)) {
> +		dev_info(&hb->dev, "Failed to claim %s Stream ID %d\n",
> +			 stream_id == PCI_IDE_RESERVED_STREAM_ID ? "reserved" :
> +								   "active",
> +			 stream_id);

Good to have a comment on why we carry on anyway.

> +		return false;
> +	}
> +
> +	/* No stream index to reserve in the Link IDE case */
> +	if (!pdev)
> +		return true;
> +
> +	if (!reserve_stream_index(pdev, stream_idx)) {
> +		pci_info(pdev, "Failed to claim active Selective Stream %d\n",
> +			 stream_idx);
Likewise. Why is this not an error.
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  void pci_ide_init(struct pci_dev *pdev)
>  {
> +	struct pci_host_bridge *hb = pci_find_host_bridge(pdev->bus);
>  	u16 nr_link_ide, nr_ide_mem, nr_streams;
>  	u16 ide_cap;
>  	u32 val;
> @@ -83,6 +129,7 @@ void pci_ide_init(struct pci_dev *pdev)
>  		int pos = __sel_ide_offset(ide_cap, nr_link_ide, i, nr_ide_mem);
>  		int nr_assoc;
>  		u32 val;
> +		u8 id;
>  
>  		pci_read_config_dword(pdev, pos + PCI_IDE_SEL_CAP, &val);
>  
> @@ -98,6 +145,51 @@ void pci_ide_init(struct pci_dev *pdev)
>  		}
>  
>  		nr_ide_mem = nr_assoc;
> +
> +		/*
> +		 * Claim Stream IDs and Selective Stream blocks that are already
> +		 * active on the device
> +		 */
> +		pci_read_config_dword(pdev, pos + PCI_IDE_SEL_CTL, &val);
> +		id = FIELD_GET(PCI_IDE_SEL_CTL_ID, val);
> +		if ((val & PCI_IDE_SEL_CTL_EN) &&
> +		    !claim_stream(hb, id, pdev, i))
> +			return;
Related to above, I'm not sure why we just eat this problem with a print.

> +	}
> +
> +	/* Reserve link stream-ids that are already active on the device */
> +	for (u16 i = 0; i < nr_link_ide; ++i) {
> +		int pos = ide_cap + PCI_IDE_LINK_STREAM_0 + i * PCI_IDE_LINK_BLOCK_SIZE;
> +		u8 id;
> +
> +		pci_read_config_dword(pdev, pos + PCI_IDE_LINK_CTL_0, &val);
> +		id = FIELD_GET(PCI_IDE_LINK_CTL_ID, val);
> +		if ((val & PCI_IDE_LINK_CTL_EN) &&
> +		    !claim_stream(hb, id, NULL, -1))
> +			return;
> +	}


> @@ -301,6 +393,28 @@ void pci_ide_stream_release(struct pci_ide *ide)
>  }
>  EXPORT_SYMBOL_GPL(pci_ide_stream_release);
>  
> +struct pci_ide_stream_id {
> +	struct pci_host_bridge *hb;
> +	u8 stream_id;
> +};
> +
> +static struct pci_ide_stream_id *alloc_stream_id(struct pci_host_bridge *hb,
> +						 u8 stream_id,
> +						 struct pci_ide_stream_id *sid)

Doesn't feel like an allocation function to me. Maybe a rename if
it doesn't gain some allocation abilities later?

> +{
> +	if (!reserve_stream_id(hb, stream_id))
> +		return NULL;
> +
> +	*sid = (struct pci_ide_stream_id) {
> +		.hb = hb,
> +		.stream_id = stream_id,
> +	};
> +
> +	return sid;
> +}
> +DEFINE_FREE(free_stream_id, struct pci_ide_stream_id *,
> +	    if (_T) ida_free(&_T->hb->ide_stream_ids_ida, _T->stream_id))
> +
>  /**
>   * pci_ide_stream_register() - Prepare to activate an IDE Stream
>   * @ide: IDE settings descriptor
> @@ -313,6 +427,7 @@ int pci_ide_stream_register(struct pci_ide *ide)
>  {
>  	struct pci_dev *pdev = ide->pdev;
>  	struct pci_host_bridge *hb = pci_find_host_bridge(pdev->bus);
> +	struct pci_ide_stream_id __sid;
>  	u8 ep_stream, rp_stream;
>  	int rc;
>  
> @@ -321,6 +436,13 @@ int pci_ide_stream_register(struct pci_ide *ide)
>  		return -ENXIO;
>  	}
>  
> +	struct pci_ide_stream_id *sid __free(free_stream_id) =
> +		alloc_stream_id(hb, ide->stream_id, &__sid);

Given the use of __sid as magic storage, I wonder if this can
be a CLASS with that storage wrapped up alongside a flag
we clear to make the destructor a no op. Similar to what happens for
spin_lock_irqsave where we stash flags etc via __DEFINE_UNLOCK_GUARD() 

Would need something a little more complex than current retain_and_null_ptr()
as it would need to set _T.ptr = NULL rather that _T = NULL.


> +	if (!sid) {
> +		pci_err(pdev, "Setup fail: Stream ID %d in use\n", ide->stream_id);
> +		return -EBUSY;
> +	}
> +
>  	ep_stream = ide->partner[PCI_IDE_EP].stream_index;
>  	rp_stream = ide->partner[PCI_IDE_RP].stream_index;
>  	const char *name __free(kfree) = kasprintf(GFP_KERNEL, "stream%d.%d.%d",
> @@ -335,6 +457,9 @@ int pci_ide_stream_register(struct pci_ide *ide)
>  
>  	ide->name = no_free_ptr(name);
>  
> +	/* Stream ID reservation recorded in @ide is now successfully registered */
> +	retain_and_null_ptr(sid);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(pci_ide_stream_register);