Re: [PATCH v6 10/11] 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:
- [High] The driver attempts to set the extcon state using an unallocated `priv->extcon` pointer during initialization, causing the initial connection state to be lost.
- [High] The driver writes instance-specific IRQ numbers into a globally shared static array, breaking driver re-entrancy and multi-instance support.
- [High] The driver suppresses the `-EPROBE_DEFER` error code when requesting IRQs, permanently failing the probe.
- [High] The USB DM/DP pin configuration and extcon logic incorrectly handles Dedicated Charging Ports (DCP) and Charging Downstream Ports (CDP).
- [High] Unconditional dereference of `platform_get_device_id(pdev)` will cause a kernel panic when probed via Device Tree.
- [High] Missing synchronization locks allow concurrent threaded IRQ handlers and the `remove` callback to race and corrupt shared hardware state.
- [Medium] A bitmask register value is evaluated using an exact-match `switch` statement, leading to silent failures if multiple bits are set.
--

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

extcon: add support for Samsung S2M series PMIC extcon devices

Add a driver for MUIC devices found in certain Samsung S2M series PMICs.
These are USB port accessory detectors. 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
> --- /dev/null
> +++ b/drivers/extcon/extcon-s2m.c
[ ... ]
> +static int s2mu005_muic_attach(struct s2m_muic *priv)
> +{
> +	unsigned int type;
> +	int ret;
[ ... ]
> +	/*
> +	 * All USB connections which require communication via its D+
> +	 * and D- wires need it.
> +	 */
> +	if (type & (S2MU005_MUIC_OTG | S2MU005_MUIC_DCP | S2MU005_MUIC_SDP)) {
> +		ret = regmap_update_bits(priv->regmap, S2MU005_REG_MUIC_SWCTRL,

Does this pin configuration logic erroneously include S2MU005_MUIC_DCP (which
shorts D+/D- and performs no data communication) and omit S2MU005_MUIC_CDP
(which does require data communication)?

[ ... ]
> +	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;

Since the type variable was previously checked using a bitwise AND, indicating
it acts as a bitmask, could this exact-match switch statement fail to process
the attachment if any other bits or status flags are set in the register?

> +	case S2MU005_MUIC_CDP:
> +		dev_dbg(priv->dev, "USB CDP connection detected\n");
> +		extcon_set_state_sync(priv->extcon, EXTCON_USB, true);
> +		extcon_set_state_sync(priv->extcon, EXTCON_CHG_USB_CDP, true);
> +		priv->attached = true;
> +		break;
> +	case S2MU005_MUIC_SDP:
> +		dev_dbg(priv->dev, "USB SDP connection detected\n");
> +		extcon_set_state_sync(priv->extcon, EXTCON_USB, true);
> +		extcon_set_state_sync(priv->extcon, EXTCON_CHG_USB_SDP, true);
> +		priv->attached = true;
> +		break;
> +	case S2MU005_MUIC_DCP:
> +		dev_dbg(priv->dev, "USB DCP connection detected\n");
> +		extcon_set_state_sync(priv->extcon, EXTCON_USB, true);

Is it correct to assert the EXTCON_USB state for DCP connections? EXTCON_USB
typically signals a standard data connection to the system. Setting it for a
charger with shorted data pins could wake the USB PHY and cause enumeration
timeouts.

[ ... ]
> +static int s2m_muic_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sec_pmic_dev *pmic_drvdata = dev_get_drvdata(dev->parent);
> +	struct s2m_muic *priv;
> +	int ret;
> +	int i;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +	priv->dev = dev;
> +	priv->regmap = pmic_drvdata->regmap_pmic;
> +
> +	switch (platform_get_device_id(pdev)->driver_data) {

Can platform_get_device_id(pdev) return NULL when the driver is matched via
Device Tree, since the platform bus does not populate pdev->id_entry?
Dereferencing it unconditionally could cause a NULL pointer dereference crash.
Would device_get_match_data() be more robust here?

> +	case S2MU005:
> +		priv->extcon_cable = s2mu005_muic_extcon_cable;
> +		priv->irq_data = s2mu005_muic_irq_data;
> +		/* Initialize MUIC */
> +		ret = s2mu005_muic_init(priv);
> +		break;
> +	default:
> +		return dev_err_probe(dev, -ENODEV,
> +				     "device type %d is not supported by driver\n",
> +				     pmic_drvdata->device_type);
> +	}
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to initialize MUIC\n");
> +
> +	priv->extcon = devm_extcon_dev_allocate(dev, priv->extcon_cable);

Since s2mu005_muic_init() configures the hardware and calls s2mu005_muic_attach(),
it will attempt to set the extcon state before priv->extcon is allocated:

    extcon_set_state_sync(priv->extcon, EXTCON_USB_HOST, true);

Because priv->extcon is NULL at that point, the extcon subsystem will return
-EINVAL. Will this cause the driver to silently drop the initial cable state if
an accessory is already connected at boot?

[ ... ]
> +	for (i = 0; priv->irq_data[i].handler; i++) {
> +		int irq = platform_get_irq_byname_optional(pdev,
> +							   priv->irq_data[i].name);
> +		if (irq == -ENXIO)
> +			continue;
> +		if (irq <= 0)
> +			return dev_err_probe(dev, -EINVAL, "failed to get IRQ %s\n",
> +					     priv->irq_data[i].name);

If platform_get_irq_byname_optional() returns -EPROBE_DEFER, will returning a
hardcoded -EINVAL here prevent the driver from properly participating in the
kernel's deferred probing mechanism?

> +
> +		priv->irq_data[i].irq = irq;

Because priv->irq_data points to s2mu005_muic_irq_data, which is a global static
array, does modifying the array here break driver re-entrancy and multi-instance
support?

If multiple devices are instantiated or a device is rebound, the global array
is overwritten, which could cause earlier instances to fail to match their
interrupts and silently drop them.

> +		ret = devm_request_threaded_irq(dev, 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");
> +	}
> +
> +	return 0;
> +}
> +
> +static void s2m_muic_remove(struct platform_device *pdev)
> +{
> +	struct s2m_muic *priv = dev_get_drvdata(&pdev->dev);
> +
> +	/*
> +	 * 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);
> +		break;
> +	}
> +}

Is there a race condition between the threaded IRQ handlers and the remove
callback?

The IRQ handlers perform read-modify-write operations on the shared
priv->attached flag and issue regmap commands without holding any locks. Since
the IRQs are managed by devres, they remain active during the execution of
s2m_muic_remove().

If an interrupt fires during module unload, it could run concurrently with
s2mu005_muic_detach(), potentially corrupting shared hardware state or
re-enabling manual switching right as the driver is removed.

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