| Message ID | 20190619122143.12028-1-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: rip off the refcount from sgx_encl_add_page flow | expand |
On Wed, Jun 19, 2019 at 03:21:43PM +0300, Jarkko Sakkinen wrote: > With the file bound approach we can flush the work queue when the file > is closed. > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > arch/x86/kernel/cpu/sgx/driver/ioctl.c | 3 +-- > arch/x86/kernel/cpu/sgx/driver/main.c | 1 + > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > index d17c60dca114..f56d3c712dc4 100644 > --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c > @@ -172,7 +172,7 @@ static void sgx_add_page_worker(struct work_struct *work) > > next: > kfree(req); > - } while (!kref_put(&encl->refcount, sgx_encl_release) && !is_empty); > + } while (!is_empty); > } > > static u32 sgx_calc_ssaframesize(u32 miscselect, u64 xfrm) > @@ -520,7 +520,6 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, > req->encl_page = encl_page; > req->mrmask = mrmask; > empty = list_empty(&encl->add_page_reqs); > - kref_get(&encl->refcount); > list_add_tail(&req->list, &encl->add_page_reqs); > if (empty) > queue_work(sgx_encl_wq, &encl->work); > diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c > index 0c831ee5e2de..9ea082372682 100644 > --- a/arch/x86/kernel/cpu/sgx/driver/main.c > +++ b/arch/x86/kernel/cpu/sgx/driver/main.c > @@ -47,6 +47,7 @@ static int sgx_release(struct inode *inode, struct file *file) > { > struct sgx_encl *encl = file->private_data; > > + flush_work(&encl->work); We should set SGX_ENCL_DEAD before flush_work(). Other than that I can't think of any issues with this approach. Though hopefully we can purge the worker altogether and make it a moot point :-). > kref_put(&encl->refcount, sgx_encl_release); > > return 0; > -- > 2.20.1 >
On Wed, Jun 19, 2019 at 08:43:41AM -0700, Sean Christopherson wrote: > We should set SGX_ENCL_DEAD before flush_work(). Other than that I can't > think of any issues with this approach. Though hopefully we can purge the > worker altogether and make it a moot point :-). Thanks, I squashed the change now. And definitely we can considere to completely purge that. This change probably won't make it more difficult :-) I also removed PM notifier and SGX_ENCL_SUSPEND. Did not bother to cycle that change here because on that we've already made conclusions. Just have forgot to do that change. /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c index d17c60dca114..f56d3c712dc4 100644 --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c @@ -172,7 +172,7 @@ static void sgx_add_page_worker(struct work_struct *work) next: kfree(req); - } while (!kref_put(&encl->refcount, sgx_encl_release) && !is_empty); + } while (!is_empty); } static u32 sgx_calc_ssaframesize(u32 miscselect, u64 xfrm) @@ -520,7 +520,6 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, req->encl_page = encl_page; req->mrmask = mrmask; empty = list_empty(&encl->add_page_reqs); - kref_get(&encl->refcount); list_add_tail(&req->list, &encl->add_page_reqs); if (empty) queue_work(sgx_encl_wq, &encl->work); diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c index 0c831ee5e2de..9ea082372682 100644 --- a/arch/x86/kernel/cpu/sgx/driver/main.c +++ b/arch/x86/kernel/cpu/sgx/driver/main.c @@ -47,6 +47,7 @@ static int sgx_release(struct inode *inode, struct file *file) { struct sgx_encl *encl = file->private_data; + flush_work(&encl->work); kref_put(&encl->refcount, sgx_encl_release); return 0;
With the file bound approach we can flush the work queue when the file is closed. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- arch/x86/kernel/cpu/sgx/driver/ioctl.c | 3 +-- arch/x86/kernel/cpu/sgx/driver/main.c | 1 + 2 files changed, 2 insertions(+), 2 deletions(-)