Re: [PATCH v2 7/7] OF: Don't set default coherent DMA mask

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 07/26/2018 06:52 PM, Grygorii Strashko wrote:
> 
> 
> On 07/23/2018 05:16 PM, Robin Murphy wrote:
>> Now that we can track upstream DMA constraints properly with
>> bus_dma_mask instead of trying (and failing) to maintain it in
>> coherent_dma_mask, it doesn't make much sense for the firmware code to
>> be touching the latter at all. It's merely papering over bugs wherein a
>> driver has failed to call dma_set_coherent_mask() *and* the bus code has
>> not initialised any default value.
>>
>> We don't really want to encourage more drivers coercing dma_mask so
>> we'll continue to fix that up if necessary, but add a warning to help
>> flush out any such buggy bus code that remains.
>>
>> CC: Rob Herring <robh+dt@xxxxxxxxxx>
>> CC: Frank Rowand <frowand.list@xxxxxxxxx>
>> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
>> ---
>>    drivers/of/device.c | 20 ++++++++++----------
>>    1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index 0d39633e8545..5957cd4fa262 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -127,20 +127,20 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
>>    	}
>>    
>>    	/*
>> -	 * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
>> -	 * setup the correct supported mask.
>> +	 * If @dev is expected to be DMA-capable then the bus code that created
>> +	 * it should have initialised its dma_mask pointer by this point. For
>> +	 * now, we'll continue the legacy behaviour of coercing it to the
>> +	 * coherent mask if not, but we'll no longer do so quietly.
>>    	 */
>> -	if (!dev->coherent_dma_mask)
>> -		dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> -	/*
>> -	 * Set it to coherent_dma_mask by default if the architecture
>> -	 * code has not set it.
>> -	 */
>> -	if (!dev->dma_mask)
>> +	if (!dev->dma_mask) {
>> +		dev_warn(dev, "DMA mask not set\n");
>>    		dev->dma_mask = &dev->coherent_dma_mask;
>> +	}
>>    
>> -	if (!size)
>> +	if (!size && dev->coherent_dma_mask)
>>    		size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
>> +	else if (!size)
>> +		size = 1ULL << 32;
>>    
>>    	dev->dma_pfn_offset = offset;
>>    
>>
> 
> the result of this change is pretty strange as for me :(
> Resulted code:
> 
> 	/*
> 	 * If @dev is expected to be DMA-capable then the bus code that created
> 	 * it should have initialised its dma_mask pointer by this point. For
> 	 * now, we'll continue the legacy behaviour of coercing it to the
> 	 * coherent mask if not, but we'll no longer do so quietly.
> 	 */
> 	if (!dev->dma_mask) {
> 		dev_warn(dev, "DMA mask not set\n");
> 		dev->dma_mask = &dev->coherent_dma_mask;
> ^this will always produce warning in case of platform-bus or if there are no bus driver.
> even if DT contains correct definition of dma-range
> 	}
> 
> 	if (!size && dev->coherent_dma_mask)
> 
> ^ coherent_dma_mask is zero, so size will not be calculated
pls, ignore this particular comment 

> 		size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
> 	else if (!size)
> 		size = 1ULL << 32;
> 
> 	dev->dma_pfn_offset = offset;
> 
> 	/*
> 	 * Limit coherent and dma mask based on size and default mask
> 	 * set by the driver.
> 	 */
> 	mask = DMA_BIT_MASK(ilog2(dma_addr + size - 1) + 1);
> 	dev->bus_dma_mask = mask;
> 	dev->coherent_dma_mask &= mask;
> 
> ^^ if nobody set coherent_dma_mask before it will stay null forever unless drivers
> will overwrite it. Again even if DT has correct definition for dma-range.
> 
> 	*dev->dma_mask &= mask;
> 
> 	coherent = of_dma_is_coherent(np);
> 	dev_dbg(dev, "device is%sdma coherent\n",
> 		coherent ? " " : " not ");
> 
> 

FYI, with below diff i can boot at least:

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 5957cd4..f7dc121 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -150,8 +150,8 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
         */
        mask = DMA_BIT_MASK(ilog2(dma_addr + size - 1) + 1);
        dev->bus_dma_mask = mask;
-       dev->coherent_dma_mask &= mask;
-       *dev->dma_mask &= mask;
+       dev->coherent_dma_mask = mask;
+       *dev->dma_mask = mask;
 
        coherent = of_dma_is_coherent(np);
        dev_dbg(dev, "device is%sdma coherent\n",



-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux