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

[Bart Van Assche <Bart.VanAssche@sandisk.com>]

On Tue, 2017-05-23 at 06:19 -0700, Dupuis, Chad wrote:
> Expose this information for interested applications.
> 
> Signed-off-by: Chad Dupuis <chad.dupuis@qlogic.com>
> ---
>  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.