RE: [PATCH v7 5/5] EDAC/VersalNET: Add support for error notification

[Datta, Shubhrajyoti]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Borislav Petkov <bp@xxxxxxxxx>
> Sent: Thursday, July 3, 2025 11:01 PM
> To: Datta, Shubhrajyoti <shubhrajyoti.datta@xxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> edac@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>; Krzysztof Kozlowski
> <krzk@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Tony Luck <tony.luck@xxxxxxxxx>; James Morse
> <james.morse@xxxxxxx>; Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>;
> Robert Richter <rric@xxxxxxxxxx>; Gupta, Nipun <Nipun.Gupta@xxxxxxx>;
> Agarwal, Nikhil <nikhil.agarwal@xxxxxxx>
> 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;
> > +             }
> > +     }
> > +