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? Thanks, Jason
