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

[Jacob Keller <jacob.e.keller@intel.com>]

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.