linux-kernel - Re: [PATCH] staging: Initial driver submission for pureLiFi devices

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]

Date:   Thu, 24 Sep 2020 22:07:58 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Srinivasan Raju <srini.raju@...elifi.com>
Cc:     "open list:STAGING SUBSYSTEM" <devel@...verdev.osuosl.org>,
        Rob Herring <robh@...nel.org>,
        pureLiFi Ltd <info@...elifi.com>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        mostafa.afgani@...elifi.com,
        open list <linux-kernel@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] staging: Initial driver submission for pureLiFi devices

On Thu, Sep 24, 2020 at 08:48:51PM +0530, Srinivasan Raju wrote:
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +
> +#include "def.h"
> +#include "chip.h"
> +#include "mac.h"
> +#include "usb.h"
> +#include "log.h"
> +
> +void purelifi_chip_init(struct purelifi_chip *chip,
> +			struct ieee80211_hw *hw,
> +		struct usb_interface *intf
> +		)

There is a bunch of really trivial messiness like this.  It should
look like:

void purelifi_chip_init(struct purelifi_chip *chip,
			struct ieee80211_hw *hw,
			struct usb_interface *intf)


> +{
> +	memset(chip, 0, sizeof(*chip));
> +	mutex_init(&chip->mutex);
> +	purelifi_usb_init(&chip->usb, hw, intf);
> +}
> +
> +void purelifi_chip_clear(struct purelifi_chip *chip)
> +{
> +	PURELIFI_ASSERT(!mutex_is_locked(&chip->mutex));
> +	purelifi_usb_clear(&chip->usb);
> +	mutex_destroy(&chip->mutex);
> +	PURELIFI_MEMCLEAR(chip, sizeof(*chip));

Get rid of the PURELIFI_MEMCLEAR() macro.  The weird thing about
PURELIFI_MEMCLEAR() is that sometimes it's a no-op.  It seems
unnecessary to memset() the struct here.

I'm not a fan of all these tiny functions.  It feels like I have to
jump around a lot to understand the code.  What does "clear" mean in
this context.  Probably "release" is a better name.

> +}
> +
> +static int scnprint_mac_oui(struct purelifi_chip *chip, char *buffer,
> +			    size_t size)
> +{
> +	u8 *addr = purelifi_mac_get_perm_addr(purelifi_chip_to_mac(chip));
> +
> +	return scnprintf(buffer, size, "%02x-%02x-%02x",
> +			addr[0], addr[1], addr[2]);
> +}
> +
> +/* Prints an identifier line, which will support debugging. */
> +static int scnprint_id(struct purelifi_chip *chip, char *buffer, size_t size)

This function name is too vague.  What ID is it printing?

> +{
> +	int i = 0;

The initialization is not required.  "i" means "iterator".  This should
be "cnt" instead.

> +
> +	i = scnprintf(buffer, size, "purelifi%s chip ", "");
> +	i += purelifi_usb_scnprint_id(&chip->usb, buffer + i, size - i);
> +	i += scnprintf(buffer + i, size - i, " ");
> +	i += scnprint_mac_oui(chip, buffer + i, size - i);
> +	i += scnprintf(buffer + i, size - i, " ");
> +	return i;

This is an example of how tiny functions obfuscate the code.  It should
be written like this:

static void print_whatever(struct purelifi_chip *chip)
{
	u8 *addr = purelifi_mac_get_perm_addr(purelifi_chip_to_mac(chip));
	struct usb_device *udev = interface_to_usbdev(chip->usb.intf);

	pr_info("purelifi chip 04hx:%04hx v%04hx %s %02x-%02x-%02x\n",
		le16_to_cpu(udev->descriptor.idVendor),
		le16_to_cpu(udev->descriptor.idProduct),
		get_bcd_device(udev),
		speed(udev->speed),
		addr[0], addr[1], addr[2]);
}

> +}
> +
> +static void print_id(struct purelifi_chip *chip)
> +{
> +	char buffer[80];
> +
> +	scnprint_id(chip, buffer, sizeof(buffer));
> +	buffer[sizeof(buffer) - 1] = 0;

snprintf() functions alway put a NUL terminator on the end of the string.

> +	pl_dev_info(purelifi_chip_dev(chip), "%s\n", buffer);
> +}
> +
> +/* MAC address: if custom mac addresses are to be used CR_MAC_ADDR_P1 and
> + *              CR_MAC_ADDR_P2 must be overwritten
> + */
> +int purelifi_write_mac_addr(struct purelifi_chip *chip, const u8 *mac_addr)
> +{
> +	int r;
> +
> +	r = usb_write_req(mac_addr, ETH_ALEN, USB_REQ_MAC_WR);
> +	return r;

Delete the "r" variable.

	return usb_write_req(mac_addr, ETH_ALEN, USB_REQ_MAC_WR);

Again, I'm not a huge fan of one line functions for no reason. Actually,
the function is never called.  Just delete it.

> +}
> +
> +int purelifi_set_beacon_interval(struct purelifi_chip *chip, u16 interval,
> +				 u8 dtim_period, int type)
> +{
> +	int r;
> +
> +	if (!interval || (chip->beacon_set &&
> +			  chip->beacon_interval == interval)) {
> +		return 0;
> +	}
> +
> +	chip->beacon_interval = interval;
> +	chip->beacon_set = true;
> +	r = usb_write_req((const u8 *)&chip->beacon_interval,
> +			  sizeof(chip->beacon_interval),
> +			  USB_REQ_BEACON_INTERVAL_WR);
> +	return r;

Delete the "r" variable.

> +}
> +
> +static int hw_init(struct purelifi_chip *chip)
> +{
> +	return purelifi_set_beacon_interval(chip, 100, 0, 0);
> +}

This is a oneline function which is only called once.  Move it inline.

> +
> +int purelifi_chip_init_hw(struct purelifi_chip *chip)
> +{
> +	int r;
> +
> +	r = hw_init(chip);
> +	if (r)
> +		goto out;

Just return directly.  The little bunny hop doesn't add anything.

> +
> +	print_id(chip);
> +out:
> +	return r;
> +}

Anyway, those are some ideas.

regards,
dan carpenter