Re: [PATCH net-next v9 1/2] driver core: auxiliary bus: show auxiliary device IRQs

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

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 :)