netdev - Re: [PATCH net-next 3/3] net: phy: mediatek: add driver for built-in 2.5G ethernet PHY on MT7988

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: <Z4hnv2lzy8Ntd_Hp@makrotopia.org>
Date: Thu, 16 Jan 2025 01:58:23 +0000
From: Daniel Golle <daniel@...rotopia.org>
To: Sky Huang <SkyLake.Huang@...iatek.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Qingfang Deng <dqfext@...il.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org,
	Steven Liu <Steven.Liu@...iatek.com>
Subject: Re: [PATCH net-next 3/3] net: phy: mediatek: add driver for built-in
 2.5G ethernet PHY on MT7988

Hi Sky,

On Thu, Jan 16, 2025 at 09:21:58AM +0800, Sky Huang wrote:
> From: Sky Huang <skylake.huang@...iatek.com>
> 
> Add support for internal 2.5Gphy on MT7988. This driver will load
> necessary firmware and add appropriate time delay to make sure
> that firmware works stably. Also, certain control registers will
> be set to fix link-up issues.
> 
> Signed-off-by: Sky Huang <skylake.huang@...iatek.com>
> ---
>  MAINTAINERS                          |   1 +
>  drivers/net/phy/mediatek/Kconfig     |  11 +
>  drivers/net/phy/mediatek/Makefile    |   1 +
>  drivers/net/phy/mediatek/mtk-2p5ge.c | 343 +++++++++++++++++++++++++++
>  4 files changed, 356 insertions(+)
>  create mode 100644 drivers/net/phy/mediatek/mtk-2p5ge.c
> 
> [...]
> +static int mt798x_2p5ge_phy_probe(struct phy_device *phydev)
> +{
> +	struct mtk_i2p5ge_phy_priv *priv;
> +
> +	priv = devm_kzalloc(&phydev->mdio.dev,
> +			    sizeof(struct mtk_i2p5ge_phy_priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	switch (phydev->drv->phy_id) {
> +	case MTK_2P5GPHY_ID_MT7988:
> +		/* The original hardware only sets MDIO_DEVS_PMAPMD */
> +		phydev->c45_ids.mmds_present |= MDIO_DEVS_PCS |
> +						MDIO_DEVS_AN |
> +						MDIO_DEVS_VEND1 |
> +						MDIO_DEVS_VEND2;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	priv->fw_loaded = false;
> +	phydev->priv = priv;
> +
> +	mtk_phy_leds_state_init(phydev);

Calling mtk_phy_leds_state_init() can't work without also defining
led_hw_control_get() for that driver.

This is what mtk_phy_leds_state_init() does:
        for (i = 0; i < 2; ++i)
                phydev->drv->led_hw_control_get(phydev, i, NULL);

The driver lacking led_hw_control_get() method (see below) will make
this a call to a NULL function pointer.

Imho it's fine to add the driver without support for the LEDs for now
and add LED support later on. But in that case you also shouldn't call
mtk_phy_leds_state_init().

> +
> +	return 0;
> +}
> +
> +static struct phy_driver mtk_2p5gephy_driver[] = {
> +	{
> +		PHY_ID_MATCH_MODEL(MTK_2P5GPHY_ID_MT7988),
> +		.name = "MediaTek MT7988 2.5GbE PHY",
> +		.probe = mt798x_2p5ge_phy_probe,
> +		.config_init = mt798x_2p5ge_phy_config_init,
> +		.config_aneg = mt798x_2p5ge_phy_config_aneg,
> +		.get_features = mt798x_2p5ge_phy_get_features,
> +		.read_status = mt798x_2p5ge_phy_read_status,
> +		.get_rate_matching = mt798x_2p5ge_phy_get_rate_matching,
> +		.suspend = genphy_suspend,
> +		.resume = genphy_resume,
> +		.read_page = mtk_phy_read_page,
> +		.write_page = mtk_phy_write_page,
> +	},
> +};
> +
> +module_phy_driver(mtk_2p5gephy_driver);
> +
> +static struct mdio_device_id __maybe_unused mtk_2p5ge_phy_tbl[] = {
> +	{ PHY_ID_MATCH_VENDOR(0x00339c00) },
> +	{ }
> +};
> +
> +MODULE_DESCRIPTION("MediaTek 2.5Gb Ethernet PHY driver");
> +MODULE_AUTHOR("SkyLake Huang <SkyLake.Huang@...iatek.com>");
> +MODULE_LICENSE("GPL");
> +
> +MODULE_DEVICE_TABLE(mdio, mtk_2p5ge_phy_tbl);
> +MODULE_FIRMWARE(MT7988_2P5GE_PMB_FW);
> -- 
> 2.45.2
>