Received-SPF: pass (google.com: domain of linux-kernel+bounces-151139-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <cbf77a8b-b37c-41d8-a7b5-a1c935571a1d@samsung.com>
Date: Fri, 19 Apr 2024 10:00:47 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 11/15] tty: msm_serial: use dmaengine_prep_slave_sg()
To: Jiri Slaby <jirislaby@kernel.org>, gregkh@linuxfoundation.org
Cc: linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Bjorn
	Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>,
	linux-arm-msm@vger.kernel.org, Anders Roxell <anders.roxell@linaro.org>
Content-Language: en-US
From: Marek Szyprowski <m.szyprowski@samsung.com>
In-Reply-To: <cbf6d34a-663c-440d-84e2-86054c605c8e@kernel.org>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20240405060826.2521-1-jirislaby@kernel.org>
	<20240405060826.2521-12-jirislaby@kernel.org>
	<CGME20240415211716eucas1p10050cc8d4024707dd6f6331111cd3ce1@eucas1p1.samsung.com>
	<d3eb9f21-f3e1-43ec-bf41-984c6aa5cfc8@samsung.com>
	<54679d54-3957-489d-a8b5-b98ea1c8a93c@kernel.org>
	<0335b679-da36-42c1-a1ba-8affb7a98d44@samsung.com>
	<783c05cd-0cd2-4b0e-9dce-2a9fdfee7c74@kernel.org>
	<c26a049c-07ef-4837-9c1f-ac19b1251c3b@samsung.com>
	<8947962f-e39e-4e96-9d1e-691e40d49349@kernel.org>
	<cbf6d34a-663c-440d-84e2-86054c605c8e@kernel.org>

On 19.04.2024 09:53, Jiri Slaby wrote:
> On 19. 04. 24, 9:43, Jiri Slaby wrote:
>> On 17. 04. 24, 14:45, Marek Szyprowski wrote:
>>> I alse tried to change the "if (dma->mapped)" check in 
>>> msm_start_tx() to:
>>>
>>> 1. if (dma->tx_sg.length)
>>>
>>> 2. if (dma->tx_sg.page_link & ~SG_PAGE_LINK_MASK)
>>>
>>> but none of the above worked what is really strange and 
>>> incomprehensible
>>> for me.
>>>
>>
>> Aaaah, nevermind, what about this?
>>
>> Two bugs:
>> 1) dma_map_sg() returns the number of mapped entries. Not zero!
>> 2) And I forgot to zero tx_sg in case of error.
>>
>> --- a/drivers/tty/serial/msm_serial.c
>> +++ b/drivers/tty/serial/msm_serial.c
>> @@ -506,8 +506,8 @@ static int msm_handle_tx_dma(struct msm_port 
>> *msm_port, unsigned int count)
>>          kfifo_dma_out_prepare(&tport->xmit_fifo, &dma->tx_sg, 1, 
>> count);
>>
>>          ret = dma_map_sg(port->dev, &dma->tx_sg, 1, dma->dir);
>> -       if (ret)
>> -               return ret;
>> +       if (!ret)
>
> Still wrong, ret = -EIO missing in here.

"if (ret <= 0)" seems to be better here.

Indeed this fixed the issue. I checked the code many times, but I missed 
this dma_map_sg() return value issue.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

>
>> +               goto zero_out;
>>
>>          dma->desc = dmaengine_prep_slave_sg(dma->chan, &dma->tx_sg, 1,
>>                                                  DMA_MEM_TO_DEV,
>> @@ -548,6 +548,7 @@ static int msm_handle_tx_dma(struct msm_port 
>> *msm_port, unsigned int count)
>>          return 0;
>>   unmap:
>>          dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir);
>> +zero_out:
>>          sg_init_table(&dma->tx_sg, 1);
>>          return ret;
>>   }
>>
>>
>> thanks,
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland