linux-kernel - Re: [PATCH 3/3] EDAC: mv64x60: replace in_le32/out

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: <7c7bfec63342409ebd58913836a0739a@svr-chch-ex1.atlnz.lc>
Date:   Wed, 17 May 2017 21:16:47 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Borislav Petkov <bp@...en8.de>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>
CC:     "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] EDAC: mv64x60: replace in_le32/out_le32 with
 ioread32/iowrite32

On 18/05/17 06:18, Borislav Petkov wrote:
> Top-posting so that the PPC list can see the whole patch below.
>
> Since I don't know PPC, let me add PPC ML to CC for a confirmation this
> change is correct.
>
> Which brings me to the tangential: this driver is from 2006-ish and
> is for some "Marvell MV64x60 Memory Controller kernel module for PPC
> platforms".

Many of the peripherals from the PPC MV64x60 were brought through to the 
ARM Orion, Kirkwood, and Armada SoCs (e.g. mv643xx_eth.c, 
i2c-mv64xxx.c). I believe the memory controller ECC is also in the same 
category.

I haven't looked into the cache/pci controller aspects of the 
mv64x60_edac.c driver yet.

> If you're going to touch it, then you should test on the PPC
> hardware too, so that you don't break the driver there.

I don't disagree. I however don't have access to any such hardware (I've 
got 2 armada variant boards that have ECC). Maybe someone on the PPC 
list can help me out?

One thing I would like confirmation on is is in_le32 -> ioread32 the 
correct change? I tossed up between ioread32 and readl. Looking at 
mv643xx_eth.c which supports both the MV643XX and Orion it's using readl 
so perhaps I should be using that.

> Unless that hardware is obsolete now and we don't care and, and ..?

MV64x60 is pretty old. I considered gutting mv64x60_edac.c to make a 
separate driver but that would just be more code to maintain.

