| Message ID | 20171012140816.6siefvahj6ww7uzf@pd.tnic (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
On 10/12/17 9:08 AM, Borislav Petkov wrote: ... > Well, if you're going to have a global var, why not pull up the misc > device instead? > > And mind you, I've moved out this assignments: > > + psp->sev_misc = psp_misc_dev; > + init_waitqueue_head(&psp->sev_int_queue); > + dev_info(dev, "registered SEV device\n"); > > outside of the if-conditional as I'm assuming you want to do this for > each psp device for which sev_ops_init() is called. > > Or am I wrong here? I am OK with your patch except one minor issue highlighted below. > --- > diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c > index 175cb3c3b8ef..d50aaa1ca75b 100644 > --- a/drivers/crypto/ccp/psp-dev.c > +++ b/drivers/crypto/ccp/psp-dev.c > @@ -31,7 +31,7 @@ > #define DEVICE_NAME "sev" > > static DEFINE_MUTEX(sev_cmd_mutex); > -static bool sev_fops_registered; > +static struct miscdevice *psp_misc_dev; > > static struct psp_device *psp_alloc_struct(struct sp_device *sp) > { > @@ -242,7 +242,6 @@ EXPORT_SYMBOL_GPL(sev_guest_df_flush); > static int sev_ops_init(struct psp_device *psp) > { > struct device *dev = psp->dev; > - struct miscdevice *misc; > int ret; > > /* > @@ -252,26 +251,24 @@ static int sev_ops_init(struct psp_device *psp) > * sev_do_cmd() finds the right master device to which to issue the > * command to the firmware. > */ > - if (!sev_fops_registered) { > - > - misc = devm_kzalloc(dev, sizeof(*misc), GFP_KERNEL); > - if (!misc) > + if (!psp_misc_dev) { > + psp_misc_dev = devm_kzalloc(dev, sizeof(struct miscdevice), GFP_KERNEL); > + if (!psp_misc_dev) > return -ENOMEM; > > - misc->minor = MISC_DYNAMIC_MINOR; > - misc->name = DEVICE_NAME; > - misc->fops = &sev_fops; > + psp_misc_dev->minor = MISC_DYNAMIC_MINOR; > + psp_misc_dev->name = DEVICE_NAME; > + psp_misc_dev->fops = &sev_fops; > > - ret = misc_register(misc); > + ret = misc_register(psp_misc_dev); > if (ret) > return ret; > - > - sev_fops_registered = true; > - psp->sev_misc = misc; > - init_waitqueue_head(&psp->sev_int_queue); > - dev_info(dev, "registered SEV device\n"); > } > > + psp->sev_misc = psp_misc_dev; > + init_waitqueue_head(&psp->sev_int_queue); > + dev_info(dev, "registered SEV device\n"); > + > return 0; > } > > @@ -288,8 +285,8 @@ static int sev_init(struct psp_device *psp) > > static void sev_exit(struct psp_device *psp) > { > - if (psp->sev_misc) > - misc_deregister(psp->sev_misc); > + if (psp_misc_dev) > + misc_deregister(psp_misc_dev); The sev_exit() will be called for all the psp_device instance. we need to set psp_misc_dev = NULL after deregistering the device. if (psp_misc_dev) { misc_deregister(psp_misc_dev); psp_misc_dev = NULL; } > } > > int psp_dev_init(struct sp_device *sp) >
On Thu, Oct 12, 2017 at 04:11:18PM -0500, Brijesh Singh wrote: > The sev_exit() will be called for all the psp_device instance. we need > to set psp_misc_dev = NULL after deregistering the device. > > if (psp_misc_dev) { > misc_deregister(psp_misc_dev); > psp_misc_dev = NULL; Right, except we assign that misc device to every psp device: psp->sev_misc = psp_misc_dev; so the question now is, how do you want this to work: the misc device should be destroyed after the last psp device is destroyed or how? Because after the first: psp_misc_dev = NULL; the respective psp->sev_misc will still keep references to that device but the device itself will be already deregistered. And that sounds strange.
On 10/12/17 4:41 PM, Borislav Petkov wrote: > On Thu, Oct 12, 2017 at 04:11:18PM -0500, Brijesh Singh wrote: >> The sev_exit() will be called for all the psp_device instance. we need >> to set psp_misc_dev = NULL after deregistering the device. >> >> if (psp_misc_dev) { >> misc_deregister(psp_misc_dev); >> psp_misc_dev = NULL; > Right, except we assign that misc device to every psp device: > > psp->sev_misc = psp_misc_dev; > > so the question now is, how do you want this to work: the misc device > should be destroyed after the last psp device is destroyed or how? We don't know when the last psp device is getting destroyed. Since we are making the sev_misc_dev as a global variable then there is no reason for 'sev_misc' field in the psp_device. > Because after the first: > > psp_misc_dev = NULL; > > the respective psp->sev_misc will still keep references to that device > but the device itself will be already deregistered. And that sounds > strange. See my above comment, I think the simplest solution is remove psp->sev_misc
On Thu, Oct 12, 2017 at 04:52:32PM -0500, Brijesh Singh wrote:
> See my above comment, I think the simplest solution is remove psp->sev_misc
Ok, so far so good.
But now you still need to track which is the last psp device and to call
misc_deregister() only when the last device exits. Because if you do
that for the first psp device as it is now, all the following devices
will see a deregistered misc device. And I don't think we want that.
You probably could do something with reference counting: Documentation/kref.txt
to track that and have the last device deregister the misc device.
Or have the "enclosing" sp-dev deregister the misc device when it is
exiting and when it is sure that there are no more psp devices...
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c index 175cb3c3b8ef..d50aaa1ca75b 100644 --- a/drivers/crypto/ccp/psp-dev.c +++ b/drivers/crypto/ccp/psp-dev.c @@ -31,7 +31,7 @@ #define DEVICE_NAME "sev" static DEFINE_MUTEX(sev_cmd_mutex); -static bool sev_fops_registered; +static struct miscdevice *psp_misc_dev; static struct psp_device *psp_alloc_struct(struct sp_device *sp) { @@ -242,7 +242,6 @@ EXPORT_SYMBOL_GPL(sev_guest_df_flush); static int sev_ops_init(struct psp_device *psp) { struct device *dev = psp->dev; - struct miscdevice *misc; int ret; /* @@ -252,26 +251,24 @@ static int sev_ops_init(struct psp_device *psp) * sev_do_cmd() finds the right master device to which to issue the * command to the firmware. */ - if (!sev_fops_registered) { - - misc = devm_kzalloc(dev, sizeof(*misc), GFP_KERNEL); - if (!misc) + if (!psp_misc_dev) { + psp_misc_dev = devm_kzalloc(dev, sizeof(struct miscdevice), GFP_KERNEL); + if (!psp_misc_dev) return -ENOMEM; - misc->minor = MISC_DYNAMIC_MINOR; - misc->name = DEVICE_NAME; - misc->fops = &sev_fops; + psp_misc_dev->minor = MISC_DYNAMIC_MINOR; + psp_misc_dev->name = DEVICE_NAME; + psp_misc_dev->fops = &sev_fops; - ret = misc_register(misc); + ret = misc_register(psp_misc_dev); if (ret) return ret; - - sev_fops_registered = true; - psp->sev_misc = misc; - init_waitqueue_head(&psp->sev_int_queue); - dev_info(dev, "registered SEV device\n"); } + psp->sev_misc = psp_misc_dev; + init_waitqueue_head(&psp->sev_int_queue); + dev_info(dev, "registered SEV device\n"); + return 0; } @@ -288,8 +285,8 @@ static int sev_init(struct psp_device *psp) static void sev_exit(struct psp_device *psp) { - if (psp->sev_misc) - misc_deregister(psp->sev_misc); + if (psp_misc_dev) + misc_deregister(psp_misc_dev); } int psp_dev_init(struct sp_device *sp)