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-owner@xxxxxxxxxxxxxxx [mailto:linux-scsi-owner@xxxxxxxxxxxxxxx] On Behalf Of Matthew R. Ochs > Sent: Monday, April 27, 2015 2:50 PM > To: linux-scsi@xxxxxxxxxxxxxxx; James.Bottomley@xxxxxxxxxxxxxxxxxxxxx; nab@xxxxxxxxxxxxxxx; brking@xxxxxxxxxxxxxxxxxx > Cc: mikey@xxxxxxxxxxx; imunsie@xxxxxxxxxxx; 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
