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

dan.j.williams
2025-11-05T23:51:56Z

Jonathan Cameron wrote:
> 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

Sure.

> > +}
> > +
> > +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.

...but we do not carry on. When claim_stream() fails pci_ide_init()
fails. So this dev_info() is there to clue in an admin wondering why IDE
capabilities may not be available for some devices.

[..]
> > @@ -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.

Explained above.

[..]
> > @@ -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?

No, it does not, rename sounds good.

diff --git a/drivers/pci/ide.c b/drivers/pci/ide.c
index 33b3c54c62a1..60c22a6ee322 100644
--- a/drivers/pci/ide.c
+++ b/drivers/pci/ide.c
@@ -398,9 +394,9 @@ struct pci_ide_stream_id {
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)
+static struct pci_ide_stream_id *
+request_stream_id(struct pci_host_bridge *hb, u8 stream_id,
+ struct pci_ide_stream_id *sid)
{
if (!reserve_stream_id(hb, stream_id))
return NULL;
@@ -437,7 +433,7 @@ int pci_ide_stream_register(struct pci_ide *ide)
}

struct pci_ide_stream_id *sid __free(free_stream_id) =
- alloc_stream_id(hb, ide->stream_id, &__sid);
+ request_stream_id(hb, ide->stream_id, &__sid);
if (!sid) {
pci_err(pdev, "Setup fail: Stream ID %d in use\n", ide->stream_id);
return -EBUSY;
>
> > +{
> > + 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.

Interesting. It is rare to have this kind of request model in core code.
Most of the "discover the platform published resource + request it"
happens in driver probe contexts and devm cleanup is available for that
(e.g. devm_request_mem_region()). If we can find more users for such a
scope-based-cleanup model I would cheer on the person that wanted to
take that on.

Otherwise the "magic storage" approach is also taken with:

struct stream_index __stream[PCI_IDE_HB + 1];
...
alloc_stream_index(..., &__stream[...]);