[net-next,03/19] net: usb: aqc111: Add implementation of read and write commands

[net-next,03/19] net: usb: aqc111: Add implementation of read and write commands

[Igor Russkikh]

From: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>

Read/write command register defines and functions

Signed-off-by: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
 drivers/net/usb/aqc111.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/usb/aqc111.h |  19 ++++++++
 2 files changed, 143 insertions(+)
 create mode 100644 drivers/net/usb/aqc111.h

diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
index c914e19387f2..7f3e5a615750 100644
--- a/drivers/net/usb/aqc111.c
+++ b/drivers/net/usb/aqc111.c
@@ -14,6 +14,130 @@
 #include <linux/usb/cdc.h>
 #include <linux/usb/usbnet.h>
 
+#include "aqc111.h"
+
+static int __aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
+			     u16 index, u16 size, void *data, int nopm)
+{
+	int ret;
+	int (*fn)(struct usbnet *dev, u8 cmd, u8 reqtype, u16 value,
+		  u16 index, void *data, u16 size);
+
+	if (nopm)
+		fn = usbnet_read_cmd_nopm;
+	else
+		fn = usbnet_read_cmd;
+
+	ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		 value, index, data, size);
+	if (size == 2)
+		le16_to_cpus(data);
+
+	if (unlikely(ret < 0))
+		netdev_warn(dev->net,
+			    "Failed to read(0x%x) reg index 0x%04x: %d\n",
+			    cmd, index, ret);
+	return ret;
+}
+
+static int aqc111_read_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
+				u16 index, u16 size, void *data)
+{
+	return __aqc111_read_cmd(dev, cmd, value, index, size, data, 1);
+}
+
+static int aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
+			   u16 index, u16 size, void *data)
+{
+	return __aqc111_read_cmd(dev, cmd, value, index, size, data, 0);
+}
+
+static int __aq_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
+			  u16 value, u16 index, const void *data, u16 size)
+{
+	void *buf = NULL;
+	int err = -ENOMEM;
+
+	netdev_dbg(dev->net,
+		   "%s cmd=%#x reqtype=%#x value=%#x index=%#x size=%d\n",
+		   __func__, cmd, reqtype, value, index, size);
+
+	if (data) {
+		buf = kmemdup(data, size, GFP_KERNEL);
+		if (!buf)
+			goto out;
+	}
+
+	if (size == 2)
+		cpu_to_le16s(buf);
+	else if (size == 4)
+		cpu_to_le32s(buf);
+
+	err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
+			      cmd, reqtype, value, index, buf, size,
+			      (cmd == AQ_PHY_POWER) ? AQ_USB_PHY_SET_TIMEOUT :
+			      AQ_USB_SET_TIMEOUT);
+
+	kfree(buf);
+
+out:
+	return err;
+}
+
+static int aq_write_cmd_nopm(struct usbnet *dev, u8 cmd, u8 reqtype,
+			     u16 value, u16 index, const void *data, u16 size)
+{
+	return __aq_write_cmd(dev, cmd, reqtype, value, index,
+			      data, size);
+}
+
+static int aq_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
+			u16 value, u16 index, const void *data, u16 size)
+{
+	int ret;
+
+	if (usb_autopm_get_interface(dev->intf) < 0)
+		return -ENODEV;
+	ret = __aq_write_cmd(dev, cmd, reqtype, value, index, data, size);
+	usb_autopm_put_interface(dev->intf);
+	return ret;
+}
+
+static int __aqc111_write_cmd(struct usbnet *dev, u8 cmd, u16 value,
+			      u16 index, u16 size, void *data, int nopm)
+{
+	int ret;
+	int (*fn)(struct usbnet *dev, u8 cmd, u8 reqtype, u16 value,
+		  u16 index, const void *data, u16 size);
+
+	if (nopm)
+		fn = aq_write_cmd_nopm;
+	else
+		fn = aq_write_cmd;
+
+	ret = fn(dev, cmd, USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		 value, index, data, size);
+
+	if (unlikely(ret < 0))
+		netdev_warn(dev->net,
+			    "Failed to write(0x%x) reg 0x%04x: %d\n",
+			    cmd, value, ret);
+
+	return ret;
+}
+
+static int aqc111_write_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
+				 u16 index, u16 size, void *data)
+{
+	return __aqc111_write_cmd(dev, cmd, value, index, size, data, 1);
+}
+
+static int aqc111_write_cmd(struct usbnet *dev, u8 cmd, u16 value,
+			    u16 index, u16 size, void *data)
+{
+	return __aqc111_write_cmd(dev, cmd, value, index, size, data, 0);
+}
+
 static const struct net_device_ops aqc111_netdev_ops = {
 	.ndo_open		= usbnet_open,
 	.ndo_stop		= usbnet_stop,
diff --git a/drivers/net/usb/aqc111.h b/drivers/net/usb/aqc111.h
new file mode 100644
index 000000000000..1698a6a104fe
--- /dev/null
+++ b/drivers/net/usb/aqc111.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ * Aquantia Corp. Aquantia AQtion USB to 5GbE Controller
+ * Copyright (C) 2003-2005 David Hollis <dhollis@davehollis.com>
+ * Copyright (C) 2005 Phil Chang <pchang23@sbcglobal.net>
+ * Copyright (C) 2002-2003 TiVo Inc.
+ * Copyright (C) 2017-2018 ASIX
+ * Copyright (C) 2018 Aquantia Corp.
+ */
+
+#ifndef __LINUX_USBNET_AQC111_H
+#define __LINUX_USBNET_AQC111_H
+
+#define AQ_PHY_POWER			0x31
+
+#define AQ_USB_PHY_SET_TIMEOUT		10000
+#define AQ_USB_SET_TIMEOUT		4000
+
+#endif /* __LINUX_USBNET_AQC111_H */
+

[net-next,03/19] net: usb: aqc111: Add implementation of read and write commands

[David Miller]

From: Igor Russkikh <Igor.Russkikh@aquantia.com>
Date: Fri, 5 Oct 2018 10:24:44 +0000

> +static int __aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
> +			     u16 index, u16 size, void *data, int nopm)
> +{
> +	int ret;
> +	int (*fn)(struct usbnet *dev, u8 cmd, u8 reqtype, u16 value,
> +		  u16 index, void *data, u16 size);

Again, please order local variables from longest to shortest line.

I won't explicitly point out the others, you need to audit your entire
submission for this problem and fix it up.

Thank you.

[net-next,03/19] net: usb: aqc111: Add implementation of read and write commands

[Oliver Neukum]

On Fr, 2018-10-05 at 10:24 +0000, Igor Russkikh wrote:
> From: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
> 
> Read/write command register defines and functions
> 
> Signed-off-by: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
> ---
>  drivers/net/usb/aqc111.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/usb/aqc111.h |  19 ++++++++
>  2 files changed, 143 insertions(+)
>  create mode 100644 drivers/net/usb/aqc111.h
> 
> diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
> index c914e19387f2..7f3e5a615750 100644
> --- a/drivers/net/usb/aqc111.c
> +++ b/drivers/net/usb/aqc111.c
> @@ -14,6 +14,130 @@
>  #include <linux/usb/cdc.h>
>  #include <linux/usb/usbnet.h>
>  
> +#include "aqc111.h"
> +
> +static int __aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
> +			     u16 index, u16 size, void *data, int nopm)
> +{
> +	int ret;
> +	int (*fn)(struct usbnet *dev, u8 cmd, u8 reqtype, u16 value,
> +		  u16 index, void *data, u16 size);
> +
> +	if (nopm)
> +		fn = usbnet_read_cmd_nopm;
> +	else
> +		fn = usbnet_read_cmd;

If you really want to do this, pass the function.

> +
> +	ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> +		 value, index, data, size);
> +	if (size == 2)
> +		le16_to_cpus(data);

