[PATCH -next] i2c: designware: Fix improper usage of readl

[PATCH -next] i2c: designware: Fix improper usage of readl

[Jan Dabros]

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(-)

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

Re: [PATCH -next] i2c: designware: Fix improper usage of readl

[Andy Shevchenko]

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>

> 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(-)
> 
> 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

Re: [PATCH -next] i2c: designware: Fix improper usage of readl

[Jarkko Nikula]

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>

Re: [PATCH -next] i2c: designware: Fix improper usage of readl

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH -next] i2c: designware: Fix improper usage of readl

[Jan Dąbroś]

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

Re: [PATCH -next] i2c: designware: Fix improper usage of readl

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 234 bytes --]


> 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!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]