linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] serial: 8250_aspeed_vuart: fix duplicate __release_region() on unbind
@ 2021-05-10  1:42 Zev Weiss
  2021-05-10  1:42 ` [PATCH 1/3] serial: 8250_aspeed_vuart: factor out aspeed_vuart_{read, write}b() helper functions Zev Weiss
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Zev Weiss @ 2021-05-10  1:42 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: openbmc, Zev Weiss, Jiri Slaby, Joel Stanley, Andrew Jeffery,
	Johan Hovold, linux-serial, linux-arm-kernel, linux-aspeed,
	linux-kernel

Hello,

This series addresses a bug reported by Andrew Jeffery [0] wherein the
aspeed-vuart driver triggers a "Trying to free nonexistent resource"
warning from __release_region() when unbound from a device.  The first
two patches provide some preparatory refactoring and cleanups; the
actual fix is in the final patch.

Joel, note that this series has a dependency on the "generalized DT
properties" series [1] (specifically the second patch), which has been
merged upstream but is not yet in the OpenBMC dev-5.10 tree.

[0] https://github.com/openbmc/linux/issues/203
[1] https://lore.kernel.org/openbmc/20210412034712.16778-1-zev@bewilderbeest.net/

Zev Weiss (3):
  serial: 8250_aspeed_vuart: factor out aspeed_vuart_{read,write}b()
    helper functions
  serial: 8250_aspeed_vuart: initialize vuart->port in
    aspeed_vuart_probe()
  serial: 8250_aspeed_vuart: use UPF_IOREMAP to set up register mapping

 drivers/tty/serial/8250/8250_aspeed_vuart.c | 50 +++++++++++----------
 1 file changed, 27 insertions(+), 23 deletions(-)

-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] serial: 8250_aspeed_vuart: factor out aspeed_vuart_{read, write}b() helper functions
  2021-05-10  1:42 [PATCH 0/3] serial: 8250_aspeed_vuart: fix duplicate __release_region() on unbind Zev Weiss
@ 2021-05-10  1:42 ` Zev Weiss
  2021-05-13  1:21   ` Andrew Jeffery
  2021-05-10  1:42 ` [PATCH 2/3] serial: 8250_aspeed_vuart: initialize vuart->port in aspeed_vuart_probe() Zev Weiss
  2021-05-10  1:42 ` [PATCH 3/3] serial: 8250_aspeed_vuart: use UPF_IOREMAP to set up register mapping Zev Weiss
  2 siblings, 1 reply; 8+ messages in thread
From: Zev Weiss @ 2021-05-10  1:42 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: openbmc, Zev Weiss, Jiri Slaby, Joel Stanley, Andrew Jeffery,
	Johan Hovold, linux-serial, linux-arm-kernel, linux-aspeed,
	linux-kernel

This is a small prepatory step for changing the way this driver does
its I/O accesses.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 38 +++++++++++++--------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 61550f24a2d3..9e8b2e8e32b6 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -64,14 +64,24 @@ static const int unthrottle_timeout = HZ/10;
  * different system (though most of them use 3f8/4).
  */
 
+static inline u8 aspeed_vuart_readb(struct aspeed_vuart *vuart, u8 reg)
+{
+	return readb(vuart->regs + reg);
+}
+
+static inline void aspeed_vuart_writeb(struct aspeed_vuart *vuart, u8 val, u8 reg)
+{
+	writeb(val, vuart->regs + reg);
+}
+
 static ssize_t lpc_address_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	struct aspeed_vuart *vuart = dev_get_drvdata(dev);
 	u16 addr;
 
-	addr = (readb(vuart->regs + ASPEED_VUART_ADDRH) << 8) |
-		(readb(vuart->regs + ASPEED_VUART_ADDRL));
+	addr = (aspeed_vuart_readb(vuart, ASPEED_VUART_ADDRH) << 8) |
+		(aspeed_vuart_readb(vuart, ASPEED_VUART_ADDRL));
 
 	return snprintf(buf, PAGE_SIZE - 1, "0x%x\n", addr);
 }
@@ -81,8 +91,8 @@ static int aspeed_vuart_set_lpc_address(struct aspeed_vuart *vuart, u32 addr)
 	if (addr > U16_MAX)
 		return -EINVAL;
 
-	writeb(addr >> 8, vuart->regs + ASPEED_VUART_ADDRH);
-	writeb(addr >> 0, vuart->regs + ASPEED_VUART_ADDRL);
+	aspeed_vuart_writeb(vuart, addr >> 8, ASPEED_VUART_ADDRH);
+	aspeed_vuart_writeb(vuart, addr >> 0, ASPEED_VUART_ADDRL);
 
 	return 0;
 }
@@ -111,7 +121,7 @@ static ssize_t sirq_show(struct device *dev,
 	struct aspeed_vuart *vuart = dev_get_drvdata(dev);
 	u8 reg;
 
-	reg = readb(vuart->regs + ASPEED_VUART_GCRB);
+	reg = aspeed_vuart_readb(vuart, ASPEED_VUART_GCRB);
 	reg &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK;
 	reg >>= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT;
 
@@ -128,10 +138,10 @@ static int aspeed_vuart_set_sirq(struct aspeed_vuart *vuart, u32 sirq)
 	sirq <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT;
 	sirq &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK;
 
-	reg = readb(vuart->regs + ASPEED_VUART_GCRB);
+	reg = aspeed_vuart_readb(vuart, ASPEED_VUART_GCRB);
 	reg &= ~ASPEED_VUART_GCRB_HOST_SIRQ_MASK;
 	reg |= sirq;
-	writeb(reg, vuart->regs + ASPEED_VUART_GCRB);
+	aspeed_vuart_writeb(vuart, reg, ASPEED_VUART_GCRB);
 
 	return 0;
 }
@@ -159,7 +169,7 @@ static ssize_t sirq_polarity_show(struct device *dev,
 	struct aspeed_vuart *vuart = dev_get_drvdata(dev);
 	u8 reg;
 
-	reg = readb(vuart->regs + ASPEED_VUART_GCRA);
+	reg = aspeed_vuart_readb(vuart, ASPEED_VUART_GCRA);
 	reg &= ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
 
 	return snprintf(buf, PAGE_SIZE - 1, "%u\n", reg ? 1 : 0);
@@ -168,14 +178,14 @@ static ssize_t sirq_polarity_show(struct device *dev,
 static void aspeed_vuart_set_sirq_polarity(struct aspeed_vuart *vuart,
 					   bool polarity)
 {
-	u8 reg = readb(vuart->regs + ASPEED_VUART_GCRA);
+	u8 reg = aspeed_vuart_readb(vuart, ASPEED_VUART_GCRA);
 
 	if (polarity)
 		reg |= ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
 	else
 		reg &= ~ASPEED_VUART_GCRA_HOST_SIRQ_POLARITY;
 
-	writeb(reg, vuart->regs + ASPEED_VUART_GCRA);
+	aspeed_vuart_writeb(vuart, reg, ASPEED_VUART_GCRA);
 }
 
 static ssize_t sirq_polarity_store(struct device *dev,
@@ -210,14 +220,14 @@ static const struct attribute_group aspeed_vuart_attr_group = {
 
 static void aspeed_vuart_set_enabled(struct aspeed_vuart *vuart, bool enabled)
 {
-	u8 reg = readb(vuart->regs + ASPEED_VUART_GCRA);
+	u8 reg = aspeed_vuart_readb(vuart, ASPEED_VUART_GCRA);
 
 	if (enabled)
 		reg |= ASPEED_VUART_GCRA_VUART_EN;
 	else
 		reg &= ~ASPEED_VUART_GCRA_VUART_EN;
 
-	writeb(reg, vuart->regs + ASPEED_VUART_GCRA);
+	aspeed_vuart_writeb(vuart, reg, ASPEED_VUART_GCRA);
 }
 
 static void aspeed_vuart_set_host_tx_discard(struct aspeed_vuart *vuart,
@@ -225,7 +235,7 @@ static void aspeed_vuart_set_host_tx_discard(struct aspeed_vuart *vuart,
 {
 	u8 reg;
 
-	reg = readb(vuart->regs + ASPEED_VUART_GCRA);
+	reg = aspeed_vuart_readb(vuart, ASPEED_VUART_GCRA);
 
 	/* If the DISABLE_HOST_TX_DISCARD bit is set, discard is disabled */
 	if (!discard)
@@ -233,7 +243,7 @@ static void aspeed_vuart_set_host_tx_discard(struct aspeed_vuart *vuart,
 	else
 		reg &= ~ASPEED_VUART_GCRA_DISABLE_HOST_TX_DISCARD;
 
-	writeb(reg, vuart->regs + ASPEED_VUART_GCRA);
+	aspeed_vuart_writeb(vuart, reg, ASPEED_VUART_GCRA);
 }
 
 static int aspeed_vuart_startup(struct uart_port *uart_port)
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] serial: 8250_aspeed_vuart: initialize vuart->port in aspeed_vuart_probe()
  2021-05-10  1:42 [PATCH 0/3] serial: 8250_aspeed_vuart: fix duplicate __release_region() on unbind Zev Weiss
  2021-05-10  1:42 ` [PATCH 1/3] serial: 8250_aspeed_vuart: factor out aspeed_vuart_{read, write}b() helper functions Zev Weiss
