diff mbox series

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

Message ID ebf0867900ae849581fbd20b52ee9e855e6345c8.1538734658.git.igor.russkikh@aquantia.com (mailing list archive)
State Superseded
Headers show
Series Add support for Aquantia AQtion USB to 5/2.5GbE devices | expand

Commit Message

Igor Russkikh Oct. 5, 2018, 10:24 a.m. UTC 
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

Comments

David Miller Oct. 5, 2018, 5:40 p.m. UTC | #1 
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.
Oliver Neukum Oct. 8, 2018, 1:44 p.m. UTC | #2 
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
Bjørn Mork Oct. 9, 2018, 1:33 p.m. UTC | #3 
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
diff mbox series

Patch

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 */
+