RE: [PATCH RFC 1/2] cxlflash: Base support for IBM CXL Flash Adapter
-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-inf
Re: [PATCH RFC 1/2] cxlflash: Base support for IBM CXL Flash Adapter
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_POR
Re: [PATCH RFC 1/2] cxlflash: Base support for IBM CXL Flash Adapter
Christoph, > On Sun, Apr 26, 2015 at 11:50:20PM -0500, Matthew R. Ochs wrote: > > SCSI device driver to support filesystem access on the IBM CXL Flash > > adapter. > > Please drop all the code related to your weird direct access ioctls > from this patch. Without that it looks mostly mergeable. What's your issue with this? Do you just not like direct access at all, or the way it's been done? The CAPI hardware allows devices to have direct access to userspace memory via an MMU. What this driver is doing is enabling this direct access so the flash controller can share memory (like queues) directly with userspace without the kernel overhead. Mikey -- 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