linux-kernel - Re: [PATCH net-next V4] net: lan743x: Add support to SGMII register dump for PCI11010/PCI11414 chips

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: <20221019164344.52cf16dd@kernel.org>
Date:   Wed, 19 Oct 2022 16:43:44 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
Cc:     <netdev@...r.kernel.org>, <davem@...emloft.net>,
        <linux-kernel@...r.kernel.org>, <Bryan.Whitehead@...rochip.com>,
        <edumazet@...gle.com>, <pabeni@...hat.com>,
        <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net-next V4] net: lan743x: Add support to SGMII register
 dump for PCI11010/PCI11414 chips

On Tue, 18 Oct 2022 11:44:25 +0530 Raju Lakkaraju wrote:
> Add support to SGMII register dump

> +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> @@ -24,6 +24,9 @@
>  #define LOCK_TIMEOUT_MAX_CNT		    (100) // 1 sec (10 msce * 100)
>  
>  #define LAN743X_CSR_READ_OP(offset)	     lan743x_csr_read(adapter, offset)
> +#define VSPEC1			MDIO_MMD_VEND1
> +#define VSPEC2			MDIO_MMD_VEND2
> +#define SGMII_RD(adp, dev, adr) lan743x_sgmii_dump_read(adp, dev, adr)

These defines help limit the line length?
Please don't obfuscate code like that, see below.

> +static void lan743x_sgmii_regs(struct net_device *dev, void *p)
> +{
> +	struct lan743x_adapter *adp = netdev_priv(dev);
> +	u32 *rb = p;
> +
> +	rb[ETH_SR_VSMMD_DEV_ID1]                = SGMII_RD(adp, VSPEC1, 0x0002);
> +	rb[ETH_SR_VSMMD_DEV_ID2]                = SGMII_RD(adp, VSPEC1, 0x0003);
> +	rb[ETH_SR_VSMMD_PCS_ID1]                = SGMII_RD(adp, VSPEC1, 0x0004);
> +	rb[ETH_SR_VSMMD_PCS_ID2]                = SGMII_RD(adp, VSPEC1, 0x0005);
> +	rb[ETH_SR_VSMMD_STS]                    = SGMII_RD(adp, VSPEC1, 0x0008);
> +	rb[ETH_SR_VSMMD_CTRL]                   = SGMII_RD(adp, VSPEC1, 0x0009);
> +	rb[ETH_SR_MII_CTRL]                     = SGMII_RD(adp, VSPEC2, 0x0000);
> +	rb[ETH_SR_MII_STS]                      = SGMII_RD(adp, VSPEC2, 0x0001);
> +	rb[ETH_SR_MII_DEV_ID1]                  = SGMII_RD(adp, VSPEC2, 0x0002);
> +	rb[ETH_SR_MII_DEV_ID2]                  = SGMII_RD(adp, VSPEC2, 0x0003);
> +	rb[ETH_SR_MII_AN_ADV]                   = SGMII_RD(adp, VSPEC2, 0x0004);
> +	rb[ETH_SR_MII_LP_BABL]                  = SGMII_RD(adp, VSPEC2, 0x0005);
> +	rb[ETH_SR_MII_EXPN]                     = SGMII_RD(adp, VSPEC2, 0x0006);
> +	rb[ETH_SR_MII_EXT_STS]                  = SGMII_RD(adp, VSPEC2, 0x000F);
> +	rb[ETH_SR_MII_TIME_SYNC_ABL]            = SGMII_RD(adp, VSPEC2, 0x0708);
> +	rb[ETH_SR_MII_TIME_SYNC_TX_MAX_DLY_LWR] = SGMII_RD(adp, VSPEC2, 0x0709);

You can declare a structure holding the params and save the info there:

	struct {
		u8 id;
		u8 dev;
		u16 addr;
	} regs[] = {
		{ ETH_SR_MII_TIME_SYNC_TX_MAX_DLY_LWR,	MDIO_MMD_VEND2,	0x0709 },
	};

that should fit on the line.

You can then read the values in a loop. And inside that loop you can
handle errors (perhaps avoiding the need for lan743x_sgmii_dump_read()
which seems rather unnecessary as lan743x_sgmii_read() already prints 
errors).

FWIW I like Andrew's suggestion from v3 to use version as a bitfield, too.