| Message ID | 20241127-defer-dma-request-chan-v1-1-203db7baf470@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | dmaengine: dma_request_chan_by_mask() defer probing unconditionally | expand |
On 2024-11-27 8:23 am, Enric Balletbo i Serra wrote: > Having no DMA devices registered is not a guarantee that the device > doesn't exist, it could be that is not registered yet, so return > EPROBE_DEFER unconditionally so the caller can wait for the required > DMA device registered. > > Signed-off-by: Enric Balletbo i Serra <eballetb@redhat.com> > --- > This patch fixes the following error on TI AM69-SK > > [ 2.854501] cadence-qspi 47040000.spi: error -ENODEV: No Rx DMA available > > The DMA device is probed after cadence-qspi driver, so deferring it > solves the problem. Conversely, though, it does carry some risk that if there really is no DMA device/driver, other callers (e.g. spi-ti-qspi) may now get stuck deferring forever where the -ENODEV would have let them proceed with a fallback to non-DMA operation. driver_deferred_probe_check_state() is typically a good tool for these situations, but I guess it's a bit tricky in a context where we don't actually have the dependent device to hand :/ Thanks, Robin. > --- > drivers/dma/dmaengine.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c > index c1357d7f3dc6ca7899c4d68a039567e73b0f089d..57f07b477a5d9ad8f2656584b8c0d6dffb2ab469 100644 > --- a/drivers/dma/dmaengine.c > +++ b/drivers/dma/dmaengine.c > @@ -889,10 +889,10 @@ struct dma_chan *dma_request_chan_by_mask(const dma_cap_mask_t *mask) > chan = __dma_request_channel(mask, NULL, NULL, NULL); > if (!chan) { > mutex_lock(&dma_list_mutex); > - if (list_empty(&dma_device_list)) > - chan = ERR_PTR(-EPROBE_DEFER); > - else > - chan = ERR_PTR(-ENODEV); > + /* If the required DMA device is not registered yet, > + * return EPROBE_DEFER > + */ > + chan = ERR_PTR(-EPROBE_DEFER); > mutex_unlock(&dma_list_mutex); > } > > > --- > base-commit: 43fb83c17ba2d63dfb798f0be7453ed55ca3f9c2 > change-id: 20241127-defer-dma-request-chan-4f26c62c8691 > > Best regards,
Hi Enric, On 29/11/24 22:14, Robin Murphy wrote: > On 2024-11-27 8:23 am, Enric Balletbo i Serra wrote: >> Having no DMA devices registered is not a guarantee that the device >> doesn't exist, it could be that is not registered yet, so return >> EPROBE_DEFER unconditionally so the caller can wait for the required >> DMA device registered. >> >> Signed-off-by: Enric Balletbo i Serra <eballetb@redhat.com> >> --- >> This patch fixes the following error on TI AM69-SK >> >> [ 2.854501] cadence-qspi 47040000.spi: error -ENODEV: No Rx DMA >> available >> >> The DMA device is probed after cadence-qspi driver, so deferring it >> solves the problem. > > Conversely, though, it does carry some risk that if there really is no > DMA device/driver, other callers (e.g. spi-ti-qspi) may now get stuck > deferring forever where the -ENODEV would have let them proceed with a > fallback to non-DMA operation. driver_deferred_probe_check_state() is > typically a good tool for these situations, but I guess it's a bit > tricky in a context where we don't actually have the dependent device to > hand :/ +1. There is no explicit dependency that can be modeled (via DT or otherwise) for memcpy DMA channels. And the IP (cadence-qspi) is not specific to TI platforms. Its very much possible that a non TI platform may not have memcpy DMA channel at all. Wont this end up breaking such platforms wrt using SPI flash using CPU bound IO? > > Thanks, > Robin. > >> --- >> drivers/dma/dmaengine.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c >> index >> c1357d7f3dc6ca7899c4d68a039567e73b0f089d..57f07b477a5d9ad8f2656584b8c0d6dffb2ab469 100644 >> --- a/drivers/dma/dmaengine.c >> +++ b/drivers/dma/dmaengine.c >> @@ -889,10 +889,10 @@ struct dma_chan *dma_request_chan_by_mask(const >> dma_cap_mask_t *mask) >> chan = __dma_request_channel(mask, NULL, NULL, NULL); >> if (!chan) { >> mutex_lock(&dma_list_mutex); >> - if (list_empty(&dma_device_list)) >> - chan = ERR_PTR(-EPROBE_DEFER); >> - else >> - chan = ERR_PTR(-ENODEV); >> + /* If the required DMA device is not registered yet, >> + * return EPROBE_DEFER >> + */ >> + chan = ERR_PTR(-EPROBE_DEFER); >> mutex_unlock(&dma_list_mutex); >> } >> >> --- >> base-commit: 43fb83c17ba2d63dfb798f0be7453ed55ca3f9c2 >> change-id: 20241127-defer-dma-request-chan-4f26c62c8691 >> >> Best regards, >
Hi, On Tue, Dec 3, 2024 at 5:35 AM Vignesh Raghavendra <vigneshr@ti.com> wrote: > > Hi Enric, > > On 29/11/24 22:14, Robin Murphy wrote: > > On 2024-11-27 8:23 am, Enric Balletbo i Serra wrote: > >> Having no DMA devices registered is not a guarantee that the device > >> doesn't exist, it could be that is not registered yet, so return > >> EPROBE_DEFER unconditionally so the caller can wait for the required > >> DMA device registered. > >> > >> Signed-off-by: Enric Balletbo i Serra <eballetb@redhat.com> > >> --- > >> This patch fixes the following error on TI AM69-SK > >> > >> [ 2.854501] cadence-qspi 47040000.spi: error -ENODEV: No Rx DMA > >> available > >> > >> The DMA device is probed after cadence-qspi driver, so deferring it > >> solves the problem. > > > > Conversely, though, it does carry some risk that if there really is no > > DMA device/driver, other callers (e.g. spi-ti-qspi) may now get stuck > > deferring forever where the -ENODEV would have let them proceed with a > > fallback to non-DMA operation. driver_deferred_probe_check_state() is > > typically a good tool for these situations, but I guess it's a bit > > tricky in a context where we don't actually have the dependent device to > > hand :/ > > > +1. There is no explicit dependency that can be modeled (via DT or > otherwise) for memcpy DMA channels. And the IP (cadence-qspi) is not > specific to TI platforms. Its very much possible that a non TI platform > may not have memcpy DMA channel at all. Wont this end up breaking such > platforms wrt using SPI flash using CPU bound IO? > To be honest, I didn't take into account this, so yes, this patch will break such platforms. But isn't it already broken in such cases? If I understand correctly, if there are no dma devices at all, we're already deferring the probe [1]. if (list_empty(&dma_device_list)) chan = ERR_PTR(-EPROBE_DEFER); [1] https://elixir.bootlin.com/linux/v6.13-rc1/source/drivers/dma/dmaengine.c#L892 Thanks, Enric > > > > Thanks, > > Robin. > > > >> --- > >> drivers/dma/dmaengine.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c > >> index > >> c1357d7f3dc6ca7899c4d68a039567e73b0f089d..57f07b477a5d9ad8f2656584b8c0d6dffb2ab469 100644 > >> --- a/drivers/dma/dmaengine.c > >> +++ b/drivers/dma/dmaengine.c > >> @@ -889,10 +889,10 @@ struct dma_chan *dma_request_chan_by_mask(const > >> dma_cap_mask_t *mask) > >> chan = __dma_request_channel(mask, NULL, NULL, NULL); > >> if (!chan) { > >> mutex_lock(&dma_list_mutex); > >> - if (list_empty(&dma_device_list)) > >> - chan = ERR_PTR(-EPROBE_DEFER); > >> - else > >> - chan = ERR_PTR(-ENODEV); > >> + /* If the required DMA device is not registered yet, > >> + * return EPROBE_DEFER > >> + */ > >> + chan = ERR_PTR(-EPROBE_DEFER); > >> mutex_unlock(&dma_list_mutex); > >> } > >> > >> --- > >> base-commit: 43fb83c17ba2d63dfb798f0be7453ed55ca3f9c2 > >> change-id: 20241127-defer-dma-request-chan-4f26c62c8691 > >> > >> Best regards, > > > > -- > Regards > Vignesh > https://ti.com/opensource >
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c index c1357d7f3dc6ca7899c4d68a039567e73b0f089d..57f07b477a5d9ad8f2656584b8c0d6dffb2ab469 100644 --- a/drivers/dma/dmaengine.c +++ b/drivers/dma/dmaengine.c @@ -889,10 +889,10 @@ struct dma_chan *dma_request_chan_by_mask(const dma_cap_mask_t *mask) chan = __dma_request_channel(mask, NULL, NULL, NULL); if (!chan) { mutex_lock(&dma_list_mutex); - if (list_empty(&dma_device_list)) - chan = ERR_PTR(-EPROBE_DEFER); - else - chan = ERR_PTR(-ENODEV); + /* If the required DMA device is not registered yet, + * return EPROBE_DEFER + */ + chan = ERR_PTR(-EPROBE_DEFER); mutex_unlock(&dma_list_mutex); }
Having no DMA devices registered is not a guarantee that the device doesn't exist, it could be that is not registered yet, so return EPROBE_DEFER unconditionally so the caller can wait for the required DMA device registered. Signed-off-by: Enric Balletbo i Serra <eballetb@redhat.com> --- This patch fixes the following error on TI AM69-SK [ 2.854501] cadence-qspi 47040000.spi: error -ENODEV: No Rx DMA available The DMA device is probed after cadence-qspi driver, so deferring it solves the problem. --- drivers/dma/dmaengine.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) --- base-commit: 43fb83c17ba2d63dfb798f0be7453ed55ca3f9c2 change-id: 20241127-defer-dma-request-chan-4f26c62c8691 Best regards,