Re: [PATCH] i2c: altera: Fix race between xfer_msg and isr thread

[Wolfram Sang <wsa@the-dreams.de>]

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

On Fri, Apr 10, 2020 at 10:06:40AM +0900, Atsushi Nemoto wrote:
> Use a mutex to protect access to idev->msg_len, idev->buf, etc. which
> are modified by both altr_i2c_xfer_msg() and altr_i2c_isr().
> 
> Signed-off-by: Atsushi Nemoto <atsushi.nemoto@sord.co.jp>
> ---
>  drivers/i2c/busses/i2c-altera.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-altera.c b/drivers/i2c/busses/i2c-altera.c
> index 20ef63820c77..3db7d77c5a1e 100644
> --- a/drivers/i2c/busses/i2c-altera.c
> +++ b/drivers/i2c/busses/i2c-altera.c
> @@ -70,6 +70,7 @@
>   * @isr_mask: cached copy of local ISR enables.
>   * @isr_status: cached copy of local ISR status.
>   * @lock: spinlock for IRQ synchronization.
> + * @mutex: mutex for IRQ thread.

I think the name 'mutex' is too unspecific. (Same goes for 'lock' above
which is not part of your patch, obviously.)

>   */
>  struct altr_i2c_dev {
>  	void __iomem *base;
> @@ -86,6 +87,7 @@ struct altr_i2c_dev {
>  	u32 isr_mask;
>  	u32 isr_status;
>  	spinlock_t lock;	/* IRQ synchronization */
> +	struct mutex mutex;
>  };

Has it been checked if we really need both, the spinlock and the mutex?
From a glimpse, it looks like the spinlock became obsolete now.


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