Re: [for-next 2/5] RDMA/bnxt_re: Add support for query firmware version

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 7811 bytes --]

On Tue, Jan 09, 2018 at 11:38:51PM +0530, Devesh Sharma wrote:
> On Tue, Jan 9, 2018 at 7:48 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Fri, Jan 05, 2018 at 06:40:31AM -0500, Devesh Sharma wrote:
> >> From: Selvin Xavier <selvin.xavier-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> >>
> >> The device now reports firmware version thus, removing
> >> the hard coded values of the FW version string and
> >> redundant fw_rev hook from sysfs. Adding code to query
> >> firmware version from underlying device and report it
> >> through the kernel verb to get firmware version string.
> >>
> >> Signed-off-by: Selvin Xavier <selvin.xavier-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> >> Signed-off-by: Devesh Sharma <devesh.sharma-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> >> ---
> >>  drivers/infiniband/hw/bnxt_re/ib_verbs.c   | 14 ++++++++++++--
> >>  drivers/infiniband/hw/bnxt_re/ib_verbs.h   |  1 +
> >>  drivers/infiniband/hw/bnxt_re/main.c       | 11 +----------
> >>  drivers/infiniband/hw/bnxt_re/qplib_rcfw.c |  3 ++-
> >>  drivers/infiniband/hw/bnxt_re/qplib_sp.c   | 25 ++++++++++++++++++++++++-
> >>  drivers/infiniband/hw/bnxt_re/qplib_sp.h   |  3 ++-
> >>  6 files changed, 42 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> >> index 6dd2fe1..8a80e95 100644
> >> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> >> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> >> @@ -141,8 +141,9 @@ int bnxt_re_query_device(struct ib_device *ibdev,
> >>       struct bnxt_qplib_dev_attr *dev_attr = &rdev->dev_attr;
> >>
> >>       memset(ib_attr, 0, sizeof(*ib_attr));
> >> -
> >> -     ib_attr->fw_ver = (u64)(unsigned long)(dev_attr->fw_ver);
> >> +     memcpy(&ib_attr->fw_ver, dev_attr->fw_ver,
> >> +            min(sizeof(dev_attr->fw_ver),
> >> +                sizeof(ib_attr->fw_ver)));
> >>       bnxt_qplib_get_guid(rdev->netdev->dev_addr,
> >>                           (u8 *)&ib_attr->sys_image_guid);
> >>       ib_attr->max_mr_size = BNXT_RE_MAX_MR_SIZE;
> >> @@ -281,6 +282,15 @@ int bnxt_re_get_port_immutable(struct ib_device *ibdev, u8 port_num,
> >>       return 0;
> >>  }
> >>
> >> +void bnxt_re_query_fw_str(struct ib_device *ibdev, char *str)
> >> +{
> >> +     struct bnxt_re_dev *rdev = to_bnxt_re_dev(ibdev, ibdev);
> >> +
> >> +     snprintf(str, IB_FW_VERSION_NAME_MAX, "%d.%d.%d.%d",
> >> +              rdev->dev_attr.fw_ver[0], rdev->dev_attr.fw_ver[1],
> >> +              rdev->dev_attr.fw_ver[2], rdev->dev_attr.fw_ver[3]);
> >> +}
> >> +
> >>  int bnxt_re_query_pkey(struct ib_device *ibdev, u8 port_num,
> >>                      u16 index, u16 *pkey)
> >>  {
> >> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.h b/drivers/infiniband/hw/bnxt_re/ib_verbs.h
> >> index 1df11ed2..66dd8d2 100644
> >> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.h
> >> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.h
> >> @@ -143,6 +143,7 @@ int bnxt_re_query_port(struct ib_device *ibdev, u8 port_num,
> >>                      struct ib_port_attr *port_attr);
> >>  int bnxt_re_get_port_immutable(struct ib_device *ibdev, u8 port_num,
> >>                              struct ib_port_immutable *immutable);
> >> +void bnxt_re_query_fw_str(struct ib_device *ibdev, char *str);
> >>  int bnxt_re_query_pkey(struct ib_device *ibdev, u8 port_num,
> >>                      u16 index, u16 *pkey);
> >>  int bnxt_re_del_gid(struct ib_device *ibdev, u8 port_num,
> >> diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> >> index d825df0..38a5d4b 100644
> >> --- a/drivers/infiniband/hw/bnxt_re/main.c
> >> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> >> @@ -572,6 +572,7 @@ static int bnxt_re_register_ib(struct bnxt_re_dev *rdev)
> >>
> >>       ibdev->query_port               = bnxt_re_query_port;
> >>       ibdev->get_port_immutable       = bnxt_re_get_port_immutable;
> >> +     ibdev->get_dev_fw_str           = bnxt_re_query_fw_str;
> >>       ibdev->query_pkey               = bnxt_re_query_pkey;
> >>       ibdev->query_gid                = bnxt_re_query_gid;
> >>       ibdev->get_netdev               = bnxt_re_get_netdev;
> >> @@ -623,14 +624,6 @@ static ssize_t show_rev(struct device *device, struct device_attribute *attr,
> >>       return scnprintf(buf, PAGE_SIZE, "0x%x\n", rdev->en_dev->pdev->vendor);
> >>  }
> >>
> >> -static ssize_t show_fw_ver(struct device *device, struct device_attribute *attr,
> >> -                        char *buf)
> >> -{
> >> -     struct bnxt_re_dev *rdev = to_bnxt_re_dev(device, ibdev.dev);
> >> -
> >> -     return scnprintf(buf, PAGE_SIZE, "%s\n", rdev->dev_attr.fw_ver);
> >> -}
> >> -
> >>  static ssize_t show_hca(struct device *device, struct device_attribute *attr,
> >>                       char *buf)
> >>  {
> >> @@ -640,12 +633,10 @@ static ssize_t show_hca(struct device *device, struct device_attribute *attr,
> >>  }
> >>
> >>  static DEVICE_ATTR(hw_rev, 0444, show_rev, NULL);
> >> -static DEVICE_ATTR(fw_rev, 0444, show_fw_ver, NULL);
> >>  static DEVICE_ATTR(hca_type, 0444, show_hca, NULL);
> >>
> >>  static struct device_attribute *bnxt_re_attributes[] = {
> >>       &dev_attr_hw_rev,
> >> -     &dev_attr_fw_rev,
> >>       &dev_attr_hca_type
> >>  };
> >>
> >> diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> >> index bb5574a..6a3633a 100644
> >> --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> >> +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> >> @@ -93,7 +93,8 @@ static int __send_message(struct bnxt_qplib_rcfw *rcfw, struct cmdq_base *req,
> >>       opcode = req->opcode;
> >>       if (!test_bit(FIRMWARE_INITIALIZED_FLAG, &rcfw->flags) &&
> >>           (opcode != CMDQ_BASE_OPCODE_QUERY_FUNC &&
> >> -          opcode != CMDQ_BASE_OPCODE_INITIALIZE_FW)) {
> >> +          opcode != CMDQ_BASE_OPCODE_INITIALIZE_FW &&
> >> +          opcode != CMDQ_BASE_OPCODE_QUERY_VERSION)) {
> >>               dev_err(&rcfw->pdev->dev,
> >>                       "QPLIB: RCFW not initialized, reject opcode 0x%x",
> >>                       opcode);
> >> diff --git a/drivers/infiniband/hw/bnxt_re/qplib_sp.c b/drivers/infiniband/hw/bnxt_re/qplib_sp.c
> >> index f5450b7..60377a3 100644
> >> --- a/drivers/infiniband/hw/bnxt_re/qplib_sp.c
> >> +++ b/drivers/infiniband/hw/bnxt_re/qplib_sp.c
> >> @@ -64,6 +64,29 @@ static bool bnxt_qplib_is_atomic_cap(struct bnxt_qplib_rcfw *rcfw)
> >>       return !!(pcie_ctl2 & PCI_EXP_DEVCTL2_ATOMIC_REQ);
> >>  }
> >>
> >> +static void bnxt_qplib_query_version(struct bnxt_qplib_rcfw *rcfw,
> >> +                                  char *fw_ver)
> >> +{
> >> +     struct cmdq_query_version req;
> >> +     struct creq_query_version_resp resp;
> >> +     u16 cmd_flags = 0;
> >> +     int rc = 0;
> >> +
> >> +     RCFW_CMD_PREP(req, QUERY_VERSION, cmd_flags);
> >> +
> >> +     rc = bnxt_qplib_rcfw_send_message(rcfw, (void *)&req,
> >> +                                       (void *)&resp, NULL, 0);
> >> +     if (rc) {
> >> +             dev_err(&rcfw->pdev->dev, "QPLIB: Failed to query version");
> >> +             return;
> >> +     }
> >
> > What will be the result on old FW and new kernel? Will it print error and fail?
> >
> >> +
> >> +     fw_ver[0] = resp.fw_maj;
> >> +     fw_ver[1] = resp.fw_minor;
> >> +     fw_ver[2] = resp.fw_bld;
> >> +     fw_ver[3] = resp.fw_rsvd;
> >> +}
> >> +
> >
> > Thanks
> With the older f/ws There will be an error message in the syslog and
> version string will be all 0s.

You already print errors to syslog in bnxt_qplib_rcfw_send_message
and there is no need in another error print here. To be honest, it
is unclear to me if users are going to be happy to see error print
just because of that.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]