* [PATCH v3 0/2] mmc: support crash logging to MMC block devices @ 2020-12-07 11:57 Bhaskara Budiredla 2020-12-07 11:57 ` [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk Bhaskara Budiredla 2020-12-07 11:57 ` [PATCH 2/2] mmc: cavium: Add MMC polling method to support kmsg panic/oops write Bhaskara Budiredla 0 siblings, 2 replies; 16+ messages in thread From: Bhaskara Budiredla @ 2020-12-07 11:57 UTC (permalink / raw) To: ulf.hansson, keescook, ccross, tony.luck, sgoutham Cc: linux-mmc, linux-kernel, Bhaskara Budiredla This patch introduces to mmcpstore. v3: - Justify new host ops requirement through commit msg - remove 'default n' in Kconfig v2: - Fix modpost issue with ARCH=sh - Fix usage of ifdefs in common functions - Add justification of new APIs to mmc_host_ops - Compile mmcpstore as part of mmc blk v1: https://lore.kernel.org/linux-mmc/20201112062422.32212-1-bbudiredla@marvell.com/T/#t v2: https://lore.kernel.org/linux-mmc/6762a763-5284-04dc-e636-486c74dedd34@alum.wpi.edu/T/#u 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 | 14 +- 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 +++++++++++++++++++++++++++++ drivers/mmc/host/cavium-thunderx.c | 10 + drivers/mmc/host/cavium.c | 67 +++++++ drivers/mmc/host/cavium.h | 3 + include/linux/mmc/core.h | 4 + include/linux/mmc/host.h | 12 ++ 11 files changed, 464 insertions(+), 1 deletion(-) create mode 100644 drivers/mmc/core/mmcpstore.c -- 2.17.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk 2020-12-07 11:57 [PATCH v3 0/2] mmc: support crash logging to MMC block devices Bhaskara Budiredla @ 2020-12-07 11:57 ` Bhaskara Budiredla 2020-12-11 11:31 ` Ulf Hansson 2020-12-07 11:57 ` [PATCH 2/2] mmc: cavium: Add MMC polling method to support kmsg panic/oops write Bhaskara Budiredla 1 sibling, 1 reply; 16+ messages in thread From: Bhaskara Budiredla @ 2020-12-07 11:57 UTC (permalink / raw) To: ulf.hansson, keescook, ccross, tony.luck, sgoutham Cc: linux-mmc, linux-kernel, 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 and cleanup subroutines to write kmsg panic/oops logs through mmcpstore. These new host operations are needed as pstore panic write runs with interrupts disabled. Signed-off-by: Bhaskara Budiredla <bbudiredla@marvell.com> --- drivers/mmc/core/Kconfig | 14 +- 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, 384 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..4c651da4f2d2 100644 --- a/drivers/mmc/core/Kconfig +++ b/drivers/mmc/core/Kconfig @@ -34,9 +34,22 @@ 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 + 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 +93,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] 16+ messages in thread
* Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk 2020-12-07 11:57 ` [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk Bhaskara Budiredla @ 2020-12-11 11:31 ` Ulf Hansson 2020-12-15 6:52 ` [EXT] " Bhaskara Budiredla 0 siblings, 1 reply; 16+ messages in thread From: Ulf Hansson @ 2020-12-11 11:31 UTC (permalink / raw) To: Bhaskara Budiredla Cc: Kees Cook, Colin Cross, Tony Luck, Sunil Kovvuri Goutham, linux-mmc, Linux Kernel Mailing List, Christoph Hellwig + Christoph On Mon, 7 Dec 2020 at 12:58, Bhaskara Budiredla <bbudiredla@marvell.com> 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 and > cleanup subroutines to write kmsg panic/oops logs through mmcpstore. > These new host operations are needed as pstore panic write runs with > interrupts disabled. Apologies for the delay. I have tried to give this some more thinking, more comments below. [...] > 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); So, the host driver should through this callback, be able to terminate any ongoing requests/commands - and also try to make sure that the (e)MMC/SD card remains in the data transfer state. Moreover, no locks and no IRQs must be used to manage this, right? Have you really tried if this works for real? > + mmc_start_request(host, mrq); This looks like the wrong approach to me, as it will try to re-use the regular request based path, where the host driver is allowed to use locks, IRQs, DMAs, etc, etc. I would suggest inventing a new separate request path and a new host ops, to deal with these kinds of requests. In this way, the below part with host->ops->req_completion_poll, can probably be removed. Instead we may just wait for the request to return from the new request path. > + > + 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; > +} It looks like the above I/O read/write interface for pstore is intended to be used when the platform is up and running and not during a panic, correct? If so, I don't get why it can't use the regular block interface, as any other file system does, for example? > + > +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); So, this will use several locks, which may be a problem, right? Moreover, if there is an ongoing I/O request (or any other active command/request for that matter), then the host is already claimed by the mmc core. Normally, we would then wait for that request to be completed, to trigger the release of the host and then allow us to claim it here. However, because of the kernel panic, I assume it's quite likely that any ongoing request will not be completed at all, as IRQs may not work, for example. In other words, we may be hanging here forever waiting to claim the host. Unless we are lucky, because of no ongoing request, although we would still have to succeed walking through all the locking, etc, in mmc_claim_host(). Do note, as part of the mmc_claim_host() we may also runtime resume the host, if it was runtime suspended (which is quite likely). To runtime resume a host via runtime PM, we may end up ungating device clocks, power on corresponding PM domains, run a so called re-tuning sequence to restore communication with the card, etc, etc. The point is, all these things must also be possible to do, without locks and by using a polling based "mode"... > + mmc_wait_for_pstore_req(host, mrq); Okay, so let's assume that we are lucky and succeed to claim and runtime resume the host above. Then we also need to runtime resume the card, as it may be runtime suspended. In the "worst case", runtime resuming the card means a complete re-initialization of it, covering a series of commands and turning on regulators, for example. > + 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; > +} [...] Having said the above, I am not entirely convinced that it makes sense to support this, at all. Not only, will the support be highly fragile from the mmc core point of view, but there is also a significant complexity for an mmc host driver to support this (at least in general). Kind regards Uffe ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk 2020-12-11 11:31 ` Ulf Hansson @ 2020-12-15 6:52 ` Bhaskara Budiredla 2020-12-15 11:42 ` Ulf Hansson 0 siblings, 1 reply; 16+ messages in thread From: Bhaskara Budiredla @ 2020-12-15 6:52 UTC (permalink / raw) To: Ulf Hansson Cc: Kees Cook, Colin Cross, Tony Luck, Sunil Kovvuri Goutham, linux-mmc, Linux Kernel Mailing List, Christoph Hellwig >-----Original Message----- >From: Ulf Hansson <ulf.hansson@linaro.org> >Sent: Friday, December 11, 2020 5:02 PM >To: Bhaskara Budiredla <bbudiredla@marvell.com> >Cc: Kees Cook <keescook@chromium.org>; Colin Cross ><ccross@android.com>; Tony Luck <tony.luck@intel.com>; Sunil Kovvuri >Goutham <sgoutham@marvell.com>; linux-mmc@vger.kernel.org; Linux >Kernel Mailing List <linux-kernel@vger.kernel.org>; Christoph Hellwig ><hch@lst.de> >Subject: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on >pstore/blk > >External Email > >---------------------------------------------------------------------- >+ Christoph > >On Mon, 7 Dec 2020 at 12:58, Bhaskara Budiredla <bbudiredla@marvell.com> >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 and >> cleanup subroutines to write kmsg panic/oops logs through mmcpstore. >> These new host operations are needed as pstore panic write runs with >> interrupts disabled. > >Apologies for the delay. I have tried to give this some more thinking, more >comments below. > >[...] > >> 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); > >So, the host driver should through this callback, be able to terminate any >ongoing requests/commands - and also try to make sure that the (e)MMC/SD >card remains in the data transfer state. Moreover, no locks and no IRQs must >be used to manage this, right? > Yes, that's correct. >Have you really tried if this works for real? > Yes, it's a working solution. Patch were submitted after testing. >> + mmc_start_request(host, mrq); > >This looks like the wrong approach to me, as it will try to re-use the regular >request based path, where the host driver is allowed to use locks, IRQs, >DMAs, etc, etc. > No. The locks on host driver will be dropped in CONFIG_MMC_PSTORE path. Similarly, the IRQs will be replaced with polling subroutines during panic write. Please take a look at patch 2/2 which does this for cavium host driver. >I would suggest inventing a new separate request path and a new host ops, to >deal with these kinds of requests. > The polling and cleanup host operations are the ones invented for this purpose. Polling to replace IRQs and cleanup to terminate ongoing requests/commands. >In this way, the below part with host->ops->req_completion_poll, can >probably be removed. Instead we may just wait for the request to return >from the new request path. > This is not possible unless CPU itself does the mmc write (through some block interface?). If the answer is block interface, sleeping cannot be avoided as part of it. Do you really think DMA can be avoided for MMC panic writes? >> + >> + 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; >> +} > >It looks like the above I/O read/write interface for pstore is intended to be >used when the platform is up and running and not during a panic, correct? > >If so, I don't get why it can't use the regular block interface, as any other file >system does, for example? > The pstore read and write operations are used as part of pstore file system mounting to retrieve the stored logs from MMC platform backend and to manage pstore read/write counters. Sleeping would be allowed during this time. Whereas, pstore PANIC write will be called if there happens a crash in the system. Sleeping is NOT allowed at this time. It seems you are mixing the sleeping paths of the mmcpstore with that of atomic path. >> + >> +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); > >So, this will use several locks, which may be a problem, right? > No, as said above locks are present on host driver will be dropped in CONFIG_MMC_PSTORE path. >Moreover, if there is an ongoing I/O request (or any other active >command/request for that matter), then the host is already claimed by the >mmc core. Normally, we would then wait for that request to be completed, to >trigger the release of the host and then allow us to claim it here. > >However, because of the kernel panic, I assume it's quite likely that any >ongoing request will not be completed at all, as IRQs may not work, for >example. > >In other words, we may be hanging here forever waiting to claim the host. >Unless we are lucky, because of no ongoing request, although we would still >have to succeed walking through all the locking, etc, in mmc_claim_host(). > host->ops->req_cleanup_pending(host) was introduced to clean up the queued and ongoing requests/commands. Terminating ongoing requests is not a complicated thing for the host drivers. >Do note, as part of the mmc_claim_host() we may also runtime resume the >host, if it was runtime suspended (which is quite likely). To runtime resume a >host via runtime PM, we may end up ungating device clocks, power on >corresponding PM domains, run a so called re-tuning sequence to restore >communication with the card, etc, etc. The point is, all these things must also >be possible to do, without locks and by using a polling based "mode"... > >> + mmc_wait_for_pstore_req(host, mrq); > >Okay, so let's assume that we are lucky and succeed to claim and runtime >resume the host above. > >Then we also need to runtime resume the card, as it may be runtime >suspended. In the "worst case", runtime resuming the card means a complete >re-initialization of it, covering a series of commands and turning on regulators, >for example. > >> + mmc_release_host(card->host); >> +} All the above said things (runtime resuming host, clocks being updated, restoring communication with host) are valid even for the newly asked request path. They cannot be avoided as we have to terminate the ongoing requests to complete the panic write. >> + >> +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; >> +} > >[...] > >Having said the above, I am not entirely convinced that it makes sense to >support this, at all. > >Not only, will the support be highly fragile from the mmc core point of view, >but there is also a significant complexity for an mmc host driver to support this >(at least in general). > I am not sure if the comments on host driver complexity is true. Terminating ongoing requests and introducing polling functions on host drivers should be straight forward. None those would disturb the core functionality. They are completely independent. >Kind regards >Uffe ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk 2020-12-15 6:52 ` [EXT] " Bhaskara Budiredla @ 2020-12-15 11:42 ` Ulf Hansson 2020-12-15 14:34 ` Bhaskara Budiredla 2020-12-15 20:37 ` Kees Cook 0 siblings, 2 replies; 16+ messages in thread From: Ulf Hansson @ 2020-12-15 11:42 UTC (permalink / raw) To: Bhaskara Budiredla Cc: Kees Cook, Colin Cross, Tony Luck, Sunil Kovvuri Goutham, linux-mmc, Linux Kernel Mailing List, Christoph Hellwig On Tue, 15 Dec 2020 at 07:52, Bhaskara Budiredla <bbudiredla@marvell.com> wrote: > > > > >-----Original Message----- > >From: Ulf Hansson <ulf.hansson@linaro.org> > >Sent: Friday, December 11, 2020 5:02 PM > >To: Bhaskara Budiredla <bbudiredla@marvell.com> > >Cc: Kees Cook <keescook@chromium.org>; Colin Cross > ><ccross@android.com>; Tony Luck <tony.luck@intel.com>; Sunil Kovvuri > >Goutham <sgoutham@marvell.com>; linux-mmc@vger.kernel.org; Linux > >Kernel Mailing List <linux-kernel@vger.kernel.org>; Christoph Hellwig > ><hch@lst.de> > >Subject: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on > >pstore/blk > > > >External Email > > > >---------------------------------------------------------------------- > >+ Christoph > > > >On Mon, 7 Dec 2020 at 12:58, Bhaskara Budiredla <bbudiredla@marvell.com> > >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 and > >> cleanup subroutines to write kmsg panic/oops logs through mmcpstore. > >> These new host operations are needed as pstore panic write runs with > >> interrupts disabled. > > > >Apologies for the delay. I have tried to give this some more thinking, more > >comments below. > > > >[...] > > > >> 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); > > > >So, the host driver should through this callback, be able to terminate any > >ongoing requests/commands - and also try to make sure that the (e)MMC/SD > >card remains in the data transfer state. Moreover, no locks and no IRQs must > >be used to manage this, right? > > > > Yes, that's correct. > > >Have you really tried if this works for real? > > > > Yes, it's a working solution. Patch were submitted after testing. > > >> + mmc_start_request(host, mrq); > > > >This looks like the wrong approach to me, as it will try to re-use the regular > >request based path, where the host driver is allowed to use locks, IRQs, > >DMAs, etc, etc. > > > > No. The locks on host driver will be dropped in CONFIG_MMC_PSTORE path. > Similarly, the IRQs will be replaced with polling subroutines during panic > write. Please take a look at patch 2/2 which does this for cavium host driver. Yes, but why is removing the locks okay for the other regular request case? I assume the locks are there for a reason, right? > > >I would suggest inventing a new separate request path and a new host ops, to > >deal with these kinds of requests. > > > > The polling and cleanup host operations are the ones invented for this purpose. > Polling to replace IRQs and cleanup to terminate ongoing requests/commands. I understand the approach you have taken, but what I am saying is that I don't think it is the best one to choose. Then I think it's better to invent an entirely new path, rather than trying to tweak the existing one. For example, in your case you could leave the lock to be used (in patch2) for the existing regular request path, but bypass the locks and use polling in the new one. > > >In this way, the below part with host->ops->req_completion_poll, can > >probably be removed. Instead we may just wait for the request to return > >from the new request path. > > > > This is not possible unless CPU itself does the mmc write (through some block interface?). > If the answer is block interface, sleeping cannot be avoided as part of it. I wasn't thinking of the block interface, but rather of the internal behavior of an mmc host driver. For data transfers, it's common to use DMA when that is supported. If DMA doesn't work, it's probably a FIFO being read/written to based upon some IRQs telling when read/write should be done. Of course, there are other cases as well. For commands, I expect those to be managed through IRQs. That said, all these things are preferably managed through a polling based method instead, when doing the panic writes, correct? > Do you really think DMA can be avoided for MMC panic writes? That depends on the HW. Certainly there are lots of cases where DMA isn't the only way, but instead it's perfectly possible to fall back to polling based mode. > > >> + > >> + 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; > >> +} > > > >It looks like the above I/O read/write interface for pstore is intended to be > >used when the platform is up and running and not during a panic, correct? > > > >If so, I don't get why it can't use the regular block interface, as any other file > >system does, for example? > > > > The pstore read and write operations are used as part of pstore file system mounting > to retrieve the stored logs from MMC platform backend and to manage pstore read/write > counters. Sleeping would be allowed during this time. Whereas, pstore PANIC write will be > called if there happens a crash in the system. Sleeping is NOT allowed at this time. > > It seems you are mixing the sleeping paths of the mmcpstore with that of atomic path. No, I am not mixing them, but questioning them. For the non atomic path, I don't understand why the pstore file system mounting, etc, deserves to be managed through its own specific ops? Are there any specific reasons for this that I am missing? In principle, for non atomic path, I would rather see that the pstore file system should be able to be mounted on top of any generic block device partition - without requiring the block device driver to implement specific pstore ops. > > > >> + > >> +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); > > > >So, this will use several locks, which may be a problem, right? > > > > No, as said above locks are present on host driver will be dropped > in CONFIG_MMC_PSTORE path. Please have a look at the code implementing mmc_claim_host(). It's not just a simple spin_lock, but there is also a wait_queue and runtime PM being managed from there. > > >Moreover, if there is an ongoing I/O request (or any other active > >command/request for that matter), then the host is already claimed by the > >mmc core. Normally, we would then wait for that request to be completed, to > >trigger the release of the host and then allow us to claim it here. > > > >However, because of the kernel panic, I assume it's quite likely that any > >ongoing request will not be completed at all, as IRQs may not work, for > >example. > > > >In other words, we may be hanging here forever waiting to claim the host. > >Unless we are lucky, because of no ongoing request, although we would still > >have to succeed walking through all the locking, etc, in mmc_claim_host(). > > > > host->ops->req_cleanup_pending(host) was introduced to clean up the queued > and ongoing requests/commands. Terminating ongoing requests is not a complicated > thing for the host drivers. Well, I don't agree. Resetting the host controller should not be a big problem, but I am more worried about what state this will bring the eMMC/SD card in. It sounds to me that the only option is to try to rely on the mmc_claim_host() to actually succeed. This makes it certain that there is no ongoing request that needs to be terminated. Otherwise, things will just fall apart. The question is, can/should we rely on mmc_claim_host() to succeed in this path? Maybe it will work, in cases when there is no ongoing request, as it means the host should be available to be immediately claimed. Although, then the problem ends up with runtime PM, as if the host is available for claiming, it's likely that the host is runtime suspended... > > > >Do note, as part of the mmc_claim_host() we may also runtime resume the > >host, if it was runtime suspended (which is quite likely). To runtime resume a > >host via runtime PM, we may end up ungating device clocks, power on > >corresponding PM domains, run a so called re-tuning sequence to restore > >communication with the card, etc, etc. The point is, all these things must also > >be possible to do, without locks and by using a polling based "mode"... > > > >> + mmc_wait_for_pstore_req(host, mrq); > > > >Okay, so let's assume that we are lucky and succeed to claim and runtime > >resume the host above. > > > >Then we also need to runtime resume the card, as it may be runtime > >suspended. In the "worst case", runtime resuming the card means a complete > >re-initialization of it, covering a series of commands and turning on regulators, > >for example. > > > >> + mmc_release_host(card->host); > >> +} > > > All the above said things (runtime resuming host, clocks being updated, restoring > communication with host) are valid even for the newly asked request path. They cannot > be avoided as we have to terminate the ongoing requests to complete the panic write. Exactly. That's why I wonder if it's really worth it to support the panic writes at all. I realize that the host driver you are working on doesn't support runtime PM, so it's not a problem for you, but for many other cases I assume it would be. Perhaps we could check that runtime PM is disabled for the mmc host as a pre-condition to support this feature? > > >> + > >> +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; > >> +} > > > >[...] > > > >Having said the above, I am not entirely convinced that it makes sense to > >support this, at all. > > > >Not only, will the support be highly fragile from the mmc core point of view, > >but there is also a significant complexity for an mmc host driver to support this > >(at least in general). > > > > I am not sure if the comments on host driver complexity is true. Terminating > ongoing requests and introducing polling functions on host drivers should be > straight forward. None those would disturb the core functionality. They are > completely independent. I think you are underestimating the part with terminating ongoing requests. It sounds to me that you really haven't been terminating any requests at all, but rather just doing a reset of the mmc controller (which is what I observed in patch2). When it comes to adding the new host ops to support the polling functions, I have in principle no objections to that. Kind regards Uffe ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk 2020-12-15 11:42 ` Ulf Hansson @ 2020-12-15 14:34 ` Bhaskara Budiredla 2020-12-16 9:57 ` Ulf Hansson 2020-12-15 20:37 ` Kees Cook 1 sibling, 1 reply; 16+ messages in thread From: Bhaskara Budiredla @ 2020-12-15 14:34 UTC (permalink / raw) To: Ulf Hansson Cc: Kees Cook, Colin Cross, Tony Luck, Sunil Kovvuri Goutham, linux-mmc, Linux Kernel Mailing List, Christoph Hellwig >-----Original Message----- >From: Ulf Hansson <ulf.hansson@linaro.org> >Sent: Tuesday, December 15, 2020 5:13 PM >To: Bhaskara Budiredla <bbudiredla@marvell.com> >Cc: Kees Cook <keescook@chromium.org>; Colin Cross ><ccross@android.com>; Tony Luck <tony.luck@intel.com>; Sunil Kovvuri >Goutham <sgoutham@marvell.com>; linux-mmc@vger.kernel.org; Linux >Kernel Mailing List <linux-kernel@vger.kernel.org>; Christoph Hellwig ><hch@lst.de> >Subject: Re: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on >pstore/blk > >On Tue, 15 Dec 2020 at 07:52, Bhaskara Budiredla <bbudiredla@marvell.com> >wrote: >> >> >> >> >-----Original Message----- >> >From: Ulf Hansson <ulf.hansson@linaro.org> >> >Sent: Friday, December 11, 2020 5:02 PM >> >To: Bhaskara Budiredla <bbudiredla@marvell.com> >> >Cc: Kees Cook <keescook@chromium.org>; Colin Cross >> ><ccross@android.com>; Tony Luck <tony.luck@intel.com>; Sunil Kovvuri >> >Goutham <sgoutham@marvell.com>; linux-mmc@vger.kernel.org; Linux >> >Kernel Mailing List <linux-kernel@vger.kernel.org>; Christoph Hellwig >> ><hch@lst.de> >> >Subject: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on >> >pstore/blk >> > >> >External Email >> > >> >--------------------------------------------------------------------- >> >- >> >+ Christoph >> > >> >On Mon, 7 Dec 2020 at 12:58, Bhaskara Budiredla >> ><bbudiredla@marvell.com> >> >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 and >> >> cleanup subroutines to write kmsg panic/oops logs through mmcpstore. >> >> These new host operations are needed as pstore panic write runs >> >> with interrupts disabled. >> > >> >Apologies for the delay. I have tried to give this some more >> >thinking, more comments below. >> > >> >[...] >> > >> >> 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); >> > >> >So, the host driver should through this callback, be able to >> >terminate any ongoing requests/commands - and also try to make sure >> >that the (e)MMC/SD card remains in the data transfer state. Moreover, >> >no locks and no IRQs must be used to manage this, right? >> > >> >> Yes, that's correct. >> >> >Have you really tried if this works for real? >> > >> >> Yes, it's a working solution. Patch were submitted after testing. >> >> >> + mmc_start_request(host, mrq); >> > >> >This looks like the wrong approach to me, as it will try to re-use >> >the regular request based path, where the host driver is allowed to >> >use locks, IRQs, DMAs, etc, etc. >> > >> >> No. The locks on host driver will be dropped in CONFIG_MMC_PSTORE path. >> Similarly, the IRQs will be replaced with polling subroutines during >> panic write. Please take a look at patch 2/2 which does this for cavium host >driver. > >Yes, but why is removing the locks okay for the other regular request case? > >I assume the locks are there for a reason, right? > Sorry, it was my mistake. The locks will not be applied to panic write case in CONFIG_MMC_PSTORE path. They will continue to exist for regular read/write path.. For quick reference, copying a snippet from cavium host driver (patch 2/2). host->pstore will be true for panic write. 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 } >> >> >I would suggest inventing a new separate request path and a new host >> >ops, to deal with these kinds of requests. >> > >> >> The polling and cleanup host operations are the ones invented for this >purpose. >> Polling to replace IRQs and cleanup to terminate ongoing >requests/commands. > >I understand the approach you have taken, but what I am saying is that I don't >think it is the best one to choose. > >Then I think it's better to invent an entirely new path, rather than trying to >tweak the existing one. For example, in your case you could leave the lock to >be used (in patch2) for the existing regular request path, but bypass the locks >and use polling in the new one. > The above clarification should answer this. >> >> >In this way, the below part with host->ops->req_completion_poll, can >> >probably be removed. Instead we may just wait for the request to >> >return from the new request path. >> > >> >> This is not possible unless CPU itself does the mmc write (through some >block interface?). >> If the answer is block interface, sleeping cannot be avoided as part of it. > >I wasn't thinking of the block interface, but rather of the internal behavior of >an mmc host driver. > >For data transfers, it's common to use DMA when that is supported. If DMA >doesn't work, it's probably a FIFO being read/written to based upon some >IRQs telling when read/write should be done. Of course, there are other cases >as well. > >For commands, I expect those to be managed through IRQs. > >That said, all these things are preferably managed through a polling based >method instead, when doing the panic writes, correct? > >> Do you really think DMA can be avoided for MMC panic writes? > >That depends on the HW. > >Certainly there are lots of cases where DMA isn't the only way, but instead it's >perfectly possible to fall back to polling based mode. > >> >> >> + >> >> + 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; >> >> +} >> > >> >It looks like the above I/O read/write interface for pstore is >> >intended to be used when the platform is up and running and not during a >panic, correct? >> > >> >If so, I don't get why it can't use the regular block interface, as >> >any other file system does, for example? >> > >> >> The pstore read and write operations are used as part of pstore file >> system mounting to retrieve the stored logs from MMC platform backend >> and to manage pstore read/write counters. Sleeping would be allowed >> during this time. Whereas, pstore PANIC write will be called if there happens >a crash in the system. Sleeping is NOT allowed at this time. >> >> It seems you are mixing the sleeping paths of the mmcpstore with that of >atomic path. > >No, I am not mixing them, but questioning them. > >For the non atomic path, I don't understand why the pstore file system >mounting, etc, deserves to be managed through its own specific ops? >Are there any specific reasons for this that I am missing? > Seems generic way is undergoing some changes. Kees already ACKed the mmcpstore registration through block device registration. >In principle, for non atomic path, I would rather see that the pstore file system >should be able to be mounted on top of any generic block device partition - >without requiring the block device driver to implement specific pstore ops. > Scope to answer this is out of mmcpstore driver. Pstore/blk driver have to answer this. >> >> >> >> + >> >> +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); >> > >> >So, this will use several locks, which may be a problem, right? >> > >> >> No, as said above locks are present on host driver will be dropped in >> CONFIG_MMC_PSTORE path. > >Please have a look at the code implementing mmc_claim_host(). It's not just a >simple spin_lock, but there is also a wait_queue and runtime PM being >managed from there. > >> >> >Moreover, if there is an ongoing I/O request (or any other active >> >command/request for that matter), then the host is already claimed by >> >the mmc core. Normally, we would then wait for that request to be >> >completed, to trigger the release of the host and then allow us to claim it >here. >> > >> >However, because of the kernel panic, I assume it's quite likely that >> >any ongoing request will not be completed at all, as IRQs may not >> >work, for example. >> > >> >In other words, we may be hanging here forever waiting to claim the host. >> >Unless we are lucky, because of no ongoing request, although we would >> >still have to succeed walking through all the locking, etc, in >mmc_claim_host(). >> > I agree with your concerns about spin_lock, wait_queue and runtime PM in mmc_claim_host(), but not the ongoing requests presence. They must have terminated cleanly through host->ops->req_cleanup_pending(host) before reaching mmc_claim_host(). I will come up with something to address these in the next patch. >> >> host->ops->req_cleanup_pending(host) was introduced to clean up the >> host->ops->queued >> and ongoing requests/commands. Terminating ongoing requests is not a >> complicated thing for the host drivers. > >Well, I don't agree. Resetting the host controller should not be a big problem, >but I am more worried about what state this will bring the eMMC/SD card in. > I am not sure why are you saying host controller reset. No ware host controller reset was performed as part of these patches. >It sounds to me that the only option is to try to rely on the >mmc_claim_host() to actually succeed. This makes it certain that there is no >ongoing request that needs to be terminated. Otherwise, things will just fall >apart. > Agree. Seems I need to create an alternate path to forcefully gain access to the host for the case of panic write. As you pointed out mmc_claim_host(), mmc_release_host() and runtime PM can create issues. >The question is, can/should we rely on mmc_claim_host() to succeed in this >path? Maybe it will work, in cases when there is no ongoing request, as it >means the host should be available to be immediately claimed. Although, >then the problem ends up with runtime PM, as if the host is available for >claiming, it's likely that the host is runtime suspended... > An extra check can be added to see if host was runtime suspended ahead of panic write attempt. >> >> >> >Do note, as part of the mmc_claim_host() we may also runtime resume >> >the host, if it was runtime suspended (which is quite likely). To >> >runtime resume a host via runtime PM, we may end up ungating device >> >clocks, power on corresponding PM domains, run a so called re-tuning >> >sequence to restore communication with the card, etc, etc. The point >> >is, all these things must also be possible to do, without locks and by using a >polling based "mode"... >> > >> >> + mmc_wait_for_pstore_req(host, mrq); >> > >> >Okay, so let's assume that we are lucky and succeed to claim and >> >runtime resume the host above. >> > >> >Then we also need to runtime resume the card, as it may be runtime >> >suspended. In the "worst case", runtime resuming the card means a >> >complete re-initialization of it, covering a series of commands and >> >turning on regulators, for example. >> > >> >> + mmc_release_host(card->host); } >> >> >> All the above said things (runtime resuming host, clocks being >> updated, restoring communication with host) are valid even for the >> newly asked request path. They cannot be avoided as we have to terminate >the ongoing requests to complete the panic write. > >Exactly. That's why I wonder if it's really worth it to support the panic writes at >all. > Definitely this feature is desirable for those who would like to have the mmc as the platform backend. >I realize that the host driver you are working on doesn't support runtime PM, >so it's not a problem for you, but for many other cases I assume it would be. > >Perhaps we could check that runtime PM is disabled for the mmc host as a >pre-condition to support this feature? > There are two things here. Runtime PM feature enable and its suspension being in force by the time of panic write. We can ignore those panic writes whenever host was runtime suspended. In other cases panic logs can be stored to mmc. Losing few of the records is still fine as the other records provide the nature of common crashes happening on the platform. >> >> >> + >> >> +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; >> >> +} >> > >> >[...] >> > >> >Having said the above, I am not entirely convinced that it makes >> >sense to support this, at all. >> > >> >Not only, will the support be highly fragile from the mmc core point >> >of view, but there is also a significant complexity for an mmc host >> >driver to support this (at least in general). >> > >> >> I am not sure if the comments on host driver complexity is true. >> Terminating ongoing requests and introducing polling functions on host >> drivers should be straight forward. None those would disturb the core >> functionality. They are completely independent. > >I think you are underestimating the part with terminating ongoing requests. It >sounds to me that you really haven't been terminating any requests at all, but >rather just doing a reset of the mmc controller (which is what I observed in >patch2). > No, it's not true. I am not doing any reset. Please point me to specific code snippet where you have observed this. >When it comes to adding the new host ops to support the polling functions, I >have in principle no objections to that. > >Kind regards >Uffe ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk 2020-12-15 14:34 ` Bhaskara Budiredla @ 2020-12-16 9:57 ` Ulf Hansson 2020-12-16 10:42 ` Bhaskara Budiredla 0 siblings, 1 reply; 16+ messages in thread From: Ulf Hansson @ 2020-12-16 9:57 UTC (permalink / raw) To: Bhaskara Budiredla Cc: Kees Cook, Colin Cross, Tony Luck, Sunil Kovvuri Goutham, linux-mmc, Linux Kernel Mailing List, Christoph Hellwig [...] > >> > > >> >It looks like the above I/O read/write interface for pstore is > >> >intended to be used when the platform is up and running and not during a > >panic, correct? > >> > > >> >If so, I don't get why it can't use the regular block interface, as > >> >any other file system does, for example? > >> > > >> > >> The pstore read and write operations are used as part of pstore file > >> system mounting to retrieve the stored logs from MMC platform backend > >> and to manage pstore read/write counters. Sleeping would be allowed > >> during this time. Whereas, pstore PANIC write will be called if there happens > >a crash in the system. Sleeping is NOT allowed at this time. > >> > >> It seems you are mixing the sleeping paths of the mmcpstore with that of > >atomic path. > > > >No, I am not mixing them, but questioning them. > > > >For the non atomic path, I don't understand why the pstore file system > >mounting, etc, deserves to be managed through its own specific ops? > >Are there any specific reasons for this that I am missing? > > > > Seems generic way is undergoing some changes. Kees already ACKed > the mmcpstore registration through block device registration. > > >In principle, for non atomic path, I would rather see that the pstore file system > >should be able to be mounted on top of any generic block device partition - > >without requiring the block device driver to implement specific pstore ops. > > > > Scope to answer this is out of mmcpstore driver. Pstore/blk driver have to > answer this. Yep, I am open to discuss this more. > > >> > >> > >> >> + > >> >> +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); > >> > > >> >So, this will use several locks, which may be a problem, right? > >> > > >> > >> No, as said above locks are present on host driver will be dropped in > >> CONFIG_MMC_PSTORE path. > > > >Please have a look at the code implementing mmc_claim_host(). It's not just a > >simple spin_lock, but there is also a wait_queue and runtime PM being > >managed from there. > > > >> > >> >Moreover, if there is an ongoing I/O request (or any other active > >> >command/request for that matter), then the host is already claimed by > >> >the mmc core. Normally, we would then wait for that request to be > >> >completed, to trigger the release of the host and then allow us to claim it > >here. > >> > > >> >However, because of the kernel panic, I assume it's quite likely that > >> >any ongoing request will not be completed at all, as IRQs may not > >> >work, for example. > >> > > >> >In other words, we may be hanging here forever waiting to claim the host. > >> >Unless we are lucky, because of no ongoing request, although we would > >> >still have to succeed walking through all the locking, etc, in > >mmc_claim_host(). > >> > > > I agree with your concerns about spin_lock, wait_queue and runtime PM in > mmc_claim_host(), but not the ongoing requests presence. They must > have terminated cleanly through host->ops->req_cleanup_pending(host) > before reaching mmc_claim_host(). > > I will come up with something to address these in the next patch. > > >> > >> host->ops->req_cleanup_pending(host) was introduced to clean up the > >> host->ops->queued > >> and ongoing requests/commands. Terminating ongoing requests is not a > >> complicated thing for the host drivers. > > > >Well, I don't agree. Resetting the host controller should not be a big problem, > >but I am more worried about what state this will bring the eMMC/SD card in. > > > > I am not sure why are you saying host controller reset. No ware host controller > reset was performed as part of these patches. > > >It sounds to me that the only option is to try to rely on the > >mmc_claim_host() to actually succeed. This makes it certain that there is no > >ongoing request that needs to be terminated. Otherwise, things will just fall > >apart. > > > > Agree. Seems I need to create an alternate path to forcefully gain access to the host > for the case of panic write. As you pointed out mmc_claim_host(), mmc_release_host() > and runtime PM can create issues. > > >The question is, can/should we rely on mmc_claim_host() to succeed in this > >path? Maybe it will work, in cases when there is no ongoing request, as it > >means the host should be available to be immediately claimed. Although, > >then the problem ends up with runtime PM, as if the host is available for > >claiming, it's likely that the host is runtime suspended... > > > > An extra check can be added to see if host was runtime suspended ahead > of panic write attempt. What if that is the case, should we just return an error? Moreover, even the device belonging to the mmc card can be runtime suspended too. So if that is the case, we should return an error too? [...] > >> >[...] > >> > > >> >Having said the above, I am not entirely convinced that it makes > >> >sense to support this, at all. > >> > > >> >Not only, will the support be highly fragile from the mmc core point > >> >of view, but there is also a significant complexity for an mmc host > >> >driver to support this (at least in general). > >> > > >> > >> I am not sure if the comments on host driver complexity is true. > >> Terminating ongoing requests and introducing polling functions on host > >> drivers should be straight forward. None those would disturb the core > >> functionality. They are completely independent. > > > >I think you are underestimating the part with terminating ongoing requests. It > >sounds to me that you really haven't been terminating any requests at all, but > >rather just doing a reset of the mmc controller (which is what I observed in > >patch2). > > > > No, it's not true. I am not doing any reset. Please point me to specific code snippet > where you have observed this. I was looking at patch2 and the ->req_cleanup_pending() callback that you have assigned to cvm_req_cleanup_pending(). In there you clear a potentially running DMA job, which is *kind* of a reset of the controller. More importantly, it's definitely *not* terminating an ongoing request, in a way that you can expect the eMMC/SD card to be ready for new communications afterwards. This is my main point. Kind regards Uffe ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk 2020-12-16 9:57 ` Ulf Hansson @ 2020-12-16 10:42 ` Bhaskara Budiredla 2020-12-16 10:52 ` Ulf Hansson 0 siblings, 1 reply; 16+ messages in thread From: Bhaskara Budiredla @ 2020-12-16 10:42 UTC (permalink / raw) To: Ulf Hansson Cc: Kees Cook, Colin Cross, Tony Luck, Sunil Kovvuri Goutham, linux-mmc, Linux Kernel Mailing List, Christoph Hellwig >-----Original Message----- >From: Ulf Hansson <ulf.hansson@linaro.org> >Sent: Wednesday, December 16, 2020 3:27 PM >To: Bhaskara Budiredla <bbudiredla@marvell.com> >Cc: Kees Cook <keescook@chromium.org>; Colin Cross ><ccross@android.com>; Tony Luck <tony.luck@intel.com>; Sunil Kovvuri >Goutham <sgoutham@marvell.com>; linux-mmc@vger.kernel.org; Linux >Kernel Mailing List <linux-kernel@vger.kernel.org>; Christoph Hellwig ><hch@lst.de> >Subject: Re: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on >pstore/blk > >[...] > >> >> > >> >> >It looks like the above I/O read/write interface for pstore is >> >> >intended to be used when the platform is up and running and not >> >> >during a >> >panic, correct? >> >> > >> >> >If so, I don't get why it can't use the regular block interface, >> >> >as any other file system does, for example? >> >> > >> >> >> >> The pstore read and write operations are used as part of pstore >> >> file system mounting to retrieve the stored logs from MMC platform >> >> backend and to manage pstore read/write counters. Sleeping would be >> >> allowed during this time. Whereas, pstore PANIC write will be >> >> called if there happens >> >a crash in the system. Sleeping is NOT allowed at this time. >> >> >> >> It seems you are mixing the sleeping paths of the mmcpstore with >> >> that of >> >atomic path. >> > >> >No, I am not mixing them, but questioning them. >> > >> >For the non atomic path, I don't understand why the pstore file >> >system mounting, etc, deserves to be managed through its own specific >ops? >> >Are there any specific reasons for this that I am missing? >> > >> >> Seems generic way is undergoing some changes. Kees already ACKed the >> mmcpstore registration through block device registration. >> >> >In principle, for non atomic path, I would rather see that the pstore >> >file system should be able to be mounted on top of any generic block >> >device partition - without requiring the block device driver to implement >specific pstore ops. >> > >> >> Scope to answer this is out of mmcpstore driver. Pstore/blk driver >> have to answer this. > >Yep, I am open to discuss this more. > >> >> >> >> >> >> >> >> + >> >> >> +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); >> >> > >> >> >So, this will use several locks, which may be a problem, right? >> >> > >> >> >> >> No, as said above locks are present on host driver will be dropped >> >> in CONFIG_MMC_PSTORE path. >> > >> >Please have a look at the code implementing mmc_claim_host(). It's >> >not just a simple spin_lock, but there is also a wait_queue and >> >runtime PM being managed from there. >> > >> >> >> >> >Moreover, if there is an ongoing I/O request (or any other active >> >> >command/request for that matter), then the host is already claimed >> >> >by the mmc core. Normally, we would then wait for that request to >> >> >be completed, to trigger the release of the host and then allow us >> >> >to claim it >> >here. >> >> > >> >> >However, because of the kernel panic, I assume it's quite likely >> >> >that any ongoing request will not be completed at all, as IRQs may >> >> >not work, for example. >> >> > >> >> >In other words, we may be hanging here forever waiting to claim the >host. >> >> >Unless we are lucky, because of no ongoing request, although we >> >> >would still have to succeed walking through all the locking, etc, >> >> >in >> >mmc_claim_host(). >> >> > >> >> I agree with your concerns about spin_lock, wait_queue and runtime PM >> in mmc_claim_host(), but not the ongoing requests presence. They must >> have terminated cleanly through host->ops->req_cleanup_pending(host) >> before reaching mmc_claim_host(). >> >> I will come up with something to address these in the next patch. >> >> >> >> >> host->ops->req_cleanup_pending(host) was introduced to clean up the >> >> host->ops->queued >> >> and ongoing requests/commands. Terminating ongoing requests is not >> >> a complicated thing for the host drivers. >> > >> >Well, I don't agree. Resetting the host controller should not be a >> >big problem, but I am more worried about what state this will bring the >eMMC/SD card in. >> > >> >> I am not sure why are you saying host controller reset. No ware host >> controller reset was performed as part of these patches. >> >> >It sounds to me that the only option is to try to rely on the >> >mmc_claim_host() to actually succeed. This makes it certain that >> >there is no ongoing request that needs to be terminated. Otherwise, >> >things will just fall apart. >> > >> >> Agree. Seems I need to create an alternate path to forcefully gain >> access to the host for the case of panic write. As you pointed out >> mmc_claim_host(), mmc_release_host() and runtime PM can create issues. >> >> >The question is, can/should we rely on mmc_claim_host() to succeed in >> >this path? Maybe it will work, in cases when there is no ongoing >> >request, as it means the host should be available to be immediately >> >claimed. Although, then the problem ends up with runtime PM, as if >> >the host is available for claiming, it's likely that the host is runtime >suspended... >> > >> >> An extra check can be added to see if host was runtime suspended ahead >> of panic write attempt. > >What if that is the case, should we just return an error? > Yes. >Moreover, even the device belonging to the mmc card can be runtime >suspended too. So if that is the case, we should return an error too? > Yes, same here. Assuming ->req_cleanup_pending() properly terminates the ongoing DMA transfers, mmc_claim_host() and mmc_release_host() can be dropped in panic write case as it has then exclusive access from then on. >[...] > >> >> >[...] >> >> > >> >> >Having said the above, I am not entirely convinced that it makes >> >> >sense to support this, at all. >> >> > >> >> >Not only, will the support be highly fragile from the mmc core >> >> >point of view, but there is also a significant complexity for an >> >> >mmc host driver to support this (at least in general). >> >> > >> >> >> >> I am not sure if the comments on host driver complexity is true. >> >> Terminating ongoing requests and introducing polling functions on >> >> host drivers should be straight forward. None those would disturb >> >> the core functionality. They are completely independent. >> > >> >I think you are underestimating the part with terminating ongoing >> >requests. It sounds to me that you really haven't been terminating >> >any requests at all, but rather just doing a reset of the mmc >> >controller (which is what I observed in patch2). >> > >> >> No, it's not true. I am not doing any reset. Please point me to >> specific code snippet where you have observed this. > >I was looking at patch2 and the ->req_cleanup_pending() callback that you >have assigned to cvm_req_cleanup_pending(). > >In there you clear a potentially running DMA job, which is *kind* of a reset of >the controller. More importantly, it's definitely *not* terminating an ongoing >request, in a way that you can expect the eMMC/SD card to be ready for new >communications afterwards. This is my main point. > I am not sure that clearing an ongoing DMA will reset the controller. These are host controller specific. The idea is: To drop ongoing transfers, whatever a host software has to do it will does through this cleanup function. We may not generalize this, providing a hook and letting each host controller handling it seems better. >Kind regards >Uffe Thanks, Bhaskara ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk 2020-12-16 10:42 ` Bhaskara Budiredla @ 2020-12-16 10:52 ` Ulf Hansson 2020-12-17 11:36 ` Bhaskara Budiredla 0 siblings, 1 reply; 16+ messages in thread From: Ulf Hansson @ 2020-12-16 10:52 UTC (permalink / raw) To: Bhaskara Budiredla Cc: Kees Cook, Colin Cross, Tony Luck, Sunil Kovvuri Goutham, linux-mmc, Linux Kernel Mailing List, Christoph Hellwig [...] > >> > >> Agree. Seems I need to create an alternate path to forcefully gain > >> access to the host for the case of panic write. As you pointed out > >> mmc_claim_host(), mmc_release_host() and runtime PM can create issues. > >> > >> >The question is, can/should we rely on mmc_claim_host() to succeed in > >> >this path? Maybe it will work, in cases when there is no ongoing > >> >request, as it means the host should be available to be immediately > >> >claimed. Although, then the problem ends up with runtime PM, as if > >> >the host is available for claiming, it's likely that the host is runtime > >suspended... > >> > > >> > >> An extra check can be added to see if host was runtime suspended ahead > >> of panic write attempt. > > > >What if that is the case, should we just return an error? > > > Yes. > > >Moreover, even the device belonging to the mmc card can be runtime > >suspended too. So if that is the case, we should return an error too? > > > > Yes, same here. > > Assuming ->req_cleanup_pending() properly terminates the ongoing DMA transfers, > mmc_claim_host() and mmc_release_host() can be dropped in panic write case > as it has then exclusive access from then on. Again, I think you have misunderstood how it works from the core point of view. ->req_cleanup_pending() will never be able to terminate a request in a proper way, without involving the mmc core's knowledge about the eMMC/SD protocol. Yes, it could clean up from an ongoing DMA transfer, but that doesn't bring the eMMC/SD card back into a state where it can accept new requests. > > > >[...] > > > >> >> >[...] > >> >> > > >> >> >Having said the above, I am not entirely convinced that it makes > >> >> >sense to support this, at all. > >> >> > > >> >> >Not only, will the support be highly fragile from the mmc core > >> >> >point of view, but there is also a significant complexity for an > >> >> >mmc host driver to support this (at least in general). > >> >> > > >> >> > >> >> I am not sure if the comments on host driver complexity is true. > >> >> Terminating ongoing requests and introducing polling functions on > >> >> host drivers should be straight forward. None those would disturb > >> >> the core functionality. They are completely independent. > >> > > >> >I think you are underestimating the part with terminating ongoing > >> >requests. It sounds to me that you really haven't been terminating > >> >any requests at all, but rather just doing a reset of the mmc > >> >controller (which is what I observed in patch2). > >> > > >> > >> No, it's not true. I am not doing any reset. Please point me to > >> specific code snippet where you have observed this. > > > >I was looking at patch2 and the ->req_cleanup_pending() callback that you > >have assigned to cvm_req_cleanup_pending(). > > > >In there you clear a potentially running DMA job, which is *kind* of a reset of > >the controller. More importantly, it's definitely *not* terminating an ongoing > >request, in a way that you can expect the eMMC/SD card to be ready for new > >communications afterwards. This is my main point. > > > > I am not sure that clearing an ongoing DMA will reset the controller. These are host > controller specific. The idea is: To drop ongoing transfers, whatever a host software > has to do it will does through this cleanup function. We may not generalize this, > providing a hook and letting each host controller handling it seems better. I fully understand the approach, but again, it doesn't work. Sorry. As I said, the only thing that *could* work is to call mmc_claim_host() in the panic write hook - and hope for the best. If it succeeds, there is nothing to cleanup or terminate. Kind regards Uffe ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk 2020-12-16 10:52 ` Ulf Hansson @ 2020-12-17 11:36 ` Bhaskara Budiredla 2020-12-17 17:11 ` Ulf Hansson 0 siblings, 1 reply; 16+ messages in thread From: Bhaskara Budiredla @ 2020-12-17 11:36 UTC (permalink / raw) To: Ulf Hansson Cc: Kees Cook, Colin Cross, Tony Luck, Sunil Kovvuri Goutham, linux-mmc, Linux Kernel Mailing List, Christoph Hellwig [...] >> >> An extra check can be added to see if host was runtime suspended >> >> ahead of panic write attempt. >> > >> >What if that is the case, should we just return an error? >> > >> Yes. >> >> >Moreover, even the device belonging to the mmc card can be runtime >> >suspended too. So if that is the case, we should return an error too? >> > >> >> Yes, same here. >> Please comment if returning error is sufficient here or can there be an attempt to wake the device through either of the atomic activation calls: pm_runtime_get(), pm_request_resume()? Thanks, Bhaskara ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk 2020-12-17 11:36 ` Bhaskara Budiredla @ 2020-12-17 17:11 ` Ulf Hansson 2020-12-18 4:02 ` Bhaskara Budiredla 0 siblings, 1 reply; 16+ messages in thread From: Ulf Hansson @ 2020-12-17 17:11 UTC (permalink / raw) To: Bhaskara Budiredla Cc: Kees Cook, Colin Cross, Tony Luck, Sunil Kovvuri Goutham, linux-mmc, Linux Kernel Mailing List, Christoph Hellwig On Thu, 17 Dec 2020 at 12:36, Bhaskara Budiredla <bbudiredla@marvell.com> wrote: > > > [...] > > >> >> An extra check can be added to see if host was runtime suspended > >> >> ahead of panic write attempt. > >> > > >> >What if that is the case, should we just return an error? > >> > > >> Yes. > >> > >> >Moreover, even the device belonging to the mmc card can be runtime > >> >suspended too. So if that is the case, we should return an error too? > >> > > >> > >> Yes, same here. > >> > > Please comment if returning error is sufficient here or > can there be an attempt to wake the device through either of the atomic activation calls: > pm_runtime_get(), pm_request_resume()? Hmm, I would start with playing with the below. mmc_claim_host supports also nested claims. mmc_claim_host(host) - this will call pm_runtime_get_sync(host) mmc_get_card(card, NULL) - this will call can pm_runtime_get_sync(card)) and also try to claim the host Kind regards Uffe ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk 2020-12-17 17:11 ` Ulf Hansson @ 2020-12-18 4:02 ` Bhaskara Budiredla 0 siblings, 0 replies; 16+ messages in thread From: Bhaskara Budiredla @ 2020-12-18 4:02 UTC (permalink / raw) To: Ulf Hansson Cc: Kees Cook, Colin Cross, Tony Luck, Sunil Kovvuri Goutham, linux-mmc, Linux Kernel Mailing List, Christoph Hellwig >-----Original Message----- >From: Ulf Hansson <ulf.hansson@linaro.org> >Sent: Thursday, December 17, 2020 10:42 PM >To: Bhaskara Budiredla <bbudiredla@marvell.com> >Cc: Kees Cook <keescook@chromium.org>; Colin Cross ><ccross@android.com>; Tony Luck <tony.luck@intel.com>; Sunil Kovvuri >Goutham <sgoutham@marvell.com>; linux-mmc@vger.kernel.org; Linux >Kernel Mailing List <linux-kernel@vger.kernel.org>; Christoph Hellwig ><hch@lst.de> >Subject: Re: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on >pstore/blk > >On Thu, 17 Dec 2020 at 12:36, Bhaskara Budiredla <bbudiredla@marvell.com> >wrote: >> >> >> [...] >> >> >> >> An extra check can be added to see if host was runtime suspended >> >> >> ahead of panic write attempt. >> >> > >> >> >What if that is the case, should we just return an error? >> >> > >> >> Yes. >> >> >> >> >Moreover, even the device belonging to the mmc card can be runtime >> >> >suspended too. So if that is the case, we should return an error too? >> >> > >> >> >> >> Yes, same here. >> >> >> >> Please comment if returning error is sufficient here or can there be >> an attempt to wake the device through either of the atomic activation calls: >> pm_runtime_get(), pm_request_resume()? > >Hmm, I would start with playing with the below. mmc_claim_host supports >also nested claims. > >mmc_claim_host(host) - this will call pm_runtime_get_sync(host) >mmc_get_card(card, NULL) - this will call can >pm_runtime_get_sync(card)) and also try to claim the host > As you suggested I am creating a parallel path that avoids wait queue to claim the host. The *_sync()* routines could sleep, I can't use them as part of panic write. >Kind regards >Uffe Thanks, Bhaskara ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk 2020-12-15 11:42 ` Ulf Hansson 2020-12-15 14:34 ` Bhaskara Budiredla @ 2020-12-15 20:37 ` Kees Cook 2020-12-16 10:44 ` Ulf Hansson 1 sibling, 1 reply; 16+ messages in thread From: Kees Cook @ 2020-12-15 20:37 UTC (permalink / raw) To: Ulf Hansson Cc: Bhaskara Budiredla, Colin Cross, Tony Luck, Sunil Kovvuri Goutham, linux-mmc, Linux Kernel Mailing List, Christoph Hellwig On Tue, Dec 15, 2020 at 12:42:58PM +0100, Ulf Hansson wrote: > In principle, for non atomic path, I would rather see that the pstore > file system should be able to be mounted on top of any generic block > device partition - without requiring the block device driver to > implement specific pstore ops. > [...] > Exactly. That's why I wonder if it's really worth it to support the > panic writes at all. pstore/blk already provides the generic hooking -- but it can't do the panic write part (which that's very device/driver-specific). The design was for individual backing devices to provide that directly (which would needed read/write support too). And for those that don't have panic/read/write support, they could still use the generic hooks but they wouldn't be able to reliably (or at all?) catch panics (just console writes, ftrace, pmsg, etc). -- Kees Cook ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk 2020-12-15 20:37 ` Kees Cook @ 2020-12-16 10:44 ` Ulf Hansson 2020-12-18 18:50 ` Kees Cook 0 siblings, 1 reply; 16+ messages in thread From: Ulf Hansson @ 2020-12-16 10:44 UTC (permalink / raw) To: Kees Cook Cc: Bhaskara Budiredla, Colin Cross, Tony Luck, Sunil Kovvuri Goutham, linux-mmc, Linux Kernel Mailing List, Christoph Hellwig On Tue, 15 Dec 2020 at 21:37, Kees Cook <keescook@chromium.org> wrote: > > On Tue, Dec 15, 2020 at 12:42:58PM +0100, Ulf Hansson wrote: > > In principle, for non atomic path, I would rather see that the pstore > > file system should be able to be mounted on top of any generic block > > device partition - without requiring the block device driver to > > implement specific pstore ops. > > [...] > > Exactly. That's why I wonder if it's really worth it to support the > > panic writes at all. > > pstore/blk already provides the generic hooking -- but it can't do > the panic write part (which that's very device/driver-specific). The > design was for individual backing devices to provide that directly > (which would needed read/write support too). And for those that don't > have panic/read/write support, they could still use the generic hooks > but they wouldn't be able to reliably (or at all?) catch panics (just > console writes, ftrace, pmsg, etc). I understand the motivation behind pstore's hook for panic-writes. It's a special thing and perhaps it's easier to support this via a specific hook, rather than adopting the regular block device request path to cope with some special I/O request. On the other hand, in the discussion I have had with Bhaskara, I have pointed out several severe implications for mmc to support these panic writes (and believe me, there are even more than those I have brought up). So I am starting to think that, perhaps there is a better option. In any case, I didn't catch *why* pstore needs to force block device drivers to implement specific pstore hooks to support the pstore file system. I don't think this is the way it should work, for many reasons. The pstore file system should be able to be extended, to support the regular block device request path, no? Kind regards Uffe ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk 2020-12-16 10:44 ` Ulf Hansson @ 2020-12-18 18:50 ` Kees Cook 0 siblings, 0 replies; 16+ messages in thread From: Kees Cook @ 2020-12-18 18:50 UTC (permalink / raw) To: Ulf Hansson Cc: Bhaskara Budiredla, Colin Cross, Tony Luck, Sunil Kovvuri Goutham, linux-mmc, Linux Kernel Mailing List, Christoph Hellwig On Wed, Dec 16, 2020 at 11:44:26AM +0100, Ulf Hansson wrote: > In any case, I didn't catch *why* pstore needs to force block device > drivers to implement specific pstore hooks to support the pstore file > system. I don't think this is the way it should work, for many For panic, pstore must avoid any path that might sleep. In a perfect world, it should also use as little code as possible, to avoid potentially tripping over areas of the kernel that might be broken. -- Kees Cook ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] mmc: cavium: Add MMC polling method to support kmsg panic/oops write 2020-12-07 11:57 [PATCH v3 0/2] mmc: support crash logging to MMC block devices Bhaskara Budiredla 2020-12-07 11:57 ` [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk Bhaskara Budiredla @ 2020-12-07 11:57 ` Bhaskara Budiredla 1 sibling, 0 replies; 16+ messages in thread From: Bhaskara Budiredla @ 2020-12-07 11:57 UTC (permalink / raw) To: ulf.hansson, keescook, ccross, tony.luck, sgoutham Cc: linux-mmc, linux-kernel, 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] 16+ messages in thread
end of thread, other threads:[~2020-12-18 18:51 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-07 11:57 [PATCH v3 0/2] mmc: support crash logging to MMC block devices Bhaskara Budiredla 2020-12-07 11:57 ` [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk Bhaskara Budiredla 2020-12-11 11:31 ` Ulf Hansson 2020-12-15 6:52 ` [EXT] " Bhaskara Budiredla 2020-12-15 11:42 ` Ulf Hansson 2020-12-15 14:34 ` Bhaskara Budiredla 2020-12-16 9:57 ` Ulf Hansson 2020-12-16 10:42 ` Bhaskara Budiredla 2020-12-16 10:52 ` Ulf Hansson 2020-12-17 11:36 ` Bhaskara Budiredla 2020-12-17 17:11 ` Ulf Hansson 2020-12-18 4:02 ` Bhaskara Budiredla 2020-12-15 20:37 ` Kees Cook 2020-12-16 10:44 ` Ulf Hansson 2020-12-18 18:50 ` Kees Cook 2020-12-07 11:57 ` [PATCH 2/2] mmc: cavium: Add MMC polling method to support kmsg panic/oops write Bhaskara Budiredla
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.