linux-kernel - Re: [PATCH] Packet hub driver of Topcliff PCH

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:	Wed, 30 Jun 2010 23:58:11 -0700
From:	Andy Isaacson <adi@...apodia.org>
To:	Masayuki Ohtak <masa-korg@....okisemi.com>
Cc:	Randy Dunlap <randy.dunlap@...cle.com>,
	Arnd Bergmann <arnd@...db.de>,
	"Wang, Yong Y" <yong.y.wang@...el.com>, qi.wang@...el.com,
	joel.clark@...el.com, andrew.chih.howe.khor@...el.com,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Packet hub driver of Topcliff PCH

On Thu, Jul 01, 2010 at 02:14:11PM +0900, Masayuki Ohtak wrote:
> Hi Andy and Randy
> 
> I have modified for your comments.
> Please confirm below.

Your style is looking better, and the additional documentation is much
appreciated.

I still have concerns about the userland interface design.  It still
seems to me that the MAC interface should be used by something in
drivers/net and there's no reason to expose the SROM in /dev unless it
contains more than just the MAC address.


A few more style issues I saw on a quick scan through -- this was not a
comprehensive review:


> +static long pch_phub_ioctl(struct file *file, unsigned int cmd,
> +			   unsigned long arg)
[snip]
> +		ret_value = copy_to_user(varg,
> +					 mac_addr, sizeof(mac_addr));

Reformat this to fit on one line:
+		ret_value = copy_to_user(varg, mac_addr, sizeof(mac_addr));


[snip]
> +	case IOCTL_PHUB_WRITE_MAC_ADDR:
> +		ret_value = copy_from_user(mac_addr, varg, sizeof(mac_addr));
> +
> +		if (ret_value)

Here we need to break:

+               {
> +			ret_value = -EFAULT;
+                       break;
+               }

because if copy_from_user failed ...

> +		for (i = 0; i < 6; i++)
> +			pch_phub_write_gbe_mac_addr(i, mac_addr[i]);

... we would pass garbage to pch_phub_write_gbe_mac_addr.

[snip]
> +		dev_dbg(&pdev->dev,
> +				"\npch_phub_probe : pci_enable_device FAILED");

Prefix \n is not correct.  This should be

+		dev_dbg(&pdev->dev, "pch_phub_probe: pci_enable_device FAILED");

In general dev_dbg format strings should fit on one 80-char line.  If
your format strings are longer than that, it's a clue you're doing
something wrong.  For example:

> +	dev_dbg(&pdev->dev, "pch_phub_probe : "
> +			"pci_enable_device returns %d\n", ret);

+	dev_dbg(&pdev->dev, "pch_phub_probe: pci_enable_device returns %d\n",
+		 ret);

> +	ret = pci_request_regions(pdev, MODULE_NAME);
> +	if (ret) {
> +		dev_dbg(&pdev->dev,
> +				"pch_phub_probe : pci_request_regions FAILED");

If you have a dev_dbg, please print ret.  It may give an important clue.

[snip a bunch more I don't have time to review right now]

Thanks,
-andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/