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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Registration of power supply before acquiring `extcon` creates a race condition that can lead to a kernel panic.
- [High] `extcon_get_state()` negative error codes are incorrectly evaluated as true in boolean contexts, risking hardware damage.
- [High] Missing `else if` in `s2mu005_chgr_get_usb_type()` overwrites valid USB types.
- [High] Infinite probe deferral loop if the mandatory 'muic' device tree node is missing.
- [High] Ignored return value from `devm_extcon_register_notifier_all()` silently masks registration failures.
- [Medium] The initial hardware charger mode is not synchronized with the physical cable state during probe.
- [Low] Misleading indentation in `s2mu005_chgr_mode_set_charger()` due to missing curly braces.
--

commit e4bc7879e2d0f0f694862fddb5772356c6262c9b
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 includes initial support for the S2MU005 PMIC charger.

> diff --git a/drivers/power/supply/s2m-charger.c b/drivers/power/supply/s2m-charger.c
> new file mode 100644
> index 0000000000000..b32cea55b8b04
> --- /dev/null
> +++ b/drivers/power/supply/s2m-charger.c
[ ... ]
> +static void s2mu005_chgr_get_usb_type(struct s2m_chgr *priv, int *value)
> +{
> +	if (extcon_get_state(priv->extcon, EXTCON_CHG_USB_CDP))
> +		*value = POWER_SUPPLY_USB_TYPE_CDP;
> +	if (extcon_get_state(priv->extcon, EXTCON_CHG_USB_SDP))
> +		*value = POWER_SUPPLY_USB_TYPE_SDP;
> +	if (extcon_get_state(priv->extcon, EXTCON_CHG_USB_DCP))
> +		*value = POWER_SUPPLY_USB_TYPE_DCP;
> +	else
> +		*value = POWER_SUPPLY_USB_TYPE_UNKNOWN;
> +}

Does the missing else if on the DCP check overwrite valid CDP or SDP states?
If a CDP or SDP charger is attached, DCP evaluates to false, which executes
the else block and overwrites *value with POWER_SUPPLY_USB_TYPE_UNKNOWN.

Also, extcon_get_state() can return negative error codes (e.g. -EINVAL) if
the cable ID is unsupported. Does evaluating this directly in a boolean
context cause it to be treated as true? Should this explicitly check for > 0?

[ ... ]
> +static int s2mu005_chgr_mode_set_charger(struct s2m_chgr *priv)
> +{
[ ... ]
> +	/* then set to charging mode */
> +	ret = regmap_update_bits(priv->regmap, S2MU005_REG_CHGR_CTRL0,
> +				 S2MU005_CHGR_OP_MODE,
> +				 FIELD_PREP(S2MU005_CHGR_OP_MODE,
> +					    S2MU005_CHGR_OP_MODE_CHG));
> +	if (ret < 0)
> +		dev_err(priv->dev, "failed to set opmode to charging (%d)\n", ret);
> +		return ret;
> +}

Is there a missing curly brace for the if (ret < 0) block? While return ret
executes unconditionally, the indentation is misleading.

[ ... ]
> +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))
> +		s2mu005_chgr_mode_set_host(priv);
> +	else if (extcon_get_state(priv->extcon, EXTCON_USB))
> +		s2mu005_chgr_mode_set_charger(priv);
> +	else
> +		s2mu005_chgr_mode_unset(priv);
> +
> +	power_supply_changed(priv->psy);
> +}

Could a negative return from extcon_get_state() evaluate as true here? This
might trigger s2mu005_chgr_mode_set_host() erroneously, turning on the 5.1V
boost converter on the VBUS line. If this triggers while connected to an
external power source, back-driving 5.1V could damage the hardware.

[ ... ]
> +static int s2m_chgr_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	psy_cfg.drv_data = priv;
> +	psy_cfg.fwnode = dev_fwnode(dev->parent);
> +	priv->psy = devm_power_supply_register(dev, psy_desc, &psy_cfg);
> +	if (IS_ERR(priv->psy))
> +		return dev_err_probe(dev, PTR_ERR(priv->psy),
> +				     "failed to register power supply subsystem\n");
> +
> +	/* MUIC is mandatory. If unavailable, request probe deferral */
> +	extcon_node = of_get_child_by_name(dev->parent->of_node, "muic");
> +	priv->extcon = extcon_find_edev_by_node(extcon_node);
> +	if (IS_ERR(priv->extcon))
> +		return -EPROBE_DEFER;

Could registering the power supply before acquiring extcon introduce a race
condition? If userspace (like udevd) reads the usb_type property
concurrently via sysfs:

sysfs read
  s2mu005_chgr_get_usb_type()
    extcon_get_state(priv->extcon, ...)

If this occurs while priv->extcon is still NULL, it passes NULL to
extcon_get_state(), returning -EINVAL.

Additionally, if extcon_find_edev_by_node() defers and returns
ERR_PTR(-EPROBE_DEFER), priv->extcon becomes an error pointer. Passing an
error pointer to extcon_get_state() bypasses its !edev NULL check, leading
to a dereference of an invalid address.

Furthermore, if the muic node is completely missing from the device tree,
of_get_child_by_name() returns NULL. Passing NULL to
extcon_find_edev_by_node() returns -EPROBE_DEFER.

Does this cause an infinite probe deferral loop for hardware that will never
appear? Should this explicitly check for !extcon_node and return -ENODEV
instead?

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

Is there a missing return statement before dev_err_probe()? Execution will
fall through to return 0, leaving the driver falsely reporting success but
unable to receive extcon state changes.

Also, the driver registers an extcon notifier, but is the initial hardware
charger mode ever synchronized with the physical cable state? Since
notifiers only fire on state transitions, if a USB cable is already plugged
in during boot, the PMIC might remain in its default unconfigured state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-s2mu005-pmic-v6-0-1979106992d4@disroot.org?part=11