On Wed, 17 May 2023 14:25:01 +0200
Nico Boehr <nrb@xxxxxxxxxxxxx> wrote:
> Quoting Claudio Imbrenda (2023-05-16 19:17:24)
> [...]
> > > diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> > > index 3f993a363ae2..1180ec44d72f 100644
> > > --- a/lib/s390x/interrupt.c
> > > +++ b/lib/s390x/interrupt.c
> [...]
> > > +void irq_set_dat_mode(bool dat, uint64_t as)
> > > +{
> [...]
> > > + for (struct psw *irq_psw = irq_psws[0]; irq_psw != NULL; irq_psw++) {
> >
> > just call it psw, or cur_psw, it's a little confusing otherwise
>
> will do.
>
> [...]
> > alternatively, you can redefine psw with a bitfield (as you mentioned
> > offline):
> >
> > cur_psw->mask.dat = dat;
> > if (dat)
> > cur_psw->mask.as = as;
>
> Yep, I'll go with that.
>
> >
> > > + else
> > > + irq_psw->mask |= PSW_MASK_DAT | as << (63 - 16);
> >
> > otherwise here you're ORing stuff to other stuff, if you had 3 and you
> > OR 0 you get 3, but you actually want 0
>
> And that's the advantage of the bitfield. :)
>
> >
> > > + }
> > > +
> > > + mb();
> >
> > what's the purpose of this?
>
> Make sure that the lowcore really has been written, but I think it's quite
> useless, since a function is a sequence point, right?
yes
