[PROTO] [VIRTIO] [PATCH 1/2] Add virtio gpu device specification.
From: Gerd Hoffmann
Resuming the effort to get the gpu device specs merged.
Support for 2d mode (3d/virgl mode is not covered by this patch) has
been added to the linux kernel version 4.2 and to qemu version 2.4.
git branch:
https://www.kraxel.org/cgit/virtio-spec/commit/?h=virtio-gpu
Rendered versions are available here:
https://www.kraxel.org/virtio/virtio-v1.0-cs03-virtio-gpu.pdf
https://www.kraxel.org/virtio/virtio-v1.0-cs03-virtio-gpu.html#x1-287
Signed-off-by: Gerd Hoffmann
---
content.tex| 2 +
virtio-gpu.tex | 481 +
2 files changed, 483 insertions(+)
create mode 100644 virtio-gpu.tex
diff --git a/content.tex b/content.tex
index be1823431dd1..d41c2f8d7627 100644
--- a/content.tex
+++ b/content.tex
@@ -5325,6 +5325,8 @@ descriptor for the \field{sense_len}, \field{residual},
\field{status_qualifier}, \field{status}, \field{response} and
\field{sense} fields.
+\input{virtio-gpu.tex}
+
\chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
Currently these device-independent feature bits defined:
diff --git a/virtio-gpu.tex b/virtio-gpu.tex
new file mode 100644
index ..34cf493bbcd1
--- /dev/null
+++ b/virtio-gpu.tex
@@ -0,0 +1,481 @@
+\section{GPU Device}\label{sec:Device Types / GPU Device}
+
+virtio-gpu is a virtio based graphics adapter. It can operate in 2D
+mode and in 3D (virgl) mode. 3D mode will offload rendering ops to
+the host gpu and therefore requires a gpu with 3D support on the host
+machine.
+
+3D mode is not covered (yet) in this specification, even though it is
+mentioned here and there due to some details of the virtual hardware
+being designed with 3D mode in mind.
+
+In 2D mode the virtio-gpu device provides support for ARGB Hardware
+cursors and multiple scanouts (aka heads).
+
+\subsection{Device ID}\label{sec:Device Types / GPU Device / Device ID}
+
+16
+
+\subsection{Virtqueues}\label{sec:Device Types / GPU Device / Virtqueues}
+
+\begin{description}
+\item[0] controlq - queue for sending control commands
+\item[1] cursorq - queue for sending cursor updates
+\end{description}
+
+Both queues have the same format. Each request and each response have
+a fixed header, followed by command specific data fields. The
+separate cursor queue is the "fast track" for cursor commands
+(VIRTIO_GPU_CMD_UPDATE_CURSOR and VIRTIO_GPU_CMD_MOVE_CURSOR), so they
+go though without being delayed by time-consuming commands in the
+control queue.
+
+\subsection{Feature bits}\label{sec:Device Types / GPU Device / Feature bits}
+
+\begin{description}
+\item[VIRTIO_GPU_F_VIRGL (0)] virgl 3D mode is supported.
+\end{description}
+
+\subsection{Device configuration layout}\label{sec:Device Types / GPU Device /
Device configuration layout}
+
+\begin{lstlisting}
+#define VIRTIO_GPU_EVENT_DISPLAY (1 << 0)
+
+struct virtio_gpu_config {
+le32 events_read;
+le32 events_clear;
+le32 num_scanouts;
+le32 reserved;
+}
+\end{lstlisting}
+
+\subsubsection{Device configuration fields}
+
+\begin{description}
+\item[\field{events_read}] signals pending events to the driver. The
+ driver MUST NOT write to this field.
+\item[\field{events_clear}] clears pending events in the device.
+ Writing a '1' into a bit will clear the corresponding bit in
+ \field{events_read}, mimicking write-to-clear behavior.
+\item[\field{num_scanouts}] specifies the maximum number of scanouts
+ supported by the device. Minimum value is 1, maximum value is 16.
+\end{description}
+
+\subsubsection{Events}
+
+\begin{description}
+\item[VIRTIO_GPU_EVENT_DISPLAY] Display configuration has changed.
+ The driver SHOULD use the VIRTIO_GPU_CMD_GET_DISPLAY_INFO command to
+ fetch the information from the device.
+\end{description}
+
+\devicenormative{\subsection}{Device Initialization}{Device Types / GPU Device
/ Device Initialization}
+
+The driver SHOULD query the display information from the device using
+the VIRTIO_GPU_CMD_GET_DISPLAY_INFO command and use that information
+for the initial scanout setup. In case no information is available or
+all displays are disabled the driver MAY choose to use a fallback,
+such as 1024x768 at display 0.
+
+\subsection{Device Operation}\label{sec:Device Types / GPU Device / Device
Operation}
+
+The virtio-gpu is based around the concept of resources private to the
+host, the guest must DMA transfer into these resources. This is a
+design requirement in order to interface with future 3D rendering. In
+the unaccelerated 2D mode there is no support for DMA transfers from
+resources, just to them.
+
+Resources are initially simple 2D resources, consisting of a width,
+height and format along with an identifier. The guest must then attach
+backing store to the resources in order for DMA transfers to
+work. This is like a GART in a real GPU.
+
+\subsubsection{Device Operation: Create a framebuffer and configure scanout}
+
+\begin{itemize*}
+\item Create a host
[PROTO] [VIRTIO] [PATCH 2/2] virtio-gpu: Support host-allocated backing storage
The virtio-gpu protocol is based on opaque host resources backed by
guest-allocated memory. This requires transferring data from guest
framebuffers to host display device buffers.
When the host supports VIRGL, the GPU is used for both 2D and 3D
rendering, in which case the memory transfer can take the form of a DMA
operation without incurring significant costs. However, when VIRGL
support isn't available, 2D transfers require a CPU memory copy for
every page flip. Display is slowed down to an unusable state.
To solve this problem, extend the virtio-gpu protocol with the ability
for the host to allocate resource backing storage and map it to the
guest. The allocated memory can then be used directly by the host-side
display device, removing the need for memory copy operations.
Signed-off-by: Laurent Pinchart
---
virtio-gpu.tex | 48
1 file changed, 48 insertions(+)
diff --git a/virtio-gpu.tex b/virtio-gpu.tex
index 34cf493bbcd1..22546cc1748f 100644
--- a/virtio-gpu.tex
+++ b/virtio-gpu.tex
@@ -34,6 +34,7 @@ control queue.
\begin{description}
\item[VIRTIO_GPU_F_VIRGL (0)] virgl 3D mode is supported.
+\item[VIRTIO_GPU_F_HOST_ALLOC (1)] buffer allocation by the host is supported.
\end{description}
\subsection{Device configuration layout}\label{sec:Device Types / GPU Device /
Device configuration layout}
@@ -90,6 +91,11 @@ height and format along with an identifier. The guest must
then attach
backing store to the resources in order for DMA transfers to
work. This is like a GART in a real GPU.
+Additionally, when the VIRTIO_GPU_F_HOST_ALLOC feature is present, the guest
+can request the host to allocate backing store for resources on its behalf, in
+which case the allocated resources can be accessed by both the host and the
+guest without any need for DMA transfers.
+
\subsubsection{Device Operation: Create a framebuffer and configure scanout}
\begin{itemize*}
@@ -120,6 +126,18 @@ using VIRTIO_GPU_CMD_SET_SCANOUT and
VIRTIO_GPU_CMD_RESOURCE_FLUSH,
and update the invisible framebuffer using
VIRTIO_GPU_CMD_TRANSFER_SEND_2D.
+\subsubsection{Device Operation: Create a framebuffer from host-allocated
memory}
+
+\begin{itemize*}
+\item Create a host resource using VIRTIO_GPU_CMD_RESOURCE_CREATE_2D. This
+ step is identical to the guest-allocated backing store case.
+\item Request the host to allocate the backing storage for the resource just
+ created, using VIRTIO_GPU_CMD_RESOURCE_ALLOC_BACKING. The host will map the
+ memory to the guest and will return the memory address.
+\item Use VIRTIO_GPU_CMD_SET_SCANOUT, VIRTIO_GPU_CMD_TRANSFER_SEND_2D and
+ VIRTIO_GPU_CMD_RESOURCE_FLUSH as for guest-allocated memory.
+\end{itemize*}
+
\subsubsection{Device Operation: Multihead setup}
In case two or more displays are present there are different ways to
@@ -171,6 +189,7 @@ enum virtio_gpu_ctrl_type {
VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D,
VIRTIO_GPU_CMD_RESOURCE_ATTACH_BACKING,
VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING,
+VIRTIO_GPU_CMD_RESOURCE_ALLOC_BACKING,
/* cursor commands */
VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
@@ -179,6 +198,7 @@ enum virtio_gpu_ctrl_type {
/* success responses */
VIRTIO_GPU_RESP_OK_NODATA = 0x1100,
VIRTIO_GPU_RESP_OK_DISPLAY_INFO,
+VIRTIO_GPU_RESP_OK_ALLOC_BACKING,
/* error responses */
VIRTIO_GPU_RESP_ERR_UNSPEC = 0x1200,
@@ -417,6 +437,34 @@ struct virtio_gpu_resource_detach_backing {
This detaches any backing pages from a resource, to be used in case of
guest swapping or object destruction.
+\item[VIRTIO_GPU_CMD_RESOURCE_ALLOC_BACKING] Request the host to allocate
+ backing pages for a resource. Request data is \field{struct
+virtio_gpu_resource_alloc_backing}. Response type is
+VIRTIO_GPU_RESP_OK_ALLOC_BACKING, response data is \field{struct
+virtio_gpu_resp_resource_alloc_backing}.
+
+\begin{lstlisting}
+struct virtio_gpu_resource_alloc_backing {
+struct virtio_gpu_ctrl_hdr hdr;
+le32 resource_id;
+le32 padding;
+};
+
+struct virtio_gpu_resp_resource_alloc_backing {
+struct virtio_gpu_ctrl_hdr hdr;
+le64 addr;
+};
+\end{lstlisting}
+
+This allocates backing store memory for a resource on the host, and maps the
+memory physically contiguous to the guest. The host returns the guest memory
+address in the \field{addr} field. If memory can't be allocated, the response
+is VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY with no data.
+
+When the backing store is detached from the resource with
+VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING, the memory is freed and unmapped from
+the guest.
+
\end{description}
\subsubsection{Device Operation: cursorq}\label{sec:Device Types / GPU Device
/ Device Operation / Device Operation: cursorq}
--
Regards,
Laurent Pinchart
Re: [PATCH] serial: sh-sci: Stop RX FIFO timer during port shutdown
On Fri, Jun 29, 2018 at 04:28:03PM +0200, Geert Uytterhoeven wrote:
> The RX FIFO timer may be armed when the port is shut down, hence the
> timer function may still be called afterwards.
>
> Fix this race condition by deleting the timer during port shutdown.
>
> Fixes: 039403765e5da3c6 ("serial: sh-sci: SCIFA/B RX FIFO software timeout")
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Simon Horman
> ---
> Tested with scifa0 on r8a7740/armadillo, after
>
> echo 2000 > /sys/devices/platform/e6c4.serial/rx_fifo_timeout
> ---
> drivers/tty/serial/sh-sci.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 5c870ab80b98e25b..04f5b754e69e28dd 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2100,6 +2100,8 @@ static void sci_shutdown(struct uart_port *port)
> }
> #endif
>
> + if (s->rx_trigger > 1 && s->rx_fifo_timeout > 0)
> + del_timer_sync(>rx_fifo_timer);
> sci_free_irq(s);
> sci_free_dma(port);
> }
> --
> 2.17.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
[PATCH] arm64: dts: renesas: salvator-common: Prefer HSCIF1 over SCIF1
HSCIF is superior to SCIF (larger FIFOs, more accurate and wider
supported range of bitrates).
Signed-off-by: Geert Uytterhoeven
---
arch/arm64/boot/dts/renesas/salvator-common.dtsi | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
index 6439d05fd7ecd99f..9687166c44732a9f 100644
--- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
+++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
@@ -33,7 +33,7 @@
/ {
aliases {
serial0 =
- serial1 =
+ serial1 =
ethernet0 =
};
@@ -344,7 +344,7 @@
uart-has-rtscts;
/* Please only enable hscif1 or scif1 */
- /* status = "okay"; */
+ status = "okay";
};
{
@@ -723,7 +723,7 @@
uart-has-rtscts;
/* Please only enable hscif1 or scif1 */
- status = "okay";
+ /* status = "okay"; */
};
{
--
2.17.1
Re: [PATCH v2 2/2] mmc: renesas_sdhi_internal_dmac: Cannot clear the RX_IN_USE in abort
On Fri, Jun 29, 2018 at 07:01:45PM +0900, Yoshihiro Shimoda wrote:
> This patch is fixes an issue that the SDHI_INTERNAL_DMAC_RX_IN_USE
> flag cannot be cleared because tmio_mmc_core sets the host->data
> to NULL before the tmio_mmc_core calls tmio_mmc_abort_dma().
>
> So, this patch clears the SDHI_INTERNAL_DMAC_RX_IN_USE in
> the renesas_sdhi_internal_dmac_abort_dma() anyway. This doesn't
> cause any side effects.
>
> Fixes: 0cbc94daa554 ("mmc: renesas_sdhi_internal_dmac: limit DMA RX for old
> SoCs")
> Cc: # v4.17+
> Signed-off-by: Yoshihiro Shimoda
> Reviewed-by: Geert Uytterhoeven
Reviewed-by: Simon Horman
Re: [PATCH v2 1/2] mmc: renesas_sdhi_internal_dmac: Fix missing unmap in error patch
On Fri, Jun 29, 2018 at 07:01:44PM +0900, Yoshihiro Shimoda wrote:
> This patch fixes an issue that lacks the dma_unmap_sg() calling in
> the error patch of renesas_sdhi_internal_dmac_start_dma().
>
> Fixes: 0cbc94daa554 ("mmc: renesas_sdhi_internal_dmac: limit DMA RX for old
> SoCs")
> Cc: # v4.17+
> Signed-off-by: Yoshihiro Shimoda
> Reviewed-by: Geert Uytterhoeven
Reviewed-by: Simon Horman
Re: [PATCH 1/3] serial: sh-sci: Postpone DMA release when falling back to PIO
On Fri, Jun 29, 2018 at 4:26 PM Geert Uytterhoeven
wrote:
> When the sh-sci driver detects an issue with DMA during operation, it
> falls backs to PIO, and releases all DMA resources.
>
> As releasing DMA resources immediately has no advantages, but
> complicates the code, and is susceptible to races, it is better to
> postpone this to port shutdown.
>
> This allows to remove the locking from sci_rx_dma_release() and
> sci_tx_dma_release(), but requires keeping a copy of the DMA channel
> pointers for release during port shutdown.
>
> Signed-off-by: Geert Uytterhoeven
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1212,25 +1214,17 @@ static int sci_dma_rx_find_active(struct sci_port *s)
> return -1;
> }
>
> -static void sci_rx_dma_release(struct sci_port *s, bool enable_pio)
> +static void sci_rx_dma_release(struct sci_port *s)
> {
> - struct dma_chan *chan = s->chan_rx;
> + struct dma_chan *chan = s->chan_rx_saved;
> struct uart_port *port = >port;
Brown paper bag #2: unused variable port. Let's blame it on the warm Friday
afternoon.
So yes, there will be a v2, eventually...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH] arm64: dts: renesas: r8a77965: Add second port to rcar_sound placeholder
On Thu, Jun 28, 2018 at 03:33:59PM +0200, Geert Uytterhoeven wrote: > On Thu, Jun 28, 2018 at 2:43 PM Simon Horman > wrote: > > This node is just a placeholder but fills that function better if it does > > not trigger build warnings. This update satisfies the requirement that > > nodes with #address-cells/#size-cells properties have more than one child > > node. > > > > This is flagged by dtc as follows: > > # make dtbs W=1 > > arch/arm64/boot/dts/renesas/r8a77965-salvator-x.dtb: Warning > > (graph_child_address): /soc/sound@ec50/ports: graph node has single > > child node 'port@0', #address-cells/#size-cells are not necessary > > arch/arm64/boot/dts/renesas/r8a77965-salvator-xs.dtb: Warning > > (graph_child_address): /soc/sound@ec50/ports: graph node has single > > child node 'port@0', #address-cells/#size-cells are not necessary > > > > Signed-off-by: Simon Horman > > Reviewed-by: Geert Uytterhoeven Thanks, applied.
Re: [PATCH 2/4] serial: sh-sci: Stop using deprecated dmaengine_terminate_all()
On Fri, Jun 29, 2018 at 4:25 PM Geert Uytterhoeven
wrote:
> As of commit b36f09c3c441a6e5 ("dmaengine: Add transfer termination
> synchronization support"), dmaengine_terminate_all() is deprecated.
>
> Replace calls to dmaengine_terminate_all() in DMA release code by calls
> to dmaengine_terminate_sync(), as the latter waits until all running
> completion callbacks have finished.
>
> Replace calls to dmaengine_terminate_all() in DMA failure paths by calls
> to dmaengine_terminate_async(), as these are usually done in atomic
> context.
>
> Signed-off-by: Geert Uytterhoeven
> ---
> drivers/tty/serial/sh-sci.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 898c1034cad23a88..061660ccf9442d02 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1613,7 +1613,7 @@ dev_dbg_dma(port->dev, "%s\n", __func__);
> s->chan_rx_saved = s->chan_rx = NULL;
> s->cookie_rx[0] = s->cookie_rx[1] = -EINVAL;
> WARN(!chan, "RX DMA channel already released\n");
Woops, I had some old versions of the patches lying around, and accidentally
sent them with the real series.
Sorry for that, the "[x/4]" patches should be ignored.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH] serial: sh-sci: Stop RX FIFO timer during port shutdown
The RX FIFO timer may be armed when the port is shut down, hence the
timer function may still be called afterwards.
Fix this race condition by deleting the timer during port shutdown.
Fixes: 039403765e5da3c6 ("serial: sh-sci: SCIFA/B RX FIFO software timeout")
Signed-off-by: Geert Uytterhoeven
---
Tested with scifa0 on r8a7740/armadillo, after
echo 2000 > /sys/devices/platform/e6c4.serial/rx_fifo_timeout
---
drivers/tty/serial/sh-sci.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 5c870ab80b98e25b..04f5b754e69e28dd 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2100,6 +2100,8 @@ static void sci_shutdown(struct uart_port *port)
}
#endif
+ if (s->rx_trigger > 1 && s->rx_fifo_timeout > 0)
+ del_timer_sync(>rx_fifo_timer);
sci_free_irq(s);
sci_free_dma(port);
}
--
2.17.1
[PATCH 3/3] serial: sh-sci: Stop using deprecated dmaengine_terminate_all()
As of commit b36f09c3c441a6e5 ("dmaengine: Add transfer termination
synchronization support"), dmaengine_terminate_all() is deprecated.
Replace calls to dmaengine_terminate_all() in DMA release code by calls
to dmaengine_terminate_sync(), as the latter waits until all running
completion callbacks have finished.
Replace calls to dmaengine_terminate_all() in DMA failure paths by calls
to dmaengine_terminate_async(), as these are usually done in atomic
context.
Signed-off-by: Geert Uytterhoeven
---
drivers/tty/serial/sh-sci.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 1e0b6fe6865dcff6..5c870ab80b98e25b 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1221,7 +1221,7 @@ static void sci_rx_dma_release(struct sci_port *s)
s->chan_rx_saved = s->chan_rx = NULL;
s->cookie_rx[0] = s->cookie_rx[1] = -EINVAL;
- dmaengine_terminate_all(chan);
+ dmaengine_terminate_sync(chan);
dma_free_coherent(chan->device->dev, s->buf_len_rx * 2, s->rx_buf[0],
sg_dma_address(>sg_rx[0]));
dma_release_channel(chan);
@@ -1298,7 +1298,7 @@ static void sci_tx_dma_release(struct sci_port *s)
cancel_work_sync(>work_tx);
s->chan_tx_saved = s->chan_tx = NULL;
s->cookie_tx = -EINVAL;
- dmaengine_terminate_all(chan);
+ dmaengine_terminate_sync(chan);
dma_unmap_single(chan->device->dev, s->tx_dma_addr, UART_XMIT_SIZE,
DMA_TO_DEVICE);
dma_release_channel(chan);
@@ -1336,7 +1336,7 @@ static void sci_submit_rx(struct sci_port *s)
fail:
if (i)
- dmaengine_terminate_all(chan);
+ dmaengine_terminate_async(chan);
for (i = 0; i < 2; i++)
s->cookie_rx[i] = -EINVAL;
s->active_rx = -EINVAL;
@@ -1454,7 +1454,7 @@ static enum hrtimer_restart rx_timer_fn(struct hrtimer *t)
}
/* Handle incomplete DMA receive */
- dmaengine_terminate_all(s->chan_rx);
+ dmaengine_terminate_async(s->chan_rx);
read = sg_dma_len(>sg_rx[active]) - state.residue;
if (read) {
--
2.17.1
[PATCH 2/4] serial: sh-sci: Stop using deprecated dmaengine_terminate_all()
As of commit b36f09c3c441a6e5 ("dmaengine: Add transfer termination
synchronization support"), dmaengine_terminate_all() is deprecated.
Replace calls to dmaengine_terminate_all() in DMA release code by calls
to dmaengine_terminate_sync(), as the latter waits until all running
completion callbacks have finished.
Replace calls to dmaengine_terminate_all() in DMA failure paths by calls
to dmaengine_terminate_async(), as these are usually done in atomic
context.
Signed-off-by: Geert Uytterhoeven
---
drivers/tty/serial/sh-sci.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 898c1034cad23a88..061660ccf9442d02 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1613,7 +1613,7 @@ dev_dbg_dma(port->dev, "%s\n", __func__);
s->chan_rx_saved = s->chan_rx = NULL;
s->cookie_rx[0] = s->cookie_rx[1] = -EINVAL;
WARN(!chan, "RX DMA channel already released\n");
- dmaengine_terminate_all(chan);
+ dmaengine_terminate_sync(chan);
dma_free_coherent(chan->device->dev, s->buf_len_rx * 2, s->rx_buf[0],
sg_dma_address(>sg_rx[0]));
dma_release_channel(chan);
@@ -1708,7 +1708,7 @@ dev_dbg_dma(port->dev, "%s\n", __func__);
s->chan_tx_saved = s->chan_tx = NULL;
s->cookie_tx = -EINVAL;
WARN(!chan, "TX DMA channel already released\n");
- dmaengine_terminate_all(chan);
+ dmaengine_terminate_sync(chan);
dma_unmap_single(chan->device->dev, s->tx_dma_addr, UART_XMIT_SIZE,
DMA_TO_DEVICE);
dma_release_channel(chan);
@@ -1747,7 +1747,7 @@ dev_dbg_dma(s->port.dev, " %s\n", __func__);
fail:
if (i)
- dmaengine_terminate_all(chan);
+ dmaengine_terminate_async(chan);
// FIXME Not needed?
for (i = 0; i < 2; i++)
s->cookie_rx[i] = -EINVAL;
@@ -1877,9 +1877,9 @@ dev_dbg_dma(port->dev, " %s active %d\n", __func__,
active);
/* Handle incomplete DMA receive */
dev_dbg_dma(port->dev, " %s:%u dmaengine_terminate_all()\n", __func__,
__LINE__);
- dmaengine_terminate_all(s->chan_rx);
+ dmaengine_terminate_async(s->chan_rx);
// FIXME More data may have been transfered in between
- // FIXME dmaengine_tx_status() and dmaengine_terminate_all()
+ // FIXME dmaengine_tx_status() and dmaengine_terminate_async()
// FIXME Call dmaengine_pause() first
dev_dbg_dma(port->dev, " %s:%u terminated\n", __func__, __LINE__);
read = sg_dma_len(>sg_rx[active]) - state.residue;
--
2.17.1
[PATCH 3/4] serial: sh-sci: Stop TX DMA workqueue during port shutdown
The transmit DMA workqueue is never stopped, hence the work function may be called after the port has been shut down. Fix this race condition by cancelling queued work, if any, before DMA release. Don't initialize the work if DMA initialization failed, as it won't be used anyway. Signed-off-by: Geert Uytterhoeven --- drivers/tty/serial/sh-sci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 061660ccf9442d02..58016386558c6789 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -1704,6 +1704,7 @@ static void sci_tx_dma_release(struct sci_port *s) struct dma_chan *chan = s->chan_tx_saved; struct uart_port *port = >port; + cancel_work_sync(>work_tx); dev_dbg_dma(port->dev, "%s\n", __func__); s->chan_tx_saved = s->chan_tx = NULL; s->cookie_tx = -EINVAL; @@ -1978,10 +1979,9 @@ static void sci_request_dma(struct uart_port *port) UART_XMIT_SIZE, port->state->xmit.buf, >tx_dma_addr); + INIT_WORK(>work_tx, work_fn_tx); s->chan_tx_saved = s->chan_tx = chan; } - - INIT_WORK(>work_tx, work_fn_tx); } chan = sci_request_dma_chan(port, DMA_DEV_TO_MEM); -- 2.17.1
[PATCH 1/3] serial: sh-sci: Postpone DMA release when falling back to PIO
When the sh-sci driver detects an issue with DMA during operation, it
falls backs to PIO, and releases all DMA resources.
As releasing DMA resources immediately has no advantages, but
complicates the code, and is susceptible to races, it is better to
postpone this to port shutdown.
This allows to remove the locking from sci_rx_dma_release() and
sci_tx_dma_release(), but requires keeping a copy of the DMA channel
pointers for release during port shutdown.
Signed-off-by: Geert Uytterhoeven
---
drivers/tty/serial/sh-sci.c | 81 +++--
1 file changed, 41 insertions(+), 40 deletions(-)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index c181eb37f98509e6..0ed91692f53ad859 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -135,6 +135,8 @@ struct sci_port {
struct dma_chan *chan_rx;
#ifdef CONFIG_SERIAL_SH_SCI_DMA
+ struct dma_chan *chan_tx_saved;
+ struct dma_chan *chan_rx_saved;
dma_cookie_tcookie_tx;
dma_cookie_tcookie_rx[2];
dma_cookie_tactive_rx;
@@ -1212,25 +1214,17 @@ static int sci_dma_rx_find_active(struct sci_port *s)
return -1;
}
-static void sci_rx_dma_release(struct sci_port *s, bool enable_pio)
+static void sci_rx_dma_release(struct sci_port *s)
{
- struct dma_chan *chan = s->chan_rx;
+ struct dma_chan *chan = s->chan_rx_saved;
struct uart_port *port = >port;
- unsigned long flags;
- spin_lock_irqsave(>lock, flags);
- s->chan_rx = NULL;
+ s->chan_rx_saved = s->chan_rx = NULL;
s->cookie_rx[0] = s->cookie_rx[1] = -EINVAL;
- spin_unlock_irqrestore(>lock, flags);
dmaengine_terminate_all(chan);
dma_free_coherent(chan->device->dev, s->buf_len_rx * 2, s->rx_buf[0],
sg_dma_address(>sg_rx[0]));
dma_release_channel(chan);
- if (enable_pio) {
- spin_lock_irqsave(>lock, flags);
- sci_start_rx(port);
- spin_unlock_irqrestore(>lock, flags);
- }
}
static void start_hrtimer_us(struct hrtimer *hrt, unsigned long usec)
@@ -1289,33 +1283,31 @@ static void sci_dma_rx_complete(void *arg)
fail:
spin_unlock_irqrestore(>lock, flags);
dev_warn(port->dev, "Failed submitting Rx DMA descriptor\n");
- sci_rx_dma_release(s, true);
+ /* Switch to PIO */
+ spin_lock_irqsave(>lock, flags);
+ s->chan_rx = NULL;
+ sci_start_rx(port);
+ spin_unlock_irqrestore(>lock, flags);
}
-static void sci_tx_dma_release(struct sci_port *s, bool enable_pio)
+static void sci_tx_dma_release(struct sci_port *s)
{
- struct dma_chan *chan = s->chan_tx;
+ struct dma_chan *chan = s->chan_tx_saved;
struct uart_port *port = >port;
- unsigned long flags;
- spin_lock_irqsave(>lock, flags);
- s->chan_tx = NULL;
+ s->chan_tx_saved = s->chan_tx = NULL;
s->cookie_tx = -EINVAL;
- spin_unlock_irqrestore(>lock, flags);
dmaengine_terminate_all(chan);
dma_unmap_single(chan->device->dev, s->tx_dma_addr, UART_XMIT_SIZE,
DMA_TO_DEVICE);
dma_release_channel(chan);
- if (enable_pio) {
- spin_lock_irqsave(>lock, flags);
- sci_start_tx(port);
- spin_unlock_irqrestore(>lock, flags);
- }
}
static void sci_submit_rx(struct sci_port *s)
{
struct dma_chan *chan = s->chan_rx;
+ struct uart_port *port = >port;
+ unsigned long flags;
int i;
for (i = 0; i < 2; i++) {
@@ -1347,7 +1339,11 @@ static void sci_submit_rx(struct sci_port *s)
for (i = 0; i < 2; i++)
s->cookie_rx[i] = -EINVAL;
s->active_rx = -EINVAL;
- sci_rx_dma_release(s, true);
+ /* Switch to PIO */
+ spin_lock_irqsave(>lock, flags);
+ s->chan_rx = NULL;
+ sci_start_rx(port);
+ spin_unlock_irqrestore(>lock, flags);
}
static void work_fn_tx(struct work_struct *work)
@@ -1357,6 +1353,7 @@ static void work_fn_tx(struct work_struct *work)
struct dma_chan *chan = s->chan_tx;
struct uart_port *port = >port;
struct circ_buf *xmit = >state->xmit;
+ unsigned long flags;
dma_addr_t buf;
/*
@@ -1378,9 +1375,7 @@ static void work_fn_tx(struct work_struct *work)
DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
if (!desc) {
dev_warn(port->dev, "Failed preparing Tx DMA descriptor\n");
- /* switch to PIO */
- sci_tx_dma_release(s, true);
- return;
+ goto switch_to_pio;
}
dma_sync_single_for_device(chan->device->dev, buf, s->tx_dma_len,
@@ -1393,15 +1388,21 @@ static void work_fn_tx(struct work_struct *work)
[PATCH 0/3] serial: sh-sci: Fix port shutdown DMA race conditions
Hi all,
This patch series fixes race conditions between DMA use and port
shutdown on Renesas "SCIF" serial ports, causing e.g.
sh-sci e655.serial: Failed preparing Tx DMA descriptor
Unable to handle kernel read from unreadable memory at virtual address
...
Call trace:
sci_tx_dma_release+0x50/0xfc
work_fn_tx+0x128/0x22c
process_one_work+0x394/0x62c
worker_thread+0x21c/0x324
kthread+0x118/0x128
ret_from_fork+0x10/0x18
I have no guaranteed way to reproducis this issue. I see it sometimes
when doing a partial login on a port running getty, and letting getty
time out.
The first two patches simplify DMA release handling, and make sure the
work function is not called after port shutdown.
The last patch switches the driver to the new
dmaengine_terminate_(a)sync() functions, now DMA release is done from a
single point.
Please review. Thanks!
Geert Uytterhoeven (3):
serial: sh-sci: Postpone DMA release when falling back to PIO
serial: sh-sci: Stop TX DMA workqueue during port shutdown
serial: sh-sci: Stop using deprecated dmaengine_terminate_all()
drivers/tty/serial/sh-sci.c | 93 +++--
1 file changed, 47 insertions(+), 46 deletions(-)
--
2.17.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH 1/4] serial: sh-sci: Postpone DMA release when falling back to PIO
When the sh-sci driver detects an issue with DMA during operation, it
falls backs to PIO, and releases all DMA resources.
As releasing DMA resources immediately has no advantages, but
complicates the code, and is susceptible to races, it is better to
postpone this to port shutdown.
This allows to remove the locking from sci_rx_dma_release() and
sci_tx_dma_release(), but requires keeping a copy of the DMA channel
pointers for release during port shutdown.
Signed-off-by: Geert Uytterhoeven
---
drivers/tty/serial/sh-sci.c | 81 +++--
1 file changed, 41 insertions(+), 40 deletions(-)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index cf8c394c6f185792..898c1034cad23a88 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -414,6 +414,8 @@ struct sci_port {
struct dma_chan *chan_rx;
#ifdef CONFIG_SERIAL_SH_SCI_DMA
+ struct dma_chan *chan_tx_saved;
+ struct dma_chan *chan_rx_saved;
dma_cookie_tcookie_tx;
dma_cookie_tcookie_rx[2];
dma_cookie_tactive_rx;
@@ -1602,27 +1604,19 @@ static int sci_dma_rx_find_active(struct sci_port *s)
return -1;
}
-static void sci_rx_dma_release(struct sci_port *s, bool enable_pio)
+static void sci_rx_dma_release(struct sci_port *s)
{
- struct dma_chan *chan = s->chan_rx;
+ struct dma_chan *chan = s->chan_rx_saved;
struct uart_port *port = >port;
- unsigned long flags;
dev_dbg_dma(port->dev, "%s\n", __func__);
- spin_lock_irqsave(>lock, flags);
- s->chan_rx = NULL;
+ s->chan_rx_saved = s->chan_rx = NULL;
s->cookie_rx[0] = s->cookie_rx[1] = -EINVAL;
- spin_unlock_irqrestore(>lock, flags);
WARN(!chan, "RX DMA channel already released\n");
dmaengine_terminate_all(chan);
dma_free_coherent(chan->device->dev, s->buf_len_rx * 2, s->rx_buf[0],
sg_dma_address(>sg_rx[0]));
dma_release_channel(chan);
- if (enable_pio) {
- spin_lock_irqsave(>lock, flags);
- sci_start_rx(port);
- spin_unlock_irqrestore(>lock, flags);
- }
}
static void start_hrtimer_us(struct hrtimer *hrt, unsigned long usec)
@@ -1698,35 +1692,33 @@ dev_dbg_dma(port->dev, " submit new desc #%u\n",
active);
fail:
spin_unlock_irqrestore(>lock, flags);
dev_warn(port->dev, "Failed submitting Rx DMA descriptor\n");
- sci_rx_dma_release(s, true);
+ /* Switch to PIO */
+ spin_lock_irqsave(>lock, flags);
+ s->chan_rx = NULL;
+ sci_start_rx(port);
+ spin_unlock_irqrestore(>lock, flags);
}
-static void sci_tx_dma_release(struct sci_port *s, bool enable_pio)
+static void sci_tx_dma_release(struct sci_port *s)
{
- struct dma_chan *chan = s->chan_tx;
+ struct dma_chan *chan = s->chan_tx_saved;
struct uart_port *port = >port;
- unsigned long flags;
dev_dbg_dma(port->dev, "%s\n", __func__);
- spin_lock_irqsave(>lock, flags);
- s->chan_tx = NULL;
+ s->chan_tx_saved = s->chan_tx = NULL;
s->cookie_tx = -EINVAL;
- spin_unlock_irqrestore(>lock, flags);
WARN(!chan, "TX DMA channel already released\n");
dmaengine_terminate_all(chan);
dma_unmap_single(chan->device->dev, s->tx_dma_addr, UART_XMIT_SIZE,
DMA_TO_DEVICE);
dma_release_channel(chan);
- if (enable_pio) {
- spin_lock_irqsave(>lock, flags);
- sci_start_tx(port);
- spin_unlock_irqrestore(>lock, flags);
- }
}
static void sci_submit_rx(struct sci_port *s)
{
struct dma_chan *chan = s->chan_rx;
+ struct uart_port *port = >port;
+ unsigned long flags;
int i;
dev_dbg_dma(s->port.dev, " %s\n", __func__);
@@ -1760,7 +1752,11 @@ dev_dbg_dma(s->port.dev, " %s\n", __func__);
for (i = 0; i < 2; i++)
s->cookie_rx[i] = -EINVAL;
s->active_rx = -EINVAL;
- sci_rx_dma_release(s, true);
+ /* Switch to PIO */
+ spin_lock_irqsave(>lock, flags);
+ s->chan_rx = NULL;
+ sci_start_rx(port);
+ spin_unlock_irqrestore(>lock, flags);
}
static void work_fn_tx(struct work_struct *work)
@@ -1770,6 +1766,7 @@ static void work_fn_tx(struct work_struct *work)
struct dma_chan *chan = s->chan_tx;
struct uart_port *port = >port;
struct circ_buf *xmit = >state->xmit;
+ unsigned long flags;
dma_addr_t buf;
dev_dbg_dma(port->dev, "WORK %s\n", __func__);
@@ -1792,9 +1789,7 @@ dev_dbg_dma(port->dev, "WORK %s\n", __func__);
DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
if (!desc) {
dev_warn(port->dev, "Failed preparing Tx DMA descriptor\n");
- /* switch to PIO */
-
[PATCH 4/4] serial: sh-sci: Stop RX FIFO timer during port shutdown
The RX FIFO timer may be armed when the port is shut down, hence the
timer function may still be called afterwards.
Fix this race condition by deleting the timer during port shutdown.
Fixes: 039403765e5da3c6 ("serial: sh-sci: SCIFA/B RX FIFO software timeout")
Signed-off-by: Geert Uytterhoeven
---
Tested with scifa0 on r8a7740/armadillo, after
echo 2000 > /sys/devices/platform/e6c4.serial/rx_fifo_timeout
---
drivers/tty/serial/sh-sci.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 58016386558c6789..cca3bbc62a6b1110 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2571,6 +2571,8 @@ static void sci_shutdown(struct uart_port *port)
}
#endif
+ if (s->rx_trigger > 1 && s->rx_fifo_timeout > 0)
+ del_timer_sync(>rx_fifo_timer);
sci_free_irq(s);
sci_free_dma(port);
}
--
2.17.1
[PATCH 2/3] serial: sh-sci: Stop TX DMA workqueue during port shutdown
The transmit DMA workqueue is never stopped, hence the work function may be called after the port has been shut down. Fix this race condition by cancelling queued work, if any, before DMA release. Don't initialize the work if DMA initialization failed, as it won't be used anyway. Signed-off-by: Geert Uytterhoeven --- drivers/tty/serial/sh-sci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 0ed91692f53ad859..1e0b6fe6865dcff6 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -1295,6 +1295,7 @@ static void sci_tx_dma_release(struct sci_port *s) struct dma_chan *chan = s->chan_tx_saved; struct uart_port *port = >port; + cancel_work_sync(>work_tx); s->chan_tx_saved = s->chan_tx = NULL; s->cookie_tx = -EINVAL; dmaengine_terminate_all(chan); @@ -1550,10 +1551,9 @@ static void sci_request_dma(struct uart_port *port) __func__, UART_XMIT_SIZE, port->state->xmit.buf, >tx_dma_addr); + INIT_WORK(>work_tx, work_fn_tx); s->chan_tx_saved = s->chan_tx = chan; } - - INIT_WORK(>work_tx, work_fn_tx); } chan = sci_request_dma_chan(port, DMA_DEV_TO_MEM); -- 2.17.1
Re: [PATCH] PCI: rcar: Clean up PHY init on failure
On Fri, May 25, 2018 at 08:33:26PM +0200, Marek Vasut wrote:
> If the Gen3 PHY fails to power up, the code does not undo the
> initialization caused by phy_init(). Add the missing failure
> handling to the rcar_pcie_phy_init_gen3() function.
>
> Signed-off-by: Marek Vasut
> Reported-by: Geert Uytterhoeven
> Cc: Geert Uytterhoeven
> Cc: Lorenzo Pieralisi
> Cc: Phil Edworthy
> Cc: Simon Horman
> Cc: Wolfram Sang
> Cc: linux-renesas-soc@vger.kernel.org
> Fixes: 517ca93a7159 ("PCI: rcar: Add R-Car gen3 PHY support")
> ---
> drivers/pci/host/pcie-rcar.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
Applied to pci/controller-fixes to be tentatively merged for
-rc4, thanks.
Lorenzo
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index 695781934f0a..477bf40cc031 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -678,7 +678,11 @@ static int rcar_pcie_phy_init_gen3(struct rcar_pcie
> *pcie)
> if (err)
> return err;
>
> - return phy_power_on(pcie->phy);
> + err = phy_power_on(pcie->phy);
> + if (err)
> + phy_exit(pcie->phy);
> +
> + return err;
> }
>
> static int rcar_msi_alloc(struct rcar_msi *chip)
> --
> 2.16.2
>
Re: [PATCH v4 6/6] PCI: rcar: Shut the PHY down in failpath
On Thu, May 24, 2018 at 04:36:24PM +0200, Marek Vasut wrote:
> If anything fails past phy_init_fn() and the system is a Gen3 with
> a PHY, the PHY will be left on and inited. This is caused by the
> phy_init_fn, which is in fact a pointer to rcar_pcie_phy_init_gen3()
> function, which starts the PHY, yet has no counterpart in the failpath.
> Add that counterpart.
>
> Signed-off-by: Marek Vasut
> Cc: Geert Uytterhoeven
> Cc: Lorenzo Pieralisi
> Cc: Phil Edworthy
> Cc: Simon Horman
> Cc: Wolfram Sang
> Cc: linux-renesas-soc@vger.kernel.org
> Fixes: 517ca93a7159 ("PCI: rcar: Add R-Car gen3 PHY support")
> ---
> V4: New patch
> ---
> drivers/pci/host/pcie-rcar.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
Applied to pci/controller-fixes to be tentatively merged at -rc4,
thanks.
Lorenzo
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index 636c3c5095d2..695781934f0a 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -1163,7 +1163,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
> if (rcar_pcie_hw_init(pcie)) {
> dev_info(dev, "PCIe link down\n");
> err = -ENODEV;
> - goto err_clk_disable;
> + goto err_phy_shutdown;
> }
>
> data = rcar_pci_read_reg(pcie, MACSR);
> @@ -1175,7 +1175,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
> dev_err(dev,
> "failed to enable MSI support: %d\n",
> err);
> - goto err_clk_disable;
> + goto err_phy_shutdown;
> }
> }
>
> @@ -1189,6 +1189,12 @@ static int rcar_pcie_probe(struct platform_device
> *pdev)
> if (IS_ENABLED(CONFIG_PCI_MSI))
> rcar_pcie_teardown_msi(pcie);
>
> +err_phy_shutdown:
> + if (pcie->phy) {
> + phy_power_off(pcie->phy);
> + phy_exit(pcie->phy);
> + }
> +
> err_clk_disable:
> clk_disable_unprepare(pcie->bus_clk);
>
> --
> 2.16.2
>
[PATCH v2 2/2] mmc: renesas_sdhi_internal_dmac: Cannot clear the RX_IN_USE in abort
This patch is fixes an issue that the SDHI_INTERNAL_DMAC_RX_IN_USE
flag cannot be cleared because tmio_mmc_core sets the host->data
to NULL before the tmio_mmc_core calls tmio_mmc_abort_dma().
So, this patch clears the SDHI_INTERNAL_DMAC_RX_IN_USE in
the renesas_sdhi_internal_dmac_abort_dma() anyway. This doesn't
cause any side effects.
Fixes: 0cbc94daa554 ("mmc: renesas_sdhi_internal_dmac: limit DMA RX for old
SoCs")
Cc: # v4.17+
Signed-off-by: Yoshihiro Shimoda
Reviewed-by: Geert Uytterhoeven
---
drivers/mmc/host/renesas_sdhi_internal_dmac.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index d676a20..d032bd6 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -139,8 +139,7 @@
renesas_sdhi_internal_dmac_dm_write(host, DM_CM_RST,
RST_RESERVED_BITS | val);
- if (host->data && host->data->flags & MMC_DATA_READ)
- clear_bit(SDHI_INTERNAL_DMAC_RX_IN_USE, _flags);
+ clear_bit(SDHI_INTERNAL_DMAC_RX_IN_USE, _flags);
renesas_sdhi_internal_dmac_enable_dma(host, true);
}
--
1.9.1
[PATCH v2 0/2] mmc: renesas_sdhi_internal_dmac: fix two issues for error path
This patch set is based on the mmc.git / fixes branch. Changes from v1: - Add Reviewed-by Geert-san. - Add a new goto label for error path. Yoshihiro Shimoda (2): mmc: renesas_sdhi_internal_dmac: Fix missing unmap in error patch mmc: renesas_sdhi_internal_dmac: Cannot clear the RX_IN_USE in abort drivers/mmc/host/renesas_sdhi_internal_dmac.c | 15 +++ 1 file changed, 7 insertions(+), 8 deletions(-) -- 1.9.1
[PATCH v2 1/2] mmc: renesas_sdhi_internal_dmac: Fix missing unmap in error patch
This patch fixes an issue that lacks the dma_unmap_sg() calling in
the error patch of renesas_sdhi_internal_dmac_start_dma().
Fixes: 0cbc94daa554 ("mmc: renesas_sdhi_internal_dmac: limit DMA RX for old
SoCs")
Cc: # v4.17+
Signed-off-by: Yoshihiro Shimoda
Reviewed-by: Geert Uytterhoeven
---
drivers/mmc/host/renesas_sdhi_internal_dmac.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index f7f9773..d676a20 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -164,17 +164,14 @@
goto force_pio;
/* This DMAC cannot handle if buffer is not 8-bytes alignment */
- if (!IS_ALIGNED(sg_dma_address(sg), 8)) {
- dma_unmap_sg(>pdev->dev, sg, host->sg_len,
-mmc_get_dma_dir(data));
- goto force_pio;
- }
+ if (!IS_ALIGNED(sg_dma_address(sg), 8))
+ goto force_pio_with_unmap;
if (data->flags & MMC_DATA_READ) {
dtran_mode |= DTRAN_MODE_CH_NUM_CH1;
if (test_bit(SDHI_INTERNAL_DMAC_ONE_RX_ONLY, _flags) &&
test_and_set_bit(SDHI_INTERNAL_DMAC_RX_IN_USE,
_flags))
- goto force_pio;
+ goto force_pio_with_unmap;
} else {
dtran_mode |= DTRAN_MODE_CH_NUM_CH0;
}
@@ -189,6 +186,9 @@
return;
+force_pio_with_unmap:
+ dma_unmap_sg(>pdev->dev, sg, host->sg_len, mmc_get_dma_dir(data));
+
force_pio:
host->force_pio = true;
renesas_sdhi_internal_dmac_enable_dma(host, false);
--
1.9.1
RE: [PATCH 1/2] mmc: renesas_sdhi_internal_dmac: Fix missing unmap in error patch
Hi Geert-san,
> From: Geert Uytterhoeven, Sent: Thursday, June 28, 2018 9:50 PM
>
> Hi Shimoda-san,
>
> On Thu, Jun 28, 2018 at 1:53 PM Yoshihiro Shimoda
> wrote:
> > This patch fixes an issue that lacks the dma_unmap_sg() calling in
> > the error patch of renesas_sdhi_internal_dmac_start_dma().
>
> Nice catch!
> Thanks for your patch!
>
> > Fixes: 0cbc94daa554 ("mmc: renesas_sdhi_internal_dmac: limit DMA RX for old
> > SoCs")
> > Cc: # v4.17+
> > Signed-off-by: Yoshihiro Shimoda
>
> Reviewed-by: Geert Uytterhoeven
Thank you for your review!
> > --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> > +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> > @@ -173,8 +173,11 @@
> > if (data->flags & MMC_DATA_READ) {
> > dtran_mode |= DTRAN_MODE_CH_NUM_CH1;
> > if (test_bit(SDHI_INTERNAL_DMAC_ONE_RX_ONLY, _flags)
> > &&
> > - test_and_set_bit(SDHI_INTERNAL_DMAC_RX_IN_USE,
> > _flags))
> > + test_and_set_bit(SDHI_INTERNAL_DMAC_RX_IN_USE,
> > _flags)) {
> > + dma_unmap_sg(>pdev->dev, sg, host->sg_len,
> > +mmc_get_dma_dir(data));
>
> Given there is already a call to dma_unmap_sg() a few lines earlier , you
> may want to introduce a new label before force_pio, and move the call to
> dma_unmap_sg() there.
Thank you for the comment. So, I'll submit v2 patch.
Best regards,
Yoshihiro Shimoda
[PATCH v2 0/2] i2c: gpio: fault-injector: add new injector
Hi, here comes another fault injector which stress tests I2C bus recovery implementations. The generic_scl algorithm in the I2C core doesn't handle this yet. I am at it. But first, here is the test environment for it. Please share your thoughts if you have any. Thanks, Wolfram Changes since v1: * rebased to v4.18-rc2 * made documentation hopefully more clear Wolfram Sang (2): i2c: gpio: fault-injector: refactor incomplete transfer i2c: gpio: fault-injector: add incomplete_write_byte Documentation/i2c/gpio-fault-injection | 49 ++--- drivers/i2c/busses/i2c-gpio.c | 57 +- 2 files changed, 81 insertions(+), 25 deletions(-) -- 2.11.0
[PATCH v2 2/2] i2c: gpio: fault-injector: add incomplete_write_byte
Add another injector for an incomplete transfer. As mentioned in the
docs, this one is important to check bus recovery algorithms with it.
Otherwise random data may be sent to devices!
Signed-off-by: Wolfram Sang
---
Documentation/i2c/gpio-fault-injection | 19 +++
drivers/i2c/busses/i2c-gpio.c | 21 +
2 files changed, 40 insertions(+)
diff --git a/Documentation/i2c/gpio-fault-injection
b/Documentation/i2c/gpio-fault-injection
index 37542461cea4..a4ce62090fd5 100644
--- a/Documentation/i2c/gpio-fault-injection
+++ b/Documentation/i2c/gpio-fault-injection
@@ -60,3 +60,22 @@ above, the bus master under test should detect this
condition and try a bus
recovery. This time, however, it should succeed and the device should release
SDA after toggling SCL.
+"incomplete_write_byte"
+---
+
+Similar to above, this file is write only and you need to write the address of
+an existing I2C client device to it.
+
+The injector will again stop at one ACK phase, so the device will keep SDA low
+because it acknowledges data. However, there are two differences compared to
+'incomplete_address_phase':
+
+a) the message sent out will be a write message
+b) after the address byte, a 0x00 byte will be transferred. Then, stop at ACK.
+
+This is a highly delicate state, the device is set up to write any data to
+register 0x00 (if it has registers) when further clock pulses happen on SCL.
+This is why bus recovery (up to 9 clock pulses) must either check SDA or send
+additional STOP conditions to ensure the bus has been released. Otherwise
+random data will be written to a device!
+
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index b69bb46bdb9d..ac00b8e08251 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -143,6 +143,25 @@ static int fops_incomplete_addr_phase_set(void *data, u64
addr)
}
DEFINE_DEBUGFS_ATTRIBUTE(fops_incomplete_addr_phase, NULL,
fops_incomplete_addr_phase_set, "%llu\n");
+static int fops_incomplete_write_byte_set(void *data, u64 addr)
+{
+ struct i2c_gpio_private_data *priv = data;
+ u32 pattern;
+
+ if (addr > 0x7f)
+ return -EINVAL;
+
+ /* ADDR (7 bit) + WR (1 bit) + Client ACK (1 bit) */
+ pattern = (addr << 2) | 1;
+ /* 0x00 (8 bit) + Client ACK, keep SDA hi (1 bit) */
+ pattern = (pattern << 9) | 1;
+
+ i2c_gpio_incomplete_transfer(priv, pattern, 18);
+
+ return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(fops_incomplete_write_byte, NULL,
fops_incomplete_write_byte_set, "%llu\n");
+
static void i2c_gpio_fault_injector_init(struct platform_device *pdev)
{
struct i2c_gpio_private_data *priv = platform_get_drvdata(pdev);
@@ -166,6 +185,8 @@ static void i2c_gpio_fault_injector_init(struct
platform_device *pdev)
debugfs_create_file_unsafe("sda", 0600, priv->debug_dir, priv,
_sda);
debugfs_create_file_unsafe("incomplete_address_phase", 0200,
priv->debug_dir,
priv, _incomplete_addr_phase);
+ debugfs_create_file_unsafe("incomplete_write_byte", 0200,
priv->debug_dir,
+ priv, _incomplete_write_byte);
}
static void i2c_gpio_fault_injector_exit(struct platform_device *pdev)
--
2.11.0
[PATCH v2 1/2] i2c: gpio: fault-injector: refactor incomplete transfer
Make the incomplete_transfer routine reusable, so we can add other test
cases with different patterns later. Prepare the docs for that, too.
Signed-off-by: Wolfram Sang
---
Documentation/i2c/gpio-fault-injection | 30 +---
drivers/i2c/busses/i2c-gpio.c | 36 +-
2 files changed, 41 insertions(+), 25 deletions(-)
diff --git a/Documentation/i2c/gpio-fault-injection
b/Documentation/i2c/gpio-fault-injection
index e0c4f775e239..37542461cea4 100644
--- a/Documentation/i2c/gpio-fault-injection
+++ b/Documentation/i2c/gpio-fault-injection
@@ -34,21 +34,29 @@ I2C specification version 4, section 3.1.16) using the
helpers of the Linux I2C
core (see 'struct bus_recovery_info'). However, the bus recovery will not
succeed because SDA is still pinned low until you manually release it again
with "echo 1 > sda". A test with an automatic release can be done with the
-'incomplete_transfer' file.
+following class of fault injectors.
-"incomplete_transfer"
--
+Introduction to incomplete transfers
+
+
+The following fault injectors create situations where SDA will be held low by a
+device. Bus recovery should be able to fix these situations. But please note:
+there are I2C client devices which detect a stuck SDA on their side and release
+it on their own after a few milliseconds. Also, there might be an external
+device deglitching and monitoring the I2C bus. It could also detect a stuck SDA
+and will init a bus recovery on its own. If you want to implement bus recovery
+in a bus master driver, make sure you checked your hardware setup for such
+devices before. And always verify with a scope or logic analyzer!
+
+"incomplete_address_phase"
+--
This file is write only and you need to write the address of an existing I2C
-client device to it. Then, a transfer to this device will be started, but it
-will stop at the ACK phase after the address of the client has been
+client device to it. Then, a read transfer to this device will be started, but
+it will stop at the ACK phase after the address of the client has been
transmitted. Because the device will ACK its presence, this results in SDA
being pulled low by the device while SCL is high. So, similar to the "sda" file
above, the bus master under test should detect this condition and try a bus
recovery. This time, however, it should succeed and the device should release
-SDA after toggling SCL. Please note: there are I2C client devices which detect
-a stuck SDA on their side and release it on their own after a few milliseconds.
-Also, there are external devices deglitching and monitoring the I2C bus. They
-can also detect a stuck SDA and will init a bus recovery on their own. If you
-want to implement bus recovery in a bus master driver, make sure you checked
-your hardware setup carefully before.
+SDA after toggling SCL.
+
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index 66f85bbf3591..b69bb46bdb9d 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -101,17 +101,11 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_##wire, fops_##wire##_get,
fops_##wire##_set, "%ll
WIRE_ATTRIBUTE(scl);
WIRE_ATTRIBUTE(sda);
-static int fops_incomplete_transfer_set(void *data, u64 addr)
+static void i2c_gpio_incomplete_transfer(struct i2c_gpio_private_data *priv,
+ u32 pattern, u8 pattern_size)
{
- struct i2c_gpio_private_data *priv = data;
struct i2c_algo_bit_data *bit_data = >bit_data;
- int i, pattern;
-
- if (addr > 0x7f)
- return -EINVAL;
-
- /* ADDR (7 bit) + RD (1 bit) + SDA hi (1 bit) */
- pattern = (addr << 2) | 3;
+ int i;
i2c_lock_adapter(>adap);
@@ -119,8 +113,8 @@ static int fops_incomplete_transfer_set(void *data, u64
addr)
setsda(bit_data, 0);
udelay(bit_data->udelay);
- /* Send ADDR+RD, request ACK, don't send STOP */
- for (i = 8; i >= 0; i--) {
+ /* Send pattern, request ACK, don't send STOP */
+ for (i = pattern_size - 1; i >= 0; i--) {
setscl(bit_data, 0);
udelay(bit_data->udelay / 2);
setsda(bit_data, (pattern >> i) & 1);
@@ -130,10 +124,24 @@ static int fops_incomplete_transfer_set(void *data, u64
addr)
}
i2c_unlock_adapter(>adap);
+}
+
+static int fops_incomplete_addr_phase_set(void *data, u64 addr)
+{
+ struct i2c_gpio_private_data *priv = data;
+ u32 pattern;
+
+ if (addr > 0x7f)
+ return -EINVAL;
+
+ /* ADDR (7 bit) + RD (1 bit) + Client ACK, keep SDA hi (1 bit) */
+ pattern = (addr << 2) | 3;
+
+ i2c_gpio_incomplete_transfer(priv, pattern, 9);
return 0;
}
-DEFINE_DEBUGFS_ATTRIBUTE(fops_incomplete_transfer, NULL,
fops_incomplete_transfer_set, "%llu\n");
Re: [RFC PATCH] i2c: gpio: initialize SCL to HIGH again
On Sat, Jun 16, 2018 at 09:56:36PM +0900, Wolfram Sang wrote:
> It seems that during the conversion from gpio* to gpiod*, the initial
> state of SCL was wrongly switched to LOW. Fix it to be HIGH again.
>
> Fixes: 7bb75029ef34 ("i2c: gpio: Enforce open drain through gpiolib")
> Signed-off-by: Wolfram Sang
Applied to for-current, thanks!
signature.asc
Description: PGP signature
Re: [PATCH 1/1] i2c: rcar: handle RXDMA HW behaviour on Gen3
> > + /* Gen3 needs a reset before allowing RXDMA once */
> > + if (priv->devtype == I2C_RCAR_GEN3) {
>
> So on R-Car Gen3 the device is always reset, even if no reads will be done?
> Or is that something you cannot check for at this point?
You got a point. I copied the behaviour from the BSP to do this as early
as possible. Still, it might be worthwhile to merge this into
rcar_i2c_request_dma() and move the call to it to the front. Will check.
> > + if (priv->devtype == I2C_RCAR_GEN3) {
> > + priv->rstc = devm_reset_control_get_exclusive(>dev,
> > NULL);
> > + if (!IS_ERR(priv->rstc)) {
> > + ret = reset_control_status(priv->rstc);
>
> Why this call and check? To check if .status() is implemented (it always is
> on R-Car Gen3, if CONFIG_RESET_CONTROLLER is enabled), and to avoid the
> timeout in rcar_i2c_do_reset() on every transfer?
Yes. We can save all the hazzle if it is not implemented. And I didn't
want to implicitly assume that it will be implemented anywhere forever.
That could be a very subtle bug later.
Thanks,
Wolfram
signature.asc
Description: PGP signature
Re: [PATCH 1/1] i2c: rcar: handle RXDMA HW behaviour on Gen3
Hi Wolfram,
On Thu, Jun 28, 2018 at 10:45 PM Wolfram Sang
wrote:
> On Gen3, we can only do RXDMA once per transfer reliably. For that, we
> must reset the device, then we can have RXDMA once. This patch
> implements this. When there is no reset controller or the reset fails,
> RXDMA will be blocked completely. Otherwise, it will be disabled after
> the first RXDMA transfer. Based on a commit from the BSP by Hiromitsu
> Yamasaki, yet completely refactored to handle multiple read messages
> within one transfer.
>
> Signed-off-by: Wolfram Sang
Thanks for your patch!
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -751,6 +780,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
>
> pm_runtime_get_sync(dev);
>
> + /* Gen3 needs a reset before allowing RXDMA once */
> + if (priv->devtype == I2C_RCAR_GEN3) {
So on R-Car Gen3 the device is always reset, even if no reads will be done?
Or is that something you cannot check for at this point?
> + priv->flags |= ID_P_NO_RXDMA;
> + if (!IS_ERR(priv->rstc)) {
> + ret = rcar_i2c_do_reset(priv);
> + if (ret == 0)
> + priv->flags &= ~ID_P_NO_RXDMA;
> + }
> + }
> +
> rcar_i2c_init(priv);
>
> ret = rcar_i2c_bus_barrier(priv);
> @@ -921,6 +960,15 @@ static int rcar_i2c_probe(struct platform_device *pdev)
> if (ret < 0)
> goto out_pm_put;
>
> + if (priv->devtype == I2C_RCAR_GEN3) {
> + priv->rstc = devm_reset_control_get_exclusive(>dev,
> NULL);
> + if (!IS_ERR(priv->rstc)) {
> + ret = reset_control_status(priv->rstc);
Why this call and check? To check if .status() is implemented (it always is
on R-Car Gen3, if CONFIG_RESET_CONTROLLER is enabled), and to avoid the
timeout in rcar_i2c_do_reset() on every transfer?
> + if (ret < 0)
> + priv->rstc = ERR_PTR(-ENOTSUPP);
> + }
> + }
Anyway:
Reviewed-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
