Re: [PATCH 1/1] edac: new ppc4xx driver module

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 30 Jan 2009 09:54:42 -0700
dougthompson@xmission.com wrote:

> From: Grant Erickson <gerickson@nuovations.com>

Perhaps a powerpc mailing list should have been cc'ed?

> This adds support for an EDAC memory controller adaptation driver for
> the "ibm,sdram-4xx-ddr2" ECC controller realized in the AMCC PowerPC
> 405EX[r].
> 
> At present, this driver has been developed and tested against the
> controller realization in the AMCC PPC405EX[r] on the AMCC Kilauea and
> Haleakala boards (256 MiB w/o ECC memory soldered onto the board) and
> a proprietary board based on those designs (128 MiB ECC memory, also
> soldered onto the board).
> 
> In the future, dynamic feature detection and handling needs to be
> added for the other realizations of this controller found in the
> 440SP, 440SPe, 460EX, 460GT and 460SX.
> 
> Eventually, this driver will likely be evolved and adapted to the
> above variant realizations of this controller as well as broken apart
> to handle the other known ECC-capable controllers prevalent in other
> PPC4xx processors:
> 
>   - IBM SDRAM (405GP, 405CR and 405EP) "ibm,sdram-4xx"
>   - IBM DDR1 (440GP, 440GX, 440EP and 440GR) "ibm,sdram-4xx-ddr"
>   - Denali DDR1/DDR2 (440EPX and 440GRX) "denali,sdram-4xx-ddr2"
> 
> Patch v2 removes the 'type' field from the of_device_id match
> structure to reflect the corresponding removal of the 'device_type'
> field from the device tree node.
> 
> Patch v3 incorporates feedback from Josh Boyer regarding compliance of
> switch statements with Linux coding style, removing gratuitous printk
> wrappers, and using the DCR mapping from the device tree rather than
> using a fixed mapping.
> 
> Patch v4 resync's the patch against the top-of-tree as 'git apply' was
> reporting "fatal: new file drivers/edac/ppc4xx_edac.h depends on old
> contents".
> 
>
> ...
>
> +/**
> + * This routine writes the provided data to the controller's specified
> + * indirect DCR register.
> + *
> + * @param dcr_host A pointer to the DCR mapping.
> + * @param idcr_n The indirect DCR register to write.
> + * @param value The data to write.
> + */

These comments try really hard to be in kerneldoc form, but don't quite
succeed.

The first line should be of the form

 * mtsdram - write the provided data to the controller's specified indirect DCR register.

(note that it has to all be on a single line, too).

