RE: [PATCH] serial: samsung: use dma_ops of DMA if attached

["M Tamseel Shams" <m.shams@samsung.com>]

> > Hi,
> >
> >>
> >> Hi,
> >>
> >> Thanks for the patch.
> >>
> >> On 21/06/2021 06:49, Tamseel Shams wrote:
> >>> When DMA is used for TX and RX by serial driver, it should pass the
> >>> DMA device pointer to DMA API instead of UART device pointer.
> >>
> >> Hmmm, but why DMA device pointer should be used?
> >>
> >>>
> >>> This patch is necessary to fix the SMMU page faults which is
> >>> observed when a DMA(with SMMU enabled) is attached to UART for
> transfer.
> >>>
> >>> Signed-off-by: Tamseel Shams <m.shams@samsung.com>
> >>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> >>> ---
> >>>  drivers/tty/serial/samsung_tty.c | 60
> >>> +++++++++++++++++++++++++-------
> >>>  1 file changed, 48 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/tty/serial/samsung_tty.c
> >>> b/drivers/tty/serial/samsung_tty.c
> >>> index b923683e6a25..5bdc7dd2a5e2 100644
> >>> --- a/drivers/tty/serial/samsung_tty.c
> >>> +++ b/drivers/tty/serial/samsung_tty.c
> >>> @@ -284,8 +284,13 @@ static void s3c24xx_serial_stop_tx(struct
> >>> uart_port
> >> *port)
> >>>  	struct s3c24xx_uart_dma *dma = ourport->dma;
> >>>  	struct circ_buf *xmit = &port->state->xmit;
> >>>  	struct dma_tx_state state;
> >>> +	struct device *dma_map_ops_dev = ourport->port.dev;
> >>>  	int count;
> >>>
> >>> +	/* Pick dma_ops of DMA device if DMA device is attached */
> >>
> >> You mention here and further comments "dma_ops". I don't see you
> >> changing the DMA ops, but the device. It's quite confusing. I think
> >> you meant a DMA device shall be passed to DMA API?
> >>
> > Yes, DMA device should be used for DMA API because only the DMA device
> > is aware of how the device connects to the memory. There might be an
> > extra level of address translation due to a SMMU attached to the DMA
> > device. When serial device pointer device is used for DMA API, the DMA API
> will have no clue of the SMMU attached to the DMA device.
> 
> Thanks, this should be in commit msg.
> 
Sure, will add this in commit msg.
> >
> >> Second question: you write that DMA devices should be used if DMA is
> >> attached and in the code you follow such pattern a lot:
> >>
> >>> +	if (dma && dma->tx_chan)
> >>> +		dma_map_ops_dev = dma->tx_chan->device->dev;
> >>> +
> >>
> >> Are you trying to say that if DMA is not attached, UART device should
> >> be used? If DMA is not attached, how are the DMA operations used then?
> >>
> > If DMA is not attached, this part of code related to dma_engine or DMA
> > API do not get called. There will not be any DMA operations at all.
> 
> Now I get it. The "When" in your description followed by multiple comments "if
> DMA device is attached" confused me that you expect to use UART device for
> DMA operations if DMA is not attached...
> 
I will change the comments, to avoid this confusion.
> >
> >>>  	if (!ourport->tx_enabled)
> >>>  		return;
> >>>
> >>> @@ -298,7 +303,7 @@ static void s3c24xx_serial_stop_tx(struct
> >>> uart_port
> >> *port)
> >>>  		dmaengine_pause(dma->tx_chan);
> >>>  		dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
> >>>  		dmaengine_terminate_all(dma->tx_chan);
> >>> -		dma_sync_single_for_cpu(ourport->port.dev,
> >>> +		dma_sync_single_for_cpu(dma_map_ops_dev,
> >>>  			dma->tx_transfer_addr, dma->tx_size,
> >> DMA_TO_DEVICE);
> >>>  		async_tx_ack(dma->tx_desc);
> >>>  		count = dma->tx_bytes_requested - state.residue; @@ -324,15
> >> +329,19
> >>> @@ static void s3c24xx_serial_tx_dma_complete(void *args)
> >>>  	struct circ_buf *xmit = &port->state->xmit;
> >>>  	struct s3c24xx_uart_dma *dma = ourport->dma;
> >>>  	struct dma_tx_state state;
> >>> +	struct device *dma_map_ops_dev = ourport->port.dev;
> >>>  	unsigned long flags;
> >>>  	int count;
> >>>
> >>> +	/* Pick dma_ops of DMA device if DMA device is attached */
> >>> +	if (dma && dma->tx_chan)
> >>> +		dma_map_ops_dev = dma->tx_chan->device->dev;
> 
> Example is this one - you use here "if" suggesting there is "else". So what is the
> else condition? There is none...
> 
> >>>
> >>>  	dmaengine_tx_status(dma->tx_chan, dma->tx_cookie, &state);
> >>>  	count = dma->tx_bytes_requested - state.residue;
> >>>  	async_tx_ack(dma->tx_desc);
> >>>
> >>> -	dma_sync_single_for_cpu(ourport->port.dev, dma->tx_transfer_addr,
> >>> +	dma_sync_single_for_cpu(dma_map_ops_dev, dma->tx_transfer_addr,
> >>>  				dma->tx_size, DMA_TO_DEVICE);
> >>>
> >>>  	spin_lock_irqsave(&port->lock, flags); @@ -408,7 +417,11 @@ static
> >>> int s3c24xx_serial_start_tx_dma(struct s3c24xx_uart_port *ourport,
> >>>  	struct uart_port *port = &ourport->port;
> >>>  	struct circ_buf *xmit = &port->state->xmit;
> >>>  	struct s3c24xx_uart_dma *dma = ourport->dma;
> >>> +	struct device *dma_map_ops_dev = ourport->port.dev;
> >>>
> >>> +	/* Pick dma_ops of DMA device if DMA device is attached */
> >>> +	if (dma && dma->tx_chan)
> >>> +		dma_map_ops_dev = dma->tx_chan->device->dev;
> >>>
> >>>  	if (ourport->tx_mode != S3C24XX_TX_DMA)
> >>>  		enable_tx_dma(ourport);
> >>> @@ -416,7 +429,7 @@ static int s3c24xx_serial_start_tx_dma(struct
> >> s3c24xx_uart_port *ourport,
> >>>  	dma->tx_size = count & ~(dma_get_cache_alignment() - 1);
> >>>  	dma->tx_transfer_addr = dma->tx_addr + xmit->tail;
> >>>
> >>> -	dma_sync_single_for_device(ourport->port.dev, dma-
> >>> tx_transfer_addr,
> >>> +	dma_sync_single_for_device(dma_map_ops_dev, dma-
> >>> tx_transfer_addr,
> >>>  				dma->tx_size, DMA_TO_DEVICE);
> >>>
> >>>  	dma->tx_desc = dmaengine_prep_slave_single(dma->tx_chan,
> >>> @@ -483,12 +496,17 @@ static void s3c24xx_uart_copy_rx_to_tty(struct
> >> s3c24xx_uart_port *ourport,
> >>>  		struct tty_port *tty, int count)
> >>>  {
> >>>  	struct s3c24xx_uart_dma *dma = ourport->dma;
> >>> +	struct device *dma_map_ops_dev = ourport->port.dev;
> >>>  	int copied;
> >>>
> >>> +	/* Pick dma_ops of DMA device if DMA device is attached */
> >>> +	if (dma && dma->rx_chan)
> >>> +		dma_map_ops_dev = dma->rx_chan->device->dev;
> >>> +
> >>>  	if (!count)
> >>>  		return;
> >>>
> >>> -	dma_sync_single_for_cpu(ourport->port.dev, dma->rx_addr,
> >>> +	dma_sync_single_for_cpu(dma_map_ops_dev, dma->rx_addr,
> >>>  				dma->rx_size, DMA_FROM_DEVICE);
> >>>
> >>>  	ourport->port.icount.rx += count;
> >>> @@ -600,8 +618,13 @@ static void s3c24xx_serial_rx_dma_complete(void
> >>> *args)  static void s3c64xx_start_rx_dma(struct s3c24xx_uart_port
> >>> *ourport)  {
> >>>  	struct s3c24xx_uart_dma *dma = ourport->dma;
> >>> +	struct device *dma_map_ops_dev = ourport->port.dev;
> >>>
> >>> -	dma_sync_single_for_device(ourport->port.dev, dma->rx_addr,
> >>> +	/* Pick dma_ops of DMA device if DMA device is attached */
> >>> +	if (dma && dma->rx_chan)
> >>> +		dma_map_ops_dev = dma->rx_chan->device->dev;
> >>> +
> >>> +	dma_sync_single_for_device(dma_map_ops_dev, dma->rx_addr,
> >>>  				dma->rx_size, DMA_FROM_DEVICE);
> >>>
> >>>  	dma->rx_desc = dmaengine_prep_slave_single(dma->rx_chan,
> >>> @@ -983,6 +1006,7 @@ static int s3c24xx_serial_request_dma(struct
> >>> s3c24xx_uart_port *p)
> >>
> >> Offset of hunks looks here significantly different than mainline. The
> >> patch should be based and tested mainline tree. Which one did you choose as
> base?
> >>
> >> Using my email address not from get_maintainers.pl also suggests that
> >> you don't use anything recent as a base.
> >>
> > I used "master" branch of main linux-next tree as the base.
> > I will rebase on "tty-next" branch of TTY tree and post again.
> >
> > Thanks & Regards,
> > Tamseel Shams
> >
> 
> 
> Best regards,
> Krzysztof

Thanks & Regards,
Tamseel Shams


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel