This patch introduces to mmcpstore.
Bhaskara Budiredla (2):
mmc: Support kmsg dumper based on pstore/blk
mmc:cavium: Add MMC polling method to support kmsg panic/oops write
drivers/mmc/core/Kconfig | 7 +
drivers/mmc/core/Makefile | 1 +
drivers/mmc/core/block.c | 20 ++
drivers/mmc/core/block.h | 3 +
drivers/mmc/core/core.c | 24 +++
drivers/mmc/core/mmcpstore.c | 318 +++++++++++++++++++++++++++++
drivers/mmc/host/cavium-thunderx.c | 10 +
drivers/mmc/host/cavium.c | 67 ++++++
drivers/mmc/host/cavium.h | 3 +
include/linux/mmc/card.h | 4 +
include/linux/mmc/core.h | 4 +
include/linux/mmc/host.h | 6 +
12 files changed, 467 insertions(+)
create mode 100644 drivers/mmc/core/mmcpstore.c
--
2.17.1
This patch introduces to mmcpstore. The functioning of mmcpstore is
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 <[email protected]>
---
drivers/mmc/core/Kconfig | 7 +
drivers/mmc/core/Makefile | 1 +
drivers/mmc/core/block.c | 20 +++
drivers/mmc/core/block.h | 3 +
drivers/mmc/core/core.c | 24 +++
drivers/mmc/core/mmcpstore.c | 318 +++++++++++++++++++++++++++++++++++
include/linux/mmc/card.h | 4 +
include/linux/mmc/core.h | 4 +
include/linux/mmc/host.h | 6 +
9 files changed, 387 insertions(+)
create mode 100644 drivers/mmc/core/mmcpstore.c
diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
index c12fe13e4b14..cafb367c482d 100644
--- a/drivers/mmc/core/Kconfig
+++ b/drivers/mmc/core/Kconfig
@@ -81,3 +81,10 @@ config MMC_TEST
This driver is only of interest to those developing or
testing a host driver. Most people should say N here.
+config MMC_PSTORE
+ bool "Log panic/oops to a MMC buffer"
+ depends on PSTORE
+ depends on PSTORE_BLK
+ help
+ Backend driver to store the kmsg crash dumps to a user specified MMC
+ device. The driver is based on pstore/blk.
diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
index 95ffe008ebdf..5f4230b79ac6 100644
--- a/drivers/mmc/core/Makefile
+++ b/drivers/mmc/core/Makefile
@@ -17,4 +17,5 @@ mmc_core-$(CONFIG_DEBUG_FS) += debugfs.o
obj-$(CONFIG_MMC_BLOCK) += mmc_block.o
mmc_block-objs := block.o queue.o
obj-$(CONFIG_MMC_TEST) += mmc_test.o
+obj-$(CONFIG_MMC_PSTORE) += mmcpstore.o
obj-$(CONFIG_SDIO_UART) += sdio_uart.o
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 8d3df0be0355..f11c21d60b67 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 */
+#ifdef 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,11 @@ static int mmc_blk_probe(struct mmc_card *card)
goto out;
}
+#ifdef CONFIG_MMC_PSTORE
+ if (mmc_card_mmc(card) || mmc_card_sd(card))
+ mmcpstore_card_set(card, md->disk->disk_name);
+#endif
+
/* Add two debugfs entries */
mmc_blk_add_debugfs(card, md);
diff --git a/drivers/mmc/core/block.h b/drivers/mmc/core/block.h
index 31153f656f41..2a2b81635508 100644
--- a/drivers/mmc/core/block.h
+++ b/drivers/mmc/core/block.h
@@ -16,5 +16,8 @@ void mmc_blk_mq_recovery(struct mmc_queue *mq);
struct work_struct;
void mmc_blk_mq_complete_work(struct work_struct *work);
+#ifdef CONFIG_MMC_PSTORE
+sector_t mmc_blk_get_part(struct mmc_card *card, int part_num, sector_t *size);
+#endif
#endif
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index d42037f0f10d..7cc3d81f6a9a 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);
+
+#ifdef 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 new MMC custom command request for a host, and
+ * wait for the command to complete based on request data 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);
+ }
+}
+#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..ffa44c859e10
--- /dev/null
+++ b/drivers/mmc/core/mmcpstore.c
@@ -0,0 +1,318 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " 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;
+
+ if (!conf->device[0])
+ 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;
+}
+EXPORT_SYMBOL(mmcpstore_card_set);
+
+static int __init mmcpstore_init(void)
+{
+ struct mmcpstore_context *cxt = &oops_cxt;
+ struct pstore_blk_config *conf = &cxt->conf;
+ int err;
+
+ err = pstore_blk_get_config(conf);
+ if (unlikely(err))
+ return err;
+
+ if (!conf->device[0]) {
+ pr_err("psblk backend is empty\n");
+ return -EINVAL;
+ }
+
+ return err;
+}
+
+static void __exit mmcpstore_exit(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;
+}
+
+module_init(mmcpstore_init);
+module_exit(mmcpstore_exit);
+
+MODULE_AUTHOR("Bhaskara Budiredla <[email protected]>");
+MODULE_DESCRIPTION("MMC backend for pstore/blk");
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 42df06c6b19c..76ae8ae61d31 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -322,6 +322,10 @@ static inline bool mmc_large_sector(struct mmc_card *card)
bool mmc_card_is_blockaddr(struct mmc_card *card);
+#ifdef CONFIG_MMC_PSTORE
+void mmcpstore_card_set(struct mmc_card *card, const char *disk_name);
+#endif /* CONFIG_MMC_PSTORE */
+
#define mmc_card_mmc(c) ((c)->type == MMC_TYPE_MMC)
#define mmc_card_sd(c) ((c)->type == MMC_TYPE_SD)
#define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO)
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 29aa50711626..21dcd79f8f0e 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -166,6 +166,10 @@ struct mmc_request {
struct mmc_card;
+#ifdef CONFIG_MMC_PSTORE
+extern void mmc_wait_for_pstore_req(struct mmc_host *, struct mmc_request *);
+#endif /* CONFIG_MMC_PSTORE */
+
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..ef57313ee3ea 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -173,6 +173,12 @@ struct mmc_host_ops {
*/
int (*multi_io_quirk)(struct mmc_card *card,
unsigned int direction, int blk_size);
+
+#ifdef CONFIG_MMC_PSTORE
+ 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
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 <[email protected]>
---
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..efd3422939af 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)
{
+#ifdef 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)
{
+#ifdef 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..8f62f6612ac0 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);
}
+#ifdef 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,
+#ifdef 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;
+#ifdef 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..b72dea9a81eb 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;
+#ifdef CONFIG_MMC_PSTORE
+ bool pstore;
+#endif
struct gpio_desc *global_pwr_gpiod;
atomic_t shared_power_users;
--
2.17.1
Hi Bhaskara,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.10-rc3]
[cannot apply to next-20201112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Bhaskara-Budiredla/mmc-support-crash-logging-to-MMC-block-devices/20201112-142552
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3d5e28bff7ad55aea081c1af516cc1c94a5eca7d
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/fcb0b29143a613365939640cb07f0285fdab9a1f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Bhaskara-Budiredla/mmc-support-crash-logging-to-MMC-block-devices/20201112-142552
git checkout fcb0b29143a613365939640cb07f0285fdab9a1f
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>, old ones prefixed by <<):
ERROR: modpost: "clk_set_min_rate" [sound/soc/atmel/snd-soc-mchp-spdifrx.ko] undefined!
>> ERROR: modpost: "mmcpstore_card_set" [drivers/mmc/core/mmc_block.ko] undefined!
ERROR: modpost: "scp_get_venc_hw_capa" [drivers/media/platform/mtk-vcodec/mtk-vcodec-common.ko] undefined!
ERROR: modpost: "scp_ipi_send" [drivers/media/platform/mtk-vcodec/mtk-vcodec-common.ko] undefined!
ERROR: modpost: "scp_put" [drivers/media/platform/mtk-vcodec/mtk-vcodec-common.ko] undefined!
ERROR: modpost: "scp_get" [drivers/media/platform/mtk-vcodec/mtk-vcodec-common.ko] undefined!
ERROR: modpost: "scp_get_vdec_hw_capa" [drivers/media/platform/mtk-vcodec/mtk-vcodec-common.ko] undefined!
ERROR: modpost: "scp_ipi_register" [drivers/media/platform/mtk-vcodec/mtk-vcodec-common.ko] undefined!
ERROR: modpost: "scp_mapping_dm_addr" [drivers/media/platform/mtk-vcodec/mtk-vcodec-common.ko] undefined!
ERROR: modpost: "scp_get_rproc" [drivers/media/platform/mtk-vcodec/mtk-vcodec-common.ko] undefined!
ERROR: modpost: "rproc_boot" [drivers/media/platform/mtk-vcodec/mtk-vcodec-common.ko] undefined!
ERROR: modpost: "__delay" [drivers/net/mdio/mdio-cavium.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Thu, 12 Nov 2020 at 07:24, Bhaskara Budiredla <[email protected]> wrote:
>
> This patch introduces to mmcpstore. The functioning of mmcpstore is
> 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.
I don't like that changes to host drivers are needed to support this,
but perhaps there is no other good way!?
>
> Signed-off-by: Bhaskara Budiredla <[email protected]>
> ---
> drivers/mmc/core/Kconfig | 7 +
> drivers/mmc/core/Makefile | 1 +
> drivers/mmc/core/block.c | 20 +++
> drivers/mmc/core/block.h | 3 +
> drivers/mmc/core/core.c | 24 +++
> drivers/mmc/core/mmcpstore.c | 318 +++++++++++++++++++++++++++++++++++
> include/linux/mmc/card.h | 4 +
> include/linux/mmc/core.h | 4 +
> include/linux/mmc/host.h | 6 +
> 9 files changed, 387 insertions(+)
> create mode 100644 drivers/mmc/core/mmcpstore.c
>
> diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
> index c12fe13e4b14..cafb367c482d 100644
> --- a/drivers/mmc/core/Kconfig
> +++ b/drivers/mmc/core/Kconfig
> @@ -81,3 +81,10 @@ config MMC_TEST
> This driver is only of interest to those developing or
> testing a host driver. Most people should say N here.
>
> +config MMC_PSTORE
> + bool "Log panic/oops to a MMC buffer"
> + depends on PSTORE
> + depends on PSTORE_BLK
> + help
> + Backend driver to store the kmsg crash dumps to a user specified MMC
> + device. The driver is based on pstore/blk.
> diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
> index 95ffe008ebdf..5f4230b79ac6 100644
> --- a/drivers/mmc/core/Makefile
> +++ b/drivers/mmc/core/Makefile
> @@ -17,4 +17,5 @@ mmc_core-$(CONFIG_DEBUG_FS) += debugfs.o
> obj-$(CONFIG_MMC_BLOCK) += mmc_block.o
> mmc_block-objs := block.o queue.o
> obj-$(CONFIG_MMC_TEST) += mmc_test.o
> +obj-$(CONFIG_MMC_PSTORE) += mmcpstore.o
> obj-$(CONFIG_SDIO_UART) += sdio_uart.o
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 8d3df0be0355..f11c21d60b67 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 */
>
> +#ifdef 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,11 @@ static int mmc_blk_probe(struct mmc_card *card)
> goto out;
> }
>
> +#ifdef CONFIG_MMC_PSTORE
Avoid using ifdefs in common functions like these, please. I think
it's more clean to add a stub function and then just call it
unconditionally here.
> + if (mmc_card_mmc(card) || mmc_card_sd(card))
> + mmcpstore_card_set(card, md->disk->disk_name);
> +#endif
> +
> /* Add two debugfs entries */
> mmc_blk_add_debugfs(card, md);
>
> diff --git a/drivers/mmc/core/block.h b/drivers/mmc/core/block.h
> index 31153f656f41..2a2b81635508 100644
> --- a/drivers/mmc/core/block.h
> +++ b/drivers/mmc/core/block.h
> @@ -16,5 +16,8 @@ void mmc_blk_mq_recovery(struct mmc_queue *mq);
> struct work_struct;
>
> void mmc_blk_mq_complete_work(struct work_struct *work);
> +#ifdef CONFIG_MMC_PSTORE
> +sector_t mmc_blk_get_part(struct mmc_card *card, int part_num, sector_t *size);
> +#endif
>
> #endif
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index d42037f0f10d..7cc3d81f6a9a 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);
>
> +
> +#ifdef 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 new MMC custom command request for a host, and
> + * wait for the command to complete based on request data 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);
> + }
> +}
As I said above, I would like to avoid host specific deployments from
being needed. Is there a way we can avoid this?
> +#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..ffa44c859e10
> --- /dev/null
> +++ b/drivers/mmc/core/mmcpstore.c
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " 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;
> +
> + if (!conf->device[0])
> + 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);
By looking at all of the code above, lots are duplicated from the mmc
block device implementation. Isn't there a way to make the pstore
block device to push a request through the regular blk-mq path
instead?
That said, I wonder why you don't call register_pstore_blk(), as I
thought that was the interface to be used for regular block devices,
no?
> + 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;
> +}
> +EXPORT_SYMBOL(mmcpstore_card_set);
> +
> +static int __init mmcpstore_init(void)
> +{
> + struct mmcpstore_context *cxt = &oops_cxt;
> + struct pstore_blk_config *conf = &cxt->conf;
> + int err;
> +
> + err = pstore_blk_get_config(conf);
> + if (unlikely(err))
> + return err;
Looks like that this can be moved to mmc_blk_probe() function instead.
> +
> + if (!conf->device[0]) {
> + pr_err("psblk backend is empty\n");
> + return -EINVAL;
> + }
> +
> + return err;
> +}
> +
> +static void __exit mmcpstore_exit(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;
Can we do this via mmc_blk_remove() instead?
> +}
> +
> +module_init(mmcpstore_init);
> +module_exit(mmcpstore_exit);
> +
> +MODULE_AUTHOR("Bhaskara Budiredla <[email protected]>");
> +MODULE_DESCRIPTION("MMC backend for pstore/blk");
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 42df06c6b19c..76ae8ae61d31 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -322,6 +322,10 @@ static inline bool mmc_large_sector(struct mmc_card *card)
>
> bool mmc_card_is_blockaddr(struct mmc_card *card);
>
> +#ifdef CONFIG_MMC_PSTORE
> +void mmcpstore_card_set(struct mmc_card *card, const char *disk_name);
> +#endif /* CONFIG_MMC_PSTORE */
> +
> #define mmc_card_mmc(c) ((c)->type == MMC_TYPE_MMC)
> #define mmc_card_sd(c) ((c)->type == MMC_TYPE_SD)
> #define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO)
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 29aa50711626..21dcd79f8f0e 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -166,6 +166,10 @@ struct mmc_request {
>
> struct mmc_card;
>
> +#ifdef CONFIG_MMC_PSTORE
> +extern void mmc_wait_for_pstore_req(struct mmc_host *, struct mmc_request *);
> +#endif /* CONFIG_MMC_PSTORE */
> +
> 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..ef57313ee3ea 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -173,6 +173,12 @@ struct mmc_host_ops {
> */
> int (*multi_io_quirk)(struct mmc_card *card,
> unsigned int direction, int blk_size);
> +
> +#ifdef CONFIG_MMC_PSTORE
> + void (*req_cleanup_pending)(struct mmc_host *host);
> + int (*req_completion_poll)(struct mmc_host *host,
> + unsigned long timeout);
If these really will be needed, please add some comments about what
they are intended to help with.
> +#endif
> };
>
> struct mmc_cqe_ops {
Kind regards
Uffe
Big thanks Uffe, my answers in line to yours.
>-----Original Message-----
>From: Ulf Hansson <[email protected]>
>Sent: Friday, November 20, 2020 6:51 PM
>To: Bhaskara Budiredla <[email protected]>
>Cc: Kees Cook <[email protected]>; Colin Cross
><[email protected]>; Tony Luck <[email protected]>; Sunil Kovvuri
>Goutham <[email protected]>; [email protected]; Linux
>Kernel Mailing List <[email protected]>
>Subject: [EXT] Re: [PATCH v1 1/2] mmc: Support kmsg dumper based on
>pstore/blk
>
>External Email
>
>----------------------------------------------------------------------
>On Thu, 12 Nov 2020 at 07:24, Bhaskara Budiredla
><[email protected]> wrote:
>>
>> This patch introduces to mmcpstore. The functioning of mmcpstore is 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.
>
>I don't like that changes to host drivers are needed to support this, but
>perhaps there is no other good way!?
Yes, I couldn't think of a better way as polling functionality is host specific.
>
>>
>> Signed-off-by: Bhaskara Budiredla <[email protected]>
>> ---
>> drivers/mmc/core/Kconfig | 7 +
>> drivers/mmc/core/Makefile | 1 +
>> drivers/mmc/core/block.c | 20 +++
>> drivers/mmc/core/block.h | 3 +
>> drivers/mmc/core/core.c | 24 +++
>> drivers/mmc/core/mmcpstore.c | 318
>+++++++++++++++++++++++++++++++++++
>> include/linux/mmc/card.h | 4 +
>> include/linux/mmc/core.h | 4 +
>> include/linux/mmc/host.h | 6 +
>> 9 files changed, 387 insertions(+)
>> create mode 100644 drivers/mmc/core/mmcpstore.c
>>
>> diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig index
>> c12fe13e4b14..cafb367c482d 100644
>> --- a/drivers/mmc/core/Kconfig
>> +++ b/drivers/mmc/core/Kconfig
>> @@ -81,3 +81,10 @@ config MMC_TEST
>> This driver is only of interest to those developing or
>> testing a host driver. Most people should say N here.
>>
>> +config MMC_PSTORE
>> + bool "Log panic/oops to a MMC buffer"
>> + depends on PSTORE
>> + depends on PSTORE_BLK
>> + help
>> + Backend driver to store the kmsg crash dumps to a user specified
>MMC
>> + device. The driver is based on pstore/blk.
>> diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
>> index 95ffe008ebdf..5f4230b79ac6 100644
>> --- a/drivers/mmc/core/Makefile
>> +++ b/drivers/mmc/core/Makefile
>> @@ -17,4 +17,5 @@ mmc_core-$(CONFIG_DEBUG_FS) += debugfs.o
>> obj-$(CONFIG_MMC_BLOCK) += mmc_block.o
>> mmc_block-objs := block.o queue.o
>> obj-$(CONFIG_MMC_TEST) += mmc_test.o
>> +obj-$(CONFIG_MMC_PSTORE) += mmcpstore.o
>> obj-$(CONFIG_SDIO_UART) += sdio_uart.o
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
>> 8d3df0be0355..f11c21d60b67 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 */
>>
>> +#ifdef 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,11 @@
>> static int mmc_blk_probe(struct mmc_card *card)
>> goto out;
>> }
>>
>> +#ifdef CONFIG_MMC_PSTORE
>
>Avoid using ifdefs in common functions like these, please. I think it's more
>clean to add a stub function and then just call it unconditionally here.
Sure, will do this.
>
>> + if (mmc_card_mmc(card) || mmc_card_sd(card))
>> + mmcpstore_card_set(card, md->disk->disk_name); #endif
>> +
>> /* Add two debugfs entries */
>> mmc_blk_add_debugfs(card, md);
>>
>> diff --git a/drivers/mmc/core/block.h b/drivers/mmc/core/block.h index
>> 31153f656f41..2a2b81635508 100644
>> --- a/drivers/mmc/core/block.h
>> +++ b/drivers/mmc/core/block.h
>> @@ -16,5 +16,8 @@ void mmc_blk_mq_recovery(struct mmc_queue *mq);
>> struct work_struct;
>>
>> void mmc_blk_mq_complete_work(struct work_struct *work);
>> +#ifdef CONFIG_MMC_PSTORE
>> +sector_t mmc_blk_get_part(struct mmc_card *card, int part_num,
>> +sector_t *size); #endif
>>
>> #endif
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
>> d42037f0f10d..7cc3d81f6a9a 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);
>>
>> +
>> +#ifdef 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 new MMC custom command request for a host, and
>> + * wait for the command to complete based on request data 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);
>> + }
>> +}
>
>As I said above, I would like to avoid host specific deployments from being
>needed. Is there a way we can avoid this?
>
I don't see an alternative.
>> +#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..ffa44c859e10
>> --- /dev/null
>> +++ b/drivers/mmc/core/mmcpstore.c
>> @@ -0,0 +1,318 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " 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;
>> +
>> + if (!conf->device[0])
>> + 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);
>
>By looking at all of the code above, lots are duplicated from the mmc block
>device implementation. Isn't there a way to make the pstore block device to
>push a request through the regular blk-mq path instead?
>
The regular path has pre, post processing’s and locking semantics that
are not suitable for panic write scenario. Further, the locking mechanisms are
implemented in host drivers. This is preferred to quickly complete the write
before the kernel dies.
>That said, I wonder why you don't call register_pstore_blk(), as I thought that
>was the interface to be used for regular block devices, no?
>
register_pstore_blk() is for arbitrary block devices for which best effort is not defined.
>
>> + 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;
>> +}
>> +EXPORT_SYMBOL(mmcpstore_card_set);
>> +
>> +static int __init mmcpstore_init(void) {
>> + struct mmcpstore_context *cxt = &oops_cxt;
>> + struct pstore_blk_config *conf = &cxt->conf;
>> + int err;
>> +
>> + err = pstore_blk_get_config(conf);
>> + if (unlikely(err))
>> + return err;
>
>Looks like that this can be moved to mmc_blk_probe() function instead.
>
I will move this to mmcpstore_card_set, which in turn would be called from mmc_blk_probe()
>> +
>> + if (!conf->device[0]) {
>> + pr_err("psblk backend is empty\n");
>> + return -EINVAL;
>> + }
>> +
>> + return err;
>> +}
>> +
>> +static void __exit mmcpstore_exit(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;
>
>Can we do this via mmc_blk_remove() instead?
>
The unregisters here are related to mmcpstore, nothing specific to card. Anyhow,
I would like to make a small change to compile mmcpstore as part of mmc blk.
>> +}
>> +
>> +module_init(mmcpstore_init);
>> +module_exit(mmcpstore_exit);
>> +
>> +MODULE_AUTHOR("Bhaskara Budiredla <[email protected]>");
>> +MODULE_DESCRIPTION("MMC backend for pstore/blk");
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index
>> 42df06c6b19c..76ae8ae61d31 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -322,6 +322,10 @@ static inline bool mmc_large_sector(struct
>> mmc_card *card)
>>
>> bool mmc_card_is_blockaddr(struct mmc_card *card);
>>
>> +#ifdef CONFIG_MMC_PSTORE
>> +void mmcpstore_card_set(struct mmc_card *card, const char
>> +*disk_name); #endif /* CONFIG_MMC_PSTORE */
>> +
>> #define mmc_card_mmc(c) ((c)->type == MMC_TYPE_MMC)
>> #define mmc_card_sd(c) ((c)->type == MMC_TYPE_SD)
>> #define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO)
>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h index
>> 29aa50711626..21dcd79f8f0e 100644
>> --- a/include/linux/mmc/core.h
>> +++ b/include/linux/mmc/core.h
>> @@ -166,6 +166,10 @@ struct mmc_request {
>>
>> struct mmc_card;
>>
>> +#ifdef CONFIG_MMC_PSTORE
>> +extern void mmc_wait_for_pstore_req(struct mmc_host *, struct
>> +mmc_request *); #endif /* CONFIG_MMC_PSTORE */
>> +
>> 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..ef57313ee3ea 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -173,6 +173,12 @@ struct mmc_host_ops {
>> */
>> int (*multi_io_quirk)(struct mmc_card *card,
>> unsigned int direction, int
>> blk_size);
>> +
>> +#ifdef CONFIG_MMC_PSTORE
>> + void (*req_cleanup_pending)(struct mmc_host *host);
>> + int (*req_completion_poll)(struct mmc_host *host,
>> + unsigned long timeout);
>
>If these really will be needed, please add some comments about what they
>are intended to help with.
>
Sure, will add the comments.
>> +#endif
>> };
>>
>> struct mmc_cqe_ops {
>
>Kind regards
>Uffe
Thanks,
Bhaskara
[...]
> >
> >As I said above, I would like to avoid host specific deployments from being
> >needed. Is there a way we can avoid this?
> >
>
> I don't see an alternative.
Well, if not, can you please explain why?
[...]
> >> +
> >> +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;
> >> +
> >> + if (!conf->device[0])
> >> + 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);
> >
> >By looking at all of the code above, lots are duplicated from the mmc block
> >device implementation. Isn't there a way to make the pstore block device to
> >push a request through the regular blk-mq path instead?
> >
> The regular path has pre, post processing’s and locking semantics that
> are not suitable for panic write scenario. Further, the locking mechanisms are
> implemented in host drivers. This is preferred to quickly complete the write
> before the kernel dies.
I am sorry, but this doesn't make sense to me.
When it comes to complete the data write, the regular block I/O path
is supposed to be optimized. If there is a problem with this path,
then we should fix it, rather than adding a new path along the side
(unless there are very good reasons not to).
>
> >That said, I wonder why you don't call register_pstore_blk(), as I thought that
> >was the interface to be used for regular block devices, no?
> >
> register_pstore_blk() is for arbitrary block devices for which best effort is not defined.
Exactly why isn't "best effort" good enough for mmc?
As there are no other users of register_pstore_blk(), it makes me
wonder, when it should be used then?
[...]
> >> +
> >> +static void __exit mmcpstore_exit(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;
> >
> >Can we do this via mmc_blk_remove() instead?
> >
> The unregisters here are related to mmcpstore, nothing specific to card.
I am not sure I understand. If a card is removed, which has been
registered for pstore - then what should we do?
At least, it looks like a card removal will trigger a life cycle issue
for the allocated data structures. No?
[...]
Kind regards
Uffe
>-----Original Message-----
>From: Ulf Hansson <[email protected]>
>Sent: Monday, November 23, 2020 5:49 PM
>To: Bhaskara Budiredla <[email protected]>
>Cc: Kees Cook <[email protected]>; Colin Cross
><[email protected]>; Tony Luck <[email protected]>; Sunil Kovvuri
>Goutham <[email protected]>; [email protected]; Linux
>Kernel Mailing List <[email protected]>
>Subject: Re: [EXT] Re: [PATCH v1 1/2] mmc: Support kmsg dumper based on
>pstore/blk
>
>[...]
>
>> >
>> >As I said above, I would like to avoid host specific deployments from
>> >being needed. Is there a way we can avoid this?
>> >
>>
>> I don't see an alternative.
>
>Well, if not, can you please explain why?
>
The solution has to be polling based as panic write runs with interrupts disabled.
I am not sure if there is a way to write a polling function that works of all kinds
of host/dma drivers. That’s the reason I have provided hooks to define host
specific deployments. If you have better ideas, please help.
>[...]
>
>> >> +
>> >> +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;
>> >> +
>> >> + if (!conf->device[0])
>> >> + 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);
>> >
>> >By looking at all of the code above, lots are duplicated from the mmc
>> >block device implementation. Isn't there a way to make the pstore
>> >block device to push a request through the regular blk-mq path instead?
>> >
>> The regular path has pre, post processing’s and locking semantics that
>> are not suitable for panic write scenario. Further, the locking
>> mechanisms are implemented in host drivers. This is preferred to
>> quickly complete the write before the kernel dies.
>
>I am sorry, but this doesn't make sense to me.
It seems there was some confusion. My comments were specific to
mmcpstore_panic_write() as it runs with interrupts disabled.
mmcpstore_read()/mmcpstore_write() indeed go through regular
blk-mq path.
>
>When it comes to complete the data write, the regular block I/O path is
>supposed to be optimized. If there is a problem with this path, then we should
>fix it, rather than adding a new path along the side (unless there are very good
>reasons not to).
>
>>
>> >That said, I wonder why you don't call register_pstore_blk(), as I
>> >thought that was the interface to be used for regular block devices, no?
>> >
>> register_pstore_blk() is for arbitrary block devices for which best effort is not
>defined.
>
>Exactly why isn't "best effort" good enough for mmc?
>
register_pstore_blk() definitely does the work. If you prefer to take that route,
it should be fine.
>As there are no other users of register_pstore_blk(), it makes me wonder,
>when it should be used then?
>
Pstore/blk folks might help us on this.
Hi Kees - for the benefit of everyone could you please tell us the scenarios
To prefer register_pstore_blk() and register_pstore_device()?
>[...]
>
>> >> +
>> >> +static void __exit mmcpstore_exit(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;
>> >
>> >Can we do this via mmc_blk_remove() instead?
>> >
>> The unregisters here are related to mmcpstore, nothing specific to card.
>
>I am not sure I understand. If a card is removed, which has been registered
>for pstore - then what should we do?
>
>At least, it looks like a card removal will trigger a life cycle issue for the
>allocated data structures. No?
>
I have posted patch v2. I think it has addressed your concern.
>[...]
>
>Kind regards
>Uffe
+ Christoph
On Tue, 24 Nov 2020 at 05:09, Bhaskara Budiredla <[email protected]> wrote:
>
>
>
> >-----Original Message-----
> >From: Ulf Hansson <[email protected]>
> >Sent: Monday, November 23, 2020 5:49 PM
> >To: Bhaskara Budiredla <[email protected]>
> >Cc: Kees Cook <[email protected]>; Colin Cross
> ><[email protected]>; Tony Luck <[email protected]>; Sunil Kovvuri
> >Goutham <[email protected]>; [email protected]; Linux
> >Kernel Mailing List <[email protected]>
> >Subject: Re: [EXT] Re: [PATCH v1 1/2] mmc: Support kmsg dumper based on
> >pstore/blk
> >
> >[...]
> >
> >> >
> >> >As I said above, I would like to avoid host specific deployments from
> >> >being needed. Is there a way we can avoid this?
> >> >
> >>
> >> I don't see an alternative.
> >
> >Well, if not, can you please explain why?
> >
>
> The solution has to be polling based as panic write runs with interrupts disabled.
Right, that's a good reason. :-) Please clarify that in the commit
message and add some information around the definition of the new host
ops callbacks should be used.
> I am not sure if there is a way to write a polling function that works of all kinds
> of host/dma drivers. That’s the reason I have provided hooks to define host
> specific deployments. If you have better ideas, please help.
No, at this point I don't, sorry. I will think more about it.
>
> >[...]
> >
> >> >> +
> >> >> +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;
> >> >> +
> >> >> + if (!conf->device[0])
> >> >> + 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);
> >> >
> >> >By looking at all of the code above, lots are duplicated from the mmc
> >> >block device implementation. Isn't there a way to make the pstore
> >> >block device to push a request through the regular blk-mq path instead?
> >> >
> >> The regular path has pre, post processing’s and locking semantics that
> >> are not suitable for panic write scenario. Further, the locking
> >> mechanisms are implemented in host drivers. This is preferred to
> >> quickly complete the write before the kernel dies.
> >
> >I am sorry, but this doesn't make sense to me.
>
> It seems there was some confusion. My comments were specific to
> mmcpstore_panic_write() as it runs with interrupts disabled.
> mmcpstore_read()/mmcpstore_write() indeed go through regular
> blk-mq path.
>
> >
> >When it comes to complete the data write, the regular block I/O path is
> >supposed to be optimized. If there is a problem with this path, then we should
> >fix it, rather than adding a new path along the side (unless there are very good
> >reasons not to).
> >
> >>
> >> >That said, I wonder why you don't call register_pstore_blk(), as I
> >> >thought that was the interface to be used for regular block devices, no?
> >> >
> >> register_pstore_blk() is for arbitrary block devices for which best effort is not
> >defined.
> >
> >Exactly why isn't "best effort" good enough for mmc?
> >
>
> register_pstore_blk() definitely does the work. If you prefer to take that route,
> it should be fine.
It looks like Christoph is planning for some rewrite of the pstore
code, so let's see what that means in regards to this.
[...]
Kind regards
Uffe