Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220826144629.11507-1-ilpo.jarvinen@linux.intel.com> <20220826144629.11507-4-ilpo.jarvinen@linux.intel.com>
In-Reply-To: <20220826144629.11507-4-ilpo.jarvinen@linux.intel.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Fri, 26 Aug 2022 18:50:54 +0300
Message-ID: <CAHp75Vc4NfZE6DxFnfeAS9fxnZHpxMjacHy1TsG8ib+FiCqFLQ@mail.gmail.com>
Subject: Re: [PATCH 3/3] serial: Add kserial_rs485 to avoid wasted space due
 to .padding
To:     =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Vladimir Zapolskiy <vz@mleia.com>,
        Russell King <linux@armlinux.org.uk>,
        Richard Genoud <richard.genoud@gmail.com>,
        Nicolas Ferre <nicolas.ferre@microchip.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Claudiu Beznea <claudiu.beznea@microchip.com>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        Alexandre Torgue <alexandre.torgue@foss.st.com>,
        Linux Documentation List <linux-doc@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
        linux-stm32@st-md-mailman.stormreply.com,
        Lino Sanfilippo <LinoSanfilippo@gmx.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Aug 26, 2022 at 5:51 PM Ilpo J=C3=A4rvinen
<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 =3D *((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) =3D *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 (exc=
ept 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 (millisecond=
s) */
> +       __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 paddi=
ng.

struct kserial_rs485

--=20
With Best Regards,
Andy Shevchenko