On Sun, 26 Apr 2020 10:30:31 +0200, Wolfram Sang <wsa@xxxxxxxxxxxxx> wrote:
>> --- 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.)
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.
---
Atsushi Nemoto
