On Wed, Jul 03, 2024 at 10:38:57AM +0300, Shay Drory wrote:
> +/**
> + * 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.
> + * The driver is responsible to add a unique irq for the auxiliary device. The
> + * driver can invoke this function from multiple thread context safely for
> + * unique irqs of the auxiliary devices. The driver must not invoke this API
> + * multiple times if the irq is already added previously.
> + *
> + * Return: zero on success or an error code on failure.
> + */
> +int auxiliary_device_sysfs_irq_add(struct auxiliary_device *auxdev, int irq)
> +{
> + struct auxiliary_irq_info *info __free(kfree) = NULL;
> + struct device *dev = &auxdev->dev;
> + char *name __free(kfree) = NULL;
> + 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);
> + name = kasprintf(GFP_KERNEL, "%d", irq);
> + if (!name)
> + return -ENOMEM;
> +
> + ret = xa_insert(&auxdev->irqs, irq, info, GFP_KERNEL);
> + if (ret)
> + return ret;
> +
> + info->sysfs_attr.attr.name = name;
> + ret = sysfs_add_file_to_group(&dev->kobj, &info->sysfs_attr.attr,
> + auxiliary_irqs_group.name);
> + if (ret)
> + goto sysfs_add_err;
> +
> + info->sysfs_attr.attr.name = no_free_ptr(name);
This assignment of a name AFTER it has been created is odd. I think I
know why you are doing this, but please make it obvious and perhaps
solve it in a cleaner way. Assigning this "deep" in a sysfs structure
is not ok.
> + xa_store(&auxdev->irqs, irq, no_free_ptr(info), GFP_KERNEL);
> + return 0;
> +
> +sysfs_add_err:
> + xa_erase(&auxdev->irqs, irq);
> + 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.
> + * The driver must invoke this API when IRQ is released by the device.
> + */
> +void auxiliary_device_sysfs_irq_remove(struct auxiliary_device *auxdev, int irq)
> +{
> + struct auxiliary_irq_info *info __free(kfree) = xa_load(&auxdev->irqs, irq);
No verification that this is an actual entry before you dereferenced it?
Bold move...
greg k-h
