linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk
@ 2020-11-23 11:19 Bhaskara Budiredla
  2020-11-23 11:19 ` [PATCH v2 2/2] mmc: cavium: Add MMC polling method to support kmsg panic/oops write Bhaskara Budiredla
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Bhaskara Budiredla @ 2020-11-23 11:19 UTC (permalink / raw)
  To: ulf.hansson, keescook, ccross, tony.luck, sgoutham
  Cc: linux-mmc, linux-kernel, outgoing2/0000-cover-letter.patch,
	Bhaskara Budiredla

This patch introduces to mmcpstore. The functioning of mmcpstore
is similar to mtdpstore. mmcpstore works on FTL based flash devices
whereas mtdpstore works on raw flash devices. When the system crashes,
mmcpstore stores the kmsg panic and oops logs to a user specified
MMC device.

It collects the details about the host MMC device through pstore/blk
"blkdev" parameter. The user can specify the MMC device in many ways
by checking in Documentation/admin-guide/pstore-blk.rst.

The individual mmc host drivers have to define suitable polling
subroutines to write kmsg panic/oops logs through mmcpstore.

Signed-off-by: Bhaskara Budiredla <bbudiredla@marvell.com>
---
 drivers/mmc/core/Kconfig     |  15 +-
 drivers/mmc/core/Makefile    |   1 +
 drivers/mmc/core/block.c     |  19 +++
 drivers/mmc/core/block.h     |   9 ++
 drivers/mmc/core/core.c      |  24 +++
 drivers/mmc/core/mmcpstore.c | 302 +++++++++++++++++++++++++++++++++++
 include/linux/mmc/core.h     |   4 +
 include/linux/mmc/host.h     |  12 ++
 8 files changed, 385 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mmc/core/mmcpstore.c

diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
index c12fe13e4b14..505450a6ea2b 100644
--- a/drivers/mmc/core/Kconfig
+++ b/drivers/mmc/core/Kconfig
@@ -34,9 +34,23 @@ config PWRSEQ_SIMPLE
 	  This driver can also be built as a module. If so, the module
 	  will be called pwrseq_simple.
 
+config MMC_PSTORE_BACKEND
+	bool "Log panic/oops to a MMC buffer"
+	depends on MMC_BLOCK
+	default n
+	help
+	  This option will let you create platform backend to store kmsg
+	  crash dumps to a user specified MMC device. This is primarily
+	  based on pstore/blk.
+
+config MMC_PSTORE
+	tristate
+	select PSTORE_BLK
+
 config MMC_BLOCK
 	tristate "MMC block device driver"
 	depends on BLOCK
+	select MMC_PSTORE if MMC_PSTORE_BACKEND=y
 	default y
 	help
 	  Say Y here to enable the MMC block device driver support.
@@ -80,4 +94,3 @@ config MMC_TEST
 
 	  This driver is only of interest to those developing or
 	  testing a host driver. Most people should say N here.
-
diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
index 95ffe008ebdf..7cb9a3af4827 100644
--- a/drivers/mmc/core/Makefile
+++ b/drivers/mmc/core/Makefile
@@ -16,5 +16,6 @@ obj-$(CONFIG_PWRSEQ_EMMC)	+= pwrseq_emmc.o
 mmc_core-$(CONFIG_DEBUG_FS)	+= debugfs.o
 obj-$(CONFIG_MMC_BLOCK)		+= mmc_block.o
 mmc_block-objs			:= block.o queue.o
+mmc_block-$(CONFIG_MMC_PSTORE)	+= mmcpstore.o
 obj-$(CONFIG_MMC_TEST)		+= mmc_test.o
 obj-$(CONFIG_SDIO_UART)		+= sdio_uart.o
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 8d3df0be0355..ed012a91e3a3 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2870,6 +2870,21 @@ static void mmc_blk_remove_debugfs(struct mmc_card *card,
 
 #endif /* CONFIG_DEBUG_FS */
 
+#if IS_ENABLED(CONFIG_MMC_PSTORE)
+sector_t mmc_blk_get_part(struct mmc_card *card, int part_num, sector_t *size)
+{
+	struct mmc_blk_data *md = dev_get_drvdata(&card->dev);
+	struct gendisk *disk = md->disk;
+	struct disk_part_tbl *part_tbl = disk->part_tbl;
+
+	if (part_num < 0 || part_num >= part_tbl->len)
+		return 0;
+
+	*size = part_tbl->part[part_num]->nr_sects << SECTOR_SHIFT;
+	return part_tbl->part[part_num]->start_sect;
+}
+#endif
+
 static int mmc_blk_probe(struct mmc_card *card)
 {
 	struct mmc_blk_data *md, *part_md;
@@ -2913,6 +2928,9 @@ static int mmc_blk_probe(struct mmc_card *card)
 			goto out;
 	}
 
+	if (mmc_card_mmc(card) || mmc_card_sd(card))
+		mmcpstore_card_set(card, md->disk->disk_name);
+
 	/* Add two debugfs entries */
 	mmc_blk_add_debugfs(card, md);
 
@@ -3060,6 +3078,7 @@ static void __exit mmc_blk_exit(void)
 	unregister_blkdev(MMC_BLOCK_MAJOR, "mmc");
 	unregister_chrdev_region(mmc_rpmb_devt, MAX_DEVICES);
 	bus_unregister(&mmc_rpmb_bus_type);
+	unregister_mmcpstore();
 }
 
 module_init(mmc_blk_init);
diff --git a/drivers/mmc/core/block.h b/drivers/mmc/core/block.h
index 31153f656f41..2a4ee5568194 100644
--- a/drivers/mmc/core/block.h
+++ b/drivers/mmc/core/block.h
@@ -16,5 +16,14 @@ void mmc_blk_mq_recovery(struct mmc_queue *mq);
 struct work_struct;
 
 void mmc_blk_mq_complete_work(struct work_struct *work);
+#if IS_ENABLED(CONFIG_MMC_PSTORE)
+sector_t mmc_blk_get_part(struct mmc_card *card, int part_num, sector_t *size);
+void mmcpstore_card_set(struct mmc_card *card, const char *disk_name);
+void unregister_mmcpstore(void);
+#else
+static inline void mmcpstore_card_set(struct mmc_card *card,
+					const char *disk_name) {}
+static inline void unregister_mmcpstore(void) {}
+#endif
 
 #endif
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index d42037f0f10d..7682b267f1d5 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -569,6 +569,30 @@ int mmc_cqe_recovery(struct mmc_host *host)
 }
 EXPORT_SYMBOL(mmc_cqe_recovery);
 
+#if IS_ENABLED(CONFIG_MMC_PSTORE)
+/**
+ *	mmc_wait_for_pstore_req - initiate a blocking mmc request
+ *	@host: MMC host to start command
+ *	@mrq: MMC request to start
+ *
+ *	Start a blocking MMC request for a host and wait for the request
+ *	to complete that is based on polling and timeout.
+ */
+void mmc_wait_for_pstore_req(struct mmc_host *host, struct mmc_request *mrq)
+{
+	unsigned int timeout;
+
+	host->ops->req_cleanup_pending(host);
+	mmc_start_request(host, mrq);
+
+	if (mrq->data) {
+		timeout = mrq->data->timeout_ns / NSEC_PER_MSEC;
+		host->ops->req_completion_poll(host, timeout);
+	}
+}
+EXPORT_SYMBOL(mmc_wait_for_pstore_req);
+#endif
+
 /**
  *	mmc_is_req_done - Determine if a 'cap_cmd_during_tfr' request is done
  *	@host: MMC host
diff --git a/drivers/mmc/core/mmcpstore.c b/drivers/mmc/core/mmcpstore.c
new file mode 100644
index 000000000000..1113eae0756c
--- /dev/null
+++ b/drivers/mmc/core/mmcpstore.c
@@ -0,0 +1,302 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MMC pstore support based on pstore/blk
+ *
+ * Copyright (c) 2020 Marvell.
+ * Author: Bhaskara Budiredla <bbudiredla@marvell.com>
+ */
+
+#define pr_fmt(fmt) "mmcpstore: " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pstore_blk.h>
+#include <linux/blkdev.h>
+#include <linux/mount.h>
+#include <linux/slab.h>
+#include <linux/mmc/mmc.h>
+#include <linux/mmc/host.h>
+#include <linux/mmc/card.h>
+#include <linux/scatterlist.h>
+#include "block.h"
+#include "card.h"
+#include "core.h"
+
+static struct mmcpstore_context {
+	char dev_name[BDEVNAME_SIZE];
+	int partno;
+	sector_t start_sect;
+	sector_t size;
+	struct pstore_device_info dev;
+	struct pstore_blk_config conf;
+	struct pstore_blk_info info;
+
+	char *sub;
+	struct mmc_card	*card;
+	struct mmc_request *mrq;
+} oops_cxt;
+
+static void mmc_prep_req(struct mmc_request *mrq,
+		unsigned int sect_offset, unsigned int nsects,
+		struct scatterlist *sg, u32 opcode, unsigned int flags)
+{
+	mrq->cmd->opcode = opcode;
+	mrq->cmd->arg = sect_offset;
+	mrq->cmd->flags = MMC_RSP_R1 | MMC_CMD_ADTC;
+
+	if (nsects == 1) {
+		mrq->stop = NULL;
+	} else {
+		mrq->stop->opcode = MMC_STOP_TRANSMISSION;
+		mrq->stop->arg = 0;
+		mrq->stop->flags = MMC_RSP_R1B | MMC_CMD_AC;
+	}
+
+	mrq->data->blksz = SECTOR_SIZE;
+	mrq->data->blocks = nsects;
+	mrq->data->flags = flags;
+	mrq->data->sg = sg;
+	mrq->data->sg_len = 1;
+}
+
+static int mmcpstore_rdwr_req(const char *buf, unsigned int nsects,
+			unsigned int sect_offset, unsigned int flags)
+{
+	struct mmcpstore_context *cxt = &oops_cxt;
+	struct mmc_request *mrq = cxt->mrq;
+	struct mmc_card *card = cxt->card;
+	struct mmc_host *host = card->host;
+	struct scatterlist sg;
+	u32 opcode;
+
+	if (flags == MMC_DATA_READ)
+		opcode	= (nsects > 1) ?
+			MMC_READ_MULTIPLE_BLOCK : MMC_READ_SINGLE_BLOCK;
+	else
+		opcode = (nsects > 1) ?
+			MMC_WRITE_MULTIPLE_BLOCK : MMC_WRITE_BLOCK;
+
+	mmc_prep_req(mrq, sect_offset, nsects, &sg, opcode, flags);
+	sg_init_one(&sg, buf, (nsects << SECTOR_SHIFT));
+	mmc_set_data_timeout(mrq->data, cxt->card);
+
+	mmc_claim_host(host);
+	mmc_wait_for_req(host, mrq);
+	mdelay(mrq->data->timeout_ns / NSEC_PER_MSEC);
+	mmc_release_host(host);
+
+	if (mrq->cmd->error) {
+		pr_err("Cmd error: %d\n", mrq->cmd->error);
+		return mrq->cmd->error;
+	}
+	if (mrq->data->error) {
+		pr_err("Data error: %d\n", mrq->data->error);
+		return mrq->data->error;
+	}
+
+	return 0;
+}
+
+static ssize_t mmcpstore_write(const char *buf, size_t size, loff_t off)
+{
+	struct mmcpstore_context *cxt = &oops_cxt;
+	int ret;
+
+	ret = mmcpstore_rdwr_req(buf, (size >> SECTOR_SHIFT),
+		cxt->start_sect + (off >> SECTOR_SHIFT), MMC_DATA_WRITE);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static ssize_t mmcpstore_read(char *buf, size_t size, loff_t off)
+{
+	struct mmcpstore_context *cxt = &oops_cxt;
+	unsigned int sect_off = cxt->start_sect  + (off >> SECTOR_SHIFT);
+	unsigned long sects = (cxt->conf.kmsg_size >> SECTOR_SHIFT);
+	int ret;
+
+	if (unlikely(!buf || !size))
+		return -EINVAL;
+
+	ret = mmcpstore_rdwr_req(cxt->sub, sects, sect_off, MMC_DATA_READ);
+	if (ret)
+		return ret;
+	memcpy(buf, cxt->sub, size);
+
+	return size;
+}
+
+static void mmcpstore_panic_write_req(const char *buf,
+		unsigned int nsects, unsigned int sect_offset)
+{
+	struct mmcpstore_context *cxt = &oops_cxt;
+	struct mmc_request *mrq = cxt->mrq;
+	struct mmc_card *card = cxt->card;
+	struct mmc_host *host = card->host;
+	struct scatterlist sg;
+	u32 opcode;
+
+	opcode = (nsects > 1) ? MMC_WRITE_MULTIPLE_BLOCK : MMC_WRITE_BLOCK;
+	mmc_prep_req(mrq, sect_offset, nsects, &sg, opcode, MMC_DATA_WRITE);
+	sg_init_one(&sg, buf, (nsects << SECTOR_SHIFT));
+	mmc_set_data_timeout(mrq->data, cxt->card);
+
+	mmc_claim_host(host);
+	mmc_wait_for_pstore_req(host, mrq);
+	mmc_release_host(card->host);
+}
+
+static ssize_t mmcpstore_panic_write(const char *buf, size_t size, loff_t off)
+{
+	struct mmcpstore_context *cxt = &oops_cxt;
+
+	mmcpstore_panic_write_req(buf, (size >> SECTOR_SHIFT),
+			cxt->start_sect + (off >> SECTOR_SHIFT));
+	return size;
+}
+
+static struct block_device *mmcpstore_open_backend(const char *device)
+{
+	struct block_device *bdev;
+	dev_t devt;
+
+	bdev = blkdev_get_by_path(device, FMODE_READ, NULL);
+	if (IS_ERR(bdev)) {
+		devt = name_to_dev_t(device);
+		if (devt == 0)
+			return ERR_PTR(-ENODEV);
+
+		bdev = blkdev_get_by_dev(devt, FMODE_READ, NULL);
+		if (IS_ERR(bdev))
+			return bdev;
+	}
+
+	return bdev;
+}
+
+static void mmcpstore_close_backend(struct block_device *bdev)
+{
+	if (!bdev)
+		return;
+	blkdev_put(bdev, FMODE_READ);
+}
+
+void mmcpstore_card_set(struct mmc_card *card, const char *disk_name)
+{
+	struct mmcpstore_context *cxt = &oops_cxt;
+	struct pstore_blk_config *conf = &cxt->conf;
+	struct pstore_device_info *dev = &cxt->dev;
+	struct block_device *bdev;
+	struct mmc_command *stop;
+	struct mmc_command *cmd;
+	struct mmc_request *mrq;
+	struct mmc_data *data;
+	int ret;
+
+	ret = pstore_blk_get_config(conf);
+	if (!conf->device[0]) {
+		pr_debug("psblk backend is empty\n");
+		return;
+	}
+
+	/* Multiple backend devices not allowed */
+	if (cxt->dev_name[0])
+		return;
+
+	bdev =  mmcpstore_open_backend(conf->device);
+	if (IS_ERR(bdev)) {
+		pr_err("%s failed to open with %ld\n",
+				conf->device, PTR_ERR(bdev));
+		return;
+	}
+
+	bdevname(bdev, cxt->dev_name);
+	cxt->partno = bdev->bd_part->partno;
+	mmcpstore_close_backend(bdev);
+
+	if (strncmp(cxt->dev_name, disk_name, strlen(disk_name)))
+		return;
+
+	cxt->start_sect = mmc_blk_get_part(card, cxt->partno, &cxt->size);
+	if (!cxt->start_sect) {
+		pr_err("Non-existent partition %d selected\n", cxt->partno);
+		return;
+	}
+
+	/* Check for host mmc panic write polling function definitions */
+	if (!card->host->ops->req_cleanup_pending ||
+			!card->host->ops->req_completion_poll)
+		return;
+
+	cxt->card = card;
+
+	cxt->sub = kmalloc(conf->kmsg_size, GFP_KERNEL);
+	if (!cxt->sub)
+		goto out;
+
+	mrq = kzalloc(sizeof(struct mmc_request), GFP_KERNEL);
+	if (!mrq)
+		goto free_sub;
+
+	cmd = kzalloc(sizeof(struct mmc_command), GFP_KERNEL);
+	if (!cmd)
+		goto free_mrq;
+
+	stop = kzalloc(sizeof(struct mmc_command), GFP_KERNEL);
+	if (!stop)
+		goto free_cmd;
+
+	data = kzalloc(sizeof(struct mmc_data), GFP_KERNEL);
+	if (!data)
+		goto free_stop;
+
+	mrq->cmd = cmd;
+	mrq->data = data;
+	mrq->stop = stop;
+	cxt->mrq = mrq;
+
+	dev->total_size = cxt->size;
+	dev->flags = PSTORE_FLAGS_DMESG;
+	dev->read = mmcpstore_read;
+	dev->write = mmcpstore_write;
+	dev->erase = NULL;
+	dev->panic_write = mmcpstore_panic_write;
+
+	ret = register_pstore_device(&cxt->dev);
+	if (ret) {
+		pr_err("%s registering with psblk failed (%d)\n",
+				cxt->dev_name, ret);
+		goto free_data;
+	}
+
+	pr_info("%s registered as psblk backend\n", cxt->dev_name);
+	return;
+
+free_data:
+	kfree(data);
+free_stop:
+	kfree(stop);
+free_cmd:
+	kfree(cmd);
+free_mrq:
+	kfree(mrq);
+free_sub:
+	kfree(cxt->sub);
+out:
+	return;
+}
+
+void unregister_mmcpstore(void)
+{
+	struct mmcpstore_context *cxt = &oops_cxt;
+
+	unregister_pstore_device(&cxt->dev);
+	kfree(cxt->mrq->data);
+	kfree(cxt->mrq->stop);
+	kfree(cxt->mrq->cmd);
+	kfree(cxt->mrq);
+	kfree(cxt->sub);
+	cxt->card = NULL;
+}
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 29aa50711626..3889c2a90faa 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -166,6 +166,10 @@ struct mmc_request {
 
 struct mmc_card;
 
+#if IS_ENABLED(CONFIG_MMC_PSTORE)
+void mmc_wait_for_pstore_req(struct mmc_host *, struct mmc_request *);
+#endif
+
 void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq);
 int mmc_wait_for_cmd(struct mmc_host *host, struct mmc_command *cmd,
 		int retries);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index c079b932330f..7d6751005ac6 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -173,6 +173,18 @@ struct mmc_host_ops {
 	 */
 	int	(*multi_io_quirk)(struct mmc_card *card,
 				  unsigned int direction, int blk_size);
+
+#if IS_ENABLED(CONFIG_MMC_PSTORE)
+	/*
+	 * The following two APIs are introduced to support mmcpstore
+	 * functionality. Cleanup API to terminate the ongoing and
+	 * pending requests before a panic write post, and polling API
+	 * to ensure that write succeeds before the Kernel dies.
+	 */
+	void	(*req_cleanup_pending)(struct mmc_host *host);
+	int	(*req_completion_poll)(struct mmc_host *host,
+					unsigned long timeout);
+#endif
 };
 
 struct mmc_cqe_ops {
-- 
2.17.1


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

* [PATCH v2 2/2] mmc: cavium: Add MMC polling method to support kmsg panic/oops write
  2020-11-23 11:19 [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk Bhaskara Budiredla
@ 2020-11-23 11:19 ` Bhaskara Budiredla
  2020-11-24 11:37 ` [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk Christoph Hellwig
  2020-12-01 20:26 ` Kees Cook
  2 siblings, 0 replies; 9+ messages in thread
