| Message ID | 1424749613-24765-2-git-send-email-jorge.ramirez-ortiz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 23 February 2015 22:46:53 Jorge Ramirez-Ortiz wrote: > This patch addresses 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). How do you want to handle the case where there is a DMA channel in DT, but no driver for it built into the kernel? > @@ -275,15 +277,23 @@ 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"); > + if (queue_dma_probe && plat && plat->dma_filter) { > + (*queue_dma_probe)(dev, uap); > + return 0; > + } > + > + chan = dma_request_slave_channel_reason(dev, "tx"); > + if (IS_ERR(chan)) { > + > + /* the dma driver has not been initialized yet */ > + if (PTR_ERR(chan) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > > - if (!chan) { > /* We need platform data */ > if (!plat || !plat->dma_filter) { > dev_info(uap->port.dev, "no DMA platform data\n"); > - return; > + return 0; > } It would be nice to print the error code here now that you know it and can tell the difference between no DMA channel being supplied and the DMA channel from the DT properties being unavailable. > /* Try to acquire a generic DMA engine slave TX channel */ > dma_cap_zero(mask); > dma_cap_set(DMA_SLAVE, mask); > @@ -292,7 +302,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port * > plat->dma_tx_param); > if (!chan) { > dev_err(uap->port.dev, "no TX DMA channel!\n"); > - return; > + return 0; > } > } > > @@ -310,7 +320,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port * > > if (!chan) { > dev_err(uap->port.dev, "no RX DMA channel!\n"); > - return; > + return 0; > } > } I think the condition is wrong now that 'chan' contains a ERR_PTR value in case of failure. Arnd
On 02/24/2015 03:29 AM, Arnd Bergmann wrote: > On Monday 23 February 2015 22:46:53 Jorge Ramirez-Ortiz wrote: >> This patch addresses 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). > How do you want to handle the case where there is a DMA channel in DT, but > no driver for it built into the kernel? In my view that should be fixed in the of-dma and the acpi-dma implementation (just don't return EPROBE_DEFER a second time since the driver wasn't found after the late_initcall associated to deferred probing). That would allow drivers that use the defer probing mechanism to wait for optional controllers to complete their probing sequence if those controllers don't become available. In any case, I can track the pending requests (up to twelve, one per UART) with a list in the driver. If a second request to probe fails with EPROBE_DEFER we can conclude that the driver is not present. I have tested such an implementation and it works as expected. would that be acceptable? >> @@ -275,15 +277,23 @@ 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"); >> + if (queue_dma_probe && plat && plat->dma_filter) { >> + (*queue_dma_probe)(dev, uap); >> + return 0; >> + } >> + >> + chan = dma_request_slave_channel_reason(dev, "tx"); >> + if (IS_ERR(chan)) { >> + >> + /* the dma driver has not been initialized yet */ >> + if (PTR_ERR(chan) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; >> >> - if (!chan) { >> /* We need platform data */ >> if (!plat || !plat->dma_filter) { >> dev_info(uap->port.dev, "no DMA platform data\n"); >> - return; >> + return 0; >> } > It would be nice to print the error code here now that you know it > and can tell the difference between no DMA channel being supplied > and the DMA channel from the DT properties being unavailable. ACKd > >> /* Try to acquire a generic DMA engine slave TX channel */ >> dma_cap_zero(mask); >> dma_cap_set(DMA_SLAVE, mask); >> @@ -292,7 +302,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port * >> plat->dma_tx_param); >> if (!chan) { >> dev_err(uap->port.dev, "no TX DMA channel!\n"); >> - return; >> + return 0; >> } >> } >> >> @@ -310,7 +320,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port * >> >> if (!chan) { >> dev_err(uap->port.dev, "no RX DMA channel!\n"); >> - return; >> + return 0; >> } >> } > I think the condition is wrong now that 'chan' contains a ERR_PTR > value in case of failure. Actually that is not the case on the RX DMA case: on the RX DMA case, chan contains either NULL or a valid pointer (not an ERR_PTR value) chan only contains an ERR_PTR on the first attempt to access the DMA controller (just so we can handle the defer probing case) On subsequent accesses, it assumes that it is there (since it would have been deferred if it wasn't).
On 02/24/2015 02:32 PM, Jorge Ramirez-Ortiz wrote: > On 02/24/2015 03:29 AM, Arnd Bergmann wrote: >> On Monday 23 February 2015 22:46:53 Jorge Ramirez-Ortiz wrote: >>> This patch addresses 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). >> How do you want to handle the case where there is a DMA channel in DT, but >> no driver for it built into the kernel? > > In my view that should be fixed in the of-dma and the acpi-dma implementation > (just don't return EPROBE_DEFER a second time since the driver wasn't found > after the late_initcall associated to deferred probing). That would allow > drivers that use the defer probing mechanism to wait for optional controllers to > complete their probing sequence if those controllers don't become available. > > In any case, I can track the pending requests (up to twelve, one per UART) with > a list in the driver. > If a second request to probe fails with EPROBE_DEFER we can conclude that the > driver is not present. > > I have tested such an implementation and it works as expected. > would that be acceptable? Something along these lines +struct dma_deferred { + struct list_head node; + struct device *dev; +}; + +static LIST_HEAD(dma_deferred_reqs); + +static int pl011_handle_dma_probe_defer(struct device *dev) +{ + struct list_head *node, *tmp; + struct dma_deferred *entry; + + list_for_each_safe(node, tmp, &dma_deferred_reqs) { + entry = list_entry(node, struct dma_deferred, node); + if (entry->dev == dev) { + dev_warn(dev, "DMA driver not present \n"); + list_del(node); + kfree(entry); + return -ENODEV; + } + } + + entry = kzalloc(sizeof(struct dma_deferred), GFP_KERNEL); + if (entry) { + entry->dev = dev; + list_add_tail(&entry->node, &dma_deferred_reqs); + dev_info(dev, "DMA controller not ready \n"); + return -EPROBE_DEFER; + } + + return 0; +} + +static int pl011_dma_probe(struct device *dev, struct uart_amba_port *uap, + void (*queue_dma_probe)(struct device *, struct uart_amba_port *)) { /* DMA is the sole user of the platform data right now */ struct amba_pl011_data *plat = dev_get_platdata(uap->port.dev); @@ -275,15 +310,25 @@ 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"); + if (queue_dma_probe && plat && plat->dma_filter) { + (*queue_dma_probe)(dev, uap); + return 0; + } + + chan = dma_request_slave_channel_reason(dev, "tx"); + if (IS_ERR(chan)) { + + /* the dma driver has not been initialized yet */ + if (PTR_ERR(chan) == -EPROBE_DEFER) + return pl011_handle_dma_probe_defer(dev); + + dev_info(uap->port.dev, "no OF or ACPI data \n"); > >>> @@ -275,15 +277,23 @@ 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"); >>> + if (queue_dma_probe && plat && plat->dma_filter) { >>> + (*queue_dma_probe)(dev, uap); >>> + return 0; >>> + } >>> + >>> + chan = dma_request_slave_channel_reason(dev, "tx"); >>> + if (IS_ERR(chan)) { >>> + >>> + /* the dma driver has not been initialized yet */ >>> + if (PTR_ERR(chan) == -EPROBE_DEFER) >>> + return -EPROBE_DEFER; >>> >>> - if (!chan) { >>> /* We need platform data */ >>> if (!plat || !plat->dma_filter) { >>> dev_info(uap->port.dev, "no DMA platform data\n"); >>> - return; >>> + return 0; >>> } >> It would be nice to print the error code here now that you know it >> and can tell the difference between no DMA channel being supplied >> and the DMA channel from the DT properties being unavailable. > ACKd >>> /* Try to acquire a generic DMA engine slave TX channel */ >>> dma_cap_zero(mask); >>> dma_cap_set(DMA_SLAVE, mask); >>> @@ -292,7 +302,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port * >>> plat->dma_tx_param); >>> if (!chan) { >>> dev_err(uap->port.dev, "no TX DMA channel!\n"); >>> - return; >>> + return 0; >>> } >>> } >>> >>> @@ -310,7 +320,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port * >>> >>> if (!chan) { >>> dev_err(uap->port.dev, "no RX DMA channel!\n"); >>> - return; >>> + return 0; >>> } >>> } >> I think the condition is wrong now that 'chan' contains a ERR_PTR >> value in case of failure. > Actually that is not the case on the RX DMA case: on the RX DMA case, chan > contains either NULL or a valid pointer (not an ERR_PTR value) > > chan only contains an ERR_PTR on the first attempt to access the DMA controller > (just so we can handle the defer probing case) > On subsequent accesses, it assumes that it is there (since it would have been > deferred if it wasn't). > > >
On Tuesday 24 February 2015 17:02:08 Jorge Ramirez-Ortiz wrote: > On 02/24/2015 02:32 PM, Jorge Ramirez-Ortiz wrote: > > On 02/24/2015 03:29 AM, Arnd Bergmann wrote: > >> On Monday 23 February 2015 22:46:53 Jorge Ramirez-Ortiz wrote: > >>> This patch addresses 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). > >> How do you want to handle the case where there is a DMA channel in DT, but > >> no driver for it built into the kernel? > > > > In my view that should be fixed in the of-dma and the acpi-dma implementation > > (just don't return EPROBE_DEFER a second time since the driver wasn't found > > after the late_initcall associated to deferred probing). That would allow > > drivers that use the defer probing mechanism to wait for optional controllers to > > complete their probing sequence if those controllers don't become available. > > > > In any case, I can track the pending requests (up to twelve, one per UART) with > > a list in the driver. > > If a second request to probe fails with EPROBE_DEFER we can conclude that the > > driver is not present. > > > > I have tested such an implementation and it works as expected. > > would that be acceptable? > > Something along these lines > > +struct dma_deferred { > + struct list_head node; > + struct device *dev; > +}; > + > +static LIST_HEAD(dma_deferred_reqs); > + > +static int pl011_handle_dma_probe_defer(struct device *dev) > +{ > + struct list_head *node, *tmp; > + struct dma_deferred *entry; > + > + list_for_each_safe(node, tmp, &dma_deferred_reqs) { > + entry = list_entry(node, struct dma_deferred, node); > + if (entry->dev == dev) { > + dev_warn(dev, "DMA driver not present \n"); > + list_del(node); > + kfree(entry); > + return -ENODEV; > + } > + } I don't see how this would help: Assume you probe the uart first, it defers because of missing DMA, then you probe DMA, which defers because of some other resource (clock, irq, ...), then probe the uart a second time and it fails, then you probe the DMA driver which succeeds because the resources are there. Another case would be the DMA driver being a loadable module that only gets loaded after all deferred probes from built-in drivers have completed. I don't think we can make any reasonable assumption about the number of times you need to defer the probing here. Also, the UART is a rather central device, at least the one that is used for the console, and we probably want to use it as early as possible even if that means never attaching the dma engine. A better solution might be to move the dma_request_slave_channel_reason() call out of the probe() function entirely and into the open() function: we use DMA if it is available by the time the device gets opened, otherwise we don't. Arnd
On 02/25/2015 05:22 AM, Arnd Bergmann wrote: > On Tuesday 24 February 2015 17:02:08 Jorge Ramirez-Ortiz wrote: >> On 02/24/2015 02:32 PM, Jorge Ramirez-Ortiz wrote: >>> On 02/24/2015 03:29 AM, Arnd Bergmann wrote: >>>> On Monday 23 February 2015 22:46:53 Jorge Ramirez-Ortiz wrote: >>>>> This patch addresses 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). >>>> How do you want to handle the case where there is a DMA channel in DT, but >>>> no driver for it built into the kernel? >>> In my view that should be fixed in the of-dma and the acpi-dma implementation >>> (just don't return EPROBE_DEFER a second time since the driver wasn't found >>> after the late_initcall associated to deferred probing). That would allow >>> drivers that use the defer probing mechanism to wait for optional controllers to >>> complete their probing sequence if those controllers don't become available. >>> >>> In any case, I can track the pending requests (up to twelve, one per UART) with >>> a list in the driver. >>> If a second request to probe fails with EPROBE_DEFER we can conclude that the >>> driver is not present. >>> >>> I have tested such an implementation and it works as expected. >>> would that be acceptable? >> Something along these lines >> >> +struct dma_deferred { >> + struct list_head node; >> + struct device *dev; >> +}; >> + >> +static LIST_HEAD(dma_deferred_reqs); >> + >> +static int pl011_handle_dma_probe_defer(struct device *dev) >> +{ >> + struct list_head *node, *tmp; >> + struct dma_deferred *entry; >> + >> + list_for_each_safe(node, tmp, &dma_deferred_reqs) { >> + entry = list_entry(node, struct dma_deferred, node); >> + if (entry->dev == dev) { >> + dev_warn(dev, "DMA driver not present \n"); >> + list_del(node); >> + kfree(entry); >> + return -ENODEV; >> + } >> + } > I don't see how this would help: Assume you probe the uart first, it defers > because of missing DMA, then you probe DMA, which defers because of some > other resource (clock, irq, ...), then probe the uart a second time and > it fails, then you probe the DMA driver which succeeds because the resources > are there. Another case would be the DMA driver being a loadable module > that only gets loaded after all deferred probes from built-in drivers > have completed. yes you are right. it is just as incomplete a solution as replacing the original device_initcall by late_deviceinitcall (my first impulse) > > I don't think we can make any reasonable assumption about the number of > times you need to defer the probing here. agreed. And even if we did, the current defer probing framework would not reliably support it either, since it might stop sending deferred probes with requests still queued in the deferred_probe_pending_list.So the wait could be eternal. I believe this happens when all the in kernel drivers have been tried and the list is now waiting for some module to successfuly load -which would trigger the event that iterates through the list. If not for this particular case, maybe there is some value in extending the current defer driver probing behavior; For example having two types of defer: 1. -EPROBE_DEFER (as per today, for ever) 2. -EPROBE_DEFERK (ie, defer until all the in-kernel devices have been handled and the list is quiescent now waiting for modules) On this second case we could flush the list probing the device with a flag in struct device indicating this is indeed the last call under EPROBE_DEFERK. I am not very sure myself if this would be of use to anybody (although I would have probably used it on this case) > > Also, the UART is a rather central device, at least the one that is used > for the console, and we probably want to use it as early as possible even > if that means never attaching the dma engine. yes, defer probing clearly does not fit here. > > A better solution might be to move the dma_request_slave_channel_reason() > call out of the probe() function entirely and into the open() function: > we use DMA if it is available by the time the device gets opened, otherwise > we don't. thanks! will do that. > > Arnd
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 8d94c19..d631a0a 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -29,6 +29,7 @@ * and hooked into this driver. */ +#define pr_fmt(fmt) "amba-pl011: "fmt #if defined(CONFIG_SERIAL_AMBA_PL011_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ) #define SUPPORT_SYSRQ @@ -261,7 +262,8 @@ 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 int pl011_dma_probe(struct device *dev, struct uart_amba_port *uap, + void (*queue_dma_probe)(struct device *, struct uart_amba_port *)) { /* DMA is the sole user of the platform data right now */ struct amba_pl011_data *plat = dev_get_platdata(uap->port.dev); @@ -275,15 +277,23 @@ 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"); + if (queue_dma_probe && plat && plat->dma_filter) { + (*queue_dma_probe)(dev, uap); + return 0; + } + + chan = dma_request_slave_channel_reason(dev, "tx"); + if (IS_ERR(chan)) { + + /* the dma driver has not been initialized yet */ + if (PTR_ERR(chan) == -EPROBE_DEFER) + return -EPROBE_DEFER; - if (!chan) { /* We need platform data */ if (!plat || !plat->dma_filter) { dev_info(uap->port.dev, "no DMA platform data\n"); - return; + return 0; } - /* Try to acquire a generic DMA engine slave TX channel */ dma_cap_zero(mask); dma_cap_set(DMA_SLAVE, mask); @@ -292,7 +302,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port * plat->dma_tx_param); if (!chan) { dev_err(uap->port.dev, "no TX DMA channel!\n"); - return; + return 0; } } @@ -310,7 +320,7 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port * if (!chan) { dev_err(uap->port.dev, "no RX DMA channel!\n"); - return; + return 0; } } @@ -383,14 +393,16 @@ static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port * dev_info(uap->port.dev, "DMA channel RX %s\n", dma_chan_name(uap->dmarx.chan)); } + + return 0; } -#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. + * On platforms with no OF or ACPI support, we stack up the UARTs and let the + * below initcall be done at late 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; @@ -400,22 +412,23 @@ struct dma_uap { static LIST_HEAD(pl011_dma_uarts); -static int __init pl011_dma_initcall(void) +static int __init pl011_dequeue_dma_probe_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); + pl011_dma_probe(dmau->dev, dmau->uap, NULL); list_del(node); kfree(dmau); } return 0; } -device_initcall(pl011_dma_initcall); +late_initcall_sync(pl011_dequeue_dma_probe_initcall); -static void pl011_dma_probe(struct device *dev, struct uart_amba_port *uap) +static void pl011_queue_dma_probe(struct device *dev, + struct uart_amba_port *uap) { struct dma_uap *dmau = kzalloc(sizeof(struct dma_uap), GFP_KERNEL); if (dmau) { @@ -424,12 +437,6 @@ static void pl011_dma_probe(struct device *dev, struct uart_amba_port *uap) 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) { @@ -1142,7 +1149,14 @@ 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 int pl011_dma_probe(struct device *dev, struct uart_amba_port *uap, + void (*queue_dma_probe)(struct device *, struct uart_amba_port *)) +{ + return 0; +} + +static void pl011_queue_dma_probe(struct device *dev, + struct uart_amba_port *uap) { } @@ -2218,7 +2232,12 @@ 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); + + ret = pl011_dma_probe(&dev->dev, uap, &pl011_queue_dma_probe); + if (ret == -EPROBE_DEFER) { + devm_kfree(&dev->dev, uap); + return ret; + } /* Ensure interrupts from this UART are masked and cleared */ writew(0, uap->port.membase + UART011_IMSC);
This patch addresses 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 proposed implementation for OF and ACPI uses deferred driver probing to wait for the DMA controller to be registered. It defaults to legacy behaviour when platform data is present. Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> --- drivers/tty/serial/amba-pl011.c | 67 ++++++++++++++++++++++++++--------------- 1 file changed, 43 insertions(+), 24 deletions(-)