On Thu, Jun 06, 2019 at 02:57:22PM -0600, Robert Hancock wrote:
> 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).
That's a good point, one that we should address separately. Rather
than re-using the existing mutex (which would be difficult to hold
across calling the state machine due to locking inversion) how about
this:
drivers/net/phy/sfp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 8a21294d1ce8..5ff427dcbb31 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -190,6 +190,7 @@ struct sfp {
struct delayed_work poll;
struct delayed_work timeout;
struct mutex sm_mutex;
+ struct mutex st_mutex;
unsigned char sm_mod_state;
unsigned char sm_dev_state;
unsigned short sm_state;
@@ -1976,6 +1977,7 @@ static void sfp_check_state(struct sfp *sfp)
{
unsigned int state, i, changed;
+ mutex_lock(&sfp->st_mutex);
state = sfp_get_state(sfp);
changed = state ^ sfp->state;
changed &= SFP_F_PRESENT | SFP_F_LOS | SFP_F_TX_FAULT;
@@ -2001,6 +2003,7 @@ static void sfp_check_state(struct sfp *sfp)
sfp_sm_event(sfp, state & SFP_F_LOS ?
SFP_E_LOS_HIGH : SFP_E_LOS_LOW);
rtnl_unlock();
+ mutex_unlock(&sfp->st_mutex);
}
static irqreturn_t sfp_irq(int irq, void *data)
@@ -2031,6 +2034,7 @@ static struct sfp *sfp_alloc(struct device *dev)
sfp->dev = dev;
mutex_init(&sfp->sm_mutex);
+ mutex_init(&sfp->st_mutex);
INIT_DELAYED_WORK(&sfp->poll, sfp_poll);
INIT_DELAYED_WORK(&sfp->timeout, sfp_timeout);
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
