Message-ID: <42af42b6-ccdd-4d0d-8e5a-306c74f330f4@nvidia.com>
Date: Mon, 17 Jun 2024 09:38:30 +0300
From: Shay Drori <shayd@...dia.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: <netdev@...r.kernel.org>, <pabeni@...hat.com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <edumazet@...gle.com>, <david.m.ertman@...el.com>,
	<rafael@...nel.org>, <ira.weiny@...el.com>, <linux-rdma@...r.kernel.org>,
	<leon@...nel.org>, <tariqt@...dia.com>, Parav Pandit <parav@...dia.com>
Subject: Re: [PATCH net-next v6 1/2] driver core: auxiliary bus: show
 auxiliary device IRQs



On 13/06/2024 19:33, Greg KH wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Jun 13, 2024 at 07:19:11PM +0300, Shay Drory wrote:
>> PCI subfunctions (SF) are anchored on the auxiliary bus. PCI physical
>> and virtual functions are anchored on the PCI bus. The irq information
>> of each such function is visible to users via sysfs directory "msi_irqs"
>> containing files for each irq entry. However, for PCI SFs such
>> information is unavailable. Due to this users have no visibility on IRQs
>> used by the SFs.
>> Secondly, an SF can be multi function device supporting rdma, netdevice
>> and more. Without irq information at the bus level, the user is unable
>> to view or use the affinity of the SF IRQs.
>>
>> Hence to match to the equivalent PCI PFs and VFs, add "irqs" directory,
>> for supporting auxiliary devices, containing file for each irq entry.
>>
>> For example:
>> $ ls /sys/bus/auxiliary/devices/mlx5_core.sf.1/irqs/
>> 50  51  52  53  54  55  56  57  58
>>
>> Reviewed-by: Parav Pandit <parav@...dia.com>
>> Signed-off-by: Shay Drory <shayd@...dia.com>
>>
>> ---
>> v5-v6:
>> - removed concept of shared and exclusive and hence global xarray (Greg)
>> v4-v5:
>> - restore global mutex and replace refcount_t with simple integer (Greg)
>> v3->4:
>> - remove global mutex (Przemek)
>> v2->v3:
>> - fix function declaration in case SYSFS isn't defined
>> v1->v2:
>> - move #ifdefs from drivers/base/auxiliary.c to
>>    include/linux/auxiliary_bus.h (Greg)
>> - use EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL (Greg)
>> - Fix kzalloc(ref) to kzalloc(*ref) (Simon)
>> - Add return description in auxiliary_device_sysfs_irq_add() kdoc (Simon)
>> - Fix auxiliary_irq_mode_show doc (kernel test boot)
>> ---
>>   Documentation/ABI/testing/sysfs-bus-auxiliary |  7 ++
>>   drivers/base/auxiliary.c                      | 96 ++++++++++++++++++-
>>   include/linux/auxiliary_bus.h                 | 24 ++++-
>>   3 files changed, 124 insertions(+), 3 deletions(-)
>>   create mode 100644 Documentation/ABI/testing/sysfs-bus-auxiliary
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-auxiliary b/Documentation/ABI/testing/sysfs-bus-auxiliary
>> new file mode 100644
>> index 000000000000..e8752c2354bc
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-auxiliary
>> @@ -0,0 +1,7 @@
>> +What:                /sys/bus/auxiliary/devices/.../irqs/
>> +Date:                April, 2024
>> +Contact:     Shay Drory <shayd@...dia.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.
>> diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
>> index d3a2c40c2f12..fcd7dbf20f88 100644
>> --- a/drivers/base/auxiliary.c
>> +++ b/drivers/base/auxiliary.c
>> @@ -158,6 +158,94 @@
>>    *   };
>>    */
>>
>> +#ifdef CONFIG_SYSFS
> 
> People really build boxes without sysfs?  Ok :(
> 
> But if so, why not move this to a whole new file?  That would make it
> simpler to maintain.

sounds good. Will move them to new sysfs.c

> 
>> +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 const struct attribute_group *auxiliary_irqs_groups[] = {
>> +     &auxiliary_irqs_group,
>> +     NULL
>> +};
>> +
>> +/**
>> + * 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;
>> +
>> +     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);
>> +     if (ret)
>> +             goto auxdev_xa_err;
>> +
>> +     ret = sysfs_add_file_to_group(&dev->kobj, &info->sysfs_attr.attr,
>> +                                   auxiliary_irqs_group.name);
> 
> Dynamic attributes are rough, because:

Your response after "because" is missing.
Can you please elaborate?

> 
> 
>> +     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);
>> +     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);
> 
> What is forcing you to remove the irqs after a device is removed from
> the system?

We are removing the irqs _before_ removing the device.
Irqs removal is following the exact mirror of add flow.

> 
> Why not just remove them all automatically?  Why would you ever want to
> remove them after they were added, will they ever actually change over
> the lifespan of a device?

IRQs of the SFs are allocated and removed when the resources are
created.
for example, devlink reload flow that re-initialize the whole device by
releasing and re-allocating new set of IRQs.
Certain driver internal health recovery flow can also trigger similar
re-initialize.

> 
>>   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)
>> +int __auxiliary_device_add(struct auxiliary_device *auxdev, const char *modname,
>> +                        bool irqs_sysfs_enable);
>> +#define auxiliary_device_add(auxdev) __auxiliary_device_add(auxdev, KBUILD_MODNAME, false)
>> +#define auxiliary_device_add_with_irqs(auxdev) \
>> +     __auxiliary_device_add(auxdev, KBUILD_MODNAME, true)
> 
> Ick, no, that way lies madness.
> 
> Just keep the original function:
>          auxiliary_device_add()
> as is.
> 
> Then, if someone DOES call auxiliary_device_sysfs_irq_add() then add the
> irq directory and file as needed then.
> 
> That way no "norml" paths are messed up and over time, we don't keep
> having an explosion of combinations of function calls to create an aux
> device (as we all know, this is NOT going to be the last feature ever
> added to them...)

Thanks for the suggestion, will change in next version.

> 
> thanks,
> 
> greg k-h

Powered by blists - more mailing lists