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

[Leon Romanovsky <leon@kernel.org>]

On Fri, Feb 20, 2026 at 07:34:02AM +0100, Greg Kroah-Hartman wrote:
> 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.

Unfortunately no, https://lore.kernel.org/all/20260220080413.GB10607@unreal/.

Just to summarize:
1. atomic is not a replacement for locking.
2. drivers already handle this irq thing.
3. init->add->remove->destroy is standard Linux kernel coding pattern.

Thanks