Re: [PATCH 2/4] media: rc: sunxi-cir: add support for the A523

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect device registration order in probe and missing `rc_unregister_device` on failure leads to race conditions and Use-After-Free.
- [High] Rounding mismatch in timeout calculations causes the maximum timeout to wrap around to 0, breaking IR reception.
- [Medium] Malformed `clock-frequency` causes a division by zero kernel panic during timeout calculations.
- [Medium] Inconsistent use of DT-requested clock frequency versus actual clock rate introduces timing inaccuracies.
--

commit ca10429d8baf92c8b3413bded6e91ab88182edf2
Author: Justin Suess <utilityemal77@gmail.com>

media: rc: sunxi-cir: add support for the A523

This patch adds support for the A523 (sun55i) SoC to the sunxi-cir
driver. It introduces support for programming pulse polarities and a
parameterized sample clock divider required by the newer IP revision.

> diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> --- a/drivers/media/rc/sunxi-cir.c
> +++ b/drivers/media/rc/sunxi-cir.c

[ ... ]

> @@ -140,17 +165,19 @@ static irqreturn_t sunxi_ir_irq(int irqno, void *dev_id)
>  
>  /* Convert idle threshold to usec */
> -static unsigned int sunxi_ithr_to_usec(unsigned int base_clk, unsigned int ithr)
> +static unsigned int sunxi_ithr_to_usec(unsigned int base_clk, unsigned int div,
> +				       unsigned int ithr)
>  {
>  	return DIV_ROUND_CLOSEST(USEC_PER_SEC * (ithr + 1),
> -				 base_clk / (128 * 64));
> +				 base_clk / (128 * div));

[Severity: Medium]
This is a pre-existing issue, but does this calculation risk a division by
zero panic during device probe?

If the clock provider rate evaluates to a value smaller than 128 * div,
base_clk / (128 * div) evaluates to 0. This resulting zero is then passed
as the divisor to DIV_ROUND_CLOSEST(), which would cause a panic.

>  }
>  
>  /* Convert usec to idle threshold */
> -static unsigned int sunxi_usec_to_ithr(unsigned int base_clk, unsigned int usec)
> +static unsigned int sunxi_usec_to_ithr(unsigned int base_clk, unsigned int div,
> +				       unsigned int usec)
>  {
>  	/* make sure we don't end up with a timeout less than requested */
> -	return DIV_ROUND_UP((base_clk / (128 * 64)) * usec,  USEC_PER_SEC) - 1;
> +	return DIV_ROUND_UP((base_clk / (128 * div)) * usec,  USEC_PER_SEC) - 1;
>  }
>  
>  static int sunxi_ir_set_timeout(struct rc_dev *rc_dev, unsigned int timeout)
>  {
>  	struct sunxi_ir *ir = rc_dev->priv;
>  	unsigned int base_clk = clk_get_rate(ir->clk);
>  
> -	unsigned int ithr = sunxi_usec_to_ithr(base_clk, timeout);
> +	unsigned int ithr = sunxi_usec_to_ithr(base_clk, ir->sample_divisor,
> +					       timeout);
>  
>  	dev_dbg(rc_dev->dev.parent, "setting idle threshold to %u\n", ithr);
>  
> -	/* Set noise threshold and idle threshold */
> -	writel(REG_CIR_NTHR(SUNXI_IR_RXNOISE) | REG_CIR_ITHR(ithr),
> +	/* Set sample clock divider, noise threshold and idle threshold */
> +	writel(REG_CIR_SDIV(ir->sample_div_sel) |
> +	       REG_CIR_NTHR(SUNXI_IR_RXNOISE) | REG_CIR_ITHR(ithr),
>  	       ir->base + SUNXI_IR_CIR_REG);

[Severity: High]
This is a pre-existing issue, but does the rounding mismatch between
sunxi_ithr_to_usec() and sunxi_usec_to_ithr() cause the maximum timeout
to wrap around to 0?

Because sunxi_ithr_to_usec() uses DIV_ROUND_CLOSEST, it can round the
fractional value up when calculating the maximum timeout. If userspace
requests this maximum timeout, sunxi_usec_to_ithr() uses DIV_ROUND_UP,
which pushes the resulting threshold over the maximum of 255 to 256.

Since REG_CIR_ITHR masks the value to 8 bits, passing 256 wraps it to 0
when written to the hardware, which would immediately break IR reception.

[ ... ]

> @@ -325,10 +363,10 @@ static int sunxi_ir_probe(struct platform_device *pdev)
>  	ir->rc->dev.parent = dev;
>  	ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
>  	/* Frequency after IR internal divider with sample period in us */
> -	ir->rc->rx_resolution = (USEC_PER_SEC / (b_clk_freq / 64));
> +	ir->rc->rx_resolution = (USEC_PER_SEC / (b_clk_freq / ir->sample_divisor));

[Severity: Medium]
This is a pre-existing issue, but does this calculation introduce timing
inaccuracies by using the DT-requested clock frequency instead of the
actual clock rate?

If the clock provider rounds the requested rate set earlier in
sunxi_ir_probe(), using the raw b_clk_freq here instead of
clk_get_rate(ir->clk) means the software's sample period drifts from the
actual hardware timing.

[Severity: High]
This is a pre-existing issue, but does the registration order in
sunxi_ir_probe() create a race condition and potential Use-After-Free?

Looking later in sunxi_ir_probe(), rc_register_device() is called before
sunxi_ir_hw_init() enables the hardware clocks:

sunxi_ir_probe() {
    ...
    ret = rc_register_device(ir->rc);
    ...
    ret = sunxi_ir_hw_init(dev);
    ...
}

If userspace accesses the device via ioctl before the hardware clocks are
enabled, it will access clock-gated memory and cause a bus fault.

Additionally, if probe fails during hardware initialization or IRQ request,
it branches to the exit_free_dev label:

exit_free_dev:
    rc_free_device(ir->rc);

This only calls rc_free_device() without calling rc_unregister_device()
first. The device remains exposed to userspace while its backing driver
data is freed, which could lead to a Use-After-Free memory corruption.

>  	ir->rc->timeout = IR_DEFAULT_TIMEOUT;
> -	ir->rc->min_timeout = sunxi_ithr_to_usec(b_clk_freq, 0);
> -	ir->rc->max_timeout = sunxi_ithr_to_usec(b_clk_freq, 255);
> +	ir->rc->min_timeout = sunxi_ithr_to_usec(b_clk_freq, ir->sample_divisor, 0);
> +	ir->rc->max_timeout = sunxi_ithr_to_usec(b_clk_freq, ir->sample_divisor, 255);
>  	ir->rc->s_timeout = sunxi_ir_set_timeout;
>  	ir->rc->driver_name = SUNXI_IR_DEV;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702214750.3428694-1-utilityemal77@gmail.com?part=2