linux-kernel - Re: [PATCH 3/4] cdx: create sysfs resource files

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: <2023071303-encore-emporium-b847@gregkh>
Date:   Thu, 13 Jul 2023 07:57:17 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     "Gangurde, Abhijit" <abhijit.gangurde@....com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Gupta, Nipun" <Nipun.Gupta@....com>,
        "Agarwal, Nikhil" <nikhil.agarwal@....com>,
        "Gupta, Puneet (DCG-ENG)" <puneet.gupta@....com>,
        "git (AMD-Xilinx)" <git@....com>,
        "Simek, Michal" <michal.simek@....com>,
        "Jansen Van Vuuren, Pieter" <pieter.jansen-van-vuuren@....com>
Subject: Re: [PATCH 3/4] cdx: create sysfs resource files

On Thu, Jul 13, 2023 at 05:36:25AM +0000, Gangurde, Abhijit wrote:
> [AMD Official Use Only - General]
> 
> > > > > Resource files provides the basic MMIO regions info to the
> > > > > user-space. Also, resources<x> devices can be used to mmap the
> > > > > MMIO regions in the user-space.
> > > > >
> > > > > Co-developed-by: Puneet Gupta <puneet.gupta@....com>
> > > > > Signed-off-by: Puneet Gupta <puneet.gupta@....com>
> > > > > Co-developed-by: Nipun Gupta <nipun.gupta@....com>
> > > > > Signed-off-by: Nipun Gupta <nipun.gupta@....com>
> > > > > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@....com>
> > > > > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-
> > > > vuuren@....com>
> > > > > Tested-by: Nikhil Agarwal <nikhil.agarwal@....com>
> > > > > ---
> > > > >  Documentation/ABI/testing/sysfs-bus-cdx |  15 +++
> > > > >  drivers/cdx/cdx.c                       | 139 +++++++++++++++++++++++-
> > > > >  include/linux/cdx/cdx_bus.h             |  10 ++
> > > > >  3 files changed, 163 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-cdx
> > > > b/Documentation/ABI/testing/sysfs-bus-cdx
> > > > > index d9e00058471d..6ca47b6442ce 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-bus-cdx
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-cdx
> > > > > @@ -76,3 +76,18 @@ Description:
> > > > >             For example::
> > > > >
> > > > >               # echo 1 > /sys/bus/cdx/devices/.../remove
> > > > > +
> > > > > +What:              /sys/bus/cdx/devices/.../resource
> > > > > +Date:              July 2023
> > > > > +Contact:   puneet.gupta@....com
> > > > > +Description:
> > > > > +           The resource file contains host addresses of CDX device
> > > > > +           resources. Each line of the resource file describes a region
> > > > > +           with start, end, and flag fields.
> > > >
> > > > If you documented what this file looked like here, it would be obvious
> > > > that this is not an acceptable sysfs file in any sense of the word.
> > > >
> > > > Please do so, and then fix the patch to not do that at all.
> > >
> > > Similar interface exist for pci and we intended to keep it same way. Could you
> > please elaborate on this.
> > >
> > > # cat /sys/bus/pci/devices/0000\:01\:00.0/resource
> > > 0x0000000092100000 0x00000000921fffff 0x000000000014220c
> > > 0x0000000000000000 0x0000000000000000 0x0000000000000000
> >
> > Please don't propagate incorrect decisions in the past.
> >
> > Why do you need all of these "resources" in userspace?  What tool is
> > going to read and parse them and do something with them?
> >
> > This really violates the "one value per file" sysfs rule, you are going
> > to have to have a huge reason why this is not applicable here, AND you
> > are going to have to document it very very well and get everyone to
> > agree with it.
> 
> We don't have any strong reason apart from that this is being used by some
> test and debug applications. Will drop this one now and revisit later by
> complying to the specifications.

If it's only for debug stuff, then use debugfs, that's what it is there
for.

thanks,

greg k-h