Re: [PATCH] Packet hub driver of Topcliff PCH

Andy Isaacson
2010-07-01T06:58:13Z

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