> -----Original Message----- > From: Peter Maydell <[email protected]> > Sent: 16 February 2026 16:57 > To: Shameer Kolothum Thodi <[email protected]> > Cc: Nicolin Chen <[email protected]>; [email protected]; qemu- > [email protected]; [email protected]; Nathan Chen > <[email protected]>; Matt Ochs <[email protected]>; Jiandi An > <[email protected]>; Jason Gunthorpe <[email protected]>; > [email protected]; [email protected]; > [email protected]; Krishnakant Jaju <[email protected]> > Subject: Re: [PATCH v6 3/5] hw/arm/smmuv3-accel: Allocate vEVENTQ for > accelerated SMMUv3 devices > > External email: Use caution opening links or attachments > > > On Mon, 16 Feb 2026 at 08:58, Shameer Kolothum Thodi > <[email protected]> wrote: > > > > > > > > > -----Original Message----- > > > From: Nicolin Chen <[email protected]> > > > Sent: 13 February 2026 22:19 > > > To: Shameer Kolothum Thodi <[email protected]> > > > Cc: [email protected]; [email protected]; > > > [email protected]; [email protected]; Nathan Chen > > > <[email protected]>; Matt Ochs <[email protected]>; Jiandi An > > > <[email protected]>; Jason Gunthorpe <[email protected]>; > > > [email protected]; [email protected]; > > > [email protected]; Krishnakant Jaju <[email protected]> > > > Subject: Re: [PATCH v6 3/5] hw/arm/smmuv3-accel: Allocate vEVENTQ for > > > accelerated SMMUv3 devices > > > > > > On Fri, Feb 13, 2026 at 10:39:40AM +0000, Shameer Kolothum wrote: > > > > static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset, > > > > uint64_t data, MemTxAttrs attrs) { > > > > - Error *local_err = NULL; > > > > + Error *err = NULL, *local_err = NULL; > > > > > > > > switch (offset) { > > > > case A_CR0: > > > > s->cr[0] = data; > > > > s->cr0ack = data & ~SMMU_CR0_RESERVED; > > > > /* in case the command queue has been enabled */ > > > > - smmuv3_cmdq_consume(s, &local_err); > > > > + smmuv3_cmdq_consume(s, &err); > > > > + /* Allocate vEVENTQ if EVENTQ is enabled and a vIOMMU is > available > > > */ > > > > + smmuv3_accel_alloc_veventq(s, &local_err); > > > > + error_propagate(&err, local_err); > > > > > > It would be cleaner to have: > > > > > > Error *local_err2 = NULL; > > > Error *local_err = NULL; > > > > Yeah. I had a bit of back and forth on this. My thinking was to use err as > > an accumulating error pointer in case additional error paths are added > > later, with local_err_x used for intermediate calls. > > > > Either way is fine I guess for now. > > Generally speaking we should try to stick to one of the > defined standard patterns for Error handling in the > big comment in include/qapi/error.h. > > Yours is the "receive and accumulate multiple errors" pattern, I think?
Yes, this currently uses that pattern. > That works, but do you really want to throw away the second error > message without reporting it if both calls fail? This was considered earlier, but switched to the error_propagate() pattern to follow the accumulation style. Since the two calls are independent, I agree it is better to report both explicitly. I will update this in the next revision if there are no further concerns. > Plus, from the commit message: > > > Also, rename the local error variable and refactor smmu_writel() to use > > a single error accumulator with error_propagate(). > > If you find yourself writing "Also, ..." in a commit message, > it's often a sign that you should consider splitting the > commit in two. Sure. Jonathan also mentioned that. (I was lazy) Thanks, Shameer