@ 2021-05-10  1:42 ` Zev Weiss
  2021-05-13  1:34   ` Andrew Jeffery
  2021-05-10  1:42 ` [PATCH 3/3] serial: 8250_aspeed_vuart: use UPF_IOREMAP to set up register mapping Zev Weiss
  2 siblings, 1 reply; 8+ messages in thread
From: Zev Weiss @ 2021-05-10  1:42 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: openbmc, Zev Weiss, Jiri Slaby, Joel Stanley, Andrew Jeffery,
	Johan Hovold, linux-serial, linux-arm-kernel, linux-aspeed,
	linux-kernel

Previously this had only been initialized if we hit the throttling path
in aspeed_vuart_handle_irq(); moving it to the probe function is a
slight consistency improvement and avoids redundant reinitialization in
the interrupt handler.  It also serves as preparation for converting the
driver's I/O accesses to use port->port.membase instead of its own
vuart->regs.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 9e8b2e8e32b6..249164dc397b 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -349,11 +349,9 @@ static int aspeed_vuart_handle_irq(struct uart_port *port)
 			struct aspeed_vuart *vuart = port->private_data;
 			__aspeed_vuart_set_throttle(up, true);
 
-			if (!timer_pending(&vuart->unthrottle_timer)) {
-				vuart->port = up;
+			if (!timer_pending(&vuart->unthrottle_timer))
 				mod_timer(&vuart->unthrottle_timer,
 					  jiffies + unthrottle_timeout);
-			}
 
 		} else {
 			count = min(space, 256);
@@ -511,6 +509,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 		goto err_clk_disable;
 
 	vuart->line = rc;
+	vuart->port = serial8250_get_port(vuart->line);
 
 	rc = of_parse_phandle_with_fixed_args(
 		np, "aspeed,sirq-polarity-sense", 2, 0,
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/3] serial: 8250_aspeed_vuart: use UPF_IOREMAP to set up register mapping
  2021-05-10  1:42 [PATCH 0/3] serial: 8250_aspeed_vuart: fix duplicate __release_region() on unbind Zev Weiss
  2021-05-10  1:42 ` [PATCH 1/3] serial: 8250_aspeed_vuart: factor out aspeed_vuart_{read, write}b() helper functions Zev Weiss
  2021-05-10  1:42 ` [PATCH 2/3] serial: 8250_aspeed_vuart: initialize vuart->port in aspeed_vuart_probe() Zev Weiss
@ 2021-05-10  1:42 ` Zev Weiss
  2 siblings, 0 replies; 8+ messages in thread
