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(). */ ? -- PMM