Re: [RFC PATCH kernel] powerpc/ioda: Set "read" permission when "write" is set

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 01/20/2016 06:01 AM, Douglas Miller wrote:
>
>
> On 01/18/2016 09:52 PM, Alexey Kardashevskiy wrote:
>> On 01/13/2016 01:24 PM, Douglas Miller wrote:
>>>
>>>
>>> On 01/12/2016 05:07 PM, Benjamin Herrenschmidt wrote:
>>>> On Tue, 2016-01-12 at 15:40 +1100, Alexey Kardashevskiy wrote:
>>>>> Quite often drivers set only "write" permission assuming that this
>>>>> includes "read" permission as well and this works on plenty
>>>>> platforms.
>>>>> However IODA2 is strict about this and produces an EEH when "read"
>>>>> permission is not and reading happens.
>>>>>
>>>>> This adds a workaround in IODA code to always add the "read" bit when
>>>>> the "write" bit is set.
>>>>>
>>>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>> ---
>>>>>
>>>>>
>>>>> Ben, what was the driver which did not set "read" and caused EEH?
>>>> aacraid
>>>>
>>>> Cheers,
>>>> Ben.
>>> Just to be precise, the driver wasn't responsible for setting READ. The
>>> driver called scsi_dma_map() and the scsicmd was set (by scsi layer) as
>>> DMA_FROM_DEVICE so the current code would set the permissions to
>>> WRITE-ONLY. Previously, and in other architectures, this scsicmd would have
>>> resulted in READ+WRITE permissions on the DMA map.
>>
>>
>> Does the patch fix the issue? Thanks.
>>
>>
>>
>>>>
>>>>> ---
>>>>>   arch/powerpc/platforms/powernv/pci.c | 6 ++++++
>>>>>   1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/arch/powerpc/platforms/powernv/pci.c
>>>>> b/arch/powerpc/platforms/powernv/pci.c
>>>>> index f2dd772..c7dcae5 100644
>>>>> --- a/arch/powerpc/platforms/powernv/pci.c
>>>>> +++ b/arch/powerpc/platforms/powernv/pci.c
>>>>> @@ -601,6 +601,9 @@ int pnv_tce_build(struct iommu_table *tbl, long
>>>>> index, long npages,
>>>>>       u64 rpn = __pa(uaddr) >> tbl->it_page_shift;
>>>>>       long i;
>>>>> +    if (proto_tce & TCE_PCI_WRITE)
>>>>> +        proto_tce |= TCE_PCI_READ;
>>>>> +
>>>>>       for (i = 0; i < npages; i++) {
>>>>>           unsigned long newtce = proto_tce |
>>>>>               ((rpn + i) << tbl->it_page_shift);
>>>>> @@ -622,6 +625,9 @@ int pnv_tce_xchg(struct iommu_table *tbl, long
>>>>> index,
>>>>>       BUG_ON(*hpa & ~IOMMU_PAGE_MASK(tbl));
>>>>> +    if (newtce & TCE_PCI_WRITE)
>>>>> +        newtce |= TCE_PCI_READ;
>>>>> +
>>>>>       oldtce = xchg(pnv_tce(tbl, idx), cpu_to_be64(newtce));
>>>>>       *hpa = be64_to_cpu(oldtce) & ~(TCE_PCI_READ |
>>>>> TCE_PCI_WRITE);
>>>>>       *direction = iommu_tce_direction(oldtce);
>>>> _______________________________________________
>>>> Linuxppc-dev mailing list
>>>> Linuxppc-dev@lists.ozlabs.org
>>>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>>>
>>> _______________________________________________
>>> Linuxppc-dev mailing list
>>> Linuxppc-dev@lists.ozlabs.org
>>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>>
>>
> I am still working on getting a machine to try this on. From code
> inspection, it looks like it should work. The problem is shortage of
> machines and machines tied-up by Test.

Any progress here? Thanks.




-- 
Alexey