linux-kernel - Re: [PATCH v3 01/22] drm: Include ddc adapter pointer in struct drm

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>] [day] [month] [year] [list]

Message-ID: <ef458f71-d061-4a8f-c53c-aa325b577d42@samsung.com>
Date:   Fri, 5 Jul 2019 11:44:52 +0200
From:   Andrzej Hajda <a.hajda@...sung.com>
To:     Thomas Zimmermann <tzimmermann@...e.de>,
        Andrzej Pietrasiewicz <andrzej.p@...labora.com>,
        dri-devel@...ts.freedesktop.org
Cc:     kernel@...labora.com, Alex Deucher <alexander.deucher@....com>,
        Christian König <christian.koenig@....com>,
        "David (ChunMing) Zhou" <David1.Zhou@....com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Dave Airlie <airlied@...hat.com>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        Sean Paul <sean@...rly.run>, Inki Dae <inki.dae@...sung.com>,
        Seung-Woo Kim <sw0312.kim@...sung.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        amd-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org,
        linux-mediatek@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
        freedreno@...ts.freedesktop.org,
        linux-rockchip@...ts.infradead.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH v3 01/22] drm: Include ddc adapter pointer in struct
 drm_connector

On 01.07.2019 16:41, Thomas Zimmermann wrote:
> Hi
>
> Am 01.07.19 um 15:27 schrieb Andrzej Pietrasiewicz:
>> Hi Thomas,
>>
>> Thank you for your comments. Please see inline.
>>
>> W dniu 30.06.2019 o 10:12, Thomas Zimmermann pisze:
>>> Hi
>>>
>>> I like the idea, but would prefer a more structured approach.
>>>
>>> Setting connector->ddc before calling drm_sysfs_connector_add() seems
>>> error prone. The dependency is not really clear from the code or
>>> interfaces.
>>>
>>> The other thing is that drivers I mostly work on, ast and mgag200, have
>>> code like this:
>>>
>>>    struct ast_i2c_chan {
>>>     struct i2c_adapter adapter;
>>>     struct drm_device *dev;
>>>     struct i2c_algo_bit_data bit;
>>>    };
>>>
>>>    struct ast_connector {
>>>     struct drm_connector base;
>>>     struct ast_i2c_chan *i2c;
>>>    };
>>>
>>> It already encodes the connection between connector and ddc adapter.
>>>
>>> I suggest to introduce struct drm_i2c_adapter
>>>
>>>    struct drm_i2c_adapter {
>>>     struct i2c_adapter base;
>>>     struct drm_connector *connector;
>>>    };
>>>
>>> and convert drivers over to it. Ast would look like this:
>>>
>>>    struct ast_i2c_chan {
>>>     struct drm_i2c_adapter adapter;
>>>     struct i2c_algo_bit_data bit;
>>>    };
>>>
>> There are few flavors of these drivers. In some of them
>> the i2c_adapter for ddc is allocated and initialized by
>> the driver, which must provide a place in memory to hold
>> the adapter. ast is an example of this approach.
>>
>> But there are others, such as for example exynos_hdmi.c.
>> It gets its ddc adapter with of_find_i2c_adapter_by_node()
>> and merely stores a pointer to it, while not managing the
>> memory needed to hold the i2c_adapter.
> I see.
>
>> Do you have any idea how to accommodate these various
>> flavors of drivers in the scheme you propose?
> Something like
>
>   struct drm_i2c_adapter {
> 	struct i2c_adapter *adapter;
> 	struct drm_connector *connector;
>   };
>
> with adapter either being allocated dynamically or acquired via
> of_find_i2c_adapter_by_node(); with separate init and finish functions
> for each variant. But it looks like over-abstraction to me. Without
> prototyping the idea, I cannot say if it's worth the effort.
>
> For something more simple, maybe just have a function to attach an i2c
> adapter to a connector:
>
>   drm_connector_attach_i2c_adapter(connector, i2c_adapter)
>
> which sets the connector's ddc pointer and creates the sysfs entry if
> the connector's entry is already present.


I am not sure if such function is really necessary. Assigning ddc field
before connector registration seems to be much simpler and quite
standard - many fields are initialized this way.


Reviewed-by: Andrzej Hajda <a.hajda@...sung.com>

 --
Regards
Andrzej



> Best regards
> Thomas
>
>> Andrzej
>>
>>