> Maybe someone has some insights...
>
> On Fri, May 12, 2017 at 04:20:02PM +1200, Chris Packham wrote:
>> To allow this driver to be used on non-powerpc platforms it needs to use
>> io accessors suitable for all platforms.
>>
>> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
>> ---
>>  drivers/edac/mv64x60_edac.c | 84 ++++++++++++++++++++++-----------------------
>>  1 file changed, 42 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/edac/mv64x60_edac.c b/drivers/edac/mv64x60_edac.c
>> index ddc5082f7577..102ec29f864b 100644
>> --- a/drivers/edac/mv64x60_edac.c
>> +++ b/drivers/edac/mv64x60_edac.c
>> @@ -32,21 +32,21 @@ static void mv64x60_pci_check(struct edac_pci_ctl_info *pci)
>>  	struct mv64x60_pci_pdata *pdata = pci->pvt_info;
>>  	u32 cause;
>>
>> -	cause = in_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE);
>> +	cause = ioread32(pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE);
>>  	if (!cause)
>>  		return;
>>
>>  	printk(KERN_ERR "Error in PCI %d Interface\n", pdata->pci_hose);
>>  	printk(KERN_ERR "Cause register: 0x%08x\n", cause);
>>  	printk(KERN_ERR "Address Low: 0x%08x\n",
>> -	       in_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_ADDR_LO));
>> +	       ioread32(pdata->pci_vbase + MV64X60_PCI_ERROR_ADDR_LO));
>>  	printk(KERN_ERR "Address High: 0x%08x\n",
>> -	       in_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_ADDR_HI));
>> +	       ioread32(pdata->pci_vbase + MV64X60_PCI_ERROR_ADDR_HI));
>>  	printk(KERN_ERR "Attribute: 0x%08x\n",
>> -	       in_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_ATTR));
>> +	       ioread32(pdata->pci_vbase + MV64X60_PCI_ERROR_ATTR));
>>  	printk(KERN_ERR "Command: 0x%08x\n",
>> -	       in_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_CMD));
>> -	out_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE, ~cause);
>> +	       ioread32(pdata->pci_vbase + MV64X60_PCI_ERROR_CMD));
>> +	iowrite32(~cause, pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE);
>>
>>  	if (cause & MV64X60_PCI_PE_MASK)
>>  		edac_pci_handle_pe(pci, pci->ctl_name);
>> @@ -61,7 +61,7 @@ static irqreturn_t mv64x60_pci_isr(int irq, void *dev_id)
>>  	struct mv64x60_pci_pdata *pdata = pci->pvt_info;
>>  	u32 val;
>>
>> -	val = in_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE);
>> +	val = ioread32(pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE);
>>  	if (!val)
>>  		return IRQ_NONE;
>>
>> @@ -93,7 +93,7 @@ static int __init mv64x60_pci_fixup(struct platform_device *pdev)
>>  	if (!pci_serr)
>>  		return -ENOMEM;
>>
>> -	out_le32(pci_serr, in_le32(pci_serr) & ~0x1);
>> +	iowrite32(ioread32(pci_serr) & ~0x1, pci_serr);
>>  	iounmap(pci_serr);
>>
>>  	return 0;
>> @@ -161,10 +161,10 @@ static int mv64x60_pci_err_probe(struct platform_device *pdev)
>>  		goto err;
>>  	}
>>
>> -	out_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE, 0);
>> -	out_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_MASK, 0);
>> -	out_le32(pdata->pci_vbase + MV64X60_PCI_ERROR_MASK,
>> -		 MV64X60_PCIx_ERR_MASK_VAL);
>> +	iowrite32(0, pdata->pci_vbase + MV64X60_PCI_ERROR_CAUSE);
>> +	iowrite32(0, pdata->pci_vbase + MV64X60_PCI_ERROR_MASK);
>> +	iowrite32(MV64X60_PCIx_ERR_MASK_VAL,
>> +		  pdata->pci_vbase + MV64X60_PCI_ERROR_MASK);
>>
>>  	if (edac_pci_add_device(pci, pdata->edac_idx) > 0) {
>>  		edac_dbg(3, "failed edac_pci_add_device()\n");
>> @@ -233,23 +233,23 @@ static void mv64x60_sram_check(struct edac_device_ctl_info *edac_dev)
>>  	struct mv64x60_sram_pdata *pdata = edac_dev->pvt_info;
>>  	u32 cause;
>>
>> -	cause = in_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE);
>> +	cause = ioread32(pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE);
>>  	if (!cause)
>>  		return;
>>
>>  	printk(KERN_ERR "Error in internal SRAM\n");
>>  	printk(KERN_ERR "Cause register: 0x%08x\n", cause);
>>  	printk(KERN_ERR "Address Low: 0x%08x\n",
>> -	       in_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_ADDR_LO));
>> +	       ioread32(pdata->sram_vbase + MV64X60_SRAM_ERR_ADDR_LO));
>>  	printk(KERN_ERR "Address High: 0x%08x\n",
>> -	       in_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_ADDR_HI));
>> +	       ioread32(pdata->sram_vbase + MV64X60_SRAM_ERR_ADDR_HI));
>>  	printk(KERN_ERR "Data Low: 0x%08x\n",
>> -	       in_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_DATA_LO));
>> +	       ioread32(pdata->sram_vbase + MV64X60_SRAM_ERR_DATA_LO));
>>  	printk(KERN_ERR "Data High: 0x%08x\n",
>> -	       in_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_DATA_HI));
>> +	       ioread32(pdata->sram_vbase + MV64X60_SRAM_ERR_DATA_HI));
>>  	printk(KERN_ERR "Parity: 0x%08x\n",
>> -	       in_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_PARITY));
>> -	out_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE, 0);
>> +	       ioread32(pdata->sram_vbase + MV64X60_SRAM_ERR_PARITY));
>> +	iowrite32(0, pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE);
>>
>>  	edac_device_handle_ue(edac_dev, 0, 0, edac_dev->ctl_name);
>>  }
>> @@ -260,7 +260,7 @@ static irqreturn_t mv64x60_sram_isr(int irq, void *dev_id)
>>  	struct mv64x60_sram_pdata *pdata = edac_dev->pvt_info;
>>  	u32 cause;
>>
>> -	cause = in_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE);
>> +	cause = ioread32(pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE);
>>  	if (!cause)
>>  		return IRQ_NONE;
>>
>> @@ -322,7 +322,7 @@ static int mv64x60_sram_err_probe(struct platform_device *pdev)
>>  	}
>>
>>  	/* setup SRAM err registers */
>> -	out_le32(pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE, 0);
>> +	iowrite32(0, pdata->sram_vbase + MV64X60_SRAM_ERR_CAUSE);
>>
>>  	edac_dev->mod_name = EDAC_MOD_STR;
>>  	edac_dev->ctl_name = pdata->name;
>> @@ -398,7 +398,7 @@ static void mv64x60_cpu_check(struct edac_device_ctl_info *edac_dev)
>>  	struct mv64x60_cpu_pdata *pdata = edac_dev->pvt_info;
>>  	u32 cause;
>>
>> -	cause = in_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE) &
>> +	cause = ioread32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE) &
>>  	    MV64x60_CPU_CAUSE_MASK;
>>  	if (!cause)
>>  		return;
>> @@ -406,16 +406,16 @@ static void mv64x60_cpu_check(struct edac_device_ctl_info *edac_dev)
>>  	printk(KERN_ERR "Error on CPU interface\n");
>>  	printk(KERN_ERR "Cause register: 0x%08x\n", cause);
>>  	printk(KERN_ERR "Address Low: 0x%08x\n",
>> -	       in_le32(pdata->cpu_vbase[0] + MV64x60_CPU_ERR_ADDR_LO));
>> +	       ioread32(pdata->cpu_vbase[0] + MV64x60_CPU_ERR_ADDR_LO));
>>  	printk(KERN_ERR "Address High: 0x%08x\n",
>> -	       in_le32(pdata->cpu_vbase[0] + MV64x60_CPU_ERR_ADDR_HI));
>> +	       ioread32(pdata->cpu_vbase[0] + MV64x60_CPU_ERR_ADDR_HI));
>>  	printk(KERN_ERR "Data Low: 0x%08x\n",
>> -	       in_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_DATA_LO));
>> +	       ioread32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_DATA_LO));
>>  	printk(KERN_ERR "Data High: 0x%08x\n",
>> -	       in_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_DATA_HI));
>> +	       ioread32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_DATA_HI));
>>  	printk(KERN_ERR "Parity: 0x%08x\n",
>> -	       in_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_PARITY));
>> -	out_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE, 0);
>> +	       ioread32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_PARITY));
>> +	iowrite32(0, pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE);
>>
>>  	edac_device_handle_ue(edac_dev, 0, 0, edac_dev->ctl_name);
>>  }
>> @@ -426,7 +426,7 @@ static irqreturn_t mv64x60_cpu_isr(int irq, void *dev_id)
>>  	struct mv64x60_cpu_pdata *pdata = edac_dev->pvt_info;
>>  	u32 cause;
>>
>> -	cause = in_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE) &
>> +	cause = ioread32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE) &
>>  	    MV64x60_CPU_CAUSE_MASK;
>>  	if (!cause)
>>  		return IRQ_NONE;
>> @@ -515,9 +515,9 @@ static int mv64x60_cpu_err_probe(struct platform_device *pdev)
>>  	}
>>
>>  	/* setup CPU err registers */
>> -	out_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE, 0);
>> -	out_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_MASK, 0);
>> -	out_le32(pdata->cpu_vbase[1] + MV64x60_CPU_ERR_MASK, 0x000000ff);
>> +	iowrite32(0, pdata->cpu_vbase[1] + MV64x60_CPU_ERR_CAUSE);
>> +	iowrite32(0, pdata->cpu_vbase[1] + MV64x60_CPU_ERR_MASK);
>> +	iowrite32(0x000000ff, pdata->cpu_vbase[1] + MV64x60_CPU_ERR_MASK);
>>
>>  	edac_dev->mod_name = EDAC_MOD_STR;
>>  	edac_dev->ctl_name = pdata->name;
>> @@ -596,13 +596,13 @@ static void mv64x60_mc_check(struct mem_ctl_info *mci)
>>  	u32 comp_ecc;
>>  	u32 syndrome;
>>
>> -	reg = in_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR);
>> +	reg = ioread32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR);
>>  	if (!reg)
>>  		return;
>>
>>  	err_addr = reg & ~0x3;
>> -	sdram_ecc = in_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_RCVD);
>> -	comp_ecc = in_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_CALC);
>> +	sdram_ecc = ioread32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_RCVD);
>> +	comp_ecc = ioread32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_CALC);
>>  	syndrome = sdram_ecc ^ comp_ecc;
>>
>>  	/* first bit clear in ECC Err Reg, 1 bit error, correctable by HW */
>> @@ -620,7 +620,7 @@ static void mv64x60_mc_check(struct mem_ctl_info *mci)
>>  				     mci->ctl_name, "");
>>
>>  	/* clear the error */
>> -	out_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR, 0);
>> +	iowrite32(0, pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR);
>>  }
>>
>>  static irqreturn_t mv64x60_mc_isr(int irq, void *dev_id)
>> @@ -629,7 +629,7 @@ static irqreturn_t mv64x60_mc_isr(int irq, void *dev_id)
>>  	struct mv64x60_mc_pdata *pdata = mci->pvt_info;
>>  	u32 reg;
>>
>> -	reg = in_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR);
>> +	reg = ioread32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR);
>>  	if (!reg)
>>  		return IRQ_NONE;
>>
>> @@ -664,7 +664,7 @@ static void mv64x60_init_csrows(struct mem_ctl_info *mci,
>>
>>  	get_total_mem(pdata);
>>
>> -	ctl = in_le32(pdata->mc_vbase + MV64X60_SDRAM_CONFIG);
>> +	ctl = ioread32(pdata->mc_vbase + MV64X60_SDRAM_CONFIG);
>>
>>  	csrow = mci->csrows[0];
>>  	dimm = csrow->channels[0]->dimm;
>> @@ -753,7 +753,7 @@ static int mv64x60_mc_err_probe(struct platform_device *pdev)
>>  		goto err;
>>  	}
>>
>> -	ctl = in_le32(pdata->mc_vbase + MV64X60_SDRAM_CONFIG);
>> +	ctl = ioread32(pdata->mc_vbase + MV64X60_SDRAM_CONFIG);
>>  	if (!(ctl & MV64X60_SDRAM_ECC)) {
>>  		/* Non-ECC RAM? */
>>  		printk(KERN_WARNING "%s: No ECC DIMMs discovered\n", __func__);
>> @@ -779,10 +779,10 @@ static int mv64x60_mc_err_probe(struct platform_device *pdev)
>>  	mv64x60_init_csrows(mci, pdata);
>>
>>  	/* setup MC registers */
>> -	out_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR, 0);
>> -	ctl = in_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_CNTL);
>> +	iowrite32(0, pdata->mc_vbase + MV64X60_SDRAM_ERR_ADDR);
>> +	ctl = ioread32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_CNTL);
>>  	ctl = (ctl & 0xff00ffff) | 0x10000;
>> -	out_le32(pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_CNTL, ctl);
>> +	iowrite32(ctl, pdata->mc_vbase + MV64X60_SDRAM_ERR_ECC_CNTL);
>>
>>  	res = edac_mc_add_mc(mci);
>>  	if (res) {
>> --
>> 2.11.0.24.ge6920cf
>>
>