Re: [PATCH v6 13/15] ASoC: msm8916-wcd-analog: add pm8950 codec

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Missing NULL pointer check for the return value of `of_device_get_match_data()` leads to a kernel panic if the driver is bound via a non-OF mechanism.

Pre-existing issues:
- [High] Interrupt handlers are requested and enabled before the component state they depend on is initialized, leading to a NULL pointer dereference if the interrupt fires early.
- [High] Component removal powers off regulators while threaded IRQ handlers are still active, leading to hardware accesses to a powered-off device.
--

--- Patch [13]: [PATCH v6 13/15] ASoC: msm8916-wcd-analog: add pm8950 codec ---
Note: The format of this review report is altered to provide a free-form summary of findings without directly quoting the original patch code due to recitation restrictions.

commit 16b89870656fc786971dbd432019e6b7585632a2
Author: Adam Skladowski <a39.skl@gmail.com>

ASoC: msm8916-wcd-analog: add pm8950 codec
    
Add regs overrides for PM8950 codec and implement matching reg overrides
via compatible.

In pm8916_wcd_analog_probe(), the code retrieves match data using
of_device_get_match_data() and assigns it to wcd_reg_init_data, which is
then immediately used in a loop condition.

[Severity: High]
Does this code handle cases where the driver might be bound via a non-OF
mechanism? If of_device_get_match_data() returns NULL, the immediate
dereference of wcd_reg_init_data->seq_size will cause a kernel panic. Could
a NULL pointer check be added before accessing the match data?

In pm8916_wcd_analog_spmi_probe(), threaded IRQs are requested using
devm_request_threaded_irq() and unmasked. The ASoC component state
priv->component is initialized later in pm8916_wcd_analog_probe().

[Severity: High]
This isn't a bug introduced by this patch, but can the interrupt handlers
execute before priv->component is properly initialized? If an interrupt fires
early during initialization, pm8916_mbhc_switch_irq_handler() will
dereference the uninitialized priv->component pointer, leading to a crash.
Should the interrupt registration be deferred, or the IRQs kept disabled
until the component probe completes?

In pm8916_wcd_analog_remove(), the regulators are disabled using
regulator_bulk_disable(). The threaded IRQ handlers are tied to the platform
device via devm_request_threaded_irq() and remain active.

[Severity: High]
This is a pre-existing issue, but does this function properly synchronize
or disable the associated threaded IRQ handlers before powering off the
regulators? If an interrupt fires right before the regulators are disabled,
the IRQ thread could wake up and attempt to access the hardware via
snd_soc_component_read() while the device is powered off, potentially
causing a bus timeout or crash. Should the IRQs be explicitly disabled
before turning off the regulators?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730174353.108023-1-mailingradian@gmail.com?part=13