Hi Bart,
On 2/13/19, 4:55 PM, "Bart Van Assche" <bvanassche@xxxxxxx> 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
