On Wed, Jun 08, 2016 at 12:01:20PM +0200, Marek Szyprowski wrote:
Hi Liviu,
Hi Marek,
[HDLCD bug report content removed]
OK, the problem is that commit 59ce4039727ef40 has changed the behaviour from when there is no "memory-region" phandle in the DT: before it used to return -ENODEV, now it returns -EINVAL.
Marek, I quite liked the old behaviour to detect if the DT had the optional (from my driver's point of view) "memory-region" phandle. Plus the check for dev is superfluous when using of_reserved_mem_device_init() as that uses dev->of_node for np so it would crash before the check anyway. Maybe move the check there?
Until then I suggest reverting the 59ce4039727ef40 commit.
I've just send a fix for this issue. I'm sorry for the regression. I hope the fix fill quickly get into next to solve your problem.
Thanks for the patch, however I have some comments to it.
The additional check for null dev make sense, because the new function of_reserved_mem_device_init_by_idx can be called with any device and node pointer not embedded with it, so I would like to keep it safe.
And I have to admit I find that scary. Why do you accept any node that is *not* related to the device? If you want just the ability to parse multiple "memory-region" phandles (where are the bindings defined for that?) I would have modified of_reserved_mem_device_init() to the the parsing and accept either the single entry style or a node with multiple "memory-region" phandles in it. Otherwise I can steal the "memory-region" of another device and that device would have no idea that I have done this.
The idea is not to steal memory region from another device, but to let driver to use multiple regions assigned to the supported device with dma-mapping api. To use them with dma-mapping subsystem, one needs separate struct device for each reserved region. The idea is to create child devices of the device, which has memory-region property. Then for each such child device, driver can call of_reserved_mem_device_init_by_idx() to enable usage of dma-mapping api based on the selected reserved region. Such approach was already used for long time in the media/platform/s5p-mfc driver and now it has been converted to use generic reserved memory regions.
If you feel scary about this approach, maybe a check if the device provided to of_reserved_mem_device_init_by_idx() function is one of the successors of the device hidden behind the provided node pointer (or points to the same device)?
That would not hurt to have.
Can you point me to the latest thread where this patch has been discussed?
This patch is a part of "Exynos: MFC driver: reserved memory cleanup and IOMMU support" thread and has been around for a while: https://www.mail-archive.com/linux-media@vger.kernel.org/msg97645.html
Thanks! I'm not subscribed to linux-media and way behind a lot of other MLs, so I have not noticed it.
I've had a look at the patchset, found one case where you might leak some memory.
I also think you could've had an of_reserved_mem_device_init_list() function that inits all the "memory-region" nodes and returns a list struct reserved_mem* that the driver can then assign to whatever internal variables it has. That way you can validate that all the "memory-region" phandles are used by the same parent device.
Best regards, Liviu
Best regards
Marek Szyprowski, PhD Samsung R&D Institute Poland