linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 0/2] Remove useless param of devcoredump functions and fix bugs
@ 2022-08-17 12:39 Duoming Zhou
  2022-08-17 12:39 ` [PATCH v7 1/2] devcoredump: remove the useless gfp_t parameter in dev_coredumpv and dev_coredumpm Duoming Zhou
  2022-08-17 12:39 ` [PATCH v7 2/2 RESEND] mwifiex: fix sleep in atomic context bugs caused by dev_coredumpv Duoming Zhou
  0 siblings, 2 replies; 9+ messages in thread
From: Duoming Zhou @ 2022-08-17 12:39 UTC (permalink / raw)
  To: linux-kernel, linux-wireless
  Cc: briannorris, amitkarwar, ganapathi017, sharvari.harisangam,
	huxinming820, kvalo, davem, edumazet, kuba, pabeni, netdev,
	johannes, gregkh, rafael, Duoming Zhou

The first patch removes the extra gfp_t param of dev_coredumpv()
and dev_coredumpm().

The second patch fix sleep in atomic context bugs of mwifiex
caused by dev_coredumpv().

Duoming Zhou (2):
  devcoredump: remove the useless gfp_t parameter in dev_coredumpv and
    dev_coredumpm
  mwifiex: fix sleep in atomic context bugs caused by dev_coredumpv

 drivers/base/devcoredump.c                       | 16 ++++++----------
 drivers/bluetooth/btmrvl_sdio.c                  |  2 +-
 drivers/bluetooth/hci_qca.c                      |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c       |  2 +-
 drivers/gpu/drm/etnaviv/etnaviv_dump.c           |  2 +-
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c     |  4 ++--
 drivers/gpu/drm/msm/msm_gpu.c                    |  4 ++--
 drivers/media/platform/qcom/venus/core.c         |  2 +-
 drivers/net/can/spi/mcp251xfd/mcp251xfd-dump.c   |  2 +-
 drivers/net/wireless/ath/ath10k/coredump.c       |  2 +-
 .../net/wireless/ath/wil6210/wil_crash_dump.c    |  2 +-
 .../wireless/broadcom/brcm80211/brcmfmac/debug.c |  2 +-
 drivers/net/wireless/intel/iwlwifi/fw/dbg.c      |  6 ++----
 drivers/net/wireless/marvell/mwifiex/init.c      |  9 +++++----
 drivers/net/wireless/marvell/mwifiex/main.c      |  3 +--
 drivers/net/wireless/marvell/mwifiex/main.h      |  3 ++-
 drivers/net/wireless/marvell/mwifiex/sta_event.c |  6 +++---
 drivers/net/wireless/mediatek/mt76/mt7615/mac.c  |  3 +--
 drivers/net/wireless/mediatek/mt76/mt7921/mac.c  |  3 +--
 drivers/net/wireless/realtek/rtw88/main.c        |  2 +-
 drivers/net/wireless/realtek/rtw89/ser.c         |  2 +-
 drivers/remoteproc/qcom_q6v5_mss.c               |  2 +-
 drivers/remoteproc/remoteproc_coredump.c         |  8 ++++----
 include/drm/drm_print.h                          |  2 +-
 include/linux/devcoredump.h                      | 13 ++++++-------
 sound/soc/intel/avs/apl.c                        |  2 +-
 sound/soc/intel/avs/skl.c                        |  2 +-
 sound/soc/intel/catpt/dsp.c                      |  2 +-
 28 files changed, 51 insertions(+), 59 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v7 1/2] devcoredump: remove the useless gfp_t parameter in dev_coredumpv and dev_coredumpm
  2022-08-17 12:39 [PATCH v7 0/2] Remove useless param of devcoredump functions and fix bugs Duoming Zhou
