Re: [Intel-wired-lan] [bug report] igc: add lock preventing multiple simultaneous PTM transactions

Jacob Keller
2025-04-16T17:38:44Z

On 4/16/2025 4:09 AM, Dan Carpenter wrote:
> Hello Christopher S M Hall,
>
> Commit 1a931c4f5e68 ("igc: add lock preventing multiple simultaneous
> PTM transactions") from Apr 1, 2025 (linux-next), leads to the
> following Smatch static checker warning:
>
> drivers/net/ethernet/intel/igc/igc_ptp.c:1311 igc_ptp_reset()
> warn: sleeping in atomic context
>
> drivers/net/ethernet/intel/igc/igc_ptp.c
> 1280 void igc_ptp_reset(struct igc_adapter *adapter)
> 1281 {
> 1282 struct igc_hw *hw = &adapter->hw;
> 1283 u32 cycle_ctrl, ctrl, stat;
> 1284 unsigned long flags;
> 1285 u32 timadj;
> 1286
> 1287 if (!(adapter->ptp_flags & IGC_PTP_ENABLED))
> 1288 return;
> 1289
> 1290 /* reset the tstamp_config */
> 1291 igc_ptp_set_timestamp_mode(adapter, &adapter->tstamp_config);
> 1292
> 1293 spin_lock_irqsave(&adapter->tmreg_lock, flags);
> ^^^^^^^^^^^^^^^^^
> Holding a spin_lock
>
> 1294
> 1295 switch (adapter->hw.mac.type) {
> 1296 case igc_i225:
> 1297 timadj = rd32(IGC_TIMADJ);
> 1298 timadj |= IGC_TIMADJ_ADJUST_METH;
> 1299 wr32(IGC_TIMADJ, timadj);
> 1300
> 1301 wr32(IGC_TSAUXC, 0x0);
> 1302 wr32(IGC_TSSDP, 0x0);
> 1303 wr32(IGC_TSIM,
> 1304 IGC_TSICR_INTERRUPTS |
> 1305 (adapter->pps_sys_wrap_on ? IGC_TSICR_SYS_WRAP : 0));
> 1306 wr32(IGC_IMS, IGC_IMS_TS);
> 1307
> 1308 if (!igc_is_crosststamp_supported(adapter))
> 1309 break;
> 1310
> --> 1311 mutex_lock(&adapter->ptm_lock);
> ^^^^^^^^^^
> So we can't take a mutex.
>
> 1312 wr32(IGC_PCIE_DIG_DELAY, IGC_PCIE_DIG_DELAY_DEFAULT);
> 1313 wr32(IGC_PCIE_PHY_DELAY, IGC_PCIE_PHY_DELAY_DEFAULT);
> 1314
> 1315 cycle_ctrl = IGC_PTM_CYCLE_CTRL_CYC_TIME(IGC_PTM_CYC_TIME_DEFAULT);
> 1316
> 1317 wr32(IGC_PTM_CYCLE_CTRL, cycle_ctrl);
> 1318
> 1319 ctrl = IGC_PTM_CTRL_EN |
> 1320 IGC_PTM_CTRL_START_NOW |
> 1321 IGC_PTM_CTRL_SHRT_CYC(IGC_PTM_SHORT_CYC_DEFAULT) |
> 1322 IGC_PTM_CTRL_PTM_TO(IGC_PTM_TIMEOUT_DEFAULT);
> 1323
> 1324 wr32(IGC_PTM_CTRL, ctrl);
> 1325
> 1326 /* Force the first cycle to run. */
> 1327 igc_ptm_trigger(hw);
> 1328
> 1329 if (readx_poll_timeout_atomic(rd32, IGC_PTM_STAT, stat,
> 1330 stat, IGC_PTM_STAT_SLEEP,
> 1331 IGC_PTM_STAT_TIMEOUT))
> 1332 netdev_err(adapter->netdev, "Timeout reading IGC_PTM_STAT register\n");
> 1333
> 1334 igc_ptm_reset(hw);
> 1335 mutex_unlock(&adapter->ptm_lock);
> 1336 break;

I'm not sure why this was a mutex to begin with. We already use
readx_poll_timeout_atomic which doesn't sleep, so I think this ptm_lock
could just be converted to a spin_lock to fix this.