[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/block/nvme: fix aer logic
From: |
Klaus Jensen |
Subject: |
Re: [PATCH] hw/block/nvme: fix aer logic |
Date: |
Mon, 19 Oct 2020 19:48:38 +0200 |
On Oct 19 09:43, Keith Busch wrote:
> On Mon, Oct 19, 2020 at 08:54:16AM +0200, Klaus Jensen wrote:
> > @@ -844,6 +838,12 @@ static void nvme_enqueue_event(NvmeCtrl *n, uint8_t
> > event_type,
> > return;
> > }
> >
> > + /* ignore if masked (cqe posted, but event not cleared) */
> > + if (n->aer_mask & (1 << event_type)) {
> > + trace_pci_nvme_aer_masked(event_type, n->aer_mask);
> > + return;
> > + }
>
> The 'mask' means the host hasn't yet acknowledged the AER with the
> appropriate log. The controller should continue to internally enqueue
> subsequent events of this type, but suppress sending the notification
> for them until the host unlatches the event type.
>
Ugh. Looks like you are right. Again.
Notice events are definitely a good case for when we want to queue up
the events internally since the information correspond to different log
pages but use the same type.
> > event = g_new(NvmeAsyncEvent, 1);
> > event->result = (NvmeAerResult) {
> > .event_type = event_type,
> > @@ -859,9 +859,15 @@ static void nvme_enqueue_event(NvmeCtrl *n, uint8_t
> > event_type,
> >
> > static void nvme_clear_events(NvmeCtrl *n, uint8_t event_type)
> > {
> > + NvmeAsyncEvent *event, *next;
> > +
> > n->aer_mask &= ~(1 << event_type);
> > - if (!QTAILQ_EMPTY(&n->aer_queue)) {
> > - nvme_process_aers(n);
> > +
> > + QTAILQ_FOREACH_SAFE(event, &n->aer_queue, entry, next) {
> > + if (event->result.event_type == event_type) {
> > + QTAILQ_REMOVE(&n->aer_queue, event, entry);
>
> Memory leaking the 'event'?
>
Thanks, good catch, but this change is also irrelevant now.
> > + n->aer_queued--;
> > + }
> > }
> > }
>
signature.asc
Description: PGP signature