On Sun, 26 Apr 2020 19:39:16 +0900 (JST), Atsushi Nemoto <anemo@xxxxxxxxxxxxx> wrote:
>>> + * @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.)
>
> Then, how about 'isr_mutex' ? Should I resend a patch with new name?
>
>>> */
>>> 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.
>
> Yes, but just dropping the spinlock will leak two code pathes unlocked:
>
> 1. altr_i2c_int_enable() at end of altr_i2c_init()
> 2. altr_i2c_int_enable() just after wait_for_completion_timeout()
>
> We can kill the spinlock by adding some more mutex_lock, but I think
> that is not a _fix_ but _cleanup_, so I would like to do in another
> patch.
I will send a revised patch renaming 'mutex' to 'isr_mutex' (with
Thor's Acked-by line) and a patch to cleanup the spinlock.
---
Atsushi Nemoto
