| Message ID | 20190213185324.21652-7-hmadhani@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qla2xxx: Misc bug fixes for the driver | expand |
On Wed, 2019-02-13 at 10:53 -0800, Himanshu Madhani wrote: > static ssize_t > +qla2x00_sysfs_set_port_speed(struct file *filp, struct kobject *kobj, > + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) > +{ > + struct scsi_qla_host *vha = shost_priv(dev_to_shost(container_of(kobj, > + struct device, kobj))); > + int type; > + int mode = QLA_SET_DATA_RATE_LR; > + int rval; > + struct qla_hw_data *ha = vha->hw; > + int speed, oldspeed; > + > + if (!IS_QLA27XX(vha->hw)) { > + ql_log(ql_log_warn, vha, 0x70d8, > + "Speed setting not supported \n"); > + return -EINVAL; > + } > + > + speed = type = simple_strtol(buf, NULL, 10); > + if (type == 40 || type == 80 || type == 160 || > + type == 320) { > + ql_log(ql_log_warn, vha, 0x70d9, > + "Setting will be affected after a loss of sync\n"); > + type = type/10; > + mode = QLA_SET_DATA_RATE_NOLR; > + } Anil, you are supposed to use checkpatch before submitting a patch. Checkpatch complains about the above code: WARNING: simple_strtol is obsolete, use kstrtol instead #197: FILE: drivers/scsi/qla2xxx/qla_attr.c:651: + speed = type = simple_strtol(buf, NULL, 10); > + oldspeed = ha->set_data_rate; > + > + switch (type) { > + case 0: > + ha->set_data_rate = PORT_SPEED_AUTO; > + break; > + case 4: > + ha->set_data_rate = PORT_SPEED_4GB; > + break; > + case 8: > + ha->set_data_rate = PORT_SPEED_8GB; > + break; > + case 16: > + ha->set_data_rate = PORT_SPEED_16GB; > + break; > + case 32: > + ha->set_data_rate = PORT_SPEED_32GB; > + break; > + default: > + ql_log(ql_log_warn, vha, 0x1199, > + "Unrecognized speed setting:%d. Setting Autoneg\n", > + speed); > + ha->set_data_rate = PORT_SPEED_AUTO; > + } The default case is weird. Most sysfs store methods do not change any settings if an invalid input parameter has been supplied. > + > + if (qla2x00_chip_is_down(vha) || (oldspeed == ha->set_data_rate)) > + return count; Wouldn't -EAGAIN be a better return value in this case? > +static ssize_t > +qla2x00_sysfs_get_port_speed(struct file *filp, struct kobject *kobj, > + struct bin_attribute *bin_attr, > + char *buf, loff_t off, size_t count) > +{ > + struct scsi_qla_host *vha = shost_priv(dev_to_shost(container_of(kobj, > + struct device, kobj))); > + struct qla_hw_data *ha = vha->hw; > + ssize_t rval; > + char *spd[7] = {"0", "0", "0", "4", "8", "16", "32"}; This should be a "static const char *" array. > + ql_log(ql_log_info, vha, 0x70d6, > + "port speed:%d\n", ha->link_data_rate); This looks like a debug statement. Log level "debug" is probably more appropriate. > +static struct bin_attribute sysfs_port_speed_attr = { > + .attr = { > + .name = "port_speed", > + .mode = 0600, > + }, > + .size = 16, > + .write = qla2x00_sysfs_set_port_speed, > + .read = qla2x00_sysfs_get_port_speed, > +}; This needs an explanation of why you choose to make this a binary attribute. And if you don't have a very good reason to make this attribute a binary attribute, it should be changed into a regular attribute. > +/* Set the specified data rate */ > +int > +qla2x00_set_data_rate(scsi_qla_host_t *vha, uint16_t mode) > +{ > + int rval; > + mbx_cmd_t mc; > + mbx_cmd_t *mcp = &mc; > + struct qla_hw_data *ha = vha->hw; > + uint16_t val; > + > + ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x1106, > + "Entered %s speed:0x%x mode:0x%x.\n", __func__, ha->set_data_rate, > + mode); > + > + if (!IS_FWI2_CAPABLE(ha)) > + return QLA_FUNCTION_FAILED; > + > + memset(mcp, 0, sizeof(mbx_cmd_t)); Please change sizeof(mbx_cmd_t) into sizeof(*mcp). Thanks, Bart.
Hi Bart,
On 2/13/19, 4:55 PM, "Bart Van Assche" <bvanassche@acm.org> wrote:
On Wed, 2019-02-13 at 10:53 -0800, Himanshu Madhani wrote:
> static ssize_t
> +qla2x00_sysfs_set_port_speed(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count)
> +{
> + struct scsi_qla_host *vha = shost_priv(dev_to_shost(container_of(kobj,
> + struct device, kobj)));
> + int type;
> + int mode = QLA_SET_DATA_RATE_LR;
> + int rval;
> + struct qla_hw_data *ha = vha->hw;
> + int speed, oldspeed;
> +
> + if (!IS_QLA27XX(vha->hw)) {
> + ql_log(ql_log_warn, vha, 0x70d8,
> + "Speed setting not supported \n");
> + return -EINVAL;
> + }
> +
> + speed = type = simple_strtol(buf, NULL, 10);
> + if (type == 40 || type == 80 || type == 160 ||
> + type == 320) {
> + ql_log(ql_log_warn, vha, 0x70d9,
> + "Setting will be affected after a loss of sync\n");
> + type = type/10;
> + mode = QLA_SET_DATA_RATE_NOLR;
> + }
Anil, you are supposed to use checkpatch before submitting a patch. Checkpatch
complains about the above code:
WARNING: simple_strtol is obsolete, use kstrtol instead
#197: FILE: drivers/scsi/qla2xxx/qla_attr.c:651:
+ speed = type = simple_strtol(buf, NULL, 10);
This Warning got missed in my checkpatch testing as well. Will fix up and repost this patch.
> + oldspeed = ha->set_data_rate;
> +
> + switch (type) {
> + case 0:
> + ha->set_data_rate = PORT_SPEED_AUTO;
> + break;
> + case 4:
> + ha->set_data_rate = PORT_SPEED_4GB;
> + break;
> + case 8:
> + ha->set_data_rate = PORT_SPEED_8GB;
> + break;
> + case 16:
> + ha->set_data_rate = PORT_SPEED_16GB;
> + break;
> + case 32:
> + ha->set_data_rate = PORT_SPEED_32GB;
> + break;
> + default:
> + ql_log(ql_log_warn, vha, 0x1199,
> + "Unrecognized speed setting:%d. Setting Autoneg\n",
> + speed);
> + ha->set_data_rate = PORT_SPEED_AUTO;
> + }
The default case is weird. Most sysfs store methods do not change any settings
if an invalid input parameter has been supplied.
We do want to set data rate to Auto in case we get as default, if we don’t recognize user input.
> +
> + if (qla2x00_chip_is_down(vha) || (oldspeed == ha->set_data_rate))
> + return count;
Wouldn't -EAGAIN be a better return value in this case?
Agree. Will fix this up
> +static ssize_t
> +qla2x00_sysfs_get_port_speed(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr,
> + char *buf, loff_t off, size_t count)
> +{
> + struct scsi_qla_host *vha = shost_priv(dev_to_shost(container_of(kobj,
> + struct device, kobj)));
> + struct qla_hw_data *ha = vha->hw;
> + ssize_t rval;
> + char *spd[7] = {"0", "0", "0", "4", "8", "16", "32"};
This should be a "static const char *" array.
Yes. Will update patch
> + ql_log(ql_log_info, vha, 0x70d6,
> + "port speed:%d\n", ha->link_data_rate);
This looks like a debug statement. Log level "debug" is probably more appropriate.
Yes. Will update
> +static struct bin_attribute sysfs_port_speed_attr = {
> + .attr = {
> + .name = "port_speed",
> + .mode = 0600,
> + },
> + .size = 16,
> + .write = qla2x00_sysfs_set_port_speed,
> + .read = qla2x00_sysfs_get_port_speed,
> +};
This needs an explanation of why you choose to make this a binary attribute. And if
you don't have a very good reason to make this attribute a binary attribute, it should
be changed into a regular attribute.
It does look like there is no specific reason for this attribute to be binary. Will update it to use regular attribute and post revised patch
> +/* Set the specified data rate */
> +int
> +qla2x00_set_data_rate(scsi_qla_host_t *vha, uint16_t mode)
> +{
> + int rval;
> + mbx_cmd_t mc;
> + mbx_cmd_t *mcp = &mc;
> + struct qla_hw_data *ha = vha->hw;
> + uint16_t val;
> +
> + ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x1106,
> + "Entered %s speed:0x%x mode:0x%x.\n", __func__, ha->set_data_rate,
> + mode);
> +
> + if (!IS_FWI2_CAPABLE(ha))
> + return QLA_FUNCTION_FAILED;
> +
> + memset(mcp, 0, sizeof(mbx_cmd_t));
Please change sizeof(mbx_cmd_t) into sizeof(*mcp).
Will Do.
Thanks,
Bart.
Thanks,
-- Himanshu
diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c index 8b4dd72011bf..dcdfcf3bc955 100644 --- a/drivers/scsi/qla2xxx/qla_attr.c +++ b/drivers/scsi/qla2xxx/qla_attr.c @@ -631,6 +631,108 @@ static struct bin_attribute sysfs_sfp_attr = { }; static ssize_t +qla2x00_sysfs_set_port_speed(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) +{ + struct scsi_qla_host *vha = shost_priv(dev_to_shost(container_of(kobj, + struct device, kobj))); + int type; + int mode = QLA_SET_DATA_RATE_LR; + int rval; + struct qla_hw_data *ha = vha->hw; + int speed, oldspeed; + + if (!IS_QLA27XX(vha->hw)) { + ql_log(ql_log_warn, vha, 0x70d8, + "Speed setting not supported \n"); + return -EINVAL; + } + + speed = type = simple_strtol(buf, NULL, 10); + if (type == 40 || type == 80 || type == 160 || + type == 320) { + ql_log(ql_log_warn, vha, 0x70d9, + "Setting will be affected after a loss of sync\n"); + type = type/10; + mode = QLA_SET_DATA_RATE_NOLR; + } + + oldspeed = ha->set_data_rate; + + switch (type) { + case 0: + ha->set_data_rate = PORT_SPEED_AUTO; + break; + case 4: + ha->set_data_rate = PORT_SPEED_4GB; + break; + case 8: + ha->set_data_rate = PORT_SPEED_8GB; + break; + case 16: + ha->set_data_rate = PORT_SPEED_16GB; + break; + case 32: + ha->set_data_rate = PORT_SPEED_32GB; + break; + default: + ql_log(ql_log_warn, vha, 0x1199, + "Unrecognized speed setting:%d. Setting Autoneg\n", + speed); + ha->set_data_rate = PORT_SPEED_AUTO; + } + + if (qla2x00_chip_is_down(vha) || (oldspeed == ha->set_data_rate)) + return count; + + ql_log(ql_log_info, vha, 0x70da, + "Setting speed to %d Gbps \n", type); + + rval = qla2x00_set_data_rate(vha, mode); + if (rval != QLA_SUCCESS) + return -EIO; + + return count; +} + +static ssize_t +qla2x00_sysfs_get_port_speed(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, + char *buf, loff_t off, size_t count) +{ + struct scsi_qla_host *vha = shost_priv(dev_to_shost(container_of(kobj, + struct device, kobj))); + struct qla_hw_data *ha = vha->hw; + ssize_t rval; + char *spd[7] = {"0", "0", "0", "4", "8", "16", "32"}; + + 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; + } + + ql_log(ql_log_info, vha, 0x70d6, + "port speed:%d\n", ha->link_data_rate); + + rval = memory_read_from_buffer(buf, count, + &off, spd[ha->link_data_rate], sizeof(ha->link_data_rate)); + + return rval; +} + +static struct bin_attribute sysfs_port_speed_attr = { + .attr = { + .name = "port_speed", + .mode = 0600, + }, + .size = 16, + .write = qla2x00_sysfs_set_port_speed, + .read = qla2x00_sysfs_get_port_speed, +}; + +static ssize_t qla2x00_sysfs_write_reset(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) @@ -943,6 +1045,7 @@ static struct sysfs_entry { { "issue_logo", &sysfs_issue_logo_attr, }, { "xgmac_stats", &sysfs_xgmac_stats_attr, 3 }, { "dcbx_tlv", &sysfs_dcbx_tlv_attr, 3 }, + { "port_speed", &sysfs_port_speed_attr, }, { NULL }, }; diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h index c256ba7fba84..c0f7593666a1 100644 --- a/drivers/scsi/qla2xxx/qla_def.h +++ b/drivers/scsi/qla2xxx/qla_def.h @@ -3698,12 +3698,14 @@ struct qla_hw_data { #define PORT_SPEED_UNKNOWN 0xFFFF #define PORT_SPEED_1GB 0x00 #define PORT_SPEED_2GB 0x01 +#define PORT_SPEED_AUTO 0x02 #define PORT_SPEED_4GB 0x03 #define PORT_SPEED_8GB 0x04 #define PORT_SPEED_16GB 0x05 #define PORT_SPEED_32GB 0x06 #define PORT_SPEED_10GB 0x13 uint16_t link_data_rate; /* F/W operating speed */ + uint16_t set_data_rate; /* Set by user */ uint8_t current_topology; uint8_t prev_topology; @@ -4232,6 +4234,10 @@ struct qla_hw_data { #define FW_ABILITY_MAX_SPEED(ha) \ (ha->fw_ability_mask & FW_ABILITY_MAX_SPEED_MASK) +#define QLA_GET_DATA_RATE 0 +#define QLA_SET_DATA_RATE_NOLR 1 +#define QLA_SET_DATA_RATE_LR 2 /* Set speed and initiate LR */ + /* * Qlogic scsi host structure */ diff --git a/drivers/scsi/qla2xxx/qla_gbl.h b/drivers/scsi/qla2xxx/qla_gbl.h index bcc17a7261e7..3c59006e64bb 100644 --- a/drivers/scsi/qla2xxx/qla_gbl.h +++ b/drivers/scsi/qla2xxx/qla_gbl.h @@ -899,5 +899,6 @@ void qlt_update_host_map(struct scsi_qla_host *, port_id_t); void qlt_remove_target_resources(struct qla_hw_data *); void qlt_clr_qp_table(struct scsi_qla_host *vha); void qlt_set_mode(struct scsi_qla_host *); +int qla2x00_set_data_rate(scsi_qla_host_t *vha, uint16_t mode); #endif /* _QLA_GBL_H */ diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c index ba5da365ee4a..bb9bccb734f8 100644 --- a/drivers/scsi/qla2xxx/qla_init.c +++ b/drivers/scsi/qla2xxx/qla_init.c @@ -3880,8 +3880,17 @@ qla24xx_config_rings(struct scsi_qla_host *vha) WRT_REG_DWORD(®->isp24.rsp_q_in, 0); WRT_REG_DWORD(®->isp24.rsp_q_out, 0); } + qlt_24xx_config_rings(vha); + /* If the user has configured the speed, set it here */ + if (ha->set_data_rate) { + ql_dbg(ql_dbg_init, vha, 0x00fd, + "Speed set by user : %s Gbps \n", + qla2x00_get_link_speed_str(ha, ha->set_data_rate)); + icb->firmware_options_3 = (ha->set_data_rate << 13); + } + /* PCI posting */ RD_REG_DWORD(&ioreg->hccr); } diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c index 6c911f2e4cdb..913dc36c48bc 100644 --- a/drivers/scsi/qla2xxx/qla_mbx.c +++ b/drivers/scsi/qla2xxx/qla_mbx.c @@ -5250,6 +5250,66 @@ qla81xx_write_mpi_register(scsi_qla_host_t *vha, uint16_t *mb) return rval; } +/* Set the specified data rate */ +int +qla2x00_set_data_rate(scsi_qla_host_t *vha, uint16_t mode) +{ + int rval; + mbx_cmd_t mc; + mbx_cmd_t *mcp = &mc; + struct qla_hw_data *ha = vha->hw; + uint16_t val; + + ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x1106, + "Entered %s speed:0x%x mode:0x%x.\n", __func__, ha->set_data_rate, + mode); + + if (!IS_FWI2_CAPABLE(ha)) + return QLA_FUNCTION_FAILED; + + memset(mcp, 0, sizeof(mbx_cmd_t)); + switch (ha->set_data_rate) { + case PORT_SPEED_AUTO: + case PORT_SPEED_4GB: + case PORT_SPEED_8GB: + case PORT_SPEED_16GB: + case PORT_SPEED_32GB: + val = ha->set_data_rate; + break; + default: + ql_log(ql_log_warn, vha, 0x1199, + "Unrecognized speed setting:%d. Setting Autoneg\n", + ha->set_data_rate); + val = ha->set_data_rate = PORT_SPEED_AUTO; + break; + } + + mcp->mb[0] = MBC_DATA_RATE; + mcp->mb[1] = mode; + mcp->mb[2] = val; + + mcp->out_mb = MBX_2|MBX_1|MBX_0; + mcp->in_mb = MBX_2|MBX_1|MBX_0; + if (IS_QLA83XX(ha) || IS_QLA27XX(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) + ql_dbg(ql_dbg_mbx, vha, 0x1179, + "Speed set:0x%x\n", mcp->mb[1]); + + ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x1108, + "Done %s.\n", __func__); + } + + return rval; +} + int qla2x00_get_data_rate(scsi_qla_host_t *vha) { @@ -5265,7 +5325,7 @@ qla2x00_get_data_rate(scsi_qla_host_t *vha) return QLA_FUNCTION_FAILED; mcp->mb[0] = MBC_DATA_RATE; - mcp->mb[1] = 0; + 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))