On Wed, Apr 12, 2017 at 11:31:37AM -0700, Vishwanathapura, Niranjana wrote:
> 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.
I'll leave this decision to Doug. IMHO open coded variant is preferable.
>
> > > +/* 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.
I agree with you regarding error messages, I disagree regarding
flow/info messages.
>
> > > +
> > > + 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.
The ENOTSUPP is for NFS and it can't be returned to user space. The proper
error is EOPNOTSUPP.
Thanks
>
> Niranjana
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: PGP signature
