| Message ID | 1394712342-15778-370-Taiwan-albertk@realtek.com |
|---|---|
| State | Accepted |
| Commit | 3da0291ba9b4a429ed9226569c9014f5c7e13ac3 |
| Headers | show |
| Series | eth/r8152: minor corrections | expand |
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 <hayeswang at 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 ?
Marek Vasut [mailto:marex at denx.de] > Sent: Friday, May 22, 2020 9:22 PM [...] > > - 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: Ok, I'll do it and resend this patch. Thanks. > 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 ? Yes. Best Regards, Hayes
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; + } } else { if (!ep_out_found) { ss->ep_out = ep_addr &
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 <hayeswang at realtek.com> --- drivers/usb/eth/r8152.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)