From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     "Chatre, Reinette" <reinette.chatre@intel.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "darwi@linutronix.de" <darwi@linutronix.de>,
        "bhelgaas@google.com" <bhelgaas@google.com>,
        "maz@kernel.org" <maz@kernel.org>,
        "bagasdotme@gmail.com" <bagasdotme@gmail.com>
CC:     "Liu, Jing2" <jing2.liu@intel.com>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [RFC PATCH] PCI/MSI: Clarify usage of pci_msix_free_irq()
Thread-Topic: [RFC PATCH] PCI/MSI: Clarify usage of pci_msix_free_irq()
Thread-Index: AQHZQLk2P/B6OLz9Wk6Od5Zj591Mm67PPPHQ
Date:   Wed, 15 Feb 2023 01:50:50 +0000
Message-ID: <BN9PR11MB527628F9C11C51A334A2861C8CA39@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <4c3e7a50d6e70f408812cd7ab199c6b4b326f9de.1676408572.git.reinette.chatre@intel.com>
In-Reply-To: <4c3e7a50d6e70f408812cd7ab199c6b4b326f9de.1676408572.git.reinette.chatre@intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?1WGNKSDqjdOoV/zTgE3um6IeUXwav9oGF3KoBh74m0L4iEjAdzADCMbH5g38?=
 =?us-ascii?Q?cXwwx89pt2B4PJVKLBj9ma/RtWa60ZQ3xCLxAu+stpnqSdtguh8CsKcBGZeC?=
 =?us-ascii?Q?7O3WIETnkFHuYzjcPuAETOANMcbz5aP1SJ3/cmoapSi16OQIqUAT4vRLGObS?=
 =?us-ascii?Q?CiA84Aq+W7P8a9A+v5+pizSalK2nxNjvbOD2Ibuacsq2hYWvbTZ5F8hc0Gtu?=
 =?us-ascii?Q?2a0uvKZhIYzfnDWSWjaUAupg0q3j8ftIbLFl58JXPhW6Arb9D+i4jNsb+Wzf?=
 =?us-ascii?Q?JecyQ2ZzTeMJHgS1BN8wFBJpem+9UuGyaG41lQzZGECXecAOdUtbUkcUJU40?=
 =?us-ascii?Q?GuXv36g6ob8FZLYtt+Mm6cZeDWVbEDs3ykIJ4FIlaFkj7hEYKcdBL3fUO9uT?=
 =?us-ascii?Q?pVIFUm88c8HS3ZF8yHtk/tvSfZ5DRHbgnr4jtTYbePmAv4eWUEWFxkH9x5X2?=
 =?us-ascii?Q?pqHDiJL5hOiieaALlwQmeectgYIAf8uzCjk975FA5rtGVL1f2O/NpiUzh9cE?=
 =?us-ascii?Q?jpk6/UQclrCHyr74XoXbMXPt65mQrUY5Z0FZU1VsUxYh/Cs4wFPDf30bgGYW?=
 =?us-ascii?Q?6nq4kDdrc06NYoWSNitdP4joTwvtqtc+KWHCu//UmHDX28UV9GnO/dlaMCNk?=
 =?us-ascii?Q?ThqV4wsZC5RNM/zCGc8LQgKjb6jfa0Q2RhXW8w70Z6LudsYnnkakWD29AvR/?=
 =?us-ascii?Q?NRF/te1rYKTHubOhYW2Q0eVv6ALanhsNLyJwf1MxL6D281utdRsrbHyITUZb?=
 =?us-ascii?Q?ZuVmY+MkAtedwAXCtQcRbwTrP8Ce5hgDh/nzTRoUrhIrP8uMdyGpY6lqKHMc?=
 =?us-ascii?Q?OuaMDr84XGw/8ikT9YjfEax1Ykt685t7uUAIJxb7a54TZP2MO97bB4lXYR+2?=
 =?us-ascii?Q?EF1kFXQ1uL+YBNqvVmGN258p9xziBLC4efV5SrZSpgk4eXrzVNZw5bWp9t5i?=
 =?us-ascii?Q?6wp92cxBwp8lXEkdkit9Gag67No0gFEKVRb6XlXJoRMMWXBmJJ6XTorEhbhp?=
 =?us-ascii?Q?KeFhG1WY2wlwknbqS/oRyVKpaduvXUqParx5qxGPjyoGy+deVmGNTVxxqo0N?=
 =?us-ascii?Q?JOcKB4sGFlycSQRyhUbsz3Vm8JNGUs8CH4rc276sdIlZCRHts4/Oy1wg7Cs5?=
 =?us-ascii?Q?H1+Yiv97+qYO0uD0+NYEpQ3FUJvOtkXB+4z1hjMHnzuS18RPHixn9SkyHHfj?=
 =?us-ascii?Q?UczZ7zbAT7MYzq61ixaAcTCU9PTTxtrdXC4X/EJA5pQaGn7JmVFqfdHeVW5Z?=
 =?us-ascii?Q?q3rNaa0tuRvnHXQlXzhPXKzvwUpMKlNJxu6uPv5pmXVQccJxEqqcn4Ir4h2C?=
 =?us-ascii?Q?87q+xJClpA8LGUkzYUejYFkStqCxHQHAR3Xoaynts3ExZBPb3/1He9LJnaUR?=
 =?us-ascii?Q?sghGiNSTZU82K/lGOJM/IfJq9pYCkAwHA0FqU/lLC2w4FKElboqT65uhVS8T?=
 =?us-ascii?Q?ihU4HDsp3DYRHLSuipwKlX7bubTQ0SZY1ck6vi/eVb9/xWg3wC8aw5H8VjD+?=
 =?us-ascii?Q?Z+B5+jDXdXnCdILnl4bAZDnoqHCaMcJMaLgDQU/jBVkXK8021rAzZcu7jfwz?=
 =?us-ascii?Q?sa304TfsIUV3AU/mvjzrqSU+XMsnA+DpIlxFj+Ni?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a1516844-c41e-4d90-296f-08db0ef7111b
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Feb 2023 01:50:50.9292
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: NnL3aimc8qcjXmx2Aj89kKi0DV0ea7sPZtZgJW5C6Okw8dXAzADcEGXekc8u1xy+xgCREBR8A4z44iV35ChDvA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5056
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Chatre, Reinette <reinette.chatre@intel.com>
> Sent: Wednesday, February 15, 2023 5:13 AM
>=20
> pci_msix_free_irq() is used to free an interrupt on
> a PCI/MSI-X interrupt domain.
>=20
> The API description specifies that the interrupt to
> be freed was allocated via pci_msix_alloc_irq_at().
> This description limits the usage of pci_msix_free_irq()
> since pci_msix_free_irq() can also be used to free
> MSI-X interrupts allocated with, for example,
> pci_alloc_irq_vectors().
>=20
> Remove the text stating that the interrupt to be freed had
> to be allocated with pci_msix_alloc_irq_at(). The needed
> struct msi_map need not be from pci_msix_alloc_irq_at()
> but can be created from scratch using pci_irq_vector()
> to obtain the Linux IRQ number. Highlight that
> pci_msix_free_irq() cannot be used to disable MSI-X to
> guide users that, for example, pci_free_irq_vectors()
> remains to be needed.
>=20
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> Dear Maintainers,
>=20
> Is my understanding correct?
>=20
> For example, from my understanding the following is
> correct API usage:
>=20
> /*
>  * Enable MSI-X and allocate num interrupts.
>  */
> pci_alloc_irq_vectors()
>=20
> /*
>  * Free one of the interrupts allocated via pci_alloc_irq_vectors().
>  * Possibly called num times, but not required.
>  */
> pci_msix_free_irq()
>=20
> /*
>  * Ensure all remaining interrupts freed and MSI-X disabled.
>  */
> pci_free_irq_vectors()
>=20
> This could be interpreted as an asymmetric usage of the API, yet
> found to be practical when trying to use these new calls.
>=20
> Your feedback is appreciated.

