Hello qla2xxx maintainers,
I have some questions about the qla2xxx module after investigating something downstream
and finding the same issues upstream (there are 3 issues below).
The first issue is in qla2x00_get_data_rate() there are two code segments that are
identical:
int
qla2x00_get_data_rate(scsi_qla_host_t *vha) {
int rval;
mbx_cmd_t mc;
mbx_cmd_t *mcp = &mc;
struct qla_hw_data *ha = vha->hw;
ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x1106,
"Entered %s.\n", __func__);
if (!IS_FWI2_CAPABLE(ha))
return QLA_FUNCTION_FAILED;
mcp->mb[0] = MBC_DATA_RATE;
mcp->mb[1] = QLA_GET_DATA_RATE;
mcp->out_mb = MBX_1|MBX_0;
mcp->in_mb = MBX_2|MBX_1|MBX_0;
if (IS_QLA83XX(ha) || IS_QLA27XX(ha) || IS_QLA28XX(ha))
mcp->in_mb |= MBX_4|MBX_3;
mcp->tov = MBX_TOV_SECONDS;
mcp->flags = 0;
rval = qla2x00_mailbox_command(vha, mcp);
if (rval != QLA_SUCCESS) {
ql_dbg(ql_dbg_mbx, vha, 0x1107,
"Failed=%x mb[0]=%x.\n", rval, mcp->mb[0]);
} else {
if (mcp->mb[1] != 0x7) <<<<<<<<<<<<<
ha->link_data_rate = mcp->mb[1]; <<<<<<<<<<<<<
if (IS_QLA83XX(ha) || IS_QLA27XX(ha) || IS_QLA28XX(ha)) {
if (mcp->mb[4] & BIT_0)
ql_log(ql_log_info, vha, 0x11a2,
"FEC=enabled (data rate).\n");
}
ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x1108,
"Done %s.\n", __func__);
if (mcp->mb[1] != 0x7) <<<<<<<<<<<<<
ha->link_data_rate = mcp->mb[1]; <<<<<<<<<<<<<
}
return rval;
}
As far as I can determine nothing in between them should change link_data_rate.
This was introduced in the change 75666f4a8c41 ("scsi: qla2xxx: Display message
for FCE enabled") and you can see the start of the duplicated code. Was the second
instance of the if meant to be removed? It's also FEC not FCE.
In the message why use "(data rate)" when you could get the speed itself from
qla2x00_get_link_speed_str()?
The second issue is this function prints a message to the kernel message buffer but it
causes an issue in that qla2x00_get_data_rate() can be called from
qla2x00_configure_loop() and from here (that is fine for the former but not the later):
static ssize_t
qla2x00_port_speed_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct scsi_qla_host *vha = shost_priv(dev_to_shost(dev));
struct qla_hw_data *ha = vha->hw;
ssize_t rval;
u16 i;
char *speed = "Unknown";
rval = qla2x00_get_data_rate(vha);
if (rval != QLA_SUCCESS) {
ql_log(ql_log_warn, vha, 0x70db,
"Unable to get port speed rval:%zd\n", rval);
return -EINVAL;
}
for (i = 0; i < ARRAY_SIZE(port_speed_str); i++) {
if (port_speed_str[i].rate != ha->link_data_rate)
continue;
speed = port_speed_str[i].str;
break;
}
return scnprintf(buf, PAGE_SIZE, "%s\n", speed);
}
So every time some reads the port_speed file it can log a kernel message so if you
read the sysfs port_speed file a lot you can generate a lot of messages in the kernel
message buffer (messages have been changed manually):
2023-06-01T11:59:11.180034+02:00 example kernel: [2399544.415344][ XXXX] qla2xxx
[ 0000:61:00.1]-11a2:1: FEC=enabled (data rate).
2023-06-01T11:59:11.810042+02:00 example kernel: [2399544.415764][ XXXX] qla2xxx
[ 0000:61:00.1]-11a2:1: FEC=enabled (data rate).
Is that a defect? I wouldn't have expected reading a sysfs file to always generate
a kernel message into the kernel message buffer. The change that introduced the
message doesn't seem to have taken into account that function can be called from
two places.
Also, why isn't qla2x00_get_link_speed_str() used instead of adding
another structure containing almost identical speed data?
That leads onto the 3rd issue the really confusing ql_log function:
2572 ql_log(uint level, scsi_qla_host_t *vha, uint id, const char *fmt, ...)
2573 {
2574 va_list va;
2575 struct va_format vaf;
2576 char pbuf[128];
2577
2578 if (level > ql_errlev)
2579 return;
2580
2581 ql_ktrace(0, level, pbuf, NULL, vha, id, fmt);
2582
2583 if (!pbuf[0]) /* set by ql_ktrace */
2584 ql_dbg_prefix(pbuf, ARRAY_SIZE(pbuf), NULL, vha, id);
2585
2586 va_start(va, fmt);
2587
2588 vaf.fmt = fmt;
2589 vaf.va = &va;
2590
2591 switch (level) {
2592 case ql_log_fatal: /* FATAL LOG */
2593 pr_crit("%s%pV", pbuf, &vaf);
2594 break;
2595 case ql_log_warn: <<<<<<<<<<<<<<<<<<<<<<<<
2596 pr_err("%s%pV", pbuf, &vaf); <<<<<<<<<<<<<<<<<<<<<<<<
2597 break;
2598 case ql_log_info: <<<<<<<<<<<<<<<<<<<<<<<<
2599 pr_warn("%s%pV", pbuf, &vaf); <<<<<<<<<<<<<<<<<<<<<<<<
2600 break;
2601 default:
2602 pr_info("%s%pV", pbuf, &vaf);
2603 break;
2604 }
2605
2606 va_end(va);
2607 }
When I was looking at the driver I hadn't looked at ql_log() and was having issues suppressing
the kernel messages from issue #2 above. When I did look I noticed that ql_log_info messages
are printed with pr_warn() (so it's really a warning not info). Should the log levels used in the
qla2xxx driver more generally reflect the kernel usage of message levels (i.e. ql_log_info
should use pr_info, etc)?
Thanks
Shane
