Re: [PATCH v10 4/4] vpci/msix: Free MSIX resources when init_msix() fails

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Aug 05, 2025 at 10:40:00AM +0200, Jan Beulich wrote:
> On 05.08.2025 10:27, Chen, Jiqian wrote:
> > On 2025/8/5 16:10, Jan Beulich wrote:
> >> On 05.08.2025 05:49, Jiqian Chen wrote:
> >>> When MSI-X initialization fails vPCI will hide the capability, but
> >>> remove of handlers and data won't be performed until the device is
> >>> deassigned.  Introduce a MSI-X cleanup hook that will be called when
> >>> initialization fails to cleanup MSI-X related hooks and free it's
> >>> associated data.
> >>>
> >>> As all supported capabilities have been switched to use the cleanup
> >>> hooks call those from vpci_deassign_device() instead of open-code the
> >>> capability specific cleanup in there.
> >>>
> >>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> >>> ---
> >>> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> >>> ---
> >>> v9->v10 changes:
> >>> * Call all cleanup hook in vpci_deassign_device() instead of 
> >>> cleanup_msix().
> >>
> >> Isn't this rather an omission in an earlier change, and hence may want to
> >> come separately and with a Fixes: tag?
> > This is not really an omission, after all, all the cleanup hooks were 
> > implemented at the end of this series.
> > And judging from the commit message(which was written by Roger in v8), 
> > Roger also agreed to add them in this patch.
> 
> I disagree. Of the two xfree()-s you remove here from vpci_deassign_device(),
> one should have been removed by patch 3 already. Which would require the
> part of the patch here to be put in place earlier on.

I don't think a Fixes tag is strictly required - the previous
functionality will not lead to malfunction, albeit it's best to use
the cleanup hooks introduced here.  Up until the hooks are executed as
part of vpci_deassign_device() the xfree() calls need to remain in
place.

Conceptually it would have been better to add the calls to the
->cleanup() hooks in vpci_deassign_device() in the same patch that
added the vpci_init_capabilities() ->init() and ->cleanup() logic.

It was my bad for noting this only when reviewing patch 8, and then
not asking for it to be placed in the right patch.

Thanks, Roger.