| Message ID | 20220218133348.628962-1-jsd@semihalf.com |
|---|---|
| State | Accepted |
| Commit | 17ba1e87fca9e6bad7eacbb1ef042c83358d245e |
| Headers | show |
| Series | [-next] i2c: designware: Fix improper usage of readl | expand |
On 2/23/22 17:07, Andy Shevchenko wrote: > On Fri, Feb 18, 2022 at 02:33:48PM +0100, Jan Dabros wrote: >> Kernel test robot reported incorrect type in argument 1 of readl(), but >> more importantly it brought attention that MMIO accessor shouldn't be >> used in this case, since req->hdr.status is part of a command-response >> buffer in system memory. >> >> Since its value may be altered by PSP outside of the scope of current >> thread (somehow similar to IRQ handler case), we need to use >> READ_ONCE() to ensure compiler won't optimize this call. >> >> Fix also 'status' variable type to reflect that corresponding field in >> command-response buffer is platform-independent u32. > > Thanks for the fix, seems reasonable to me. > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
On Fri, Feb 18, 2022 at 02:33:48PM +0100, Jan Dabros wrote: > Kernel test robot reported incorrect type in argument 1 of readl(), but > more importantly it brought attention that MMIO accessor shouldn't be > used in this case, since req->hdr.status is part of a command-response > buffer in system memory. > > Since its value may be altered by PSP outside of the scope of current > thread (somehow similar to IRQ handler case), we need to use > READ_ONCE() to ensure compiler won't optimize this call. > > Fix also 'status' variable type to reflect that corresponding field in > command-response buffer is platform-independent u32. > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > Reported-by: kernel test robot <lkp@intel.com> Applied to for-next, thanks! Jan, I wonder if you want to be the maintainer for this driver? If you'd like, then please send me the patch adding you to MAINTAINERS. So, you will get notified if people want to enhance this driver.
wt., 1 mar 2022 o 16:16 Wolfram Sang <wsa@kernel.org> napisał(a): > > On Fri, Feb 18, 2022 at 02:33:48PM +0100, Jan Dabros wrote: > > Kernel test robot reported incorrect type in argument 1 of readl(), but > > more importantly it brought attention that MMIO accessor shouldn't be > > used in this case, since req->hdr.status is part of a command-response > > buffer in system memory. > > > > Since its value may be altered by PSP outside of the scope of current > > thread (somehow similar to IRQ handler case), we need to use > > READ_ONCE() to ensure compiler won't optimize this call. > > > > Fix also 'status' variable type to reflect that corresponding field in > > command-response buffer is platform-independent u32. > > > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > > Reported-by: kernel test robot <lkp@intel.com> > > Applied to for-next, thanks! Thanks! > Jan, I wonder if you want to be the maintainer for this driver? If you'd > like, then please send me the patch adding you to MAINTAINERS. So, you > will get notified if people want to enhance this driver. So actually I've already added myself as a R:eviewer for i2c-designware-* files in one of the previous patches with the purpose of reviewing code touching this driver. This makes sense since I can also test modifications on my device. Best Regards, Jan
> So actually I've already added myself as a R:eviewer for > i2c-designware-* files in one of the previous patches with the purpose Ah, this is perfect then. I was only grepping for amdpsp, so I missed it. Thanks for the heads up!
diff --git a/drivers/i2c/busses/i2c-designware-amdpsp.c b/drivers/i2c/busses/i2c-designware-amdpsp.c index 752e0024db03..c64e459afb5c 100644 --- a/drivers/i2c/busses/i2c-designware-amdpsp.c +++ b/drivers/i2c/busses/i2c-designware-amdpsp.c @@ -160,9 +160,10 @@ static int psp_send_cmd(struct psp_i2c_req *req) /* Helper to verify status returned by PSP */ static int check_i2c_req_sts(struct psp_i2c_req *req) { - int status; + u32 status; - status = readl(&req->hdr.status); + /* Status field in command-response buffer is updated by PSP */ + status = READ_ONCE(req->hdr.status); switch (status) { case PSP_I2C_REQ_STS_OK:
Kernel test robot reported incorrect type in argument 1 of readl(), but more importantly it brought attention that MMIO accessor shouldn't be used in this case, since req->hdr.status is part of a command-response buffer in system memory. Since its value may be altered by PSP outside of the scope of current thread (somehow similar to IRQ handler case), we need to use READ_ONCE() to ensure compiler won't optimize this call. Fix also 'status' variable type to reflect that corresponding field in command-response buffer is platform-independent u32. Signed-off-by: Jan Dabros <jsd@semihalf.com> Reported-by: kernel test robot <lkp@intel.com> --- drivers/i2c/busses/i2c-designware-amdpsp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)