On Tue, Jan 09, 2024 at 12:04:17PM +1000, Jonathan Matthew wrote:
> On Wed, Jan 03, 2024 at 10:14:12AM +0100, Hrvoje Popovski wrote:
> > On 3.1.2024. 7:51, Jonathan Matthew wrote:
> > > On Wed, Jan 03, 2024 at 01:50:06AM +0100, Alexander Bluhm wrote:
> > >> On Wed, Jan 03, 2024 at 12:26:26AM +0100, Hrvoje Popovski wrote:
> > >>> While testing kettenis@ ipl diff from tech@ and doing iperf3 to bnxt
> > >>> interface and ifconfig bnxt0 down/up at the same time I can trigger
> > >>> panic. Panic can be triggered without kettenis@ diff...
> > >> It is easy to reproduce. ifconfig bnxt1 down/up a few times while
> > >> receiving TCP traffic with iperf3. Machine still has kettenis@ diff.
> > >> My panic looks different.
> > > It looks like I wasn't trying very hard when I wrote bnxt_down().
> > > I think there's also a problem with bnxt_up() unwinding after failure
> > > in various places, but that's a different issue.
> > >
> > > This makes it a more resilient for me, though it still logs
> > > 'bnxt0: unexpected completion type 3' a lot if I take the interface
> > > down while it's in use. I'll look at that separately.
> >
> > Hi,
> >
> > with this diff I can still panic box with ifconfig up/down but not as
> > fast as without it
>
> Right, this is the other problem where bnxt_up() wasn't cleaning up properly
> after failing part way through. This diff should fix that, but I don't think
> it will fix the 'HWRM_RING_ALLOC command returned RESOURCE_ALLOC_ERROR error'
> problem, so the interface will still stop working at that point.
OK bluhm@
> Index: if_bnxt.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/if_bnxt.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 if_bnxt.c
> --- if_bnxt.c 10 Nov 2023 15:51:20 -0000 1.39
> +++ if_bnxt.c 9 Jan 2024 01:59:38 -0000
> @@ -1073,7 +1081,7 @@ bnxt_up(struct bnxt_softc *sc)
> if (bnxt_hwrm_vnic_ctx_alloc(sc, &sc->sc_vnic.rss_id) != 0) {
> printf("%s: failed to allocate vnic rss context\n",
> DEVNAME(sc));
> - goto down_queues;
> + goto down_all_queues;
> }
>
> sc->sc_vnic.id = (uint16_t)HWRM_NA_SIGNATURE;
> @@ -1139,8 +1147,11 @@ dealloc_vnic:
> bnxt_hwrm_vnic_free(sc, &sc->sc_vnic);
> dealloc_vnic_ctx:
> bnxt_hwrm_vnic_ctx_free(sc, &sc->sc_vnic.rss_id);
> +
> +down_all_queues:
> + i = sc->sc_nqueues;
> down_queues:
> - for (i = 0; i < sc->sc_nqueues; i++)
> + while (i-- > 0)
> bnxt_queue_down(sc, &sc->sc_queues[i]);
>
> bnxt_dmamem_free(sc, sc->sc_rx_cfg);
