Re: [PATCH RESEND] driver for i5400 MCH

[Mauro Carvalho Chehab]

On Tue, 2 Dec 2008 22:33:47 -0800
Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Mon, 1 Dec 2008 19:46:23 -0200 Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxx> wrote:
> 
> > EDAC driver for i5400 MCH (Seaburg)
> > 
> > This driver adds support for i5400 MCH chipset.
> 
> checkpatch has fun with this one.

Ok. I fixed the remaining errors. 

I'll just keep the 80 char warns on lines with comments explaining each field, like:

WARNING: line over 80 characters
#200: FILE: drivers/edac/i5400_edac.c:151:
+	EMASK_M9  = 1<<8,  /* Non-Aliased Uncorrectable Non-Mirrored Demand Data ECC */

Where breaking the line wouldn't be that fun.

> > Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> 
> Both you and Ben Woodard are listed under MODULE_AUTHOR.  Should this
> patch have Ben's Signed-off-by:?  And/or his From:?

I'll ask his SOB.

> > +#ifndef PCI_DEVICE_ID_INTEL_5400_ERR
> 
> This ifndef is unneeded: PCI_DEVICE_ID_INTEL_5400_ERR _is_ defined.
> 
> What's more, it is actually bad, because if pci_ids.h were to #define
> PCI_DEVICE_ID_INTEL_5400_ERR to something other than 0x4030, the ifndef
> would prevent the compiler from informing us of this potential problem.

Fixed.
 
> > +char *error_name[] = {
> 
> error_name is a global symbol.

Changed. Also changed it to const.

> > +/* masks for non-fatal error register */
> > +#define TO_NF_MASK(a)		(((a) & EMASK_M29) | ((a) >> 3))
> > +#define FROM_NF_FERR(a)		(((a) & EMASK_M29) | (((a) << 3) & ((1 << 30)-1)))
> 
> These macros will misbehave if passed an expression with side-effects. 
> Not really a problem as they're only used for constructing constants. 
> I guess if one was being paranoid, one could #undef them again as soon
> as possible.

Good point. I've converted both macros into inline functions, since gcc will likely optimize
the code into a constant, instead of calculating at runtime.

> > +#ifdef CONFIG_EDAC_DEBUG
> > +/* MTR NUMROW */
> > +static char *numrow_toString[] = {
> > +	"8,192 - 13 rows",
> > +	"16,384 - 14 rows",
> > +	"32,768 - 15 rows",
> > +	"65,536 - 16 rows"
> > +};
> > +
> > +/* MTR NUMCOL */
> > +static char *numcol_toString[] = {
> > +	"1,024 - 10 columns",
> > +	"2,048 - 11 columns",
> > +	"4,096 - 12 columns",
> > +	"reserved"
> > +};
> 
> These could possibly be made const.  We don't modify them at runtime...

Fixed.

> 202 bytes.  Tolerable, I guess.
> 
> Heaven knows why and how this dimension was chosen, and under which
> circumstances it will overflow :(
> 
> It looks like such a hack that perhaps you should give up and use
> kasprintf()+kfree()?

I also didn't like that code. It is present also on other edac drivers.

I'll fix this on a later patch. I'll probably propose some patches to the other
edac drivers that use this kind of construction.

> > +	/* Decrement usage count for devices */
> > +	if (pvt->branch_1)
> > +		pci_dev_put(pvt->branch_1);
> > +
> > +	if (pvt->branch_0)
> > +		pci_dev_put(pvt->branch_0);
> > +
> > +	if (pvt->fsb_error_regs)
> > +		pci_dev_put(pvt->fsb_error_regs);
> > +
> > +	if (pvt->branchmap_werrors)
> > +		pci_dev_put(pvt->branchmap_werrors);
> 
> pci_dev_put(NULL) is legal.

Fixed.

> > +	mci->ctl_name = i5400_devs[dev_idx].ctl_name;
> 
> This is a bit strange.  i5400_devs[] has a single entry, so what's the
> point in indexing it with `dev_idx', which MUST have a value of zero
> anyway?

I'm not sure if we covered all PCI ID's for i5400 (maybe some i5400 variant may
be released?). That's why I preferred to keep a table: makes easier to add
other entries there, but I see your point.

I'll add a code to prevent dev_idx values equal or higher than
ARRAY_SIZE, to avoid the risk of trying to acess something outside the buffer.

> > +static struct pci_driver i5400_driver = {
> > +	.name = KBUILD_BASENAME,
> > +	.probe = i5400_init_one,
> > +	.remove = __devexit_p(i5400_remove_one),
> > +	.id_table = i5400_pci_tbl,
> > +};
> 
> Only one other driver in the tree uses KBUILD_BASENAME, and that is
> drivers/edac/i5100_edac.c, from which I suspect the above was copied.
> 
> I suspect that something is being done wrongly here.

Fixed.

I'll run some tests here, and submit a version 2 of the patch soon.

Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/