From: Zev Weiss @ 2021-05-10  1:42 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: openbmc, Zev Weiss, Andrew Jeffery, Jiri Slaby, Joel Stanley,
	Johan Hovold, linux-serial, linux-arm-kernel, linux-aspeed,
	linux-kernel

Previously this driver's use of devm_ioremap_resource() led to
duplicated calls to __release_region() when unbinding it (one from
serial8250_release_std_resource() and one from devres_release_all()),
the second of which resulted in a warning message:

  # echo 1e787000.serial > /sys/bus/platform/drivers/aspeed-vuart/unbind
  [33091.774200] Trying to free nonexistent resource <000000001e787000-000000001e78703f>

With this change the driver uses the generic serial8250 code's
UPF_IOREMAP to take care of the register mapping automatically instead
of doing its own devm_ioremap_resource(), thus avoiding the duplicate
__release_region() on unbind.

In doing this we eliminate vuart->regs, since it merely duplicates
vuart->port->port.membase, which we now use for our I/O accesses.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
Reported-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 249164dc397b..2bf1d8582d9a 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -34,7 +34,6 @@
 
 struct aspeed_vuart {
 	struct device		*dev;
-	void __iomem		*regs;
 	struct clk		*clk;
 	int			line;
 	struct timer_list	unthrottle_timer;
@@ -66,12 +65,12 @@ static const int unthrottle_timeout = HZ/10;
 
 static inline u8 aspeed_vuart_readb(struct aspeed_vuart *vuart, u8 reg)
 {
-	return readb(vuart->regs + reg);
+	return readb(vuart->port->port.membase + reg);
 }
 
 static inline void aspeed_vuart_writeb(struct aspeed_vuart *vuart, u8 val, u8 reg)
 {
-	writeb(val, vuart->regs + reg);
+	writeb(val, vuart->port->port.membase + reg);
 }
 
 static ssize_t lpc_address_show(struct device *dev,
@@ -429,13 +428,9 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 	timer_setup(&vuart->unthrottle_timer, aspeed_vuart_unthrottle_exp, 0);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	vuart->regs = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(vuart->regs))
-		return PTR_ERR(vuart->regs);
 
 	memset(&port, 0, sizeof(port));
 	port.port.private_data = vuart;
-	port.port.membase = vuart->regs;
 	port.port.mapbase = res->start;
 	port.port.mapsize = resource_size(res);
 	port.port.startup = aspeed_vuart_startup;
@@ -492,7 +487,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 	port.port.iotype = UPIO_MEM;
 	port.port.type = PORT_16550A;
 	port.port.uartclk = clk;
-	port.port.flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF
+	port.port.flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_IOREMAP
 		| UPF_FIXED_PORT | UPF_FIXED_TYPE | UPF_NO_THRE_TEST;
 
 	if (of_property_read_bool(np, "no-loopback-test"))
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] serial: 8250_aspeed_vuart: factor out aspeed_vuart_{read, write}b() helper functions
  2021-05-10  1:42 ` [PATCH 1/3] serial: 8250_aspeed_vuart: factor out aspeed_vuart_{read, write}b() helper functions Zev Weiss
@ 2021-05-13  1:21   ` Andrew Jeffery
  0 siblings, 0 replies; 8+ messages in thread
