Re: [RFC PATCH 01/14] treewide: Add backend for macsmc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mfd: macsmc: Kconfig split disables Apple SMC drivers in arm64 defconfig
- [Medium] mfd: macsmc: missing notification cleanup on apple_smc_probe() error
- [Low] mfd: macsmc: endianness type mismatch in apple_smc_probe() #KEY read
--

commit f3824312f6532aec08553002200ae557d8b3b0da
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

treewide: Add backend for macsmc

This commit splits the Apple SMC driver into a core MFD driver
(drivers/mfd/macsmc-core.c) and an RTKit backend driver
(drivers/soc/apple/macsmc-rtkit.c) so that additional SMC backends such as
ACPI on Intel Macs can share the core and sub-drivers.

[ ... ]

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 87ed55c155f03..c58cc3315f22a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -339,24 +339,15 @@ config MFD_LOCHNAGAR
>  	help
>  	  Support for Cirrus Logic Lochnagar audio development board.
>  
> -config MFD_MACSMC
> -	tristate "Apple Silicon System Management Controller (SMC)"
> -	depends on ARCH_APPLE || COMPILE_TEST
> -	depends on OF
> -	depends on APPLE_RTKIT
> +config MFD_MACSMC_CORE
> +	tristate "Apple System Management Controller (SMC) Core"
>  	select MFD_CORE

[Severity: High]
Does removing this symbol without updating arch/arm64/configs/defconfig or
providing defaults cause a regression where all Apple SMC drivers become
disabled on arm64 builds?

In arch/arm64/configs/defconfig:

    CONFIG_MFD_MACSMC=m

With that symbol removed and neither the new core option nor the new rtkit
backend option in drivers/soc/apple/Kconfig having a default, running
olddefconfig or building arm64 defconfig drops the unrecognized symbol and
disables the core, backend, and all dependent sub-drivers (gpio, reset,
hwmon, power supply, and rtc).

Should arch/arm64/configs/defconfig be updated and/or should the rtkit
backend select the core option and default to m or y when the Apple platform
dependencies are enabled?

[ ... ]

> diff --git a/drivers/mfd/macsmc-core.c b/drivers/mfd/macsmc-core.c
> new file mode 100644
> index 0000000000000..b8905c834c448
> --- /dev/null
> +++ b/drivers/mfd/macsmc-core.c
> @@ -0,0 +1,164 @@

[ ... ]

> +int apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops,
> +		void *cookie, struct blocking_notifier_head *event_handlers, bool is_acpi)
> +{
> +	struct apple_smc *smc;
> +	__be32 count;
> +	int ret;
> +
> +	smc = devm_kzalloc(dev, sizeof(*smc), GFP_KERNEL);
> +	if (!smc)
> +		return -ENOMEM;
> +
> +	smc->dev = dev;
> +	smc->be_cookie = cookie;
> +	smc->be = ops;
> +	smc->is_acpi = is_acpi;
> +	smc->event_handlers = event_handlers;
> +	mutex_init(&smc->mutex);
> +	spin_lock_init(&smc->lock);
> +
> +	ret = apple_smc_read_u32(smc, SMC_KEY(#KEY), &count);
> +	if (ret < 0)
> +		return ret;
> +	smc->key_count = be32_to_cpu(count);

[Severity: Low]
Doesn't passing &count (declared as __be32) to apple_smc_read_u32() trigger
a sparse endianness warning?

In include/linux/mfd/macsmc.h, apple_smc_read_u32() expects a host-endian
u32 * pointer:

    static inline int apple_smc_read_u32(struct apple_smc *smc, smc_key key, u32 *p)
    {
        int ret = apple_smc_read(smc, key, p, sizeof(*p));
        return (ret < 0) ? ret : ((ret != sizeof(*p)) ? -EINVAL : 0);
    }

Previously, apple_smc_probe() called apple_smc_read() directly with &count,
which took a void * buffer. Should apple_smc_probe() either call
apple_smc_read() as before or use a helper that accepts __be32?

[ ... ]

> diff --git a/drivers/mfd/macsmc.c b/drivers/soc/apple/macsmc-rtkit.c
> similarity index 70%
> rename from drivers/mfd/macsmc.c
> rename to drivers/soc/apple/macsmc-rtkit.c
> index 514cba7dc897a..eaebeb1be3734 100644
> --- a/drivers/mfd/macsmc.c
> +++ b/drivers/soc/apple/macsmc-rtkit.c

[ ... ]

> @@ -458,49 +450,32 @@ static int apple_smc_probe(struct platform_device *pdev)
>  		return -EIO;
>  	}
>  
> -	dev_set_drvdata(&pdev->dev, smc);
>  	BLOCKING_INIT_NOTIFIER_HEAD(&smc->event_handlers);
>  
> -	ret = apple_smc_read(smc, SMC_KEY(#KEY), &count, sizeof(count));
> -	if (ret >= 0 && ret != sizeof(count))
> -		ret = -EINVAL;
> -	if (ret < 0)
> -		return dev_err_probe(smc->dev, ret, "Failed to get key count");
> -	smc->key_count = be32_to_cpu(count);
> -
>  	/* Enable notifications */
> -	apple_smc_write_flag(smc, SMC_KEY(NTAP), true);
> -	ret = devm_add_action_or_reset(dev, apple_smc_disable_notifications, smc);
> -	if (ret)
> -		return ret;
> +	apple_smc_rtkit_rw_locked(smc, SMC_KEY(NTAP), &flag, sizeof(flag), NULL, 0);
>  
> -	ret = devm_mfd_add_devices(smc->dev, PLATFORM_DEVID_NONE,
> -				   apple_smc_devs, ARRAY_SIZE(apple_smc_devs),
> -				   NULL, 0, NULL);
> -	if (ret)
> -		return dev_err_probe(smc->dev, ret, "Failed to register sub-devices");
> -
> -
> -	return 0;
> +	return apple_smc_probe(dev, &apple_smc_rtkit_backend_ops, smc,
> +			&smc->event_handlers, false);
>  }

[Severity: Medium]
If apple_smc_probe() fails before registering the
apple_smc_disable_notifications() cleanup action, won't SMC notifications
remain enabled on the error path?

In apple_smc_rtkit_probe(), notifications are enabled before calling
apple_smc_probe(). Inside apple_smc_probe() in drivers/mfd/macsmc-core.c,
several operations can fail and return early before
devm_add_action_or_reset() is called:

    smc = devm_kzalloc(dev, sizeof(*smc), GFP_KERNEL);
    if (!smc)
        return -ENOMEM;
    ...
    ret = apple_smc_read_u32(smc, SMC_KEY(#KEY), &count);
    if (ret < 0)
        return ret;
    smc->key_count = be32_to_cpu(count);

    ret = apple_smc_get_key_by_index(smc, 0, &smc->first_key);
    if (ret < 0)
        return ret;

    ret = apple_smc_get_key_by_index(smc, smc->key_count - 1, &smc->last_key);
    if (ret < 0)
        return ret;

    dev_set_drvdata(dev, smc);

    ret = devm_add_action_or_reset(dev, apple_smc_disable_notifications, smc);

Previously, notifications were only enabled after reading the key count and
the devm cleanup action was registered immediately afterward. In addition,
apple_smc_disable_notifications(), apple_smc_enter_atomic(),
apple_smc_write_atomic(), and apple_smc_rw() in drivers/mfd/macsmc-core.c
unconditionally call backend ops without checking if the callback pointer is
null.

Could notification enabling and its devm cleanup action be kept together
(or guarded for backends that do not implement all callbacks)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029085504.3547-1-atharvatiwarilinuxdev@gmail.com?part=1