| Message ID | 1652322094-20698-1-git-send-email-quic_linyyuan@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: make f_loopback/f_sourcesink standalone | expand |
On Thu, May 12, 2022 at 10:21:34AM +0800, Linyu Yuan wrote: > First remove lb_modinit() and lb_modexit() call from f_sourcesink as both > function belong to f_loopback.c, also there is no need to export > disable_endpoints() from f_sourcesink, change it to static type. > > After first step, we can use DECLARE_USB_FUNCTION_INIT() macro in > f_sourcesink to create module init/exit function implicit as it only > register/unregister one usb function driver. > > In f_loopback disable_loopback() function, just add two usb_ep_disable() > call, it will safe to remove original disable_endpoints() call > which belong to f_sourcesink, and it also safe to use macro > DECLARE_USB_FUNCTION_INIT() for module init/exit purpose. > > Now it is safe to remove function prototype of lb_modinit(), > lb_modexit() and disable_endpoints() from g_zero.h. > > Change Makefile to build f_loopback/f_sourcesink as standalone module. This describes a lot of what you are doing, but not why you want to do any of this. Please read the kernel documentation for how to write a good kernel changelog, as-is, I do not understand why this change should be accepted at all. thanks, greg k-h
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Sent: Thursday, May 12, 2022 12:10 PM > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > Subject: Re: [PATCH] usb: gadget: make f_loopback/f_sourcesink standalone > > On Thu, May 12, 2022 at 10:21:34AM +0800, Linyu Yuan wrote: > > First remove lb_modinit() and lb_modexit() call from f_sourcesink as both > > function belong to f_loopback.c, also there is no need to export > > disable_endpoints() from f_sourcesink, change it to static type. > > > > After first step, we can use DECLARE_USB_FUNCTION_INIT() macro in > > f_sourcesink to create module init/exit function implicit as it only > > register/unregister one usb function driver. > > > > In f_loopback disable_loopback() function, just add two usb_ep_disable() > > call, it will safe to remove original disable_endpoints() call > > which belong to f_sourcesink, and it also safe to use macro > > DECLARE_USB_FUNCTION_INIT() for module init/exit purpose. > > > > Now it is safe to remove function prototype of lb_modinit(), > > lb_modexit() and disable_endpoints() from g_zero.h. > > > > Change Makefile to build f_loopback/f_sourcesink as standalone module. > > This describes a lot of what you are doing, but not why you want to do > any of this. > > Please read the kernel documentation for how to write a good kernel > changelog, as-is, I do not understand why this change should be accepted > at all. Can you share more about it ? is it description reason or code change itself ? > > thanks, > > greg k-h
On Thu, May 12, 2022 at 04:20:32AM +0000, Linyu Yuan (QUIC) wrote: > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Sent: Thursday, May 12, 2022 12:10 PM > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > > Subject: Re: [PATCH] usb: gadget: make f_loopback/f_sourcesink standalone > > > > On Thu, May 12, 2022 at 10:21:34AM +0800, Linyu Yuan wrote: > > > First remove lb_modinit() and lb_modexit() call from f_sourcesink as both > > > function belong to f_loopback.c, also there is no need to export > > > disable_endpoints() from f_sourcesink, change it to static type. > > > > > > After first step, we can use DECLARE_USB_FUNCTION_INIT() macro in > > > f_sourcesink to create module init/exit function implicit as it only > > > register/unregister one usb function driver. > > > > > > In f_loopback disable_loopback() function, just add two usb_ep_disable() > > > call, it will safe to remove original disable_endpoints() call > > > which belong to f_sourcesink, and it also safe to use macro > > > DECLARE_USB_FUNCTION_INIT() for module init/exit purpose. > > > > > > Now it is safe to remove function prototype of lb_modinit(), > > > lb_modexit() and disable_endpoints() from g_zero.h. > > > > > > Change Makefile to build f_loopback/f_sourcesink as standalone module. > > > > This describes a lot of what you are doing, but not why you want to do > > any of this. > > > > Please read the kernel documentation for how to write a good kernel > > changelog, as-is, I do not understand why this change should be accepted > > at all. > > Can you share more about it ? is it description reason or code change itself ? Here's the relevant text from my patch bot, which I should have triggered for this submission, that explains it a lot better: ----------- - You did not specify a description of why the patch is needed, or possibly, any description at all, in the email body. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/SubmittingPatches for what is needed in order to properly describe the change. - You did not write a descriptive Subject: for the patch, allowing Greg, and everyone else, to know what this patch is all about. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/SubmittingPatches for what a proper Subject: line should look like. --------------- Does that help? greg k-h
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Sent: Thursday, May 12, 2022 12:48 PM > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > Subject: Re: [PATCH] usb: gadget: make f_loopback/f_sourcesink standalone > > On Thu, May 12, 2022 at 04:20:32AM +0000, Linyu Yuan (QUIC) wrote: > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Sent: Thursday, May 12, 2022 12:10 PM > > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > > > Subject: Re: [PATCH] usb: gadget: make f_loopback/f_sourcesink > standalone > > > > > > On Thu, May 12, 2022 at 10:21:34AM +0800, Linyu Yuan wrote: > > > > First remove lb_modinit() and lb_modexit() call from f_sourcesink as > both > > > > function belong to f_loopback.c, also there is no need to export > > > > disable_endpoints() from f_sourcesink, change it to static type. > > > > > > > > After first step, we can use DECLARE_USB_FUNCTION_INIT() macro in > > > > f_sourcesink to create module init/exit function implicit as it only > > > > register/unregister one usb function driver. > > > > > > > > In f_loopback disable_loopback() function, just add two > usb_ep_disable() > > > > call, it will safe to remove original disable_endpoints() call > > > > which belong to f_sourcesink, and it also safe to use macro > > > > DECLARE_USB_FUNCTION_INIT() for module init/exit purpose. > > > > > > > > Now it is safe to remove function prototype of lb_modinit(), > > > > lb_modexit() and disable_endpoints() from g_zero.h. > > > > > > > > Change Makefile to build f_loopback/f_sourcesink as standalone > module. > > > > > > This describes a lot of what you are doing, but not why you want to do > > > any of this. > > > > > > Please read the kernel documentation for how to write a good kernel > > > changelog, as-is, I do not understand why this change should be accepted > > > at all. > > > > Can you share more about it ? is it description reason or code change itself ? > > Here's the relevant text from my patch bot, which I should have > triggered for this submission, that explains it a lot better: > > ----------- > > - You did not specify a description of why the patch is needed, or > possibly, any description at all, in the email body. Please read the > section entitled "The canonical patch format" in the kernel file, > Documentation/SubmittingPatches for what is needed in order to > properly describe the change. > > - You did not write a descriptive Subject: for the patch, allowing Greg, > and everyone else, to know what this patch is all about. Please read > the section entitled "The canonical patch format" in the kernel file, > Documentation/SubmittingPatches for what a proper Subject: line should > look like. > > --------------- > > Does that help? Got it, Or not I think I made some cray thing that you said "I do not understand why this change should be accepted at all" > > greg k-h
> From: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > Sent: Thursday, May 12, 2022 1:07 PM > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Linyu Yuan (QUIC) > <quic_linyyuan@quicinc.com> > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > Subject: RE: [PATCH] usb: gadget: make f_loopback/f_sourcesink standalone > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Sent: Thursday, May 12, 2022 12:48 PM > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > > Subject: Re: [PATCH] usb: gadget: make f_loopback/f_sourcesink > standalone Sorry for this noise, seem when enable legacy gadget zero, it expect f_loopback/f_sourcesink design as current.
diff --git a/drivers/usb/gadget/function/Makefile b/drivers/usb/gadget/function/Makefile index 5d3a6cf..6d01495 100644 --- a/drivers/usb/gadget/function/Makefile +++ b/drivers/usb/gadget/function/Makefile @@ -9,8 +9,8 @@ ccflags-y += -I$(srctree)/drivers/usb/gadget/udc/ # USB Functions usb_f_acm-y := f_acm.o obj-$(CONFIG_USB_F_ACM) += usb_f_acm.o -usb_f_ss_lb-y := f_loopback.o f_sourcesink.o -obj-$(CONFIG_USB_F_SS_LB) += usb_f_ss_lb.o +obj-$(CONFIG_USB_F_SS_LB) += f_loopback.o +obj-$(CONFIG_USB_F_SS_LB) += f_sourcesink.o obj-$(CONFIG_USB_U_SERIAL) += u_serial.o usb_f_serial-y := f_serial.o obj-$(CONFIG_USB_F_SERIAL) += usb_f_serial.o diff --git a/drivers/usb/gadget/function/f_loopback.c b/drivers/usb/gadget/function/f_loopback.c index ae41f55..77fbed0 100644 --- a/drivers/usb/gadget/function/f_loopback.c +++ b/drivers/usb/gadget/function/f_loopback.c @@ -296,9 +296,18 @@ static void loopback_complete(struct usb_ep *ep, struct usb_request *req) static void disable_loopback(struct f_loopback *loop) { struct usb_composite_dev *cdev; + int value; cdev = loop->function.config->cdev; - disable_endpoints(cdev, loop->in_ep, loop->out_ep, NULL, NULL); + + value = usb_ep_disable(loop->in_ep); + if (value < 0) + DBG(cdev, "disable %s --> %d\n", loop->in_ep->name, value); + + value = usb_ep_disable(loop->out_ep); + if (value < 0) + DBG(cdev, "disable %s --> %d\n", loop->out_ep->name, value); + VDBG(cdev, "%s disabled\n", loop->function.name); } @@ -583,16 +592,6 @@ static struct usb_function_instance *loopback_alloc_instance(void) return &lb_opts->func_inst; } -DECLARE_USB_FUNCTION(Loopback, loopback_alloc_instance, loopback_alloc); - -int __init lb_modinit(void) -{ - return usb_function_register(&Loopbackusb_func); -} - -void __exit lb_modexit(void) -{ - usb_function_unregister(&Loopbackusb_func); -} +DECLARE_USB_FUNCTION_INIT(Loopback, loopback_alloc_instance, loopback_alloc); MODULE_LICENSE("GPL"); diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c index 6803cd6..9441285 100644 --- a/drivers/usb/gadget/function/f_sourcesink.c +++ b/drivers/usb/gadget/function/f_sourcesink.c @@ -301,7 +301,7 @@ static void disable_ep(struct usb_composite_dev *cdev, struct usb_ep *ep) DBG(cdev, "disable %s --> %d\n", ep->name, value); } -void disable_endpoints(struct usb_composite_dev *cdev, +static void disable_endpoints(struct usb_composite_dev *cdev, struct usb_ep *in, struct usb_ep *out, struct usb_ep *iso_in, struct usb_ep *iso_out) { @@ -1263,27 +1263,7 @@ static struct usb_function_instance *source_sink_alloc_inst(void) return &ss_opts->func_inst; } -DECLARE_USB_FUNCTION(SourceSink, source_sink_alloc_inst, +DECLARE_USB_FUNCTION_INIT(SourceSink, source_sink_alloc_inst, source_sink_alloc_func); -static int __init sslb_modinit(void) -{ - int ret; - - ret = usb_function_register(&SourceSinkusb_func); - if (ret) - return ret; - ret = lb_modinit(); - if (ret) - usb_function_unregister(&SourceSinkusb_func); - return ret; -} -static void __exit sslb_modexit(void) -{ - usb_function_unregister(&SourceSinkusb_func); - lb_modexit(); -} -module_init(sslb_modinit); -module_exit(sslb_modexit); - MODULE_LICENSE("GPL"); diff --git a/drivers/usb/gadget/function/g_zero.h b/drivers/usb/gadget/function/g_zero.h index 98b8462..23f55e7 100644 --- a/drivers/usb/gadget/function/g_zero.h +++ b/drivers/usb/gadget/function/g_zero.h @@ -62,12 +62,4 @@ struct f_lb_opts { int refcnt; }; -void lb_modexit(void); -int lb_modinit(void); - -/* common utilities */ -void disable_endpoints(struct usb_composite_dev *cdev, - struct usb_ep *in, struct usb_ep *out, - struct usb_ep *iso_in, struct usb_ep *iso_out); - #endif /* __G_ZERO_H */
First remove lb_modinit() and lb_modexit() call from f_sourcesink as both function belong to f_loopback.c, also there is no need to export disable_endpoints() from f_sourcesink, change it to static type. After first step, we can use DECLARE_USB_FUNCTION_INIT() macro in f_sourcesink to create module init/exit function implicit as it only register/unregister one usb function driver. In f_loopback disable_loopback() function, just add two usb_ep_disable() call, it will safe to remove original disable_endpoints() call which belong to f_sourcesink, and it also safe to use macro DECLARE_USB_FUNCTION_INIT() for module init/exit purpose. Now it is safe to remove function prototype of lb_modinit(), lb_modexit() and disable_endpoints() from g_zero.h. Change Makefile to build f_loopback/f_sourcesink as standalone module. Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- drivers/usb/gadget/function/Makefile | 4 ++-- drivers/usb/gadget/function/f_loopback.c | 23 +++++++++++------------ drivers/usb/gadget/function/f_sourcesink.c | 24 ++---------------------- drivers/usb/gadget/function/g_zero.h | 8 -------- 4 files changed, 15 insertions(+), 44 deletions(-)