On Thu, Aug 25, 2016 at 00:32:13 -0700, Yuval Shaia wrote:
> On Wed, Aug 03, 2016 at 04:27:33PM -0700, Adit Ranadive wrote:
> > This patch implements the remaining Verbs functions registered with
> > the core RDMA stack.
> >
> > Changes v2->v3:
> > - Removed the boolean from pvrdma_cmd_post call.
> >
> > Reviewed-by: Jorgen Hansen <jhansen@xxxxxxxxxx>
> > Reviewed-by: George Zhang <georgezhang@xxxxxxxxxx>
> > Reviewed-by: Aditya Sarwade <asarwade@xxxxxxxxxx>
> > Reviewed-by: Bryan Tan <bryantan@xxxxxxxxxx>
> > Signed-off-by: Adit Ranadive <aditr@xxxxxxxxxx>
> > ---
> > drivers/infiniband/hw/pvrdma/pvrdma_verbs.c | 593
> > ++++++++++++++++++++++++++++
> > drivers/infiniband/hw/pvrdma/pvrdma_verbs.h | 108 +++++
> > 2 files changed, 701 insertions(+)
> > create mode 100644 drivers/infiniband/hw/pvrdma/pvrdma_verbs.c
> > create mode 100644 drivers/infiniband/hw/pvrdma/pvrdma_verbs.h
...
> > +
> > + err = pvrdma_cmd_post(to_vdev(ibdev), &req, &rsp);
> > + if (err < 0 || rsp.hdr.ack != PVRDMA_CMD_QUERY_PKEY_RESP) {
> > + struct pvrdma_dev *dev = to_vdev(ibdev);
> > +
> > + dev_warn(&dev->pdev->dev, "could not query device
> pkey\n");
>
> Error description or at least error code would help.
> (this apply to all cmd errors below)
Ok. I separated this out so that we log the error correctly.
>
> > + return -EFAULT;
> > + }
> > +
> > + *pkey = rsp.query_pkey_resp.pkey;
> > +
> > + return 0;
> > +}
> > +
> > +enum rdma_link_layer pvrdma_port_link_layer(struct ib_device *ibdev,
> > + u8 port)
> > +{
> > + return IB_LINK_LAYER_ETHERNET;
> > +}
> > +
> > +int pvrdma_modify_device(struct ib_device *ibdev, int mask,
> > + struct ib_device_modify *props)
> > +{
> > + unsigned long flags;
> > + int ret;
> > +
> > + if (mask & ~(IB_DEVICE_MODIFY_SYS_IMAGE_GUID |
> > + IB_DEVICE_MODIFY_NODE_DESC)) {
> > + struct pvrdma_dev *dev = to_vdev(ibdev);
> > +
> > + dev_warn(&dev->pdev->dev, "unsupported device modify
> mask\n");
> > + ret = -EOPNOTSUPP;
> > + goto err_out;
> > + }
> > +
> > + if (mask & IB_DEVICE_MODIFY_NODE_DESC) {
> > + spin_lock_irqsave(&to_vdev(ibdev)->desc_lock, flags);
> > + memcpy(ibdev->node_desc, props->node_desc, 64);
> > + spin_unlock_irqrestore(&to_vdev(ibdev)->desc_lock, flags);
> > + }
> > +
> > + if (mask & IB_DEVICE_MODIFY_SYS_IMAGE_GUID) {
> > + mutex_lock(&to_vdev(ibdev)->port_mutex);
> > + to_vdev(ibdev)->sys_image_guid =
> > + cpu_to_be64(props->sys_image_guid);
> > + mutex_unlock(&to_vdev(ibdev)->port_mutex);
> > + }
>
> Suggesting restructure:
> if (mask & IB_DEVICE_MODIFY_NODE_DESC) {
> do stuff
> goto out;
> }
> if (mask & IB_DEVICE_MODIFY_SYS_IMAGE_GUID) {
> do stuff
> goto out;
> }
>
> dev_warn(&dev->pdev->dev, "Unsupported device modify mask
> 0x%x\n", mask);
> ret = -EOPNOTSUPP;
>
> out:
> return err;
>
> Reasons:
> 1. Code will be easy to maintain when adding support for new mask.
> 2. Less & and | operations.
>
Thanks for the suggestion but not sure if that would work. We need to
check whether the mask is correct, which means checking if either one or
both the masks is/are specified. Also, if both masks are specified we need to
do stuff for both. In your suggestion, if the DEVICE_MODIFY mask is specified
we would skip the SYS_IMAGE mask.
I did remove the goto though in the first if check and the other suggestions.
Thanks,
Adit
--
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
