| 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
| ||
|
Message-ID:
<SA1PR12MB89471D2D39E1C10624BAB3B1815DA@SA1PR12MB8947.namprd12.prod.outlook.com>
Date: Mon, 21 Jul 2025 04:08:17 +0000
From: "Datta, Shubhrajyoti" <shubhrajyoti.datta@....com>
To: Borislav Petkov <bp@...en8.de>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>, "git (AMD-Xilinx)"
<git@....com>, Krzysztof Kozlowski <krzk@...nel.org>, Rob Herring
<robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Tony Luck
<tony.luck@...el.com>, James Morse <james.morse@....com>, Mauro Carvalho
Chehab <mchehab@...nel.org>, Robert Richter <rric@...nel.org>, "Gupta, Nipun"
<Nipun.Gupta@....com>, "Agarwal, Nikhil" <nikhil.agarwal@....com>
Subject: RE: [PATCH v7 5/5] EDAC/VersalNET: Add support for error notification
[AMD Official Use Only - AMD Internal Distribution Only]
> -----Original Message-----
> From: Borislav Petkov <bp@...en8.de>
> Sent: Thursday, July 3, 2025 11:01 PM
> To: Datta, Shubhrajyoti <shubhrajyoti.datta@....com>
> Cc: linux-kernel@...r.kernel.org; devicetree@...r.kernel.org; linux-
> edac@...r.kernel.org; git (AMD-Xilinx) <git@....com>; Krzysztof Kozlowski
> <krzk@...nel.org>; Rob Herring <robh@...nel.org>; Conor Dooley
> <conor+dt@...nel.org>; Tony Luck <tony.luck@...el.com>; James Morse
> <james.morse@....com>; Mauro Carvalho Chehab <mchehab@...nel.org>;
> Robert Richter <rric@...nel.org>; Gupta, Nipun <Nipun.Gupta@....com>;
> Agarwal, Nikhil <nikhil.agarwal@....com>
> Subject: Re: [PATCH v7 5/5] EDAC/VersalNET: Add support for error notification
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Thu, May 29, 2025 at 12:30:17PM +0530, Shubhrajyoti Datta wrote:
> > Hardware errors can corrupt the RAM or other system components,
> > without detection it can lead to crashes and system failure, this
> > driver helps in reporting of errors to user space, triggering corrective actions.
>
> Kinda useless paragraph.
>
> > drivers/edac/Kconfig | 11 +
> > drivers/edac/Makefile | 1 +
> > drivers/edac/versalnet_edac.c | 1108 +++++++++++++++++++++++++++++
> > include/linux/cdx/edac_cdx_pcol.h | 28 +
>
> I'd need a MAINTAINERS entry for this driver so that you can get CCed on fixes.
> And I'd need that email to be responsive and not stuff to disappear into the void.
> If it does and no one cares for it, I don't have a problem with removing this driver
> again from the tree.
>
> > +/* Granularity of reported error in bytes */
> > +#define DDRMC5_EDAC_ERR_GRAIN 1
> > +#define MC_CMD_EDAC_GET_DDR_CONFIG_IN_LEN 4
> > +
> > +#define DDRMC5_EDAC_MSG_SIZE 256
> > +
> > +#define DDRMC5_IRQ_CE_MASK GENMASK(18, 15)
> > +#define DDRMC5_IRQ_UE_MASK GENMASK(14, 11)
> > +
> > +#define DDRMC5_RANK_1_MASK GENMASK(11, 6)
> > +#define MASK_24 GENMASK(29, 24)
> > +#define MASK_0 GENMASK(5, 0)
> > +
> > +#define DDRMC5_LRANK_1_MASK GENMASK(11, 6)
> > +#define DDRMC5_LRANK_2_MASK GENMASK(17, 12)
> > +#define DDRMC5_BANK1_MASK GENMASK(11, 6)
> > +#define DDRMC5_GRP_0_MASK GENMASK(17, 12)
> > +#define DDRMC5_GRP_1_MASK GENMASK(23, 18)
> > +
> > +#define ECCR_UE_CE_ADDR_HI_ROW_MASK GENMASK(10, 0)
> > +
> > +#define DDRMC5_MAX_ROW_CNT 18
> > +#define DDRMC5_MAX_COL_CNT 11
> > +#define DDRMC5_MAX_RANK_CNT 2
> > +#define DDRMC5_MAX_LRANK_CNT 4
> > +#define DDRMC5_MAX_BANK_CNT 2
> > +#define DDRMC5_MAX_GRP_CNT 3
> > +
> > +#define DDRMC5_REGHI_ROW 7
> > +#define DDRMC5_EACHBIT 1
> > +#define DDRMC5_ERR_TYPE_CE 0
> > +#define DDRMC5_ERR_TYPE_UE 1
> > +#define DDRMC5_HIGH_MEM_EN BIT(20)
> > +#define DDRMC5_MEM_MASK GENMASK(19, 0)
> > +#define DDRMC5_X16_BASE 256
> > +#define DDRMC5_X16_ECC 32
> > +#define DDRMC5_X16_SIZE (DDRMC5_X16_BASE +
> DDRMC5_X16_ECC)
> > +#define DDRMC5_X32_SIZE 576
> > +#define DDRMC5_HIMEM_BASE (256 * SZ_1M)
> > +#define DDRMC5_ILC_HIMEM_EN BIT(28)
> > +#define DDRMC5_ILC_MEM GENMASK(27, 0)
> > +#define DDRMC5_INTERLEAVE_SEL GENMASK(3, 0)
> > +#define DDRMC5_BUS_WIDTH_MASK GENMASK(19, 18)
> > +#define DDRMC5_NUM_CHANS_MASK BIT(17)
> > +#define DDRMC5_RANK_MASK GENMASK(15, 14)
> > +#define DDRMC5_DWIDTH_MASK GENMASK(5, 4)
> > +
> > +#define AMD_MIN_BUF_LEN 0x28
> > +#define AMD_ERROR_LEVEL 2
> > +#define AMD_ERRORID 3
> > +#define TOTAL_ERR_LENGTH 5
> > +#define AMD_MSG_ERR_OFFSET 8
> > +#define AMD_MSG_ERR_LENGTH 9
> > +#define AMD_ERR_DATA 10
> > +#define MCDI_RESPONSE 0xFF
> > +
> > +#define ERR_NOTIFICATION_MAX 96
> > +#define REG_MAX 152
> > +#define ADEC_MAX 152
> > +#define NUM_CONTROLLERS 8
> > +#define REGS_PER_CONTROLLER 19
> > +#define ADEC_NUM 19
> > +#define MC_CMD_EDAC_GET_OVERALL_DDR_CONFIG 2
> > +#define BUFFER_SZ 80
> > +
> > +#define XDDR5_BUS_WIDTH_64 0
> > +#define XDDR5_BUS_WIDTH_32 1
> > +#define XDDR5_BUS_WIDTH_16 2
>
> For all those defines above:
>
> - remove the unused ones
> - remove "EDAC" from the name
> - shorten them
>
> which will allow you to have shorter lines in general and vertical alignment will be
> closer to the left.
>
> > +
> > +#define AMD_ERR "[VERSAL_EDAC_ERR_ID: %d] Error type:"
>
> This is not now this is done - see the edac_*_printk() macros in
> drivers/edac/edac_mc.h. You could use them, extend them and so on. No need
> for homegrown hackery.
The AMD_ERR is used for the snprintf and then the string is passed to the log_non_standard_event.
Should I plan to add a edac_snprintf to the drivers/edac/edac_mc.h.
Or should I just merge AMD_ERR to snprintf.
> > + snprintf(mc_priv->message, DDRMC5_EDAC_MSG_SIZE,
> > + AMD_ERR "CFU error", error_id);
> > + break;
> > + /* CFRAME error */
> > + case 7:
> > + snprintf(mc_priv->message, DDRMC5_EDAC_MSG_SIZE,
> > + AMD_ERR "CFRAME error", error_id);
> > + break;
> > + /* Microblaze correctable error */
> > + case 10:
> > + snprintf(mc_priv->message, DDRMC5_EDAC_MSG_SIZE,
> > + AMD_ERR "DDRMC Microblaze Correctable ECC error", error_id);
> > + break;
>
...
;
> > + }
> > +
> > + priv->mci = mci;
> > + priv->dwidth = dt;
> > + mc_init(mci, pdev);
> > + rc = edac_mc_add_mc(mci);
> > + if (rc) {
> > + edac_printk(KERN_ERR, EDAC_MC,
> > + "Failed to register with EDAC core\n");
> > + goto free_edac_mc;
>
> This is wrong as it frees only the current mci which the loop has allocated.
> You need to loop back from i to 0, free all those previous mcis and unwind all the
> setup work properly.
>
We are registering the first valid EDAC controller and free that .
However I understand from your comment that I should be registering all the valid
EDAC controllers and free them.
Will fix in the next version.
> > + }
> > + return 0;
> > + }
> > + }
> > +
Powered by blists - more mailing lists