On 04/07/2024 13:41, Greg KH wrote:
External email: Use caution opening links or attachments
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.
I am doing it since I want the name memory to be freed in case of
sysfs_add_file_to_group() fails.
I don’t see a cleaner way available with cleanup.h.
Assigning this "deep" in a sysfs structure is not ok.
when creating sysfs dynamically, there isn't a cleaner way to assign the
name memory.
The closest and exact same use case for pci irq sysfs which uses dynamic
sysfs is msi_sysfs_populate_desc().
It does not use cleanup.h but still has to assign.
I Don’t have any other ideas on how to implement it any more elegantly
with cleanup.h.
Do you prefer to assign it before sysfs_add_file_to_group() similar to
msi_sysfs_populate_desc() and avoid cleanup.h for now?
+ 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...
Driver must do this for allocated irq. So xa_load cannot fail.
In previous versions we had WARN_ON to catch driver bugs, but you didn’t
like it.
I think this is fine the way it is in v9.
greg k-h