That is incredibly dirty

> +
> +	if (unlikely(ret < 0))
> +		netdev_warn(dev->net,
> +			    "Failed to read(0x%x) reg index 0x%04x: %d\n",
> +			    cmd, index, ret);
> +	return ret;
> +}
> +
> +static int aqc111_read_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
> +				u16 index, u16 size, void *data)
> +{
> +	return __aqc111_read_cmd(dev, cmd, value, index, size, data, 1);
> +}
> +
> +static int aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
> +			   u16 index, u16 size, void *data)
> +{
> +	return __aqc111_read_cmd(dev, cmd, value, index, size, data, 0);
> +}
> +
> +static int __aq_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
> +			  u16 value, u16 index, const void *data, u16 size)
> +{
> +	void *buf = NULL;
> +	int err = -ENOMEM;
> +
> +	netdev_dbg(dev->net,
> +		   "%s cmd=%#x reqtype=%#x value=%#x index=%#x size=%d\n",
> +		   __func__, cmd, reqtype, value, index, size);
> +
> +	if (data) {
> +		buf = kmemdup(data, size, GFP_KERNEL);

Under which contexts is this used?

	Regards
		Oliver

[net-next,03/19] net: usb: aqc111: Add implementation of read and write commands

[Bjørn Mork]

Igor Russkikh <Igor.Russkikh@aquantia.com> writes:

> +static int __aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
> +			     u16 index, u16 size, void *data, int nopm)
> +{
> +	int ret;
> +	int (*fn)(struct usbnet *dev, u8 cmd, u8 reqtype, u16 value,
> +		  u16 index, void *data, u16 size);
> +
> +	if (nopm)
> +		fn = usbnet_read_cmd_nopm;
> +	else
> +		fn = usbnet_read_cmd;
> +
> +	ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> +		 value, index, data, size);
> +	if (size == 2)
> +		le16_to_cpus(data);
> +
> +	if (unlikely(ret < 0))
> +		netdev_warn(dev->net,
> +			    "Failed to read(0x%x) reg index 0x%04x: %d\n",
> +			    cmd, index, ret);
> +	return ret;
> +}
> +
> +static int aqc111_read_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
> +				u16 index, u16 size, void *data)
> +{
> +	return __aqc111_read_cmd(dev, cmd, value, index, size, data, 1);
> +}
> +
> +static int aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value,
> +			   u16 index, u16 size, void *data)
> +{
> +	return __aqc111_read_cmd(dev, cmd, value, index, size, data, 0);
> +}
> +

Why would you want to do something like this instead of simply
implementing aqc111_read_cmd_nopm() and aqc111_read_cmd() as separate
functions?  The function pointer stuff is incredibly ugly, as Oliver
pointed out.  It wasn't done like that in usbnet.c, so why should we do
it like that here?

And the "if (size == 2) le16_to_cpus(data)" looks like something that
will come back and haunt you.  Will this code never read larger
integers?  Maybe add some sanity checks then, just in case...

Or simply add more helpers.  An additional pair of helpers for reading
16bit integers might simplify your code quite a bit.


Bjørn