netdev - Re: [PATCH v3 net-next] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

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: <YwPf8yXud3mYFvnW@lunn.ch>
Date:   Mon, 22 Aug 2022 21:58:43 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Chunhao Lin <hau@...ltek.com>
Cc:     hkallweit1@...il.com, netdev@...r.kernel.org, nic_swsd@...ltek.com,
        kuba@...nel.org, davem@...emloft.net
Subject: Re: [PATCH v3 net-next] r8169: add support for rtl8168h(revid 0x2a)
 + rtl8211fs fiber application

> @@ -914,8 +952,12 @@ static void r8168g_mdio_write(struct rtl8169_private *tp, int reg, int value)
>  	if (tp->ocp_base != OCP_STD_PHY_BASE)
>  		reg -= 0x10;
>  
> -	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR)
> +	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR) {
> +		if (tp->sfp_if_type != RTL_SFP_IF_NONE && value & BMCR_PDOWN)
> +			return;
> +

Please could you explain this change.

> +/* Data I/O pin control */
> +static void rtl_mdio_dir(struct mdiobb_ctrl *ctrl, int output)
> +{
> +	struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
> +	struct rtl8169_private *tp = bitbang->tp;
> +	const u16 reg = PINOE;
> +	const u16 mask = bitbang->sfp_mask.mdio_oe_mask;
> +	u16 value;

Reverse christmas tree please. Please sort this, longest first.

> +/* MDIO bus init function */
> +static int rtl_mdio_bitbang_init(struct rtl8169_private *tp)
> +{
> +	struct bb_info *bitbang;
> +	struct device *d = tp_to_dev(tp);
> +	struct mii_bus *new_bus;
> +
> +	/* create bit control struct for PHY */
> +	bitbang = devm_kzalloc(d, sizeof(struct bb_info), GFP_KERNEL);
> +	if (!bitbang)
> +		return -ENOMEM;
> +
> +	/* bitbang init */
> +	bitbang->tp = tp;
> +	bitbang->ctrl.ops = &bb_ops;
> +	bitbang->ctrl.op_c22_read = MDIO_READ;
> +	bitbang->ctrl.op_c22_write = MDIO_WRITE;
> +
> +	/* MII controller setting */
> +	new_bus = devm_mdiobus_alloc(d);
> +	if (!new_bus)
> +		return -ENOMEM;
> +
> +	new_bus->read = mdiobb_read;
> +	new_bus->write = mdiobb_write;
> +	new_bus->priv = &bitbang->ctrl;

Please use alloc_mdio_bitbang().

> +static u16 rtl_sfp_mdio_read(struct rtl8169_private *tp,
> +				  u8 reg)
> +{
> +	struct mii_bus *bus = tp->mii_bus;
> +	struct bb_info *bitbang;
> +
> +	if (!bus)
> +		return ~0;
> +
> +	bitbang = container_of(bus->priv, struct bb_info, ctrl);
> +
> +	return bus->read(bus, bitbang->sfp_mask.phy_addr, reg);

By doing this, you are bypassing all the locking. You don't normally
need operations like this. When you register the MDIO bus to the core,
it will go find any PHYs on the bus. You can then use phylib to access
the PHY. A MAC accessing the PHY is generally wrong.

    Andrew