Re: [PATCH net-next v7 1/2] driver core: auxiliary bus: show auxiliary device IRQs

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Jun 18, 2024 at 06:09:01PM +0300, Shay Drory wrote:
> diff --git a/drivers/base/auxiliary_sysfs.c b/drivers/base/auxiliary_sysfs.c
> new file mode 100644
> index 000000000000..3f112fd26e72
> --- /dev/null
> +++ b/drivers/base/auxiliary_sysfs.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/slab.h>
> +
> +struct auxiliary_irq_info {
> +	struct device_attribute sysfs_attr;
> +};
> +
> +static struct attribute *auxiliary_irq_attrs[] = {
> +	NULL
> +};
> +
> +static const struct attribute_group auxiliary_irqs_group = {
> +	.name = "irqs",
> +	.attrs = auxiliary_irq_attrs,
> +};
> +
> +static int auxiliary_irq_dir_prepare(struct auxiliary_device *auxdev)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&auxdev->lock);
> +	if (auxdev->dir_exists)
> +		goto unlock;

You do know about cleanup.h, right?  Please use it.

But what exactly are you trying to protect here?  How will you race and
add two irqs at the same time?  Driver probe is always single threaded,
so what would be calling this at the same time from multiple places?


> +
> +	xa_init(&auxdev->irqs);
> +	ret = devm_device_add_group(&auxdev->dev, &auxiliary_irqs_group);
> +	if (!ret)
> +		auxdev->dir_exists = 1;
> +
> +unlock:
> +	mutex_unlock(&auxdev->lock);
> +	return ret;
> +}
> +
> +/**
> + * auxiliary_device_sysfs_irq_add - add a sysfs entry for the given IRQ
> + * @auxdev: auxiliary bus device to add the sysfs entry.
> + * @irq: The associated interrupt number.
> + *
> + * This function should be called after auxiliary device have successfully
> + * received the irq.
> + *
> + * Return: zero on success or an error code on failure.
> + */
> +int auxiliary_device_sysfs_irq_add(struct auxiliary_device *auxdev, int irq)
> +{
> +	struct device *dev = &auxdev->dev;
> +	struct auxiliary_irq_info *info;
> +	int ret;
> +
> +	ret = auxiliary_irq_dir_prepare(auxdev);
> +	if (ret)
> +		return ret;
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	sysfs_attr_init(&info->sysfs_attr.attr);
> +	info->sysfs_attr.attr.name = kasprintf(GFP_KERNEL, "%d", irq);
> +	if (!info->sysfs_attr.attr.name) {
> +		ret = -ENOMEM;
> +		goto name_err;
> +	}
> +
> +	ret = xa_insert(&auxdev->irqs, irq, info, GFP_KERNEL);

So no lock happening here, either use it always, or not at all?


> +	if (ret)
> +		goto auxdev_xa_err;
> +
> +	ret = sysfs_add_file_to_group(&dev->kobj, &info->sysfs_attr.attr,
> +				      auxiliary_irqs_group.name);

You do know that you are never going to see these files from the
userspace library tools that watch sysfs, right?  libudev will never see
them as you are adding them AFTER the device is created.

So, because of that, who is really going to use these files?


> +	if (ret)
> +		goto sysfs_add_err;
> +
> +	return 0;
> +
> +sysfs_add_err:
> +	xa_erase(&auxdev->irqs, irq);
> +auxdev_xa_err:
> +	kfree(info->sysfs_attr.attr.name);
> +name_err:
> +	kfree(info);

Again, cleanup.h is your friend.

> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_add);
> +
> +/**
> + * auxiliary_device_sysfs_irq_remove - remove a sysfs entry for the given IRQ
> + * @auxdev: auxiliary bus device to add the sysfs entry.
> + * @irq: the IRQ to remove.
> + *
> + * This function should be called to remove an IRQ sysfs entry.
> + */
> +void auxiliary_device_sysfs_irq_remove(struct auxiliary_device *auxdev, int irq)
> +{
> +	struct auxiliary_irq_info *info = xa_load(&auxdev->irqs, irq);
> +	struct device *dev = &auxdev->dev;
> +
> +	sysfs_remove_file_from_group(&dev->kobj, &info->sysfs_attr.attr,
> +				     auxiliary_irqs_group.name);
> +	xa_erase(&auxdev->irqs, irq);
> +	kfree(info->sysfs_attr.attr.name);
> +	kfree(info);
> +}
> +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_remove);
> diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
> index de21d9d24a95..96be140bd1ff 100644
> --- a/include/linux/auxiliary_bus.h
> +++ b/include/linux/auxiliary_bus.h
> @@ -58,6 +58,7 @@
>   *       in
>   * @name: Match name found by the auxiliary device driver,
>   * @id: unique identitier if multiple devices of the same name are exported,
> + * @irqs: irqs xarray contains irq indices which are used by the device,
>   *
>   * An auxiliary_device represents a part of its parent device's functionality.
>   * It is given a name that, combined with the registering drivers
> @@ -138,7 +139,10 @@
>  struct auxiliary_device {
>  	struct device dev;
>  	const char *name;
> +	struct xarray irqs;
> +	struct mutex lock; /* Protects "irqs" directory creation */

Protects it from what?


>  	u32 id;
> +	u8 dir_exists:1;

I don't think this is needed, but if it really is, just use a bool.


>  };
>  
>  /**
> @@ -212,8 +216,24 @@ int auxiliary_device_init(struct auxiliary_device *auxdev);
>  int __auxiliary_device_add(struct auxiliary_device *auxdev, const char *modname);
>  #define auxiliary_device_add(auxdev) __auxiliary_device_add(auxdev, KBUILD_MODNAME)
>  
> +#ifdef CONFIG_SYSFS
> +int auxiliary_device_sysfs_irq_add(struct auxiliary_device *auxdev, int irq);
> +void auxiliary_device_sysfs_irq_remove(struct auxiliary_device *auxdev,
> +				       int irq);

You can use longer lines :)

thanks,

greg k-h