The following drivers: 8250_core, atmel_serial, max310x, mcf, omap-serial
and sci16is7xx implement code to handle RS485 ioctls.
In order to avoid code duplication, we implement a simple ioctl handler
on the serial_core layer.
This handler can be used by all the other drivers instead of duplicating
code.
Until this is the only RS485 ioctl handler, it will try first the
rs485_config callback and if it is not present it will call the driver
specific ioctl.
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jiri Slaby <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
drivers/tty/serial/serial_core.c | 45 ++++++++++++++++++++++++++++++++++++++++
include/linux/serial_core.h | 3 +++
2 files changed, 48 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index df3a8c7..6015fd2 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1151,6 +1151,39 @@ static int uart_get_icount(struct tty_struct *tty,
return 0;
}
+static int uart_get_rs485_config(struct uart_port *port,
+ struct serial_rs485 __user *rs485)
+{
+ if (!port->rs485_config)
+ return -ENOIOCTLCMD;
+
+ if (copy_to_user(rs485, &port->rs485, sizeof(port->rs485)))
+ return -EFAULT;
+ return 0;
+}
+
+static int uart_set_rs485_config(struct uart_port *port,
+ struct serial_rs485 __user *rs485_user)
+{
+ struct serial_rs485 rs485;
+ int ret;
+
+ if (!port->rs485_config)
+ return -ENOIOCTLCMD;
+
+ if (copy_from_user(&rs485, rs485_user, sizeof(rs485_user)))
+ return -EFAULT;
+
+ ret = port->rs485_config(port, &rs485);
+ if (ret)
+ return ret;
+
+ if (copy_to_user(rs485_user, &port->rs485, sizeof(port->rs485)))
+ return -EFAULT;
+
+ return 0;
+}
+
/*
* Called via sys_ioctl. We can use spin_lock_irq() here.
*/
@@ -1218,6 +1251,18 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd,
* protected against the tty being hung up.
*/
switch (cmd) {
+ case TIOCGRS485:
+ ret = uart_get_rs485_config(state->uart_port, uarg);
+ break;
+
+ case TIOCSRS485:
+ ret = uart_set_rs485_config(state->uart_port, uarg);
+ break;
+ }
+ if (ret != -ENOIOCTLCMD)
+ goto out;
+
+ switch (cmd) {
case TIOCSERGETLSR: /* Get line status register */
ret = uart_get_lsr_info(tty, state, uarg);
break;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 21c2e05..60a4ebf 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -131,6 +131,8 @@ struct uart_port {
void (*pm)(struct uart_port *, unsigned int state,
unsigned int old);
void (*handle_break)(struct uart_port *);
+ int (*rs485_config)(struct uart_port *,
+ struct serial_rs485 *rs485);
unsigned int irq; /* irq number */
unsigned long irqflags; /* irq flags */
unsigned int uartclk; /* base uart clock */
@@ -214,6 +216,7 @@ struct uart_port {
unsigned char unused[2];
struct attribute_group *attr_group; /* port specific attributes */
const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
+ struct serial_rs485 rs485;
void *private_data; /* generic platform data pointer */
};
--
2.1.1
On Wed, 2014-10-08 at 21:57 +0200, Ricardo Ribalda Delgado wrote:
> The following drivers: 8250_core, atmel_serial, max310x, mcf, omap-serial
> and sci16is7xx implement code to handle RS485 ioctls.
>
> +static int uart_get_rs485_config(struct uart_port *port,
> + struct serial_rs485 __user *rs485)
> +{
> + if (!port->rs485_config)
> + return -ENOIOCTLCMD;
> +
> + if (copy_to_user(rs485, &port->rs485, sizeof(port->rs485)))
> + return -EFAULT;
> + return 0;
> +}
> +
> +static int uart_set_rs485_config(struct uart_port *port,
> + struct serial_rs485 __user *rs485_user)
> +{
> + struct serial_rs485 rs485;
> + int ret;
> +
> + if (!port->rs485_config)
> + return -ENOIOCTLCMD;
> +
> + if (copy_from_user(&rs485, rs485_user, sizeof(rs485_user)))
> + return -EFAULT;
> +
> + ret = port->rs485_config(port, &rs485);
> + if (ret)
> + return ret;
> +
> + if (copy_to_user(rs485_user, &port->rs485, sizeof(port->rs485)))
> + return -EFAULT;
> +
> + return 0;
> +
What is the locking between setting/getting/driver use of the config ?
This really needs a lock (termios sem I think is perhaps appropriate
given when the values are normally referenced).
Alan
Hello Alan
This patchset adds no extra locking features, if the drivers did not
implement a locking mechanism (and none did) there is chance of
conflict.
I can add a call to lock/unlock around uart_[gs]et_rs485_config. And
then, inside the drivers, use the lock when the structure is used.
I would prefer to add it as new patches in the patchset, so in case
this adds some bugs they can be bisected easily.
Thanks for your fast reply
On Wed, Oct 8, 2014 at 10:11 PM, Alan Cox <[email protected]> wrote:
> On Wed, 2014-10-08 at 21:57 +0200, Ricardo Ribalda Delgado wrote:
>> The following drivers: 8250_core, atmel_serial, max310x, mcf, omap-serial
>> and sci16is7xx implement code to handle RS485 ioctls.
>
>>
>> +static int uart_get_rs485_config(struct uart_port *port,
>> + struct serial_rs485 __user *rs485)
>> +{
>> + if (!port->rs485_config)
>> + return -ENOIOCTLCMD;
>> +
>> + if (copy_to_user(rs485, &port->rs485, sizeof(port->rs485)))
>> + return -EFAULT;
>> + return 0;
>> +}
>> +
>> +static int uart_set_rs485_config(struct uart_port *port,
>> + struct serial_rs485 __user *rs485_user)
>> +{
>> + struct serial_rs485 rs485;
>> + int ret;
>> +
>> + if (!port->rs485_config)
>> + return -ENOIOCTLCMD;
>> +
>> + if (copy_from_user(&rs485, rs485_user, sizeof(rs485_user)))
>> + return -EFAULT;
>> +
>> + ret = port->rs485_config(port, &rs485);
>> + if (ret)
>> + return ret;
>> +
>> + if (copy_to_user(rs485_user, &port->rs485, sizeof(port->rs485)))
>> + return -EFAULT;
>> +
>> + return 0;
>> +
>
> What is the locking between setting/getting/driver use of the config ?
> This really needs a lock (termios sem I think is perhaps appropriate
> given when the values are normally referenced).
>
> Alan
>
>
--
Ricardo Ribalda
On Wed, 2014-10-08 at 22:43 +0200, Ricardo Ribalda Delgado wrote:
> Hello Alan
>
> This patchset adds no extra locking features, if the drivers did not
> implement a locking mechanism (and none did) there is chance of
> conflict.
>
> I can add a call to lock/unlock around uart_[gs]et_rs485_config. And
> then, inside the drivers, use the lock when the structure is used.
>
> I would prefer to add it as new patches in the patchset, so in case
> this adds some bugs they can be bisected easily.
Makes sense.
Hello Alan
>
> What is the locking between setting/getting/driver use of the config ?
> This really needs a lock (termios sem I think is perhaps appropriate
> given when the values are normally referenced).
I tried implementing it with the sermios sem
((&(uart_port)->state->port.tty->termios_rwsem)), but some drivers
access the rs485 structure inside their irq handler. So I have see
options here
1) Protect the structure with uart_port->lock spinlock
2) Assume that an assignment is atomic on critical sections where I
cannot hold the rwsem.
I think 1) is more correct. Any issues that I continue in this
direction? Any better idea?
Thanks!
--
Ricardo Ribalda
On Thu, 2014-10-09 at 10:07 +0200, Ricardo Ribalda Delgado wrote:
> Hello Alan
>
> >
> > What is the locking between setting/getting/driver use of the config ?
> > This really needs a lock (termios sem I think is perhaps appropriate
> > given when the values are normally referenced).
>
> I tried implementing it with the sermios sem
> ((&(uart_port)->state->port.tty->termios_rwsem)), but some drivers
> access the rs485 structure inside their irq handler. So I have see
> options here
>
> 1) Protect the structure with uart_port->lock spinlock
> 2) Assume that an assignment is atomic on critical sections where I
> cannot hold the rwsem.
>
> I think 1) is more correct. Any issues that I continue in this
> direction? Any better idea?
For uart #1 sounds right to me too.
Alan
Hello Alan
After almost one month with no other comments I am planning to resend
the patchset including the lock patch. May I add your Reviewed or
Acked by to any of the patches?
Thanks :)
On Thu, Oct 9, 2014 at 4:16 PM, Alan Cox <[email protected]> wrote:
> On Thu, 2014-10-09 at 10:07 +0200, Ricardo Ribalda Delgado wrote:
>> Hello Alan
>>
>> >
>> > What is the locking between setting/getting/driver use of the config ?
>> > This really needs a lock (termios sem I think is perhaps appropriate
>> > given when the values are normally referenced).
>>
>> I tried implementing it with the sermios sem
>> ((&(uart_port)->state->port.tty->termios_rwsem)), but some drivers
>> access the rs485 structure inside their irq handler. So I have see
>> options here
>>
>> 1) Protect the structure with uart_port->lock spinlock
>> 2) Assume that an assignment is atomic on critical sections where I
>> cannot hold the rwsem.
>>
>> I think 1) is more correct. Any issues that I continue in this
>> direction? Any better idea?
>
> For uart #1 sounds right to me too.
>
> Alan
>
>
--
Ricardo Ribalda