[PATCH 0/7] hvcs: Various hvcs device hotplug fixes

[PATCH 0/7] hvcs: Various hvcs device hotplug fixes

[Brian King]

This patch series fixes a number of issues with hotplugging
hvcs devices including memory leaks as well as, the inability
to reconnect to a console device after it has been hot added
back, since it was not getting cleaned up properly on the
hotplug remove path.

Brian King (7):
  hvcs: Fix hvcs port reference counting
  hvcs: Remove sysfs file prior to vio unregister
  hvcs: Remove sysfs group earlier
  hvcs: Get reference to tty in remove
  hvcs: Use vhangup in hotplug remove
  hvcs: Synchronize hotplug remove with port free
  powerpc: Fix device node refcounting

 arch/powerpc/platforms/pseries/reconfig.c |  1 +
 drivers/tty/hvc/hvcs.c                    | 61 +++++++++--------------
 2 files changed, 25 insertions(+), 37 deletions(-)

-- 
2.31.1

[PATCH 1/7] hvcs: Fix hvcs port reference counting

[Brian King]

The hvcs driver only ever gets two references to the port. One
at initialization time, and one at install time. Remove the code
that was trying to do multiple port puts for each open, which
would result in more puts than gets.

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/tty/hvc/hvcs.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index 4ba24963685e..faf5ccfc561e 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -1215,12 +1215,9 @@ static void hvcs_hangup(struct tty_struct * tty)
 {
 	struct hvcs_struct *hvcsd = tty->driver_data;
 	unsigned long flags;
-	int temp_open_count;
 	int irq;
 
 	spin_lock_irqsave(&hvcsd->lock, flags);
-	/* Preserve this so that we know how many kref refs to put */
-	temp_open_count = hvcsd->port.count;
 
 	/*
 	 * Don't kref put inside the spinlock because the destruction
@@ -1247,21 +1244,6 @@ static void hvcs_hangup(struct tty_struct * tty)
 	spin_unlock_irqrestore(&hvcsd->lock, flags);
 
 	free_irq(irq, hvcsd);
-
-	/*
-	 * We need to kref_put() for every open_count we have since the
-	 * tty_hangup() function doesn't invoke a close per open connection on a
-	 * non-console device.
-	 */
-	while(temp_open_count) {
-		--temp_open_count;
-		/*
-		 * The final put will trigger destruction of the hvcs_struct.
-		 * NOTE:  If this hangup was signaled from user space then the
-		 * final put will never happen.
-		 */
-		tty_port_put(&hvcsd->port);
-	}
 }
 
 /*
-- 
2.31.1

[PATCH 2/7] hvcs: Remove sysfs file prior to vio unregister

[Brian King]

This moves the removal of the rescan sysfs attribute to occur
before the call to unregister the vio to ensure the removal
does not fail due to the vio driver already being freed.

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/tty/hvc/hvcs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index faf5ccfc561e..9131dcb2e8d8 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -1519,6 +1519,8 @@ static int __init hvcs_module_init(void)
 
 static void __exit hvcs_module_exit(void)
 {
+	driver_remove_file(&hvcs_vio_driver.driver, &driver_attr_rescan);
+
 	/*
 	 * This driver receives hvcs_remove callbacks for each device upon
 	 * module removal.
@@ -1538,8 +1540,6 @@ static void __exit hvcs_module_exit(void)
 	hvcs_pi_buff = NULL;
 	spin_unlock(&hvcs_pi_lock);
 
-	driver_remove_file(&hvcs_vio_driver.driver, &driver_attr_rescan);
-
 	tty_unregister_driver(hvcs_tty_driver);
 
 	hvcs_free_index_list();
-- 
2.31.1

[PATCH 3/7] hvcs: Remove sysfs group earlier

[Brian King]

Cleanup the sysfs group earlier in remove. This eliminates
errors coming from kernfs when attempting to remove a console
device that is in use.

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/tty/hvc/hvcs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index 9131dcb2e8d8..9c5887d0c882 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -688,8 +688,6 @@ static void hvcs_destruct_port(struct tty_port *p)
 	spin_unlock_irqrestore(&hvcsd->lock, flags);
 	spin_unlock(&hvcs_structs_lock);
 
-	sysfs_remove_group(&vdev->dev.kobj, &hvcs_attr_group);
-
 	kfree(hvcsd);
 }
 
@@ -814,6 +812,8 @@ static void hvcs_remove(struct vio_dev *dev)
 	 */
 	tty_port_put(&hvcsd->port);
 
+	sysfs_remove_group(&dev->dev.kobj, &hvcs_attr_group);
+
 	/*
 	 * The hangup is a scheduled function which will auto chain call
 	 * hvcs_hangup.  The tty should always be valid at this time unless a
-- 
2.31.1

[PATCH 4/7] hvcs: Get reference to tty in remove

[Brian King]

Grab a reference to the tty when removing the hvcs to ensure
it does not get freed unexpectedly.

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/tty/hvc/hvcs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index 9c5887d0c882..b28ddfc46e42 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -802,7 +802,7 @@ static void hvcs_remove(struct vio_dev *dev)
 
 	spin_lock_irqsave(&hvcsd->lock, flags);
 
-	tty = hvcsd->port.tty;
+	tty = tty_port_tty_get(&hvcsd->port);
 
 	spin_unlock_irqrestore(&hvcsd->lock, flags);
 
@@ -819,8 +819,10 @@ static void hvcs_remove(struct vio_dev *dev)
 	 * hvcs_hangup.  The tty should always be valid at this time unless a
 	 * simultaneous tty close already cleaned up the hvcs_struct.
 	 */
