Re: [PATCH] Packet hub driver of Topcliff PCH

[Andy Isaacson <adi@hexapodia.org>]

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