Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 26 Aug 2022 18:50:54 +0300 | Subject | Re: [PATCH 3/3] serial: Add kserial_rs485 to avoid wasted space due to .padding |
| |
On Fri, Aug 26, 2022 at 5:51 PM Ilpo Järvinen <ilpo.jarvinen@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
| |