Russell King says: ==================== net: phy: avoid race when erroring stopping PHY This series addresses a problem reported by Jijie Shao where the PHY state machine can race with phy_stop() leading to an incorrect state. The issue centres around phy_state_machine() dropping the phydev->lock mutex briefly, which allows phy_stop() to get in half-way through the state machine, and when the state machine resumes, it overwrites phydev->state with a value incompatible with a stopped PHY. This causes a subsequent phy_start() to issue a warning. We address this firstly by using versions of functions that do not take tne lock, moving them into the locked region. The only function that this can't be done with is phy_suspend() which needs to call into the driver without taking the lock. For phy_suspend(), we split the state machine into two parts - the initial part which runs under the phydev->lock, and the second part which runs without the lock. We finish off by using the split state machine in phy_stop() which removes another unnecessary unlock-lock sequence from phylib. Changes from RFC: - Added Jijie Shao's tested-by ====================
Unified · Split
| @@ -1231,9 +1231,7 @@ static void phy_error_precise(struct phy_device *phydev, · 1230 unmodified lines | ||
| 1231 | 1231 | const void *func, int err) |
| 1232 | 1232 | { |
| 1233 | 1233 | WARN(1, "%pS: returned: %d\n", func, err); |
| 1234 | − mutex_lock(&phydev->lock); | |
| 1235 | 1234 | phy_process_error(phydev); |
| 1236 | − mutex_unlock(&phydev->lock); | |
| 1237 | 1235 | } |
| 1238 | 1236 | |
| 1239 | 1237 | /** |
| @@ -1355,6 +1353,113 @@ void phy_free_interrupt(struct phy_device *phydev) · 115 unmodified lines | ||
| 1355 | 1353 | } |
| 1356 | 1354 | EXPORT_SYMBOL(phy_free_interrupt); |
| 1357 | 1355 | |
| 1356 | +enum phy_state_work { | |
| 1357 | + PHY_STATE_WORK_NONE, | |
| 1358 | + PHY_STATE_WORK_ANEG, | |
| 1359 | + PHY_STATE_WORK_SUSPEND, | |
| 1360 | +}; | |
| 1361 | + | |
| 1362 | +static enum phy_state_work _phy_state_machine(struct phy_device *phydev) | |
| 1363 | +{ | |
| 1364 | + enum phy_state_work state_work = PHY_STATE_WORK_NONE; | |
| 1365 | + struct net_device *dev = phydev->attached_dev; | |
| 1366 | + enum phy_state old_state = phydev->state; | |
| 1367 | + const void *func = NULL; | |
| 1368 | + bool finished = false; | |
| 1369 | + int err = 0; | |
| 1370 | + | |
| 1371 | + switch (phydev->state) { | |
| 1372 | + case PHY_DOWN: | |
| 1373 | + case PHY_READY: | |
| 1374 | + break; | |
| 1375 | + case PHY_UP: | |
| 1376 | + state_work = PHY_STATE_WORK_ANEG; | |
| 1377 | + break; | |
| 1378 | + case PHY_NOLINK: | |
| 1379 | + case PHY_RUNNING: | |
| 1380 | + err = phy_check_link_status(phydev); | |
| 1381 | + func = &phy_check_link_status; | |
| 1382 | + break; | |
| 1383 | + case PHY_CABLETEST: | |
| 1384 | + err = phydev->drv->cable_test_get_status(phydev, &finished); | |
| 1385 | + if (err) { | |
| 1386 | + phy_abort_cable_test(phydev); | |
| 1387 | + netif_testing_off(dev); | |
| 1388 | + state_work = PHY_STATE_WORK_ANEG; | |
| 1389 | + phydev->state = PHY_UP; | |
| 1390 | + break; | |
| 1391 | + } | |
| 1392 | + | |
| 1393 | + if (finished) { | |
| 1394 | + ethnl_cable_test_finished(phydev); | |
| 1395 | + netif_testing_off(dev); | |
| 1396 | + state_work = PHY_STATE_WORK_ANEG; | |
| 1397 | + phydev->state = PHY_UP; | |
| 1398 | + } | |
| 1399 | + break; | |
| 1400 | + case PHY_HALTED: | |
| 1401 | + case PHY_ERROR: | |
| 1402 | + if (phydev->link) { | |
| 1403 | + phydev->link = 0; | |
| 1404 | + phy_link_down(phydev); | |
| 1405 | + } | |
| 1406 | + state_work = PHY_STATE_WORK_SUSPEND; | |
| 1407 | + break; | |
| 1408 | + } | |
| 1409 | + | |
| 1410 | + if (state_work == PHY_STATE_WORK_ANEG) { | |
| 1411 | + err = _phy_start_aneg(phydev); | |
| 1412 | + func = &_phy_start_aneg; | |
| 1413 | + } | |
| 1414 | + | |
| 1415 | + if (err == -ENODEV) | |
| 1416 | + return state_work; | |
| 1417 | + | |
| 1418 | + if (err < 0) | |
| 1419 | + phy_error_precise(phydev, func, err); | |
| 1420 | + | |
| 1421 | + phy_process_state_change(phydev, old_state); | |
| 1422 | + | |
| 1423 | + /* Only re-schedule a PHY state machine change if we are polling the | |
| 1424 | + * PHY, if PHY_MAC_INTERRUPT is set, then we will be moving | |
| 1425 | + * between states from phy_mac_interrupt(). | |
| 1426 | + * | |
| 1427 | + * In state PHY_HALTED the PHY gets suspended, so rescheduling the | |
| 1428 | + * state machine would be pointless and possibly error prone when | |
| 1429 | + * called from phy_disconnect() synchronously. | |
| 1430 | + */ | |
| 1431 | + if (phy_polling_mode(phydev) && phy_is_started(phydev)) | |
| 1432 | + phy_queue_state_machine(phydev, PHY_STATE_TIME); | |
| 1433 | + | |
| 1434 | + return state_work; | |
| 1435 | +} | |
| 1436 | + | |
| 1437 | +/* unlocked part of the PHY state machine */ | |
| 1438 | +static void _phy_state_machine_post_work(struct phy_device *phydev, | |
| 1439 | + enum phy_state_work state_work) | |
| 1440 | +{ | |
| 1441 | + if (state_work == PHY_STATE_WORK_SUSPEND) | |
| 1442 | + phy_suspend(phydev); | |
| 1443 | +} | |
| 1444 | + | |
| 1445 | +/** | |
| 1446 | + * phy_state_machine - Handle the state machine | |
| 1447 | + * @work: work_struct that describes the work to be done | |
| 1448 | + */ | |
| 1449 | +void phy_state_machine(struct work_struct *work) | |
| 1450 | +{ | |
| 1451 | + struct delayed_work *dwork = to_delayed_work(work); | |
| 1452 | + struct phy_device *phydev = | |
| 1453 | + container_of(dwork, struct phy_device, state_queue); | |
| 1454 | + enum phy_state_work state_work; | |
| 1455 | + | |
| 1456 | + mutex_lock(&phydev->lock); | |
| 1457 | + state_work = _phy_state_machine(phydev); | |
| 1458 | + mutex_unlock(&phydev->lock); | |
| 1459 | + | |
| 1460 | + _phy_state_machine_post_work(phydev, state_work); | |
| 1461 | +} | |
| 1462 | + | |
| 1358 | 1463 | /** |
| 1359 | 1464 | * phy_stop - Bring down the PHY link, and stop checking the status |
| 1360 | 1465 | * @phydev: target phy_device struct |
| @@ -1362,6 +1467,7 @@ EXPORT_SYMBOL(phy_free_interrupt); · 1 unmodified line | ||
| 1362 | 1467 | void phy_stop(struct phy_device *phydev) |
| 1363 | 1468 | { |
| 1364 | 1469 | struct net_device *dev = phydev->attached_dev; |
| 1470 | + enum phy_state_work state_work; | |
| 1365 | 1471 | enum phy_state old_state; |
| 1366 | 1472 | |
| 1367 | 1473 | if (!phy_is_started(phydev) && phydev->state != PHY_DOWN && |
| @@ -1385,9 +1491,10 @@ void phy_stop(struct phy_device *phydev) · 17 unmodified lines | ||
| 1385 | 1491 | phydev->state = PHY_HALTED; |
| 1386 | 1492 | phy_process_state_change(phydev, old_state); |
| 1387 | 1493 | |
| 1494 | + state_work = _phy_state_machine(phydev); | |
| 1388 | 1495 | mutex_unlock(&phydev->lock); |
| 1389 | 1496 | |
| 1390 | − phy_state_machine(&phydev->state_queue.work); | |
| 1497 | + _phy_state_machine_post_work(phydev, state_work); | |
| 1391 | 1498 | phy_stop_machine(phydev); |
| 1392 | 1499 | |
| 1393 | 1500 | /* Cannot call flush_scheduled_work() here as desired because |
| @@ -1431,97 +1538,6 @@ out: · 37 unmodified lines | ||
| 1431 | 1538 | } |
| 1432 | 1539 | EXPORT_SYMBOL(phy_start); |
| 1433 | 1540 | |
| 1434 | −/** | |
| 1435 | − * phy_state_machine - Handle the state machine | |
| 1436 | − * @work: work_struct that describes the work to be done | |
| 1437 | − */ | |
| 1438 | −void phy_state_machine(struct work_struct *work) | |
| 1439 | −{ | |
| 1440 | − struct delayed_work *dwork = to_delayed_work(work); | |
| 1441 | − struct phy_device *phydev = | |
| 1442 | − container_of(dwork, struct phy_device, state_queue); | |
| 1443 | − struct net_device *dev = phydev->attached_dev; | |
| 1444 | − bool needs_aneg = false, do_suspend = false; | |
| 1445 | − enum phy_state old_state; | |
| 1446 | − const void *func = NULL; | |
| 1447 | − bool finished = false; | |
| 1448 | − int err = 0; | |
| 1449 | − | |
| 1450 | − mutex_lock(&phydev->lock); | |
| 1451 | − | |
| 1452 | − old_state = phydev->state; | |
| 1453 | − | |
| 1454 | − switch (phydev->state) { | |
| 1455 | − case PHY_DOWN: | |
| 1456 | − case PHY_READY: | |
| 1457 | − break; | |
| 1458 | − case PHY_UP: | |
| 1459 | − needs_aneg = true; | |
| 1460 | − | |
| 1461 | − break; | |
| 1462 | − case PHY_NOLINK: | |
| 1463 | − case PHY_RUNNING: | |
| 1464 | − err = phy_check_link_status(phydev); | |
| 1465 | − func = &phy_check_link_status; | |
| 1466 | − break; | |
| 1467 | − case PHY_CABLETEST: | |
| 1468 | − err = phydev->drv->cable_test_get_status(phydev, &finished); | |
| 1469 | − if (err) { | |
| 1470 | − phy_abort_cable_test(phydev); | |
| 1471 | − netif_testing_off(dev); | |
| 1472 | − needs_aneg = true; | |
| 1473 | − phydev->state = PHY_UP; | |
| 1474 | − break; | |
| 1475 | − } | |
| 1476 | − | |
| 1477 | − if (finished) { | |
| 1478 | − ethnl_cable_test_finished(phydev); | |
| 1479 | − netif_testing_off(dev); | |
| 1480 | − needs_aneg = true; | |
| 1481 | − phydev->state = PHY_UP; | |
| 1482 | − } | |
| 1483 | − break; | |
| 1484 | − case PHY_HALTED: | |
| 1485 | − case PHY_ERROR: | |
| 1486 | − if (phydev->link) { | |
| 1487 | − phydev->link = 0; | |
| 1488 | − phy_link_down(phydev); | |
| 1489 | − } | |
| 1490 | − do_suspend = true; | |
| 1491 | − break; | |
| 1492 | − } | |
| 1493 | − | |
| 1494 | − mutex_unlock(&phydev->lock); | |
| 1495 | − | |
| 1496 | − if (needs_aneg) { | |
| 1497 | − err = phy_start_aneg(phydev); | |
| 1498 | − func = &phy_start_aneg; | |
| 1499 | − } else if (do_suspend) { | |
| 1500 | − phy_suspend(phydev); | |
| 1501 | − } | |
| 1502 | − | |
| 1503 | − if (err == -ENODEV) | |
| 1504 | − return; | |
| 1505 | − | |
| 1506 | − if (err < 0) | |
| 1507 | − phy_error_precise(phydev, func, err); | |
| 1508 | − | |
| 1509 | − phy_process_state_change(phydev, old_state); | |
| 1510 | − | |
| 1511 | − /* Only re-schedule a PHY state machine change if we are polling the | |
| 1512 | − * PHY, if PHY_MAC_INTERRUPT is set, then we will be moving | |
| 1513 | − * between states from phy_mac_interrupt(). | |
| 1514 | − * | |
| 1515 | − * In state PHY_HALTED the PHY gets suspended, so rescheduling the | |
| 1516 | − * state machine would be pointless and possibly error prone when | |
| 1517 | − * called from phy_disconnect() synchronously. | |
| 1518 | − */ | |
| 1519 | − mutex_lock(&phydev->lock); | |
| 1520 | − if (phy_polling_mode(phydev) && phy_is_started(phydev)) | |
| 1521 | − phy_queue_state_machine(phydev, PHY_STATE_TIME); | |
| 1522 | − mutex_unlock(&phydev->lock); | |
| 1523 | −} | |
| 1524 | − | |
| 1525 | 1541 | /** |
| 1526 | 1542 | * phy_mac_interrupt - MAC says the link has changed |
| 1527 | 1543 | * @phydev: phy_device struct with changed link |