On Mon, Feb 14, 2022 at 02:59:34PM +0100, Jan Dąbroś wrote:
> pon., 14 lut 2022 o 14:28 Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> napisał(a):
> >
> > On Mon, Feb 14, 2022 at 01:27:35PM +0100, Jan Dąbroś wrote:
> > > pt., 11 lut 2022 o 22:24 kernel test robot <lkp@xxxxxxxxx> napisał(a):
> >
> > > > 159
> > > > 160 /* Helper to verify status returned by PSP */
> > > > 161 static int check_i2c_req_sts(struct psp_i2c_req *req)
> > > > 162 {
> > > > 163 int status;
> > > > 164
> > > > > 165 status = readl(&req->hdr.status);
> > >
> > > Actually the above error points to something hidden but important -
> > > for reading from command-response buffer, we shouldn't use __iomem
> > > specifier (nor readl() family of functions) since this is normal
> > > memory - however updated by PSP. Thus I will refactor this to use
> > > 'volatile u32 *' and reading status by de-referencing pointer.
> >
> > Not sure volatile is a good idea. Perhaps READ_ONCE() is what you need.
> > Is this a system memory?
>
> Yes, this is system memory.
>
> Actually looking at asm-generic/rwonce.h:
> #define __READ_ONCE(x) (*(const volatile __unqual_scalar_typeof(x) *)&(x))
> it is more-less based on volatile, so that compiler will not be able
> to (among others) optimize out such reads of memory which may be
> changed outside of the scope of "program".
>
> I believe that I will get the same outcome from using READ_ONCE and
> explicit volatile, is the first way preferred in the kernel?
READ_ONCE() may be different on different arches. I believe that's why
it's preferred.
--
With Best Regards,
Andy Shevchenko
