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.c...@amd.com> > >>> --- > >>> cc: "Roger Pau Monné" <roger....@citrix.com> > >>> --- > >>> 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.
