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: <1036fcf7661d0b6544c416fd8c9423836b9eb1e9.camel@mediatek.com>
Date:   Wed, 18 Jan 2023 02:02:07 +0000
From:   Chunfeng Yun (云春峰) 
        <Chunfeng.Yun@...iatek.com>
To:     "vkoul@...nel.org" <vkoul@...nel.org>
CC:     "llvm@...ts.linux.dev" <llvm@...ts.linux.dev>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "nathan@...nel.org" <nathan@...nel.org>,
        "kishon@...nel.org" <kishon@...nel.org>,
        Eddie Hung (洪正鑫) 
        <Eddie.Hung@...iatek.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>,
        "trix@...hat.com" <trix@...hat.com>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>,
        "ndesaulniers@...gle.com" <ndesaulniers@...gle.com>
Subject: Re: [PATCH v6 3/3] phy: mediatek: tphy: add debugfs files

On Fri, 2023-01-13 at 23:45 +0530, Vinod Koul wrote:
> On 04-01-23, 21:26, Chunfeng Yun wrote:
> > These debugfs files are mainly used to make eye diagram test
> > easier,
> > especially helpful to do HQA test for a new IC without efuse
> > enabled.
> > 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@...iatek.com>
> > ---
> > v6: no changes
> > 
> > v5: using common debugfs config CONFIG_DEBUG_FS
> > 
> > v4: fix build warning of sometimes uninitialized variable
> > 
> > v3: fix typo of "debugfs" suggested by AngeloGioacchino
> > 
> > v2: add CONFIG_PHY_MTK_TPHY_DEBUGFS suggested by AngeloGioacchino
> > ---
> >  drivers/phy/mediatek/phy-mtk-tphy.c | 405
> > +++++++++++++++++++++++++++-
> >  1 file changed, 404 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/phy/mediatek/phy-mtk-tphy.c
> > b/drivers/phy/mediatek/phy-mtk-tphy.c
> > index e906a82791bd..923e5ee119f3 100644
> > --- a/drivers/phy/mediatek/phy-mtk-tphy.c
> > +++ b/drivers/phy/mediatek/phy-mtk-tphy.c
> > @@ -7,6 +7,7 @@
> >  
> >  #include <dt-bindings/phy/phy.h>
> >  #include <linux/clk.h>
> > +#include <linux/debugfs.h>
> >  #include <linux/delay.h>
> >  #include <linux/iopoll.h>
> >  #include <linux/mfd/syscon.h>
> > @@ -264,6 +265,8 @@
> >  <skip>

> > +
> > +static void tphy_debugfs_init(struct mtk_tphy *tphy, struct
> > mtk_phy_instance *inst)
> > +{
> > +	char name[16];
> > +
> > +	snprintf(name, sizeof(name) - 1, "phy.%d", inst->index);
> 
> I wouold suggest driver name/ device name rather than phy.foo...
> again
> folks needs to see what is foo
> 
I compare the way you suggested again on mt2712 platform that have two
phy controller, t-phy@...90000 [1] and t-phy@...e0000 [2],
[1]: 
https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/mediatek/mt2712e.dtsi#L827
[2]:

https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/mediatek/mt2712e.dtsi#L891


Each phy controller driver will created a root folder according its
device name under "/sys/kernel/debug/phy", like as:
#ls /sys/kernel/debug/phy
t-phy@...90000  t-phy@...e0000

The phy controller has some subphy node, each one indicate a phy
struct, if use the phy device name to create a folder, it shows as
below:
#cd /sys/kernel/debug/phy
# ls *
t-phy@...90000:
phy-t-phy@...90000.0  phy-t-phy@...90000.1

t-phy@...e0000:
phy-t-phy@...e0000.3  phy-t-phy@...e0000.4  phy-t-phy@...e0000.5

phy's device name is created as:
"dev_set_name(&phy->dev, "phy-%s.%d", dev_name(dev), id)"

dev_name(dev) is the parent name, it's t-phy@...90000 and 
t-phy@...e0000 in example, id is the phy->id.

seems no need to provide parent name again.
another problem is that the id may vary if other phys are enabled, that
means the path may vary, it's difficult to write scripts to do test.

when use the current way,
"snprintf(name, sizeof(name) - 1, "phy.%d", inst->index)"
it shows as:
# ls *
t-phy@...90000:
phy.0  phy.1

t-phy@...e0000:
phy.0  phy.1  phy.2

@inst->index is the sequence number of subphy node in each phy
controller's node, when the dts is fixed, the @index is also fixed,
then for each phy, the path is also fixed, seems more convenient to
write scripts.

So I prefer to leave the code unchanged.

Thanks a lot

> 
> > +	inst->dbgfs = debugfs_create_dir(name, tphy->dbgfs_root);
> > +
> > +	debugfs_create_file("type", 0444, inst->dbgfs, inst,
> > &tphy_type_fops);
> > +
> > +	switch (inst->type) {
> > +	case PHY_TYPE_USB2:
> > +		u2_phy_dbgfs_files_create(inst);
> > +		break;
> > +	case PHY_TYPE_USB3:
> > +	case PHY_TYPE_PCIE:
> > +		u3_phy_dbgfs_files_create(inst);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> > +static void tphy_debugfs_exit(struct mtk_phy_instance *inst)
> > +{
> > +	debugfs_remove_recursive(inst->dbgfs);
> > +	inst->dbgfs = NULL;
> > +}
> > +
> > +static void tphy_debugfs_root_create(struct mtk_tphy *tphy)
> > +{
> > +	tphy->dbgfs_root = debugfs_create_dir(dev_name(tphy->dev),
> > phy_debug_root);
> > +}
> > +
> > +static void tphy_debugfs_root_remove(struct mtk_tphy *tphy)
> > +{
> > +	debugfs_remove_recursive(tphy->dbgfs_root);
> > +	tphy->dbgfs_root = NULL;
> > +}
> > +
> > +#else
> > +<skip>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