Re: [PATCH V2] driver core: auxiliary bus: Fix sysfs creation on bind

[Greg Kroah-Hartman]

On Fri, Feb 20, 2026 at 12:59:45AM +0100, Danilo Krummrich wrote:
> On Thu Feb 19, 2026 at 10:04 PM CET, Tariq Toukan wrote:
> > +/**
> > + * auxiliary_device_sysfs_irq_dir_init - initialize the IRQ sysfs directory
> > + * @auxdev: auxiliary bus device to initialize the sysfs directory.
> > + *
> > + * This function should be called by drivers to initialize the IRQ directory
> > + * before adding any IRQ sysfs entries. The driver is responsible for ensuring
> > + * this function is called only once and for handling any concurrency control
> > + * if needed.
> > + *
> > + * Drivers must call auxiliary_device_sysfs_irq_dir_destroy() to clean up when
> > + * done.
> > + *
> > + * Return: zero on success or an error code on failure.
> > + */
> > +int auxiliary_device_sysfs_irq_dir_init(struct auxiliary_device *auxdev)
> >  {
> > -	int ret = 0;
> > -
> > -	guard(mutex)(&auxdev->sysfs.lock);
> > -	if (auxdev->sysfs.irq_dir_exists)
> > -		return 0;
> > +	int ret;
> >  
> > -	ret = devm_device_add_group(&auxdev->dev, &auxiliary_irqs_group);
> > +	ret = sysfs_create_group(&auxdev->dev.kobj, &auxiliary_irqs_group);
> >  	if (ret)
> >  		return ret;
> >  
> > -	auxdev->sysfs.irq_dir_exists = true;
> >  	xa_init(&auxdev->sysfs.irqs);
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_dir_init);
> > +
> > +/**
> > + * auxiliary_device_sysfs_irq_dir_destroy - destroy the IRQ sysfs directory
> > + * @auxdev: auxiliary bus device to destroy the sysfs directory.
> > + *
> > + * This function should be called by drivers to clean up the IRQ directory
> > + * after all IRQ sysfs entries have been removed. The driver is responsible
> > + * for ensuring all IRQs are removed before calling this function.
> > + */
> > +void auxiliary_device_sysfs_irq_dir_destroy(struct auxiliary_device *auxdev)
> > +{
> > +	xa_destroy(&auxdev->sysfs.irqs);
> > +	sysfs_remove_group(&auxdev->dev.kobj, &auxiliary_irqs_group);
> > +}
> > +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_dir_destroy);
> >  
> >  /**
> >   * auxiliary_device_sysfs_irq_add - add a sysfs entry for the given IRQ
> > @@ -45,7 +70,8 @@ static int auxiliary_irq_dir_prepare(struct auxiliary_device *auxdev)
> >   * @irq: The associated interrupt number.
> >   *
> >   * This function should be called after auxiliary device have successfully
> > - * received the irq.
> > + * received the irq. The driver must call auxiliary_device_sysfs_irq_dir_init()
> > + * before calling this function for the first time.
> 
> I'm not convinced by this approach. This adds two new sources of bugs for
> drivers.
> 
>   1. Drivers can now forget to call auxiliary_device_sysfs_irq_dir_init()
>      *before* auxiliary_device_sysfs_irq_add().
> 
>   2. Drivers can forget to call auxiliary_device_sysfs_irq_dir_destroy().
> 
> Instead, I suggest to keep the current approach and just replace
> devm_device_add_group() with devm_auxiliary_device_add_group(), which in its
> devres callback additionally clears auxdev->sysfs.irq_dir_exists.
> 
> In terms of the auxdev->sysfs.lock, I think this can still be removed, as it
> wasn't needed in the first place.
> 
> auxiliary_device_sysfs_irq_add() must only be called from a scope where the
> auxiliary device is guaranteed to be bound, so there can't be a concurrent
> unbind.
> 
> There may only be multiple concurrent calls to auxiliary_device_sysfs_irq_add()
> itself, and in this case irq_dir_exists can just be an atomic.
> 
> Yes, we're still stuck with an atomic for irq_dir_exists, but the driver API
> remains much simpler and less error prone.

I agree, your recommendations make sense.  And I still hate this irq
addition to auxdevices :(

thanks,

greg k-h