Re: [PATCH v7 8/9] PCI/IDE: Report available IDE streams

[<dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> On Thu, 23 Oct 2025 19:04:17 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > The limited number of link-encryption (IDE) streams that a given set of
> > host bridges supports is a platform specific detail. Provide
> > pci_ide_init_nr_streams() as a generic facility for either platform TSM
> > drivers, or PCI core native IDE, to report the number available streams.
> > After invoking pci_ide_init_nr_streams() an "available_secure_streams"
> > attribute appears in PCI host bridge sysfs to convey that count.
> > 
> > Introduce a device-type, @pci_host_bridge_type, now that both a release
> > method and sysfs attribute groups are being specified for all 'struct
> > pci_host_bridge' instances.
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Samuel Ortiz <sameo@rivosinc.com>
> > Cc: Alexey Kardashevskiy <aik@amd.com>
> > Cc: Xu Yilun <yilun.xu@linux.intel.com>
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> New day, new comments.  Nothing huge, but I would avoid the defining
> an attr group to NULL as that feels like storing up bugs for the future.
> 
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index d3f16be40102..8b356dd09105 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -616,9 +616,12 @@ static inline void pci_doe_sysfs_teardown(struct pci_dev *pdev) { }
> >  #ifdef CONFIG_PCI_IDE
> >  void pci_ide_init(struct pci_dev *dev);
> >  void pci_ide_init_host_bridge(struct pci_host_bridge *hb);
> > +extern const struct attribute_group pci_ide_attr_group;
> > +#define PCI_IDE_ATTR_GROUP (&pci_ide_attr_group)
> >  #else
> >  static inline void pci_ide_init(struct pci_dev *dev) { }
> >  static inline void pci_ide_init_host_bridge(struct pci_host_bridge *hb) { }
> > +#define PCI_IDE_ATTR_GROUP NULL
> 
> This only works if we assume nothing else ever ends up in pci_host_bridge_groups.
> i.e. it's fragile - think of someone adding something after this.
> Whilst I don't like ifdefs inline, there isn't a better option for this case that
> I can think of.

Fair enough, a comment explaining the hazard is about as ugly as the
ifdef.

[..]
> > +/**
> > + * pci_ide_set_nr_streams() - sets size of the pool of IDE Stream resources
> > + * @hb: host bridge boundary for the stream pool
> > + * @nr: number of streams
> > + *
> > + * Platform PCI init and/or expert test module use only. Limit IDE
> > + * Stream establishment by setting the number of stream resources
> > + * available at the host bridge. Platform init code must set this before
> > + * the first pci_ide_stream_alloc() call if the platform has less than the
> > + * default of 256 streams per host-bridge.
> > + *
> > + * The "PCI_IDE" symbol namespace is required because this is typically
> > + * a detail that is settled in early PCI init. I.e. this export is not
> > + * for endpoint drivers.
> > + */
> > +void pci_ide_set_nr_streams(struct pci_host_bridge *hb, u16 nr)
> > +{
> > +	if (nr > 256)
> > +		nr = 256;
> 
> 
> hb->nr_ide_streams = min(nr, 256);
> maybe

Sure.

 8:  8bcdc030d445 !  8:  37a609df4dd5 PCI/IDE: Report available IDE streams
    @@ drivers/pci/pci.h: static inline void pci_doe_sysfs_teardown(struct pci_dev *pde
      void pci_ide_init(struct pci_dev *dev);
      void pci_ide_init_host_bridge(struct pci_host_bridge *hb);
     +extern const struct attribute_group pci_ide_attr_group;
    -+#define PCI_IDE_ATTR_GROUP (&pci_ide_attr_group)
      #else
      static inline void pci_ide_init(struct pci_dev *dev) { }
      static inline void pci_ide_init_host_bridge(struct pci_host_bridge *hb) { }
    -+#define PCI_IDE_ATTR_GROUP NULL
    - #endif
    - 
    - #ifdef CONFIG_PCI_TSM
     
      ## include/linux/pci-ide.h ##
     @@ include/linux/pci-ide.h: struct pci_ide {
    @@ drivers/pci/ide.c: void pci_ide_init_host_bridge(struct pci_host_bridge *hb)
     + */
     +void pci_ide_set_nr_streams(struct pci_host_bridge *hb, u16 nr)
     +{
    -+	if (nr > 256)
    -+		nr = 256;
    -+	hb->nr_ide_streams = nr;
    ++	hb->nr_ide_streams = min(nr, 256);
     +	WARN_ON_ONCE(!ida_is_empty(&hb->ide_stream_ida));
     +	sysfs_update_group(&hb->dev.kobj, &pci_ide_attr_group);
     +}
    @@ drivers/pci/probe.c: static void pci_release_host_bridge_dev(struct device *dev)
      }
      
     +static const struct attribute_group *pci_host_bridge_groups[] = {
    -+	PCI_IDE_ATTR_GROUP,
    ++#ifdef CONFIG_PCI_IDE
    ++	&pci_ide_attr_group,
    ++#endif
     +	NULL
     +};
     +