Heitor,

Very nice patch. A few comments below.

Thanks!

Brian


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.

> +     .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?

> +
>  static struct device_attribute *ipr_ioa_attrs[] = {
>       &ipr_fw_version_attr,
>       &ipr_log_level_attr,




-- 
Brian King
Power Linux I/O
IBM Linux Technology Center


------------------------------------------------------------------------------
_______________________________________________
Iprdd-devel mailing list
Iprdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iprdd-devel

Reply via email to