Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 16 May 2022 12:31:01 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     David Kahurani <k.kahurani@gmail.com>
Cc:     netdev@vger.kernel.org,
        syzbot+d3dbdf31fbe9d8f5f311@syzkaller.appspotmail.com,
        davem@davemloft.net, jgg@ziepe.ca, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org, phil@philpotter.co.uk,
        syzkaller-bugs@googlegroups.com, arnd@arndb.de,
        dan.carpenter@oracle.com
Subject: Re: [PATCH] net: ax88179: add proper error handling of usb read
 errors
Message-ID: <20220516123101.467ad206@kernel.org>
In-Reply-To: <20220514133234.33796-1-k.kahurani@gmail.com>
References: <20220514133234.33796-1-k.kahurani@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sat, 14 May 2022 16:32:34 +0300 David Kahurani wrote:
> -			ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> -					 1, 1, &buf);
> +			ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_SROM_CMD,
> +					       1, 1, &buf);
> +			if (ret) {
> +				netdev_dbg(dev->net,
> +					   "Failed to read SROM_CMD: %d\n",
> +					   ret);
> +				return ret;
> +			}
>  
>  			if (time_after(jiffies, jtimeout))
>  				return -EINVAL;
>  
>  		} while (buf & EEP_BUSY);

I agree with Pavel, this seems unnecessarily strict. If the error is
not ENODEV we can keep looping.

> @@ -1581,7 +1731,13 @@ static int ax88179_link_reset(struct usbnet *dev)
>  				  &ax179_data->rxctl);
>  
>  		/*link up, check the usb device control TX FIFO full or empty*/
> -		ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
> +		ret = ax88179_read_cmd(dev, 0x81, 0x8c, 0, 4, &tmp32);
> +
> +		if (ret) {
> +			netdev_dbg(dev->net, "Failed to read TX FIFO: %d\n",
> +				   ret);
> +			return ret;
> +		}

Please don't add any empty lines within the error checking.
Empty lines are supposed to separate logically separate blocks of code.
Error checking is very much logically part of the call. And no empty
line betwee netdev_dbg() and return ret; either. In this submission you
have all possible configurations of having or not having empty lines
before the if or before the return. None of them should be there, and
please be consistent.