On Fri, Jul 05, 2024 at 08:35:33AM +0300, Shay Drori wrote: > > > 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? No, what msi_sysfs_populate_desc() does is not good, the only objection here is the assignment after-the-fact you are doing just to work around cleanup.h. Surely there's a better way to tell it not to free the pointer at this point in time other than this. > > > + 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. Yes, because if something can happen, you handle the error properly, you don't reboot a machine. > I think this is fine the way it is in v9. No, you are now causing a NULL dereference (or close to it) if something went wrong. Properly check this and handle it correctly. thanks, greg k-h