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]

Message-ID: <CALFZZQEoiikyXmHJyEgEJTRNryTN8ScNK4P3bDCp-9v9GtvU8g@mail.gmail.com>
Date:   Thu, 9 May 2019 09:52:50 +0200
From:   Dariusz Marcinkiewicz <darekm@...gle.com>
To:     Hans Verkuil <hverkuil@...all.nl>
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

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.
...
> >       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.
...
> > +/**
> > + * 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!