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 <[email protected]>
Reported-by: kernel test robot <[email protected]>
---
drivers/i2c/busses/i2c-designware-amdpsp.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
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:
--
2.35.1.265.g69c8d7142f-goog
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 <[email protected]>
> Signed-off-by: Jan Dabros <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> ---
> drivers/i2c/busses/i2c-designware-amdpsp.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> 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:
> --
> 2.35.1.265.g69c8d7142f-goog
>
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
>
Acked-by: Jarkko Nikula <[email protected]>
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 <[email protected]>
> Reported-by: kernel test robot <[email protected]>
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.
> 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!
wt., 1 mar 2022 o 16:16 Wolfram Sang <[email protected]> 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 <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
>
> 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