RE: [patch V2 07/33] genirq/msi: Provide msi_create/free_device_irq_domain()

[Thomas Gleixner]

On Wed, Nov 23 2022 at 08:02, Kevin Tian wrote:
>> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Sent: Monday, November 21, 2022 10:38 PM
>> 
>> 
>> +static inline void msi_remove_device_irqdomains(struct device *dev, struct
>> msi_device_data *md)
>> +{
>
> 'md' is unused

Duh, yes.

>> + *
>> + * Return: True on success, false otherwise
>
> Can you elaborate why boolean type is selected instead of returning the
> actual error codes? the outmost callers are all new functions:
>
> 	pci_setup_msi[x]_device_domain()
> 	pci_create_ims_domain()
>
> I didn't find out any legacy convention forcing this way...

What's the value of error codes? 99% of all callsites do:

       ret = foo();
       if (ret)
          goto fail;

Nothing evaluates the error codes, unless there is real useful
information like EAGAIN or ENOSPC which can tell the caller to retry
eventually or with different parameters. But for the above, the error
code is just useless.

>> +	bundle = kmemdup(template, sizeof(*bundle), GFP_KERNEL);
>> +	if (!bundle)
>> +		return false;
>> +
>> +	bundle->info.hwsize = hwsize ? hwsize : MSI_MAX_INDEX;
>
> patch04 marks that hwsize being 0 means unknown or unlimited in the
> header file.
>
> but here info.hwsize always gets a value i.e. the meaning of 0 only exists
> in this function. What about removing the trailing words about 0 in
> patch04?
>
> - + * @hwsize:		The hardware table size (0 if unknown/unlimited)
> + + * @hwsize:		The hardware table size

Fair enough, though I rather make that:

 * @hwsize:		The hardware table size or the software defined
                        index limit