[PATCH 0/7] tty: synclink_gt: mark as BROKEN

[PATCH 0/7] tty: synclink_gt: mark as BROKEN

[Jiri Slaby (SUSE)]

I did some cleanups in the driver (in this series), but then decided not
to go on and mark the driver as BROKEN instead. See the last patch for
explanation.

Feel free to take only the last patch. I don't assume anyone appears to
take care of the driver, so we will drop it sooner or later anyway. The
changes only demonstrate how unmaintained the driver is...

Jiri Slaby (SUSE) (7):
  tty: synclink_gt: convert CALC_REGADDR() macro to an inline
  tty: synclink_gt: drop global slgt_driver_name array
  tty: synclink_gt: define global strings as const strings
  tty: synclink_gt: drop info messages from init/exit functions
  tty: synclink_gt: use PCI_VDEVICE
  tty: synclink_gt: make default_params const
  tty: synclink_gt: mark as BROKEN

 drivers/tty/Kconfig       |  1 +
 drivers/tty/synclink_gt.c | 65 ++++++++++++++++-----------------------
 2 files changed, 28 insertions(+), 38 deletions(-)

-- 
2.41.0

[PATCH 1/7] tty: synclink_gt: convert CALC_REGADDR() macro to an inline

[Jiri Slaby (SUSE)]

It makes the code more readable and less error-prone as the result is
returned and not stored in a variable newly defined inside the macro.

Note that cast to 'unsigned long' and back to 'void *' was eliminated as
info->reg_addr is 'char *' already (so the addition is per bytes
already).

This nicely cleans up the callers too.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/synclink_gt.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index 16e469e581ec..00efed2c139e 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -3734,47 +3734,47 @@ module_exit(slgt_exit);
  * register access routines
  */
 
-#define CALC_REGADDR() \
-	unsigned long reg_addr = ((unsigned long)info->reg_addr) + addr; \
-	if (addr >= 0x80) \
-		reg_addr += (info->port_num) * 32; \
-	else if (addr >= 0x40)	\
-		reg_addr += (info->port_num) * 16;
+static inline void __iomem *calc_regaddr(struct slgt_info *info,
+					 unsigned int addr)
+{
+	void __iomem *reg_addr = info->reg_addr + addr;
+
+	if (addr >= 0x80)
+		reg_addr += info->port_num * 32;
+	else if (addr >= 0x40)
+		reg_addr += info->port_num * 16;
+
+	return reg_addr;
+}
 
 static __u8 rd_reg8(struct slgt_info *info, unsigned int addr)
 {
-	CALC_REGADDR();
-	return readb((void __iomem *)reg_addr);
+	return readb(calc_regaddr(info, addr));
 }
 
 static void wr_reg8(struct slgt_info *info, unsigned int addr, __u8 value)
 {
-	CALC_REGADDR();
-	writeb(value, (void __iomem *)reg_addr);
+	writeb(value, calc_regaddr(info, addr));
 }
 
 static __u16 rd_reg16(struct slgt_info *info, unsigned int addr)
 {
-	CALC_REGADDR();
-	return readw((void __iomem *)reg_addr);
+	return readw(calc_regaddr(info, addr));
 }
 
 static void wr_reg16(struct slgt_info *info, unsigned int addr, __u16 value)
 {
-	CALC_REGADDR();
-	writew(value, (void __iomem *)reg_addr);
+	writew(value, calc_regaddr(info, addr));
 }
 
 static __u32 rd_reg32(struct slgt_info *info, unsigned int addr)
 {
-	CALC_REGADDR();
-	return readl((void __iomem *)reg_addr);
+	return readl(calc_regaddr(info, addr));
 }
 
 static void wr_reg32(struct slgt_info *info, unsigned int addr, __u32 value)
 {
-	CALC_REGADDR();
-	writel(value, (void __iomem *)reg_addr);
+	writel(value, calc_regaddr(info, addr));
 }
 
 static void rdma_reset(struct slgt_info *info)
-- 
2.41.0

[PATCH 2/7] tty: synclink_gt: drop global slgt_driver_name array

[Jiri Slaby (SUSE)]

It's used on one place, so put the containing string there directly.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/synclink_gt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index 00efed2c139e..c2ab7ecf0900 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -88,7 +88,6 @@
  * module identification
  */
 static char *driver_name     = "SyncLink GT";
-static char *slgt_driver_name = "synclink_gt";
 static char *tty_dev_prefix  = "ttySLG";
 MODULE_LICENSE("GPL");
 #define MAX_DEVICES 32
@@ -3683,7 +3682,7 @@ static int __init slgt_init(void)
 
 	/* Initialize the tty_driver structure */
 
-	serial_driver->driver_name = slgt_driver_name;
+	serial_driver->driver_name = "synclink_gt";
 	serial_driver->name = tty_dev_prefix;
 	serial_driver->major = ttymajor;
 	serial_driver->minor_start = 64;
-- 
2.41.0

[PATCH 3/7] tty: synclink_gt: define global strings as const strings

[Jiri Slaby (SUSE)]

And not non-const pointers to strings.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/synclink_gt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index c2ab7ecf0900..a8716a81ac74 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -87,8 +87,8 @@
 /*
  * module identification
  */
-static char *driver_name     = "SyncLink GT";
-static char *tty_dev_prefix  = "ttySLG";
+static const char driver_name[] = "SyncLink GT";
+static const char tty_dev_prefix[] = "ttySLG";
 MODULE_LICENSE("GPL");
 #define MAX_DEVICES 32
 
-- 
2.41.0

[PATCH 4/7] tty: synclink_gt: drop info messages from init/exit functions

[Jiri Slaby (SUSE)]

