Re: [PATCH v2] device property: preserve usecount for node passed to of_fwnode_graph_get_port_parent()

[Sakari Ailus]

Hi Rob,

On Tue, Aug 22, 2017 at 02:42:10PM -0500, Rob Herring wrote:
> On Tue, Aug 22, 2017 at 10:00 AM, Niklas Söderlund
>  wrote:
> > Hi Rob,
> >
> > On 2017-08-22 09:49:35 -0500, Rob Herring wrote:
> >> On Mon, Aug 21, 2017 at 7:19 PM, Niklas Söderlund
> >>  wrote:
> >> > Using CONFIG_OF_DYNAMIC=y uncovered an imbalance in the usecount of the
> >> > node being passed to of_fwnode_graph_get_port_parent(). Preserve the
> >> > usecount by using of_get_parent() instead of of_get_next_parent() which
> >> > don't decrement the usecount of the node passed to it.
> >> >
> >> > Fixes: 3b27d00e7b6d7c88 ("device property: Move fwnode graph ops to 
> >> > firmware specific locations")
> >> > Signed-off-by: Niklas Söderlund 
> >> > Acked-by: Sakari Ailus 
> >> > ---
> >> >  drivers/of/property.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> Isn't this already fixed with this fix:
> >>
> >> commit c0a480d1acf7dc184f9f3e7cf724483b0d28dc2e
> >> Author: Tony Lindgren 
> >> Date:   Fri Jul 28 01:23:15 2017 -0700
> >>
> >> device property: Fix usecount for of_graph_get_port_parent()
> >
> > No, that commit fixes it for of_graph_get_port_parent() while this
> > commit fixes it for of_fwnode_graph_get_port_parent(). But in essence it
> > is the same issue but needs two separate fixes.
> 
> Ah, because one takes the port node and one takes the endpoint node.
> That won't confuse anyone.

Yes, I think we've had this for some time in naming of a few graph
functions and increasingly so lately. It began with
of_graph_get_remote_port_parent(), which likely was named so to avoid the
name getting really long. The function itself gets a remove of the endpoint
given as an argument, then the port related to the entpoint and finally the
parent node of the port node (which is not the "ports" node). That's a lot
of work for a single interface function.

What comes to of_fwnode_graph_get_port_parent() --- it's the OF callback
function for the fwnode graph API that, as the name suggests, gets the
parent of the port node, no more, no less. The function is used in the
fwnode graph API implementation and is not available in the API as such.
The fwnode graph API itself only implements functionality already available
in the OF graph API under the corresponding name.

> 
> Can we please align this mess. I've tried to make the graph parsing
> not a free for all, open coded mess. There's no reason to have the
> port node handle and then need the parent device. Either you started
> with the parent device to parse local ports and endpoints or you got
> the remote endpoint with .graph_get_remote_endpoint(). Most of the
> time you don't even need the endpoint node handles. You really just
> need to know what is the remote device connected to port X, endpoint Y
> of my local device.

Perhaps most of the time, yes. V4L2 devices store bus (e.g. MIPI CSI-2)
specific information in the endpoint nodes.

The current OF graph API is geared towards providing convenience functions
to the extent that a single function performs actions a driver would
typically need. More recently functions implementing a single operation has
been added; the primitives that just perform a single operation would
likely be easier to manage.

The convenience functions have been, well, convenient as getting and
putting nodes could have been somewhat ignored in drivers. If the OF graph
API usage can be moved out of the drivers we'll likely have way fewer users
and thus there's no real need for convenience functions. That has other
benefits, too, such as parsing the graph correctly: most V4L2 drivers have
issues in this area.

The OF graph API (or the fwnode equivalent) is used effectively in all V4L2
drivers that support OF (there are about 20 of them); we're moving these to
the V4L2 framework but it'll take some time. That should make it easier for
cleaning things up. Based on a quick look V4L2 and V4L2 drivers together
represent a large proportion of all users in the kernel.

What do you think?

-- 
Kind regards,

Sakari Ailus
sakari.ai...@linux.intel.com

Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling

[Mauro Carvalho Chehab]

Em Thu, 17 Aug 2017 16:14:46 +0200
Wolfram Sang  escreveu:

> Signed-off-by: Wolfram Sang 
> ---
>  Documentation/i2c/DMA-considerations | 50 
> 
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/i2c/DMA-considerations
> 
> diff --git a/Documentation/i2c/DMA-considerations 
> b/Documentation/i2c/DMA-considerations
> new file mode 100644
> index 00..a4b4a107102452
> --- /dev/null
> +++ b/Documentation/i2c/DMA-considerations
> @@ -0,0 +1,50 @@
> +Linux I2C and DMA
> +-

I would use, instead:

=
Linux I2C and DMA
=

As this is the way we're starting document titles, after converted to
ReST. So, better to have it already using the right format, as one day
someone will convert i2c documentation to ReST. So, it would be
really cool if this document could be just renamed without needing
to patch it during such conversion :-)

