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: <7e24c3c4c3ea42e482a70160aec6930c@realtek.com>
Date:   Tue, 23 Aug 2022 15:17:16 +0000
From:   Hau <hau@...ltek.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        nic_swsd <nic_swsd@...ltek.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net" <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.
H/W has issue. If we power down phy, then H/W will become abnormal.

> > +/* 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.
I will correct this.
 
> > +/* 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().
>
I have tried to use alloc_mdio_bitbang(). But I will get error message " rmmod: ERROR: Module r8169 is in use " when I try to unload r8169.
After debug, I found it is cause by "__module_get(ctrl->ops->owner); " in 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.
> 
Because there is a issue when use alloc_mdio_bitbang() and I just want to use read/write function from bitbanged MDIO framework.
So I did not register MDIO bus.

Thanks,
Hau

> ------Please consider the environment before printing this e-mail.