linux-kernel - Re: [PATCH] Topcliff PHUB: Generate PacketHub driver

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]

Message-ID: <20100630182829.GJ29166@hexapodia.org>
Date:	Wed, 30 Jun 2010 11:28:29 -0700
From:	Andy Isaacson <adi@...apodia.org>
To:	Masayuki Ohtake <masa-korg@....okisemi.com>
Cc:	Arnd Bergmann <arnd@...db.de>,
	"Wang, Yong Y" <yong.y.wang@...el.com>,
	Wang Qi <qi.wang@...el.com>, Intel OTC <joel.clark@...el.com>,
	Andrew <andrew.chih.howe.khor@...el.com>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Topcliff PHUB: Generate PacketHub driver

On Wed, Jun 30, 2010 at 02:58:25PM +0900, Masayuki Ohtake wrote:
> > > + unsigned int i;
> > > + void __iomem *p = pch_phub_reg.pch_phub_base_address;
> > > +
> > > + dev_dbg(&pdev->dev, "pch_phub_restore_reg_conf ENTRY\n");
> > > + /* to store contents of PHUB_ID register */
> > > + iowrite32(pch_phub_reg.phub_id_reg, p + PCH_PHUB_PHUB_ID_REG);
> >
> > Don't include comments that just duplicate the code.  Also, rename your
> > constants from PCH_PHUB_PHUB_ to, I dunno, PHUB_ or something.
> 
> Sorry, I can't understand your intention.
> Please give us more information.

My mistake, I merged two comments into one paragraph, let me clarify.

1. When writing comments, do not write comments that duplicate the code.
Instead of writing
	/* store PHUB_ID */
	iowrite32(..._PHUB_ID_REG);
	/* store PHUB_FOO */
	iowrite32(..._PHUB_FOO_REG);
you should delete the line-by-line comments and just write
	iowrite32(..._PHUB_ID_REG);
	iowrite32(..._PHUB_FOO_REG);

2. your register names are very long.  Since the #define names are
private to this driver, there's no need to make them extremely
descriptive.  Instead of naming your registers 
PCH_PHUB_PHUB_ID_REG, you should change the names to be shorter, like
PHUB_ID_REG or PCH_ID_REG.  This will make your source code much more
readable by reducing linewrapping.

> > I seriously doubt that your device is special enough to warrant a custom
> > /dev node with proprietary semantics.  If this is just part of an
> > Ethernet driver, please implement it in drivers/net/; if this is a
> > generic PROM accessor, there must be some semi-standardized EPROM access
> > interface but I don't know what it is offhand.
>
> Since SROM is not in GbE HW but Phub HW, Phub is not part of Ethernet driver.
> Packet hub is not generic driver but special device.

It sounds like PHUB is a system-level device which provides access to a
SROM which contains GbE configuration data.  If that is correct, then I
have two comments:

1.  There are many other systems with similar configurations -- MIPS
SiByte, Alpha SRM, SPARC OpenFirmware, and some ARM systems, just to
name a few.  None of them expose the SROM as a custom /dev node AFAIK.
Is there a shared infrastructure that you can implement?

2. How does your GbE driver get the MAC address from the SPROM?  If
there is an in-kernel user of the PHUB interface, it might be much
easier to understand the design.

Thanks for responding to my review so quickly!
-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/