Re: [PATCH] Packet hub driver of Topcliff PCH

["Masayuki Ohtake" <masa-korg@dsn.okisemi.com>]

Hi Andy,

> 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.
SROM has not only MAC address but also Option ROM data.
Thus, we think SROM should be exposed.

> A few more style issues I saw on a quick scan through -- this was not a
> comprehensive review:
We are now modifying.

Thanks, Ohtake

----- Original Message ----- 
From: "Andy Isaacson" <adi@hexapodia.org>
To: "Masayuki Ohtak" <masa-korg@dsn.okisemi.com>
Cc: "Randy Dunlap" <randy.dunlap@oracle.com>; "Arnd Bergmann" <arnd@arndb.de>; "Wang, Yong Y" <yong.y.wang@intel.com>;
<qi.wang@intel.com>; <joel.clark@intel.com>; <andrew.chih.howe.khor@intel.com>; "Alan Cox" <alan@lxorguk.ukuu.org.uk>;
"LKML" <linux-kernel@vger.kernel.org>
Sent: Thursday, July 01, 2010 3:58 PM
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
>