linux-kernel - Re: [PATCH] serial: sc16is7xx: Add polling feature if no IRQ usage possible

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2024121931-turbojet-preamble-b520@gregkh>
Date: Thu, 19 Dec 2024 09:56:17 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Andre Werner <andre.werner@...tec-electronic.com>
Cc: jirislaby@...nel.org, hvilleneuve@...onoff.com, andy@...nel.org,
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	lech.perczak@...lingroup.com
Subject: Re: [PATCH] serial: sc16is7xx: Add polling feature if no IRQ usage
 possible

On Thu, Dec 19, 2024 at 09:46:38AM +0100, Andre Werner wrote:
> Fall back to polling mode if no interrupt is configured because not
> possible. If "interrupts" property is missing in devicetree the driver
> uses a delayed worker to pull state of interrupt status registers.
> 
> Signed-off-by: Andre Werner <andre.werner@...tec-electronic.com>
> ---
> This driver was tested on Linux 5.10. We had a custom board that was not
> able to connect the interrupt port. Only I2C was available.

Could you not test this on the latest tree?  5.10 is _VERY_ old now.

> ---
> ---
>  drivers/tty/serial/sc16is7xx.c | 40 ++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index a3093e09309f..31962fdca178 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -314,6 +314,7 @@
>  #define SC16IS7XX_FIFO_SIZE		(64)
>  #define SC16IS7XX_GPIOS_PER_BANK	4
>  
> +#define SC16IS7XX_POLL_PERIOD 10 /*ms*/

Please use a tab here.

>  #define SC16IS7XX_RECONF_MD		BIT(0)
>  #define SC16IS7XX_RECONF_IER		BIT(1)
>  #define SC16IS7XX_RECONF_RS485		BIT(2)
> @@ -348,6 +349,9 @@ struct sc16is7xx_port {
>  	u8				mctrl_mask;
>  	struct kthread_worker		kworker;
>  	struct task_struct		*kworker_task;
> +	struct kthread_delayed_work	poll_work;
> +	bool polling;
> +	bool shutdown;
>  	struct sc16is7xx_one		p[];
>  };
>  
> @@ -861,6 +865,19 @@ static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static void sc16is7xx_transmission_poll(struct kthread_work *work)
> +{
> +	struct sc16is7xx_port *s = container_of(work, struct sc16is7xx_port, poll_work.work);
> +
> +	/* Resuse standard IRQ handler. Interrupt ID is unused in this context. */
> +	sc16is7xx_irq(0, s);
> +
> +	/* setup delay based on SC16IS7XX_POLL_PERIOD */
> +	if (!s->shutdown)
> +		kthread_queue_delayed_work(&s->kworker, &s->poll_work,
> +					   msecs_to_jiffies(SC16IS7XX_POLL_PERIOD));
> +}
> +
>  static void sc16is7xx_tx_proc(struct kthread_work *ws)
>  {
>  	struct uart_port *port = &(to_sc16is7xx_one(ws, tx_work)->port);
> @@ -1149,6 +1166,7 @@ static int sc16is7xx_config_rs485(struct uart_port *port, struct ktermios *termi
>  static int sc16is7xx_startup(struct uart_port *port)
>  {
>  	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
> +	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
>  	unsigned int val;
>  	unsigned long flags;
>  
> @@ -1210,6 +1228,11 @@ static int sc16is7xx_startup(struct uart_port *port)
>  	uart_port_lock_irqsave(port, &flags);
>  	sc16is7xx_enable_ms(port);
>  	uart_port_unlock_irqrestore(port, flags);
> +	if (s->polling) {
> +		s->shutdown = false;
> +		kthread_queue_delayed_work(&s->kworker, &s->poll_work,
> +					   msecs_to_jiffies(SC16IS7XX_POLL_PERIOD));
> +	}
>  
>  	return 0;
>  }
> @@ -1232,6 +1255,10 @@ static void sc16is7xx_shutdown(struct uart_port *port)
>  
>  	sc16is7xx_power(port, 0);
>  
> +	if (s->polling) {
> +		s->shutdown = true;
> +		kthread_cancel_delayed_work_sync(&s->poll_work);
> +	}
>  	kthread_flush_worker(&s->kworker);
>  }
>  
> @@ -1537,7 +1564,13 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  
>  	/* Always ask for fixed clock rate from a property. */
>  	device_property_read_u32(dev, "clock-frequency", &uartclk);
> +	s->polling = !device_property_present(dev, "interrupts");
>  
> +	if (s->polling) {
> +		dev_warn(dev,
> +			 "No interrupt definition found. Falling back to polling mode.\n");

What is a user supposed to do with this message?  And why would a device
NOT have any interrupts?  This feels like it is just going to pound on
the device and cause a lot of power drain for just a simple little uart.

Why can't your system provide a valid irq line?

thanks,

greg k-h