@ 2022-08-17 12:39 ` Duoming Zhou
  2022-08-17 12:43   ` Greg KH
  2022-08-17 12:39 ` [PATCH v7 2/2 RESEND] mwifiex: fix sleep in atomic context bugs caused by dev_coredumpv Duoming Zhou
  1 sibling, 1 reply; 9+ messages in thread
From: Duoming Zhou @ 2022-08-17 12:39 UTC (permalink / raw)
  To: linux-kernel, linux-wireless
  Cc: briannorris, amitkarwar, ganapathi017, sharvari.harisangam,
	huxinming820, kvalo, davem, edumazet, kuba, pabeni, netdev,
	johannes, gregkh, rafael, Duoming Zhou

The dev_coredumpv() and dev_coredumpm() could not be used in atomic
context, because they call kvasprintf_const() and kstrdup() with
GFP_KERNEL parameter. The process is shown below:

dev_coredumpv(.., gfp_t gfp)
  dev_coredumpm(.., gfp_t gfp)
    dev_set_name
      kobject_set_name_vargs
        kvasprintf_const(GFP_KERNEL, ...); //may sleep
          kstrdup(s, GFP_KERNEL); //may sleep

This patch removes gfp_t parameter of dev_coredumpv() and dev_coredumpm()
and changes the gfp_t parameter of kzalloc() in dev_coredumpm() to
GFP_KERNEL in order to show they could not be used in atomic context.

Fixes: 833c95456a70 ("device coredump: add new device coredump class")
Reviewed-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
Changes in v7:
  - Remove gfp_t flag in amdgpu device.

 drivers/base/devcoredump.c                       | 16 ++++++----------
 drivers/bluetooth/btmrvl_sdio.c                  |  2 +-
 drivers/bluetooth/hci_qca.c                      |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c       |  2 +-
 drivers/gpu/drm/etnaviv/etnaviv_dump.c           |  2 +-
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c     |  4 ++--
 drivers/gpu/drm/msm/msm_gpu.c                    |  4 ++--
 drivers/media/platform/qcom/venus/core.c         |  2 +-
 drivers/net/can/spi/mcp251xfd/mcp251xfd-dump.c   |  2 +-
 drivers/net/wireless/ath/ath10k/coredump.c       |  2 +-
 .../net/wireless/ath/wil6210/wil_crash_dump.c    |  2 +-
 .../wireless/broadcom/brcm80211/brcmfmac/debug.c |  2 +-
 drivers/net/wireless/intel/iwlwifi/fw/dbg.c      |  6 ++----
 drivers/net/wireless/marvell/mwifiex/main.c      |  3 +--
 drivers/net/wireless/mediatek/mt76/mt7615/mac.c  |  3 +--
 drivers/net/wireless/mediatek/mt76/mt7921/mac.c  |  3 +--
 drivers/net/wireless/realtek/rtw88/main.c        |  2 +-
 drivers/net/wireless/realtek/rtw89/ser.c         |  2 +-
 drivers/remoteproc/qcom_q6v5_mss.c               |  2 +-
 drivers/remoteproc/remoteproc_coredump.c         |  8 ++++----
 include/drm/drm_print.h                          |  2 +-
 include/linux/devcoredump.h                      | 13 ++++++-------
 sound/soc/intel/avs/apl.c                        |  2 +-
 sound/soc/intel/avs/skl.c                        |  2 +-
 sound/soc/intel/catpt/dsp.c                      |  2 +-
 25 files changed, 41 insertions(+), 51 deletions(-)

diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
index f4d794d6bb8..8535f0bd5df 100644
--- a/drivers/base/devcoredump.c
+++ b/drivers/base/devcoredump.c
@@ -173,15 +173,13 @@ static void devcd_freev(void *data)
  * @dev: the struct device for the crashed device
  * @data: vmalloc data containing the device coredump
  * @datalen: length of the data
- * @gfp: allocation flags
  *
  * This function takes ownership of the vmalloc'ed data and will free
  * it when it is no longer used. See dev_coredumpm() for more information.
  */
-void dev_coredumpv(struct device *dev, void *data, size_t datalen,
-		   gfp_t gfp)
+void dev_coredumpv(struct device *dev, void *data, size_t datalen)
 {
-	dev_coredumpm(dev, NULL, data, datalen, gfp, devcd_readv, devcd_freev);
+	dev_coredumpm(dev, NULL, data, datalen, devcd_readv, devcd_freev);
 }
 EXPORT_SYMBOL_GPL(dev_coredumpv);
 
@@ -236,7 +234,6 @@ static ssize_t devcd_read_from_sgtable(char *buffer, loff_t offset,
  * @owner: the module that contains the read/free functions, use %THIS_MODULE
  * @data: data cookie for the @read/@free functions
  * @datalen: length of the data
- * @gfp: allocation flags
  * @read: function to read from the given buffer
  * @free: function to free the given buffer
  *
@@ -246,7 +243,7 @@ static ssize_t devcd_read_from_sgtable(char *buffer, loff_t offset,
  * function will be called to free the data.
  */
 void dev_coredumpm(struct device *dev, struct module *owner,
-		   void *data, size_t datalen, gfp_t gfp,
+		   void *data, size_t datalen,
 		   ssize_t (*read)(char *buffer, loff_t offset, size_t count,
 				   void *data, size_t datalen),
 		   void (*free)(void *data))
@@ -268,7 +265,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
 	if (!try_module_get(owner))
 		goto free;
 
-	devcd = kzalloc(sizeof(*devcd), gfp);
+	devcd = kzalloc(sizeof(*devcd), GFP_KERNEL);
 	if (!devcd)
 		goto put_module;
 
@@ -318,7 +315,6 @@ EXPORT_SYMBOL_GPL(dev_coredumpm);
  * @dev: the struct device for the crashed device
  * @table: the dump data
  * @datalen: length of the data
- * @gfp: allocation flags
  *
  * Creates a new device coredump for the given device. If a previous one hasn't
  * been read yet, the new coredump is discarded. The data lifetime is determined
@@ -326,9 +322,9 @@ EXPORT_SYMBOL_GPL(dev_coredumpm);
  * it will free the data.
  */
 void dev_coredumpsg(struct device *dev, struct scatterlist *table,
-		    size_t datalen, gfp_t gfp)
+		    size_t datalen)
 {
-	dev_coredumpm(dev, NULL, table, datalen, gfp, devcd_read_from_sgtable,
+	dev_coredumpm(dev, NULL, table, datalen, devcd_read_from_sgtable,
 		      devcd_free_sgtable);
 }
 EXPORT_SYMBOL_GPL(dev_coredumpsg);
diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index ba057ebfda5..ab1b93b27ff 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -1502,7 +1502,7 @@ static void btmrvl_sdio_coredump(struct device *dev)
 	/* fw_dump_data will be free in device coredump release function
 	 * after 5 min
 	 */
-	dev_coredumpv(&card->func->dev, fw_dump_data, fw_dump_len, GFP_KERNEL);
+	dev_coredumpv(&card->func->dev, fw_dump_data, fw_dump_len);
 	BT_INFO("== btmrvl firmware dump to /sys/class/devcoredump end");
 }
 
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 8df11016fd5..95a97b38082 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1120,7 +1120,7 @@ static void qca_controller_memdump(struct work_struct *work)
 				    qca_memdump->ram_dump_size);
 			memdump_buf = qca_memdump->memdump_buf_head;
 			dev_coredumpv(&hu->serdev->dev, memdump_buf,
-				      qca_memdump->received_dump, GFP_KERNEL);
+				      qca_memdump->received_dump);
 			cancel_delayed_work(&qca->ctrl_memdump_timeout);
 			kfree(qca->qca_memdump);
 			qca->qca_memdump = NULL;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index c4a6fe3070b..e1a5cd48ace 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4726,7 +4726,7 @@ static void amdgpu_reset_capture_coredumpm(struct amdgpu_device *adev)
 	struct drm_device *dev = adev_to_drm(adev);
 
 	ktime_get_ts64(&adev->reset_time);
-	dev_coredumpm(dev->dev, THIS_MODULE, adev, 0, GFP_KERNEL,
+	dev_coredumpm(dev->dev, THIS_MODULE, adev, 0,
 		      amdgpu_devcoredump_read, amdgpu_devcoredump_free);
 }
 #endif
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_dump.c b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
index f418e0b7577..519fcb234b3 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_dump.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
@@ -225,5 +225,5 @@ void etnaviv_core_dump(struct etnaviv_gem_submit *submit)
 
 	etnaviv_core_dump_header(&iter, ETDUMP_BUF_END, iter.data);
 
-	dev_coredumpv(gpu->dev, iter.start, iter.data - iter.start, GFP_KERNEL);
+	dev_coredumpv(gpu->dev, iter.start, iter.data - iter.start);
 }
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
index e75b97127c0..f057d294c30 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
@@ -74,8 +74,8 @@ static void _msm_disp_snapshot_work(struct kthread_work *work)
 	 * If there is a codedump pending for the device, the dev_coredumpm()
 	 * will also free new coredump state.
 	 */
-	dev_coredumpm(disp_state->dev, THIS_MODULE, disp_state, 0, GFP_KERNEL,
-			disp_devcoredump_read, msm_disp_state_free);
+	dev_coredumpm(disp_state->dev, THIS_MODULE, disp_state, 0,
+		      disp_devcoredump_read, msm_disp_state_free);
 }
 
 void msm_disp_snapshot_state(struct drm_device *drm_dev)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index c2bfcf3f1f4..c10e5d25a17 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -295,8 +295,8 @@ static void msm_gpu_crashstate_capture(struct msm_gpu *gpu,
 	gpu->crashstate = state;
 
 	/* FIXME: Release the crashstate if this errors out? */
-	dev_coredumpm(gpu->dev->dev, THIS_MODULE, gpu, 0, GFP_KERNEL,
-		msm_gpu_devcoredump_read, msm_gpu_devcoredump_free);
+	dev_coredumpm(gpu->dev->dev, THIS_MODULE, gpu, 0,
+		      msm_gpu_devcoredump_read, msm_gpu_devcoredump_free);
 }
 #else
 static void msm_gpu_crashstate_capture(struct msm_gpu *gpu,
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 990a1519f96..8f7df02dbff 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -49,7 +49,7 @@ static void venus_coredump(struct venus_core *core)
 
 	memcpy(data, mem_va, mem_size);
 	memunmap(mem_va);
-	dev_coredumpv(dev, data, mem_size, GFP_KERNEL);
+	dev_coredumpv(dev, data, mem_size);
 }
 
 static void venus_event_notify(struct venus_core *core, u32 event)
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-dump.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-dump.c
index 004eaf96262..a9dfa9ef7a8 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-dump.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-dump.c
@@ -281,5 +281,5 @@ void mcp251xfd_dump(const struct mcp251xfd_priv *priv)
 	mcp251xfd_dump_end(priv, &iter);
 
 	dev_coredumpv(&priv->spi->dev, iter.start,
-		      iter.data - iter.start, GFP_KERNEL);
+		      iter.data - iter.start);
 }
diff --git a/drivers/net/wireless/ath/ath10k/coredump.c b/drivers/net/wireless/ath/ath10k/coredump.c
index fe6b6f97a91..dc923706992 100644
--- a/drivers/net/wireless/ath/ath10k/coredump.c
+++ b/drivers/net/wireless/ath/ath10k/coredump.c
@@ -1607,7 +1607,7 @@ int ath10k_coredump_submit(struct ath10k *ar)
 		return -ENODATA;
 	}
 
-	dev_coredumpv(ar->dev, dump, le32_to_cpu(dump->len), GFP_KERNEL);
+	dev_coredumpv(ar->dev, dump, le32_to_cpu(dump->len));
 
 	return 0;
 }
diff --git a/drivers/net/wireless/ath/wil6210/wil_crash_dump.c b/drivers/net/wireless/ath/wil6210/wil_crash_dump.c
index 89c12cb2aaa..79299609dd6 100644
--- a/drivers/net/wireless/ath/wil6210/wil_crash_dump.c
+++ b/drivers/net/wireless/ath/wil6210/wil_crash_dump.c
@@ -117,6 +117,6 @@ void wil_fw_core_dump(struct wil6210_priv *wil)
 	/* fw_dump_data will be free in device coredump release function
 	 * after 5 min
 	 */
-	dev_coredumpv(wil_to_dev(wil), fw_dump_data, fw_dump_size, GFP_KERNEL);
+	dev_coredumpv(wil_to_dev(wil), fw_dump_data, fw_dump_size);
 	wil_info(wil, "fw core dumped, size %d bytes\n", fw_dump_size);
 }
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.c
index eecf8a38d94..87f3652ef3b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.c
@@ -37,7 +37,7 @@ int brcmf_debug_create_memdump(struct brcmf_bus *bus, const void *data,
 		return err;
 	}
 
-	dev_coredumpv(bus->dev, dump, len + ramsize, GFP_KERNEL);
+	dev_coredumpv(bus->dev, dump, len + ramsize);
 
 	return 0;
 }
diff --git a/drivers/net/wireless/intel/iwlwifi/fw/dbg.c b/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
index abf49022edb..f2f7cf494a8 100644
--- a/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
+++ b/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
@@ -2601,8 +2601,7 @@ static void iwl_fw_error_dump(struct iwl_fw_runtime *fwrt,
 					     fw_error_dump.trans_ptr->data,
 					     fw_error_dump.trans_ptr->len,
 					     fw_error_dump.fwrt_len);
-		dev_coredumpsg(fwrt->trans->dev, sg_dump_data, file_len,
-			       GFP_KERNEL);
+		dev_coredumpsg(fwrt->trans->dev, sg_dump_data, file_len);
 	}
 	vfree(fw_error_dump.fwrt_ptr);
 	vfree(fw_error_dump.trans_ptr);
@@ -2647,8 +2646,7 @@ static void iwl_fw_error_ini_dump(struct iwl_fw_runtime *fwrt,
 					     entry->data, entry->size, offs);
 			offs += entry->size;
 		}
-		dev_coredumpsg(fwrt->trans->dev, sg_dump_data, file_len,
-			       GFP_KERNEL);
+		dev_coredumpsg(fwrt->trans->dev, sg_dump_data, file_len);
 	}
 	iwl_dump_ini_list_free(&dump_list);
 }
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index da2e6557e68..9cadd07bb79 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1103,8 +1103,7 @@ void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter)
 	 */
 	mwifiex_dbg(adapter, MSG,
 		    "== mwifiex dump information to /sys/class/devcoredump start\n");
-	dev_coredumpv(adapter->dev, adapter->devdump_data, adapter->devdump_len,
-		      GFP_KERNEL);
+	dev_coredumpv(adapter->dev, adapter->devdump_data, adapter->devdump_len);
 	mwifiex_dbg(adapter, MSG,
 		    "== mwifiex dump information to /sys/class/devcoredump end\n");
 
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mac.c b/drivers/net/wireless/mediatek/mt76/mt7615/mac.c
index ad6c7d632ee..002472b4e63 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/mac.c
@@ -2376,6 +2376,5 @@ void mt7615_coredump_work(struct work_struct *work)
 
 		dev_kfree_skb(skb);
 	}
-	dev_coredumpv(dev->mt76.dev, dump, MT76_CONNAC_COREDUMP_SZ,
-		      GFP_KERNEL);
+	dev_coredumpv(dev->mt76.dev, dump, MT76_CONNAC_COREDUMP_SZ);
 }
diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
index 47f0aa81ab0..ff764b7eafa 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/mac.c
@@ -986,8 +986,7 @@ void mt7921_coredump_work(struct work_struct *work)
 	}
 
 	if (dump)
-		dev_coredumpv(dev->mt76.dev, dump, MT76_CONNAC_COREDUMP_SZ,
-			      GFP_KERNEL);
+		dev_coredumpv(dev->mt76.dev, dump, MT76_CONNAC_COREDUMP_SZ);
 
 	mt7921_reset(&dev->mt76);
 }
diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index 76dc9da88f6..25178ed0a0f 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -414,7 +414,7 @@ static void rtw_fwcd_dump(struct rtw_dev *rtwdev)
 	 * framework. Note that a new dump will be discarded if a previous one
 	 * hasn't been released yet.
 	 */
-	dev_coredumpv(rtwdev->dev, desc->data, desc->size, GFP_KERNEL);
+	dev_coredumpv(rtwdev->dev, desc->data, desc->size);
 }
 
 static void rtw_fwcd_free(struct rtw_dev *rtwdev, bool free_self)
diff --git a/drivers/net/wireless/realtek/rtw89/ser.c b/drivers/net/wireless/realtek/rtw89/ser.c
index 726223f25dc..1a77b7c874b 100644
--- a/drivers/net/wireless/realtek/rtw89/ser.c
+++ b/drivers/net/wireless/realtek/rtw89/ser.c
@@ -127,7 +127,7 @@ static void rtw89_ser_cd_send(struct rtw89_dev *rtwdev,
 	 * will be discarded if a previous one hasn't been released by
 	 * framework yet.
 	 */
-	dev_coredumpv(rtwdev->dev, buf, sizeof(*buf), GFP_KERNEL);
+	dev_coredumpv(rtwdev->dev, buf, sizeof(*buf));
 }
 
 static void rtw89_ser_cd_free(struct rtw89_dev *rtwdev,
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index fddb63cffee..d09e2f4f2f3 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -598,7 +598,7 @@ static void q6v5_dump_mba_logs(struct q6v5 *qproc)
 	data = vmalloc(MBA_LOG_SIZE);
 	if (data) {
 		memcpy(data, mba_region, MBA_LOG_SIZE);
-		dev_coredumpv(&rproc->dev, data, MBA_LOG_SIZE, GFP_KERNEL);
+		dev_coredumpv(&rproc->dev, data, MBA_LOG_SIZE);
 	}
 	memunmap(mba_region);
 }
diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c
index 4b093420d98..cd55c2abd22 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -309,7 +309,7 @@ void rproc_coredump(struct rproc *rproc)
 		phdr += elf_size_of_phdr(class);
 	}
 	if (dump_conf == RPROC_COREDUMP_ENABLED) {
-		dev_coredumpv(&rproc->dev, data, data_size, GFP_KERNEL);
+		dev_coredumpv(&rproc->dev, data, data_size);
 		return;
 	}
 
@@ -318,7 +318,7 @@ void rproc_coredump(struct rproc *rproc)
 	dump_state.header = data;
 	init_completion(&dump_state.dump_done);
 
-	dev_coredumpm(&rproc->dev, NULL, &dump_state, data_size, GFP_KERNEL,
+	dev_coredumpm(&rproc->dev, NULL, &dump_state, data_size,
 		      rproc_coredump_read, rproc_coredump_free);
 
 	/*
@@ -449,7 +449,7 @@ void rproc_coredump_using_sections(struct rproc *rproc)
 	}
 
 	if (dump_conf == RPROC_COREDUMP_ENABLED) {
-		dev_coredumpv(&rproc->dev, data, data_size, GFP_KERNEL);
+		dev_coredumpv(&rproc->dev, data, data_size);
 		return;
 	}
 
@@ -458,7 +458,7 @@ void rproc_coredump_using_sections(struct rproc *rproc)
 	dump_state.header = data;
 	init_completion(&dump_state.dump_done);
 
-	dev_coredumpm(&rproc->dev, NULL, &dump_state, data_size, GFP_KERNEL,
+	dev_coredumpm(&rproc->dev, NULL, &dump_state, data_size,
 		      rproc_coredump_read, rproc_coredump_free);
 
 	/* Wait until the dump is read and free is called. Data is freed
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 22fabdeed29..b41850366bc 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -162,7 +162,7 @@ struct drm_print_iterator {
  *	void makecoredump(...)
  *	{
  *		...
- *		dev_coredumpm(dev, THIS_MODULE, data, 0, GFP_KERNEL,
+ *		dev_coredumpm(dev, THIS_MODULE, data, 0,
  *			coredump_read, ...)
  *	}
  *
diff --git a/include/linux/devcoredump.h b/include/linux/devcoredump.h
index c008169ed2c..c7d840d824c 100644
--- a/include/linux/devcoredump.h
+++ b/include/linux/devcoredump.h
@@ -52,27 +52,26 @@ static inline void _devcd_free_sgtable(struct scatterlist *table)
 
 
 #ifdef CONFIG_DEV_COREDUMP
-void dev_coredumpv(struct device *dev, void *data, size_t datalen,
-		   gfp_t gfp);
+void dev_coredumpv(struct device *dev, void *data, size_t datalen);
 
 void dev_coredumpm(struct device *dev, struct module *owner,
-		   void *data, size_t datalen, gfp_t gfp,
+		   void *data, size_t datalen,
 		   ssize_t (*read)(char *buffer, loff_t offset, size_t count,
 				   void *data, size_t datalen),
 		   void (*free)(void *data));
 
 void dev_coredumpsg(struct device *dev, struct scatterlist *table,
-		    size_t datalen, gfp_t gfp);
+		    size_t datalen);
 #else
 static inline void dev_coredumpv(struct device *dev, void *data,
-				 size_t datalen, gfp_t gfp)
+				 size_t datalen)
 {
 	vfree(data);
 }
 
 static inline void
 dev_coredumpm(struct device *dev, struct module *owner,
-	      void *data, size_t datalen, gfp_t gfp,
+	      void *data, size_t datalen,
 	      ssize_t (*read)(char *buffer, loff_t offset, size_t count,
 			      void *data, size_t datalen),
 	      void (*free)(void *data))
@@ -81,7 +80,7 @@ dev_coredumpm(struct device *dev, struct module *owner,
 }
 
 static inline void dev_coredumpsg(struct device *dev, struct scatterlist *table,
-				  size_t datalen, gfp_t gfp)
+				  size_t datalen)
 {
 	_devcd_free_sgtable(table);
 }
diff --git a/sound/soc/intel/avs/apl.c b/sound/soc/intel/avs/apl.c
index b8e2b23c9f6..1ff57f1a483 100644
--- a/sound/soc/intel/avs/apl.c
+++ b/sound/soc/intel/avs/apl.c
@@ -164,7 +164,7 @@ static int apl_coredump(struct avs_dev *adev, union avs_notify_msg *msg)
 	} while (offset < msg->ext.coredump.stack_dump_size);
 
 exit:
-	dev_coredumpv(adev->dev, dump, dump_size, GFP_KERNEL);
+	dev_coredumpv(adev->dev, dump, dump_size);
 
 	return 0;
 }
diff --git a/sound/soc/intel/avs/skl.c b/sound/soc/intel/avs/skl.c
index bda5ec7510f..3413162768d 100644
--- a/sound/soc/intel/avs/skl.c
+++ b/sound/soc/intel/avs/skl.c
@@ -88,7 +88,7 @@ static int skl_coredump(struct avs_dev *adev, union avs_notify_msg *msg)
 		return -ENOMEM;
 
 	memcpy_fromio(dump, avs_sram_addr(adev, AVS_FW_REGS_WINDOW), AVS_FW_REGS_SIZE);
-	dev_coredumpv(adev->dev, dump, AVS_FW_REGS_SIZE, GFP_KERNEL);
+	dev_coredumpv(adev->dev, dump, AVS_FW_REGS_SIZE);
 
 	return 0;
 }
diff --git a/sound/soc/intel/catpt/dsp.c b/sound/soc/intel/catpt/dsp.c
index 346bec00030..d2afe9ff1e3 100644
--- a/sound/soc/intel/catpt/dsp.c
+++ b/sound/soc/intel/catpt/dsp.c
@@ -539,7 +539,7 @@ int catpt_coredump(struct catpt_dev *cdev)
 		pos += CATPT_DMA_REGS_SIZE;
 	}
 
-	dev_coredumpv(cdev->dev, dump, dump_size, GFP_KERNEL);
+	dev_coredumpv(cdev->dev, dump, dump_size);
 
 	return 0;
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v7 2/2 RESEND] mwifiex: fix sleep in atomic context bugs caused by dev_coredumpv
  2022-08-17 12:39 [PATCH v7 0/2] Remove useless param of devcoredump functions and fix bugs Duoming Zhou
  2022-08-17 12:39 ` [PATCH v7 1/2] devcoredump: remove the useless gfp_t parameter in dev_coredumpv and dev_coredumpm Duoming Zhou
