Re: [PATCH] hw/block/nvme: fix aer logic

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 1798 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]