| 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
| ||
|
Date: Fri, 11 Dec 2020 13:09:35 +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 Fri, Dec 11, 2020 at 09:18:24AM +0000, József Horváth wrote:
> On Fri, Dec 11, 2020 at 09:43:31AM +0100, 'Greg Kroah-Hartman' wrote:
> > On Fri, Dec 11, 2020 at 08:16:34AM +0000, József Horváth wrote:
> > > On Fri, Dec 11, 2020 at 08:33:17AM +0100, 'Greg Kroah-Hartman' wrote:
> > > > On Fri, Dec 11, 2020 at 06:37:52AM +0000, József Horváth wrote:
> > > > > On Fri, Dec 11, 2020 at 07:20:41AM +0100, 'Greg Kroah-Hartman' wrote:
> > > > > > On Fri, Dec 11, 2020 at 06:09:43AM +0000, József Horváth wrote:
> > > > > > > On Fri, Dec 11, 2020 at 06:50:58AM +0100, 'Greg Kroah-Hartman' wrote:
> > > > > > > > On Thu, Dec 10, 2020 at 07:46:25PM +0000, József Horváth wrote:
> > > > > > > > > On Thu, Dec 10, 2020 at 08:03:22PM +0100, 'Greg Kroah-Hartman' wrote:
> > > > > > > > > > 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.
> > > > > > > > > > > +
> > > > > > > > > > > +#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.
> > > > > > > > >
> > > > > > > > > I checked the ioctl.h and serial_core.h, but I not found any similar definition, like BASE_VIDIOC_PRIVATE in videodev2.h.
> > > > > > > > > In this case the name of macro BASE_TTYIOC_PRIVATE means the base value of special ioctl commands owned by this driver.
> > > > > > > >
> > > > > > > > My point is, a serial driver should NOT have any custom ioctls.
> > > > > > > >
> > > > > > > > > I can change it to BASE_TTYIOC or SI4455_IOC_BASE
> > > > > > > > >
> > > > > > > > > > Implement the basic serial driver first, and then we can talk about
> > > > > > > > > > "custom" configurations and the like, using the correct apis.
> > > > > > > > >
> > > > > > > > > Without the SI4455_IOC_SEZC call, the driver can't configure the Si4455 and not working at all.
> > > > > > > > > The cofiguration for interface is provided by user for application.
> > > > > > > >
> > > > > > > > That is what a device tree is for, to configure the device to have the
> > > > > > > > correct system configuration, why can't that be the same here?
> > > > > > > >
> > > > > > > > > It contains the base frequency, channel spacing, modulation, and a lot
> > > > > > > > > of more stuff, and generated by Silicon Labs Wireless Development
> > > > > > > > > Suite.
> > > > > > > > > The generated configuration is in a non public(compressed,
> > > > > > > > > encrypted...who knows) format, so without this the driver can't
> > > > > > > > > provide configuration parameters to Si4455.
> > > > > > > >
> > > > > > > > So we have to take a "custom" userspace blob and send it to the device
> > > > > > > > to configure it properly? Like Jiri said, sounds like firmware, so just
> > > > > > > > use that interface instead.
> > > > > > >
> > > > > > > I checked Jiri's suggestion, and it is a good solution to replace SI4455_IOC_SEZC(configuration) and SI4455_IOC_SEZP(firmware patch).
> > > > > > > I can move SI4455_IOC_SSIZ(package size) to device tree property.
> > > > > > >
> > > > > > > Maybe you have good suggestion for the following:
> > > > > > > SI4455_IOC_STXC -> Radio transmit channel index. It is a real use case to control this parameter by user at runtime.
> > > > > > > SI4455_IOC_SRXC -> Radio receive channel index. It is a real use case to control this parameter by user at runtime.
> > > > > >
> > > > > > These are not serial port things, why would a serial port care about
> > > > > > these?
> > > > >
> > > > > You are right, these are not regular serial port things, but this device is not a regular uart, it is a sub-GHz transciever, digital radio.
> > > > > This driver tries to represent it as a serial port to user.
> > > >
> > > > Is that the correct representation to be using here? Why not act like a
> > > > proper radio device instead? That way you get to use the normal kernel
> > > > apis for radio devices.
> > >
> > > In my mind it is absolute a serial device by the application.
> >
> > What is the application? Traditionally serial ports don't need radio signals :)
>
> The application is connecting newly developed sensors(with only rf interface) and legacy sensors(with regular serial communication over rs-485 with modbus) keeping the legacy user software.
>
> User sw [Java]
> <-> /dev/ttyXXX
> <-> si4455[driver]
> <-> si4455[hardware]
> <---air---> new device[si4455+ARM Cortex-M0] 1
> +-> new device[si4455+ARM Cortex-M0] 2
> +-> new device[si4455+ARM Cortex-M0] n
> +-> gateway[si4455+ARM Cortex-M0]<---RS485--> Legacy device 1
> +-> Legacy device 2
> +-> Legacy device n
If these are "sensors", why are you using a tty interface at all, and
not just using the correct iio interface for them?
thanks,
greg k-h
Powered by blists - more mailing lists