On Tue, 2017-12-19 at 22:56 -0800, Himanshu Madhani wrote: 
> +#define QLA_ENA_FW_RES_TRACKING(_ha) { \
> +     int i; \
> +     _ha->base_qpair->fw_res_tracking = 1; \
> +     for (i = 0; i < _ha->max_qpairs; i++) { \
> +             if (_ha->queue_pair_map[i]) \
> +             _ha->queue_pair_map[i]->fw_res_tracking = 1; \
> +     } \
> +}
> +
> +#define QLA_DIS_FW_RES_TRACKING(_ha) { \
> +     int i; \
> +     _ha->base_qpair->fw_res_tracking = 0; \
> +     for (i = 0; i < _ha->max_qpairs; i++) { \
> +             if (_ha->queue_pair_map[i]) \
> +             _ha->queue_pair_map[i]->fw_res_tracking = 0; \
> +     } \
> +}

Has this patch been verified with checkpatch? Did checkpatch suggest to surround
these macros with do / while (0)?

>  int
>  qla2x00_dfs_setup(scsi_qla_host_t *vha)
> @@ -504,6 +776,33 @@ qla2x00_dfs_setup(scsi_qla_host_t *vha)
>                           "Unable to create debugFS naqp node.\n");
>                       goto out;
>               }
> +
> +             if (ql2xtrackfwres) {
> +                     ha->tgt.dfs_ini_iocbs =
> +                             debugfs_create_file("reserve_ini_iocbs",
> +                             0400, ha->dfs_dir, vha, &dfs_ini_iocbs_ops);
> +                     if (!ha->tgt.dfs_ini_iocbs) {
> +                             ql_log(ql_log_warn, vha, 0xd011,
> +                                 "Unable to create debugFS reserve_ini_iocbs 
> node.\n");
> +                             goto out;
> +                     }

The patch description shows how to control the new attributes through sysfs
but this code adds new debugfs attributes. Sorry but that really confuses me.

> +int ql2xtrackfwres;
> +module_param(ql2xtrackfwres, int, 0444);
> +MODULE_PARM_DESC(ql2xtrackfwres,
> +              "Track FW resource.  0(default): disabled");

Why is this a module parameter instead of e.g. a sysfs attribute? Module
parameters are annoying if the qla2xxx driver is not built as a module.

> @@ -6412,6 +6433,9 @@ qlt_enable_vha(struct scsi_qla_host *vha)
>               qla24xx_disable_vp(vha);
>               qla24xx_enable_vp(vha);
>       } else {
> +             if (ql2xtrackfwres && (IS_QLA83XX(ha) || IS_QLA27XX(ha)))
> +                     QLA_ENA_FW_RES_TRACKING(ha);

Why is there a dependency on the adapter type in the above code?

Thanks,

Bart.

Reply via email to