HDLCD crashes with 6d910bfa809e

On Wed, Jun 08, 2016 at 12:01:20PM +0200, Marek Szyprowski wrote:

Hi Liviu,

Hi Marek,

[HDLCD bug report content removed]

quoted

quoted

quoted

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.

quoted

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.

quoted

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 at 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

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?