linux-kernel - Re: [RFC PATCH V2 net-next 03/11] net: hibmcge: Add mdio and hardware configuration supported in this module

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: <e2f0e003-b5a4-4068-b923-eda6f7accd63@huawei.com>
Date: Fri, 16 Aug 2024 11:07:45 +0800
From: Jijie Shao <shaojijie@...wei.com>
To: Andrew Lunn <andrew@...n.ch>
CC: <shaojijie@...wei.com>, <yisen.zhuang@...wei.com>,
	<salil.mehta@...wei.com>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <horms@...nel.org>,
	<shenjian15@...wei.com>, <wangpeiyang1@...wei.com>, <liuyonglong@...wei.com>,
	<sudongming1@...wei.com>, <xujunsheng@...wei.com>, <shiyongbang@...wei.com>,
	<jdamato@...tly.com>, <jonathan.cameron@...wei.com>,
	<shameerali.kolothum.thodi@...wei.com>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH V2 net-next 03/11] net: hibmcge: Add mdio and hardware
 configuration supported in this module


on 2024/8/16 10:14, Andrew Lunn wrote:
>> +int hbg_hw_adjust_link(struct hbg_priv *priv, u32 speed, u32 duplex)
>> +{
>> +	if (speed != HBG_PORT_MODE_SGMII_10M &&
>> +	    speed != HBG_PORT_MODE_SGMII_100M &&
>> +	    speed != HBG_PORT_MODE_SGMII_1000M)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (duplex != DUPLEX_FULL && duplex != DUPLEX_HALF)
>> +		return -EOPNOTSUPP;
>
> Can this happen? We try to avoid defensive code, preferring to ensure
> it can never happen. So long as you have told phylib the limits of
> your hardware, it should enforce these.

Ok, I'll delete these.

>
>> @@ -26,11 +27,11 @@ static int hbg_init(struct hbg_priv *priv)
>>   		return dev_err_probe(dev, PTR_ERR(regmap), "failed to init regmap\n");
>>   
>>   	priv->regmap = regmap;
>> -	ret = hbg_hw_event_notify(priv, HBG_HW_EVENT_INIT);
>> +	ret = hbg_hw_init(priv);
>>   	if (ret)
>>   		return ret;
>>   
>> -	return hbg_hw_dev_specs_init(priv);
>> +	return hbg_mdio_init(priv);
> I've not read the previous patches, but that looks odd. Why is code
> you just added in previous patches getting replaced?

In previous patch, we did not introduce the hbg_hw_init().
In this patch, the hbg_hw_init() is introduced,
and the hbg_hw_event_notify() and hbg_hw_dev_specs_init() functions are moved to hbg_hw_init().


hbg_hw_init() can be moved to the previous patch.

>
>> +static int hbg_phy_connect(struct hbg_priv *priv)
>> +{
>> +	struct phy_device *phydev = priv->mac.phydev;
>> +	struct device *dev = &priv->pdev->dev;
>> +	struct hbg_mac *mac = &priv->mac;
>> +	int ret;
>> +
>> +	ret = phy_connect_direct(priv->netdev, mac->phydev, hbg_phy_adjust_link,
>> +				 PHY_INTERFACE_MODE_SGMII);
>> +	if (ret)
>> +		return dev_err_probe(dev, -ENOMEM, "failed to connect phy\n");
> Don't replace the error code. Doing so actually makes dev_err_probe()
> pointless because it is not going to see the EPROBE_DEFER.
>
Sorry, It is a mistake.
I'll fix it in the next version.

>> +int hbg_mdio_init(struct hbg_priv *priv)
>> +{
>> +	struct device *dev = &priv->pdev->dev;
>> +	struct hbg_mac *mac = &priv->mac;
>> +	struct phy_device *phydev;
>> +	struct mii_bus *mdio_bus;
>> +	int ret;
>> +
>> +	mac->phy_addr = priv->dev_specs.phy_addr;
>> +	mdio_bus = devm_mdiobus_alloc(dev);
>> +	if (!mdio_bus)
>> +		return dev_err_probe(dev, -ENOMEM, "failed to alloc MDIO bus\n");
>> +
>> +	mdio_bus->parent = dev;
>> +	mdio_bus->priv = priv;
>> +	mdio_bus->phy_mask = ~(1 << mac->phy_addr);
>> +	mdio_bus->name = "hibmcge mii bus";
>> +	mac->mdio_bus = mdio_bus;
>> +
>> +	mdio_bus->read = hbg_mdio_read22;
>> +	mdio_bus->write = hbg_mdio_write22;
>> +	snprintf(mdio_bus->id, MII_BUS_ID_SIZE, "%s-%s", "mii", dev_name(dev));
>> +
>> +	ret = devm_mdiobus_register(dev, mdio_bus);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to register MDIO bus\n");
>> +
>> +	phydev = mdiobus_get_phy(mdio_bus, mac->phy_addr);
>> +	if (!phydev)
>> +		return dev_err_probe(dev, -EIO, "failed to get phy device\n");
> ENODEV is probably better, since the device does not exist.
>
> 	Andrew

Thanks a lot!

	Jijie Shao