netdev - Re: [PATCH 2/2] tg3: Add code to allow ethtool to enable/disable loopback.

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: <20110429024652.GB20805@mcarlson.broadcom.com>
Date:	Thu, 28 Apr 2011 19:46:52 -0700
From:	"Matt Carlson" <mcarlson@...adcom.com>
To:	"Mahesh Bandewar" <maheshb@...gle.com>
cc:	"Matthew Carlson" <mcarlson@...adcom.com>,
	"David Miller" <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	"Michael Chan" <mchan@...adcom.com>,
	"Ben Hutchings" <bhutchings@...arflare.com>,
	"Micha? Miros?aw" <mirq-linux@...e.qmqm.pl>
Subject: Re: [PATCH 2/2] tg3: Add code to allow ethtool to
 enable/disable loopback.

On Thu, Apr 28, 2011 at 06:42:02PM -0700, Mahesh Bandewar wrote:
> >> + ? ? spin_unlock_bh(&tp->lock);
> >> +
> >> + ? ? return err;
> >> +}
> >> +
> >> ?static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int new_mtu)
> >> ?{
> >> @@ -15028,6 +15055,7 @@ static const struct net_device_ops tg3_netdev_ops = {
> >> ? ? ? .ndo_tx_timeout ? ? ? ? = tg3_tx_timeout,
> >> ? ? ? .ndo_change_mtu ? ? ? ? = tg3_change_mtu,
> >> ? ? ? .ndo_fix_features ? ? ? = tg3_fix_features,
> >> + ? ? .ndo_set_features ? ? ? = tg3_set_features,
> >> ?#ifdef CONFIG_NET_POLL_CONTROLLER
> >> ? ? ? .ndo_poll_controller ? ?= tg3_poll_controller,
> >> ?#endif
> >> @@ -15044,6 +15072,7 @@ static const struct net_device_ops tg3_netdev_ops_dma_bug = {
> >> ? ? ? .ndo_do_ioctl ? ? ? ? ? = tg3_ioctl,
> >> ? ? ? .ndo_tx_timeout ? ? ? ? = tg3_tx_timeout,
> >> ? ? ? .ndo_change_mtu ? ? ? ? = tg3_change_mtu,
> >> + ? ? .ndo_set_features ? ? ? = tg3_set_features,
> >> ?#ifdef CONFIG_NET_POLL_CONTROLLER
> >> ? ? ? .ndo_poll_controller ? ?= tg3_poll_controller,
> >> ?#endif
> >> @@ -15241,6 +15270,9 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
> >> ? ? ? dev->features |= hw_features;
> >> ? ? ? dev->vlan_features |= hw_features;
> >>
> >> + ? ? /* Add the loopback capability */
> >> + ? ? dev->hw_features |= NETIF_F_LOOPBACK;
> >
> > Not all tg3 devices can do MAC loopback. ?I'd suggest qualifying this
> > with:
> >
> > ? ? ? ? ? ? ? ?if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5780 ||
> > ? ? ? ? ? ? ? ? ? ?(tp->tg3_flags & TG3_FLAG_CPMU_PRESENT))
> >
> > But that will exclude a lot of our newer devices. ?Does it matter what
> > type of loopback is used? ?Newer devices prefer internal phy loopback
> > over MAC loopback.
> >
> As long as device supports some sort of loopback, we should be setting
> this capability and move this logic (or similar) to set_features() and
> choose the method that is supported. Since several devices support
> loopback at various levels, to keep it consistent, we should be
> setting the loopback closest to the host. So can I simply set the
> int-phy loopback in the else part of the above 'provided if' or would
> need other logic? or in other words -
> 
>  if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5780 ||
> (tp->tg3_flags & TG3_FLAG_CPMU_PRESENT))
>     supported_mode = MAC;
> else
>     supported_mode = INTPHY;
> 
> if (supported_mode == MAC)
>     cur_mode = tr32(MAC_MODE);
> else
>     tg3_readphy(tp, MII_BMCR, &cur_mode);
> 
> Would something like this work?

That might be where we want to end up, but it'll be some work to get
there.  Maybe it makes sense to just keep MAC loopback mode for now and
only enable it for a subset of devices.  Adding phy loopback mode sounds
like it will require some surgery.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html