| 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
| ||
|
Message-ID: <f94a042172a644e1aa16b12d92faae65@realtek.com>
Date: Wed, 12 Oct 2022 16:28:47 +0000
From: Hau <hau@...ltek.com>
To: Heiner Kallweit <hkallweit1@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
nic_swsd <nic_swsd@...ltek.com>, Andrew Lunn <andrew@...n.ch>
Subject: RE: [PATCH net-next v4] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application
>
> On 15.09.2022 16:48, Chunhao Lin wrote:
> > rtl8168h(revid 0x2a) + rtl8211fs is for fiber related application.
> > rtl8168h is connected to rtl8211fs mdio bus via its eeprom or gpio pins.
> >
> > In this patch, use bitbanged MDIO framework to access rtl8211fs via
> > rtl8168h's eeprom or gpio pins.
> >
> > And set mdiobb_ops owner to NULL to avoid increase module's refcount
> > to prevent rmmod cannot be done.
> > https://patchwork.kernel.org/project/linux-renesas-soc/patch/202007301
> > 00151.7490-1-ashiduka@...itsu.com/
> >
> > Signed-off-by: Chunhao Lin <hau@...ltek.com>
> > ---
> > drivers/net/ethernet/realtek/Kconfig | 1 +
> > drivers/net/ethernet/realtek/r8169_main.c | 286
> > +++++++++++++++++++++-
> > 2 files changed, 286 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/realtek/Kconfig
> > b/drivers/net/ethernet/realtek/Kconfig
> > index 93d9df55b361..20367114ac72 100644
> > --- a/drivers/net/ethernet/realtek/Kconfig
> > +++ b/drivers/net/ethernet/realtek/Kconfig
> > @@ -100,6 +100,7 @@ config R8169
> > depends on PCI
> > select FW_LOADER
> > select CRC32
> > + select MDIO_BITBANG
> > select PHYLIB
> > select REALTEK_PHY
> > help
> > diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> > b/drivers/net/ethernet/realtek/r8169_main.c
> > index f6f63ba6593a..395eae62050a 100644
> > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > @@ -28,6 +28,7 @@
> > #include <linux/bitfield.h>
> > #include <linux/prefetch.h>
> > #include <linux/ipv6.h>
> > +#include <linux/mdio-bitbang.h>
> > #include <asm/unaligned.h>
> > #include <net/ip6_checksum.h>
> >
> > @@ -333,6 +334,15 @@ enum rtl8125_registers {
> > EEE_TXIDLE_TIMER_8125 = 0x6048,
> > };
> >
> > +enum rtl8168_sfp_registers {
> > + MDIO_IN = 0xdc04,
> > + PINOE = 0xdc06,
> > + PIN_I_SEL_1 = 0xdc08,
> > + PIN_I_SEL_2 = 0xdc0A,
> > + PINPU = 0xdc18,
> > + GPOUTPIN_SEL = 0xdc20,
> > +};
> > +
> > #define RX_VLAN_INNER_8125 BIT(22)
> > #define RX_VLAN_OUTER_8125 BIT(23)
> > #define RX_VLAN_8125 (RX_VLAN_INNER_8125 |
> RX_VLAN_OUTER_8125)
> > @@ -573,6 +583,24 @@ struct rtl8169_tc_offsets {
> > __le16 rx_missed;
> > };
> >
> > +struct rtl_sfp_if_info {
> > + u16 mdio_oe_i;
> > + u16 mdio_oe_o;
> > + u16 mdio_pu;
> > + u16 mdio_pd;
> > + u16 mdc_pu;
> > + u16 mdc_pd;
> > +};
> > +
> > +struct rtl_sfp_if_mask {
> > + const u16 pin;
> > + const u16 mdio_oe;
> > + const u16 mdio;
> > + const u16 mdc;
> > + const u16 phy_addr;
> > + const u16 rb_pos;
> > +};
> > +
> > enum rtl_flag {
> > RTL_FLAG_TASK_ENABLED = 0,
> > RTL_FLAG_TASK_RESET_PENDING,
> > @@ -585,6 +613,12 @@ enum rtl_dash_type {
> > RTL_DASH_EP,
> > };
> >
> > +enum rtl_sfp_if_type {
> > + RTL_SFP_IF_NONE,
> > + RTL_SFP_IF_EEPROM,
> > + RTL_SFP_IF_GPIO,
> > +};
> > +
> > struct rtl8169_private {
> > void __iomem *mmio_addr; /* memory map physical address */
> > struct pci_dev *pci_dev;
> > @@ -624,6 +658,10 @@ struct rtl8169_private {
> > struct rtl_fw *rtl_fw;
> >
> > u32 ocp_base;
> > +
> > + enum rtl_sfp_if_type sfp_if_type;
> > +
> > + struct mii_bus *mii_bus; /* MDIO bus control */
>
> This is at least misleading because mii_bus is unused for the integrated
> copper PHY's. Maybe you can store the mii_bus in struct bb_info and store
> the bb_info instance in struct rtl8169_private.
>
I will do this.
> > };
> >
> > typedef void (*rtl_generic_fct)(struct rtl8169_private *tp); @@
> > -1199,6 +1237,242 @@ static enum rtl_dash_type rtl_check_dash(struct
> rtl8169_private *tp)
> > }
> > }
> >
> > +struct bb_info {
> > + struct rtl8169_private *tp;
> > + struct mdiobb_ctrl ctrl;
> > + struct rtl_sfp_if_mask mask;
> > + u16 pinoe;
> > + u16 pin_i_sel_1;
> > + u16 pin_i_sel_2;
> > +};
> > +
> > +/* Data I/O pin control */
> > +static void rtl_bb_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 mask = bitbang->mask.mdio_oe;
> > + const u16 reg = PINOE;
> > + u16 value;
> > +
> > + value = bitbang->pinoe;
> > + if (output)
> > + value |= mask;
> > + else
> > + value &= ~mask;
> > + r8168_mac_ocp_write(tp, reg, value); }
> > +
> > +/* Set bit data*/
> > +static void rtl_bb_set_mdio(struct mdiobb_ctrl *ctrl, int set) {
> > + struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
> > + struct rtl8169_private *tp = bitbang->tp;
> > + const u16 mask = bitbang->mask.mdio;
> > + const u16 reg = PIN_I_SEL_2;
> > + u16 value;
> > +
> > + value = bitbang->pin_i_sel_2;
> > + if (set)
> > + value |= mask;
> > + else
> > + value &= ~mask;
> > + r8168_mac_ocp_write(tp, reg, value); }
> > +
> > +/* Get bit data*/
> > +static int rtl_bb_get_mdio(struct mdiobb_ctrl *ctrl) {
> > + struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
> > + struct rtl8169_private *tp = bitbang->tp;
> > + const u16 reg = MDIO_IN;
> > +
> > + return (r8168_mac_ocp_read(tp, reg) & BIT(bitbang-
> >mask.rb_pos)) !=
> > +0; }
> > +
> > +/* MDC pin control */
> > +static void rtl_bb_mdc_ctrl(struct mdiobb_ctrl *ctrl, int set) {
> > + struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
> > + struct rtl8169_private *tp = bitbang->tp;
> > + const u16 mask = bitbang->mask.mdc;
> > + const u16 mdc_reg = PIN_I_SEL_1;
> > + u16 value;
> > +
> > + value = bitbang->pin_i_sel_1;
> > + if (set)
> > + value |= mask;
> > + else
> > + value &= ~mask;
> > + r8168_mac_ocp_write(tp, mdc_reg, value); }
> > +
> > +/* mdio bus control struct */
> > +static const struct mdiobb_ops bb_ops = {
> > + .owner = NULL, /* set to NULL for not increase refcount */
> > + .set_mdc = rtl_bb_mdc_ctrl,
> > + .set_mdio_dir = rtl_bb_mdio_dir,
> > + .set_mdio_data = rtl_bb_set_mdio,
> > + .get_mdio_data = rtl_bb_get_mdio,
> > +};
> > +
> > +#define MDIO_READ 2
> > +#define MDIO_WRITE 1
>
> This duplicates the defines in drivers/net/mdio/mdio-bitbang.c
>
I will check this.
> > +/* MDIO bus init function */
> > +static int rtl_mdio_bitbang_init(struct rtl8169_private *tp) {
> > + struct device *d = tp_to_dev(tp);
> > + struct bb_info *bitbang;
> > + 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;
>
> MDIO_READ/WRITE are assigned by alloc_mdio_bitbang(), you don't need
> this here.
>
I will check this.
> > +
> > + /* MII controller setting */
> > + new_bus = alloc_mdio_bitbang(&bitbang->ctrl);
>
> It seems the allocated MDIO bus is never registered.
> This should be added.
>
> > + if (!new_bus)
> > + return -ENOMEM;
> > +
> > + tp->mii_bus = new_bus;
> > +
> > + return 0;
> > +}
> > +
> > +static void rtl_sfp_bitbang_init(struct rtl8169_private *tp,
> > + struct rtl_sfp_if_mask *mask)
> > +{
> > + struct bb_info *bitbang =
> > + container_of(tp->mii_bus->priv, struct bb_info, ctrl);
> > +
> > + r8168_mac_ocp_modify(tp, PINPU, mask->pin, 0);
> > + r8168_mac_ocp_modify(tp, PINOE, 0, mask->pin);
> > + bitbang->pinoe = r8168_mac_ocp_read(tp, PINOE);
> > + bitbang->pin_i_sel_1 = r8168_mac_ocp_read(tp, PIN_I_SEL_1);
> > + bitbang->pin_i_sel_2 = r8168_mac_ocp_read(tp, PIN_I_SEL_2);
> > + memcpy(&bitbang->mask, mask, sizeof(struct rtl_sfp_if_mask)); }
> > +
> > +static void rtl_sfp_mdio_write(struct rtl8169_private *tp,
> > + u8 reg,
> > + u16 val)
> > +{
> > + struct mii_bus *bus = tp->mii_bus;
> > + struct bb_info *bitbang;
> > +
> > + if (!bus)
> > + return;
> > +
> > + bitbang = container_of(bus->priv, struct bb_info, ctrl);
> > + bus->write(bus, bitbang->mask.phy_addr, reg, val); }
> > +
> > +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->mask.phy_addr, reg); }
> > +
> > +static void rtl_sfp_mdio_modify(struct rtl8169_private *tp, u32 reg, u16
> mask,
> > + u16 set)
> > +{
> > + u16 data = rtl_sfp_mdio_read(tp, reg);
> > +
> > + rtl_sfp_mdio_write(tp, reg, (data & ~mask) | set); }
> > +
> > +#define RTL8211FS_PHY_ID_1 0x001c
> > +#define RTL8211FS_PHY_ID_2 0xc916
> > +
>
> As stated earlier, there should be no dependency on a specific external PHY
> in a MAC driver. It's not clear why the bitbanged MDIO bus can't be used with
> other PHY's.
>
I will try to remove this restriction.
> > +static enum rtl_sfp_if_type rtl8168h_check_sfp(struct rtl8169_private
> > +*tp) {
> > + static struct rtl_sfp_if_mask rtl_sfp_if_eeprom_mask = {
> > + 0x0050, 0x0040, 0x000f, 0x0f00, 0, 6};
> > + static struct rtl_sfp_if_mask rtl_sfp_if_gpo_mask = {
> > + 0x0210, 0x0200, 0xf000, 0x0f00, 1, 9};
>
> You hard-code PHY addresses 0/1 here. Why not auto-probing?
> And can't there be designs using other PHY addresses?
>
Not this application only for rtl8211fs.
> > + int const checkcnt = 4;
> > + int i;
> > +
> > + if (rtl_mdio_bitbang_init(tp))
> > + return RTL_SFP_IF_NONE;
> > +
> > + rtl_sfp_bitbang_init(tp, &rtl_sfp_if_eeprom_mask);
> > + rtl_sfp_mdio_write(tp, 0x1f, 0x0000);
> > + for (i = 0; i < checkcnt; i++) {
> > + if (rtl_sfp_mdio_read(tp, MII_PHYSID1) !=
> RTL8211FS_PHY_ID_1 ||
> > + rtl_sfp_mdio_read(tp, MII_PHYSID2) !=
> RTL8211FS_PHY_ID_2)
> > + break;
>
> See earlier comment on dependency on a specific PHY model.
>
> > + }
> > +
> > + if (i == checkcnt)
> > + return RTL_SFP_IF_EEPROM;
> > +
> > + rtl_sfp_bitbang_init(tp, &rtl_sfp_if_gpo_mask);
> > + rtl_sfp_mdio_write(tp, 0x1f, 0x0000);
> > + for (i = 0; i < checkcnt; i++) {
> > + if (rtl_sfp_mdio_read(tp, MII_PHYSID1) !=
> RTL8211FS_PHY_ID_1 ||
> > + rtl_sfp_mdio_read(tp, MII_PHYSID2) !=
> RTL8211FS_PHY_ID_2)
> > + break;
> > + }
> > +
> > + if (i == checkcnt)
> > + return RTL_SFP_IF_GPIO;
> > +
> > + return RTL_SFP_IF_NONE;
> > +}
> > +
> > +static enum rtl_sfp_if_type rtl_check_sfp(struct rtl8169_private *tp)
> > +{
> > + switch (tp->mac_version) {
> > + case RTL_GIGA_MAC_VER_46:
> > + if (tp->pci_dev->revision == 0x2a)
> > + return rtl8168h_check_sfp(tp);
> > + else
> > + return RTL_SFP_IF_NONE;
> > + default:
> > + return RTL_SFP_IF_NONE;
> > + }
> > +}
> > +
> > +static void rtl_hw_sfp_phy_config(struct rtl8169_private *tp) {
>
> This PHY model specific initialization belongs to the PHY driver.
> Presumably RTL8211FS support should be added to drivers/net/phy/realtek.c
>
I am not familiar with phy driver. May I know how to do this?
> > + /* disable ctap */
> > + rtl_sfp_mdio_write(tp, 0x1f, 0x0a43);
> > + rtl_sfp_mdio_modify(tp, 0x19, BIT(6), 0);
> > +
> > + /* change Rx threshold */
> > + rtl_sfp_mdio_write(tp, 0x1f, 0x0dcc);
> > + rtl_sfp_mdio_modify(tp, 0x14, 0, BIT(2) | BIT(3) | BIT(4));
> > +
> > + /* switch pin34 to PMEB pin */
> > + rtl_sfp_mdio_write(tp, 0x1f, 0x0d40);
> > + rtl_sfp_mdio_modify(tp, 0x16, 0, BIT(5));
> > +
> > + /* enable pass_linkctl_en */
> > + rtl_sfp_mdio_write(tp, 0x1f, 0x0a4b);
> > + rtl_sfp_mdio_modify(tp, 0x11, 0, BIT(4));
> > +
> > + rtl_sfp_mdio_write(tp, 0x1f, 0x0000);
> > +
> > + /* disable ctap */
> > + phy_modify_paged(tp->phydev, 0x0a43, 0x11, BIT(6), 0); }
> > +
> > static void rtl_set_d3_pll_down(struct rtl8169_private *tp, bool
> > enable) {
> > switch (tp->mac_version) {
> > @@ -2168,6 +2442,9 @@ static void rtl8169_init_phy(struct rtl8169_private
> *tp)
> > tp->pci_dev->subsystem_device == 0xe000)
> > phy_write_paged(tp->phydev, 0x0001, 0x10, 0xf01b);
> >
> > + if (tp->sfp_if_type != RTL_SFP_IF_NONE)
> > + rtl_hw_sfp_phy_config(tp);
> > +
> > /* We may have called phy_speed_down before */
> > phy_speed_up(tp->phydev);
> >
> > @@ -2224,7 +2501,8 @@ static void rtl_prepare_power_down(struct
> rtl8169_private *tp)
> > rtl_ephy_write(tp, 0x19, 0xff64);
> >
> > if (device_may_wakeup(tp_to_dev(tp))) {
> > - phy_speed_down(tp->phydev, false);
> > + if (tp->sfp_if_type == RTL_SFP_IF_NONE)
> > + phy_speed_down(tp->phydev, false);
> > rtl_wol_enable_rx(tp);
> > }
> > }
> > @@ -4866,6 +5144,10 @@ static void rtl_remove_one(struct pci_dev
> *pdev)
> > if (tp->dash_type != RTL_DASH_NONE)
> > rtl8168_driver_stop(tp);
> >
> > + /* free bitbang info */
> > + if (tp->mii_bus)
> > + free_mdio_bitbang(tp->mii_bus);
> > +
> > rtl_release_firmware(tp);
> >
> > /* restore original MAC address */
> > @@ -5210,6 +5492,8 @@ static int rtl_init_one(struct pci_dev *pdev,
> > const struct pci_device_id *ent)
> >
> > tp->dash_type = rtl_check_dash(tp);
> >
> > + tp->sfp_if_type = rtl_check_sfp(tp);
> > +
> > tp->cp_cmd = RTL_R16(tp, CPlusCmd) & CPCMD_MASK;
> >
> > if (sizeof(dma_addr_t) > 4 && tp->mac_version >=
> RTL_GIGA_MAC_VER_18
> > &&
>
>
> ------Please consider the environment before printing this e-mail.
Powered by blists - more mailing lists