linux-kernel - Re: [PATCH v3 2/2] drm/bridge: tc358767: Expose test mode functionality via debugfs

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

Message-ID: <CAHQ1cqE-7qof2ogCv-00uK9KBk+Uy2xFQ8RsxKFm_ts1EH75fw@mail.gmail.com>
Date:   Mon, 9 Dec 2019 07:24:21 -0800
From:   Andrey Smirnov <andrew.smirnov@...il.com>
To:     Tomi Valkeinen <tomi.valkeinen@...com>
Cc:     dri-devel@...ts.freedesktop.org,
        Andrzej Hajda <a.hajda@...sung.com>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Cory Tusar <cory.tusar@....aero>,
        Chris Healy <cphealy@...il.com>,
        Lucas Stach <l.stach@...gutronix.de>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH v3 2/2] drm/bridge: tc358767: Expose test mode
 functionality via debugfs

On Mon, Dec 9, 2019 at 7:05 AM Tomi Valkeinen <tomi.valkeinen@...com> wrote:
>
> On 09/12/2019 16:38, Andrey Smirnov wrote:
> > On Mon, Dec 9, 2019 at 1:38 AM Tomi Valkeinen <tomi.valkeinen@...com> wrote:
> >>
> >> (Cc'ing Daniel for the last paragraph)
> >>
> >> On 09/12/2019 07:08, Andrey Smirnov wrote:
> >>> Presently, the driver code artificially limits test pattern mode to a
> >>> single pattern with fixed color selection. It being a kernel module
> >>> parameter makes switching "test pattern" <-> "proper output" modes
> >>> on-the-fly clunky and outright impossible if the driver is built into
> >>> the kernel.
> >>
> >> That's not correct, /sys/module/tc358767/parameters/test is there even if the driver is built-in.
> >>
> >
> > True, I'll drop the "impossible" part of the descrption. Having to
> > unbind and bind device to the driver to use that parameter definitely
> > falls under "clunky" for me still, so I'll just stick to that in the
> > description.
>
> You don't need to re-bind. You can change the module parameter at runtime, and if the driver happens
> to use the value, then it uses the new value. If I recall right, changing the module parameter and
> then doing a full modeset from userspace made the driver to use the test mode (I'm not 100% sure,
> though).
>
> In any case, I'm not advocating for the use of module parameter here =)
>
> >> Hmm, actually, just echoing 0 to tstctl multiple times, it makes the screen go black and then
> >> restores it with every other echo.
> >>
> >
> > Strange, works on my setup every time. No error messages in kernel log
> > I assume? Probably unrelated, but when you echo "0" and the screen
>
> No errors.
>
> > stays black, what do you see in DP_SINK_STATUS register:
> >
> > dd if=/dev/drm_dp_aux0 bs=1 skip=$((0x205)) count=1 2>/dev/null | hexdump -Cv
> >
> > ? Note that this needs CONFIG_DRM_DP_AUX_CHARDEV to be enabled.
>
> I'll check this later, and do a few more tests.
>
> >>> +     debugfs = debugfs_create_dir(dev_name(dev), NULL);
> >>> +     if (!IS_ERR(debugfs)) {
> >>> +             debugfs_create_file_unsafe("tstctl", 0200, debugfs, tc,
> >>> +                                        &tc_tstctl_fops);
> >>> +             devm_add_action_or_reset(dev, tc_remove_debugfs, debugfs);
> >>> +     }
> >>> +
> >>
> >> For me this creates debugfs/3-000f/tstctl. I don't think that's a clear or usable path, and could
> >> even cause a name conflict in the worst case.
> >>
> >
> > I agree on usability aspect, but I am not sure I can see how a
> > conflict can happen. What scenario do you have in mind that would
> > cause that? My thinking was that the combination of I2C bus number +
> > I2C address should always be unique on the system, but maybe I am
> > missing something?
>
> Well, the dir name doesn't have "i2c" anywhere, so at least in theory, some other bus could have
> "3-000f" address too.
>
> Maybe bigger problem is that it's not at all obvious what "3-000f" means. All the other debugfs dirs
> make sense when you look at the name, and "3-000f" looks very odd there.
>

Fair enough, so what if we changed the name say "tc358767-3-000f" (i.
e. used "tc358767-" + dev_name(dev)), would that be a reasonable path
forward?

Thanks,
Andrey Smirnov