Re: [PATCH v5 2/2] EDAC: add EDAC driver for DMC520

[Lei Wang <leiwang_git@outlook.com>]

Thanks Borislav! I addressed most of your comments. There are a few that 
need your further feedback. Thanks!

>> +++ b/drivers/edac/dmc520_edac.c
>> @@ -0,0 +1,632 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* EDAC driver for DMC-520 */
> 
> That comment is kinda useless.
> 

Added some more comments for the file:

/*
  * dmc520_edac.c, EDAC driver for DMC-520 memory controller
  *
  * Authors:	Rui Zhao <ruizhao@microsoft.com>
  * 		Lei Wang <lewan@microsoft.com>
  */

> 
>> +{ \
>> +	struct mem_ctl_info *mci; \
>> +	struct dmc520_edac *edac; \
>> +	mci = data; \
>> +	edac = mci->pvt_info; \
>> +	return dmc520_edac_dram_all_isr(irq, mci, edac->interrupt_masks[index]); \
>> +}
>> +
>> +DECLARE_ISR(0)
>> +DECLARE_ISR(1)
>> +/* More DECLARE_ISR(index) can be added to support more interrupt lines. */
>> +
>> +irq_handler_t dmc520_isr_array[] = {
>> +	dmc520_isr_0,
>> +	dmc520_isr_1
>> +	/* More dmc520_isr_index can be added to support more interrupt lines. */
> 
> What are those comments really for?
> 

These comments tell how to potentially expand the driver functions to 
support more interrupts besides what are already here.

>> +
>> +	/* If in the future there are more supported interrupts in a different
>> +	 * platform, more condition statements can be added here for each
>> +	 * interrupt flag, together with its corresponding isr implementations.
>> +	 */
> 
> This comment above is useless.
> 
> Also, kernel comments style is:
> 
> 	/*
> 	 * A sentence ending with a full-stop.
> 	 * Another sentence. ...
> 	 * More sentences. ...
> 	 */
>

As above comments, this comment is to guide potential future adding to 
this driver to support other interrupts. I modified the comment format.

>> +
>> +	mci = edac_mc_alloc(dmc520_mc_idx++, ARRAY_SIZE(layers), layers,
>> +			    sizeof(struct dmc520_edac) + sizeof(u32) * nintr);
>> +	if (!mci) {
>> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
>> +			    "Failed to allocate memory for mc instance\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	edac = mci->pvt_info;
>> +	edac->reg_base = reg_base;
>> +	edac->nintr = nintr;
>> +	edac->interrupt_mask_all = 0;
>> +	spin_lock_init(&edac->ecc_lock);
> 
> Move that checking...
> 
>> +
>> +	ret = of_property_read_u32_array(dev->of_node, "interrupt-config",
>> +			edac->interrupt_masks, nintr);
>> +	if (ret) {
>> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
>> +			"Failed to get interrupt-config arrays.\n");
>> +		goto err_free_mc;
>> +	}
>> +
>> +	for (intr_index = 0; intr_index < nintr; ++intr_index) {
>> +		if (edac->interrupt_mask_all & edac->interrupt_masks[intr_index]) {
>> +			edac_printk(KERN_ERR, EDAC_MC,
>> +				"interrupt-config error: "
>> +				"element %d's interrupt mask %d has overlap.\n",
>> +				intr_index, edac->interrupt_masks[intr_index]);
>> +			goto err_free_mc;
>> +		}
>> +
>> +		edac->interrupt_mask_all |= edac->interrupt_masks[intr_index];
>> +	}
>> +
>> +	if ((edac->interrupt_mask_all | ALL_INT_MASK) != ALL_INT_MASK) {
>> +		edac_printk(KERN_WARNING, EDAC_MOD_NAME,
>> +			"interrupt-config warning: "
>> +			"interrupt mask (0x%x) is not supported by dmc520 (0x%lx).\n",
>> +			edac->interrupt_mask_all, ALL_INT_MASK);
>> +	}
>> +	edac->interrupt_mask_all &= ALL_INT_MASK;
>> +
>> +	platform_set_drvdata(pdev, mci);
> 
> ... up-to-here before the edac_mc_alloc() call so that you don't have to
> needlessly goto err_free_mc and free the memory you just allocated if
> any of those properties reading fails. IOW, do all your hw init first
> and then call edac_mc_alloc(), when you have everything needed.
> 

After edac_mc_alloc(), if succeeds, the above code updates dmc520_edac 
struct data. If moving edac_mc_alloc as suggested, I will need to use 
local variables to store the data, and then after edac_mc_alloc() update 
the dmc520_edac. By ordering this way, the dmc520_edac can be updated 
directly.

Planning not fix.

>> +
>> +	mci->pdev = dev;
>> +	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR4;
>> +	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
>> +	mci->edac_cap = EDAC_FLAG_SECDED;
>> +	mci->scrub_cap = SCRUB_FLAG_HW_SRC;
>> +	mci->scrub_mode = dmc520_get_scrub_type(edac);
>> +	mci->ctl_name = EDAC_CTL_NAME;
>> +	mci->dev_name = dev_name(mci->pdev);
>> +	mci->mod_name = EDAC_MOD_NAME;
>> +	mci->ctl_page_to_phys = NULL;
>> +
>> +	edac_op_state = EDAC_OPSTATE_INT;
>> +
>> +	dmc520_init_csrow(mci);
>> +
>> +	ret = edac_mc_add_mc(mci);
>> +	if (ret) {
>> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
>> +			"Failed to register with EDAC core\n");
>> +		goto err_free_mc;
>> +	}
>> +
>> +	/* Clear interrupts, not affecting other unrelated interrupts */
>> +	reg_val = dmc520_read_reg(edac, REG_OFFSET_INTERRUPT_CONTROL);
>> +	dmc520_write_reg(edac, reg_val & (~(edac->interrupt_mask_all)),
>> +			REG_OFFSET_INTERRUPT_CONTROL);
>> +	dmc520_write_reg(edac, edac->interrupt_mask_all,
>> +			REG_OFFSET_INTERRUPT_CLR);
>> +
>> +	for (intr_index = 0; intr_index < nintr; ++intr_index) {
>> +		int irq_id = platform_get_irq(pdev, intr_index);
>> +
>> +		if (irq_id < 0) {
>> +			edac_printk(KERN_ERR, EDAC_MC,
>> +				    "Failed to get irq #%d\n", intr_index);
>> +			ret = -ENODEV;
>> +			goto err_free_irq;
>> +		}
>> +
>> +		ret = devm_request_irq(&pdev->dev, irq_id,
>> +				dmc520_isr_array[intr_index], IRQF_SHARED,
>> +				dev_name(&pdev->dev), mci);
>> +		if (ret < 0) {
>> +			edac_printk(KERN_ERR, EDAC_MC,
>> +				    "Failed to request irq %d\n", irq_id);
>> +			goto err_free_irq;
>> +		}
>> +
>> +		++nintr_registered;
>> +	}
> 
> The same with this IRQ requesting and you'll save yourself the err_free_irq too.

Do you mean having an array to keep all the irq_id, and then only 
devm_request_irq on them if all of the platform_get_irq are success? Yes 
that was one option I was thinking too when writing this part. But the # 
of interrupts are not fixed and are read from the dt configuration and I 
didn't want to use a fixed MAX array size for it. Also it adds a local 
array to store the irq_id's. So eventually I favored this 
implementation: platform_get_irq one interrupt, and devm_request_irq one 
interrupt, in a loop.

Planning not fix.



Thanks again!

-Lei