| Message ID | 20230606221518.7054-1-ddiss@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: f_mass_storage: remove unnecessary open check | expand |
On Wed, Jun 07, 2023 at 12:15:18AM +0200, David Disseldorp wrote: > fsg_lun_is_open() will always and only be true after a successful > fsg_lun_open() call, so drop the unnecessary conditional. I don't follow the reasoning. The relevant code is: if (cfg->filename) { rc = fsg_lun_open(lun, cfg->filename); if (rc) goto error_lun; } ... if (fsg_lun_is_open(lun)) { So at this point, the fsg_lun_is_open() condition is true iff cfg->filename was set upon entry. What makes this test unnecessary? > Similarly, > the error_lun label will never be reached with an open lun (non-null > filp) so remove the unnecessary fsg_lun_close() call. That makes more sense. Alan Stern > Signed-off-by: David Disseldorp <ddiss@suse.de> > --- > drivers/usb/gadget/function/f_mass_storage.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c > index 3a30feb47073f..da07e45ae6df5 100644 > --- a/drivers/usb/gadget/function/f_mass_storage.c > +++ b/drivers/usb/gadget/function/f_mass_storage.c > @@ -2857,7 +2857,7 @@ int fsg_common_create_lun(struct fsg_common *common, struct fsg_lun_config *cfg, > const char **name_pfx) > { > struct fsg_lun *lun; > - char *pathbuf, *p; > + char *pathbuf = NULL, *p = "(no medium)"; > int rc = -ENOMEM; > > if (id >= ARRAY_SIZE(common->luns)) > @@ -2907,12 +2907,9 @@ int fsg_common_create_lun(struct fsg_common *common, struct fsg_lun_config *cfg, > rc = fsg_lun_open(lun, cfg->filename); > if (rc) > goto error_lun; > - } > > - pathbuf = kmalloc(PATH_MAX, GFP_KERNEL); > - p = "(no medium)"; > - if (fsg_lun_is_open(lun)) { > p = "(error)"; > + pathbuf = kmalloc(PATH_MAX, GFP_KERNEL); > if (pathbuf) { > p = file_path(lun->filp, pathbuf, PATH_MAX); > if (IS_ERR(p)) > @@ -2931,7 +2928,6 @@ int fsg_common_create_lun(struct fsg_common *common, struct fsg_lun_config *cfg, > error_lun: > if (device_is_registered(&lun->dev)) > device_unregister(&lun->dev); > - fsg_lun_close(lun); > common->luns[id] = NULL; > error_sysfs: > kfree(lun); > -- > 2.35.3 >
Thanks for the feedback, Alan... On Tue, 6 Jun 2023 18:33:22 -0400, Alan Stern wrote: > On Wed, Jun 07, 2023 at 12:15:18AM +0200, David Disseldorp wrote: > > fsg_lun_is_open() will always and only be true after a successful > > fsg_lun_open() call, so drop the unnecessary conditional. > > I don't follow the reasoning. The relevant code is: > > if (cfg->filename) { > rc = fsg_lun_open(lun, cfg->filename); > if (rc) > goto error_lun; > } > ... > if (fsg_lun_is_open(lun)) { > > So at this point, the fsg_lun_is_open() condition is true iff > cfg->filename was set upon entry. What makes this test unnecessary? The fsg_lun_is_open() test is unnecessary as it will always be true following a successful fsg_lun_open() call and will always be false if cfg->filename is unset. This means that the logic from the fsg_lun_is_open() conditional block can be appended directly after the error_lun goto. pathbuf allocation ('...' above) is only needed for the open case, so can also be wrapped into the conditional block. I'd be happy to update the commit message if the explanation above makes things clearer. I should probably also mention that I've tested this using dummy-hcd. Cheers, David
On Wed, Jun 07, 2023 at 10:15:03AM +0200, David Disseldorp wrote: > Thanks for the feedback, Alan... > > On Tue, 6 Jun 2023 18:33:22 -0400, Alan Stern wrote: > > > On Wed, Jun 07, 2023 at 12:15:18AM +0200, David Disseldorp wrote: > > > fsg_lun_is_open() will always and only be true after a successful > > > fsg_lun_open() call, so drop the unnecessary conditional. > > > > I don't follow the reasoning. The relevant code is: > > > > if (cfg->filename) { > > rc = fsg_lun_open(lun, cfg->filename); > > if (rc) > > goto error_lun; > > } > > ... > > if (fsg_lun_is_open(lun)) { > > > > So at this point, the fsg_lun_is_open() condition is true iff > > cfg->filename was set upon entry. What makes this test unnecessary? > > The fsg_lun_is_open() test is unnecessary as it will always be true > following a successful fsg_lun_open() call and will always be false if > cfg->filename is unset. This means that the logic from the > fsg_lun_is_open() conditional block can be appended directly after the > error_lun goto. pathbuf allocation ('...' above) is only needed for > the open case, so can also be wrapped into the conditional block. > > I'd be happy to update the commit message if the explanation above > makes things clearer. I should probably also mention that I've tested > this using dummy-hcd. Yes, please do that. All you have to say is that the fsg_lun_is_open() test can be eliminated and the code merged with the preceding conditional, because the LUN won't be open if cfg->filename wasn't set. Alan Stern
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c index 3a30feb47073f..da07e45ae6df5 100644 --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -2857,7 +2857,7 @@ int fsg_common_create_lun(struct fsg_common *common, struct fsg_lun_config *cfg, const char **name_pfx) { struct fsg_lun *lun; - char *pathbuf, *p; + char *pathbuf = NULL, *p = "(no medium)"; int rc = -ENOMEM; if (id >= ARRAY_SIZE(common->luns)) @@ -2907,12 +2907,9 @@ int fsg_common_create_lun(struct fsg_common *common, struct fsg_lun_config *cfg, rc = fsg_lun_open(lun, cfg->filename); if (rc) goto error_lun; - } - pathbuf = kmalloc(PATH_MAX, GFP_KERNEL); - p = "(no medium)"; - if (fsg_lun_is_open(lun)) { p = "(error)"; + pathbuf = kmalloc(PATH_MAX, GFP_KERNEL); if (pathbuf) { p = file_path(lun->filp, pathbuf, PATH_MAX); if (IS_ERR(p)) @@ -2931,7 +2928,6 @@ int fsg_common_create_lun(struct fsg_common *common, struct fsg_lun_config *cfg, error_lun: if (device_is_registered(&lun->dev)) device_unregister(&lun->dev); - fsg_lun_close(lun); common->luns[id] = NULL; error_sysfs: kfree(lun);
fsg_lun_is_open() will always and only be true after a successful fsg_lun_open() call, so drop the unnecessary conditional. Similarly, the error_lun label will never be reached with an open lun (non-null filp) so remove the unnecessary fsg_lun_close() call. Signed-off-by: David Disseldorp <ddiss@suse.de> --- drivers/usb/gadget/function/f_mass_storage.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)