Re: [PATCHv2 2/3] EDAC: ti: add support for TI keystone and DRA7xx EDAC

[Tero Kristo <t-kristo@ti.com>]

On 11/11/17 12:46, Borislav Petkov wrote:
> On Fri, Nov 10, 2017 at 10:25:37AM +0200, Tero Kristo wrote:
>> TI Keystone and DRA7xx SoCs have support for EDAC on DDR3 memory that can
>> correct one bit errors and detect two bit errors. Add EDAC driver for this
>> feature which plugs into the generic kernel EDAC framework.
> 
> ...
> 
>> +static int ti_edac_probe(struct platform_device *pdev)
>> +{
>> +	int error_irq = 0, ret = -ENODEV;
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res;
>> +	void __iomem *reg;
>> +	struct mem_ctl_info *mci;
>> +	struct edac_mc_layer layers[1];
>> +	const struct of_device_id *id;
>> +	struct ti_edac *edac;
>> +	int my_id;
>> +
>> +	id = of_match_device(ti_edac_of_match, &pdev->dev);
>> +	if (!id)
>> +		return -ENODEV;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	reg = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(reg)) {
>> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
>> +			    "EMIF controller regs not defined\n");
>> +		return PTR_ERR(reg);
>> +	}
>> +
>> +	layers[0].type = EDAC_MC_LAYER_ALL_MEM;
>> +	layers[0].size = 1;
>> +
>> +	/* Allocate ID number for our EMIF controller */
>> +	my_id = atomic_inc_return(&edac_id) - 1;
> 
> You can do ATOMIC_INIT(-1) and then you don't need that silly - 1.
> 
> But there's something else wrong with this ID generation:
> 
>> +
>> +	mci = edac_mc_alloc(my_id, 1, layers, sizeof(*edac));
>> +	if (!mci)
>> +		return -ENOMEM;
> 
> <--- if you return here due to error, the next one which succeeds will
> get the next number and you'd have a hole in the numbering and wonder
> where did instance 0 go.

Yeah, that definitely can happen.

> 
> So I'm wondering if instead you could derive the ID from some controller
> registers which are specific to the instance. Or some hardware detail
> which is instance-specific.
> 
> In my driver, for example, the 0th instance has PCI device functions
> 00:18.x which is node 0, then node 1 has 00:19.x and so on, and this is
> a stable numbering. If you could do that for your hw you'll be able to
> pinpoint which instance and which DIMMs we're talking about reliably.

The only reliable way to determine the instance is to sort them by 
physical address space they occupy. This will require me to lookup all 
nodes that have same compatible string, and check their regs. This 
should probably be okay seeing we have only two EMIF instances at most.

I'll check how to do this most reliably.


> 
>> +
>> +	mci->pdev = &pdev->dev;
>> +	edac = mci->pvt_info;
>> +	edac->reg = reg;
>> +	platform_set_drvdata(pdev, mci);
>> +
>> +	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
>> +	mci->edac_ctl_cap = EDAC_FLAG_SECDED | EDAC_FLAG_NONE;
>> +	mci->mod_name = EDAC_MOD_NAME;
>> +	mci->ctl_name = id->compatible;
>> +	mci->dev_name = dev_name(&pdev->dev);
>> +
>> +	/* Setup memory layout */
>> +	ti_edac_setup_dimm(mci, (u32)(id->data));
>> +
>> +	ret = edac_mc_add_mc(mci);
> 
> Do that...
> 
>> +	if (ret) {
>> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
>> +			    "Failed to register mci: %d.\n", ret);
>> +		goto err;
>> +	}
>> +
>> +	/* add EMIF ECC error handler */
>> +	error_irq = platform_get_irq(pdev, 0);
>> +	if (!error_irq) {
>> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
>> +			    "EMIF irq number not defined.\n");
>> +		goto err2;
>> +	}
>> +
>> +	ret = devm_request_irq(dev, error_irq, ti_edac_isr, 0,
>> +			       "emif-edac-irq", mci);
>> +	if (ret) {
>> +		edac_printk(KERN_ERR, EDAC_MOD_NAME,
>> +			    "request_irq fail for EMIF EDAC irq\n");
>> +		goto err2;
>> +	}
> 
> ... here, after you've requested IRQs successfully and you can save
> yourself the err2 error path.

Ok.

> 
> Thx.
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki