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