I asked same question before. Thomas gave exactly same thought. [1]

"
The preallocated descriptors are only relevant during setup, but not
post setup. So yes, you can do:

pci_alloc_irq_vectors(dev, 10, 10, PCI_IRQ_MSIX);

and then

    map =3D { .index =3D 9, .virq =3D $IRQ };
    pci_msix_free_irq(dev, &map);

It just works.
"

but looks the comment was not adjusted accordingly.

So,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>=20

[1] https://lore.kernel.org/lkml/87r0xsd8j4.ffs@tglx/

>=20
> Regards,
>=20
> Reinette
>=20
>  drivers/pci/msi/api.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>=20
> diff --git a/drivers/pci/msi/api.c b/drivers/pci/msi/api.c
> index b8009aa11f3c..be679aa5db64 100644
> --- a/drivers/pci/msi/api.c
> +++ b/drivers/pci/msi/api.c
> @@ -163,11 +163,11 @@ EXPORT_SYMBOL_GPL(pci_msix_alloc_irq_at);
>=20
>  /**
>   * pci_msix_free_irq - Free an interrupt on a PCI/MSIX interrupt domain
> - *		      which was allocated via pci_msix_alloc_irq_at()
>   *
>   * @dev:	The PCI device to operate on
>   * @map:	A struct msi_map describing the interrupt to free
> - *		as returned from the allocation function.
> + *
> + * Undo an interrupt vector allocation. Does not disable MSI-X.
>   */
>  void pci_msix_free_irq(struct pci_dev *dev, struct msi_map map)
>  {
> --
> 2.34.1