Re: [PATCH v4 02/17] octeontx2-pf: Mailbox communication with AF

On Fri, Jan 24, 2020 at 9:11 PM Maciej Fijalkowski
[off-list ref] wrote:

On Tue, Jan 21, 2020 at 06:51:36PM +0530, sunil.kovvuri@gmail.com wrote:

quoted

From: Sunil Goutham <sgoutham@marvell.com>

+
+static void otx2_queue_work(struct mbox *mw, struct workqueue_struct *mbox_wq,
+                         int first, int mdevs, u64 intr, int type)
+{
+     struct otx2_mbox_dev *mdev;
+     struct otx2_mbox *mbox;
+     struct mbox_hdr *hdr;
+     int i;
+
+     for (i = first; i < mdevs; i++) {
+             /* start from 0 */
+             if (!(intr & BIT_ULL(i - first)))
+                     continue;
+
+             mbox = &mw->mbox;
+             mdev = &mbox->dev[i];
+             if (type == TYPE_PFAF)
+                     otx2_sync_mbox_bbuf(mbox, i);
+             hdr = mdev->mbase + mbox->rx_start;
+             /* The hdr->num_msgs is set to zero immediately in the interrupt
+              * handler to  ensure that it holds a correct value next time
+              * when the interrupt handler is called.
+              * pf->mbox.num_msgs holds the data for use in pfaf_mbox_handler
+              * pf>mbox.up_num_msgs holds the data for use in
+              * pfaf_mbox_up_handler.
+              */
+             if (hdr->num_msgs) {
+                     mw[i].num_msgs = hdr->num_msgs;
+                     hdr->num_msgs = 0;
+                     if (type == TYPE_PFAF)
+                             memset(mbox->hwbase + mbox->rx_start, 0,
+                                    ALIGN(sizeof(struct mbox_hdr),
+                                          sizeof(u64)));
+
+                     queue_work(mbox_wq, &mw[i].mbox_wrk);
+             }
+
+             mbox = &mw->mbox_up;

You could have a two separate stack variables for these two mboxes instead
of flipping the single variable on each loop iteration.

quoted

+             mdev = &mbox->dev[i];
+             if (type == TYPE_PFAF)
+                     otx2_sync_mbox_bbuf(mbox, i);
+             hdr = mdev->mbase + mbox->rx_start;
+             if (hdr->num_msgs) {
+                     mw[i].up_num_msgs = hdr->num_msgs;
+                     hdr->num_msgs = 0;
+                     if (type == TYPE_PFAF)
+                             memset(mbox->hwbase + mbox->rx_start, 0,
+                                    ALIGN(sizeof(struct mbox_hdr),
+                                          sizeof(u64)));
+
+                     queue_work(mbox_wq, &mw[i].mbox_up_wrk);
+             }

Does it make sense to pull out the logic above onto separate function?

Thanks for the feedback.
I will relook into the logic and see if this can be cleanedup and
submit along with next patchset.

quoted

+     }
+}
+
+static void otx2_pfaf_mbox_destroy(struct otx2_nic *pf)
+{
+     struct mbox *mbox = &pf->mbox;
+
+     if (pf->mbox_wq) {
+             flush_workqueue(pf->mbox_wq);
+             destroy_workqueue(pf->mbox_wq);
+             pf->mbox_wq = NULL;
+     }
+
+     if (mbox->mbox.hwbase)
+             iounmap((void __iomem *)mbox->mbox.hwbase);
+
+     otx2_mbox_destroy(&mbox->mbox);
+     otx2_mbox_destroy(&mbox->mbox_up);
+}
+
+static int otx2_pfaf_mbox_init(struct otx2_nic *pf)
+{
+     struct mbox *mbox = &pf->mbox;
+     void __iomem *hwbase;
+     int err;
+
+     mbox->pfvf = pf;
+     pf->mbox_wq = alloc_workqueue("otx2_pfaf_mailbox",
+                                   WQ_UNBOUND | WQ_HIGHPRI |
+                                   WQ_MEM_RECLAIM, 1);
+     if (!pf->mbox_wq)
+             return -ENOMEM;
+
+     /* Mailbox is a reserved memory (in RAM) region shared between
+      * admin function (i.e AF) and this PF, shouldn't be mapped as
+      * device memory to allow unaligned accesses.
+      */
+     hwbase = ioremap_wc(pci_resource_start(pf->pdev, PCI_MBOX_BAR_NUM),
+                         pci_resource_len(pf->pdev, PCI_MBOX_BAR_NUM));
+     if (!hwbase) {
+             dev_err(pf->dev, "Unable to map PFAF mailbox region\n");
+             err = -ENOMEM;
+             goto exit;
+     }
+
+     err = otx2_mbox_init(&mbox->mbox, hwbase, pf->pdev, pf->reg_base,
+                          MBOX_DIR_PFAF, 1);
+     if (err)
+             goto exit;
+
+     err = otx2_mbox_init(&mbox->mbox_up, hwbase, pf->pdev, pf->reg_base,
+                          MBOX_DIR_PFAF_UP, 1);

There is a chance that the first otx2_mbox_init succeeded and second one
failed. In that case you will be leaking the mbox->dev that otx2_mbox_init
is internally allocating as the caller of otx2_pfaf_mbox_init in case of
error has a 'goto err_free_irq_vectors', so otx2_mbox_destroy won't be
called for the mbox->mbox. Furthermore the iounmap() would be skipped as
well.

I'm not sure whether PCI subsystem will call the remove() callback in case
when probe() failed?

Thanks for catching this, will fix and resubmit.

Sunil.