On Wed, Apr 13, 2022 at 03:36:57PM +0300, David Kahurani wrote: > On Mon, Apr 4, 2022 at 6:32 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > > Hi Dan > > > > int ret; > > > int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16); > > > @@ -201,9 +202,12 @@ static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index, > > > ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > > > value, index, data, size); > > > > > > - if (unlikely(ret < 0)) > > > + if (unlikely(ret < size)) { > > > + ret = ret < 0 ? ret : -ENODATA; > > > + > > > netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n", > > > index, ret); > > > + } > > > > > > return ret; > > > > It would be better to make __ax88179_read_cmd() return 0 on success > > instead of returning size on success. Non-standard returns lead to bugs. > > > > I don't suppose this would have much effect on the structure of the > code and indeed plan to do this but just some minor clarification. > > Isn't it standard for reader functions to return the number of bytes read? > Not really. There are some functions that do it, but it has historically lead to bug after bug. For example, see commit 719b8f2850d3 ("USB: add usb_control_msg_send() and usb_control_msg_recv()") where USB is moving away from that to avoid bugs. If you return zero on success then it's simple: if (ret) return ret; If you return the bytes people will try call kinds of things: if (ret) return ret; Bug: Now the driver is broken. (Not everyone can test the hardware). if (ret != size) return ret; Bug: returns a positive. if (ret != size) return -EIO; Bug: forgot to propagate the error code. if (ret < sizeof(foo)) return -EIO; Bug: because of type promotion negative error codes are treated as success. if (ret < 0) return ret; Bug: buffer partially filled. Information leak. If you return the bytes then the only correct way to write error handling is: if (ret < 0) return ret; if (ret != size) return -EIO; regards, dan carpenter