Re: [RFC] ACPI, APEI: Fix incorrect bit width + offset check condition

[Gary Hade <garyhade@xxxxxxxxxx>]

On Thu, Jun 14, 2012 at 10:09:07AM +0200, Jean Delvare wrote:
> On Thu, 14 Jun 2012 14:14:30 +0800, Xiao, Hui wrote:
> > From your "good example of a valid case" above. I believe we might have different
> > understanding of the "Bit Width" field. 
> > 
> > Just to make sure, do you take "Bit Width" here(1 bit) as the bit length one should 
> > got for mask /*after*/ shifting bit offset(31 bit) of the access_width(32 bit) 
> > one read from the register(length unknown, or should equal to access length?) ?
> > 
> > That's why you think:
> >        bit_width + bit_offset <= *access_bit_width
> > is valid.
> 
> I am not Gary, but it is also how I read the specification.

Thanks, Jean.  It seemed like the correct interpretation to me.

> 
> > For me I take "Bit Width" as bits of the register for access boundary,
> > so I think:
> >        (*access_bit_width <= bit_width) && (bit_offset < *access_bit_width)
> > is valid. 

This is not the check that the patch contains.  It also does not
verify that an access will read or write all of the register bits.

> > 
> > For you above case, personally I saw you got a 1-bit register, but want to
> > read 32bit from it, and want to get bit[31] by shifting 31bit and mask 0x1.
> > 
> > Please correct me if I am wrong. Not sure which should be the case ACPI SPEC
> > expected. I also have not found any specific explanation on these assumption. 
> 
> What makes me believe Gary is right is the granularity of each field.
> bit_width and bit_offset can be set with a 1-bit granularity, while
> access_bit_width can only be 8, 16, 32 or 64. This clearly means that
> access_bit_width (and Access Size before that) is a hardware thing,
> while bit_width and bit_offset can only be software things. You've
> never seen I/O ports that can be read 3 or 5 bits at a time...

The "<= Access Size" in this diagram will hopefully clarify the
"bit_width + bit_offset <= *access_bit_width" check:

    |<------------------  <= Access Size ----------------->|
    |<-- Register Bit Width -->|<-- Register Bit Offset -->|
 |<--------------------- Access Size --------------------->|
                                                           ^
                                                           |
                                                 Address --+

The case described in the patch header looks like:

   |<-- Register Bit Width (64) -->|<-- Register Bit Offset (0) -->|
   |<-------+------>|<----------- Access Size (32)---------------->|
            |                                                      ^
            +-- **neglected register bits**                        |        
                                                         Address --+

The 1 bit width register example I provided looks like:

   |<-- Register Bit Width (1) -->|<-- Register Bit Offset (31) -->|
   |<------------------ Access Size (32)-------------------------->|
                                                                   ^
                                                                   |
                                                         Address --+

Gary

-- 
Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
garyhade@xxxxxxxxxx
http://www.ibm.com/linux/ltc

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html