| 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: <20161123171302.GD19856@linaro.org>
Date: Wed, 23 Nov 2016 10:13:02 -0700
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Sascha Hauer <s.hauer@...gutronix.de>
Cc: linux-serial@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
kernel@...gutronix.de
Subject: Re: [PATCH 1/4] serial: core: Add LED trigger support
On Wed, Nov 23, 2016 at 11:01:03AM +0100, Sascha Hauer wrote:
> With this patch the serial core provides LED triggers for RX and TX.
>
> As the serial core layer does not know when the hardware actually sends
> or receives characters, this needs help from the UART drivers. The
> LED triggers are registered in uart_add_led_triggers() called from
> the UART drivers which want to support LED triggers. All the driver
> has to do then is to call uart_led_trigger_[tx|rx] to indicate
> activite.
Hello Sascha,
>
> Signed-off-by: Sascha Hauer <s.hauer@...gutronix.de>
> ---
> drivers/tty/serial/serial_core.c | 73 ++++++++++++++++++++++++++++++++++++++++
> include/linux/serial_core.h | 10 ++++++
> 2 files changed, 83 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index f2303f3..3e8afb7 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -34,6 +34,7 @@
> #include <linux/serial_core.h>
> #include <linux/delay.h>
> #include <linux/mutex.h>
> +#include <linux/leds.h>
>
> #include <asm/irq.h>
> #include <asm/uaccess.h>
> @@ -2703,6 +2704,77 @@ static const struct attribute_group tty_dev_attr_group = {
> .attrs = tty_dev_attrs,
> };
>
> +void uart_led_trigger_tx(struct uart_port *uport)
> +{
> + unsigned long delay = 50;
> +
> + led_trigger_blink_oneshot(uport->led_trigger_tx, &delay, &delay, 0);
> +}
> +
> +void uart_led_trigger_rx(struct uart_port *uport)
> +{
> + unsigned long delay = 50;
> +
> + led_trigger_blink_oneshot(uport->led_trigger_rx, &delay, &delay, 0);
For both rx/tx the core constrains the delay_on/off along with the inversion.
Instead of adding the led_trigger_rx/tx and led_trigger_rx/tx_name to the
struct uart_port, wouldn't it be better to add a new struct led_trigger that
would encapsulate those along wit the on/off delay and the inversion?
That way those values could be communicated to the core at registration time
instead of hard-coding things.
> +}
> +
> +/**
> + * uart_add_led_triggers - register LED triggers for a UART
> + * @drv: pointer to the uart low level driver structure for this port
> + * @uport: uart port structure to use for this port.
> + *
> + * Called by the driver to register LED triggers for a UART. It's the
> + * drivers responsibility to call uart_led_trigger_rx/tx on received
> + * and transmitted chars then.
> + */
> +int uart_add_led_triggers(struct uart_driver *drv, struct uart_port *uport)
> +{
> + int ret;
> +
> + if (!IS_ENABLED(CONFIG_LEDS_TRIGGERS))
> + return 0;
Since this is a public interface, checking for valid led_trigger_tx/rx before
moving on with the rest of the initialisation is probably a good idea.
Thanks,
Mathieu
> +
> + uport->led_trigger_tx_name = kasprintf(GFP_KERNEL, "%s%d-tx",
> + drv->dev_name, uport->line);
> + uport->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx",
> + drv->dev_name, uport->line);
> + if (!uport->led_trigger_tx_name || !uport->led_trigger_rx_name) {
> + ret = -ENOMEM;
> + goto err_alloc;
> + }
> +
> + led_trigger_register_simple(uport->led_trigger_tx_name,
> + &uport->led_trigger_tx);
> + led_trigger_register_simple(uport->led_trigger_rx_name,
> + &uport->led_trigger_rx);
> +
> + return 0;
> +
> +err_alloc:
> + kfree(uport->led_trigger_tx_name);
> + kfree(uport->led_trigger_rx_name);
> +
> + return ret;
> +}
> +
> +/**
> + * uart_remove_led_triggers - remove LED triggers
> + * @drv: pointer to the uart low level driver structure for this port
> + * @uport: uart port structure to use for this port.
> + *
> + * Remove LED triggers previously registered with uart_add_led_triggers
> + */
> +void uart_remove_led_triggers(struct uart_port *uport)
> +{
> + if (uport->led_trigger_rx)
> + led_trigger_unregister_simple(uport->led_trigger_rx);
> + kfree(uport->led_trigger_rx_name);
> +
> + if (uport->led_trigger_tx)
> + led_trigger_unregister_simple(uport->led_trigger_tx);
> + kfree(uport->led_trigger_tx_name);
> +}
> +
> /**
> * uart_add_one_port - attach a driver-defined port structure
> * @drv: pointer to the uart low level driver structure for this port
> @@ -2872,6 +2944,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> WARN_ON(atomic_dec_return(&state->refcount) < 0);
> wait_event(state->remove_wait, !atomic_read(&state->refcount));
> state->uart_port = NULL;
> +
> mutex_unlock(&port->mutex);
> out:
> mutex_unlock(&port_mutex);
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 3442014..1b0a169 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -29,6 +29,7 @@
> #include <linux/tty.h>
> #include <linux/mutex.h>
> #include <linux/sysrq.h>
> +#include <linux/leds.h>
> #include <uapi/linux/serial_core.h>
>
> #ifdef CONFIG_SERIAL_CORE_CONSOLE
> @@ -249,6 +250,10 @@ struct uart_port {
> const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
> struct serial_rs485 rs485;
> void *private_data; /* generic platform data pointer */
> + struct led_trigger *led_trigger_rx;
> + char *led_trigger_rx_name;
> + struct led_trigger *led_trigger_tx;
> + char *led_trigger_tx_name;
> };
>
> static inline int serial_port_in(struct uart_port *up, int offset)
> @@ -392,6 +397,11 @@ void uart_console_write(struct uart_port *port, const char *s,
> unsigned int count,
> void (*putchar)(struct uart_port *, int));
>
> +int uart_add_led_triggers(struct uart_driver *drv, struct uart_port *uport);
> +void uart_remove_led_triggers(struct uart_port *uport);
> +void uart_led_trigger_tx(struct uart_port *port);
> +void uart_led_trigger_rx(struct uart_port *port);
> +
> /*
> * Port/driver registration/removal
> */
> --
> 2.10.2
>
Powered by blists - more mailing lists