Re: [PATCH v1 1/1] mfd: si476x-i2c: Get rid of duplicate NULL checks

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

Pre-existing issues:
- [High] Use-after-free of the `core` structure and `rds_fifo` buffer due to uncancelled `rds_fifo_drainer` work.
- [High] Shared static variable `io_errors_count` creates a concurrency race and cross-device contamination.
- [High] Premature termination of the polling loop causes the power-down command to reliably timeout.
- [Medium] Dynamic mutation of `mfd_cell` array violates MFD subsystem guidelines.
- [Medium] Usage of bespoke parent-child data accessor function bypasses standard device APIs.
- [Medium] Manual computation of cell IDs instead of using `PLATFORM_DEVID_AUTO`.
- [Medium] Unintended hardware reset asserted during a soft-start error path.
--

--- Patch [1]: [PATCH v1 1/1] mfd: si476x-i2c: Get rid of duplicate NULL checks ---
commit ed17e6e9e7ceb43eb92448b1c1f64b14a925bc07
Author: Andy Shevchenko <[email protected]>

mfd: si476x-i2c: Get rid of duplicate NULL checks

GPIO descriptor APIs are NULL-aware and since the requested line is optional
we don't need to have an additional check each time we want to toggle GPIO.

Get rid of duplicate NULL checks.

> diff --git a/drivers/mfd/si476x-i2c.c b/drivers/mfd/si476x-i2c.c
> index 55700ce711f49..9bce720a0a082 100644
> --- a/drivers/mfd/si476x-i2c.c
> +++ b/drivers/mfd/si476x-i2c.c

[ ... ]

> @@ -197,8 +196,7 @@ int si476x_core_start(struct si476x_core *core, bool soft)
>  	else
>  		cancel_delayed_work_sync(&core->status_monitor);
>  
> -	if (core->reset)
> -		gpiod_set_value_cansleep(core->reset, 1);
> +	gpiod_set_value_cansleep(core->reset, 1);

[Severity: Medium]
This is a pre-existing issue, but doesn't this unconditionally assert the
hardware reset during a soft-start error path?

If si476x_core_start() is called with soft = true (to avoid toggling the reset
line), but an intermediate initialization step fails, the function jumps to
the disable_irq label and unconditionally calls
gpiod_set_value_cansleep(core->reset, 1).  Could this assert a hardware reset
against the caller's request?

>  
>  	return err;
>  }
> @@ -242,10 +240,9 @@ int si476x_core_stop(struct si476x_core *core, bool soft)
>  	else
>  		cancel_delayed_work_sync(&core->status_monitor);
>  
> -	if (!soft) {
> -		if (core->reset)
> -			gpiod_set_value_cansleep(core->reset, 1);
> -	}
> +	if (!soft)
> +		gpiod_set_value_cansleep(core->reset, 1);
> +
>  	return err;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does premature termination of
the polling loop cause the power-down command to reliably timeout?

In si476x_core_stop(), when called with soft=true:

drivers/mfd/si476x-i2c.c:si476x_core_stop() {
    ...
    atomic_set(&core->is_alive, 0);

    if (soft) {
        ...
        err = si476x_core_cmd_power_down(core, &args);
    }
    ...
}

Because is_alive is cleared immediately, the polling loop stops:

drivers/mfd/si476x-i2c.c:si476x_core_poll_loop() {
    ...
    if (atomic_read(&core->is_alive))
        si476x_core_schedule_polling_work(core);
}

When si476x_core_cmd_power_down() is called, it waits for the CTS flag, but
without the polling loop, is the CTS never checked or signaled, guaranteeing
a timeout?

[Severity: High]
This is a pre-existing issue, but could the driver use a freed FIFO buffer
when unloading?

In si476x_core_remove(), the rds_fifo is freed without canceling the
rds_fifo_drainer worker:

drivers/mfd/si476x-i2c.c:si476x_core_remove() {
    ...
    if (client->irq)
        disable_irq(client->irq);
    else
        cancel_delayed_work_sync(&core->status_monitor);

    kfifo_free(&core->rds_fifo);
}

If an RDS interrupt fires or the polling loop queues rds_fifo_drainer just as
the driver is being removed, the worker will write to the freed FIFO:

drivers/mfd/si476x-i2c.c:si476x_core_drain_rds_fifo() {
    ...
    kfifo_in(&core->rds_fifo, report.rds, sizeof(report.rds));
    ...
}

Can this lead to memory corruption and a use-after-free panic?

[Severity: High]
This isn't a bug introduced by this patch, but does the shared static variable
io_errors_count create a concurrency race and cross-device contamination?

drivers/mfd/si476x-i2c.c:si476x_core_i2c_xfer() {
    ...
    static int io_errors_count;
    ...
}

If multiple Si476x chips exist on the system, any I2C transfer failure
increments this global counter.  Would this artificially inflate errors for
healthy chips and potentially cause incorrect device death pronunciation?

[Severity: Medium]
This is a pre-existing issue, but is the dynamic mutation of the mfd_cell
array violating MFD subsystem guidelines?

drivers/mfd/si476x-i2c.c:si476x_core_probe() {
    ...
    cell = &core->cells[SI476X_RADIO_CELL];
    cell->name = "si476x-radio";
    ...
}

The MFD subsystem guidelines require all mfd_cell arrays to be defined as
static const to prevent initialization ordering races and memory safety
hazards.  Is it safe to create a mutable copy here?

[Severity: Medium]
This isn't a bug introduced by this patch, but does the usage of a bespoke
parent-child data accessor function bypass standard device APIs?

include/linux/mfd/si476x-core.h:
static inline struct si476x_core *i2c_mfd_cell_to_core(struct device *dev)
{
    ...
}

MFD guidelines strictly prohibit bespoke accessors and require child drivers
to use standard APIs like dev_get_drvdata(pdev->dev.parent).  Might this
introduce tight coupling between parent and child drivers?

[Severity: Medium]
This is a pre-existing issue, but should the driver manually compute cell IDs
instead of using PLATFORM_DEVID_AUTO?

drivers/mfd/si476x-i2c.c:si476x_core_probe() {
    ...
    rval = mfd_add_devices(&client->dev,
                           (client->adapter->nr << 8) + client->addr,
                           core->cells, cell_num,
                           NULL, 0, NULL);
    ...
}

The driver manually maps cell IDs to instance numbers.  Doesn't the MFD
subsystem require using PLATFORM_DEVID_AUTO to avoid device naming collisions
in sysfs and load failures if hardware ID formulas overlap?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1