From: Andrew Jeffery @ 2021-05-13  1:21 UTC (permalink / raw)
  To: Zev Weiss, Greg Kroah-Hartman
  Cc: openbmc, Jiri Slaby, Joel Stanley, Johan Hovold, linux-serial,
	linux-arm-kernel, linux-aspeed, linux-kernel



On Mon, 10 May 2021, at 11:12, Zev Weiss wrote:
> This is a small prepatory step for changing the way this driver does
> its I/O accesses.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] serial: 8250_aspeed_vuart: initialize vuart->port in aspeed_vuart_probe()
  2021-05-10  1:42 ` [PATCH 2/3] serial: 8250_aspeed_vuart: initialize vuart->port in aspeed_vuart_probe() Zev Weiss
@ 2021-05-13  1:34   ` Andrew Jeffery
  2021-05-13 19:25     ` Zev Weiss
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Jeffery @ 2021-05-13  1:34 UTC (permalink / raw)
  To: Zev Weiss, Greg Kroah-Hartman, Jeremy Kerr
  Cc: openbmc, Jiri Slaby, Joel Stanley, Johan Hovold, linux-serial,
	linux-arm-kernel, linux-aspeed, linux-kernel



On Mon, 10 May 2021, at 11:12, Zev Weiss wrote:
> Previously this had only been initialized if we hit the throttling path
> in aspeed_vuart_handle_irq(); moving it to the probe function is a
> slight consistency improvement and avoids redundant reinitialization in
> the interrupt handler.  It also serves as preparation for converting the
> driver's I/O accesses to use port->port.membase instead of its own
> vuart->regs.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  drivers/tty/serial/8250/8250_aspeed_vuart.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c 
> b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index 9e8b2e8e32b6..249164dc397b 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -349,11 +349,9 @@ static int aspeed_vuart_handle_irq(struct 
> uart_port *port)
>  			struct aspeed_vuart *vuart = port->private_data;
>  			__aspeed_vuart_set_throttle(up, true);
>  
> -			if (!timer_pending(&vuart->unthrottle_timer)) {
> -				vuart->port = up;
> +			if (!timer_pending(&vuart->unthrottle_timer))
>  				mod_timer(&vuart->unthrottle_timer,
>  					  jiffies + unthrottle_timeout);
> -			}
>  
>  		} else {
>  			count = min(space, 256);
> @@ -511,6 +509,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>  		goto err_clk_disable;
>  
>  	vuart->line = rc;
> +	vuart->port = serial8250_get_port(vuart->line);

The documentation of serial8250_get_port() is somewhat concerning wrt 
the use:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/8250/8250_core.c?h=v5.13-rc1#n399

However, given the existing behaviour it shouldn't be problematic?

Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] serial: 8250_aspeed_vuart: initialize vuart->port in aspeed_vuart_probe()
  2021-05-13  1:34   ` Andrew Jeffery
