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: <BANLkTimA9YGFKjB0_UMsAJeSGEbKGoFJXQ@mail.gmail.com>
Date:	Thu, 28 Apr 2011 18:42:02 -0700
From:	Mahesh Bandewar <maheshb@...gle.com>
To:	Matt Carlson <mcarlson@...adcom.com>
Cc:	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 5:28 PM, Matt Carlson <mcarlson@...adcom.com> wrote:
> On Thu, Apr 28, 2011 at 04:33:19PM -0700, Mahesh Bandewar wrote:
>> ---
>>  drivers/net/tg3.c |   32 ++++++++++++++++++++++++++++++++
>>  1 files changed, 32 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
>> index fa57e3d..208884d 100644
>> --- a/drivers/net/tg3.c
>> +++ b/drivers/net/tg3.c
>> @@ -6319,6 +6319,33 @@ static u32 tg3_fix_features(struct net_device *dev, u32 features)
>>       return features;
>>  }
>>
>> +static int tg3_set_features(struct net_device *dev, u32 features)
>> +{
>> +     struct tg3 *tp = netdev_priv(dev);
>> +     u32 cur_mode = 0;
>> +     int err = 0;
>> +
>> +     spin_lock_bh(&tp->lock);
>> +     cur_mode = tr32(MAC_MODE);
>> +
>> +     if (features & NETIF_F_LOOPBACK) {
>> +             /* Enable internal MAC loopback mode */
>> +             tw32(MAC_MODE, cur_mode | MAC_MODE_PORT_INT_LPBACK);
>> +             netif_carrier_on(tp->dev);
>> +             netdev_info(dev, "Internal MAC loopback mode enabled.\n");
>> +     } else {
>> +             /* Disable internal MAC loopback mode */
>> +             tw32(MAC_MODE, cur_mode & ~MAC_MODE_PORT_INT_LPBACK);
>> +             /* Force link status check */
>> +             if (netif_running(dev))
>> +                     tg3_setup_phy(tp, 1);
>> +             netdev_info(dev, "Internal MAC loopback mode disabled.\n");
>> +     }
>
> I think we should be checking netif_running() before touching the
> hardware at all.  It might be in a low power state.
>
makes sense. I'll change that.

> Also, wouldn't it be better to verify this particular bit changed before
> doing any of these operations?
>
Right! Since netdev infrastructure initiates ndo_set_features() only
when the feature-set is changed, and loopback is the only thing that
is set; I lazily ignored that. But in future additional features could
be set and it's not going to hurt checking before setting that bit.

>> +     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?

Thanks,
--mahesh..

>>       if (tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 &&
>>           !tg3_flag(tp, TSO_CAPABLE) &&
>>           !(tr32(TG3PCI_PCISTATE) & PCISTATE_BUS_SPEED_HIGH)) {
>> --
>> 1.7.3.1
>>
>>
>
>
--
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