@ 2022-08-17 12:39 ` Duoming Zhou
  1 sibling, 0 replies; 9+ messages in thread
From: Duoming Zhou @ 2022-08-17 12:39 UTC (permalink / raw)
  To: linux-kernel, linux-wireless
  Cc: briannorris, amitkarwar, ganapathi017, sharvari.harisangam,
	huxinming820, kvalo, davem, edumazet, kuba, pabeni, netdev,
	johannes, gregkh, rafael, Duoming Zhou, stable

There are sleep in atomic context bugs when uploading device dump
data in mwifiex. The root cause is that dev_coredumpv could not
be used in atomic contexts, because it calls dev_set_name which
include operations that may sleep. The call tree shows execution
paths that could lead to bugs:

   (Interrupt context)
fw_dump_timer_fn
  mwifiex_upload_device_dump
    dev_coredumpv(..., GFP_KERNEL)
      dev_coredumpm()
        kzalloc(sizeof(*devcd), gfp); //may sleep
        dev_set_name
          kobject_set_name_vargs
            kvasprintf_const(GFP_KERNEL, ...); //may sleep
            kstrdup(s, GFP_KERNEL); //may sleep

The corresponding fail log is shown below:

[  135.275938] usb 1-1: == mwifiex dump information to /sys/class/devcoredump start
[  135.281029] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:265
...
[  135.293613] Call Trace:
[  135.293613]  <IRQ>
[  135.293613]  dump_stack_lvl+0x57/0x7d
[  135.293613]  __might_resched.cold+0x138/0x173
[  135.293613]  ? dev_coredumpm+0xca/0x2e0
[  135.293613]  kmem_cache_alloc_trace+0x189/0x1f0
[  135.293613]  ? devcd_match_failing+0x30/0x30
[  135.293613]  dev_coredumpm+0xca/0x2e0
[  135.293613]  ? devcd_freev+0x10/0x10
[  135.293613]  dev_coredumpv+0x1c/0x20
[  135.293613]  ? devcd_match_failing+0x30/0x30
[  135.293613]  mwifiex_upload_device_dump+0x65/0xb0
[  135.293613]  ? mwifiex_dnld_fw+0x1b0/0x1b0
[  135.293613]  call_timer_fn+0x122/0x3d0
[  135.293613]  ? msleep_interruptible+0xb0/0xb0
[  135.293613]  ? lock_downgrade+0x3c0/0x3c0
[  135.293613]  ? __next_timer_interrupt+0x13c/0x160
[  135.293613]  ? lockdep_hardirqs_on_prepare+0xe/0x220
[  135.293613]  ? mwifiex_dnld_fw+0x1b0/0x1b0
[  135.293613]  __run_timers.part.0+0x3f8/0x540
[  135.293613]  ? call_timer_fn+0x3d0/0x3d0
[  135.293613]  ? arch_restore_msi_irqs+0x10/0x10
[  135.293613]  ? lapic_next_event+0x31/0x40
[  135.293613]  run_timer_softirq+0x4f/0xb0
[  135.293613]  __do_softirq+0x1c2/0x651
...
[  135.293613] RIP: 0010:default_idle+0xb/0x10
[  135.293613] RSP: 0018:ffff888006317e68 EFLAGS: 00000246
[  135.293613] RAX: ffffffff82ad8d10 RBX: ffff888006301cc0 RCX: ffffffff82ac90e1
[  135.293613] RDX: ffffed100d9ff1b4 RSI: ffffffff831ad140 RDI: ffffffff82ad8f20
[  135.293613] RBP: 0000000000000003 R08: 0000000000000000 R09: ffff88806cff8d9b
[  135.293613] R10: ffffed100d9ff1b3 R11: 0000000000000001 R12: ffffffff84593410
[  135.293613] R13: 0000000000000000 R14: 0000000000000000 R15: 1ffff11000c62fd2
...
[  135.389205] usb 1-1: == mwifiex dump information to /sys/class/devcoredump end

This patch uses delayed work to replace timer and moves the operations
that may sleep into a delayed work in order to mitigate bugs, it was
tested on Marvell 88W8801 chip whose port is usb and the firmware is
usb8801_uapsta.bin. The following is the result after using delayed
work to replace timer.

[  134.936453] usb 1-1: == mwifiex dump information to /sys/class/devcoredump start
[  135.043344] usb 1-1: == mwifiex dump information to /sys/class/devcoredump end

As we can see, there is no bug now.

Cc: stable@vger.kernel.org
Fixes: f5ecd02a8b20 ("mwifiex: device dump support for usb interface")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Changes since v6:
  - Use clang-format to adjust the format of code.

 drivers/net/wireless/marvell/mwifiex/init.c      | 9 +++++----
 drivers/net/wireless/marvell/mwifiex/main.h      | 3 ++-
 drivers/net/wireless/marvell/mwifiex/sta_event.c | 6 +++---
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index fc77489cc51..7dddb4b5dea 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -51,9 +51,10 @@ static void wakeup_timer_fn(struct timer_list *t)
 		adapter->if_ops.card_reset(adapter);
 }
 
-static void fw_dump_timer_fn(struct timer_list *t)
+static void fw_dump_work(struct work_struct *work)
 {
-	struct mwifiex_adapter *adapter = from_timer(adapter, t, devdump_timer);
+	struct mwifiex_adapter *adapter =
+		container_of(work, struct mwifiex_adapter, devdump_work.work);
 
 	mwifiex_upload_device_dump(adapter);
 }
@@ -309,7 +310,7 @@ static void mwifiex_init_adapter(struct mwifiex_adapter *adapter)
 	adapter->active_scan_triggered = false;
 	timer_setup(&adapter->wakeup_timer, wakeup_timer_fn, 0);
 	adapter->devdump_len = 0;
-	timer_setup(&adapter->devdump_timer, fw_dump_timer_fn, 0);
+	INIT_DELAYED_WORK(&adapter->devdump_work, fw_dump_work);
 }
 
 /*
@@ -388,7 +389,7 @@ static void
 mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
 {
 	del_timer(&adapter->wakeup_timer);
-	del_timer_sync(&adapter->devdump_timer);
+	cancel_delayed_work_sync(&adapter->devdump_work);
 	mwifiex_cancel_all_pending_cmd(adapter);
 	wake_up_interruptible(&adapter->cmd_wait_q.wait);
 	wake_up_interruptible(&adapter->hs_activate_wait_q);
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 87729d251fe..63f861e6b28 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -37,6 +37,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/of_irq.h>
+#include <linux/workqueue.h>
 
 #include "decl.h"
 #include "ioctl.h"
@@ -1043,7 +1044,7 @@ struct mwifiex_adapter {
 	/* Device dump data/length */
 	void *devdump_data;
 	int devdump_len;
-	struct timer_list devdump_timer;
+	struct delayed_work devdump_work;
 
 	bool ignore_btcoex_events;
 };
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_event.c b/drivers/net/wireless/marvell/mwifiex/sta_event.c
index b95e90a7d12..e80e372cce8 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_event.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_event.c
@@ -611,8 +611,8 @@ mwifiex_fw_dump_info_event(struct mwifiex_private *priv,
 		 * transmission event get lost, in this cornel case,
 		 * user would still get partial of the dump.
 		 */
-		mod_timer(&adapter->devdump_timer,
-			  jiffies + msecs_to_jiffies(MWIFIEX_TIMER_10S));
+		schedule_delayed_work(&adapter->devdump_work,
+				      msecs_to_jiffies(MWIFIEX_TIMER_10S));
 	}
 
 	/* Overflow check */
@@ -631,7 +631,7 @@ mwifiex_fw_dump_info_event(struct mwifiex_private *priv,
 	return;
 
 upload_dump:
-	del_timer_sync(&adapter->devdump_timer);
+	cancel_delayed_work_sync(&adapter->devdump_work);
 	mwifiex_upload_device_dump(adapter);
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v7 1/2] devcoredump: remove the useless gfp_t parameter in dev_coredumpv and dev_coredumpm
  2022-08-17 12:39 ` [PATCH v7 1/2] devcoredump: remove the useless gfp_t parameter in dev_coredumpv and dev_coredumpm Duoming Zhou
