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

Reply via email to