It is preferred NOT to print anything from init and exit functions of a
module. (If everything goes fine.)

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/synclink_gt.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index a8716a81ac74..4a93e0e48156 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -3628,8 +3628,6 @@ static void slgt_cleanup(void)
 	struct slgt_info *info;
 	struct slgt_info *tmp;
 
-	printk(KERN_INFO "unload %s\n", driver_name);
-
 	if (serial_driver) {
 		for (info=slgt_device_list ; info != NULL ; info=info->next_device)
 			tty_unregister_device(serial_driver, info->line);
@@ -3671,8 +3669,6 @@ static int __init slgt_init(void)
 {
 	int rc;
 
-	printk(KERN_INFO "%s\n", driver_name);
-
 	serial_driver = tty_alloc_driver(MAX_DEVICES, TTY_DRIVER_REAL_RAW |
 			TTY_DRIVER_DYNAMIC_DEV);
 	if (IS_ERR(serial_driver)) {
@@ -3701,9 +3697,6 @@ static int __init slgt_init(void)
 		goto error;
 	}
 
-	printk(KERN_INFO "%s, tty major#%d\n",
-	       driver_name, serial_driver->major);
-
 	slgt_device_count = 0;
 	if ((rc = pci_register_driver(&pci_driver)) < 0) {
 		printk("%s pci_register_driver error=%d\n", driver_name, rc);
@@ -3711,9 +3704,6 @@ static int __init slgt_init(void)
 	}
 	pci_registered = true;
 
-	if (!slgt_device_list)
-		printk("%s no devices found\n",driver_name);
-
 	return 0;
 
 error:
-- 
2.41.0

[PATCH 5/7] tty: synclink_gt: use PCI_VDEVICE

[Jiri Slaby (SUSE)]

It makes the device entries quite a bit readable.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/synclink_gt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index 4a93e0e48156..381b2e22fa96 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -93,11 +93,11 @@ MODULE_LICENSE("GPL");
 #define MAX_DEVICES 32
 
 static const struct pci_device_id pci_table[] = {
-	{PCI_VENDOR_ID_MICROGATE, SYNCLINK_GT_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,},
-	{PCI_VENDOR_ID_MICROGATE, SYNCLINK_GT2_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,},
-	{PCI_VENDOR_ID_MICROGATE, SYNCLINK_GT4_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,},
-	{PCI_VENDOR_ID_MICROGATE, SYNCLINK_AC_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,},
-	{0,}, /* terminate list */
+	{ PCI_VDEVICE(MICROGATE, SYNCLINK_GT_DEVICE_ID) },
+	{ PCI_VDEVICE(MICROGATE, SYNCLINK_GT2_DEVICE_ID) },
+	{ PCI_VDEVICE(MICROGATE, SYNCLINK_GT4_DEVICE_ID) },
+	{ PCI_VDEVICE(MICROGATE, SYNCLINK_AC_DEVICE_ID) },
+	{ 0 }, /* terminate list */
 };
 MODULE_DEVICE_TABLE(pci, pci_table);
 
-- 
2.41.0

[PATCH 6/7] tty: synclink_gt: make default_params const

[Jiri Slaby (SUSE)]

default_params are only read, so move them from .data to .rodata using
'const'.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/synclink_gt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index 381b2e22fa96..fe53e9c2c9b4 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -322,7 +322,7 @@ struct slgt_info {
 
 };
 
-static MGSL_PARAMS default_params = {
+static const MGSL_PARAMS default_params = {
 	.mode            = MGSL_MODE_HDLC,
 	.loopback        = 0,
 	.flags           = HDLC_FLAG_UNDERRUN_ABORT15,
-- 
2.41.0

[PATCH 7/7] tty: synclink_gt: mark as BROKEN

[Jiri Slaby (SUSE)]

After walking and trying to clean up the worst in the driver, I came
across the pci_driver::remove() _empty_ implementation. That would crash
the system at least during hot-unplug (or write to remove in sysfs).

There are many other problems:
* Initialization + deinitialization apparently comes from no-hotplug
  support age. It needs a rewrite.
* Hairy debug macros. Drop them.
* Use of self-baked lists. Replace by list.
* The order of the functions should be inverted and fwd decls dropped.
* Coding style from the stone age. Fix.
* I assume there are many bugs, but the code is unreadable at times, so
  hard to judge. There is one example posted [1].

I was able to find only one user back in 2016. So mark the driver as
BROKEN for some time. Either someone will notice and we can bring the
driver to this century. Or we will drop it completely if noone cares.

[1] https://lore.kernel.org/all/20230728123901.64225-1-dg573847474@gmail.com/

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Chengfeng Ye <dg573847474@gmail.com>
---
 drivers/tty/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index 341abaed4ce2..907a7cb1d186 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -236,6 +236,7 @@ config MOXA_SMARTIO
 config SYNCLINK_GT
 	tristate "SyncLink GT/AC support"
 	depends on SERIAL_NONSTANDARD && PCI
+	depends on BROKEN
 	help
 	  Support for SyncLink GT and SyncLink AC families of
 	  synchronous and asynchronous serial adapters
-- 
2.41.0

Re: [PATCH 0/7] tty: synclink_gt: mark as BROKEN

[Greg KH]

On Mon, Jul 31, 2023 at 10:59:55AM +0200, Jiri Slaby (SUSE) wrote:
> I did some cleanups in the driver (in this series), but then decided not
> to go on and mark the driver as BROKEN instead. See the last patch for
> explanation.
> 
> Feel free to take only the last patch. I don't assume anyone appears to
> take care of the driver, so we will drop it sooner or later anyway. The
> changes only demonstrate how unmaintained the driver is...

I'll take them all, thanks for them, as I don't want to waste the work
you did here.  We can schedule this for deletion in a few releases as I
doubt anyone will care :(

thanks,

greg k-h