| 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
| ||
|
Message-Id: <20250113095324.560a091d90642528ae820aae@hugovil.com>
Date: Mon, 13 Jan 2025 09:53:24 -0500
From: Hugo Villeneuve <hugo@...ovil.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Andre Werner <andre.werner@...tec-electronic.com>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"jirislaby@...nel.org" <jirislaby@...nel.org>, "hvilleneuve@...onoff.com"
<hvilleneuve@...onoff.com>, "andy@...nel.org" <andy@...nel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
"lech.perczak@...lingroup.com" <lech.perczak@...lingroup.com>,
"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
<conor+dt@...nel.org>, "robh@...nel.org" <robh@...nel.org>
Subject: Re: [PATCH v5 2/2] serial: sc16is7xx: Add polling mode if no IRQ
pin is available
On Fri, 10 Jan 2025 20:37:29 +0200
Andy Shevchenko <andy.shevchenko@...il.com> wrote:
> пʼятниця, 10 січня 2025 р. Andre Werner <andre.werner@...tec-electronic.com>
> пише:
>
> > Fall back to polling mode if no interrupt is configured because there
> > is no possibility to connect the interrupt pin.
> > If "interrupts" property is missing in devicetree the driver
> > uses a delayed worker to pull the state of interrupt status registers.
> >
> > Signed-off-by: Andre Werner <andre.werner@...tec-electronic.com>
> > ---
> > V2:
> > - Change warning for polling mode to debug log entry
> > - Correct typo: Resuse -> Reuse
> > - Format define with missing tabs for SC16IS7XX_POLL_PERIOD
> > - Format struct declaration sc16is7xx_one_config with missing tabs for
> > polling and shutdown
> > - Adapt dtbinding with new polling feature
> > V3:
> > - Use suffix with units and drop a comment SC16IS7XX_POLL_PERIOD_MS. Sorry
> > for that miss.
> > - Make Kernel lowercase.
> > V4:
> > - Reword commit messages for better understanding.
> > - Remove 'shutdown' property for canceling delayed worker.
> > - Rename worker function: sc16is7xx_transmission_poll ->
> > sc16is7xx_poll_proc
> > - Unify argument for worker functions: kthread_work *work -> kthread_work
> > *ws
> > V5:
> > - Replace of_property check with IRQ number check to set polling
> > property. This will add support
>
>
> >
> It other way around, i.e. it won’t break the existing support of
> interrupt-driven non-DT setups.
>
>
> > for usage without device tree
> > definitions. Thanks for that advice.
> > - Add blank line es requested.
> > ---
> > drivers/tty/serial/sc16is7xx.c | 37 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 37 insertions(+)
> >
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/
> > sc16is7xx.c
> > index a3093e09309f..7b51cdc274fd 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_MS 10
> > #define SC16IS7XX_RECONF_MD BIT(0)
> > #define SC16IS7XX_RECONF_IER BIT(1)
> > #define SC16IS7XX_RECONF_RS485 BIT(2)
> > @@ -348,6 +349,8 @@ struct sc16is7xx_port {
> > u8 mctrl_mask;
> > struct kthread_worker kworker;
> > struct task_struct *kworker_task;
> > + struct kthread_delayed_work poll_work;
> > + bool polling;
> > struct sc16is7xx_one p[];
> > };
> >
> > @@ -861,6 +864,18 @@ static irqreturn_t sc16is7xx_irq(int irq, void
> > *dev_id)
> > return IRQ_HANDLED;
> > }
> >
> > +static void sc16is7xx_poll_proc(struct kthread_work *ws)
> > +{
> > + struct sc16is7xx_port *s = container_of(ws, struct sc16is7xx_port,
> > poll_work.work);
> > +
> > + /* Reuse standard IRQ handler. Interrupt ID is unused in this
> > context. */
> > + sc16is7xx_irq(0, s);
> > +
> > + /* Setup delay based on SC16IS7XX_POLL_PERIOD_MS */
> > + kthread_queue_delayed_work(&s->kworker, &s->poll_work,
> > + msecs_to_jiffies(SC16IS7XX_
> > POLL_PERIOD_MS));
> > +}
> > +
> > static void sc16is7xx_tx_proc(struct kthread_work *ws)
> > {
> > struct uart_port *port = &(to_sc16is7xx_one(ws, tx_work)->port);
> > @@ -1149,6 +1164,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;
> >
> > @@ -1211,6 +1227,10 @@ static int sc16is7xx_startup(struct uart_port *port)
> > sc16is7xx_enable_ms(port);
> > uart_port_unlock_irqrestore(port, flags);
> >
> > + if (s->polling)
> > + kthread_queue_delayed_work(&s->kworker, &s->poll_work,
> > + msecs_to_jiffies(SC16IS7XX_
> > POLL_PERIOD_MS));
> > +
> > return 0;
> > }
> >
> > @@ -1232,6 +1252,9 @@ static void sc16is7xx_shutdown(struct uart_port
> > *port)
> >
> > sc16is7xx_power(port, 0);
> >
> > + if (s->polling)
> > + kthread_cancel_delayed_work_sync(&s->poll_work);
> > +
> > kthread_flush_worker(&s->kworker);
> > }
> >
> > @@ -1538,6 +1561,11 @@ 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 = !!irq;
>
>
> This is incorrect, you should check for positive numbers only for IRQ
> support and for the rest otherwise. I do not see that frameworks guarantee
> this never be negative.
Hi Greg,
I just noticed that v5 of this patch is now in the tty-testing
branch. It should not, as there is currently a v6 fixing it, and
possibly a v7 coming.
Hugo.
> > + if (s->polling)
> > + dev_dbg(dev,
> > + "No interrupt pin definition, falling back to
> > polling mode\n");
> > +
> > s->clk = devm_clk_get_optional(dev, NULL);
> > if (IS_ERR(s->clk))
> > return PTR_ERR(s->clk);
> > @@ -1665,6 +1693,12 @@ int sc16is7xx_probe(struct device *dev, const
> > struct sc16is7xx_devtype *devtype,
> > goto out_ports;
> > #endif
> >
> > + if (s->polling) {
> > + /* Initialize kernel thread for polling */
> > + kthread_init_delayed_work(&s->poll_work,
> > sc16is7xx_poll_proc);
> > + return 0;
>
>
>
> > + }
> > +
> > /*
> > * Setup interrupt. We first try to acquire the IRQ line as level
> > IRQ.
> > * If that succeeds, we can allow sharing the interrupt as well.
> > @@ -1724,6 +1758,9 @@ void sc16is7xx_remove(struct device *dev)
> > sc16is7xx_power(&s->p[i].port, 0);
> > }
> >
> > + if (s->polling)
> > + kthread_cancel_delayed_work_sync(&s->poll_work);
> > +
> > kthread_flush_worker(&s->kworker);
> > kthread_stop(s->kworker_task);
> >
> > --
> > 2.47.1
> >
> >
--
Hugo Villeneuve
Powered by blists - more mailing lists