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

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@...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>
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
>


--
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/