@ 2022-08-17 12:43   ` Greg KH
  2022-08-17 14:05     ` duoming
  0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2022-08-17 12:43 UTC (permalink / raw)
  To: Duoming Zhou
  Cc: linux-kernel, linux-wireless, briannorris, amitkarwar,
	ganapathi017, sharvari.harisangam, huxinming820, kvalo, davem,
	edumazet, kuba, pabeni, netdev, johannes, rafael

On Wed, Aug 17, 2022 at 08:39:12PM +0800, Duoming Zhou wrote:
> The dev_coredumpv() and dev_coredumpm() could not be used in atomic
> context, because they call kvasprintf_const() and kstrdup() with
> GFP_KERNEL parameter. The process is shown below:
> 
> dev_coredumpv(.., gfp_t gfp)
>   dev_coredumpm(.., gfp_t gfp)
>     dev_set_name
>       kobject_set_name_vargs
>         kvasprintf_const(GFP_KERNEL, ...); //may sleep
>           kstrdup(s, GFP_KERNEL); //may sleep
> 
> This patch removes gfp_t parameter of dev_coredumpv() and dev_coredumpm()
> and changes the gfp_t parameter of kzalloc() in dev_coredumpm() to
> GFP_KERNEL in order to show they could not be used in atomic context.
> 
> Fixes: 833c95456a70 ("device coredump: add new device coredump class")
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> ---
> Changes in v7:
>   - Remove gfp_t flag in amdgpu device.

