On 11 December 2014 at 03:34, Peter Maydell <peter.mayd...@linaro.org>
wrote:
> On 10 December 2014 at 23:18, Greg Bellows <greg.bell...@linaro.org>
> wrote:
> >
> >
> > On 10 December 2014 at 16:50, Peter Maydell <peter.mayd...@linaro.org>
> > wrote:
> >>
> >> On 10 December 2014 at 22:26, Greg Bellows <greg.bell...@linaro.org>
> >> wrote:
> >> >
> >> >
> >> > On 9 December 2014 at 13:46, Peter Maydell <peter.mayd...@linaro.org>
> >> > wrote:
> >> >> +static bool raw_accessors_valid(const ARMCPRegInfo *ri)
> >> >> +{
> >> >> + /* Return true if a raw access on this register is OK (ie will
> not
> >> >> + * fall into the assert in raw_read() or raw_write())
> >> >> + */
> >> >
> >> >
> >> > I believe this comment is somewhat misleading as there are registers
> >> > that
> >> > would return true from this function yet still hit the aforementioned
> >> > asserts.
> >>
> >> Really? I think it is misleading (really it will return false if
> >> a raw access is definitely not valid, but may return true even if
> >> a raw access is still a bad idea), but I don't think there are any
> >> cases that would return true and then hit the assert.
> >>
> >
> > If you called the routine on PMCCNTR, for instance, this routine would
> > return true and if you then called raw_read or raw_write you would hit
> the
> > assert, correct? This may be contrived, but I believe there are cases
> that
> > the comment is incorrect.
>
> Ah, I see the confusion. By 'raw access' in the comment I meant "a call
> to read_raw_cp_reg/write_raw_cp_reg" -- doing that for PMCCNTR will
> end up calling its read and write accessors, so we don't fall into
> the raw_read() or raw_write() calls and won't hit the assert.
> How about we invert the sense of the function and call it
> raw_accessors_invalid(), and make the comment read:
>
> /* Return true if the regdef would cause an assertion if you called
> * read_raw_cp_reg() or write_raw_cp_reg() on it (ie if it is a
> * program bug for it not to have the NO_RAW flag).
> * NB that returning false here doesn't necessarily mean that calling
> * read/write_raw_cp_reg() is safe, because we can't distinguish "has
> * read/write access functions which are safe for raw use" from "has
> * read/write access functions which have side effects but has forgotten
> * to provide raw access functions".
> * The tests here line up with the conditions in read/write_raw_cp_reg()
> * and assertions in raw_read()/raw_write().
> */
>
> ?
>
>
The comment is certainly clearer. I'm not sure you need the NB, but there
is never harm in too much detail.
> -- PMM
>
