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

[Andrew Lunn <andrew@lunn.ch>]

> @@ -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