On 07/27/2016 07:30 PM, Brian King wrote: > Heitor, > > Very nice patch. A few comments below. > > Thanks! > > Brian >
Thank you for the review, Brian! Some quick questions below. > On 07/27/2016 09:39 AM, Heitor Ricardo Alves de Siqueira wrote: >> This patch implements functions for pushing HCAM (host controlled >> asynchronous messages) error buffers to userspace through sysfs attributes. >> Reads to the "async_err_log" attribute will result in a single HCAM buffer >> being copied to userspace; one can process the next HCAM buffer by writing >> any string to the same attribute. >> >> A new list was added to the ioa_cfg structure to store the HCAM buffers for >> later reporting. Another read-only sysfs attribute ("current_hcam_count") >> indicates how many buffers are being held by this report queue. We also >> send a KOBJ_CHANGE event whenever a new HCAM buffer is made available to >> userspace. >> >> Signed-off-by: Heitor Ricardo Alves de Siqueira <hal...@linux.vnet.ibm.com> >> Signed-off-by: Gabriel Krisman Bertazi <kris...@linux.vnet.ibm.com> >> --- >> drivers/scsi/ipr.c | 168 >> +++++++++++++++++++++++++++++++++++++++++++++++++---- >> drivers/scsi/ipr.h | 7 ++- >> 2 files changed, 161 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c >> index d6803a9e5ab8..bdd36f6d1569 100644 >> --- a/drivers/scsi/ipr.c >> +++ b/drivers/scsi/ipr.c >> @@ -1472,7 +1472,7 @@ static void ipr_process_ccn(struct ipr_cmnd *ipr_cmd) >> struct ipr_hostrcb *hostrcb = ipr_cmd->u.hostrcb; >> u32 ioasc = be32_to_cpu(ipr_cmd->s.ioasa.hdr.ioasc); >> >> - list_del(&hostrcb->queue); >> + list_del_init(&hostrcb->queue); >> list_add_tail(&ipr_cmd->queue, &ipr_cmd->hrrq->hrrq_free_q); >> >> if (ioasc) { >> @@ -2551,6 +2551,41 @@ static void ipr_handle_log_data(struct ipr_ioa_cfg >> *ioa_cfg, >> } >> } >> >> +static struct ipr_hostrcb *ipr_get_free_hostrcb(struct ipr_ioa_cfg *ioa) >> +{ >> + struct ipr_hostrcb *hostrcb; >> + >> + hostrcb = list_first_entry_or_null(&ioa->hostrcb_free_q, >> + struct ipr_hostrcb, queue); >> + >> + if (unlikely(!hostrcb)) { >> + dev_err(&ioa->pdev->dev, >> + "hcam pool exausted. Reclaiming reserved >> hostrcb\n"); > > Suggest rewording to: "Reclaiming async error buffers." Also change to > dev_info. > >> + >> + hostrcb = list_first_entry_or_null(&ioa->hostrcb_report_q, >> + struct ipr_hostrcb, queue); >> + } >> + >> + list_del_init(&hostrcb->queue); >> + return hostrcb; >> +} >> + >> +static int ipr_report_error(struct ipr_ioa_cfg *ioa, >> + struct ipr_hostrcb *hostrcb) >> +{ >> + u32 ioasc; >> + struct ipr_hcam *hcam = &hostrcb->hcam; >> + struct ipr_error_table_t hcam_error_entry; >> + >> + if (ioa->sis64) >> + ioasc = be32_to_cpu(hcam->u.error64.fd_ioasc); >> + else >> + ioasc = be32_to_cpu(hcam->u.error.fd_ioasc); >> + >> + hcam_error_entry = ipr_error_table[ipr_get_error(ioasc)]; >> + return hcam_error_entry.log_hcam >= ioa->log_level; > > Let's remove this function altogether and ship everything out to > userspace and let userspace do any filtering it desires rather > than overloading the log_level, which currently helps filter what > goes to dmesg. > >> +} >> + >> /** >> * ipr_process_error - Op done function for an adapter error log. >> * @ipr_cmd: ipr command struct >> @@ -2568,13 +2603,14 @@ static void ipr_process_error(struct ipr_cmnd >> *ipr_cmd) >> struct ipr_hostrcb *hostrcb = ipr_cmd->u.hostrcb; >> u32 ioasc = be32_to_cpu(ipr_cmd->s.ioasa.hdr.ioasc); >> u32 fd_ioasc; >> + static const char * const env[] = { "ASYNC_ERR_LOG=1", NULL}; >> >> if (ioa_cfg->sis64) >> fd_ioasc = be32_to_cpu(hostrcb->hcam.u.error64.fd_ioasc); >> else >> fd_ioasc = be32_to_cpu(hostrcb->hcam.u.error.fd_ioasc); >> >> - list_del(&hostrcb->queue); >> + list_del_init(&hostrcb->queue); >> list_add_tail(&ipr_cmd->queue, &ipr_cmd->hrrq->hrrq_free_q); >> >> if (!ioasc) { >> @@ -2587,6 +2623,13 @@ static void ipr_process_error(struct ipr_cmnd >> *ipr_cmd) >> "Host RCB failed with IOASC: 0x%08X\n", ioasc); >> } >> >> + if (ipr_report_error(ioa_cfg, hostrcb)) { >> + list_add_tail(&hostrcb->queue, &ioa_cfg->hostrcb_report_q); >> + hostrcb = ipr_get_free_hostrcb(ioa_cfg); >> + kobject_uevent_env(&ioa_cfg->host->shost_dev.kobj, >> + KOBJ_CHANGE, env); >> + } >> + >> ipr_send_hcam(ioa_cfg, IPR_HCAM_CDB_OP_CODE_LOG_DATA, hostrcb); >> } >> >> @@ -4089,6 +4132,89 @@ static struct device_attribute ipr_ioa_fw_type_attr = >> { >> .show = ipr_show_fw_type >> }; >> >> +static ssize_t ipr_read_async_err_log(struct file *filep, struct kobject >> *kobj, >> + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) > > Indentation here looks a little strange. > >> +{ >> + struct device *cdev = container_of(kobj, struct device, kobj); >> + struct Scsi_Host *shost = class_to_shost(cdev); >> + struct ipr_ioa_cfg *ioa_cfg = (struct ipr_ioa_cfg *)shost->hostdata; >> + struct ipr_hostrcb *hostrcb; >> + unsigned long lock_flags = 0; >> + int ret; >> + >> + spin_lock_irqsave(ioa_cfg->host->host_lock, lock_flags); >> + hostrcb = list_first_entry_or_null(&ioa_cfg->hostrcb_report_q, >> + struct ipr_hostrcb, queue); >> + spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags); >> + if (!hostrcb) >> + return 0; >> + >> + >> + ret = memory_read_from_buffer(buf, count, &off, &hostrcb->hcam, >> + sizeof(hostrcb->hcam)); > > Can you tuck this memory_read_from_buffer up under the spin_lock above? Since > the hostrcb's don't have any refcounting associated with them, I'm worried > about > racing with the adapter remove path. > >> + return ret; >> +} >> + >> +static ssize_t ipr_next_async_err_log(struct file *filep, struct kobject >> *kobj, >> + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) >> +{ >> + struct device *cdev = container_of(kobj, struct device, kobj); >> + struct Scsi_Host *shost = class_to_shost(cdev); >> + struct ipr_ioa_cfg *ioa_cfg = (struct ipr_ioa_cfg *)shost->hostdata; >> + struct ipr_hostrcb *hostrcb; >> + unsigned long lock_flags = 0; >> + >> + spin_lock_irqsave(ioa_cfg->host->host_lock, lock_flags); >> + hostrcb = list_first_entry_or_null(&ioa_cfg->hostrcb_report_q, >> + struct ipr_hostrcb, queue); >> + if (!hostrcb) { >> + spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags); >> + return 0; >> + } >> + >> + /* Reclaim hostrcb before exit */ >> + list_move_tail(&hostrcb->queue, &ioa_cfg->hostrcb_free_q); >> + spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags); >> + return count; >> +} >> + >> +static struct bin_attribute ipr_ioa_async_err_log = { >> + .attr = { >> + .name = "async_err_log", >> + .mode = S_IRUGO | S_IWUSR, >> + }, >> + .size = 0, > > No need to initialize static variables to zero. Don't we need this to make it clear that there is no maximum size to our binary attribute? I understand that initializing this to zero is probably a verbose convention, but other binary attributes I've seen follow this as well. Could we keep this as is, or should I just leave it uninitialized? >> + .read = ipr_read_async_err_log, >> + .write = ipr_next_async_err_log >> +}; >> + >> +static ssize_t ipr_show_current_hcam_count(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct Scsi_Host *shost = class_to_shost(dev); >> + struct ipr_ioa_cfg *ioa_cfg = (struct ipr_ioa_cfg *)shost->hostdata; >> + struct ipr_hostrcb *hostrcb, *temp; >> + int len, hcam_count = 0; >> + unsigned long lock_flags = 0; >> + >> + spin_lock_irqsave(ioa_cfg->host->host_lock, lock_flags); >> + list_for_each_entry_safe(hostrcb, temp, >> + &ioa_cfg->hostrcb_report_q, queue) { >> + hcam_count++; >> + } >> + spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags); >> + len = snprintf(buf, PAGE_SIZE, "%d\n", hcam_count); >> + return len; >> +} >> + >> +static struct device_attribute ipr_ioa_current_hcam_count = { >> + .attr = { >> + .name = "current_hcam_count", >> + .mode = S_IRUGO, >> + }, >> + .show = ipr_show_current_hcam_count >> +}; > > This attribute doesn't seem necessary. We should be able to just read the > async_err_log attribute and see if there is any data there, can't we? I was keeping this in because I thought it was a nice addition to keep track of the currently held HCAM buffers from a user's point of view. You're right though, since the HCAM buffers will probably be used by a user space daemon, we could just read async_err_log and check if there is any data (there is also the uevent notification). I agree, let's do away with this attribute. >> + >> static struct device_attribute *ipr_ioa_attrs[] = { >> &ipr_fw_version_attr, >> &ipr_log_level_attr, > > > > I'll change those things you mentioned and send out a v2 shortly. Thanks! -- Heitor Ricardo Alves de Siqueira Power Linux IO IBM Linux Technology Center ------------------------------------------------------------------------------ _______________________________________________ Iprdd-devel mailing list Iprdd-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/iprdd-devel