On Wed, 29 Nov 2023, David E. Box wrote:
> Commit 936874b77dd0 ("platform/x86/intel/vsec: Add PCI error recovery
> support to Intel PMT") added an xarray to track the list of vsec devices to
> be recovered after a PCI error. But it did not provide cleanup for the list
> leading to a memory leak that was caught by kmemleak. Do xa_alloc() before
> devm_add_action_or_reset() so that the list may be cleaned up with
> xa_erase() in the release function.
>
> Fixes: 936874b77dd0 ("platform/x86/intel/vsec: Add PCI error recovery support to Intel PMT")
> Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> ---
>
> V6 - Move xa_alloc() before ida_alloc() to reduce mutex use during error
> recovery.
> - Fix return value after id_alloc() fail
> - Add Fixes tag
> - Add more detail to changelog
>
> V5 - New patch
>
> drivers/platform/x86/intel/vsec.c | 24 ++++++++++++++----------
> drivers/platform/x86/intel/vsec.h | 1 +
> 2 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c
> index c1f9e4471b28..2d568466b4e2 100644
> --- a/drivers/platform/x86/intel/vsec.c
> +++ b/drivers/platform/x86/intel/vsec.c
> @@ -120,6 +120,8 @@ static void intel_vsec_dev_release(struct device *dev)
> {
> struct intel_vsec_device *intel_vsec_dev = dev_to_ivdev(dev);
>
> + xa_erase(&auxdev_array, intel_vsec_dev->id);
> +
> mutex_lock(&vsec_ida_lock);
> ida_free(intel_vsec_dev->ida, intel_vsec_dev->auxdev.id);
> mutex_unlock(&vsec_ida_lock);
> @@ -135,19 +137,27 @@ int intel_vsec_add_aux(struct pci_dev *pdev, struct device *parent,
> struct auxiliary_device *auxdev = &intel_vsec_dev->auxdev;
> int ret, id;
>
> - mutex_lock(&vsec_ida_lock);
> - ret = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL);
> - mutex_unlock(&vsec_ida_lock);
> + ret = xa_alloc(&auxdev_array, &intel_vsec_dev->id, intel_vsec_dev,
> + PMT_XA_LIMIT, GFP_KERNEL);
> if (ret < 0) {
> kfree(intel_vsec_dev->resource);
> kfree(intel_vsec_dev);
> return ret;
> }
>
> + mutex_lock(&vsec_ida_lock);
> + id = ida_alloc(intel_vsec_dev->ida, GFP_KERNEL);
> + mutex_unlock(&vsec_ida_lock);
> + if (id < 0) {
> + kfree(intel_vsec_dev->resource);
> + kfree(intel_vsec_dev);
> + return id;
Thanks, this looks much better this way around but it is missing
xa_alloc() rollback now that the order is reversed.
Once that is fixed,
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
--
i.
