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

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

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