netdev - Re: [PATCH 1/1] Add support for GPIOs for SMSC LAN95xx chips.

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: <20140513.180121.1755290421111591028.davem@davemloft.net>
Date:	Tue, 13 May 2014 18:01:21 -0400 (EDT)
From:	David Miller <davem@...emloft.net>
To:	boger@...tactless.ru
Cc:	steve.glendinning@...well.net, netdev@...r.kernel.org,
	linux-usb@...r.kernel.org
Subject: Re: [PATCH 1/1] Add support for GPIOs for SMSC LAN95xx chips.

From: Evgeny Boger <boger@...tactless.ru>
Date: Sun, 11 May 2014 17:34:14 +0400

> There might be 11 GPIOs in total. Last three GPIOs  (offsets 8-11, 0-based) are shared
> with FDX, LNKA, SPD LEDs respectively. The LEDs are driven by chip by default
> at startup time. Once the corresponding GPIO is requested, the chip LED drive
> logic is disabled.
> 
> Based on commented out code presumably from SMSC original driver.
> https://github.com/olerem/barebox/blob/master/drivers/net/usb/smsc95xx.c
> 
> Signed-off-by: Evgeny Boger <boger@...tactless.ru>

Won't you need to add some GPIOLIB logic to the Kconfig entry for this
driver?

Also:

> +}
> +
> +
> +static int smsc95xx_gpio_request(struct gpio_chip *gpio, unsigned offset)

One empty line is sufficient between functions, please delete on of these
two.

> +{
> +	int ret = -1;
> +	u32 val, reg;
> +	int type_shift;
> +
> +	struct smsc95xx_priv *pdata =
> +			container_of(gpio, struct smsc95xx_priv, gpio);

Please do not put empty lines in the middle of local function
variable declarations.

> +	int ret = -1;
> +	u32 val, reg;
> +	int type_shift;
> +
> +	struct smsc95xx_priv *pdata =
> +			container_of(gpio, struct smsc95xx_priv, gpio);

Likewise.

> +	int ret = -1;
> +	u32 val, reg;
> +
> +	struct smsc95xx_priv *pdata =
> +			container_of(gpio, struct smsc95xx_priv, gpio);
> +	reg = smsc95xx_gpio_get_register(offset);

Likewise.

> +	int ret = -1;
> +	u32 val, reg;
> +
> +	struct smsc95xx_priv *pdata =
> +			container_of(gpio, struct smsc95xx_priv, gpio);

Likewise.


> +	int ret = -1;
> +	u32 val, reg;
> +
> +	struct smsc95xx_priv *pdata =
> +			container_of(gpio, struct smsc95xx_priv, gpio);
> +	reg = smsc95xx_gpio_get_register(offset);

Likewise and do put an empty line after the variable declarations
and before the first real C statement.

> @@ -1153,17 +1404,32 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
>  	dev->net->flags |= IFF_MULTICAST;
>  	dev->net->hard_header_len += SMSC95XX_TX_OVERHEAD_CSUM;
>  	dev->hard_mtu = dev->net->mtu + dev->net->hard_header_len;
> +
> +	ret = smsc95xx_register_gpio(dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	return 0;
>  }

I think you will leak dev->data[0] if you exit here with an error and
without explicitly freeing dev->data[0] like the unbind method does.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html