| 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
| ||
|
Message-ID: <8fa353a5fd6e37f570f3a9d4812158a2@codeaurora.org>
Date: Wed, 08 Dec 2021 15:54:18 -0800
From: abhinavk@...eaurora.org
To: Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: Rob Clark <robdclark@...il.com>,
Stephen Boyd <swboyd@...omium.org>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Sean Paul <sean@...rly.run>, David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>, linux-arm-msm@...r.kernel.org,
dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
linux-kernel@...r.kernel.org
Subject: Re: [Freedreno] [PATCH] drm/msm/dp: Move debugfs files into
subdirectory
On 2021-10-18 11:36, Bjorn Andersson wrote:
> On Mon 18 Oct 11:07 PDT 2021, abhinavk@...eaurora.org wrote:
>
>> Hi Bjorn
>>
>> On 2021-10-17 09:42, Bjorn Andersson wrote:
>> > On Fri 15 Oct 16:53 PDT 2021, abhinavk@...eaurora.org wrote:
>> >
>> > > On 2021-10-15 16:17, Bjorn Andersson wrote:
>> > > > In the cleanup path of the MSM DP driver the DP driver's debugfs files
>> > > > are destroyed by invoking debugfs_remove_recursive() on debug->root,
>> > > > which during initialization has been set to minor->debugfs_root.
>> > > >
>> > > > To allow cleaning up the DP driver's debugfs files either each dentry
>> > > > needs to be kept track of or the files needs to be put in a subdirectory
>> > > > which can be removed in one go.
>> > > >
>> > > > By choosing to put the debugfs files in a subdirectory, based on the
>> > > > name of the associated connector this also solves the problem that these
>> > > > names would collide as support for multiple DP instances are introduced.
>> > > >
>> > > > One alternative solution to the problem with colliding file names would
>> > > > have been to put keep track of the individual files and put them under
>> > > > the connector's debugfs directory. But while the drm_connector has been
>> > > > allocated, its associated debugfs directory has not been created at the
>> > > > time of initialization of the dp_debug.
>> > > >
>> > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
>> > >
>> > > I have been thinking about this problem ever since multi-DP has been
>> > > posted
>> > > :)
>> > > Creating sub-directories seems right but at the moment it looks like
>> > > IGT
>> > > which
>> > > uses these debugfs nodes doesnt check sub-directories:
>> > >
>> > > https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tools/msm_dp_compliance.c#L215
>> > >
>> > > It looks for the DP debugfs nodes under /sys/kernel/debug/dri/*/
>> > >
>> > > We have to fix IGT too to be able to handle multi-DP cases. I will
>> > > try to
>> > > come up
>> > > with a proposal to address this.
>> > >
>> > > Till then, can we go with the other solution to keep track of the
>> > > dentries?
>> > >
>> >
>> > I'm afraid I don't see what you're proposing.
>> >
>> > Afaict we need one set of dp_test{type,active,data} per DP controller,
>> > so even doing this by keeping track of the dentries requires that we
>> > rename the files based on some identifier (id or connector name) - which
>> > will cause igt to break.
>>
>> Yes, I also thought the same that there needs to be some identifier.
>>
>> "To allow cleaning up the DP driver's debugfs files either each dentry
>> needs to be kept track of or the files needs to be put in a
>> subdirectory
>> which can be removed in one go"
>>
>> I guess I misunderstood your statement in the commit text thinking
>> that you
>> had some other way to keep track of the dentries as it mentioned that
>> use a subdirectory OR keep track of each dentry.
>>
>
> No, I did write that code as well and then ditched it.
>
> Unfortunately I don't think it would help you, because we still need to
> add some identifier to the file names and preferably we should add that
> to the single case as well to make things consistent.
>
>> >
>> > As such, I think the practical path forward is that we merge the
>> > multi-DP series as currently proposed. This will not cause any issues on
>> > single-DP systems, but on multi-DP systems we will have warnings about
>> > duplicate debugfs entries in the kernel logs.
>> >
>> > Then you can figure out how to rework igt to deal with the multiple DP
>> > instances and update the dp_debug interface accordingly.
>> >
>>
>> Fine with me, I will take care of this.
>>
>
> Cool, thanks.
>
> Regards,
> Bjorn
>
Following up on this, Rob has posted the igt change today which i acked.
https://patchwork.freedesktop.org/patch/465930/
With this in place, we can actually go ahead with this change.
Hence,
Reviewed-by: Abhinav Kumar <quic_abhinavk@...cinc.com>
>> >
>> > Which also implies that we should hold this patch back. But if we go
>> > that path, I think we should fix dp_debug_deinit() so that it doesn't
>> > remove /sys/kernel/debug/dri/128 when the DP driver is unloaded.
>> Yes, lets hold this patch back till I fix multi-DP for IGT.
>> >
>> > Regards,
>> > Bjorn
>> >
>> > > > ---
>> > > >
>> > > > This depends on
>> > > > https://lore.kernel.org/linux-arm-msm/20211010030435.4000642-1-bjorn.andersson@linaro.org/
>> > > > reducing the connector from a double pointer.
>> > > >
>> > > > drivers/gpu/drm/msm/dp/dp_debug.c | 15 +++++++++------
>> > > > 1 file changed, 9 insertions(+), 6 deletions(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c
>> > > > b/drivers/gpu/drm/msm/dp/dp_debug.c
>> > > > index da4323556ef3..67da4c69eca1 100644
>> > > > --- a/drivers/gpu/drm/msm/dp/dp_debug.c
>> > > > +++ b/drivers/gpu/drm/msm/dp/dp_debug.c
>> > > > @@ -210,26 +210,29 @@ static const struct file_operations
>> > > > test_active_fops = {
>> > > > static int dp_debug_init(struct dp_debug *dp_debug, struct drm_minor
>> > > > *minor)
>> > > > {
>> > > > int rc = 0;
>> > > > + char path[64];
>> > > > struct dp_debug_private *debug = container_of(dp_debug,
>> > > > struct dp_debug_private, dp_debug);
>> > > >
>> > > > - debugfs_create_file("dp_debug", 0444, minor->debugfs_root,
>> > > > + snprintf(path, sizeof(path), "msm_dp-%s", debug->connector->name);
>> > > > +
>> > > > + debug->root = debugfs_create_dir(path, minor->debugfs_root);
>> > > > +
>> > > > + debugfs_create_file("dp_debug", 0444, debug->root,
>> > > > debug, &dp_debug_fops);
>> > > >
>> > > > debugfs_create_file("msm_dp_test_active", 0444,
>> > > > - minor->debugfs_root,
>> > > > + debug->root,
>> > > > debug, &test_active_fops);
>> > > >
>> > > > debugfs_create_file("msm_dp_test_data", 0444,
>> > > > - minor->debugfs_root,
>> > > > + debug->root,
>> > > > debug, &dp_test_data_fops);
>> > > >
>> > > > debugfs_create_file("msm_dp_test_type", 0444,
>> > > > - minor->debugfs_root,
>> > > > + debug->root,
>> > > > debug, &dp_test_type_fops);
>> > > >
>> > > > - debug->root = minor->debugfs_root;
>> > > > -
>> > > > return rc;
>> > > > }
Powered by blists - more mailing lists