On Wed, 2019-02-13 at 10:53 -0800, Himanshu Madhani wrote: +AD4 static ssize+AF8-t +AD4 +-qla2x00+AF8-sysfs+AF8-set+AF8-port+AF8-speed(struct file +ACo-filp, struct kobject +ACo-kobj, +AD4 +- struct bin+AF8-attribute +ACo-bin+AF8-attr, char +ACo-buf, loff+AF8-t off, size+AF8-t count) +AD4 +-+AHs +AD4 +- struct scsi+AF8-qla+AF8-host +ACo-vha +AD0 shost+AF8-priv(dev+AF8-to+AF8-shost(container+AF8-of(kobj, +AD4 +- struct device, kobj)))+ADs +AD4 +- int type+ADs +AD4 +- int mode +AD0 QLA+AF8-SET+AF8-DATA+AF8-RATE+AF8-LR+ADs +AD4 +- int rval+ADs +AD4 +- struct qla+AF8-hw+AF8-data +ACo-ha +AD0 vha-+AD4-hw+ADs +AD4 +- int speed, oldspeed+ADs +AD4 +- +AD4 +- if (+ACE-IS+AF8-QLA27XX(vha-+AD4-hw)) +AHs +AD4 +- ql+AF8-log(ql+AF8-log+AF8-warn, vha, 0x70d8, +AD4 +- +ACI-Speed setting not supported +AFw-n+ACI)+ADs +AD4 +- return -EINVAL+ADs +AD4 +- +AH0 +AD4 +- +AD4 +- speed +AD0 type +AD0 simple+AF8-strtol(buf, NULL, 10)+ADs +AD4 +- if (type +AD0APQ 40 +AHwAfA type +AD0APQ 80 +AHwAfA type +AD0APQ 160 +AHwAfA +AD4 +- type +AD0APQ 320) +AHs +AD4 +- ql+AF8-log(ql+AF8-log+AF8-warn, vha, 0x70d9, +AD4 +- +ACI-Setting will be affected after a loss of sync+AFw-n+ACI)+ADs +AD4 +- type +AD0 type/10+ADs +AD4 +- mode +AD0 QLA+AF8-SET+AF8-DATA+AF8-RATE+AF8-NOLR+ADs +AD4 +- +AH0 Anil, you are supposed to use checkpatch before submitting a patch. Checkpatch complains about the above code: WARNING: simple+AF8-strtol is obsolete, use kstrtol instead +ACM-197: FILE: drivers/scsi/qla2xxx/qla+AF8-attr.c:651: +- speed +AD0 type +AD0 simple+AF8-strtol(buf, NULL, 10)+ADs +AD4 +- oldspeed +AD0 ha-+AD4-set+AF8-data+AF8-rate+ADs +AD4 +- +AD4 +- switch (type) +AHs +AD4 +- case 0: +AD4 +- ha-+AD4-set+AF8-data+AF8-rate +AD0 PORT+AF8-SPEED+AF8-AUTO+ADs +AD4 +- break+ADs +AD4 +- case 4: +AD4 +- ha-+AD4-set+AF8-data+AF8-rate +AD0 PORT+AF8-SPEED+AF8-4GB+ADs +AD4 +- break+ADs +AD4 +- case 8: +AD4 +- ha-+AD4-set+AF8-data+AF8-rate +AD0 PORT+AF8-SPEED+AF8-8GB+ADs +AD4 +- break+ADs +AD4 +- case 16: +AD4 +- ha-+AD4-set+AF8-data+AF8-rate +AD0 PORT+AF8-SPEED+AF8-16GB+ADs +AD4 +- break+ADs +AD4 +- case 32: +AD4 +- ha-+AD4-set+AF8-data+AF8-rate +AD0 PORT+AF8-SPEED+AF8-32GB+ADs +AD4 +- break+ADs +AD4 +- default: +AD4 +- ql+AF8-log(ql+AF8-log+AF8-warn, vha, 0x1199, +AD4 +- +ACI-Unrecognized speed setting:+ACU-d. Setting Autoneg+AFw-n+ACI, +AD4 +- speed)+ADs +AD4 +- ha-+AD4-set+AF8-data+AF8-rate +AD0 PORT+AF8-SPEED+AF8-AUTO+ADs +AD4 +- +AH0 The default case is weird. Most sysfs store methods do not change any settings if an invalid input parameter has been supplied. +AD4 +- +AD4 +- if (qla2x00+AF8-chip+AF8-is+AF8-down(vha) +AHwAfA (oldspeed +AD0APQ ha-+AD4-set+AF8-data+AF8-rate)) +AD4 +- return count+ADs Wouldn't -EAGAIN be a better return value in this case? +AD4 +-static ssize+AF8-t +AD4 +-qla2x00+AF8-sysfs+AF8-get+AF8-port+AF8-speed(struct file +ACo-filp, struct kobject +ACo-kobj, +AD4 +- struct bin+AF8-attribute +ACo-bin+AF8-attr, +AD4 +- char +ACo-buf, loff+AF8-t off, size+AF8-t count) +AD4 +-+AHs +AD4 +- struct scsi+AF8-qla+AF8-host +ACo-vha +AD0 shost+AF8-priv(dev+AF8-to+AF8-shost(container+AF8-of(kobj, +AD4 +- struct device, kobj)))+ADs +AD4 +- struct qla+AF8-hw+AF8-data +ACo-ha +AD0 vha-+AD4-hw+ADs +AD4 +- ssize+AF8-t rval+ADs +AD4 +- char +ACo-spd+AFs-7+AF0 +AD0 +AHsAIg-0+ACI, +ACI-0+ACI, +ACI-0+ACI, +ACI-4+ACI, +ACI-8+ACI, +ACI-16+ACI, +ACI-32+ACIAfQA7 This should be a +ACI-static const char +ACoAIg array. +AD4 +- ql+AF8-log(ql+AF8-log+AF8-info, vha, 0x70d6, +AD4 +- +ACI-port speed:+ACU-d+AFw-n+ACI, ha-+AD4-link+AF8-data+AF8-rate)+ADs This looks like a debug statement. Log level +ACI-debug+ACI is probably more appropriate. +AD4 +-static struct bin+AF8-attribute sysfs+AF8-port+AF8-speed+AF8-attr +AD0 +AHs +AD4 +- .attr +AD0 +AHs +AD4 +- .name +AD0 +ACI-port+AF8-speed+ACI, +AD4 +- .mode +AD0 0600, +AD4 +- +AH0, +AD4 +- .size +AD0 16, +AD4 +- .write +AD0 qla2x00+AF8-sysfs+AF8-set+AF8-port+AF8-speed, +AD4 +- .read +AD0 qla2x00+AF8-sysfs+AF8-get+AF8-port+AF8-speed, +AD4 +-+AH0AOw 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. +AD4 +-/+ACo Set the specified data rate +ACo-/ +AD4 +-int +AD4 +-qla2x00+AF8-set+AF8-data+AF8-rate(scsi+AF8-qla+AF8-host+AF8-t +ACo-vha, uint16+AF8-t mode) +AD4 +-+AHs +AD4 +- int rval+ADs +AD4 +- mbx+AF8-cmd+AF8-t mc+ADs +AD4 +- mbx+AF8-cmd+AF8-t +ACo-mcp +AD0 +ACY-mc+ADs +AD4 +- struct qla+AF8-hw+AF8-data +ACo-ha +AD0 vha-+AD4-hw+ADs +AD4 +- uint16+AF8-t val+ADs +AD4 +- +AD4 +- ql+AF8-dbg(ql+AF8-dbg+AF8-mbx +- ql+AF8-dbg+AF8-verbose, vha, 0x1106, +AD4 +- +ACI-Entered +ACU-s speed:0x+ACU-x mode:0x+ACU-x.+AFw-n+ACI, +AF8AXw-func+AF8AXw, ha-+AD4-set+AF8-data+AF8-rate, +AD4 +- mode)+ADs +AD4 +- +AD4 +- if (+ACE-IS+AF8-FWI2+AF8-CAPABLE(ha)) +AD4 +- return QLA+AF8-FUNCTION+AF8-FAILED+ADs +AD4 +- +AD4 +- memset(mcp, 0, sizeof(mbx+AF8-cmd+AF8-t))+ADs Please change sizeof(mbx+AF8-cmd+AF8-t) into sizeof(+ACo-mcp). Thanks, Bart.
