[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <daf98755-2818-1b3a-625b-8d899437fd02@intel.com>
Date: Thu, 3 Dec 2020 11:12:32 +0200
From: "Neftin, Sasha" <sasha.neftin@...el.com>
To: Kai-Heng Feng <kai.heng.feng@...onical.com>,
jesse.brandeburg@...el.com, anthony.l.nguyen@...el.com
Cc: "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
open list <linux-kernel@...r.kernel.org>,
stable@...r.kernel.org,
"moderated list:INTEL ETHERNET DRIVERS"
<intel-wired-lan@...ts.osuosl.org>,
Jakub Kicinski <kuba@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
"Neftin, Sasha" <sasha.neftin@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH] igc: Report speed and duplex as unknown
when device is runtime suspended
On 12/2/2020 09:50, Kai-Heng Feng wrote:
> Similar to commit 165ae7a8feb5 ("igb: Report speed and duplex as unknown
> when device is runtime suspended"), if we try to read speed and duplex
> sysfs while the device is runtime suspeneded, igc will complain and
> stops working:
>
> [ 123.449883] igc 0000:03:00.0 enp3s0: PCIe link lost, device now detached
> [ 123.450052] BUG: kernel NULL pointer dereference, address: 0000000000000008
> [ 123.450056] #PF: supervisor read access in kernel mode
> [ 123.450058] #PF: error_code(0x0000) - not-present page
> [ 123.450059] PGD 0 P4D 0
> [ 123.450064] Oops: 0000 [#1] SMP NOPTI
> [ 123.450068] CPU: 0 PID: 2525 Comm: udevadm Tainted: G U W OE 5.10.0-1002-oem #2+rkl2-Ubuntu
> [ 123.450078] RIP: 0010:igc_rd32+0x1c/0x90 [igc]
> [ 123.450080] Code: c0 5d c3 b8 fd ff ff ff c3 0f 1f 44 00 00 0f 1f 44 00 00 55 89 f0 48 89 e5 41 56 41 55 41 54 49 89 c4 53 48 8b 57 08 48 01 d0 <44> 8b 28 41 83 fd ff 74 0c 5b 44 89 e8 41 5c 41 5d 4
>
> [ 123.450083] RSP: 0018:ffffb0d100d6fcc0 EFLAGS: 00010202
> [ 123.450085] RAX: 0000000000000008 RBX: ffffb0d100d6fd30 RCX: 0000000000000000
> [ 123.450087] RDX: 0000000000000000 RSI: 0000000000000008 RDI: ffff945a12716c10
> [ 123.450089] RBP: ffffb0d100d6fce0 R08: ffff945a12716550 R09: ffff945a09874000
> [ 123.450090] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000008
> [ 123.450092] R13: ffff945a12716000 R14: ffff945a037da280 R15: ffff945a037da290
> [ 123.450094] FS: 00007f3b34c868c0(0000) GS:ffff945b89200000(0000) knlGS:0000000000000000
> [ 123.450096] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 123.450098] CR2: 0000000000000008 CR3: 00000001144de006 CR4: 0000000000770ef0
> [ 123.450100] PKRU: 55555554
> [ 123.450101] Call Trace:
> [ 123.450111] igc_ethtool_get_link_ksettings+0xd6/0x1b0 [igc]
> [ 123.450118] __ethtool_get_link_ksettings+0x71/0xb0
> [ 123.450123] duplex_show+0x74/0xc0
> [ 123.450129] dev_attr_show+0x1d/0x40
> [ 123.450134] sysfs_kf_seq_show+0xa1/0x100
> [ 123.450137] kernfs_seq_show+0x27/0x30
> [ 123.450142] seq_read+0xb7/0x400
> [ 123.450148] ? common_file_perm+0x72/0x170
> [ 123.450151] kernfs_fop_read+0x35/0x1b0
> [ 123.450155] vfs_read+0xb5/0x1b0
> [ 123.450157] ksys_read+0x67/0xe0
> [ 123.450160] __x64_sys_read+0x1a/0x20
> [ 123.450164] do_syscall_64+0x38/0x90
> [ 123.450168] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 123.450170] RIP: 0033:0x7f3b351fe142
> [ 123.450173] Code: c0 e9 c2 fe ff ff 50 48 8d 3d 3a ca 0a 00 e8 f5 19 02 00 0f 1f 44 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 0f 05 <48> 3d 00 f0 ff ff 77 56 c3 0f 1f 44 00 00 48 83 ec 28 48 89 54 24
> [ 123.450174] RSP: 002b:00007fffef2ec138 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
> [ 123.450177] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f3b351fe142
> [ 123.450179] RDX: 0000000000001001 RSI: 00005644c047f070 RDI: 0000000000000003
> [ 123.450180] RBP: 00007fffef2ec340 R08: 00005644c047f070 R09: 00007f3b352d9320
> [ 123.450182] R10: 00005644c047c010 R11: 0000000000000246 R12: 00005644c047cbf0
> [ 123.450184] R13: 00005644c047e6d0 R14: 0000000000000003 R15: 00007fffef2ec140
> [ 123.450189] Modules linked in: rfcomm ccm cmac algif_hash algif_skcipher af_alg bnep toshiba_acpi industrialio toshiba_haps hp_accel lis3lv02d btusb btrtl btbcm btintel bluetooth ecdh_generic ecc joydev input_leds nls_iso8859_1 snd_sof_pci snd_sof_intel_byt snd_sof_intel_ipc snd_sof_intel_hda_common snd_soc_hdac_hda snd_hda_codec_hdmi snd_sof_xtensa_dsp snd_sof_intel_hda snd_sof snd_hda_ext_core snd_soc_acpi_intel_match snd_soc_acpi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio snd_hda_intel snd_intel_dspcfg soundwire_intel soundwire_generic_allocation soundwire_cadence snd_hda_codec snd_hda_core ath10k_pci snd_hwdep intel_rapl_msr intel_rapl_common ath10k_core soundwire_bus snd_soc_core x86_pkg_temp_thermal ath intel_powerclamp snd_compress ac97_bus snd_pcm_dmaengine mac80211 snd_pcm coretemp snd_seq_midi snd_seq_midi_event snd_rawmidi kvm_intel cfg80211 snd_seq snd_seq_device snd_timer mei_hdcp kvm libarc4 snd crct10dif_pclmul ghash_clmulni_intel aesni_intel
> mei_me dell_wmi
> [ 123.450266] dell_smbios soundcore sparse_keymap dcdbas crypto_simd cryptd mei dell_uart_backlight glue_helper ee1004 wmi_bmof intel_wmi_thunderbolt dell_wmi_descriptor mac_hid efi_pstore acpi_pad acpi_tad intel_cstate sch_fq_codel parport_pc ppdev lp parport ip_tables x_tables autofs4 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq libcrc32c raid1 raid0 multipath linear dm_mirror dm_region_hash dm_log hid_generic usbhid hid i915 i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops cec crc32_pclmul rc_core drm intel_lpss_pci i2c_i801 ahci igc intel_lpss i2c_smbus idma64 xhci_pci libahci virt_dma xhci_pci_renesas wmi video pinctrl_tigerlake
> [ 123.450335] CR2: 0000000000000008
> [ 123.450338] ---[ end trace 9f731e38b53c35cc ]---
>
> The more generic approach will be wrap get_link_ksettings() with begin()
> and complete() callbacks, and calls runtime resume and runtime suspend
> routine respectively. However, igc is like igb, runtime resume routine
> uses rtnl_lock() which upper ethtool layer also uses.
>
> So to prevent a deadlock on rtnl, take a different approach, use
> pm_runtime_suspended() to avoid reading register while device is runtime
> suspended.
>
> Cc: <stable@...r.kernel.org>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.com>
> ---
> drivers/net/ethernet/intel/igc/igc_ethtool.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 61d331ce38cd..4b9eac9dc090 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -1708,7 +1708,8 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
> Asym_Pause);
> }
>
> - status = rd32(IGC_STATUS);
> + status = pm_runtime_suspended(&adapter->pdev->dev) ?
> + 0 : rd32(IGC_STATUS);
>
> if (status & IGC_STATUS_LU) {
> if (status & IGC_STATUS_SPEED_1000) {
>
Acked-by: Sasha Neftin <sasha.neftin@...el.com>
Powered by blists - more mailing lists