netdev - Re: [PATCH net-next] net: sfp: Stop SFP polling and interrupt handling during shutdown

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 for Android: free password hash cracker in your pocket

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

Message-ID: <1d43c48d-1460-d12b-2213-9a0eeb6affb8@gmail.com>
Date:   Thu, 6 Jun 2019 20:21:24 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Robert Hancock <hancock@...systems.ca>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     netdev@...r.kernel.org, andrew@...n.ch, hkallweit1@...il.com
Subject: Re: [PATCH net-next] net: sfp: Stop SFP polling and interrupt
 handling during shutdown



On 6/6/2019 1:57 PM, Robert Hancock wrote:
> On 2019-06-06 12:09 p.m., Russell King - ARM Linux admin wrote:
>>> @@ -1466,6 +1467,11 @@ static void sfp_sm_mod_remove(struct sfp *sfp)
>>>  static void sfp_sm_event(struct sfp *sfp, unsigned int event)
>>>  {
>>>  	mutex_lock(&sfp->sm_mutex);
>>> +	if (unlikely(sfp->shutdown)) {
>>> +		/* Do not handle any more state machine events. */
>>> +		mutex_unlock(&sfp->sm_mutex);
>>> +		return;
>>> +	}
>>>  
>>>  	dev_dbg(sfp->dev, "SM: enter %s:%s:%s event %s\n",
>>>  		mod_state_to_str(sfp->sm_mod_state),
>>> @@ -1704,6 +1710,13 @@ static void sfp_check_state(struct sfp *sfp)
>>>  {
>>>  	unsigned int state, i, changed;
>>>  
>>> +	mutex_lock(&sfp->sm_mutex);
>>> +	if (unlikely(sfp->shutdown)) {
>>> +		/* No more state checks */
>>> +		mutex_unlock(&sfp->sm_mutex);
>>> +		return;
>>> +	}
>>> +
>>
>> I don't think you need to add the mutex locking - just check for
>> sfp->shutdown and be done with it...
> 
> The idea there was to deal with the case where GPIO interrupts were
> previously raised before shutdown and not yet handled by the threaded
> interrupt handler by the time shutdown is called. After shutdown on the
> SFP completes, the bus the GPIO stuff is on could potentially be shut
> down at any moment, so we really don't want to be digging into the GPIO
> states after that. Locking the mutex there ensures that we don't read a
> stale value for the shutdown flag in the interrupt handler, since AFAIK
> there's no other synchronization around that value.
> 
> It may also be helpful that the lock is now held for the subsequent code
> in sfp_check_state that's comparing the previous and new states - it
> seems like you could otherwise run into trouble if that function was
> being concurrently called from the polling thread and the interrupt
> handler (for example if you had an SFP where some GPIOs supported
> interrupts and some didn't).

Would not it be sufficient to call disable_irq() or devm_free_irq() (to
match the devm_request_threaded_irq call) in order to achieve what you
want here?
-- 
Florian