| Message ID | 20220306075524.706660-4-mailhol.vincent@wanadoo.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: rework usb_maxpacket() and remove its third argument | expand |
On Sun, Mar 06, 2022 at 04:55:17PM +0900, Vincent Mailhol wrote: > This is a transitional patch with the goal of changing the prototype > of usb_maxpacket() from: > | static inline __u16 > | usb_maxpacket(struct usb_device *udev, int pipe, int is_out) > > into: > | static inline u16 usb_maxpacket(struct usb_device *dev, int pipe) > > The third argument of usb_maxpacket(): is_out gets removed because it > can be derived from its second argument: pipe using > usb_pipeout(pipe). Furthermore, in the current version, > ubs_pipeout(pipe) is called regardless in order to sanitize the is_out > parameter. > > In order to make a smooth change, we first deprecate the is_out > parameter by simply ignoring it (using a variadic function) and will > remove it latter, once all the callers get updated. > > Finally, the body of the function is reworked in order not to reinvent > the wheel and just relies on the usb_pipe_endpoint() helper function > instead. > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- > include/linux/usb.h | 24 +++--------------------- > 1 file changed, 3 insertions(+), 21 deletions(-) > > diff --git a/include/linux/usb.h b/include/linux/usb.h > index 200b7b79acb5..588aa7dc3d10 100644 > --- a/include/linux/usb.h > +++ b/include/linux/usb.h > @@ -1969,30 +1969,12 @@ usb_pipe_endpoint(struct usb_device *dev, unsigned int pipe) > return eps[usb_pipeendpoint(pipe)]; > } > > -/*-------------------------------------------------------------------------*/ > - > -static inline __u16 > -usb_maxpacket(struct usb_device *udev, int pipe, int is_out) > +static inline u16 usb_maxpacket(struct usb_device *dev, int pipe, > + /* int is_out deprecated */ ...) No need to change from udev->dev, right? > { > - struct usb_host_endpoint *ep; > - unsigned epnum = usb_pipeendpoint(pipe); > - > - if (is_out) { > - WARN_ON(usb_pipein(pipe)); > - ep = udev->ep_out[epnum]; > - } else { > - WARN_ON(usb_pipeout(pipe)); > - ep = udev->ep_in[epnum]; > - } > - if (!ep) > - return 0; > - > - /* NOTE: only 0x07ff bits are for packet size... */ > - return usb_endpoint_maxp(&ep->desc); > + return usb_endpoint_maxp(&usb_pipe_endpoint(dev, pipe)->desc); The change to use usb_pipe_endpoint() can be done separately. Let's make these in tiny steps so that we can easily roll things back if things are not working. thanks, greg k-h
On Wed. 16 mars 2022 at 02:25, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Sun, Mar 06, 2022 at 04:55:17PM +0900, Vincent Mailhol wrote: > > This is a transitional patch with the goal of changing the prototype > > of usb_maxpacket() from: > > | static inline __u16 > > | usb_maxpacket(struct usb_device *udev, int pipe, int is_out) > > > > into: > > | static inline u16 usb_maxpacket(struct usb_device *dev, int pipe) > > > > The third argument of usb_maxpacket(): is_out gets removed because it > > can be derived from its second argument: pipe using > > usb_pipeout(pipe). Furthermore, in the current version, > > ubs_pipeout(pipe) is called regardless in order to sanitize the is_out > > parameter. > > > > In order to make a smooth change, we first deprecate the is_out > > parameter by simply ignoring it (using a variadic function) and will > > remove it latter, once all the callers get updated. > > > > Finally, the body of the function is reworked in order not to reinvent > > the wheel and just relies on the usb_pipe_endpoint() helper function > > instead. > > > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > > --- > > include/linux/usb.h | 24 +++--------------------- > > 1 file changed, 3 insertions(+), 21 deletions(-) > > > > diff --git a/include/linux/usb.h b/include/linux/usb.h > > index 200b7b79acb5..588aa7dc3d10 100644 > > --- a/include/linux/usb.h > > +++ b/include/linux/usb.h > > @@ -1969,30 +1969,12 @@ usb_pipe_endpoint(struct usb_device *dev, unsigned int pipe) > > return eps[usb_pipeendpoint(pipe)]; > > } > > > > -/*-------------------------------------------------------------------------*/ > > - > > -static inline __u16 > > -usb_maxpacket(struct usb_device *udev, int pipe, int is_out) > > +static inline u16 usb_maxpacket(struct usb_device *dev, int pipe, > > + /* int is_out deprecated */ ...) > > No need to change from udev->dev, right? Right. The motivation of this change was to align with other functions (the majority of the functions in linux/usb.h name it dev, not udev). Comment taken, I will keep the udev name in v3. > > { > > - struct usb_host_endpoint *ep; > > - unsigned epnum = usb_pipeendpoint(pipe); > > - > > - if (is_out) { > > - WARN_ON(usb_pipein(pipe)); > > - ep = udev->ep_out[epnum]; > > - } else { > > - WARN_ON(usb_pipeout(pipe)); > > - ep = udev->ep_in[epnum]; > > - } > > - if (!ep) > > - return 0; > > - > > - /* NOTE: only 0x07ff bits are for packet size... */ > > - return usb_endpoint_maxp(&ep->desc); > > + return usb_endpoint_maxp(&usb_pipe_endpoint(dev, pipe)->desc); > > The change to use usb_pipe_endpoint() can be done separately. > > Let's make these in tiny steps so that we can easily roll things back if > things are not working. ACK. I will respin the patch series as below: * Make usb_maxpacket variadic * Migrate the callers of usb_maxpacket() * Suppress the third argument of usb_maxpacket() * Change the body of usb_maxpacket() using usb_pipe_endpoint(). > thanks, > > greg k-h
diff --git a/include/linux/usb.h b/include/linux/usb.h index 200b7b79acb5..588aa7dc3d10 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -1969,30 +1969,12 @@ usb_pipe_endpoint(struct usb_device *dev, unsigned int pipe) return eps[usb_pipeendpoint(pipe)]; } -/*-------------------------------------------------------------------------*/ - -static inline __u16 -usb_maxpacket(struct usb_device *udev, int pipe, int is_out) +static inline u16 usb_maxpacket(struct usb_device *dev, int pipe, + /* int is_out deprecated */ ...) { - struct usb_host_endpoint *ep; - unsigned epnum = usb_pipeendpoint(pipe); - - if (is_out) { - WARN_ON(usb_pipein(pipe)); - ep = udev->ep_out[epnum]; - } else { - WARN_ON(usb_pipeout(pipe)); - ep = udev->ep_in[epnum]; - } - if (!ep) - return 0; - - /* NOTE: only 0x07ff bits are for packet size... */ - return usb_endpoint_maxp(&ep->desc); + return usb_endpoint_maxp(&usb_pipe_endpoint(dev, pipe)->desc); } -/* ----------------------------------------------------------------------- */ - /* translate USB error codes to codes user space understands */ static inline int usb_translate_errors(int error_code) {
This is a transitional patch with the goal of changing the prototype of usb_maxpacket() from: | static inline __u16 | usb_maxpacket(struct usb_device *udev, int pipe, int is_out) into: | static inline u16 usb_maxpacket(struct usb_device *dev, int pipe) The third argument of usb_maxpacket(): is_out gets removed because it can be derived from its second argument: pipe using usb_pipeout(pipe). Furthermore, in the current version, ubs_pipeout(pipe) is called regardless in order to sanitize the is_out parameter. In order to make a smooth change, we first deprecate the is_out parameter by simply ignoring it (using a variadic function) and will remove it latter, once all the callers get updated. Finally, the body of the function is reworked in order not to reinvent the wheel and just relies on the usb_pipe_endpoint() helper function instead. Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> --- include/linux/usb.h | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-)