| 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
| ||
|
Message-ID: <20210105150214.GA6031@dev>
Date: Tue, 5 Jan 2021 15:02:15 +0000
From: József Horváth <info@...istro.hu>
To: 'Greg Kroah-Hartman' <gregkh@...uxfoundation.org>
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 v7,1/2] Serial: silabs si4455 serial driver
On Tue, Jan 05, 2021 at 02:28:16PM +0100, 'Greg Kroah-Hartman' wrote:
> On Tue, Jan 05, 2021 at 10:29:25AM +0000, József Horváth wrote:
> > This is a serial port driver for
> > Silicon Labs Si4455 Sub-GHz transciver.
> >
> > The goal of this driver is to removing wires
> > between central(linux) device and remote serial devices/sensors,
> > but keeping the original user software.
> > It represents regular serial interface for the user space.
> >
> > Datasheet: https://www.silabs.com/documents/public/data-sheets/Si4455.pdf
> >
> > Guide: https://github.com/dministro/linux-serial-si4455
> >
> > Signed-off-by: Jozsef Horvath <info@...istro.hu>
> > ---
> >
> >
> > +config SERIAL_SI4455
> > + tristate "Si4455 support"
> > + depends on SPI
> > + select SERIAL_CORE
> > + help
> > + This driver is for Silicon Labs's Si4455 Sub-GHz transciver.
> > + Say 'Y' here if you wish to use it as serial port.
> > +
>
> No module name?
Sorry, I dont understand your question. Can you explain it?
>
> > endmenu
> > +#include <linux/string.h>
> > +#include <linux/firmware.h>
> > +#include <linux/timer.h>
> > +#ifdef CONFIG_DEBUG_FS
> > +#include <linux/debugfs.h>
> > +#endif
>
> No need for #ifdef for .h files.
>
Ok, its clear. I'll remove it from here and below.
> > +
> > +#define PORT_SI4455 1096
> > +#define SI4455_NAME "Si4455"
> > +#define SI4455_MAJOR 432
> > +#define SI4455_MINOR 567
>
> Where are these major/minor numbers being used and where did they come
> from? Why do you need them?
>
> > +struct si4455_port {
> > + struct uart_port port;
> > +#ifdef CONFIG_DEBUG_FS
> > + struct dentry *dbgfs_dir;
> > +#endif
>
> Do not put #ifdefs in .c code, you never need to check for this type of
> thing.
>
> > +static struct uart_driver si4455_uart = {
> > + .owner = THIS_MODULE,
> > + .driver_name = SI4455_NAME,
> > +#ifdef CONFIG_DEVFS_FS
> > + .dev_name = "ttySI%d",
>
> Looks like you are porting this from a _VERY_ old kernel. That config
> option went away 15+ years ago. Are you sure this works?
>
Ok, I'll remove it.
>
> > +#else
> > + .dev_name = "ttySI",
>
> Where did you get that name from?
>
This is my suggestion. I dont know the naming rules.
>
> > +static int si4455_begin_tx(struct uart_port *port, u32 channel, int length,
> > + u8 *data)
> > +{
> > + int ret = 0;
> > + struct si4455_int_status int_status = { 0 };
> > + struct si4455_fifo_info fifo_info = { 0 };
> > +
> > + dev_dbg(port->dev, "%s(%u, %u)\n", __func__, channel, length);
>
> No need for these types of debugging lines, just use ftrace.
>
> Please remove them, you have them in a few places (same for the end of
> functions.)
>
> > +static void si4455_null_void(struct uart_port *port)
> > +{
> > + /* Do nothing */
>
> Why do you need this???
I'll check this.
>
> > +#ifdef CONFIG_DEBUG_FS
>
> Again, no #ifdef needed.
>
> > +static int si4455_debugfs_init(struct device *dev)
> > +{
> > + struct si4455_port *s = dev_get_drvdata(dev);
> > + struct dentry *dbgfs_si_dir;
> > + struct dentry *dbgfs_partinfo_dir;
> > + struct dentry *dbgfs_entry;
> > +
> > + s->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
> > + if (IS_ERR(s->dbgfs_dir))
> > + return PTR_ERR(s->dbgfs_dir);
>
> No need to check any debugfs return value, just use it and move on.
>
> > +
> > + dbgfs_si_dir = debugfs_create_dir("si4455", s->dbgfs_dir);
> > + if (IS_ERR(dbgfs_si_dir))
> > + return PTR_ERR(dbgfs_si_dir);
> > +
> > + dbgfs_entry = debugfs_create_u32("cts_error_count", 0444,
> > + dbgfs_si_dir, &s->cts_error_count);
> > + if (IS_ERR(dbgfs_entry))
> > + return PTR_ERR(dbgfs_entry);
>
> Same for all of these, no need to check anything.
Ok, its clear.
>
> > +
> > + dbgfs_entry = debugfs_create_u32("tx_error_count", 0444,
> > + dbgfs_si_dir, &s->tx_error_count);
> > + if (IS_ERR(dbgfs_entry))
> > + return PTR_ERR(dbgfs_entry);
> > +
> > + dbgfs_partinfo_dir = debugfs_create_dir("partinfo", dbgfs_si_dir);
> > + if (IS_ERR(dbgfs_partinfo_dir))
> > + return PTR_ERR(dbgfs_partinfo_dir);
> > +
> > + dbgfs_entry = debugfs_create_u8("chip_rev", 0444,
> > + dbgfs_partinfo_dir,
> > + &s->part_info.chip_rev);
>
> Wait, did you even build this code? Does it work? It shouldn't, these
> debugfs calls have changed...
>
> I'm stopping reviewing here.
Working test systems:
- #1 - one Si4455 connected to spi1.2:
$ uname -r
4.19.66-v7+
$ ls -R /sys/kernel/debug/spi1.2/si4455
/sys/kernel/debug/spi1.2/si4455:
cts_error_count partinfo tx_error_count
/sys/kernel/debug/spi1.2/si4455/partinfo:
chip_rev part rom_id
$ ls /dev | grep ttySI
ttySI0
- #2 - one Si4455 connected to spi0.0 and an other to spi0.1:
$ uname -r
5.4.79-v7+
$ ls -R /sys/kernel/debug/spi0.0/si4455
/sys/kernel/debug/spi0.0/si4455:
cts_error_count partinfo tx_error_count
/sys/kernel/debug/spi0.0/si4455/partinfo:
chip_rev part rom_id
$ ls -R /sys/kernel/debug/spi0.1/si4455
/sys/kernel/debug/spi0.1/si4455:
cts_error_count partinfo tx_error_count
/sys/kernel/debug/spi0.1/si4455/partinfo:
chip_rev part rom_id
$ cat /sys/kernel/debug/spi0.0/si4455/partinfo/chip_rev
34 <- Its valid
$ ls /dev | grep ttySI
ttySI0
ttySI1
I made a short guide to using the interfaces, generating the firmware and a simple setup.
You can see it: https://github.com/dministro/linux-serial-si4455
I always test, compile, test, compile, test, test, test, checkpatch before sending my patch.
So my answer is yes, but you are right too :)
I found the answer just now, I compiled this for kernel v4.19.66 and v5.4.79 but not for v5.10.
Sorry for this, and thank you for suggestions.
>
> thanks,
>
> greg k-h
Üdvözlettel / Best regards:
József Horváth
Powered by blists - more mailing lists