Re: [PATCH v2 6/8] EDAC/aspeed: Replace regmap with direct register access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Mixing scope-based cleanup helpers with traditional goto-based error handling within the same function.
--

--- Patch [6]: [PATCH v2 6/8] EDAC/aspeed: Replace regmap with direct register access ---
commit 707abf914ceaccc2e73503abf17a6380b0fc949b
Author: Ryan Chen <ryan_chen@aspeedtech.com>

EDAC/aspeed: Replace regmap with direct register access

The driver instantiates its own regmap purely as an MMIO wrapper: it has
no register cache, uses custom .reg_read()/.reg_write() callbacks, and is
not shared as a syscon with other drivers. So it brings nothing here
beyond the spinlock that regmap takes around each access when fast_io is
set.

> diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
> index 352910e1defc5..26d2c456cc0d6 100644
> --- a/drivers/edac/aspeed_edac.c
> +++ b/drivers/edac/aspeed_edac.c

[ ... ]

> @@ -274,13 +246,11 @@ static int aspeed_probe(struct platform_device *pdev)
>  	if (IS_ERR(regs))
>  		return PTR_ERR(regs);
>  
> -	aspeed_regmap = devm_regmap_init(dev, NULL, (__force void *)regs,
> -					 &aspeed_regmap_config);
> -	if (IS_ERR(aspeed_regmap))
> -		return PTR_ERR(aspeed_regmap);
> +	scoped_guard(raw_spinlock_irqsave, &aspeed_lock)
> +		aspeed_regs = regs;

[Severity: Low]
Is it okay to mix scoped_guard() with goto-based error handling in this
function?

According to include/linux/cleanup.h, the expectation is that usage of goto
and cleanup helpers should not be mixed in the same function.

In aspeed_probe(), we introduce scoped_guard() here, but the function still
has goto probe_exit01 and goto probe_exit02 later on:

aspeed_probe() {
    ...
    scoped_guard(raw_spinlock_irqsave, &aspeed_lock)
        aspeed_regs = regs;
    ...
    rc = init_csrows(mci);
    if (rc) {
        dev_err(&pdev->dev, "failed to init csrows\n");
        goto probe_exit02;
    }
    ...
probe_exit01:
    edac_mc_del_mc(&pdev->dev);
probe_exit02:
    edac_mc_free(mci);
    ...
}

Should the remaining error paths be converted to scope-based cleanups, or
would it be better to avoid using scope-based cleanup helpers here?

>  
>  	/* bail out if ECC mode is not configured */
> -	regmap_read(aspeed_regmap, ASPEED_MCR_CONF, &reg04);
> +	reg04 = readl(regs + ASPEED_MCR_CONF);
>  	if (!(reg04 & ASPEED_MCR_CONF_ECC)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-edac-v2-0-c8d8bb693586@aspeedtech.com?part=6