From: <dan.j.williams@intel.com>
To: Jonathan Cameron <jonathan.cameron@huawei.com>,
Dan Williams <dan.j.williams@intel.com>
Cc: <linux-pci@vger.kernel.org>, <linux-coco@lists.linux.dev>,
<bhelgaas@google.com>, <aneesh.kumar@kernel.org>,
<yilun.xu@linux.intel.com>, <aik@amd.com>
Subject: Re: [PATCH 3/6] PCI/IDE: Initialize an ID for all IDE streams
Date: Wed, 5 Nov 2025 15:51:49 -0800 [thread overview]
Message-ID: <690be315ed059_74f761007a@dwillia2-mobl4.notmuch> (raw)
In-Reply-To: <20251105152704.00002741@huawei.com>
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[...]);
next prev parent reply other threads:[~2025-11-05 23:51 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-05 4:00 [PATCH 0/6] PCI/TSM: Finalize "Link" TSM infrastructure Dan Williams
2025-11-05 4:00 ` [PATCH 1/6] resource: Introduce resource_assigned() for discerning active resources Dan Williams
2025-11-05 9:17 ` Jonathan Cameron
2025-11-05 21:57 ` dan.j.williams
2025-11-05 4:00 ` [PATCH 2/6] PCI/IDE: Add Address Association Register setup for downstream MMIO Dan Williams
2025-11-05 9:58 ` Jonathan Cameron
2025-11-05 23:04 ` dan.j.williams
2025-11-10 11:49 ` Jonathan Cameron
2025-11-05 4:00 ` [PATCH 3/6] PCI/IDE: Initialize an ID for all IDE streams Dan Williams
2025-11-05 15:27 ` Jonathan Cameron
2025-11-05 23:51 ` dan.j.williams [this message]
2025-11-10 11:52 ` Jonathan Cameron
2025-11-05 4:00 ` [PATCH 4/6] PCI/TSM: Add pci_tsm_bind() helper for instantiating TDIs Dan Williams
2025-11-05 4:59 ` Aneesh Kumar K.V
2025-11-05 21:49 ` dan.j.williams
2025-11-05 15:31 ` Jonathan Cameron
2025-11-06 0:11 ` dan.j.williams
2025-11-05 4:00 ` [PATCH 5/6] PCI/TSM: Add pci_tsm_guest_req() for managing TDIs Dan Williams
2025-11-05 15:38 ` Jonathan Cameron
2025-11-06 0:13 ` dan.j.williams
2025-11-05 4:00 ` [PATCH 6/6] PCI/TSM: Add 'dsm' and 'bound' attributes for dependent functions Dan Williams
2025-11-05 17:53 ` Jonathan Cameron
2025-11-13 12:10 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=690be315ed059_74f761007a@dwillia2-mobl4.notmuch \
--to=dan.j.williams@intel.com \
--cc=aik@amd.com \
--cc=aneesh.kumar@kernel.org \
--cc=bhelgaas@google.com \
--cc=jonathan.cameron@huawei.com \
--cc=linux-coco@lists.linux.dev \
--cc=linux-pci@vger.kernel.org \
--cc=yilun.xu@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).