Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
MIME-Version: 1.0
References: <20231202150807.2571103-1-srasheed@marvell.com> <CADEbmW12OWS6et2wp3skicUM=V81x8dS4_aySYP1Ok0kEc2M9Q@mail.gmail.com>
In-Reply-To: <CADEbmW12OWS6et2wp3skicUM=V81x8dS4_aySYP1Ok0kEc2M9Q@mail.gmail.com>
From:   Michal Schmidt <mschmidt@redhat.com>
Date:   Mon, 4 Dec 2023 23:13:24 +0100
Message-ID: <CADEbmW3K7QkfniBtmMt=SZtwZWez30F+sM=656wqmZR8=ig1jQ@mail.gmail.com>
Subject: Re: [PATCH net v1] octeon_ep: initialise control mbox tasks before
 using APIs
To:     Shinas Rasheed <srasheed@marvell.com>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        hgani@marvell.com, vimleshk@marvell.com, egallen@redhat.com,
        pabeni@redhat.com, horms@kernel.org, kuba@kernel.org,
        davem@davemloft.net, wizhao@redhat.com, konguyen@redhat.com,
        Veerasenareddy Burru <vburru@marvell.com>,
        Sathesh Edara <sedara@marvell.com>,
        Eric Dumazet <edumazet@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Dec 4, 2023 at 11:10=E2=80=AFPM Michal Schmidt <mschmidt@redhat.com=
> wrote:
>
> On Sat, Dec 2, 2023 at 4:08=E2=80=AFPM Shinas Rasheed <srasheed@marvell.c=
om> wrote:
> > Do INIT_WORK for the various workqueue tasks before the first
> > invocation of any control net APIs. Since octep_ctrl_net_get_info
> > was called before the control net receive work task was even
> > initialised, the function call wasn't returning actual firmware
> > info queried from Octeon.
>
> It might be more accurate to say that octep_ctrl_net_get_info depends
> on the processing of OEI events. This happens in intr_poll_task.
> That's why intr_poll_task needs to be queued earlier.
> Did octep_send_mbox_req previously always fail with EAGAIN after
          ^^^^^^^^^^^^^^^^^^^^^
I meant octep_ctrl_net_get_info here.

> running into the 500 ms timeout in octep_send_mbox_req?
>
> Apropos octep_send_mbox_req... I think it has a race. "d" is put on
> the ctrl_req_wait_list after sending the request to the hardware. If
> the response arrives quickly, "d" might not yet be on the list when
> process_mbox_resp looks for it.
> Also, what protects ctrl_req_wait_list from concurrent access?
>
> Michal
>
> > Fixes: 8d6198a14e2b ("octeon_ep: support to fetch firmware info")
> > Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> > ---
> >  .../net/ethernet/marvell/octeon_ep/octep_main.c    | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/driv=
ers/net/ethernet/marvell/octeon_ep/octep_main.c
> > index 552970c7dec0..3e7bfd3e0f56 100644
> > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > @@ -1193,6 +1193,13 @@ int octep_device_setup(struct octep_device *oct)
> >         if (ret)
> >                 return ret;
> >
> > +       INIT_WORK(&oct->tx_timeout_task, octep_tx_timeout_task);
> > +       INIT_WORK(&oct->ctrl_mbox_task, octep_ctrl_mbox_task);
> > +       INIT_DELAYED_WORK(&oct->intr_poll_task, octep_intr_poll_task);
> > +       oct->poll_non_ioq_intr =3D true;
> > +       queue_delayed_work(octep_wq, &oct->intr_poll_task,
> > +                          msecs_to_jiffies(OCTEP_INTR_POLL_TIME_MSECS)=
);
> > +
> >         atomic_set(&oct->hb_miss_cnt, 0);
> >         INIT_DELAYED_WORK(&oct->hb_task, octep_hb_timeout_task);
> >
> > @@ -1333,13 +1340,6 @@ static int octep_probe(struct pci_dev *pdev, con=
st struct pci_device_id *ent)
> >         queue_delayed_work(octep_wq, &octep_dev->hb_task,
> >                            msecs_to_jiffies(octep_dev->conf->fw_info.hb=
_interval));
> >
> > -       INIT_WORK(&octep_dev->tx_timeout_task, octep_tx_timeout_task);
> > -       INIT_WORK(&octep_dev->ctrl_mbox_task, octep_ctrl_mbox_task);
> > -       INIT_DELAYED_WORK(&octep_dev->intr_poll_task, octep_intr_poll_t=
ask);
> > -       octep_dev->poll_non_ioq_intr =3D true;
> > -       queue_delayed_work(octep_wq, &octep_dev->intr_poll_task,
> > -                          msecs_to_jiffies(OCTEP_INTR_POLL_TIME_MSECS)=
);
> > -
> >         netdev->netdev_ops =3D &octep_netdev_ops;
> >         octep_set_ethtool_ops(netdev);
> >         netif_carrier_off(netdev);
> > --
> > 2.25.1
> >