Again, this creates a "flag day" where we have to be sure we hit all
users of this api at the exact same time.  This will prevent any new
driver that comes into a maintainer tree during the next 3 months from
ever being able to use this api without cauing build breakages in the
linux-next tree.

Please evolve this api to work properly for everyone at the same time,
like was previously asked for so that we can take this change.  It will
take 2 releases, but that's fine.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v7 1/2] devcoredump: remove the useless gfp_t parameter in dev_coredumpv and dev_coredumpm
  2022-08-17 12:43   ` Greg KH
@ 2022-08-17 14:05     ` duoming
  2022-08-18 14:58       ` Greg KH
  0 siblings, 1 reply; 9+ messages in thread
From: duoming @ 2022-08-17 14:05 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-kernel, linux-wireless, briannorris, amitkarwar,
	ganapathi017, sharvari.harisangam, huxinming820, kvalo, davem,
	edumazet, kuba, pabeni, netdev, johannes, rafael

Hello,

On Wed, 17 Aug 2022 14:43:29 +0200 Greg KH wrote:

> On Wed, Aug 17, 2022 at 08:39:12PM +0800, Duoming Zhou wrote:
> > The dev_coredumpv() and dev_coredumpm() could not be used in atomic
> > context, because they call kvasprintf_const() and kstrdup() with
> > GFP_KERNEL parameter. The process is shown below:
> > 
> > dev_coredumpv(.., gfp_t gfp)
> >   dev_coredumpm(.., gfp_t gfp)
> >     dev_set_name
> >       kobject_set_name_vargs
> >         kvasprintf_const(GFP_KERNEL, ...); //may sleep
> >           kstrdup(s, GFP_KERNEL); //may sleep
> > 
> > This patch removes gfp_t parameter of dev_coredumpv() and dev_coredumpm()
> > and changes the gfp_t parameter of kzalloc() in dev_coredumpm() to
> > GFP_KERNEL in order to show they could not be used in atomic context.
> > 
> > Fixes: 833c95456a70 ("device coredump: add new device coredump class")
> > Reviewed-by: Brian Norris <briannorris@chromium.org>
> > Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
> > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> > ---
> > Changes in v7:
> >   - Remove gfp_t flag in amdgpu device.
> 
> Again, this creates a "flag day" where we have to be sure we hit all
> users of this api at the exact same time.  This will prevent any new
> driver that comes into a maintainer tree during the next 3 months from
> ever being able to use this api without cauing build breakages in the
> linux-next tree.
> 
> Please evolve this api to work properly for everyone at the same time,
> like was previously asked for so that we can take this change.  It will
> take 2 releases, but that's fine.

Thank you for your reply, I will evolve this api to work properly for everyone.
If there are not any new drivers that use this api during the next 3 months, 
I will send this patch again. Otherwise, I will wait until there are not new
users anymore.

Best regards,
Duoming Zhou


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v7 1/2] devcoredump: remove the useless gfp_t parameter in dev_coredumpv and dev_coredumpm
  2022-08-17 14:05     ` duoming
