On 5/22/20 10:54 AM, Hayes Wang wrote:
> Although I think it never occurs, the code doesn't make sense, because
> it may allow to assign an IN endpoint to ss->ep_out.
> 
> Signed-off-by: Hayes Wang <hayesw...@realtek.com>
> ---
>  drivers/usb/eth/r8152.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/eth/r8152.c b/drivers/usb/eth/r8152.c
> index 61b8683230..7c48663370 100644
> --- a/drivers/usb/eth/r8152.c
> +++ b/drivers/usb/eth/r8152.c
> @@ -1354,9 +1354,8 @@ int r8152_eth_probe(struct usb_device *dev, unsigned 
> int ifnum,
>       struct usb_interface *iface;
>       struct usb_interface_descriptor *iface_desc;
>       int ep_in_found = 0, ep_out_found = 0;
> -     int i;
> -
>       struct r8152 *tp;
> +     int i;
>  
>       /* let's examine the device now */
>       iface = &dev->config.if_desc[ifnum];
> @@ -1399,10 +1398,13 @@ int r8152_eth_probe(struct usb_device *dev, unsigned 
> int ifnum,
>               if ((iface->ep_desc[i].bmAttributes &
>                    USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {
>                       u8 ep_addr = iface->ep_desc[i].bEndpointAddress;
> -                     if ((ep_addr & USB_DIR_IN) && !ep_in_found) {
> -                             ss->ep_in = ep_addr &
> -                                     USB_ENDPOINT_NUMBER_MASK;
> -                             ep_in_found = 1;
> +
> +                     if (ep_addr & USB_DIR_IN) {
> +                             if (!ep_in_found) {
> +                                     ss->ep_in = ep_addr &
> +                                             USB_ENDPOINT_NUMBER_MASK;
> +                                     ep_in_found = 1;
> +                             }

So why don't you rework the code this way instead, to make it easier to
understand:

if ((ep_addr & USB_DIR_IN) && !ep_in_found) {
 ... do in stuff ...
}

if ((ep_addr & USB_DIR_OUT) && !ep_out_found) {
 ... do out stuff ...
}

Would that work ?

Reply via email to