Re: [PATCH -next] net: usb: asix: Add check for usbnet_get_endpoints

["yiyang (D)" <yiyang13@xxxxxxxxxx>]

On 2024/4/3 18:53, Simon Horman wrote:
> On Tue, Apr 02, 2024 at 11:30:48AM +0000, Yi Yang wrote:
> > Add check for usbnet_get_endpoints() and return the error if it fails
> > in order to transfer the error.
> >
> > Signed-off-by: Yi Yang <yiyang13@xxxxxxxxxx>
>
> Hi,
>
> I am wondering if this is a fix for a user-visible problem and as such
> should:
> 1. Be targeted at net
> 2. Have a Fixes tag
> 3. CC stable
I will split this patch to two patch. one for bugfix, anorther one for 
cleanup.
> See: https://docs.kernel.org/process/maintainer-netdev.html
>
> > ---
> >   drivers/net/usb/asix_devices.c | 20 +++++++++++++-------
> >   1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> > index f7cff58fe044..11417ed86d9e 100644
> > --- a/drivers/net/usb/asix_devices.c
> > +++ b/drivers/net/usb/asix_devices.c
> > @@ -230,7 +230,9 @@ static int ax88172_bind(struct usbnet *dev, struct usb_interface *intf)
> >   	int i;
> >   	unsigned long gpio_bits = dev->driver_info->data;
> >   
> > -	usbnet_get_endpoints(dev,intf);
> > +	ret = usbnet_get_endpoints(dev, intf);
> > +	if (ret)
> > +		goto out;
> >   
> >   	/* Toggle the GPIOs in a manufacturer/model specific way */
> >   	for (i = 2; i >= 0; i--) {
> > @@ -834,7 +836,9 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
> >   
> >   	dev->driver_priv = priv;
> >   
> > -	usbnet_get_endpoints(dev, intf);
> > +	ret = usbnet_get_endpoints(dev, intf);
> > +	if (ret)
> > +		goto mdio_err;
> >   
> >   	/* Maybe the boot loader passed the MAC address via device tree */
> >   	if (!eth_platform_get_mac_address(&dev->udev->dev, buf)) {
> > @@ -858,7 +862,7 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
> >   		if (ret < 0) {
> >   			netdev_dbg(dev->net, "Failed to read MAC address: %d\n",
> >   				   ret);
> > -			return ret;
> > +			goto mdio_err;
> >   		}
> >   	}
> >   
>
> The two hunks above do not seem related to the subject of the patch, but
> rather separate cleanups. So I think they should not be part of this patch.
> Instead they could be a separate patch, targeted at net-next.  (FWIIW, I
> would go the other way and drop the mdio_err label from this function.)
>
> > @@ -871,7 +875,7 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
> >   
> >   	ret = asix_read_phy_addr(dev, true);
> >   	if (ret < 0)
> > -		return ret;
> > +		goto mdio_err;
> >   
> >   	priv->phy_addr = ret;
> >   	priv->embd_phy = ((priv->phy_addr & 0x1f) == AX_EMBD_PHY_ADDR);
> > @@ -880,7 +884,7 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
> >   			    &priv->chipcode, 0);
> >   	if (ret < 0) {
> >   		netdev_dbg(dev->net, "Failed to read STATMNGSTS_REG: %d\n", ret);
> > -		return ret;
> > +		goto mdio_err;
> >   	}
> >   
> >   	priv->chipcode &= AX_CHIPCODE_MASK;
> > @@ -895,7 +899,7 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
> >   	ret = priv->reset(dev, 0);
> >   	if (ret < 0) {
> >   		netdev_dbg(dev->net, "Failed to reset AX88772: %d\n", ret);
> > -		return ret;
> > +		goto mdio_err;
> >   	}
> >   
> >   	/* Asix framing packs multiple eth frames into a 2K usb bulk transfer */
> > @@ -1258,7 +1262,9 @@ static int ax88178_bind(struct usbnet *dev, struct usb_interface *intf)
> >   	int ret;
> >   	u8 buf[ETH_ALEN] = {0};
> >   
> > -	usbnet_get_endpoints(dev,intf);
> > +	ret = usbnet_get_endpoints(dev, intf);
> > +	if (ret)
> > +		return ret;
> >   
> >   	/* Get the MAC address */
> >   	ret = asix_read_cmd(dev, AX_CMD_READ_NODE_ID, 0, 0, ETH_ALEN, buf, 0);
> > --
> > 2.25.1
> .