* [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).