[PATCH 3/3] serial: Add kserial_rs485 to avoid wasted space due to .padding

[Andy Shevchenko]

On Fri, Aug 26, 2022 at 5:51 PM Ilpo Järvinen
<ilpo.jarvinen at linux.intel.com> 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