>
> ...
>
> +/**
> + * This routine generates to the provided buffer the portion of the
> + * driver-unique report message associated with the ECCESS[BKNER]
> + * field of the specified ECC status.
> + *
> + * @param mci A pointer to the EDAC memory controller instance
> + * associated with the bank message being generated.
> + * @param status A pointer to the ECC status structure to generate the
> + * message from.
> + * @param buffer A pointer to the buffer in which to generate the
> + * message.
> + * @param size The size, in bytes, of space available in buffer.
> + *
> + * @return The number of characters generated on success; otherwise, <
> + * 0 on error.

And I don't think that kerneldoc understands `@return'?  It should :(


> + */
> +static int
> +ppc4xx_edac_generate_bank_message(const struct mem_ctl_info *mci,
> +				  const struct ppc4xx_ecc_status *status,
> +				  char *buffer,
> +				  size_t size)
> +{
> +	int n, total = 0;
> +	size_t row, rows;

It seems strange to use a size_t here.

> +	n = snprintf(buffer, size, "%s: Banks: ", mci->dev_name);
> +
> +	if (n < 0 || n >= size)
> +		goto fail;
> +
> +	buffer += n;
> +	size -= n;
> +	total += n;
> +
> +	for (rows = 0, row = 0; row < mci->nr_csrows; row++)
> +		if (ppc4xx_edac_check_bank_error(status, row)) {
> +				n = snprintf(buffer, size,
> +					 "%s%d",
> +					 (rows++ ? ", " : ""), row);

You can't print a size_t with %d.  It should use %z.

> +			if (n < 0 || n >= size)
> +				goto fail;
> +
> +			buffer += n;
> +			size -= n;
> +			total += n;
> +		}
> +
> +	n = snprintf(buffer, size, "%s; ", rows ? "" : "None");
> +
> +	if (n < 0 || n >= size)
> +		goto fail;
> +
> +	buffer += n;
> +	size -= n;
> +	total += n;
> +
> + fail:
> +	return total;
> +}
> +
>
> ...
>
> +static int
> +ppc4xx_edac_generate_lane_message(const struct mem_ctl_info *mci,
> +				  const struct ppc4xx_ecc_status *status,
> +				  char *buffer,
> +				  size_t size)
> +{
> +	int n, total = 0;
> +	size_t lane, lanes;
> +	const unsigned int first_lane = 0;
> +	const unsigned int lane_count = 16;
> +
> +	n = snprintf(buffer, size, "; Byte Lane Errors: ");
> +
> +	if (n < 0 || n >= size)
> +		goto fail;
> +
> +	buffer += n;
> +	size -= n;
> +	total += n;
> +
> +	for (lanes = 0, lane = first_lane; lane < lane_count; lane++) {
> +		if ((status->ecces & SDRAM_ECCES_BNCE_ENCODE(lane)) != 0) {
> +			n = snprintf(buffer, size,
> +				     "%s%d",
> +				     (lanes++ ? ", " : ""), lane);

Various dittoes.

> +			if (n < 0 || n >= size)
> +				goto fail;
> +
> +			buffer += n;
> +			size -= n;
> +			total += n;
> +		}
> +	}
> +
> +	n = snprintf(buffer, size, "%s; ", lanes ? "" : "None");
> +
> +	if (n < 0 || n >= size)
> +		goto fail;
> +
> +	buffer += n;
> +	size -= n;
> +	total += n;
> +
> + fail:
> +	return total;
> +}
> +
>
> ...
>
> +static int
> +ppc4xx_edac_generate_plb_message(const struct mem_ctl_info *mci,
> +				 const struct ppc4xx_ecc_status *status,
> +				 char *buffer,
> +				 size_t size)
> +{
> +	unsigned int master;
> +	bool read;
> +
> +	if ((status->besr & SDRAM_BESR_MASK) == 0)
> +		return 0;
> +
> +	if ((status->besr & SDRAM_BESR_M0ET_MASK) == SDRAM_BESR_M0ET_NONE)
> +		return 0;
> +
> +	read = ((status->besr & SDRAM_BESR_M0RW_MASK) == SDRAM_BESR_M0RW_READ);
> +
> +	master = SDRAM_BESR_M0ID_DECODE(status->besr);
> +
> +	return snprintf(buffer, size,
> +			"%s error w/ PLB master %u \"%s\"; ",

more

> +			(read ? "Read" : "Write"),
> +			master,
> +			(((master >= SDRAM_PLB_M0ID_FIRST) &&
> +			  (master <= SDRAM_PLB_M0ID_LAST)) ?
> +			 ppc4xx_plb_masters[master] : "UNKNOWN"));
> +}
> +
>
> ...
>
> +/**
> + * This routine handles an ibm,sdram-4xx-ddr2 controller ECC
> + * correctable error. Per the aforementioned discussion, there's not
> + * enough status available to use the full EDAC correctable error
> + * interface, so we just pass driver-unique message to the "no info"
> + * interface.
> + *
> + * @param mci A pointer to the EDAC memory controller instance
> + * associated with the correctable error being handled and reported.
> + * @param status A pointer to the ECC status structure associated with
> + * the correctable error being handled and reported.
> + */
> +static void
> +ppc4xx_edac_handle_ce(struct mem_ctl_info *mci,
> +		      const struct ppc4xx_ecc_status *status)
> +{
> +	int row;
> +	char message[PPC4XX_EDAC_MESSAGE_SIZE];

256 bytes on the stack is getting a bit large.

> +	ppc4xx_edac_generate_message(mci, status, message, sizeof(message));
> +
> +	for (row = 0; row < mci->nr_csrows; row++)
> +		if (ppc4xx_edac_check_bank_error(status, row))
> +			edac_mc_handle_ce_no_info(mci, message);
> +}
> +
> +/**
> + * This routine handles an ibm,sdram-4xx-ddr2 controller ECC
> + * uncorrectable error.
> + *
> + * @param mci A pointer to the EDAC memory controller instance
> + * associated with the uncorrectable error being handled and reported.
> + * @param status A pointer to the ECC status structure associated with
> + * the uncorrectable error being handled and reported.
> + */
> +static void
> +ppc4xx_edac_handle_ue(struct mem_ctl_info *mci,
> +		      const struct ppc4xx_ecc_status *status)
> +{
> +	const u64 bear = ((u64)status->bearh << 32 | status->bearl);
> +	const unsigned long pfn = bear >> PAGE_SHIFT;

The term `pfn' has a specific meaining in-kernel, and I have a
suspicion that this variable doesn't match it.

> +	const unsigned long poff = bear & ~PAGE_MASK;
> +	int row;
> +	char message[PPC4XX_EDAC_MESSAGE_SIZE];

stack..

> +	ppc4xx_edac_generate_message(mci, status, message, sizeof(message));
> +
> +	for (row = 0; row < mci->nr_csrows; row++)
> +		if (ppc4xx_edac_check_bank_error(status, row))
> +			edac_mc_handle_ue(mci, pfn, poff, row, message);
> +}
> +
>
> ...
>
> +/**
> + * This routine implements the interrupt handler for both correctable
> + * and uncorrectable ECC errors for the ibm,sdram-4xx-ddr2
> + * controller. It simply calls through to the same routine used during
> + * polling to check, report and clear the ECC status.
> + *
> + * @param irq The virtual interrupt number being serviced.
> + * @param dev_id A pointer to the EDAC memory controller instance
> + * associated with the interrupt being handled.
> + *
> + * @return
> + */
> +static irqreturn_t
> +ppc4xx_edac_isr(int irq, void *dev_id)
> +{
> +	struct mem_ctl_info *mci = (struct mem_ctl_info *)dev_id;

unneeded and undesirable cast of void*.

> +	ppc4xx_edac_check(mci);
> +
> +	return IRQ_HANDLED;
> +}
> +
>
> ...
>
> +static int __devinit
> +ppc4xx_edac_mc_init(struct mem_ctl_info *mci,
> +		    struct of_device *op,
> +		    const struct of_device_id *match,
> +		    const dcr_host_t *dcr_host,
> +		    u32 mcopt1)
> +{
> +	int status = 0;
> +	const u32 memcheck = (mcopt1 & SDRAM_MCOPT1_MCHK_MASK);
> +	struct ppc4xx_edac_pdata *pdata = NULL;
> +	const struct device_node *np = op->node;
> +
> +	if (mci == NULL || op == NULL || match == NULL || dcr_host == NULL)
> +		return -EINVAL;

Can all of these really happen?

> +	/* Initial driver pointers and private data */
> +
> +	mci->dev		= &op->dev;
> +
> +	dev_set_drvdata(mci->dev, mci);
> +
> +	pdata			= mci->pvt_info;
> +
> +	pdata->dcr_host		= *dcr_host;
> +	pdata->irqs.sec		= NO_IRQ;
> +	pdata->irqs.ded		= NO_IRQ;
> +
> +	/* Initialize controller capabilities and configuration */
> +
> +	mci->mtype_cap		= (MEM_FLAG_DDR | MEM_FLAG_RDDR |
> +				   MEM_FLAG_DDR2 | MEM_FLAG_RDDR2);
> +
> +	mci->edac_ctl_cap	= (EDAC_FLAG_NONE |
> +				   EDAC_FLAG_EC |
> +				   EDAC_FLAG_SECDED);
> +
> +	mci->scrub_cap		= SCRUB_NONE;
> +	mci->scrub_mode		= SCRUB_NONE;
> +
>
> ...
>