Re: [PATCH v5 5/7] pinctrl: add NXP MC33978/MC34978 pinctrl driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Output emulation and open-drain semantics are broken for Programmable (SP) pins due to logic flaws in `mc33978_set()` and `mc33978_set_multiple()`.
- [High] `mc33978_set_multiple()` can return an uninitialized variable and silently masks regmap error codes.
- [Medium] Missing `mpc->lock` acquisition in `mc33978_pinconf_get()` and `mc33978_pinconf_set()` breaks hardware sequence atomicity.
- [Medium] Silent omission of SPI read failures in `mc33978_get()` leads to incorrect physical state interpretation.
- [Medium] Unconditional invocation of optional parent irq_chip callbacks `irq_bus_lock` and `irq_bus_sync_unlock`.
--

--- Patch [5]: [PATCH v5 5/7] pinctrl: add NXP MC33978/MC34978 pinctrl driver ---
commit 98645888889b3328cc6079ef5439da9e7d2a7837
Author: David Jander <david@protonic.nl>

pinctrl: add NXP MC33978/MC34978 pinctrl driver

Add pin control and GPIO driver for the NXP MC33978/MC34978 Multiple
Switch Detection Interface (MSDI) devices.

> diff --git a/drivers/pinctrl/pinctrl-mc33978.c b/drivers/pinctrl/pinctrl-mc33978.c
> new file mode 100644
> index 0000000000000..31065d6333173
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-mc33978.c

[ ... ]

> +static int mc33978_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
> +			       unsigned long *config)
> +{
> +	struct mc33978_pinctrl *mpc = pinctrl_dev_get_drvdata(pctldev);
> +	enum pin_config_param param = pinconf_to_config_param(*config);
> +	unsigned int data, status;
> +	int ret;
> +
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		ret = mc33978_get_pull(mpc, pin, &data);

[Severity: Medium]
The mpc->lock is documented as protecting atomic reads and multi-register
hardware sequences. Does the lack of this lock in mc33978_pinconf_get() and
mc33978_pinconf_set() expose the hardware state reads and updates to race
conditions with concurrent GPIO operations?

[ ... ]

> +static int mc33978_get(struct gpio_chip *chip, unsigned int offset)
> +{

[ ... ]

> +	/* Determine current topology for SP pins */
> +	if (mc33978_is_sp(offset)) {
> +		int config_reg;
> +
> +		ret = mc33978_read(mpc, MC33978_REG_CONFIG, &config_reg);
> +		if (ret == 0) {
> +			/* CONFIG: 0 = Switch-to-Ground (PU), 1 = Switch-to-Battery (PD) */
> +			if (config_reg & MC33978_PINMASK(offset))
> +				is_switch_to_ground = false;
> +		}
> +	}

[Severity: Medium]
If the SPI read in mc33978_read() fails, the code ignores the error code in
ret and proceeds with is_switch_to_ground remaining true. 

Should the error code in ret be propagated back to the caller instead of
falling back to a default topology guess that might misinterpret the physical
state?

[ ... ]

> +static int mc33978_set(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> +	struct mc33978_pinctrl *mpc = gpiochip_get_data(chip);
> +	int pull;
> +	int ret;
> +
> +	/*
> +	 * We emulate open-drain/-source outputs by routing or isolating the
> +	 * active wetting current sources.
> +	 * To drive the line, we apply the current source.
> +	 * To turn the line OFF (achieve High-Impedance), we MUST use the
> +	 * hardware TRI_SP / TRI_SG tri-state registers to physically isolate
> +	 * it.
> +	 */
> +	if (mc33978_is_sp(offset)) {
> +		pull = value ? MC33978_PU : MC33978_PD;
> +		value = 1;
> +	} else {
> +		pull = MC33978_PU;
> +	}

[Severity: High]
For SP pins, value is unconditionally forced to 1. Will this prevent the 
turn OFF branch (value == 0) below from ever being executed for these pins?

> +
> +	guard(mutex)(&mpc->lock);
> +
> +	/*
> +	 * Break-before-make sequencing to prevent hardware glitches (spikes).
> +	 * Since SPI transfers take time, writing the pull and tri-state
> +	 * registers in the wrong order causes a brief moment where current
> +	 * flows to the pin before it is masked, causing a visible LED flash.
> +	 */
> +	if (value) {
> +		/*
> +		 * Turn ON: Configure the underlying current source (pull) first,
> +		 * then route it to the pin by disabling tri-state.
> +		 */
> +		ret = mc33978_set_pull(mpc, offset, pull);

[ ... ]

> +static int mc33978_set_multiple(struct gpio_chip *chip,
> +				unsigned long *mask, unsigned long *bits)
> +{
> +	unsigned int sgmask = (*mask & MC33978_SG_MASK) >> MC33978_SG_SHIFT;
> +	unsigned int sgbits = (*bits & MC33978_SG_MASK) >> MC33978_SG_SHIFT;
> +	unsigned int spmask = (*mask & MC33978_SP_MASK) >> MC33978_SP_SHIFT;
> +	unsigned int spbits = (*bits & MC33978_SP_MASK) >> MC33978_SP_SHIFT;
> +	struct mc33978_pinctrl *mpc = gpiochip_get_data(chip);
> +	int ret;
> +
> +	guard(mutex)(&mpc->lock);
> +	if (spmask)
> +		ret = mc33978_update_bits(mpc, MC33978_REG_TRI_SP, spmask,
> +					  ~spbits);

[Severity: High]
Does this direct update to MC33978_REG_TRI_SP contradict the expected state
setup in mc33978_set(), since the required pull direction is not being
configured here?

Also, if both spmask and sgmask evaluate to zero, will this function return
an uninitialized ret?

If both spmask and sgmask are non-zero, will the second call to
mc33978_update_bits() silently overwrite an error code returned by the first
call?

> +	if (sgmask)
> +		ret = mc33978_update_bits(mpc, MC33978_REG_TRI_SG, sgmask,
> +					  ~sgbits);
> +
> +	return ret;
> +}

[ ... ]

> +static void mc33978_gpio_irq_bus_lock(struct irq_data *data)
> +{
> +	struct irq_data *parent = data->parent_data;
> +
> +	parent->chip->irq_bus_lock(parent);
> +}

[Severity: Medium]
Since irq_bus_lock is an optional callback in struct irq_chip, is it possible
for parent->chip->irq_bus_lock to be NULL here?

Could this cause a NULL pointer dereference panic if the parent domain does
not implement it?

> +
> +static void mc33978_gpio_irq_bus_sync_unlock(struct irq_data *data)
> +{
> +	struct irq_data *parent = data->parent_data;
> +
> +	parent->chip->irq_bus_sync_unlock(parent);
> +}

[Severity: Medium]
Similarly, could parent->chip->irq_bus_sync_unlock be NULL and lead to a 
NULL pointer dereference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260316140514.1406588-1-o.rempel@pengutronix.de?part=5