Re: [PATCH 06/15] qedf: Add fka_period SCSI host attribute to show fip keep alive period.

[Bart Van Assche]

On Tue, 2017-05-23 at 06:19 -0700, Dupuis, Chad wrote:
> Expose this information for interested applications.
> 
> Signed-off-by: Chad Dupuis 
> ---
>  drivers/scsi/qedf/qedf_attr.c | 60 
> +--
>  1 file changed, 41 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/qedf/qedf_attr.c b/drivers/scsi/qedf/qedf_attr.c
> index 1349f8a..68e2b77 100644
> --- a/drivers/scsi/qedf/qedf_attr.c
> +++ b/drivers/scsi/qedf/qedf_attr.c
> @@ -8,6 +8,25 @@
>   */
>  #include "qedf.h"
>  
> +inline bool qedf_is_vport(struct qedf_ctx *qedf)
> +{
> + return (!(qedf->lport->vport == NULL));
> +}

Have you considered to write the return statement as follows?

return qedf->lport->vport != NULL;

Checkpatch should have recommended not to use parentheses in the return
statement.

> +
> +/* Get base qedf for physical port from vport */
> +static struct qedf_ctx *qedf_get_base_qedf(struct qedf_ctx *qedf)
> +{
> + struct fc_lport *lport;
> + struct fc_lport *base_lport;
> +
> + if (!(qedf_is_vport(qedf)))
> + return NULL;
> +
> + lport = qedf->lport;
> + base_lport = shost_priv(vport_to_shost(lport->vport));
> + return (struct qedf_ctx *)(lport_priv(base_lport));
> +}

lport_priv() returns a void pointer so the cast in the return statement is not
necessary.

> +static ssize_t
> +qedf_fka_period_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fc_lport *lport = shost_priv(class_to_shost(dev));
> + struct qedf_ctx *qedf = lport_priv(lport);
> + int fka_period = -1;
> +
> + if (qedf_is_vport(qedf))
> + qedf = qedf_get_base_qedf(qedf);
> +
> + if (!qedf->ctlr.sel_fcf)
> + goto out;
> +
> + fka_period = qedf->ctlr.sel_fcf->fka_period;
> +
> +out:
> + return scnprintf(buf, PAGE_SIZE, "%d\n", fka_period);
> +}

Do we really need a goto statement to skip a single statement? How about the
following:

if (qedf->ctlr.sel_fcf)
fka_period = qedf->ctlr.sel_fcf->fka_period;

return scnprintf(buf, PAGE_SIZE, "%d\n", fka_period);

or this:

return scnprintf(buf, PAGE_SIZE, "%d\n", qedf->ctlr.sel_fcf ?
 qedf->ctlr.sel_fcf->fka_period : -1);

Bart.

[PATCH 06/15] qedf: Add fka_period SCSI host attribute to show fip keep alive period.

[Dupuis, Chad]

Expose this information for interested applications.

Signed-off-by: Chad Dupuis 
---
 drivers/scsi/qedf/qedf_attr.c | 60 +--
 1 file changed, 41 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/qedf/qedf_attr.c b/drivers/scsi/qedf/qedf_attr.c
index 1349f8a..68e2b77 100644
--- a/drivers/scsi/qedf/qedf_attr.c
+++ b/drivers/scsi/qedf/qedf_attr.c
@@ -8,6 +8,25 @@
  */
 #include "qedf.h"
 
+inline bool qedf_is_vport(struct qedf_ctx *qedf)
+{
+   return (!(qedf->lport->vport == NULL));
+}
+
+/* Get base qedf for physical port from vport */
+static struct qedf_ctx *qedf_get_base_qedf(struct qedf_ctx *qedf)
+{
+   struct fc_lport *lport;
+   struct fc_lport *base_lport;
+
+   if (!(qedf_is_vport(qedf)))
+   return NULL;
+
+   lport = qedf->lport;
+   base_lport = shost_priv(vport_to_shost(lport->vport));
+   return (struct qedf_ctx *)(lport_priv(base_lport));
+}
+
 static ssize_t
 qedf_fcoe_mac_show(struct device *dev,
struct device_attribute *attr, char *buf)
@@ -26,34 +45,37 @@
return scnprintf(buf, PAGE_SIZE, "%pM\n", fcoe_mac);
 }
 
+static ssize_t
+qedf_fka_period_show(struct device *dev,
+   struct device_attribute *attr, char *buf)
+{
+   struct fc_lport *lport = shost_priv(class_to_shost(dev));
+   struct qedf_ctx *qedf = lport_priv(lport);
+   int fka_period = -1;
+
+   if (qedf_is_vport(qedf))
+   qedf = qedf_get_base_qedf(qedf);
+
+   if (!qedf->ctlr.sel_fcf)
+   goto out;
+
+   fka_period = qedf->ctlr.sel_fcf->fka_period;
+
+out:
+   return scnprintf(buf, PAGE_SIZE, "%d\n", fka_period);
+}
+
 static DEVICE_ATTR(fcoe_mac, S_IRUGO, qedf_fcoe_mac_show, NULL);
+static DEVICE_ATTR(fka_period, S_IRUGO, qedf_fka_period_show, NULL);
 
 struct device_attribute *qedf_host_attrs[] = {
&dev_attr_fcoe_mac,
+   &dev_attr_fka_period,
NULL,
 };
 
 extern const struct qed_fcoe_ops *qed_ops;
 
-inline bool qedf_is_vport(struct qedf_ctx *qedf)
-{
-   return (!(qedf->lport->vport == NULL));
-}
-
-/* Get base qedf for physical port from vport */
-static struct qedf_ctx *qedf_get_base_qedf(struct qedf_ctx *qedf)
-{
-   struct fc_lport *lport;
-   struct fc_lport *base_lport;
-
-   if (!(qedf_is_vport(qedf)))
-   return NULL;
-
-   lport = qedf->lport;
-   base_lport = shost_priv(vport_to_shost(lport->vport));
-   return (struct qedf_ctx *)(lport_priv(base_lport));
-}
-
 void qedf_capture_grc_dump(struct qedf_ctx *qedf)
 {
struct qedf_ctx *base_qedf;
-- 
1.8.5.6