There are also a couple of things here that Sphinx would complain.
So, it could be worth to rename it to *.rst, while you're writing
it, and see what:
make htmldocs
will complain and how it will look in html.

> +
> +Given that I2C is a low-speed bus where largely small messages are 
> transferred,
> +it is not considered a prime user of DMA access. At this time of writing, 
> only
> +10% of I2C bus master drivers have DMA support implemented.

Are you sure about that? I'd say that, on media, at least half of the
drivers use DMA for I2C bus access, as the I2C bus is on a remote
board that talks with CPU via USB, using DMA, and all communication
with USB should be DMA-safe.

I guess what you really wanted to say on most of this section is
about SoC (and other CPUs) where the I2C bus master is is at the
mainboard, and not on some peripheral.

> And the vast
> +majority of transactions are so small that setting up DMA for it will likely
> +add more overhead than a plain PIO transfer.
> +
> +Therefore, it is *not* mandatory that the buffer of an I2C message is DMA 
> safe.

Again, that may not be true on media boards. The code that implements the
I2C transfers there, on most boards, have to be DMA safe, as it won't
otherwise send/receive commands from the chips that are after the USB
bridge.

> +It does not seem reasonable to apply additional burdens when the feature is 
> so
> +rarely used. However, it is recommended to use a DMA-safe buffer if your
> +message size is likely applicable for DMA. Most drivers have this threshold
> +around 8 bytes (as of today, this is mostly an educated guess, however). For
> +any message of 16 byte or larger, it is probably a really good idea.
> +
> +If you use such a buffer in a i2c_msg, set the I2C_M_DMA_SAFE flag with it.
> +Then, the I2C core and drivers know they can safely operate DMA on it. Note
> +that setting this flag makes only sense in kernel space. User space data is
> +copied into kernel space anyhow. The I2C core makes sure the destination
> +buffers in kernel space are always DMA capable.
> +
> +FIXME: Need to implement i2c_master_{send|receive}_dma and proper buffers 
> for i2c_smbus_xfer_emulated.
> +
> +Drivers wishing to implement DMA can use helper functions from the I2C core.
> +One gives you a DMA-safe buffer for a given i2c_msg as long as a certain
> +threshold is met.
> +
> + dma_buf = i2c_get_dma_safe_msg_buf(msg, threshold_in_byte);

I'm concerned about the new bits added by this call. Right now,
USB drivers just use kalloc() for transfer buffers used to send and
receive URB control messages for both setting the main device and
for I2C messages. Before this changeset, buffers allocated this
way are DMA save and have been working for years.

When you add some flags that would make the I2C subsystem aware
that a buffer is now DMA safe, I guess you could be breaking
those drivers, as a DMA safe buffer will now be considered as
DMA-unsafe.

So, you'll likely need to patch all media USB drivers to fix it,
or come up with some other solution.

> +
> +If a buffer is returned, it either msg->buf for the I2C_M_DMA_SAFE case or a
> +bounce buffer. But you don't need to care about that detail. If NULL is
> +returned, the threshold was not met or a bounce buffer could not be 
> allocated.
> +Fall back to PIO in that case.
> +
> +In any case, a buffer obtained from above needs to be released. It ensures 
> data
> +is copied back to the message and a potentially used bounce buffer is freed.
> +
> + i2c_release_dma_safe_msg_buf(msg, dma_buf);
> +
> +The bounce buffer handling from the core is generic and simple. It will 
> always
> +allocate a new bounce buffer. If you want a more sophisticated handling (e.g.
> +reusing pre-allocated buffers), you are free to implement your own.
> +
> +Please also check the in-kernel documentation for details. The i2c-sh_mobile
> +driver can be used as a reference example how to use the above helpers.
> +
> +Final