From: Bhaskara Budiredla @ 2020-11-23 11:19 UTC (permalink / raw)
  To: ulf.hansson, keescook, ccross, tony.luck, sgoutham
  Cc: linux-mmc, linux-kernel, outgoing2/0000-cover-letter.patch,
	Bhaskara Budiredla

To enable the writing of panic and oops logs, a cavium specific MMC
polling method is defined and thereby ensure the functioning of mmcpstore.

Signed-off-by: Bhaskara Budiredla <bbudiredla@marvell.com>
---
 drivers/mmc/host/cavium-thunderx.c | 10 +++++
 drivers/mmc/host/cavium.c          | 67 ++++++++++++++++++++++++++++++
 drivers/mmc/host/cavium.h          |  3 ++
 3 files changed, 80 insertions(+)

diff --git a/drivers/mmc/host/cavium-thunderx.c b/drivers/mmc/host/cavium-thunderx.c
index 76013bbbcff3..83f25dd6820a 100644
--- a/drivers/mmc/host/cavium-thunderx.c
+++ b/drivers/mmc/host/cavium-thunderx.c
@@ -19,12 +19,22 @@
 
 static void thunder_mmc_acquire_bus(struct cvm_mmc_host *host)
 {
+#if IS_ENABLED(CONFIG_MMC_PSTORE)
+	if (!host->pstore)
+		down(&host->mmc_serializer);
+#else
 	down(&host->mmc_serializer);
+#endif
 }
 
 static void thunder_mmc_release_bus(struct cvm_mmc_host *host)
 {
+#if IS_ENABLED(CONFIG_MMC_PSTORE)
+	if (!host->pstore)
+		up(&host->mmc_serializer);
+#else
 	up(&host->mmc_serializer);
+#endif
 }
 
 static void thunder_mmc_int_enable(struct cvm_mmc_host *host, u64 val)
