Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     "Tian, Kevin" <kevin.tian@intel.com>,
        LKML <linux-kernel@vger.kernel.org>
Cc:     "x86@kernel.org" <x86@kernel.org>, Joerg Roedel <joro@8bytes.org>,
        Will Deacon <will@kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Marc Zyngier <maz@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jason Gunthorpe <jgg@mellanox.com>,
        "Jiang, Dave" <dave.jiang@intel.com>,
        Alex Williamson <alex.williamson@redhat.com>,
        "Williams, Dan J" <dan.j.williams@intel.com>,
        Logan Gunthorpe <logang@deltatee.com>,
        "Raj, Ashok" <ashok.raj@intel.com>, Jon Mason <jdmason@kudzu.us>,
        Allen Hubbe <allenbh@gmail.com>
Subject: RE: [patch V2 07/33] genirq/msi: Provide
 msi_create/free_device_irq_domain()
In-Reply-To: <BN9PR11MB527604DE2A881FF615B7D0748C0C9@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20221121083657.157152924@linutronix.de>
 <20221121091326.879869866@linutronix.de>
 <BN9PR11MB527604DE2A881FF615B7D0748C0C9@BN9PR11MB5276.namprd11.prod.outlook.com>
Date:   Wed, 23 Nov 2022 12:38:08 +0100
Message-ID: <8735a9gau7.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Wed, Nov 23 2022 at 08:02, Kevin Tian wrote:
>> From: Thomas Gleixner <tglx@linutronix.de>
>> 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