-	if (tty)
+	if (tty) {
 		tty_hangup(tty);
+		tty_kref_put(tty);
+	}
 
 	printk(KERN_INFO "HVCS: vty-server@%X removed from the"
 			" vio bus.\n", dev->unit_address);
-- 
2.31.1

[PATCH 5/7] hvcs: Use vhangup in hotplug remove

[Brian King]

When hotplug removing an hvcs device, we need to ensure the
hangup processing is done prior to exiting the remove function,
so use tty_vhangup to do the hangup processing directly
rather than using tty_hangup which simply schedules the hangup
work for later execution.

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/tty/hvc/hvcs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index b28ddfc46e42..24541fc53625 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -815,12 +815,11 @@ static void hvcs_remove(struct vio_dev *dev)
 	sysfs_remove_group(&dev->dev.kobj, &hvcs_attr_group);
 
 	/*
-	 * The hangup is a scheduled function which will auto chain call
-	 * hvcs_hangup.  The tty should always be valid at this time unless a
+	 * The tty should always be valid at this time unless a
 	 * simultaneous tty close already cleaned up the hvcs_struct.
 	 */
 	if (tty) {
-		tty_hangup(tty);
+		tty_vhangup(tty);
 		tty_kref_put(tty);
 	}
 
-- 
2.31.1

[PATCH 6/7] hvcs: Synchronize hotplug remove with port free

[Brian King]

Synchronizes hotplug remove with the freeing of the port.
This ensures we have freed all the memory associated with
this port and are not leaking memory.

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/tty/hvc/hvcs.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index 24541fc53625..2b038d4b3a63 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -52,6 +52,7 @@
 
 #include <linux/device.h>
 #include <linux/init.h>
+#include <linux/completion.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/kref.h>
@@ -285,6 +286,7 @@ struct hvcs_struct {
 	char p_location_code[HVCS_CLC_LENGTH + 1]; /* CLC + Null Term */
 	struct list_head next; /* list management */
 	struct vio_dev *vdev;
+	struct completion *destroyed;
 };
 
 static LIST_HEAD(hvcs_structs);
