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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87d0sj9rdm.fsf@miraculix.mork.no>
Date:   Tue, 09 Oct 2018 15:33:25 +0200
From:   Bjørn Mork <bjorn@...k.no>
To:     Igor Russkikh <Igor.Russkikh@...antia.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        "linux-usb\@vger.kernel.org" <linux-usb@...r.kernel.org>,
        "netdev\@vger.kernel.org" <netdev@...r.kernel.org>,
        Dmitry Bezrukov <Dmitry.Bezrukov@...antia.com>
Subject: Re: [PATCH net-next 03/19] net: usb: aqc111: Add implementation of read and write commands

Igor Russkikh <Igor.Russkikh@...antia.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