Re: [PATCH v7 09/10] extcon: add support for Samsung S2M series PMIC extcon devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Self-deadlock in `s2m_muic_remove()` due to recursive acquisition of the internal extcon spinlock.
- [High] Sleeping I2C regmap accesses are performed in atomic context while holding a spinlock.
- [High] Missing mutual exclusion allows unprotected concurrent execution of attach/detach handlers, leading to state corruption.
- [Medium] Unhandled 'no device attached' state causes a spurious warning on boot.
- [Medium] The driver uses a fragile `switch` statement on a register read that is explicitly treated as a bitmask.
- [Low] Misuse of `WARN_ON_ONCE` by passing a string literal instead of a logical condition.
- [Low] A failed memory allocation error is masked if the subsequent initialization function fails, and initialization is performed unnecessarily.
--

commit fa85a485ca17ccd71c4ade607d5194878ea06e29
Author: Kaustabh Chakraborty <kauschluss@disroot.org>
extcon: add support for Samsung S2M series PMIC extcon devices

This commit adds a driver for MUIC devices found in certain Samsung S2M
series PMICs. These devices report multiple cable states depending on the
ID-GND resistance measured by an internal ADC.

> diff --git a/drivers/extcon/extcon-s2m.c b/drivers/extcon/extcon-s2m.c
> new file mode 100644
> index 0000000000000..8450fdf2ff3ed
> --- /dev/null
> +++ b/drivers/extcon/extcon-s2m.c
[ ... ]
> +static int s2mu005_muic_attach(struct s2m_muic *priv)
> +{
[ ... ]
> +	ret = regmap_read(priv->regmap, S2MU005_REG_MUIC_DEV1, &type);
[ ... ]
> +	if (type & (S2MU005_MUIC_OTG | S2MU005_MUIC_CDP | S2MU005_MUIC_SDP)) {
> +		ret = regmap_update_bits(priv->regmap, S2MU005_REG_MUIC_SWCTRL,
> +					 S2MU005_MUIC_DM_DP,
> +					 FIELD_PREP(S2MU005_MUIC_DM_DP,
> +						    S2MU005_MUIC_DM_DP_USB));
[ ... ]
> +	switch (type) {
> +	case S2MU005_MUIC_OTG:
> +		dev_dbg(priv->dev, "USB OTG connection detected\n");
> +		extcon_set_state_sync(priv->extcon, EXTCON_USB_HOST, true);
> +		priv->attached = true;
> +		break;
[ ... ]
> +	default:
> +		dev_warn(priv->dev,
> +			 "failed to recognize the device attached, unknown or bad type\n");
> +	}
> +
> +	return ret;
> +}

Could this switch fail to match if multiple bits are set simultaneously,
given that the earlier code treats type as a bitmask? If multiple flags
can be set, independent bitwise checks might be safer.

Also, since call_on_probe is true for the attach handler, if no cable
is attached at boot, won't type be 0? Does this cause the driver to emit
this warning on boot when no device is attached?

[ ... ]
> +static int s2m_muic_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	switch (platform_get_device_id(pdev)->driver_data) {
> +	case S2MU005:
> +		priv->extcon_cable = s2mu005_muic_extcon_cable;
> +		priv->irq_data = devm_kmemdup(dev, s2mu005_muic_irq_data,
> +					      sizeof(s2mu005_muic_irq_data),
> +					      GFP_KERNEL);
> +		/* Initialize MUIC */
> +		ret = s2mu005_muic_init(priv);
> +		break;
> +	default:
> +		return dev_err_probe(dev, -ENODEV, "device type not supported by driver\n");
> +	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to initialize MUIC\n");
> +
> +	if (!priv->irq_data)
> +		return -ENOMEM;

If devm_kmemdup() fails but s2mu005_muic_init() succeeds, does this
mask the -ENOMEM error? It also seems to perform hardware I2C initialization
even if the core data structure allocation failed. Could the allocation
check be moved immediately after the allocation?

[ ... ]
> +	for (i = 0; priv->irq_data[i].handler; i++) {
> +		ret = platform_get_irq_byname_optional(pdev, priv->irq_data[i].name);
> +		if (ret == -ENXIO)
> +			continue;
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "failed to get IRQ %s\n",
> +					     priv->irq_data[i].name);
> +
> +		priv->irq_data[i].irq = ret;
> +		ret = devm_request_threaded_irq(dev, priv->irq_data[i].irq, NULL,
> +						s2m_muic_irq_func, IRQF_ONESHOT,
> +						priv->irq_data[i].name, priv);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "failed to request IRQ\n");
> +
> +		if (priv->irq_data[i].call_on_probe)
> +			priv->irq_data[i].handler(priv);
> +	}
> +
> +	return 0;
> +}

Is there a possibility of concurrent execution here? The attach handler is
invoked manually after devm_request_threaded_irq() has already enabled the
interrupt.

If the IRQ fires immediately, the probe thread and the IRQ thread could
execute s2mu005_muic_attach() simultaneously. Is there a need for
driver-level synchronization (like a mutex) to protect the priv->attached
flag and the regmap configuration sequence?

[ ... ]
> +static void s2m_muic_remove(struct platform_device *pdev)
> +{
> +	struct s2m_muic *priv = dev_get_drvdata(&pdev->dev);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->extcon->lock, flags);
> +
> +	/*
> +	 * Disabling the MUIC device is important as it disables manual
> +	 * switching mode, thereby enabling auto switching mode.
> +	 *
> +	 * This is to ensure that when the board is powered off, it
> +	 * goes into LPM charging mode when a USB charger is connected.
> +	 */
> +	switch (platform_get_device_id(pdev)->driver_data) {
> +	case S2MU005:
> +		s2mu005_muic_detach(priv);

Will this cause a self-deadlock when the driver is unbound?
s2mu005_muic_detach() calls extcon_get_state() and extcon_set_state_sync():

s2mu005_muic_detach()
    extcon_get_state()
        spin_lock_irqsave(&edev->lock, flags);

Since s2m_muic_remove() already acquired the exact same priv->extcon->lock
before calling s2mu005_muic_detach(), and Linux spinlocks are not
recursive, won't this hang the kernel?

Additionally, is it safe to perform I2C operations in this context?
s2m_muic_remove() acquires a spinlock with IRQs disabled. While holding
this lock, s2mu005_muic_detach() performs multiple regmap_set_bits()
operations. Since the PMIC is an I2C device, these operations can sleep,
which violates atomic context rules.

Could this be avoided by using a driver-level mutex instead of bypassing the
extcon API to access the internal lock?

> +		break;
> +	default:
> +		WARN_ON_ONCE("execution shouldn't have reached here!");

This isn't a bug, but WARN_ON_ONCE expects a boolean-like condition, not
a string literal. While the string evaluates to true, it causes a
pointer-to-integer conversion warning in static analysis, and the string
itself isn't formatted into the output message. Should this be
WARN_ONCE(1, ...)?

> +	}
> +
> +	spin_unlock_irqrestore(&priv->extcon->lock, flags);
> +}

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