netdev - Re: [RFC v1 for accelerated IPoIB 25/25] mlx5_ib: skeleton for mlx5_ib to support ipoib

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:   Tue, 14 Mar 2017 16:53:24 +0200
From:   Erez Shitrit <erezsh@....mellanox.co.il>
To:     Jason Gunthorpe <jgunthorpe@...idianresearch.com>
Cc:     Erez Shitrit <erezsh@...lanox.com>,
        Doug Ledford <dledford@...hat.com>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        netdev@...r.kernel.org, valex@...lanox.com,
        Leon Romanovsky <leonro@...lanox.com>, saedm@...lanox.com
Subject: Re: [RFC v1 for accelerated IPoIB 25/25] mlx5_ib: skeleton for
 mlx5_ib to support ipoib_ops

On Mon, Mar 13, 2017 at 10:27 PM, Jason Gunthorpe
<jgunthorpe@...idianresearch.com> wrote:
> On Mon, Mar 13, 2017 at 08:31:36PM +0200, Erez Shitrit wrote:
>
>> +struct net_device *mlx5_alloc_rdma_netdev(struct ib_device *hca,
>> +                                  u8 port_num,
>> +                                  enum rdma_netdev_t type,
>> +                                  const char *name,
>> +                                  unsigned char name_assign_type,
>> +                                  void (*setup)(struct net_device *));
>> +void mlx5_free_rdma_netdev(struct net_device *netdev);
>
> Seems like OK signatures to me..
>
>> +     dev->ib_dev.alloc_rdma_netdev   = mlx5_alloc_rdma_netdev;
>> +     dev->ib_dev.free_rdma_netdev    = mlx5_free_rdma_netdev;
>
> Since mlx5_free_rdma_netdev is empty this should just be NULL

OK,

>
>> +int mlx5_ib_dev_init(struct net_device *dev, struct ib_device *hca,
>> +                  int *qp_num)
>> +{
>> +     void *next_priv = ipoib_dev_priv(dev);
>> +     struct rdma_netdev *rn = netdev_priv(dev);
>> +     struct mlx5_ib_dev *ib_dev = to_mdev(hca);
>> +     int ret;
>> +
>> +     ret = mlx5i_attach(ib_dev->mdev, next_priv);
>> +     if (ret) {
>> +             pr_err("Failed resources allocation for device: %s ret: %d\n",
>> +                    dev->name, ret);
>> +             return ret;
>> +     }
>> +
>> +     *qp_num = rn->qp_num;
>> +
>> +     pr_debug("resources allocated for device: %s\n", dev->name);
>> +
>> +     return 0;
>> +}
>> +
>> +void mlx5_ib_dev_cleanup(struct net_device *dev, struct ib_device *hca)
>> +{
>> +     void *next_priv = ipoib_dev_priv(dev);
>> +     struct rdma_netdev *rn = netdev_priv(dev);
>> +     struct mlx5_ib_dev *ib_dev = to_mdev(hca);
>> +     struct mlx5_qp_context context;
>> +     int ret;
>> +
>> +     /* detach qp from flow-steering by reset it */
>> +     ret = mlx5_core_qp_modify(ib_dev->mdev,
>> +                               MLX5_CMD_OP_2RST_QP, 0, &context,
>> +                               (struct mlx5_core_qp *)rn->context);
>> +     if (ret)
>> +             pr_err("%s failed (ret: %d) to reset QP\n", __func__, ret);
>> +
>> +     mlx5i_detach(ib_dev->mdev, next_priv);
>> +
>> +     mlx5_ib_clean_qp(ib_dev, (struct mlx5_core_qp *)rn->context);
>> +}
>
> Why isn't this stuff in open/close?

According to ipoib control flows, there is a different between
open/close to init/cleanup
for example, in open/close the driver doesn't destroy hw resources,
just change the state, it destroys them in cleanup.

>
>> +void mlx5_ib_send(struct net_device *dev, struct sk_buff *skb,
>> +               struct ipoib_ah *address, u32 dqpn, u32 dqkey)
>> +{
>> +     void *next_priv = ipoib_dev_priv(dev);
>> +
>> +     mlx5i_xmit(skb, next_priv, &to_mah(address->ah)->av, dqpn, dqkey);
>
> How come the qkey is not available via ipoib_ah ?
>
> to_mah(address->ah)->av->key.qkey.qkey
>
> ?

It is, i will change the signature of that function accordingly.

>
>> +static const struct net_device_ops ipoib_netdev_default_pf = {
>
> That is a weird name for a mlx5 specific structure.

OK, will change that.

>
>> +     param.size_base_priv = sizeof(struct ipoib_rdma_netdev);
>
> This is really weird, the code in mlx5i_create_netdev calls
> ipoib_dev_priv so it must assume the struct is a ipoib_rdma_netdev.

It is the same attitude as in the vnic/hfi
(https://patchwork.kernel.org/patch/9587815/)
The lower driver allocates space for the rdma_netdev.
the only struct that is known between the layers is rdma_netdev.

>
>> +     /* set func pointers */
>> +     rn = netdev_priv(dev);
>> +     rn->qp_num = qp->qpn;
>> +     rn->context = qp;
>
> No for using context.. You need your own driver priv, like this:
>
> struct mlx4_rn_priv
> {
>     struct mlx5e_priv priv;
>     struct mlx5_core_qp *qp;
> };

OK, will try to fix it (i have a priv which is shared with the en
driver, so i don't want to mix it with ib objects like qp, will find a
solution for that, thanks.)



>
> Jason