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

[Andrew Lunn]

On Tue, May 21, 2024 at 08:17:32AM +0000, SkyLake Huang (黃啟澤) wrote:
> On Mon, 2024-05-20 at 15:35 +0200, Andrew Lunn wrote:
> >  	 
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >  > +static int mt798x_2p5ge_phy_config_init(struct phy_device
> > *phydev)
> > > +{
> > > +struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> > > +struct device *dev = &phydev->mdio.dev;
> > > +const struct firmware *fw;
> > > +struct pinctrl *pinctrl;
> > > +int ret, i;
> > > +u16 reg;
> > > +
> > > +if (!priv->fw_loaded) {
> > > +if (!priv->md32_en_cfg_base || !priv->pmb_addr) {
> > > +dev_err(dev, "MD32_EN_CFG base & PMB addresses aren't valid\n");
> > > +return -EINVAL;
> > > +}
> > 
> > ...
> > 
> > > +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:
> > > +priv->pmb_addr = ioremap(MT7988_2P5GE_PMB_BASE,
> > MT7988_2P5GE_PMB_LEN);
> > > +if (!priv->pmb_addr)
> > > +return -ENOMEM;
> > > +priv->md32_en_cfg_base = ioremap(MT7988_2P5GE_MD32_EN_CFG_BASE,
> > > + MT7988_2P5GE_MD32_EN_CFG_LEN);
> > > +if (!priv->md32_en_cfg_base)
> > > +return -ENOMEM;
> > > +
> > > +/* 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;
> > > +}
> > 
> > How can priv->md32_en_cfg_base or priv->pmb_addr not be set in
> > mt798x_2p5ge_phy_config_init()
> > 
> > Andrew
> Use command "$ifconfig eth1 down" and then "$ifconfig eth1 up",
> mt798x_2p5ge_phy_config_init() will be called again and again. priv-
> >md32_en_cfg_base & priv->pmb_addr are released after first firmware
> loading. So just check these two values again for safety once priv-
> >fw_loaded is overrided unexpectedly.

So the code is unsymmetrical. The memory is mapped in
mt798x_2p5ge_phy_probe() but unmapped in
mt798x_2p5ge_phy_config_init(). It would be better style to unmap it
in mt798x_2p5ge_phy_remove(). Alternatively, just map it when
downloading firmware, and unmap it straight afterwards.

Also, we generally discourage defensive programming. It is much better
to actually understand the code and know something is not possible.

	Andrew