On 17.12.18 12:19, Janosch Frank wrote: > On 17.12.18 11:16, David Hildenbrand wrote: >> Looking at what the kernel does, looks like we should >> - Check for more errors >> - Try to execute !FORCED read if the FORCED one is unknown >> - Set the function code to a normal write >> - Set control_mask[2] to SCLP_VARIABLE_LENGTH_RESPONSE >> >> The kernel seems to set the function code to 0x80. Looking at where that >> comes from, turns out this is a "dump indicator" to produce more >> meaningful dumps. Looking for other function codes used in the kernel, I >> discovered 0x40, which is used for "single increment assign" to speed up >> memory hotplug. I was not able to find out what "control_mask[2] = 0x80" >> means. > > We figured out what control_mask[2] = 0x80 is, so you can drop that. > Maybe rephrase that anyway and put it to the second to last paragraph. > Does it actually make sense to set 0x80 at all or should we simply drop it? > >> >> Tested-by: Janosch Frank <frankja@xxxxxxxxxxxxx> >> Reviewed-by: Janosch Frank <frankja@xxxxxxxxxxxxx> >> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx> >> --- >> >> v1 -> v2: >> - Use define for control mask value 0x80 >> - Rename SCLP_VARIABLE_LENGTH_RESPONSE to SCLP_CM2_VARIABLE_LENGTH_RESPONSE >> - Cleanup sclp.h header file a bit (move to sections) >> - Make sure control mask and function code are in a deterministic state >> when issuing sclp_service_call >> >> lib/s390x/sclp-ascii.c | 7 +++++++ >> lib/s390x/sclp.c | 26 ++++++++++++++++++++++++-- >> lib/s390x/sclp.h | 13 +++++++++---- >> 3 files changed, 40 insertions(+), 6 deletions(-) >> >> diff --git a/lib/s390x/sclp-ascii.c b/lib/s390x/sclp-ascii.c >> index 893ca17..344a414 100644 >> --- a/lib/s390x/sclp-ascii.c >> +++ b/lib/s390x/sclp-ascii.c >> @@ -38,6 +38,10 @@ static void sclp_set_write_mask(void) >> WriteEventMask *sccb = (void *)_sccb; >> >> sccb->h.length = sizeof(WriteEventMask); >> + sccb->h.function_code = SCLP_FC_NORMAL_WRITE; >> + sccb->h.control_mask[0] = 0; >> + sccb->h.control_mask[1] = 0; >> + sccb->h.control_mask[2] = 0; > > I opted for a memset and I would prefer it here too, as I'll introduce > it anyway. Make sense, but only for the mask and not the whole header I guess? -- Thanks, David / dhildenb
