[PATCH] drivers/tty: amba-pl011: defer driver probing if external dma is not ready.

[Jorge Ramirez-Ortiz]

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