@@ -658,11 +660,13 @@ static void hvcs_destruct_port(struct tty_port *p)
 {
 	struct hvcs_struct *hvcsd = container_of(p, struct hvcs_struct, port);
 	struct vio_dev *vdev;
+	struct completion *comp;
 	unsigned long flags;
 
 	spin_lock(&hvcs_structs_lock);
 	spin_lock_irqsave(&hvcsd->lock, flags);
 
+	comp = hvcsd->destroyed;
 	/* the list_del poisons the pointers */
 	list_del(&(hvcsd->next));
 
@@ -682,6 +686,7 @@ static void hvcs_destruct_port(struct tty_port *p)
 
 	hvcsd->p_unit_address = 0;
 	hvcsd->p_partition_ID = 0;
+	hvcsd->destroyed = NULL;
 	hvcs_return_index(hvcsd->index);
 	memset(&hvcsd->p_location_code[0], 0x00, HVCS_CLC_LENGTH + 1);
 
@@ -689,6 +694,8 @@ static void hvcs_destruct_port(struct tty_port *p)
 	spin_unlock(&hvcs_structs_lock);
 
 	kfree(hvcsd);
+	if (comp)
+		complete(comp);
 }
 
 static const struct tty_port_operations hvcs_port_ops = {
@@ -795,6 +802,7 @@ static int hvcs_probe(
 static void hvcs_remove(struct vio_dev *dev)
 {
 	struct hvcs_struct *hvcsd = dev_get_drvdata(&dev->dev);
+	DECLARE_COMPLETION_ONSTACK(comp);
 	unsigned long flags;
 	struct tty_struct *tty;
 
@@ -802,16 +810,11 @@ static void hvcs_remove(struct vio_dev *dev)
 
 	spin_lock_irqsave(&hvcsd->lock, flags);
 
+	hvcsd->destroyed = &comp;
 	tty = tty_port_tty_get(&hvcsd->port);
 
 	spin_unlock_irqrestore(&hvcsd->lock, flags);
 
-	/*
-	 * Let the last holder of this object cause it to be removed, which
-	 * would probably be tty_hangup below.
-	 */
-	tty_port_put(&hvcsd->port);
-
 	sysfs_remove_group(&dev->dev.kobj, &hvcs_attr_group);
 
 	/*
@@ -823,6 +826,8 @@ static void hvcs_remove(struct vio_dev *dev)
 		tty_kref_put(tty);
 	}
 
+	tty_port_put(&hvcsd->port);
+	wait_for_completion(&comp);
 	printk(KERN_INFO "HVCS: vty-server@%X removed from the"
 			" vio bus.\n", dev->unit_address);
 };
@@ -1172,7 +1177,10 @@ static void hvcs_close(struct tty_struct *tty, struct file *filp)
 	hvcsd = tty->driver_data;
 
 	spin_lock_irqsave(&hvcsd->lock, flags);
-	if (--hvcsd->port.count == 0) {
+	if (hvcsd->port.count == 0) {
+		spin_unlock_irqrestore(&hvcsd->lock, flags);
+		return;
+	} else if (--hvcsd->port.count == 0) {
 
 		vio_disable_interrupts(hvcsd->vdev);
 
@@ -1228,11 +1236,7 @@ static void hvcs_hangup(struct tty_struct * tty)
 	vio_disable_interrupts(hvcsd->vdev);
 
 	hvcsd->todo_mask = 0;
-
-	/* I don't think the tty needs the hvcs_struct pointer after a hangup */
-	tty->driver_data = NULL;
 	hvcsd->port.tty = NULL;
-
 	hvcsd->port.count = 0;
 
 	/* This will drop any buffered data on the floor which is OK in a hangup
-- 
2.31.1

[PATCH 7/7] powerpc: Fix device node refcounting

[Brian King]

While testing fixes to the hvcs hotplug code, kmemleak was reporting
potential memory leaks. This was tracked down to the struct device_node
object associated with the hvcs device. Looking at the leaked
object in crash showed that the kref in the kobject in the device_node
had a reference count of 1 still, and the release function was never
getting called as a result of this. This adds an of_node_put in
pSeries_reconfig_remove_node in order to balance the refcounting
so that we actually free the device_node in the case of it being
allocated in pSeries_reconfig_add_node.

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/reconfig.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 599bd2c78514..8cb7309b19a4 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -77,6 +77,7 @@ static int pSeries_reconfig_remove_node(struct device_node *np)
 	}
 
 	of_detach_node(np);
+	of_node_put(np);
 	of_node_put(parent);
 	return 0;
 }
-- 
2.31.1

Re: [PATCH 0/7] hvcs: Various hvcs device hotplug fixes

[Christophe Leroy]

Le 30/01/2023 à 23:43, Brian King a écrit :
> This patch series fixes a number of issues with hotplugging
> hvcs devices including memory leaks as well as, the inability
> to reconnect to a console device after it has been hot added
> back, since it was not getting cleaned up properly on the
> hotplug remove path.
> 
> Brian King (7):
>    hvcs: Fix hvcs port reference counting
>    hvcs: Remove sysfs file prior to vio unregister
>    hvcs: Remove sysfs group earlier
>    hvcs: Get reference to tty in remove
>    hvcs: Use vhangup in hotplug remove
>    hvcs: Synchronize hotplug remove with port free
>    powerpc: Fix device node refcounting
> 
>   arch/powerpc/platforms/pseries/reconfig.c |  1 +
>   drivers/tty/hvc/hvcs.c                    | 61 +++++++++--------------
>   2 files changed, 25 insertions(+), 37 deletions(-)
> 

As far as I can see, most recent patches in drivers/tty/hvc/ are taken 
by Greg Kroah-Hartman <gregkh@linuxfoundation.org> so I'd suggest you 
send you series to Greg as he maintains the tty tree.

By the way, does last patch require the 6 previous ones or can it be 
applied independently ?

Christophe

Re: [PATCH 0/7] hvcs: Various hvcs device hotplug fixes

[Brian King]

On 2/1/23 4:32 AM, Christophe Leroy wrote:
> 
> 
> Le 30/01/2023 à 23:43, Brian King a écrit :
>> This patch series fixes a number of issues with hotplugging
>> hvcs devices including memory leaks as well as, the inability
>> to reconnect to a console device after it has been hot added
>> back, since it was not getting cleaned up properly on the
>> hotplug remove path.
>>
>> Brian King (7):
>>    hvcs: Fix hvcs port reference counting
>>    hvcs: Remove sysfs file prior to vio unregister
>>    hvcs: Remove sysfs group earlier
>>    hvcs: Get reference to tty in remove
>>    hvcs: Use vhangup in hotplug remove
>>    hvcs: Synchronize hotplug remove with port free
>>    powerpc: Fix device node refcounting
>>
>>   arch/powerpc/platforms/pseries/reconfig.c |  1 +
>>   drivers/tty/hvc/hvcs.c                    | 61 +++++++++--------------
>>   2 files changed, 25 insertions(+), 37 deletions(-)
>>
> 
> As far as I can see, most recent patches in drivers/tty/hvc/ are taken 
> by Greg Kroah-Hartman <gregkh@linuxfoundation.org> so I'd suggest you 
> send you series to Greg as he maintains the tty tree.

Sure. I just followed what was in the maintainers file, but I can resend
and cc Greg and linux-serial if that makes sense.


> By the way, does last patch require the 6 previous ones or can it be 
> applied independently ?

Yes. It is independent, so I'll pull this out of the series and can send
it separately to this list only.

Thanks,

Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center