RE: [PATCH] EDAC: i7300: disable error reporting if init fails

[Zhuo, Qiuxu]

> From: Tushar Tibude <tushar.tibude1000@xxxxxxxxx>
> Sent: Monday, April 27, 2026 8:26 PM
> To: mchehab@xxxxxxxxxx; bp@xxxxxxxxx; Luck, Tony <tony.luck@xxxxxxxxx>;
> linux-edac@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: Tushar Tibude <tushar.tibude1000@xxxxxxxxx>
> Subject: [PATCH] EDAC: i7300: disable error reporting if init fails
> 
> If error reporting is enabled during initialization but initialization fails
> immediately after, error reporting is left enabled in the mask register even
> after exit.
> 
> Create the i7300_disable_error_reporting() function to disable error reporting
> bits in the mask register EMASK_FBD. Call it at initialization failure, before call
> to i7300_put_devices() for cleanup.
> 
> This ensures clean hardware handling by disabling any unused error reporting
> bits before exiting.
> 
> Signed-off-by: Tushar Tibude <tushar.tibude1000@xxxxxxxxx>
> ---
>  drivers/edac/i7300_edac.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/edac/i7300_edac.c b/drivers/edac/i7300_edac.c index
> 69068f8d0..989cf53e9 100644
> --- a/drivers/edac/i7300_edac.c
> +++ b/drivers/edac/i7300_edac.c
> @@ -570,6 +570,27 @@ static void i7300_enable_error_reporting(struct
> mem_ctl_info *mci)
>  			       EMASK_FBD, fbd_error_mask);
>  }
> 
> +/**
> + * i7300_disable_error_reporting() - Disable the memory reporting logic at
> the
> + *				     hardware
> + * @mci: struct mem_ctl_info pointer
> + */
> +static void i7300_disable_error_reporting(struct mem_ctl_info *mci) {
> +	struct i7300_pvt *pvt = mci->pvt_info;
> +	u32 fbd_error_mask;
> +
> +	/* Read the FBD Error Mask Register */
> +	pci_read_config_dword(pvt->pci_dev_16_1_fsb_addr_map,
> +			      EMASK_FBD, &fbd_error_mask);
> +
> +	/* Disable by writing '1' */
> +	fbd_error_mask |= EMASK_FBD_ERR_MASK;
> +
> +	pci_write_config_dword(pvt->pci_dev_16_1_fsb_addr_map,
> +			       EMASK_FBD, fbd_error_mask);
> +}
> +

i7300_enable_error_reporting() and i7300_disable_error_reporting() have very
similar implementations. Would it make sense to combine them into a single helper, 
e.g. i7300_enable_error_reporting(mci, bool enable), to reduce code duplication?

'enable' is true to enable error reporting and false to disable error reporting.

>  /************************************************
>   * i7300 Functions related to memory enumberation
>   ************************************************/
> @@ -1025,6 +1046,7 @@ static int i7300_init_one(struct pci_dev *pdev, const
> struct pci_device_id *id)
>  	struct edac_mc_layer layers[3];
>  	struct i7300_pvt *pvt;
>  	int rc;
> +	bool enabled_error_reporting;
> 
>  	/* wake up device */
>  	rc = pci_enable_device(pdev);
> @@ -1084,20 +1106,22 @@ static int i7300_init_one(struct pci_dev *pdev,
> const struct pci_device_id *id)
> 
>  	/* initialize the MC control structure 'csrows' table
>  	 * with the mapping and control information */
> +	enabled_error_reporting = false;

Move this flag setting to

>  	if (i7300_get_mc_regs(mci)) {
>  		edac_dbg(0, "MC: Setting mci->edac_cap to
> EDAC_FLAG_NONE because i7300_init_csrows() returned nonzero value\n");
>  		mci->edac_cap = EDAC_FLAG_NONE;	/* no csrows found */

here.

>  	} else {
>  		edac_dbg(1, "MC: Enable error reporting now\n");
>  		i7300_enable_error_reporting(mci);
> +		enabled_error_reporting = true;
>  	}
> 
>  	/* add this new MC control structure to EDAC's list of MCs */
>  	if (edac_mc_add_mc(mci)) {
>  		edac_dbg(0, "MC: failed edac_mc_add_mc()\n");
> -		/* FIXME: perhaps some code should go here that disables
> error
> -		 * reporting if we just enabled it
> -		 */
> +		/* Disable error reporting if we just enabled it */
> +		if (enabled_error_reporting)
> +			i7300_disable_error_reporting(mci);
>  		goto fail1;
>  	}
> 
> --
> 2.43.0
> 

Need to disable error reporting in i7300_remove_one()?
Enabling it during initialization and disabling it during teardown ensures proper pairing.

Thanks!
-Qiuxu