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.