@ 2022-08-18 14:58       ` Greg KH
  2022-08-18 15:46         ` duoming
  0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2022-08-18 14:58 UTC (permalink / raw)
  To: duoming
  Cc: linux-kernel, linux-wireless, briannorris, amitkarwar,
	ganapathi017, sharvari.harisangam, huxinming820, kvalo, davem,
	edumazet, kuba, pabeni, netdev, johannes, rafael

On Wed, Aug 17, 2022 at 10:05:37PM +0800, duoming@zju.edu.cn wrote:
> Hello,
> 
> On Wed, 17 Aug 2022 14:43:29 +0200 Greg KH wrote:
> 
> > On Wed, Aug 17, 2022 at 08:39:12PM +0800, Duoming Zhou wrote:
> > > The dev_coredumpv() and dev_coredumpm() could not be used in atomic
> > > context, because they call kvasprintf_const() and kstrdup() with
> > > GFP_KERNEL parameter. The process is shown below:
> > > 
> > > dev_coredumpv(.., gfp_t gfp)
> > >   dev_coredumpm(.., gfp_t gfp)
> > >     dev_set_name
> > >       kobject_set_name_vargs
> > >         kvasprintf_const(GFP_KERNEL, ...); //may sleep
> > >           kstrdup(s, GFP_KERNEL); //may sleep
> > > 
> > > This patch removes gfp_t parameter of dev_coredumpv() and dev_coredumpm()
> > > and changes the gfp_t parameter of kzalloc() in dev_coredumpm() to
> > > GFP_KERNEL in order to show they could not be used in atomic context.
> > > 
> > > Fixes: 833c95456a70 ("device coredump: add new device coredump class")
> > > Reviewed-by: Brian Norris <briannorris@chromium.org>
> > > Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
> > > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> > > ---
> > > Changes in v7:
> > >   - Remove gfp_t flag in amdgpu device.
> > 
> > Again, this creates a "flag day" where we have to be sure we hit all
> > users of this api at the exact same time.  This will prevent any new
> > driver that comes into a maintainer tree during the next 3 months from
> > ever being able to use this api without cauing build breakages in the
> > linux-next tree.
> > 
> > Please evolve this api to work properly for everyone at the same time,
> > like was previously asked for so that we can take this change.  It will
> > take 2 releases, but that's fine.
> 
> Thank you for your reply, I will evolve this api to work properly for everyone.
> If there are not any new drivers that use this api during the next 3 months, 
> I will send this patch again. Otherwise, I will wait until there are not new
> users anymore.

