| Message ID | 1528968239-2447-4-git-send-email-climbbb.kim@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 14 Jun 2018, Jaejoong Kim wrote: > Removing EXPORT_SYMBOL_GPL from kref_{put, get} since it is used > only in f_mass_storage > > Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com> This is only a partial solution. In fact, fsg_common_get() isn't used anywhere, and fsg_common_put() is used in only one place. It would be better to remove those two routines, get rid of common->ref entirely, and make fsg_free_inst() call fsg_common_release() directly. Alan Stern > --- > drivers/usb/gadget/function/f_mass_storage.c | 6 ++---- > drivers/usb/gadget/function/f_mass_storage.h | 4 ---- > 2 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c > index b6e2930..ba0eb7e 100644 > --- a/drivers/usb/gadget/function/f_mass_storage.c > +++ b/drivers/usb/gadget/function/f_mass_storage.c > @@ -2558,17 +2558,15 @@ static void fsg_lun_release(struct device *dev) > /* Nothing needs to be done */ > } > > -void fsg_common_get(struct fsg_common *common) > +static void __maybe_unused fsg_common_get(struct fsg_common *common) > { > kref_get(&common->ref); > } > -EXPORT_SYMBOL_GPL(fsg_common_get); > > -void fsg_common_put(struct fsg_common *common) > +static void fsg_common_put(struct fsg_common *common) > { > kref_put(&common->ref, fsg_common_release); > } > -EXPORT_SYMBOL_GPL(fsg_common_put); > > static struct fsg_common *fsg_common_setup(struct fsg_common *common) > { > diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h > index 58857fc..3b8c4ce 100644 > --- a/drivers/usb/gadget/function/f_mass_storage.h > +++ b/drivers/usb/gadget/function/f_mass_storage.h > @@ -115,10 +115,6 @@ fsg_opts_from_func_inst(const struct usb_function_instance *fi) > return container_of(fi, struct fsg_opts, func_inst); > } > > -void fsg_common_get(struct fsg_common *common); > - > -void fsg_common_put(struct fsg_common *common); > - > void fsg_common_set_sysfs(struct fsg_common *common, bool sysfs); > > int fsg_common_set_num_buffers(struct fsg_common *common, unsigned int n); > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello! On 06/14/2018 12:23 PM, Jaejoong Kim wrote: > Removing EXPORT_SYMBOL_GPL from kref_{put, get} since it is used I thought you removed the exports from the real kref_{get|put}(). :-) Your patch subject and description are simply misleading the way they are written. > only in f_mass_storage > > Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com> > --- > drivers/usb/gadget/function/f_mass_storage.c | 6 ++---- > drivers/usb/gadget/function/f_mass_storage.h | 4 ---- > 2 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c > index b6e2930..ba0eb7e 100644 > --- a/drivers/usb/gadget/function/f_mass_storage.c > +++ b/drivers/usb/gadget/function/f_mass_storage.c > @@ -2558,17 +2558,15 @@ static void fsg_lun_release(struct device *dev) > /* Nothing needs to be done */ > } > > -void fsg_common_get(struct fsg_common *common) > +static void __maybe_unused fsg_common_get(struct fsg_common *common) > { > kref_get(&common->ref); > } > -EXPORT_SYMBOL_GPL(fsg_common_get); > > -void fsg_common_put(struct fsg_common *common) > +static void fsg_common_put(struct fsg_common *common) > { > kref_put(&common->ref, fsg_common_release); > } > -EXPORT_SYMBOL_GPL(fsg_common_put); > > static struct fsg_common *fsg_common_setup(struct fsg_common *common) > { [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the review it. 2018년 6월 14일 (목) 오후 11:48, Alan Stern <stern@rowland.harvard.edu>님이 작성: > > On Thu, 14 Jun 2018, Jaejoong Kim wrote: > > > Removing EXPORT_SYMBOL_GPL from kref_{put, get} since it is used > > only in f_mass_storage > > > > Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com> > > This is only a partial solution. In fact, fsg_common_get() isn't used > anywhere, and fsg_common_put() is used in only one place. Right. Actually my first approach was to remove fsg_common_get/put and just use kref APIs(kref_get/put). But I kept current codes because this module author might have another reason. > > It would be better to remove those two routines, get rid of common->ref > entirely, and make fsg_free_inst() call fsg_common_release() directly. Hm.. If you want to call fsg_common_release () in fsg_free_inst (), the struct kref in this moduel is meaningless. I prefer hold kref and just use kref APIs not fsg_common_get/put. Thanks, Jaejoong > > Alan Stern > > > --- > > drivers/usb/gadget/function/f_mass_storage.c | 6 ++---- > > drivers/usb/gadget/function/f_mass_storage.h | 4 ---- > > 2 files changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c > > index b6e2930..ba0eb7e 100644 > > --- a/drivers/usb/gadget/function/f_mass_storage.c > > +++ b/drivers/usb/gadget/function/f_mass_storage.c > > @@ -2558,17 +2558,15 @@ static void fsg_lun_release(struct device *dev) > > /* Nothing needs to be done */ > > } > > > > -void fsg_common_get(struct fsg_common *common) > > +static void __maybe_unused fsg_common_get(struct fsg_common *common) > > { > > kref_get(&common->ref); > > } > > -EXPORT_SYMBOL_GPL(fsg_common_get); > > > > -void fsg_common_put(struct fsg_common *common) > > +static void fsg_common_put(struct fsg_common *common) > > { > > kref_put(&common->ref, fsg_common_release); > > } > > -EXPORT_SYMBOL_GPL(fsg_common_put); > > > > static struct fsg_common *fsg_common_setup(struct fsg_common *common) > > { > > diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h > > index 58857fc..3b8c4ce 100644 > > --- a/drivers/usb/gadget/function/f_mass_storage.h > > +++ b/drivers/usb/gadget/function/f_mass_storage.h > > @@ -115,10 +115,6 @@ fsg_opts_from_func_inst(const struct usb_function_instance *fi) > > return container_of(fi, struct fsg_opts, func_inst); > > } > > > > -void fsg_common_get(struct fsg_common *common); > > - > > -void fsg_common_put(struct fsg_common *common); > > - > > void fsg_common_set_sysfs(struct fsg_common *common, bool sysfs); > > > > int fsg_common_set_num_buffers(struct fsg_common *common, unsigned int n); > > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018년 6월 15일 (금) 오전 3:14, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>님이 작성: > > Hello! > > On 06/14/2018 12:23 PM, Jaejoong Kim wrote: > > > Removing EXPORT_SYMBOL_GPL from kref_{put, get} since it is used > > I thought you removed the exports from the real kref_{get|put}(). :-) > Your patch subject and description are simply misleading the way they are > written. Thanks, I will update with V2. > > > only in f_mass_storage > > > > Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com> > > --- > > drivers/usb/gadget/function/f_mass_storage.c | 6 ++---- > > drivers/usb/gadget/function/f_mass_storage.h | 4 ---- > > 2 files changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c > > index b6e2930..ba0eb7e 100644 > > --- a/drivers/usb/gadget/function/f_mass_storage.c > > +++ b/drivers/usb/gadget/function/f_mass_storage.c > > @@ -2558,17 +2558,15 @@ static void fsg_lun_release(struct device *dev) > > /* Nothing needs to be done */ > > } > > > > -void fsg_common_get(struct fsg_common *common) > > +static void __maybe_unused fsg_common_get(struct fsg_common *common) > > { > > kref_get(&common->ref); > > } > > -EXPORT_SYMBOL_GPL(fsg_common_get); > > > > -void fsg_common_put(struct fsg_common *common) > > +static void fsg_common_put(struct fsg_common *common) > > { > > kref_put(&common->ref, fsg_common_release); > > } > > -EXPORT_SYMBOL_GPL(fsg_common_put); > > > > static struct fsg_common *fsg_common_setup(struct fsg_common *common) > > { > [...] > > MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 15 Jun 2018, Jaejoong Kim wrote: > Thanks for the review it. > > 2018년 6월 14일 (목) 오후 11:48, Alan Stern <stern@rowland.harvard.edu>님이 작성: > > > > On Thu, 14 Jun 2018, Jaejoong Kim wrote: > > > > > Removing EXPORT_SYMBOL_GPL from kref_{put, get} since it is used > > > only in f_mass_storage > > > > > > Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com> > > > > This is only a partial solution. In fact, fsg_common_get() isn't used > > anywhere, and fsg_common_put() is used in only one place. > > Right. Actually my first approach was to remove fsg_common_get/put and just > use kref APIs(kref_get/put). But I kept current codes because this module author > might have another reason. > > > > > It would be better to remove those two routines, get rid of common->ref > > entirely, and make fsg_free_inst() call fsg_common_release() directly. > > Hm.. If you want to call fsg_common_release () in fsg_free_inst (), > the struct kref > in this moduel is meaningless. I prefer hold kref and just use kref APIs not > fsg_common_get/put. Michal Nazarewicz is the author of this part of the driver. He should make the decision of whether to keep the kref or eliminate it. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Getting rid of kref seems sensible to me. Reference counting used to be needed because sharing of fsg_common among multiple USB function instances was handled by fsg. Now this is handled by configfs layer and I don’t think the kref is necessary any more. 2018-06-14 19:23 GMT+01:00 Jaejoong Kim <climbbb.kim@gmail.com>: > Thanks for the review it. > > 2018년 6월 14일 (목) 오후 11:48, Alan Stern <stern@rowland.harvard.edu>님이 작성: >> >> On Thu, 14 Jun 2018, Jaejoong Kim wrote: >> >> > Removing EXPORT_SYMBOL_GPL from kref_{put, get} since it is used >> > only in f_mass_storage >> > >> > Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com> >> >> This is only a partial solution. In fact, fsg_common_get() isn't used >> anywhere, and fsg_common_put() is used in only one place. > > Right. Actually my first approach was to remove fsg_common_get/put and just > use kref APIs(kref_get/put). But I kept current codes because this module author > might have another reason. > >> >> It would be better to remove those two routines, get rid of common->ref >> entirely, and make fsg_free_inst() call fsg_common_release() directly. > > Hm.. If you want to call fsg_common_release () in fsg_free_inst (), > the struct kref > in this moduel is meaningless. I prefer hold kref and just use kref APIs not > fsg_common_get/put. > > Thanks, Jaejoong >> >> Alan Stern >> >> > --- >> > drivers/usb/gadget/function/f_mass_storage.c | 6 ++---- >> > drivers/usb/gadget/function/f_mass_storage.h | 4 ---- >> > 2 files changed, 2 insertions(+), 8 deletions(-) >> > >> > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c >> > index b6e2930..ba0eb7e 100644 >> > --- a/drivers/usb/gadget/function/f_mass_storage.c >> > +++ b/drivers/usb/gadget/function/f_mass_storage.c >> > @@ -2558,17 +2558,15 @@ static void fsg_lun_release(struct device *dev) >> > /* Nothing needs to be done */ >> > } >> > >> > -void fsg_common_get(struct fsg_common *common) >> > +static void __maybe_unused fsg_common_get(struct fsg_common *common) >> > { >> > kref_get(&common->ref); >> > } >> > -EXPORT_SYMBOL_GPL(fsg_common_get); >> > >> > -void fsg_common_put(struct fsg_common *common) >> > +static void fsg_common_put(struct fsg_common *common) >> > { >> > kref_put(&common->ref, fsg_common_release); >> > } >> > -EXPORT_SYMBOL_GPL(fsg_common_put); >> > >> > static struct fsg_common *fsg_common_setup(struct fsg_common *common) >> > { >> > diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h >> > index 58857fc..3b8c4ce 100644 >> > --- a/drivers/usb/gadget/function/f_mass_storage.h >> > +++ b/drivers/usb/gadget/function/f_mass_storage.h >> > @@ -115,10 +115,6 @@ fsg_opts_from_func_inst(const struct usb_function_instance *fi) >> > return container_of(fi, struct fsg_opts, func_inst); >> > } >> > >> > -void fsg_common_get(struct fsg_common *common); >> > - >> > -void fsg_common_put(struct fsg_common *common); >> > - >> > void fsg_common_set_sysfs(struct fsg_common *common, bool sysfs); >> > >> > int fsg_common_set_num_buffers(struct fsg_common *common, unsigned int n); >> > >> -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c index b6e2930..ba0eb7e 100644 --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -2558,17 +2558,15 @@ static void fsg_lun_release(struct device *dev) /* Nothing needs to be done */ } -void fsg_common_get(struct fsg_common *common) +static void __maybe_unused fsg_common_get(struct fsg_common *common) { kref_get(&common->ref); } -EXPORT_SYMBOL_GPL(fsg_common_get); -void fsg_common_put(struct fsg_common *common) +static void fsg_common_put(struct fsg_common *common) { kref_put(&common->ref, fsg_common_release); } -EXPORT_SYMBOL_GPL(fsg_common_put); static struct fsg_common *fsg_common_setup(struct fsg_common *common) { diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h index 58857fc..3b8c4ce 100644 --- a/drivers/usb/gadget/function/f_mass_storage.h +++ b/drivers/usb/gadget/function/f_mass_storage.h @@ -115,10 +115,6 @@ fsg_opts_from_func_inst(const struct usb_function_instance *fi) return container_of(fi, struct fsg_opts, func_inst); } -void fsg_common_get(struct fsg_common *common); - -void fsg_common_put(struct fsg_common *common); - void fsg_common_set_sysfs(struct fsg_common *common, bool sysfs); int fsg_common_set_num_buffers(struct fsg_common *common, unsigned int n);
Removing EXPORT_SYMBOL_GPL from kref_{put, get} since it is used only in f_mass_storage Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com> --- drivers/usb/gadget/function/f_mass_storage.c | 6 ++---- drivers/usb/gadget/function/f_mass_storage.h | 4 ---- 2 files changed, 2 insertions(+), 8 deletions(-)