diff --git a/drivers/mmc/host/cavium.c b/drivers/mmc/host/cavium.c
index c5da3aaee334..708bec9d0345 100644
--- a/drivers/mmc/host/cavium.c
+++ b/drivers/mmc/host/cavium.c
@@ -510,6 +510,66 @@ irqreturn_t cvm_mmc_interrupt(int irq, void *dev_id)
 	return IRQ_RETVAL(emm_int != 0);
 }
 
+#if IS_ENABLED(CONFIG_MMC_PSTORE)
+static int cvm_req_completion_poll(struct mmc_host *host, unsigned long msecs)
+{
+	struct cvm_mmc_slot *slot = mmc_priv(host);
+	struct cvm_mmc_host *cvm_host = slot->host;
+	u64 emm_int;
+
+	while (msecs) {
+		emm_int = readq(cvm_host->base + MIO_EMM_INT(cvm_host));
+
+		if (emm_int & MIO_EMM_INT_DMA_DONE)
+			return 0;
+		else if (emm_int & MIO_EMM_INT_DMA_ERR)
+			return -EIO;
+		mdelay(1);
+		msecs--;
+	}
+
+	return -ETIMEDOUT;
+}
+
+static void cvm_req_cleanup_pending(struct mmc_host *host)
+{
+	struct cvm_mmc_slot *slot = mmc_priv(host);
+	struct cvm_mmc_host *cvm_host = slot->host;
+	u64 fifo_cfg;
+	u64 dma_cfg;
+	u64 emm_int;
+
+	cvm_host->pstore = 1;
+
+	/* Clear pending DMA FIFO queue */
+	fifo_cfg = readq(cvm_host->dma_base + MIO_EMM_DMA_FIFO_CFG(cvm_host));
+	if (FIELD_GET(MIO_EMM_DMA_FIFO_CFG_COUNT, fifo_cfg))
+		writeq(MIO_EMM_DMA_FIFO_CFG_CLR,
+			cvm_host->dma_base + MIO_EMM_DMA_FIFO_CFG(cvm_host));
+
+	/* Clear ongoing DMA, if there is any */
+	dma_cfg = readq(cvm_host->dma_base + MIO_EMM_DMA_CFG(cvm_host));
+	if (dma_cfg & MIO_EMM_DMA_CFG_EN) {
+		dma_cfg |= MIO_EMM_DMA_CFG_CLR;
+		writeq(dma_cfg, cvm_host->dma_base +
+				MIO_EMM_DMA_CFG(cvm_host));
+		do {
+			dma_cfg = readq(cvm_host->dma_base +
+					MIO_EMM_DMA_CFG(cvm_host));
+		} while (dma_cfg & MIO_EMM_DMA_CFG_EN);
+	}
+
+	/* Clear pending DMA interrupts */
+	emm_int = readq(cvm_host->base + MIO_EMM_INT(cvm_host));
+	if (emm_int)
+		writeq(emm_int, cvm_host->base + MIO_EMM_INT(cvm_host));
+
+	/* Clear prepared and yet to be fired DMA requests */
+	cvm_host->current_req = NULL;
+	cvm_host->dma_active = false;
+}
+#endif
+
 /*
  * Program DMA_CFG and if needed DMA_ADR.
  * Returns 0 on error, DMA address otherwise.
@@ -901,6 +961,10 @@ static const struct mmc_host_ops cvm_mmc_ops = {
 	.set_ios        = cvm_mmc_set_ios,
 	.get_ro		= mmc_gpio_get_ro,
 	.get_cd		= mmc_gpio_get_cd,
+#if IS_ENABLED(CONFIG_MMC_PSTORE)
+	.req_cleanup_pending = cvm_req_cleanup_pending,
+	.req_completion_poll = cvm_req_completion_poll,
+#endif
 };
 
 static void cvm_mmc_set_clock(struct cvm_mmc_slot *slot, unsigned int clock)
@@ -1058,6 +1122,9 @@ int cvm_mmc_of_slot_probe(struct device *dev, struct cvm_mmc_host *host)
 	slot->bus_id = id;
 	slot->cached_rca = 1;
 
+#if IS_ENABLED(CONFIG_MMC_PSTORE)
+	host->pstore = 0;
+#endif
 	host->acquire_bus(host);
 	host->slot[id] = slot;
 	cvm_mmc_switch_to(slot);
diff --git a/drivers/mmc/host/cavium.h b/drivers/mmc/host/cavium.h
index f3eea5eaa678..248a5a6e3522 100644
--- a/drivers/mmc/host/cavium.h
+++ b/drivers/mmc/host/cavium.h
@@ -75,6 +75,9 @@ struct cvm_mmc_host {
 	spinlock_t irq_handler_lock;
 	struct semaphore mmc_serializer;
 
+#if IS_ENABLED(CONFIG_MMC_PSTORE)
+	bool pstore;
+#endif
 	struct gpio_desc *global_pwr_gpiod;
 	atomic_t shared_power_users;
 
-- 
2.17.1


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

* Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk
  2020-11-23 11:19 [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk Bhaskara Budiredla
  2020-11-23 11:19 ` [PATCH v2 2/2] mmc: cavium: Add MMC polling method to support kmsg panic/oops write Bhaskara Budiredla
@ 2020-11-24 11:37 ` Christoph Hellwig
  2020-11-24 14:22   ` Ulf Hansson
  2020-12-01 20:28   ` Kees Cook
  2020-12-01 20:26 ` Kees Cook
  2 siblings, 2 replies; 9+ messages in thread
From: Christoph Hellwig @ 2020-11-24 11:37 UTC (permalink / raw)
  To: Bhaskara Budiredla
  Cc: ulf.hansson, keescook, ccross, tony.luck, sgoutham, linux-mmc,
	linux-kernel, outgoing2/0000-cover-letter.patch

Please hold this off for now.  I have a major rewrite of the pstore/blk
interface pending..

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

* Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk
  2020-11-24 11:37 ` [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk Christoph Hellwig
@ 2020-11-24 14:22   ` Ulf Hansson
  2020-12-01 20:28   ` Kees Cook
  1 sibling, 0 replies; 9+ messages in thread
From: Ulf Hansson @ 2020-11-24 14:22 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Bhaskara Budiredla, Kees Cook, Colin Cross, Tony Luck,
	Sunil Kovvuri Goutham, linux-mmc, Linux Kernel Mailing List,
	outgoing2/0000-cover-letter.patch

On Tue, 24 Nov 2020 at 12:37, Christoph Hellwig <hch@infradead.org> wrote:
>
> Please hold this off for now.  I have a major rewrite of the pstore/blk
> interface pending..

Sure, thanks for letting us know!

Kind regards
Uffe

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

* Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk
  2020-11-23 11:19 [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk Bhaskara Budiredla
  2020-11-23 11:19 ` [PATCH v2 2/2] mmc: cavium: Add MMC polling method to support kmsg panic/oops write Bhaskara Budiredla
  2020-11-24 11:37 ` [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk Christoph Hellwig
@ 2020-12-01 20:26 ` Kees Cook
  2020-12-02  6:36   ` [EXT] " Bhaskara Budiredla
  2 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2020-12-01 20:26 UTC (permalink / raw)
  To: Bhaskara Budiredla
  Cc: ulf.hansson, ccross, tony.luck, sgoutham, linux-mmc,
	linux-kernel, outgoing2/0000-cover-letter.patch

On Mon, Nov 23, 2020 at 04:49:24PM +0530, Bhaskara Budiredla wrote:
> This patch introduces to mmcpstore. The functioning of mmcpstore
> is similar to mtdpstore. mmcpstore works on FTL based flash devices
> whereas mtdpstore works on raw flash devices. When the system crashes,
> mmcpstore stores the kmsg panic and oops logs to a user specified
> MMC device.
> 
> It collects the details about the host MMC device through pstore/blk
> "blkdev" parameter. The user can specify the MMC device in many ways
> by checking in Documentation/admin-guide/pstore-blk.rst.
> 
> The individual mmc host drivers have to define suitable polling
> subroutines to write kmsg panic/oops logs through mmcpstore.
> 
> Signed-off-by: Bhaskara Budiredla <bbudiredla@marvell.com>
> ---
>  drivers/mmc/core/Kconfig     |  15 +-
>  drivers/mmc/core/Makefile    |   1 +
>  drivers/mmc/core/block.c     |  19 +++
>  drivers/mmc/core/block.h     |   9 ++
>  drivers/mmc/core/core.c      |  24 +++
>  drivers/mmc/core/mmcpstore.c | 302 +++++++++++++++++++++++++++++++++++
>  include/linux/mmc/core.h     |   4 +
>  include/linux/mmc/host.h     |  12 ++
>  8 files changed, 385 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mmc/core/mmcpstore.c
> 
> diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
> index c12fe13e4b14..505450a6ea2b 100644
> --- a/drivers/mmc/core/Kconfig
> +++ b/drivers/mmc/core/Kconfig
> @@ -34,9 +34,23 @@ config PWRSEQ_SIMPLE
>  	  This driver can also be built as a module. If so, the module
>  	  will be called pwrseq_simple.
>  
> +config MMC_PSTORE_BACKEND
> +	bool "Log panic/oops to a MMC buffer"
> +	depends on MMC_BLOCK
> +	default n

"default n" is redundant and can be dropped.

> +	help
> +	  This option will let you create platform backend to store kmsg
> +	  crash dumps to a user specified MMC device. This is primarily
> +	  based on pstore/blk.
> +
> +config MMC_PSTORE
> +	tristate
> +	select PSTORE_BLK

I don't understand why this is separate?

> +
>  config MMC_BLOCK
>  	tristate "MMC block device driver"
>  	depends on BLOCK
> +	select MMC_PSTORE if MMC_PSTORE_BACKEND=y
>  	default y
>  	help
>  	  Say Y here to enable the MMC block device driver support.
> @@ -80,4 +94,3 @@ config MMC_TEST
>  
>  	  This driver is only of interest to those developing or
>  	  testing a host driver. Most people should say N here.
> -

Why isn't this just written as:

config MMC_PSTORE
	bool "Log panic/oops to a MMC buffer"
	depends on MMC_BLOCK
	select PSTORE_BLK
	help
	  This option will let you create platform backend to store kmsg
	  crash dumps to a user specified MMC device. This is primarily
	  based on pstore/blk.




> diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
> index 95ffe008ebdf..7cb9a3af4827 100644
> --- a/drivers/mmc/core/Makefile
> +++ b/drivers/mmc/core/Makefile
> @@ -16,5 +16,6 @@ obj-$(CONFIG_PWRSEQ_EMMC)	+= pwrseq_emmc.o
>  mmc_core-$(CONFIG_DEBUG_FS)	+= debugfs.o
>  obj-$(CONFIG_MMC_BLOCK)		+= mmc_block.o
>  mmc_block-objs			:= block.o queue.o
> +mmc_block-$(CONFIG_MMC_PSTORE)	+= mmcpstore.o
>  obj-$(CONFIG_MMC_TEST)		+= mmc_test.o
>  obj-$(CONFIG_SDIO_UART)		+= sdio_uart.o
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 8d3df0be0355..ed012a91e3a3 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -2870,6 +2870,21 @@ static void mmc_blk_remove_debugfs(struct mmc_card *card,
>  
>  #endif /* CONFIG_DEBUG_FS */
>  
> +#if IS_ENABLED(CONFIG_MMC_PSTORE)
> +sector_t mmc_blk_get_part(struct mmc_card *card, int part_num, sector_t *size)
> +{
> +	struct mmc_blk_data *md = dev_get_drvdata(&card->dev);
> +	struct gendisk *disk = md->disk;
> +	struct disk_part_tbl *part_tbl = disk->part_tbl;
> +
> +	if (part_num < 0 || part_num >= part_tbl->len)
> +		return 0;
> +
> +	*size = part_tbl->part[part_num]->nr_sects << SECTOR_SHIFT;
> +	return part_tbl->part[part_num]->start_sect;
> +}
> +#endif
> +
>  static int mmc_blk_probe(struct mmc_card *card)
>  {
>  	struct mmc_blk_data *md, *part_md;
> @@ -2913,6 +2928,9 @@ static int mmc_blk_probe(struct mmc_card *card)
>  			goto out;
>  	}
>  
> +	if (mmc_card_mmc(card) || mmc_card_sd(card))
> +		mmcpstore_card_set(card, md->disk->disk_name);
> +
>  	/* Add two debugfs entries */
>  	mmc_blk_add_debugfs(card, md);
>  
> @@ -3060,6 +3078,7 @@ static void __exit mmc_blk_exit(void)
>  	unregister_blkdev(MMC_BLOCK_MAJOR, "mmc");
>  	unregister_chrdev_region(mmc_rpmb_devt, MAX_DEVICES);
>  	bus_unregister(&mmc_rpmb_bus_type);
> +	unregister_mmcpstore();
>  }
>  
>  module_init(mmc_blk_init);
> diff --git a/drivers/mmc/core/block.h b/drivers/mmc/core/block.h
> index 31153f656f41..2a4ee5568194 100644
> --- a/drivers/mmc/core/block.h
> +++ b/drivers/mmc/core/block.h
> @@ -16,5 +16,14 @@ void mmc_blk_mq_recovery(struct mmc_queue *mq);
>  struct work_struct;
>  
>  void mmc_blk_mq_complete_work(struct work_struct *work);
> +#if IS_ENABLED(CONFIG_MMC_PSTORE)
> +sector_t mmc_blk_get_part(struct mmc_card *card, int part_num, sector_t *size);
> +void mmcpstore_card_set(struct mmc_card *card, const char *disk_name);
> +void unregister_mmcpstore(void);
> +#else
> +static inline void mmcpstore_card_set(struct mmc_card *card,
> +					const char *disk_name) {}
> +static inline void unregister_mmcpstore(void) {}
> +#endif
>  
>  #endif
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index d42037f0f10d..7682b267f1d5 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -569,6 +569,30 @@ int mmc_cqe_recovery(struct mmc_host *host)
>  }
>  EXPORT_SYMBOL(mmc_cqe_recovery);
>  
> +#if IS_ENABLED(CONFIG_MMC_PSTORE)
> +/**
> + *	mmc_wait_for_pstore_req - initiate a blocking mmc request
> + *	@host: MMC host to start command
> + *	@mrq: MMC request to start
> + *
> + *	Start a blocking MMC request for a host and wait for the request
> + *	to complete that is based on polling and timeout.
> + */
> +void mmc_wait_for_pstore_req(struct mmc_host *host, struct mmc_request *mrq)
> +{
> +	unsigned int timeout;
> +
> +	host->ops->req_cleanup_pending(host);
> +	mmc_start_request(host, mrq);
> +
> +	if (mrq->data) {
> +		timeout = mrq->data->timeout_ns / NSEC_PER_MSEC;
> +		host->ops->req_completion_poll(host, timeout);
> +	}
> +}
> +EXPORT_SYMBOL(mmc_wait_for_pstore_req);
> +#endif
> +
>  /**
>   *	mmc_is_req_done - Determine if a 'cap_cmd_during_tfr' request is done
>   *	@host: MMC host
> diff --git a/drivers/mmc/core/mmcpstore.c b/drivers/mmc/core/mmcpstore.c
> new file mode 100644
> index 000000000000..1113eae0756c
> --- /dev/null
> +++ b/drivers/mmc/core/mmcpstore.c
> @@ -0,0 +1,302 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MMC pstore support based on pstore/blk
> + *
> + * Copyright (c) 2020 Marvell.
> + * Author: Bhaskara Budiredla <bbudiredla@marvell.com>
> + */
> +
> +#define pr_fmt(fmt) "mmcpstore: " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pstore_blk.h>
> +#include <linux/blkdev.h>
> +#include <linux/mount.h>
> +#include <linux/slab.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/card.h>
> +#include <linux/scatterlist.h>
> +#include "block.h"
> +#include "card.h"
> +#include "core.h"
> +
> +static struct mmcpstore_context {
> +	char dev_name[BDEVNAME_SIZE];
> +	int partno;
> +	sector_t start_sect;
> +	sector_t size;
> +	struct pstore_device_info dev;
> +	struct pstore_blk_config conf;
> +	struct pstore_blk_info info;
> +
> +	char *sub;
> +	struct mmc_card	*card;
> +	struct mmc_request *mrq;
> +} oops_cxt;
> +
> +static void mmc_prep_req(struct mmc_request *mrq,
> +		unsigned int sect_offset, unsigned int nsects,
> +		struct scatterlist *sg, u32 opcode, unsigned int flags)
> +{
> +	mrq->cmd->opcode = opcode;
> +	mrq->cmd->arg = sect_offset;
> +	mrq->cmd->flags = MMC_RSP_R1 | MMC_CMD_ADTC;
> +
> +	if (nsects == 1) {
> +		mrq->stop = NULL;
> +	} else {
> +		mrq->stop->opcode = MMC_STOP_TRANSMISSION;
> +		mrq->stop->arg = 0;
> +		mrq->stop->flags = MMC_RSP_R1B | MMC_CMD_AC;
> +	}
> +
> +	mrq->data->blksz = SECTOR_SIZE;
> +	mrq->data->blocks = nsects;
> +	mrq->data->flags = flags;
> +	mrq->data->sg = sg;
> +	mrq->data->sg_len = 1;
> +}
> +
> +static int mmcpstore_rdwr_req(const char *buf, unsigned int nsects,
> +			unsigned int sect_offset, unsigned int flags)
> +{
> +	struct mmcpstore_context *cxt = &oops_cxt;
> +	struct mmc_request *mrq = cxt->mrq;
> +	struct mmc_card *card = cxt->card;
> +	struct mmc_host *host = card->host;
> +	struct scatterlist sg;
> +	u32 opcode;
> +
> +	if (flags == MMC_DATA_READ)
> +		opcode	= (nsects > 1) ?
> +			MMC_READ_MULTIPLE_BLOCK : MMC_READ_SINGLE_BLOCK;
> +	else
> +		opcode = (nsects > 1) ?
> +			MMC_WRITE_MULTIPLE_BLOCK : MMC_WRITE_BLOCK;
> +
> +	mmc_prep_req(mrq, sect_offset, nsects, &sg, opcode, flags);
> +	sg_init_one(&sg, buf, (nsects << SECTOR_SHIFT));
> +	mmc_set_data_timeout(mrq->data, cxt->card);
> +
> +	mmc_claim_host(host);
> +	mmc_wait_for_req(host, mrq);
> +	mdelay(mrq->data->timeout_ns / NSEC_PER_MSEC);
> +	mmc_release_host(host);
> +
> +	if (mrq->cmd->error) {
> +		pr_err("Cmd error: %d\n", mrq->cmd->error);
> +		return mrq->cmd->error;
> +	}
> +	if (mrq->data->error) {
> +		pr_err("Data error: %d\n", mrq->data->error);
> +		return mrq->data->error;
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t mmcpstore_write(const char *buf, size_t size, loff_t off)
> +{
> +	struct mmcpstore_context *cxt = &oops_cxt;
> +	int ret;
> +
> +	ret = mmcpstore_rdwr_req(buf, (size >> SECTOR_SHIFT),
> +		cxt->start_sect + (off >> SECTOR_SHIFT), MMC_DATA_WRITE);
> +	if (ret)
> +		return ret;
> +
> +	return size;
> +}
> +
> +static ssize_t mmcpstore_read(char *buf, size_t size, loff_t off)
> +{
> +	struct mmcpstore_context *cxt = &oops_cxt;
> +	unsigned int sect_off = cxt->start_sect  + (off >> SECTOR_SHIFT);
> +	unsigned long sects = (cxt->conf.kmsg_size >> SECTOR_SHIFT);
> +	int ret;
> +
> +	if (unlikely(!buf || !size))
> +		return -EINVAL;
> +
> +	ret = mmcpstore_rdwr_req(cxt->sub, sects, sect_off, MMC_DATA_READ);
> +	if (ret)
> +		return ret;
> +	memcpy(buf, cxt->sub, size);
> +
> +	return size;
> +}
> +
> +static void mmcpstore_panic_write_req(const char *buf,
> +		unsigned int nsects, unsigned int sect_offset)
> +{
> +	struct mmcpstore_context *cxt = &oops_cxt;
> +	struct mmc_request *mrq = cxt->mrq;
> +	struct mmc_card *card = cxt->card;
> +	struct mmc_host *host = card->host;
> +	struct scatterlist sg;
> +	u32 opcode;
> +
> +	opcode = (nsects > 1) ? MMC_WRITE_MULTIPLE_BLOCK : MMC_WRITE_BLOCK;
> +	mmc_prep_req(mrq, sect_offset, nsects, &sg, opcode, MMC_DATA_WRITE);
> +	sg_init_one(&sg, buf, (nsects << SECTOR_SHIFT));
> +	mmc_set_data_timeout(mrq->data, cxt->card);
> +
> +	mmc_claim_host(host);
> +	mmc_wait_for_pstore_req(host, mrq);
> +	mmc_release_host(card->host);
> +}
> +
> +static ssize_t mmcpstore_panic_write(const char *buf, size_t size, loff_t off)
> +{
> +	struct mmcpstore_context *cxt = &oops_cxt;
> +
> +	mmcpstore_panic_write_req(buf, (size >> SECTOR_SHIFT),
> +			cxt->start_sect + (off >> SECTOR_SHIFT));
> +	return size;
> +}
> +
> +static struct block_device *mmcpstore_open_backend(const char *device)
> +{
> +	struct block_device *bdev;
> +	dev_t devt;
> +
> +	bdev = blkdev_get_by_path(device, FMODE_READ, NULL);
> +	if (IS_ERR(bdev)) {
> +		devt = name_to_dev_t(device);
> +		if (devt == 0)
> +			return ERR_PTR(-ENODEV);
> +
> +		bdev = blkdev_get_by_dev(devt, FMODE_READ, NULL);
> +		if (IS_ERR(bdev))
> +			return bdev;
> +	}
> +
> +	return bdev;
> +}
> +
> +static void mmcpstore_close_backend(struct block_device *bdev)
> +{
> +	if (!bdev)
> +		return;
> +	blkdev_put(bdev, FMODE_READ);
> +}
> +
> +void mmcpstore_card_set(struct mmc_card *card, const char *disk_name)
> +{
> +	struct mmcpstore_context *cxt = &oops_cxt;
> +	struct pstore_blk_config *conf = &cxt->conf;
> +	struct pstore_device_info *dev = &cxt->dev;
> +	struct block_device *bdev;
> +	struct mmc_command *stop;
> +	struct mmc_command *cmd;
> +	struct mmc_request *mrq;
> +	struct mmc_data *data;
> +	int ret;
> +
> +	ret = pstore_blk_get_config(conf);
> +	if (!conf->device[0]) {
> +		pr_debug("psblk backend is empty\n");
> +		return;
> +	}
> +
> +	/* Multiple backend devices not allowed */
> +	if (cxt->dev_name[0])
> +		return;
> +
> +	bdev =  mmcpstore_open_backend(conf->device);
> +	if (IS_ERR(bdev)) {
> +		pr_err("%s failed to open with %ld\n",
> +				conf->device, PTR_ERR(bdev));
> +		return;
> +	}
> +
> +	bdevname(bdev, cxt->dev_name);
> +	cxt->partno = bdev->bd_part->partno;
> +	mmcpstore_close_backend(bdev);
> +
> +	if (strncmp(cxt->dev_name, disk_name, strlen(disk_name)))
> +		return;

Why isn't this just strcmp()?

> +
> +	cxt->start_sect = mmc_blk_get_part(card, cxt->partno, &cxt->size);
> +	if (!cxt->start_sect) {
> +		pr_err("Non-existent partition %d selected\n", cxt->partno);
> +		return;
> +	}
> +
> +	/* Check for host mmc panic write polling function definitions */
> +	if (!card->host->ops->req_cleanup_pending ||
> +			!card->host->ops->req_completion_poll)
> +		return;
> +
> +	cxt->card = card;
> +
> +	cxt->sub = kmalloc(conf->kmsg_size, GFP_KERNEL);
> +	if (!cxt->sub)
> +		goto out;
> +
> +	mrq = kzalloc(sizeof(struct mmc_request), GFP_KERNEL);
> +	if (!mrq)
> +		goto free_sub;
> +
> +	cmd = kzalloc(sizeof(struct mmc_command), GFP_KERNEL);
> +	if (!cmd)
> +		goto free_mrq;
> +
> +	stop = kzalloc(sizeof(struct mmc_command), GFP_KERNEL);
> +	if (!stop)
> +		goto free_cmd;
> +
> +	data = kzalloc(sizeof(struct mmc_data), GFP_KERNEL);
> +	if (!data)
> +		goto free_stop;
> +
> +	mrq->cmd = cmd;
> +	mrq->data = data;
> +	mrq->stop = stop;
> +	cxt->mrq = mrq;
> +
> +	dev->total_size = cxt->size;
> +	dev->flags = PSTORE_FLAGS_DMESG;

Can't this support more than just DMESG? I don't see anything specific
to that. This is using pstore/zone ultimately, which can support
whatever frontends it needs to.

> +	dev->read = mmcpstore_read;
> +	dev->write = mmcpstore_write;
> +	dev->erase = NULL;

No way to remove the records?

> +	dev->panic_write = mmcpstore_panic_write;
> +
> +	ret = register_pstore_device(&cxt->dev);
> +	if (ret) {
> +		pr_err("%s registering with psblk failed (%d)\n",
> +				cxt->dev_name, ret);
> +		goto free_data;
> +	}
> +
> +	pr_info("%s registered as psblk backend\n", cxt->dev_name);
> +	return;
> +
> +free_data:
> +	kfree(data);
> +free_stop:
> +	kfree(stop);
> +free_cmd:
> +	kfree(cmd);
> +free_mrq:
> +	kfree(mrq);
> +free_sub:
> +	kfree(cxt->sub);
> +out:
> +	return;
> +}
> +
> +void unregister_mmcpstore(void)
> +{
> +	struct mmcpstore_context *cxt = &oops_cxt;
> +
> +	unregister_pstore_device(&cxt->dev);
> +	kfree(cxt->mrq->data);
> +	kfree(cxt->mrq->stop);
> +	kfree(cxt->mrq->cmd);
> +	kfree(cxt->mrq);
> +	kfree(cxt->sub);
> +	cxt->card = NULL;
> +}
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 29aa50711626..3889c2a90faa 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -166,6 +166,10 @@ struct mmc_request {
>  
>  struct mmc_card;
>  
> +#if IS_ENABLED(CONFIG_MMC_PSTORE)
> +void mmc_wait_for_pstore_req(struct mmc_host *, struct mmc_request *);
> +#endif
> +
>  void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq);
>  int mmc_wait_for_cmd(struct mmc_host *host, struct mmc_command *cmd,
>  		int retries);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index c079b932330f..7d6751005ac6 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -173,6 +173,18 @@ struct mmc_host_ops {
>  	 */
>  	int	(*multi_io_quirk)(struct mmc_card *card,
>  				  unsigned int direction, int blk_size);
> +
> +#if IS_ENABLED(CONFIG_MMC_PSTORE)
> +	/*
> +	 * The following two APIs are introduced to support mmcpstore
> +	 * functionality. Cleanup API to terminate the ongoing and
> +	 * pending requests before a panic write post, and polling API
> +	 * to ensure that write succeeds before the Kernel dies.
> +	 */
> +	void	(*req_cleanup_pending)(struct mmc_host *host);
> +	int	(*req_completion_poll)(struct mmc_host *host,
> +					unsigned long timeout);
> +#endif
>  };
>  
>  struct mmc_cqe_ops {
> -- 
> 2.17.1
> 

Otherwise, sure, this looks good to me as far as pstore is concerned.

-- 
Kees Cook

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

* Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk
  2020-11-24 11:37 ` [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk Christoph Hellwig
  2020-11-24 14:22   ` Ulf Hansson
@ 2020-12-01 20:28   ` Kees Cook
  1 sibling, 0 replies; 9+ messages in thread
From: Kees Cook @ 2020-12-01 20:28 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Bhaskara Budiredla, ulf.hansson, ccross, tony.luck, sgoutham,
	linux-mmc, linux-kernel

On Tue, Nov 24, 2020 at 11:37:20AM +0000, Christoph Hellwig wrote:
> Please hold this off for now.  I have a major rewrite of the pstore/blk
> interface pending..

I'm fine with taking these patches -- they're using the pstore "device"
interface, not the pstore "blk" interface, and I don't expect to be
making big changes to the existing structures.

With feedback on patch 1 addressed, I'd be happy to Ack this going via
the mmc tree.

-- 
Kees Cook

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

* RE: [EXT] Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk
  2020-12-01 20:26 ` Kees Cook
@ 2020-12-02  6:36   ` Bhaskara Budiredla
  2020-12-02 19:32     ` Kees Cook
  0 siblings, 1 reply; 9+ messages in thread
From: Bhaskara Budiredla @ 2020-12-02  6:36 UTC (permalink / raw)
  To: Kees Cook
  Cc: ulf.hansson, ccross, tony.luck, Sunil Kovvuri Goutham, linux-mmc,
	linux-kernel, outgoing2/0000-cover-letter.patch



>-----Original Message-----
>From: Kees Cook <keescook@chromium.org>
>Sent: Wednesday, December 2, 2020 1:56 AM
>To: Bhaskara Budiredla <bbudiredla@marvell.com>
>Cc: ulf.hansson@linaro.org; ccross@android.com; tony.luck@intel.com; Sunil
>Kovvuri Goutham <sgoutham@marvell.com>; linux-mmc@vger.kernel.org;
>linux-kernel@vger.kernel.org; outgoing2/0000-cover-letter.patch@mx0b-
>0016f401.pphosted.com
>Subject: [EXT] Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on
>pstore/blk
>
>External Email
>
>----------------------------------------------------------------------
>On Mon, Nov 23, 2020 at 04:49:24PM +0530, Bhaskara Budiredla wrote:
>> This patch introduces to mmcpstore. The functioning of mmcpstore is
>> similar to mtdpstore. mmcpstore works on FTL based flash devices
>> whereas mtdpstore works on raw flash devices. When the system crashes,
>> mmcpstore stores the kmsg panic and oops logs to a user specified MMC
>> device.
>>
>> It collects the details about the host MMC device through pstore/blk
>> "blkdev" parameter. The user can specify the MMC device in many ways
>> by checking in Documentation/admin-guide/pstore-blk.rst.
>>
>> The individual mmc host drivers have to define suitable polling
>> subroutines to write kmsg panic/oops logs through mmcpstore.
>>
>> Signed-off-by: Bhaskara Budiredla <bbudiredla@marvell.com>
>> ---
>>  drivers/mmc/core/Kconfig     |  15 +-
>>  drivers/mmc/core/Makefile    |   1 +
>>  drivers/mmc/core/block.c     |  19 +++
>>  drivers/mmc/core/block.h     |   9 ++
>>  drivers/mmc/core/core.c      |  24 +++
>>  drivers/mmc/core/mmcpstore.c | 302
>+++++++++++++++++++++++++++++++++++
>>  include/linux/mmc/core.h     |   4 +
>>  include/linux/mmc/host.h     |  12 ++
>>  8 files changed, 385 insertions(+), 1 deletion(-)  create mode 100644
>> drivers/mmc/core/mmcpstore.c
>>
>> diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig index
>> c12fe13e4b14..505450a6ea2b 100644
>> --- a/drivers/mmc/core/Kconfig
>> +++ b/drivers/mmc/core/Kconfig
>> @@ -34,9 +34,23 @@ config PWRSEQ_SIMPLE
>>  	  This driver can also be built as a module. If so, the module
>>  	  will be called pwrseq_simple.
>>
>> +config MMC_PSTORE_BACKEND
>> +	bool "Log panic/oops to a MMC buffer"
>> +	depends on MMC_BLOCK
>> +	default n
>
>"default n" is redundant and can be dropped.

Yes, I have removed it.

>
>> +	help
>> +	  This option will let you create platform backend to store kmsg
>> +	  crash dumps to a user specified MMC device. This is primarily
>> +	  based on pstore/blk.
>> +
>> +config MMC_PSTORE
>> +	tristate
>> +	select PSTORE_BLK
>
>I don't understand why this is separate?
>
>> +
>>  config MMC_BLOCK
>>  	tristate "MMC block device driver"
>>  	depends on BLOCK
>> +	select MMC_PSTORE if MMC_PSTORE_BACKEND=y
>>  	default y
>>  	help
>>  	  Say Y here to enable the MMC block device driver support.
>> @@ -80,4 +94,3 @@ config MMC_TEST
>>
>>  	  This driver is only of interest to those developing or
>>  	  testing a host driver. Most people should say N here.
>> -
>
>Why isn't this just written as:
>
>config MMC_PSTORE
>	bool "Log panic/oops to a MMC buffer"
>	depends on MMC_BLOCK
>	select PSTORE_BLK
>	help
>	  This option will let you create platform backend to store kmsg
>	  crash dumps to a user specified MMC device. This is primarily
>	  based on pstore/blk.
>

The idea was to compile MMC_PSTORE as part of MMC_BLOCK driver,
provided it is optionally enabled.
The above arrangement compiles MMC_PSTORE 
as module: if (CONFIG_MMC_PSTORE_BACKEND == y && CONFIG_MMC_BLOCK == m)
as static:     if (CONFIG_MMC_PSTORE_BACKEND == y && CONFIG_MMC_BLOCK == y)

>
>
>
>> diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
>> index 95ffe008ebdf..7cb9a3af4827 100644
>> --- a/drivers/mmc/core/Makefile
>> +++ b/drivers/mmc/core/Makefile
>> @@ -16,5 +16,6 @@ obj-$(CONFIG_PWRSEQ_EMMC)	+=
>pwrseq_emmc.o
>>  mmc_core-$(CONFIG_DEBUG_FS)	+= debugfs.o
>>  obj-$(CONFIG_MMC_BLOCK)		+= mmc_block.o
>>  mmc_block-objs			:= block.o queue.o
>> +mmc_block-$(CONFIG_MMC_PSTORE)	+= mmcpstore.o
>>  obj-$(CONFIG_MMC_TEST)		+= mmc_test.o
>>  obj-$(CONFIG_SDIO_UART)		+= sdio_uart.o
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
>> 8d3df0be0355..ed012a91e3a3 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -2870,6 +2870,21 @@ static void mmc_blk_remove_debugfs(struct
>> mmc_card *card,
>>
>>  #endif /* CONFIG_DEBUG_FS */
>>
>> +#if IS_ENABLED(CONFIG_MMC_PSTORE)
>> +sector_t mmc_blk_get_part(struct mmc_card *card, int part_num,
>> +sector_t *size) {
>> +	struct mmc_blk_data *md = dev_get_drvdata(&card->dev);
>> +	struct gendisk *disk = md->disk;
>> +	struct disk_part_tbl *part_tbl = disk->part_tbl;
>> +
>> +	if (part_num < 0 || part_num >= part_tbl->len)
>> +		return 0;
>> +
>> +	*size = part_tbl->part[part_num]->nr_sects << SECTOR_SHIFT;
>> +	return part_tbl->part[part_num]->start_sect;
>> +}
>> +#endif
>> +
>>  static int mmc_blk_probe(struct mmc_card *card)  {
>>  	struct mmc_blk_data *md, *part_md;
>> @@ -2913,6 +2928,9 @@ static int mmc_blk_probe(struct mmc_card *card)
>>  			goto out;
>>  	}
>>
>> +	if (mmc_card_mmc(card) || mmc_card_sd(card))
>> +		mmcpstore_card_set(card, md->disk->disk_name);
>> +
>>  	/* Add two debugfs entries */
>>  	mmc_blk_add_debugfs(card, md);
>>
>> @@ -3060,6 +3078,7 @@ static void __exit mmc_blk_exit(void)
>>  	unregister_blkdev(MMC_BLOCK_MAJOR, "mmc");
>>  	unregister_chrdev_region(mmc_rpmb_devt, MAX_DEVICES);
>>  	bus_unregister(&mmc_rpmb_bus_type);
>> +	unregister_mmcpstore();
>>  }
>>
>>  module_init(mmc_blk_init);
>> diff --git a/drivers/mmc/core/block.h b/drivers/mmc/core/block.h index
>> 31153f656f41..2a4ee5568194 100644
>> --- a/drivers/mmc/core/block.h
>> +++ b/drivers/mmc/core/block.h
>> @@ -16,5 +16,14 @@ void mmc_blk_mq_recovery(struct mmc_queue
>*mq);
>> struct work_struct;
>>
>>  void mmc_blk_mq_complete_work(struct work_struct *work);
>> +#if IS_ENABLED(CONFIG_MMC_PSTORE)
>> +sector_t mmc_blk_get_part(struct mmc_card *card, int part_num,
>> +sector_t *size); void mmcpstore_card_set(struct mmc_card *card, const
>> +char *disk_name); void unregister_mmcpstore(void); #else static
>> +inline void mmcpstore_card_set(struct mmc_card *card,
>> +					const char *disk_name) {}
>> +static inline void unregister_mmcpstore(void) {} #endif
>>
>>  #endif
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
>> d42037f0f10d..7682b267f1d5 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -569,6 +569,30 @@ int mmc_cqe_recovery(struct mmc_host *host)  }
>> EXPORT_SYMBOL(mmc_cqe_recovery);
>>
>> +#if IS_ENABLED(CONFIG_MMC_PSTORE)
>> +/**
>> + *	mmc_wait_for_pstore_req - initiate a blocking mmc request
>> + *	@host: MMC host to start command
>> + *	@mrq: MMC request to start
>> + *
>> + *	Start a blocking MMC request for a host and wait for the request
>> + *	to complete that is based on polling and timeout.
>> + */
>> +void mmc_wait_for_pstore_req(struct mmc_host *host, struct
>> +mmc_request *mrq) {
>> +	unsigned int timeout;
>> +
>> +	host->ops->req_cleanup_pending(host);
>> +	mmc_start_request(host, mrq);
>> +
>> +	if (mrq->data) {
>> +		timeout = mrq->data->timeout_ns / NSEC_PER_MSEC;
>> +		host->ops->req_completion_poll(host, timeout);
>> +	}
>> +}
>> +EXPORT_SYMBOL(mmc_wait_for_pstore_req);
>> +#endif
>> +
>>  /**
>>   *	mmc_is_req_done - Determine if a 'cap_cmd_during_tfr' request is
>done
>>   *	@host: MMC host
>> diff --git a/drivers/mmc/core/mmcpstore.c
>> b/drivers/mmc/core/mmcpstore.c new file mode 100644 index
>> 000000000000..1113eae0756c
>> --- /dev/null
>> +++ b/drivers/mmc/core/mmcpstore.c
>> @@ -0,0 +1,302 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * MMC pstore support based on pstore/blk
>> + *
>> + * Copyright (c) 2020 Marvell.
>> + * Author: Bhaskara Budiredla <bbudiredla@marvell.com>  */
>> +
>> +#define pr_fmt(fmt) "mmcpstore: " fmt
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/pstore_blk.h>
>> +#include <linux/blkdev.h>
>> +#include <linux/mount.h>
>> +#include <linux/slab.h>
>> +#include <linux/mmc/mmc.h>
>> +#include <linux/mmc/host.h>
>> +#include <linux/mmc/card.h>
>> +#include <linux/scatterlist.h>
>> +#include "block.h"
>> +#include "card.h"
>> +#include "core.h"
>> +
>> +static struct mmcpstore_context {
>> +	char dev_name[BDEVNAME_SIZE];
>> +	int partno;
>> +	sector_t start_sect;
>> +	sector_t size;
>> +	struct pstore_device_info dev;
>> +	struct pstore_blk_config conf;
>> +	struct pstore_blk_info info;
>> +
>> +	char *sub;
>> +	struct mmc_card	*card;
>> +	struct mmc_request *mrq;
>> +} oops_cxt;
>> +
>> +static void mmc_prep_req(struct mmc_request *mrq,
>> +		unsigned int sect_offset, unsigned int nsects,
>> +		struct scatterlist *sg, u32 opcode, unsigned int flags) {
>> +	mrq->cmd->opcode = opcode;
>> +	mrq->cmd->arg = sect_offset;
>> +	mrq->cmd->flags = MMC_RSP_R1 | MMC_CMD_ADTC;
>> +
>> +	if (nsects == 1) {
>> +		mrq->stop = NULL;
>> +	} else {
>> +		mrq->stop->opcode = MMC_STOP_TRANSMISSION;
>> +		mrq->stop->arg = 0;
>> +		mrq->stop->flags = MMC_RSP_R1B | MMC_CMD_AC;
>> +	}
>> +
>> +	mrq->data->blksz = SECTOR_SIZE;
>> +	mrq->data->blocks = nsects;
>> +	mrq->data->flags = flags;
>> +	mrq->data->sg = sg;
>> +	mrq->data->sg_len = 1;
>> +}
>> +
>> +static int mmcpstore_rdwr_req(const char *buf, unsigned int nsects,
>> +			unsigned int sect_offset, unsigned int flags) {
>> +	struct mmcpstore_context *cxt = &oops_cxt;
>> +	struct mmc_request *mrq = cxt->mrq;
>> +	struct mmc_card *card = cxt->card;
>> +	struct mmc_host *host = card->host;
>> +	struct scatterlist sg;
>> +	u32 opcode;
>> +
>> +	if (flags == MMC_DATA_READ)
>> +		opcode	= (nsects > 1) ?
>> +			MMC_READ_MULTIPLE_BLOCK :
>MMC_READ_SINGLE_BLOCK;
>> +	else
>> +		opcode = (nsects > 1) ?
>> +			MMC_WRITE_MULTIPLE_BLOCK :
>MMC_WRITE_BLOCK;
>> +
>> +	mmc_prep_req(mrq, sect_offset, nsects, &sg, opcode, flags);
>> +	sg_init_one(&sg, buf, (nsects << SECTOR_SHIFT));
>> +	mmc_set_data_timeout(mrq->data, cxt->card);
>> +
>> +	mmc_claim_host(host);
>> +	mmc_wait_for_req(host, mrq);
>> +	mdelay(mrq->data->timeout_ns / NSEC_PER_MSEC);
>> +	mmc_release_host(host);
>> +
>> +	if (mrq->cmd->error) {
>> +		pr_err("Cmd error: %d\n", mrq->cmd->error);
>> +		return mrq->cmd->error;
>> +	}
>> +	if (mrq->data->error) {
>> +		pr_err("Data error: %d\n", mrq->data->error);
>> +		return mrq->data->error;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t mmcpstore_write(const char *buf, size_t size, loff_t
>> +off) {
>> +	struct mmcpstore_context *cxt = &oops_cxt;
>> +	int ret;
>> +
>> +	ret = mmcpstore_rdwr_req(buf, (size >> SECTOR_SHIFT),
>> +		cxt->start_sect + (off >> SECTOR_SHIFT),
>MMC_DATA_WRITE);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return size;
>> +}
>> +
>> +static ssize_t mmcpstore_read(char *buf, size_t size, loff_t off) {
>> +	struct mmcpstore_context *cxt = &oops_cxt;
>> +	unsigned int sect_off = cxt->start_sect  + (off >> SECTOR_SHIFT);
>> +	unsigned long sects = (cxt->conf.kmsg_size >> SECTOR_SHIFT);
>> +	int ret;
>> +
>> +	if (unlikely(!buf || !size))
>> +		return -EINVAL;
>> +
>> +	ret = mmcpstore_rdwr_req(cxt->sub, sects, sect_off,
>MMC_DATA_READ);
>> +	if (ret)
>> +		return ret;
>> +	memcpy(buf, cxt->sub, size);
>> +
>> +	return size;
>> +}
>> +
>> +static void mmcpstore_panic_write_req(const char *buf,
>> +		unsigned int nsects, unsigned int sect_offset) {
>> +	struct mmcpstore_context *cxt = &oops_cxt;
>> +	struct mmc_request *mrq = cxt->mrq;
>> +	struct mmc_card *card = cxt->card;
>> +	struct mmc_host *host = card->host;
>> +	struct scatterlist sg;
>> +	u32 opcode;
>> +
>> +	opcode = (nsects > 1) ? MMC_WRITE_MULTIPLE_BLOCK :
>MMC_WRITE_BLOCK;
>> +	mmc_prep_req(mrq, sect_offset, nsects, &sg, opcode,
>MMC_DATA_WRITE);
>> +	sg_init_one(&sg, buf, (nsects << SECTOR_SHIFT));
>> +	mmc_set_data_timeout(mrq->data, cxt->card);
>> +
>> +	mmc_claim_host(host);
>> +	mmc_wait_for_pstore_req(host, mrq);
>> +	mmc_release_host(card->host);
>> +}
>> +
>> +static ssize_t mmcpstore_panic_write(const char *buf, size_t size,
>> +loff_t off) {
>> +	struct mmcpstore_context *cxt = &oops_cxt;
>> +
>> +	mmcpstore_panic_write_req(buf, (size >> SECTOR_SHIFT),
>> +			cxt->start_sect + (off >> SECTOR_SHIFT));
>> +	return size;
>> +}
>> +
>> +static struct block_device *mmcpstore_open_backend(const char
>> +*device) {
>> +	struct block_device *bdev;
>> +	dev_t devt;
>> +
>> +	bdev = blkdev_get_by_path(device, FMODE_READ, NULL);
>> +	if (IS_ERR(bdev)) {
>> +		devt = name_to_dev_t(device);
>> +		if (devt == 0)
>> +			return ERR_PTR(-ENODEV);
>> +
>> +		bdev = blkdev_get_by_dev(devt, FMODE_READ, NULL);
>> +		if (IS_ERR(bdev))
>> +			return bdev;
>> +	}
>> +
>> +	return bdev;
>> +}
>> +
>> +static void mmcpstore_close_backend(struct block_device *bdev) {
>> +	if (!bdev)
>> +		return;
>> +	blkdev_put(bdev, FMODE_READ);
>> +}
>> +
>> +void mmcpstore_card_set(struct mmc_card *card, const char *disk_name)
>> +{
>> +	struct mmcpstore_context *cxt = &oops_cxt;
>> +	struct pstore_blk_config *conf = &cxt->conf;
>> +	struct pstore_device_info *dev = &cxt->dev;
>> +	struct block_device *bdev;
>> +	struct mmc_command *stop;
>> +	struct mmc_command *cmd;
>> +	struct mmc_request *mrq;
>> +	struct mmc_data *data;
>> +	int ret;
>> +
>> +	ret = pstore_blk_get_config(conf);
>> +	if (!conf->device[0]) {
>> +		pr_debug("psblk backend is empty\n");
>> +		return;
>> +	}
>> +
>> +	/* Multiple backend devices not allowed */
>> +	if (cxt->dev_name[0])
>> +		return;
>> +
>> +	bdev =  mmcpstore_open_backend(conf->device);
>> +	if (IS_ERR(bdev)) {
>> +		pr_err("%s failed to open with %ld\n",
>> +				conf->device, PTR_ERR(bdev));
>> +		return;
>> +	}
>> +
>> +	bdevname(bdev, cxt->dev_name);
>> +	cxt->partno = bdev->bd_part->partno;
>> +	mmcpstore_close_backend(bdev);
>> +
>> +	if (strncmp(cxt->dev_name, disk_name, strlen(disk_name)))
>> +		return;
>
>Why isn't this just strcmp()?

The mmc disk name (disk_name) doesn't include the partition number. 
strncmp is restricted to something like /dev/mmcblk0, it doesn't cover full /dev/mmcblk0pn.
The partition number check is carried out in the next statement.
 
>
>> +
>> +	cxt->start_sect = mmc_blk_get_part(card, cxt->partno, &cxt->size);
>> +	if (!cxt->start_sect) {
>> +		pr_err("Non-existent partition %d selected\n", cxt->partno);
>> +		return;
>> +	}
>> +
>> +	/* Check for host mmc panic write polling function definitions */
>> +	if (!card->host->ops->req_cleanup_pending ||
>> +			!card->host->ops->req_completion_poll)
>> +		return;
>> +
>> +	cxt->card = card;
>> +
>> +	cxt->sub = kmalloc(conf->kmsg_size, GFP_KERNEL);
>> +	if (!cxt->sub)
>> +		goto out;
>> +
>> +	mrq = kzalloc(sizeof(struct mmc_request), GFP_KERNEL);
>> +	if (!mrq)
>> +		goto free_sub;
>> +
>> +	cmd = kzalloc(sizeof(struct mmc_command), GFP_KERNEL);
>> +	if (!cmd)
>> +		goto free_mrq;
>> +
>> +	stop = kzalloc(sizeof(struct mmc_command), GFP_KERNEL);
>> +	if (!stop)
>> +		goto free_cmd;
>> +
>> +	data = kzalloc(sizeof(struct mmc_data), GFP_KERNEL);
>> +	if (!data)
>> +		goto free_stop;
>> +
>> +	mrq->cmd = cmd;
>> +	mrq->data = data;
>> +	mrq->stop = stop;
>> +	cxt->mrq = mrq;
>> +
>> +	dev->total_size = cxt->size;
>> +	dev->flags = PSTORE_FLAGS_DMESG;
>
>Can't this support more than just DMESG? I don't see anything specific to that.
>This is using pstore/zone ultimately, which can support whatever frontends it
>needs to.
>

Yes, as of now the support is only for DMESG. We will extend this to other frontends
on need basis.

>> +	dev->read = mmcpstore_read;
>> +	dev->write = mmcpstore_write;
>> +	dev->erase = NULL;
>
>No way to remove the records?
>

Yes, at this time, no removal of records.

>> +	dev->panic_write = mmcpstore_panic_write;
>> +
>> +	ret = register_pstore_device(&cxt->dev);
>> +	if (ret) {
>> +		pr_err("%s registering with psblk failed (%d)\n",
>> +				cxt->dev_name, ret);
>> +		goto free_data;
>> +	}
>> +
>> +	pr_info("%s registered as psblk backend\n", cxt->dev_name);
>> +	return;
>> +
>> +free_data:
>> +	kfree(data);
>> +free_stop:
>> +	kfree(stop);
>> +free_cmd:
>> +	kfree(cmd);
>> +free_mrq:
>> +	kfree(mrq);
>> +free_sub:
>> +	kfree(cxt->sub);
>> +out:
>> +	return;
>> +}
>> +
>> +void unregister_mmcpstore(void)
>> +{
>> +	struct mmcpstore_context *cxt = &oops_cxt;
>> +
>> +	unregister_pstore_device(&cxt->dev);
>> +	kfree(cxt->mrq->data);
>> +	kfree(cxt->mrq->stop);
>> +	kfree(cxt->mrq->cmd);
>> +	kfree(cxt->mrq);
>> +	kfree(cxt->sub);
>> +	cxt->card = NULL;
>> +}
>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h index
>> 29aa50711626..3889c2a90faa 100644
>> --- a/include/linux/mmc/core.h
>> +++ b/include/linux/mmc/core.h
>> @@ -166,6 +166,10 @@ struct mmc_request {
>>
>>  struct mmc_card;
>>
>> +#if IS_ENABLED(CONFIG_MMC_PSTORE)
>> +void mmc_wait_for_pstore_req(struct mmc_host *, struct mmc_request
>> +*); #endif
>> +
>>  void mmc_wait_for_req(struct mmc_host *host, struct mmc_request
>> *mrq);  int mmc_wait_for_cmd(struct mmc_host *host, struct
>mmc_command *cmd,
>>  		int retries);
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index
>> c079b932330f..7d6751005ac6 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -173,6 +173,18 @@ struct mmc_host_ops {
>>  	 */
>>  	int	(*multi_io_quirk)(struct mmc_card *card,
>>  				  unsigned int direction, int blk_size);
>> +
>> +#if IS_ENABLED(CONFIG_MMC_PSTORE)
>> +	/*
>> +	 * The following two APIs are introduced to support mmcpstore
>> +	 * functionality. Cleanup API to terminate the ongoing and
>> +	 * pending requests before a panic write post, and polling API
>> +	 * to ensure that write succeeds before the Kernel dies.
>> +	 */
>> +	void	(*req_cleanup_pending)(struct mmc_host *host);
>> +	int	(*req_completion_poll)(struct mmc_host *host,
>> +					unsigned long timeout);
>> +#endif
>>  };
>>
>>  struct mmc_cqe_ops {
>> --
>> 2.17.1
>>
>
>Otherwise, sure, this looks good to me as far as pstore is concerned.
>
>--
>Kees Cook

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

* Re: [EXT] Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk
  2020-12-02  6:36   ` [EXT] " Bhaskara Budiredla
@ 2020-12-02 19:32     ` Kees Cook
  2020-12-03  5:41       ` Bhaskara Budiredla
  0 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2020-12-02 19:32 UTC (permalink / raw)
  To: Bhaskara Budiredla
  Cc: ulf.hansson, ccross, tony.luck, Sunil Kovvuri Goutham, linux-mmc,
	linux-kernel, outgoing2/0000-cover-letter.patch

On Wed, Dec 02, 2020 at 06:36:21AM +0000, Bhaskara Budiredla wrote:
> >From: Kees Cook <keescook@chromium.org>
> >On Mon, Nov 23, 2020 at 04:49:24PM +0530, Bhaskara Budiredla wrote:
> >Why isn't this just written as:
> >
> >config MMC_PSTORE
> >	bool "Log panic/oops to a MMC buffer"
> >	depends on MMC_BLOCK
> >	select PSTORE_BLK
> >	help
> >	  This option will let you create platform backend to store kmsg
> >	  crash dumps to a user specified MMC device. This is primarily
> >	  based on pstore/blk.
> >
> 
> The idea was to compile MMC_PSTORE as part of MMC_BLOCK driver,
> provided it is optionally enabled.
> The above arrangement compiles MMC_PSTORE 
> as module: if (CONFIG_MMC_PSTORE_BACKEND == y && CONFIG_MMC_BLOCK == m)
> as static:     if (CONFIG_MMC_PSTORE_BACKEND == y && CONFIG_MMC_BLOCK == y)

Ah, okay. If it's a tri-state, wouldn't it track CONFIG_MMC_BLOCK's
state? As in, does this work:

config MMC_PSTORE
	tristate "Log panic/oops to a MMC buffer"
	depends on MMC_BLOCK
	select PSTORE_BLK
	help
	  This option will let you create platform backend to store kmsg
	  crash dumps to a user specified MMC device. This is primarily
	  based on pstore/blk.

> >> +	if (strncmp(cxt->dev_name, disk_name, strlen(disk_name)))
> >> +		return;
> >
> >Why isn't this just strcmp()?
> 
> The mmc disk name (disk_name) doesn't include the partition number. 
> strncmp is restricted to something like /dev/mmcblk0, it doesn't cover full /dev/mmcblk0pn.
> The partition number check is carried out in the next statement.

Okay, gotcha; thanks!

> >> +	dev->flags = PSTORE_FLAGS_DMESG;
> >
> >Can't this support more than just DMESG? I don't see anything specific to that.
> >This is using pstore/zone ultimately, which can support whatever frontends it
> >needs to.
> 
> Yes, as of now the support is only for DMESG. We will extend this to other frontends
> on need basis.

Okay -- I assume this has mostly to do with not having erasure (below).

> >> +	dev->erase = NULL;
> >
> >No way to remove the records?
> 
> Yes, at this time, no removal of records.

Okay. (I think this might be worth mentioning in docs somewhere.)

-- 
Kees Cook

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

* RE: [EXT] Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk
  2020-12-02 19:32     ` Kees Cook
@ 2020-12-03  5:41       ` Bhaskara Budiredla
  0 siblings, 0 replies; 9+ messages in thread
From: Bhaskara Budiredla @ 2020-12-03  5:41 UTC (permalink / raw)
  To: Kees Cook
  Cc: ulf.hansson, ccross, tony.luck, Sunil Kovvuri Goutham, linux-mmc,
	linux-kernel, outgoing2/0000-cover-letter.patch



>-----Original Message-----
>From: Kees Cook <keescook@chromium.org>
>Sent: Thursday, December 3, 2020 1:02 AM
>To: Bhaskara Budiredla <bbudiredla@marvell.com>
>Cc: ulf.hansson@linaro.org; ccross@android.com; tony.luck@intel.com; Sunil
>Kovvuri Goutham <sgoutham@marvell.com>; linux-mmc@vger.kernel.org;
>linux-kernel@vger.kernel.org; outgoing2/0000-cover-letter.patch@mx0b-
>0016f401.pphosted.com
>Subject: Re: [EXT] Re: [PATCH v2 1/2] mmc: Support kmsg dumper based on
>pstore/blk
>
>On Wed, Dec 02, 2020 at 06:36:21AM +0000, Bhaskara Budiredla wrote:
>> >From: Kees Cook <keescook@chromium.org> On Mon, Nov 23, 2020 at
>> >04:49:24PM +0530, Bhaskara Budiredla wrote:
>> >Why isn't this just written as:
>> >
>> >config MMC_PSTORE
>> >	bool "Log panic/oops to a MMC buffer"
>> >	depends on MMC_BLOCK
>> >	select PSTORE_BLK
>> >	help
>> >	  This option will let you create platform backend to store kmsg
>> >	  crash dumps to a user specified MMC device. This is primarily
>> >	  based on pstore/blk.
>> >
>>
>> The idea was to compile MMC_PSTORE as part of MMC_BLOCK driver,
>> provided it is optionally enabled.
>> The above arrangement compiles MMC_PSTORE as module: if
>> (CONFIG_MMC_PSTORE_BACKEND == y && CONFIG_MMC_BLOCK == m)
>> as static:     if (CONFIG_MMC_PSTORE_BACKEND == y &&
>CONFIG_MMC_BLOCK == y)
>
>Ah, okay. If it's a tri-state, wouldn't it track CONFIG_MMC_BLOCK's state? As
>in, does this work:
>

Yes, it's a tri-state but not compiled as a separate driver. 

>config MMC_PSTORE
>	tristate "Log panic/oops to a MMC buffer"
>	depends on MMC_BLOCK
>	select PSTORE_BLK
>	help
>	  This option will let you create platform backend to store kmsg
>	  crash dumps to a user specified MMC device. This is primarily
>	  based on pstore/blk.
>

No, this will cause problems for MMC_PSTORE=m and MMC_BLOCK=y
MMC_PSTORE automatically have to be selected as module or static
based on MMC_BLOCK selection. There were couple of function calls
from the code in MMC_BLOCK to MMC_PSTORE. Uffe prefers them
to be unconditional calls (as per discussion in v1).  

>> >> +	if (strncmp(cxt->dev_name, disk_name, strlen(disk_name)))
>> >> +		return;
>> >
>> >Why isn't this just strcmp()?
>>
>> The mmc disk name (disk_name) doesn't include the partition number.
>> strncmp is restricted to something like /dev/mmcblk0, it doesn't cover full
>/dev/mmcblk0pn.
>> The partition number check is carried out in the next statement.
>
>Okay, gotcha; thanks!
>
>> >> +	dev->flags = PSTORE_FLAGS_DMESG;
>> >
>> >Can't this support more than just DMESG? I don't see anything specific to
>that.
>> >This is using pstore/zone ultimately, which can support whatever
>> >frontends it needs to.
>>
>> Yes, as of now the support is only for DMESG. We will extend this to
>> other frontends on need basis.
>
>Okay -- I assume this has mostly to do with not having erasure (below).
>
>> >> +	dev->erase = NULL;
>> >
>> >No way to remove the records?
>>
>> Yes, at this time, no removal of records.
>
>Okay. (I think this might be worth mentioning in docs somewhere.)
>

Would it be sufficient to add corresponding notes to the commit message? 

>--
>Kees Cook

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

end of thread, other threads:[~2020-12-03  5:42 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-23 11:19 [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk Bhaskara Budiredla
2020-11-23 11:19 ` [PATCH v2 2/2] mmc: cavium: Add MMC polling method to support kmsg panic/oops write Bhaskara Budiredla
2020-11-24 11:37 ` [PATCH v2 1/2] mmc: Support kmsg dumper based on pstore/blk Christoph Hellwig
2020-11-24 14:22   ` Ulf Hansson
2020-12-01 20:28   ` Kees Cook
2020-12-01 20:26 ` Kees Cook
2020-12-02  6:36   ` [EXT] " Bhaskara Budiredla
2020-12-02 19:32     ` Kees Cook
2020-12-03  5:41       ` Bhaskara Budiredla

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).