Shane, All great suggestions.
I've gone ahead and implemented these and also applied your comments to our other attributes as well. Will include in our next push of fixes. -matt On Jul 2, 2015, at 3:08 AM, Seymour, Shane M wrote: > -----Original Message----- > From: linux-scsi-ow...@vger.kernel.org > [mailto:linux-scsi-ow...@vger.kernel.org] On Behalf Of Matthew R. Ochs > Sent: Monday, April 27, 2015 2:50 PM > To: linux-scsi@vger.kernel.org; james.bottom...@hansenpartnership.com; > n...@linux-iscsi.org; brk...@linux.vnet.ibm.com > Cc: mi...@neuling.org; imun...@au1.ibm.com; Manoj N. Kumar > Subject: [PATCH RFC 1/2] cxlflash: Base support for IBM CXL Flash Adapter > >> +static ssize_t cxlflash_show_port_status(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct Scsi_Host *shost = class_to_shost(dev); >> + struct cxlflash *cxlflash = (struct cxlflash *)shost->hostdata; >> + struct afu *afu = cxlflash->afu; >> + >> + char *disp_status; >> + int rc; >> + u32 port; >> + u64 status; >> + volatile u64 *fc_regs; >> + >> + rc = kstrtouint((attr->attr.name + 4), 10, &port); >> + if (rc || (port > NUM_FC_PORTS)) >> + return 0; >> + >> + fc_regs = &afu->afu_map->global.fc_regs[port][0]; >> + status = >> + (readq_be(&fc_regs[FC_MTIP_STATUS / 8]) & FC_MTIP_STATUS_MASK); >> + >> + if (status == FC_MTIP_STATUS_ONLINE) >> + disp_status = "online"; >> + else if (status == FC_MTIP_STATUS_OFFLINE) >> + disp_status = "offline"; >> + else >> + disp_status = "unknown"; >> + >> + return snprintf(buf, PAGE_SIZE, "%s\n", disp_status); > > You need to use scnprintf() instead of snprintf() here > >> +static ssize_t cxlflash_show_lun_mode(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct Scsi_Host *shost = class_to_shost(dev); >> + struct cxlflash *cxlflash = (struct cxlflash *)shost->hostdata; >> + struct afu *afu = cxlflash->afu; >> + >> + return snprintf(buf, PAGE_SIZE, "%u\n", afu->internal_lun); > > See above about snprintf() > > +static DEVICE_ATTR(port0, S_IRUGO, cxlflash_show_port_status, NULL); > +static DEVICE_ATTR(port1, S_IRUGO, cxlflash_show_port_status, NULL); > +static DEVICE_ATTR(lun_mode, S_IRUGO | S_IWUSR, cxlflash_show_lun_mode, > + cxlflash_store_lun_mode); > > Please use DEVICE_ATTR_RO and DEVICE_ATTR_RW as appropriate if you can, you > will need to change the show/store function names. The main reason I know for > doing this is (using DEVICE_ATTR_RO as an example) if someone sees a sysfs > attribute called port0 or port1 they should be able to search the kernel > source to find a function called port0_show or port1_show without having to > spend any extra time searching to find out what the driver is and then look > at the driver source to find that they need to look at > cxlflash_show_port_status. Using DEVICE_ATTR_RO for port0 and port1 would > probably change the code to looking something like this: > > static ssize_t cxlflash_show_port_status(u32 port, > struct afu *afu, char *buf) > { > char *disp_status; > u64 status; > volatile u64 *fc_regs; > > fc_regs = &afu->afu_map->global.fc_regs[port][0]; > /* I split this line into two because I had to really look at where > * the brackets were and this made it more obvious to me > * what it was doing but that could just be me. */ > status = readq_be(&fc_regs[FC_MTIP_STATUS / 8]); > status &= FC_MTIP_STATUS_MASK; > > if (status == FC_MTIP_STATUS_ONLINE) > disp_status = "online"; > else if (status == FC_MTIP_STATUS_OFFLINE) > disp_status = "offline"; > else > disp_status = "unknown"; > > return scnprintf(buf, PAGE_SIZE, "%s\n", disp_status); > } > > Since you have fixed values you could use also sprintf here (although the > documentation currently says to only use scnprintf) and that would make > port0_show and port1_show to be: > > static ssize_t port0_show(struct device *dev, > struct device_attribute *attr, > char *buf) > { > struct Scsi_Host *shost = class_to_shost(dev); > struct cxlflash *cxlflash = (struct cxlflash *)shost->hostdata; > struct afu *afu = cxlflash->afu; > > return cxlflash_show_port_status(0, afu, buf); > } > > static ssize_t port1_show(struct device *dev, > struct device_attribute *attr, > char *buf) > { > struct Scsi_Host *shost = class_to_shost(dev); > struct cxlflash *cxlflash = (struct cxlflash *)shost->hostdata; > struct afu *afu = cxlflash->afu; > > return cxlflash_show_port_status(1, afu, buf); > } > > I'm assuming that 0 and 1 are always valid for port number and don't need to > be checked against NUM_FC_PORTS. That's just a suggestion and I haven't > attempted to compile the above example and you'd need to test it to make sure > that they still work as expected. > > Shane > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
