On Tue, 30 Aug 2022 13:51:18 -0300 Jason Gunthorpe <jgg@xxxxxxxxxx> wrote: > On Tue, Aug 30, 2022 at 10:42:31AM -0600, Alex Williamson wrote: > > On Mon, 29 Aug 2022 12:28:07 -0300 > > Jason Gunthorpe <jgg@xxxxxxxxxx> wrote: > > > > > On Mon, Aug 29, 2022 at 12:41:30AM +0000, Tian, Kevin wrote: > > > > > From: Jason Gunthorpe <jgg@xxxxxxxxxx> > > > > > Sent: Thursday, August 18, 2022 12:07 AM > > > > > > > > > > Make it clear that this is the body of the ioctl - keep the mutex outside > > > > > the function since this function doesn't have and wouldn't benefit from > > > > > error unwind. > > > > > > > > but doing so make unset_container() unpair with set_container() and > > > > be the only one doing additional things in main ioctl body. > > > > > > > > I'd prefer to moving mutex inside unset_container() for better readability. > > > > > > Yes, I tried both ways and ended up here since adding the goto unwind > > > was kind of ungainly for this function. Don't mind either way > > > > > > The functions are not intended as strict pairs, they are ioctl > > > dispatch functions. > > > > The lockdep annotation seems sufficient, but what about simply > > prefixing the unset ioctl function with underscores to reinforce the > > locking requirement, as done by the called function > > __vfio_group_unset_container()? Thanks, > > Could do, but IMHO, that is not a popular convention in VFIO > (anymore?). > > I've been adding lockdep annotations, not adding __ to indicate > the function is called with some kind of lock. > > In my tree __vfio_group_get_from_iommu() is the only one left using > that style. uset_container was turned into > vfio_container_detatch_group() in aother series > > Conversly we have __vfio_register_dev() which I guess __ indicates is > internal or wrappered, not some statement about locking. I think this > has become the more popular usage of the __ generally in the kernel > > So I will do a v2 with the extra goto unwind then > > Are there any other remarks before I do it? None from me. The pile of things relying on this series is growing, so it looks like time to push v2. Thanks, Alex
