Re: [PATCH v4 2/2] media: rc: ir-spi: constrain carrier frequency

[Sean Young]

On Wed, Jun 11, 2025 at 02:23:44PM +0300, Cosmin Tanislav wrote:
> Carrier frequency is currently unconstrained, allowing the SPI transfer
> to be allocated and filled only for it to be later rejected by the SPI
> controller since the frequency is too large.
> 
> Add a check to constrain the carrier frequency inside
> ir_spi_set_tx_carrier().
> 
> Also, move the number of bits per pulse to a macro since it is not used
> in multiple places.
> 
> Signed-off-by: Cosmin Tanislav <demonsingur@xxxxxxxxx>
> ---
>  drivers/media/rc/ir-spi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/ir-spi.c b/drivers/media/rc/ir-spi.c
> index 50e30e2fae22..bf731204c81e 100644
> --- a/drivers/media/rc/ir-spi.c
> +++ b/drivers/media/rc/ir-spi.c
> @@ -21,6 +21,7 @@
>  #define IR_SPI_DRIVER_NAME		"ir-spi"
>  
>  #define IR_SPI_DEFAULT_FREQUENCY	38000
> +#define IR_SPI_BITS_PER_PULSE		16
>  
>  struct ir_spi_data {
>  	u32 freq;
> @@ -70,7 +71,7 @@ static int ir_spi_tx(struct rc_dev *dev, unsigned int *buffer, unsigned int coun
>  
>  	memset(&xfer, 0, sizeof(xfer));
>  
> -	xfer.speed_hz = idata->freq * 16;
> +	xfer.speed_hz = idata->freq * IR_SPI_BITS_PER_PULSE;
>  	xfer.len = len * sizeof(*tx_buf);
>  	xfer.tx_buf = tx_buf;
>  
> @@ -98,6 +99,9 @@ static int ir_spi_set_tx_carrier(struct rc_dev *dev, u32 carrier)
>  	if (!carrier)
>  		return -EINVAL;
>  
> +	if (carrier * IR_SPI_BITS_PER_PULSE > idata->spi->max_speed_hz)
> +		return -EINVAL;

Just a nitpick.

I think carrier * IR_SPI_BITS_PER_PULSE could overflow, and then the check
wouldn't work. It might be better to do:

	if (carrier > idata->spi->max_speed_hz / IR_SPI_BITS_PER_PULSE)

However since IR_SPI_BITS_PER_PULSE is 16, which is just a shift left by 4,
I don't think this can be abused in any useful way.


Sean

> +
>  	idata->freq = carrier;
>  
>  	return 0;
> -- 
> 2.49.0
>