linux-kernel - Re: [PATCH v2] Serial: silabs si4455 serial driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 10 Dec 2020 20:03:22 +0100
From:   'Greg Kroah-Hartman' <gregkh@...uxfoundation.org>
To:     József Horváth <info@...istro.hu>
Cc:     'Rob Herring' <robh+dt@...nel.org>,
        'Jiri Slaby' <jirislaby@...nel.org>,
        linux-serial@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] Serial: silabs si4455 serial driver

On Thu, Dec 10, 2020 at 05:04:46PM +0000, József Horváth wrote:
> This is a serial port driver for
> Silicon Labs Si4455 Sub-GHz transciver.
> 
> Signed-off-by: József Horváth <info@...istro.hu>
> ---
>  .../bindings/serial/silabs,si4455.yaml        |   53 +
>  MAINTAINERS                                   |    7 +
>  drivers/tty/serial/Kconfig                    |    8 +
>  drivers/tty/serial/Makefile                   |    1 +
>  drivers/tty/serial/si4455.c                   | 1235 +++++++++++++++++
>  drivers/tty/serial/si4455_api.h               |   56 +

First thing, a single .c file should not need a .h file.

But then I looked at the .h file and see:

> --- /dev/null
> +++ b/drivers/tty/serial/si4455_api.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright (C) 2020 József Horváth <info@...istro.hu>
> + *
> + */
> +#ifndef SI4455_API_H_
> +#define SI4455_API_H_
> +
> +struct si4455_iocbuff {
> +	uint32_t length;
> +	uint8_t	data[4096];

If you do have an ioctl, use proper data types.  These are not the
correct ones (hint, __u32 and __u8).

> +};
> +
> +#define BASE_TTYIOC_PRIVATE		0xA0
> +/* Set EZConfig.
> + * After this ioctl call, the driver restarts the si4455,
> + * then apply the new configuration and patch.
> + */
> +#define SI4455_IOC_SEZC		_IOW('T', \
> +				     BASE_TTYIOC_PRIVATE + 0x01, \
> +				     struct si4455_iocbuff)

Why does a serial driver have private ioctls?  Please no, don't do that.
Implement the basic serial driver first, and then we can talk about
"custom" configurations and the like, using the correct apis.

thanks,

greg k-h