No subject

No subject

[Jorge Ramirez-Ortiz]

From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Subject: [PATCH v5] drivers/tty: amba: defer probing DMA availability until hw_init
In-Reply-To: [PATCH v4] drivers/tty: amba: defer probing DMA availability until hw_init

checkpatch.pl didn't catch 'struct device * const uap_dev' 
resending 

[PATCH v5] drivers/tty: amba: defer probing DMA availability until hw_init

[Jorge Ramirez-Ortiz]

Fix a race condition that happens when device_initcall(pl011_dma_initicall)
is executed before all the devices have been probed - this issue was observed on
an hisi_6220 SoC (HiKey board from Linaro).

The deferred driver probing framework relies on late_initcall to trigger
deferred probes so it is just possible that, even with a valid DMA driver ready
to be loaded, we fail to synchronize with it.

The proposed implementation delays probing of the DMA until the uart device is
opened. As hw_init is invoked on port startup and port resume - but the DMA
probe is only required once - we avoid calling multiple times using a new field
in uart_amba_port to track this scenario.

This commit allows for subsequent attempts to associate an external DMA if the
DMA driver was deferred at UART hw_init time.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 drivers/tty/serial/amba-pl011.c | 95 +++++++++++++----------------------------
 1 file changed, 30 insertions(+), 65 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 8d94c19..4ce98e9 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -164,6 +164,7 @@ struct uart_amba_port {
 	bool			using_rx_dma;
 	struct pl011_dmarx_data dmarx;
 	struct pl011_dmatx_data	dmatx;
+	bool			dma_probed;
 #endif
 };
 
@@ -261,10 +262,11 @@ static void pl011_sgbuf_free(struct dma_chan *chan, struct pl011_sgbuf *sg,
 	}
 }
 
