netdev - Re: [PATCH net] dsa: mv88e6xxx: Ensure all pending interrupts are handled prior to exit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 11 Feb 2019 19:57:03 -0500
From:   John David Anglin <dave.anglin@...l.net>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Russell King <linux@....linux.org.uk>,
        Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
        Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net] dsa: mv88e6xxx: Ensure all pending interrupts are
 handled prior to exit

On 2019-02-11 6:33 p.m., Andrew Lunn wrote:
>> Signed-off-by:  John David Anglin <dave.anglin@...l.net>
>> ---
>>  drivers/net/dsa/mv88e6xxx/chip.c | 28 ++++++++++++++++++++++------
>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 8dca2c949e73..12fd7ce3f1ff 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -261,6 +261,7 @@ static irqreturn_t mv88e6xxx_g1_irq_thread_work(struct mv88e6xxx_chip *chip)
>>  	unsigned int sub_irq;
>>  	unsigned int n;
>>  	u16 reg;
>> +	u16 ctl1;
>>  	int err;
>>
>>  	mutex_lock(&chip->reg_lock);
>> @@ -270,13 +271,28 @@ static irqreturn_t mv88e6xxx_g1_irq_thread_work(struct mv88e6xxx_chip *chip)
>>  	if (err)
>>  		goto out;
>>
>> -	for (n = 0; n < chip->g1_irq.nirqs; ++n) {
>> -		if (reg & (1 << n)) {
>> -			sub_irq = irq_find_mapping(chip->g1_irq.domain, n);
>> -			handle_nested_irq(sub_irq);
>> -			++nhandled;
>> +	do {
>> +		for (n = 0; n < chip->g1_irq.nirqs; ++n) {
>> +			if (reg & (1 << n)) {
>> +				sub_irq = irq_find_mapping(chip->g1_irq.domain,
>> +							   n);
>> +				handle_nested_irq(sub_irq);
>> +				++nhandled;
>> +			}
>>  		}
>> -	}
>> +
>> +		mutex_lock(&chip->reg_lock);
>> +		err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_CTL1, &ctl1);
>> +		if (err)
>> +			goto unlock;
>> +		err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_STS, &reg);
>> +unlock:
>> +		mutex_unlock(&chip->reg_lock);
>> +		if (err)
>> +			goto out;
>> +		ctl1 &= GENMASK(chip->g1_irq.nirqs, 0);
>> +	} while (reg & ctl1);
> Hi David
>
> I just tested this on one of my boards. It loops endlessly:
>
> [   47.173396] mv88e6xxx_g1_irq_thread_work: c881 a8 80                         
> [   47.182108] mv88e6xxx_g1_irq_thread_work: c881 a8 80                         
> [   47.190820] mv88e6xxx_g1_irq_thread_work: c881 a8 80                         
> [   47.199535] mv88e6xxx_g1_irq_thread_work: c881 a8 80                         
> [   47.208254] mv88e6xxx_g1_irq_thread_work: c881 a8 80   
>
> These are reg, ctl1, reg & ctl1.
>
> So there is an unhandled device interrupt. I think this is because
> device interrupts are not masked before installing the interrupt
> handler. But i've not fully got to the bottom of this yet.
Yes, it is true the PHY and SERDES enables in Global 2 should be cleared before the interrupt handler
is installed for device interrupts.  That's what is done for the interrupts enables in Global 1.  I'm
not seeing that these enables are initialized.

Which switch?  The device interrupts are not be cleared properly on that board.  Would it be possible
to also print the Global 2 status and enables?  Unplugging the cable that's causing the loop might
cause the loop to stop.

I suspect the same would happen if level interrupts were used.

I tested both edge and polling on espressobin with Armada 3700.  There's no problem with
looping there.  I've booted it many times.  I've unplugged and plugged cables many times.

Dave

-- 
John David Anglin  dave.anglin@...l.net