linux-kernel - Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 21 Dec 2015 19:24:12 +0200
From:	Andy Shevchenko <andy.shevchenko@...il.com>
To:	Måns Rullgård <mans@...sr.com>
Cc:	Viresh Kumar <viresh.linux@...il.com>,
	Julian Margetson <runaway@...dw.ms>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Tejun Heo <tj@...nel.org>, linux-ide@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find
 dma channel

On Mon, Dec 21, 2015 at 2:15 PM, Måns Rullgård <mans@...sr.com> wrote:
> Andy Shevchenko <andy.shevchenko@...il.com> writes:
>
>> +Viresh
>>
>> On Mon, Dec 21, 2015 at 2:58 AM, Måns Rullgård <mans@...sr.com> wrote:
>>> Andy Shevchenko <andy.shevchenko@...il.com> writes:
>>>
>>>> On Sun, Dec 20, 2015 at 8:49 PM, Måns Rullgård <mans@...sr.com> wrote:
>>>>> Julian Margetson <runaway@...dw.ms> writes:
>>>>>> On 12/20/2015 1:11 PM, Måns Rullgård wrote:
>>>>>>> Julian Margetson <runaway@...dw.ms> writes:
>>>>
>>>>>> [   48.769671] ata3.00: failed command: READ FPDMA QUEUED
>>>>>
>>>>> Well, that didn't help.  I still think it's part of the problem, but
>>>>> something else must be wrong as well.  The various Master Select fields
>>>>> look like a good place to start.
>>>>
>>>> Master number (which is here would be either 1 or 0) should not affect
>>>> as long as they are connected to the same AHB bus (I would be
>>>> surprised if they are not).
>>>
>>> I think they are not.  The relevant part of the block diagram for the
>>> 460EX looks something like this:
>>>
>>>       +-----+
>>>       | CPU |
>>>       +-----+
>>>          |
>>>  +---------------+
>>>  |      BUS      |
>>>  +---------------+
>>>     |         |
>>>  +-----+   +-----+
>>>  | DMA |   | RAM |
>>>  +-----+   +-----+
>>>     |
>>>  +------+
>>>  | SATA |
>>>  +------+
>>>
>>> The DMA-SATA link is private and ignores the address, which is the only
>>> reason the driver can possibly work (it's programming a CPU virtual
>>> address there).
>>
>> If you look at the original code the SMS and DMS are programmed
>> statically independent on DMA direction, so LLP is programmed always
>> to master 1. I don't think your scheme is reflecting this right. I
>> could imagine two AHB buses, one of them connects CPU, SATA and RAM,
>> and the other CPU and DMA.
>
> Check the code again.  The original code swaps SMS and DMS depending on
> direction, and it sets LMS to 1.  Put differently, it always sets the
> memory side 1 and the device side to 0.  The dw_dma driver sets SMS and
> DMS to the src/dst_master values provided through dma_request_channel()
> regardless of the current direction and LMS always zero.

I used to have a patch to implement this in dw_dmac driver. However, I
dropped it at some point. Seems we need it back and now I possible
have a good explanation why.

> If those
> values didn't matter, why would the fields exist in the first place?

Because someone can have more than one AHB bus on the system and
connect DMA to all of them (up to 4).

>> In any case on all Intel SoCs and AVR32, and as far as I can tell on
>> Spear13xx (Viresh?) there is not a case, that's why I hardly imagine
>> that the problem is in master numbers by themselves.
>
> The 460EX is a PowerPC system.  Expect unusual topologies.

Yeah, that's right.

>>>>> Also, the manual says the LLP_SRC_EN
>>>>> and LLP_DST_EN flags should be cleared on the last in a chain of blocks.
>>>>> The old sata_dwc driver does this whereas dw_dma does not.
>>>>
>>>> Easy to fix, however I can't get how it might affect.
>>>
>>> From the Atmel doc:
>>>
>>>   In Table 17-1 on page 185, all other combinations of LLPx.LOC = 0,
>>>   CTLx.LLP_S_EN, CFGx.RELOAD_SR, CTLx.LLP_D_EN, and CFGx.RELOAD_DS are
>>>   illegal, and causes indeterminate or erroneous behavior.
>>
>> I will check Synospys documentation later on.

Yes, we have to clear those bits. I will do a patch or you already have one?

>>> Most likely nothing happens, but I think it ought to be fixed.  In fact,
>>> I have a patch already.
>>
>> Good. Send with Fixes tag if it's upstream ready.
>>
>>> Come to think of it, I have an AVR32 dev somewhere.  Maybe I should dust
>>> it off.
>>
>> I have ATNGW100.
>
> I have an AT32ATK1006.  Can you suggest a good test to exercise the DMA
> engine?

On that board I tried MMC (the only available user for me), though it
is not reliable, I also tried the dmatest module.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/