| Message ID | 1582537357-10339-3-git-send-email-sherry.sun@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve the output message for CE/UE | expand |
Hi Sherry, On 24/02/2020 09:42, Sherry Sun wrote: > The origin way which call sprintf() function two or three times to (original? 'The current way' may be better) > output message for CE/UE is incorrect, because it won't output all the > message needed, instead it will only output the last message in > sprintf(). Nice! > So the simplest and most effective way to fix this problem > is reorganizing all the output message needed for CE/UE into one > sprintf() function. This is a bug, but its in the middle of a series doing some cleanup, meaning the maintainer can't easily pick it up in isolation. Could you post it separately? 'Reorganize' in the subject makes this sound like cleanup. Would "EDAC: synopsys: Fix back to back snprintf() messages for CE/UE" be better? Please add: Fixes: b500b4a029d57 ("EDAC, synopsys: Add ECC support for ZynqMP DDR controller") in the signed-off-by area so that stable trees pick this up. and for what its worth: Reviewed-by: James Morse <james.morse@arm.com> Thanks! James > diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c > index 7046b8929522..ef7e907c7956 100644 > --- a/drivers/edac/synopsys_edac.c > +++ b/drivers/edac/synopsys_edac.c > @@ -485,20 +485,14 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p) > pinf = &p->ceinfo; > if (!priv->p_data->quirks) { > snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > - "DDR ECC error type:%s Row %d Bank %d Col %d ", > - "CE", pinf->row, pinf->bank, pinf->col); > - snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > - "Bit Position: %d Data: 0x%08x\n", > + "DDR ECC error type:%s Row %d Bank %d Col %d Bit Position: %d Data: 0x%08x", > + "CE", pinf->row, pinf->bank, pinf->col, > pinf->bitpos, pinf->data); > } else { > snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > - "DDR ECC error type:%s Row %d Bank %d ", > - "CE", pinf->row, pinf->bank); > - snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > - "BankGroup Number %d Block Number %d ", > - pinf->bankgrpnr, pinf->blknr); > - snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > - "Bit Position: %d Data: 0x%08x\n", > + "DDR ECC error type:%s Row %d Bank %d BankGroup Number %d Block Number %d Bit Position: %d Data: 0x%08x", > + "CE", pinf->row, pinf->bank, > + pinf->bankgrpnr, pinf->blknr, > pinf->bitpos, pinf->data); > } > > @@ -515,10 +509,8 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p) > "UE", pinf->row, pinf->bank, pinf->col); > } else { > snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > - "DDR ECC error type :%s Row %d Bank %d ", > - "UE", pinf->row, pinf->bank); > - snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > - "BankGroup Number %d Block Number %d", > + "DDR ECC error type :%s Row %d Bank %d BankGroup Number %d Block Number %d", > + "UE", pinf->row, pinf->bank, > pinf->bankgrpnr, pinf->blknr); > } > >
Hi James, > -----Original Message----- > From: linux-edac-owner@vger.kernel.org <linux-edac- > owner@vger.kernel.org> On Behalf Of James Morse > Sent: 2020年2月27日 1:49 > To: Sherry Sun <sherry.sun@nxp.com> > Cc: bp@alien8.de; mchehab@kernel.org; tony.luck@intel.com; > rrichter@marvell.com; michal.simek@xilinx.com; linux- > edac@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Frank Li > <frank.li@nxp.com> > Subject: Re: [PATCH 2/3] EDAC: synopsys: Reorganizing the output message > for CE/UE > > Hi Sherry, > > On 24/02/2020 09:42, Sherry Sun wrote: > > The origin way which call sprintf() function two or three times to > > (original? 'The current way' may be better) > Sorry for that, I will correct it to 'The current way'. > > > output message for CE/UE is incorrect, because it won't output all the > > message needed, instead it will only output the last message in > > sprintf(). > > Nice! > > > > So the simplest and most effective way to fix this problem is > > reorganizing all the output message needed for CE/UE into one > > sprintf() function. > > This is a bug, but its in the middle of a series doing some cleanup, meaning > the maintainer can't easily pick it up in isolation. Could you post it separately? > > 'Reorganize' in the subject makes this sound like cleanup. Would "EDAC: > synopsys: Fix back to back snprintf() messages for CE/UE" be better? > Sure, I will send this patch separately, and will correct the subject as you suggested. > > Please add: > Fixes: b500b4a029d57 ("EDAC, synopsys: Add ECC support for ZynqMP DDR > controller") > > in the signed-off-by area so that stable trees pick this up. > > and for what its worth: > Reviewed-by: James Morse <james.morse@arm.com> > > Thanks, I will add it. Best regards Sherry Sun > Thanks! > > James > > > > > diff --git a/drivers/edac/synopsys_edac.c > > b/drivers/edac/synopsys_edac.c index 7046b8929522..ef7e907c7956 > 100644 > > --- a/drivers/edac/synopsys_edac.c > > +++ b/drivers/edac/synopsys_edac.c > > @@ -485,20 +485,14 @@ static void handle_error(struct mem_ctl_info > *mci, struct synps_ecc_status *p) > > pinf = &p->ceinfo; > > if (!priv->p_data->quirks) { > > snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > > - "DDR ECC error type:%s Row %d Bank %d > Col %d ", > > - "CE", pinf->row, pinf->bank, pinf->col); > > - snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > > - "Bit Position: %d Data: 0x%08x\n", > > + "DDR ECC error type:%s Row %d Bank %d > Col %d Bit Position: %d Data: 0x%08x", > > + "CE", pinf->row, pinf->bank, pinf->col, > > pinf->bitpos, pinf->data); > > } else { > > snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > > - "DDR ECC error type:%s Row %d Bank %d ", > > - "CE", pinf->row, pinf->bank); > > - snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > > - "BankGroup Number %d Block Number %d ", > > - pinf->bankgrpnr, pinf->blknr); > > - snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > > - "Bit Position: %d Data: 0x%08x\n", > > + "DDR ECC error type:%s Row %d Bank %d > BankGroup Number %d Block Number %d Bit Position: %d Data: 0x%08x", > > + "CE", pinf->row, pinf->bank, > > + pinf->bankgrpnr, pinf->blknr, > > pinf->bitpos, pinf->data); > > } > > > > @@ -515,10 +509,8 @@ static void handle_error(struct mem_ctl_info *mci, > struct synps_ecc_status *p) > > "UE", pinf->row, pinf->bank, pinf->col); > > } else { > > snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > > - "DDR ECC error type :%s Row %d Bank %d ", > > - "UE", pinf->row, pinf->bank); > > - snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > > - "BankGroup Number %d Block Number %d", > > + "DDR ECC error type :%s Row %d Bank %d > BankGroup Number %d Block Number %d", > > + "UE", pinf->row, pinf->bank, > > pinf->bankgrpnr, pinf->blknr); > > } > > > >
diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c index 7046b8929522..ef7e907c7956 100644 --- a/drivers/edac/synopsys_edac.c +++ b/drivers/edac/synopsys_edac.c @@ -485,20 +485,14 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p) pinf = &p->ceinfo; if (!priv->p_data->quirks) { snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, - "DDR ECC error type:%s Row %d Bank %d Col %d ", - "CE", pinf->row, pinf->bank, pinf->col); - snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, - "Bit Position: %d Data: 0x%08x\n", + "DDR ECC error type:%s Row %d Bank %d Col %d Bit Position: %d Data: 0x%08x", + "CE", pinf->row, pinf->bank, pinf->col, pinf->bitpos, pinf->data); } else { snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, - "DDR ECC error type:%s Row %d Bank %d ", - "CE", pinf->row, pinf->bank); - snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, - "BankGroup Number %d Block Number %d ", - pinf->bankgrpnr, pinf->blknr); - snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, - "Bit Position: %d Data: 0x%08x\n", + "DDR ECC error type:%s Row %d Bank %d BankGroup Number %d Block Number %d Bit Position: %d Data: 0x%08x", + "CE", pinf->row, pinf->bank, + pinf->bankgrpnr, pinf->blknr, pinf->bitpos, pinf->data); } @@ -515,10 +509,8 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p) "UE", pinf->row, pinf->bank, pinf->col); } else { snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, - "DDR ECC error type :%s Row %d Bank %d ", - "UE", pinf->row, pinf->bank); - snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, - "BankGroup Number %d Block Number %d", + "DDR ECC error type :%s Row %d Bank %d BankGroup Number %d Block Number %d", + "UE", pinf->row, pinf->bank, pinf->bankgrpnr, pinf->blknr); }
The origin way which call sprintf() function two or three times to output message for CE/UE is incorrect, because it won't output all the message needed, instead it will only output the last message in sprintf(). So the simplest and most effective way to fix this problem is reorganizing all the output message needed for CE/UE into one sprintf() function. Signed-off-by: Sherry Sun <sherry.sun@nxp.com> --- drivers/edac/synopsys_edac.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)