No, that is not necessary.  Do the work now so that there is no flag day
and you don't have to worry about new users, it will all "just work".

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v7 1/2] devcoredump: remove the useless gfp_t parameter in dev_coredumpv and dev_coredumpm
  2022-08-18 14:58       ` Greg KH
@ 2022-08-18 15:46         ` duoming
  2022-08-22 18:21           ` Brian Norris
  0 siblings, 1 reply; 9+ messages in thread
From: duoming @ 2022-08-18 15:46 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-kernel, linux-wireless, briannorris, amitkarwar,
	ganapathi017, sharvari.harisangam, huxinming820, kvalo, davem,
	edumazet, kuba, pabeni, netdev, johannes, rafael

Hello,

On Thu, 18 Aug 2022 16:58:01 +0200 Greg KH wrote:

> On Wed, Aug 17, 2022 at 10:05:37PM +0800, duoming@zju.edu.cn wrote:
> > Hello,
> > 
> > On Wed, 17 Aug 2022 14:43:29 +0200 Greg KH wrote:
> > 
> > > On Wed, Aug 17, 2022 at 08:39:12PM +0800, Duoming Zhou wrote:
> > > > The dev_coredumpv() and dev_coredumpm() could not be used in atomic
> > > > context, because they call kvasprintf_const() and kstrdup() with
> > > > GFP_KERNEL parameter. The process is shown below:
> > > > 
> > > > dev_coredumpv(.., gfp_t gfp)
> > > >   dev_coredumpm(.., gfp_t gfp)
> > > >     dev_set_name
> > > >       kobject_set_name_vargs
> > > >         kvasprintf_const(GFP_KERNEL, ...); //may sleep
> > > >           kstrdup(s, GFP_KERNEL); //may sleep
> > > > 
> > > > This patch removes gfp_t parameter of dev_coredumpv() and dev_coredumpm()
> > > > and changes the gfp_t parameter of kzalloc() in dev_coredumpm() to
> > > > GFP_KERNEL in order to show they could not be used in atomic context.
> > > > 
> > > > Fixes: 833c95456a70 ("device coredump: add new device coredump class")
> > > > Reviewed-by: Brian Norris <briannorris@chromium.org>
> > > > Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
> > > > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> > > > ---
> > > > Changes in v7:
> > > >   - Remove gfp_t flag in amdgpu device.
> > > 
> > > Again, this creates a "flag day" where we have to be sure we hit all
> > > users of this api at the exact same time.  This will prevent any new
> > > driver that comes into a maintainer tree during the next 3 months from
> > > ever being able to use this api without cauing build breakages in the
> > > linux-next tree.
> > > 
> > > Please evolve this api to work properly for everyone at the same time,
> > > like was previously asked for so that we can take this change.  It will
> > > take 2 releases, but that's fine.
> > 
> > Thank you for your reply, I will evolve this api to work properly for everyone.
> > If there are not any new drivers that use this api during the next 3 months, 
> > I will send this patch again. Otherwise, I will wait until there are not new
> > users anymore.
> 
> No, that is not necessary.  Do the work now so that there is no flag day
> and you don't have to worry about new users, it will all "just work".

Do you mean we should replace dev_set_name() in dev_coredumpm() to some other
functions that could work both in interrupt context and process context?

Best regards,
Duoming Zhou

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v7 1/2] devcoredump: remove the useless gfp_t parameter in dev_coredumpv and dev_coredumpm
  2022-08-18 15:46         ` duoming
