> On Dec 20, 2017, at 10:51 AM, Bart Van Assche <bart.vanassche@xxxxxxx> wrote:
>
> 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)?
>
Checkpatch did not suggest to use do/while loop.
I did however saw indentation warning which i did not change since it looked like false positive
WARNING: suspect code indent for conditional statements (16, 16)
#133: FILE: drivers/scsi/qla2xxx/qla_def.h:4460:
+ if (_ha->queue_pair_map[i]) \
+ _ha->queue_pair_map[i]->fw_res_tracking = 1; \
WARNING: suspect code indent for conditional statements (16, 16)
#142: FILE: drivers/scsi/qla2xxx/qla_def.h:4469:
+ if (_ha->queue_pair_map[i]) \
+ _ha->queue_pair_map[i]->fw_res_tracking = 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.
>
Looks like sysfs example is used instead of DebugFS in the commit message
>> @@ -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?
>
We do not want to support this feature on 8G and older adapters.
> Thanks,
>
> Bart.
Thanks,
- Himanshu
