| Message ID | 20240703073858.932299-2-shayd@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introduce auxiliary bus IRQs sysfs | expand |
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
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
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
On 7/5/24 07:35, 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? I've overlooked it earlier, sorry. easiest solution for "general" case would be: info->sysfs_attr.attr.name = no_free_ptr(name); ret = sysfs_add_file_to_group(&dev->kobj, &info->sysfs_attr.attr, auxiliary_irqs_group.name); if (ret) { /* freeing manualy since auto cleanup was * disabled by no_free_ptr() */ kfree(info->sysfs_attr.attr.name); goto sysfs_add_err; } but in your case it will be cleaner to alloc the space for name together with struct auxiliary_irq_info, by placing a char array there, either with static size or a flex one (if such case would be generic) going one step further would be be to reorder struct device_attribute and struct attribute fields to have @name as the last one and make it a flex array - but it is perhaps for another series ;) >>> +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 Perhaps this is more about trust boundaries?, I would like to learn something from this case :)
diff --git a/Documentation/ABI/testing/sysfs-bus-auxiliary b/Documentation/ABI/testing/sysfs-bus-auxiliary new file mode 100644 index 000000000000..cc856079690f --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-auxiliary @@ -0,0 +1,9 @@ +What: /sys/bus/auxiliary/devices/.../irqs/ +Date: April, 2024 +Contact: Shay Drory <shayd@nvidia.com> +Description: + The /sys/devices/.../irqs directory contains a variable set of + files, with each file is named as irq number similar to PCI PF + or VF's irq number located in msi_irqs directory. + These irq files are added and removed dynamically when an IRQ + is requested and freed respectively for the PCI SF. diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 3079bfe53d04..7fb21768ca36 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_NUMA) += node.o obj-$(CONFIG_MEMORY_HOTPLUG) += memory.o ifeq ($(CONFIG_SYSFS),y) obj-$(CONFIG_MODULES) += module.o +obj-$(CONFIG_AUXILIARY_BUS) += auxiliary_sysfs.o endif obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o obj-$(CONFIG_REGMAP) += regmap/ diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c index d3a2c40c2f12..55bde375150f 100644 --- a/drivers/base/auxiliary.c +++ b/drivers/base/auxiliary.c @@ -287,6 +287,7 @@ int auxiliary_device_init(struct auxiliary_device *auxdev) dev->bus = &auxiliary_bus_type; device_initialize(&auxdev->dev); + mutex_init(&auxdev->lock); return 0; } EXPORT_SYMBOL_GPL(auxiliary_device_init); diff --git a/drivers/base/auxiliary_sysfs.c b/drivers/base/auxiliary_sysfs.c new file mode 100644 index 000000000000..f4e267971d70 --- /dev/null +++ b/drivers/base/auxiliary_sysfs.c @@ -0,0 +1,111 @@ +// 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; + + guard(mutex)(&auxdev->lock); + if (auxdev->irq_dir_exists) + return 0; + + ret = devm_device_add_group(&auxdev->dev, &auxiliary_irqs_group); + if (ret) + return ret; + + auxdev->irq_dir_exists = true; + xa_init(&auxdev->irqs); + return 0; +} + +/** + * 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); + 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); + const char *name __free(kfree) = info->sysfs_attr.attr.name; + 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); +} +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_remove); diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h index de21d9d24a95..ee738379d5f2 100644 --- a/include/linux/auxiliary_bus.h +++ b/include/linux/auxiliary_bus.h @@ -58,6 +58,9 @@ * 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, + * @lock: Synchronize irq sysfs creation, + * @irq_dir_exists: whether "irqs" directory exists, * * 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 +141,10 @@ struct auxiliary_device { struct device dev; const char *name; + struct xarray irqs; + struct mutex lock; /* Synchronize irq sysfs creation */ u32 id; + bool irq_dir_exists; }; /** @@ -212,8 +218,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); +#else /* CONFIG_SYSFS */ +static inline int +auxiliary_device_sysfs_irq_add(struct auxiliary_device *auxdev, int irq) +{ + return 0; +} + +static inline void +auxiliary_device_sysfs_irq_remove(struct auxiliary_device *auxdev, int irq) {} +#endif + static inline void auxiliary_device_uninit(struct auxiliary_device *auxdev) { + mutex_destroy(&auxdev->lock); put_device(&auxdev->dev); }