Re: [PATCH v7 10/10] power: supply: add support for Samsung S2M series PMIC charger device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Missing initial configuration of hardware state on probe.
- [High] Race condition between devres cleanup and extcon notifier leads to use-after-free.
- [High] Missing hardware disablement upon driver unbind leaves PMIC sourcing power.
- [High] Dangling pointer to `extcon_dev` if the provider unbinds, leading to use-after-free.
- [Medium] Unconditional charger reset on every extcon notification disrupts charging.
- [Low] Missing explicit header includes for `msleep()` and `FIELD_PREP()`.
--

commit 41ed08773e6f6eb12cab89eb389202b30e4d9e42
Author: Kaustabh Chakraborty <kauschluss@disroot.org>

power: supply: add support for Samsung S2M series PMIC charger device

Add a driver for charger controllers found in certain Samsung S2M series
PMICs. The driver has very basic support for the device, with only
charger online reporting working, and USB 2.0 device negotiations
working.

> diff --git a/drivers/power/supply/s2m-charger.c b/drivers/power/supply/s2m-charger.c
> new file mode 100644
> index 0000000000000..4d1f2c2c71446
> --- /dev/null
> +++ b/drivers/power/supply/s2m-charger.c
> @@ -0,0 +1,313 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Battery Charger Driver for Samsung S2M series PMICs.
> + *
> + * Copyright (c) 2015 Samsung Electronics Co., Ltd
> + * Copyright (c) 2026 Kaustabh Chakraborty <kauschluss@disroot.org>
> + * Copyright (c) 2026 Łukasz Lebiedziński <kernel@lvkasz.us>
> + */
> +
> +#include <linux/devm-helpers.h>
> +#include <linux/extcon.h>
> +#include <linux/mfd/samsung/core.h>
> +#include <linux/mfd/samsung/s2mu005.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>

This isn't a bug, but is it possible to add explicit includes for
<linux/delay.h> (for msleep) and <linux/bitfield.h> (for FIELD_PREP)? It
appears the driver relies on implicit transitive inclusions.

[ ... ]
> +static void s2mu005_chgr_extcon_work(struct work_struct *work)
> +{
> +	struct s2m_chgr *priv = container_of(work, struct s2m_chgr, extcon_work);
> +
> +	if (extcon_get_state(priv->extcon, EXTCON_USB_HOST) > 0)
> +		s2mu005_chgr_mode_set_host(priv);
> +	else if (extcon_get_state(priv->extcon, EXTCON_USB) > 0)
> +		s2mu005_chgr_mode_set_charger(priv);

Since the notifier is registered for all cables, a single physical USB
connection can generate multiple extcon notifications (e.g., EXTCON_USB
followed by EXTCON_CHG_USB_SDP after negotiation).

Does this unconditionally clear the OP_MODE register and interrupt charging
on every notification? It seems the driver should verify the current hardware
state and only reset if a state change is actually required.

[ ... ]
> +static int s2m_chgr_extcon_notifier(struct notifier_block *nb,
> +					unsigned long event, void *param)
> +{
> +	struct s2m_chgr *priv = container_of(nb, struct s2m_chgr, extcon_nb);
> +
> +	schedule_work(&priv->extcon_work);
> +
> +	return NOTIFY_OK;
> +}

If an extcon event fires concurrently during driver unbind, can the notifier
callback invoke schedule_work() after cancel_work_sync() has already returned?

During devres cleanup, devm_extcon_unregister_notifier_all() runs first,
followed by cancel_work_sync() from devm_work_autocancel(). Extcon uses a
raw_notifier_chain, so unregistering the notifier unlinks it but does not wait
for currently executing callbacks to finish.

Could this queue a freed work_struct into the system workqueue, resulting in a
use-after-free when the work executes?

[ ... ]
> +	/* MUIC is mandatory. If unavailable, request probe deferral */
> +	extcon_node = of_get_child_by_name(dev->parent->of_node, "muic");
> +	if (!extcon_node)
> +		return dev_err_probe(dev, -ENODEV, "MUIC node required but not found\n");
> +
> +	priv->extcon = extcon_find_edev_by_node(extcon_node);
> +	if (IS_ERR(priv->extcon))
> +		return -EPROBE_DEFER;

If the extcon provider (e.g., the MUIC driver) is manually unbound via sysfs,
the extcon_dev structure is freed. Does the charger driver retain a dangling
pointer in priv->extcon?

Because extcon_find_edev_by_node() does not increment the reference count or
establish a device link, any subsequent power supply property query or delayed
execution of s2mu005_chgr_extcon_work() will dereference this freed pointer
when calling extcon_get_state(), causing a use-after-free.

[ ... ]
> +	priv->extcon_nb.notifier_call = s2m_chgr_extcon_notifier;
> +	ret = devm_extcon_register_notifier_all(dev, priv->extcon, &priv->extcon_nb);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register extcon notifier\n");
> +
> +	return 0;
> +}

Extcon notifiers only fire on state changes. If a USB cable is already
connected when the device boots and the driver loads, wouldn't the hardware
remain unconfigured in its default mode and not begin charging?

Should the driver explicitly schedule priv->extcon_work or manually synchronize
the initial state at the end of s2m_chgr_probe()?

Also, if the driver is unbound via sysfs while operating in OTG mode (sourcing
5V on the VBUS line), is the hardware left indefinitely sourcing power with no
software managing it?

There doesn't appear to be a .remove callback or a dedicated devm teardown
action to transition the hardware to a safe state (e.g., calling
s2mu005_chgr_mode_unset()) during teardown.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-s2mu005-pmic-v7-0-73f9702fb461@disroot.org?part=10