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

Jonathan Cameron
2025-11-10T11:52:59Z

> > > ---
> > > 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

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

Ok. Failure isn't an error as such, but stuff just doesn't get set up.
Fair enough.

> > > + 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[...]);
>
Agreed. Potentially this is something for another day.

Jonathan