@ 2021-05-13 19:25     ` Zev Weiss
  2021-05-14  1:58       ` Andrew Jeffery
  0 siblings, 1 reply; 8+ messages in thread
From: Zev Weiss @ 2021-05-13 19:25 UTC (permalink / raw)
  To: Andrew Jeffery
  Cc: Greg Kroah-Hartman, Jeremy Kerr, openbmc, Jiri Slaby,
	Joel Stanley, Johan Hovold, linux-serial, linux-arm-kernel,
	linux-aspeed, linux-kernel

On Wed, May 12, 2021 at 08:34:06PM CDT, Andrew Jeffery wrote:
>
>
>On Mon, 10 May 2021, at 11:12, Zev Weiss wrote:
>> Previously this had only been initialized if we hit the throttling path
>> in aspeed_vuart_handle_irq(); moving it to the probe function is a
>> slight consistency improvement and avoids redundant reinitialization in
>> the interrupt handler.  It also serves as preparation for converting the
>> driver's I/O accesses to use port->port.membase instead of its own
>> vuart->regs.
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> ---
>>  drivers/tty/serial/8250/8250_aspeed_vuart.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> b/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> index 9e8b2e8e32b6..249164dc397b 100644
>> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> @@ -349,11 +349,9 @@ static int aspeed_vuart_handle_irq(struct
>> uart_port *port)
>>  			struct aspeed_vuart *vuart = port->private_data;
>>  			__aspeed_vuart_set_throttle(up, true);
>>
>> -			if (!timer_pending(&vuart->unthrottle_timer)) {
>> -				vuart->port = up;
>> +			if (!timer_pending(&vuart->unthrottle_timer))
>>  				mod_timer(&vuart->unthrottle_timer,
>>  					  jiffies + unthrottle_timeout);
>> -			}
>>
>>  		} else {
>>  			count = min(space, 256);
>> @@ -511,6 +509,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>>  		goto err_clk_disable;
>>
>>  	vuart->line = rc;
>> +	vuart->port = serial8250_get_port(vuart->line);
>
>The documentation of serial8250_get_port() is somewhat concerning wrt
>the use:
>
>https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/8250/8250_core.c?h=v5.13-rc1#n399

Hmm, good point -- though despite that comment it looks like there is 
some existing code using it outside of suspend/resume callbacks (in 
8250_pci.c and 8250_pnp.c).  I'm not certain if those would necessarily 
be considered good precedent to follow for this, but I don't see any 
obvious better way of getting hold of the corresponding uart_8250_port 
(or its port.membase).

I did receive a notification that Greg had added this series to his 
tty-testing branch; not sure if that means he thinks it's OK or if it 
just kind of slipped by unnoticed though.

>
>However, given the existing behaviour it shouldn't be problematic?
>

"existing behaviour" referring to what here?


Thanks,
Zev


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] serial: 8250_aspeed_vuart: initialize vuart->port in aspeed_vuart_probe()
  2021-05-13 19:25     ` Zev Weiss
@ 2021-05-14  1:58       ` Andrew Jeffery
  0 siblings, 0 replies; 8+ messages in thread