@ 2022-08-22 18:21           ` Brian Norris
  2022-08-23 11:26             ` duoming
  0 siblings, 1 reply; 9+ messages in thread
From: Brian Norris @ 2022-08-22 18:21 UTC (permalink / raw)
  To: Duoming Zhou
  Cc: Greg KH, Linux Kernel, linux-wireless, amit karwar,
	Ganapathi Bhat, Sharvari Harisangam, Xinming Hu, kvalo,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	<netdev@vger.kernel.org>,
	Johannes Berg, Rafael J. Wysocki

Hi,

On Thu, Aug 18, 2022 at 8:47 AM <duoming@zju.edu.cn> wrote:
> On Thu, 18 Aug 2022 16:58:01 +0200 Greg KH wrote:
> > No, that is not necessary.  Do the work now so that there is no flag day
> > and you don't have to worry about new users, it will all "just work".
>
> Do you mean we should replace dev_set_name() in dev_coredumpm() to some other
> functions that could work both in interrupt context and process context?

No.

I believe the suggestion is that rather than change the signature for
dev_coredumpv() (which means everyone has to agree on the new
signature on day 1), you should introduce a new API, like
dev_coredumpv_noatomic() (I'm not good at naming [1]) with the
signature you want, and then migrate users over. Once we have a
release with no users of the old API, we drop it.

There are plenty of examples of the kernel community doing similar
transitions. You can search around for examples, but a quick search of
my own shows something like this:
https://lwn.net/Articles/735887/
(In particular, timer_setup() was introduced, and all setup_timer()
users were migrated to it within a release or two.)

Brian

[1] Seriously, dev_coredumpv_noatomic() is not a name I want to see
last very long. Maybe some other trivial modification? Examples:

dev_core_dumpv()
dev_coredump_v()
device_coredumpv()
...

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v7 1/2] devcoredump: remove the useless gfp_t parameter in dev_coredumpv and dev_coredumpm
  2022-08-22 18:21           ` Brian Norris
@ 2022-08-23 11:26             ` duoming
  0 siblings, 0 replies; 9+ messages in thread
From: duoming @ 2022-08-23 11:26 UTC (permalink / raw)
  To: Brian Norris
  Cc: Greg KH, Linux Kernel, linux-wireless, amit karwar,
	Ganapathi Bhat, Sharvari Harisangam, Xinming Hu, kvalo,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	<netdev@vger.kernel.org>,
	Johannes Berg, Rafael J. Wysocki

Hello,

On Mon, 22 Aug 2022 11:21:36 -0700 Brian Norris wrote:

> Hi,
> 
> On Thu, Aug 18, 2022 at 8:47 AM <duoming@zju.edu.cn> wrote:
> > On Thu, 18 Aug 2022 16:58:01 +0200 Greg KH wrote:
> > > No, that is not necessary.  Do the work now so that there is no flag day
> > > and you don't have to worry about new users, it will all "just work".
> >
> > Do you mean we should replace dev_set_name() in dev_coredumpm() to some other
> > functions that could work both in interrupt context and process context?
> 
> No.
> 
> I believe the suggestion is that rather than change the signature for
> dev_coredumpv() (which means everyone has to agree on the new
> signature on day 1), you should introduce a new API, like
> dev_coredumpv_noatomic() (I'm not good at naming [1]) with the
> signature you want, and then migrate users over. Once we have a
> release with no users of the old API, we drop it.
> 
> There are plenty of examples of the kernel community doing similar
> transitions. You can search around for examples, but a quick search of
> my own shows something like this:
> https://lwn.net/Articles/735887/
> (In particular, timer_setup() was introduced, and all setup_timer()
> users were migrated to it within a release or two.)
> 
> Brian
> 
> [1] Seriously, dev_coredumpv_noatomic() is not a name I want to see
> last very long. Maybe some other trivial modification? Examples:
> 
> dev_core_dumpv()
> dev_coredump_v()
> device_coredumpv()
> ...

Thank you very much for your timer and suggestions!

Best regards,
Duoming Zhou

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-08-23 14:18 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-17 12:39 [PATCH v7 0/2] Remove useless param of devcoredump functions and fix bugs Duoming Zhou
2022-08-17 12:39 ` [PATCH v7 1/2] devcoredump: remove the useless gfp_t parameter in dev_coredumpv and dev_coredumpm Duoming Zhou
2022-08-17 12:43   ` Greg KH
2022-08-17 14:05     ` duoming
2022-08-18 14:58       ` Greg KH
2022-08-18 15:46         ` duoming
2022-08-22 18:21           ` Brian Norris
2022-08-23 11:26             ` duoming
2022-08-17 12:39 ` [PATCH v7 2/2 RESEND] mwifiex: fix sleep in atomic context bugs caused by dev_coredumpv Duoming Zhou

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).