linux-kernel - Re: [PATCH v8 3/4] drm/atmel-hlcdc: iterate over all output endpoints

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 24 Aug 2018 09:47:01 +0200
From:   Boris Brezillon <boris.brezillon@...tlin.com>
To:     Peter Rosin <peda@...ntia.se>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org, jmondi <jacopo@...ndi.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Andrzej Hajda <a.hajda@...sung.com>,
        David Airlie <airlied@...ux.ie>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Sakari Ailus <sakari.ailus@....fi>,
        Jacopo Mondi <jacopo+renesas@...ndi.org>,
        Jyri Sarha <jsarha@...com>, Daniel Vetter <daniel@...ll.ch>,
        Russell King <linux@...linux.org.uk>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v8 3/4] drm/atmel-hlcdc: iterate over all output
 endpoints

On Tue, 14 Aug 2018 17:05:29 +0200
Peter Rosin <peda@...ntia.se> wrote:

> >>>>>> @@ -77,13 +79,29 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device *dev, int endpoint)
> >>>>>>
> >>>>>>  int atmel_hlcdc_create_outputs(struct drm_device *dev)
> >>>>>>  {
> >>>>>> -    int endpoint, ret = 0;
> >>>>>> -
> >>>>>> -    for (endpoint = 0; !ret; endpoint++)
> >>>>>> -            ret = atmel_hlcdc_attach_endpoint(dev, endpoint);
> >>>>>> +    struct of_endpoint endpoint;
> >>>>>> +    struct device_node *node = NULL;
> >>>>>> +    int count = 0;
> >>>>>> +    int ret = 0;
> >>>>>> +
> >>>>>> +    for_each_endpoint_of_node(dev->dev->of_node, node) {
> >>>>>> +            of_graph_parse_endpoint(node, &endpoint);  
> >>>
> >>> I'd really like to kill off of_graph_parse_endpoint, not add more
> >>> users (check the git history on this code). You should know what are
> >>> possible port and endpoint numbers, so iterate over those.  
> >>
> >> So, add a comment to that effect in the docs of the of_graph_parse_endpoint
> >> function.
> >>
> >> How can the hlcdc driver know the actual number of endpoints? It's a
> >> one-way signal path out from that port, and it can easily be routed to
> >> 1, 2, 3 or even more places. As shown above, forcing the endpoint id
> >> to start at zero is a nuisance, and I don't see the point of it.
> >>
> >> But I welcome suggestions on how to arrange the above dtsi/dts fragments
> >> in a world where the endpoint id absolutely has to start at zero.  
> > 
> > Your dts file arrangement seems fine. Can't you just not exit the loop
> > on -ENODEV? IOW, just iterate til you find an enabled endpoint.  
> 
> That would regress cases where two (or more) endpoints are enabled
> (which is currently supported). As I see it, the driver will have a
> very hard time knowing when to stop iterating with any solution not
> involving the equivalent of the functions for_each_endpoint_of_node
> and of_graph_parse_endpoint. Open-coding of_graph_parse_endpoint just
> to avoid it is a bit more than silly IMHO...

I agree, and actually, I think this is Rob who suggested to do what we
do here :-) (iterate from 0 to X, and stop as soon as
-ENODEV is returned).