From: Andrew Jeffery @ 2021-05-14  1:58 UTC (permalink / raw)
  To: Zev Weiss
  Cc: Greg Kroah-Hartman, Jeremy Kerr, openbmc, Jiri Slaby,
	Joel Stanley, Johan Hovold, linux-serial, linux-arm-kernel,
	linux-aspeed, linux-kernel



On Fri, 14 May 2021, at 04:55, Zev Weiss wrote:
> On Wed, May 12, 2021 at 08:34:06PM CDT, Andrew Jeffery wrote:
> >
> >
> >On Mon, 10 May 2021, at 11:12, Zev Weiss wrote:
> >> Previously this had only been initialized if we hit the throttling path
> >> in aspeed_vuart_handle_irq(); moving it to the probe function is a
> >> slight consistency improvement and avoids redundant reinitialization in
> >> the interrupt handler.  It also serves as preparation for converting the
> >> driver's I/O accesses to use port->port.membase instead of its own
> >> vuart->regs.
> >>
> >> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> >> ---
> >>  drivers/tty/serial/8250/8250_aspeed_vuart.c | 5 ++---
> >>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> >> b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> >> index 9e8b2e8e32b6..249164dc397b 100644
> >> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> >> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> >> @@ -349,11 +349,9 @@ static int aspeed_vuart_handle_irq(struct
> >> uart_port *port)
> >>  			struct aspeed_vuart *vuart = port->private_data;
> >>  			__aspeed_vuart_set_throttle(up, true);
> >>
> >> -			if (!timer_pending(&vuart->unthrottle_timer)) {
> >> -				vuart->port = up;
> >> +			if (!timer_pending(&vuart->unthrottle_timer))
> >>  				mod_timer(&vuart->unthrottle_timer,
> >>  					  jiffies + unthrottle_timeout);
> >> -			}
> >>
> >>  		} else {
> >>  			count = min(space, 256);
> >> @@ -511,6 +509,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
> >>  		goto err_clk_disable;
> >>
> >>  	vuart->line = rc;
> >> +	vuart->port = serial8250_get_port(vuart->line);
> >
> >The documentation of serial8250_get_port() is somewhat concerning wrt
> >the use:
> >
> >https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/8250/8250_core.c?h=v5.13-rc1#n399
> 
> Hmm, good point -- though despite that comment it looks like there is 
> some existing code using it outside of suspend/resume callbacks (in 
> 8250_pci.c and 8250_pnp.c).  I'm not certain if those would necessarily 
> be considered good precedent to follow for this, but I don't see any 
> obvious better way of getting hold of the corresponding uart_8250_port 
> (or its port.membase).
> 
> I did receive a notification that Greg had added this series to his 
> tty-testing branch; not sure if that means he thinks it's OK or if it 
> just kind of slipped by unnoticed though.

Yeah, I just highlighted it in case anyone else wanted to weigh in.

Essentially I'm just deferring to Greg. If he's picked them up, great!

> 
> >
> >However, given the existing behaviour it shouldn't be problematic?
> >
> 
> "existing behaviour" referring to what here?

Well, we were poking at the registers through vuart->regs anyway.

So I don't think what you've done is any less correct.

Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-05-14  2:01 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-10  1:42 [PATCH 0/3] serial: 8250_aspeed_vuart: fix duplicate __release_region() on unbind Zev Weiss
2021-05-10  1:42 ` [PATCH 1/3] serial: 8250_aspeed_vuart: factor out aspeed_vuart_{read, write}b() helper functions Zev Weiss
2021-05-13  1:21   ` Andrew Jeffery
2021-05-10  1:42 ` [PATCH 2/3] serial: 8250_aspeed_vuart: initialize vuart->port in aspeed_vuart_probe() Zev Weiss
2021-05-13  1:34   ` Andrew Jeffery
2021-05-13 19:25     ` Zev Weiss
2021-05-14  1:58       ` Andrew Jeffery
2021-05-10  1:42 ` [PATCH 3/3] serial: 8250_aspeed_vuart: use UPF_IOREMAP to set up register mapping Zev Weiss

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).