netdev - Re: [PATCH rdma-next v1 04/12] IB/opa-vnic: Virtual Network Interface Controller (VNIC) netdev

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]

Date:   Wed, 12 Apr 2017 11:31:37 -0700
From:   "Vishwanathapura, Niranjana" <niranjana.vishwanathapura@...el.com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     dledford@...hat.com, linux-rdma@...r.kernel.org,
        netdev@...r.kernel.org, dennis.dalessandro@...el.com,
        ira.weiny@...el.com, Sadanand Warrier <sadanand.warrier@...el.com>,
        Sudeep Dutt <sudeep.dutt@...el.com>,
        Andrzej Kacprowski <andrzej.kacprowski@...el.com>
Subject: Re: [PATCH rdma-next v1 04/12] IB/opa-vnic: Virtual Network
 Interface Controller (VNIC) netdev

On Wed, Apr 12, 2017 at 10:08:30AM +0300, Leon Romanovsky wrote:
>> +#define v_dbg(format, arg...) \
>> +	netdev_dbg(adapter->netdev, format, ## arg)
>> +#define v_err(format, arg...) \
>> +	netdev_err(adapter->netdev, format, ## arg)
>> +#define v_info(format, arg...) \
>> +	netdev_info(adapter->netdev, format, ## arg)
>> +#define v_warn(format, arg...) \
>> +	netdev_warn(adapter->netdev, format, ## arg)
>> +
>
>IMHO, these wrappers are redundant.
>

Using same constructs as some Intel standard ethernet drivers.

>> +/* opa_netdev_open - activate network interface */
>> +static int opa_netdev_open(struct net_device *netdev)
>> +{
>> +	struct opa_vnic_adapter *adapter = opa_vnic_priv(netdev);
>> +	int rc;
>> +
>> +	rc = adapter->rn_ops->ndo_open(adapter->netdev);
>> +	if (rc) {
>> +		v_dbg("open failed %d\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	v_info("opened\n");
>
>All these v_info are achieved by tracepoints (function tracer).
>

Some of these messages are useful for analysing reported logs.
Let me change these opened/closed messges to debug level.

>> +
>> +	netdev = ibdev->alloc_rdma_netdev(ibdev, port_num,
>> +					  RDMA_NETDEV_OPA_VNIC,
>> +					  "veth%d", NET_NAME_UNKNOWN,
>> +					  ether_setup);
>> +	if (!netdev)
>> +		return ERR_PTR(-ENOMEM);
>> +	else if (IS_ERR(netdev))
>> +		return ERR_CAST(netdev);
>> +
>

>Erez and Jason came to this code for IPoIB, it is better to have same
>error handling for all alloc_rdma_netdev callers.
>+       if (hca->alloc_rdma_netdev) {
>+               dev = hca->alloc_rdma_netdev(hca, port,
>+                                            RDMA_NETDEV_IPOIB, name,
>+                                            NET_NAME_UNKNOWN,
>+                                            ipoib_setup_common);
>+               if (IS_ERR_OR_NULL(dev) && PTR_ERR(dev) != -EOPNOTSUPP)
>+                       return NULL;
>+       }
>
>

IPoIB handles EOPNOTSUPP differently (by assigning default operations).
It is not applicable to OPA VNIC, hence it just returns the error code.

I just noticed that IPoIB is using EOPNOTSUPP, however OPA VNIC is using 
ENOTSUPP. EOPNOTSUPP seesm to be widely used, so I will change OPA VNIC to use 
the same. Will also document this requirement in ib_verbs.h where this function 
is defined.

Niranjana