On Fri, Aug 26, 2022 at 5:51 PM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>
> Struct serial_rs485 has a .padding field to make uapi updates easier.
The struct
> It wastes space, however. Create struct kserial_rs485 which is a kerner
> counterpart w/o padding.
>
> kernel_serial_rs485_to_user_rs485()'s rs485 can now become const as
> padding is dealt within the local variable.
...
> -static int user_rs485_to_kernel_serial_rs485(struct serial_rs485 *rs485,
> +static int user_rs485_to_kernel_serial_rs485(struct kserial_rs485 *rs485,
> const struct serial_rs485 __user *rs485_user)
> {
> - if (copy_from_user(rs485, rs485_user, sizeof(*rs485)))
> + struct serial_rs485 rs485_uapi;
> +
> + if (copy_from_user(&rs485_uapi, rs485_user, sizeof(*rs485)))
> return -EFAULT;
> + *rs485 = *((struct kserial_rs485 *)&rs485_uapi);
So with all assets we have we can be sure that on BE64 / BE32 machines
this will be flawless. Is this assumption correct?
> return 0;
> }
...
> static int kernel_serial_rs485_to_user_rs485(struct serial_rs485 __user *rs485_user,
> - struct serial_rs485 *rs485)
> + const struct kserial_rs485 *rs485)
> {
> + struct serial_rs485 rs485_uapi;
> + *((struct kserial_rs485 *)&rs485_uapi) = *rs485;
Ditto.
+ Blank line?
> /* Return clean padding area to userspace */
> - memset(rs485->padding0, 0, sizeof(rs485->padding0));
> - memset(rs485->padding1, 0, sizeof(rs485->padding1));
> + memset(rs485_uapi.padding0, 0, sizeof(rs485_uapi.padding0));
> + memset(rs485_uapi.padding1, 0, sizeof(rs485_uapi.padding1));
>
> - if (copy_to_user(rs485_user, rs485, sizeof(*rs485)))
> + if (copy_to_user(rs485_user, &rs485_uapi, sizeof(rs485_uapi)))
> return -EFAULT;
>
> return 0;
...
> +/* Compile-time asserts for kserial_rs485 and serial_rs485 equality (except padding) */
struct kserial_rs485
struct serial_rs485
(rationale: standard representation in text / comments and be a link
in case if this is converted to kernel doc)
...
> +/*
> + * Must match with serial_rs485 in include/uapi/linux/serial.h excluding the
Ditto.
> + * padding.
> + */
> +struct kserial_rs485 {
> + __u32 flags; /* RS485 feature flags */
> + __u32 delay_rts_before_send; /* Delay before send (milliseconds) */
> + __u32 delay_rts_after_send; /* Delay after send (milliseconds) */
> + struct {
> + __u8 addr_recv;
> + __u8 addr_dest;
> + };
Btw, can't we convert them to kernel doc?
> +};
...
> + * There's kernel counterpart kserial_rs485 of this struct without padding.
struct kserial_rs485
--
With Best Regards,
Andy Shevchenko
