| Message ID | 20241029183907.3536683-3-dionnaglaze@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support SEV firmware hotloading | expand |
On Tue, Oct 29, 2024 at 06:39:02PM +0000, Dionna Glaze wrote: > If a kernel module registers a firmware upload API ops set, then it's > unable to be moved due to effectively a cyclic reference that the module > depends on the upload which depends on the module. > > Instead, only require the try_module_get when an upload is requested to > disallow unloading a module only while the upload is in progress. > > Fixes: 97730bbb242cd ("firmware_loader: Add firmware-upload support") > > CC: Luis Chamberlain <mcgrof@kernel.org> > CC: Russ Weight <russ.weight@linux.dev> > CC: Danilo Krummrich <dakr@redhat.com> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > CC: "Rafael J. Wysocki" <rafael@kernel.org> > CC: Tianfei zhang <tianfei.zhang@intel.com> > > Signed-off-by: Dionna Glaze <dionnaglaze@google.com> > Change-Id: Ifac9513de2f58a5120d4c0a681e969cd71cd3c68 Please always run checkpatch on a change before sending it out :(
On Tue, Oct 29, 2024 at 06:39:02PM +0000, Dionna Glaze wrote: > If a kernel module registers a firmware upload API ops set, then it's > unable to be moved due to effectively a cyclic reference that the module > depends on the upload which depends on the module. > > Instead, only require the try_module_get when an upload is requested to > disallow unloading a module only while the upload is in progress. > > Fixes: 97730bbb242cd ("firmware_loader: Add firmware-upload support") > > CC: Luis Chamberlain <mcgrof@kernel.org> > CC: Russ Weight <russ.weight@linux.dev> > CC: Danilo Krummrich <dakr@redhat.com> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > CC: "Rafael J. Wysocki" <rafael@kernel.org> > CC: Tianfei zhang <tianfei.zhang@intel.com> > > Signed-off-by: Dionna Glaze <dionnaglaze@google.com> > Change-Id: Ifac9513de2f58a5120d4c0a681e969cd71cd3c68 > --- > drivers/base/firmware_loader/sysfs_upload.c | 28 ++++++++++++++------- > 1 file changed, 19 insertions(+), 9 deletions(-) > > diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c > index 829270067d163..97b0ae855b5f0 100644 > --- a/drivers/base/firmware_loader/sysfs_upload.c > +++ b/drivers/base/firmware_loader/sysfs_upload.c > @@ -103,6 +103,10 @@ static ssize_t cancel_store(struct device *dev, struct device_attribute *attr, > if (fwlp->progress == FW_UPLOAD_PROG_IDLE) > ret = -ENODEV; > > + /* > + * Not idle, so fw_upload_start already called try_module_get. > + * No need to get/put around cancel. > + */ I'm not sure why you think this comment is needed. I think it can be removed > fwlp->ops->cancel(fwlp->fw_upload); > mutex_unlock(&fwlp->lock); > > @@ -164,11 +168,13 @@ static void fw_upload_main(struct work_struct *work) > enum fw_upload_err ret; > struct device *fw_dev; > struct fw_upload *fwl; > + struct module *module; No need for a local variable here - module is only accessed once, so just reference it as fwlp->module. > > fwlp = container_of(work, struct fw_upload_priv, work); > fwl = fwlp->fw_upload; > fw_sysfs = (struct fw_sysfs *)fwl->priv; > fw_dev = &fw_sysfs->dev; > + module = fwlp->module; > > fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PREPARING); > ret = fwlp->ops->prepare(fwl, fwlp->data, fwlp->remaining_size); > @@ -204,6 +210,7 @@ static void fw_upload_main(struct work_struct *work) > fwlp->ops->cleanup(fwl); > > putdev_exit: > + module_put(module); > put_device(fw_dev->parent); > > /* > @@ -238,7 +245,11 @@ int fw_upload_start(struct fw_sysfs *fw_sysfs) > return 0; > } > > + Remove the extra blank line above. > fwlp = fw_sysfs->fw_upload_priv; > + if (!try_module_get(fwlp->module)) /* released in fw_upload_main */ > + return -EFAULT; > + > mutex_lock(&fwlp->lock); > > /* Do not interfere with an on-going fw_upload */ > @@ -310,13 +321,10 @@ firmware_upload_register(struct module *module, struct device *parent, > return ERR_PTR(-EINVAL); > } > > - if (!try_module_get(module)) > - return ERR_PTR(-EFAULT); > - > fw_upload = kzalloc(sizeof(*fw_upload), GFP_KERNEL); > if (!fw_upload) { > ret = -ENOMEM; > - goto exit_module_put; > + goto exit_err; > } > > fw_upload_priv = kzalloc(sizeof(*fw_upload_priv), GFP_KERNEL); > @@ -358,7 +366,7 @@ firmware_upload_register(struct module *module, struct device *parent, > if (ret) { > dev_err(fw_dev, "%s: device_register failed\n", __func__); > put_device(fw_dev); > - goto exit_module_put; > + goto exit_err; > } > > return fw_upload; > @@ -372,8 +380,7 @@ firmware_upload_register(struct module *module, struct device *parent, > free_fw_upload: > kfree(fw_upload); > > -exit_module_put: > - module_put(module); > +exit_err: > > return ERR_PTR(ret); > } > @@ -387,7 +394,6 @@ void firmware_upload_unregister(struct fw_upload *fw_upload) > { > struct fw_sysfs *fw_sysfs = fw_upload->priv; > struct fw_upload_priv *fw_upload_priv = fw_sysfs->fw_upload_priv; > - struct module *module = fw_upload_priv->module; > > mutex_lock(&fw_upload_priv->lock); > if (fw_upload_priv->progress == FW_UPLOAD_PROG_IDLE) { > @@ -395,6 +401,11 @@ void firmware_upload_unregister(struct fw_upload *fw_upload) > goto unregister; > } > > + /* > + * No need to try_module_get/module_put around the op since only the > + * module itself will call unregister, usually when the refcount has > + * dropped to zero and it's cleaning up dependencies to destroy itself. > + */ This comment is not needed. Thanks, - Russ > fw_upload_priv->ops->cancel(fw_upload); > mutex_unlock(&fw_upload_priv->lock); > > @@ -403,6 +414,5 @@ void firmware_upload_unregister(struct fw_upload *fw_upload) > > unregister: > device_unregister(&fw_sysfs->dev); > - module_put(module); > } > EXPORT_SYMBOL_GPL(firmware_upload_unregister); > -- > 2.47.0.163.g1226f6d8fa-goog >
diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c index 829270067d163..97b0ae855b5f0 100644 --- a/drivers/base/firmware_loader/sysfs_upload.c +++ b/drivers/base/firmware_loader/sysfs_upload.c @@ -103,6 +103,10 @@ static ssize_t cancel_store(struct device *dev, struct device_attribute *attr, if (fwlp->progress == FW_UPLOAD_PROG_IDLE) ret = -ENODEV; + /* + * Not idle, so fw_upload_start already called try_module_get. + * No need to get/put around cancel. + */ fwlp->ops->cancel(fwlp->fw_upload); mutex_unlock(&fwlp->lock); @@ -164,11 +168,13 @@ static void fw_upload_main(struct work_struct *work) enum fw_upload_err ret; struct device *fw_dev; struct fw_upload *fwl; + struct module *module; fwlp = container_of(work, struct fw_upload_priv, work); fwl = fwlp->fw_upload; fw_sysfs = (struct fw_sysfs *)fwl->priv; fw_dev = &fw_sysfs->dev; + module = fwlp->module; fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PREPARING); ret = fwlp->ops->prepare(fwl, fwlp->data, fwlp->remaining_size); @@ -204,6 +210,7 @@ static void fw_upload_main(struct work_struct *work) fwlp->ops->cleanup(fwl); putdev_exit: + module_put(module); put_device(fw_dev->parent); /* @@ -238,7 +245,11 @@ int fw_upload_start(struct fw_sysfs *fw_sysfs) return 0; } + fwlp = fw_sysfs->fw_upload_priv; + if (!try_module_get(fwlp->module)) /* released in fw_upload_main */ + return -EFAULT; + mutex_lock(&fwlp->lock); /* Do not interfere with an on-going fw_upload */ @@ -310,13 +321,10 @@ firmware_upload_register(struct module *module, struct device *parent, return ERR_PTR(-EINVAL); } - if (!try_module_get(module)) - return ERR_PTR(-EFAULT); - fw_upload = kzalloc(sizeof(*fw_upload), GFP_KERNEL); if (!fw_upload) { ret = -ENOMEM; - goto exit_module_put; + goto exit_err; } fw_upload_priv = kzalloc(sizeof(*fw_upload_priv), GFP_KERNEL); @@ -358,7 +366,7 @@ firmware_upload_register(struct module *module, struct device *parent, if (ret) { dev_err(fw_dev, "%s: device_register failed\n", __func__); put_device(fw_dev); - goto exit_module_put; + goto exit_err; } return fw_upload; @@ -372,8 +380,7 @@ firmware_upload_register(struct module *module, struct device *parent, free_fw_upload: kfree(fw_upload); -exit_module_put: - module_put(module); +exit_err: return ERR_PTR(ret); } @@ -387,7 +394,6 @@ void firmware_upload_unregister(struct fw_upload *fw_upload) { struct fw_sysfs *fw_sysfs = fw_upload->priv; struct fw_upload_priv *fw_upload_priv = fw_sysfs->fw_upload_priv; - struct module *module = fw_upload_priv->module; mutex_lock(&fw_upload_priv->lock); if (fw_upload_priv->progress == FW_UPLOAD_PROG_IDLE) { @@ -395,6 +401,11 @@ void firmware_upload_unregister(struct fw_upload *fw_upload) goto unregister; } + /* + * No need to try_module_get/module_put around the op since only the + * module itself will call unregister, usually when the refcount has + * dropped to zero and it's cleaning up dependencies to destroy itself. + */ fw_upload_priv->ops->cancel(fw_upload); mutex_unlock(&fw_upload_priv->lock); @@ -403,6 +414,5 @@ void firmware_upload_unregister(struct fw_upload *fw_upload) unregister: device_unregister(&fw_sysfs->dev); - module_put(module); } EXPORT_SYMBOL_GPL(firmware_upload_unregister);
If a kernel module registers a firmware upload API ops set, then it's unable to be moved due to effectively a cyclic reference that the module depends on the upload which depends on the module. Instead, only require the try_module_get when an upload is requested to disallow unloading a module only while the upload is in progress. Fixes: 97730bbb242cd ("firmware_loader: Add firmware-upload support") CC: Luis Chamberlain <mcgrof@kernel.org> CC: Russ Weight <russ.weight@linux.dev> CC: Danilo Krummrich <dakr@redhat.com> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> CC: "Rafael J. Wysocki" <rafael@kernel.org> CC: Tianfei zhang <tianfei.zhang@intel.com> Signed-off-by: Dionna Glaze <dionnaglaze@google.com> Change-Id: Ifac9513de2f58a5120d4c0a681e969cd71cd3c68 --- drivers/base/firmware_loader/sysfs_upload.c | 28 ++++++++++++++------- 1 file changed, 19 insertions(+), 9 deletions(-)