netdev - Re: [PATCH v4 net-next 09/19] ionic: Add the basic NDO callbacks for netdev support

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: <3a149646-2cb4-1fc8-7b2e-6dff5a36d6c5@pensando.io>
Date:   Tue, 30 Jul 2019 11:35:27 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Saeed Mahameed <saeedm@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH v4 net-next 09/19] ionic: Add the basic NDO callbacks for
 netdev support

On 7/24/19 4:45 PM, Saeed Mahameed wrote:
> On Mon, 2019-07-22 at 14:40 -0700, Shannon Nelson wrote:
>> Set up the initial NDO structure and callbacks for netdev
>> to use, and register the netdev.  This will allow us to do
>> a few basic operations on the device, but no traffic yet.
>>
>> Signed-off-by: Shannon Nelson<snelson@...sando.io>
>> ---
>>   drivers/net/ethernet/pensando/ionic/ionic.h   |   1 +
>>   .../ethernet/pensando/ionic/ionic_bus_pci.c   |   9 +
>>   .../net/ethernet/pensando/ionic/ionic_dev.h   |   2 +
>>   .../net/ethernet/pensando/ionic/ionic_lif.c   | 348
>> ++++++++++++++++++
>>   .../net/ethernet/pensando/ionic/ionic_lif.h   |   5 +
>>   5 files changed, 365 insertions(+)
>>
[...]
>>   
>> +static int ionic_set_nic_features(struct lif *lif, netdev_features_t
>> features);
>>   static int ionic_notifyq_clean(struct lif *lif, int budget);
>>   
>> +int ionic_open(struct net_device *netdev)
>> +{
>> +	struct lif *lif = netdev_priv(netdev);
>> +
>> +	netif_carrier_off(netdev);
>> +
>> +	set_bit(LIF_UP, lif->state);
>> +
>> +	if (netif_carrier_ok(netdev))
> always false ? you just invoked netif_carrier_off two lines ago..

Hmmm... an artifact of splitting up an existing driver.  This makes more 
sense when there's a link status check in between these, which comes in 
about 3 patches later.  Unless this really causes someone significant 
heartburn, I'm going to leave this as is for now.


>> +		netif_tx_wake_all_queues(netdev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ionic_lif_stop(struct lif *lif)
>> +{
>> +	struct net_device *ndev = lif->netdev;
>> +	int err = 0;
>> +
>> +	if (!test_bit(LIF_UP, lif->state)) {
>> +		dev_dbg(lif->ionic->dev, "%s: %s state=DOWN\n",
>> +			__func__, lif->name);
>> +		return 0;
>> +	}
>> +	dev_dbg(lif->ionic->dev, "%s: %s state=UP\n", __func__, lif-
>>> name);
>> +	clear_bit(LIF_UP, lif->state);
>> +
>> +	/* carrier off before disabling queues to avoid watchdog
>> timeout */
>> +	netif_carrier_off(ndev);
>> +	netif_tx_stop_all_queues(ndev);
>> +	netif_tx_disable(ndev);
>> +	synchronize_rcu();
> why synchronize_rcu ?

Looks like a little leakage from a feature in the internal driver, I'll 
remove it.

>> +
>> +	return err;
>> +}
>> +
>> +int ionic_stop(struct net_device *netdev)
>> +{
>> +	struct lif *lif = netdev_priv(netdev);
>> +
>> +	return ionic_lif_stop(lif);
>> +}
>> +
>> +int ionic_reset_queues(struct lif *lif)
>> +{
>> +	bool running;
>> +	int err = 0;
>> +
>> +	/* Put off the next watchdog timeout */
>> +	netif_trans_update(lif->netdev);
> this doesn't seem right to me also this won't help you if the next
> while loop takes too long.. also netif_trans_update is marked to be
> only used for legacy drivers.

If the loop takes too long, then I don't mind if the watchdog goes off.

Yes, its primary use is now handled by netdev_start_xmit(), but it is 
still a handy gizmo to give a little more cushion until the carrier is 
set off in ionic_lif_stop().

sln