-static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *uap)
+static void pl011_dma_probe(struct uart_amba_port *uap)
 {
 	/* DMA is the sole user of the platform data right now */
-	struct amba_pl011_data *plat = dev_get_platdata(uap->port.dev);
+	struct device *const uap_dev = uap->port.dev;
+	struct amba_pl011_data *plat = dev_get_platdata(uap_dev);
 	struct dma_slave_config tx_conf = {
 		.dst_addr = uap->port.mapbase + UART01x_DR,
 		.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE,
@@ -275,12 +277,19 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
 	struct dma_chan *chan;
 	dma_cap_mask_t mask;
 
-	chan = dma_request_slave_channel(dev, "tx");
+	uap->dma_probed = true;
+
+	chan = dma_request_slave_channel_reason(uap_dev, "tx");
+	if (IS_ERR(chan)) {
+		if (PTR_ERR(chan) == -EPROBE_DEFER) {
+			dev_info(uap_dev, "DMA driver not ready\n");
+			uap->dma_probed = false;
+			return;
+		}
 
-	if (!chan) {
 		/* We need platform data */
 		if (!plat || !plat->dma_filter) {
-			dev_info(uap->port.dev, "no DMA platform data\n");
+			dev_info(uap_dev, "no DMA platform data\n");
 			return;
 		}
 
@@ -291,7 +300,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
 		chan = dma_request_channel(mask, plat->dma_filter,
 						plat->dma_tx_param);
 		if (!chan) {
-			dev_err(uap->port.dev, "no TX DMA channel!\n");
+			dev_err(uap_dev, "no TX DMA channel!\n");
 			return;
 		}
 	}
@@ -299,17 +308,17 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
 	dmaengine_slave_config(chan, &tx_conf);
 	uap->dmatx.chan = chan;
 
-	dev_info(uap->port.dev, "DMA channel TX %s\n",
+	dev_info(uap_dev, "DMA channel TX %s\n",
 		 dma_chan_name(uap->dmatx.chan));
 
 	/* Optionally make use of an RX channel as well */
-	chan = dma_request_slave_channel(dev, "rx");
+	chan = dma_request_slave_channel(uap_dev, "rx");
 
 	if (!chan && plat->dma_rx_param) {
 		chan = dma_request_channel(mask, plat->dma_filter, plat->dma_rx_param);
 
 		if (!chan) {
-			dev_err(uap->port.dev, "no RX DMA channel!\n");
+			dev_err(uap_dev, "no RX DMA channel!\n");
 			return;
 		}
 	}
@@ -333,7 +342,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
 			if (caps.residue_granularity ==
 					DMA_RESIDUE_GRANULARITY_DESCRIPTOR) {
 				dma_release_channel(chan);
-				dev_info(uap->port.dev,
+				dev_info(uap_dev,
 					"RX DMA disabled - no residue processing\n");
 				return;
 			}
@@ -362,75 +371,29 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
 					plat->dma_rx_poll_timeout;
 			else
 				uap->dmarx.poll_timeout = 3000;
-		} else if (!plat && dev->of_node) {
+		} else if (!plat && uap_dev->of_node) {
 			uap->dmarx.auto_poll_rate = of_property_read_bool(
-						dev->of_node, "auto-poll");
+						uap_dev->of_node, "auto-poll");
 			if (uap->dmarx.auto_poll_rate) {
 				u32 x;
 
-				if (0 == of_property_read_u32(dev->of_node,
+				if (0 == of_property_read_u32(uap_dev->of_node,
 						"poll-rate-ms", &x))
 					uap->dmarx.poll_rate = x;
 				else
 					uap->dmarx.poll_rate = 100;
-				if (0 == of_property_read_u32(dev->of_node,
+				if (0 == of_property_read_u32(uap_dev->of_node,
 						"poll-timeout-ms", &x))
 					uap->dmarx.poll_timeout = x;
 				else
 					uap->dmarx.poll_timeout = 3000;
 			}
 		}
-		dev_info(uap->port.dev, "DMA channel RX %s\n",
+		dev_info(uap_dev, "DMA channel RX %s\n",
 			 dma_chan_name(uap->dmarx.chan));
 	}
 }
 
-#ifndef MODULE
-/*
- * Stack up the UARTs and let the above initcall be done at device
- * initcall time, because the serial driver is called as an arch
- * initcall, and at this time the DMA subsystem is not yet registered.
- * At this point the driver will switch over to using DMA where desired.
- */
-struct dma_uap {
-	struct list_head node;
-	struct uart_amba_port *uap;
-	struct device *dev;
-};
-
-static LIST_HEAD(pl011_dma_uarts);
-
-static int __init pl011_dma_initcall(void)
-{
-	struct list_head *node, *tmp;
-
-	list_for_each_safe(node, tmp, &pl011_dma_uarts) {
-		struct dma_uap *dmau = list_entry(node, struct dma_uap, node);
-		pl011_dma_probe_initcall(dmau->dev, dmau->uap);
-		list_del(node);
-		kfree(dmau);
-	}
-	return 0;
-}
-
-device_initcall(pl011_dma_initcall);
-
-static void pl011_dma_probe(struct device *dev, struct uart_amba_port *uap)
-{
-	struct dma_uap *dmau = kzalloc(sizeof(struct dma_uap), GFP_KERNEL);
-	if (dmau) {
-		dmau->uap = uap;
-		dmau->dev = dev;
-		list_add_tail(&dmau->node, &pl011_dma_uarts);
-	}
-}
-#else
-static void pl011_dma_probe(struct device *dev, struct uart_amba_port *uap)
-{
-	pl011_dma_probe_initcall(dev, uap);
-}
-#endif
-
 static void pl011_dma_remove(struct uart_amba_port *uap)
 {
 	/* TODO: remove the initcall if it has not yet executed */
@@ -1142,7 +1105,7 @@ static inline bool pl011_dma_rx_running(struct uart_amba_port *uap)
 
 #else
 /* Blank functions if the DMA engine is not available */
-static inline void pl011_dma_probe(struct device *dev, struct uart_amba_port *uap)
+static inline void pl011_dma_probe(struct uart_amba_port *uap)
 {
 }
 
@@ -1548,6 +1511,9 @@ static int pl011_hwinit(struct uart_port *port)
 	uap->im = readw(uap->port.membase + UART011_IMSC);
 	writew(UART011_RTIM | UART011_RXIM, uap->port.membase + UART011_IMSC);
 
+	if (!uap->dma_probed)
+		pl011_dma_probe(uap);
+
 	if (dev_get_platdata(uap->port.dev)) {
 		struct amba_pl011_data *plat;
 
@@ -2218,7 +2184,6 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	uap->port.ops = &amba_pl011_pops;
 	uap->port.flags = UPF_BOOT_AUTOCONF;
 	uap->port.line = i;
-	pl011_dma_probe(&dev->dev, uap);
 
 	/* Ensure interrupts from this UART are masked and cleared */
 	writew(0, uap->port.membase + UART011_IMSC);
@@ -2233,7 +2198,8 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	if (!amba_reg.state) {
 		ret = uart_register_driver(&amba_reg);
 		if (ret < 0) {
-			pr_err("Failed to register AMBA-PL011 driver\n");
+			dev_err(&dev->dev,
+				"Failed to register AMBA-PL011 driver\n");
 			return ret;
 		}
 	}
@@ -2242,7 +2208,6 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	if (ret) {
 		amba_ports[i] = NULL;
 		uart_unregister_driver(&amba_reg);
-		pl011_dma_remove(uap);
 	}
 
 	return ret;
-- 
1.9.1

[PATCH v5] drivers/tty: amba: defer probing DMA availability until hw_init

[Jorge Ramirez-Ortiz]

On 02/26/2015 11:56 AM, Jorge Ramirez-Ortiz wrote:
> Fix a race condition that happens when device_initcall(pl011_dma_initicall)
> is executed before all the devices have been probed - this issue was observed on
> an hisi_6220 SoC (HiKey board from Linaro).
>
> The deferred driver probing framework relies on late_initcall to trigger
> deferred probes so it is just possible that, even with a valid DMA driver ready
> to be loaded, we fail to synchronize with it.
>
> The proposed implementation delays probing of the DMA until the uart device is
> opened. As hw_init is invoked on port startup and port resume - but the DMA
> probe is only required once - we avoid calling multiple times using a new field
> in uart_amba_port to track this scenario.
>
> This commit allows for subsequent attempts to associate an external DMA if the
> DMA driver was deferred at UART hw_init time.
>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> ---
>  drivers/tty/serial/amba-pl011.c | 95 +++++++++++++----------------------------
>  1 file changed, 30 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 8d94c19..4ce98e9 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -164,6 +164,7 @@ struct uart_amba_port {
>  	bool			using_rx_dma;
>  	struct pl011_dmarx_data dmarx;
>  	struct pl011_dmatx_data	dmatx;
> +	bool			dma_probed;
>  #endif
>  };
>  
> @@ -261,10 +262,11 @@ static void pl011_sgbuf_free(struct dma_chan *chan, struct pl011_sgbuf *sg,
>  	}
>  }
>  
> -static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *uap)
> +static void pl011_dma_probe(struct uart_amba_port *uap)
>  {
>  	/* DMA is the sole user of the platform data right now */
> -	struct amba_pl011_data *plat = dev_get_platdata(uap->port.dev);
> +	struct device *const uap_dev = uap->port.dev;
> +	struct amba_pl011_data *plat = dev_get_platdata(uap_dev);
>  	struct dma_slave_config tx_conf = {
>  		.dst_addr = uap->port.mapbase + UART01x_DR,
>  		.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE,
> @@ -275,12 +277,19 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>  	struct dma_chan *chan;
>  	dma_cap_mask_t mask;
>  
> -	chan = dma_request_slave_channel(dev, "tx");
> +	uap->dma_probed = true;
> +
> +	chan = dma_request_slave_channel_reason(uap_dev, "tx");
> +	if (IS_ERR(chan)) {
> +		if (PTR_ERR(chan) == -EPROBE_DEFER) {
> +			dev_info(uap_dev, "DMA driver not ready\n");
> +			uap->dma_probed = false;
> +			return;
> +		}
>  
> -	if (!chan) {
>  		/* We need platform data */
>  		if (!plat || !plat->dma_filter) {
> -			dev_info(uap->port.dev, "no DMA platform data\n");
> +			dev_info(uap_dev, "no DMA platform data\n");
>  			return;
>  		}
>  
> @@ -291,7 +300,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>  		chan = dma_request_channel(mask, plat->dma_filter,
>  						plat->dma_tx_param);
>  		if (!chan) {
> -			dev_err(uap->port.dev, "no TX DMA channel!\n");
> +			dev_err(uap_dev, "no TX DMA channel!\n");
>  			return;
>  		}
>  	}
> @@ -299,17 +308,17 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>  	dmaengine_slave_config(chan, &tx_conf);
>  	uap->dmatx.chan = chan;
>  
> -	dev_info(uap->port.dev, "DMA channel TX %s\n",
> +	dev_info(uap_dev, "DMA channel TX %s\n",
>  		 dma_chan_name(uap->dmatx.chan));
>  
>  	/* Optionally make use of an RX channel as well */
> -	chan = dma_request_slave_channel(dev, "rx");
> +	chan = dma_request_slave_channel(uap_dev, "rx");
>  
>  	if (!chan && plat->dma_rx_param) {
>  		chan = dma_request_channel(mask, plat->dma_filter, plat->dma_rx_param);
>  
>  		if (!chan) {
> -			dev_err(uap->port.dev, "no RX DMA channel!\n");
> +			dev_err(uap_dev, "no RX DMA channel!\n");
>  			return;
>  		}
>  	}
> @@ -333,7 +342,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>  			if (caps.residue_granularity ==
>  					DMA_RESIDUE_GRANULARITY_DESCRIPTOR) {
>  				dma_release_channel(chan);
> -				dev_info(uap->port.dev,
> +				dev_info(uap_dev,
>  					"RX DMA disabled - no residue processing\n");
>  				return;
>  			}
> @@ -362,75 +371,29 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>  					plat->dma_rx_poll_timeout;
>  			else
>  				uap->dmarx.poll_timeout = 3000;
> -		} else if (!plat && dev->of_node) {
> +		} else if (!plat && uap_dev->of_node) {
>  			uap->dmarx.auto_poll_rate = of_property_read_bool(
> -						dev->of_node, "auto-poll");
> +						uap_dev->of_node, "auto-poll");
>  			if (uap->dmarx.auto_poll_rate) {
>  				u32 x;
>  
> -				if (0 == of_property_read_u32(dev->of_node,
> +				if (0 == of_property_read_u32(uap_dev->of_node,
>  						"poll-rate-ms", &x))
>  					uap->dmarx.poll_rate = x;
>  				else
>  					uap->dmarx.poll_rate = 100;
> -				if (0 == of_property_read_u32(dev->of_node,
> +				if (0 == of_property_read_u32(uap_dev->of_node,
>  						"poll-timeout-ms", &x))
>  					uap->dmarx.poll_timeout = x;
>  				else
>  					uap->dmarx.poll_timeout = 3000;
>  			}
>  		}
> -		dev_info(uap->port.dev, "DMA channel RX %s\n",
> +		dev_info(uap_dev, "DMA channel RX %s\n",
>  			 dma_chan_name(uap->dmarx.chan));
>  	}
>  }
>  
> -#ifndef MODULE
> -/*
> - * Stack up the UARTs and let the above initcall be done at device
> - * initcall time, because the serial driver is called as an arch
> - * initcall, and at this time the DMA subsystem is not yet registered.
> - * At this point the driver will switch over to using DMA where desired.
> - */
> -struct dma_uap {
> -	struct list_head node;
> -	struct uart_amba_port *uap;
> -	struct device *dev;
> -};
> -
> -static LIST_HEAD(pl011_dma_uarts);
> -
> -static int __init pl011_dma_initcall(void)
> -{
> -	struct list_head *node, *tmp;
> -
> -	list_for_each_safe(node, tmp, &pl011_dma_uarts) {
> -		struct dma_uap *dmau = list_entry(node, struct dma_uap, node);
> -		pl011_dma_probe_initcall(dmau->dev, dmau->uap);
> -		list_del(node);
> -		kfree(dmau);
> -	}
> -	return 0;
> -}
> -
> -device_initcall(pl011_dma_initcall);
> -
> -static void pl011_dma_probe(struct device *dev, struct uart_amba_port *uap)
> -{
> -	struct dma_uap *dmau = kzalloc(sizeof(struct dma_uap), GFP_KERNEL);
> -	if (dmau) {
> -		dmau->uap = uap;
> -		dmau->dev = dev;
> -		list_add_tail(&dmau->node, &pl011_dma_uarts);
> -	}
> -}
> -#else
> -static void pl011_dma_probe(struct device *dev, struct uart_amba_port *uap)
> -{
> -	pl011_dma_probe_initcall(dev, uap);
> -}
> -#endif
> -
>  static void pl011_dma_remove(struct uart_amba_port *uap)
>  {
>  	/* TODO: remove the initcall if it has not yet executed */
> @@ -1142,7 +1105,7 @@ static inline bool pl011_dma_rx_running(struct uart_amba_port *uap)
>  
>  #else
>  /* Blank functions if the DMA engine is not available */
> -static inline void pl011_dma_probe(struct device *dev, struct uart_amba_port *uap)
> +static inline void pl011_dma_probe(struct uart_amba_port *uap)
>  {
>  }
>  
> @@ -1548,6 +1511,9 @@ static int pl011_hwinit(struct uart_port *port)
>  	uap->im = readw(uap->port.membase + UART011_IMSC);
>  	writew(UART011_RTIM | UART011_RXIM, uap->port.membase + UART011_IMSC);
>  
> +	if (!uap->dma_probed)
> +		pl011_dma_probe(uap);
> +
>  	if (dev_get_platdata(uap->port.dev)) {
>  		struct amba_pl011_data *plat;
>  
> @@ -2218,7 +2184,6 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
>  	uap->port.ops = &amba_pl011_pops;
>  	uap->port.flags = UPF_BOOT_AUTOCONF;
>  	uap->port.line = i;
> -	pl011_dma_probe(&dev->dev, uap);
>  
>  	/* Ensure interrupts from this UART are masked and cleared */
>  	writew(0, uap->port.membase + UART011_IMSC);
> @@ -2233,7 +2198,8 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
>  	if (!amba_reg.state) {
>  		ret = uart_register_driver(&amba_reg);
>  		if (ret < 0) {
> -			pr_err("Failed to register AMBA-PL011 driver\n");
> +			dev_err(&dev->dev,
> +				"Failed to register AMBA-PL011 driver\n");
>  			return ret;
>  		}
>  	}
> @@ -2242,7 +2208,6 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
>  	if (ret) {
>  		amba_ports[i] = NULL;
>  		uart_unregister_driver(&amba_reg);
> -		pl011_dma_remove(uap);
>  	}
>  
>  	return ret;


just following up on this fix (the current driver is broken on my tests).
Is it acceptable at this point?

[PATCH v5] drivers/tty: amba: defer probing DMA availability until hw_init

[Rob Herring]

On Thu, Feb 26, 2015 at 10:56 AM, Jorge Ramirez-Ortiz
<jorge.ramirez-ortiz@linaro.org> wrote:
> Fix a race condition that happens when device_initcall(pl011_dma_initicall)
> is executed before all the devices have been probed - this issue was observed on
> an hisi_6220 SoC (HiKey board from Linaro).
>
> The deferred driver probing framework relies on late_initcall to trigger
> deferred probes so it is just possible that, even with a valid DMA driver ready
> to be loaded, we fail to synchronize with it.
>
> The proposed implementation delays probing of the DMA until the uart device is
> opened. As hw_init is invoked on port startup and port resume - but the DMA
> probe is only required once - we avoid calling multiple times using a new field
> in uart_amba_port to track this scenario.
>
> This commit allows for subsequent attempts to associate an external DMA if the
> DMA driver was deferred at UART hw_init time.
>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

You need to send this to Greg KH and linux-serial if you want it applied.

[...]

> @@ -261,10 +262,11 @@ static void pl011_sgbuf_free(struct dma_chan *chan, struct pl011_sgbuf *sg,
>         }
>  }
>
> -static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *uap)
> +static void pl011_dma_probe(struct uart_amba_port *uap)
>  {
>         /* DMA is the sole user of the platform data right now */
> -       struct amba_pl011_data *plat = dev_get_platdata(uap->port.dev);
> +       struct device *const uap_dev = uap->port.dev;

Call this "dev" so you don't have all the needless renaming.

With that change:

Reviewed-by: Rob Herring <robh@kernel.org>

> +       struct amba_pl011_data *plat = dev_get_platdata(uap_dev);
>         struct dma_slave_config tx_conf = {
>                 .dst_addr = uap->port.mapbase + UART01x_DR,
>                 .dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE,
> @@ -275,12 +277,19 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>         struct dma_chan *chan;
>         dma_cap_mask_t mask;
>
> -       chan = dma_request_slave_channel(dev, "tx");
> +       uap->dma_probed = true;
> +
> +       chan = dma_request_slave_channel_reason(uap_dev, "tx");
> +       if (IS_ERR(chan)) {
> +               if (PTR_ERR(chan) == -EPROBE_DEFER) {
> +                       dev_info(uap_dev, "DMA driver not ready\n");
> +                       uap->dma_probed = false;
> +                       return;
> +               }
>
> -       if (!chan) {
>                 /* We need platform data */
>                 if (!plat || !plat->dma_filter) {
> -                       dev_info(uap->port.dev, "no DMA platform data\n");
> +                       dev_info(uap_dev, "no DMA platform data\n");
>                         return;
>                 }
>
> @@ -291,7 +300,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>                 chan = dma_request_channel(mask, plat->dma_filter,
>                                                 plat->dma_tx_param);
>                 if (!chan) {
> -                       dev_err(uap->port.dev, "no TX DMA channel!\n");
> +                       dev_err(uap_dev, "no TX DMA channel!\n");
>                         return;
>                 }
>         }
> @@ -299,17 +308,17 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>         dmaengine_slave_config(chan, &tx_conf);
>         uap->dmatx.chan = chan;
>
> -       dev_info(uap->port.dev, "DMA channel TX %s\n",
> +       dev_info(uap_dev, "DMA channel TX %s\n",
>                  dma_chan_name(uap->dmatx.chan));
>
>         /* Optionally make use of an RX channel as well */
> -       chan = dma_request_slave_channel(dev, "rx");
> +       chan = dma_request_slave_channel(uap_dev, "rx");
>
>         if (!chan && plat->dma_rx_param) {
>                 chan = dma_request_channel(mask, plat->dma_filter, plat->dma_rx_param);
>
>                 if (!chan) {
> -                       dev_err(uap->port.dev, "no RX DMA channel!\n");
> +                       dev_err(uap_dev, "no RX DMA channel!\n");
>                         return;
>                 }
>         }
> @@ -333,7 +342,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>                         if (caps.residue_granularity ==
>                                         DMA_RESIDUE_GRANULARITY_DESCRIPTOR) {
>                                 dma_release_channel(chan);
> -                               dev_info(uap->port.dev,
> +                               dev_info(uap_dev,
>                                         "RX DMA disabled - no residue processing\n");
>                                 return;
>                         }
> @@ -362,75 +371,29 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>                                         plat->dma_rx_poll_timeout;
>                         else
>                                 uap->dmarx.poll_timeout = 3000;
> -               } else if (!plat && dev->of_node) {
> +               } else if (!plat && uap_dev->of_node) {
>                         uap->dmarx.auto_poll_rate = of_property_read_bool(
> -                                               dev->of_node, "auto-poll");
> +                                               uap_dev->of_node, "auto-poll");
>                         if (uap->dmarx.auto_poll_rate) {
>                                 u32 x;
>
> -                               if (0 == of_property_read_u32(dev->of_node,
> +                               if (0 == of_property_read_u32(uap_dev->of_node,
>                                                 "poll-rate-ms", &x))
>                                         uap->dmarx.poll_rate = x;
>                                 else
>                                         uap->dmarx.poll_rate = 100;
> -                               if (0 == of_property_read_u32(dev->of_node,
> +                               if (0 == of_property_read_u32(uap_dev->of_node,
>                                                 "poll-timeout-ms", &x))
>                                         uap->dmarx.poll_timeout = x;
>                                 else
>                                         uap->dmarx.poll_timeout = 3000;
>                         }
>                 }
> -               dev_info(uap->port.dev, "DMA channel RX %s\n",
> +               dev_info(uap_dev, "DMA channel RX %s\n",
>                          dma_chan_name(uap->dmarx.chan));
>         }
>  }
>
> -#ifndef MODULE
> -/*
> - * Stack up the UARTs and let the above initcall be done at device
> - * initcall time, because the serial driver is called as an arch
> - * initcall, and at this time the DMA subsystem is not yet registered.
> - * At this point the driver will switch over to using DMA where desired.
> - */
> -struct dma_uap {
> -       struct list_head node;
> -       struct uart_amba_port *uap;
> -       struct device *dev;
> -};
> -
> -static LIST_HEAD(pl011_dma_uarts);
> -
> -static int __init pl011_dma_initcall(void)
> -{
> -       struct list_head *node, *tmp;
> -
> -       list_for_each_safe(node, tmp, &pl011_dma_uarts) {
> -               struct dma_uap *dmau = list_entry(node, struct dma_uap, node);
> -               pl011_dma_probe_initcall(dmau->dev, dmau->uap);
> -               list_del(node);
> -               kfree(dmau);
> -       }
> -       return 0;
> -}
> -
> -device_initcall(pl011_dma_initcall);
> -
> -static void pl011_dma_probe(struct device *dev, struct uart_amba_port *uap)
> -{
> -       struct dma_uap *dmau = kzalloc(sizeof(struct dma_uap), GFP_KERNEL);
> -       if (dmau) {
> -               dmau->uap = uap;
> -               dmau->dev = dev;
> -               list_add_tail(&dmau->node, &pl011_dma_uarts);
> -       }
> -}
> -#else
> -static void pl011_dma_probe(struct device *dev, struct uart_amba_port *uap)
> -{
> -       pl011_dma_probe_initcall(dev, uap);
> -}
> -#endif
> -
>  static void pl011_dma_remove(struct uart_amba_port *uap)
>  {
>         /* TODO: remove the initcall if it has not yet executed */
> @@ -1142,7 +1105,7 @@ static inline bool pl011_dma_rx_running(struct uart_amba_port *uap)
>
>  #else
>  /* Blank functions if the DMA engine is not available */
> -static inline void pl011_dma_probe(struct device *dev, struct uart_amba_port *uap)
> +static inline void pl011_dma_probe(struct uart_amba_port *uap)
>  {
>  }
>
> @@ -1548,6 +1511,9 @@ static int pl011_hwinit(struct uart_port *port)
>         uap->im = readw(uap->port.membase + UART011_IMSC);
>         writew(UART011_RTIM | UART011_RXIM, uap->port.membase + UART011_IMSC);
>
> +       if (!uap->dma_probed)
> +               pl011_dma_probe(uap);
> +
>         if (dev_get_platdata(uap->port.dev)) {
>                 struct amba_pl011_data *plat;
>
> @@ -2218,7 +2184,6 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
>         uap->port.ops = &amba_pl011_pops;
>         uap->port.flags = UPF_BOOT_AUTOCONF;
>         uap->port.line = i;
> -       pl011_dma_probe(&dev->dev, uap);
>
>         /* Ensure interrupts from this UART are masked and cleared */
>         writew(0, uap->port.membase + UART011_IMSC);
> @@ -2233,7 +2198,8 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
>         if (!amba_reg.state) {
>                 ret = uart_register_driver(&amba_reg);
>                 if (ret < 0) {
> -                       pr_err("Failed to register AMBA-PL011 driver\n");
> +                       dev_err(&dev->dev,
> +                               "Failed to register AMBA-PL011 driver\n");
>                         return ret;
>                 }
>         }
> @@ -2242,7 +2208,6 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
>         if (ret) {
>                 amba_ports[i] = NULL;
>                 uart_unregister_driver(&amba_reg);
> -               pl011_dma_remove(uap);
>         }
>
>         return ret;
> --
> 1.9.1
>

[PATCH v5] drivers/tty: amba: defer probing DMA availability until hw_init

[Jorge Ramirez-Ortiz]

On 03/03/2015 11:13 AM, Rob Herring wrote:
> On Thu, Feb 26, 2015 at 10:56 AM, Jorge Ramirez-Ortiz
> <jorge.ramirez-ortiz@linaro.org> wrote:
>> Fix a race condition that happens when device_initcall(pl011_dma_initicall)
>> is executed before all the devices have been probed - this issue was observed on
>> an hisi_6220 SoC (HiKey board from Linaro).
>>
>> The deferred driver probing framework relies on late_initcall to trigger
>> deferred probes so it is just possible that, even with a valid DMA driver ready
>> to be loaded, we fail to synchronize with it.
>>
>> The proposed implementation delays probing of the DMA until the uart device is
>> opened. As hw_init is invoked on port startup and port resume - but the DMA
>> probe is only required once - we avoid calling multiple times using a new field
>> in uart_amba_port to track this scenario.
>>
>> This commit allows for subsequent attempts to associate an external DMA if the
>> DMA driver was deferred at UART hw_init time.
>>
>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> You need to send this to Greg KH and linux-serial if you want it applied.
>
> [...]

ok.

>
>> @@ -261,10 +262,11 @@ static void pl011_sgbuf_free(struct dma_chan *chan, struct pl011_sgbuf *sg,
>>         }
>>  }
>>
>> -static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *uap)
>> +static void pl011_dma_probe(struct uart_amba_port *uap)
>>  {
>>         /* DMA is the sole user of the platform data right now */
>> -       struct amba_pl011_data *plat = dev_get_platdata(uap->port.dev);
>> +       struct device *const uap_dev = uap->port.dev;
> Call this "dev" so you don't have all the needless renaming.
>
> With that change:
>
> Reviewed-by: Rob Herring <robh@kernel.org>


OK.

while I am at it, you initially suggested to do the probe in pl011_dma_startup.

Instead I probed from pl011_hwinit since it matched the original behaviour of
also probing the DMA when CONFIG_CONSOLE_POLL was enabled.
However, since the CONFIG_CONSOLE_POLL implementation for the amba-pl011 doesn't
use the DMA it actually doesn't help to keep it in pl011_hwinit.

is it ok with you if I move this to pl011_dma_startup as you initially suggested?


>
>> +       struct amba_pl011_data *plat = dev_get_platdata(uap_dev);
>>         struct dma_slave_config tx_conf = {
>>                 .dst_addr = uap->port.mapbase + UART01x_DR,
>>                 .dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE,
>> @@ -275,12 +277,19 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>>         struct dma_chan *chan;
>>         dma_cap_mask_t mask;
>>
>> -       chan = dma_request_slave_channel(dev, "tx");
>> +       uap->dma_probed = true;
>> +
>> +       chan = dma_request_slave_channel_reason(uap_dev, "tx");
>> +       if (IS_ERR(chan)) {
>> +               if (PTR_ERR(chan) == -EPROBE_DEFER) {
>> +                       dev_info(uap_dev, "DMA driver not ready\n");
>> +                       uap->dma_probed = false;
>> +                       return;
>> +               }
>>
>> -       if (!chan) {
>>                 /* We need platform data */
>>                 if (!plat || !plat->dma_filter) {
>> -                       dev_info(uap->port.dev, "no DMA platform data\n");
>> +                       dev_info(uap_dev, "no DMA platform data\n");
>>                         return;
>>                 }amba-pl011.c
>>
>> @@ -291,7 +300,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>>                 chan = dma_request_channel(mask, plat->dma_filter,
>>                                                 plat->dma_tx_param);
>>                 if (!chan) {
>> -                       dev_err(uap->port.dev, "no TX DMA channel!\n");
>> +                       dev_err(uap_dev, "no TX DMA channel!\n");
>>                         return;
>>                 }
>>         }
>> @@ -299,17 +308,17 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>>         dmaengine_slave_config(chan, &tx_conf);
>>         uap->dmatx.chan = chan;
>>
>> -       dev_info(uap->port.dev, "DMA channel TX %s\n",
>> +       dev_info(uap_dev, "DMA channel TX %s\n",
>>                  dma_chan_name(uap->dmatx.chan));
>>
>>         /* Optionally make use of an RX channel as well */
>> -       chan = dma_request_slave_channel(dev, "rx");
>> +       chan = dma_request_slave_channel(uap_dev, "rx");
>>
>>         if (!chan && plat->dma_rx_param) {
>>                 chan = dma_request_channel(mask, plat->dma_filter, plat->dma_rx_param);
>>
>>                 if (!chan) {
>> -                       dev_err(uap->port.dev, "no RX DMA channel!\n");
>> +                       dev_err(uap_dev, "no RX DMA channel!\n");
>>                         return;
>>                 }
>>         }
>> @@ -333,7 +342,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>>                         if (caps.residue_granularity ==
>>                                         DMA_RESIDUE_GRANULARITY_DESCRIPTOR) {
>>                                 dma_release_channel(chan);
>> -                               dev_info(uap->port.dev,
>> +                               dev_info(uap_dev,
>>                                         "RX DMA disabled - no residue processing\n");
>>                                 return;
>>                         }
>> @@ -362,75 +371,29 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
>>                                         plat->dma_rx_poll_timeout;
>>                         else
>>                                 uap->dmarx.poll_timeout = 3000;
>> -               } else if (!plat && dev->of_node) {
>> +               } else if (!plat && uap_dev->of_node) {
>>                         uap->dmarx.auto_poll_rate = of_property_read_bool(
>> -                                               dev->of_node, "auto-poll");
>> +                                               uap_dev->of_node, "auto-poll");
>>                         if (uap->dmarx.auto_poll_rate) {
>>                                 u32 x;
>>
>> -                               if (0 == of_property_read_u32(dev->of_node,
>> +                               if (0 == of_property_read_u32(uap_dev->of_node,
>>                                                 "poll-rate-ms", &x))
>>                                         uap->dmarx.poll_rate = x;
>>                                 else
>>                                         uap->dmarx.poll_rate = 100;
>> -                               if (0 == of_property_read_u32(dev->of_node,
>> +                               if (0 == of_property_read_u32(uap_dev->of_node,
>>                                                 "poll-timeout-ms", &x))
>>                                         uap->dmarx.poll_timeout = x;
>>                                 else
>>                                         uap->dmarx.poll_timeout = 3000;
>>                         }
>>                 }
>> -               dev_info(uap->port.dev, "DMA channel RX %s\n",
>> +               dev_info(uap_dev, "DMA channel RX %s\n",
>>                          dma_chan_name(uap->dmarx.chan));
>>         }
>>  }
>>
>> -#ifndef MODULE
>> -/*
>> - * Stack up the UARTs and let the above initcall be done at device
>> - * initcall time, because the serial driver is called as an arch
>> - * initcall, and at this time the DMA subsystem is not yet registered.
>> - * At this point the driver will switch over to using DMA where desired.
>> - */
>> -struct dma_uap {
>> -       struct list_head node;
>> -       struct uart_amba_port *uap;
>> -       struct device *dev;
>> -};
>> -
>> -static LIST_HEAD(pl011_dma_uarts);
>> -
>> -static int __init pl011_dma_initcall(void)
>> -{
>> -       struct list_head *node, *tmp;
>> -
>> -       list_for_each_safe(node, tmp, &pl011_dma_uarts) {
>> -               struct dma_uap *dmau = list_entry(node, struct dma_uap, node);
>> -               pl011_dma_probe_initcall(dmau->dev, dmau->uap);
>> -               list_del(node);
>> -               kfree(dmau);
>> -       }
>> -       return 0;
>> -}
>> -
>> -device_initcall(pl011_dma_initcall);
>> -
>> -static void pl011_dma_probe(struct device *dev, struct uart_amba_port *uap)
>> -{
>> -       struct dma_uap *dmau = kzalloc(sizeof(struct dma_uap), GFP_KERNEL);
>> -       if (dmau) {
>> -               dmau->uap = uap;
>> -               dmau->dev = dev;
>> -               list_add_tail(&dmau->node, &pl011_dma_uarts);
>> -       }
>> -}
>> -#else
>> -static void pl011_dma_probe(struct device *dev, struct uart_amba_port *uap)
>> -{
>> -       pl011_dma_probe_initcall(dev, uap);
>> -}
>> -#endif
>> -
>>  static void pl011_dma_remove(struct uart_amba_port *uap)
>>  {
>>         /* TODO: remove the initcall if it has not yet executed */
>> @@ -1142,7 +1105,7 @@ static inline bool pl011_dma_rx_running(struct uart_amba_port *uap)
>>
>>  #else
>>  /* Blank functions if the DMA engine is not available */
>> -static inline void pl011_dma_probe(struct device *dev, struct uart_amba_port *uap)
>> +static inline void pl011_dma_probe(struct uart_amba_port *uap)
>>  {
>>  }
>>
>> @@ -1548,6 +1511,9 @@ static int pl011_hwinit(struct uart_port *port)
>>         uap->im = readw(uap->port.membase + UART011_IMSC);
>>         writew(UART011_RTIM | UART011_RXIM, uap->port.membase + UART011_IMSC);
>>
>> +       if (!uap->dma_probed)
>> +               pl011_dma_probe(uap);
>> +
>>         if (dev_get_platdata(uap->port.dev)) {
>>                 struct amba_pl011_data *plat;
>>
>> @@ -2218,7 +2184,6 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
>>         uap->port.ops = &amba_pl011_pops;
>>         uap->port.flags = UPF_BOOT_AUTOCONF;
>>         uap->port.line = i;
>> -       pl011_dma_probe(&dev->dev, uap);
>>
>>         /* Ensure interrupts from this UART are masked and cleared */
>>         writew(0, uap->port.membase + UART011_IMSC);
>> @@ -2233,7 +2198,8 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
>>         if (!amba_reg.state) {
>>                 ret = uart_register_driver(&amba_reg);
>>                 if (ret < 0) {
>> -                       pr_err("Failed to register AMBA-PL011 driver\n");
>> +                       dev_err(&dev->dev,
>> +                               "Failed to register AMBA-PL011 driver\n");
>>                         return ret;
>>                 }
>>         }
>> @@ -2242,7 +2208,6 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
>>         if (ret) {
>>                 amba_ports[i] = NULL;
>>                 uart_unregister_driver(&amba_reg);
>> -               pl011_dma_remove(uap);
>>         }
>>
>>         return ret;
>> --
>> 1.9.1
>>

[PATCH v5] drivers/tty: amba: defer probing DMA availability until hw_init

[Rob Herring]

On Wed, Mar 4, 2015 at 3:33 PM, Jorge Ramirez-Ortiz
<jorge.ramirez-ortiz@linaro.org> wrote:
> On 03/03/2015 11:13 AM, Rob Herring wrote:
>> On Thu, Feb 26, 2015 at 10:56 AM, Jorge Ramirez-Ortiz
>> <jorge.ramirez-ortiz@linaro.org> wrote:
>>> Fix a race condition that happens when device_initcall(pl011_dma_initicall)
>>> is executed before all the devices have been probed - this issue was observed on
>>> an hisi_6220 SoC (HiKey board from Linaro).

[...]

>>> @@ -261,10 +262,11 @@ static void pl011_sgbuf_free(struct dma_chan *chan, struct pl011_sgbuf *sg,
>>>         }
>>>  }
>>>
>>> -static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *uap)
>>> +static void pl011_dma_probe(struct uart_amba_port *uap)
>>>  {
>>>         /* DMA is the sole user of the platform data right now */
>>> -       struct amba_pl011_data *plat = dev_get_platdata(uap->port.dev);
>>> +       struct device *const uap_dev = uap->port.dev;
>> Call this "dev" so you don't have all the needless renaming.
>>
>> With that change:
>>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>
>
> OK.
>
> while I am at it, you initially suggested to do the probe in pl011_dma_startup.
>
> Instead I probed from pl011_hwinit since it matched the original behaviour of
> also probing the DMA when CONFIG_CONSOLE_POLL was enabled.
> However, since the CONFIG_CONSOLE_POLL implementation for the amba-pl011 doesn't
> use the DMA it actually doesn't help to keep it in pl011_hwinit.
>
> is it ok with you if I move this to pl011_dma_startup as you initially suggested?

That sounds fine.

Rob

[PATCH v5] drivers/tty: amba: defer probing DMA availability until hw_init

[Russell King - ARM Linux]

On Tue, Mar 03, 2015 at 11:06:32AM -0500, Jorge Ramirez-Ortiz wrote:
> On 02/26/2015 11:56 AM, Jorge Ramirez-Ortiz wrote:
> > -	chan = dma_request_slave_channel(dev, "tx");
> > +	uap->dma_probed = true;
> > +
> > +	chan = dma_request_slave_channel_reason(uap_dev, "tx");
> > +	if (IS_ERR(chan)) {
> > +		if (PTR_ERR(chan) == -EPROBE_DEFER) {
> > +			dev_info(uap_dev, "DMA driver not ready\n");

I still object to this.

It _can't_ be right that we plaster the kernel console with these
messages when the is a DMA possible, but the DMA driver is a module
which hasn't been loaded yet.

You probably don't realise it, but init daemons tend to open the
console, write their message, and then close it again.  What your
message above means is that each time an init daemon does that, we
get a "DMA driver not ready" message.

IMHO, that is unacceptable.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v5] drivers/tty: amba: defer probing DMA availability until hw_init

[Russell King - ARM Linux]

On Tue, Mar 03, 2015 at 10:13:21AM -0600, Rob Herring wrote:
> On Thu, Feb 26, 2015 at 10:56 AM, Jorge Ramirez-Ortiz
> <jorge.ramirez-ortiz@linaro.org> wrote:
> > Fix a race condition that happens when device_initcall(pl011_dma_initicall)
> > is executed before all the devices have been probed - this issue was observed on
> > an hisi_6220 SoC (HiKey board from Linaro).
> >
> > The deferred driver probing framework relies on late_initcall to trigger
> > deferred probes so it is just possible that, even with a valid DMA driver ready
> > to be loaded, we fail to synchronize with it.
> >
> > The proposed implementation delays probing of the DMA until the uart device is
> > opened. As hw_init is invoked on port startup and port resume - but the DMA
> > probe is only required once - we avoid calling multiple times using a new field
> > in uart_amba_port to track this scenario.
> >
> > This commit allows for subsequent attempts to associate an external DMA if the
> > DMA driver was deferred at UART hw_init time.
> >
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> 
> You need to send this to Greg KH and linux-serial if you want it applied.

ARM PRIMECELL UART PL010 AND PL011 DRIVERS
M:      Russell King <linux@arm.linux.org.uk>
S:      Maintained
F:      drivers/tty/serial/amba-pl01*.c
F:      include/linux/amba/serial.h

And you really should wait for _my_ approval and ack, especially as I've
already highlighted concerns in previous replies to the change of DMA
management thing.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v5] drivers/tty: amba: defer probing DMA availability until hw_init

[Jorge Ramirez-Ortiz]

On 03/09/2015 11:57 AM, Russell King - ARM Linux wrote:
> On Tue, Mar 03, 2015 at 11:06:32AM -0500, Jorge Ramirez-Ortiz wrote:
>> On 02/26/2015 11:56 AM, Jorge Ramirez-Ortiz wrote:
>>> -	chan = dma_request_slave_channel(dev, "tx");
>>> +	uap->dma_probed = true;
>>> +
>>> +	chan = dma_request_slave_channel_reason(uap_dev, "tx");
>>> +	if (IS_ERR(chan)) {
>>> +		if (PTR_ERR(chan) == -EPROBE_DEFER) {
>>> +			dev_info(uap_dev, "DMA driver not ready\n");
> I still object to this.

it was an oversight, not intentional. my fault.

>
> It _can't_ be right that we plaster the kernel console with these
> messages when the is a DMA possible, but the DMA driver is a module
> which hasn't been loaded yet.
>
> You probably don't realise it, but init daemons tend to open the
> console, write their message, and then close it again.  What your
> message above means is that each time an init daemon does that, we
> get a "DMA driver not ready" message.

I initially - before your first remarks- did think about the init daemons and
balancing the value I saw in having the message in (for product developers) I
thought it was worth having it.

What I didnt know relates to your second point about modules:

- That for as long as the device tree declares a DMA name that matches the one
that the UART requests in its DT settings, attempting to request a channel on it
before said DMA driver has been registered would be returning EPROBE_DEFER (same
thing for ACPI).

- And yes, as the situation described above could go on for ever (maybe the
driver was blacklisted) the clutter in the log would be unacceptable.

[I had only looked at defer driver probing from the perspective of its internal
lists (deferred_probe_ending/active_list) and did not have the overall picture
in mind. So I missed this point]

>
> IMHO, that is unacceptable.
>

agree.