Re: [PATCH 02/15] mpi3mr: Add framework to issue cnfg requests

[Sreekanth Reddy <sreekanth.reddy@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 18210 bytes --]

On Fri, Jul 29, 2022 at 10:30 PM Himanshu Madhani
<himanshu.madhani@oracle.com> wrote:
>
>
>
> > On Jul 29, 2022, at 6:16 AM, Sreekanth Reddy <sreekanth.reddy@broadcom.com> wrote:
> >
> > Added framework to issue config requests commands to
> > controller firmware.
> >
> > Signed-off-by: Sreekanth Reddy <sreekanth.reddy@broadcom.com>
> > ---
> > drivers/scsi/mpi3mr/mpi3mr.h    |  28 ++++
> > drivers/scsi/mpi3mr/mpi3mr_fw.c | 256 ++++++++++++++++++++++++++++++++
> > drivers/scsi/mpi3mr/mpi3mr_os.c |   1 +
> > 3 files changed, 285 insertions(+)
> >
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr.h b/drivers/scsi/mpi3mr/mpi3mr.h
> > index 0935b2e..e15ad0e 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr.h
> > +++ b/drivers/scsi/mpi3mr/mpi3mr.h
> > @@ -97,6 +97,7 @@ extern atomic64_t event_counter;
> > #define MPI3MR_HOSTTAG_PEL_ABORT      3
> > #define MPI3MR_HOSTTAG_PEL_WAIT               4
> > #define MPI3MR_HOSTTAG_BLK_TMS                5
> > +#define MPI3MR_HOSTTAG_CFG_CMDS              6
> >
> > #define MPI3MR_NUM_DEVRMCMD           16
> > #define MPI3MR_HOSTTAG_DEVRMCMD_MIN   (MPI3MR_HOSTTAG_BLK_TMS + 1)
> > @@ -126,6 +127,8 @@ extern atomic64_t event_counter;
> >
> > #define MPI3MR_WATCHDOG_INTERVAL              1000 /* in milli seconds */
> >
> > +#define MPI3MR_DEFAULT_CFG_PAGE_SZ           1024 /* in bytes */
> > +
> > #define MPI3MR_SCMD_TIMEOUT    (60 * HZ)
> > #define MPI3MR_EH_SCMD_TIMEOUT (60 * HZ)
> >
> > @@ -274,6 +277,7 @@ enum mpi3mr_reset_reason {
> >       MPI3MR_RESET_FROM_SYSFS = 23,
> >       MPI3MR_RESET_FROM_SYSFS_TIMEOUT = 24,
> >       MPI3MR_RESET_FROM_FIRMWARE = 27,
> > +     MPI3MR_RESET_FROM_CFG_REQ_TIMEOUT = 29,
> > };
> >
> > /* Queue type definitions */
> > @@ -679,6 +683,21 @@ struct mpi3mr_drv_cmd {
> >           struct mpi3mr_drv_cmd *drv_cmd);
> > };
> >
> > +/**
> > + * struct dma_memory_desc - memory descriptor structure to store
> > + * virtual address, dma address and size for any generic dma
> > + * memory allocations in the driver.
> > + *
> > + * @size: buffer size
> > + * @addr: virtual address
> > + * @dma_addr: dma address
> > + */
> > +struct dma_memory_desc {
> > +     u32 size;
> > +     void *addr;
> > +     dma_addr_t dma_addr;
> > +};
> > +
> >
> > /**
> >  * struct chain_element - memory descriptor structure to store
> > @@ -756,6 +775,7 @@ struct scmd_priv {
> >  * @num_op_reply_q: Number of operational reply queues
> >  * @op_reply_qinfo: Operational reply queue info pointer
> >  * @init_cmds: Command tracker for initialization commands
> > + * @cfg_cmds: Command tracker for configuration requests
> >  * @facts: Cached IOC facts data
> >  * @op_reply_desc_sz: Operational reply descriptor size
> >  * @num_reply_bufs: Number of reply buffers allocated
> > @@ -854,6 +874,9 @@ struct scmd_priv {
> >  * @io_throttle_low: I/O size to stop throttle in 512b blocks
> >  * @num_io_throttle_group: Maximum number of throttle groups
> >  * @throttle_groups: Pointer to throttle group info structures
> > + * @cfg_page: Default memory for configuration pages
> > + * @cfg_page_dma: Configuration page DMA address
> > + * @cfg_page_sz: Default configuration page memory size
> >  */
> > struct mpi3mr_ioc {
> >       struct list_head list;
> > @@ -904,6 +927,7 @@ struct mpi3mr_ioc {
> >       struct op_reply_qinfo *op_reply_qinfo;
> >
> >       struct mpi3mr_drv_cmd init_cmds;
> > +     struct mpi3mr_drv_cmd cfg_cmds;
> >       struct mpi3mr_ioc_facts facts;
> >       u16 op_reply_desc_sz;
> >
> > @@ -1025,6 +1049,10 @@ struct mpi3mr_ioc {
> >       u32 io_throttle_low;
> >       u16 num_io_throttle_group;
> >       struct mpi3mr_throttle_group_info *throttle_groups;
> > +
> > +     void *cfg_page;
> > +     dma_addr_t cfg_page_dma;
> > +     u16 cfg_page_sz;
> > };
> >
> > /**
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> > index 0866dfd..da6eceb 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> > @@ -299,6 +299,8 @@ mpi3mr_get_drv_cmd(struct mpi3mr_ioc *mrioc, u16 host_tag,
> >       switch (host_tag) {
> >       case MPI3MR_HOSTTAG_INITCMDS:
> >               return &mrioc->init_cmds;
> > +     case MPI3MR_HOSTTAG_CFG_CMDS:
> > +             return &mrioc->cfg_cmds;
> >       case MPI3MR_HOSTTAG_BSG_CMDS:
> >               return &mrioc->bsg_cmds;
> >       case MPI3MR_HOSTTAG_BLK_TMS:
> > @@ -907,6 +909,7 @@ static const struct {
> >       { MPI3MR_RESET_FROM_SYSFS, "sysfs invocation" },
> >       { MPI3MR_RESET_FROM_SYSFS_TIMEOUT, "sysfs TM timeout" },
> >       { MPI3MR_RESET_FROM_FIRMWARE, "firmware asynchronous reset" },
> > +     { MPI3MR_RESET_FROM_CFG_REQ_TIMEOUT, "configuration request timeout"},
> > };
> >
> > /**
> > @@ -3738,6 +3741,14 @@ retry_init:
> >
> >       mpi3mr_print_ioc_info(mrioc);
> >
> > +     dprint_init(mrioc, "allocating config page buffers\n");
> > +     mrioc->cfg_page = dma_alloc_coherent(&mrioc->pdev->dev,
> > +         MPI3MR_DEFAULT_CFG_PAGE_SZ, &mrioc->cfg_page_dma, GFP_KERNEL);
> > +     if (!mrioc->cfg_page)
> > +             goto out_failed_noretry;
> > +
> > +     mrioc->cfg_page_sz = MPI3MR_DEFAULT_CFG_PAGE_SZ;
> > +
> >       retval = mpi3mr_alloc_reply_sense_bufs(mrioc);
> >       if (retval) {
> >               ioc_err(mrioc,
> > @@ -4362,6 +4373,10 @@ static void mpi3mr_flush_drv_cmds(struct mpi3mr_ioc *mrioc)
> >
> >       cmdptr = &mrioc->init_cmds;
> >       mpi3mr_drv_cmd_comp_reset(mrioc, cmdptr);
> > +
> > +     cmdptr = &mrioc->cfg_cmds;
> > +     mpi3mr_drv_cmd_comp_reset(mrioc, cmdptr);
> > +
> >       cmdptr = &mrioc->bsg_cmds;
> >       mpi3mr_drv_cmd_comp_reset(mrioc, cmdptr);
> >       cmdptr = &mrioc->host_tm_cmds;
> > @@ -4786,3 +4801,244 @@ out:
> >           ((retval == 0) ? "successful" : "failed"));
> >       return retval;
> > }
> > +
> > +
> > +/**
> > + * mpi3mr_free_config_dma_memory - free memory for config page
> > + * @mrioc: Adapter instance reference
> > + * @mem_desc: memory descriptor structure
> > + *
> > + * Check whether the size of the buffer specified by the memory
> > + * descriptor is greater than the default page size if so then
> > + * free the memory pointed by the descriptor.
> > + *
> > + * Return: 0 on success, non-zero on failure.
>
> Nit: the above comment is incorrect. This function returns nothing.

Agree. Will update in the next patch version set.

>
> > + */
> > +static void mpi3mr_free_config_dma_memory(struct mpi3mr_ioc *mrioc,
> > +     struct dma_memory_desc *mem_desc)
> > +{
> > +     if ((mem_desc->size > mrioc->cfg_page_sz) && mem_desc->addr) {
> > +             dma_free_coherent(&mrioc->pdev->dev, mem_desc->size,
> > +                 mem_desc->addr, mem_desc->dma_addr);
> > +             mem_desc->addr = NULL;
> > +     }
> > +}
> > +
> > +
> > +
>
> Remove extra newline

Agree. Will remove it next patch version set.
>
> > +/**
> > + * mpi3mr_alloc_config_dma_memory - Alloc memory for config page
> > + * @mrioc: Adapter instance reference
> > + * @mem_desc: Memory descriptor to hold dma memory info
> > + *
> > + * This function allocates new dmaable memory or provides the
> > + * default config page dmaable memory based on the memory size
> > + * described by the descriptor.
> > + *
> > + * Return: 0 on success, non-zero on failure.
> > + */
> > +static int mpi3mr_alloc_config_dma_memory(struct mpi3mr_ioc *mrioc,
> > +     struct dma_memory_desc *mem_desc)
> > +{
> > +     if (mem_desc->size > mrioc->cfg_page_sz) {
> > +             mem_desc->addr = dma_alloc_coherent(&mrioc->pdev->dev,
> > +                 mem_desc->size, &mem_desc->dma_addr, GFP_KERNEL);
> > +             if (!mem_desc->addr)
> > +                     return -ENOMEM;
> > +     } else {
> > +             mem_desc->addr = mrioc->cfg_page;
> > +             mem_desc->dma_addr = mrioc->cfg_page_dma;
> > +             memset(mem_desc->addr, 0, mrioc->cfg_page_sz);
> > +     }
> > +     return 0;
> > +}
> > +
> > +
> > +/**
> > + * mpi3mr_post_cfg_req - Issue config requests and wait
> > + * @mrioc: Adapter instance reference
> > + * @cfg_req: Configuration request
> > + * @timeout: Timeout in seconds
> > + * @ioc_status: Pointer to return ioc status
> > + *
> > + * A generic function for posting MPI3 configuration request to
> > + * the firmware. This blocks for the completion of request for
> > + * timeout seconds and if the request times out this function
> > + * faults the controller with proper reason code.
> > + *
> > + * On successful completion of the request this function returns
> > + * appropriate ioc status from the firmware back to the caller.
> > + *
> > + * Return: 0 on success, non-zero on failure.
> > + */
> > +static int mpi3mr_post_cfg_req(struct mpi3mr_ioc *mrioc,
> > +     struct mpi3_config_request *cfg_req, int timeout, u16 *ioc_status)
> > +{
> > +     int retval = 0;
> > +
> > +     mutex_lock(&mrioc->cfg_cmds.mutex);
> > +     if (mrioc->cfg_cmds.state & MPI3MR_CMD_PENDING) {
> > +             retval = -1;
> > +             ioc_err(mrioc, "sending config request failed due to command in use\n");
> > +             mutex_unlock(&mrioc->cfg_cmds.mutex);
> > +             goto out;
> > +     }
> > +     mrioc->cfg_cmds.state = MPI3MR_CMD_PENDING;
> > +     mrioc->cfg_cmds.is_waiting = 1;
>
> You are setting is_waiting here, but I don’t see this being cleared anywhere.

It is getting cleared in mpi3mr_process_admin_reply_desc() with below lines,

                        if (cmdptr->is_waiting) {
                                complete(&cmdptr->done);
                                cmdptr->is_waiting = 0;
                        } else if (cmdptr->callback)
                                cmdptr->callback(mrioc, cmdptr);

>
> > +     mrioc->cfg_cmds.callback = NULL;
> > +     mrioc->cfg_cmds.ioc_status = 0;
> > +     mrioc->cfg_cmds.ioc_loginfo = 0;
> > +
> > +     cfg_req->host_tag = cpu_to_le16(MPI3MR_HOSTTAG_CFG_CMDS);
> > +     cfg_req->function = MPI3_FUNCTION_CONFIG;
> > +
> > +     init_completion(&mrioc->cfg_cmds.done);
> > +     dprint_cfg_info(mrioc, "posting config request\n");
> > +     if (mrioc->logging_level & MPI3_DEBUG_CFG_INFO)
> > +             dprint_dump(cfg_req, sizeof(struct mpi3_config_request),
> > +                 "mpi3_cfg_req");
> > +     retval = mpi3mr_admin_request_post(mrioc, cfg_req, sizeof(*cfg_req), 1);
> > +     if (retval) {
> > +             ioc_err(mrioc, "posting config request failed\n");
> > +             goto out_unlock;
> > +     }
> > +     wait_for_completion_timeout(&mrioc->cfg_cmds.done, (timeout * HZ));
> > +     if (!(mrioc->cfg_cmds.state & MPI3MR_CMD_COMPLETE)) {
> > +             mpi3mr_check_rh_fault_ioc(mrioc,
> > +                 MPI3MR_RESET_FROM_CFG_REQ_TIMEOUT);
> > +             ioc_err(mrioc, "config request timed out\n");
> > +             retval = -1;
> > +             goto out_unlock;
> > +     }
> > +     *ioc_status = mrioc->cfg_cmds.ioc_status & MPI3_IOCSTATUS_STATUS_MASK;
> > +     if ((*ioc_status) != MPI3_IOCSTATUS_SUCCESS)
> > +             dprint_cfg_err(mrioc,
> > +                 "cfg_page request returned with ioc_status(0x%04x), log_info(0x%08x)\n",
> > +                 *ioc_status, mrioc->cfg_cmds.ioc_loginfo);
> > +
> > +out_unlock:
> > +     mrioc->cfg_cmds.state = MPI3MR_CMD_NOTUSED;
>
> Should you not set mrioc->cfg_cmds.is_waiting = 0 here? Or am I missing something?

cfg_cmds's is_waiting is cleared in mpi3mr_process_admin_reply_desc().

>
> > +     mutex_unlock(&mrioc->cfg_cmds.mutex);
> > +
> > +out:
> > +     return retval;
> > +}
> > +
> > +/**
> > + * mpi3mr_process_cfg_req - config page request processor
> > + * @mrioc: Adapter instance reference
> > + * @cfg_req: Configuration request
> > + * @cfg_hdr: Configuration page header
> > + * @timeout: Timeout in seconds
> > + * @ioc_status: Pointer to return ioc status
> > + * @cfg_buf: Memory pointer to copy config page or header
> > + * @cfg_buf_sz: Size of the memory to get config page or header
> > + *
> > + * This is handler for config page read, write and config page
> > + * header read operations.
> > + *
> > + * This function expects the cfg_req to be populated with page
> > + * type, page number, action for the header read and with page
> > + * address for all other operations.
> > + *
> > + * The cfg_hdr can be passed as null for reading required header
> > + * details for read/write pages the cfg_hdr should point valid
> > + * configuration page header.
> > + *
> > + * This allocates dmaable memory based on the size of the config
> > + * buffer and set the SGE of the cfg_req.
> > + *
> > + * For write actions, the config page data has to be passed in
> > + * the cfg_buf and size of the data has to be mentioned in the
> > + * cfg_buf_sz.
> > + *
> > + * For read/header actions, on successful completion of the
> > + * request with successful ioc_status the data will be copied
> > + * into the cfg_buf limited to a minimum of actual page size and
> > + * cfg_buf_sz
> > + *
> > + *
> > + * Return: 0 on success, non-zero on failure.
> > + */
> > +static int mpi3mr_process_cfg_req(struct mpi3mr_ioc *mrioc,
> > +     struct mpi3_config_request *cfg_req,
> > +     struct mpi3_config_page_header *cfg_hdr, int timeout, u16 *ioc_status,
> > +     void *cfg_buf, u32 cfg_buf_sz)
> > +{
> > +     struct dma_memory_desc mem_desc;
> > +     int retval = -1;
> > +     u8 invalid_action = 0;
> > +     u8 sgl_flags = MPI3MR_SGEFLAGS_SYSTEM_SIMPLE_END_OF_LIST;
> > +
> > +     memset(&mem_desc, 0, sizeof(struct dma_memory_desc));
> > +
> > +     if (cfg_req->action == MPI3_CONFIG_ACTION_PAGE_HEADER)
> > +             mem_desc.size = sizeof(struct mpi3_config_page_header);
> > +     else {
> > +             if (!cfg_hdr) {
> > +                     ioc_err(mrioc, "null config header passed for config action(%d), page_type(0x%02x), page_num(%d)\n",
> > +                         cfg_req->action, cfg_req->page_type,
> > +                         cfg_req->page_number);
> > +                     goto out;
> > +             }
> > +             switch (cfg_hdr->page_attribute & MPI3_CONFIG_PAGEATTR_MASK) {
> > +             case MPI3_CONFIG_PAGEATTR_READ_ONLY:
> > +                     if (cfg_req->action
> > +                         != MPI3_CONFIG_ACTION_READ_CURRENT)
> > +                             invalid_action = 1;
> > +                     break;
> > +             case MPI3_CONFIG_PAGEATTR_CHANGEABLE:
> > +                     if ((cfg_req->action ==
> > +                          MPI3_CONFIG_ACTION_READ_PERSISTENT) ||
> > +                         (cfg_req->action ==
> > +                          MPI3_CONFIG_ACTION_WRITE_PERSISTENT))
> > +                             invalid_action = 1;
> > +                     break;
> > +             case MPI3_CONFIG_PAGEATTR_PERSISTENT:
> > +             default:
> > +                     break;
> > +             }
> > +             if (invalid_action) {
> > +                     ioc_err(mrioc,
> > +                         "config action(%d) is not allowed for page_type(0x%02x), page_num(%d) with page_attribute(0x%02x)\n",
> > +                         cfg_req->action, cfg_req->page_type,
> > +                         cfg_req->page_number, cfg_hdr->page_attribute);
> > +                     goto out;
> > +             }
> > +             mem_desc.size = le16_to_cpu(cfg_hdr->page_length) * 4;
> > +             cfg_req->page_length = cfg_hdr->page_length;
> > +             cfg_req->page_version = cfg_hdr->page_version;
> > +     }
> > +     if (mpi3mr_alloc_config_dma_memory(mrioc, &mem_desc))
> > +             goto out;
> > +
> > +     mpi3mr_add_sg_single(&cfg_req->sgl, sgl_flags, mem_desc.size,
> > +         mem_desc.dma_addr);
> > +
> > +     if ((cfg_req->action == MPI3_CONFIG_ACTION_WRITE_PERSISTENT) ||
> > +         (cfg_req->action == MPI3_CONFIG_ACTION_WRITE_CURRENT)) {
> > +             memcpy(mem_desc.addr, cfg_buf, min_t(u16, mem_desc.size,
> > +                 cfg_buf_sz));
> > +             dprint_cfg_info(mrioc, "config buffer to be written\n");
> > +             if (mrioc->logging_level & MPI3_DEBUG_CFG_INFO)
> > +                     dprint_dump(mem_desc.addr, mem_desc.size, "cfg_buf");
> > +     }
> > +
> > +     if (mpi3mr_post_cfg_req(mrioc, cfg_req, timeout, ioc_status))
> > +             goto out;
> > +
> > +     retval = 0;
> > +     if ((*ioc_status == MPI3_IOCSTATUS_SUCCESS) &&
> > +         (cfg_req->action != MPI3_CONFIG_ACTION_WRITE_PERSISTENT) &&
> > +         (cfg_req->action != MPI3_CONFIG_ACTION_WRITE_CURRENT)) {
> > +             memcpy(cfg_buf, mem_desc.addr, min_t(u16, mem_desc.size,
> > +                 cfg_buf_sz));
> > +             dprint_cfg_info(mrioc, "config buffer read\n");
> > +             if (mrioc->logging_level & MPI3_DEBUG_CFG_INFO)
> > +                     dprint_dump(mem_desc.addr, mem_desc.size, "cfg_buf");
> > +     }
> > +
> > +out:
> > +     mpi3mr_free_config_dma_memory(mrioc, &mem_desc);
> > +     return retval;
> > +}
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> > index 8bdf927..40bed22 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> > @@ -4574,6 +4574,7 @@ mpi3mr_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >       mpi3mr_init_drv_cmd(&mrioc->init_cmds, MPI3MR_HOSTTAG_INITCMDS);
> >       mpi3mr_init_drv_cmd(&mrioc->host_tm_cmds, MPI3MR_HOSTTAG_BLK_TMS);
> >       mpi3mr_init_drv_cmd(&mrioc->bsg_cmds, MPI3MR_HOSTTAG_BSG_CMDS);
> > +     mpi3mr_init_drv_cmd(&mrioc->cfg_cmds, MPI3MR_HOSTTAG_CFG_CMDS);
> >
> >       for (i = 0; i < MPI3MR_NUM_DEVRMCMD; i++)
> >               mpi3mr_init_drv_cmd(&mrioc->dev_rmhs_cmds[i],
> > --
> > 2.27.0
> >
>
> --
> Himanshu Madhani        Oracle Linux Engineering
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4218 bytes --]