linux-kernel - Re: [PATCH v3 1/5] drm/dp: add connector backpointer to drm_dp

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:   Mon, 9 Jan 2017 10:03:46 +0100
From:   Tomeu Vizoso <tomeu.vizoso@...labora.com>
To:     Sean Paul <seanpaul@...omium.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ville Syrjälä <ville.syrjala@...ux.intel.com>,
        Thierry Reding <thierry.reding@...il.com>,
        intel-gfx-trybot@...ts.freedesktop.org,
        Emil Velikov <emil.l.velikov@...il.com>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        David Airlie <airlied@...ux.ie>
Subject: Re: [PATCH v3 1/5] drm/dp: add connector backpointer to drm_dp_aux

On 6 January 2017 at 16:56, Sean Paul <seanpaul@...omium.org> wrote:
> On Fri, Jan 6, 2017 at 9:30 AM, Tomeu Vizoso <tomeu.vizoso@...labora.com> wrote:
>> This backpointer allows DP helpers to access the connector it's being
>> used for.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@...labora.com>
>> ---
>>
>>  include/drm/drm_dp_helper.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index 55bbeb0ff594..4fa77b434594 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -721,6 +721,7 @@ struct drm_dp_aux_msg {
>>   * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
>>   * @ddc: I2C adapter that can be used for I2C-over-AUX communication
>>   * @dev: pointer to struct device that is the parent for this AUX channel
>> + * @connector: backpointer to connector that uses this AUX channel
>>   * @hw_mutex: internal mutex used for locking transfers
>>   * @transfer: transfers a message representing a single AUX transaction
>>   *
>> @@ -757,6 +758,7 @@ struct drm_dp_aux {
>>         const char *name;
>>         struct i2c_adapter ddc;
>>         struct device *dev;
>> +       struct drm_connector *connector;
>
> Perhaps I'm misreading the series, but it seems like you could instead
> add int crc_pipe along with the other crc fields. Then when you start
> the crc, set the pipe number. If you have the pipe in the crc worker,
> you can wait vblank on that pipe (no pointers needed).
>
> Reasonable?

I think that would work fine, but is it any better? I like that the
connector isn't going to change during the lifetime of the drm_dp_aux,
but crc_pipe could be changing between sampling sessions and any bugs
in keeping that field updated would be quite disconcerting and
cumbersome to debug.

crc_pipe's advantage is that we wouldn't need to update all callers of
drm_dp_aux_register, but I think it's better to have a connector field
that is mistakenly NULL, than a crc_pipe field with the wrong value.

Regards,

Tomeu

> Sean
>
>>         struct mutex hw_mutex;
>>         ssize_t (*transfer)(struct drm_dp_aux *aux,
>>                             struct drm_dp_aux_msg *msg);
>> --
>> 2.9.3
>>
>
>
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS