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

[Sunil Kovvuri <sunil.kovvuri@gmail.com>]

On Fri, Jan 24, 2020 at 9:11 PM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Tue, Jan 21, 2020 at 06:51:36PM +0530, sunil.kovvuri@gmail.com wrote:
> > 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.
>
> > +             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.

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