linux-kernel - Re: [RFC PATCH v2] media: cec: expose HDMI connector to CEC dev mapping

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:   Thu, 9 May 2019 11:31:01 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Dariusz Marcinkiewicz <darekm@...gle.com>
Cc:     linux-media@...r.kernel.org, hans.verkuil@...co.com,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2] media: cec: expose HDMI connector to CEC dev
 mapping

On 5/9/19 9:52 AM, Dariusz Marcinkiewicz wrote:
> Hi Hans.
> 
> On Wed, Apr 24, 2019 at 2:09 PM Hans Verkuil <hverkuil@...all.nl> wrote:
>>
>> Hi Dariusz,
>>
>> This is getting close, so I think for the next version you can drop
>> the RFC tag.
>>
>> Some comments:
>>
> ...
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
>>> @@ -261,7 +261,7 @@ static int dw_hdmi_cec_probe(struct platform_device *pdev)
>>>       cec->adap = cec_allocate_adapter(&dw_hdmi_cec_ops, cec, "dw_hdmi",
>>>                                        CEC_CAP_LOG_ADDRS | CEC_CAP_TRANSMIT |
>>>                                        CEC_CAP_RC | CEC_CAP_PASSTHROUGH,
>>> -                                      CEC_MAX_LOG_ADDRS);
>>> +                                      CEC_MAX_LOG_ADDRS, NULL);
>>
>> Hmm, the connector information is actually available through cec->hdmi.
>>
>> I think it would make sense to create a helper function that fills in
>> struct cec_connector_info based on a struct drm_connector pointer.
>> And add a function to drivers/gpu/drm/bridge/synopsys/dw-hdmi.c that
>> dw-hdmi-cec.c can call that does the same.
> 
> Looking at the code here, is the connector info guaranteed to be
> available at the time cec_allocate_adapter is called here?
> drm_connector won't be initialized until dw_hdmi_bridge_attach is
> called, which happens after the cec platform device is created.

Good point. The creation of the cec platform device should probably
be moved to dw_hdmi_bridge_attach.

> ...
>>>       priv->adap = cec_allocate_adapter(&tda9950_cec_ops, priv, "tda9950",
>>>                                         CEC_CAP_DEFAULTS,
>>> -                                       CEC_MAX_LOG_ADDRS);
>>> +                                       CEC_MAX_LOG_ADDRS,
>>> +                                       NULL);
>>
>> Here too the drm_connector can be found via struct tda9950_glue.
>> So it is easy to provide proper connector information.
> 
> The same concern as with the comment before.

Same solution: this has to be moved.

I have hardware to test patches for both drivers. It might take 2-3 weeks
before I can test as I don't always has access to the hardware, but at
least I can verify that moving this code won't break anything.

It's best to first move the code in separate patches before applying the
"expose HDMI connector to CEC dev mapping" patch on top of them.

> ...
>>> +/**
>>> + * struct cec_event_connector - tells if and which connector is associated
>>> + * with the CEC adapter.
>>> + * @card_no: drm card number, -1 if no connector
>>
>> If there is no connector, then type is NO_CONNECTOR. So this
>> doesn't make sense. Wouldn't it be better to just use '__u32 card_no'?
>>
> Yes, removed (leftover from previous revision where there was no
> connector type field).
> This and remaining comments are (hopefully) resolved in the new
> version of the patch, I've just sent.
> 
> Will add more docs in subsequent revs.
> 
> Thank you!
> 

Regards,

	Hans