Re: [Qemu-devel] [PATCH v2 2/2] s390x/css: generate solicited crw for rchp completion signaling

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 1 Aug 2017 17:16:37 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 08/01/2017 09:57 AM, Dong Jia Shi wrote:
> [..]
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -1745,10 +1745,10 @@ int css_do_rchp(uint8_t cssid, uint8_t chpid)
> >      }
> > 
> >      /* We don't really use a channel path, so we're done here. */
> > -    css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT,
> > +    css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1,
> >                    channel_subsys.max_cssid > 0 ? 1 : 0, chpid);
> >      if (channel_subsys.max_cssid > 0) {
> > -        css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 0, real_cssid << 8);
> > +        css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1, 0, real_cssid << 8);
> >      }
> >      return 0;
> >  }
> > @@ -2028,7 +2028,8 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid, uint16_t schid,
> >      }
> >  }
> > 
> > -void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid)
> > +void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
> > +                   int chain, uint16_t rsid)  
> 
> I think you could make the parameters solicited and chain bool (AFAIU
> they are conceptually bool) for clearer semantic. If you go with that
> you could also get rid of the superfluous ternary operator ( we have
> stuff like some_cond ? 1 : 0 for the chain parameter in more than
> one place.

Just adding the new parameter is the minimum change, and we should keep
it consistent. I'm not convinced about converting to bool yet.

> 
> Btw. I find bool flags easy to mix up and difficult to read. I have no better
> idea how to write this (in C) though. I was considering throwing chain and
> solicited together into a single flags parameter, but looking at the client code
> it does not look like a good idea.

Yes, combining them would do nothing for the code.