| Message ID | 66c08e4df2032fde82a2f97544f41fd3a2f24a94.1532382222.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Stop losing firmware-set DMA masks | expand |
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@kernel.org> > CC: Frank Rowand <frowand.list@gmail.com> > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > 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 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 ");
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@kernel.org> >> CC: Frank Rowand <frowand.list@gmail.com> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> --- >> 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",
On 27/07/18 01:22, Grygorii Strashko wrote: [...] >> 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. That is intentional. Consider a 32-bit device on a bus with an upstream DMA range of 40 bits (which could easily happen with e.g. PCI). If the firmware code gives that device 40-bit DMA masks and the driver doesn't change them, then DMA is not going to work correctly. Therefore the firmware code cannot safely make {coherent_}dma_mask larger than the default value passed in, and if the default is 0 then that should be fixed in whatever code created the device, not worked around later. Robin. >> >> *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", > > >
On 07/27/2018 06:36 AM, Robin Murphy wrote: > On 27/07/18 01:22, Grygorii Strashko wrote: > [...] >>> 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. > > That is intentional. Consider a 32-bit device on a bus with an upstream > DMA range of 40 bits (which could easily happen with e.g. PCI). If the > firmware code gives that device 40-bit DMA masks and the driver doesn't > change them, then DMA is not going to work correctly. Therefore the > firmware code cannot safely make {coherent_}dma_mask larger than the > default value passed in, and if the default is 0 then that should be > fixed in whatever code created the device, not worked around later. Could you clarify what do you mean "firmware" here? On most ARM32 platforms there is only DT + kernel. And DT is usually only one source of information about DT configuration. Sry, but seems this changes makes impossible using DT for DMA parameters configuration any more. > > Robin. > >>> >>> *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", >> >> >>
On Fri, Jul 27, 2018 at 12:36:00PM +0100, Robin Murphy wrote: > That is intentional. Consider a 32-bit device on a bus with an upstream DMA > range of 40 bits (which could easily happen with e.g. PCI). If the firmware > code gives that device 40-bit DMA masks and the driver doesn't change them, > then DMA is not going to work correctly. Therefore the firmware code cannot > safely make {coherent_}dma_mask larger than the default value passed in, and > if the default is 0 then that should be fixed in whatever code created the > device, not worked around later. I think you're missing the point. When OF creates platform devices, they are created without any DMA masks what so ever. It is only during device probe that dma_configure() gets called, which then calls of_dma_configure(), where the DMA mask for any DT declared device is setup. What this means is that your change has broken all existing DT devices that are trying to use DMA, because they now end up with a zero coherent DMA mask and/or streaming DMA mask. Your original idea behind the patch may be a sound one, but since the implementation has caused a regression, the implementation is obviously broken, and that needs fixing or reworking in some way.
On 07/27/2018 01:13 PM, Russell King - ARM Linux wrote: > On Fri, Jul 27, 2018 at 12:36:00PM +0100, Robin Murphy wrote: >> That is intentional. Consider a 32-bit device on a bus with an upstream DMA >> range of 40 bits (which could easily happen with e.g. PCI). If the firmware >> code gives that device 40-bit DMA masks and the driver doesn't change them, >> then DMA is not going to work correctly. Therefore the firmware code cannot >> safely make {coherent_}dma_mask larger than the default value passed in, and >> if the default is 0 then that should be fixed in whatever code created the >> device, not worked around later. > > I think you're missing the point. > > When OF creates platform devices, they are created without any DMA masks > what so ever. It is only during device probe that dma_configure() gets > called, which then calls of_dma_configure(), where the DMA mask for any > DT declared device is setup. > > What this means is that your change has broken all existing DT devices > that are trying to use DMA, because they now end up with a zero coherent > DMA mask and/or streaming DMA mask. > > Your original idea behind the patch may be a sound one, but since the > implementation has caused a regression, the implementation is obviously > broken, and that needs fixing or reworking in some way. > There was additional patch [1] which fixes an issue. But I have a question to all: - The patch [1] sets default DMA mask to DMA_BIT_MASK(32) + dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); + if (!dev->dev.dma_mask) + dev->dev.dma_mask = &dev->dev.coherent_dma_mask; this will also work the same way for ARM64 - But of_dma_configure() still have code which does: dev->coherent_dma_mask &= mask; *dev->dma_mask &= mask; As result, DMA mask supplied from DT will always be truncated to 32 bit for platform devices. for example: soc0: soc0 { compatible = "simple-bus"; #address-cells = <2>; #size-cells = <2>; ranges; + dma-ranges = <0 0 0 0 0x10000 0>; ^ 48 bit DMA mask expected to be generated and assigned. But real mask will be DMA_BIT_MASK(32). As result, any DMA capable driver will have be modified to do dma_set_xxx_mask(), which disregards DT DMA configuration (especially for HW modules which are used on ARM32 and ARM64). Could it be considered to do one the changes below? 1) assign mask in case of dt 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); 2) use BITS_PER_LONG for default DMA mask for of_platform devices diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 7ba90c2..3f326e2 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -185,7 +185,7 @@ static struct platform_device *of_platform_device_create_pdata( if (!dev) goto err_clear_flag; - dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); + dev->dev.coherent_dma_mask = DMA_BIT_MASK(BITS_PER_LONG); if (!dev->dev.dma_mask) dev->dev.dma_mask = &dev->dev.coherent_dma_mask; 3) ... [1] https://www.spinics.net/lists/arm-kernel/msg668934.html
On 2018-07-27 7:13 PM, Russell King - ARM Linux wrote: > On Fri, Jul 27, 2018 at 12:36:00PM +0100, Robin Murphy wrote: >> That is intentional. Consider a 32-bit device on a bus with an upstream DMA >> range of 40 bits (which could easily happen with e.g. PCI). If the firmware >> code gives that device 40-bit DMA masks and the driver doesn't change them, >> then DMA is not going to work correctly. Therefore the firmware code cannot >> safely make {coherent_}dma_mask larger than the default value passed in, and >> if the default is 0 then that should be fixed in whatever code created the >> device, not worked around later. > > I think you're missing the point. No, I'm just failing to make it clearly enough. Sorry about that. > When OF creates platform devices, they are created without any DMA masks > what so ever. It is only during device probe that dma_configure() gets > called, which then calls of_dma_configure(), where the DMA mask for any > DT declared device is setup. Indeed, because the DMA mask initialisation which was originally part of OF platform device creation got factored out into of_dma_configure(), then of_dma_configure() got repurposed into a non-platform-device-specific helper, then DMA configuration got generalised more and moved from device creation to probe time, and it now transpires that what looked like an unnecessary vestigial leftover was in fact some OF-platform-specific code all along. Which, having now made that realisitaion, I've put back where it originally was and conceptually should be. > What this means is that your change has broken all existing DT devices > that are trying to use DMA, because they now end up with a zero coherent > DMA mask and/or streaming DMA mask. All existing DT devices that are trying to use DMA *without first calling dma_set_mask() etc. as the API expects* - I could point you at lines 166-171 of DMA-API-HOWTO.txt, but given that the last person to touch half this stuff was you, that feels almost impolite. It appears all the drivers on the machines I boot-tested are well-behaved and do do that. In a few hours we'll have a useful datapoint from the kernelci.org boot logs about how many aren't and don't. Happy accidents 'n'all... > Your original idea behind the patch may be a sound one, but since the > implementation has caused a regression, the implementation is obviously > broken, and that needs fixing or reworking in some way. Already done: http://git.infradead.org/users/hch/dma-mapping.git/commitdiff/a5516219b10218a87abb3352c82248ce3088e94a Robin.
On 2018-07-27 6:34 PM, Grygorii Strashko wrote: > On 07/27/2018 06:36 AM, Robin Murphy wrote: >> On 27/07/18 01:22, Grygorii Strashko wrote: >> [...] >>>> 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. >> >> That is intentional. Consider a 32-bit device on a bus with an >> upstream DMA range of 40 bits (which could easily happen with e.g. >> PCI). If the firmware code gives that device 40-bit DMA masks and the >> driver doesn't change them, then DMA is not going to work correctly. >> Therefore the firmware code cannot safely make {coherent_}dma_mask >> larger than the default value passed in, and if the default is 0 then >> that should be fixed in whatever code created the device, not worked >> around later. > > Could you clarify what do you mean "firmware" here? By "firmware code" in this context I mean of_dma_configure(), acpi_dma_configure() and anything else which may also do the equivalent in future, i.e. the code which processes dma coherency attributes and addressing restrictions from the firmware-provided machine description. DT is conceptually firmware, regardless of how embedded ARM bootloaders might provide it. > On most ARM32 platforms there is only DT + kernel. > And DT is usually only one source of information about DT configuration. > Sry, but seems this changes makes impossible using DT for DMA parameters > configuration any more. Well, it was also impossible in general before. of_dma_configure() has in practice never been able to set device masks to wider than 32 bits. Even if it did, that would just lead to breakage elsewhere, as above. There'll doubtless be a few more rounds of whack-a-mole until *all*B the brokenness has been flushed out. Robin.
On 2018-07-27 7:45 PM, Grygorii Strashko wrote: [...] > But I have a question to all: > - The patch [1] sets default DMA mask to DMA_BIT_MASK(32) > + dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > + if (!dev->dev.dma_mask) > + dev->dev.dma_mask = &dev->dev.coherent_dma_mask; > > this will also work the same way for ARM64 > > - But of_dma_configure() still have code which does: > dev->coherent_dma_mask &= mask; > *dev->dma_mask &= mask; > > As result, DMA mask supplied from DT will always be truncated > to 32 bit for platform devices. for example: > > soc0: soc0 { > compatible = "simple-bus"; > #address-cells = <2>; > #size-cells = <2>; > ranges; > + dma-ranges = <0 0 0 0 0x10000 0>; > > ^ 48 bit DMA mask expected to be generated and assigned. > > But real mask will be DMA_BIT_MASK(32). As result, any > DMA capable driver will have be modified to do dma_set_xxx_mask(), > which disregards DT DMA configuration (especially for HW modules > which are used on ARM32 and ARM64). That has always been the case. Drivers which want to use larger-than-32-bit masks *must* explicitly say so. The issue that the DT dma-ranges (and ACPI equivalents) cannot be preserved in the device DMA masks is the entire purpose of this series. At the moment there's only a couple of point fixes for specific places with known problems, but this is just the start of some ongoing work. > Could it be considered to do one the changes below? > > 1) assign mask in case of dt > 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); No, because that leads to a risk of DMA address truncation in hardware (and thus at worst random memory corruption) when drivers expect the default mask to be 32-bit and fail to explicitly set it as such. > 2) use BITS_PER_LONG for default DMA mask for of_platform devices > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index 7ba90c2..3f326e2 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -185,7 +185,7 @@ static struct platform_device *of_platform_device_create_pdata( > if (!dev) > goto err_clear_flag; > > - dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > + dev->dev.coherent_dma_mask = DMA_BIT_MASK(BITS_PER_LONG); > if (!dev->dev.dma_mask) > dev->dev.dma_mask = &dev->dev.coherent_dma_mask; No, because that leads to a risk of DMA address truncation in hardware (and thus at worst random memory corruption) when drivers expect the default mask to be 32-bit and fail to explicitly set it as such. > 3) ... Remember when we found out how many drivers expect the default mask to be 32-bit and fail to explicitly set it as such, because they all broke when some chump set it to 0 in linux-next for a day? ;) Robin.
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;
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@kernel.org> CC: Frank Rowand <frowand.list@gmail.com> Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/of/device.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)